Fix storage type mismatches where values didn't match their column types#19911
Open
erikjohnston wants to merge 7 commits into
Open
Fix storage type mismatches where values didn't match their column types#19911erikjohnston wants to merge 7 commits into
erikjohnston wants to merge 7 commits into
Conversation
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) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01W3G4M92AmwSSZCbmtMJU3d (cherry picked from commit 8a7b3a2cbd6f2b068a59dcdc899c900a94042223)
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) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01W3G4M92AmwSSZCbmtMJU3d
(cherry picked from commit 55baa50ece083e3423088a95cdd3c76b447436dd)
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) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01W3G4M92AmwSSZCbmtMJU3d (cherry picked from commit 89dbdf52465ae4fec05c1028eaf70058ac6eaa89)
erikjohnston
force-pushed
the
erikj/storage-type-correctness
branch
from
68a7215 to
7e83fed
Compare
`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) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01W3G4M92AmwSSZCbmtMJU3d (cherry picked from commit fb6f0d67dc01145df9c060efac811288da3b5e11)
`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) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01W3G4M92AmwSSZCbmtMJU3d (cherry picked from commit 4f52404c4a38443c548c6e99f202deac69ec424d)
…g site `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) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01W3G4M92AmwSSZCbmtMJU3d (cherry picked from commit abb44668ad0d7ef918202a2df502955501aa213e)
erikjohnston
force-pushed
the
erikj/storage-type-correctness
branch
from
ec24963 to
d4ef221
Compare
reivilibre
approved these changes
Jul 6, 2026
reivilibre
reviewed
Jul 6, 2026
| @@ -0,0 +1 @@ | |||
| Fix storage type mismatches where values were bound with a type that didn't match their database column. | |||
Contributor
There was a problem hiding this comment.
is this actually misc because it didn't cause any user-visible bugs?
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
A handful of places in the storage layer bound a value whose Python type didn't match the declared column type — an
intinto aTEXTcolumn, or astrinto aBIGINTcolumn — and relied on psycopg2's loose coercion to paper over the mismatch. These are latent correctness bugs: they only work because the driver silently converts, and a stricter driver that binds typed parameters rejects them outright.Found during a Rust port of the database pool where the driver does not coerce automatically.
Each fix binds the value with the type the column actually declares, rather than depending on driver-specific coercion. All changes are behaviour-preserving on psycopg2 and sqlite.
There is also a fix to the multi-writer id-gen tests where we forgot to commit. This is tangential, but was found during the same effort.
Changes
device_lists_remote_extremeties.stream_id (TEXT)—_update_remote_device_list_cache_txn(typedstream_id: int) bound an int; store it as a string, matching the column and the sibling_update_remote_device_list_cache_entry_txn(typedstream_id: str). The old mismatch, when rejected, was swallowed inside the device-list resync and hungquery_devices.user_filters.filter_id (BIGINT)—get_user_filterbound the rawint | str(a string, from sync requests). It already validates viaint(filter_id); bind that int so it matches the column.rejections.last_check (TEXT)— both writers storedclock.time_msec()(an int); store the timestamp as a string.user-directory temp position — the
populate_user_directorybackground update's staging column_temp_populate_user_directory_position.positionwasTEXTbut held an int (read back into aBIGINTcolumn). Declare itBIGINT. The temp table is created and dropped within the background update, so there's no migration.test_batched_state_group_storing— selected fromstate_group_edgeswith a stringifiedstate_group; bind the int directly (the column is an integer).Multi-writer id-generator tests — constructing a
MultiWriterIdGeneratorprunes stalestream_positionsrows, but the harness never committed, so the cleanup only survived because adbapi keeps one connection per thread with its transaction open. Commit after construction so it persists regardless of pool semantics (the delete is legitimate work that should be committed anyway).