refactor: dedicated read and write dispatchers [WPB-21390]

[MohamadJaara]

WPB-21390 [Android] seperate SqlCipher read from write scopes

---

PR Submission Checklist for internal contributors

• The PR Title

  • conforms to the style of semantic commits messages¹ supported in Wire's Github Workflow²
  • contains a reference JIRA issue number like SQPIT-764
  • answers the question: If merged, this PR will: ... ³

• The PR Description

  • is free of optional paragraphs and you have filled the relevant parts to the best of your ability

---

What's new in this PR?

Issues

The application experiences ANR (Application Not Responding) due to SQLCipher connection pool exhaustion, where multiple concurrent database operations deadlock while waiting for available connections.

Causes (Optional)

sqldelight driver is a blocking driver

Solutions

create seperated dispatchers for read and writes with limitedParallelism 3 for reads and 1 for write this way only 4 threads max are blocked for execution while the rest are suspended

Dependencies (Optional)

If there are some other pull requests related to this one (e.g. new releases of frameworks), specify them here.

Needs releases with:

• GitHub link to other pull request

Testing

Test Coverage (Optional)

• I have added automated test to this contribution

How to Test

Briefly describe how this change was tested and if applicable the exact steps taken to verify that it works as expected.

Notes (Optional)

Specify here any other facts that you think are important for this issue.

Attachments (Optional)

Attachments like images, videos, etc. (drag and drop in the text box)

---

PR Post Submission Checklist for internal contributors (Optional)

• Wire's Github Workflow has automatically linked the PR to a JIRA issue

---

PR Post Merge Checklist for internal contributors

• If any soft of configuration variable was introduced by this PR, it has been added to the relevant documents and the CI jobs have been updated.

---

References

1. https://sparkbox.com/foundry/semantic_commit_messages
2. https://github.com/wireapp/.github#usage
3. E.g. feat(conversation-list): Sort conversations by most emojis in the title #SQPIT-764.

[github-actions]

Test Results

3 397 tests   - 644   3 388 ✅  - 542   3m 0s ⏱️ - 3m 0s
    4 suites  - 680       9 💤  - 102 
    4 files    - 680       0 ❌ ±  0 

Results for commit 5e83f0a. ± Comparison against base commit bdf3161.

This pull request removes 4041 and adds 3397 tests. Note that renamed tests count towards both.

com.wire.backup.BackupEndToEndTest ‑ givenBackedUpAssetMessage_whenRestoring_thenShouldReadTheSameContent[js, browser]
com.wire.backup.BackupEndToEndTest ‑ givenBackedUpAssetMessage_whenRestoring_thenShouldReadTheSameContent[jvm]
com.wire.backup.BackupEndToEndTest ‑ givenBackedUpLocationMessage_whenRestoring_thenShouldReadTheSameContent[js, browser]
com.wire.backup.BackupEndToEndTest ‑ givenBackedUpLocationMessage_whenRestoring_thenShouldReadTheSameContent[jvm]
com.wire.backup.BackupEndToEndTest ‑ givenBackedUpTextMessages_whenRestoring_thenShouldReadTheSameContent[js, browser]
com.wire.backup.BackupEndToEndTest ‑ givenBackedUpTextMessages_whenRestoring_thenShouldReadTheSameContent[jvm]
com.wire.backup.BackupEndToEndTest ‑ givenBackupWithPassword_whenPeeking_thenShouldBeEncrypted[js, browser]
com.wire.backup.BackupEndToEndTest ‑ givenBackupWithPassword_whenPeeking_thenShouldBeEncrypted[jvm]
com.wire.backup.BackupEndToEndTest ‑ givenBackupWithoutPassword_whenPeeking_thenShouldNotBeEncrypted[js, browser]
com.wire.backup.BackupEndToEndTest ‑ givenBackupWithoutPassword_whenPeeking_thenShouldNotBeEncrypted[jvm]
…

com.wire.kalium.cells.domain.CellsApiTest ‑ givenNodeAPI_whenMoveNodeIsCalled_thenInvokeAPIOnce
com.wire.kalium.cells.domain.CellsApiTest ‑ given_existing_file_pre_check_then_existing_file_is_returned
com.wire.kalium.cells.domain.CellsApiTest ‑ given_new_file_pre_check_then_success_is_returned
com.wire.kalium.cells.domain.CellsApiTest ‑ given_pre_check_failure_then_failure_is_returned
com.wire.kalium.cells.domain.NodeUploadManagerTest ‑ given_FailedUpload_when_RetryInvoked_and_FileExists_then_UploadStarts
com.wire.kalium.cells.domain.NodeUploadManagerTest ‑ given_FailedUpload_when_RetryInvoked_and_FileMissing_then_UploadFails
com.wire.kalium.cells.domain.NodeUploadManagerTest ‑ given_existing_file_pre_check_new_node_with_suggested_name_returned
com.wire.kalium.cells.domain.NodeUploadManagerTest ‑ given_failed_upload_upload_error_event_is_emitted
com.wire.kalium.cells.domain.NodeUploadManagerTest ‑ given_failed_upload_upload_job_error_flag_is_set
com.wire.kalium.cells.domain.NodeUploadManagerTest ‑ given_new_file_pre_check_new_node_returned
…

