diff --git a/crates/levcs-store/src/segment.rs b/crates/levcs-store/src/segment.rs index d057aeb..9cd8212 100644 --- a/crates/levcs-store/src/segment.rs +++ b/crates/levcs-store/src/segment.rs @@ -1053,6 +1053,18 @@ mod root_lock_tests { /// believes is exclusive ownership of one root. That is the scope 3.1 /// property every single-writer argument above this layer depends on. /// + /// # What this proves, and what it does not + /// + /// It proves that a `LOCK` whose name resolves to something other than a + /// regular file cannot be locked, which closes case 1 of the two in scope + /// 3.1. It is **not** proof of exclusion against *replacement*: planting a + /// fresh regular file here instead of a symlink still produces a second + /// holder, because the second caller opens and locks a new, unlocked inode + /// and no check at this layer can distinguish that from the first open. + /// [`replacing_the_lock_file_still_admits_a_second_holder`] pins that limit + /// deliberately. The lock method here is what is under test; the planting is + /// only how a wrong-typed name is arranged. + /// /// The first lock is taken before the link is planted, and is still held /// when the second is attempted, so the arrangement is a state and not a /// race. Against the previous open this test fails by taking the second @@ -1078,14 +1090,50 @@ mod root_lock_tests { } Ok(_) => panic!( "a second caller took the root lock while the first still held it, because \ - the symlink at LOCK sent its flock to a foreign inode. Two processes now \ - own one root and scope 3.1 exclusion no longer holds." + the symlink at LOCK sent its flock to a foreign inode. A name that does not \ + resolve to a regular file must be refused before flock." ), Err(other) => panic!("expected UnrecognizedLayout, got {other:?}"), } drop(held); } + /// The limit of what a lock on a file inside the root can give, pinned so it + /// cannot be mistaken for a property. + /// + /// Replacing `LOCK` with a fresh **regular** file while a holder holds it + /// produces a second holder. The no-follow open cannot help: both opens are + /// of a regular file at exactly the right name, and the only difference is + /// which inode the name resolved to, which the second caller has no way to + /// know was ever different. This is true of any regular file at any name. + /// + /// Scope 3.1 therefore states the assumption explicitly — no noncooperating + /// mutation of the root directory's entries while the root is held — and this + /// test is the machine-readable form of it. It asserts the *current* behavior + /// on purpose: if a stable locking object is ever adopted (§3.1 names locking + /// the root directory as the candidate), this test is expected to fail, and + /// that failure is the signal that the assumption changed. + #[test] + fn replacing_the_lock_file_still_admits_a_second_holder() { + let dir = tempfile::tempdir().expect("temp root"); + let layout = RootLayout::new(dir.path()); + let held = lock_root(&layout).expect("the first holder"); + + std::fs::remove_file(layout.lock_path()).expect("unlink the locked name"); + let second = lock_root(&layout); + + match second { + Ok(_) => {} + Err(other) => panic!( + "a stable locking object appears to have been adopted, or lock_root changed: a \ + replaced LOCK was refused with {other:?}. If that is intended, scope 3.1's \ + assumption about noncooperating mutation of the root directory is now \ + stronger than documented and both should be updated together." + ), + } + drop(held); + } + /// The same open, in its destructive form: a *dangling* link at `LOCK` and /// `create(true)` brings a file into being outside the root. #[test] @@ -1126,6 +1174,12 @@ mod root_lock_tests { "directory", (|path: &Path| std::fs::create_dir(path).expect("mkdir")) as fn(&Path), ), + // Refused by `open(2)` itself with `ENXIO`, so it never reaches the + // `fstat` — which is why the primitive maps that errno rather than + // letting it surface as an `Io` the caller would have to know about. + ("unix socket", |path: &Path| { + std::os::unix::net::UnixListener::bind(path).expect("bind"); + }), ("fifo", |path: &Path| { let name = std::ffi::CString::new(path.as_os_str().as_encoded_bytes()) .expect("a path with no interior NUL"); diff --git a/crates/levcs-store/src/sys.rs b/crates/levcs-store/src/sys.rs index 925b5a7..41e4d0e 100644 --- a/crates/levcs-store/src/sys.rs +++ b/crates/levcs-store/src/sys.rs @@ -441,8 +441,14 @@ pub(crate) fn create_new_nofollow(path: &Path) -> io::Result> { /// arriving at the same root takes it too, and both hold what each believes is /// exclusive ownership. That defeats the exclusion scope 3.1 is built on, which /// no amount of care at the `flock` call itself can restore. `O_NOFOLLOW` plus -/// this `fstat` is what makes the locked inode provably the one the caller -/// named. +/// this `fstat` is what makes the locked inode the one the caller's *name* +/// resolved to. +/// +/// It does not make the binding permanent, and scope 3.1 is explicit about the +/// difference: a name replaced with a fresh regular file while a holder holds it +/// still yields a second holder, because both opens are then of a regular file at +/// exactly the right name. That case is an assumption about the deployment, not a +/// property this function can supply. /// /// One `O_CREAT` open, not an `O_EXCL` create followed by a plain open on /// `EEXIST`: the single call has no window between deciding the name is taken @@ -470,8 +476,14 @@ pub(crate) fn open_or_create_regular_nofollow(path: &Path) -> io::Result return Ok(None), - // A directory refuses `O_RDWR` before any type check runs. - Err(rustix::io::Errno::ISDIR) => return Ok(None), + // A directory refuses `O_RDWR` before any type check runs, and a Unix + // socket refuses `open(2)` with `ENXIO` — so neither ever reaches the + // `fstat`. Both are mapped here rather than left to surface as `Io`, + // because "the name is occupied by something that is not a regular file" + // is precisely what they mean, and a caller that has to handle that + // answer in two shapes will handle one of them wrongly. `ENXIO` on a + // deviceless device node says the same thing. + Err(rustix::io::Errno::ISDIR) | Err(rustix::io::Errno::NXIO) => return Ok(None), Err(e) => return Err(io::Error::from_raw_os_error(e.raw_os_error())), }; let file = File::from(fd); diff --git a/crates/levcs-store/tests/d0_contract.rs b/crates/levcs-store/tests/d0_contract.rs index 8215bf0..4407ac5 100644 --- a/crates/levcs-store/tests/d0_contract.rs +++ b/crates/levcs-store/tests/d0_contract.rs @@ -262,6 +262,137 @@ fn privileged_construction_is_not_reachable_from_an_engine() { } } +/// Calls the durability funnel exists to intercept. +/// +/// Writes are here for a reason found in review: `checkpoint.rs` used +/// `File::write_all` directly, so checkpoint bytes and short writes were +/// invisible to `DurabilityCounters` and the ENOSPC / short-write / cursor fault +/// seam could not reach checkpoint installation at all. The original guard +/// scanned only sync, rename, and unlink, so it passed. A funnel that covers +/// durability but not the writes being made durable is not a funnel. +const FORBIDDEN: &[&str] = &[ + "sync_all(", + "sync_data(", + "std::fs::rename(", + "std::fs::remove_file(", + "fs::rename(", + "fs::remove_file(", + ".write_all(", + ".write_vectored(", + ".set_len(", + "std::fs::write(", + "fs::write(", +]; + +/// Scan one file's text for [`FORBIDDEN`] calls outside test-only code. +/// +/// `Err` is a scanner failure and fails the guard exactly as an offender does: +/// a shape it cannot reason about is not a shape it may assume is safe. Split +/// out of the test so the scanner itself is testable on synthetic input — +/// mutating real sources only ever probes the shapes those sources happen to +/// contain, which is how the two defects below survived. +/// +/// # How test-only code is exempted, and why not by name +/// +/// The trigger is the `#[cfg(test)]` attribute at column zero — the thing that +/// actually removes code from a release build. An earlier version matched the +/// literal `mod tests`, which exempted only modules that happen to be called +/// that (`segment.rs`'s `root_lock_tests` and `recovery.rs`'s +/// `production_session_tests` were scanned as production code) while letting a +/// file evade the guard entirely by naming a module `tests`. +/// +/// The exemption is then bounded by the **shape of the attributed item**, which +/// is the second defect. Ending it at the next column-zero `}` is right for a +/// braced item and wrong for anything else: after +/// +/// ```text +/// #[cfg(test)] +/// use crate::test_support; +/// +/// fn shipping_code() { +/// std::fs::write(..); +/// } +/// ``` +/// +/// the first column-zero `}` is *`shipping_code`'s*, so every line of it was +/// skipped. A semicolon-terminated item therefore exempts only itself, and any +/// third shape — including an item header rustfmt has split across lines — is a +/// scanner error rather than a guess. +fn scan_for_unfunnelled_calls(text: &str) -> Result, String> { + #[derive(PartialEq)] + enum Exempt { + No, + /// Until the item's closing brace at column zero. + UntilUnindentedBrace, + } + + let lines: Vec<&str> = text.lines().collect(); + let mut offenders = Vec::new(); + let mut exempt = Exempt::No; + let mut index = 0; + while index < lines.len() { + let line = lines[index]; + let code = line.trim_start(); + + if exempt == Exempt::UntilUnindentedBrace { + if line == "}" { + exempt = Exempt::No; + } + index += 1; + continue; + } + + if line.starts_with("#[cfg(test)]") { + // The item this attribute applies to, past any further attributes, + // doc comments and blank lines. + let mut head = index + 1; + while head < lines.len() { + let candidate = lines[head].trim_start(); + if candidate.is_empty() || candidate.starts_with('#') || candidate.starts_with("//") + { + head += 1; + continue; + } + break; + } + let Some(item) = lines.get(head).map(|l| l.trim_end()) else { + return Err(format!( + "line {}: `#[cfg(test)]` with no item after it", + index + 1 + )); + }; + if item.ends_with('{') { + exempt = Exempt::UntilUnindentedBrace; + index = head + 1; + continue; + } + if item.ends_with(';') { + // Only the item itself. Whatever follows is production code + // until something says otherwise. + index = head + 1; + continue; + } + return Err(format!( + "line {}: the scanner cannot bound a `#[cfg(test)]` item of this shape, so it \ + cannot tell where the exemption ends: `{}`. Keep the item header on one line, \ + or teach the scanner the shape — do not leave it guessing.", + head + 1, + item.trim() + )); + } + + if !code.starts_with("//") { + for needle in FORBIDDEN { + if code.contains(needle) { + offenders.push((index + 1, line.trim().to_string())); + } + } + } + index += 1; + } + Ok(offenders) +} + /// Nothing outside `sys.rs` may call a durability syscall directly. /// /// The counters are what turn "exactly one fence per group" and "no per-object @@ -269,26 +400,6 @@ fn privileged_construction_is_not_reachable_from_an_engine() { /// that bypasses the funnel is invisible to them (scope 2.3). #[test] fn durability_syscalls_go_only_through_the_sys_funnel() { - // Writes are here for a reason found in review: `checkpoint.rs` used - // `File::write_all` directly, so checkpoint bytes and short writes were - // invisible to `DurabilityCounters` and the ENOSPC / short-write / cursor - // fault seam could not reach checkpoint installation at all. The original - // guard scanned only sync, rename, and unlink, so it passed. A funnel that - // covers durability but not the writes being made durable is not a funnel. - const FORBIDDEN: &[&str] = &[ - "sync_all(", - "sync_data(", - "std::fs::rename(", - "std::fs::remove_file(", - "fs::rename(", - "fs::remove_file(", - ".write_all(", - ".write_vectored(", - ".set_len(", - "std::fs::write(", - "fs::write(", - ]; - let mut offenders = Vec::new(); for path in rust_sources(&crate_src()) { // `sys.rs` is the funnel itself. `src/bin/**` are harness binaries that @@ -300,41 +411,13 @@ fn durability_syscalls_go_only_through_the_sys_funnel() { continue; } let text = std::fs::read_to_string(&path).expect("read source"); - // Test-only code is exempt, and how that is decided matters twice over. - // - // The trigger is the `#[cfg(test)]` attribute at column zero — the thing - // that actually removes the code from a release build — and not the - // module's *name*. Matching `mod tests` exempted only modules that - // happen to be called that, so `segment.rs`'s `root_lock_tests` and - // `recovery.rs`'s `production_session_tests` were scanned as production - // code, while a file could equally have evaded the guard by naming a - // module `tests` and putting real code in it. - // - // The exemption also *ends*, at the next column-zero `}`. Latching it on - // for the rest of the file meant anything appended after a test module - // was unscanned — the one place a durability call is least likely to be - // noticed. rustfmt puts every top-level item's closing brace at column - // zero, so that boundary is mechanical here. - let mut in_test_item = false; - for (n, line) in text.lines().enumerate() { - let code = line.trim_start(); - if line.starts_with("#[cfg(test)]") { - in_test_item = true; - } - if in_test_item { - if line == "}" { - in_test_item = false; - } - continue; - } - if code.starts_with("//") { - continue; - } - for needle in FORBIDDEN { - if code.contains(needle) { - offenders.push(format!("{}:{}: {}", path.display(), n + 1, line.trim())); - } - } + match scan_for_unfunnelled_calls(&text) { + Ok(found) => offenders.extend( + found + .into_iter() + .map(|(line, text)| format!("{}:{line}: {text}", path.display())), + ), + Err(reason) => panic!("{}: {reason}", path.display()), } } // `std::fs::remove_file(` and `fs::remove_file(` both match one line, so an @@ -350,6 +433,109 @@ fn durability_syscalls_go_only_through_the_sys_funnel() { ); } +/// The scanner above, against the shapes it has to get right. +/// +/// Synthetic input rather than mutated sources. Mutating `segment.rs` proves the +/// scanner works on the shapes `segment.rs` happens to contain, which is exactly +/// how both of its exemption defects survived a mutation check: no file in the +/// crate currently has a semicolon-terminated `#[cfg(test)]` item followed by +/// production code, so no mutation of a real file could produce one. +#[test] +fn the_funnel_scanner_bounds_a_test_exemption_by_the_shape_of_the_item() { + let lines = |found: Vec<(usize, String)>| -> Vec { + let mut out: Vec = found.into_iter().map(|(line, _)| line).collect(); + out.dedup(); + out + }; + + // Production code is found. + assert_eq!( + lines( + scan_for_unfunnelled_calls("fn ship() {\n std::fs::write(p, b\"\");\n}\n") + .expect("a plain file scans") + ), + vec![2] + ); + + // A braced test item is exempt, and the exemption ends with it. + let braced = "\ +#[cfg(test)] +mod named_anything { + fn setup() { + std::fs::write(p, b\"\"); + } +} + +fn ship() { + std::fs::remove_file(p); +} +"; + assert_eq!( + lines(scan_for_unfunnelled_calls(braced).expect("a braced item scans")), + vec![9], + "the call inside the test module must be exempt and the one after it must not" + ); + + // The defect this shape check exists for: a semicolon-terminated test item + // exempts itself and nothing else. Under the previous rule the first + // column-zero `}` was `ship`'s, so the whole function was skipped. + let terminated = "\ +#[cfg(test)] +use crate::test_support; + +fn ship() { + std::fs::write(p, b\"\"); +} +"; + assert_eq!( + lines(scan_for_unfunnelled_calls(terminated).expect("a semicolon-terminated item scans")), + vec![5], + "a `#[cfg(test)] use ...;` must not exempt the function that follows it" + ); + + // Stacked attributes and doc comments between the trigger and the item. + let stacked = "\ +#[cfg(test)] +#[allow(dead_code)] +/// A helper. +mod support { + fn setup() { + std::fs::write(p, b\"\"); + } +} +"; + assert!( + lines(scan_for_unfunnelled_calls(stacked).expect("stacked attributes scan")).is_empty(), + "further attributes and doc comments must not hide the item's shape" + ); + + // A shape the scanner cannot bound is a failure, not an assumption. A + // multi-line item header is the realistic way to produce one. + let unsupported = "\ +#[cfg(test)] +fn helper( + argument: usize, +) { + std::fs::write(p, b\"\"); +} +"; + let reason = scan_for_unfunnelled_calls(unsupported) + .expect_err("an unbounded exemption must fail the guard rather than be guessed at"); + assert!(reason.contains("cannot bound"), "{reason}"); + + // And a trailing attribute with nothing after it. + assert!(scan_for_unfunnelled_calls("#[cfg(test)]\n") + .expect_err("a dangling attribute is a scanner failure") + .contains("no item after it")); + + // Comments are not code, in either position. + assert!(lines( + scan_for_unfunnelled_calls("fn ship() {\n // std::fs::write(p, b\"\");\n}\n") + .expect("a commented call scans") + ) + .is_empty()); +} + /// `StoreError` is for inability to answer; `TransactionStatus` is for every /// state the store can actually report. Plan §5.1 makes the split normative. /// diff --git a/doc/instance-throughput-rewrite-plan.md b/doc/instance-throughput-rewrite-plan.md index c0605d4..14e5d19 100644 --- a/doc/instance-throughput-rewrite-plan.md +++ b/doc/instance-throughput-rewrite-plan.md @@ -1241,8 +1241,8 @@ through. A guarantee that holds only for one of three entry points is not a guar Measured on a reverted copy, all three distinct failures observed rather than predicted: -- **Two owners of one root.** First lock taken and *still held*; `LOCK` then replaced with a link to - an unlocked file elsewhere; the second `lock_root` returned `Ok`. A state, not a race. +- **A wrong-typed name is locked.** First lock taken and *still held*; `LOCK` then replaced with a + link to an unlocked file elsewhere; the second `lock_root` returned `Ok`. A state, not a race. - **A dangling link at `LOCK` created a file outside the root**, because `create(true)` through an unresolved link is a create at the target. - **A fifo at `LOCK` returned `Ok(RootLock)`** — the store reported holding the root lock on a pipe. @@ -1264,6 +1264,46 @@ The remaining three hazards from 2026-07-28-D stand unchanged and unfixed: `writ one primitive away from a fix, but each needs its own refusal semantics decided, and none of them breaks an exclusion property. +###### Amended after review, 2026-07-29 + +Two findings against the above, both upheld. + +**The two-owner claim was overstated, and the correction is a scope amendment rather than a code +change.** The regression arranges its wrong-typed name by *replacing* `LOCK` while the first holder +holds it — and replacing it with a fresh **regular** file succeeds just as well, since both opens +are then of a regular file at exactly the right name and nothing distinguishes the second from the +first. So the type check closes "the name already resolves to the wrong kind of object", which is +the operator-error and stale-state case, and does **not** close "the name is replaced under a +holder". Scope 3.1 now states the two cases apart, says which one is in scope, and states the +replacement case as an explicit deployment assumption: anything able to replace `LOCK` can equally +unlink a journal, so advisory locking was never the boundary that would stop it. The assumption is +pinned by `replacing_the_lock_file_still_admits_a_second_holder`, which asserts the *current* +behavior deliberately — if a stable locking object is ever adopted, that test is expected to fail, +and the failure is the signal that the documented assumption changed. §3.1 records locking the root +**directory** as the candidate, and what it would cost: a frozen-surface change, and the removal of +the "directory holding only `LOCK` is empty" special case that startup state 1 depends on. + +**The funnel guard's exemption could still latch, and the fix is a scanner that refuses to guess.** +Bounding a `#[cfg(test)]` exemption at the next column-zero `}` is right for a braced item and wrong +for every other shape: after `#[cfg(test)] use crate::test_support;` the first such brace belongs to +the *next* function, so all of it was skipped. The scanner now reads the shape of the attributed +item — braced items are exempt to their closing brace, semicolon-terminated items exempt only +themselves, and any third shape (including an item header rustfmt split across lines) **fails the +guard** rather than being assumed safe. A shape the scanner cannot bound is not a shape it may +treat as harmless. + +It is also now a function over `&str` with synthetic tests, which is the load-bearing half. Mutating +real sources only ever probes the shapes those sources happen to contain: no file in the crate has a +semicolon-terminated `#[cfg(test)]` item followed by production code, so no mutation of a real file +could have produced this defect. That is a general lesson about source-scanning guards and belongs +with the charter's item 8 — assert against the path that runs — as its analogue for tooling. + +**One promise made exact rather than caveated.** A Unix socket at the name fails `open(2)` with +`ENXIO` before any `fstat`, so it surfaced as `Io` while the documentation promised +`UnrecognizedLayout`. `ENXIO` and `EISDIR` are both mapped to "not a regular file", which is what +they mean here, and the socket case is now one of four occupants the test loop covers. Safety was +never affected; the type of the refusal was. + ##### Contract review 2026-07-28-C B4 re-pointed `store-bench` at a real `StoreEngine::submit` and found that four verification diff --git a/doc/phase1-storage-spine-scope.md b/doc/phase1-storage-spine-scope.md index b36cd77..81c1349 100644 --- a/doc/phase1-storage-spine-scope.md +++ b/doc/phase1-storage-spine-scope.md @@ -408,6 +408,40 @@ created_at_micros, checksum }`, written then directory-synced. `root_uuid` binds `LOCK` is held with `flock(LOCK_EX|LOCK_NB)` for process lifetime. Failure is `AlreadyLocked`, never a wait. +**What the root lock does and does not exclude** (contract review 2026-07-29-A, second +finding). `flock` locks an *inode*. The name `/LOCK` resolves to an inode once, at +open, and nothing binds the name to that inode afterwards — so exclusion holds exactly as +long as the name keeps resolving to the object the holder locked. + +Two cases follow, and only one of them is a defect the store can close: + +1. **The name already resolves elsewhere when a process arrives.** A symlink, a fifo, a + directory, a socket, or a device at `LOCK` — left by an operator, a restored backup, a + symlink farm, or a previous tenant of the directory. A follow-through open takes the + lock on a foreign inode, or on no inode the store owns, while the root itself stays + unlocked. **In scope and closed**: `segment::lock_root` opens through + `sys::open_or_create_regular_nofollow` and refuses anything that is not a regular file + at exactly that name, before `flock`. +2. **The name is replaced while a holder holds it.** `unlink` plus `create`, or a `rename` + over it, gives the next arrival a fresh unlocked inode and a second holder — and this is + true of *any* regular file at any name, so the type check does not address it and no + check at this layer can. **Out of scope, and stated here as an assumption rather than + left implied**: scope 3.1 exclusion assumes no noncooperating mutation of the root + directory's entries while the root is held. The assumption is sound for the deployment + this store is built for — anything able to replace `LOCK` can equally unlink a journal or + a manifest, so advisory locking was never the boundary that would stop it — but it is an + assumption, not a property, and a test in `segment.rs` pins it so it cannot quietly be + believed to be stronger than it is. + +The strongest available strengthening, if case 2 is ever brought into scope, is to hold the +lock on the **root directory** rather than on a file inside it: `rename` over a non-empty +directory fails `ENOTEMPTY`, so an adversary must move the whole root aside, after which +every subsequent path resolves into a different tree and the failure stops resembling +successful exclusion. It is not free — it changes a frozen surface, and it removes the +"directory holding only `LOCK` is empty" special case that startup state 1 currently needs, +which is a simplification but a behavioral change to `classify_root` and its tests. Not +scheduled here. + Startup states (§5.2), decided in this order and with no inference: 1. Path absent, or present and empty → initialize v2. Create the tree, write `FORMAT`,