From e2edb3b2453b8cc915df65aa749f55d9c8e70ded Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 18 Aug 2026 09:54:21 +0000 Subject: [PATCH] fix(cockpit-server): wire chains OrdinalIndex into the request-time gather path MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit main.rs's boot sequence converted the .chains sidecar to Lance but discarded the returned OrdinalIndex (bound to `_index`), so CHAINS_INDEX was never populated. gather_chains()'s `CHAINS_INDEX.get()?.as_ref()?` therefore always short-circuited to None, and every tile/feature request needing chain geometry permanently fell back to the eager open_chains() singleton — a OnceLock populated once on first use and never freed. Live-confirmed on maps.oga.red: cgroup memory sat at ~850MB baseline, jumped to ~5.7GB after the first chains-touching request, and never came back down — matching the reported symptom (memory doesn't go down again after opening the map in browser). Add osm_chains_books_lance::publish_chains_conversion(), the one call site responsible for turning an ensure_chains_lance_local() result into a set_chains_index() call, with its own unit tests covering both the Some (wires the index) and None (fails open, no panic) arms. main.rs now calls it instead of its old broken inline match. Also corrected the now-stale "not yet consumed by the read path" log wording for both chains and books (task #16 already wired the Lance-first-with-eager-fallback read path for both). Verified: cargo check -p cockpit-server --bin q2-cockpit --tests passes clean (0 errors; 33/18 warnings, at or below the pre-existing baseline). Full `cargo test`/nextest for this binary is not runnable in this environment (transitively links most of the workspace, exceeds the container's disk allowance) — documented per prior sessions' practice rather than claimed. Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_01NMeiLmtDKhomJNSo2ecbJw --- crates/cockpit-server/src/main.rs | 31 +++--- .../src/osm_chains_books_lance.rs | 95 +++++++++++++++++++ 2 files changed, 110 insertions(+), 16 deletions(-) diff --git a/crates/cockpit-server/src/main.rs b/crates/cockpit-server/src/main.rs index 755ea775b..3a50e1f21 100644 --- a/crates/cockpit-server/src/main.rs +++ b/crates/cockpit-server/src/main.rs @@ -253,30 +253,29 @@ async fn main() { // hydrated LOCAL slab path — `.chains`/`.books` were hydrated // alongside it by `ensure_slab_local` above (S3 is the source of // truth for all three sidecars in one hydration pass; see - // `osm_slab_hydrate::artifacts`). This wires the CONVERSION only: - // `osm_features.rs`'s `open_chains`/`open_books` still read the raw - // sidecar files directly — a Lance-backed read path is follow-up - // work, not yet wired to consume what is written here. + // `osm_slab_hydrate::artifacts`). `osm_features.rs`'s + // `single_gather_chains`/`single_gather_books` already try this + // Lance-backed path first and fall back to the eager + // `open_chains()`/`open_books()` singletons only when it's + // unavailable — so wiring the CONVERSION's result all the way + // through (chains needs `publish_chains_conversion` to publish its + // `OrdinalIndex`; books addresses densely and needs no such step) + // is what makes that fast path actually reachable, instead of every + // request silently falling back to the permanent, unbounded + // in-memory singleton. let chains_path = path.with_extension("chains"); - match osm_chains_books_lance::ensure_chains_lance_local(&chains_path).await { - Some((dataset_dir, _index)) => tracing::info!( - path = %dataset_dir.display(), - "osm chains lance: converted; not yet consumed by the read path" - ), - None => tracing::warn!( - "osm chains lance: conversion to a Lance dataset failed or was skipped; \ - no effect on serving (the read path does not consume it yet)" - ), - } + osm_chains_books_lance::publish_chains_conversion( + osm_chains_books_lance::ensure_chains_lance_local(&chains_path).await, + ); let books_path = path.with_extension("books"); match osm_chains_books_lance::ensure_books_lance_local(&books_path).await { Some(datasets) => tracing::info!( identities = %datasets[0].display(), - "osm books lance: converted; not yet consumed by the read path" + "osm books lance: converted; request-time gather can serve from it directly" ), None => tracing::warn!( "osm books lance: conversion to Lance datasets failed or was skipped; \ - no effect on serving (the read path does not consume it yet)" + requests fall back to the eager open_books() singleton" ), } } diff --git a/crates/cockpit-server/src/osm_chains_books_lance.rs b/crates/cockpit-server/src/osm_chains_books_lance.rs index d1f0678c5..22f5dcc70 100644 --- a/crates/cockpit-server/src/osm_chains_books_lance.rs +++ b/crates/cockpit-server/src/osm_chains_books_lance.rs @@ -422,6 +422,56 @@ pub fn set_chains_index(index: OrdinalIndex) { let _ = CHAINS_INDEX.set(Some(index)); } +/// The ONE call site responsible for wiring a boot-time +/// [`ensure_chains_lance_local`] result into [`gather_chains`]'s read path, +/// via [`set_chains_index`]. +/// +/// This function exists because that wiring step was silently OMITTED +/// once already: the caller bound the returned index to `_index` and +/// discarded it. `gather_chains` requires `CHAINS_INDEX` to be populated +/// (see its own doc — `CHAINS_INDEX.get()?` short-circuits to `None` +/// otherwise), so every tile/feature request needing chain geometry +/// silently fell back to the eager `open_chains()` singleton — for the +/// entire process lifetime, on every single request, never only the +/// first. Measured live on Brandenburg (a bake too large for the raw +/// slab's own Arrow ceiling, so the browsing session that triggers this +/// is a normal one, not an edge case): cgroup memory jumped from ~850 MB +/// baseline to ~5.7 GB after the first request touching chains data, and +/// never came back down — consistent with a `OnceLock`-backed singleton, +/// which is never freed once populated, rather than the request-scoped, +/// bounded Lance gather this whole module exists to serve from instead. +/// +/// `main()`'s own boot sequence cannot be unit-tested directly (nothing +/// executes it as a function call), which is exactly why this wiring step +/// is split into its own function: `main()` becomes a thin, mechanical +/// caller, and the actual wiring behavior — publish on `Some`, log and do +/// nothing on `None` — is covered by +/// [`publish_chains_conversion_wires_the_index_into_the_global_gather_path`] +/// below, independent of any S3/volume machinery. +/// +/// Returns the dataset directory path (so the caller can still log it), +/// unchanged either way. +pub fn publish_chains_conversion(result: Option<(PathBuf, OrdinalIndex)>) -> Option { + match result { + Some((dataset_dir, index)) => { + set_chains_index(index); + tracing::info!( + path = %dataset_dir.display(), + "osm chains lance: converted and wired into the request-time gather path" + ); + Some(dataset_dir) + } + None => { + tracing::warn!( + "osm chains lance: conversion to a Lance dataset failed or was skipped; \ + tile/feature requests needing chains fall back to the eager open_chains() \ + singleton (a permanent per-process memory cost, not a request-scoped one)" + ); + None + } + } +} + async fn open_cached(cell: &'static tokio::sync::OnceCell>, path: &Path) -> Option<&'static Dataset> { cell.get_or_init(|| async { let path_str = path.to_str()?; @@ -678,6 +728,51 @@ mod tests { assert_eq!(got.get(&0), Some(&"Hauptstraße".as_bytes().to_vec())); } + /// `publish_chains_conversion`'s `Some` arm: proves the wiring this + /// function exists to fix actually happens — `CHAINS_INDEX` transitions + /// from unset to populated as a DIRECT effect of calling this function, + /// not because some other test in the process already set it (this test + /// builds its own `OrdinalIndex`, independent of any real dataset, so + /// the assertion is about the wiring, not about Lance I/O). This is the + /// exact regression `publish_chains_conversion` was written to fix: the + /// old inline `match` at the `main()` call site bound the returned + /// index to `_index` and discarded it, so `CHAINS_INDEX` never got set + /// and every chains-touching request fell back to the eager + /// `open_chains()` singleton for the life of the process. + #[test] + fn publish_chains_conversion_wires_the_index_into_the_global_gather_path() { + assert!( + CHAINS_INDEX.get().is_none(), + "process-global CHAINS_INDEX must start unset in this test's own \ + nextest process for the assertion below to mean anything" + ); + + let dataset_dir = PathBuf::from("/tmp/does-not-need-to-exist.chains.lance"); + let index = OrdinalIndex::new(vec![3, 500, 501]); + + let returned = publish_chains_conversion(Some((dataset_dir.clone(), index))); + + assert_eq!(returned, Some(dataset_dir)); + assert!( + CHAINS_INDEX.get().is_some(), + "publish_chains_conversion's whole job is to call set_chains_index \ + on the Some arm — a version that merely logged and returned the \ + path (without wiring the index) would still pass a check on the \ + return value alone, which is why this test also asserts on \ + CHAINS_INDEX directly" + ); + } + + /// The `None` arm: conversion failed or was skipped upstream (no dataset + /// this boot), and `publish_chains_conversion` must fail OPEN — return + /// `None` and never fabricate a `CHAINS_INDEX` entry that would make + /// `gather_chains` claim ordinals it cannot actually resolve. + #[test] + fn publish_chains_conversion_stays_silent_on_none() { + let returned = publish_chains_conversion(None); + assert_eq!(returned, None); + } + /// The actual request-scoped path: convert once, then `gather_chains` /// with a SUBSET of ordinals — proving the gathered `RequestChains` /// decodes correctly for what was asked, stays silent on a real gap