Repository navigation
Fix clean writer takeover after empty epochs - #774
Conversation
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
Warning Review limit reachedYou've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Next included review available in 32 minutes. View limit detailsLimit details: You’ve used the included review currently available. Review configuration: ⚙️ Run configuration
📒 Files selected for processing (4)
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 |
|
Code Lawyer audit, exact head c7893ce:
Full four-file diff audited against current main: shared request validation, fresh takeover lease acquisition/reload, tail recovery, predecessor closure/rollback, successor identity, typed recovery-index errors, commit append and recovery paths. No WAL format change or physical power-loss guarantee. Existing descriptor inheritance work #718 remains out of scope. Full hardening suite: 126 passed, one existing ignored; Docker formatting passed. Initial snapshot had no review threads. Independent agy review and current CI remain pending; merge gate is not yet open. @codex |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Adversarial Independent Review: PR #774 (
|
| Code Path / Behavior | Production Implementation | Parallel / Comparative Path | Audit Assessment |
|---|---|---|---|
| Lease Acquisition on Takeover | causal_wal.rs:5740acquire_writer_epoch_lock(&self.root)? |
causal_wal.rs:5987acquire_writer_epoch_lock(&self.root)? in WalStorePort |
Identical file lock semantics (WriterEpochLock using flock). Prevents concurrent live writers from taking over. |
| Ledger Reload Under Lease | causal_wal.rs:5741self.reload_writer_epoch_ledger()? |
causal_wal.rs:5990self.reload_writer_epoch_ledger()? |
Identical. Rereads persisted epoch ledger before inspecting or closing active epochs. |
| Tail Cleanliness Gate | causal_wal.rs:5742-5751recover_filesystem_store(..., ReadOnly)matches!(..., RecoveryTailPosture::Clean) |
N/A in WalStorePort(Enforced in high-level takeover only) |
Refuses with WalStoreError::SegmentHasUncommittedTail before modifying active epoch, closure map, or on-disk ledger. |
| Recovery Error Mapping | causal_wal.rs:5743-5747Maps Store, Validation, and Index variants |
causal_wal.rs:10062-10072WalRecoveryError definition |
Exhaustive 1:1 typed mapping. Maps WalRecoveryIndexError to new WalStoreError::RecoveryIndex. |
| Predecessor Closure Handling | causal_wal.rs:5753-5768Closes leftover active epoch under lease |
causal_wal.rs:8281-8316Deserialization recovery of ledger |
Leftover active epoch is appended to closed_epochs, ledger persisted, rolled back on failure. |
| Successor LSN Derivation | causal_wal.rs:5776-5781match previous_closure.final_lsn:- Some(final_lsn) => final_lsn.checked_next()?- None => previous_epoch.map_or(...) |
Base 2d79ecc4 causal_wal.rs:5764-5768:incremented even when final_lsn was None |
Eliminates unused LSN advancement for empty epochs; refuses overflow on checked_next() returning None. |
| Successor Epoch Request Synthesis | causal_wal.rs:5788-5828Derives deterministic epoch evidence |
N/A (unique to filesystem store) | Increments ordinal (closed_len + 1), ensuring unique epoch_id, fencing_token, and lease_or_lock_evidence even when LSN is reused. |
| Epoch Request Validation | causal_wal.rs:5829-5834calls validate_writer_epoch_request |
causal_wal.rs:2019 (InMemoryWalStore)causal_wal.rs:5992 (FilesystemWalStore)causal_wal.rs:8287, 8306 (Ledger decode) |
causal_wal.rs:1586-1591 relaxed from <= previous_epoch.started_at_lsn to < previous_epoch.started_at_lsn. Permitted in memory, filesystem, and during deserialization. |
| Production Runtime Integration | trusted_runtime_host.rs:3432store.acquire_fresh_writer_epoch(next_lsn) |
trusted_runtime_host.rs:2508Called during TrustedRuntimeHost::open after store.recover_for_writer()? |
When TrustedRuntimeHost opens, recover_for_writer reconciles unreconciled tails first, then acquire_fresh_writer_epoch succeeds cleanly. Direct takeover without recovery refuses dirty tails. |
Merges & Integration Audit
- Commit Topology:
- Merge Base:
2d79ecc4f08181e0d2c8829b09335567373fbd9b(HEAD oforigin/main). - PR Head:
c7893cea27a54028b06b6ce7b84e4564dce64931. - Number of commits: Exactly 1.
- Merge commits in branch history: 0 (confirmed via
git log --merges 2d79ecc4..c7893cea).
- Merge Base:
- Semantic Diff Inspection:
- Head commit
c7893ceais a linear commit directly atop base2d79ecc4. - The tree contains no merge conflicts or rerouted caller collisions.
- Target base
2d79ecc4integrated PR docs: record the study audit and experimental Keep roadmap #758 (audit/study-feedback). The changes inwarp-coreandWAL.mddo not intersect or conflict with any symbols modified in PR docs: record the study audit and experimental Keep roadmap #758 (which touched onlydocs/and tooling).
- Head commit
Constants & Numeric Evidence Verification
| Claim / Constant | Location in PR / Docs | Raw Evidence Coordinate | Verification Result |
|---|---|---|---|
| RED 1 LSN Mismatch: expected 0, got 1 | PR #774 body, line 5 | retained evidence: landing-epoch-red.log:16-18 |
Verified Exact:thread panicked at causal_wal_hardening_tests.rs:967:5left: Lsn(1), right: Lsn(0) |
| RED 2 Dirty Tail Admission Mismatch | PR #774 body, line 5 | retained evidence: landing-epoch-tail-red.log:12-13 |
Verified Exact:thread panicked at causal_wal_hardening_tests.rs:89:22expected Err(..), got Ok(WriterEpoch { started_at_lsn: Lsn(1) ... }) |
Parent Commit SHA 2d79ecc4 |
PR #774 body, line 5 | landing-epoch-red.log:1landing-epoch-tail-red.log:1landing-epoch-green4.log:1 |
Verified Exact:SOURCE_VERIFIED 2d79ecc4f08181e0d2c8829b09335567373fbd9b 991 |
| GREEN Hardening Suite: 126 passed, 1 ignored | PR #774 body, line 5 | retained evidence: landing-epoch-green4.log:149 |
Verified Exact:test result: ok. 126 passed; 0 failed; 1 ignored; 0 measured; 0 filtered out |
| Existing Ignored Test Identity | PR #774 body, line 5 | retained evidence: landing-epoch-green4.log:33 |
Verified Exact:test emit_filesystem_writer_epoch_process_step ... ignored, child entrypoint exercised by the independent-process writer-epoch test |
| Formatting Gate Passed | PR #774 body, line 5 | retained evidence: landing-epoch-green4.log:1-5 |
Verified Exact:cargo fmt --all -- --check exited cleanly before test run |
| Intermediate Failures Resolved | Landing log trail | landing-epoch-green.log:6 (trailing newline)landing-epoch-green2.log:6-10 (line wrap)landing-epoch-green3.log:3-7 (missing WalStoreError::RecoveryIndex) |
Verified Exact: All intermediate formatting and compiler errors were resolved in green4. |
| Host Build Cache Usage | landing-epoch-green4.result.json:3 |
13,384,397,914 bytes (~12.46 GiB) |
Verified Within Bound: Budget <= 21,474,836,480 bytes (20 GiB) |
| Host Test Data Usage | landing-epoch-green4.result.json:4 |
4,281,693,274 bytes (~3.988 GiB) |
Verified Within Bound: Budget <= 4,294,967,296 bytes (4 GiB) |
| Log Output Usage | landing-epoch-green4.result.json:5 |
19,741,973 bytes (~18.83 MiB) |
Verified Within Bound: Budget <= 134,217,728 bytes (128 MiB) |
| Free Storage Floors | landing-epoch-green4.result.json:6-7 |
Host: 719,326,203,904 B (~670 GiB)VM: 683,667,574,784 B (~636 GiB) |
Verified Within Bound: Both floors well above required 50 GiB floor |
| Worker Process Constraints | landing-epoch-green4.launch.json:25-26,34 |
CPUs: 4.0, Memory: 6 GiB, Timeout: 1100s | Verified Exact: Matches project resource bounds |
State Machine & Error Transition Analysis
- Takeover Over Dirty / Torn Tail:
- Under lease,
recover_filesystem_storescans segments inReadOnlymode. - Any uncommitted frames or torn records cause
tail_postureto becomeWouldTruncateAfter(lsn)orWouldTruncateAll. !matches!(recovery.tail_posture, RecoveryTailPosture::Clean)fires and returnsErr(WalStoreError::SegmentHasUncommittedTail(self.segment_id)).- Zero state mutation occurs. The lease is dropped on error.
- Under lease,
- Successor After Empty Epoch:
- Active predecessor epoch is recovered and closed.
previous_closure.final_lsnisNone.required_started_at_lsnresolves toprevious_epoch.started_at_lsn.started_at_lsn = minimum_started_at_lsn.max(previous_epoch.started_at_lsn).validate_writer_epoch_requestadmitsrequest.started_at_lsn == previous_epoch.started_at_lsn.- Successor receives incremented
ordinal, yielding fresh cryptographic evidence tokens (epoch_id,storage_fencing_token,lease_or_lock_evidence). - Distinct identities prevent fencing collision despite LSN reuse.
- Successor After Committed Epoch:
previous_closure.final_lsnisSome(final_lsn).required_started_at_lsnresolves tofinal_lsn.checked_next().ok_or(WalStoreError::WriterEpochChainGap)?.- If
final_lsn == u64::MAX, refused immediately. - If normal LSN, successor must strictly start at or after
final_lsn + 1.
- Crash During Takeover Writing:
- If
persist_writer_epoch_ledgerfails when closing the previous epoch, in-memory state is restored viaself.install_writer_epoch_ledger(previous_ledger). - If
persist_writer_epoch_ledgerfails when recording the new active epoch, state is restored viaself.install_writer_epoch_ledger(closed_ledger)andself.active_epochis cleared. - Atomic replacement via temporary file and rename preserves on-disk ledger integrity.
- If
Repository Standards & AGENTS.md Conformance
- Documentation Standards (
docs/DOCUMENTATION_STANDARDS.md):- Canonical ownership respected:
docs/topics/WAL.md:374owns the WAL takeover contract. - Paragraph formatting:
docs/topics/WAL.md:374andCHANGELOG.md:18are authored as one physical line per paragraph. - License Headers: Valid SPDX identifiers present in all four modified files.
- Git Discipline: 0 amend commits, 0 rebases, 0 force-pushes, 0 merge commits. Standard linear commit on branch.
- Canonical ownership respected:
- Out-of-Scope Integrity:
- Issue Make OS lease guards prove process-bound release across inherited descriptors #718 (process-descriptor lease redesign) is untouched.
- No observation contexts or retained strands modified.
- No spurious claims of physical power-loss recovery made.
Mandatory Verification Checklist
- Every code path traced (file:line to file:line):
- High-level takeover:
causal_wal.rs:5733->causal_wal.rs:5740(lease) ->causal_wal.rs:5741(reload) ->causal_wal.rs:5742(tail scan) ->causal_wal.rs:5776(LSN derivation) ->causal_wal.rs:5829(validation) ->causal_wal.rs:5837(persist). - Validation:
causal_wal.rs:1585-1591across all callers (causal_wal.rs:2019,causal_wal.rs:5829,causal_wal.rs:5992,causal_wal.rs:8287,causal_wal.rs:8306). - Host integration:
trusted_runtime_host.rs:3432->causal_wal.rs:5733.
- High-level takeover:
- Every merge audited:
- Branch consists of 1 commit (
c7893cea) directly atop base2d79ecc4. Zero merge commits exist in PR Fix clean writer takeover after empty epochs #774.
- Branch consists of 1 commit (
- Constants and claims verified against raw evidence:
- RED 1 (
left: Lsn(1),right: Lsn(0)inlanding-epoch-red.log:17-18). - RED 2 (
SegmentHasUncommittedTailexpected, gotOk(WriterEpoch)inlanding-epoch-tail-red.log:13). - GREEN 4 (126 passed, 1 existing ignored, exit code 0 in
landing-epoch-green4.log:149andlanding-epoch-green4.result.json:9). - Resource usage bounded: build 13.38 GiB <= 20 GiB, test data 3.99 GiB <= 4 GiB, logs 18.83 MiB <= 128 MiB.
- RED 1 (
- Documentation figures checked:
- All claims in
CHANGELOG.md,docs/topics/WAL.md, and PR metadata cross-referenced against implementation and test logs.
- All claims in
- Execution vs static inspection boundaries declared:
- Static inspection and Git object reads executed. No live test commands or container workloads executed. Process-death tests verified from Docker receipts without inferring unmeasured power-loss guarantees.
APPROVE
|
Independent Codex review of PR #774, exact head No verified blocking defects found. Approval is limited to this writer-takeover correction and its integration with the previously approved snapshot-root change. Verification Checklist
Execution boundary: I executed only static source/Git/hash inspection and GitHub API reads. I did not run tests, Docker, scripts, builds, publish comments, edit files or acquire shared workers. Test results above are inspected execution evidence. They do not prove physical power-loss durability or absence of every regression. The parent must recheck head, CI and repository protections immediately before merging. Exact-head verdict for APPROVE |
Fresh filesystem takeover advanced an empty predecessor's unused LSN, creating a gap, and could admit a new epoch over an unreconciled tail. This preserves the unused coordinate, requires a clean recovered tail before ledger changes, and refuses committed-LSN overflow with a typed error.
Closes #773. Extracted from #716/#727. Independently correct on main; queued after #772 in the agreed landing sequence. Process-descriptor lease redesign (#718) remains outside scope.
Docker evidence on parent 2d79ecc: expected LSN 0 versus actual 1; expected dirty-tail refusal versus successful admission. After the fix, formatting and the full WAL hardening suite passed: 126 passed, one existing ignored. Fixtures verify live-lease exclusion, distinct successor identity, dirty-tail refusal, writable reconciliation, committed append and clean reopen. No physical power-loss claim.
Documentation: canonical WAL topic and changelog updated. Takeover can now require explicit writable recovery before retry. Raw logs, source manifests and guarded resource receipts retained. No host tests ran.