CS API txn ids are dropped on multiple requests (SYN-603)

[matrixbot]

Currently we store the latest transaction id for a (path, access_token) tuple. This is problematic in the case tjat a client sends, say, two messages at the same time and the first to arrive at the server is retried. This first txn id is inserted into the transaction store, and is then immediately clobbered by the second txn id.

This results in duplicates.

(Imported from https://matrix.org/jira/browse/SYN-603)

(Reported by @erikjohnston)

[matrixbot]

Jira watchers: @erikjohnston

[matrixbot]

Links exported from Jira:

relates to SYJS-41

[matrixbot]

Another issue is that currently Matrix uses a two-phase commit, like so:

submit  :: txn -> server
confirm :: txn -> client
// server frees txn at some point

Because the server frees the txn without knowing that the client received the confirmation, this can result in eventual duplicates no matter what storage strategy is used (except buffering forever).

(Erik raised that resubmitting past 1h is silly; I pointed out that knowing it's been 1h is depressingly nontrivial. Erik pointed out that he hates time; I concurred.)

A solution would be to use three-phase commit, like so:

submit  :: txn -> server
confirm :: txn -> client
reap    :: txn -> server // server frees txn immediately

As reap is inherently idempotent, it does not need a txn itself.

This can be optimized further, by having reap take a list of txn IDs to reap, and having it return a list of unreaped txns.

However, this raises the question of unbounded resource usage, if the client never reaps (and as Erik pointed out - unless it must, it won't.)

My suggestion is to, for txns submitted via post-reap API versions, reject the transaction with 'm.transactions.unreaped' (or similar) when the number of unreaped transactions is at a (configurable) cap.

For txns submitted by pre-reap API versions, some form of autoreaping is needed to avoid either unbounded resource usage or breaking clients. I suggest reaping the single oldest unreaped txn when the cap is reached; though Erik seems to favor a time-based component to the autoreaping.

-- Alex Elsayed

[kegsay]

This was fixed in #1624 because we no longer clobber based on the path without the txn ID. It uses the "time-based" component for reaping, which is current 30 minutes.