fix(core): defer MLS epoch recovery until notification replay finishes [WPB-21916]#21390
Open
thisisamir98 wants to merge 7 commits into
Open
fix(core): defer MLS epoch recovery until notification replay finishes [WPB-21916]#21390thisisamir98 wants to merge 7 commits into
thisisamir98 wants to merge 7 commits into
Conversation
Problem After login, the client replays a large backlog of legacy notifications while local MLS state may already be ahead, behind, or incomplete. When decrypting an MLS message-add failed with WrongEpoch, the recovery orchestrator ran an external-commit rejoin immediately. That could happen in the middle of replay, before later notifications had a chance to apply missing commits. This led to unnecessary MLSConversationRecovered system messages and made replay noisier. Pausing the rejoin queue did not help, because orchestrator recovery bypassed that queue entirely. Approach - Defer epoch-mismatch recovery for inbound handleMessageAdd while the connection is not LIVE. The failed message returns null and replay continues. - After catch-up finishes, runDeferredEpochRecovery() reconciles any conversations that still need external commit (once per conversation).
thisisamir98
requested review from
arjita-mitra,
e-maad,
otto-the-bot,
screendriver and
zskhan
as code owners
Contributor
|
🔗 Download Full Report Artifact 🧪 Playwright Test Summary
|
PR checkout only included the head commit, so nx format:check could not resolve the dev base ref and logged a misleading git error before falling back to checking the whole repo. Fetch the base branch in CI so format failures are reported clearly.
thisisamir98
force-pushed
the
WPB-21916-epoch-fix
branch
from
f952354 to
5a0c6d2
Compare
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Avoid expanding github.base_ref directly in the shell script so zizmor does not flag code injection via template expansion.
|
zskhan
reviewed
Jun 2, 2026
|
|
||
| for (const {conversationId, subconvId, trigger} of entries) { | ||
| try { | ||
| await this.recoverMLSGroupFromEpochMismatch(conversationId, subconvId, trigger, {force: true}); |
Contributor
There was a problem hiding this comment.
If entries are already cleared and after that if for some reason recoverMLSGroupFromEpochMismatch fails then there will be no way to retry the entry.
What do you think?
zskhan
approved these changes
Jun 2, 2026
e-maad
reviewed
Jun 2, 2026
| const firstEventPayload = notification.data.event.payload[0]; | ||
| const notificationTime = firstEventPayload ? this.getNotificationEventTime(firstEventPayload) : null; | ||
| if (this.connectionState !== ConnectionState.LIVE && notificationTime !== null && notificationTime.length > 0) { | ||
| if (!this.connectionStateTracker.isLive() && notificationTime !== null && notificationTime.length > 0) { |
Contributor
There was a problem hiding this comment.
Suggested change
| if (!this.connectionStateTracker.isLive() && notificationTime !== null && notificationTime.length > 0) { | |
| if (!this.connectionStateTracker.isLive() && is.nonEmptyString(notificationTime)) { |
| .push(async () => { | ||
| this.logger.info(`Resuming message sending. ${getQueueLength()} messages to be sent`); | ||
| await this.rehydrateMlsPendingProposalsTasksOnLiveTransition(); | ||
| await this.service!.conversation.runDeferredEpochRecovery(); |
Contributor
There was a problem hiding this comment.
Do we need non-null assertion here?
| trigger?: MlsEpochRecoveryTrigger, | ||
| options: {force: boolean} = {force: false}, | ||
| ) { | ||
| if (options.force === false && this.shouldDeferEpochRecovery(trigger)) { |
Contributor
There was a problem hiding this comment.
If option.force is boolean (and controlled by us) the please rename to isForced
Suggested change
| if (options.force === false && this.shouldDeferEpochRecovery(trigger)) { | |
| if (!options.force && this.shouldDeferEpochRecovery(trigger)) { |
| } | ||
|
|
||
| private getDeferredEpochRecoveryKey(conversationId: QualifiedId, subconvId?: SUBCONVERSATION_ID): string { | ||
| return `${conversationId.id}@${conversationId.domain}:${subconvId ?? 'none'}`; |
Contributor
There was a problem hiding this comment.
Is this none can be provided by any enum?
| * notification replay. Called when the connection transitions to LIVE. | ||
| */ | ||
| public async runDeferredEpochRecovery(): Promise<void> { | ||
| if (this.deferredEpochRecoveries.size === 0) { |
Contributor
There was a problem hiding this comment.
I would suggest to use is.emptySet(deferredEpochRecoveries)
| options: {force: boolean} = {force: false}, | ||
| ) { | ||
| if (options.force === false && this.shouldDeferEpochRecovery(trigger)) { | ||
| this.deferEpochRecovery(conversationId, subconversationId, trigger); |
Contributor
There was a problem hiding this comment.
You are throwing an error from deferEpochRecovery, are we handling it somewhere?
| private readonly subconversationService: SubconversationService, | ||
| private readonly isMLSConversationRecoveryEnabled: () => Promise<boolean>, | ||
| private readonly _mlsService?: MLSService, | ||
| private readonly connectionStateTracker: ConnectionStateTracker = createConnectionStateTracker( |
Contributor
There was a problem hiding this comment.
Why are we maintaining two state-managers for connection?
I can see another one inlibraries/core/src/account.ts. These states will not be synced, is this intentional?
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
After login, the client replays a large backlog of legacy notifications while local MLS state may already be ahead, behind, or incomplete. When decrypting an MLS message-add failed with WrongEpoch, the recovery orchestrator ran an external-commit rejoin immediately. That could happen in the middle of replay, before later notifications had a chance to apply missing commits.
This led to unnecessary MLSConversationRecovered system messages and made replay noisier. Pausing the rejoin queue did not help, because orchestrator recovery bypassed that queue entirely.
Approach