-
Notifications
You must be signed in to change notification settings - Fork 22
fix: messages not being sent, waiting forever for decryption to finish - WPB-17866 #3066
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Test Results3 525 tests 3 499 ✅ 4m 3s ⏱️ Results for commit 72c4951. ♻️ This comment has been updated with latest results. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work!
wire-ios-sync-engine/Source/Synchronization/IncrementalSyncObserver.swift
Outdated
Show resolved
Hide resolved
WireDomain/Sources/WireDomain/Synchronization/IncrementalSync.swift
Outdated
Show resolved
Hide resolved
wire-ios-sync-engine/Source/Synchronization/IncrementalSyncObserver.swift
Outdated
Show resolved
Hide resolved
Datadog ReportBranch report: ✅ 0 Failed, 3497 Passed, 26 Skipped, 2m 4.59s Total Time |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
left a question and wonder if a unit test can be added
added some UTs in last commit @netbe , merging the PR |
Issue
Context: Messages are stuck forever in a sending state, no way to recover. Next messages can't be sent as well, stuck in same state.
We introduced in this PR a mechanism that waits for pending events to be decrypted before proceeding to message sending.
Specifically in
MessageSender
, we await forincrementalSyncObserver.waitUntilCanSendMessage()
and it only gets through when finishing decrypting the pending events and setting thedecryptionState
to done at theIncrementalSyncObserver
level.Causes: The decryption state might never gets updated to
.done
hence blocking the message sender to proceed the message which can lead to the bugs mentioned here and here.What happens after is that if we try to send a message again we'll be stuck in a loop in
try await messageDependencyResolver.waitForDependenciesToResolve(for: message)
where the dependencies never resolve specificallydependentObjectNeedingUpdateBeforeProcessing
(code doc below) will never be nil, probably because the initial message was never really processed and it blocks the next messages./// Other entities which has to complete an update before this entity can be processed, i.e. another message /// needs to be sent first because it was scheduled for sending before this message.
Solution: Update the sync state whenever the sync is suspended or when an error occurs while pulling (and decrypting) pending events so we ensure the decryption state is changed to done so the
MessageSender
method is "unlocked" and the messages are sent or at least attempted to be sent.Testing
Tested internally
Checklist
[WPB-XXX]
.UI accessibility checklist
If your PR includes UI changes, please utilize this checklist: