Ensure clients see their transaction_ids

[DMRobertson]

Suppose

• Alice has made an incremental (i.e. with pos) sliding sync.
• The proxy has nothing to give to Alice right now and is waiting for a live update.
• Alice /sends a message into a room using a transaction_id.
• Bob is also in the room and using the proxy.
• Bob's poller sees Alice's message without a transaction ID.
• This message is handed off to Alice's /sync handler and sent to Alice without a transaction id.
• Alice's poller later sees the message with the transaction id.
• (Not sure if that txn_id ever arrives at Alice's client?)

The bold bullet is bad because it means that the client doesn't realise it has just received the event it sent, meaning that the "local echo" flickers.

We want to fix this by never notifiying a waiting sync handler about Things until its dedicated poller has seen that Thing. Do this by:

1. Introduce a buffer to the API machinery which holds events which might have a transaction ID, but don't. Have entries leave the buffer (and proceed to conns) after at most a second. Stack up new message events behind it in the bugger.
2. Change the poller to track which devices have seen an event for their user which lacks a transaction ID.
3. If a poller sees a transaction ID, or if all pollers see no transaction ID, emit a payload telling the API machinery to stop waiting for a transaction ID.
4. When the API machinery receives that payload, clear up to its corresponding event in the buffer.

[DMRobertson]

I assume everything is broken because I'm now delaying certain messages for up to a second.

[kegsay]

Looks sensible. Needs more testing and removal of visiblity checks.

[kegsay]

I'd like to see some e2e tests to try to repro this bug (it'll flakey fail but that's better than not having any e2e tests here). This ensures that we are using the right API (e.g checking the right fields, etc.

Specifically:

• The alice/bob use case described in a comment.
• Multiple devices for alice, where other devices see the event without a txn id.

[kegsay]

We neeed e2e tests for this. We cannot force the race to happen, but we can greatly encourage it by spamming events. I would do something like:

• Make alice and bob, join to same room.
• goroutine to repeatedly sliding sync as alice and accumulate the events without making assertions.
• goroutine to repeatedly sliding sync as bob and accumulate the events without making assertions.
• for i in N:
  • Alice sends message with txnid i, remember event_id -> txnid mapping.
• Terminate goroutines when final event id is seen on both alice and bob.
• Assert that alice has the correct txn IDs for all N events.

[DMRobertson]

I went for something simpler, see baa3596.

Running on that commit:

$ SYNCV3_SERVER=http://localhost:8888 SYNCV3_ADDR=http://localhost:8844 go test ./tests-e2e -v -count=10 -v -run TestTransactionIDsAppearWithMultiplePollers | egrep 'PASS|FAIL'
egrep: warning: egrep is obsolescent; using grep -E
--- PASS: TestTransactionIDsAppearWithMultiplePollers (2.53s)
--- PASS: TestTransactionIDsAppearWithMultiplePollers (2.67s)
--- PASS: TestTransactionIDsAppearWithMultiplePollers (2.67s)
--- PASS: TestTransactionIDsAppearWithMultiplePollers (2.49s)
--- PASS: TestTransactionIDsAppearWithMultiplePollers (2.71s)
--- PASS: TestTransactionIDsAppearWithMultiplePollers (2.63s)
--- PASS: TestTransactionIDsAppearWithMultiplePollers (2.80s)
--- PASS: TestTransactionIDsAppearWithMultiplePollers (2.70s)
--- PASS: TestTransactionIDsAppearWithMultiplePollers (2.66s)
--- PASS: TestTransactionIDsAppearWithMultiplePollers (2.65s)
PASS

Running on 595e1b3 (an older main):

$ SYNCV3_SERVER=http://localhost:8888 SYNCV3_ADDR=http://localhost:8844 go test ./tests-e2e -v -count=10 -v -run TestTransactionIDsAppearWithMultiplePollers | egrep 'PASS|FAIL'
egrep: warning: egrep is obsolescent; using grep -E
--- FAIL: TestTransactionIDsAppearWithMultiplePollers (1.65s)
--- FAIL: TestTransactionIDsAppearWithMultiplePollers (1.57s)
--- FAIL: TestTransactionIDsAppearWithMultiplePollers (1.80s)
--- FAIL: TestTransactionIDsAppearWithMultiplePollers (1.63s)
--- FAIL: TestTransactionIDsAppearWithMultiplePollers (1.69s)
--- FAIL: TestTransactionIDsAppearWithMultiplePollers (1.61s)
--- FAIL: TestTransactionIDsAppearWithMultiplePollers (1.69s)
--- FAIL: TestTransactionIDsAppearWithMultiplePollers (1.78s)
--- FAIL: TestTransactionIDsAppearWithMultiplePollers (1.56s)
--- FAIL: TestTransactionIDsAppearWithMultiplePollers (1.70s)
FAIL
FAIL	github.com/matrix-org/sliding-sync/tests-e2e	16.677s
FAIL

In another test of 100 runs, I saw this fail 95 times and pass 5 times.

The 1 second difference in timing makes me wonder if something has gone wrong with the all clear mechanism. I will investigate.

[DMRobertson]

Ah, we would not emit the V2TransactionID payloads if there were no new events.

28b11f8 contains the fix. With it:

$ SYNCV3_SERVER=http://localhost:8888 SYNCV3_ADDR=http://localhost:8844 go test ./tests-e2e -v -count=10 -v -run TestTransactionIDsAppearWithMultiplePollers | egrep 'PASS|FAIL'
egrep: warning: egrep is obsolescent; using grep -E
--- PASS: TestTransactionIDsAppearWithMultiplePollers (1.65s)
--- PASS: TestTransactionIDsAppearWithMultiplePollers (1.61s)
--- PASS: TestTransactionIDsAppearWithMultiplePollers (1.84s)
--- PASS: TestTransactionIDsAppearWithMultiplePollers (1.53s)
--- PASS: TestTransactionIDsAppearWithMultiplePollers (1.96s)
--- PASS: TestTransactionIDsAppearWithMultiplePollers (1.69s)
--- PASS: TestTransactionIDsAppearWithMultiplePollers (1.64s)
--- PASS: TestTransactionIDsAppearWithMultiplePollers (1.68s)
--- PASS: TestTransactionIDsAppearWithMultiplePollers (1.65s)
--- PASS: TestTransactionIDsAppearWithMultiplePollers (1.63s)
PASS