element-hq / element-hq/synapse

Incremental `/sync` and `/transactions` query events differently

Open
#11,394 0 comments 0 reactions 0 assignees View on GitHub
A-Application-Service A-Sync T-Defect
Dominant language
Python
Stars
4.6k
Forks
600
Avg merge
5d 22h
Merged PRs (30d)
51

Description

This issue has been migrated from [#11394](https://github.com/matrix-org/synapse/issues/11394).

---

*As discovered in https://github.com/matrix-org/synapse/pull/11265#discussion_r745413607*

---

`/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.", https://github.com/matrix-org/synapse/commit/1505055334ecff6516c0b388efe4c5759e59fad0 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`](https://spec.matrix.org/v1.1/client-server-api/#get_matrixclientv3sync), 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`](https://github.com/matrix-org/synapse/blob/b09d90cac9179f84024d4cb3ab4574480c1fd1df/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*](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`](https://github.com/matrix-org/synapse/blob/b09d90cac9179f84024d4cb3ab4574480c1fd1df/synapse/rest/client/sync.py#L113)
- [`wait_for_sync_for_user`](https://github.com/matrix-org/synapse/blob/b09d90cac9179f84024d4cb3ab4574480c1fd1df/synapse/handlers/sync.py#L299)
- [`_wait_for_sync_for_user`](https://github.com/matrix-org/synapse/blob/b09d90cac9179f84024d4cb3ab4574480c1fd1df/synapse/handlers/sync.py#L329)
- [`current_sync_for_user`](https://github.com/matrix-org/synapse/blob/b09d90cac9179f84024d4cb3ab4574480c1fd1df/synapse/handlers/sync.py#L399)
- [`generate_sync_result`](https://github.com/matrix-org/synapse/blob/b09d90cac9179f84024d4cb3ab4574480c1fd1df/synapse/handlers/sync.py#L1039)
- [`_generate_sync_entry_for_rooms`](https://github.com/matrix-org/synapse/blob/b09d90cac9179f84024d4cb3ab4574480c1fd1df/synapse/handlers/sync.py#L1455)
- [`_get_rooms_changed`](https://github.com/matrix-org/synapse/blob/b09d90cac9179f84024d4cb3ab4574480c1fd1df/synapse/handlers/sync.py#L1614)
- [`get_room_events_stream_for_rooms`](https://github.com/matrix-org/synapse/blob/b09d90cac9179f84024d4cb3ab4574480c1fd1df/synapse/storage/databases/main/stream.py#L409)
- [`get_room_events_stream_for_room`](https://github.com/matrix-org/synapse/blob/b09d90cac9179f84024d4cb3ab4574480c1fd1df/synapse/storage/databases/main/stream.py#L478-L527)

### `/transactions` stack trace

- [`notify_interested_services`](https://github.com/matrix-org/synapse/blob/b09d90cac9179f84024d4cb3ab4574480c1fd1df/synapse/handlers/appservice.py#L66-L87)
- [`_notify_interested_services`](https://github.com/matrix-org/synapse/blob/b09d90cac9179f84024d4cb3ab4574480c1fd1df/synapse/handlers/appservice.py#L90-L135)
- [`get_new_events_for_appservice`](https://github.com/matrix-org/synapse/blob/b09d90cac9179f84024d4cb3ab4574480c1fd1df/synapse/storage/databases/main/appservice.py#L353-L368)
- [`submit_event_for_as`](https://github.com/matrix-org/synapse/blob/b09d90cac9179f84024d4cb3ab4574480c1fd1df/synapse/handlers/appservice.py#L133-L135)

[`submit_event_for_as`](https://github.com/matrix-org/synapse/blob/b09d90cac9179f84024d4cb3ab4574480c1fd1df/synapse/appservice/scheduler.py#L94-L95) ->

- [`enqueue_event`](https://github.com/matrix-org/synapse/blob/b09d90cac9179f84024d4cb3ab4574480c1fd1df/synapse/appservice/scheduler.py#L129-L131)
- [`_send_request`](https://github.com/matrix-org/synapse/blob/b09d90cac9179f84024d4cb3ab4574480c1fd1df/synapse/appservice/scheduler.py#L137-L157)
- `appservice.scheduler.send`
- [`create_appservice_txn`](https://github.com/matrix-org/synapse/blob/b09d90cac9179f84024d4cb3ab4574480c1fd1df/synapse/appservice/scheduler.py#L196-L201)
- [`AppServiceTransaction.send`](https://github.com/matrix-org/synapse/blob/b09d90cac9179f84024d4cb3ab4574480c1fd1df/synapse/appservice/__init__.py#L338)
- [`push_bulk`](https://github.com/matrix-org/synapse/blob/b09d90cac9179f84024d4cb3ab4574480c1fd1df/synapse/appservice/api.py#L202)
- [`/transactions`](https://github.com/matrix-org/synapse/blob/b09d90cac9179f84024d4cb3ab4574480c1fd1df/synapse/appservice/api.py#L220)

Contributor guide

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.