From df33e9f2fb2bfd895854c9f58c28d6f4700848ba Mon Sep 17 00:00:00 2001 From: Jeroen Schweitzer Date: Tue, 21 Jul 2026 13:56:58 +0200 Subject: [PATCH] fix(simulation): per-body TerrainAnalysis LRU + full-path determinism test (PR #187 C1/C3) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit C1 (Tyre): the DeriveWindow branch re-ran the ~45ms run_layer1 for EVERY uncached window — the dominant pan-around-one-body path paid ~52ms per new window while the doc claimed one-time-per-body. TerrainAnalysisCache: private per-body LRU (capacity 8, ~2MB/entry, true access-recency eviction — which body the player keeps panning IS a recency signal) held as Arc> on GenerationQueue itself, the in_flight field pattern, because run_work_item executes on a Rayon worker where the main-thread caches are unreachable. DeriveWindow now calls get_or_derive; the variant doc states the real model (first window per body pays ~45ms once; subsequent windows any center/n hit the LRU; eviction re-pays). End-to-end test proves two windows on one body via two connections share exactly one cache entry. C3 (Tyre): the determinism test reused one TerrainAnalysis — proving the packer, not the derivation. New test runs run_layer1 twice independently, asserts field-by-field analysis agreement, then byte-identical packed windows end to end (D-227 save-critical proof). Packer-only test kept alongside. atlas:: 564/564; full lib 1778/1778; timing-sensitive suites 3x stable. Co-Authored-By: Claude Fable 5 --- server/src/atlas/gen_queue.rs | 335 +++++++++++++++++++++++++++++--- server/src/atlas/layer_proxy.rs | 55 ++++++ 2 files changed, 366 insertions(+), 24 deletions(-) diff --git a/server/src/atlas/gen_queue.rs b/server/src/atlas/gen_queue.rs index 6b84008c8..79b88fefe 100644 --- a/server/src/atlas/gen_queue.rs +++ b/server/src/atlas/gen_queue.rs @@ -32,6 +32,7 @@ use crate::atlas::attractor_matching::CityRecord; use crate::atlas::body_world_state::BodyWorldState; use crate::atlas::cascade::{run_cascade_from_heightmap, CascadeLayer}; use crate::atlas::district_profile::{BodyParams, ClimateConstants, DistrictPos}; +use crate::atlas::features::TerrainAnalysis; use crate::atlas::heightmap::{load_heightmap_png, GRID_H, GRID_W}; use crate::atlas::layer_proxy::{build_district_window_layer, DistrictWindowLayer}; use crate::atlas::shell::{fill_chunk, FilledChunk}; @@ -159,23 +160,32 @@ pub enum GenWorkItem { /// ~7–29 ms, which would blow the "cheap channel drain" contract /// `drain_generation_completions` documents). /// - /// **TerrainAnalysis availability (T-1137 binding decision, with numbers):** - /// `BodyWorldState` does NOT retain `TerrainAnalysis` after cascade - /// completion (T-1044 scoped its transient-carry fix to *within-cascade* - /// reuse only — `cascade.rs` drops it once `DistrictProfile`+`RoadGraph` - /// finish; see the doc on `CascadeSnapshot::terrain_analysis`). Caching it - /// alongside every `BodyWorldStateCache` entry would cost ~2 MB × 50-body - /// capacity ≈ 100 MB of PERMANENT resident cost, paid by every cached body - /// whether or not a window is ever requested for it — the exact D-203 - /// budget concern T-1044's own ticket text guarded against. Re-deriving via - /// `run_layer1` per window-serving cache miss costs ~45 ms ONE-TIME, paid - /// only when a window is actually requested, and — because this work item - /// already runs off the tick thread on the Rayon queue — that cost is - /// invisible to the main thread; it is the same order of magnitude as one - /// window derive itself, not a multiplier on it. So: re-derive - /// (`heightmap_path`/`sea_level` below), matching `aliveness_probe`'s - /// existing `--render` workaround, NOT a `TerrainAnalysis` field cached on - /// `BodyWorldState`. + /// **TerrainAnalysis availability (T-1137 binding decision, with numbers; + /// corrected 2026-07-21 per PR #187 review — Tyre C1):** `BodyWorldState` + /// does NOT retain `TerrainAnalysis` after cascade completion (T-1044 + /// scoped its transient-carry fix to *within-cascade* reuse only — + /// `cascade.rs` drops it once `DistrictProfile`+`RoadGraph` finish; see the + /// doc on `CascadeSnapshot::terrain_analysis`). Caching it alongside every + /// `BodyWorldStateCache` entry would cost ~2 MB × 50-body capacity ≈ + /// 100 MB of PERMANENT resident cost, paid by every cached body whether or + /// not a window is ever requested for it — the exact D-203 budget concern + /// T-1044's own ticket text guarded against. So `run_work_item` re-derives + /// via `run_layer1` (matching `aliveness_probe`'s existing `--render` + /// workaround) rather than persisting a field on `BodyWorldState` — but + /// NOT unconditionally on every work item: the actual model is a small + /// **per-body LRU** (`TerrainAnalysisCache`, capacity 8, ~16 MB worst + /// case), consulted before every re-derive. The FIRST `DeriveWindow` on a + /// body pays the full ~45 ms `run_layer1` cost and populates that body's + /// cache entry; EVERY SUBSEQUENT window on the SAME body (any `center`/`n`, + /// not just an exact repeat — that narrower case is what + /// `DistrictWindowCache` in `layer_proxy.rs` already catches) hits the LRU + /// and skips straight to the ~7–29 ms per-window pack in + /// `build_district_window_layer`. An LRU eviction re-pays the ~45 ms on the + /// next window for that body. Because every `DeriveWindow` work item still + /// runs off the tick thread on the Rayon queue regardless of hit or miss, + /// both costs are invisible to the main thread either way — the LRU's + /// value is throughput/worker-occupancy (bounding how many ~45 ms re-derives + /// a pan-burst across one body can force), not tick-thread latency. DeriveWindow { body_id: String, /// Coalescing/routing key (D-226 T-1124 amendment §1 "recommended" @@ -284,6 +294,14 @@ struct QueuedWork { /// Submit work with `submit()`. Drain completions with `drain_completions()` /// once per tick. The Rayon thread pool runs tasks in priority order. /// +/// Also owns the queue-scoped `TerrainAnalysisCache` (T-1137, PR #187 review +/// C1) — a small per-body LRU consulted by every `DeriveWindow` work item +/// before paying the `run_layer1` re-derive cost. Lives here (not as a +/// separate `Resource`) because it must be reachable from `run_work_item` +/// while it executes on a Rayon worker thread, the same reason +/// `in_flight`/`in_flight_count` are `Arc>` fields on this struct +/// rather than plain fields. +/// /// Priority is respected because `dispatch_next()` is gated on pool saturation /// via `in_flight_count`: it only dispatches when fewer than `n_threads` tasks /// are running. This applies to all work item types — `in_flight` (body-id set) @@ -305,6 +323,9 @@ pub struct GenerationQueue { /// Thread count — caps concurrent dispatches so pending items accumulate /// and priority ordering is consulted before the pool has free threads. n_threads: usize, + /// Per-body `TerrainAnalysis` LRU shared by every `DeriveWindow` work item + /// on this queue (T-1137, PR #187 review C1) — see [`TerrainAnalysisCache`]. + terrain_cache: Arc>, } impl std::fmt::Debug for GenerationQueue { @@ -344,6 +365,9 @@ impl GenerationQueue { in_flight: Arc::new(Mutex::new(std::collections::BTreeSet::new())), in_flight_count: Arc::new(Mutex::new(0)), n_threads, + terrain_cache: Arc::new(Mutex::new(TerrainAnalysisCache::new( + TERRAIN_ANALYSIS_CACHE_CAPACITY, + ))), } } @@ -468,9 +492,10 @@ impl GenerationQueue { let tx = self.completion_tx.clone(); let in_flight = Arc::clone(&self.in_flight); let in_flight_count = Arc::clone(&self.in_flight_count); + let terrain_cache = Arc::clone(&self.terrain_cache); self.pool.spawn(move || { - let completion = run_work_item(&item); + let completion = run_work_item(&item, &terrain_cache); // Un-mark body dedup set (AnalyzeBody only). if let Some(body_id) = item.body_id() { @@ -490,6 +515,108 @@ impl Default for GenerationQueue { } } +// --------------------------------------------------------------------------- +// TerrainAnalysisCache (T-1137, PR #187 review — Tyre C1) +// --------------------------------------------------------------------------- + +/// Small per-body LRU of re-derived [`TerrainAnalysis`] (~1.5–2 MB/entry), +/// consulted by the `DeriveWindow` execution path before paying the ~45 ms +/// `run_layer1` re-derive cost (T-1137 binding decision — see the +/// `DeriveWindow` variant doc on why `TerrainAnalysis` is re-derived rather +/// than cached on `BodyWorldState` at all). +/// +/// **Why this exists (PR #187 review finding, binding):** the original +/// T-1137 landing called `run_layer1` unconditionally on every `DeriveWindow` +/// work item — nothing memoized it within a body, so the *dominant* usage +/// pattern (panning around ONE body, many windows) paid the ~45 ms re-derive +/// on every single window instead of just the first. This cache closes that +/// gap: first window on a body pays the full re-derive and populates the +/// entry; every subsequent window on the SAME body (until eviction) hits the +/// cache and skips straight to the ~7–29 ms `build_district_window_layer` +/// pack (§4's actual per-window number). +/// +/// **Shape:** `Arc>` — lives on [`GenerationQueue`] alongside +/// `in_flight`/`in_flight_count` (the same "shared state cloned into every +/// Rayon closure" pattern), because `run_work_item` executes ON a Rayon +/// worker thread, potentially concurrently with other workers when +/// `n_threads > 1`; a queue-owned, plain (non-`Resource`) cache is the +/// correct home — `handle_atlas_request`'s `DistrictWindowCache` is main- +/// thread-only (D-225 poll loop) and cannot be reused here. +/// +/// **Capacity (8, ~16 MB worst case):** deliberately small relative to +/// `BodyWorldStateCache::CACHE_CAPACITY` (50) — this caches a re-derive +/// shortcut for bodies actually being window-browsed RIGHT NOW, not a +/// body-indexed store meant to grow with session length. True LRU +/// (access-recency, mirroring `BodyWorldStateCache`'s own eviction policy) +/// rather than `DistrictWindowCache`'s FIFO-by-insertion: unlike a +/// D-227-pure derived window (valid forever, no recency signal to track), +/// which body a player keeps panning around IS a recency signal, so +/// access-order eviction is the right fit here. +#[derive(Debug)] +struct TerrainAnalysisCache { + entries: std::collections::BTreeMap, + /// Monotonic access counter (substitutes for `BodyWorldStateCache`'s + /// `SimTick` — there is no tick concept on a background Rayon thread). + clock: u64, + capacity: usize, +} + +/// Default capacity for [`TerrainAnalysisCache`] (PR #187 review — Tyre C1 +/// binding numbers: "capacity ~8, ~2MB/entry = ~16MB worst case"). +const TERRAIN_ANALYSIS_CACHE_CAPACITY: usize = 8; + +impl TerrainAnalysisCache { + fn new(capacity: usize) -> Self { + Self { + entries: std::collections::BTreeMap::new(), + clock: 0, + capacity, + } + } + + /// Look up a cached `TerrainAnalysis` for `body_id`, re-deriving via + /// `run_layer1` on a miss and inserting the result (evicting the LRU + /// entry first if at capacity). Bumps the access clock on both a hit and + /// a fresh insert (both are "this body was just used"). + fn get_or_derive( + &mut self, + body_id: &str, + heightmap: &crate::atlas::heightmap::BodyHeightmap, + ) -> TerrainAnalysis { + self.clock += 1; + let now = self.clock; + if let Some((ta, last_used)) = self.entries.get_mut(body_id) { + *last_used = now; + return ta.clone(); + } + + let (_, ta) = crate::atlas::layer1::run_layer1(heightmap); + + if self.entries.len() >= self.capacity && !self.entries.contains_key(body_id) { + if let Some(victim) = self + .entries + .iter() + .min_by_key(|(_, (_, last_used))| *last_used) + .map(|(id, _)| id.clone()) + { + self.entries.remove(&victim); + } + } + self.entries.insert(body_id.to_string(), (ta.clone(), now)); + ta + } + + #[cfg(test)] + fn len(&self) -> usize { + self.entries.len() + } + + #[cfg(test)] + fn contains(&self, body_id: &str) -> bool { + self.entries.contains_key(body_id) + } +} + // --------------------------------------------------------------------------- // Work execution stub // --------------------------------------------------------------------------- @@ -500,7 +627,15 @@ impl Default for GenerationQueue { /// runs the real plan phase (#957, D-229) producing the skeleton + block tags; /// `FillChunk` runs the real derive phase (T-987, D-230) producing the building /// shell from the pre-resolved tags. -fn run_work_item(item: &GenWorkItem) -> GenCompletion { +/// +/// `terrain_cache` serves `DeriveWindow`'s `TerrainAnalysis` re-derive +/// shortcut (T-1137, PR #187 review C1) — unused by every other variant +/// (they don't touch `TerrainAnalysis` at all, or — `AnalyzeBody` — derive it +/// once already as part of the normal in-cascade path, T-1044). +fn run_work_item( + item: &GenWorkItem, + terrain_cache: &Arc>, +) -> GenCompletion { match item { GenWorkItem::AnalyzeBody { body_id, @@ -624,11 +759,18 @@ fn run_work_item(item: &GenWorkItem) -> GenCompletion { } else { hm }; - // Re-derive TerrainAnalysis via run_layer1 (T-1137 binding - // decision — see the DeriveWindow variant doc for the numbers). - // This is the SAME workaround aliveness_probe --render already - // uses when CascadeSnapshot.terrain_analysis is None. - let (_, ta) = crate::atlas::layer1::run_layer1(&working); + // TerrainAnalysis via the per-body LRU (T-1137 binding decision + + // PR #187 review C1): first window on a body pays the ~45 ms + // run_layer1 re-derive and populates the cache entry; every + // subsequent window on the SAME body (until eviction) hits the + // cache and skips straight to the ~7–29 ms per-window pack below. + // This is the memoized form of the SAME workaround + // aliveness_probe --render uses when CascadeSnapshot.terrain_analysis + // is None (it has no cache — a one-shot CLI run doesn't need one). + let ta = terrain_cache + .lock() + .unwrap() + .get_or_derive(body_id, &working); let climate = ClimateConstants::default(); let layer = build_district_window_layer( *body_seed, @@ -1113,4 +1255,149 @@ mod tests { "different connections requesting the same body must NOT coalesce" ); } + + // ------------------------------------------------------------------- + // TerrainAnalysisCache (T-1137, PR #187 review — Tyre C1) + // ------------------------------------------------------------------- + + fn window_test_hm() -> crate::atlas::heightmap::BodyHeightmap { + use crate::atlas::heightmap::BodyHeightmap; + let (w, h) = (32u32, 16u32); + let n = (w * h) as usize; + let data = (0..n) + .map(|i| { + let r = (i / w as usize) as f32 / h as f32; + let c = (i % w as usize) as f32 / w as f32; + (r * 0.6 + c * 0.4).min(1.0) + }) + .collect(); + BodyHeightmap { + body_id: "test".into(), + width: w, + height: h, + data, + sea_level: 0.3, + } + } + + /// A miss re-derives and populates the entry; a subsequent hit for the + /// SAME body returns an equal `TerrainAnalysis` (D-227: the same + /// heightmap always derives to the same analysis) WITHOUT growing the + /// cache — `len()` stays at 1, proving the second call short-circuited + /// past `run_layer1` rather than deriving-then-overwriting. + #[test] + fn terrain_analysis_cache_hit_reuses_entry() { + let mut cache = TerrainAnalysisCache::new(8); + let hm = window_test_hm(); + + assert!(!cache.contains("BodyA")); + let first = cache.get_or_derive("BodyA", &hm); + assert_eq!(cache.len(), 1); + assert!(cache.contains("BodyA")); + + let second = cache.get_or_derive("BodyA", &hm); + assert_eq!( + cache.len(), + 1, + "a hit must not insert a second entry for the same body" + ); + assert_eq!( + first.ocean_mask, second.ocean_mask, + "same heightmap → identical re-derived analysis (D-227)" + ); + assert_eq!(first.slope_deg, second.slope_deg); + assert_eq!(first.elev_pct, second.elev_pct); + } + + /// Different bodies get independent entries, and a capacity-2 cache + /// evicts the LEAST-RECENTLY-USED entry — not insertion order — when a + /// third body is derived. Mirrors `BodyWorldStateCache`'s own + /// `update_last_accessed_on_get` precedent: touching "BodyA" again before + /// the third insert must save it from eviction. + #[test] + fn terrain_analysis_cache_evicts_lru_not_fifo() { + let mut cache = TerrainAnalysisCache::new(2); + let hm = window_test_hm(); + + cache.get_or_derive("BodyA", &hm); + cache.get_or_derive("BodyB", &hm); + assert_eq!(cache.len(), 2); + + // Touch BodyA again — it is now the MOST recently used, so BodyB + // (untouched since its own insert) is the true LRU victim. + cache.get_or_derive("BodyA", &hm); + + // Insert a third body — capacity 2 forces an eviction. + cache.get_or_derive("BodyC", &hm); + assert_eq!(cache.len(), 2); + assert!( + cache.contains("BodyA"), + "recently re-touched BodyA must survive eviction" + ); + assert!( + !cache.contains("BodyB"), + "BodyB (true LRU — untouched since its own insert) must be evicted" + ); + assert!(cache.contains("BodyC")); + } + + /// End-to-end: two `DeriveWindow` work items for the SAME body, submitted + /// through the real `GenerationQueue` (not the bare `TerrainAnalysisCache` + /// unit above), share ONE `TerrainAnalysisCache` entry — the fix for the + /// PR #187 review C1 finding (the original landing called `run_layer1` + /// unconditionally on every `DeriveWindow`, so panning around one body + /// paid the ~45 ms re-derive on every window instead of just the first). + /// Both windows must still complete correctly (content assertions, not + /// timing — a wall-clock assertion would be flaky); the cache-population + /// count is the load-bearing proof of reuse. + #[test] + fn two_windows_on_same_body_share_one_terrain_analysis_entry() { + let q = GenerationQueue::with_threads(2); + let conn_a = ConnectionId(1); + let conn_b = ConnectionId(2); + // Two DIFFERENT connections so submit_window's coalescing (which + // supersedes same-connection/same-body pending items) doesn't collapse + // these into one work item — the point here is two DISTINCT completed + // derives sharing the cache, not coalescing (already covered above). + q.submit_window( + derive_window("SharedBody", conn_a, (0, 0)), + GenPriority::Immediate, + ); + q.submit_window( + derive_window("SharedBody", conn_b, (10, 10)), + GenPriority::Immediate, + ); + + std::thread::sleep(Duration::from_millis(200)); + let completions = q.drain_completions(); + let windows: Vec<_> = completions + .iter() + .filter_map(|c| { + if let GenCompletion::WindowDerived { body_id, layer } = c { + if body_id == "SharedBody" { + return Some(layer); + } + } + None + }) + .collect(); + assert_eq!(windows.len(), 2, "both windows must complete"); + let centers: std::collections::BTreeSet<_> = windows.iter().map(|l| l.center).collect(); + assert_eq!( + centers, + std::collections::BTreeSet::from([(0, 0), (10, 10)]), + "both distinct windows survived, not coalesced" + ); + + // The queue's own TerrainAnalysisCache must hold exactly ONE entry + // for "SharedBody" — both derives shared it rather than each + // re-deriving independently. + let cache = q.terrain_cache.lock().unwrap(); + assert_eq!( + cache.len(), + 1, + "two DeriveWindow items for the same body must share one TerrainAnalysis entry" + ); + assert!(cache.contains("SharedBody")); + } } diff --git a/server/src/atlas/layer_proxy.rs b/server/src/atlas/layer_proxy.rs index 8d1ae01d1..c4bee2ea7 100644 --- a/server/src/atlas/layer_proxy.rs +++ b/server/src/atlas/layer_proxy.rs @@ -1283,6 +1283,61 @@ mod tests { ); } + /// FULL-PATH determinism (PR #187 review — Tyre C3, binding, load-bearing + /// for save-file lineage under D-227): the test above reuses ONE `ta` for + /// both passes, which only proves `build_district_window_layer` (the + /// packer) is a pure function of its arguments — it says nothing about + /// whether re-running `run_layer1` itself (the D8 drainage pass + + /// `TerrainAnalysis::analyze`) is deterministic, which is exactly what the + /// production `DeriveWindow` path depends on (T-1137's `TerrainAnalysis` + /// re-derive / `TerrainAnalysisCache::get_or_derive` on a cache miss, and + /// `aliveness_probe --render`'s workaround before it). + /// + /// This test runs `run_layer1` TWICE, independently, from the SAME + /// `(seed, heightmap)` inputs — no shared `ta` — and asserts the two + /// COMPLETE `DistrictWindowLayer` outputs (derive AND pack) are + /// byte-identical. D-227's save-file guarantee ("same seed/body/position → + /// same derived output, always") is only as strong as the weakest link in + /// that chain; this closes the gap the packer-only test left open. + #[test] + fn full_path_two_independent_run_layer1_passes_produce_identical_window() { + let hm = window_test_hm(); + let params = window_test_params(); + let climate = crate::atlas::district_profile::ClimateConstants::default(); + let seed = SeedChain::root(11).derive(SeedDomain::Body, 5); + let n = 6u32; + let center = (4, -1); + + // Two INDEPENDENT calls to run_layer1 — each re-runs D8 drainage + + // TerrainAnalysis::analyze from scratch on the SAME heightmap, exactly + // mirroring what a cold TerrainAnalysisCache miss does on the real + // DeriveWindow path (or a second body eviction re-pay). + let (_, ta_pass1) = crate::atlas::layer1::run_layer1(&hm); + let (_, ta_pass2) = crate::atlas::layer1::run_layer1(&hm); + + // Confirm the two independent TerrainAnalysis derivations themselves + // agree field-by-field — a precise failure signal if drainage/analyze + // ever introduces nondeterminism (unordered iteration, uninitialized + // memory, etc.) BEFORE the packer even runs. + assert_eq!(ta_pass1.ocean_mask, ta_pass2.ocean_mask); + assert_eq!(ta_pass1.lake_mask, ta_pass2.lake_mask); + assert_eq!(ta_pass1.water_dist, ta_pass2.water_dist); + assert_eq!(ta_pass1.slope_deg, ta_pass2.slope_deg); + assert_eq!(ta_pass1.elev_pct, ta_pass2.elev_pct); + + // Now the FULL path: pack a DistrictWindowLayer from each independent + // TerrainAnalysis and confirm the complete served payload agrees. + let window_from_pass1 = + build_district_window_layer(seed, "test_body", ¶ms, &ta_pass1, center, n, &climate); + let window_from_pass2 = + build_district_window_layer(seed, "test_body", ¶ms, &ta_pass2, center, n, &climate); + assert_eq!( + window_from_pass1, window_from_pass2, + "two independent run_layer1 derivations from the same (seed, heightmap) \ + must pack to a byte-identical DistrictWindowLayer end to end (D-227)" + ); + } + /// [`DistrictWindowCache`] insert/get round-trips, and a capacity-1 cache /// evicts the oldest entry FIFO — mirroring `BodyWorldStateCache`'s own /// `evicts_lru_on_overflow` precedent, adapted to this cache's