This pull request removes 111 skipped tests and adds 9 skipped tests. Note that renamed tests count towards both.

com.wire.kalium.api.common.ACMEApiTest ‑ whenCallingGeTrustAnchorsApi_theResponseShouldBeConfigureCorrectly[jvm]
com.wire.kalium.api.common.ACMEApiTest ‑ whenCallingSendChallengeRequestApi_theResponseShouldBeConfigureCorrectly[jvm]
com.wire.kalium.api.v0.user.register.RegisterApiV0Test ‑ givenAValidEmail_whenActivationEmailWIthCode_theRequestShouldBeConfiguredCorrectly[jvm]
com.wire.kalium.api.v0.user.register.RegisterApiV0Test ‑ givenAValidEmail_whenRegisteringAccountWithEMail_theRequestShouldBeConfiguredCorrectly[jvm]
com.wire.kalium.api.v0.user.register.RegisterApiV0Test ‑ givenAValidEmail_whenSendingActivationEmail_theRequestShouldBeConfiguredCorrectly[jvm]
com.wire.kalium.api.v0.user.register.RegisterApiV0Test ‑ givenActivationCodeFail_thenErrorIsPropagated[jvm]
com.wire.kalium.api.v0.user.register.RegisterApiV0Test ‑ givenRegistrationFail_whenRegisteringAccountWithEMMail_thenErrorIsPropagated[jvm]
com.wire.kalium.api.v0.user.register.RegisterApiV0Test ‑ givenSendActivationCodeFail_thenErrorIsPropagated[jvm]
com.wire.kalium.cryptography.CryptoUtilsTest ‑ givenDummyText_whenEncryptedAndDecryptedWithAES256_returnsOriginalText[js, browser]
com.wire.kalium.cryptography.CryptoUtilsTest ‑ givenSomeDummyFile_whenEncryptedAndDecryptedWithAES256_returnsExpectedOriginalFile[js, browser]
…

com.wire.kalium.logic.data.conversation.ConversationRepositoryTest ‑ givenAGroupConversationHasNotNewMessages_whenGettingConversationDetails_ThenReturnZeroUnreadMessageCount
com.wire.kalium.logic.feature.backup.RestoreBackupUseCaseTest ‑ givenACorrectlyEncryptedBackup_whenRestoringWithWrongPassword_thenTheRightErrorIsThrown
com.wire.kalium.logic.feature.backup.RestoreBackupUseCaseTest ‑ givenAnEncryptedBackupFileFromDifferentUserID_whenRestoring_thenTheRightErrorIsThrown
com.wire.kalium.logic.feature.conversation.ObserveConversationInteractionAvailabilityUseCaseTest ‑ givenProteusConversationAndUserSupportsOnlyMLS_whenObserving_thenShouldReturnUnsupportedProtocol
com.wire.kalium.logic.feature.conversation.ObserveConversationListDetailsUseCaseTest ‑ null
com.wire.kalium.logic.feature.session.DeleteSessionUseCaseTest ‑ givenSuccess_WhenDeletingSessionLocally_thenSuccessAndResourcesAreFreed
com.wire.kalium.logic.sync.incremental.EventProcessingHistoryTest ‑ measureContainsTimeOverLargeAmountOfEvents
com.wire.kalium.persistence.dao.message.MessageDAOBenchmarkTest ‑ null
com.wire.kalium.persistence.kmmSettings.EncryptedSettingsBuilderTest ‑ givenShouldEncryptDataIsTrue_whenEncryptingData_thenShouldEncryptWithoutFailing

♻️ This comment has been updated with latest results.

[Copilot]

Pull Request Overview

This PR refactors the persistence layer to address Application Not Responding (ANR) issues caused by SQLCipher connection pool exhaustion. It introduces separate read and write dispatchers with limited parallelism (3 for reads, 1 for writes) to prevent database operation deadlocks.

Key changes:

• Introduced ReadDispatcher and WriteDispatcher value classes wrapping CoroutineDispatcher
• Replaced generic CoroutineContext/queriesContext parameters with typed dispatchers across all DAO implementations
• Updated database operations to use appropriate dispatcher based on read/write nature

Reviewed Changes

