From e050b6dddd33b4a8c5d9d60110a4c6e2eaa8d633 Mon Sep 17 00:00:00 2001 From: Levi Neuwirth Date: Tue, 28 Jul 2026 21:20:52 -0400 Subject: [PATCH] Release the root lock explicitly rather than by closing a handle B4's harness measured intermittent AlreadyLocked on reopening a store it had just dropped -- up to 29 retries over 147ms, 7 failures in 40 runs, reproducing on a single-shard engine. StoreEngine::drop was not the cause: it closes every channel and joins every writer, and looked correct in every single-process test. An flock is held by the open file description, not by a descriptor. A concurrently forked child transiently inherits that description, and FD_CLOEXEC closes it at exec, not at fork -- so during that window the parent closing its own descriptor releases nothing. Scope 3.1 makes AlreadyLocked a refusal and never a wait, so every consumer that closes and reopens a root -- a recovery drill, an in-place restart, the Phase 2 migrator -- could be refused with no defined retry. Releasing must therefore be an explicit act. lock_root returns an RAII RootLock that issues LOCK_UN in Drop before the file closes, and both owners -- RecoverySession and ShardDrive -- hold that guard. The unlock lives in sys.rs beside try_lock_exclusive so the locking syscalls stay in the one funnel. StoreEngine needs no change: it holds a RecoverySession, never a File, so it inherits the fix. Drop is the only release path. A release() and a file() accessor were written and deleted before landing: neither had a caller, and an uncalled second way to release a lock is exactly the decoy the charter names. A failed LOCK_UN in Drop cannot be returned and must not be swallowed, so it increments a counter the regression asserts unchanged. The rejected alternative was fixing this in StoreEngine::drop alone. That leaves ShardDrive and the drive's one-shot session exposed and makes correctness depend on a descriptor lifetime that fork can extend. The regression is synchronized rather than timed: the child forks while the lock is held, signals ready on one pipe, and blocks on a second until after the parent has released and attempted its reopen, so the inherited descriptor is provably open across the whole window and the reopen is asserted on its first attempt. With the explicit unlock removed it fails 10/10; as landed it passes 40/40. Measured under load -- 200 close-reopen cycles against 72,255 concurrent forks -- 0 refusals, worst case 1 attempt and 10.8ms; the same load kills the pre-fix behaviour within 0.05s, so the load reproduces the defect rather than merely being weak. Contract review 2026-07-28-B records the amendment. Two carry-forwards are recorded in scope 6.6: the verify-store-recovery SIGKILL cycles still drive the journal seam rather than submit, so kill -9 never lands inside a real publication and the acknowledged-crash-recovery criterion is only partly earned; and B4's bounded reopen retry must become a one-attempt assertion now that the defect it compensates for is gone. scripts/check-phase1.sh GATE_EXIT=0. Co-Authored-By: Claude Opus 5 (1M context) --- crates/levcs-store/src/drive.rs | 7 +- crates/levcs-store/src/recovery.rs | 6 +- crates/levcs-store/src/segment.rs | 191 +++++++++++++++++++++++- crates/levcs-store/src/sys.rs | 17 +++ doc/instance-throughput-rewrite-plan.md | 72 +++++++++ doc/phase1-storage-spine-scope.md | 49 ++++++ 6 files changed, 337 insertions(+), 5 deletions(-) diff --git a/crates/levcs-store/src/drive.rs b/crates/levcs-store/src/drive.rs index 20d1fa5..aba5962 100644 --- a/crates/levcs-store/src/drive.rs +++ b/crates/levcs-store/src/drive.rs @@ -30,7 +30,6 @@ #![cfg(feature = "store-internals")] -use std::fs::File; use std::path::{Path, PathBuf}; use std::sync::Arc; @@ -52,7 +51,11 @@ pub struct ShardDrive { /// Held for the drive's lifetime. `reopen_through_recovery` takes the same /// lock, so a caller must drop the drive before reopening — which is the /// point: reopening is a close-and-reopen through recovery, not a peek. - _lock: File, + /// + /// A [`segment::RootLock`] rather than the `LOCK` file, so the release is + /// an explicit `LOCK_UN` and not a consequence of closing a descriptor a + /// concurrently forked child may still share. + _lock: segment::RootLock, journal: Journal, counters: Arc, /// Journal preallocation for this drive. Small by default so a crash diff --git a/crates/levcs-store/src/recovery.rs b/crates/levcs-store/src/recovery.rs index 9c2739e..6b62a8b 100644 --- a/crates/levcs-store/src/recovery.rs +++ b/crates/levcs-store/src/recovery.rs @@ -1504,7 +1504,11 @@ pub struct RecoverySession { layout: RootLayout, root_uuid: [u8; 16], shard_count: u16, - _lock: File, + /// The root lock, released explicitly when the session drops. Not a bare + /// `File`: `flock` lives on the open file description, so a concurrently + /// forked child that inherited this descriptor would keep the lock alive + /// past the close. See [`segment::RootLock`]. + _lock: segment::RootLock, } impl std::fmt::Debug for RecoverySession { diff --git a/crates/levcs-store/src/segment.rs b/crates/levcs-store/src/segment.rs index 9697aef..857d2c2 100644 --- a/crates/levcs-store/src/segment.rs +++ b/crates/levcs-store/src/segment.rs @@ -186,9 +186,73 @@ pub fn read_format(layout: &RootLayout) -> Result/LOCK`, released explicitly when dropped. +/// +/// # Why this is a guard and not a `File` +/// +/// `lock_root` used to return the open `File` and let the release fall out of +/// closing it. That is wrong across a concurrent `fork`. `flock` is held by the +/// **open file description**, not by the descriptor; a forked child inherits a +/// descriptor onto the same description, and `FD_CLOEXEC` closes it at `exec`, +/// not at `fork`. So for as long as any concurrently forked child has not yet +/// exec'd, the owner closing its descriptor releases nothing, and the next +/// `lock_root` on that root is refused `AlreadyLocked` — a refusal scope §3.1 +/// defines as final and never a wait, produced by a process that no longer +/// exists as far as the store is concerned. +/// +/// The fix is not to make closing more prompt. It is to stop making the +/// release a consequence of a descriptor lifetime that a process outside this +/// one can extend: [`RootLock::drop`] issues `LOCK_UN` *before* the descriptor +/// closes, which releases the description's lock regardless of who else holds +/// a descriptor onto it. +pub struct RootLock { + /// Dropped after `Drop::drop` runs, so the unlock always precedes the + /// close. + file: File, +} + +/// `LOCK_UN` failures observed in [`RootLock::drop`], where no error can be +/// returned. +/// +/// A `Drop` that swallows a failed release would turn this defect back into an +/// intermittent one, and panicking in `Drop` can abort during an unwind. So the +/// failure is counted instead: charter item 7, the same rule the durability +/// counters follow. A non-zero reading means some root lock outlived its owner +/// and the next `lock_root` on that root may be spuriously refused. +static ROOT_LOCK_RELEASE_FAILURES: std::sync::atomic::AtomicU64 = + std::sync::atomic::AtomicU64::new(0); + +impl RootLock { + /// How many times a [`RootLock`] failed to release in `Drop`. Zero in every + /// healthy run. + pub fn release_failures() -> u64 { + ROOT_LOCK_RELEASE_FAILURES.load(std::sync::atomic::Ordering::Relaxed) + } +} + +impl std::fmt::Debug for RootLock { + fn fmt(&self, formatter: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + formatter.debug_struct("RootLock").finish_non_exhaustive() + } +} + +impl Drop for RootLock { + fn drop(&mut self) { + match sys::unlock(&self.file) { + Ok(()) => {} + Err(_) => { + ROOT_LOCK_RELEASE_FAILURES.fetch_add(1, std::sync::atomic::Ordering::Relaxed); + } + } + } +} + /// Take the exclusive root lock. Failure is `AlreadyLocked`, never a wait /// (scope 3.1). -pub fn lock_root(layout: &RootLayout) -> Result { +/// +/// The returned [`RootLock`] releases explicitly when dropped; see its +/// documentation for why holding a bare `File` was not equivalent. +pub fn lock_root(layout: &RootLayout) -> Result { let file = File::options() .create(true) .read(true) @@ -196,7 +260,7 @@ pub fn lock_root(layout: &RootLayout) -> Result { .truncate(false) .open(layout.lock_path())?; if sys::try_lock_exclusive(&file)? { - Ok(file) + Ok(RootLock { file }) } else { Err(StoreError::AlreadyLocked) } @@ -944,3 +1008,126 @@ impl SegmentCache { Ok(reader) } } + +// --------------------------------------------------------------------------- +// Root-lock ownership tests +// --------------------------------------------------------------------------- + +#[cfg(test)] +mod root_lock_tests { + use super::*; + + /// A second holder is refused, never queued (scope 3.1). + #[test] + fn a_second_lock_on_a_held_root_is_refused() { + let dir = tempfile::tempdir().expect("temp root"); + let layout = RootLayout::new(dir.path()); + + let held = lock_root(&layout).expect("first lock"); + match lock_root(&layout) { + Ok(_) => panic!("a second holder took a lock that is already held"), + Err(StoreError::AlreadyLocked) => {} + Err(other) => panic!("expected AlreadyLocked, got {other:?}"), + } + drop(held); + } + + /// The regression this guard exists for. + /// + /// A child is forked while the root lock is held, so it inherits a + /// descriptor onto the *same open file description* — which is where + /// `flock` lives. The child is then held open, by a pipe handshake rather + /// than by a sleep, across the parent's release and reopen. So the window + /// this reproduces is a state, not a race: for the whole duration of the + /// parent's reopen the child is provably alive and provably holding the + /// inherited descriptor, because it has written its ready byte and has not + /// yet been told to exit. + /// + /// With [`RootLock`]'s explicit `LOCK_UN`, the reopen succeeds on its + /// **first attempt**. Remove that unlock and let the release fall out of + /// closing the descriptor — what `lock_root` did before this amendment — + /// and this test fails with `AlreadyLocked` every time, because the child's + /// descriptor keeps the description's lock alive. `FD_CLOEXEC` does not + /// help: it closes at `exec`, and this child never execs. + /// + /// One attempt, not a bounded retry: a spurious `AlreadyLocked` is a + /// refusal scope §3.1 defines as final, so the only correct assertion is + /// that the very first reopen succeeds. + #[test] + fn a_forked_child_holding_the_inherited_lock_descriptor_cannot_block_a_reopen() { + let dir = tempfile::tempdir().expect("temp root"); + let layout = RootLayout::new(dir.path()); + let failures_before = RootLock::release_failures(); + + let lock = lock_root(&layout).expect("first lock"); + + let mut ready = [-1i32; 2]; + let mut go = [-1i32; 2]; + // SAFETY: both arrays are two `c_int`s, which is what `pipe` writes. + assert_eq!(unsafe { libc::pipe(ready.as_mut_ptr()) }, 0, "ready pipe"); + // SAFETY: as above. + assert_eq!(unsafe { libc::pipe(go.as_mut_ptr()) }, 0, "go pipe"); + + // SAFETY: the child branch below calls only async-signal-safe + // functions and terminates with `_exit`, so forking a multi-threaded + // test process is sound here. + let child = unsafe { libc::fork() }; + assert!( + child >= 0, + "fork failed: {}", + std::io::Error::last_os_error() + ); + if child == 0 { + let mut byte = [1u8; 1]; + // SAFETY: async-signal-safe calls on inherited descriptors only. + unsafe { + libc::write(ready[1], byte.as_ptr().cast(), 1); + // Blocks until the parent has finished its reopen. The + // inherited LOCK descriptor stays open for exactly that long. + libc::read(go[0], byte.as_mut_ptr().cast(), 1); + libc::_exit(0); + } + } + + let mut byte = [0u8; 1]; + // SAFETY: reading one byte into a one-byte buffer. + let read = unsafe { libc::read(ready[0], byte.as_mut_ptr().cast(), 1) }; + assert_eq!( + read, 1, + "the child must be alive and holding the inherited LOCK descriptor \ + before the parent releases; without that this test proves nothing" + ); + + // The parent's release. Explicit unlock first, then the close. + drop(lock); + let reopened = lock_root(&layout); + + // Release the child only after the reopen has been attempted, so the + // inherited descriptor was open for the whole of it. + // SAFETY: writing one byte from a one-byte buffer. + unsafe { libc::write(go[1], byte.as_ptr().cast(), 1) }; + let mut status = 0i32; + // SAFETY: `status` is a valid `c_int` out-parameter. + unsafe { libc::waitpid(child, &mut status, 0) }; + for fd in [ready[0], ready[1], go[0], go[1]] { + // SAFETY: each descriptor was opened by `pipe` above and is closed once. + unsafe { libc::close(fd) }; + } + + match reopened { + Ok(_) => {} + Err(StoreError::AlreadyLocked) => panic!( + "reopen was refused AlreadyLocked on its first attempt while a forked child \ + still held the inherited LOCK descriptor. flock lives on the open file \ + description, so closing the parent's descriptor is not a release; RootLock \ + must issue LOCK_UN before the close." + ), + Err(other) => panic!("reopen failed for an unrelated reason: {other:?}"), + } + assert_eq!( + RootLock::release_failures(), + failures_before, + "a root lock failed to release in Drop" + ); + } +} diff --git a/crates/levcs-store/src/sys.rs b/crates/levcs-store/src/sys.rs index 0b77e02..8539c7d 100644 --- a/crates/levcs-store/src/sys.rs +++ b/crates/levcs-store/src/sys.rs @@ -412,6 +412,23 @@ pub(crate) fn try_lock_exclusive(file: &File) -> io::Result { } } +/// Release a `flock` held on `file`'s **open file description**. +/// +/// This is not redundant with closing the descriptor. `flock` is a property of +/// the open file description, not of the descriptor: any descriptor that +/// shares the description keeps the lock alive, and `fork` hands the child +/// exactly such a descriptor. `FD_CLOEXEC` closes it at `exec`, not at `fork`, +/// so between a concurrent fork and that child's `exec` — or for as long as a +/// forked child runs without exec'ing — the parent closing its own descriptor +/// releases nothing. Releasing must therefore be an explicit act. +/// +/// Locking already lives in this module; unlocking stays beside it so the +/// durability funnel remains the only place that issues the syscall. +pub(crate) fn unlock(file: &File) -> io::Result<()> { + rustix::fs::flock(file, rustix::fs::FlockOperation::Unlock) + .map_err(|e| io::Error::from_raw_os_error(e.raw_os_error())) +} + /// Read the file position without moving it. pub(crate) fn cursor(file: &mut File) -> io::Result { file.stream_position() diff --git a/doc/instance-throughput-rewrite-plan.md b/doc/instance-throughput-rewrite-plan.md index 2b8f077..b11fa08 100644 --- a/doc/instance-throughput-rewrite-plan.md +++ b/doc/instance-throughput-rewrite-plan.md @@ -1102,6 +1102,78 @@ is B3's file and folds into B3's fix pass. The gate is transiently red between t passes, which is accepted: landing the contract first is what keeps both packages from implementing against a surface that is about to move. +##### Contract review 2026-07-28-B + +B4's crash harness found that an immediate reopen after `StoreEngine` is dropped +intermittently fails `AlreadyLocked` — up to 29 retries over about 147 ms, seven failures in +forty uninstrumented runs, on a single-shard engine. It compensated with a bounded, measured +wait and filed the finding rather than fixing a file it does not own, which is what §6.0 +asks for. The finding is granted, and the fix lands in the frozen root-lock primitive +(`segment.rs`, `sys.rs`) rather than in `StoreEngine::drop`. + +**What was actually wrong.** Not `Drop` ordering. `StoreEngine::drop` closes every shard +channel, joins every writer, and leaves no extra `Arc` behind; the refusal +comes from `segment::lock_root`'s filesystem lock on `/LOCK`, not from B3's +process-wide staging guard, which returns `Conflict`. **An `flock` is held by the open file +description, not by the file descriptor.** A concurrently forked child inherits a descriptor +onto the parent's open descriptions, and `FD_CLOEXEC` closes it at `exec`, not at `fork`. So +for the whole fork-to-exec window — and for as long as any forked child runs without +exec'ing — the parent closing its `LOCK` descriptor releases nothing. Reproduced +deterministically: an explicit `LOCK_UN` before the close makes an immediate reopen succeed +*while the child still holds its inherited descriptor*. + +**The amendment.** `segment::lock_root` returns `RootLock` instead of `File`. `RootLock` +issues `sys::unlock` — a new `LOCK_UN` beside the existing `try_lock_exclusive`, so the +syscall stays inside the durability funnel — in `Drop`, before its `File` field closes. +`Drop` is deliberately the *only* release path: an explicit `release()` returning the +`LOCK_UN` error would have been a second way to do the same thing with no caller, which +charter items 8 and 9 treat as a decoy rather than a safeguard. Both owners +hold the guard: `recovery::RecoverySession` (which `engine.rs` retains for the engine's +lifetime, so `StoreEngine` inherits the fix without an edit) and `drive::ShardDrive`. A +`LOCK_UN` that fails in `Drop` cannot be returned and must not be swallowed, so it is +counted in `RootLock::release_failures()` — charter item 7 applied to the one place where a +claim would otherwise be all there is. + +**The weaker alternative, rejected: fix it only in `StoreEngine::drop`.** It would have +turned the harness green. It leaves every other `lock_root` caller — `ShardDrive`, the +one-shot `RecoverySession` in the drive's reopen path, and whatever Phase 2's migrator +opens — holding a lock whose release is still a *consequence of a descriptor lifetime that a +process outside this one can extend*. That is the property worth stating plainly: the lock +lives on the open file description, so releasing it has to be an explicit act, and no amount +of tightening the shutdown sequence changes who else holds a descriptor onto that +description. A drop-ordering fix would also have looked correct in every single-process +test, because the defect needs a concurrent fork to appear at all — which is exactly why it +presented as a flake at one run in six and not as a failure. + +What would have gone wrong: a consumer that closes a store and reopens it — a recovery +drill, an in-place restart, the Phase 2 migrator, any of which may spawn a subprocess — gets +`AlreadyLocked`, which scope §3.1 defines as a refusal and never a wait. There is no defined +retry, so the correct consumer behaviour on that error is to give up. The store would have +been refusing itself, from a process that no longer exists, and reporting a state +indistinguishable from a genuine second owner. + +**Regression evidence.** `segment.rs` gains a synchronized fork-inheritance test: the child +is forked while the lock is held, signals readiness over a pipe, and blocks until the parent +has completed its release *and its reopen*, so the inherited descriptor is provably open +across the whole window. Nothing in it is timed. It passed 40/40 as landed; with the +explicit unlock removed it failed 10/10 with `reopen was refused AlreadyLocked on its first +attempt`. §5's Wave A record is why this had to be synchronized rather than raced: an +intermittently failing fault test gets rerun until green, at which point a real regression +and a flake are indistinguishable. + +**Measured, not asserted.** 200 close-and-immediately-reopen cycles through +`StoreEngine::open`, concurrent with four threads spawning subprocesses (72,936 fork/execs +during the run): `refusals=0`, `worst_tries=1`, `worst_elapsed=10.4ms`, against B4's +reported `tries=29 elapsed=146ms`. The same measurement on the pre-fix release behaviour +fails with `AlreadyLocked` within the first cycles under that load. + +Amended: `segment.rs` (`RootLock`, `lock_root`'s return type, the fork regression test), +`sys.rs` (`unlock`), `recovery.rs` and `drive.rs` (both owners hold the guard). `engine.rs` +required no change and none was made: it holds a `RecoverySession`, never a `File`, so no +interface request against B1 arises from this. Scope §6.6 records the consequence B4 must +land: the bounded retry at `tests/support/engine_matrix.rs:516` is now compensating for a +defect that no longer exists and must become a one-attempt assertion. + ### Phase 1 — storage engine spine Lead first defines sealed transaction/frame/snapshot interfaces and file ownership. That deliverable (D0) landed on 2026-07-24 as `crates/levcs-store`: the frozen public API compiling against `StoreError::NotImplemented`, the file-ownership split, strict configuration validation, the single durability syscall funnel with its counters and fault hooks, the failpoint registry in enforced one-to-one correspondence with `oracle::AppendFailpoint`, and the journal-level drive seam that lets the crash harness run in Wave A. The enforced gate is `scripts/check-phase1.sh`, which runs `check-phase0.sh` first so the Phase 0 freeze stays enforced. That work is scoped in `doc/phase1-storage-spine-scope.md`, which realizes this section as a file-ownership matrix, a frozen `levcs-store` API, a physical format and durability/recovery specification, per-package deliverables and acceptance criteria, the Wave A adversarial review charter, and the capacity analysis for P2 on the frozen reference hardware. This plan remains authoritative; that document is the Phase 1 realization of it and lists the decisions that must be resolved before Wave A starts. diff --git a/doc/phase1-storage-spine-scope.md b/doc/phase1-storage-spine-scope.md index 2bad7a6..98add60 100644 --- a/doc/phase1-storage-spine-scope.md +++ b/doc/phase1-storage-spine-scope.md @@ -1768,6 +1768,55 @@ Owns the crash driver, the benchmark, the matrix, and the recovery script. re-examined. If any becomes genuinely earnable, that is a schema amendment and a contract review — **request it, do not emit it.** `bench/result-schema.json` is lead-owned. +#### Carry-forward: the SIGKILL cycles still drive the journal seam + +Recorded here because until now it existed only as a comment in the harness, and **a harness +comment is not scope evidence**. It is a limitation on a §7 exit criterion, so it belongs +where the exit criteria are read. + +`scripts/verify-store-recovery.sh`'s 100 randomized `SIGKILL` cycles drive the **journal +seam** (`drive.rs`), not `StoreEngine::submit`. So `kill -9` and the driver's `_exit(3)` +never land inside a real publication: they land inside a frame append and fence, which is +one step of a publication and not the step where the status root, the sequencer, the +acknowledgment, and the checkpoint install are at risk. Every ordering hazard that only +exists between those is untested by this script, at any cycle count. + +Moving the cycles onto production submit is **blocked on `StoreEngine::open` startup state +1**: the child process cannot create a store root through the production entry point, which +still refuses that state by name (B1 deliverable 1). The same block is what forces +`engine_matrix.rs` to seed roots through `segment::initialize_root`, and it is disclosed +there as `ROOT_SEEDED_BY_NON_PRODUCTION_PATH`. + +Consequences, stated so no later reader has to reconstruct them: + +- The **acknowledged-crash-recovery exit criterion (§7) is only partly earned.** What is + earned is that the journal survives abrupt termination and recovery adopts exactly the + fenced prefix. What is not earned is that an *acknowledgment* survives a kill inside the + publication that produced it. +- The figures the script reports — `recovery_failures=0`, `acknowledged_loss=0`, + `torn_transactions=0`, `repeated_adoptions=0` — mean what **Wave A** meant by them, over + the seam Wave A had. They are not a real-engine soak result and must not be quoted as one. +- Closing this is B4 work once startup state 1 lands, and it is a re-point of an existing + script rather than a new harness. Expect the numbers to change; a change is the + measurement working. + +#### Consequence of contract review 2026-07-28-B: delete the reopen wait + +`segment::lock_root` now returns a `RootLock` guard that releases the root lock with an +explicit `LOCK_UN` before closing the descriptor, which fixes the spurious `AlreadyLocked` +B4 measured after `StoreEngine` is dropped (the lock lived on the open file description, and +a concurrently forked child kept it alive past the close). The full record is in +`doc/instance-throughput-rewrite-plan.md`. + +B4's bounded retry at `tests/support/engine_matrix.rs:516` (`reopen_after_close`) must +therefore be replaced by a **one-attempt immediate-reopen assertion**: a single +`StoreEngine::open` that must succeed, with no budget, no sleep, and no `attempts` field to +publish. The wait was correct while the defect stood and is documented as compensating for +it; it exists only for that reason and must not outlive it. Leaving it in place would hide a +recurrence of exactly this defect behind a wait that succeeds on the second try — and §3.1 +says `AlreadyLocked` is a refusal and never a wait, so a harness that waits on it is +asserting something the store does not promise. + ### 6.7 B2 — StorageReviewer Read-only durability, concurrency, and security review of the whole crate. Same charter as