Fix state events missing from MSC4222 state_after when the since token falls inside a persist batch - #20171
Draft
barodeur wants to merge 7 commits into
Draft
Fix state events missing from MSC4222 state_after when the since token falls inside a persist batch#20171barodeur wants to merge 7 commits into
state_after when the since token falls inside a persist batch#20171barodeur wants to merge 7 commits into
Conversation
A since token that falls inside a persist batch drops the deltas of the batch's state events, because current_state_delta_stream rows are stamped with the batch minimum stream ordering while the timeline is selected on the event's own ordering. A worker reading the events stream from replication routinely observes such a token, so on worker deployments a state event can be served in the sync timeline of a room the user is still joined to while being absent from state_after, breaking the MSC4222 invariant that `state at since` + `state_after` equals the state at the end of the timeline. The tests document the storage-level and replication-level preconditions, reproduce the bug end-to-end (including the gappy variant where the affected state event is truncated out of the timeline), and guard that state_after reports the resolved state at the end of the timeline rather than a replay of the timeline. Three tests fail at this commit; the next commit fixes them.
current_state_delta_stream rows are stamped with the *minimum* stream ordering of the persist batch of their event, so bounding the MSC4222 delta query on the row's stream_id drops the deltas of a batch's state events for any since token that falls inside the batch. A worker reading the events stream from replication routinely observes such a token, since RDATA advances the stream one event at a time; the state event is then in the sync timeline but missing from state_after, or -- when the timeline is truncated -- silently missing altogether. Add get_current_state_deltas_for_room_by_event_position, which bounds each delta on the maximum of the row's stream_id and its event's own stream ordering: - a state event persisted mid-batch is tracked at its own position - rows with no event (the last-local-user state clearance) keep the row's position - rows stamped after their event (the partial-state resync path, which re-announces existing state at a fresh position) also keep the row's position, preserving the re-announcement That maximum is not a bound an index can serve, so the window is fetched as the union of two index-driven sets, with exact per-writer filtering in Python via _filter_results_by_stream as the existing query does: rows whose own stream_id is in the window (an index range on (room_id, stream_id), the same cost as the existing query), and rows whose *event* is in the window (driven by the events (room_id, stream_ordering) index over the window's state events, joined back via a new partial index on current_state_delta_stream(event_id) -- a row stamped below the window with an effective position inside it must have its event inside the window). Overall cost is proportional to the window, as before. The early-return optimisation consults the events stream cache as well as the delta stream cache, since a delta's effective position can now exceed the row stamp the delta cache tracks. Only the MSC4222 sync path uses the new method; every other consumer of current_state_delta_stream is untouched.
3 tasks
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.
This PR fixes the issue described as comment here: #18793 (comment)
In Element Call, this shows up as ghost participants: someone who left the call keeps being displayed until a later state change refreshes the room.
The bug is not specific to Element Call: any state event can be affected, RTC membership just changes often enough to make it visible.
What happens
Alice has a client syncing against a homeserver where events are persisted by one worker (the event persister) and
/syncis served by another (the sync worker). Her client is parked in a long-poll:GET /sync?since=s99&timeout=30000.Bob joins a call at the same moment Carol sends a message. Carol's message reaches the persister first; Bob's
m.call.memberarrives while that write is still in flight, so the per-room persist queue groups them into one transaction:The transaction commits: both events and the delta row are now in the database, atomically.
The persister then announces the new events over replication, one RDATA token per stream ordering — rows are only merged into one token when they share a position, and 100 and 101 don't. So the sync worker's events-stream position steps 99 → 100 → 101, and on reaching 100 it pokes the notifier.
Alice's long-poll wakes at exactly that moment. Her response is built at the worker's current position —
end = 100— with RDATA 101 still in the queue:No race on the client's side is needed: the server hands out the mid-batch token as
next_batch. Alice's client re-polls with it, as every sync client does. The worker has meanwhile processed RDATA 101:A state event in the timeline with an empty
state_after. An MSC4222 client trustsstate_afterover timeline state events, so Alice's copy of Bob's call membership never updates from this response.On a single process this cannot happen: the batch's stream IDs are released as a whole, so the position visible to
/syncjumps 99 → 101 ands100is never handed out. Only a process that learns its position from replication — any sync worker — ticks through the middle of a batch.Pull Request Checklist
EventStoretoEventWorkerStore.".code blocks.