Merge id-space repair duplicates into the copy that stays

[MaxGhenis]

Builds on #218, now merged; rebased onto main.

The hazard

openmessage repair google-idspace --apply deleted each content duplicate with a plain DELETE FROM messages. Two failures, both reproduced by tests before the fix:

1. The plan failed on every run. If a reaction intent (outbox_reactions.target_message_id) or read-receipt intent (outbox_read_receipts.last_read_message_id) still named a planned-delete duplicate, the whole ApplyRepairPlan transaction failed with FOREIGN KEY constraint failed (787). Outbox rows are never deleted, so re-running failed the same way. This reproduces end to end through PlanGoogleIDSpaceRepair → ApplyGoogleIDSpaceRepair (TestGoogleIDSpaceRepairMergesDuplicatesIntoSurvivors).
2. Data on the duplicate was silently lost. Reactions, the reaction snapshot fence and attachments recorded on the duplicate were cascade-deleted instead of moving to the surviving copy.

The fix

• Delete steps name their survivor. Each delete step now carries SurvivorMessageID (survivor_message_id in the report). dedupeWithin uses the older copy. planMoves uses the planned or stored copy in the target.
• The apply merges before it deletes. ApplyRepairPlan merges the duplicate into its survivor, then deletes it. The merge is mergeDuplicateMessage, Merge echo duplicates into the local row, and run migrations with foreign keys off #218's mergeEchoDuplicate generalized to a survivor that may be in another conversation (the echo path calls it unchanged):
  • Inbound reactions: per reactor the newer row wins, and the survivor's on a tie. This is Merge echo duplicates into the local row, and run migrations with foreign keys off #218's rule, unchanged: with a snapshot fence on either row, newer means a larger (source_seq_ms, occurred_at_ms), and a reactor still active below the merged fence is tombstoned; without a fence, newer means a larger (occurred_at_ms, source_seq_ms). Moved rows take the survivor's conversation_id.
  • Inferred tombstones rank lowest. A tombstone the merge infers from a fence is stamped with the smallest time the schema allows (inferredTombstoneOccurredAtMS), not the merge time. Snapshot rows carry their write time, so it ranks below any row a snapshot wrote at that fence. If a copy merged later, in the same plan or a later repair, holds a reaction that a snapshot at that fence listed, the reaction wins. Stamped with the merge time, the tombstone outranked it and an active reaction was lost. Nothing else depends on the value: reads select active reactions only, and although ApplyReaction compares occurred_at_ms, the ingest worker sends Google reactions through ReplaceEmbeddedReactions only.
  • Effect on the echo path. Its tombstones take the same stamp. For a pair of rows the writers stored, its active reactions and fences are unchanged. Where an earlier repair left an inferred tombstone on the local row, the echo merge now also keeps a real reaction at that fence, which it used to drop.
  • Several copies into one survivor: the repair writes those tombstones once, after its last step, so within a plan every comparison is between rows the writers stored.
  • Fence and attachments: the fence keeps the larger source_seq_ms. For attachments, per ordinal, a downloaded survivor row stays; otherwise the duplicate's row replaces it.
  • Intents: reaction and read-receipt intents are repointed, and their outbox.conversation_id becomes the survivor's. The dispatcher refuses an intent whose target is outside its conversation (targetRefForLease, dispatch.go:714), so an intent left behind would retry forever as not_dispatched.
  • Read cursors follow only within one conversation, because the FK is composite: (conversation_id, last_read_message_id). Across conversations the step fails loudly; the planner never plans that case.
• The survivor must hold the same message. A delete step is refused unless the survivor has the same direction, sender, occurred_at_ms and body. A committed delete can't drop content, whoever wrote the plan.
• Survivor chains. If a step's survivor was already merged away by an earlier step, the step follows the chain to the copy that stayed. A step with no survivor, a survivor in another account, or a step that would make a message its own survivor fails the plan. Deleting a duplicate that is already gone is a no-op, so applying a plan twice works.
• Move steps carry intents into the new conversation, using the same dispatch reason. A message a read cursor names is refused with a clear error, where before it failed with a bare FK error.
• Unfinished sends pin their row. A send that is not yet confirmed, rejected or canceled reads its local message inside its own conversation at dispatch (messageForLease, dispatch.go:757) and at carry-over (ListCarryable). The planner leaves such a row where it is, and the executor refuses to delete it or move it out of the send's conversation. The base planner moved these rows.

Planner defects the property tests found (fixed here)

