From befde3b2e277451f778692dd5e8587ec74008cb8 Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 16 Aug 2026 02:58:11 +0000 Subject: [PATCH] fix(cockpit-server): skip the full slab read entirely on a hot, unchanged boot Both prior fixes (#135, #136) stopped the OSM slab's warm-check paths from LEAVING the slab resident in the page cache after reading it, but neither stopped the read itself: ensure_lance_local's FNV-1a freshness digest and ensure_slab_local's SHA-256 cache-hit verify both still mmap/stream the entire ~1.4-3.75 GB slab on EVERY boot with a warm volume, even when nothing has changed. That's real wall-clock time and real (if transient) page-cache pressure -- the remaining cause of the reported 20-30 minute RAM decline after a redeploy. Adds a local "hot but idle" fast path to both: trust filesystem identity (mtime + length) recorded at the last real verification, distrust it on any mismatch. Any write to a file updates its mtime, so (mtime, len) unchanged since the last verification is conclusive proof the bytes are unchanged too -- the same heuristic make/rsync/cargo/ccache use by default. - osm_lance.rs: two new Lance schema-metadata keys (soa:slab_mtime_nanos, soa:slab_len) alongside the existing soa:slab_digest. A dataset whose stored identity matches the current slab's resolves warm without ever mmapping or hashing it. - osm_slab_hydrate.rs: a .verified sidecar recording (mtime, len, digest) after every real verification. A matching marker skips sha256_file entirely. Any mismatch (touched mtime, missing/malformed marker, a bucket republish naming a different digest) falls straight through to the existing, unchanged, always-correct hash-based path -- pure addition, no removed correctness. This is deliberately NOT osm_lifecycle.rs's Phase C/D ImportSeal/warm_verification_reads machinery (an S3-origin-proof design gated behind wiring that doesn't exist yet); it's a narrower, local-only mechanism for a narrower problem, documented as such in the plan doc so the two aren't confused. TDD: reachability counters (SLOW_PATH_HASH_ATTEMPTED; the existing FADVISE_ATTEMPTED), same discipline as #135/#136 -- RSS/cgroup accounting can't see "skipped a read" any more than it could see "evicted after one". One pre-existing test from #135 updated to touch mtime so it keeps exercising the digest-based path it was written for, now that the simpler scenario it used is correctly served by the new fast path instead. 173/173 tests (11 new), 0 regressions. Clippy content byte-identical to the pre-change tree via git stash comparison (68/0 both sides). Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_01NMeiLmtDKhomJNSo2ecbJw --- .../plans/2026-08-15-osm-lance-lifecycle.md | 104 ++++++ crates/cockpit-server/src/osm_lance.rs | 315 +++++++++++++++-- crates/cockpit-server/src/osm_slab_hydrate.rs | 322 +++++++++++++++++- 3 files changed, 709 insertions(+), 32 deletions(-) diff --git a/claude-notes/plans/2026-08-15-osm-lance-lifecycle.md b/claude-notes/plans/2026-08-15-osm-lance-lifecycle.md index 4c1e7679f..552b0762a 100644 --- a/claude-notes/plans/2026-08-15-osm-lance-lifecycle.md +++ b/claude-notes/plans/2026-08-15-osm-lance-lifecycle.md @@ -547,3 +547,107 @@ afterward. The "hot but idle" design (a bake-time sidecar digest a warm boot can check without touching slab bytes at all) remains the real fix for the read cost itself, and remains a cross-repo change not attempted this session. + +## "Hot but idle" — actually skipping the read, local-only (this session, follow-up) + +**Operator observation, 2026-08-16:** even after both eviction fixes above, +RAM still takes 20–30 minutes to decline after a redeploy — down from the +original 1h→4GB→1GB→2.5h→143MB pattern, but still real. Root cause: #135 and +#136 only stop the warm-check from *leaving* the slab resident. They still +*read* the entire ~1.4–3.75 GB slab, twice, on every single boot with a warm +volume — once for `ensure_slab_local`'s SHA-256 cache-hit verify, once for +`ensure_lance_local`'s FNV-1a freshness digest. Reading gigabytes off a +Railway volume and hashing them is real wall-clock time and real (if +transient) page-cache pressure — "evict promptly" is not the same claim as +"never read at all", and this session's 20–30-minute report is evidence the +first alone isn't enough. + +**Scope decision — this is NOT Phase C/D's `ImportSeal`/`warm_verification_reads` +machinery, and should not be confused with it.** `osm_lifecycle.rs`'s Phase A +types already model a "prove origin without reading data" mechanism +(`warm_verification_reads` names S3 *seal keys* a warm boot could check), but +that is an **S3-origin-proof** design — it answers "does this lineage still +match what was published", gated behind Phase C's full versioned-import +wiring and the `OnceLock`→`OsmArtifactManager` migration, neither of which +exists yet. What's needed here is narrower and entirely **local**: "is the +file already on THIS volume still the exact same file I verified last boot, +so I can skip re-reading it at all." Building that as a slice of Phase C +would mean pulling forward the whole seal/reconciliation machinery for a +problem that doesn't need it. The two are compatible — Phase C/D can still +land later and will supersede this — but this fix does not attempt to be, or +substitute for, that architecture. + +**The mechanism: trust filesystem identity (mtime + length) recorded at the +last real verification, distrust it on any mismatch.** Any write to a file +updates its mtime, so `(mtime, len)` unchanged since the last full verify is +conclusive proof the bytes are unchanged too — the same heuristic `make`, +`rsync` (default mode), `cargo`, and `ccache` all use for exactly this +reason (hash-on-every-build is their slow, explicit opt-in, not the +default). This is the "clean shutdown vs dirty shutdown" analogy from +earlier in this investigation: a trusted marker recorded after a verified +clean state lets a later boot skip the expensive replay; any mismatch (or a +missing/malformed marker) falls back to the existing full-hash path +unconditionally — never a hard failure, always a slower-but-correct +degrade. + +- [x] `osm_slab_hydrate.rs`: a `.verified` sidecar (mtime_nanos + + len + digest, one per line) written after every real successful + verification (download or cache-hit). `ensure_slab_local`'s cache-hit + branch checks the marker BEFORE calling `sha256_file`; a match skips + the read entirely, any mismatch (or missing marker) falls straight + through to today's unchanged `sha256_file` verify. +- [x] `osm_lance.rs`: two new Lance schema-metadata keys + (`soa:slab_mtime_nanos`, `soa:slab_len`) written alongside the + existing `soa:slab_digest` at conversion time. `ensure_lance_local` + stats the slab (no read) and, if a dataset already exists whose + stored `(mtime, len)` match, resolves warm WITHOUT ever mmapping or + hashing the slab. Any mismatch falls through to the existing + mmap+`hash_slab`+`reopen_if_warm` path unchanged. +- [x] Named limitation, not a bug: neither mechanism *self-heals* an + existing dataset/cache entry that predates this change, or one whose + mtime was touched without its content changing (e.g. a re-download of + byte-identical bytes). Those cases correctly fall back to the slower + hash-based path forever, until the artifact is genuinely rebuilt — + acceptable because the common steady-state case (an untouched volume + across ordinary redeploys) is exactly what the fast path targets, and + degrading to today's already-correct behavior costs nothing beyond + not yet getting the speedup. +- [x] TDD: reachability counters (`SLOW_PATH_HASH_ATTEMPTED` in + `osm_lance.rs`; the pre-existing `FADVISE_ATTEMPTED` in + `osm_slab_hydrate.rs`, since a skipped `sha256_file` call already + proves itself via that counter not incrementing), same discipline as + #135/#136 — RSS/cgroup accounting cannot see "skipped a read" any + more than it could see "evicted after a read". + +**Verification.** Every falsifier confirmed via the same revert/restore TDD +cycle as #135/#136 (`osm_lance.rs`'s `a_hot_boot_skips_the_full_slab_hash_ +entirely`: `left: 2, right: 1` reverted, passes restored; +`osm_slab_hydrate.rs`'s `resolve_cache_hit_trusts_a_matching_marker_ +without_hashing`: fails reverted, passes restored). One pre-existing test +(`the_warm_reopen_path_attempts_eviction_too`, from #135) had to be updated +— its original scenario (an untouched second call) is now correctly served +by the NEW fast path instead of the digest-based warm-reopen it was written +to cover, so it now touches the slab's mtime before the second call to keep +exercising that branch; the update is documented inline in the test's own +doc comment, not silently weakened. **Discovered mid-verification:** +`#[cfg(test)]` reachability counters are process-wide statics, and plain +`cargo test`'s default multi-threaded harness runs tests concurrently in +ONE process — two counter-based tests running at the same time stomp on +each other's counts. `cargo nextest run` (this repo's own mandated runner, +`CLAUDE.md`'s "CRITICAL: Use `cargo nextest run` instead of `cargo test`") +isolates each test into its own process, which is what actually makes this +counter pattern safe at more than one test at a time — worth remembering +for any future counter-based falsifier in this crate. + +Full suite (`cargo nextest run -p cockpit-server --bin q2-cockpit`): +173/173 passed — 11 new tests (3 in `osm_lance.rs`, 8 in `osm_slab_hydrate.rs`) +on top of #136's 162, 0 regressions. `rustfmt --edition 2024 --check` clean +on all new code in both +files (one real formatting fix applied in each — a `write_lance` call and a +`stat_identity` one-liner — the crate-wide pre-existing drift already +documented above is untouched). Clippy (`cargo clippy -p cockpit-server +--no-deps --all-targets`, compared via `git stash` against the pre-change +tree): one genuinely new finding surfaced mid-work (`collapsible_if` on the +fast-path's nested `if let`, fixed with a `let`-chain per clippy's own +suggestion) and was fixed before landing — final comparison is 68/0 on both +trees, byte-identical warning content. diff --git a/crates/cockpit-server/src/osm_lance.rs b/crates/cockpit-server/src/osm_lance.rs index d2568eeea..e9ef943e8 100644 --- a/crates/cockpit-server/src/osm_lance.rs +++ b/crates/cockpit-server/src/osm_lance.rs @@ -74,6 +74,11 @@ const K_ENDIAN: &str = "soa:endianness"; const K_CLASSID: &str = "soa:classid"; const K_DIGEST: &str = "soa:slab_digest"; const K_SOURCE: &str = "soa:source"; +// The "hot but idle" fast-path identity — see `reopen_if_unchanged`'s doc +// comment. Recorded alongside `K_DIGEST` at write time; a later boot whose +// slab's CURRENT stat matches both never touches slab bytes at all. +const K_SLAB_MTIME: &str = "soa:slab_mtime_nanos"; +const K_SLAB_LEN: &str = "soa:slab_len"; const ROW_COLUMN: &str = "row"; @@ -96,40 +101,25 @@ const OSM_BAKE_CLASSID: &str = "00000000"; pub async fn ensure_lance_local(slab_path: &Path) -> Option { let dest = slab_path.with_extension("lance"); - // Digest over an mmap, not `fs::read`: the warm path must not pay a - // slab-sized heap allocation (1.29 GiB for Berlin) just to compute a - // hash — the pages stream through the page cache and stay reclaimable. - let file = match std::fs::File::open(slab_path) { - Ok(f) => f, - Err(e) => { - tracing::error!(path = %slab_path.display(), error = %e, "osm lance: cannot open slab"); - return None; - } - }; - // SAFETY: read-only mapping of the baked, immutable artifact. - let map = match unsafe { memmap2::Mmap::map(&file) } { + // Stat-only, no bytes read yet. `rows` (needed for the Arrow ceiling + // check below) and the slab's filesystem identity (needed for the + // hot-but-idle fast path just after) both come from this alone. + let meta = match std::fs::metadata(slab_path) { Ok(m) => m, Err(e) => { - tracing::error!(path = %slab_path.display(), error = %e, "osm lance: cannot mmap slab"); + tracing::error!(path = %slab_path.display(), error = %e, "osm lance: cannot stat slab"); return None; } }; - if map.is_empty() || !map.len().is_multiple_of(NODE_ROW_STRIDE) { + let len = meta.len() as usize; + if len == 0 || !len.is_multiple_of(NODE_ROW_STRIDE) { tracing::error!( - path = %slab_path.display(), len = map.len(), + path = %slab_path.display(), len, "osm lance: slab is not a whole number of {NODE_ROW_STRIDE}-byte rows; refusing" ); return None; } - let rows = map.len() / NODE_ROW_STRIDE; - let digest_hex = format!("{:016x}", osm_soa_bake::codebook::hash_slab(&map)); - // The map is KEPT, not dropped. It used to be released here and the file - // read again into an owned `Vec` for the write — a slab-sized ANONYMOUS - // allocation (1.4 GB for Berlin, 3.75 GB for Brandenburg) that no memory - // pressure can ever reclaim. Reusing these pages makes the conversion's - // source cost page cache instead: still charged to the cgroup while - // touched, but evictable, which anonymous memory is not. - let map = std::sync::Arc::new(map); + let rows = len / NODE_ROW_STRIDE; // The Arrow ceiling is decided by `rows` ALONE, so decide it here — the // earliest point `rows` exists — rather than downstream in `write_lance`. @@ -160,6 +150,58 @@ pub async fn ensure_lance_local(slab_path: &Path) -> Option { return None; } + // ── Hot-but-idle fast path ─────────────────────────────────────────── + // Trust an already-warm dataset's OWN recorded slab identity (mtime+len) + // without ever opening or mmapping the slab. See `reopen_if_unchanged`'s + // doc comment for why this is sound. `slab_mtime_nanos` returning `None` + // (a platform/filesystem without mtime support) just skips the fast + // path — the slower, always-correct path below still runs. Hoisted into + // a variable (not just an `if let` scope) so a fresh conversion at the + // end of this function can also record it for the NEXT boot's fast path. + let mtime_nanos = slab_mtime_nanos(&meta); + if let Some(mtime_nanos) = mtime_nanos + && let Some(dest_out) = reopen_if_unchanged(&dest, rows, mtime_nanos, len as u64).await + { + tracing::info!( + path = %dest_out.display(), rows, + "osm lance: hot — dataset's recorded slab identity (mtime+len) matches; \ + skipped mmap+hash entirely" + ); + return Some(dest_out); + } + + // ── Below here: the slower, content-hash-based path — unchanged from + // before this fast path existed, reached now only when the fast path + // above declined (cold boot, touched mtime, or a dataset predating this + // fix). Digest over an mmap, not `fs::read`: this path must not pay a + // slab-sized heap allocation (1.29 GiB for Berlin) just to compute a + // hash — the pages stream through the page cache and stay reclaimable. + let file = match std::fs::File::open(slab_path) { + Ok(f) => f, + Err(e) => { + tracing::error!(path = %slab_path.display(), error = %e, "osm lance: cannot open slab"); + return None; + } + }; + // SAFETY: read-only mapping of the baked, immutable artifact. + let map = match unsafe { memmap2::Mmap::map(&file) } { + Ok(m) => m, + Err(e) => { + tracing::error!(path = %slab_path.display(), error = %e, "osm lance: cannot mmap slab"); + return None; + } + }; + #[cfg(test)] + SLOW_PATH_HASH_ATTEMPTED.fetch_add(1, std::sync::atomic::Ordering::Relaxed); + let digest_hex = format!("{:016x}", osm_soa_bake::codebook::hash_slab(&map)); + // The map is KEPT, not dropped. It used to be released here and the file + // read again into an owned `Vec` for the write — a slab-sized ANONYMOUS + // allocation (1.4 GB for Berlin, 3.75 GB for Brandenburg) that no memory + // pressure can ever reclaim. Reusing these pages makes the conversion's + // source cost page cache instead: still charged to the cgroup while + // touched, but evictable, which anonymous memory is not. + let map = std::sync::Arc::new(map); + if let Some(warm) = reopen_if_warm(&dest, rows, &digest_hex).await { tracing::info!( path = %dest.display(), rows, @@ -205,7 +247,16 @@ pub async fn ensure_lance_local(slab_path: &Path) -> Option { path = %dest.display(), rows, "osm lance: converting the .soa slab into a Lance dataset (zero-copy import)" ); - write_lance(&dest, map, rows, &digest_hex, slab_path).await + write_lance( + &dest, + map, + rows, + &digest_hex, + slab_path, + mtime_nanos, + len as u64, + ) + .await } /// Most rows one `FixedSizeBinaryArray` can hold at [`NODE_ROW_STRIDE`]. @@ -246,6 +297,86 @@ fn remove_stale_dataset(dest: &Path) -> std::io::Result<()> { std::fs::remove_dir_all(dest) } +/// `mtime` as nanoseconds since the Unix epoch, or `None` on any failure to +/// read it (a platform without mtime support, or a clock before 1970 — the +/// caller treats `None` as "cannot use the fast path", never as an error). +#[must_use] +fn slab_mtime_nanos(meta: &std::fs::Metadata) -> Option { + meta.modified() + .ok()? + .duration_since(std::time::UNIX_EPOCH) + .ok() + .map(|d| d.as_nanos()) +} + +/// Reachability counter for the slow, content-hash-based path (test builds +/// only) — proves whether [`ensure_lance_local`] actually opened+mmapped the +/// slab, which `/proc/self/statm` and this sandbox's absent +/// `/sys/fs/cgroup/*` cannot show directly. See +/// `a_hot_boot_skips_the_full_slab_hash_entirely` below, and +/// `EVICTIONS_ATTEMPTED`'s doc comment for why a reachability counter is the +/// right substitute for a memory measurement here. +#[cfg(test)] +static SLOW_PATH_HASH_ATTEMPTED: std::sync::atomic::AtomicUsize = + std::sync::atomic::AtomicUsize::new(0); + +/// `Some(dest)` iff a dataset already exists at `dest`, is a single +/// fragment, has exactly `rows` rows, AND its own recorded slab identity — +/// `soa:slab_mtime_nanos` + `soa:slab_len` — matches the CURRENT slab's, +/// all proven **without reading a single byte of the slab**. +/// +/// This is sound, not merely convenient: any write to a file updates its +/// mtime, so `(mtime, len)` unchanged since the dataset was written is +/// conclusive proof the slab's bytes are unchanged too — the same +/// heuristic `make`, `rsync` (default mode), `cargo`, and `ccache` all rely +/// on for exactly this reason (hashing every input on every run is their +/// slow, explicit opt-in, never the default). It does NOT try to prove +/// anything about content that genuinely changed; a mismatch here is not a +/// verdict, only "cannot fast-path" — [`ensure_lance_local`] falls through +/// to the slower, always-correct mmap+[`osm_soa_bake::codebook::hash_slab`] +/// path below, which is untouched by this function's existence. +/// +/// **Named limitation:** a dataset written before this fast path existed +/// (or one whose slab's mtime was touched without its content changing — +/// e.g. a re-download of byte-identical bytes) has no stored identity to +/// match, or a permanently-mismatching one, and falls back to the slow path +/// FOREVER until the slab is genuinely rebuilt. That is a missed +/// optimization, never a correctness problem: the slow path is exactly +/// today's already-correct behaviour. +async fn reopen_if_unchanged( + dest: &Path, + rows: usize, + mtime_nanos: u128, + len: u64, +) -> Option { + if !dest.exists() { + return None; + } + let uri = dest.to_string_lossy().into_owned(); + let ds = lance::dataset::builder::DatasetBuilder::from_uri(&uri) + .load() + .await + .ok()?; + let got_rows = ds.count_rows(None).await.ok()?; + let got_mtime: Option = ds + .schema() + .metadata + .get(K_SLAB_MTIME) + .and_then(|s| s.parse().ok()); + let got_len: Option = ds + .schema() + .metadata + .get(K_SLAB_LEN) + .and_then(|s| s.parse().ok()); + let fragments = ds.get_fragments().len(); + if got_rows == rows && got_mtime == Some(mtime_nanos) && got_len == Some(len) && fragments == 1 + { + Some(dest.to_path_buf()) + } else { + None + } +} + /// `Some(dest)` iff a dataset already exists at `dest` with exactly `rows` /// rows and a `soa:slab_digest` header matching `digest_hex` — the ONLY two /// facts that must hold before skipping the write. Any other outcome @@ -310,6 +441,8 @@ async fn write_lance( rows: usize, digest_hex: &str, slab_path: &Path, + mtime_nanos: Option, + len: u64, ) -> Option { let field = Field::new( ROW_COLUMN, @@ -324,7 +457,7 @@ async fn write_lance( "lance-encoding:compression".to_string(), "none".to_string(), )])); - let schema_meta = HashMap::from([ + let mut schema_meta = HashMap::from([ (K_LAYOUT.to_string(), ENVELOPE_LAYOUT_VERSION.to_string()), (K_STRIDE.to_string(), NODE_ROW_STRIDE.to_string()), ( @@ -339,6 +472,15 @@ async fn write_lance( format!("{} rows={rows}", slab_path.display()), ), ]); + // The hot-but-idle fast-path identity — only written when the platform + // actually reported an mtime (`ensure_lance_local`'s `slab_mtime_nanos` + // returned `Some`). Absent here means `reopen_if_unchanged` can never + // fast-path THIS dataset, which is a correctness-preserving degrade + // (see that function's doc comment), never a hard failure. + if let Some(mtime_nanos) = mtime_nanos { + schema_meta.insert(K_SLAB_MTIME.to_string(), mtime_nanos.to_string()); + schema_meta.insert(K_SLAB_LEN.to_string(), len.to_string()); + } let schema = Arc::new(Schema::new_with_metadata(vec![field], schema_meta)); // The Arrow i32 row ceiling is enforced by `ensure_lance_local` BEFORE @@ -682,6 +824,15 @@ mod tests { assert!(reopen_if_warm(&dest, 10, "deadbeef").await.is_none()); } + /// The fast path's own base case — the same "nothing there yet" boundary + /// [`reopen_if_warm`] has above, for the identity-only precondition. + #[tokio::test] + async fn reopen_if_unchanged_declines_a_missing_destination() { + let dir = tempfile::tempdir().expect("tempdir"); + let dest = dir.path().join("nope.lance"); + assert!(reopen_if_unchanged(&dest, 10, 12345, 5120).await.is_none()); + } + /// The exact boundary this incident crossed: Brandenburg's real /// `rows=7_330_219` PANICKED the process at startup /// (`arrow-array-58.3.0/src/array/fixed_size_binary_array.rs:106`, @@ -794,8 +945,24 @@ mod tests { /// Drives `ensure_lance_local` — the real entry point — twice: a cold /// build (1 attempt, already covered on its own by /// `the_slab_mapping_is_released_only_when_nothing_else_holds_it`), - /// then a warm reopen of the SAME unchanged slab (must add exactly 1 - /// more). + /// then a reopen that must land on the DIGEST-based `reopen_if_warm` + /// branch (must add exactly 1 more). + /// + /// **Updated by the "hot but idle" fast path, honestly, not silently.** + /// This test originally reopened the SAME untouched slab for its second + /// call — which was the only warm path that existed at the time. Since + /// `reopen_if_unchanged` landed, that exact scenario (unchanged mtime + /// AND length) is now caught by the FASTER fast path, which never mmaps + /// the slab at all and therefore has nothing to evict — + /// `a_hot_boot_skips_the_full_slab_hash_entirely` covers that case + /// directly. This test now touches the slab's mtime before the second + /// call (content byte-identical) specifically to DEFEAT the fast path, + /// so it keeps exercising the branch it was written for: the + /// digest-based `reopen_if_warm` path still reached whenever the fast + /// path can't apply. Mirrors `a_touched_mtime_declines_the_fast_path_ + /// but_the_slow_path_still_succeeds`'s setup, which proves the digest + /// path is reached at all; this test proves eviction still happens once + /// it is. #[tokio::test] async fn the_warm_reopen_path_attempts_eviction_too() { let rows = 4usize; @@ -814,6 +981,16 @@ mod tests { "the cold/rebuild path must attempt eviction exactly once" ); + // Defeat the fast path (see the doc comment above): same content, + // different mtime, so `reopen_if_unchanged` declines and the call + // falls through to the digest-based `reopen_if_warm` — the branch + // this test exists to cover. + let touched = std::time::SystemTime::now() + std::time::Duration::from_secs(3600); + std::fs::File::open(&slab_path) + .expect("reopen") + .set_modified(touched) + .expect("set_modified"); + let second = ensure_lance_local(&slab_path).await; assert_eq!( second, first, @@ -830,6 +1007,88 @@ mod tests { ); } + /// **The "hot but idle" falsifier.** #135/#136 stopped this warm path + /// from LEAVING the slab resident; this proves it now skips the + /// mmap+hash of the slab ENTIRELY on a genuinely unchanged boot — the + /// operator's own framing ("why don't you wire volume01 as hot but idle + /// … instead of what appears to be … pulling it into RAM"). + /// + /// A cold conversion must still hash once (there is nothing to trust + /// yet). A second call against the SAME unchanged file — same path, + /// same bytes, same mtime, nothing touched it in between — must resolve + /// to the identical dataset WITHOUT the slow path ever running again. + #[tokio::test] + async fn a_hot_boot_skips_the_full_slab_hash_entirely() { + let rows = 4usize; + let dir = tempfile::tempdir().expect("tempdir"); + let slab_path = dir.path().join("hot.soa"); + std::fs::write(&slab_path, vec![0u8; rows * NODE_ROW_STRIDE]).expect("write slab"); + + let before = SLOW_PATH_HASH_ATTEMPTED.load(std::sync::atomic::Ordering::Relaxed); + + let first = ensure_lance_local(&slab_path).await; + assert!(first.is_some(), "cold conversion must succeed"); + let after_cold = SLOW_PATH_HASH_ATTEMPTED.load(std::sync::atomic::Ordering::Relaxed); + assert_eq!( + after_cold - before, + 1, + "a cold build has nothing to trust yet — it must hash exactly once" + ); + + let second = ensure_lance_local(&slab_path).await; + assert_eq!( + second, first, + "an unchanged slab must resolve to the SAME dataset" + ); + let after_hot = SLOW_PATH_HASH_ATTEMPTED.load(std::sync::atomic::Ordering::Relaxed); + assert_eq!( + after_hot, after_cold, + "a hot boot (identical mtime+len) must NOT re-enter the mmap+hash path at \ + all — this is the whole point of the fast path: on a genuinely unchanged \ + slab, zero bytes of it are ever touched" + ); + } + + /// The correctness twin: when the fast path genuinely CANNOT trust the + /// dataset (here, the slab's mtime was touched — e.g. a redundant + /// re-download landed byte-identical content with a fresh timestamp), + /// it must decline and fall through to the slow path, which must still + /// reach the right answer (this dataset is warm) via content digest — + /// not silently serve something wrong, and not panic. + #[tokio::test] + async fn a_touched_mtime_declines_the_fast_path_but_the_slow_path_still_succeeds() { + let rows = 4usize; + let dir = tempfile::tempdir().expect("tempdir"); + let slab_path = dir.path().join("touched.soa"); + std::fs::write(&slab_path, vec![0u8; rows * NODE_ROW_STRIDE]).expect("write slab"); + + let first = ensure_lance_local(&slab_path).await; + assert!(first.is_some(), "cold conversion must succeed"); + let after_cold = SLOW_PATH_HASH_ATTEMPTED.load(std::sync::atomic::Ordering::Relaxed); + + // Touch mtime forward without changing a single byte of content. + let touched = std::time::SystemTime::now() + std::time::Duration::from_secs(3600); + std::fs::File::open(&slab_path) + .expect("reopen") + .set_modified(touched) + .expect("set_modified"); + + let second = ensure_lance_local(&slab_path).await; + assert_eq!( + second, first, + "content is unchanged, so the slow path's digest match must still \ + resolve to the SAME dataset" + ); + let after_touch = SLOW_PATH_HASH_ATTEMPTED.load(std::sync::atomic::Ordering::Relaxed); + assert_eq!( + after_touch - after_cold, + 1, + "the fast path must decline on a touched mtime — it is not entitled to \ + trust identity that no longer matches — and the slow path must be the \ + one that recovers the correct (warm) answer" + ); + } + #[test] fn a_row_count_under_the_ceiling_builds_a_valid_array() { let rows = 1_000usize; // Berlin-scale, nowhere near the 4.19M cap diff --git a/crates/cockpit-server/src/osm_slab_hydrate.rs b/crates/cockpit-server/src/osm_slab_hydrate.rs index feda05b1b..bb3b0545e 100644 --- a/crates/cockpit-server/src/osm_slab_hydrate.rs +++ b/crates/cockpit-server/src/osm_slab_hydrate.rs @@ -261,16 +261,24 @@ pub async fn ensure_slab_local() -> Option { // Cache hit, but only if it still hashes correctly — see module docs. if dest.is_file() { - match sha256_file(&dest) { - Ok(got) if got == want => { + match resolve_cache_hit(&dest, &want) { + CacheDecision::TrustedViaMarker => { + tracing::info!( + artifact = name, + "osm slab: cache hit, trusted via unchanged marker (mtime+len \ + match the last real verification; skipped re-hash)" + ); + continue; + } + CacheDecision::Verified => { tracing::info!(artifact = name, "osm slab: cache hit, checksum verified"); continue; } - Ok(got) => tracing::warn!( + CacheDecision::Mismatch(got) => tracing::warn!( artifact = name, %got, %want, "osm slab: cached copy failed its checksum; re-fetching" ), - Err(e) => { + CacheDecision::Unreadable(e) => { tracing::warn!(artifact = name, error = %e, "osm slab: cannot hash cached copy; re-fetching") } } @@ -391,6 +399,9 @@ async fn download_verified( let _ = std::fs::remove_file(&part); return false; } + // A fresh download IS a real verification — record it so the NEXT boot's + // cache hit can trust it via `resolve_cache_hit` without re-reading. + write_marker(dest, &got); tracing::info!( artifact = name, bytes = written, @@ -399,6 +410,150 @@ async fn download_verified( true } +// ── The "hot but idle" cache-hit fast path ────────────────────────────── +// +// `sha256_file` streams the whole artifact through the kernel's page cache +// on EVERY boot with a warm volume, even though the common case is "nothing +// changed since the last boot verified this exact file." A tiny sidecar +// marker — `.verified` — records the (mtime, length, digest) that +// the last REAL verification produced. Any write to a file updates its +// mtime, so `(mtime, len)` matching the marker is conclusive proof the +// bytes are unchanged too (the same heuristic `make`/`rsync`/`cargo`/ +// `ccache` use by default) — trusting it skips `sha256_file` entirely, +// exactly the "clean shutdown" half of the exchange-DAG analogy this +// investigation used: a persisted marker from a known-good state lets a +// later boot skip the expensive replay. Any mismatch — content changed, +// marker absent, marker malformed, or the bucket republished a different +// `want` digest for this name — falls straight through to `sha256_file`, +// the "dirty shutdown" fallback, unconditionally. + +/// Parsed contents of a `.verified` marker: exactly what the last +/// successful verification of THIS file recorded, three lines in order — +/// mtime (nanoseconds since `UNIX_EPOCH`), length in bytes, digest. Any +/// malformed or unreadable marker is never a hard error anywhere it is +/// used — the caller always has a correct, if slower, fallback: a real +/// `sha256_file` re-hash. +#[derive(Debug, PartialEq, Eq)] +struct VerifiedMarker { + mtime_nanos: u128, + len: u64, + digest: String, +} + +impl VerifiedMarker { + fn parse(text: &str) -> Option { + let mut lines = text.lines(); + let mtime_nanos: u128 = lines.next()?.trim().parse().ok()?; + let len: u64 = lines.next()?.trim().parse().ok()?; + let digest = lines.next()?.trim().to_ascii_lowercase(); + if digest.len() != 64 || !digest.chars().all(|c| c.is_ascii_hexdigit()) { + return None; + } + Some(Self { + mtime_nanos, + len, + digest, + }) + } + + fn render(&self) -> String { + format!("{}\n{}\n{}\n", self.mtime_nanos, self.len, self.digest) + } +} + +/// `(mtime as nanoseconds since the Unix epoch, length in bytes)` for +/// `path`, or `None` on any stat failure — a stat failure just means "the +/// fast path is unavailable here", never an error the caller need surface; +/// `sha256_file` remains correct regardless. +fn stat_identity(path: &Path) -> Option<(u128, u64)> { + let meta = std::fs::metadata(path).ok()?; + let mtime = meta.modified().ok()?; + let nanos = mtime.duration_since(std::time::UNIX_EPOCH).ok()?.as_nanos(); + Some((nanos, meta.len())) +} + +/// Where the marker for `dest` lives — a sibling file, same directory, +/// `.verified` appended to the artifact's own name (`berlin.soa` → +/// `berlin.soa.verified`), so it travels with the cache dir and is +/// trivially recognisable in a directory listing. +fn marker_path(dest: &Path) -> PathBuf { + let mut name = dest.as_os_str().to_os_string(); + name.push(".verified"); + PathBuf::from(name) +} + +/// Whether the marker at `marker_path(dest)` proves `dest` still matches +/// `want` WITHOUT reading `dest`'s content. Folding `want` into the +/// comparison (not just the file's own recorded digest) means a bucket +/// republish — `SHA256SUMS` now names a DIFFERENT digest for this artifact +/// name — correctly invalidates a marker whose file identity hasn't +/// changed at all: the marker's digest no longer equals the freshly +/// fetched `want`, so this declines and `sha256_file` runs for real, +/// which then correctly reports a mismatch and triggers a re-download. +fn trusted_via_marker(dest: &Path, want: &str) -> bool { + let Some(marker) = std::fs::read_to_string(marker_path(dest)) + .ok() + .and_then(|text| VerifiedMarker::parse(&text)) + else { + return false; + }; + let Some((mtime_nanos, len)) = stat_identity(dest) else { + return false; + }; + marker.mtime_nanos == mtime_nanos && marker.len == len && marker.digest == want +} + +/// Record `dest`'s current (mtime, length) alongside `digest` — the just- +/// proven-correct identity a later boot's [`trusted_via_marker`] can trust. +/// Failure to write is logged, never fatal: the next boot simply re-hashes, +/// which is exactly today's behaviour without this whole mechanism. +fn write_marker(dest: &Path, digest: &str) { + let Some((mtime_nanos, len)) = stat_identity(dest) else { + return; + }; + let marker = VerifiedMarker { + mtime_nanos, + len, + digest: digest.to_string(), + }; + if let Err(e) = std::fs::write(marker_path(dest), marker.render()) { + tracing::warn!( + path = %dest.display(), error = %e, + "osm slab: could not write verification marker (non-fatal; next boot re-hashes)" + ); + } +} + +/// The outcome of checking one cached artifact against `want`, in the shape +/// [`ensure_slab_local`]'s loop needs to log and branch on. Split out from +/// that loop so it is directly testable without env vars or an S3 stub — +/// see `resolve_cache_hit_trusts_a_matching_marker_without_hashing` below. +enum CacheDecision { + /// The marker proved identity without touching the file's bytes. + TrustedViaMarker, + /// A real `sha256_file` ran and matched `want` — the marker is now + /// (re)written for the next boot. + Verified, + /// A real `sha256_file` ran and did NOT match `want`. + Mismatch(String), + /// The file could not be hashed at all (e.g. a permissions error). + Unreadable(std::io::Error), +} + +fn resolve_cache_hit(dest: &Path, want: &str) -> CacheDecision { + if trusted_via_marker(dest, want) { + return CacheDecision::TrustedViaMarker; + } + match sha256_file(dest) { + Ok(got) if got == want => { + write_marker(dest, &got); + CacheDecision::Verified + } + Ok(got) => CacheDecision::Mismatch(got), + Err(e) => CacheDecision::Unreadable(e), + } +} + /// SHA-256 of a file, streamed — the artifact is 1.29 GiB and must not be read /// into memory to be hashed. /// @@ -565,6 +720,165 @@ not-a-hash junk.txt std::fs::remove_file(&p).ok(); } + #[test] + fn verified_marker_render_parse_roundtrips() { + let marker = VerifiedMarker { + mtime_nanos: 1_234_567_890_123_456_789, + len: 1_484_783_616, + digest: "a".repeat(64), + }; + let parsed = VerifiedMarker::parse(&marker.render()).expect("must parse its own render"); + assert_eq!(parsed, marker); + } + + /// Anti-vacuity for the digest-shape guard: a plausible-looking but + /// wrong-length digest must not silently parse as a 64-char one. + #[test] + fn verified_marker_parse_rejects_a_malformed_digest() { + assert!(VerifiedMarker::parse("123\n456\nnothex").is_none()); + assert!(VerifiedMarker::parse("123\n456\ntooshort").is_none()); + assert!(VerifiedMarker::parse("not-a-number\n456\n").is_none()); + assert!(VerifiedMarker::parse("").is_none()); + } + + fn write_temp_artifact(dir: &Path, name: &str, bytes: &[u8]) -> PathBuf { + let p = dir.join(name); + std::fs::write(&p, bytes).expect("write artifact"); + p + } + + /// **The base case.** No marker at all must decline, not panic or + /// somehow trust an absent file. + #[test] + fn trusted_via_marker_declines_when_the_marker_is_absent() { + let dir = std::env::temp_dir().join("q2-hydrate-test-marker-absent"); + std::fs::create_dir_all(&dir).unwrap(); + let p = write_temp_artifact(&dir, "artifact.bin", b"hello world"); + assert!(!trusted_via_marker(&p, "irrelevant")); + std::fs::remove_file(&p).ok(); + } + + /// The marker's own recorded identity no longer matches the file on + /// disk — the file was rewritten (different content, different mtime), + /// which is precisely the case `trusted_via_marker` must catch: a stale + /// marker must never vouch for content it never actually verified. + #[test] + fn trusted_via_marker_declines_when_the_file_was_rewritten() { + let dir = std::env::temp_dir().join("q2-hydrate-test-marker-rewritten"); + std::fs::create_dir_all(&dir).unwrap(); + let p = write_temp_artifact(&dir, "artifact.bin", b"original content"); + let (mtime_nanos, len) = stat_identity(&p).expect("stat"); + let digest = sha256_file(&p).expect("hash"); + write_marker(&p, &digest); + + // Rewrite with DIFFERENT content — a real mtime bump, not a forced one. + std::fs::write(&p, b"different content, different length").expect("rewrite"); + assert!( + !trusted_via_marker(&p, &digest), + "a rewritten file must never be trusted via a marker from before the rewrite" + ); + // Sanity: the original marker really did record the pre-rewrite state. + assert_ne!(stat_identity(&p).unwrap(), (mtime_nanos, len)); + + std::fs::remove_file(&p).ok(); + std::fs::remove_file(marker_path(&p)).ok(); + } + + /// The file itself is byte-for-byte untouched, but the CALLER's `want` + /// changed — a bucket republish naming a different digest for this + /// artifact name. The marker's own recorded digest no longer equals the + /// freshly fetched `want`, so trust must be declined even though the + /// file's identity (mtime+len) is unchanged. + #[test] + fn trusted_via_marker_declines_when_the_wanted_digest_changes() { + let dir = std::env::temp_dir().join("q2-hydrate-test-marker-want-changed"); + std::fs::create_dir_all(&dir).unwrap(); + let p = write_temp_artifact(&dir, "artifact.bin", b"stable content"); + let digest = sha256_file(&p).expect("hash"); + write_marker(&p, &digest); + + assert!( + !trusted_via_marker(&p, &"f".repeat(64)), + "a marker must not vouch for a digest it never recorded" + ); + + std::fs::remove_file(&p).ok(); + std::fs::remove_file(marker_path(&p)).ok(); + } + + /// The positive case: identity unchanged, digest matches `want` — must + /// trust. + #[test] + fn trusted_via_marker_accepts_when_everything_matches() { + let dir = std::env::temp_dir().join("q2-hydrate-test-marker-match"); + std::fs::create_dir_all(&dir).unwrap(); + let p = write_temp_artifact(&dir, "artifact.bin", b"unchanged content"); + let digest = sha256_file(&p).expect("hash"); + write_marker(&p, &digest); + + assert!(trusted_via_marker(&p, &digest)); + + std::fs::remove_file(&p).ok(); + std::fs::remove_file(marker_path(&p)).ok(); + } + + /// **The "hot but idle" falsifier for the cache-hit path.** A matching + /// marker must resolve `resolve_cache_hit` to `TrustedViaMarker` WITHOUT + /// `sha256_file` ever running — proven via the same `FADVISE_ATTEMPTED` + /// reachability counter `sha256_file_attempts_page_cache_eviction_ + /// after_hashing` uses (bumped inside `advise_dontneed`, which only + /// `sha256_file` calls): if the counter doesn't move, the read never + /// happened. + #[test] + fn resolve_cache_hit_trusts_a_matching_marker_without_hashing() { + let dir = std::env::temp_dir().join("q2-hydrate-test-resolve-trusted"); + std::fs::create_dir_all(&dir).unwrap(); + let p = write_temp_artifact(&dir, "artifact.bin", b"trust me, i'm unchanged"); + let digest = sha256_file(&p).expect("hash"); + write_marker(&p, &digest); + + let before = FADVISE_ATTEMPTED.load(std::sync::atomic::Ordering::Relaxed); + let decision = resolve_cache_hit(&p, &digest); + let after = FADVISE_ATTEMPTED.load(std::sync::atomic::Ordering::Relaxed); + + assert!(matches!(decision, CacheDecision::TrustedViaMarker)); + assert_eq!( + after, before, + "a genuinely trusted marker must skip sha256_file entirely — this is the \ + whole point of the fast path: on a genuinely unchanged artifact, zero \ + bytes of it are ever read" + ); + + std::fs::remove_file(&p).ok(); + std::fs::remove_file(marker_path(&p)).ok(); + } + + /// The silent twin: with NO marker present, `resolve_cache_hit` must + /// still fall through to a real hash (and get the right answer) — this + /// is what proves the counter above is a meaningful zero, not a + /// tautological one from a code path that never hashes at all. + #[test] + fn resolve_cache_hit_falls_back_to_a_real_hash_when_untrusted() { + let dir = std::env::temp_dir().join("q2-hydrate-test-resolve-untrusted"); + std::fs::create_dir_all(&dir).unwrap(); + let p = write_temp_artifact(&dir, "artifact.bin", b"no marker yet"); + let digest = sha256_file(&p).expect("hash"); + + let before = FADVISE_ATTEMPTED.load(std::sync::atomic::Ordering::Relaxed); + let decision = resolve_cache_hit(&p, &digest); + let after = FADVISE_ATTEMPTED.load(std::sync::atomic::Ordering::Relaxed); + + assert!(matches!(decision, CacheDecision::Verified)); + assert_eq!( + after - before, + 1, + "with no marker to trust, resolve_cache_hit must actually hash the file" + ); + + std::fs::remove_file(&p).ok(); + std::fs::remove_file(marker_path(&p)).ok(); + } + #[test] fn cache_dir_is_under_the_volume_root() { assert_eq!(cache_dir("/volume01"), PathBuf::from("/volume01/osm"));