Incremental /sync and /transactions query events differently

[matrixbot]

This issue has been migrated from #11394.

---

As discovered in matrix-org/synapse#11265 (comment)

---

/sync looks for stream_ordering but excludes all outliers.

https://github.com/matrix-org/synapse/blob/b09d90cac9179f84024d4cb3ab4574480c1fd1df/synapse/storage/databases/main/stream.py#L519-L527

Whereas /transactions(Application service API) only cares about stream_ordering.

https://github.com/matrix-org/synapse/blob/b09d90cac9179f84024d4cb3ab4574480c1fd1df/synapse/storage/databases/main/appservice.py#L358-L368

The behavior of these two endpoints should probably return and push the same events.

Here is the history behind why we added AND not outlier to the incremental sync endpoint, "Don't return outliers when we get recent events for rooms.", matrix-org/synapse@1505055 but it doesn't explain the why or which interaction creates outlier events that aren't backfilled.

What does the spec say?

For /transactions, the spec doesn't distinguish which events a homeserver should and shouldn't push, https://spec.matrix.org/v1.1/application-service-api/#put_matrixappv1transactionstxnid

For /sync, there is also nothing so clear cut but it's obvious using the since pagination query parameter that it should return anything after which equates to stream_ordering in Synapse land.

Potential solutions

Exclude outliers in both

Perhaps we should exclude outliers in /transactions so they both just match?

@richvdh had some critique about this approach though:

the outlier flag seems like a poor way to decide whether we should push this data. (Yes, outliers shouldn't be sent over /transactions, but there are probably many other events which shouldn't be sent).

FederationEventHandler._process_received_pdu has a backfilled parameter (see synapse/handlers/federation_event.py#L945), whose purpose is slightly unclear, but I think one of its jobs is this sort of thing. Maybe we should use similar logic to that, somehow?

-- @richvdh, https://github.com/matrix-org/synapse/pull/11265#discussion_r746523400

But for /transactions, we can't tell whether the event was backfilled. The only indication is that the stream_ordering would be negative which is what the /transactions code already takes into account.

Only exclude backfilled events

This means only relying on stream_ordering.

Then any interaction creating outliers that we don't want to appear down /sync//transactions, should be updated to be also marked as backfilled.

Dev notes

/sync stack trace

• SyncRestServlet.on_GET
• wait_for_sync_for_user
• _wait_for_sync_for_user
• current_sync_for_user
• generate_sync_result
• _generate_sync_entry_for_rooms
• _get_rooms_changed
• get_room_events_stream_for_rooms
• get_room_events_stream_for_room

/transactions stack trace

• notify_interested_services
• _notify_interested_services
  • get_new_events_for_appservice
  • submit_event_for_as

submit_event_for_as ->

• enqueue_event
• _send_request
• appservice.scheduler.send
• create_appservice_txn
• AppServiceTransaction.send
• push_bulk
• /transactions