Skip to content

Keep same-roster Google threads distinct; re-key only threads whose id is dead - #201

Open
MaxGhenis wants to merge 1 commit into
mainfrom
fix/google-roster-rebind-distinct-groups
Open

MaxGhenis wants to merge 1 commit into
mainfrom
fix/google-roster-rebind-distinct-groups

Conversation

@MaxGhenis

Copy link
Copy Markdown
Owner

Problem

The #176 roster re-key (internal/ingest/idspace.go) treated a roster match as proof that a wire id had been re-keyed. Two groups with identical membership (5001 "Family", 5002 "Book club") therefore shared one v2 row. When 5002 first appeared it took 5001's row, then 5001's next event took it back. The wire id and title flipped on every event, both groups' messages piled into one row, and remote_rebinds kept climbing. Two 1:1 threads with the same sole peer (6001 and 6002) merged the same way. The message path behaves identically: an unbound id from a known sender, or a sender contradicting a bound row, took that peer's direct thread. Reported in the 2026-10-08 probe (reader-worker-semantics.md §5). The defect reproduces on origin/main: the six new tests in idspace_distinct_threads_test.go that only need main's API fail there, for the reported reasons.

This is common in real data. In a copy of the live v2 store, 53 Google roster sets have two or more live (non-displaced) group rows with identical rosters, and 16 peers have two or more live direct rows.

Rule

A roster match may take a new wire id only when its own id is dead (rekeyable):

  • it is displaced (a collision proved its id now names another thread); or
  • the phone announced it (a ConversationEvent reached it) only in an earlier device ID space; or
  • it is provisional (minted from a message frame, never announced) and its id was bound in an earlier ID space.

A row announced in the current ID space is a live twin. It is never taken; the new id gets its own row and rekeys_declined increments.

ID spaces. accounts.remote_idspace_epoch counts detected ID spaces. A new one starts when an id the phone announced in the current epoch arrives naming a different thread (noteIDSpaceContradiction). A contradiction against a row announced only earlier is the same reset still being discovered. A contradiction against a provisional row proves nothing.

Late detection. A reset shows up at the first collision, and a re-paired phone can deliver fresh ids before that. On 2026-09-03, 14 threads arrived in the 36 seconds before the first collision. Those rows are revisited when the reset is detected. Each row minted that way in the previous 15 minutes (frame time) is merged into the thread it declined (MergeConversationInto, one transaction). Messages move across, re-served duplicates are dropped, and the wire id moves with them. This is the rekeys_recovered counter.

Groups also need the event title to match, so a new group with an existing group's members never inherits that group's history.

Provisional threads. A thread minted from a message in the current ID space that hears from a second sender is a group whose snapshot hasn't arrived. It keeps the message and becomes a group (adoptSecondSender) instead of being scattered across its members' 1:1 threads. The live inbox has such a thread, wire 3097: 21 messages and never a ConversationEvent.

Message ids are device-local too. When a thread continued across a reset receives a new message under an id one of its old messages holds, the old message keeps its row under displaced:<id>:<message_id> instead of being overwritten (retireReusedRemoteMessageID). This applies only when the stored row has a different primary key and either the opposite direction or a timestamp at least an hour away. On the replayed inbox, real reuse was 454 to 553 hours apart; one message seen twice was at most 16 s apart.

Updated-at fix. A ConversationEvent arriving after a row minted from a message whose phone timestamp leads this clock no longer fails the updated_at_ms >= created_at_ms check and gets quarantined. refreshConversation no longer moves updated_at_ms backwards.

Schema: migration 0011_remote_idspace_epochs adds three columns.

  • accounts.remote_idspace_epoch
  • conversations.remote_announced_epoch: defaults to 0; NULL means provisional, and only ingest's message mint writes NULL.
  • conversations.remote_bound_epoch

Every existing row, and every row written by legacy migration, mirroring or repair, starts announced and bound in epoch 0. Ingest maintains the columns through dedicated store methods, and generic upserts never touch them.

Real-data differential replay

All 8,934 Google frames in the live v2 inbox (2026-07-19 to 2026-10-08, which includes the 9/3 phone swap) were fed in inbox order through origin/main and through this branch. Each run started from a fresh store, using the real GoogleDecoder and worker. The harness is not committed; it and its outputs are in ~/reviews/openmessage-roster-rebind-2026-10-08/replay/.

main branch
wire ids whose binding moved more than once 4 0
binding moves back to a row the id held before 11 0
most rows one wire id was bound to 16 2
quarantined frames 65 6
remote_rebinds 453 168
logical messages lost (not stored, no same-content copy) 1 0
re-served duplicates dropped 68 69 (51 at ingest + 18 in merges)
idspace_resets n/a 1 (the 9/3 swap)
rekeys_declined (live twins kept apart) n/a 83
rekeys_recovered (pre-detection splits merged back) n/a 14 of 14
remote_message_ids_retired n/a 7

