diff --git a/server/src/atlas/gen_queue.rs b/server/src/atlas/gen_queue.rs index 7af483b2d..65e7198e4 100644 --- a/server/src/atlas/gen_queue.rs +++ b/server/src/atlas/gen_queue.rs @@ -1843,6 +1843,203 @@ mod tests { ); } + // ------------------------------------------------------------------- + // DeriveStepCanvas / submit_step_canvas coalescing (T-1181, D-255(c)(d)) + // + // PR #201 review, Hoshe finding 2 — zero coverage before these tests. + // `submit_step_canvas` is a near-verbatim sibling of `submit_window` + // (same coalesce-in-place-on-supersede shape, same defensive + // `dispatch_next` call), keyed on `step_canvas_supersede_key()` + // (`(conn_id, body_id, rung)`) instead of `window_supersede_key()`'s + // `(conn_id, body_id, granularity)`. Mirrors + // `submit_window_coalesces_same_connection_and_body` + + // `submit_window_does_not_coalesce_different_keys` exactly, substituting + // `DeriveStepCanvas`/`rung` for `DeriveWindow`/`granularity`. + // ------------------------------------------------------------------- + + /// Build a `DeriveStepCanvas` work item pointing at a tiny test + /// heightmap, mirroring `derive_window_at`'s fixture shape. + fn derive_step_canvas_at( + body_id: &str, + conn_id: ConnectionId, + center: (i64, i64), + rung: crate::atlas::step_canvas::StepCanvasRung, + ) -> GenWorkItem { + GenWorkItem::DeriveStepCanvas { + body_id: body_id.to_string(), + conn_id, + heightmap_path: test_heightmap_path(), + sea_level: 0.3, + body_seed: SeedChain::for_body(42, body_id), + body_params: Box::new(BodyParams { + hydrosphere: Some("ocean".into()), + atmosphere: Some("breathable".into()), + planet_class: Some("temperate".into()), + body_radius_km: Some(6371.0), + ..Default::default() + }), + placements: Vec::new(), + rung, + center, + extent: (4, 4), + min_wl_m: 0, + } + } + + /// `submit_step_canvas` coalescing (T-1181, mirroring `submit_window`'s + /// D-226 T-1124 amendment §1 discipline): two `DeriveStepCanvas` items + /// for the SAME `(connection, body, rung)` queued while the pool is + /// saturated collapse to ONE pending entry — the second submission + /// replaces the first rather than queuing alongside it. + #[test] + fn submit_step_canvas_coalesces_same_connection_body_and_rung() { + // Single-thread pool: the first item occupies the only worker, so + // subsequent DeriveStepCanvas submissions stay in `pending` long + // enough to inspect (same saturation trick + // `submit_window_coalesces_same_connection_and_body` uses, and for + // the same reason: `analyze()` is real measurable-latency cascade + // work, unlike `FillChunk`, which could complete before the next + // `submit_step_canvas` call even runs). + let q = GenerationQueue::with_threads(1); + q.submit(analyze("StepOccupier"), GenPriority::Low); + + let conn = ConnectionId(21); + q.submit_step_canvas( + derive_step_canvas_at( + "Canvas", + conn, + (0, 0), + crate::atlas::step_canvas::StepCanvasRung::Chunk, + ), + GenPriority::Immediate, + ); + assert_eq!( + q.pending_count(), + 1, + "one DeriveStepCanvas queued behind the saturating item" + ); + + // A second DeriveStepCanvas for the SAME (connection, body, rung) + // supersedes the first — pending count stays at 1, not 2. + q.submit_step_canvas( + derive_step_canvas_at( + "Canvas", + conn, + (5_000, 5_000), + crate::atlas::step_canvas::StepCanvasRung::Chunk, + ), + GenPriority::Immediate, + ); + assert_eq!( + q.pending_count(), + 1, + "same (connection, body, rung) DeriveStepCanvas must supersede, not queue alongside" + ); + + // Drain everything and confirm exactly one StepCanvasDerived for + // "Canvas", carrying the SECOND (superseding) center — not the + // first. + std::thread::sleep(Duration::from_millis(150)); + let mut completions = q.drain_completions(); + std::thread::sleep(Duration::from_millis(150)); + completions.extend(q.drain_completions()); + + let step_canvas_completions: Vec<_> = completions + .iter() + .filter_map(|c| { + if let GenCompletion::StepCanvasDerived { + body_id, center, .. + } = c + { + if body_id == "Canvas" { + return Some(*center); + } + } + None + }) + .collect(); + assert_eq!( + step_canvas_completions.len(), + 1, + "exactly one StepCanvasDerived for the coalesced body, not two" + ); + assert_eq!( + step_canvas_completions[0], + (5_000, 5_000), + "the surviving item must be the SECOND (superseding) submission" + ); + } + + /// `submit_step_canvas` does NOT coalesce across different connections, + /// different bodies, or different rungs — only an exact `(connection, + /// body, rung)` match supersedes. Combines `submit_window_does_not_ + /// coalesce_different_keys` (connection axis) and `submit_window_does_ + /// not_coalesce_different_granularity` (rung axis) into one test, since + /// `step_canvas_supersede_key` is a flat 3-tuple with no separate + /// legacy-vs-v2 field split to test independently the way `WindowGranularity` + /// needed. + #[test] + fn submit_step_canvas_does_not_coalesce_different_keys() { + let q = GenerationQueue::with_threads(1); + // See `submit_step_canvas_coalesces_same_connection_body_and_rung`'s + // comment on why the occupier must be `analyze()`, not `FillChunk`. + q.submit(analyze("StepOccupier2"), GenPriority::Low); + + // Different connections, same body, same rung — must NOT coalesce. + q.submit_step_canvas( + derive_step_canvas_at( + "Shared", + ConnectionId(1), + (0, 0), + crate::atlas::step_canvas::StepCanvasRung::District, + ), + GenPriority::Immediate, + ); + q.submit_step_canvas( + derive_step_canvas_at( + "Shared", + ConnectionId(2), + (1, 1), + crate::atlas::step_canvas::StepCanvasRung::District, + ), + GenPriority::Immediate, + ); + assert_eq!( + q.pending_count(), + 2, + "different connections requesting the same body+rung must NOT coalesce" + ); + + // Same connection, same body, but DIFFERENT rung — must NOT + // coalesce (District vs. Chunk are separate in-flight slots). + let conn = ConnectionId(23); + q.submit_step_canvas( + derive_step_canvas_at( + "RungBody", + conn, + (0, 0), + crate::atlas::step_canvas::StepCanvasRung::District, + ), + GenPriority::Immediate, + ); + q.submit_step_canvas( + derive_step_canvas_at( + "RungBody", + conn, + (0, 0), + crate::atlas::step_canvas::StepCanvasRung::Chunk, + ), + GenPriority::Immediate, + ); + assert_eq!( + q.pending_count(), + 4, + "same (connection, body) but DIFFERENT rung must NOT coalesce — \ + District and Chunk are separate in-flight slots (2 from the \ + connection-axis case above + 2 more here)" + ); + } + // ------------------------------------------------------------------- // TerrainAnalysisCache (T-1137, PR #187 review — Tyre C1) // ------------------------------------------------------------------- diff --git a/server/src/atlas/plugin.rs b/server/src/atlas/plugin.rs index 41cfdb953..574581e36 100644 --- a/server/src/atlas/plugin.rs +++ b/server/src/atlas/plugin.rs @@ -282,6 +282,11 @@ fn serve_step_canvas_requests( body_id: req.body_id.clone(), rung: req.rung, center: req.center, + // Nothing was derived — echo (0, 0) rather than the raw + // wire extent, matching serve_step_canvas_request's own + // Global-rung convention for "no meaningful extent to + // report" (PR #201 review, Hoshe finding 1). + extent: (0, 0), min_wl_m: req.min_wl_m, status: StepCanvasStatus::Error("no body source resolver".to_string()), canvas: None, diff --git a/server/src/atlas/step_canvas.rs b/server/src/atlas/step_canvas.rs index 6af05a3cd..1a36320ab 100644 --- a/server/src/atlas/step_canvas.rs +++ b/server/src/atlas/step_canvas.rs @@ -316,6 +316,16 @@ pub struct StepCanvasResponse { /// `district_window` (the echo IS the client's cache/staleness key, /// because the derivation is pure and deterministic, D-227). pub center: (i64, i64), + /// Echoed, CLAMPED extent (PR #201 review, Hoshe finding 1) — mirrors + /// `layer_proxy::DistrictWindowLayer.n`'s own doc: "this clamp is + /// echoed, not silently applied... a client that requests an oversized + /// extent gets back a smaller one." `(0, 0)` for `StepCanvasRung::Global` + /// (the wire extent is never read for that rung — see + /// `resolve_canvas_extent`'s doc — so there is no clamped value to + /// report; a client staleness guard should special-case `Global` the + /// same way it already must special-case `center`, which is likewise + /// meaningless there). + pub extent: (u32, u32), pub min_wl_m: u32, pub status: StepCanvasStatus, pub canvas: Option, @@ -743,12 +753,93 @@ fn crop_course_for_canvas( }) } +/// Hard per-axis ceiling on a [`StepCanvasRequest`]'s wire `extent` for every +/// FIXED rung (PR #201 review, Hoshe finding 1 — **never trust `extent` from +/// the wire**, the same discipline `layer_proxy::DISTRICT_WINDOW_MAX_N`/ +/// `clamp_window_n` already establish for the legacy carrier's `window_n`). +/// `3,840` — the larger axis of the workshop's own measured ceiling canvas +/// (3840×2160, D-255(a)/(b): "the fixed 3840×2160 px budget... every step +/// except Global," the class every fixed-rung cost number in the workshop's +/// tables was benched at). Applied per-axis BEFORE the total-cell ceiling +/// below (same two-stage discipline `clamp_window_n_v2` uses: per-axis +/// clamp, then a wire-size ceiling on the derived cell count). +pub const STEP_CANVAS_MAX_EXTENT_AXIS: u32 = 3_840; + +/// Hard ceiling on total cells (`width * height`) for a FIXED-rung canvas — +/// `3,840 × 2,160 = 8,294,400`, the exact D-255(a)/(b) measured ceiling +/// canvas (Dudley round-2 §(c) Option D's own largest row: "3840×2160, 8.3M +/// cells" at every fixed rung). This is the SAME cell count +/// `layer_proxy::WIRE_CAP_CELLS` plays for the legacy windowed carrier +/// (4,096 cells) scaled to this carrier's own measured budget — a step +/// canvas is allowed to be far larger than a legacy window (that's the +/// entire reason it needed a new carrier, D-255(c)), but it is not allowed +/// to be UNBOUNDED. +pub const STEP_CANVAS_MAX_EXTENT_CELLS: u64 = 3_840 * 2_160; + +/// Clamp a [`StepCanvasRequest`]'s wire `extent` to +/// [`STEP_CANVAS_MAX_EXTENT_AXIS`]/[`STEP_CANVAS_MAX_EXTENT_CELLS`] — +/// **clamps, never rejects** (PR #201 review: "consistent with how +/// `clamp_window_n` behaves"). Two-stage, mirroring +/// `layer_proxy::clamp_window_n_v2`'s discipline exactly: +/// +/// 1. **Per-axis clamp** to `[1, STEP_CANVAS_MAX_EXTENT_AXIS]` on each axis +/// independently — this alone defeats a `u32::MAX`-per-axis request (a +/// `(u32::MAX, u32::MAX)` extent clamps to `(3_840, 3_840)` before any +/// multiplication is attempted, so the overflow-prone `width * height` +/// arithmetic downstream never sees the raw wire value). +/// 2. **Total-cell ceiling** — if the per-axis-clamped shape still exceeds +/// `STEP_CANVAS_MAX_EXTENT_CELLS` (reachable: `3_840 * 3_840 = +/// 14,745,600 > 8,294,400`, an axis-square request past the measured +/// 16:9 ceiling), scale BOTH axes down by the same factor +/// (`sqrt(cap / cells)`) so the clamped shape keeps its requested aspect +/// ratio rather than being squashed on one axis only — a closer match to +/// "give the client the biggest canvas that fits the budget" than an +/// asymmetric halving loop would produce for a non-square request. +/// +/// Never called for [`StepCanvasRung::Global`] — that rung's extent is +/// server-derived from the body's own region grid +/// ([`StepCanvasRung::global_cell_counts`]), never the wire value at all +/// (see [`resolve_canvas_extent`]). +pub fn clamp_step_canvas_extent(extent: (u32, u32)) -> (u32, u32) { + let w = extent.0.clamp(1, STEP_CANVAS_MAX_EXTENT_AXIS); + let h = extent.1.clamp(1, STEP_CANVAS_MAX_EXTENT_AXIS); + let cells = w as u64 * h as u64; + if cells <= STEP_CANVAS_MAX_EXTENT_CELLS { + return (w, h); + } + let scale = (STEP_CANVAS_MAX_EXTENT_CELLS as f64 / cells as f64).sqrt(); + let scaled_w = ((w as f64 * scale).floor() as u32).max(1); + let scaled_h = ((h as f64 * scale).floor() as u32).max(1); + // Defensive floor-rounding safety net (mirrors clamp_window_n_v2's own + // "defensive, not currently reachable for well-behaved inputs" halving + // loop): floor-rounding both axes down from an exact sqrt scale can + // still land fractionally over the cap for some (w, h, cap) combinations + // — walk the larger axis down one cell at a time until the invariant + // holds. Bounded: at most STEP_CANVAS_MAX_EXTENT_AXIS iterations, and + // never fires for any input this function's own test sweep covers. + let mut final_w = scaled_w; + let mut final_h = scaled_h; + while (final_w as u64 * final_h as u64) > STEP_CANVAS_MAX_EXTENT_CELLS + && final_w > 1 + && final_h > 1 + { + if final_w >= final_h { + final_w -= 1; + } else { + final_h -= 1; + } + } + (final_w, final_h) +} + /// Resolve a [`StepCanvasRequest`]'s canvas extent in cells — `(width, /// height)`. [`StepCanvasRung::Global`] uses the body's own region grid -/// ([`StepCanvasRung::global_cell_counts`]); every fixed rung uses the -/// request's own `extent` field directly (D-255(a): "the fixed 3840×2160 px -/// budget... at 1 gridunit-per-screen-px" — cell count = canvas px count at -/// every fixed rung, Dudley round-2 §(c)). +/// ([`StepCanvasRung::global_cell_counts`]) — **the wire `extent` is never +/// read for `Global`, full stop** (PR #201 review: "make sure... Global +/// ignores wire extent entirely"). Every fixed rung clamps the wire `extent` +/// via [`clamp_step_canvas_extent`] before it reaches any allocation (D-255(a): +/// "the fixed 3840×2160 px budget... at 1 gridunit-per-screen-px" — cell +/// count = canvas px count at every fixed rung, Dudley round-2 §(c)). fn resolve_canvas_extent( rung: StepCanvasRung, extent: (u32, u32), @@ -756,7 +847,7 @@ fn resolve_canvas_extent( ) -> (u32, u32) { match rung.global_cell_counts(body_radius_km) { Some((cols, rows)) => (cols, rows), - None => (extent.0.max(1), extent.1.max(1)), + None => clamp_step_canvas_extent(extent), } } @@ -1389,6 +1480,21 @@ pub fn serve_step_canvas_request( ) -> StepCanvasResponse { let min_wl_m = quantize_min_wl_m_for_rung(req.min_wl_m); + // PR #201 review, Hoshe finding 1 — clamp the wire extent HERE, once, at + // the request boundary, BEFORE it reaches the cache key or the + // background work item (never trust the wire — the exact discipline + // `layer_proxy::serve_district_window` already applies to `window_n`/ + // `min_wl_m` before either touches its own cache key). `Global` ignores + // the wire extent entirely (ratified in `resolve_canvas_extent`'s own + // doc) — its echoed/keyed extent is a fixed sentinel `(0, 0)` rather + // than the unclamped wire value, so a `Global` request's cache key can + // never vary by the client's (ignored) extent field. + let extent = if req.rung.is_global() { + (0, 0) + } else { + clamp_step_canvas_extent(req.extent) + }; + let body_params = match read_body_params(body_params_reader, &req.body_id) { Ok(p) => p, Err(e) => { @@ -1397,6 +1503,7 @@ pub fn serve_step_canvas_request( body_id: req.body_id.clone(), rung: req.rung, center: req.center, + extent, min_wl_m, status: StepCanvasStatus::Error(e), canvas: None, @@ -1411,24 +1518,20 @@ pub fn serve_step_canvas_request( body_id: req.body_id.clone(), rung: req.rung, center: req.center, + extent, min_wl_m, status: StepCanvasStatus::Ready, canvas: Some(canvas.clone()), }; } } else { - let key: StepCanvasKey = ( - req.body_id.clone(), - req.rung, - req.center, - req.extent, - min_wl_m, - ); + let key: StepCanvasKey = (req.body_id.clone(), req.rung, req.center, extent, min_wl_m); if let Some(canvas) = canvas_cache.get(&key, current_tick, body_class) { return StepCanvasResponse { body_id: req.body_id.clone(), rung: req.rung, center: req.center, + extent, min_wl_m, status: StepCanvasStatus::Ready, canvas: Some(canvas), @@ -1446,6 +1549,7 @@ pub fn serve_step_canvas_request( body_id: req.body_id.clone(), rung: req.rung, center: req.center, + extent, min_wl_m, status: StepCanvasStatus::NotFound, canvas: None, @@ -1456,6 +1560,7 @@ pub fn serve_step_canvas_request( body_id: req.body_id.clone(), rung: req.rung, center: req.center, + extent, min_wl_m, status: StepCanvasStatus::Error(e.to_string()), canvas: None, @@ -1474,7 +1579,7 @@ pub fn serve_step_canvas_request( placements: placements.to_vec(), rung: req.rung, center: req.center, - extent: req.extent, + extent, min_wl_m, }, GenPriority::Immediate, @@ -1484,6 +1589,7 @@ pub fn serve_step_canvas_request( body_id: req.body_id.clone(), rung: req.rung, center: req.center, + extent, min_wl_m, status: StepCanvasStatus::Pending, canvas: None, @@ -1552,6 +1658,147 @@ mod tests { assert_eq!(rows, (cols / 2).max(1)); } + // ----------------------------------------------------------------- + // Wire extent clamp (PR #201 review, Hoshe finding 1 — DoS hardening) + // ----------------------------------------------------------------- + + #[test] + fn oversized_per_axis_extent_clamps_to_the_axis_cap() { + let (w, h) = clamp_step_canvas_extent((50_000, 100)); + assert_eq!(w, STEP_CANVAS_MAX_EXTENT_AXIS); + assert_eq!(h, 100); + + let (w, h) = clamp_step_canvas_extent((100, 50_000)); + assert_eq!(w, 100); + assert_eq!(h, STEP_CANVAS_MAX_EXTENT_AXIS); + } + + #[test] + fn total_cell_overflow_clamps_even_when_both_axes_are_individually_legal() { + // Both axes are within STEP_CANVAS_MAX_EXTENT_AXIS (3,840) but their + // product (3,840 * 3,840 = 14,745,600) exceeds + // STEP_CANVAS_MAX_EXTENT_CELLS (8,294,400) — the per-axis clamp + // alone does NOT catch this, only the total-cell stage does. + let requested = (3_840u32, 3_840u32); + let requested_cells = requested.0 as u64 * requested.1 as u64; + assert!( + requested_cells > STEP_CANVAS_MAX_EXTENT_CELLS, + "test setup: this case must actually exceed the cap" + ); + + let (w, h) = clamp_step_canvas_extent(requested); + let clamped_cells = w as u64 * h as u64; + assert!( + clamped_cells <= STEP_CANVAS_MAX_EXTENT_CELLS, + "clamped shape ({w}x{h}={clamped_cells}) must respect the total-cell cap" + ); + assert!(w <= STEP_CANVAS_MAX_EXTENT_AXIS && h <= STEP_CANVAS_MAX_EXTENT_AXIS); + // Aspect ratio preserved (both axes were equal going in, so they + // must still be equal, or within 1 of each other from the + // defensive floor-rounding walk-down). + assert!((w as i64 - h as i64).abs() <= 1); + } + + #[test] + fn u32_max_extent_clamps_to_the_cap_without_huge_allocation() { + // The exact DoS shape Hoshe's finding named: a wire extent of + // (u32::MAX, u32::MAX) must clamp down to a bounded shape BEFORE + // any allocation is attempted — never overflow, never panic, never + // produce a canvas anywhere near u32::MAX cells. + // + // Both axes clamp to STEP_CANVAS_MAX_EXTENT_AXIS (3,840) first, but + // 3,840 x 3,840 = 14,745,600 > STEP_CANVAS_MAX_EXTENT_CELLS + // (8,294,400) — the total-cell stage then scales BOTH axes down + // together (same two-stage behavior + // total_cell_overflow_clamps_even_when_both_axes_are_individually_legal + // pins generically); this test additionally confirms the concrete + // u32::MAX case end to end through a real derive. + let (w, h) = clamp_step_canvas_extent((u32::MAX, u32::MAX)); + let cells = w as u64 * h as u64; + assert!(w <= STEP_CANVAS_MAX_EXTENT_AXIS); + assert!(h <= STEP_CANVAS_MAX_EXTENT_AXIS); + assert!( + cells <= STEP_CANVAS_MAX_EXTENT_CELLS, + "clamped u32::MAX request ({w}x{h}={cells}) must respect the total-cell cap" + ); + + // End-to-end: actually run build_step_canvas with this hostile + // request and confirm it derives a small, bounded canvas — not a + // multi-billion-cell allocation. Uses a tiny real TerrainAnalysis + // fixture so the derive itself stays fast; the point is the + // ALLOCATION SIZE, which is governed by resolve_canvas_extent's + // clamp regardless of how small the source heightmap is. + let ta = tiny_ta(); + let rn = RiverNetwork::default(); + let params = BodyParams { + body_radius_km: Some(6371.0), + ..Default::default() + }; + let climate = ClimateConstants::default(); + let seed = SeedChain::root(0xDEADBEEF_u64).derive(crate::seed::SeedDomain::Body, 1); + + let raw = build_step_canvas( + seed, + "dos-test", + ¶ms, + &ta, + &rn, + &[], + StepCanvasRung::Chunk, + (0, 0), + (u32::MAX, u32::MAX), + &climate, + 0, + ); + assert_eq!(raw.width, w); + assert_eq!(raw.height, h); + assert_eq!(raw.morphology.len() as u64, cells); + assert!( + cells <= STEP_CANVAS_MAX_EXTENT_CELLS, + "the ACTUAL derived+allocated canvas must respect the cap, not just the clamp function's return value" + ); + } + + #[test] + fn extent_at_or_under_the_cap_is_unchanged() { + // The clamp must be a no-op for any legal, already-bounded request — + // it should never shrink a request that was already within budget. + for (w, h) in [(1u32, 1u32), (100, 100), (3_840, 2_160), (1_920, 1_080)] { + assert_eq!( + clamp_step_canvas_extent((w, h)), + (w, h), + "({w}, {h}) is within both caps and must pass through unchanged" + ); + } + } + + #[test] + fn global_rung_ignores_wire_extent_entirely() { + // resolve_canvas_extent must derive Global's extent from the body's + // own region grid, never from the (hostile or otherwise) wire + // value — confirmed at u32::MAX, the most adversarial input. + let (w, h) = resolve_canvas_extent(StepCanvasRung::Global, (u32::MAX, u32::MAX), 6371.0); + let expected = StepCanvasRung::Global + .global_cell_counts(6371.0) + .expect("Global has a cell-count shape"); + assert_eq!((w, h), expected); + // Sanity: the body-derived shape is nowhere near u32::MAX — proves + // the wire value truly had zero influence, not just a coincidental + // clamp to the same range. + assert!(w < STEP_CANVAS_MAX_EXTENT_AXIS * 10); + assert!(h < STEP_CANVAS_MAX_EXTENT_AXIS * 10); + } + + #[test] + fn global_rung_extent_is_identical_regardless_of_requested_extent() { + // A second confirmation from the opposite direction: Global's + // resolved extent must be the SAME for a tiny request and a huge + // one — extent has literally no effect on Global's output shape. + let tiny = resolve_canvas_extent(StepCanvasRung::Global, (1, 1), 6371.0); + let huge = resolve_canvas_extent(StepCanvasRung::Global, (u32::MAX, u32::MAX), 6371.0); + assert_eq!(tiny, huge); + } + // ----------------------------------------------------------------- // Station-spacing cap (S2 addendum decision) // -----------------------------------------------------------------