fix(simulation): PR #68 review — version bump, tracing warns, test coverage
- 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 <noreply@anthropic.com>
This commit is contained in:
Claude Opus 4.6
@@ -314,6 +314,11 @@ pub fn process_player_input(
|
||||
}
|
||||
PlayerAction::SaveGame { ref path } => {
|
||||
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),
|
||||
});
|
||||
|
||||
@@ -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<PlayerCharacter>>();
|
||||
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
|
||||
// -----------------------------------------------------------------------
|
||||
|
||||
@@ -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<Reverse<(u64, Entity, TilePosition)>> = 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<ScopePinned> 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<ScopePinned> were absent.
|
||||
let pinned: Vec<Entity> = (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<Entity> = (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::<ActiveSim>(entity).is_some(),
|
||||
"ScopePinned NPC {} must remain ActiveSim after eviction (D-026 #98)",
|
||||
i
|
||||
);
|
||||
assert!(
|
||||
world.get::<BackgroundSim>(entity).is_none(),
|
||||
"ScopePinned NPC {} must NOT be demoted to BackgroundSim",
|
||||
i
|
||||
);
|
||||
assert!(
|
||||
world.get::<StateSaved>(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::<ActiveSim>(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)
|
||||
// -----------------------------------------------------------------------
|
||||
|
||||
Reference in New Issue
Block a user