• Remote-ID collision (pre-existing; failed the plan on every run). Moving a row onto a remote ID the target already holds failed with UNIQUE constraint failed: messages.account_id, messages.conversation_id, messages.remote_message_id. IDs from the two device ID spaces collide by chance. Now a row with the same content is deleted into the holder; a row with other content is left in place and reported in the group's detail (TestGoogleIDSpaceRepairHandlesRemoteIDTakenInTarget).
• Mutual reroute (pre-existing; lost the message). Two misfiled threads routed into each other, each holding a copy of one message: each copy was a "duplicate of content already in" the other's target. On base the plan deletes both and the message is gone. I verified this on base: 2 deletes, 0 copies left. planDelete now refuses a survivor that resolves back to its own row, so one copy stays and carries everything (TestGoogleIDSpaceRepairKeepsOneCopyWhenThreadsSwap).
• Rows left in place. A row planMoves left in place that restated a row staying in its thread was only deleted on a second run. It is now deleted on the first, into a pre-window row or any copy already left in place.
• Cursor rows in dedupeWithin. dedupeWithin no longer skips rows a read cursor names. Its survivor is always in the same conversation, so the cursor moves to the older copy. planMoves still leaves cursor-named rows in place, since their survivor is in another conversation.
• Swallowed errors. planMoves used to ignore errors from MessageHasReadCursor and the duplicate lookup, which could plan a move or delete it shouldn't. It now returns them.

Invariants

Stated for every input, and checked by the property tests below:

1. Atomic and exact. ApplyRepairPlan either commits exactly the rows the reference model derives, or fails and leaves every row as it was.
2. No dangling references. After a committed plan, nothing references a deleted message, PRAGMA foreign_key_check is clean, and every intent's target is in the intent's own conversation.
3. Conservation. Intents are conserved in count, and so are read cursors that name a message. Each ends on root(its old target), where root(m) is the survivor a delete step names for m, or m itself.
4. No content lost. A delete commits only into a survivor with the same direction, sender, occurred_at_ms and body; the executor enforces this for any plan. The planner's survivors are never themselves deleted. Kept rows keep their content, and only delete-step rows disappear.
5. Reactions follow the copies' snapshots taken together, in any merge order, across one repair or several. With F the largest fence among a root and the copies merged into it, each reactor keeps the group's row with the largest (source_seq_ms, occurred_at_ms), and a reactor still active below F is tombstoned at F. So a reactor a snapshot at F lists stays active, unless another copy holds a tombstone for it written later at F. The largest fence stays. Each reaction is in its message's conversation. Each root keeps one attachment row per ordinal, downloaded if any copy was.
6. Open sends keep their row. An unfinished send's local message is neither deleted nor moved out of the send's conversation.
7. Dedupe is complete. After one apply, no conversation holds two copies of a window row's content, except a copy an unfinished send pins.
8. Idempotence. Re-planning finds no move, delete, rebind, mint or drop, unless the first plan left a row in place.

Intended exceptions.

• To 5:
  • When two copies' rows for a reactor tie exactly on both fields, the survivor's row stays, or the copy merged first.
  • Across two repairs, a removed reactor's row can be the first repair's inferred tombstone instead of the row that removed it. Its state is the same.
  • At equal fences, a copy whose snapshot omitted a reactor it never held has no row to say so, so a reactor another copy's snapshot lists stays active even if the omitting snapshot was written later. One message receiving both snapshots would end removed. This is Merge echo duplicates into the local row, and run migrations with foreign keys off #218's rule, kept as the conservative choice.
  • Copies without a fence carry delta reactions, which order by (occurred_at_ms, source_seq_ms). Invariant 5 is stated for snapshot-fenced copies, which is what Google writes; the store model covers mixed plans step by step.
