Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

54 changes: 54 additions & 0 deletions claude-notes/plans/2026-08-15-osm-lance-lifecycle.md
Original file line number Diff line number Diff line change
Expand Up @@ -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.
7 changes: 7 additions & 0 deletions crates/cockpit-server/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
96 changes: 96 additions & 0 deletions crates/cockpit-server/src/osm_slab_hydrate.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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<String> {
use std::io::Read;
let mut f = std::fs::File::open(path)?;
Expand All @@ -413,9 +426,57 @@ fn sha256_file(path: &Path) -> std::io::Result<String> {
}
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::*;
Expand Down Expand Up @@ -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"));
Expand Down