Copilot reviewed 34 out of 34 changed files in this pull request and generated 7 comments.

Show a summary per file

File	Description
UserDatabaseBuilder.kt	Creates separate read/write dispatchers with limited parallelism and updates all DAO instantiations
Multiple DAO files	Replaces CoroutineContext with ReadDispatcher/WriteDispatcher and wraps operations with appropriate dispatcher
MessageDAO.kt	Changes countMessagesForBackup from synchronous to asynchronous function
CallDAO.kt	Changes getEstablishedCall from synchronous to asynchronous function
CallRepository.kt	Wraps blocking DAO call in runBlocking to handle new async signature
Logic layer files	Removes @OptIn(ExperimentalCoroutinesApi::class) annotations now unnecessary

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

Pull Request Overview

Copilot reviewed 34 out of 34 changed files in this pull request and generated 2 comments.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[github-actions]

Code Review for PR #3681

Overview

This PR addresses ANR issues caused by SQLCipher connection pool exhaustion by introducing separate read and write dispatchers with limited parallelism (3 for reads, 1 for writes). This is a comprehensive refactoring of the persistence layer.

Positive Changes

1. Architecture - Strong Solution

✅ Separate Read/Write Dispatchers: The introduction of ReadDispatcher and WriteDispatcher as value classes is excellent:

