Repository navigation
Conversation
…ce FK scans On a migrated v2 store every account already has a current local_installation device under a derived id. The legacy mirror (v2wire.ensureLocalDevice) tried to add "local-primary:<account>" next to it, which devices_current_local_uq rejects, so every MirrorConversation / MirrorReadCursor / SubmitText call through the mirror failed with "UNIQUE constraint failed: devices.account_id". Reproduced on a store built by migration.Transform and on a copy of the live store. - sqlite: EnsureLocalInstallationDevice inserts a local device only when the account has none (one INSERT ... WHERE NOT EXISTS) and returns the device GetLocalInstallationDevice resolves, the one ingest advances receipts on. - v2wire: the mirror uses it and writes read cursors on that device. - sqlite: UpsertDevice drops account_id from the DO UPDATE SET. Assigning it made SQLite scan the device's read_cursors children on every conflict (read_cursors references devices(account_id, device_id)). A WHERE keeps a device from moving accounts; that now fails with ErrCrossAccountDevice instead of silently succeeding when the device had no cursors. Fixing the device step un-masks two writes that ran around it: - MirrorConversation's UpsertAccount ran before the device step and rewrote existing accounts (bridge_key google_messages -> google, display name, enabled, config) even when the call then failed; v2 reads derive platform from bridge_key. It now uses EnsureAccount (insert if absent), with the bootstrap values the live adapters and the migration use. - Its UpsertConversation would, on a migrated store, rewrite the hash-keyed v2 row holding the same natural key (title, kind, archive, mute, favorite, recency, metadata) before erroring. It now uses UpsertOwnedConversation, which writes nothing and returns ErrConversationIdentityConflict (409 on /api/v1/outbox) in that case. UpsertConversation keeps its merge semantics for the migration. /api/mark-read no longer runs the legacy mirror on a v2-primary daemon, matching the /api/v1 send split: there the UI sends v2 ids the mirror cannot resolve, and a resolvable legacy id would add a legacy-keyed row to the primary store. Evidence: ~/reviews/openmessage-sqlite-plan-audit-2026-10-09/device-fix/ Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Owner
Author
|
Local verification at 96c10f7: |
Review of #217 (Subfleet 20261009-111730-om217-review, REQUEST_CHANGES): - Major: the mirror upserted under the raw legacy id, but the migration stores Signal threads under v2keys.NormalizeRemoteConversationID, so "signal: +1650..." missed the migrated row and minted a duplicate that v2read's alias lookup would then prefer. The natural key is now the migration's normalized form. A row an earlier mirror created under the legacy id keeps the remote id it was created with, so those stores do not hit their own primary key. - The ownership check now runs before the account and device bootstraps, so a refused call writes nothing. Before, an account with no local device gained one first. - Comments now say what the guard checks (ID equality, not provenance) and what the account-scoped device id used to protect against. Tests: migrated fixtures gain spaced Signal threads (migrated and post-cutover), plus an earlier-mirror-row case and a refusal-writes-nothing case. Each of the three changes is killed by its mutant. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Owner
Author
|
Local verification at 2665680: |
Round 2 review of #217 (Subfleet 20261010-073548-om217-review-r2, REQUEST_CHANGES at 2665680): - Major: the "earlier mirror row keeps its key" fallback returned whatever key the legacy-ID row holds now. Ingest rewrites a stale Google binding's key to "displaced:...", so the mirror adopted that key and dispatch sent to it. The fallback now accepts only the exact earlier-mirror shape (same account, key equal to the raw legacy ID), and a legacy-ID row with any other key or account is refused. It also no longer refreshes a raw-key row while another row holds the normalized key. - Major: the mirror routes by ID prefix, the migration by stored platform, so a "signal:" thread stored as sms was migrated under Google and the mirror created a second row under Signal that shadowed the migrated history for legacy-ID reads. mirrorNaturalKey now looks the legacy ID up the way internal/v2read resolves one: under every account, normalized for that account's platform. Any match under another conversation ID is a refusal. - Minor: a refusal is write-free only when the conflicting row exists before the call. If a concurrent writer creates it mid-call, the account and device bootstraps may already have run; the guarded upsert still leaves the other conversation unchanged. Comments say so. - Nit: a cross-account device upsert never orphaned read cursors. With cursors the foreign key failed the write; without them the device moved. Tests: a misrouted migrated thread in the migration-built fixture, a differential check that v2read returns the same history for a legacy ID before and after the mirror runs, a displaced-binding case through SubmitText, and a raw-key row beside a normalized owner. All four fail on 2665680; the two new branches are each killed by a mutant. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Round 3 review of #217 (Subfleet 20261010-091006-om217-review-r3, REQUEST_CHANGES at d6eb9c6): - Major (introduced by d6eb9c6): the all-accounts scan refused a legitimate send when an unrelated thread under another account shared the remote ID (opaque Google thread IDs can repeat across accounts). v2read resolves a direct ID match first, so that thread was never this conversation. mirrorNaturalKey now checks two places only: the routed account's normalized key, and where the migration files this legacy conversation. The second comes from the new migration.ConversationPlacement, which composes the migration's own accountForPlatform and remote-ID normalization, so the mirror does not carry a copy of that table. - Major (pre-existing, not worsened): a row another writer adds under a different account during the call is not detected. The comments now say what is guaranteed: a refusal writes nothing when the conflicting row exists before the call, and the guarded upsert covers only the routed account's key. - Nit: the mirror no longer has its own bridge-key-to-platform mapping, so the comment that wrongly called it identical to ingest's is gone. Tests: an unrelated thread under a second Google account, in both orders, through MirrorConversation, MirrorReadCursor and SubmitText (fails on d6eb9c6); a differential check that ConversationPlacement names the row the real migration wrote for every fixture thread. Dropping the placement check is killed by the misrouted-thread and v2read differential tests. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The round-3 reviewer's failing paths included reply-target reuse, so the unrelated-thread test now sends two replies to the same legacy message; the second reuses the mirrored reply target. The runbook said a refused mirror call writes nothing. That holds only when the conflicting row exists before the call, so it now states what is always true: the migrated row is left unchanged. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Round 4 review of #217 (Subfleet 20261010-095905-om217-review-r4, REQUEST_CHANGES at b55936d): - Major (made worse by this PR): the mirror recomputed the migration's placement from the conversation's current stored platform. The legacy Signal receiver can correct a "signal:" thread from sms to signal after migration, so the check looked under Signal, missed the migrated row under Google, and the mirror (no longer blocked by the device collision that stopped it on main) created a legacy-ID row that hid the history. migration.ConversationPlacements now lists every placement Transform could have given a legacy ID, one per platform it migrates, and the mirror checks them all. They sit under the migration's own accounts and are keyed by the legacy ID itself, so an unrelated thread under another account (round 3) is still not consulted. - Major (pre-existing): a cross-account owner inserted mid-call is still not detected; the comments already scope the guarantee to rows that exist before the call. Tests: a second "signal:" thread stored as sms in the migration-built fixture, and TestMirrorRefusesMigratedThreadAfterLegacyPlatformChanges, which flips the stored platform after migration and checks the mirror refuses and legacy-ID history is unchanged (fails on b55936d). The placement differential now also checks every migrated row is among ConversationPlacements and that it names only the migration's accounts. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
This branch has not been deployed
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
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.
Follow-up to #215 (SQL plan audit), items 1 and 2.
What was wrong
1. The legacy mirror could not run on a migrated store.
v2wire.ensureLocalDeviceupsertedlocal-primary:<account>as a currentlocal_installationdevice. The migration already gives every account a current local device under a derived id (9bcc1343…for Google on the live store), anddevices_current_local_uqallows one current local device per account. So every call through the mirror failed:This reproduces on
origin/mainwith a store built bymigration.Transform, and on a copy of the live v2 store (all three accounts).2.
UpsertDevicescanned read cursors on every conflict. ItsDO UPDATE SETassignedaccount_id = excluded.account_id.account_idis part of the parent key thatread_cursors(account_id, device_id)references, so SQLite compiled a scan of the device's cursors into every upsert, even when the value did not change. The bytecode confirmed it: the old statement opensread_cursors, its autoindex, andoutbox_read_receipts, withFkCounter/FkIfZerobookkeeping. The new statement opens none of them. On the live copy (797 Google cursors), a same-account conflict upsert in autocommit dropped from 892 µs to 138 µs.Two writes that ran around the device step. Fixing the device step would let them run further, so both are fixed here:
MirrorConversationcalledUpsertAccountbefore the device step, as its own autocommit. Even the failing calls above rewrote the account row:google_messages→google,whatsmeow→whatsapp,signal_cli→signal, plus display name, enabled, mode and config (observed on the migrated fixture and on the live copy).internal/v2readderives each conversation's platform frombridge_keyand passes unknown keys through, so Google threads would read asgoogleinstead ofsms. The live store's keys are still clean.UpsertConversationupserts on the natural key. On a migrated store, that would rewrite the hash-keyed v2 row with the same(account_id, remote_conversation_id)(title, kind, archive, mute, favorite, recency, metadata) and only then fail the "natural key belongs to v2 conversation" check. The failing device step was all that stopped this.Changes
sqlite.Store.EnsureLocalInstallationDevice(new)INSERT … SELECT … WHERE NOT EXISTS (local_installation for this account), then returnsGetLocalInstallationDevice. It never adds a local device next to an existing one, whatever that one's id.v2wire.ensureLocalDeviceMirrorReadCursorwrites the cursor on that device, the same one ingest'sadvanceReadCursoruses.sqlite.Store.UpsertDeviceaccount_idis out of the SET.WHERE devices.account_id = excluded.account_idplus a zero-rows-affected check now returnErrCrossAccountDevice(andErrConstraintViolation). Before, a device with no cursors moved accounts without any error.sqlite.Store.EnsureAccount(new)ON CONFLICT(account_id) DO NOTHING, returns the stored row. The mirror uses it, so existing accounts keep their metadata.v2wire.accountBootstrapgoogle_messages/whatsmeow/signal_cli). A differential test pins this against the migration's output.sqlite.Store.UpsertOwnedConversation(new)WHERE conversations.conversation_id = excluded.conversation_id. It writes nothing and returnsErrConversationIdentityConflictwhen another row owns the key.UpsertConversationis unchanged: the migration and ingest (worker.go,idspace.go,backfill_participants.go) rely on its merge.v2wire.mirrorNaturalKey(new)v2keys.NormalizeRemoteConversationID), sosignal: +1650…meets a migratedsignal:+1650…. Second, at any placement the migration could have given this legacy id. The migration routed by the platform stored at migration time and the mirror routes by id prefix, and the legacy store can change a thread's stored platform afterwards; so asignal:thread once stored as sms may sit under Google. Those placements are under the migration's own accounts and keyed by the legacy id itself, so an unrelated thread that shares an opaque Google thread id under another account is not consulted. The key written is the normalized form. The one exception is a row an earlier mirror wrote under the same account with the raw legacy id as its key, which keeps that key. A legacy-id row with any other key is refused, not adopted; ingest rewrites a stale Google binding's key todisplaced:….migration.ConversationPlacement,ConversationPlacements(new)ConversationPlacement(platform, id)is the account and normalized remote idTransformfiles a legacy conversation under, composed from the migration's ownaccountForPlatformand normalization.ConversationPlacements(id)lists that for every platform the migration handles, so a caller need not trust a stored platform that may have changed since. A differential test checks both against rows the real migration wrote./api/v1/outboxerrorsErrConversationIdentityConflict→ 409 with the message, which names the owning v2 conversation. It was previously a 500./api/mark-read!V2Primary, matching the/api/v1send split.docs/agent-runbook.mdCallers, by mode
ensureLocalDeviceis reached only throughmirrorConversation:OPENMESSAGES_V2_SEND=1on a migrated store (e.g. rollback)POST /api/v1/outbox/messages|media→SubmitText/SubmitMediaSubmitTextV2/MediaV2local-primary:*), but each call rewrote the bridge key togoogle. After: same, with no rewritesend_message,send_group_message,send_to_conversation,send_media_to_conversation→tools/v2.go submitText/submitMedia(in the daemon; transportless stdio clients post to its/api/v1/outboxviainternal/localapi)V2Primary→ native)mirrorSubmitReplyTarget→MirrorReplyTarget→MirrorConversationPOST /api/mark-read→mirrorV2ReadCursor→MirrorReadCursor(best effort, logged, always 200)On the live install the mirror never succeeded. Nothing consumes v2 read cursors for unread state yet (
v2readsetsUnreadCount: 0). Skipping the mirror on v2-primary therefore loses no working behavior; native v2 read-cursor writes are a follow-up.Invariants (property-tested)
GetLocalInstallationDevice(account), the device ingest uses.TestMirrorNeverChangesLegacyIDReads,TestMirrorRefusesMigratedThreadAfterLegacyPlatformChanges, both differential againstinternal/v2read). A row another writer adds under a different account during the call is not detected; that gap predates this PR.TestMirrorConversationIgnoresUnrelatedThreadInAnotherAccount).UpsertDevicenever changes a device's account. The compiled statement has no FK child scans; this is pinned from the bytecode, with child tables read frompragma_foreign_key_list.EnsureLocalInstallationDeviceis idempotent, and on a taken id it fails without writing.Tests:
TestEnsureLocalInstallationDeviceProperties(200 random device tables) andTestMirrorReadCursorDeviceProperties(40 random account/device shapes × up to 6 mirror calls). Differential:TestAccountBootstrapMatchesMigrationAccounts. Migrated-shape suites build the v2 store with the realmigration.Transform. Five mutants (account_id back in the SET, the device WHERE dropped, NOT EXISTS disabled, the conversation guard dropped, EnsureAccount turned into a clobbering upsert) are each killed. The two new/api/mark-readtests fail onorigin/main.Verification
go test ./...passes (34 packages), andgo vetis clean. A localgo test -race ./...run is noted in a comment.v2/store.sqlite3with its-wal/-shm(integrity_checkok,foreign_key_checkclean); the live files were never opened. Before/after output, probes, bytecode notes and mutation results:~/reviews/openmessage-sqlite-plan-audit-2026-10-09/device-fix/EVIDENCE.md.Review
Four rounds by GPT-6.1 Sol on Subfleet, each REQUEST_CHANGES; every finding is fixed. Reports:
~/reviews/openmessage-sqlite-plan-audit-2026-10-09/device-fix/review/.Round 1 (
20261009-111730-om217-review, at 96c10f7), fixed in 2665680:localDeviceIDcomment.Round 2 (
20261010-073548-om217-review-r2, at 2665680), fixed in d6eb9c6:Round 3 (
20261010-091006-om217-review-r3, at d6eb9c6), fixed in adfd636:Round 4 (
20261010-095905-om217-review-r4, at b55936d), fixed in 14916d1:signal:thread from sms to signal after migration, so the check looked under Signal and missed the migrated row under Google. On main the device collision happened to refuse this; with the device fixed, the mirror wrote a shadowing row (legacy-id history 1 → 0, executed). The mirror now checks every placement the migration could have used (migration.ConversationPlacements).The branch then merged main twice (through #243), with the touched packages (v2wire, sqlite, web, migration) re-run on the merged tree.
Each round's regression tests fail on the head they were raised against. Mutants for every fix are killed, including dropping the placement check.
Round 3 confirmed the round-2 fixes by replaying its own reproductions: the displaced binding is refused before and after another row takes the key, and
DispatchDuesends nothing; the spaced Signal thread stored as sms is refused with its history intact; the corrected device comment matches executed behavior on the base. It ran the full v2wire suite, the focused sqlite suite and a v2wire race run, all passing.Not changed
🤖 Generated with Claude Code