From b5cf9a8fb6232f8e76c990788502b99d4a3320f2 Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 12 Aug 2026 06:55:53 +0000 Subject: [PATCH] osm slab hydration: never fail silently on missing config MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The reason the deploy log had nothing to grep for: ensure_slab_local() returned None via a bare `.ok()?` on AWS_S3_BUCKET_NAME and OSM_SLAB_CACHE_DIR/RAILWAY_VOL, with zero logging on that path, and main.rs's caller has no `else` branch on None. A deploy missing either var produces LITERALLY NOTHING about OSM slab hydration in the boot log — not a warning, not an error. Searching for "osm slab" was never going to find anything on that path, which is the whole reason today's diagnosis stalled. Found by comparing against medcare-rs's bake_hydrate.rs / bake_s3.rs — the same boot-time S3-hydration-onto-a-volume shape, hardened after its own documented incident ("the silent-empty deploy earlier in this arc"). It always logs, even in the nothing-configured case, and names the exact variables needed: "no source configured — ... neither S3 (AWS_ENDPOINT_URL / AWS_S3_BUCKET_NAME / AWS_ACCESS_KEY_ID / AWS_SECRET_ACCESS_KEY) nor GITHUB_TOKEN is set" It also treats an EMPTY variable value the same as absent (`.filter(|v| !v.is_empty())` in S3Config::from_env) — a Railway variable can exist as a row with a blank value, and that must fail the same way as not existing, not attempt a real S3 call with an empty bucket name and fail later, differently, and less legibly. Ported both properties into osm_slab_hydrate.rs, scoped to logging only — the actual hydration logic (fetch, checksum-verify, cache) is unchanged: - env_var_nonempty(): empty treated as absent, matching bake_s3's rule. - missing_inputs(): pure function naming every absent piece (not just the first one hit) — testable without touching real env vars, since ensure_slab_local() is the only caller that reads std::env::var. - ensure_slab_local() now WARNs and names the missing variable(s) before returning None for a missing-config reason. Two new tests, both on the pure missing_inputs() core: names every absent variable (not just the first — the regression this fix is for), and stays empty (anti-vacuity) when both inputs are present. 97 passed, 0 failed (95 prior + 2 new). Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_01NMeiLmtDKhomJNSo2ecbJw --- crates/cockpit-server/src/main.rs | 5 +- crates/cockpit-server/src/osm_slab_hydrate.rs | 100 ++++++++++++++++-- 2 files changed, 98 insertions(+), 7 deletions(-) diff --git a/crates/cockpit-server/src/main.rs b/crates/cockpit-server/src/main.rs index 137729e6f..7d938ea93 100644 --- a/crates/cockpit-server/src/main.rs +++ b/crates/cockpit-server/src/main.rs @@ -190,7 +190,10 @@ async fn main() { // it still just mmaps `OSM_SLAB_PATH` — and stops the first request paying // for a 1.29 GiB download. A `None` here is not an error: the endpoint // answers 503 exactly as it did before, and local dev sets OSM_SLAB_PATH - // directly and never reaches S3. + // directly and never reaches S3. `ensure_slab_local` itself WARNs and + // names the missing variable(s) when it returns `None` for a missing- + // config reason — this call site deliberately stays silent on `None` + // rather than duplicating that message. if let Some(path) = osm_slab_hydrate::ensure_slab_local().await { // SAFETY: single-threaded startup, before any task or listener exists. unsafe { std::env::set_var("OSM_SLAB_PATH", &path) }; diff --git a/crates/cockpit-server/src/osm_slab_hydrate.rs b/crates/cockpit-server/src/osm_slab_hydrate.rs index 4ac933196..5ffc961a8 100644 --- a/crates/cockpit-server/src/osm_slab_hydrate.rs +++ b/crates/cockpit-server/src/osm_slab_hydrate.rs @@ -41,11 +41,30 @@ //! only after the hash matches, so a killed container leaves no file that //! looks complete — but the volume outlives this code, so the check stays. //! -//! # Absent configuration is not an error +//! # Absent configuration is not an error — but it must never be SILENT either //! //! No bucket, no volume, no credentials ⇒ `None`, and the endpoint keeps //! answering 503 exactly as it did before this module existed. Local //! development sets `OSM_SLAB_PATH` directly and never reaches S3. +//! +//! The first version of this module returned that `None` via a bare `.ok()?` +//! on each required var, with `main.rs`'s caller having no `else` branch — +//! meaning a deploy missing `AWS_S3_BUCKET_NAME` or `RAILWAY_VOL` produced +//! **zero lines in the boot log about OSM slab hydration at all**. That is +//! the exact "silent-empty deploy" failure mode `medcare-rs::bake_hydrate` +//! documents hitting and fixing first (its own module doc: "the store already +//! applies this rule... this module matches it"). This module ported that +//! fix: [`missing_inputs`] + the `tracing::warn!` in [`ensure_slab_local`] +//! below always name exactly which variable is absent, so the very next +//! deploy's log settles the question instead of a search for a line that +//! structurally cannot exist. +//! +//! `medcare-rs::bake_s3::S3Config::from_env` also treats an EMPTY variable +//! value the same as an absent one (`.filter(|v| !v.is_empty())`) — a blank +//! Railway variable (row present, value never filled in) must fail exactly +//! like an unset one, not attempt a doomed S3 call with an empty bucket name +//! and fail differently (and more slowly) than the "never configured" case. +//! [`env_var_nonempty`] carries the same rule here. use std::path::{Path, PathBuf}; @@ -69,6 +88,34 @@ fn cache_dir(vol: &str) -> PathBuf { Path::new(vol).join("osm") } +/// `std::env::var`, but an empty value is treated the same as absent. +/// +/// Mirrors `medcare-rs::bake_s3::S3Config::from_env`'s reader: a Railway +/// variable can exist as a row with an empty value (created, never filled +/// in), and that must fail the SAME way as the variable not existing at all — +/// not attempt a real S3 call with an empty bucket name, which fails later, +/// differently, and less legibly than "not configured". +fn env_var_nonempty(key: &str) -> Option { + std::env::var(key).ok().filter(|v| !v.trim().is_empty()) +} + +/// Which required inputs are missing, given what was actually resolved from +/// the environment. +/// +/// Pure and side-effect-free on purpose: [`ensure_slab_local`] is the only +/// place that touches `std::env::var`, so this can be tested with plain +/// `Option`s and never needs to mutate real process environment state. +fn missing_inputs(bucket: Option<&str>, vol: Option<&str>) -> Vec<&'static str> { + let mut missing = Vec::new(); + if bucket.is_none() { + missing.push("AWS_S3_BUCKET_NAME"); + } + if vol.is_none() { + missing.push("OSM_SLAB_CACHE_DIR (or RAILWAY_VOL)"); + } + missing +} + /// Resolve a local, verified slab path, hydrating from S3 if needed. /// /// Returns the path to set as `OSM_SLAB_PATH`, or `None` when the feature is @@ -87,13 +134,28 @@ pub async fn ensure_slab_local() -> Option { } // 2. Otherwise hydrate. Both a bucket and a destination are required. - let bucket = std::env::var("AWS_S3_BUCKET_NAME").ok()?; + let bucket_env = env_var_nonempty("AWS_S3_BUCKET_NAME"); // `OSM_SLAB_CACHE_DIR` overrides the volume — it is what makes this // testable off-Railway, where /volume01 does not exist. - let vol = std::env::var("OSM_SLAB_CACHE_DIR") - .or_else(|_| std::env::var("RAILWAY_VOL")) - .ok()?; - let prefix = std::env::var("OSM_SLAB_S3_PREFIX").unwrap_or_else(|_| DEFAULT_PREFIX.to_string()); + let vol_env = + env_var_nonempty("OSM_SLAB_CACHE_DIR").or_else(|| env_var_nonempty("RAILWAY_VOL")); + + // See the module doc's "must never be SILENT" section: this WARN is what + // was missing before, and it is the entire fix — everything below this + // block is unchanged hydration logic. + let missing = missing_inputs(bucket_env.as_deref(), vol_env.as_deref()); + if !missing.is_empty() { + tracing::warn!( + missing = %missing.join(", "), + "osm slab: hydration not configured — set {} to enable the drawn basemap; \ + the vector-basemap and feature-dot endpoints stay 503 until then", + missing.join(", "), + ); + return None; + } + let bucket = bucket_env.expect("checked non-empty above"); + let vol = vol_env.expect("checked non-empty above"); + let prefix = env_var_nonempty("OSM_SLAB_S3_PREFIX").unwrap_or_else(|| DEFAULT_PREFIX.to_string()); let dir = cache_dir(&vol); if let Err(e) = std::fs::create_dir_all(&dir) { @@ -357,4 +419,30 @@ not-a-hash junk.txt fn cache_dir_is_under_the_volume_root() { assert_eq!(cache_dir("/volume01"), PathBuf::from("/volume01/osm")); } + + /// **The regression this whole fix is for.** Before this change, + /// `ensure_slab_local` returned `None` here via a bare `.ok()?` with no + /// logging at all — a deploy missing either var produced zero lines about + /// OSM slab hydration in the boot log. `missing_inputs` is the pure core + /// of the fix: given what `ensure_slab_local` actually resolved, it must + /// name EVERY absent piece, not stop at the first. + #[test] + fn missing_inputs_names_every_absent_variable() { + assert_eq!(missing_inputs(None, None).len(), 2, "both absent: both named"); + assert_eq!( + missing_inputs(Some("my-bucket"), None), + vec!["OSM_SLAB_CACHE_DIR (or RAILWAY_VOL)"] + ); + assert_eq!( + missing_inputs(None, Some("/volume01")), + vec!["AWS_S3_BUCKET_NAME"] + ); + } + + /// Anti-vacuity: when both are present, nothing is reported missing — the + /// function does not just always return a non-empty list. + #[test] + fn missing_inputs_is_empty_when_both_are_present() { + assert!(missing_inputs(Some("my-bucket"), Some("/volume01")).is_empty()); + } }