Stabilize support for MSC3970: updated transaction semantics (scope to device_id)

[clokep]

MSC3970 passed FCP, we can rip out the unstable config for it now.

[clokep]

CC @sandhose & @hughns

[sandhose]

The complement tests are failing because we explicitly added tests to test the pre-MSC3970 behaviour, and there are post-MSC3970 tests ready under a build flag

[clokep]

The complement tests are failing because we explicitly added tests to test the pre-MSC3970 behaviour, and there are post-MSC3970 tests ready under a build flag

I'm a bit confused about this, I see:

• Tests for transaction ID semantics complement#622 -- This has some "definitely good tests" which were pulled out of...
• Test the scope of a transaction IDs complement#613 -- This tests the "current spec behaviour", but I don't know if that means pre-MSC3970 spec or post-MSC3970 spec.

@sandhose Can you help me untangle this? From my reading of the above, I'm surprised anything is failing with this PR?

[hughns]

@clokep I spoke with Q earlier and made a plan to untangle this.

We now have a single Complement PR, matrix-org/complement#637, which shows the appropriate changes to assert the new transaction ID behaviour.

However, as noted in comments in the PR the tests are currently skipped on Synapse and Dendrite due to neither supporting the new transaction behaviour yet.

How should this be handled? Effectively both this PR and the one in Complement should be merged at the same time.

(As discussed in the MSC, this is technically a breaking change hence the cyclical dependency)

[clokep]

However, as noted in comments in the PR the tests are currently skipped on Synapse and Dendrite due to neither supporting the new transaction behaviour yet.

How should this be handled? Effectively both this PR and the one in Complement should be merged at the same time.

I repushed matrix-org/complement#637 as clokep/stable-txn and added a commit to remove the test skipping for Synapse (matrix-org/complement@768f359) so that tests will run in CI via branch matching.

[reivilibre]

Nothing major; happy to leave any minor comment changes in your hands

[reivilibre]

Is there a target version of SCHEMA_COMPAT_VERSION you need? If you can find out, might be worth writing it here.

[clokep]

Errr, so right now we write to both event_txn_id and event_txn_id_device_id. I think we want to leave this as-is so we can rollback if needed.

I guess in the future we would want to:

• Wait
• Stop writing (and reading to) to event_txn_id and bump the SCHEMA_VERSION
• Wait
• Bump the SCHEMA_COMPAT_VERSION, rip out the code

Does that sound like the right series of steps?

[clokep]

We talked about this a bit: https://matrix.to/#/!vcyiEtMVHIhWXcJAfl:sw1v.org/$WQwonqO7etYpa0ik4aVzVsf32R_VOqIyPzBjTl3pjps?via=matrix.org&via=element.io&via=beeper.com

My tl;dr is that the above steps look reasonable, although you usually want to stop reading before writing. Unfortunately we do still use this for certain situations, e.g. appservice users don't have device IDs. So we would need to figure out what to do there?

My inclination is to merge this as-is (so as to not block Matrix 1.7 support) and file a follow-up.

[MadLittleMods]

Unfortunately we do still use this for certain situations, e.g. appservice users don't have device IDs. So we would need to figure out what to do there?

Probably good to add a comment to the diff on the caveat for why the access token fallback needs to stay until that is figured out

[clokep]

Yeah, I was planning to point to an issue. Was waiting to see if others had thoughts before clicking merge.

[clokep]

Taking a step back and re-evaluating my other threads with @sandhose (and re-reading the comments in this PR) I don't actually think we can remove this since it gets used for guests, appservice users, and access tokens from the admin API. 😢

I've made two changes:

• Consolidate some logic so we have fewer places we attempt to line up an event from a transaction ID.
• Added a lot more comments.

Could folks (in particular @sandhose) let me know if these still read OK? I feel like I'm missing something, but I've tried to include my current understanding in the comments.

[sandhose]

The thing is, guests and appservice users don't have access token IDs anyway, so it basically won't do anything if we keep it. The only thing left is admin-minted, device-less tokens.

Keep in mind, there are two layers of things that ensure idempotency:

