fix(xmtp_api_d14n): close two bidi ledger loss holes under overlapping catch-up waves

[tylerhawkes]

Follow-up to #3845 (merged before these landed on the branch). The deterministic proptest model built for #3850 found two real message-loss bugs in the transport ledger, both reachable only while two catch-up waves overlap on one topic — a milliseconds-wide window against a live node that the mock-wire model holds open. Both are silent losses: the frames are simply never delivered to the affected lease.

Bug 1 — virgin-topic snapshot hole

A lease registering a topic the transport had never delivered a frame for (last_seen empty) records no registration snapshot, and the owed-history lane read "no snapshot" as nothing predated this lease. If any overlapping wave's replay then advanced the wire position, the lease's own replay arrived non-fresh, hit the owed lane, and was skipped wholesale — everything below the overlap lost.

Realistic trigger: cold start. The catch-up pass and a stream subscription lease the same group back-to-back on a fresh process; the first wave's replay races ahead of the second's.

Minimal repro (proptest-shrunk, now a scripted regression — overlapping_waves_on_a_virgin_topic_lose_nothing): topic with history [1..4] never seen by the transport; lease A at floor 2, lease B at floor 0; A's wave replays (2,4] first; B's wave replays (0,4] — B never received 1,2.

Bug 2 — cross-wave watermark gap-jump

The owed lane is deliberately tag-agnostic (a lower re-add moves a topic into the re-adder's wave, so any wave may carry a lease's history). But the per-(lease, topic) replayed watermark max-folds across waves with different floors: a wave that asked from a higher floor — or one that had already replayed past the lease's floor before the lease registered — delivers its frames, the watermark jumps over the gap underneath, and the lease's own replay of that gap is then "covered" and skipped. Lost for good.

The fix

• Withhold-and-flush for fresh frames. A holder still owed history on a topic no longer takes that topic's frames from the fresh lane: they're withheld per (lease, topic) and flushed — cursor order, watermark-deduped — right before its CatchUpComplete. The one exception that keeps catch-up streaming: on an open owed range (no snapshot), frames of a wave the lease itself rides are its replay and deliver immediately, draining any withheld frames below them first.
• Missing snapshot = owed-open. The owed lane admits everything above the floor until catch-up closes the lane, and reconnect_plan re-asks from the floor (pushed past the watermark) instead of the wire position.
• Two owed-lane gap guards. A wave may serve a holder's owed history only if its replay is gap-free below the holder's progress: its asked floor must sit at-or-below max(holder floor, watermark), and the wave must not predate the holder's registration (preexisting set — what remains of an older wave's replay is unverifiable). Skipped frames re-arrive inside the holder's own wave, in order.

Net effect: per-lease per-topic delivery is now strictly increasing — order, exactly-once, and completeness in one invariant — instead of exactly-once-but-occasionally-reordered-or-lossy.

One file: the ledger demux/registration/reconnect in bidi_transport.rs, plus module docs, two new proptest-shrunk scripted regressions, and three existing scripted tests tightened to the stronger contract (they previously asserted the raced fresh delivery).

Testing

