fix(cockpit-server): skip the full OSM slab read on a hot, unchanged boot - #137
Conversation
…nged 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 <artifact>.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 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NMeiLmtDKhomJNSo2ecbJw
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_68fa454c-af64-442f-9053-21ac4de4a0cc) |
|
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 |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Summary
ensure_lance_local's FNV-1a freshness digest andensure_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 changed. That's the remaining cause of the reported 20–30 minute RAM decline after a redeploy — reading gigabytes and hashing them is real wall-clock time and real (if transient) page-cache pressure, independent of how promptly it's evicted afterward.(mtime, len)unchanged since the last verification is conclusive proof the bytes are unchanged too — the same heuristicmake/rsync/cargo/ccacheall use by default.osm_lance.rs: two new Lance schema-metadata keys (soa:slab_mtime_nanos,soa:slab_len) alongside the existingsoa: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<artifact>.verifiedsidecar recording(mtime, len, digest)after every real verification (download or cache-hit). A matching marker skipssha256_fileentirely.osm_lifecycle.rs's Phase C/DImportSeal/warm_verification_readsmachinery (an S3-origin-proof design gated behind wiring — theOnceLock→OsmArtifactManagermigration — 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 later.Test plan
SLOW_PATH_HASH_ATTEMPTED; the existingFADVISE_ATTEMPTEDfrom fix(cockpit-server): evict OSM slab from page cache after checksum hashing #136, since a skippedsha256_filecall already proves itself via that counter not incrementing) — same discipline as osm lance: evict the slab mapping on warm reopen too, not just rebuild #135/fix(cockpit-server): evict OSM slab from page cache after checksum hashing #136, since RSS/cgroup accounting can't see "skipped a read" any more than it could see "evicted after one".osm_lance.rs'sa_hot_boot_skips_the_full_slab_hash_entirely(failsleft: 2, right: 1reverted, passes restored) andosm_slab_hydrate.rs'sresolve_cache_hit_trusts_a_matching_marker_without_hashing(fails reverted, passes restored).the_warm_reopen_path_attempts_eviction_too, 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.#[cfg(test)]reachability counters are process-wide statics, and plaincargo 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) isolates each test into its own process, which is what actually makes this counter pattern safe. All verification below uses nextest.cargo nextest run -p cockpit-server --bin q2-cockpit— 173/173 passed (11 new tests: 3 inosm_lance.rs, 8 inosm_slab_hydrate.rs), 0 regressions.rustfmt --edition 2024 --checkclean on all new code in both files (two real formatting fixes applied — awrite_lancecall and astat_identityone-liner; the crate-wide pre-existing drift documented in the plan doc is untouched).git stash: one genuinely new finding surfaced mid-work (collapsible_ifon the fast path's nestedif let, fixed with alet-chain per clippy's own suggestion) — final comparison is 68/0 warnings/errors on both the pre-change and post-change tree, byte-identical warning content.claude-notes/plans/2026-08-15-osm-lance-lifecycle.md§"Hot but idle" — actually skipping the read, local-only.std+ already-present crates (lance,sha2);Cargo.lock/Cargo.tomluntouched.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 redundant re-download landing 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.
Generated by Claude Code