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