• To 3: a cursor that names no message goes with its thread when the repair drops the emptied thread (the drop step's cascade). That predates this change.
• To 8: a row left in place (a cursor or an open send names it, or the target holds its remote ID) can leave its thread with senders that disagree, and the next plan may route that thread again. The old planner behaves the same way: the generator found this on a scenario with no deletes and no collisions, which plans identically on base.

Tests

• Reproductions. TestApplyRepairPlanDeleteMergesDuplicateIntoSurvivor covers a survivor in the same or another conversation, with only cascade children or with NO ACTION references. TestGoogleIDSpaceRepairMergesDuplicatesIntoSurvivors runs plan and apply end to end. Both failed before the fix: the FK error, and missing reactions.
• Edges. Chains in either order, invalid survivors (missing, other account, other content, own survivor), a cursor that can't follow, a move under a cursor, moves and deletes under open and closed sends, a move carrying intents, deleting an already-deleted duplicate, survivor-chain resolution in the planner, the thread swap, remote-ID collisions, copies pinned by two devices' cursors, and three copies whose snapshots disagree, merged in either order by one repair or two (store and planner level).
• TestApplyRepairPlanMatchesModelProperty (testing/quick, as in Merge echo duplicates into the local row, and run migrations with foreign keys off #218). Random messages across three conversations, with random content, reactions, fences, attachments, cursors, intents and open or closed sends, and random move/delete plans that include chains, invalid survivors and refusals. Each case is compared row-for-row with an independent Go-map interpreter (repairPlanModel), and the test requires every path to be covered. On this head it passed 12,000 cases across 3 extra seeds. Twelve injected bugs were each caught: reactions keeping the old conversation, the merge or the move skipping the intent retarget, ties going to the duplicate, no chain following, a delete or a move ignoring open sends, no content check, no tombstone below the fence, fenced rows ordered like deltas, a tombstone after every merge, and an inferred tombstone stamped with the merge time.
• TestApplyRepairPlanReactionMergeIsOrderIndependentProperty. One message stored as two to five copies, each as a Google snapshot leaves it, merged into a survivor in a random order, by one repair or split across two. The survivor's reactions are compared with invariant 5's expectation computed from the original rows, not from a replay. On this head it passed 12,000 cases across 3 extra seeds. It fails with a tombstone after every merge, and with the merge-time stamp.
• TestGoogleIDSpaceRepairInvariantsProperty. Random id-space damage: re-served duplicates, misfiled rows, and remote IDs that collide across the two device ID spaces, with every kind of child row, cursors from two devices (some naming no message) and open or closed sends. Reactions are generated as snapshots leave them, and the repair is applied an hour after they were written. It runs plan, apply and re-plan, and checks invariants 2–8, with invariant 5 computed from the original rows. It then frees every pinned row (cursors gone, sends confirmed), repairs again, and checks that each message's content is on its survivor and each reactor's state still follows the original snapshots of every copy either repair merged. On this head it passed 24,000 cases across 12 seeds. It is what found the cycle and the re-plan gap. Mutations caught: no cycle guard, no in-place dedupe, no remote-ID guard, delete steps without a survivor, and no survivor resolution.
• Race-job cost. Under -race the pure-Go SQLite engine runs about 10× slower, so the two properties run 40 and 120 cases there, gated by a build-tagged raceDetectorEnabled constant. The plain Go Test job runs all 200 and 400 cases.
• Seed sweeps. OPENMESSAGE_PROPERTY_CASES and OPENMESSAGE_PROPERTY_SEED override the cases and seed, for example OPENMESSAGE_PROPERTY_CASES=2000 OPENMESSAGE_PROPERTY_SEED=7 go test ./internal/ingest/ -run TestGoogleIDSpaceRepairInvariantsProperty.
• Local runs. internal/storage/sqlite, internal/ingest and internal/messaging pass in full, cmd's repair tests pass, and the repair tests pass under -race. Merge echo duplicates into the local row, and run migrations with foreign keys off #218's echo-merge tests, including its property tests, pass against the generalized merge.

Review

Round 1 (GPT-6.1 Sol, hard tier, at the first commit) asked for changes; all four findings are fixed in the second commit, each with a test that fails without its fix:

• Queued sends. A queued text send's optimistic row could be deleted as a duplicate. Fixed, and widened to moves: see "Unfinished sends pin their row".
• In-place dedupe. It tried only the oldest copy, so two copies pinned by two devices' cursors both stayed. Fixed (TestGoogleIDSpaceRepairMergesPinnedCopiesIntoOneAnother).
• Cursor claim too broad. It ignored cursors naming no message. Now stated as the exception to invariant 3, and generated by the property test.
• Content guarantee not enforced. The executor accepted any same-account survivor. It now requires equal content.

The third commit follows #218's 9945e1d, which changed the reaction rule after round 1: both reference models and the fixtures now implement the fenced rule.

Round 2 (same reviewer, at the third commit) confirmed the four fixes and the port, and asked for one more change, fixed in the fourth commit:

• Sequential merges could lose an active reaction. With copies at fences 100 (reactor active), 200 (nobody) and 200 (reactor active), the first merge wrote a tombstone stamped with the repair's time, and the later copy's real reaction lost to it. Both reference models had the same mistake, because they replayed the merge. Fixed by tombstoning once, last; tested at the store and planner level in both orders, and by the order-independence property.
• Nit: cursor seeding. A cursor naming no message could overwrite another generated cursor. It is now seeded directly.
• Generator gaps the review noted. The store model now varies every content field, and both generators cover six send states and both kinds.

Round 3 (at the fourth commit) confirmed the one-plan fix, the echo path's equivalence and everything earlier, and found the same loss across two repairs, fixed in the fifth commit:

• A tombstone from one repair outranked a real reaction in a later repair. A copy pinned by a cursor stays through the first repair, which tombstones the reactor on the survivor at the merge time. Once the cursor moves, a second repair merges the pinned copy, and its real reaction at the same fence lost. Fixed at the cause: an inferred tombstone is stamped with the earliest time, so it never outranks a stored row at its fence. Tested by the reviewer's scenario through the planner, by two-repair variants of the store test and the order-independence property, and by a second-repair stage in the end-to-end property.
• Nit: equal-fence wording. "Active in any equal-fence snapshot stays active" overstated the rule; a later-written tombstone at that fence wins. Reworded in the code, the tests and invariant 5.

Round 4 (at the fifth commit) returned APPROVE: the two-repair loss is fixed, reproduced before and after, and nothing regressed from rounds 1 to 3. It ran 400 randomized fenced groups (1,417 successive pair merges) against its own original-row oracle. Its three wording nits are fixed in the sixth commit, which changes comments only:

• the stamp ranks below any row a snapshot wrote, not below every schema-valid row (it would tie one written in the first millisecond of 1970);
• the delta rule's arrival-order equivalence needs frames in different milliseconds;
• a stale property-test comment.

A confirmation pass on that commit verified it changes comments only and found one new sentence imprecise ("only that writer creates fences"; the merge copies a duplicate's fence to its survivor). The seventh commit rewrites that sentence, again comments only.

Two optional findings from #218's review came with the merge code:

• Equal-time delta ties. Without a fence the merge keeps the larger (occurred_at_ms, source_seq_ms), while ApplyReaction lets a later write replace an equal-time row. I kept the rule and corrected the comment. source_seq_ms is the frame's receive time in milliseconds, so the rule is exact whenever frames are applied in arrival order and arrived in different milliseconds. The suggested tie-break by updated_at_ms is not exact either: with writes local, echo, local at one occurrence time, the third identical to the first, one message keeps the local write, but the local row's updated_at_ms still shows the first write (an identical rewrite leaves the row untouched), so that tie-break picks the echo row. I ran that case. Once frames are applied out of arrival order, the stored rows don't record which write one message would have kept.
• Equal-fence comment. The same wording nit as above.

docs/agent-runbook.md ("Repairing history after a reset") now says what a repair delete carries over, and when a row stays in place.

🤖 Generated with Claude Code

[MaxGhenis]

#218 is merged (squash 9a45d4a), and this PR is retargeted to main. It needs a rebase onto main to drop #218's two pre-squash commits from its diff.

Two optional findings from #218's round-2 review (APPROVE at 9945e1d) are carried here, because this PR moves the merge into message_merge.go:

1. Delta occurrence tie. Without a fence the merge keeps the larger (occurred_at_ms, source_seq_ms). ApplyReaction orders by occurred_at_ms and, on a tie, keeps the later write. Those differ when the later write has a lower or equal source_seq_ms. Reviewer repro: peer:first at T/source 200 on the local row, then peer:second at T/source 100 on the echo → merge keeps first, one message keeps second. The writer-level property (TestOutboxRepointEchoMergeConvergesWithOneMessageProperty) never generates this, because its delta source sequences increase with processing order. A candidate fix: break occurrence ties by updated_at_ms (the row's last write time) before source_seq_ms, and let the generator pick arbitrary source sequences. The merge comment's "the order ApplyReaction applies deltas in" should be reworded either way. No current ingest path splits non-Google deltas across both rows (resolveReactionTarget uses the wire remote ID; the optimistic row holds a random request ID).
2. Equal-fence comment. "If both rows have the same fence, reactors active in either snapshot stay active" is too broad: a later-written tombstone at that sequence wins through the per-reactor rule.

Full report: ~/reviews/openmessage-fk-hazards-pr218/review-9945e1d-full.txt (repros in review-9945e1d-artifacts/writer-order-probe.go and snapshot/equal-fence-doc-repro.go).

[MaxGhenis]

Merging at head 33a9cbe on the merge gates:

• CI: gh pr checks exits 0 on run 38084820203 (Go Test, Go Race, Web E2E, Site Build, macOS App Build all pass).
• Mergeability: MERGEABLE, merge state CLEAN, not a draft, no CHANGES_REQUESTED review.
• Independent review: GPT-6.1 Sol, hard tier, six rounds.
  • Rounds 1–3 requested changes. Each finding was fixed with a test that fails without its fix.
  • Round 4 approved the code at fa4436c.
  • Rounds 5–6 confirmed the two later commits change comments only and that the comments are accurate, then approved 33a9cbe.
  • Reports: ~/reviews/openmessage-idspace-repair-pr229/review-r{1..6}-*.md.
• Scope: a bug fix to the repair google-idspace tool and the shared duplicate merge. No public claims, money, or outside sends.

🤖 Generated with Claude Code