Repository navigation
Fix one-time keys being re-uploaded on every to-device/device-list sync with matrix-sdk-crypto-wasm ≥ 18.5.0 - #5502
Closed
LpZ-squall wants to merge 1 commit into
Closed
LpZ-squall wants to merge 1 commit into
LpZ-squall wants to merge 1 commit into
Conversation
…c paths `RustCrypto.receiveSyncChanges` defaulted the one-time key count to an empty map, and two of its three callers (`preprocessToDeviceMessages` and `processDeviceLists`) relied on that default because they have no count of their own. Since matrix-sdk-crypto-wasm 18.5.0 (matrix-rust-sdk#6780), `OlmMachine.receiveSyncChanges` treats a missing `signed_curve25519` count as "zero keys on the server", so every sync carrying to-device events or device-list changes reset the account's uploaded-key count and made it generate and upload a fresh batch of 50 one-time keys. The private halves of older batches get evicted, so peers claiming those keys could no longer establish Olm sessions with the device. Fix this by keeping track, in `RustCrypto`, of the count the server last reported, and using it wherever we have to call `receiveSyncChanges` without a count of our own. The `OlmMachine` learns that count from two places, so we mirror both: - `processKeyCounts` remembers the count from each `/sync` response, and `SyncApi.processSyncResponse` now calls it before processing to-device events and device lists, so that the remembered count is the one from the same response (including on the first sync after a restart). - `OutgoingRequestProcessor` reports the `one_time_key_counts` from each `/keys/upload` response, which covers the time before `/sync` has reported a count (or servers which omit it). Without the second part, 18 existing integration tests hang against matrix-sdk-crypto-wasm >= 18.5.0: their sync responses carry no count, so the first to-device sync triggered a second key upload, which the test homeserver deliberately never answers. Add unit tests asserting the count handed to the `OlmMachine` on each path, the call ordering in `processSyncResponse`, the `/keys/upload` callback, and an integration test with the real `OlmMachine` checking that a sync carrying only to-device events and device-list changes does not cause a key upload. Fixes matrix-org#5501 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: Romain Le Godais <romain.legodais@intersec.com>
kaylendog
reviewed
Aug 27, 2026
| private async receiveSyncChanges({ | ||
| events, | ||
| oneTimeKeysCounts = new Map<string, number>(), | ||
| oneTimeKeysCounts = this.lastOneTimeKeysCounts ?? new Map<string, number>(), |
Contributor
There was a problem hiding this comment.
I don't think this is quite right. For sync v2, device_one_time_keys_count is allowed to be absent if that particular algorithm has no OTKs. However, this pulls in the previous count, so there's no way for the client to ever realise it has run out of OTKs.
E.g. I upload 50 OTKs to the server, the count of which I cache locally. Over time, these get used up until the last key is claimed, at which point the server omits the device_one_time_keys_count field. However, this parameter defaults to the last count (i.e. 1), so we assume we have one remaining OTK for the rest of time.
Contributor
|
I have been working on a similar (although more complex) fix here: #5503 |
Member
|
@LpZ-squall thanks for the contribution. The team were previously working on this so I'm going to close this in favour of #5503. |
Author
|
Thanks for your reply and for the other fix 👍. |
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
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.
Hello everyone.
This PR fixes #5501. The code was written with Claude Code (Claude Fable 5); I have tested it successfully in the Matrix client I am developing.
As explained in the issue, this is a pretty bad bug: an affected device keeps uploading a new batch of 50 one-time keys on almost every sync. The server accumulates tens of thousands of keys, and since vodozemac only keeps the private half of the most recent ~100, peers who claim an older key can no longer open an Olm session with the device — new conversations become permanently undecryptable, while existing sessions keep working, which makes it hard to diagnose.
Sable's unreleased dev branch already resolves crypto-wasm 18.5.0, so its next release would be affected, and other matrix-js-sdk clients will be as their lockfiles refresh. Element Web is not affected yet only because its lockfile still pins 18.4.0 — and, as the tests in this PR show, bumping it is currently blocked by this bug.
Why
RustCryptocallsOlmMachine.receiveSyncChangesup to three times per/syncresponse, and the to-device and device-list calls passed an empty one-time-key count because they have none of their own. Since matrix-sdk-crypto-wasm 18.5.0 (matrix-rust-sdk#6780) an empty count means "zero keys on the server", so each such sync made theOlmMachinegenerate and upload 50 new one-time keys. Details and measurements in the issue.What
RustCryptonow remembers the count the server last reported and passes it on those calls. TheOlmMachinelearns that count from two places, so both are mirrored:processKeyCountsstores the/synccount, andSyncApi.processSyncResponsenow calls it before processing to-device events and device lists, so the stored count is the one from the same response.OutgoingRequestProcessorreports theone_time_key_countsfrom each/keys/uploadresponse (new optional callback), covering the time before/synchas reported a count.The
SyncCryptoCallbacks.processKeyCountsTSDoc documents the new ordering.Testing
spec/unit/rust-crypto/rust-crypto.spec.ts,OutgoingRequestProcessor.spec.ts, newspec/unit/sync.spec.ts) assert the count handed to theOlmMachineon each path, the/keys/uploadcallback, and the call order inprocessSyncResponse.spec/integ/crypto/crypto.spec.tswith the realOlmMachine: a sync carrying only to-device events and device-list changes must not trigger a/keys/upload.pnpm add -D @matrix-org/matrix-sdk-crypto-wasm@18.6.0 --ignore-scriptsand runspec/integ/crypto. Without this change the new test fails (expected 2 to deeply equal 1) and 18 existing tests incrypto.spec.ts/verification.spec.tstime out, because the first to-device sync triggers a second key upload thatE2EKeyReceiverdeliberately never answers. With it, all 215 pass on 18.6.0, and the full suite passes on the lockfile's 18.4.0. So this also unblocks bumping the binding past 18.4.0.Notes for reviewers
T-Defectlabel.Checklist
public/exportedsymbols have accurate TSDoc documentation.