Fix missing sync events during historical batch imports

[Fizzadar]

Discovered after much in-depth investigation in #12281. Adding an explicit order_by argument to change the ordering of results returned from room pagination queries and updating use in the sync & message handlers.

Closes: #12281
Closes: #3305

Signed off by: Nick Mills-Barrett [email protected]

Pull Request Checklist

• Pull request is based on the develop branch
• Pull request includes a changelog file
• Pull request includes a sign off
• Code style is correct
  (run the linters)

[MadLittleMods]

We should add a test for #3305 so it doesn't regress.

I don't have clear reproduction steps for this one and maybe we can just consider the test we write for #12281 as a subset of this problem and only need one test 🤷

[Fizzadar]

@MadLittleMods I'm a bit confused on next steps here - this seems to have unearthed another (complex looking) bug I don't have time (currently) to work on a fix for.

My suggestion would be to roll back to the explicit argument fix which resolves this issue and create a new issue to tackle the get_messages bug (which can also remove the new argument).

[MadLittleMods]

My suggestion would be to roll back to the explicit argument fix which resolves this issue and create a new issue to tackle the get_messages bug (which can also remove the new argument).

@Fizzadar Go for it 👍 We can get some feedback from the Synapse team whether that is acceptable

[MadLittleMods]

@MadLittleMods I'm a bit confused on next steps here - this seems to have unearthed another (complex looking) bug I don't have time (currently) to work on a fix for.

As an update, I've fixed up the other complex bug via #12370

So we can probably leave this PR as-is. I've assigned it back to the Synapse core for review ⏩

[MadLittleMods]

Tested and fails on develop but passes with this PR ✅ (as expected)

[erikjohnston]

Thanks @Fizzadar and @MadLittleMods for tracking this down!

My main concern is how this changes clients back paginating using a prev_batch token from a /sync timeline section. Those tokens are currently stream tokens, but /messages will return them in topological order with a topological next token. This change means that the first /messages will return events in stream order but will still return a topological next token, meaning that the next /messages will return events in a topological order.

I don't think that is a change we really want to make. My understanding is that that behaviour works correctly currently? Or does that also have issues with backfilled/historical events?

[MadLittleMods]

My main concern is how this changes clients back paginating using a prev_batch token from a /sync timeline section. Those tokens are currently stream tokens, but /messages will return them in topological order with a topological next token. This change means that the first /messages will return events in stream order but will still return a topological next token, meaning that the next /messages will return events in a topological order.

-- @erikjohnston, #12319 (comment)

/messages could be updated to always return in topological_ordering regardless of what type of pagination token is given. That's what it was doing before this PR anyway.

I don't think that is a change we really want to make. My understanding is that that behaviour works correctly currently? Or does that also have issues with backfilled/historical events?

I think we have two assumptions that we should probably enforce:

• /messages should always sort by topological_ordering regardless of pagination token given
• /sync should always sort by stream_ordering regardless of pagination token given

This also aligns with what we have documented:

• /sync returns things in the order they arrive at the server (stream_ordering).
• /messages (and /backfill in the federation API) return them in the order determined by the event graph (topological_ordering, stream_ordering).

-- Room DAG concepts -> Depth and stream ordering

The current behavior of /messages and /sync returns events as expected but not state.

There is a bug in how incremental /sync returns state which this PR aims to fix. The best way to understand the problem is probably reading #12281 (comment) - but basically when calculating what state to return, /sync is ordering events by topological_ordering even though it should be stream_ordering.

---

I initially assumed we would want flexibility in how these endpoints sorted events according to the type of pagination token given but it's sounding like we actually want to enforce a given sort according to the endpoint. In which case, we can revert to @Fizzadar initial approach of plumbing a order_by argument which we can set.

[erikjohnston]

(Sorry for not getting to this yesterday, it sat at about the second thing on my todo list all day :/)

Thanks for the clarification. There's the added bonus that /sync will return events in pagination order for the first batch of events for the room, i.e. in an initial sync or when a user joins the room. (The rationale being that it's equivalent to getting no events then immediately doing a back pagination). This is relevant as we use get_recent_events_for_room for that case as well.

I initially assumed we would want flexibility in how these endpoints sorted events according to the type of pagination token given but it's sounding like we actually want to enforce a given sort according to the endpoint. In which case, we can revert to @Fizzadar initial approach of plumbing a order_by argument which we can set.

I wonder if we're just making this harder for ourselves by re-using the same query to do both /messages and when looking up the state for /sync? I think the query we want to run is simply:

SELECT event_id FROM events
WHERE room_id = ? AND stream_ordering <= ?
ORDER BY stream_ordering DESC
LIMIT 1

So it might be simpler to just have a separate get_last_event_in_room_before function that we call when getting the state instead? Rather than extending the already slightly horrific _paginate_room_events_txn function?

---

Entirely separately, and I'm not suggesting we necessarily do this now, but it occurs to me that we have a get_forward_extremities_for_room_at_stream_ordering store function that maps from room_id to the list of forward extremities in the room at the time. This can then be used to calculate the "current" state of the room at the time more accurately than the current method. The downside of this approach is that get_forward_extremities_for_room_at_stream_ordering will fail if the stream ordering is too old.

Another approach that has also literally just now occurred to me is that we could use the current_state_delta_stream to work out the changes in state between two stream orderings in the room, as the stream_id column correspond to the stream orderings of the changes (I think?).

Anyway, just wanted to record my thoughts here before I forget them

[erikjohnston]

@Fizzadar sorry for going around the houses really slowly on this 😞

[Fizzadar]

No worries at all @erikjohnston - totally agree a separate function makes sense here; I've pushed that up in 9a78d14. I just wrapped an existing function get_room_event_before_stream_ordering (which doesn't actually return an event) and pulled out the event.

Have undone the pagination ordering changes too!

[erikjohnston]

Thank you! Will have a look after I've finished doing the v1.57.0rc1 release ❤️

[erikjohnston]

Thank you again for bearing with us @Fizzadar ❤️