Durable outbox (ADR-006 §1) + pull-apply sibling replay (DAN-777) - #25
Conversation
… DAN-776's two deliberate deferrals (DAN-777) Finding A: EnableSyncOptions.outboxEngine takes a SEPARATE StorageEngine instance; entries and a per-client seq watermark persist at commit time and restore on boot. Pending pushes survive reloads relayed with identical mutationId/seq; the watermark is its own row (confirmed entries are deleted, and deriving seq from survivors would re-issue seqs the server ignores — the silent post-reload write-loss hazard). Pre-boot writes buffer with all commit-time capture except seq, which waits for the restored watermark. Absent engine: old behavior, now loudly documented on clientId and pinned. Finding B: a pull-applied remote change now replays still-pending same-key writers inside its own sync-pull channel, completing the reject/transform trio — pending optimistic edits no longer vanish until the push echo. Two tests that pinned the vanishing behavior mid-flight were updated to pin the replay; their final divergence-prevention assertions are unchanged. Related: ack/transform version stamps are now guarded and never regress a newer pull's stamp (D17 overlap). Six mechanisms watched to fail. Suite 630 -> 637. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
A buffered write is an unconfirmed local write; reporting 0 during the boot-load window hides exactly the writes at their most volatile. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ansform data gate chief among them B1: a transform whose version compares older than the pull-stamped one now skips its data apply and sibling replay (the guarded stamp alone made the divergence PERMANENT — re-delivery of the newer state classifies 'same'); the id remap still applies, since identity is not versioned. B2: stop() during a slow engine open() now closes the engine instead of leaking a handle that can hold an OPFS lock and degrade the next instance to in-memory. A1: a per-client confirmed high-water mark retires entries restored or adopted AFTER a delta-style mark already covered them (ghost re-push forever). A2: a loadAll fault degrades to pure in-memory instead of leaving the engine writable over the previous session's rows. A3/A4: sibling replay and id remaps now reach writes still buffered behind the boot window. A5: corrupt persisted rows are skipped, not folded into seqCounter as NaN. All seven watched red first. Suite 637 -> 643. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…se-ownership helper, stamp-follows-data G1: the stale-transform gate now checks BOTH keys — a remapped stale transform's temp id is usually unstamped while the newer state it must not overwrite is stamped under the target id (the exact B1 divergence, one key over). G2/G3: close-ownership routes through one helper with split open/writable flags — a loadAll fault degrades writes to in-memory while stop() still releases the handle, and a stop firing mid-loadAll no longer double-closes (StorageEngine forbids calls after close). G4: a 'concurrent' transform's stamp now follows its applied data, or the server's echo of that very version re-applies forever under a custom comparator. Also deleted A1's dead scaffolding block. All four watched red first. Suite 643 -> 647. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Warning Review limit reached
Next review available in: 49 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughThe coordinator adds optional durable outbox storage with restoration, buffering, sequence recovery, fault handling, and cleanup. It also replays pending mutations after pulls, protects newer state from stale verdicts, updates remapped entries, and expands conformance coverage. ChangesDurable outbox and lifecycle
Sync replay and verdict handling
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant LocalMutation
participant Coordinator
participant StorageEngine
participant SyncServer
Coordinator->>StorageEngine: open and load durable outbox
LocalMutation->>Coordinator: submit mutation during boot
Coordinator->>Coordinator: buffer mutation
StorageEngine-->>Coordinator: return rows and sequence watermark
Coordinator->>StorageEngine: persist buffered mutation
SyncServer->>Coordinator: apply pull change
Coordinator->>Coordinator: replay pending same-key mutations
SyncServer->>Coordinator: send verdict
Coordinator->>StorageEngine: retire confirmed or rejected entry
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (2)
src/coordinator-conformance.spec.ts (1)
1033-1036: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThe
writeBatchdouble resolves immediately, so it cannot expose write-ordering faults.
writeBatchapplies puts and deletes with no delay and no queueing. Every call resolves in the same microtask in call order. The durable-outbox tests therefore cannot reach the case where a fire-and-forget delete resolves before an earlier fire-and-forget put for the same key. See the related comment onsrc/coordinator.tslines 296-313.Add an optional
writeDelayMs(or a per-call jitter) so at least one test drives concurrent, out-of-order batch completion.♻️ Optional delay hook on the double
-function fakeOutboxEngine( - rows: Map<string, unknown> = new Map(), - opts: { openDelayMs?: number; failLoadAll?: boolean; loadAllDelayMs?: number } = {}, -) { +function fakeOutboxEngine( + rows: Map<string, unknown> = new Map(), + opts: { + openDelayMs?: number; + failLoadAll?: boolean; + loadAllDelayMs?: number; + /** Per-call latency, so concurrent fire-and-forget batches can complete + * out of submission order — the real-engine hazard. */ + writeDelayMs?: (callIndex: number) => number; + } = {}, +) { let closes = 0; + let writeCalls = 0;async writeBatch(puts, deletes) { + const delay = opts.writeDelayMs?.(writeCalls++); + if (delay) await new Promise((r) => setTimeout(r, delay)); for (const p of puts) rows.set(p.key, p.value); for (const d of deletes) rows.delete(d); },🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/coordinator-conformance.spec.ts` around lines 1033 - 1036, Update the writeBatch test double to support an optional writeDelayMs (or equivalent per-call jitter), delaying each batch before applying its puts and deletes and resolving. Configure at least one durable-outbox test to use the delay so concurrent fire-and-forget batches can complete out of order, while preserving immediate behavior when no delay is configured.Source: Path instructions
CHANGELOG.md (1)
40-73: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRewrite the review-process narrative from the reader's perspective.
Lines 40-42, 44-45, and 60-61 describe the internal review process, not the shipped behavior. Examples: "Six mechanisms watched to fail (mutation → named test red → revert)", "Suite 630 → 637", "A fresh-context review round then found 7 more, all fixed here with red-first tests (suite → 643)", and "The landing gauntlet then found the fixes' own siblings — 3 blockers + 1 advisory".
The path instruction for
CHANGELOG.mdrequires entries written from the reader's perspective. A consumer of this package cannot act on review-round counts, blocker counts, or suite totals. The suite totals also rot on the next test added.Keep the behavioral statements. Remove the round framing and the counts. For example, state that a stale
transformno longer applies its data and no longer replays siblings, and that the id remap still applies.The "Known limitation, by design" text at lines 69-73 is reader-facing and should stay.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@CHANGELOG.md` around lines 40 - 73, Rewrite the affected CHANGELOG narrative to describe only shipped behavioral changes from the reader’s perspective: remove review-process framing, test-suite totals, round labels, and blocker/advisory counts. Preserve the concrete behavior of stale-transform handling, lifecycle and persistence fixes, confirmation retirement, buffered writes, corrupt-row handling, and related fixes; retain the reader-facing “Known limitation, by design” section unchanged.Source: Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/adr/006-sync-adapter-interface.md`:
- Line 4: Update the DAN-777 achievement clause in the ADR implementation status
to explicitly identify the shipped opt-in durable outbox API as outboxEngine on
EnableSyncOptions, together with stable-identity reload semantics. Remove
wording that implies the requirement remains unsatisfied, while preserving the
distinction that enableSync() without outboxEngine retains in-memory behavior.
In `@src/coordinator-conformance.spec.ts`:
- Around line 1166-1222: Update both reload tests to create a separate outbox
engine for each session while preserving the shared rows map: use one engine for
the first boot and a newly constructed engine for the second boot in the tests
beginning “seq resumes…” and “a pending (unconfirmed)…”. Ensure the fake
StorageEngine double rejects calls after close by asserting !closed in each
method, so reuse of a closed engine fails loudly.
In `@src/coordinator.ts`:
- Around line 383-403: Ensure the loadAll error path in the coordinator
initialization flow does not materialize and push entries with seqCounter still
at zero: recover the watermark via loadMany using OUTBOX_META_KEY before falling
back, or mark the session unable to push and expose that state through
getRetryState(). Add a red-first test covering a seeded watermark and asserting
the first post-fault push sequence.
- Around line 296-313: The fire-and-forget outbox writes in persistEntry and
retireEntry can complete out of order. In src/coordinator.ts lines 296-313,
route both writeBatch calls through a single serialized tail promise that
preserves submission order. In src/coordinator-conformance.spec.ts lines
1033-1036, add a per-call delay hook to fakeOutboxEngine.writeBatch and a test
exercising an out-of-order put/delete pair for the same entry key, verifying the
retired row is not recreated.
- Around line 100-127: The committed API report is missing the exported
EnableSyncOptions.outboxEngine option. Update etc/colada-db.api.md to include
outboxEngine?: StorageEngine in EnableSyncOptions, then rerun the API and
package-surface checks to verify the report is synchronized.
- Around line 593-596: Update the ack handling around stampVersionIfNewer() and
applyRemoteChange() so an ack version classified as “same” or “concurrent” is
stamped consistently, preventing the server echo from being re-applied; add a
focused conformance test parallel to the concurrent transform G4 case, or revise
the ack policy if acknowledgements should not stamp versions.
---
Nitpick comments:
In `@CHANGELOG.md`:
- Around line 40-73: Rewrite the affected CHANGELOG narrative to describe only
shipped behavioral changes from the reader’s perspective: remove review-process
framing, test-suite totals, round labels, and blocker/advisory counts. Preserve
the concrete behavior of stale-transform handling, lifecycle and persistence
fixes, confirmation retirement, buffered writes, corrupt-row handling, and
related fixes; retain the reader-facing “Known limitation, by design” section
unchanged.
In `@src/coordinator-conformance.spec.ts`:
- Around line 1033-1036: Update the writeBatch test double to support an
optional writeDelayMs (or equivalent per-call jitter), delaying each batch
before applying its puts and deletes and resolving. Configure at least one
durable-outbox test to use the delay so concurrent fire-and-forget batches can
complete out of order, while preserving immediate behavior when no delay is
configured.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 25224a7f-8c34-4118-bc0e-60c4b844cce3
📒 Files selected for processing (4)
CHANGELOG.mddocs/adr/006-sync-adapter-interface.mdsrc/coordinator-conformance.spec.tssrc/coordinator.ts
…e writes, comparator policy pin R1: a loadAll fault now suspends pushes (RetryState.suspendedForOutboxFault) instead of materializing buffered writes at virgin seqs a previous session already burned — seq <= lastSeen is silently ignored server-side, so the fault path had resurrected finding A's silent write loss. R2: all engine writes ride one serialized tail; writeBatch guarantees per-batch atomicity, not inter-batch ordering, so a fast retire delete could lose to its own entry's slower put and resurrect a ghost row. R3: pinned the ack-doesn't-stamp policy under a same/concurrent comparator (one idempotent echo re-apply, then converged). Reload tests now use a fresh engine per session per the no-calls-after-close contract. R1/R2 watched red first. Suite 647 -> 650. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
@coderabbitai resolve |
✅ Action performedComments resolved. Approval is disabled; enable |
1 participant
What
The two deliberate deferrals from DAN-776's landing review, closed (DAN-777).
Finding A — the durable outbox (ADR-006 §1), Option 1, the real fix.
EnableSyncOptions.outboxEngine?: StorageEnginetakes a separate engine instance (§1's own-file/store requirement — engines are parameterizable bydbName, soidbEngine({ dbName: "cdb_outbox" })is a genuinely separate store). Entries and a per-clientseqwatermark persist at commit time and restore on boot: pending pushes survive reloads relayed with identicalmutationId/seq(never re-authored), andseqnever regresses — the watermark is its own row, never derived from surviving entries (confirmed rows are deleted; re-issued seqs are silently ignored server-side, which is the exact post-reload write-loss hazard). Writes racing the async boot buffer with everything captured at commit time (D4) exceptseq, which waits for the restored watermark. Persistence faults degrade to in-memory with a warning; absent engine = old behavior, loudly documented onclientIdand pinned by a test.Finding B — pull-apply sibling replay, completing the reject/transform trio: a pull-applied remote change now replays still-pending same-key writers inside its own
sync-pullchannel (no echo, no new outbox entries) — pending optimistic edits no longer visibly vanish until the push echo returns. Related:ack/transformversion stamps route through a guarded stamp that never regresses a newer pull's stamp (D17 overlap), and a stale transform's data is gated — checked against both the temp id's and the target id's stamps.Review history — three adversarial rounds, all findings fixed in-branch
confirmedHighWaternow retires late-restored/adopted entries), boot-window gaps in replay/remap, NaN-poisoning corrupt rows, hidden buffered writes.Gate evidence
CI=true pnpm -r test: 647 passed, 2 skipped (pre-existing sqlite pair) + 29 mcp. Suite 630 → 647.Watched to fail — 17 mechanisms
Initial six (pull-apply replay, both stamp guards, watermark restore, entry persistence, pre-boot buffer), tackle-round seven, gauntlet-round four — every one mutation → named test red → revert/fix. One of my own new tests was caught passing vacuously (a poke racing the in-flight boot pull was dropped) and re-timed until it discriminated.
Known limitation, stated: retirement is a fire-and-forget delete; a crash between a once-only delta-style mark and the delete can leave a ghost row on next boot. Wire-protocol-v1 mandates marks on every response, so the shipped stack is unaffected.
Linear: DAN-777
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Documentation