Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(event cache): fix back-pagination ordering in one specific case #4402

Merged
merged 5 commits into from
Dec 16, 2024

Conversation

bnjbvr
Copy link
Member

@bnjbvr bnjbvr commented Dec 11, 2024

This started as a refactoring of the event cache's logic for back-paginations, for other purposes, but turns out there was a bug if you were in the following conditions:

  • a room has some events
  • the room doesn't have a prev-batch token / a gap

In this case, if we ran a back-pagination, the next prev-batch token would be in the wrong position, compared to the events that we just back-paginated. Which means that the next back-pagination would insert events in the wrong place. The integration test I added acts as a regression test, which failed on main.

The conditions required to cause the bug might have been impossible to
reach in the real world, because it assumes a mix of:

- events present in the linked chunk
- no prev-batch token

However: now that we have storage, we could end up in this situation,
when reaching the start of the timeline (since there'll be no previous
gap in that case). We need to handle that better in the linked chunk
representation itself, but in the meanwhile, we should insert the gap
and the events in a relative correct order.
@bnjbvr bnjbvr requested a review from a team as a code owner December 11, 2024 12:29
@bnjbvr bnjbvr requested review from Hywan and removed request for a team December 11, 2024 12:29
Copy link

codecov bot commented Dec 11, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 85.28%. Comparing base (0264e49) to head (3b42e2b).

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #4402   +/-   ##
=======================================
  Coverage   85.27%   85.28%           
=======================================
  Files         282      282           
  Lines       31209    31211    +2     
=======================================
+ Hits        26615    26619    +4     
+ Misses       4594     4592    -2     

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

Copy link
Member

@Hywan Hywan left a comment

Choose a reason for hiding this comment

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

Looks good to me :-)

@bnjbvr bnjbvr merged commit 5a25e65 into main Dec 16, 2024
40 checks passed
@bnjbvr bnjbvr deleted the bnjbvr/fix-backpagination-ordering branch December 16, 2024 10:56
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