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.

Sign up for GitHub

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

Jump to bottom

/sync incorrectly calculates state changes for non-gappy syncs #16941

Closed
richvdh opened this issue Feb 19, 2024 · 0 comments · Fixed by #16942
Closed

/sync incorrectly calculates state changes for non-gappy syncs #16941

richvdh opened this issue Feb 19, 2024 · 0 comments · Fixed by #16942
Labels
A-Sync T-Defect

Comments

@richvdh
Copy link
Member

richvdh commented Feb 19, 2024 •
edited
Loading

Consider a DAG like this:

             E1
           ↗    ↖
          |      S2
          |
        --|---
          |
          E3

The client then initialsyncs, with limit=1 (or maybe there are lots of events at E3), represented by the horizontal dashed line.
At this point, we do not expect S2 to appear in the response at all (since it is excluded from the timeline by the limit, and the state is based on the state after the most recent event before the sync token (E3), which doesn't include S2. [cf #16940]).

Now more events arrive, and the client does an incremental sync:

             E1
           ↗    ↖
          |      S2
          |      ↑
          E3     |
          ↑      |
        --|------|----
          |      |
          E4     |
           ↖    /
             E5

S2 is still not included in the response.
This was referenced Feb 19, 2024
Merged
Open
richvdh added a commit that referenced this issue Apr 4, 2024
@richvdh
Fix bug in calculating state for non-gappy syncs (#16942)
0e68e9b 
Unfortunately, the optimisation we applied here for non-gappy syncs is
not actually valid.

Fixes #16941.

~~Based on #16930
Requires matrix-org/sytest#1374.
erikjohnston added a commit that referenced this issue Apr 8, 2024
@erikjohnston
Add back fast path for non-gappy syncs
58d9344 
PR #16942 removed an invalid optimisation that avoided pulling out state
for non-gappy syncs. This causes a large increase in DB usage. c.f. #16941
for why that optimisation was wrong.

However, we can still optimise in the simple case where the events in
the timeline are a linear chain without any branching/merging of the
DAG.
@erikjohnston erikjohnston mentioned this issue Apr 8, 2024
Merged
erikjohnston added a commit that referenced this issue Apr 8, 2024
@erikjohnston
Add back fast path for non-gappy syncs (#17064)
1f8f991 
PR #16942 removed an invalid optimisation that avoided pulling out state
for non-gappy syncs. This causes a large increase in DB usage. c.f.
#16941 for why that optimisation was wrong.

However, we can still optimise in the simple case where the events in
the timeline are a linear chain without any branching/merging of the
DAG.

cc. @richvdh
hughns pushed a commit to hughns/synapse that referenced this issue Apr 9, 2024
@erikjohnston @hughns
Add back fast path for non-gappy syncs (element-hq#17064)
172346d 
PR element-hq#16942 removed an invalid optimisation that avoided pulling out state
for non-gappy syncs. This causes a large increase in DB usage. c.f.
element-hq#16941 for why that optimisation was wrong.

However, we can still optimise in the simple case where the events in
the timeline are a linear chain without any branching/merging of the
DAG.

cc. @richvdh
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
A-Sync T-Defect
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Fix bug in calculating state for non-gappy syncs element-hq/synapse
1 participant
@richvdh