From 2755a137a24b561bdcb727f3fe221cedf034422a Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 16 Aug 2026 01:37:40 +0000 Subject: [PATCH] fix(cockpit-server): evict the OSM slab from page cache after checksum hashing MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ensure_slab_local's cache-hit path re-verifies the raw slab's SHA-256 on every boot (deliberate — guards against a half-written file from a killed mid-download container). sha256_file streams the read, so it doesn't inflate process RSS, but every byte still passes through the kernel's page cache with nothing evicting it afterward — likely the larger of the two contributors to the observed Railway RAM pattern, since this runs first, unconditionally, before ensure_lance_local's own warm-check. Fixed with posix_fadvise(POSIX_FADV_DONTNEED) after the streaming hash completes — the file-descriptor-read equivalent of MADV_DONTNEED, gated #[cfg(unix)] with a documented no-op elsewhere. libc was already locked transitively at 0.2.185, so this adds no new dependency tree. Same testing lesson as the prior ensure_lance_local fix, applied without re-deriving it: RSS/cgroup accounting can't see page-cache eviction, so this uses a #[cfg(test)]-only reachability counter instead of a memory measurement, verified via the same revert/restore TDD cycle. 162/162 tests (was 161), 0 regressions. Clippy warning content verified byte-identical to the pre-fix tree via git stash comparison (68/0, 64/64 non-summary lines match). Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_01NMeiLmtDKhomJNSo2ecbJw --- Cargo.lock | 1 + .../plans/2026-08-15-osm-lance-lifecycle.md | 54 +++++++++++ crates/cockpit-server/Cargo.toml | 7 ++ crates/cockpit-server/src/osm_slab_hydrate.rs | 96 +++++++++++++++++++ 4 files changed, 158 insertions(+) diff --git a/Cargo.lock b/Cargo.lock index 8afbc323c..b62f682d9 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1797,6 +1797,7 @@ dependencies = [ "lance-graph-contract", "lance-graph-ogar", "lance-graph-planner", + "libc", "memmap2", "ndarray 0.17.2", "notebook-query", 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 1e120317e..4c1e7679f 100644 --- a/claude-notes/plans/2026-08-15-osm-lance-lifecycle.md +++ b/claude-notes/plans/2026-08-15-osm-lance-lifecycle.md @@ -493,3 +493,57 @@ BAKE side can hash while writing rather than re-reading; the SERVER side attempted here — it is a cross-repo change (bake in `openstreetmap-website-rs` + server in `q2`) and this session's fix already closes the more expensive half (residency, not the read itself). + +## Found during the same investigation: a second, earlier eviction gap in the S3 hydrator + +The `ensure_lance_local` fix above only covers the Lance-conversion warm-check. +`ensure_slab_local` in `crates/cockpit-server/src/osm_slab_hydrate.rs` runs +**before** that — on every boot, on a cache hit, it re-verifies the raw slab's +SHA-256 against `SHA256SUMS` (deliberate, per that module's own doc: a +half-written file from a killed mid-download container must not be waved +through just because a file with the right name exists). `sha256_file` streams +the read in 1 MiB chunks, so it does not inflate **process** RSS the way the +old `osm_lance.rs` mmap bug did — but every byte still passes through the +**kernel's page cache** on the way through `read()`, and nothing evicted it +afterward. On a warm boot this runs first, unconditionally, for the whole +~1.4–3.75 GB region (Berlin/Brandenburg), before `ensure_lance_local`'s own +warm-check even starts — likely the larger of the two contributors to the +observed RAM pattern, since it always runs and the other path only sometimes +reaches its warm branch. + +Fixed with the file-descriptor-read equivalent of `MADV_DONTNEED`: +`posix_fadvise(fd, 0, 0, POSIX_FADV_DONTNEED)` (`len = 0` means "to EOF" per +POSIX), called once `sha256_file` finishes streaming. Gated `#[cfg(unix)]` +with a documented no-op fallback on other platforms (`.claude/rules/ +cross-platform.md` — no portable equivalent exists), following the exact +pattern already established in this workspace by `crates/quarto-mcp-launcher/ +src/delegate.rs`'s `libc::fcntl` use. `libc` was already locked transitively +at 0.2.185 via the rest of the workspace, so declaring it as a direct +dependency of `cockpit-server` adds no new dependency tree — the same move +`arc-swap` made for the `ensure_lance_local` fix above. + +**Same testing lesson, applied without re-deriving it:** RSS and cgroup memory +accounting can't distinguish "read then evicted" from "read then left +resident" for a streamed read any more than they could for the mmap case — +`/proc/self/statm` cannot see kernel page-cache state, and `/sys/fs/cgroup/*` +is unavailable in this dev sandbox (though it is what Railway's dashboard +measures in production). Used the same `#[cfg(test)]`-only reachability +counter pattern (`FADVISE_ATTEMPTED`) instead of a memory measurement — a +real falsifier, confirmed failing before the fix (`left: 0, right: 1`) and +passing after via the same revert/restore TDD cycle. Full crate suite: +162/162 (was 161; one new test), 0 regressions. `rustfmt --edition 2024 +--check` on the touched leaf module shows the new code is clean — the file +carries pre-existing, out-of-scope formatting drift on lines this change +never touched (same crate-wide toolchain-baseline drift noted in the section +above; left alone per "fix one bug at a time"). Clippy: `cargo clippy -p +cockpit-server --no-deps --all-targets`, compared via `git stash` against +the pre-fix tree — 68 warnings / 0 errors on both sides, and the actual +warning content (not just the count) is byte-identical (64/64 non-summary +lines match exactly) — zero new findings. + +**Still open, unchanged from the note above:** neither fix stops the +warm-check from *reading* the whole slab, only from leaving it resident +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. diff --git a/crates/cockpit-server/Cargo.toml b/crates/cockpit-server/Cargo.toml index 3518d106e..d61a0c97b 100644 --- a/crates/cockpit-server/Cargo.toml +++ b/crates/cockpit-server/Cargo.toml @@ -78,6 +78,13 @@ object_store = { version = "0.13.2", features = ["aws"] } sha2 = "0.10" hex = "0.4" futures = "0.3" +# `osm_slab_hydrate::sha256_file`: advise the kernel to drop a hashed file's +# pages from its page cache afterward (`posix_fadvise(POSIX_FADV_DONTNEED)`, +# gated `#[cfg(unix)]` — no portable equivalent, no-op elsewhere, same +# pattern as `quarto-mcp-launcher/src/delegate.rs`'s `libc::fcntl` use). +# Already locked transitively at 0.2.185 via the rest of the workspace, so +# this adds no new dependency tree — same move `arc-swap` made in PR #135. +libc = "0.2" # `osm_artifact_manager`: the atomically-replaceable-Arc serving mechanism # (Phase B of the OSM/Lance lifecycle plan). Already in the lockfile # transitively at 1.9 via lance-namespace/goosefs, so this adds no new diff --git a/crates/cockpit-server/src/osm_slab_hydrate.rs b/crates/cockpit-server/src/osm_slab_hydrate.rs index a07887b77..feda05b1b 100644 --- a/crates/cockpit-server/src/osm_slab_hydrate.rs +++ b/crates/cockpit-server/src/osm_slab_hydrate.rs @@ -401,6 +401,19 @@ async fn download_verified( /// SHA-256 of a file, streamed — the artifact is 1.29 GiB and must not be read /// into memory to be hashed. +/// +/// The streaming loop keeps process RSS flat, but every byte still passes +/// through the *kernel's* page cache on the way through `read()`, and +/// nothing evicts it afterward. This runs on EVERY boot with a warm volume +/// (the common case — [`ensure_slab_local`]'s cache-hit branch, before +/// `ensure_lance_local` even starts), so redeploying an unchanged region +/// faulted the whole ~1.4-3.75 GB slab into the page cache for a hash it +/// then threw away. Same disease [`crate::osm_lance::ensure_lance_local`]'s +/// warm-reopen path had (see that module's `release_after_write`), a +/// different call site with a different eviction primitive: `posix_fadvise` +/// is the file-descriptor-read equivalent of `MADV_DONTNEED` for mmap — +/// [`advise_dontneed`] tells the kernel it can drop these pages once we're +/// done with them. fn sha256_file(path: &Path) -> std::io::Result { use std::io::Read; let mut f = std::fs::File::open(path)?; @@ -413,9 +426,57 @@ fn sha256_file(path: &Path) -> std::io::Result { } hasher.update(&buf[..n]); } + advise_dontneed(&f); Ok(hex::encode(hasher.finalize())) } +/// Test-only reachability counter — see [`advise_dontneed`]'s doc comment +/// for why this exists instead of a memory-measurement assertion. +#[cfg(test)] +static FADVISE_ATTEMPTED: std::sync::atomic::AtomicUsize = std::sync::atomic::AtomicUsize::new(0); + +/// Advise the kernel it can drop `f`'s pages from the page cache now that +/// we're done reading it. +/// +/// No portable equivalent exists for this on non-Unix targets (see +/// `.claude/rules/cross-platform.md`), so it is a documented no-op there — +/// this is memory hygiene, not correctness, so a silent no-op is fine. +/// +/// Deliberately untestable via RSS or cgroup memory accounting, same as +/// `osm_lance.rs`'s `release_after_write`: `/proc/self/statm` cannot see +/// kernel page-cache/memcg state, and `/sys/fs/cgroup/*` is unavailable in +/// this dev sandbox (though it IS what Railway's dashboard measures in +/// production). The `#[cfg(test)]`-only [`FADVISE_ATTEMPTED`] counter below +/// proves this function is actually REACHED from `sha256_file` on every +/// call — the regression being guarded against is the call site being +/// silently skipped or removed, which a counter catches and an RSS +/// measurement cannot (see the falsifiability rule: a test that cannot +/// fail when the guard is deleted is not a test of the guard). +#[cfg(unix)] +fn advise_dontneed(f: &std::fs::File) { + #[cfg(test)] + FADVISE_ATTEMPTED.fetch_add(1, std::sync::atomic::Ordering::Relaxed); + + use std::os::unix::io::AsRawFd; + let fd = f.as_raw_fd(); + // SAFETY: `fd` is a valid, open file descriptor borrowed from `f` for + // the duration of this call. `POSIX_FADV_DONTNEED` only advises the + // kernel's page-cache policy — it cannot invalidate memory this process + // holds, and a nonzero return is just the kernel declining the hint, not + // a memory-safety concern — so the return value is intentionally not + // surfaced as a `Result`. `len = 0` means "to the end of the file" per + // POSIX, so this covers everything read above regardless of file size. + unsafe { + libc::posix_fadvise(fd, 0, 0, libc::POSIX_FADV_DONTNEED); + } +} + +#[cfg(not(unix))] +fn advise_dontneed(_f: &std::fs::File) { + #[cfg(test)] + FADVISE_ATTEMPTED.fetch_add(1, std::sync::atomic::Ordering::Relaxed); +} + #[cfg(test)] mod tests { use super::*; @@ -469,6 +530,41 @@ not-a-hash junk.txt std::fs::remove_file(&p).ok(); } + /// **The regression this fix is for.** `sha256_file` streams the whole + /// file through the kernel's page cache and, before this fix, left it + /// there — silently, on every boot with a warm volume. This counts + /// reachability of the eviction advisory rather than measuring memory + /// (RSS can't see page-cache state, and cgroup accounting is + /// unavailable in this sandbox — see `advise_dontneed`'s doc comment). + #[test] + fn sha256_file_attempts_page_cache_eviction_after_hashing() { + let dir = std::env::temp_dir().join("q2-hydrate-test-fadvise"); + std::fs::create_dir_all(&dir).unwrap(); + let p = dir.join("probe_fadvise.bin"); + std::fs::write(&p, b"abc").unwrap(); + + let before = FADVISE_ATTEMPTED.load(std::sync::atomic::Ordering::Relaxed); + let digest = sha256_file(&p).unwrap(); + let after = FADVISE_ATTEMPTED.load(std::sync::atomic::Ordering::Relaxed); + + // The hash itself must still be correct — this test must not pass + // merely because eviction ran on the wrong (or no) data. + assert_eq!( + digest, + "ba7816bf8f01cfea414140de5dae2223b00361a396177a9cb410ff61f20015ad" + ); + assert_eq!( + after - before, + 1, + "sha256_file must attempt to advise the kernel to drop this file's \ + pages from the page cache after hashing it — this is the regression: \ + it used to hash the whole ~1.4-3.75 GB slab on every boot with a warm \ + volume, and nothing ever evicted it from the page cache afterward" + ); + + std::fs::remove_file(&p).ok(); + } + #[test] fn cache_dir_is_under_the_volume_root() { assert_eq!(cache_dir("/volume01"), PathBuf::from("/volume01/osm"));