Incremental /sync and /transactions query events differently

[MadLittleMods]

As discovered in #11265 (comment)

---

/sync looks for stream_ordering but excludes all outliers.

synapse/synapse/storage/databases/main/stream.py

Lines 519 to 527 in b09d90c

	sql = """
	SELECT event_id, instance_name, topological_ordering, stream_ordering
	FROM events
	WHERE
	room_id = ?
	AND not outlier
	AND stream_ordering > ? AND stream_ordering <= ?
	ORDER BY stream_ordering %s LIMIT ?
	""" % (

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

synapse/synapse/storage/databases/main/appservice.py

Lines 358 to 368 in b09d90c

	def get_new_events_for_appservice_txn(txn):
	sql = (
	"SELECT e.stream_ordering, e.event_id"
	" FROM events AS e"
	" WHERE"
	" (SELECT stream_ordering FROM appservice_stream_position)"
	" < e.stream_ordering"
	" AND e.stream_ordering <= ?"
	" ORDER BY e.stream_ordering ASC"
	" LIMIT ?"
	)

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.", 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

[MadLittleMods]

Spawning from a discussion with the Rocket.chat folks,

One point against excluding outliers from both; currently when we persist inbound invites over federation, they are set as outliers (related #8626). And since we don't exclude outliers when pushing out events for application service /transactions, the AS can still pick up the invites. We should be careful that this use case still works.