D-MBX-A6-P3e: persistence sink = primitive cycle/WAL bootstrap (one write per sweep), scalar order explicitly provisional - #878
Merged
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
📝 WalkthroughWalkthroughAdded a public cycle-level persistence sink. It 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 architecture records. ChangesCycle persistence and causal recovery
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
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".
Comment on lines
+330
to
+334
| 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 👍 / 👎.
Comment on lines
+289
to
+293
| 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
Owner
Author
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
…imensional temporal upgrade path Documentation-only. No Rust code, tests, public APIs, temporal.rs, or the #878 persistence implementation change. New narrowly-scoped plan persistence-cycle-wal-bootstrap-v1.md (no suitable canonical plan existed — the persist_sink cycle-WAL seam was tracked only on the board/EPIPHANIES). It records the architectural ruling: - #878 is a PRIMITIVE cycle/WAL bootstrap so thinking can be wired and runnable before the full cognitive topology is solved: concurrent results → primitive slot collection/order → freeze one cycle → one amortized WAL write → one DatasetVersion. Six hard guarantees documented (single sealed Vn read horizon; open-cycle results invisible as Vn input; freeze-before-I/O; one seal = one WAL write = one version; no live mutable SoA borrow to the WAL). - The scalar slot/order model is EXPLICITLY provisional — sufficient for the execution/durability plumbing, not claimed as the final temporal representation. - Two orthogonal dimensions: horizontal = temporal.rs coherence within a frame (deterministic book→chapter→verse→span; partially-ordered neighbourhoods for medical/higher-order thought, so a scalar key is insufficient); vertical = DatasetVersion succession + cheap Stockfish-style hindsight lookup. The version table is vertical only, never horizontal causal ordering. - Planned upgrade: a wait-free-emit shadow temporal.rs coherence pass feeding revision.rs forward-correction into a LATER cycle; a sealed historical version is never silently rewritten. - Known limitations (§4) and scope exclusions (§5) recorded: provisional scalar key, unfinalized conflict semantics, concrete-sink hardening still needed, fake sink proves contract shape not crash durability; no cohort internals / slot count / actor-neighbour waiting; no new semantic/temporal/rung/witness/branch/ ancestry types; no reviving ThoughtWitness/basis/awareness_seq/per-cast WAL. Cross-references (does not re-specify): horizontal detail → temporal-markov-and-style-classes-v1.md; D-MBX-A6 tracking → STATUS_BOARD; per-row write gate → mailbox-cycle-aware-write-contract-v1.md; reshape ruling → EPIPHANIES. Registered in INTEGRATION_PLANS.md (prepended, newest-first). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KCGhDYoQBXs3poaR7sFuqp
AdaWorldAPI
changed the title
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/lance-graph-planner/src/persist_sink.rs (1)
396-443: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winConsider a carrier-object method instead of a free function; consider a named error type instead of a raw tuple.
Two independent, optional improvements to
recover_and_apply's shape:
recover_and_apply(owner: &mut O, ...)is a free function that separately receives the carrier state (owner). SinceMailboxSoaOwneris presumably defined outside this crate, an inherent method can't be added directly, but a blanket extension trait keeps the call site asowner.recover_and_apply(sealed, applied_through), matching the carrier-object idiom used elsewhere (trajectory.resolve()).- The error type
(Recovered, PersistError)is a bare tuple. A small named struct (e.g.RecoveryFailure { partial: Recovered, cause: PersistError }) documents the two fields at the destructuring call site instead of relying on positional order.♻️ Sketch of the extension-trait approach
pub trait RecoverAndApplyExt: MailboxSoaOwner { fn recover_and_apply( &mut self, sealed: &[LandedSlot], applied_through: Option<u64>, ) -> Result<Recovered, (Recovered, PersistError)>; } impl<O: MailboxSoaOwner> RecoverAndApplyExt for O { fn recover_and_apply( &mut self, sealed: &[LandedSlot], applied_through: Option<u64>, ) -> Result<Recovered, (Recovered, PersistError)> { recover_and_apply_impl(self, sealed, applied_through) } }Since this API is introduced fresh in this PR, both changes are cheap now and only touch call sites within this file (including tests).
As per coding guidelines,
**/*.rs: "Keep cognitive state and inference behavior on the carrier object: prefer methods such astrajectory.resolve()over free functions that separately receive the carrier state."🤖 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 396 - 443, Refactor recover_and_apply into a RecoverAndApplyExt extension-trait method on MailboxSoaOwner, preserving its existing recovery behavior and updating all call sites and tests to invoke it on the owner. Replace the bare (Recovered, PersistError) error tuple with a named RecoveryFailure type exposing partial recovery and cause fields, and update every construction, match, and destructuring site accordingly.Source: Coding guidelines
🧹 Nitpick comments (1)
crates/lance-graph-planner/src/persist_sink.rs (1)
210-286: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueKeep
PersistErroraligned with this crate’s error convention.
PersistErroruses manualDisplay/Errorimplementations, whilecrates/**/*.rsshould reusesnafuerror patterns. Sincelance-graph-plannerdoes not includesnafu, addsnafuhere or use the crate-wide existing pattern consistently across Rust crates.🤖 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 210 - 286, Update PersistError to follow the repository’s established snafu-based error convention instead of its manual Display and Error implementations. Add the required snafu dependency for the lance-graph-planner crate if needed, derive or define the equivalent snafu error structure for all PersistError variants, and preserve the existing messages and wrapped sources for Write and Illegal.Source: Coding guidelines
🤖 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/INTEGRATION_PLANS.md:
- Around line 41-66: Move the dated 2026-08-02 persistence-cycle-wal-bootstrap
v1 entry above the existing 2026-07-31 entry in the dated-entry list. Preserve
the entry’s content unchanged and maintain the file’s newest-first append-only
ledger ordering.
---
Outside diff comments:
In `@crates/lance-graph-planner/src/persist_sink.rs`:
- Around line 396-443: Refactor recover_and_apply into a RecoverAndApplyExt
extension-trait method on MailboxSoaOwner, preserving its existing recovery
behavior and updating all call sites and tests to invoke it on the owner.
Replace the bare (Recovered, PersistError) error tuple with a named
RecoveryFailure type exposing partial recovery and cause fields, and update
every construction, match, and destructuring site accordingly.
---
Nitpick comments:
In `@crates/lance-graph-planner/src/persist_sink.rs`:
- Around line 210-286: Update PersistError to follow the repository’s
established snafu-based error convention instead of its manual Display and Error
implementations. Add the required snafu dependency for the lance-graph-planner
crate if needed, derive or define the equivalent snafu error structure for all
PersistError variants, and preserve the existing messages and wrapped sources
for Write and Illegal.
🪄 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: 5209d261-b002-4653-8dfc-fbd71330611e
📒 Files selected for processing (4)
.claude/board/INTEGRATION_PLANS.md.claude/board/LATEST_STATE.md.claude/plans/persistence-cycle-wal-bootstrap-v1.mdcrates/lance-graph-planner/src/persist_sink.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- .claude/board/LATEST_STATE.md
Comment on lines
+41
to
+66
| ## 2026-08-02 — persistence-cycle-wal-bootstrap v1 — ACTIVE (bootstrap SHIPPED #878; temporal/revision upgrade PLANNED) — main thread | ||
|
|
||
| **Plan:** `.claude/plans/persistence-cycle-wal-bootstrap-v1.md` | ||
| Documentation-only architectural ruling recording the *role* of the #878 | ||
| persistence seam and the larger two-dimensional temporal architecture it | ||
| bootstraps toward. #878 is a **primitive cycle/WAL bootstrap** (concurrent | ||
| results → primitive slot collection/order → freeze one cycle → one amortized | ||
| WAL write → one `DatasetVersion`), with six hard guarantees (single sealed `Vn` | ||
| read horizon; open-cycle results invisible as `Vn` input; freeze-before-I/O; one | ||
| seal = one WAL write = one version; no live mutable SoA borrow to the WAL). The | ||
| scalar slot/order model is **explicitly provisional** — sufficient for the | ||
| execution/durability plumbing, not the final temporal representation. Two | ||
| orthogonal dimensions: **horizontal** = `temporal.rs` coherence within a frame | ||
| (book→chapter→verse→span for deterministic streams; partially-ordered | ||
| neighbourhoods for medical/higher-order thought — hence a scalar key is | ||
| insufficient); **vertical** = `DatasetVersion` succession + cheap | ||
| Stockfish-style hindsight lookup. Planned upgrade: a wait-free-emit shadow | ||
| `temporal.rs` coherence pass feeding `revision.rs` forward-correction into a | ||
| later cycle — a sealed version is never silently rewritten. Cross-refs (does not | ||
| re-specify): horizontal detail → `temporal-markov-and-style-classes-v1.md`; | ||
| D-MBX-A6 tracking → STATUS_BOARD; per-row write gate → | ||
| `mailbox-cycle-aware-write-contract-v1.md`; reshape ruling → EPIPHANIES | ||
| `E-THE-DURABLE-UNIT-IS-THE-CYCLE-...-1`. Scope exclusions: no cohort internals / | ||
| slot count / actor-neighbour waiting; no new semantic/temporal/rung/witness/ | ||
| branch/ancestry types; no reviving ThoughtWitness/basis/awareness_seq/per-cast | ||
| WAL. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Move the new entry above the 2026-07-31 entry to preserve newest-first order.
This entry is dated 2026-08-02, but it is inserted directly below the existing 2026-07-31 entry (starting at line 11), rather than above it. Prepend it at the top of the dated-entry list instead.
Based on learnings, "treat these files as append-only ledgers that are ordered newest-first. When adding information, prepend new entries rather than appending at the end," and "prepend genuinely new entries so the file remains in newest-first order."
🤖 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/INTEGRATION_PLANS.md around lines 41 - 66, Move the dated
2026-08-02 persistence-cycle-wal-bootstrap v1 entry above the existing
2026-07-31 entry in the dated-entry list. Preserve the entry’s content unchanged
and maintain the file’s newest-first append-only ledger ordering.
Source: Learnings
AdaWorldAPI
pushed a commit
that referenced
this pull request
Aug 2, 2026
…#878 follow-up) Documentation-only. No Rust code, tests, public APIs, persist_sink.rs, temporal.rs, or the persistence implementation change. 1) Sparse-delta ruling (persistence-cycle-wal-bootstrap-v1.md §2, NEW): "complete logical cycle ≠ full physical dataset rewrite". A globally-complete cycle persists ONLY its coalesced dirty-row delta + required durable transition metadata; unchanged rows are inherited from the sealed base version, never re-serialized because they participated. Records the verbatim storage invariant, the participation-vs-mutation split (a no-mutation participant needs no 512-byte row), the honest in-memory payload-duplication limitation (concrete sink must not persist both per-landing bytes AND the coalesced image), the capacity/backpressure ruling (dense cycle = explicit capacity event), and 5 concrete-sink falsifiers (sparse / no-op-policy / coalescing / dense-capacity / retention). Status: RATIFIED architecture, UNIMPLEMENTED in a concrete Lance sink. Preserves the horizontal(temporal.rs) / vertical(DatasetVersion) / revision.rs split — density only. Sections renumbered (§2 inserted; §3–§7 shifted, all internal §-refs updated). 2) Loop-closure driver plan (cycle-loop-closure-driver-v1.md, NEW): the seam that makes the merged persist_sink load-bearing at 64k — persist_sink has zero production callers today, so the loop is open. Closes collect → persist_cycle → sealed version → sync inline fan-step (on_version + try_advance_phase, NOT 64k async drive_once) → CognitiveWork → owner_adapter → next cycle. Mints no new types. Deliverables D-MBX-A6-P4a..f, probe-first. Home: lance-graph-supervisor. Board: EPIPHANIES E-COMPLETE-CYCLE-IS-PHYSICALLY-SPARSE-NOT-A-FULL-REWRITE-1 (prepend); INTEGRATION_PLANS cycle-loop-closure-driver v1 (prepend); STATUS_BOARD D-MBX-A6-P4 row + P3d flipped to Merged (#878, reshaped to cycle/WAL + sparse ruling). Branch restarted from main after #878 merged. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KCGhDYoQBXs3poaR7sFuqp
AdaWorldAPI
pushed a commit
that referenced
this pull request
Aug 2, 2026
…st_sink's first production caller) Closes the ZERO-caller gap on the merged #878 persist_sink: nothing drove the seal→apply loop, so the sealed version never advanced any owner. New module `lance-graph-supervisor::cycle_driver`, behind the `cycle-driver` feature (optional ONE-WAY `lance-graph-planner` path-dep — planner never deps supervisor, verified acyclic; default supervisor build stays light, no planner/ractor). Mints NO domain types — composes the shipped organs: - P4a: `collect_casts(writer, cycle, row_of)` drains a `BatchWriter<Vec<u8>>`'s staged casts into `Vec<SweepSlot>` (one slot/cast; stream_position = CastId; paired_move = first intended move). `seal_cycle(sink, frame, casts)` reads out the SPARSE `SealedTransition` set (only slots with a move, stream-ordered) then `persist_cycle` -> exactly one WAL write, one DatasetVersion. - P4b: `apply_sealed_transitions(fleet, &SealedCycle)` iterates ONLY the sealed sparse set, resolves each owner via the `MailboxFleet` trait (blanket-impl'd for HashMap<MailboxId, O>), applies one legal `try_advance_phase`; EVERY unrepresented owner stays byte-identical (never resolved). Interim rule: <=1 durable transition per owner per cycle (2nd same-owner move -> `deferred`); unknown owner -> `missing` (counted, not a crash); StalePhase/OwnerMismatch guards; reads NO dataset (version already sealed — no scan_sealed/versions/ drive_once). `run_cycle` = P4a->P4b convenience. The load-bearing rule, enforced in code: a DatasetVersion is GLOBAL KNOWLEDGE, NOT permission to advance every mailbox — only the sealed sparse set advances (E-D-MBX-SPINE-IS-STRAIGHT-TRACK-VERSION-IS-NOT-A-FLEET-STEP-SIGNAL-1 + E-COMPLETE-CYCLE-IS-PHYSICALLY-SPARSE-NOT-A-FULL-REWRITE-1). Tests: 7 cycle_driver lib tests (10 total in the crate), headline = the 64k/17 falsifier — 65_536 mailboxes, 17 sealed transitions -> exactly 17 advance, the other 65_519 byte-identical, one WAL write, ZERO dataset reads. Plus one-WAL-write amortization, empty-sparse-set-advances-nobody, interim-defer, StalePhase corruption, missing-owner-counted, run_cycle round-trip. clippy (--features cycle-driver) exit 0, fmt clean; default (no-feature) build unchanged. Durability leg stays the contract-probe fake (FakeWalSink) — control loop closed, storage NOT proven (Ladybug rule); concrete LanceShardSink still deferred. Cargo.lock updated for the new optional supervisor->planner dep. Plan `.claude/plans/cycle-loop-closure-driver-v1.md`; STATUS_BOARD D-MBX-A6-P4 -> P4a+P4b Shipped (slice); LATEST_STATE prepended. Remaining: P4c (CognitiveWork thought body + cast round-trip), P4d/P4e/P4f. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KCGhDYoQBXs3poaR7sFuqp
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR now ships the WAL-amortized cycle/sweep persistence model, not the
per-cast durable-witness model the original description (kept below as arc
history) describes. The durable unit is the cycle, sealed with one WAL
write → one
DatasetVersion. Retired:DurableWitness/DurableReceipt/DurableCoordinate/DurableWrite/persist_cast/apply_durable_step.Reshape ruling:
EPIPHANIES.mdE-THE-DURABLE-UNIT-IS-THE-CYCLE-NOT-THE-CAST-ONE-WAL-WRITE-PER-SWEEP-1.This is a deliberately PRIMITIVE cycle/WAL bootstrap — it wires the
execution + durability plumbing so thinking can be runnable before the full
cognitive topology is solved. The current scalar slot / single-key ordering
model is explicitly provisional: it is sufficient for the plumbing and is
not claimed to be the final representation of temporal or cognitive order.
This PR does not solve, and does not claim to solve, the complete horizontal
temporal projection.
Architecture context recorded in the plan
.claude/plans/persistence-cycle-wal-bootstrap-v1.md(docs-only, in thisPR): the two-dimensional model — horizontal coherence within a frame
(
temporal.rs; a scalar key is insufficient for partially-orderedneighbourhoods) × vertical durable-frame succession (
DatasetVersion, thecheap Stockfish-style hindsight axis) — plus the planned wait-free shadow
temporal.rscoherence pass +revision.rsforward-correction (a sealedversion is never silently rewritten). Horizontal-stream detail is owned by
temporal-markov-and-style-classes-v1.md, not this PR.What #878 actually guarantees (and only this): one sealed predecessor
Vnread horizon; open-cycle results invisible as
Vninput; freeze-before-I/O; oneseal = one WAL write = one
DatasetVersion; no live mutable SoA borrow to theWAL. Known accepted limitations (provisional scalar key; unfinalized cross-owner
/ equal-position conflict semantics; retry + production WAL idempotence still
need concrete-sink hardening; the fake sink proves the contract shape, not real
crash durability) are recorded in the plan §4. No concrete
LanceShardSinkisbuilt (deferred, gated on crash falsifiers).
Gates on the current head:
lance-graph-planner348 lib tests green,cargo clippy -p+cargo fmt -pclean.Arc history (SUPERSEDED — original durable-witness description)
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.
(This durable-witness shape was subsequently reshaped to the cycle/WAL model —
see the corrected framing at the top. The gap it closed — a paired move must not
be in-memory-only — is preserved in the cycle model: the move rides on the
SweepSlotthat seals with the cycle.)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. This composes with the existinglayer-2 epistemic projection (
classify/deinterlace): layer-1 rebuilds thechain, layer-2 filters it by the reader's horizon. (These query-time helpers
landed in the PR's earlier commits and remain; only the write-side
deinterlace_writeexperiment was reverted — write-side ordering lives inpersist_sink.rs.)Finding:
E-THE-PAIRED-MOVE-MUST-BE-DURABLE-CO-LOCATED-NOT-IN-MEMORY-ONLY-1(its ordering/recovery/watermark CONTRACT stands; the seam-shape half is
superseded by the cycle/WAL ruling).
🤖 Generated with Claude Code
Generated by Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Summary by CodeRabbit
New Features
Bug Fixes