From 993045d4247933217e09ff50055e81c37ddbe58e Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 16 Aug 2026 00:19:32 +0000 Subject: [PATCH] osm lance: evict the slab mapping on warm reopen too, not just rebuild The warm-check inside ensure_lance_local mmaps and FNV-1a-hashes the ENTIRE slab to decide the existing Lance dataset is still current -- unavoidable, that is how it knows -- but until now only the rebuild path called release_after_write() afterward. The warm-reopen path (the common case on a redeploy where nothing changed) just dropped the mapping with no MADV_DONTNEED. Measured in production (Railway's memory graph, 2026-08-15/16): a redeploy against an already-warm dataset held the slab resident for 1-3.5 hours before the kernel's own passive reclaim cleared it -- nothing forces reclaim under a 24 GB limit over a ~1.3 GB slab. Fix: call the same, already-proven release_after_write() on the warm branch too. TDD note: the first version of this test measured process RSS (/proc/self/statm) before/after a warm reopen, and it PASSED even with the fix reverted -- Drop's munmap already removes this process's page-table entries regardless of whether MADV_DONTNEED ran, so RSS can't see the difference. The real effect is in the kernel's page cache / cgroup memcg charge (/sys/fs/cgroup/memory.current), which this sandbox does not expose at all. Replaced with a #[cfg(test)]-only reachability counter (EVICTIONS_ATTEMPTED) that proves the call site is reached -- confirmed failing before the fix, passing after. Named follow-up (not attempted here): avoid touching every byte of the slab on a warm boot at all, via a bake-time sidecar digest instead of recomputing the hash server-side every time. Cross-repo (openstreetmap-website-rs + q2); this fix closes the more expensive half (residency) first. --- .../plans/2026-08-15-osm-lance-lifecycle.md | 43 +++++++++ crates/cockpit-server/src/osm_lance.rs | 93 +++++++++++++++++++ 2 files changed, 136 insertions(+) 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 c5510256d..1e120317e 100644 --- a/claude-notes/plans/2026-08-15-osm-lance-lifecycle.md +++ b/claude-notes/plans/2026-08-15-osm-lance-lifecycle.md @@ -450,3 +450,46 @@ unreachable because `Operation` cannot touch `serving` at all. "pointer last" and "zero traversal on warm boot" testable as decisions before any wiring exists — and also why every probe in this file proves a decision, never a running system; see the falsifiers table. + +## Found during Phase B: the warm-boot eviction gap (fixed same session) + +Railway's memory graph (2026-08-15/16) showed the pattern this whole plan +opens with, still live in the CURRENT (pre-Phase-C) code: a redeploy against +an **already-warm, unchanged** Lance dataset still spiked memory to ~4 GB and +held it there for **1–3.5 hours** before the kernel's own passive reclaim +cleared it — no active eviction, nothing forcing reclaim under a 24 GB limit +over a ~1.3 GB slab. + +Root cause, in `crates/cockpit-server/src/osm_lance.rs::ensure_lance_local`: +the warm-check itself mmaps and FNV-1a-hashes the **entire** slab (unavoidable +— that is how it knows nothing changed), but the rebuild path's eviction +(`release_after_write`, from the earlier "evict the bake from RAM" fix, +PR #132) was only wired to the REBUILD branch. The warm-reopen branch just +dropped the mapping with no `MADV_DONTNEED`. Fixed by calling the same, +already-proven `release_after_write` on the warm branch too — same mechanism, +the missing call site. + +**Testing note, worth keeping:** the first version of this fix's test measured +process RSS (`/proc/self/statm`) before/after a warm reopen — and it PASSED +even with the fix reverted, because `Drop`'s `munmap` already removes this +process's page-table entries regardless of whether `MADV_DONTNEED` ran. RSS +cannot see the difference; the real effect is in the kernel's page cache / +cgroup memcg charge (`/sys/fs/cgroup/memory.current`, unavailable in this dev +sandbox). Replaced with a `#[cfg(test)]`-only reachability counter +(`EVICTIONS_ATTEMPTED`) proving the call site is reached — a real falsifier +(confirmed failing before the fix, passing after), honestly scoped to what +this environment can prove. The production graph is the actual falsifier for +the memory claim itself. + +**Named follow-up, not attempted this session:** the fix stops the warm path +from LEAVING the slab resident, but does not stop it from TOUCHING every byte +of the slab on every single boot just to compute the freshness digest. A +cheaper design — matching the "hot but idle" framing the operator raised — +would read a small bake-time sidecar digest instead of recomputing FNV-1a +over the whole file, so a warm boot never faults the slab's pages in at all. +`hash_slab`'s own doc comment already notes a streaming form exists so the +BAKE side can hash while writing rather than re-reading; the SERVER side +(this warm-check) does not yet use anything like it. Left named rather than +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). diff --git a/crates/cockpit-server/src/osm_lance.rs b/crates/cockpit-server/src/osm_lance.rs index aa5a2a9bd..d2568eeea 100644 --- a/crates/cockpit-server/src/osm_lance.rs +++ b/crates/cockpit-server/src/osm_lance.rs @@ -165,6 +165,16 @@ pub async fn ensure_lance_local(slab_path: &Path) -> Option { path = %dest.display(), rows, "osm lance: warm — dataset already matches this slab's row count and digest" ); + // The warm path pays the SAME mmap+full-hash cost as a rebuild (it + // has to, to know it's warm) but until this line it paid that cost + // and then just dropped `map` — no `MADV_DONTNEED`, unlike the + // rebuild path's `release_after_write` below. On a redeploy where + // nothing changed (the common case in steady state), that left the + // whole slab resident with nothing to reclaim it under a memory + // limit far above what's needed to create pressure. Same fix, + // same reasoning, the other call site — see `release_after_write`'s + // own doc comment for the mechanism. + release_after_write(map, &dest); return Some(warm); } @@ -430,6 +440,24 @@ async fn write_lance( Some(dest.to_path_buf()) } +/// Reachability counter for [`release_after_write`], test builds only. +/// +/// `release_after_write`'s own return value already proves the MECHANISM +/// (evict when sole holder, decline otherwise) — see +/// `the_slab_mapping_is_released_only_when_nothing_else_holds_it` below. +/// What that test cannot prove is whether a given CALL SITE inside +/// `ensure_lance_local` actually reaches it: the warm-reopen branch used to +/// silently skip the call entirely, and process RSS cannot tell "evicted" +/// from "just dropped, kernel unmapped it anyway" apart (`Drop`'s `munmap` +/// removes this process's page-table entries either way — the difference +/// `MADV_DONTNEED` makes is to the kernel's PAGE CACHE / cgroup memcg +/// charge, which is invisible to `/proc/self/statm` and unavailable in +/// this sandbox at all: `/sys/fs/cgroup/memory.current` does not exist +/// here). This counter is the honest substitute: it proves the call site +/// is reached, which is exactly what regressed. +#[cfg(test)] +static EVICTIONS_ATTEMPTED: std::sync::atomic::AtomicUsize = std::sync::atomic::AtomicUsize::new(0); + /// Release the slab mapping once the dataset is written, if we are the last /// holder of it. /// @@ -442,6 +470,8 @@ async fn write_lance( /// reads these pages, and yanking them would be wrong. It is logged rather /// than evicted-anyway or passed over in silence. fn release_after_write(map: std::sync::Arc, dest: &Path) -> bool { + #[cfg(test)] + EVICTIONS_ATTEMPTED.fetch_add(1, std::sync::atomic::Ordering::Relaxed); match std::sync::Arc::try_unwrap(map) { Ok(m) => { advise_dontneed(&m, dest); @@ -737,6 +767,69 @@ mod tests { ); } + /// PROBE. The bug this test exists to catch: `ensure_lance_local`'s + /// WARM path (an already-current Lance dataset, so `reopen_if_warm` + /// succeeds) mmaps and hashes the FULL slab to decide it is warm — it + /// has to, that is how it knows — but used to just drop the mapping + /// afterward with no `MADV_DONTNEED`, unlike the rebuild path's + /// `release_after_write`. Measured in production (Railway's memory + /// graph, 2026-08-15/16): a redeploy against an already-warm dataset + /// held the slab's pages resident for 1-3.5 HOURS before the kernel's + /// own passive reclaim finally cleared them — nothing forces reclaim + /// under a 24 GB limit over a ~1.3 GB file, since that never creates + /// real memory pressure. + /// + /// This is a reachability probe, not a memory measurement, and that is + /// a deliberate downgrade from a first attempt at this test: a version + /// that measured `/proc/self/statm` RSS before/after a warm reopen + /// PASSED even with the fix reverted, because `Drop`'s `munmap` already + /// removes this process's page-table entries regardless of whether + /// `MADV_DONTNEED` ran — RSS cannot see the difference. The actual + /// production effect is in the kernel's page cache / cgroup memcg + /// charge, which `/sys/fs/cgroup/memory.current` reports and this + /// sandbox does not expose at all. `EVICTIONS_ATTEMPTED` proves the + /// one fact this environment CAN prove: that the warm branch reaches + /// `release_after_write` at all, which is exactly what regressed. + /// + /// 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). + #[tokio::test] + async fn the_warm_reopen_path_attempts_eviction_too() { + let rows = 4usize; + let dir = tempfile::tempdir().expect("tempdir"); + let slab_path = dir.path().join("warm.soa"); + std::fs::write(&slab_path, vec![0u8; rows * NODE_ROW_STRIDE]).expect("write slab"); + + let before = EVICTIONS_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 = EVICTIONS_ATTEMPTED.load(std::sync::atomic::Ordering::Relaxed); + assert_eq!( + after_cold - before, + 1, + "the cold/rebuild path must attempt eviction exactly once" + ); + + let second = ensure_lance_local(&slab_path).await; + assert_eq!( + second, first, + "an unchanged slab must reopen the SAME dataset warm" + ); + let after_warm = EVICTIONS_ATTEMPTED.load(std::sync::atomic::Ordering::Relaxed); + assert_eq!( + after_warm - after_cold, + 1, + "the warm-reopen path must ALSO attempt eviction — this is the regression: \ + it used to silently drop the mapping without ever calling \ + `release_after_write`, leaving the slab's pages resident with nothing to \ + reclaim them until the kernel's own passive reclaim eventually noticed" + ); + } + #[test] fn a_row_count_under_the_ceiling_builds_a_valid_array() { let rows = 1_000usize; // Berlin-scale, nowhere near the 4.19M cap