Skip to content

fix(pinned events): make sure to initialize ordering before reloading events from disk#6466

Open
bnjbvr wants to merge 2 commits into
mainfrom
bnjbvr/pinned-events-ordering-tracker
Open

fix(pinned events): make sure to initialize ordering before reloading events from disk#6466
bnjbvr wants to merge 2 commits into
mainfrom
bnjbvr/pinned-events-ordering-tracker

Conversation

@bnjbvr

@bnjbvr bnjbvr commented Apr 20, 2026

Copy link
Copy Markdown
Contributor

See the commit description. Honestly, even after writing it myself, I do find that API is a bit of a mess; relying on the linked chunk updates emitted by the same linked chunk might be too hacky for the event ordering to function properly overall. But first, let's fix the bug.

  • I've documented the public API Changes in the appropriate CHANGELOG.md files.
  • This PR was made with the help of AI.

bnjbvr added 2 commits April 20, 2026 13:10
…an be reused for other linked chunks

Makes sense to me to have it in the persistence file, since it's
reloading all the chunks quickly to set the initial ordering.
…disk

This fixes a subtle bug, found by re-enabling the assertion disabled in
github.com//pull/5332. Since the events
ordering was never properly initialized, events couldn't be ordered
against each other even after being added etc.

This could result in the incorrect latest edit being shown in a pinned
events timeline (maybe? it's hard to make a standalone test case for
this, so I'll really on people re-enabling the ordering assertion back
in the future).
@bnjbvr bnjbvr requested a review from a team as a code owner April 20, 2026 11:15
@bnjbvr bnjbvr requested review from Hywan and removed request for a team April 20, 2026 11:15
@codecov

codecov Bot commented Apr 20, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 54.32099% with 37 lines in your changes missing coverage. Please review.
✅ Project coverage is 89.89%. Comparing base (8b14a89) to head (4892c81).
✅ All tests successful. No failed tests found.

Files with missing lines Patch % Lines
crates/matrix-sdk/src/event_cache/persistence.rs 53.84% 32 Missing and 4 partials ⚠️
...ix-sdk/src/event_cache/caches/pinned_events/mod.rs 66.66% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #6466      +/-   ##
==========================================
+ Coverage   89.88%   89.89%   +0.01%     
==========================================
  Files         380      380              
  Lines      105023   105026       +3     
  Branches   105023   105026       +3     
==========================================
+ Hits        94395    94416      +21     
+ Misses       7009     6993      -16     
+ Partials     3619     3617       -2     

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

@codspeed-hq

codspeed-hq Bot commented Apr 20, 2026

Copy link
Copy Markdown

Merging this PR will not alter performance

✅ 50 untouched benchmarks


Comparing bnjbvr/pinned-events-ordering-tracker (4892c81) with main (8b14a89)

Open in CodSpeed

@Hywan Hywan 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.

Do we have a failing test to ensure this patch fixes it?

@bnjbvr

bnjbvr commented Apr 21, 2026

Copy link
Copy Markdown
Contributor Author

The answer is in the second commit's message:

This fixes a subtle bug, found by re-enabling the assertion disabled in github.com/#5332.
[...]
it's hard to make a standalone test case for this, so I'll really on people re-enabling the ordering assertion back in the future

@bnjbvr

bnjbvr commented Apr 21, 2026

Copy link
Copy Markdown
Contributor Author

Fwiw, I've tried to write a failing test for this since the previous message, but I've realized that the ordering might not even be used in the case of the pinned events, so this is only fixing the assertion re-enabled in the test anyways. In other words: there's very little chance that this affects real-world users, at this point.

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.

2 participants