fix(mls): ensure conversations are established using remote epoch state [WPB-24984]#21174
fix(mls): ensure conversations are established using remote epoch state [WPB-24984]#21174thisisamir98 wants to merge 8 commits intodevfrom
Conversation
…te [WPB-24984] Refactor MLS conversation establishment to rely on core-crypto + backend epoch checks instead of passing a local epoch through call sites. This handles locally unestablished groups more safely (wipe + rehydrate/join), introduces safe epoch retrieval via `getSafeEpoch`, and adds targeted logging around init, decrypt failures, external commits, and pending proposal commits to improve recovery/debugging.
thisisamir98
requested review from
arjita-mitra,
e-maad,
otto-the-bot,
screendriver and
zskhan
as code owners
|
🔗 Download Full Report Artifact 🧪 Playwright Test Summary
|
|
| }; | ||
|
|
||
| const mockSafeEpoch = (core: Core) => { | ||
| (core.service!.mls! as any).getSafeEpoch = jest |
There was a problem hiding this comment.
- never use
anytype assertions - never use definite assignment assertions
!
| if (remoteEpoch === 0) { | ||
| this.logger.info('Establishing conversation as remote epoch is 0', {remoteEpoch}); | ||
| await this.establishMlsGroupConversation({conversationId, groupId, epoch: remoteEpoch, core}); | ||
| return; |
There was a problem hiding this comment.
This is a missing/invalid remote epoch, now silently no-ops. The removed recovery code threw when the remote epoch was unavailable. The new implementation does nothing if remoteConversation.epoch is undefined, null or otherwise falsy but not exactly 0. In the message-send path, ensureConversationExists can return successfully and then sending continues.
| const repositoryCore = (conversationRepository as any).core as Core; | ||
| jest.spyOn(repositoryCore.service!.conversation, 'mlsGroupExistsLocally').mockResolvedValue(false); | ||
| jest | ||
| .spyOn((conversationRepository as any).conversationService, 'getConversationById') |
| return Promise.resolve(!groupId.includes('unestablished')); | ||
| }); | ||
|
|
||
| (repositoryCore.service!.mls! as any).getSafeEpoch = jest.fn().mockResolvedValue({isOk: true, error: null}); |
| }); | ||
| } | ||
| const conversationExistsOnCoreCrypto = await this.conversationService.mlsGroupExistsLocally(groupId); | ||
| const coreCryptoEpochNumberResult = await core.service?.mls?.getSafeEpoch(groupId); |
There was a problem hiding this comment.
Can we avoid optional chaining here and introduce an explicit type guard for the MLS service before calling getSafeEpoch? Right now core.service?.mls?.getSafeEpoch(groupId) can return undefined, but line 2234 treats the result as present.
|
|
||
| if (coreCryptoEpochNumberResult.isErr) { | ||
| this.logger.warn( | ||
| 'conversation existed on core crypto but there was an error when retrieving its epoch number', |
There was a problem hiding this comment.
This log message is misleading because it says the conversation existed on core-crypto, but this branch only checks getSafeEpoch failure. It can run even when conversationExistsOnCoreCrypto is false.
| ); | ||
| } | ||
|
|
||
| if (conversationExistsOnCoreCrypto && coreCryptoEpochNumber > 0) { |
There was a problem hiding this comment.
Can we avoid comparing a possibly undefined value here? coreCryptoEpochNumber is undefined when the result is Err, so this condition depends on a value that is not cleanly narrowed.
| await core.service?.conversation?.wipeMLSConversation(groupId); | ||
| } | ||
|
|
||
| const remoteConversation = await this.conversationService.getConversationById(conversationId); |
There was a problem hiding this comment.
Can we add an explicit type guard for remoteConversation.epoch right after fetching the remote conversation? This method’s behavior depends entirely on epoch being a valid number, but the current code accepts any shape and lets invalid values fall through.
| const remoteEpoch = remoteConversation.epoch; | ||
|
|
||
| // establish the conversation if remote epoch is 0 | ||
| if (remoteEpoch === 0) { |
There was a problem hiding this comment.
Can we make the epoch decision exhaustive instead of relying on partial conditions? Or in other words: this handles 0 but there is no corresponding explicit branch for invalid epoch values.
| // join by external commit | ||
| this.logger.info('Joining conversation by external commit', {conversationId, epoch}); | ||
| if (epoch && epoch > 0) { | ||
| if (remoteEpoch && remoteEpoch > 0) { |
There was a problem hiding this comment.
Can we avoid the truthy/falsy check here?
if (remoteEpoch && remoteEpoch > 0) conflates “missing”, “zero”, and “invalid” with normal control flow. Or in other words: undefined or null will skip joining and the method still resolves.
4 participants
Refactor MLS conversation establishment to rely on core-crypto + backend epoch checks instead of passing a local epoch through call sites. This handles locally unestablished groups more safely (wipe + rehydrate/join), introduces safe epoch retrieval via
getSafeEpoch, and adds targeted logging around init, decrypt failures, external commits, and pending proposal commits to improve recovery/debugging.