Skip to content

Fix _get_server_keys_json invalidations over replication as JSON - #19966

Merged
erikjohnston merged 2 commits into
developfrom
erikj/portdb6-pr1-storage-type-correctness
Jul 27, 2026
Merged

erikjohnston merged 2 commits into
developfrom
erikj/portdb6-pr1-storage-type-correctness

Conversation

@erikjohnston

@erikjohnston erikjohnston commented Jul 15, 2026 •

Copy link
Copy Markdown
Member

The cache key of _get_server_keys_json is a single argument which is itself a (server_name, key_id) tuple, and store_server_keys_response passed that nested tuple straight into the cache invalidation stream.
psycopg2 quietly serialises the inner tuple as a Postgres record, so the keys column of cache_invalidation_stream_by_instance ended up holding the record literal as a single string (e.g. {"(srv,ed25519:abc)"}) — which never matches the real cache key on the receiving side, i.e. the
invalidation has always been a silent no-op on workers. The native Rust backend's stricter parameter conversion turns the same nested tuple into a loud TypeError: unsupported parameter type for postgres: tuple.

Fix it the same way #18899 did for _get_e2e_cross_signing_signatures_for_device, which has the same nested-tuple key shape: invalidate the local cache directly, JSON-encode the key for the replication row, and decode it again in process_replication_rows.

Found as part of the the effort to port the database pool to Rust.

@erikjohnston erikjohnston changed the title Erikj/portdb6 pr1 storage type correctness Storage type correctness: user-directory temp position & server-keys cache invalidation Jul 15, 2026
erikjohnston and others added 2 commits July 15, 2026 16:03
The cache key of `_get_server_keys_json` is a single argument which is
itself a `(server_name, key_id)` tuple, and `store_server_keys_response`
passed that nested tuple straight into the cache invalidation stream.
psycopg2 quietly serialises the inner tuple as a Postgres *record*, so the
`keys` column of `cache_invalidation_stream_by_instance` ended up holding
the record literal as a single string (e.g. `{"(srv,ed25519:abc)"}`) — which
never matches the real cache key on the receiving side, i.e. the
invalidation has always been a silent no-op on workers. The native Rust
backend's stricter parameter conversion turns the same nested tuple into a
loud `TypeError: unsupported parameter type for postgres: tuple`.

Fix it the same way #18899 did for
`_get_e2e_cross_signing_signatures_for_device`, which has the same
nested-tuple key shape: invalidate the local cache directly, JSON-encode the
key for the replication row, and decode it again in
`process_replication_rows`.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_019ZdvuLkg7Lm7wDtPQzDnJx
@erikjohnston
erikjohnston force-pushed the erikj/portdb6-pr1-storage-type-correctness branch from 2b4c689 to 6f02259 Compare July 15, 2026 15:04
@erikjohnston erikjohnston changed the title Storage type correctness: user-directory temp position & server-keys cache invalidation Send _get_server_keys_json invalidations over replication as JSON Jul 15, 2026
@erikjohnston
erikjohnston marked this pull request as ready for review July 16, 2026 08:45
@erikjohnston
erikjohnston requested a review from a team as a code owner July 16, 2026 08:45
@erikjohnston erikjohnston changed the title Send _get_server_keys_json invalidations over replication as JSON Fix _get_server_keys_json invalidations over replication as JSON Jul 16, 2026
@MadLittleMods MadLittleMods added the A-Workers (includes replication) label Jul 16, 2026
@anoadragon453 anoadragon453 self-assigned this Jul 24, 2026
@anoadragon453
anoadragon453 requested a review from Copilot July 24, 2026 10:17

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR fixes multi-worker cache invalidation for _get_server_keys_json, whose cache key is a single argument that is itself a (server_name, key_id) tuple. It changes the replication payload to JSON-encode that nested tuple (so it can be represented as text[] cleanly), and adds the corresponding JSON decode path on workers to ensure the correct cache entry is invalidated.

Changes:

  • Invalidate _get_server_keys_json locally, but replicate invalidations using JSON-encoded (server_name, key_id) pairs to avoid nested tuple serialization issues.
  • Teach cache invalidation replication processing to decode _get_server_keys_json keys from JSON and invalidate the correct entry.
  • Add a regression test ensuring the invalidation round-trip works across replication, plus a Towncrier newsfragment.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.

File Description
tests/storage/databases/main/test_cache.py Adds a multi-worker regression test validating _get_server_keys_json invalidation replication round-trip.
synapse/storage/databases/main/keys.py Fixes invalidation emission by JSON-encoding nested cache keys for replication while still invalidating locally.
synapse/storage/databases/main/cache.py Decodes JSON-encoded keys for _get_server_keys_json during replication processing and invalidates the proper cache entry.
changelog.d/19966.bugfix Adds a newsfragment documenting the worker invalidation fix.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@anoadragon453 anoadragon453 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems like it'd be nice to just teach the replication tooling to recognise non-primitive data structures being passed and auto-serialise instead.

But I guess for now with only two instances of this, it's not necessary. Would be nice to prevent future footguns however.

@erikjohnston
erikjohnston merged commit 5167280 into develop Jul 27, 2026
82 of 84 checks passed
@erikjohnston
erikjohnston deleted the erikj/portdb6-pr1-storage-type-correctness branch July 27, 2026 08:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

A-Federation A-Workers (includes replication)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants