diff --git a/server/src/atlas/layer_proxy.rs b/server/src/atlas/layer_proxy.rs index c4bee2ea7..fc7b30768 100644 --- a/server/src/atlas/layer_proxy.rs +++ b/server/src/atlas/layer_proxy.rs @@ -18,10 +18,11 @@ use crate::atlas::body_params_reader::BodyParamsReader; use crate::atlas::body_world_state::{BodyWorldState, BodyWorldStateCache, SimTick}; use crate::atlas::cascade::CascadeLayer; use crate::atlas::city_context_reader::CityContextReader; -use crate::atlas::district_profile::DistrictPos; +use crate::atlas::district_profile::{BodyParams, DistrictPos}; use crate::atlas::gen_queue::{GenPriority, GenWorkItem, GenerationQueue}; use crate::atlas::layer1::Layer1Output; use crate::atlas::road_graph::RoadNodeKind; +use crate::atlas::scale::DISTRICT_M; use crate::atlas::source_resolver::{BodySourceResolver, SourceResolveError}; use crate::bridge::ConnectionId; use crate::seed::{SeedChain, SeedDomain}; @@ -745,6 +746,64 @@ pub fn build_settlement_layer(state: &BodyWorldState) -> Option Some(SettlementLayer { settlements }) } +/// Normalize a wire-supplied district-window centre against the body's +/// physical geometry (T-1142 — a letterbox-click bug sent `window_center` +/// wildly out of a body's valid range; the server accepted it, derived +/// clamped garbage inside `derive_district`, and CACHED that garbage under +/// the raw un-normalized key). Mirrors `district_profile::derive_district`'s +/// own forward mapping (`district_profile.rs:1426-1451`) EXACTLY, so a window +/// centre that survives this normalization derives identically to how +/// `derive_district` would have resolved it anyway — this function only +/// closes the gap between "derive_district silently clamps/wraps internally" +/// and "the SERVING path (cache key, coalescing key) saw the raw value". +/// +/// **Column (longitude) wraps** — `rem_euclid` against the body's +/// circumference in districts, mirroring the forward map's +/// `(wx / circumference_m).rem_euclid(1.0)` (longitude is periodic; a click +/// at column 12276 on a body whose circumference is a few hundred districts +/// wide is the SAME point as some small in-range column, not garbage). +/// +/// **Row (latitude) clamps** — to `±half_meridian_districts`, mirroring the +/// forward map's `(wy / meridian_m).clamp(-0.5, 0.5)` (latitude is NOT +/// periodic; it terminates at the poles, so out-of-range rows collapse to the +/// nearest pole rather than wrapping — same asymmetry `derive_district` +/// itself already encodes). +/// +/// Both bounds are derived from `body_radius_km` via the SAME `DISTRICT_M` +/// (2 048 m, D-243) constant the forward map uses — no independent magic +/// numbers that could silently drift out of sync with `derive_district`. +/// +/// **No radius (`body_radius_km` absent/non-positive — tiny test bodies +/// only, per `BodyParams`'s own doc: "e.g. tiny test bodies"; every real +/// `systems.db` body row carries a radius):** identity, no wrap/clamp. The +/// forward map's own no-radius branch has no periodicity concept either (it +/// clamps the FRACTIONAL PIXEL position directly against the heightmap's +/// working-grid dimensions, which aren't known at request-serving time — only +/// inside the Rayon work item once the heightmap is loaded); a real letterbox +/// click can never hit this branch, so it is out of this fix's scope. +fn normalize_window_center(params: &BodyParams, center: DistrictPos) -> DistrictPos { + let (dx, dy) = center; + match params.body_radius_km { + Some(r_km) if r_km > 0.0 => { + let circumference_m = std::f64::consts::TAU * r_km * 1000.0; + let meridian_m = std::f64::consts::PI * r_km * 1000.0; + // Whole districts per full circumference / per half-meridian — + // rounded (not truncated) so the bound matches the forward map's + // continuous fraction as closely as an integer district grid can. + let districts_per_circumference = + (circumference_m / DISTRICT_M as f64).round().max(1.0) as i32; + let half_meridian_districts = (meridian_m / DISTRICT_M as f64 / 2.0).round() as i32; + + let wrapped_dx = dx.rem_euclid(districts_per_circumference); + let clamped_dy = dy.clamp(-half_meridian_districts, half_meridian_districts); + (wrapped_dx, clamped_dy) + } + // No radius: derive_district's own fallback has no wrap/clamp concept + // at the DistrictPos level (see the doc above) — identity. + _ => center, + } +} + /// Resolve `req`'s district-window query, if any (D-226 T-1124 amendment, /// T-1137). Returns `None` immediately when `req.window_center` is absent (no /// window requested — the common case, zero cost). @@ -756,15 +815,28 @@ pub fn build_settlement_layer(state: &BodyWorldState) -> Option /// about to take its cache-hit or cache-miss branch, sharing neither's control /// flow. /// -/// Cache hit (`(body_id, center, n)` already in `window_cache`) → `Some` -/// immediately, no queue submission (D-227: a previously-derived window for -/// this body+seed is valid forever, no staleness check needed). Cache miss → -/// submit a `DeriveWindow` work item (queue-based, per the amendment's binding -/// serving model — never inline here) and return `None`; the *next* request -/// for this `(body, center, n)` re-checks the cache and finds it populated -/// once `drain_generation_completions` has processed the completion (the -/// existing D-225 poll-and-recheck-cache pattern every other layer already -/// uses, not a push). +/// **`window_center` is normalized via [`normalize_window_center`] BEFORE the +/// `DistrictWindowCache` key AND the `submit_window` coalescing key are built** +/// (T-1142) — this is why `body_params` is read HERE, unconditionally, +/// rather than only inside the former miss-branch: normalization needs +/// `body_radius_km` to compute the wrap/clamp bounds, and it must happen +/// before either key exists, or an insane request and its sane normalized +/// twin would land in different cache entries / coalesce independently +/// (exactly the bug this fix closes — a garbage `window_center` was cached +/// standalone instead of collapsing onto its valid twin). The one-time cost +/// (a single indexed `bodies` row read) is paid on every window request now, +/// not just on a cache miss — a request whose normalized center hits the +/// cache still needed this read to know WHICH key to check. +/// +/// Cache hit (`(body_id, normalized_center, n)` already in `window_cache`) → +/// `Some` immediately, no queue submission (D-227: a previously-derived +/// window for this body+seed is valid forever, no staleness check needed). +/// Cache miss → submit a `DeriveWindow` work item (queue-based, per the +/// amendment's binding serving model — never inline here) and return `None`; +/// the *next* request for this `(body, normalized_center, n)` re-checks the +/// cache and finds it populated once `drain_generation_completions` has +/// processed the completion (the existing D-225 poll-and-recheck-cache +/// pattern every other layer already uses, not a push). /// /// `window_n` is clamped to `[1, DISTRICT_WINDOW_MAX_N]` here — the ONE place /// that clamp is applied; nothing downstream re-checks the wire value. @@ -778,29 +850,15 @@ fn serve_district_window( world_seed: u64, conn_id: ConnectionId, ) -> Option { - let center = req.window_center?; + let raw_center = req.window_center?; let n = req.window_n.clamp(1, DISTRICT_WINDOW_MAX_N); - let key: DistrictWindowKey = (req.body_id.clone(), center, n); - if let Some(layer) = window_cache.get(&key) { - return Some(layer.clone()); - } - - // Miss — resolve heightmap + body params and submit a background derive. - // Read/resolve failures are non-fatal for the window (log + skip): the - // window simply stays None on this response, same as an unrun whole-body - // layer, rather than failing the entire AtlasLayerResponse. - let heightmap_path = match resolver.resolve(&req.body_id) { - Ok(p) => p, - Err(e) => { - tracing::warn!( - body_id = %req.body_id, - error = %e, - "district window request: heightmap resolve failed — window stays None" - ); - return None; - } - }; + // body_params is needed to normalize the centre BEFORE either cache key + // exists (T-1142) — read it first, unconditionally (not gated on a cache + // miss like the former structure). A read failure here can't distinguish + // "insane vs. sane center" for the key, so it's a hard skip for the whole + // window (window stays None on this response), same failure posture the + // former miss-only read already had. let Some(reader) = body_params_reader else { tracing::warn!( body_id = %req.body_id, @@ -820,6 +878,37 @@ fn serve_district_window( } }; + let center = normalize_window_center(&body_params, raw_center); + if center != raw_center { + tracing::debug!( + body_id = %req.body_id, + raw = ?raw_center, + normalized = ?center, + "district window request: out-of-range window_center normalized (T-1142)" + ); + } + + let key: DistrictWindowKey = (req.body_id.clone(), center, n); + if let Some(layer) = window_cache.get(&key) { + return Some(layer.clone()); + } + + // Miss — resolve the heightmap and submit a background derive. + // Read/resolve failures are non-fatal for the window (log + skip): the + // window simply stays None on this response, same as an unrun whole-body + // layer, rather than failing the entire AtlasLayerResponse. + let heightmap_path = match resolver.resolve(&req.body_id) { + Ok(p) => p, + Err(e) => { + tracing::warn!( + body_id = %req.body_id, + error = %e, + "district window request: heightmap resolve failed — window stays None" + ); + return None; + } + }; + queue.submit_window( GenWorkItem::DeriveWindow { body_id: req.body_id.clone(), @@ -1434,6 +1523,284 @@ mod tests { ); } + // ------------------------------------------------------------------- + // normalize_window_center (T-1142 — letterbox-click out-of-range bug) + // ------------------------------------------------------------------- + + /// A small-body fixture (500 km radius) whose bounds are hand-checkable: + /// `districts_per_circumference = round(TAU*500_000/2048) = 1534`, + /// `half_meridian_districts = round(PI*500_000/2048/2) = 383`. The + /// literal column/row this fixture uses (`12276`, `3021`) are the exact + /// values the reported T-1142 letterbox-click bug sent — at THIS radius + /// they genuinely overflow both bounds (at Earth radius, coincidentally, + /// they wouldn't — the bounds are tens of thousands of districts wide), + /// so this is a faithful small-body reproduction, not just an + /// arbitrarily-chosen out-of-range pair. + fn small_body_params() -> BodyParams { + BodyParams { + hydrosphere: Some("ocean".into()), + atmosphere: Some("breathable".into()), + planet_class: Some("temperate".into()), + body_radius_km: Some(500.0), + ..Default::default() + } + } + + /// Out-of-range COLUMN wraps (longitude is periodic) to its in-range + /// canonical twin via `rem_euclid` — mirroring `derive_district`'s own + /// `(wx / circumference_m).rem_euclid(1.0)` forward map. + #[test] + fn normalize_window_center_wraps_out_of_range_column() { + let params = small_body_params(); + // districts_per_circumference = 1534 (hand-computed above). + // 12276 rem_euclid 1534 = 4 (12276 = 8*1534 + 4). + assert_eq!( + 12276_i32.rem_euclid(1534), + 4, + "sanity: the hand-computed wrap" + ); + let (nx, ny) = normalize_window_center(¶ms, (12276, 0)); + assert_eq!( + nx, 4, + "out-of-range column wraps to its canonical in-range twin" + ); + assert_eq!(ny, 0, "an in-range row is untouched"); + + // The canonical twin normalizes to itself (idempotent). + let (nx2, _) = normalize_window_center(¶ms, (4, 0)); + assert_eq!(nx2, 4); + + // Negative columns wrap too (rem_euclid, not truncating rem) — + // longitude has no sign discontinuity. + let (nx3, _) = normalize_window_center(¶ms, (-1, 0)); + assert_eq!( + nx3, 1533, + "negative column wraps to the top of the range, not a negative remainder" + ); + } + + /// Beyond-pole ROW clamps (latitude terminates, does not wrap) to + /// `±half_meridian_districts` — mirroring `derive_district`'s own + /// `(wy / meridian_m).clamp(-0.5, 0.5)` forward map. This is the + /// asymmetry the coordinator's fix explicitly calls out: columns wrap, + /// rows clamp — never the other way around. + #[test] + fn normalize_window_center_clamps_beyond_pole_row() { + let params = small_body_params(); + // half_meridian_districts = 383 (hand-computed above). + let (_, ny) = normalize_window_center(¶ms, (0, 3021)); + assert_eq!( + ny, 383, + "beyond-pole row clamps to the pole boundary, not wraps" + ); + + let (_, ny_neg) = normalize_window_center(¶ms, (0, -9000)); + assert_eq!(ny_neg, -383, "clamping is symmetric at both poles"); + + // A row exactly at the boundary is untouched. + let (_, ny_boundary) = normalize_window_center(¶ms, (0, 383)); + assert_eq!(ny_boundary, 383); + } + + /// An in-range center (well inside both bounds) is returned UNCHANGED — + /// normalization must be a no-op for the overwhelming common case (every + /// legitimate click), not just a defensive clamp that happens to also + /// preserve valid input. + #[test] + fn normalize_window_center_leaves_in_range_center_unchanged() { + let params = small_body_params(); + let center = (100, -50); + assert_eq!(normalize_window_center(¶ms, center), center); + + // (0, 0) — the origin — is always in range regardless of body size. + assert_eq!(normalize_window_center(¶ms, (0, 0)), (0, 0)); + } + + /// No-radius bodies (tiny test-body fallback, `body_radius_km: None`) get + /// IDENTITY — `derive_district`'s own no-radius branch has no + /// wrap/clamp-in-district-space concept (see the function doc); an + /// extreme center here is out of this fix's scope by design, not an + /// oversight. + #[test] + fn normalize_window_center_no_radius_is_identity() { + let params = BodyParams::default(); // body_radius_km: None + let extreme = (999_999, -999_999); + assert_eq!(normalize_window_center(¶ms, extreme), extreme); + } + + /// End-to-end (the coordinator's core ask): an out-of-range + /// `window_center` and its already-normalized twin, requested through the + /// REAL `handle_atlas_request` path, land in the SAME `DistrictWindowCache` + /// entry and produce a byte-identical `DistrictWindowLayer` — normalization + /// happens BEFORE the cache key is built, so an insane request and its + /// sane twin never diverge into separate cache entries (the bug this fix + /// closes: the insane request was cached STANDALONE). + #[test] + fn insane_and_sane_twin_requests_share_one_cache_entry() { + let mut cache = BodyWorldStateCache::new(CACHE_CAPACITY); + let mut window_cache = DistrictWindowCache::new(DISTRICT_WINDOW_CACHE_CAPACITY); + let (_db, resolver, params_reader, _root) = + resolver_and_params_reader_with_radius("SmallMoon", 500.0); + let queue = GenerationQueue::with_threads(1); + + // The insane request — the reported T-1142 letterbox-click values. + let insane_req = AtlasLayerRequest { + body_id: "SmallMoon".to_string(), + up_to: CascadeLayer::Topography, + window_center: Some((12276, 3021)), + window_n: 4, + }; + let resp1 = handle_atlas_request( + &insane_req, + &mut cache, + &mut window_cache, + &queue, + &resolver, + None, + Some(¶ms_reader), + 42, + 1, + test_conn_id(), + ); + assert!( + resp1.district_window.is_none(), + "first request — cache miss, DeriveWindow submitted" + ); + + // Wait for the background derive to complete and drain it into the cache + // (mirrors handle_atlas_request_clamps_oversized_window_n's pattern). + std::thread::sleep(Duration::from_millis(300)); + let completions = queue.drain_completions(); + for c in completions { + if let GenCompletion::WindowDerived { body_id, layer } = c { + if body_id == "SmallMoon" { + window_cache.insert((body_id, layer.center, layer.n), *layer); + } + } + } + // Exactly ONE entry must exist in the window cache after the insane + // request's derive completes — normalization means it was keyed on + // the canonical (4, 383), not the raw (12276, 3021). + assert_eq!( + window_cache.len(), + 1, + "the insane request's derive must be cached under its NORMALIZED key" + ); + + // The "sane twin" — the already-normalized canonical center — hits + // the SAME cache entry the insane request just populated. + let sane_twin_req = AtlasLayerRequest { + body_id: "SmallMoon".to_string(), + up_to: CascadeLayer::Topography, + window_center: Some((4, 383)), // the hand-computed canonical twin + window_n: 4, + }; + let resp2 = handle_atlas_request( + &sane_twin_req, + &mut cache, + &mut window_cache, + &queue, + &resolver, + None, + Some(¶ms_reader), + 42, + 2, + test_conn_id(), + ); + let twin_layer = resp2.district_window.expect( + "the sane twin must hit the cache the insane request populated — no new derive needed", + ); + assert_eq!( + window_cache.len(), + 1, + "the sane twin must NOT create a second cache entry" + ); + + // Re-request the ORIGINAL insane center too — it must ALSO now hit + // the same populated cache entry (both requests normalize to the + // same key). + let resp3 = handle_atlas_request( + &insane_req, + &mut cache, + &mut window_cache, + &queue, + &resolver, + None, + Some(¶ms_reader), + 42, + 3, + test_conn_id(), + ); + let insane_layer_second_try = resp3 + .district_window + .expect("the insane request, re-requested, must ALSO hit the shared cache entry"); + + assert_eq!( + twin_layer, insane_layer_second_try, + "the insane request and its sane twin must resolve to a BYTE-IDENTICAL layer" + ); + assert_eq!( + window_cache.len(), + 1, + "still exactly one entry — neither re-request created a second one" + ); + } + + /// The echoed `center` on `DistrictWindowLayer` is the NORMALIZED value, + /// not the raw wire value — the client's D-227 staleness guard (D-226 + /// T-1124 amendment §2) must see what was ACTUALLY derived, so it can + /// correctly match this response against its own (now also normalized, + /// per the T-1142 fix note to the client team) cache key. + #[test] + fn echoed_center_is_the_normalized_value_not_the_raw_wire_value() { + let mut cache = BodyWorldStateCache::new(CACHE_CAPACITY); + let mut window_cache = DistrictWindowCache::new(DISTRICT_WINDOW_CACHE_CAPACITY); + let (_db, resolver, params_reader, _root) = + resolver_and_params_reader_with_radius("SmallMoon2", 500.0); + let queue = GenerationQueue::with_threads(1); + + let insane_req = AtlasLayerRequest { + body_id: "SmallMoon2".to_string(), + up_to: CascadeLayer::Topography, + window_center: Some((12276, 3021)), // raw, out-of-range + window_n: 4, + }; + handle_atlas_request( + &insane_req, + &mut cache, + &mut window_cache, + &queue, + &resolver, + None, + Some(¶ms_reader), + 42, + 1, + test_conn_id(), + ); + + std::thread::sleep(Duration::from_millis(300)); + let completions = queue.drain_completions(); + let window_completion = completions.into_iter().find_map(|c| { + if let GenCompletion::WindowDerived { body_id, layer } = c { + if body_id == "SmallMoon2" { + return Some(layer); + } + } + None + }); + let layer = window_completion.expect("DeriveWindow must complete"); + assert_eq!( + layer.center, + (4, 383), + "the completed/echoed layer.center is the NORMALIZED value, not the raw (12276, 3021)" + ); + assert_ne!( + layer.center, + (12276, 3021), + "the raw out-of-range wire value must never be echoed back" + ); + } + /// `serve_district_window` returns `None` (no window requested) when the /// request carries no `window_center` — the common case, and the ONLY /// path every pre-T-1137 caller takes (wire back-compat: an old client's @@ -2302,7 +2669,8 @@ mod tests { } /// Build a DB with the columns needed by both `BodySourceResolver` and - /// `BodyParamsReader` for the same body, plus a tiny heightmap root. + /// `BodyParamsReader` for the same body (Earth radius, 6371 km), plus a + /// tiny heightmap root. /// /// Returns (db_path, resolver, body_params_reader, _root_kept_alive). fn resolver_and_params_reader( @@ -2312,6 +2680,23 @@ mod tests { BodySourceResolver, crate::atlas::body_params_reader::BodyParamsReader, PathBuf, // root dir — must stay alive for the test duration + ) { + resolver_and_params_reader_with_radius(body_id, 6371.0) + } + + /// Same as [`resolver_and_params_reader`] with a caller-chosen + /// `body_radius_km` (T-1142: the window-centre normalization tests need a + /// SMALL body — at Earth radius the district-circumference/half-meridian + /// bounds are tens of thousands of districts wide, too large for a + /// hand-checkable out-of-range test value). + fn resolver_and_params_reader_with_radius( + body_id: &str, + r_km: f64, + ) -> ( + PathBuf, + BodySourceResolver, + crate::atlas::body_params_reader::BodyParamsReader, + PathBuf, // root dir — must stay alive for the test duration ) { let n = SEQ.fetch_add(1, Ordering::Relaxed); let db = std::env::temp_dir().join(format!("sr_proxybp_{}_{n}.db", std::process::id())); @@ -2345,8 +2730,8 @@ mod tests { .unwrap(); conn.execute( "INSERT INTO bodies (body_id, system_id, terrain_reference, hydrosphere, atmosphere, planet_class, body_radius_km, orbital_period_days, axial_tilt_deg) - VALUES (?1, 'GJ-1', ?2, 'ocean', 'breathable', 'temperate', 6371.0, 365.25, 23.5)", - rusqlite::params![body_id, REL], + VALUES (?1, 'GJ-1', ?2, 'ocean', 'breathable', 'temperate', ?3, 365.25, 23.5)", + rusqlite::params![body_id, REL, r_km], ) .unwrap(); drop(conn);