• xmtp_api_d14n scripted suite: 233/233 (incl. the two new regressions).
• Proptest ledger model (from test(xmtp_mls): randomized bidi delivery fuzz against the live node #3850): 2,000 random schedules + all shrunk regression seeds green (~11 min soak). Both bugs reproduce deterministically against main without this fix.
• Live bidi integration tests and the 10×10 racing fuzz (test(xmtp_mls): randomized bidi delivery fuzz against the live node #3850): green across multiple seeds against the containerized node.

🤖 Generated with Claude Code

https://claude.ai/code/session_017AVuJ7VAgen2fFhtJQ6zid

Note

Fix ledger loss holes in bidi transport under overlapping catch-up waves

• Withholds fresh frames for leases still owed replay history on a topic, buffering them by arrival order and kind, then flushes them in wire-arrival order before CatchUpComplete.
• Fixes reconnect_plan to resume a virgin-topic catch-up from the lease floor advanced by replayed progress, not from the wire position, preventing data loss on overlapping waves.
• Adds WITHHELD_FRAMES_CAP = 512 per lease; leases exceeding the cap are dropped for recovery rather than blocking sibling leases.
• Risk: leases that accumulate more than 512 withheld frames are now forcibly dropped, requiring reconnection.

^{Macroscope summarized 11acadd.}

[claude]

Claude finished @tylerhawkes's task —— View job

---

PR Review — 2026-07-14 06:23 UTC

Reviewing: fix(xmtp_api_d14n): close two bidi ledger loss holes under overlapping catch-up waves

• Read and analyze the implementation changes
• Evaluate correctness and edge cases
• Check for performance and security concerns
• Provide feedback

---

Summary

This is a well-documented, critical correctness fix addressing two subtle message-loss bugs in the bidi transport ledger. The implementation is sound and thoroughly tested. The author has already addressed previous review concerns about unbounded buffers and message reordering.

Key Strengths

1. Excellent documentation — Module-level docs and inline comments clearly explain the complex delivery guarantees and edge cases (lines 24-89, 737-763).

2. Comprehensive testing — New regression tests (overlapping_waves_on_a_virgin_topic_lose_nothing:4964, a_withheld_frame_overflow_drops_the_lease_for_recovery:5122) directly validate the bug fixes, plus extensive proptest model validation.

3. Memory safety — The WITHHELD_FRAMES_CAP = 512 bound with Arc-sharing (line 179) properly prevents unbounded memory growth while maintaining correct delivery order.

4. Proper recovery path — Overflow correctly triggers lease drop (lines 1548-1557) instead of blocking, allowing consumers to re-lease from durable cursors.

Minor Observations

Code Quality:

The demux method (lines 1430-1747) is complex but necessarily so — it implements the core exactly-once delivery guarantee. The three withheld-frame drain sites (lines 1569-1588, 1663-1678, 1691-1713) share similar logic but differ enough in context that extraction would add more complexity than it removes (as the author noted in their latest commit message).

Edge Case Handling:

The implementation correctly handles:

• Virgin topics with no snapshot (lines 802-817, 1158-1171)
• Gap guards for cross-wave delivery (lines 1631-1650)
• Per-lease sequence stamping for wire-order reconstruction (line 759-762)
• Floor guards preventing duplicate delivery below lease floors (lines 1521-1527, 1625-1628)

Performance:

Arc-cloning for withheld frames is efficient — one allocation per wire frame shared across all withholding leases, with Arc::unwrap_or_clone (line 1014) optimizing the common case. The 512-frame cap is reasonable for typical catch-up latencies.

Test Coverage:

82.44% patch coverage with 56 missing lines. The uncovered lines appear to be error paths (e.g., telemetry, defensive checks). Given the thorough scripted and proptest coverage, this is acceptable.

Verdict

✅ Approve — This fix correctly addresses real message-loss bugs with proper bounds, comprehensive testing, and clear documentation. No blocking issues found.

---

[macroscopeapp]

Approvability

Verdict: Needs human review

1 blocking correctness issue found. This PR introduces significant runtime behavior changes to the bidirectional transport ledger (new withheld-frame buffering, new state fields, new delivery semantics). An unresolved high-severity comment identifies a potential bug where the flush logic could deterministically drop leases during normal catch-up traffic due to bounded channel capacity.

^{You can customize Macroscope's approvability policy. Learn more.}

[codecov]

Codecov Report

❌ Patch coverage is 87.74510% with 50 lines in your changes missing coverage. Please review.
✅ Project coverage is 85.40%. Comparing base (41b9afb) to head (11acadd).

Files with missing lines	Patch %	Lines
crates/xmtp_api_d14n/src/queries/bidi_transport.rs	87.74%	50 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3852      +/-   ##
==========================================
- Coverage   85.48%   85.40%   -0.09%     
==========================================
  Files         413      413              
  Lines       65636    66012     +376     
==========================================
+ Hits        56112    56379     +267     
- Misses       9524     9633     +109     

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

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[tylerhawkes]

Addressed the review round in 977d84a:

• Withheld-buffer bound + sharing (Macroscope Medium, also raised in the automated review's memory note): buffers are now Arc-shared per frame across holders and capped at WITHHELD_FRAMES_CAP = 512 frames per lease; overflow takes the channel-wedge recovery (lease drops, consumer re-leases from durable cursors). Regression test added. This required MaybeSync on the binding's message types — real Sync on native where the actor is Send-spawned, no bound on wasm's spawn_local, matching the file's existing MaybeSend/MaybeSync split.
• riders construction: simplified to a single map/unwrap_or as suggested — no intermediate slice reborrow.
• Flush-dedup helper extraction: leaving as-is deliberately. The three sites share a watermark-skip shape but differ in what they know and mutate: the two in-admission drains are generic over the frame type with the kind's cursor_of in scope and split "deliver below / keep above" around a live cursor, while the completion flush is concretely typed per kind and drains everything. A shared helper would need to be generic over cursor extraction, watermark policy, and keep-vs-drain — more machinery than the duplication it removes, and the cap now keeps all three loops trivially small.
• BTreeSet for withheld frames: not needed now that buffers are bounded (≤512 entries, drained linearly once).

Suite 233/233 (including the new overflow test), proptest model + live fuzz green on the updated ledger. The browser-sdk shard failure on the previous run is the same flake family currently failing on main's CI (expected 1 to be 2 in a browser-sdk stream test) — unrelated to this native-only change; the new push re-runs everything.

[macroscopeapp]

🟠 High queries/bidi_transport.rs:1044

complete flushes withheld frames by emitting a separate channel event for each kind alternation (a WelcomeMessages event, then a GroupMessages event, then WelcomeMessages, etc.). Because deliver uses bounded try_send, a lease with 65 alternating group/welcome frames and a channel capacity of 64 is deterministically dropped mid-flush — the consumer cannot drain during this synchronous loop, so normal catch-up traffic triggers the wedge-recovery path and terminates the stream instead of completing. Consider coalescing all withheld frames of the same kind into a single event before sending, or batching the flush into one event per kind.

🚀 Reply "fix it for me" or copy this AI Prompt for your agent:

In file @crates/xmtp_api_d14n/src/queries/bidi_transport.rs around line 1044:

`complete` flushes withheld frames by emitting a separate channel event for each kind alternation (a `WelcomeMessages` event, then a `GroupMessages` event, then `WelcomeMessages`, etc.). Because `deliver` uses bounded `try_send`, a lease with 65 alternating group/welcome frames and a channel capacity of 64 is deterministically dropped mid-flush — the consumer cannot drain during this synchronous loop, so normal catch-up traffic triggers the wedge-recovery path and terminates the stream instead of completing. Consider coalescing all withheld frames of the same kind into a single event before sending, or batching the flush into one event per kind.