Skip to content

Handle a subtle difference in the reporting of uploaded one-time key counts between the supported sync APIs - #6780

Merged
poljar merged 9 commits into
mainfrom
poljar/one-time-key-counts-sync-difference
Jul 28, 2026
Merged

poljar merged 9 commits into
mainfrom
poljar/one-time-key-counts-sync-difference

Conversation

@poljar

@poljar poljar commented Jul 22, 2026 •

Copy link
Copy Markdown
Contributor

Classic sync and sliding sync report one-time key counts slightly differently.

The following interpretation is used when one-time key counts are absent:

  • Classic sync - No count -> count is 0
  • Sliding sync - No count -> no change in the count.

This PR adds a separate method for the MSC4186-specific sync responses.

  • I've documented the public API changes in the appropriate changelog files (see [Writing changelog entries][pull-request-template-changelog]).
  • This PR was made with the help of AI.

@poljar
poljar requested review from a team as code owners July 22, 2026 12:09
@poljar
poljar requested review from jmartinesp and uhoreg and removed request for a team July 22, 2026 12:09

@jmartinesp jmartinesp left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM in general, but I think this parameter is wrong.

unused_fallback_keys: Option<&[OneTimeKeyAlgorithm]>,
next_batch_token: Option<String>,
decryption_settings: &DecryptionSettings,
msc_4188: bool,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it MSC 4186 or 4188?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're right 86 it is: a576aee.

@andybalaam andybalaam left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, but I'd like to see a test for the new behaviour. (Maybe there is one already?)

Comment thread crates/matrix-sdk-crypto/src/machine/mod.rs
Comment thread crates/matrix-sdk-crypto/src/olm/account.rs
@andybalaam
andybalaam removed the request for review from uhoreg July 22, 2026 12:18
@codspeed

codspeed Bot commented Jul 22, 2026 •

Copy link
Copy Markdown

Merging this PR will not alter performance

✅ 50 untouched benchmarks


Comparing poljar/one-time-key-counts-sync-difference (1c3182b) with main (f883656)

Open in CodSpeed

@codecov

codecov Bot commented Jul 23, 2026 •

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 97.18310% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 90.01%. Comparing base (b641f50) to head (1c3182b).
⚠️ Report is 19 commits behind head on main.
✅ All tests successful. No failed tests found.

Files with missing lines Patch % Lines
...sdk-base/src/response_processors/e2ee/to_device.rs 85.71% 0 Missing and 1 partial ⚠️
crates/matrix-sdk-crypto/src/machine/mod.rs 94.44% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #6780      +/-   ##
==========================================
- Coverage   90.01%   90.01%   -0.01%     
==========================================
  Files         405      405              
  Lines      112979   113046      +67     
  Branches   112979   113046      +67     
==========================================
+ Hits       101700   101755      +55     
- Misses       7426     7437      +11     
- Partials     3853     3854       +1     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

@poljar
poljar requested a review from jmartinesp July 23, 2026 09:52
@jmartinesp

Copy link
Copy Markdown
Contributor

Sorry for the long delay, I didn't realise my previous review was blocking merging this 😓 .

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants