From 23fbfdbfc537b7728ded510e533176dbc5139410 Mon Sep 17 00:00:00 2001 From: Jeroen Schweitzer Date: Wed, 25 Feb 2026 12:32:35 +0100 Subject: [PATCH] =?UTF-8?q?fix(simulation):=20PR=20#68=20review=20?= =?UTF-8?q?=E2=80=94=20version=20bump,=20tracing=20warns,=20test=20coverag?= =?UTF-8?q?e?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Bump PROTOCOL_VERSION 14 → 15 for save_result field addition - Add tracing::warn on SaveLoadPending command overwrite (double-tap F5) - Add tracing::warn on KnowledgeGraph::new() fallback during save - Fix misleading WouldBlock comment in tcp.rs - Document SimSpacePressure.active_count pre-eviction timing - Document entity-based eviction tie-breaking non-determinism - Add ScopePinned eviction survival regression test - Regenerate msgpack fixtures for protocol v15 Co-Authored-By: Claude Opus 4.6 --- .../msgpack/snapshot_boundary_tick_0.msgpack | Bin 379 -> 373 bytes .../snapshot_boundary_tick_127.msgpack | Bin 379 -> 373 bytes .../snapshot_boundary_tick_2b31m1.msgpack | Bin 383 -> 377 bytes .../snapshot_boundary_tick_2b32.msgpack | Bin 387 -> 381 bytes .../snapshot_boundary_tick_32767.msgpack | Bin 381 -> 375 bytes .../fixtures/msgpack/snapshot_empty.msgpack | Bin 379 -> 373 bytes .../msgpack/snapshot_multi_entity.msgpack | Bin 784 -> 778 bytes .../fixtures/msgpack/snapshot_one_npc.msgpack | Bin 477 -> 471 bytes .../fixtures/msgpack/snapshot_player.msgpack | Bin 480 -> 474 bytes .../fixtures/msgpack/snapshot_v2_full.msgpack | Bin 644 -> 638 bytes server/src/bridge/tcp.rs | 2 +- server/src/bridge/types.rs | 4 +- server/src/simulation/input.rs | 10 ++ server/src/simulation/save_io.rs | 33 +++++- server/src/simulation/tier.rs | 106 +++++++++++++++++- server/tests/golden/proof_room_tick_10.json | 2 +- server/tests/serialization.rs | 2 +- 17 files changed, 150 insertions(+), 9 deletions(-) diff --git a/client/tests/fixtures/msgpack/snapshot_boundary_tick_0.msgpack b/client/tests/fixtures/msgpack/snapshot_boundary_tick_0.msgpack index 148ce3bcd558bbbf951cc30443441e6a809957b2..47a9ea59e8cb8ee53852956860f437296c0785de 100644 GIT binary patch delta 52 zcmey(^p%O{9)rm8vecsD%=|q5jXd6ra+{Jf5{nX(OHzyC3yM;Ui%W}A53DH2&y3H> IEH0S<03jk3XaE2J delta 58 zcmey$^qYz29)rm8vecsD%=|pQjXd6rD(g}!5_2>2QsawKi%WA#4s1%!NGwWBE=etl NF8~S^mlmZS006Bu7$pDz diff --git a/client/tests/fixtures/msgpack/snapshot_boundary_tick_127.msgpack b/client/tests/fixtures/msgpack/snapshot_boundary_tick_127.msgpack index 53c6b67504295d83e92e02d3dd0005b0e6263592..ffd3ba965b44e8ee593e53e92e9015cb9f85691c 100644 GIT binary patch delta 52 zcmey(^p%O{9)rm8vecsD%=|q5jXd6ra+{Jf5{nX(OHzyC3yM;Ui%W}A53DH2&y3H> IEH0S<03jk3XaE2J delta 58 zcmey$^qYz29)rm8vecsD%=|pQjXd6rD(g}!5_2>2QsawKi%WA#4s1%!NGwWBE=etl NF8~S^mlmZS006Bu7$pDz diff --git a/client/tests/fixtures/msgpack/snapshot_boundary_tick_2b31m1.msgpack b/client/tests/fixtures/msgpack/snapshot_boundary_tick_2b31m1.msgpack index 91f76e316921c6f2c06ace082708a12c04b63ab6..d275e542c04d342859503bc0ea49092d49227ece 100644 GIT binary patch delta 52 zcmey*^plC_9)rm8vecsD%=|q5jXeI0a+{Jf5{nX(OHzyC3yM;Ui%W}A53DH2&y3H> IEH0S<03$vXbN~PV delta 58 zcmey#^q-069)rm8vecsD%=|pQjXeI0D(g}!5_2>2QsawKi%WA#4s1%!NGwWBE=etl NF8~S^mlmZS006I<7%>0< diff --git a/client/tests/fixtures/msgpack/snapshot_boundary_tick_2b32.msgpack b/client/tests/fixtures/msgpack/snapshot_boundary_tick_2b32.msgpack index 3c851d4c8e1cbb0880e5571a5efe514e084e2166..88825035891121739ac9e6ea42a798c9987773bc 100644 GIT binary patch delta 52 zcmZo>{>#L3k3nR4S!z*nW_}+3MxJ0sxlPF#iA9OYC8VBo-wmm!uZO N7XXEdON&wu005RW7rp=h diff --git a/client/tests/fixtures/msgpack/snapshot_boundary_tick_32767.msgpack b/client/tests/fixtures/msgpack/snapshot_boundary_tick_32767.msgpack index 304069bcb2e90efaa885cbc3123132cee31db23a..13547db7c220f3b5852b7b5a5b7bc0798e168acc 100644 GIT binary patch delta 52 zcmey%^qq<49)rm8vecsD%=|q5jXb`La+{Jf5{nX(OHzyC3yM;Ui%W}A53DH2&y3H> IEH0S<03t9IZU6uP delta 58 zcmey)^p}a}9)rm8vecsD%=|pQjXb`LD(g}!5_2>2QsawKi%WA#4s1%!NGwWBE=etl NF8~S^mlmZS006FM7%Kn( diff --git a/client/tests/fixtures/msgpack/snapshot_empty.msgpack b/client/tests/fixtures/msgpack/snapshot_empty.msgpack index 148ce3bcd558bbbf951cc30443441e6a809957b2..47a9ea59e8cb8ee53852956860f437296c0785de 100644 GIT binary patch delta 52 zcmey(^p%O{9)rm8vecsD%=|q5jXd6ra+{Jf5{nX(OHzyC3yM;Ui%W}A53DH2&y3H> IEH0S<03jk3XaE2J delta 58 zcmey$^qYz29)rm8vecsD%=|pQjXd6rD(g}!5_2>2QsawKi%WA#4s1%!NGwWBE=etl NF8~S^mlmZS006Bu7$pDz diff --git a/client/tests/fixtures/msgpack/snapshot_multi_entity.msgpack b/client/tests/fixtures/msgpack/snapshot_multi_entity.msgpack index 7b28637f49e1b89fbdb8664041c0fb7331c49986..64ffe56ad347262e4f3a9326ccd1c900979304b9 100644 GIT binary patch delta 52 zcmbQh*2Ttik3nR4S!z*nW_}+3MxF;ua+{Jf5{nX(OHzyC3yM;Ui%W}A53DH2&y3H> IEH0S<01IUmFaQ7m delta 58 zcmeBTo503%k3nR4S!z*nW_}*uMxF;uD(g}!5_2>2QsawKi%WA#4s1%!NGwWBE=etl NF8~S^mlmZS005RK7wrH5 diff --git a/client/tests/fixtures/msgpack/snapshot_one_npc.msgpack b/client/tests/fixtures/msgpack/snapshot_one_npc.msgpack index a139d15d635f7f4ab7d103e3041ea3a2674895e5..6e7cbb7fa741051c66ce7d2d5013ec9b16048ae6 100644 GIT binary patch delta 52 zcmcc1e4Ux+9)rm8vecsD%=|q5jXY}^ { if let Some(ref mut sl) = save_load { + if sl.pending.is_some() { + tracing::warn!( + "SaveGame overwrites already-pending save/load command (dropped)" + ); + } sl.pending = Some(SaveLoadCommand::Save { path: std::path::PathBuf::from(path), }); @@ -324,6 +329,11 @@ pub fn process_player_input( } PlayerAction::LoadGame { ref path } => { if let Some(ref mut sl) = save_load { + if sl.pending.is_some() { + tracing::warn!( + "LoadGame overwrites already-pending save/load command (dropped)" + ); + } sl.pending = Some(SaveLoadCommand::Load { path: std::path::PathBuf::from(path), }); diff --git a/server/src/simulation/save_io.rs b/server/src/simulation/save_io.rs index a4bb3562a..47b155b57 100644 --- a/server/src/simulation/save_io.rs +++ b/server/src/simulation/save_io.rs @@ -84,9 +84,10 @@ pub fn save_to_file(path: &Path, world: &mut World) -> Result<(), SaveLoadError> // Player knowledge graph — the observer's epistemics at save time let player_knowledge = { let mut q = world.query_filtered::<&KnowledgeGraph, With>(); - q.single(world) - .cloned() - .unwrap_or_else(|_| KnowledgeGraph::new()) + q.single(world).cloned().unwrap_or_else(|_| { + tracing::warn!("save_to_file: no PlayerCharacter with KnowledgeGraph found — saving empty graph"); + KnowledgeGraph::new() + }) }; // Global NPC social web @@ -610,6 +611,32 @@ mod tests { assert!(result.error.is_some(), "error message should be present"); } + // ----------------------------------------------------------------------- + // Overwrite behaviour + // ----------------------------------------------------------------------- + + /// When two commands arrive in the same tick, the second overwrites the first. + /// The warn! in process_player_input fires; here we just confirm last-write-wins. + #[test] + fn pending_command_overwrite_last_write_wins() { + let mut pending = SaveLoadPending::default(); + + pending.pending = Some(SaveLoadCommand::Save { + path: PathBuf::from("/tmp/first.msgpack"), + }); + // Overwrite with a Load command + pending.pending = Some(SaveLoadCommand::Load { + path: PathBuf::from("/tmp/second.msgpack"), + }); + + match pending.pending.unwrap() { + SaveLoadCommand::Load { ref path } => { + assert_eq!(path.to_str().unwrap(), "/tmp/second.msgpack"); + } + other => panic!("expected Load, got {:?}", other), + } + } + // ----------------------------------------------------------------------- // SaveLoadError display // ----------------------------------------------------------------------- diff --git a/server/src/simulation/tier.rs b/server/src/simulation/tier.rs index 3fe5ac4de..9a312fbd8 100644 --- a/server/src/simulation/tier.rs +++ b/server/src/simulation/tier.rs @@ -67,7 +67,12 @@ pub struct LastInteractionTick(pub u64); /// Updated each tick by `evict_excess_active`. #[derive(Resource, Debug, Clone)] pub struct SimSpacePressure { - /// Number of entities currently in `ActiveSim`. + /// Number of entities in `ActiveSim` at the start of the current tick's eviction pass. + /// + /// Set by `evict_excess_active` *before* any evictions run. Eviction commands are + /// deferred (applied after the system), so `active_count` reflects the pre-eviction + /// count, not the post-eviction count. Consumers (e.g., HUD pressure display) should + /// treat this as the high-water mark for the tick. pub active_count: usize, /// Capacity ceiling. pub capacity: usize, @@ -369,6 +374,10 @@ pub fn evict_excess_active( // Min-heap keyed by LastInteractionTick (oldest = smallest = evicted first). // Entities without LastInteractionTick get tick 0 (most stale). + // NOTE: Ties in tick value are broken by Entity index, which is non-deterministic + // across runs (bevy Entity allocation order). For v0.1 this is acceptable — + // deterministic replay (D-010 principle 4) replays inputs, not eviction order. + // If eviction order must be deterministic, key by (tick, StableId) instead. let mut heap: BinaryHeap> = BinaryHeap::new(); for (entity, pos, maybe_tick) in &active_npcs { let tick = maybe_tick.map(|t| t.0).unwrap_or(0); @@ -1213,6 +1222,101 @@ mod tests { assert_eq!(pressure.active_count, 3, "pressure tracks pre-eviction count"); } + #[test] + fn scope_pinned_npcs_survive_eviction_at_scale() { + // Regression: evict_excess_active must never demote a ScopePinned NPC, + // even when many NPCs are over capacity (D-026, #97, #98). + // + // Setup: 85 Active NPCs (capacity = 80 → 5 must be evicted). + // - 10 are ScopePinned (must ALL remain ActiveSim after eviction). + // - 75 are unpinned (5 oldest are eviction targets; 70 survive). + // + // The Without query filter in evict_excess_active is the + // core invariant under test. This test fails immediately if that filter + // is removed or mis-applied. + let mut world = World::new(); + world.insert_resource(SimSpacePressure { + active_count: 0, + capacity: 80, + }); + + // Player at origin — all NPCs are within BACKGROUND_RADIUS. + world.spawn((PlayerCharacter, make_pos(0, 0))); + + // Spawn 10 ScopePinned NPCs. Give them the oldest ticks so they would + // be prime eviction candidates if Without were absent. + let pinned: Vec = (0..10) + .map(|i| { + world + .spawn(( + Npc, + ActiveSim, + ScopePinned, + make_pos(5 + i, 0), + LastInteractionTick(i as u64), + )) + .id() + }) + .collect(); + + // Spawn 5 unpinned NPCs with old ticks — these are the actual eviction targets. + let unpinned_oldest: Vec = (0..5) + .map(|i| { + world + .spawn(( + Npc, + ActiveSim, + make_pos(20 + i, 0), + LastInteractionTick(i as u64), + )) + .id() + }) + .collect(); + + // Spawn 70 unpinned NPCs with newer ticks — these survive. + for i in 0..70i32 { + world.spawn(( + Npc, + ActiveSim, + make_pos(30 + i, 0), + LastInteractionTick(100 + i as u64), + )); + } + + // Total: 10 pinned + 5 oldest-unpinned + 70 newer-unpinned = 85 active. + // cap = 80 → exactly 5 must be evicted. + run_evict_excess_active(&mut world); + + // Core invariant: ALL pinned entities remain ActiveSim. + for (i, &entity) in pinned.iter().enumerate() { + assert!( + world.get::(entity).is_some(), + "ScopePinned NPC {} must remain ActiveSim after eviction (D-026 #98)", + i + ); + assert!( + world.get::(entity).is_none(), + "ScopePinned NPC {} must NOT be demoted to BackgroundSim", + i + ); + assert!( + world.get::(entity).is_none(), + "ScopePinned NPC {} must NOT be demoted to StateSaved", + i + ); + } + + // Sanity: the 5 oldest unpinned were the ones evicted. + let evicted_count = unpinned_oldest + .iter() + .filter(|&&e| world.get::(e).is_none()) + .count(); + assert_eq!( + evicted_count, 5, + "exactly 5 unpinned NPCs (the oldest) should have been evicted to reach capacity" + ); + } + // ----------------------------------------------------------------------- // LastInteractionTick component tests (#97) // ----------------------------------------------------------------------- diff --git a/server/tests/golden/proof_room_tick_10.json b/server/tests/golden/proof_room_tick_10.json index fdbc7e163..3b8bbad72 100644 --- a/server/tests/golden/proof_room_tick_10.json +++ b/server/tests/golden/proof_room_tick_10.json @@ -73,7 +73,7 @@ "scan_events": [], "sound_events": [], "tick": 8, - "version": 14, + "version": 15, "visible_tiles": [ { "tile_kind": "Wall", diff --git a/server/tests/serialization.rs b/server/tests/serialization.rs index 57bae1cf4..8a505e69b 100644 --- a/server/tests/serialization.rs +++ b/server/tests/serialization.rs @@ -346,7 +346,7 @@ fn protocol_version_constant_matches_snapshot() { let snapshot = test_snapshot(0, vec![]); assert_eq!(snapshot.version, PROTOCOL_VERSION); assert_eq!( - PROTOCOL_VERSION, 14, + PROTOCOL_VERSION, 15, "bump this assertion when protocol version changes" ); }