Breakdown:

  • Quarantines. 59 of main's 65 are the updated_at CHECK failure, which drops real ConversationEvents. The other 6 are unchanged on both sides (see Follow-ups).
  • Twins. Main's flip-flops were wires 3097, 2578, 1571 and 1570. The branch has 93 more conversations than main (653 vs 560), mostly the 83 twins kept apart.

Invariants, with property tests

idspace_properties_test.go runs a real worker and store against random phones (default 60 cases each; run with OPENMESSAGE_PROPERTY_CASES=1500 locally, all passing). The generators reuse rosters on purpose so that several live threads share one. Direct peers and group members are drawn from disjoint pools, and any two group rosters are either identical or disjoint. The overlapping-but-unequal case is treated as one evolving group by design (rostersConsistent) and is outside these properties.

With no reset:

  • N1. Once a thread is announced, its id stays bound to the same row for every later frame.
  • N2. Announced threads never share a row.
  • N3. Each announced thread's row has that thread's title.
  • N4. Messages to an announced thread land in its row.
  • N5. If every thread is announced before its first message, nothing is rebound or displaced.
  • N6. No message is lost.

With one reset followed by repeated listings from the new phone:

  • R1. Every new-phone thread has its own correctly titled row, and its messages land there.
  • R2. A further listing changes no binding.
  • R3. If the first new-phone frame collides with a different old thread, every thread with a unique roster continues its old row.
  • R4. No message is lost.

Tests

Worker tests (idspace_distinct_threads_test.go, real store):

  • equal-roster groups stay distinct
  • same-sole-peer 1:1s stay distinct
  • an unbound message doesn't take the peer's announced thread
  • a group's messages arriving ahead of its conversation event leave the 1:1s intact and all land in the group
  • after a reset, stale threads re-key but new-phone twins don't
  • the ID-space state survives a worker restart
  • a ConversationEvent after a future-dated message is applied, not quarantined
  • the 9/3 replica: late detection merges the interim rows
  • the 3097 replica: a group with no conversation event stays one thread
  • after a reset, same-roster groups re-key by title
  • old-phone provisional threads re-key
  • a reused message id keeps both messages

Store tests (rebind_test.go):

  • candidate lists
  • epoch advance with compare-and-swap semantics
  • binding transitions
  • MergeConversationInto (move, duplicate, collision and drop cases)
  • the 0011 upgrade from a populated v10 store

Existing suites. idspace_collision_test.go and repair_idspace_test.go pass unmodified. Migration-count pins are bumped from 10 to 11, as #166 does. The full go test ./... and go test -race on ingest, sqlite and migration pass locally.

Design review

Before implementation, GPT-6.1 Sol (subfleet run --task review --tier hard) adversarially reviewed the first version of this rule and returned UNSOUND with 14 counterexample families (~/reviews/openmessage-roster-rebind-2026-10-08/design-review-full.md). Several of them showed up in the replay, and this version answers them:

  • legacy-migrated and other writers' rows defaulting to unannounced, which blocked resets
  • provisional rows blocking re-keys after a reset
  • provisional groups fragmenting, which is the real 3097 pattern
  • recency-ordered swaps of same-roster groups, now gated by title
  • the late-detection splits, measured at 14 on the real data, now recovered

Known ambiguities (residual, by design)

  • Message-only bindings after a reset. After a detected reset, an unbound id from a peer whose thread was re-keyed only by messages, never announced, can move that thread again until the phone announces it. TestGoogleIDSpaceResetRoutesIncomingBySenderNotByCollidingID requires this: its 4001 step cannot be told apart from a second live 1:1 using frame contents alone.
  • Twins inside the recovery window. A genuinely new twin thread that first appears within the 15 minutes before a reset is detected is merged into its twin.
  • Same wire id and same message id reused across phones. These give an identical derived primary key, so the overwrite can't be detected. On the replay, every reuse had a different wire id.
  • Repair doesn't advance the epoch. Rows openmessage repair google-idspace rebinds keep their stored epochs. The repair tool exists for resets processed by pre-fix code; with this change the live worker advances the epoch itself.

Interactions

  • Truthful send states, hard channel contract, and outbox safeguards #166 also adds a migration 0011 (0011_outbox_expiry). Whichever of the two lands second must renumber, and update migration0011Checksum and the count pins.
  • fix/google-history-to-v2 (no PR yet). That branch applies fetched ConversationEvents only for unbound wire ids, and those go through the same unbound roster path changed here. Before this PR, a fetched snapshot of group B could take live group A's row; with it, B gets its own row. History frames for bound ids are skipped, so they never mark a row announced. Separately, that branch's historyMessageExists treats any stored message with the same remote id in the bound thread as already present. After this PR, a fetched new message whose id an old message holds across a reset would be skipped. It should apply differentMessage before skipping.