• the in-memory HTTP transaction cache, which properly covers guests, appservices and device-less tokens
• the db-persisted (for 24h) transaction_id -> event_id mapping, which only worked for access tokens with IDs in it, so no guests and no appservices

Also, this is only for idempotency, the transaction ID echo in event serialisation is handled by the event internal metadata JSON blob, which is separate from those tables.

If I'm not mistaken, I think this would only affect

• device-less real users
• when they replay a request, either load-balanced to another worker, or after a Synapse restart

I don't mind keeping this, but also I don't think it's worth keeping it around.

[clokep]

Also, this is only for idempotency, the transaction ID echo in event serialisation is handled by the event internal metadata JSON blob, which is separate from those tables.

And I suppose the same rules apply to the event internal metadata info -- guests and appservices users don't have access token IDs so we can't be storing it there anyway.

[clokep]

Thinking through this a bit more I think we need two separate approaches:

Database transaction mapping

This describes what to do with "the db-persisted (for 24h) transaction_id -> event_id mapping".

This only survives for 24 hours, but that's not really relevant to the conversation of rolling back (or forwards). As background, there's two tables (event_txn_id and event_txn_id_device_id) for access token IDs and device IDs, respectively. To gracefully handle up/downgrades I propose:

1. (This PR = N) Always write to both and read data from either (first event_txn_id_device_id, fallback to event_txn_id). Bump the schema version to V.
2. In a future PR (N+2 versions), stop reading and writing to event_txn_id, bump the schema version to V+1.

At this point we have a database schema still compatible with today, but we're not using data event_txn_id, so rolling back to e.g. N is fine, but not before N. To enforce this, bump the minimum schema version to V.

3. In a future PR (N+4 versions), add a schema delta which drops the event_txn_id table.

At this point we no longer have a database schema compatible with N, but we can still rollback OK to N+2. So bump the minimum schema version to V+1.

Internal metadata

The device ID & access token ID are unconditionally written into the internal metadata of events. It then follows similar logic where the transaction ID is returned to a user when serializing the event if:

1. The device ID exists in the internal metadata and matches that of the sender. ¹
2. The requester & sender user IDs match and:
   • The token ID exists in the internal metadata and matches that of the sender.
   • The requester is a guest.
   • The requester is an appservice.

A similar thing should be followed:

1. (This PR = N) Start using the device ID in-lieu of the token ID, if it is set. (Note that both are currently saved in the internal metadata.)
2. In a future PR (N+2), stop writing the token ID into the internal metadata.

This implies that we would leave the logic for the fallback "forever", but I think we're already at am minimum schema version where we've always written both values, so we only need to be concerned with old data.

An option here would be to do a data migration for any event who has a token ID which still exists in the database, add the device ID. Then remove the fallback code. This seems a bit onerous though.

Footnotes

1. Device IDs matching should also be gated by the user ID, I think. There's more risk of those overlapping that the token IDs since they're client controllable. ↩

[clokep]

#16042 was filed to handle follow-up.

[sandhose]

Overall looks good, we just need to decide whether we should keep the (txn_id, token_id) -> event_id mapping in DB or not

[sandhose]

The thing is, guests and appservice users don't have access token IDs anyway, so it basically won't do anything if we keep it. The only thing left is admin-minted, device-less tokens.

Keep in mind, there are two layers of things that ensure idempotency:

• the in-memory HTTP transaction cache, which properly covers guests, appservices and device-less tokens
• the db-persisted (for 24h) transaction_id -> event_id mapping, which only worked for access tokens with IDs in it, so no guests and no appservices

Also, this is only for idempotency, the transaction ID echo in event serialisation is handled by the event internal metadata JSON blob, which is separate from those tables.

If I'm not mistaken, I think this would only affect

• device-less real users
• when they replay a request, either load-balanced to another worker, or after a Synapse restart

I don't mind keeping this, but also I don't think it's worth keeping it around.

[clokep]

I've put this back into the review queue -- I split some changes out and made a few additional changes to this PR. See above for an explanation of future work / how the database changes will work.