osm lance: evict the slab mapping on warm reopen too, not just rebuild - #135
Conversation
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.
|
Important Review available on request
Reviews should be triggered manually for repositories with fewer than 10 stars. Select Trigger review above or comment ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Bugbot couldn't run - usage limit reachedBugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit. A user or team admin can review and increase usage limits in the Cursor dashboard. (requestId: serverGenReqId_3d3f0a95-80c4-4fc7-a7ec-d52d2ce8a85d) |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Summary
Real production regression, found while checking on Phase B: Railway's memory
graph (2026-08-15/16) showed the exact pattern this whole OSM/Lance lifecycle
plan opens with, still live in the current code — a redeploy against an
already-warm, unchanged Lance dataset spiked memory to ~4 GB and held it
there 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.
Root cause —
crates/cockpit-server/src/osm_lance.rs::ensure_lance_local:the warm-check mmaps and FNV-1a-hashes the entire slab to confirm the
existing Lance dataset is still current (unavoidable — that's how it knows),
but
release_after_write(the "evict the bake from RAM" fix, PR #132) wasonly wired to the rebuild branch. The warm-reopen branch — the common
case on a redeploy where nothing actually changed — just dropped the mapping
with no
MADV_DONTNEED.Fix: call the same, already-proven
release_after_writeon the warmbranch too. Same mechanism (already unit-tested in isolation via
the_slab_mapping_is_released_only_when_nothing_else_holds_it), the missingcall site.
A test-honesty note worth flagging explicitly
The first version of this fix's regression test measured process RSS
(
/proc/self/statm) before/after a warm reopen — and it passed even withthe fix reverted. Root cause:
Drop'smunmapalready removes thisprocess's page-table entries regardless of whether
MADV_DONTNEEDran, soprocess RSS cannot distinguish the two cases. The actual production effect is
in the kernel's page cache / cgroup memcg charge
(
/sys/fs/cgroup/memory.current), which this dev sandbox does not expose atall (
cat /sys/fs/cgroup/memory.current→ "No such file or directory" here).Replaced with a
#[cfg(test)]-only reachability counter(
EVICTIONS_ATTEMPTED) that proves the warm branch actually reachesrelease_after_write— a genuine falsifier, verified failing before the fix(
left: 0, right: 1) and passing after. It proves reachability, not thememory-residency outcome itself; the production graph after this deploys is
the real falsifier for that claim.
What's NOT in this PR
Named as a follow-up in the plan (
claude-notes/plans/2026-08-15-osm-lance-lifecycle.md),not attempted here: the fix stops the warm path from leaving the slab
resident, but not from touching every byte of it on every boot just to
compute the freshness digest. A cheaper design would read a small bake-time
sidecar digest instead of recomputing FNV-1a over the whole file server-side
every time —
hash_slab's own doc comment already notes a streaming formexists for the bake side to use. Cross-repo (
openstreetmap-website-rs+q2); this fix closes the more expensive half (residency) first.Test plan
cargo test -p cockpit-server— 161/161 pass (1 new test, 0 regressions)(
left: 0, right: 1), restored the fix, confirmed it passescargo clippy -p cockpit-server --no-deps --all-targets -- -D warnings— verified via
git stashcomparison against the unmodified branch tip:53 findings before AND after this diff — this PR contributes zero
rustfmt --edition 2024 --check crates/cockpit-server/src/osm_lance.rs— clean (leaf module, safe to check directly)
🤖 Generated with Claude Code
Generated by Claude Code