Follow-ups (not in this PR)

  • 3 quarantines on both sides: a derived message_id already exists in another conversation, and the primary key collides (reported in the probe, §2).
  • 3 quarantines on both sides: a conversation frame with an empty participant number fails IdentityKey, so the whole snapshot is quarantined.

🤖 Generated with Claude Code

…d is dead

Two groups with the same members, or two 1:1 threads with one person, are
both live on the phone under their own wire ids. The #176 roster re-key moved
one's id onto the other's row, so the id and title flipped on every event and
both threads' messages piled into one row. A thread now takes a new wire id
only when its own id is dead: displaced by a collision, or announced by a
ConversationEvent only in an earlier device ID space.

- Migration 0011 adds accounts.remote_idspace_epoch and, per conversation,
  remote_announced_epoch (NULL = provisional, minted from a message) and
  remote_bound_epoch. Existing and externally written rows default to epoch 0.
- A new epoch starts when an id the phone announced in the current epoch
  arrives naming another thread. Declines made in the 15 minutes before that
  detection are merged into the threads they continue.
- Group re-keys also require the title to match.
- A provisional thread that hears from a second sender becomes a group instead
  of being scattered across its members' 1:1 threads.
- Google reuses message ids across a reset; a continued thread's older message
  keeps its row under a displaced remote id instead of being overwritten.
- A ConversationEvent after a message whose phone timestamp leads this clock
  no longer fails updated_at_ms >= created_at_ms and gets quarantined.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@MaxGhenis

MaxGhenis commented Oct 9, 2026 •

Copy link
Copy Markdown
Owner Author

Migration-numbering heads-up from #210, which adds 0011_inbox_codec_received (an index on inbox(codec, received_at_ms)). Four open PRs claim 0011: this one, #196, #166 and #210, so whichever merges later renumbers to the next free number. That means:

  • rename the SQL file;
  • update the //go:embed and newMigration lines in migrations.go;
  • move the internal/migration/validate.go pin to the new last migration;
  • bump the 10 → 11 count assertions one further.

No live store has applied any 0011 yet: Max's v2 ledger is at version 10.

🤖 Generated with Claude Code

@MaxGhenis

Copy link
Copy Markdown
Owner Author

Coordination with #211: it edits refreshConversation right before the ReplaceConversationParticipants call, next to this PR's markGoogleConversationAnnounced hunk, so expect a textual conflict there.

The live-inbox replay in #211 also attributes 59 of 66 replay quarantines on main to the updated_at_ms >= created_at_ms CHECK that this PR fixes. Evidence is in ~/reviews/openmessage-live-roster-2026-10-09/results/.

@MaxGhenis

Copy link
Copy Markdown
Owner Author

Migration numbering: #215 (SQL plan audit) also adds a migration numbered 0011 (read_path_indexes: three indexes, no row changes), as do #196, #201, #166 and #210. Whichever merges first keeps 0011 and the rest renumber to the next free version; the version-count lines every migration PR edits (internal/storage/sqlite/*_test.go, internal/migration/validate.go, internal/migration/transform_test.go) move with it. #215 finds its own migration by name in its tests, so renumbering it is mechanical.

Code overlap in internal/storage/sqlite/rebind.go: this PR splits FindDirectConversationBySolePeer and FindGroupConversationByPeerSet into List* variants with the same SQL, and #215 rewrites that SQL so it starts from conversation_participants (without statistics the current form iterates every conversation of the account: 71 ms and 22 ms at 30x scale against 0.2 and 0.3 ms). Whichever lands second carries the other's change over: directConversationBySolePeerQuery without its LIMIT 1, and groupConversationsWithMemberQuery as the candidate list (exact when all matches are returned, since a match must contain every wanted identity).

@MaxGhenis

Copy link
Copy Markdown
Owner Author

Migration numbering: #223 (FTS5 trigram substring search, stacked on #215) adds migration 0012 substring_search after #215's 0011. Same rule as above: whichever lands first keeps its number and the rest renumber. #223's tests find its migration by name; its version-count lines and internal/migration/validate.go's version/checksum gate (now the validationPassed function) move with it.

@MaxGhenis

Copy link
Copy Markdown
Owner Author

Migration numbering update: #215 merged to main (29a9a8e) and took 0011 (read_path_indexes: three indexes, no row changes), so this PR's migration needs the next free number. Renumber it to 0012, or later if another lands first. That touches:

  • the SQL file name, plus the //go:embed and newMigration lines in internal/storage/sqlite/migrations.go;
  • the count lines (want 11 → want 12) in internal/storage/sqlite/*_test.go and internal/migration/transform_test.go;
  • the internal/migration/validate.go pin, which must name this PR's own migration and checksum as the last one.

Two more things changed on main:

  • #218's rebuild fixture in migrations_fk_test.go now recreates every messages index, including 0011's two.
  • Read clients (read/status, the MCP client) and repair dry runs now open the store with sqlite.OpenWithoutMigrating. Any test that expected those clients to migrate a store must provision it at the current version first.

🤖 Generated with Claude Code

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant