D-MBX-A6-P3d: durable-witness reshape (crash-durable paired move) + temporal.rs layer-1 causal deinterlacing - #878
D-MBX-A6-P3d: durable-witness reshape (crash-durable paired move) + temporal.rs layer-1 causal deinterlacing#878AdaWorldAPI wants to merge 8 commits into
Conversation
…nbanstep) The POST-write half of the fire-and-forget flow (pre-write half = owner_adapter, #877). Runs on the independent persistence path — the thinker already cast and moved on. persist_then_step(owner, write, paired, bytes): durable write FIRST, then — and only on a successful write — apply the cast's PAIRED move via the owner's checked try_advance_phase. Ordering invariant proven by 5 lance-free falsifiable probes: - no successful write => no step (owner untouched); - successful write => the PAIRED move applied exactly once; - applies paired.to NOT next_phases().first() — from Evaluation the generic successor is Commit, but a paired Evaluation->Prune veto (free-won't) is applied instead (key falsifier); - a landed write with no paired move is a durable no-step; - an illegal paired edge is surfaced (PersistError::Illegal), owner untouched. The DurableWrite seam is the durable-append surface; its concrete impl (next slice, a lance-having crate) wires lance's OFFICIAL MemWAL (dataset::mem_wal::WalAppender::append, Copyright The Lance Authors) over Arrow RecordBatch — hand-rolling no WAL, inventing nothing. Keeping it a trait leaves the ordering core lance-free / protoc-free (probe-first). Co-Authored-By: Claude <noreply@anthropic.com>
…(operator ruling) A monolithic persist_then_step(&mut owner, ..., bytes).await would hold &mut owner AND owner-borrowed bytes across the WAL I/O await — immobilizing the owner during object-store latency and defeating fire-and-forget. Split into: - persist_cast(sink, PersistCast) -> DurableReceipt (async; NO owner in the signature — never borrowed across the await; the owner stays free while durability runs); - apply_durable_step(&mut owner, DurableReceipt) (sync; NO await; the exclusive owner borrow is held only here, outside the storage-latency window). Durability type corrected: a WAL append yields a DurableCoordinate (shard + writer_epoch + wal_entry_position), NOT a DatasetVersion (that arrives later via MemTable flush + manifest commit). DurableReceipt carries the coordinate + the paired move; a receipt is refused for a foreign owner (OwnerMismatch). 7 probes green: success->receipt, failure->no receipt (=> no step reachable), paired-move applied, paired!=generic (Evaluation->Prune veto not Commit), no-move no-step, illegal edge surfaced, foreign-owner refused. Concrete impl (next): lance 7 ShardWriter::put (enable_memtable + durable_write) over an Arrow RecordBatch with shared-buffer ownership. Co-Authored-By: Claude <noreply@anthropic.com>
…terlacing Closes the crash-durability gap the operator named on the persistence sink, and wires temporal.rs layer-1 — both mandated BEFORE any concrete LanceShardSink. ## The gap (persist_sink) At the prior split-phase shape, persist_cast appended only the payload; the paired move rode solely in the in-memory DurableReceipt. WAL append lands → process dies before apply_durable_step → the receipt evaporates → the paired move is lost → the KanbanStep never fires, though the durable SoA state moved. Silent: storage advanced, lifecycle did not. ## The reshape - DurableWitness (owner, cast_id, cycle, paired_move) is CO-LOCATED with the SoA payload in ONE persistence generation: DurableWrite::append(&witness, &payload). The witness is the crash-durable copy of the move, never in-memory-only. - DurableReceipt now merely REFERENCES that durable material (via its DurableCoordinate); it is not the only copy of the move. - DurableWrite::scan_witnesses replay seam reads the co-located witnesses back — a read of what durable storage already holds, NOT a separate ack ledger. - recover_and_apply(owner, witnesses): crash recovery reads the witnesses, runs temporal layer-1 to find the owner's pending tail, and replays in cast order; stale moves (already reflected in the recovered state) are skipped (idempotent). - apply_durable_step gains a from==phase guard (PersistError::StalePhase) so a stale/out-of-order apply on the synchronous path is surfaced loudly. Durability proof stays the DurableCoordinate (shard/epoch/wal position), never a LanceVersion — the write is durable before any base manifest version attaches. ## temporal.rs layer-1 — CAUSAL deinterlacing New LocalCausalRow trait (owner + cast_seq) + local_trajectories / local_trajectory_of split a globally-interleaved durable log into per-owner LOCAL causal chains, ordered by cast_seq. A@s0,C@s0,B@s0,A@s1 → owner A's chain [A@s0,A@s1]; the interleaved owners are REMOVED from A's timeline. This is the layer-1 counterpart to the existing layer-2 epistemic projection (classify / deinterlace); the two compose (layer-1 reconstructs the chain, layer-2 filters it by the reader's horizon). DurableWitness implements LocalCausalRow. ## Falsifiers (all green — the operator's five integration falsifiers) 1. layer-1 deinterlaces interleaved global log → owner-local chain (+ vacuity guard: other owners' rows dropped, not reordered) 2. one owner's batch replays in cast order (deliberately seq-descending log) 3. WAL succeeds + crash (receipt dropped) → recovery reconstructs+applies the move from the durable witness; idempotent re-run applies nothing 4. WAL fails → no witness lands → nothing to replay → no move, no step 5. WAL-visible-before-manifest → recovery identifies latest local state from cast order, no dataset version exists 343 planner lib tests pass; clippy clean; fmt clean. Builds NO concrete sink. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KCGhDYoQBXs3poaR7sFuqp
Board hygiene for the persist_sink / temporal changes (same logical unit): - EPIPHANIES: E-THE-PAIRED-MOVE-MUST-BE-DURABLE-CO-LOCATED-NOT-IN-MEMORY-ONLY-1 (the crash gap, the co-location rule, temporal's missing layer-1, 5 falsifiers) - STATUS_BOARD: D-MBX-A6-P3d row (persistence ordering core + reshape + layer-1) - LATEST_STATE: persist_sink + temporal layer-1 contract-inventory entries Concrete LanceShardSink remains deferred per operator ruling. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KCGhDYoQBXs3poaR7sFuqp
|
Warning Review limit reached
Next review available in: 50 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: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughAdded a public cycle-level persistence sink. It stably orders and coalesces cycle landings, commits each cycle with one WAL append, exposes sealed reads, and supports watermark-based owner recovery. Added causal per-owner trajectory helpers and updated contract records. ChangesCycle persistence and causal recovery
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
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 |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using high effort and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit c7424cd. Configure here.
| fn cast_seq(&self) -> u64 { | ||
| self.cast_id.0 | ||
| } | ||
| } |
There was a problem hiding this comment.
Witness replay ignores writer epoch
High Severity
Crash recovery orders an owner’s witnesses only by cast_id.0, but DurableWitness does not carry writer_epoch or WAL position even though DurableCoordinate does. BatchWriter assigns CastId from a counter that resets on restart, so scan_witnesses can return multiple rows with the same cast_seq from different writer lifetimes. Stable sort then depends on scan order, and recover_and_apply may apply the wrong moves or skip pending steps.
Additional Locations (2)
Reviewed by Cursor Bugbot for commit c7424cd. Configure here.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c7424cd986
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if mv.from != owner.phase() { | ||
| // Stale: already reflected in the recovered durable state. Skip | ||
| // (recovery is idempotent), do not surface — unlike the synchronous | ||
| // single-receipt path, a stale move here is expected, not an error. | ||
| continue; |
There was a problem hiding this comment.
When a recovered owner has completed the legal Evaluation → Plan → Planning loop, its phase is Planning again, so the first historical Planning → CognitiveWork witness matches and this function replays the entire old cycle. The owner returns to Planning, meaning every subsequent recovery repeats the cycle, increments its cycle counter, and emits duplicate steps despite the claimed idempotence. Select the pending tail using a durable cast/cycle watermark rather than treating every phase mismatch as proof that a witness was already applied.
Useful? React with 👍 / 👎.
| if mv.from != owner.phase() { | ||
| return Err(PersistError::StalePhase { | ||
| owner_phase: owner.phase(), | ||
| move_from: mv.from, | ||
| }); |
There was a problem hiding this comment.
When two stacked casts for one owner are persisted concurrently, the later append can complete first. Applying that receipt while the owner is still at the earlier phase returns StalePhase, but the receipt is consumed by value and the error does not return it; after the earlier receipt succeeds, the owner remains stuck until a crash-time full scan happens. Since DurableWrite explicitly supports concurrent drains, normal completion reordering must be buffered by cast_id or otherwise remain retryable instead of discarding the receipt.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (6)
crates/lance-graph-planner/src/temporal.rs (1)
397-424: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winDocument the tie-break for a duplicated
cast_seq.
sort_by_keyis a stable sort, so two rows of one owner that share acast_seqkeep their global durable-log order.recover_and_applyreplays in exactly this order, which means a duplicatedcast_seqmakes replay order depend on log arrival.The
cast_seqdoc calls the value "monotonic", but nothing in the trait or inlocal_trajectoriesenforces uniqueness, andDurableWitness::cast_seqjust returnsCastId.0. A retried append or an overlapping writer epoch would produce a duplicate.State the precondition and the tie-break on the trait method, so a future implementor knows that uniqueness per owner is required and that ties fall back to log order.
📝 Proposed doc change
/// The owner-local monotonic cast sequence (e.g. `CastId.0`) — the ordering /// key WITHIN one owner's trajectory. Cross-owner values are never compared; /// only rows sharing an `owner()` are ordered against each other. + /// + /// PRECONDITION: unique per owner. The layer-1 sort is stable, so rows of one + /// owner sharing a `cast_seq` keep their global durable-log order — a + /// tie-break that makes replay order depend on log arrival. Implementors must + /// not reuse a `cast_seq` within one owner. fn cast_seq(&self) -> u64;🤖 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 `@crates/lance-graph-planner/src/temporal.rs` around lines 397 - 424, Update the documentation for LocalCausalRow::cast_seq to state that cast_seq must be unique and monotonic within each owner; if duplicate values occur, local_trajectories preserves their original global durable-log order as the tie-break because the stable sort is used.crates/lance-graph-planner/src/persist_sink.rs (5)
406-419: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoffThe test double cannot exercise the documented concurrency claim.
FakeSinkusesCellandRefCell, so it is!Sync. TheDurableWritedoc at lines 136-137 states that&selfis shared because "many casts drain concurrently". No test drains two casts concurrently, and this double cannot be shared across tasks to do so.If you want the concurrency claim covered before
LanceShardSinklands, switchlandedtoMutex<Vec<_>>andcallstoAtomicU32, then add one#[tokio::test(flavor = "multi_thread")]that joins twopersist_castfutures and asserts both witnesses landed. Otherwise the claim rests on the concrete sink alone.🤖 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 `@crates/lance-graph-planner/src/persist_sink.rs` around lines 406 - 419, The FakeSink test double cannot be shared across concurrent persist_cast calls because it uses Cell and RefCell, leaving the documented DurableWrite concurrency behavior untested. Update FakeSink to use AtomicU32 for calls and Mutex<Vec<DurableWitness>> for landed, then add a multi-threaded tokio test that joins two persist_cast futures and verifies both witnesses were recorded.
694-723: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd recovery coverage for a witness that carries no paired move.
recover_and_applyline 329 skips a witness whosepaired_moveisNone. No test reaches that branch. Every helper in the recovery tests (cast_of) always setsSome(...).Extend this test with a durable no-step cast interleaved into owner 42's chain. The assertion should show that the no-step witness is skipped and the following move still applies in cast order. This pins the interaction between durable no-steps and cast-ordered replay.
🤖 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 `@crates/lance-graph-planner/src/persist_sink.rs` around lines 694 - 723, Extend recovery_replays_one_owners_batch_in_cast_order_ignoring_interleaved_owners with a durable witness for owner 42 whose paired_move is None, interleaved between its two cast_of moves. Update the expected applied destinations and final phase assertions to confirm the no-step witness is skipped while the subsequent move still replays in cast order.
83-85: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdopt the
snafuerror pattern for the public error types.
WriteFailedandPersistErrorare plain types. They do not implementDisplayorstd::error::Error. Callers outside the crate cannot propagate them into a boxed error or print a message. The crate guideline asks forsnafuerror patterns.As per coding guidelines: "reuse
snafuerror patterns".♻️ Sketch of the snafu shape
-#[derive(Debug, Clone, PartialEq, Eq)] -pub struct WriteFailed(pub String); +#[derive(Debug, Clone, PartialEq, Eq, snafu::Snafu)] +#[snafu(display("durable write did not land: {reason}"))] +pub struct WriteFailed { + pub reason: String, +}Apply the same treatment to
PersistErrorwith one#[snafu(display(...))]per variant.Also applies to: 209-233
🤖 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 `@crates/lance-graph-planner/src/persist_sink.rs` around lines 83 - 85, Adopt the established snafu error pattern for the public `WriteFailed` and `PersistError` types in the persist sink module. Add the required snafu derives and one `#[snafu(display(...))]` message per error variant, preserving each type’s existing payloads and semantics so both implement `Display` and `std::error::Error` for external callers.Source: Coding guidelines
336-341: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winReport the partially applied moves when recovery aborts.
If
try_advance_phasefails mid-chain,recover_and_applyreturnsErr(PersistError::Illegal(_))and dropsapplied. The owner is already mutated by every earlier move in the chain. The caller therefore cannot tell how far recovery progressed, and a retry restarts from an unknown position.Either carry the applied prefix in the error variant, or document on the function that the owner is left mid-chain on
Illegal.🤖 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 `@crates/lance-graph-planner/src/persist_sink.rs` around lines 336 - 341, The recover_and_apply flow must preserve information about moves already applied when owner.try_advance_phase fails. Update PersistError::Illegal or the recover_and_apply contract so callers receive the applied prefix alongside the failure, or explicitly document that Illegal leaves the owner mid-chain and exposes the partial progress; ensure earlier applied moves are not silently discarded.
155-160: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winConsider bounding the replay scan before the concrete sink lands.
scan_witnessesreturns every witness the durable log ever held. Two costs follow:
- Memory grows with the full log length, not with the pending tail.
- Recovering
kowners callsrecover_and_applyktimes, and each call scans and clones the whole slice inlocal_trajectory_of. That is O(k × total_log).The trait is the right place to fix this, because the seam shape is fixed once
LanceShardSinkimplements it. Two options:
- Add a
from: Option<DurableCoordinate>argument so recovery reads only the tail after the last applied coordinate.- For multi-owner recovery, call
temporal::local_trajectoriesonce and apply each owner's chain, instead of callinglocal_trajectory_ofper owner.The module states the concrete sink is deferred, so this is cheap to change now.
🤖 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 `@crates/lance-graph-planner/src/persist_sink.rs` around lines 155 - 160, Update the scan_witnesses trait seam to support bounded replay by accepting an optional DurableCoordinate lower bound and returning only witnesses after that coordinate, so recovery does not load the entire durable log. Propagate this change through the recovery flow, and ensure multi-owner recovery derives local trajectories once via temporal::local_trajectories rather than repeatedly scanning and cloning the full witness slice through local_trajectory_of.
🤖 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 @.claude/board/STATUS_BOARD.md:
- Line 966: Preserve the append-only history in STATUS_BOARD.md by moving the
new D-MBX-A6-P3d governance row to the beginning of the board entries rather
than inserting it between existing historical rows. Keep all existing rows in
their current order; only use an in-place insertion if repository guidance
explicitly documents this file as a snapshot table.
In `@crates/lance-graph-planner/src/persist_sink.rs`:
- Around line 246-251: Validate that cast.paired_move.mailbox matches cast.owner
before constructing DurableWitness in persist_cast, rejecting mismatches so
cross-owner moves are never persisted. Also add the same invariant check in
apply_durable_step if the existing defense-in-depth pattern requires it.
- Around line 328-340: Update recover_and_apply to use the owner’s durable
last-applied cast_id as the idempotence watermark: skip witnesses whose cast_id
is at or below that watermark, and retain the mv.from versus owner.phase() check
only as a corruption guard. Ensure the watermark is persisted with the recovered
state so replaying a complete cyclic lap after recovery remains idempotent;
otherwise document the limitation and add a regression test covering the full
cycle.
---
Nitpick comments:
In `@crates/lance-graph-planner/src/persist_sink.rs`:
- Around line 406-419: The FakeSink test double cannot be shared across
concurrent persist_cast calls because it uses Cell and RefCell, leaving the
documented DurableWrite concurrency behavior untested. Update FakeSink to use
AtomicU32 for calls and Mutex<Vec<DurableWitness>> for landed, then add a
multi-threaded tokio test that joins two persist_cast futures and verifies both
witnesses were recorded.
- Around line 694-723: Extend
recovery_replays_one_owners_batch_in_cast_order_ignoring_interleaved_owners with
a durable witness for owner 42 whose paired_move is None, interleaved between
its two cast_of moves. Update the expected applied destinations and final phase
assertions to confirm the no-step witness is skipped while the subsequent move
still replays in cast order.
- Around line 83-85: Adopt the established snafu error pattern for the public
`WriteFailed` and `PersistError` types in the persist sink module. Add the
required snafu derives and one `#[snafu(display(...))]` message per error
variant, preserving each type’s existing payloads and semantics so both
implement `Display` and `std::error::Error` for external callers.
- Around line 336-341: The recover_and_apply flow must preserve information
about moves already applied when owner.try_advance_phase fails. Update
PersistError::Illegal or the recover_and_apply contract so callers receive the
applied prefix alongside the failure, or explicitly document that Illegal leaves
the owner mid-chain and exposes the partial progress; ensure earlier applied
moves are not silently discarded.
- Around line 155-160: Update the scan_witnesses trait seam to support bounded
replay by accepting an optional DurableCoordinate lower bound and returning only
witnesses after that coordinate, so recovery does not load the entire durable
log. Propagate this change through the recovery flow, and ensure multi-owner
recovery derives local trajectories once via temporal::local_trajectories rather
than repeatedly scanning and cloning the full witness slice through
local_trajectory_of.
In `@crates/lance-graph-planner/src/temporal.rs`:
- Around line 397-424: Update the documentation for LocalCausalRow::cast_seq to
state that cast_seq must be unique and monotonic within each owner; if duplicate
values occur, local_trajectories preserves their original global durable-log
order as the tie-break because the stable sort is used.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 789afb32-68d7-495c-8b71-98cbb38d35c9
📒 Files selected for processing (6)
.claude/board/EPIPHANIES.md.claude/board/LATEST_STATE.md.claude/board/STATUS_BOARD.mdcrates/lance-graph-planner/src/lib.rscrates/lance-graph-planner/src/persist_sink.rscrates/lance-graph-planner/src/temporal.rs
| | D-MBX-A6-P3a | StyleStrategy: thinking-style -> cluster -> mechanism -> recipe_kernels Tactic selection (planning substrate; carries tau JIT addr) | lance-graph-planner | 130 | LOW | **In PR** | #439; first cut of A6-P3 consumer wiring; planner now consumes contract recipes/styles; deferred: i4-32D decode, Outcome->Candidate, tau->JIT, membrane commit | | ||
| | D-MBX-A6-P3b | output overhaul: `StrategyOutcome{reliability, intended_move: Option<KanbanMove>}` carrier on `PlanInput.outcome`; StyleStrategy retires the dead-store `_reliability`, SURFACES reliability + a bootstrap intended move (Planning→CognitiveWork, owner 0, warden-BOOTSTRAP-OK) — plan still pure | lance-graph-planner | 130 | LOW | **In progress** | additive Option field (6 in-crate literals); UNBLOCKED (no mint, not OQ-11.7); deferred: compose thread-out + contract-promote + owner-consume; E-STRATEGY-OUTCOME-CARRIER-1 | | ||
| | D-MBX-A6-P3c | owner-consume: `lance_graph_planner::owner_adapter` = the `Outcome → KanbanMove` bootstrap-rebind + ahead-cast adapter. `rebind_bootstrap` (mailbox 0/cycle 0 sentinel → live owner; refuses an already-owned move = no ownership theft) + `emit_bootstrap_intent` → `BatchWriter::cast(on_behalf = owner)`. Fire-and-forget (no ack/ledger/WAL/arbitration/callback); the move is the pre-write "parcel address", the lifecycle STEP stays post-write. Completes P3b's deferred `owner-consume`. | lance-graph-planner | 90 | LOW | **In PR** | 5 falsifiable probes (rebind 0→live anti-vacuity + no-theft + on-behalf cast + non-vacuous no-op silence); lance-free, builds without protoc. Persistence sink (drain→Lance 7 `mem_wal::WalAppender::append`) verified-but-gated (protoc missing + disk); knowledge doc `.claude/v3/knowledge/d-mbx-a6-owner-consume-and-persistence.md`; `E-KANBANMOVE-IS-THE-PARCEL-ADDRESS-STEP-IS-THE-DELIVERY-SCAN-1` | | ||
| | D-MBX-A6-P3d | persistence sink ORDERING CORE + durable-witness reshape + temporal layer-1 (the POST-write half). `lance_graph_planner::persist_sink`: two clock domains (async `persist_cast` no-owner-borrow → `DurableReceipt`; sync `apply_durable_step` no-await → `try_advance_phase`). Crash-durability: `DurableWitness{owner,cast_id,cycle,paired_move}` CO-LOCATED with the SoA payload in one generation via `DurableWrite::append(&witness,&payload)`; `scan_witnesses` replay seam; `recover_and_apply` replays the pending tail in cast order (idempotent, stale-skip); `StalePhase` from-guard on the sync path. `temporal::{LocalCausalRow, local_trajectories, local_trajectory_of}` = layer-1 CAUSAL deinterlacing (global interleaved log → per-owner local chain), composing with the existing layer-2 epistemic projection. Durability proof = `DurableCoordinate`, never `LanceVersion`. | lance-graph-planner | 130 | LOW | **In progress** | 5 operator falsifiers green (deinterlace+vacuity, cast-order replay, crash→reconstruct+apply, WAL-fail→no-step, WAL-before-manifest); 343 planner lib tests, clippy+fmt clean; builds NO concrete `LanceShardSink` (deferred per operator — gated on these falsifiers); `E-THE-PAIRED-MOVE-MUST-BE-DURABLE-CO-LOCATED-NOT-IN-MEMORY-ONLY-1` | |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Preserve append-only board history.
Line 966 inserts a new governance row between existing historical rows. Prepend new entries in newest-first order instead of inserting them into the existing history. If STATUS_BOARD.md is intended to be a snapshot table, document that exception in the repository guidance.
As per coding guidelines, governance entries in .claude/board/*.md must be append-only. Based on learnings, new board entries must be prepended without reordering existing history.
🤖 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 @.claude/board/STATUS_BOARD.md at line 966, Preserve the append-only history
in STATUS_BOARD.md by moving the new D-MBX-A6-P3d governance row to the
beginning of the board entries rather than inserting it between existing
historical rows. Keep all existing rows in their current order; only use an
in-place insertion if repository guidance explicitly documents this file as a
snapshot table.
Sources: Coding guidelines, Learnings
#878 review) Three confirmed correctness fixes from Bugbot/Codex/CodeRabbit review, all in the crash-recovery path the operator mandated: - **cast_id resets across restarts (Bugbot High).** BatchWriter::next_id resets to 0 on restart, so witnesses from different writer lifetimes collide on cast_id — ordering replay by it is unstable. Replay now orders by the durable DurableCoordinate::log_order (WAL entry position, monotonic across restarts, never reset). scan_witnesses returns LandedWitness{coordinate, witness}, which is the LocalCausalRow implementor; cast_id survives as provenance only. - **phase equality is unsound idempotence on a cyclic lifecycle (Codex + CodeRabbit Critical).** Planning→CognitiveWork→Evaluation→Plan→Planning is a real lap; after it the owner is back at Planning, so a phase-only "already applied" check replays the whole lap. recover_and_apply now takes a durable watermark (applied_through): skip ≤ watermark; above it a non-matching `from` is genuine corruption (StalePhase), not a benign skip. Returns Recovered{applied, watermark} to persist WITH the SoA phase. New falsifier proves cyclic idempotence WITH the watermark and, as a negative control, reproduces the double-lap WITHOUT it (watermark is load-bearing). - **cross-owner move could become durable (CodeRabbit Major).** persist_cast now rejects paired_move.mailbox != owner before the append; apply_durable_step adds the same defense-in-depth check. Also: scan_witnesses(from) is bounded (tail read); WriteFailed/PersistError impl Display+Error (no snafu dep added); FakeSink is Sync (Mutex+Atomic) with a multi-thread concurrent-drain test; a durable-no-step recovery case; the layer-1 cast_seq doc states the monotonic-across-restarts precondition. StalePhase on the sync path documented safe-to-drop (the durable witness replays). 348 planner lib tests pass; clippy + fmt clean. No Cargo.lock change. Still builds NO concrete LanceShardSink (operator-deferred). Board entries (EPIPHANIES Correction, LATEST_STATE, STATUS_BOARD) updated to the corrected design. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KCGhDYoQBXs3poaR7sFuqp
Review round addressed in
|
…t framing (PR #878 review 2) ChatGPT review (taken with a grain of salt — it reviewed the pre-fix c7424cd; findings 1/2/4 were already fixed in 08fa9f3). Three further points held up: - **Reordered receipts must stay retryable, not dropped (Codex/ChatGPT).** A stale receipt dropped on the sync/happy path would only be re-applied on a crash+restart — recovery is NOT the backstop for ordinary concurrent-completion reordering. apply_durable_step now BORROWS &DurableReceipt; on StalePhase the caller keeps it and retries once the missing prefix lands. New falsifier a_reordered_receipt_is_retryable_not_lost (later receipt applied before its predecessor → neither lost, both fire in order, no crash needed). The earlier "safe to drop" doc claim was wrong and is corrected. - **API-honest durable coordinate (ChatGPT/prior note).** ShardWriter::put returns a batch position, not WalAppendResult::entry_position, so the field wal_entry_position over-promised. Renamed to an opaque backend-defined `seq` (the sink maps whichever position its API yields); no field names a WAL offset the chosen API does not return. - **Honest test/claims framing (ChatGPT; the Ladybug compile≠storage lesson).** The FakeSink tests are labelled ordering/recovery CONTRACT probes, not crash/WAL integration tests (no real MemWAL, restart, atomic RecordBatch co-location, or fencing). The fake now STORES the payload (was ignored) and a probe asserts it landed alongside the witness. Module doc + board wording downgraded from "crash-durable / high confidence" to "contract-probed; real durability unproven until the concrete sink". Taken with salt, NOT actioned as blocking: the per-cast-vs-generation persistence SEAM SHAPE (finding 5) is a real architectural fork but not a correctness bug (the operator's own bb5fcdd used per-cast) — surfaced to the operator for decision, not unilaterally reshaped. 349 planner lib tests pass; clippy + fmt clean; no Cargo.lock change. Still builds NO concrete LanceShardSink. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KCGhDYoQBXs3poaR7sFuqp
…er cast (PR #878) The operator ruled the seam-shape fork surfaced in Correction 2(c) of E-THE-PAIRED-MOVE-...-1: the durable unit is the cycle/sweep, not the cast. 64k concurrent thoughts stage into an owned Vec<SweepSlot>; persist_cycle folds+freezes them; WalSink::commit_cycle does exactly ONE atomic append -> one DatasetVersion. Modelling durability per-cast paid WAL header + durability-wait + version-metadata 64k times; this amortizes it. The layer's ONLY concerns are storage + no physical/epistemic race: - Physical race: order_cycle_stably (generic over the caller's key) stable- orders casts by their EXISTING stream_position in DetachedCycleBatch::freeze BEFORE the single append. scan_sealed returns stored order and NEVER sorts. Write-side ordering lives here, NOT in temporal.rs (which owns query-time reading); the deinterlace_write additions made there this session are reverted (temporal.rs untouched). - Epistemic race: sealed read horizon (read Vn / write Vn+1); an uncommitted cycle is invisible to scan_sealed. - Coalescing: real per-row fold (row -> last payload in stream order), not last-chunk-wins. No semantic/witness/ancestry/awareness/branch/rung/temporal-policy type is minted in this layer. CycleFrame is storage-only {cycle, base_version}; SweepSlot is a boring landing (no basis). Vocabulary reused, not re-minted: DatasetVersion, KanbanMove/KanbanColumn, MailboxId/MailboxSoaOwner, stream_position. recover_and_apply + the durable watermark idempotence + StalePhase/OwnerMismatch guards survive unchanged. Retired: DurableWitness, DurableReceipt, DurableCoordinate, DurableWrite, persist_cast, apply_durable_step, scan_witnesses, LandedWitness. Tests: 13 storage/race CONTRACT probes over an in-process FakeWalSink, honestly labelled (compile+test green != storage proven, the Ladybug lesson). No concrete Lance sink built (deferred, gated on crash falsifiers). lance-graph-planner: 344 lib tests green, clippy+fmt clean. Board: EPIPHANIES E-THE-DURABLE-UNIT-IS-THE-CYCLE-...-1 + LATEST_STATE D-MBX-A6-P3e (prepended, same commit). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KCGhDYoQBXs3poaR7sFuqp
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (4)
crates/lance-graph-planner/src/persist_sink.rs (4)
828-891: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a test for the no-step watermark branch.
recover_and_applyadvances the watermark for a landing whosepaired_moveisNone(line 357). No test drives that branch throughrecover_and_apply. TheNonelandings in this file appear only in the ordering and coalescing tests, which never call recovery.The branch matters: if it did not advance, a no-step landing would be re-scanned forever and would block the watermark behind every following step.
Add a case with a mixed chain, for example a step at position 0, a no-step landing at position 1, and a step at position 2. Assert that
rec.watermark == Some(2)and that a second pass with that watermark applies nothing.🤖 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 `@crates/lance-graph-planner/src/persist_sink.rs` around lines 828 - 891, Add a recovery test alongside cyclic_recovery_is_idempotent_only_with_the_watermark that persists a mixed chain containing a step at position 0, a no-step landing at position 1, and a step at position 2, then calls recover_and_apply. Assert the first result advances rec.watermark to Some(2), and a second recover_and_apply call using that watermark applies nothing.
225-249: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider
snafufor these error types, or at least implementsource().The repository guideline asks Rust crates to reuse
snafuerror patterns.WriteFailedandPersistErroruse hand-writtenDisplayand emptyErrorimpls instead. The concrete cost is a broken error chain:PersistError::WriteandPersistError::Illegalwrap inner errors, butError::sourcereturnsNone, so callers cannot walk to the cause.If you keep the manual impls, implement
source()for the wrapping variants.♻️ Minimal alternative without adding `snafu`
-impl std::error::Error for PersistError {} +impl std::error::Error for PersistError { + fn source(&self) -> Option<&(dyn std::error::Error + 'static)> { + match self { + Self::Write(e) => Some(e), + _ => None, + } + } +}As per coding guidelines: "reuse
snafuerror patterns".🤖 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 `@crates/lance-graph-planner/src/persist_sink.rs` around lines 225 - 249, Update PersistError’s std::error::Error implementation to expose wrapped causes through source(), returning the underlying error for Write and Illegal and None for OwnerMismatch and StalePhase. Preserve the existing Display messages and avoid unrelated refactoring; apply the same error-chain treatment to WriteFailed if it is defined nearby and manually implements Error.Source: Coding guidelines
737-758: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThe sealed-predecessor assertion is tautological.
Line 750 asserts
f2.base_version == s1, but line 749 constructsf2withs1as that exact argument. The assertion restates the constructor input and exercises no sink behaviour. The test name claims the stronger property that a cycle cannot read an in-flight sibling.
FakeWalSink::commit_cycleignores itsbaseparameter entirely, so no test proves that a commit against a stalebase_versionis fenced. Make the fake comparebaseagainst the current sealed head and reject a mismatch, then assert that rejection here.🤖 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 `@crates/lance-graph-planner/src/persist_sink.rs` around lines 737 - 758, Strengthen the test by updating FakeWalSink::commit_cycle to compare its base argument with the current sealed head and reject mismatches, thereby fencing stale or in-flight predecessors. In a_cycle_reads_the_sealed_predecessor_not_an_in_flight_sibling, remove the tautological f2.base_version assertion and assert that committing with a stale sibling base is rejected, while preserving the successful sealed-predecessor commit and resulting DatasetVersion(2) checks.
260-286: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDocument the non-
SendWalSinkmethods or make the returned futuresSend.
WalSinkis public andpersist_cycle<WalSink>returns itsasyncmethods, but async trait methods still produce non-Sendfutures. Do nottokio::spawncalls outside this crate, or changecommit_cycle,scan_sealed, andversionsto return explicitimpl Future<Output = ...> + Sendfutures.🤖 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 `@crates/lance-graph-planner/src/persist_sink.rs` around lines 260 - 286, Update the public WalSink contract and persist_cycle usage to explicitly document that commit_cycle, scan_sealed, and versions return non-Send futures; do not introduce tokio::spawn calls outside the crate. Alternatively, if these operations must be spawnable, change all three methods to return explicit Send futures while preserving their existing result types and behavior.
🤖 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 @.claude/board/EPIPHANIES.md:
- Around line 35-37: Preserve the original 2026-08-01 board entry unchanged,
except for permitted Status or Confidence updates. Move the correction text
currently embedded in that entry into a new dated entry at the top, keeping
.claude/board entries append-only and ordered newest-first.
In @.claude/board/LATEST_STATE.md:
- Line 4: Update the temporal.rs statement in LATEST_STATE.md to clarify that
only the write-side deinterlace_write additions were reverted, while the
existing local_trajectories and local_trajectory_of changes remain included. Do
not state that temporal.rs is entirely untouched.
In `@crates/lance-graph-planner/src/persist_sink.rs`:
- Around line 343-380: Update recover_and_apply to preserve and return the
accumulated Recovered value when a later landing fails: include the current
applied steps and watermark alongside errors from OwnerMismatch, StalePhase, and
try_advance_phase’s Illegal mapping. Ensure callers can persist the watermark
for the successfully processed prefix, and add a mid-chain failure test
asserting that the returned partial watermark covers that prefix.
- Around line 298-313: Add a dedicated permanent error variant for cast/frame
cycle mismatches alongside the existing OwnerMismatch variant, then update the
cycle check in the casts validation loop to return it instead of
PersistError::Write. Preserve the mismatch details in the new error so retry
logic can distinguish this caller error from retryable seal failures.
- Around line 122-143: Document that SweepSlot::stream_position is a globally
strictly monotonic per-owner ordering and recovery key across sealed cycles, and
clarify that Recovered::watermark uses the same cross-cycle scope. Add a
multi-cycle recovery test covering landings from later cycles, verifying
recovery does not skip positions at or below an earlier cycle’s watermark.
---
Nitpick comments:
In `@crates/lance-graph-planner/src/persist_sink.rs`:
- Around line 828-891: Add a recovery test alongside
cyclic_recovery_is_idempotent_only_with_the_watermark that persists a mixed
chain containing a step at position 0, a no-step landing at position 1, and a
step at position 2, then calls recover_and_apply. Assert the first result
advances rec.watermark to Some(2), and a second recover_and_apply call using
that watermark applies nothing.
- Around line 225-249: Update PersistError’s std::error::Error implementation to
expose wrapped causes through source(), returning the underlying error for Write
and Illegal and None for OwnerMismatch and StalePhase. Preserve the existing
Display messages and avoid unrelated refactoring; apply the same error-chain
treatment to WriteFailed if it is defined nearby and manually implements Error.
- Around line 737-758: Strengthen the test by updating FakeWalSink::commit_cycle
to compare its base argument with the current sealed head and reject mismatches,
thereby fencing stale or in-flight predecessors. In
a_cycle_reads_the_sealed_predecessor_not_an_in_flight_sibling, remove the
tautological f2.base_version assertion and assert that committing with a stale
sibling base is rejected, while preserving the successful sealed-predecessor
commit and resulting DatasetVersion(2) checks.
- Around line 260-286: Update the public WalSink contract and persist_cycle
usage to explicitly document that commit_cycle, scan_sealed, and versions return
non-Send futures; do not introduce tokio::spawn calls outside the crate.
Alternatively, if these operations must be spawnable, change all three methods
to return explicit Send futures while preserving their existing result types and
behavior.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 935b11c2-30cc-400c-83f7-de6b193b7461
📒 Files selected for processing (5)
.claude/board/EPIPHANIES.md.claude/board/LATEST_STATE.md.claude/board/STATUS_BOARD.mdcrates/lance-graph-planner/src/persist_sink.rscrates/lance-graph-planner/src/temporal.rs
🚧 Files skipped from review as they are similar to previous changes (2)
- .claude/board/STATUS_BOARD.md
- crates/lance-graph-planner/src/temporal.rs
…hape (PR #878) Five valid findings + nitpicks, verified against the code and fixed: - recover_and_apply now returns Result<Recovered, (Recovered, PersistError)> (Major): it mutates the owner as it walks the chain, so a mid-chain failure had already advanced the owner's phase for the applied prefix but dropped the accumulated Recovered on Err — the caller could not persist the watermark it earned, replaying applied moves against an advanced owner (permanent StalePhase stall acyclic / double-apply cyclic). Each error path now returns the partial Recovered. New falsifier a_mid_chain_failure_returns_the_applied_ prefix_watermark. - PersistError::CycleMismatch (Minor): a cast whose cycle != frame.cycle is a permanent caller error, not the RETRYABLE Write class (retry would loop). New variant + a_cast_for_the_wrong_cycle_is_a_permanent_cycle_mismatch_not_write. - stream_position cross-cycle monotonicity (Major, doc gap): documented on SweepSlot::stream_position + Recovered::watermark that the key is monotonic per owner ACROSS cycles (recover_and_apply compares it over the multi-cycle scan_sealed stream). New falsifier recovery_watermark_spans_multiple_sealed_ cycles. - PersistError::source() (nitpick): wrapping variants (Write/Illegal) now forward to their inner error so callers can walk the chain. - Sealed-predecessor test was tautological (nitpick): FakeWalSink::commit_cycle now fences on a stale base (optimistic-concurrency: base must equal the sealed head), and the test asserts a stale-base commit is REJECTED instead of restating the constructor input — the epistemic horizon is now proven, not assumed. - No-step watermark branch (nitpick): new falsifier a_no_step_landing_advances_ the_watermark_and_is_not_re_scanned (mixed step/no-step chain). - WalSink futures documented as non-Send (nitpick). - LATEST_STATE wording corrected: only the write-side deinterlace_write additions were reverted; temporal.rs's existing query-time causal helpers (LocalCausalRow/local_trajectories) landed in earlier PR commits and remain — the reshape commit leaves temporal.rs untouched, NOT the whole PR. Not actioned (false positive): the "EPIPHANIES append-only" flag on lines 35-37 — those are the unchanged 2026-08-01 entry's Corrections; the new entry is prepended at the top (lines 1-17). The diff mis-attributes because prepending shifts line numbers. lance-graph-planner: 348 lib tests green (+4), clippy + fmt clean. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KCGhDYoQBXs3poaR7sFuqp
2 participants
What & why
The POST-write half of the D-MBX-A6 persistence sink, reshaped to close a
crash-durability gap the operator named, plus the missing layer-1 of
temporal.rs. Both are mandated before any concreteLanceShardSink—this PR builds no production sink.
The gap
At the prior split-phase shape,
persist_castappended only the payload; thepaired
KanbanMovelived solely in the in-memoryDurableReceipt. Then:Silent: storage advanced, lifecycle did not. An in-memory receipt is not a
durable record.
The reshape (
persist_sink)DurableWitness{owner, cast_id, cycle, paired_move}is CO-LOCATED with theSoA payload in ONE persistence generation —
DurableWrite::append(&witness, &payload)lands both atomically. The witness is the crash-durable copy of themove, never in-memory-only.
DurableReceiptnow merely references that durable material (via itsDurableCoordinate); it is not the only copy.DurableWrite::scan_witnesses— replay seam; reads the co-located witnessesback (a read of what durable storage holds, not a separate ack ledger —
E-ACK-ELIMINATED-1stands).recover_and_apply(owner, witnesses)— crash recovery: runs temporal layer-1to find the owner's pending tail and replays in cast order; stale moves
(already reflected in the recovered state) are skipped (idempotent).
apply_durable_stepgains afrom == phaseguard (PersistError::StalePhase)so a stale/out-of-order apply on the synchronous path is surfaced loudly.
DurableCoordinate(shard/epoch/wal-position),never a
LanceVersion— the write is durable before any base manifestversion attaches.
temporal.rslayer-1 — CAUSAL deinterlacingNew
LocalCausalRow{owner, cast_seq}+local_trajectories/local_trajectory_ofsplit a globally-interleaved durable log into per-ownerlocal causal chains, ordered by
cast_seq:A@s0, C@s0, B@s0, A@s1→owner A's chain
[A@s0, A@s1], with the interleaved owners removed from A'stimeline. This composes with the existing layer-2 epistemic projection
(
classify/deinterlace): layer-1 rebuilds the chain, layer-2 filters it bythe reader's horizon.
DurableWitnessimplementsLocalCausalRow.Falsifiers (the operator's five integration falsifiers — all green)
(+ vacuity guard: other owners' rows dropped, not reordered)
seq-descending log)
move from the durable witness; idempotent re-run applies nothing
cast order; no dataset version exists
Gates
lance-graph-plannerlib tests pass;cargo clippy -p lance-graph-plannercargo fmt -pclean.LanceShardSink(deferred per operator ruling — gatedon these crash falsifiers going green).
Cargo.lockuntouched).Deliberately NOT done
Concrete
LanceShardSink(ShardWriter::put,enable_memtable + durable_write);confirmation ledger / replay queue / per-thought ack / custom WAL / ractor
hot-path callback.
Finding:
E-THE-PAIRED-MOVE-MUST-BE-DURABLE-CO-LOCATED-NOT-IN-MEMORY-ONLY-1.🤖 Generated with Claude Code
Generated by Claude Code
Summary by CodeRabbit
New Features
Bug Fixes