Skip already-processed transactions on restart

[robacourt]

Summary

On restart, the persistent replication slot can replay transactions the consumer has already applied and persisted. The consumer deduplicated these on its multi-fragment path, but the complete-transaction fast paths short-circuited before reaching that check — so a replayed transaction was re-written to the shape log (duplicating ops) and re-notified to dependent subquery materializers, which re-applied it and crashed with Key ... already exists.

This is bug 2 from the restart-aware oracle property-test triage (see #4648 / packages/sync-service/bugs.md).

Problem

After a StackSupervisor restart, the source consumer reconnects to the persistent slot and replays from its confirmed LSN, which can lag the shape's on-disk offset. Complete single-fragment transactions (the common case — one statement per txn) took the fast path and were re-processed:

• Normal shapes: duplicate / orphan operations appended to the shape log.
• Subquery shapes: the re-notified duplicate is re-applied by the dependency materializer, which is a stateful reducer and crashes on the duplicate insert ("Key ... already exists"). That crash then cascades into tearing down and removing the healthy dependent shapes and returning 409 must-refetch to clients.

Solution

Consolidate the offset dedup into a single handle_txn_fragment/2 clause that skips any fragment already at or below latest_offset (routing to the existing skip_txn_fragment), placed before the complete-transaction fast paths. The dedup now lives in one place and covers single- and multi-fragment paths uniformly, replacing the separate fragment_already_processed?/2 check. Normal operation is untouched: new transactions have offset > latest_offset, so the guard is false and they fall through to the existing clauses.

This relies on a transaction's fragments being entirely at-or-below or entirely above latest_offset (which only advances at commit boundaries) — the same assumption the previous per-fragment check already made.

Test Plan

• test/electric/shapes/consumer_test.exs — two regression tests (single-fragment and multi-fragment) replay an already-applied transaction straight to the consumer, bypassing the log collector's own dedup (as happens on restart with a fresh collector), and assert it is skipped: no append_to_log!, no :new_changes notification, log unchanged. Fail without the fix.
• test/electric/shapes/ + test/electric/replication/ — 937 tests, 100 doctests, 6 properties, 0 failures.

---

Generated with Claude Code

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 59.99%. Comparing base (ed04ee4) to head (fcb5ffe).
✅ All tests successful. No failed tests found.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #4668      +/-   ##
==========================================
- Coverage   60.15%   59.99%   -0.17%     
==========================================
  Files         410      395      -15     
  Lines       44348    43747     -601     
  Branches    12582    12580       -2     
==========================================
- Hits        26677    26244     -433     
+ Misses      17593    17425     -168     
  Partials       78       78              

Flag	Coverage Δ
electric-telemetry	?
elixir	?
packages/agents	72.64% <ø> (ø)
packages/agents-mcp	77.70% <ø> (ø)
packages/agents-mobile	80.67% <ø> (ø)
packages/agents-runtime	83.72% <ø> (-0.02%)	⬇️
packages/agents-server	75.40% <ø> (-0.05%)	⬇️
packages/agents-server-ui	8.32% <ø> (ø)
packages/electric-ax	51.06% <ø> (ø)
packages/experimental	87.73% <ø> (ø)
packages/react-hooks	86.48% <ø> (ø)
packages/start	82.83% <ø> (ø)
packages/typescript-client	91.83% <ø> (ø)
packages/y-electric	56.05% <ø> (ø)
typescript	59.99% <ø> (-0.02%)	⬇️
unit-tests	59.99% <ø> (-0.17%)	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[claude]

Claude Code Review

Summary

This PR fixes a real, well-diagnosed restart bug: the persistent replication slot replays already-applied transactions, and the complete-transaction fast paths short-circuited before the offset-dedup check — duplicating ops in the shape log and crashing dependent subquery materializers. Hoisting the dedup into a single handle_txn_fragment/2 clause ahead of the fast paths is the right fix. Iteration 3 adds a defensive guard that converts the one documented "impossible" edge (a transaction straddling latest_offset) from a cryptic BadMapError into a loud, self-explaining failure. The change remains tightly scoped and correct.

What's Working Well

• Correct root-cause fix (unchanged from iteration 2). The old fragment_already_processed?/2 check lived only inside process_txn_fragment/2, which complete single-fragment transactions never reach. Hoisting the guard into its own clause before the fast paths (consumer.ex:601) makes dedup cover single- and multi-fragment paths uniformly, and the is_log_offset_lte/2 defguard keeps it zero-overhead in the hot path.
• The new defensive clause is right, and correctly ordered. process_txn_fragment(%TransactionFragment{}, %State{pending_txn: nil}) (consumer.ex:678) sits before the general %State{pending_txn: txn} clause (:686). That ordering matters: the general clause binds txn to anything, including nil, so without the new clause first a straddling transaction would fall through and crash on nil.consider_flushed?. The new clause intercepts it and raises a message that names the offset and the exact invariant violated. Better failure mode than the implicit BadMapError, and it documents the reasoning inline.
• The invariant analysis holds. I traced every path into process_txn_fragment/2: the two has_begin?: true clauses (:631, :645) always run after pending_txn is set (the pending_txn: nil begin clause at :631 matches first and creates it), and the catch-all at :669 is only reached by non-begin, non-complete, not-already-applied fragments — which in normal operation always follow a BEGIN that created pending_txn. process_buffered_txn_fragments/1 (:892) also routes back through handle_txn_fragment/2, so buffered replays get the same offset-dedup. The only way to reach the nil clause is the straddle, which the commit-aligned restore + whole-transaction replay invariant rules out. Sound.
• Strong regression tests (single- and multi-fragment) reproduce the restart scenario faithfully and assert no storage write, no :new_changes notification, and an unchanged log. Changeset included.

Issues Found

Critical (Must Fix)

None.

Important (Should Fix)

None.

Suggestions (Nice to Have)

• The new raise path is untested (optional). The defensive clause guards a "should be impossible" state, so leaving it uncovered is defensible. But since it's now a documented, named failure mode, a small test that hand-delivers a non-begin fragment to a consumer with pending_txn: nil and asserts the raise would lock the behavior in and prevent a future refactor from silently reintroducing the BadMapError. The Codecov comment predates this commit, so its "all lines covered" note does not reflect this line.

Issue Conformance

No linked issue on the PR (minor process note — PRs should reference the issue they address), but the body ties this to bug 2 from the restart-aware oracle property-test triage (#4648 / bugs.md) with a clear problem statement, solution rationale, and test plan. Implementation matches the description with no scope creep. Consider a one-line note in the PR body mentioning the added defensive guard, since it post-dates the original description.

Previous Review Status

All iteration 1 and 2 feedback remains addressed. The one open discussion point from iteration 2 — the straddle invariant being a documented-but-unenforced impossibility — has now been hardened into an explicit loud failure (commit fcb5ffed7), which is a strict improvement over relying on documentation alone. No regressions; core dedup logic is unchanged and remains correct. Ready to merge.

---

Review iteration: 3 | 2026-07-01