• Uses @JvmInline for zero runtime overhead
• Clear documentation explaining rationale (SQLCipher pool exhaustion)
• Sensible parallelism limits: 3 for reads, 1 for writes (aligns with SQLite's single-writer model)

@JvmInline
value class ReadDispatcher(val value: CoroutineDispatcher)

@JvmInline  
value class WriteDispatcher(val value: CoroutineDispatcher)

2. Consistent Refactoring

✅ All DAO implementations have been systematically updated to use the new dispatchers
✅ Proper separation: read operations use readDispatcher, write operations use writeDispatcher
✅ Clean removal of the generic queriesContext: CoroutineContext in favor of specific dispatchers

3. Code Cleanup

✅ Removal of unnecessary @OptIn(ExperimentalCoroutinesApi::class) annotations - limitedParallelism() is now stable
✅ Consistent pattern across all DAOs

Critical Issues

⚠️ 1. Use of runBlocking in Callback (High Priority)

Location: CallManagerImpl.kt:184

private val constantBitRateStateChangeHandler =
    ConstantBitRateStateChangeHandler { userId: String, clientId: String, isEnabled: Boolean, arg: Pointer? ->
        // ...
        runBlocking { callRepository.updateIsCbrEnabled(isEnabled) }
    }.keepingStrongReference()

Problem: runBlocking blocks the calling thread until the coroutine completes. This is particularly dangerous in a callback handler from native code (likely the calling library), as it can:

• Block critical native threads
• Cause deadlocks if the dispatcher queue is full
• Defeat the purpose of the dispatcher changes

Recommendation: Consider one of these alternatives:

1. Launch a coroutine on an appropriate scope without blocking:

scope.launch { 
    callRepository.updateIsCbrEnabled(isEnabled) 
}

2. If the callback must complete synchronously, document why and consider using a dedicated dispatcher
3. If ordering matters, use scope.launch(start = CoroutineStart.UNDISPATCHED) to start immediately

⚠️ 2. Missing Dispatcher in Some DAOs

Locations: Multiple files

Some DAOs now only receive readDispatcher when they may need both:

• SearchDAOImpl - only has readDispatcher (line 73) ✅ This appears correct as it only has read operations
• MessageMetadataDAOImpl - only has readDispatcher - Verify this DAO has no write operations
• CompositeMessageDAOImpl - only has writeDispatcher - Verify this DAO has no read operations

Action: Please verify these DAOs truly don't need both dispatchers.

⚠️ 3. MetadataDAOImpl Constructor Change

Location: MetadataDAOImpl.kt

The constructor signature changed from receiving queriesContext to only writeDispatcher:

class MetadataDAOImpl internal constructor(
    private val metadataQueries: MetadataQueries,
    private val metadataCache: FlowCache<String, String?>,
    private val databaseScope: CoroutineScope,
-   private val queriesContext: CoroutineContext
+   private val writeDispatcher: WriteDispatcher,
) : MetadataDAO

But metadata reads (e.g., valueByKeyFlow) don't seem to use any dispatcher for the database query itself - they rely on the cache. Question: Are cached read operations performant enough, or should direct DB reads also use readDispatcher?

Minor Issues

📝 1. Test Coverage

The PR includes only 2 test file changes (both minor constructor updates):

• ConversationExtensionsTest.kt
• MessageExtensionsTest.kt

Recommendation: Consider adding integration tests that:

• Verify concurrent reads work correctly with limited parallelism
• Test that writes are properly serialized
• Validate the ANR issue is resolved under load

📝 2. Documentation

The PR description mentions "only 4 threads max are blocked" but the actual parallelism is 3 reads + 1 write. Consider:

• Adding KDoc to explain the dispatcher strategy at the UserDatabaseBuilder level
• Document the threading model assumptions (especially the SQLCipher pool size)

📝 3. Constants Placement

private companion object {
    const val MAX_READ_PARALLELISM = 3
    const val MAX_WRITE_PARALLELISM = 1
}

These are in UserDatabaseBuilder. Consider:

• Making them configurable for testing
• Adding comments explaining why these specific values (why 3 reads? what's the SQLCipher pool size?)

Security Considerations

✅ No security concerns identified. The change is purely architectural and maintains the same security boundaries.

Performance Considerations

✅ Expected improvement: Limiting parallelism should reduce thread contention and prevent pool exhaustion
⚠️ Potential concern: Limiting reads to 3 concurrent operations might bottleneck read-heavy workflows. Recommend monitoring read latency in production.

Summary

This is a well-structured architectural improvement that should address the ANR issues. The main concerns are:

1. MUST FIX: Remove runBlocking from the callback handler
2. SHOULD VERIFY: Ensure DAOs have the correct dispatcher(s)
3. NICE TO HAVE: Add integration tests and better documentation

The dispatcher separation is a solid solution, and the implementation is consistent across the codebase. Once the runBlocking issue is addressed, this should be safe to merge.

---

🤖 Generated with Claude Code

[codecov-commenter]

Codecov Report

❌ Patch coverage is 41.88912% with 283 lines in your changes missing coverage. Please review.
✅ Project coverage is 57.40%. Comparing base (bdf3161) to head (5e83f0a).

Files with missing lines	Patch %	Lines
...ersistence/dao/conversation/ConversationDAOImpl.kt	32.18%	14 Missing and 45 partials ⚠️
...e/kalium/persistence/dao/message/MessageDAOImpl.kt	38.70%	9 Missing and 29 partials ⚠️
...lin/com/wire/kalium/persistence/dao/UserDAOImpl.kt	18.60%	16 Missing and 19 partials ⚠️
...ire/kalium/persistence/dao/client/ClientDAOImpl.kt	29.16%	4 Missing and 13 partials ⚠️
...messageattachment/MessageAttachmentDraftDaoImpl.kt	0.00%	17 Missing ⚠️
...ce/dao/message/attachment/MessageAttachmentsDao.kt	0.00%	14 Missing ⚠️
...om/wire/kalium/persistence/dao/member/MemberDAO.kt	23.52%	3 Missing and 10 partials ⚠️
...om/wire/kalium/persistence/dao/call/CallDAOImpl.kt	35.29%	8 Missing and 3 partials ⚠️
...m/wire/kalium/persistence/dao/ConnectionDAOImpl.kt	23.07%	6 Missing and 4 partials ⚠️
.../wire/kalium/persistence/dao/event/EventDAOImpl.kt	33.33%	3 Missing and 5 partials ⚠️
... and 17 more

Additional details and impacted files

@@           Coverage Diff            @@
##           develop    #3681   +/-   ##
========================================
  Coverage    57.40%   57.40%           
========================================
  Files         1785     1785           
  Lines        59342    59415   +73     
  Branches      5774     5775    +1     
========================================
+ Hits         34064    34110   +46     
- Misses       22853    22873   +20     
- Partials      2425     2432    +7     

Files with missing lines	Coverage Δ
.../com/wire/kalium/logic/data/call/CallRepository.kt	84.61% <ø> (-0.31%)	⬇️
...logic/feature/conversation/mls/OneOnOneResolver.kt	66.66% <ø> (ø)
...lium/logic/feature/keypackage/KeyPackageManager.kt	79.48% <ø> (ø)
.../logic/feature/message/PendingProposalScheduler.kt	92.50% <ø> (ø)
...in/com/wire/kalium/persistence/TestUserDatabase.kt	87.50% <ø> (-2.50%)	⬇️
...in/com/wire/kalium/persistence/dao/call/CallDAO.kt	100.00% <ø> (ø)
...istence/dao/conversation/ConversationExtensions.kt	90.90% <100.00%> (ø)
...wire/kalium/persistence/dao/message/KaliumPager.kt	100.00% <100.00%> (ø)
.../wire/kalium/persistence/dao/message/MessageDAO.kt	100.00% <ø> (ø)
...lium/persistence/dao/message/MessageMetadataDAO.kt	80.00% <50.00%> (ø)
... and 26 more

... and 1 file with indirect coverage changes

---

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update bdf3161...5e83f0a. Read the comment docs.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud