From af09b0ce2cc1c24c5f571693ed7c77ac9a8135c8 Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Fri, 3 Jul 2026 11:43:07 +0000 Subject: [PATCH 1/7] Commit id-generator construction in the multi-writer id-gen tests MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Constructing a `MultiWriterIdGenerator` prunes stale `stream_positions` rows for writers no longer in the config. The test harness built each generator inside a `runWithConnection` callback and never committed, so that cleanup only persisted because adbapi keeps one connection per thread and leaves its transaction open across calls — a later generator on the same thread then saw the uncommitted delete. A pool that hands out a fresh connection per call (rolling back any left mid-transaction) loses the cleanup, so test_writer_config_change read a stale writer position (persisted-upto 3 instead of 6). Commit the connection after constructing the generator, so the cleanup persists regardless of pool semantics; the delete is legitimate work that should be committed anyway. Found during a Rust port. Fixes test_writer_config_change; psycopg2 and sqlite still pass. Co-Authored-By: Claude Opus 4.8 (1M context) Claude-Session: https://claude.ai/code/session_01W3G4M92AmwSSZCbmtMJU3d (cherry picked from commit 8a7b3a2cbd6f2b068a59dcdc899c900a94042223) --- tests/storage/test_id_generators.py | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/tests/storage/test_id_generators.py b/tests/storage/test_id_generators.py index 051c5de44dd..9a338607eef 100644 --- a/tests/storage/test_id_generators.py +++ b/tests/storage/test_id_generators.py @@ -78,7 +78,7 @@ def _create_id_generator( writers: list[str] | None = None, ) -> MultiWriterIdGenerator: def _create(conn: LoggingDatabaseConnection) -> MultiWriterIdGenerator: - return MultiWriterIdGenerator( + id_gen = MultiWriterIdGenerator( db_conn=conn, db=self.db_pool, notifier=self.hs.get_replication_notifier(), @@ -90,6 +90,15 @@ def _create(conn: LoggingDatabaseConnection) -> MultiWriterIdGenerator: writers=writers or ["master"], positive=self.positive, ) + # Constructing the generator prunes stale `stream_positions` rows + # (writers no longer in the config); commit so that persists for the + # next generator we create. + # + # Note we need to commit manually here as the generator is created + # in a `runWithConnection` call, which doesn't automatically + # commit/rollback. + conn.commit() + return id_gen self.instances[instance_name] = self.get_success_or_raise( self.db_pool.runWithConnection(_create) From c4c3838a5b63c09c52472c52119393887b2ef686 Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Fri, 3 Jul 2026 11:16:51 +0000 Subject: [PATCH 2/7] Bind an int, not a stringified int, to a BIGINT column in the state test MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit test_batched_state_group_storing selected from `state_group_edges` with `keyvalues={"state_group": str(context.state_group_after_event)}` — a Python str bound to a BIGINT column. This only worked because psycopg2 coerces a numeric string to an integer; a stricter driver that binds typed parameters rejects it (`error serializing parameter 0`), as it should — the column is an integer. Pass the int directly. Works identically on psycopg2 and sqlite, without reintroducing loose int/text coercion. Found during a Rust port. Co-Authored-By: Claude Opus 4.8 (1M context) Claude-Session: https://claude.ai/code/session_01W3G4M92AmwSSZCbmtMJU3d (cherry picked from commit 55baa50ece083e3423088a95cdd3c76b447436dd) --- tests/storage/test_state.py | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/tests/storage/test_state.py b/tests/storage/test_state.py index dbbede812d9..6c3506f348a 100644 --- a/tests/storage/test_state.py +++ b/tests/storage/test_state.py @@ -637,9 +637,7 @@ def test_batched_state_group_storing(self) -> None: self.get_success( self.store.db_pool.simple_select_list( table="state_group_edges", - keyvalues={ - "state_group": str(context.state_group_after_event) - }, + keyvalues={"state_group": context.state_group_after_event}, retcols=("prev_state_group",), ) ), From ba765134c5c2f8ec171ccf1741f838968acc467f Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Fri, 3 Jul 2026 10:01:49 +0000 Subject: [PATCH 3/7] Store the user-directory temp position as BIGINT MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The `populate_user_directory` background update stored its stream position in a `TEXT` column (`_temp_populate_user_directory_position.position`) but wrote an int into it and read it back into the `BIGINT` `user_directory_stream_pos.stream_id` — the one place in the storage layer that bound an int to a text column (and a numeric string back to an int column). This only worked because psycopg2 coerces such literals; a stricter driver that binds typed parameters rejects the mismatch. Fix it at the source: make the temp column `BIGINT`, matching the value it holds (and `update_user_directory_stream_pos`'s `int` type hint, which the `TEXT` column had been quietly violating). The temp table is created and dropped within the background update, so there's no migration. Found during a Rust port. Tested: user_directory storage + handler tests pass on psycopg2 and sqlite. Co-Authored-By: Claude Opus 4.8 (1M context) Claude-Session: https://claude.ai/code/session_01W3G4M92AmwSSZCbmtMJU3d (cherry picked from commit 89dbdf52465ae4fec05c1028eaf70058ac6eaa89) --- synapse/storage/databases/main/user_directory.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/synapse/storage/databases/main/user_directory.py b/synapse/storage/databases/main/user_directory.py index 6c5abc71aea..e3f16ef7547 100644 --- a/synapse/storage/databases/main/user_directory.py +++ b/synapse/storage/databases/main/user_directory.py @@ -127,7 +127,7 @@ def _make_staging_area(txn: LoggingTransaction) -> None: sql = f""" CREATE TABLE IF NOT EXISTS {TEMP_TABLE}_position ( - position TEXT NOT NULL + position BIGINT NOT NULL ) """ txn.execute(sql) From 8bfc01e48863e105a9890650792da1648e0f47e0 Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Fri, 3 Jul 2026 14:49:37 +0000 Subject: [PATCH 4/7] Store rejections.last_check as text, matching its column type MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `rejections.last_check` is a TEXT column, but both writers stored `clock.time_msec()` (an int) into it, relying on the driver to coerce int→text. A stricter driver that binds typed parameters rejects that (error serializing parameter), so marking an event rejected failed — e.g. paginating through rejected events (test_room_messages_paginate_through_rejected_events). Store the timestamp as a string, matching the declared TEXT column, rather than papering over the mismatch with loose int/text coercion. psycopg2 and sqlite are unaffected (a text value is equally valid there). Found during a Rust port. Co-Authored-By: Claude Opus 4.8 (1M context) Claude-Session: https://claude.ai/code/session_01W3G4M92AmwSSZCbmtMJU3d (cherry picked from commit fb6f0d67dc01145df9c060efac811288da3b5e11) --- synapse/storage/databases/main/events.py | 5 ++++- synapse/storage/databases/main/events_worker.py | 5 ++++- 2 files changed, 8 insertions(+), 2 deletions(-) diff --git a/synapse/storage/databases/main/events.py b/synapse/storage/databases/main/events.py index 84b38f4bf2a..d92bbeeae31 100644 --- a/synapse/storage/databases/main/events.py +++ b/synapse/storage/databases/main/events.py @@ -3513,7 +3513,10 @@ def _store_rejections_txn( values={ "event_id": event_id, "reason": reason, - "last_check": self._clock.time_msec(), + # `last_check` is a TEXT column, so store the timestamp as a + # string rather than relying on the driver to coerce an int. + # (Ideally we'd fix the schema, but that is non-trivial) + "last_check": str(self._clock.time_msec()), }, ) diff --git a/synapse/storage/databases/main/events_worker.py b/synapse/storage/databases/main/events_worker.py index d40240a0a5c..27dab290b39 100644 --- a/synapse/storage/databases/main/events_worker.py +++ b/synapse/storage/databases/main/events_worker.py @@ -2702,7 +2702,10 @@ def mark_event_rejected_txn( keyvalues={"event_id": event_id}, values={ "reason": rejection_reason, - "last_check": self.clock.time_msec(), + # `last_check` is a TEXT column, so store the timestamp as a + # string rather than relying on the driver to coerce an int. + # (Ideally we'd fix the schema, but that is non-trivial) + "last_check": str(self.clock.time_msec()), }, ) self.db_pool.simple_update_txn( From 1991aa50706e1bb0518166833f68f2bca2051386 Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Fri, 3 Jul 2026 15:15:42 +0000 Subject: [PATCH 5/7] Bind filter_id as an int in get_user_filter, matching the BIGINT column MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `get_user_filter` receives `filter_id` as `int | str` — from a sync request it arrives as a string — and bound it straight into the `user_filters.filter_id` BIGINT column. psycopg2 coerced the numeric string, but binding a string to an integer column is incorrect and a stricter driver that binds typed parameters rejects it (error serializing parameter), 500ing filtered syncs. The function already validated it with `int(filter_id)`; use that value so an int is bound. psycopg2 and sqlite are unaffected. Found during a Rust port. Co-Authored-By: Claude Opus 4.8 (1M context) Claude-Session: https://claude.ai/code/session_01W3G4M92AmwSSZCbmtMJU3d (cherry picked from commit 4f52404c4a38443c548c6e99f202deac69ec424d) --- synapse/storage/databases/main/filtering.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/synapse/storage/databases/main/filtering.py b/synapse/storage/databases/main/filtering.py index 2019ad9904e..c334457af31 100644 --- a/synapse/storage/databases/main/filtering.py +++ b/synapse/storage/databases/main/filtering.py @@ -156,7 +156,7 @@ async def get_user_filter( # filter_id is BIGINT UNSIGNED, so if it isn't a number, fail # with a coherent error message rather than 500 M_UNKNOWN. try: - int(filter_id) + filter_id = int(filter_id) except ValueError: raise SynapseError(400, "Invalid filter ID", Codes.INVALID_PARAM) From c1d46219921bb4f6956611fd398642a654cbd632 Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Fri, 3 Jul 2026 15:51:07 +0000 Subject: [PATCH 6/7] Store device_lists_remote_extremeties.stream_id as text at the caching site MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `device_lists_remote_extremeties.stream_id` is a TEXT column, but `_update_remote_device_list_cache_txn` (typed `stream_id: int`) bound an int into it, relying on the database driver's int→text coercion. A stricter driver that binds typed parameters rejects the mismatch (error serializing parameter) — and because the error was raised inside the remote-device-list resync and swallowed upstream, the caching call never completed, hanging `query_devices` (the request Deferred never fired). Store the stream id as a string, matching the declared column type — as the sibling `_update_remote_device_list_cache_entry_txn` (typed `stream_id: str`) already does. psycopg2 and sqlite are unaffected. Found during a Rust port. Fixes test_e2e_keys.test_query_all_devices_caches_result. Co-Authored-By: Claude Opus 4.8 (1M context) Claude-Session: https://claude.ai/code/session_01W3G4M92AmwSSZCbmtMJU3d (cherry picked from commit abb44668ad0d7ef918202a2df502955501aa213e) --- synapse/storage/databases/main/devices.py | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/synapse/storage/databases/main/devices.py b/synapse/storage/databases/main/devices.py index 8670d68f38c..b293b47fea6 100644 --- a/synapse/storage/databases/main/devices.py +++ b/synapse/storage/databases/main/devices.py @@ -2019,7 +2019,10 @@ def _update_remote_device_list_cache_txn( txn, table="device_lists_remote_extremeties", keyvalues={"user_id": user_id}, - values={"stream_id": stream_id}, + # `stream_id` is a TEXT column, so store it as a string (this method + # takes an int) rather than relying on the driver to coerce it. + # (Ideally we'd fix the schema, but that is non-trivial) + values={"stream_id": str(stream_id)}, ) async def add_device_change_to_streams( From d4ef221d88ed44b6210d298f7aafb9bf186d204f Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Mon, 6 Jul 2026 09:57:54 +0100 Subject: [PATCH 7/7] Newsfile --- changelog.d/19911.bugfix | 1 + 1 file changed, 1 insertion(+) create mode 100644 changelog.d/19911.bugfix diff --git a/changelog.d/19911.bugfix b/changelog.d/19911.bugfix new file mode 100644 index 00000000000..11331455aa3 --- /dev/null +++ b/changelog.d/19911.bugfix @@ -0,0 +1 @@ +Fix storage type mismatches where values were bound with a type that didn't match their database column.