diff --git a/Cargo.lock b/Cargo.lock index 97ead63b03..405cee11b4 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1649,6 +1649,15 @@ version = "0.2.186" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "68ab91017fe16c622486840e4c83c9a37afeff978bd239b5293d61ece587de66" +[[package]] +name = "libmimalloc-sys" +version = "0.1.49" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "6a45a52f43e1c16f667ccfe4dd8c85b7f7c204fd5e3bf46c5b0db9a5c3c0b8e9" +dependencies = [ + "cc", +] + [[package]] name = "libsqlite3-sys" version = "0.30.1" @@ -1784,6 +1793,15 @@ version = "2.8.0" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "f8ca58f447f06ed17d5fc4043ce1b10dd205e060fb3ce5b979b8ed8e59ff3f79" +[[package]] +name = "mimalloc" +version = "0.1.52" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "2d4139bb28d14ad1facf21d5eb8825051b326e172d216b39f6d31df53cc97862" +dependencies = [ + "libmimalloc-sys", +] + [[package]] name = "mime" version = "0.3.17" @@ -2049,6 +2067,7 @@ dependencies = [ name = "phase-ai" version = "0.40.0" dependencies = [ + "mimalloc", "phase-engine", "rand 0.9.4", "rand_chacha 0.9.0", diff --git a/crates/engine/src/game/zone_pipeline.rs b/crates/engine/src/game/zone_pipeline.rs index e97a7d852c..f0f7c1eea2 100644 --- a/crates/engine/src/game/zone_pipeline.rs +++ b/crates/engine/src/game/zone_pipeline.rs @@ -969,6 +969,15 @@ pub(crate) fn move_object_with_terminal( if matches!(req.cause, ZoneChangeCause::DebugCommand) { let delivery_start = events.len(); zones::move_to_zone(state, req.object_id, req.to, events); + // pod-lab loop-3 Q5: debug-staged board setup (GameScenario, the + // shared seam nearly every engine integration test builds boards + // through) stays maximally conservative and out of scope for the + // move_to_zone incremental-flush carve-out below — there is zero + // gameplay perf benefit to incrementalizing test/debug setup, and + // the blast radius of a subtle divergence here is the whole test + // suite. Unconditional, regardless of what move_to_zone's own + // (now axis-gated) internal decision would otherwise have been. + crate::game::layers::mark_layers_full(state); return ZoneMoveTerminalResult::Completed(zone_move_completion_from_delivery( member, &events[delivery_start..], @@ -2302,8 +2311,17 @@ pub(crate) fn deliver_replaced_zone_change( obj.face_down = true; } } + // pod-lab loop-3 Q5: tracks whether this delivery took the plain, + // non-merge, non-library-placement `move_to_zone` branch — the ONLY + // branch whose own internal dirty-mark decision (see the carve-out + // added to `move_to_zone` above) is trustworthy enough to let the + // redundant check below skip re-marking `Full`. `false` for both the + // library-placement branch and the merge-survivor branch, neither of + // which is analyzed by that carve-out. + let took_plain_zone_transfer; match (to, library_placement.as_ref()) { (Zone::Library, Some(position)) => { + took_plain_zone_transfer = false; let index = match position { LibraryPosition::Top => Some(0), LibraryPosition::Bottom => None, @@ -2334,8 +2352,10 @@ pub(crate) fn deliver_replaced_zone_change( // replacement consult. Deliver the resulting destination // with the CR 730.3 `from: None` event shape rather than // pretending it independently left the battlefield. + took_plain_zone_transfer = false; crate::game::merge::put_component_into_zone(state, object_id, to, events); } else { + took_plain_zone_transfer = true; zones::move_to_zone(state, object_id, to, events); } } @@ -2397,7 +2417,20 @@ pub(crate) fn deliver_replaced_zone_change( // and before the zone-change record snapshots is_token. That is the sole // path a copy (only ever created on the stack by `Effect::CastCopyOfCard`) // reaches the battlefield, so no un-flipped copy can arrive here. - if to == Zone::Battlefield || from == Zone::Battlefield { + // pod-lab loop-3 Q5: `move_to_zone` (above) already made the correct, + // precise dirty-mark decision for a plain transfer that actually + // landed on the battlefield — this check no longer re-clobbers it to + // `Full`. Gated on `entered_battlefield && took_plain_zone_transfer` + // together, not `took_plain_zone_transfer` alone: a rejected entry + // (Grafdigger's Cage-class `CantEnterBattlefieldFrom`, CR 614.1d) has + // `entered_battlefield == false` and must keep this unconditional + // mark, since `move_to_zone` never reached its own mark block for a + // rejected entry at all. A merge-survivor delivery or a + // library-placement delivery is `took_plain_zone_transfer == false` + // and is untouched, exactly as before. + if from == Zone::Battlefield + || (to == Zone::Battlefield && !(entered_battlefield && took_plain_zone_transfer)) + { crate::game::layers::mark_layers_full(state); } // CR 708.3: An object put onto the battlefield face down is turned face @@ -4017,3 +4050,552 @@ mod face_down_exile_entry_tests { ); } } + +/// pod-lab loop-3 Q5: verifies the `move_to_zone`/`deliver_replaced_zone_change` +/// incremental-flush carve-out through the FULL production pipeline +/// (`move_object`/`ZoneMoveRequest::effect`), not just `zones::move_to_zone` in +/// isolation — so `entered_battlefield`/`took_plain_zone_transfer`, computed in +/// this file, are genuinely exercised. Every assertion reads +/// `state.layers_dirty` directly (the dirty-lattice/flush-arm seam itself), +/// not just final board state, per this fix's own verification-matrix +/// requirement that a test must fail if the carve-out is reverted or +/// mis-scoped — board state alone is identical either way for a plain +/// creature entry, so it cannot prove which path was taken. +#[cfg(test)] +mod layers_incremental_flush_tests { + use super::*; + use crate::game::zones::create_object; + use crate::types::ability::{ + ContinuousModification, FilterProp, StaticDefinition, TargetFilter, TypeFilter, TypedFilter, + }; + use crate::types::card_type::CoreType; + use crate::types::game_state::LayersDirty; + use crate::types::identifiers::CardId; + use crate::types::statics::StaticMode; + + fn reset_clean(state: &mut GameState) { + state.layers_dirty = LayersDirty::Clean; + } + + /// Row 1 (verification matrix): the dominant real-game case — a plain + /// creature resolving from the Stack — takes the cheap `EnteredObjects` + /// path, not `Full`. This is the fix's entire perf payoff; if this + /// assertion regresses to `Full`, the carve-out has been reverted or + /// over-narrowed. + #[test] + fn stack_to_battlefield_plain_entry_marks_entered_not_full() { + let mut state = GameState::new_two_player(42); + let source = create_object( + &mut state, + CardId(80001), + PlayerId(0), + "Effect Source".to_string(), + Zone::Battlefield, + ); + let spell = create_object( + &mut state, + CardId(80002), + PlayerId(0), + "Vanilla Creature".to_string(), + Zone::Stack, + ); + reset_clean(&mut state); + + let mut events = Vec::new(); + let _ = move_object( + &mut state, + ZoneMoveRequest::effect(spell, Zone::Battlefield, source), + &mut events, + ); + + assert_eq!(state.objects[&spell].zone, Zone::Battlefield); + match &state.layers_dirty { + LayersDirty::EnteredObjects(ids) => { + assert!( + ids.contains(&spell), + "the entering object must be tracked in EnteredObjects" + ); + } + other => panic!( + "a plain Stack-to-Battlefield entry must take the incremental \ + EnteredObjects path, got {other:?}" + ), + } + } + + /// Row 2: Hand->Battlefield (land plays, Elvish Piper, Sneak Attack, Show + /// and Tell) must keep forcing `Full` UNCONDITIONALLY. `layers.rs`'s + /// zone-reading classifier hardcodes `QuantityRef::HandSize` to `false`, + /// so a live HandSize-gated static (Carnage Interpreter class) would go + /// undetected by `static_dependency_before`/`after` alone — this + /// unconditional exclusion is that class's only protection. + #[test] + fn hand_to_battlefield_still_marks_full_via_pipeline() { + let mut state = GameState::new_two_player(42); + let source = create_object( + &mut state, + CardId(80011), + PlayerId(0), + "Effect Source".to_string(), + Zone::Battlefield, + ); + let land = create_object( + &mut state, + CardId(80012), + PlayerId(0), + "Forest".to_string(), + Zone::Hand, + ); + reset_clean(&mut state); + + let mut events = Vec::new(); + let _ = move_object( + &mut state, + ZoneMoveRequest::effect(land, Zone::Battlefield, source), + &mut events, + ); + + assert_eq!(state.objects[&land].zone, Zone::Battlefield); + assert!( + matches!(state.layers_dirty, LayersDirty::Full), + "a Hand-origin battlefield entry must still force a full \ + re-evaluation, got {:?}", + state.layers_dirty + ); + } + + /// Row 7b (round-3 review blocker): Exile->Battlefield (reanimation, + /// flicker return, "you may cast this from exile") must ALSO keep forcing + /// `Full` unconditionally, for the identical reason as Hand. + /// `QuantityRef::CardsExiledBySource`/`ExiledCardPower`/`TrackedSetSize`/ + /// `FilteredTrackedSetSize`/`TrackedSetAggregate` (Unlicensed Hearse, + /// Veteran Survivor, Sutured Ghoul class) are ALL hardcoded to `false` in + /// the same classifier, and their count is live-filtered on + /// `obj.zone == Zone::Exile` — it changes the instant a linked card + /// leaves Exile, with no Axis-2 flush-time analog to catch it. + #[test] + fn exile_to_battlefield_still_marks_full_via_pipeline() { + let mut state = GameState::new_two_player(42); + let source = create_object( + &mut state, + CardId(80021), + PlayerId(0), + "Effect Source".to_string(), + Zone::Battlefield, + ); + let exiled = create_object( + &mut state, + CardId(80022), + PlayerId(0), + "Exiled Creature".to_string(), + Zone::Exile, + ); + reset_clean(&mut state); + + let mut events = Vec::new(); + let _ = move_object( + &mut state, + ZoneMoveRequest::effect(exiled, Zone::Battlefield, source), + &mut events, + ); + + assert_eq!(state.objects[&exiled].zone, Zone::Battlefield); + assert!( + matches!(state.layers_dirty, LayersDirty::Full), + "an Exile-origin battlefield entry must still force a full \ + re-evaluation, got {:?}", + state.layers_dirty + ); + } + + /// Row 3: a battlefield entry REJECTED by a `CantEnterBattlefieldFrom` + /// static (Grafdigger's Cage class, CR 614.1d) must still mark `Full` via + /// `zone_pipeline.rs`'s `entered_battlefield` gate — `move_to_zone` never + /// reaches its own (now axis-gated) mark block for a rejected entry at + /// all, so this file's redundant check is the ONLY thing marking + /// anything for this case, exactly as before the carve-out existed. + #[test] + fn rejected_battlefield_entry_still_marks_full() { + let mut state = GameState::new_two_player(42); + let source = create_object( + &mut state, + CardId(80031), + PlayerId(0), + "Effect Source".to_string(), + Zone::Battlefield, + ); + let cage = create_object( + &mut state, + CardId(80032), + PlayerId(0), + "Grafdigger's Cage".to_string(), + Zone::Battlefield, + ); + { + let obj = state.objects.get_mut(&cage).unwrap(); + obj.card_types.core_types.push(CoreType::Artifact); + obj.static_definitions.push( + StaticDefinition::new(StaticMode::CantEnterBattlefieldFrom).affected( + TargetFilter::Typed( + TypedFilter::default() + .with_type(TypeFilter::Creature) + .properties(vec![FilterProp::InAnyZone { + zones: vec![Zone::Graveyard, Zone::Library], + }]), + ), + ), + ); + } + let caged = create_object( + &mut state, + CardId(80033), + PlayerId(0), + "Caged Creature".to_string(), + Zone::Library, + ); + { + let obj = state.objects.get_mut(&caged).unwrap(); + obj.card_types.core_types = vec![CoreType::Creature]; + obj.base_card_types = obj.card_types.clone(); + } + reset_clean(&mut state); + + let mut events = Vec::new(); + let _ = move_object( + &mut state, + ZoneMoveRequest::effect(caged, Zone::Battlefield, source), + &mut events, + ); + + assert_eq!( + state.objects[&caged].zone, + Zone::Library, + "a CantEnterBattlefieldFrom static must keep the card in its origin zone" + ); + assert!( + matches!(state.layers_dirty, LayersDirty::Full), + "a rejected battlefield entry must still force a full \ + re-evaluation via entered_battlefield, got {:?}", + state.layers_dirty + ); + } + + /// F5 (CodeRabbit, PR #6777 round): the incremental `EnteredObjects` path + /// (Row 1) is only safe when nothing on the board reads membership of the + /// entering object's origin or destination zone. A Graveyard->Battlefield + /// entry (reanimation) is neither the Hand nor Exile carve-out, so it + /// would wrongly take the cheap path unless `static_dependency_before`/ + /// `after` itself catches it: a live static (Tarmogoyf class) whose + /// `affected` filter reads `Zone::Graveyard` must still force `Full` when + /// a card leaves that zone for the battlefield. Round 4: the watcher + /// carries a real modification (it sources a live effect) and the fixture + /// is primed by a real flush, so the before arm is proven through the + /// INDEXED path — bucket membership asserted, no empty-index fallback. + #[test] + fn battlefield_entry_with_static_dependency_marks_full_via_pipeline() { + let mut state = GameState::new_two_player(42); + let watcher = create_object( + &mut state, + CardId(80041), + PlayerId(0), + "Graveyard Watcher".to_string(), + Zone::Battlefield, + ); + { + let obj = state.objects.get_mut(&watcher).unwrap(); + obj.card_types.core_types.push(CoreType::Creature); + obj.base_card_types = obj.card_types.clone(); + // Round-4 fix (maintainer, PR #6777): a real companion modification + // so the watcher sources a LIVE continuous effect — active effects + // are built by iterating `def.modifications` + // (`active_continuous_effects_from_static_definitions`), so an + // affected-filter-only def would source nothing. + obj.static_definitions.push( + StaticDefinition::new(StaticMode::Continuous) + .affected(TargetFilter::Typed( + TypedFilter::default() + .with_type(TypeFilter::Creature) + .properties(vec![FilterProp::InAnyZone { + zones: vec![Zone::Graveyard], + }]), + )) + .modifications(vec![ContinuousModification::AddPower { value: 1 }]), + ); + } + let source = create_object( + &mut state, + CardId(80042), + PlayerId(0), + "Effect Source".to_string(), + Zone::Battlefield, + ); + let reanimated = create_object( + &mut state, + CardId(80043), + PlayerId(0), + "Graveyard Creature".to_string(), + Zone::Graveyard, + ); + { + let obj = state.objects.get_mut(&reanimated).unwrap(); + obj.card_types.core_types = vec![CoreType::Creature]; + obj.base_card_types = obj.card_types.clone(); + } + // Round-4 fix (maintainer, PR #6777): prime with a real flush so the + // live watcher is INDEXED, then prove the before arm fires through the + // indexed path (buckets non-empty — no empty-index fallback involved). + crate::game::layers::mark_layers_full(&mut state); + crate::game::layers::flush_layers(&mut state); + assert_eq!( + state.static_source_index.battlefield_sources.len(), + 1, + "fixture premise: the live graveyard-reading watcher is indexed after the priming flush" + ); + assert!( + crate::game::layers::static_layer_dependency_for_zone_transition( + &state, + Zone::Graveyard, + Zone::Battlefield + ), + "the indexed watcher must make the pre-transition dependency check true" + ); + + let mut events = Vec::new(); + let _ = move_object( + &mut state, + ZoneMoveRequest::effect(reanimated, Zone::Battlefield, source), + &mut events, + ); + + assert_eq!(state.objects[&reanimated].zone, Zone::Battlefield); + assert!( + matches!(state.layers_dirty, LayersDirty::Full), + "a Graveyard-origin battlefield entry with a live zone-membership-dependent static must force a full re-evaluation via static_dependency_before/after, got {:?}", + state.layers_dirty + ); + } + + /// F5 discrimination companion (maintainer, PR #6777 round 2): the sibling + /// test's watcher static is live on the battlefield BEFORE the transition, + /// and `move_object` ORs `static_dependency_before || static_dependency_after` + /// — so that test alone cannot catch the post-entry arm being dropped. + /// Here the zone-reading static rides ON the entering object itself: while + /// it sits in the Graveyard it is not a static-effect source (only + /// battlefield/command objects generate continuous effects), so the + /// before-check is false, and only the post-entry re-check + /// (`static_dependency_after`) can see the now-live static and force + /// `Full`. Removing the after arm turns this mark into the cheap + /// `EnteredObjects` path and fails this test. + #[test] + fn battlefield_entry_whose_own_zone_reading_static_marks_full_post_entry() { + let mut state = GameState::new_two_player(42); + let source = create_object( + &mut state, + CardId(80044), + PlayerId(0), + "Effect Source".to_string(), + Zone::Battlefield, + ); + let entering_watcher = create_object( + &mut state, + CardId(80045), + PlayerId(0), + "Entering Graveyard Watcher".to_string(), + Zone::Graveyard, + ); + { + let obj = state.objects.get_mut(&entering_watcher).unwrap(); + obj.card_types.core_types = vec![CoreType::Creature]; + obj.base_card_types = obj.card_types.clone(); + // Round-4 fix (maintainer, PR #6777): a real companion modification + // so the arriving watcher sources a LIVE continuous effect once on + // the battlefield (active effects iterate `def.modifications`). + obj.static_definitions.push( + StaticDefinition::new(StaticMode::Continuous) + .affected(TargetFilter::Typed( + TypedFilter::default() + .with_type(TypeFilter::Creature) + .properties(vec![FilterProp::InAnyZone { + zones: vec![Zone::Graveyard], + }]), + )) + .modifications(vec![ContinuousModification::AddPower { value: 1 }]), + ); + } + + // Round-3 fix (maintainer, PR #6777): prime with a REAL layer flush so + // the fixture carries exactly the index state production would — not a + // hand-reset that leaves `static_source_index` unbuilt. For this board + // shape the rebuild leaves the index PRECISELY empty: + // `StaticSourceIndex::rebuild_from_state` keys on generators only + // (static_source_index.rs), the lone battlefield object carries no + // static, and the graveyard watcher sits outside both indexed buckets. + // At the post-entry check the index is then stale-EMPTY, not + // legitimately empty: the watcher has arrived and IS a generator, but + // nothing rebuilds the index mid-move, so `use_fallback` in + // `for_each_static_effect_source` fires and the direct scan sees the + // newcomer. That is exactly the production shape of "the first + // generator enters a generator-free board" — reached in any real game + // — so the `Full` mark pinned here is live behavior, not a recovery + // path for hand-built state. The populated-bucket shape, where the + // after arm provably cannot see the newcomer, is pinned by + // `populated_index_entry_defers_zone_reading_static_to_flush_escalation`. + crate::game::layers::mark_layers_full(&mut state); + crate::game::layers::flush_layers(&mut state); + assert!( + matches!(state.layers_dirty, LayersDirty::Clean), + "priming flush must leave the dirty lattice clean, got {:?}", + state.layers_dirty + ); + assert!( + state.static_source_index.battlefield_sources.is_empty() + && state.static_source_index.command_sources.is_empty(), + "fixture premise: a generator-free board rebuilds to a precisely empty index" + ); + + // Precondition for discrimination: with the watcher still in the + // Graveyard, no battlefield/command object reads zone membership, so + // the pre-transition check must come up empty and the post-entry arm + // is the only guard under test. + assert!( + !crate::game::layers::static_layer_dependency_for_zone_transition( + &state, + Zone::Graveyard, + Zone::Battlefield + ), + "fixture invalid: a pre-transition static dependency would let the before arm mask the after arm" + ); + + let mut events = Vec::new(); + let _ = move_object( + &mut state, + ZoneMoveRequest::effect(entering_watcher, Zone::Battlefield, source), + &mut events, + ); + + assert_eq!(state.objects[&entering_watcher].zone, Zone::Battlefield); + assert!( + matches!(state.layers_dirty, LayersDirty::Full), + "an entering object that itself carries a zone-membership-reading static must force a full re-evaluation via static_dependency_after (the before check is provably false here), got {:?}", + state.layers_dirty + ); + } + + /// Round-3 companion (maintainer, PR #6777): the POPULATED-index shape. + /// With an unrelated generator on the battlefield the indexed buckets are + /// non-empty, and the mid-mutation dependency checks read a stale-by-design + /// index (rebuilt only at the top of a flush pass — see the "Authority" + /// note in static_source_index.rs): the just-entered watcher is not yet a + /// bucket member, so BOTH the before and after arms are false and + /// `move_object` proposes the cheap `EnteredObjects` mark. Safety for this + /// shape is delivered at flush time, not at the mutation site: + /// `prepare_incremental_flush` escalates to a full pass (arm (1) of + /// `entered_object_blocks_incremental` fires first on the entering + /// object's live continuous effect; the recipient-sourced active-effect + /// check just after the index rebuild is a redundant backstop). This + /// test pins that handoff end-to-end — cheap mark at the seam, escalation + /// plus full evaluation at the flush — so neither half of the contract can + /// silently regress. + #[test] + fn populated_index_entry_defers_zone_reading_static_to_flush_escalation() { + let mut state = GameState::new_two_player(42); + let generator = create_object( + &mut state, + CardId(80046), + PlayerId(0), + "Benign Generator".to_string(), + Zone::Battlefield, + ); + { + let obj = state.objects.get_mut(&generator).unwrap(); + obj.card_types.core_types = vec![CoreType::Creature]; + obj.base_card_types = obj.card_types.clone(); + // Plain typed filter: no InZone/InAnyZone prop, so per + // `target_filter_reads_zone` it reads membership of NEITHER + // transition zone — it exists only to populate the index bucket. + obj.static_definitions.push( + StaticDefinition::new(StaticMode::Continuous) + .affected(TargetFilter::Typed( + TypedFilter::default().with_type(TypeFilter::Creature), + )) + .modifications(vec![ContinuousModification::AddPower { value: 1 }]), + ); + } + let source = create_object( + &mut state, + CardId(80047), + PlayerId(0), + "Effect Source".to_string(), + Zone::Battlefield, + ); + let entering_watcher = create_object( + &mut state, + CardId(80048), + PlayerId(0), + "Entering Graveyard Watcher".to_string(), + Zone::Graveyard, + ); + { + let obj = state.objects.get_mut(&entering_watcher).unwrap(); + obj.card_types.core_types = vec![CoreType::Creature]; + obj.base_card_types = obj.card_types.clone(); + // A real modification (not just a zone-reading affected filter) so + // the entered object sources a live continuous effect once on the + // battlefield — the exact condition `entered_object_blocks_incremental` + // arm (1) escalates on. + obj.static_definitions.push( + StaticDefinition::new(StaticMode::Continuous) + .affected(TargetFilter::Typed( + TypedFilter::default() + .with_type(TypeFilter::Creature) + .properties(vec![FilterProp::InAnyZone { + zones: vec![Zone::Graveyard], + }]), + )) + .modifications(vec![ContinuousModification::AddPower { value: 1 }]), + ); + } + + crate::game::layers::mark_layers_full(&mut state); + crate::game::layers::flush_layers(&mut state); + assert_eq!( + state.static_source_index.battlefield_sources.len(), + 1, + "fixture premise: exactly the benign generator is indexed after the priming flush" + ); + assert!( + !crate::game::layers::static_layer_dependency_for_zone_transition( + &state, + Zone::Graveyard, + Zone::Battlefield + ), + "fixture invalid: the generator must not read either transition zone" + ); + + let mut events = Vec::new(); + let _ = move_object( + &mut state, + ZoneMoveRequest::effect(entering_watcher, Zone::Battlefield, source), + &mut events, + ); + + assert_eq!(state.objects[&entering_watcher].zone, Zone::Battlefield); + assert!( + matches!(state.layers_dirty, LayersDirty::EnteredObjects(_)), + "with populated (stale-by-design) buckets the mutation-site arms cannot see the entering object's own static; the cheap mark is the designed outcome here, got {:?}", + state.layers_dirty + ); + + crate::game::perf_counters::reset(); + crate::game::layers::flush_layers(&mut state); + let counters = crate::game::perf_counters::snapshot(); + assert_eq!( + counters.layers_escalated, 1, + "flush must escalate: the entered object sources a live continuous effect" + ); + assert_eq!( + counters.layers_full_eval, 1, + "the escalation must land in a full evaluation" + ); + } +} diff --git a/crates/engine/src/game/zones.rs b/crates/engine/src/game/zones.rs index 1ea1e3fa11..7606e6a395 100644 --- a/crates/engine/src/game/zones.rs +++ b/crates/engine/src/game/zones.rs @@ -1216,10 +1216,48 @@ pub fn move_to_zone( let static_dependency_after = crate::game::layers::static_layer_dependency_for_zone_transition(state, from, to); - // CR 611.3a + CR 400.3: Hand size affects continuous effects gated on the - // controller's hand (Carnage Interpreter, issue #3991) and hand-zone - // effects (Miracle in hand). Re-evaluate layers on any hand entry/exit. + // pod-lab loop-3 Q5: a plain Battlefield entry that doesn't originate + // from Hand or Exile, and isn't itself the source of a live + // zone-membership-dependent static (static_dependency_before/after), + // can take the cheaper `mark_layers_entered` path instead of forcing a + // full re-evaluation of every object's characteristics. This does NOT + // skip re-verification: `prepare_incremental_flush` (layers.rs) re-runs + // its own full Axis-1/Axis-2 safety analysis fresh from live state at + // flush time regardless of which mark got set here, and escalates to a + // full pass itself whenever that analysis can't prove the entering + // object is safe (a sourced continuous effect, a CDA, counters, + // attachments, or a population-perturbing static). This call only + // proposes the cheap mark when the mutation site itself has nothing + // else forcing a full re-evaluation; it is not the safety net. + // + // Hand and Exile are excluded UNCONDITIONALLY here, not merely folded + // into static_dependency_before/after, because both have a proven blind + // spot in that check: + // - CR 611.3a + CR 400.3: hand size affects continuous effects gated + // on the controller's hand (Carnage Interpreter, issue #3991), and + // `layers.rs`'s `quantity_ref_reads_zone` classifier maps + // `QuantityRef::HandSize` to a hardcoded `false` — a live + // HandSize-gated static is not detected as a zone dependency at all. + // - CR 613.1: characteristics set by "for each card exiled with/by + // [this]"-style statics (`QuantityRef::CardsExiledBySource`, + // `ExiledCardPower`, `TrackedSetSize`, `FilteredTrackedSetSize`, + // `TrackedSetAggregate` — e.g. Unlicensed Hearse, Veteran Survivor, + // Sutured Ghoul) have the identical blind spot: the same classifier + // maps all of them to `false`, and the count is live-filtered on + // `obj.zone == Zone::Exile` (see `linked_exile_for_context` / + // `players.rs`), so it changes the instant a linked card leaves + // Exile for the Battlefield. Neither axis has a Axis-2 analog in + // `prepare_incremental_flush` (which is exclusively board-population + // framed), so there is no flush-time safety net for either — the + // unconditional mark at this mutation site is these statics' ONLY + // protection, exactly as it is today. if to == Zone::Battlefield + && from != Zone::Hand + && from != Zone::Exile + && !(static_dependency_before || static_dependency_after) + { + crate::game::layers::mark_layers_entered(state, object_id); + } else if to == Zone::Battlefield || from == Zone::Battlefield || to == Zone::Hand || from == Zone::Hand @@ -1310,6 +1348,17 @@ pub(crate) fn restore_after_rollback( events: &mut Vec, ) { move_to_zone(state, object_id, to, events); + // CR 601.2 + CR 733.1: reversing an incomplete action needs full + // reconciliation regardless of which mark move_to_zone's own + // axis-gated internal logic picked — an undone action is rare + // (not gameplay-hot) and can leave board state in a shape the + // entry-only incremental-flush safety classifier was never designed to + // reason about, so there is no perf case for trusting it here. This is + // conservatively at-or-above today's marking, not byte-for-byte + // identical to it: some rollback transitions `move_to_zone` marks + // nothing for today (e.g. Stack->Library) become `Full` here, which is + // strictly safe, never a behavior change a test could observe as wrong. + crate::game::layers::mark_layers_full(state); } /// CR 603.10a: Record that every member of `group` left the battlefield in the @@ -4069,4 +4118,41 @@ mod tests { "SBA zone movement must still publish the unattach event for triggers" ); } + + /// pod-lab loop-3 Q5, row 5: `restore_after_rollback` targeting the + /// battlefield must still force a full layers re-evaluation + /// unconditionally — CR 601.2 + CR 733.1, reversing an incomplete action + /// is rare (not gameplay-hot) and can leave board state in a shape the + /// entry-only incremental-flush safety classifier was never designed to + /// reason about, so there is no perf case for trusting `move_to_zone`'s + /// own (now axis-gated) internal decision here. Today's only production + /// caller targets Graveyard, not Battlefield, so this exercises the + /// function's general contract directly rather than replaying an + /// existing call site. + #[test] + fn restore_after_rollback_to_battlefield_marks_full() { + let mut state = setup(); + let id = create_object( + &mut state, + CardId(1), + PlayerId(0), + "Rolled Back Spell".to_string(), + Zone::Stack, + ); + state.layers_dirty = crate::types::game_state::LayersDirty::Clean; + + let mut events = Vec::new(); + restore_after_rollback(&mut state, id, Zone::Battlefield, &mut events); + + assert_eq!(state.objects[&id].zone, Zone::Battlefield); + assert!( + matches!( + state.layers_dirty, + crate::types::game_state::LayersDirty::Full + ), + "restore_after_rollback targeting the battlefield must \ + unconditionally force a full re-evaluation, got {:?}", + state.layers_dirty + ); + } } diff --git a/crates/phase-ai/Cargo.toml b/crates/phase-ai/Cargo.toml index d4ceea8184..e044d25c51 100644 --- a/crates/phase-ai/Cargo.toml +++ b/crates/phase-ai/Cargo.toml @@ -15,6 +15,16 @@ tracing-subscriber = { version = "0.3", features = ["env-filter"] } web-time = "1" rayon = { version = "1", optional = true } +# Native-binary throughput lever (pod-lab loop-3 Q5). Target-gated, not a +# plain [dependencies] entry: `engine-wasm`/`draft-wasm` both depend on this +# crate's lib for their wasm32 builds, which are explicitly size-tuned +# (`opt-level = 'z'`, the #6313 25 MiB pages-deploy guard) -- an ungated +# entry would pull mimalloc's C sources into those builds too. `[[bin]]` +# targets are never part of that wasm-bindgen graph, so no further per-bin +# cfg is needed once the dependency itself is scoped here. +[target.'cfg(not(target_arch = "wasm32"))'.dependencies] +mimalloc = "0.1" + [features] tune = ["rayon"] # Scenario-backed benchmark binaries are opt-in: build them with diff --git a/crates/phase-ai/baselines/perf-baseline.json b/crates/phase-ai/baselines/perf-baseline.json index 2af1803926..7867321727 100644 --- a/crates/phase-ai/baselines/perf-baseline.json +++ b/crates/phase-ai/baselines/perf-baseline.json @@ -1,7 +1,7 @@ { - "schema_version": 3, - "git_sha": "a496f13efa1d", - "card_data_hash": "976e3bffb3fdb726460f4beb902125c329e82428", + "schema_version": 4, + "git_sha": "64b65e58e249", + "card_data_hash": "e2db8a6d4711e34097b307c454032b29fbce8d4f", "base_seed": 2654435769, "action_cap": 3000, "sample_count": 5, @@ -11,34 +11,35 @@ "enchantress-mirror" ], "counters": { - "attackable_player_sweeps": 1476, - "auto_tap_source_cache_builds": 1426, + "attackable_player_sweeps": 830, + "auto_tap_source_cache_builds": 31, "cached_auto_tap_source_rejects": 0, - "cached_auto_tap_source_reuses": 1316, + "cached_auto_tap_source_reuses": 0, "combat_shadow_block_scans": 0, - "crew_eligibility_scans": 10655, + "crew_eligibility_scans": 7337, "granted_ability_provider_scans": 0, - "layers_escalated": 35, - "layers_full_eval": 5147, - "layers_incremental": 307, - "legal_actions_spell_cost_sweeps": 1426, - "legend_rule_mode_gate_scans": 15588, - "mana_aura_trigger_scans": 22942, - "mana_display_sweeps": 385, - "mana_display_swept_objects": 5128, - "priority_cast_probe_builds": 1426, + "layers_escalated": 93, + "layers_full_eval": 3495, + "layers_incremental": 491, + "legal_actions_spell_cost_sweeps": 31, + "legend_rule_mode_gate_scans": 10274, + "mana_aura_trigger_scans": 14286, + "mana_display_sweeps": 270, + "mana_display_swept_objects": 2718, + "priority_cast_probe_builds": 31, "restriction_static_exact_scans": 0, - "restriction_static_mode_gate_scans": 48012, - "sba_battlefield_snapshot_builds": 15322, - "sba_empty_battlefield_short_circuits": 40, + "restriction_static_mode_gate_scans": 46421, + "sba_battlefield_snapshot_builds": 10205, + "sba_empty_battlefield_short_circuits": 57, + "spell_keyword_grant_scans": 0, "stack_batch_candidates": 0, "stack_batch_observer_refusals": 0, "stack_batch_plans": 0, "stack_batched_entries": 0, "stack_inert_noop_batches": 0, "stack_inert_noop_entries": 0, - "state_clone_for_legality": 13683, - "static_full_scans": 0 + "state_clone_for_legality": 6489, + "static_full_scans": 15 }, - "wall_clock_ms": 12131 + "wall_clock_ms": 196164 } \ No newline at end of file diff --git a/crates/phase-ai/src/bin/ai_bench_state.rs b/crates/phase-ai/src/bin/ai_bench_state.rs index ac884713b7..39a1744432 100644 --- a/crates/phase-ai/src/bin/ai_bench_state.rs +++ b/crates/phase-ai/src/bin/ai_bench_state.rs @@ -2,6 +2,12 @@ //! //! Usage: `cargo run --release --bin ai-bench-state -- [--difficulty medium] [--iters N] [--assert-under-ms N]` +// pod-lab loop-3 Q5: native-binary throughput lever, gated in Cargo.toml so +// wasm32 builds of this crate's lib (pulled in by engine-wasm/draft-wasm) +// never see it. +#[global_allocator] +static GLOBAL: mimalloc::MiMalloc = mimalloc::MiMalloc; + use std::fs; use std::time::Instant; diff --git a/crates/phase-ai/src/bin/ai_commander.rs b/crates/phase-ai/src/bin/ai_commander.rs index 96f438fe75..87aebca92b 100644 --- a/crates/phase-ai/src/bin/ai_commander.rs +++ b/crates/phase-ai/src/bin/ai_commander.rs @@ -5,10 +5,16 @@ //! AI until the game ends (or an action budget is hit). Reports per-turn //! life totals and the final outcome. //! -//! Usage: -//! cargo run --release --bin ai-commander -- client/public -//! cargo run --release --bin ai-commander -- client/public --seed 7 --difficulty Easy -//! cargo run --release --bin ai-commander -- client/public --difficulty Easy \ +//! Usage (pod-lab loop-3 Q5): `--profile server-release`, not plain +//! `--release` -- `[profile.release]` (workspace `Cargo.toml`) sets `panic = +//! 'abort'` (it exists to keep the WASM build small), which silently defeats +//! `run_batch_isolated`'s `catch_unwind`-based per-game panic isolation +//! below: under `abort`, one game's panic takes the whole batch process down +//! instead of being caught and reported. `server-release` inherits `release` +//! but overrides `panic = 'unwind'` for exactly this reason. +//! cargo run --profile server-release --bin ai-commander -- client/public +//! cargo run --profile server-release --bin ai-commander -- client/public --seed 7 --difficulty Easy +//! cargo run --profile server-release --bin ai-commander -- client/public --difficulty Easy \ //! --difficulty-p2 VeryHard --action-cap 50000 //! //! Batch mode (pod-lab simulation-acceleration plan, Tier 1 item 1): play many @@ -20,8 +26,15 @@ //! panic-isolated (`run_batch_isolated`) and its result is flushed to stdout //! immediately, so a batch survives an individual game panicking or the whole //! process being killed mid-batch (pod-lab enforces an external wall-clock -//! timeout on the process). -//! cargo run --release --bin ai-commander -- client/public --games-file games.txt +//! timeout on the process) -- but only under a `panic = 'unwind'` profile; +//! see the note above. +//! cargo run --profile server-release --bin ai-commander -- client/public --games-file games.txt + +// pod-lab loop-3 Q5: native-binary throughput lever, gated in Cargo.toml so +// wasm32 builds of this crate's lib (pulled in by engine-wasm/draft-wasm) +// never see it. +#[global_allocator] +static GLOBAL: mimalloc::MiMalloc = mimalloc::MiMalloc; use std::any::Any; use std::collections::{HashMap, HashSet}; @@ -34,6 +47,7 @@ use engine::database::CardDatabase; use engine::game::deck_loading::{ load_deck_into_state, resolve_deck_list, DeckList, DeckPayload, PlayerDeckList, }; +use engine::types::events::GameEvent; use engine::types::format::FormatConfig; use engine::types::game_state::{GameState, WaitingFor}; use engine::types::player::PlayerId; @@ -59,13 +73,91 @@ const GAME_THREAD_STACK_SIZE: usize = 32 << 20; fn main() { let args: Vec = std::env::args().collect(); + // Harness escape hatch (pod-lab): forces the run into measurement mode + // even on the single-game route. Read exactly once, here, and threaded + // through as a plain `bool` — this repo's "no `set_var`" rule. + // `parse_cli` decides what to do with it. + // Contract: presence-based, matching PHASE_DUMP_*'s convention -- var + // set to any value (including "") counts as true, not `== "1"`. Presence + // only, so `var_os` (no Unicode validation needed for a bool check). + let measurement_env = std::env::var_os("PHASE_AI_MEASUREMENT").is_some(); + + let cli = match parse_cli(&args, measurement_env) { + Ok(cli) => cli, + Err(e) => { + eprintln!("{e}"); + std::process::exit(1); + } + }; - let cards_path = args - .iter() - .skip(1) - .find(|a| !a.starts_with("--")) - .cloned() - .unwrap_or_else(|| "client/public".to_string()); + // Argument parsing and validation above needs no special stack; the actual + // game-driving work (both the single-game path and `--games-file` batch + // mode) does — see `GAME_THREAD_STACK_SIZE`. Spawning unconditionally + // (rather than only under `--games-file`) fixes the pre-existing + // single-game overflow too, not just the batch case. + let handle = std::thread::Builder::new() + .name("ai-commander-driver".to_string()) + .stack_size(GAME_THREAD_STACK_SIZE) + .spawn(move || run(cli)) + .expect("failed to spawn game-driving thread"); + + // A non-batch (`games_file.is_none()`) game that panics is NOT caught (see + // `run`'s doc) — it unwinds only the spawned thread, which by itself would + // leave the process silently exiting 0. Detect that here and exit 101, the + // same code an uncaught panic on the plain main thread would have produced + // before this thread split existed, so a caller keying off exit status sees + // an unmistakable failure either way. + let exit_code = handle.join().unwrap_or(101); + std::process::exit(exit_code); +} + +/// Bundles the parsed CLI arguments `run` needs. A plain struct (not more +/// positional params on `run`) so the seam between "parse args" (plain main +/// thread) and "drive games" (large-stack spawned thread) stays one value to +/// move across the `thread::spawn` boundary, not eight. +struct CliArgs { + cards_path: String, + feed: String, + seed: u64, + difficulty: AiDifficulty, + seat_difficulty: [Option; 4], + action_cap: usize, + games_file: Option, + batch_games: Option>, + watch_cards: HashSet, + run_context: RunContext, +} + +/// Which execution mode every seat's `AiConfig` should run under for a given +/// process (see `build_seat_config`). `Measurement` disables the wall-clock +/// search deadline so every decision is a pure function of the game's inputs +/// (see `build_seat_config`'s doc for why that matters); `Interactive` leaves +/// the deadline in place. +/// +/// Route table (`parse_cli`): `--games-file` (batch) always resolves to +/// `Measurement` (the cross-game wall-clock-deadline leak fix, see +/// `build_seat_config`'s doc, applies regardless of the env override). A +/// solo invocation resolves to `Interactive` by default, unless the +/// `PHASE_AI_MEASUREMENT` harness escape hatch forces it to `Measurement`. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +enum RunContext { + Interactive, + Measurement, +} + +/// Pure argument parser: consumes the raw process args plus the one +/// env-derived flag `main` reads once (`measurement_env`, from +/// `PHASE_AI_MEASUREMENT` -- presence-based, matching `PHASE_DUMP_*` +/// convention, not `== "1"`) and returns either a fully resolved `CliArgs` +/// or an error message ready for the caller to print verbatim before exiting +/// with status 1. Contains the entire pre-`run` pre-pass (positional +/// `cards_path` resolution, the flag loop, and `--games-file` validation) +/// that used to live directly in `main`, so `main` itself does nothing but +/// read the env var, call this, and report success/failure. Never calls +/// `std::process::exit` itself -- every error path here becomes an `Err` +/// instead, exactly mirroring the direct-exit message each replaces. +fn parse_cli(args: &[String], measurement_env: bool) -> Result { + let mut cards_path: Option = None; let mut seed: u64 = 42; let mut difficulty = AiDifficulty::Easy; @@ -75,6 +167,10 @@ fn main() { let mut action_cap: usize = DEFAULT_ACTION_CAP; let mut feed: String = "feeds/mtggoldfish-commander.json".to_string(); let mut games_file: Option = None; + // pod-lab swap-liveness telemetry (loop-3 Q3(b)): empty means the + // per-event scan in `play_one_game` is skipped entirely, not merely a + // no-op HashSet lookup — a run that doesn't pass this flag pays nothing. + let mut watch_cards: HashSet = HashSet::new(); let mut args_iter = args.iter().skip(1).peekable(); while let Some(arg) = args_iter.next() { match arg.as_str() { @@ -102,9 +198,22 @@ fn main() { } "--games-file" => match args_iter.next() { Some(v) => games_file = Some(v.clone()), + None => return Err("error: --games-file requires a path".to_string()), + }, + "--watch-cards" => match args_iter.next() { + Some(v) => { + watch_cards = v + .split(',') + .map(str::trim) + .filter(|s| !s.is_empty()) + .map(str::to_string) + .collect(); + } None => { - eprintln!("error: --games-file requires a path"); - std::process::exit(1); + return Err( + "error: --watch-cards requires a comma-separated card name list" + .to_string(), + ); } }, other => { @@ -118,41 +227,54 @@ fn main() { let value = args_iter.next().map(String::as_str); match parse_seat_override(suffix, value) { Ok((idx, label)) => seat_difficulty[idx] = Some(parse_difficulty(label)), - Err(e) => { - eprintln!("{e}"); - std::process::exit(1); - } + Err(e) => return Err(e), + } + } else if !other.starts_with("--") { + // F4: first non-`--`-prefixed token is the positional + // `cards_path`. This arm only sees tokens that were NOT + // consumed as a value by one of the flag arms above, so a + // preceding `--flag value` pair's value can never land + // here mistaken for the positional path. A bare unknown + // `--flag` (still starts with `--`) falls through this + // `else if` and is silently ignored without consuming the + // next token, so it can't eat the real positional either. + if cards_path.is_none() { + cards_path = Some(other.to_string()); } } } } } + let cards_path = cards_path.unwrap_or_else(|| "client/public".to_string()); + // `--games-file` batch entries are validated up front — same hard-fail-at- // startup discipline as `parse_difficulty`/`parse_action_cap`/ // `parse_seat_override` above: a garbled batch file should fail before the // ~1.8s database load, not silently skip or misinterpret one line mid-batch. - let batch_games: Option> = - games_file - .as_deref() - .map(|path| match parse_games_file(path) { - Ok(games) if games.is_empty() => { - eprintln!("error: --games-file {path:?} contains no games"); - std::process::exit(1); - } - Ok(games) => games, - Err(e) => { - eprintln!("error: {e}"); - std::process::exit(1); - } - }); + let batch_games: Option> = match games_file.as_deref() { + None => None, + Some(path) => match parse_games_file(path) { + Ok(games) if games.is_empty() => { + return Err(format!("error: --games-file {path:?} contains no games")); + } + Ok(games) => Some(games), + Err(e) => return Err(format!("error: {e}")), + }, + }; - // Argument parsing and validation above needs no special stack; the actual - // game-driving work (both the single-game path and `--games-file` batch - // mode) does — see `GAME_THREAD_STACK_SIZE`. Spawning unconditionally - // (rather than only under `--games-file`) fixes the pre-existing - // single-game overflow too, not just the batch case. - let cli = CliArgs { + // F3 route table: `--games-file` (batch) always runs every seat in + // Measurement mode (cross-game wall-clock-deadline leak fix, see + // `build_seat_config`'s doc). A solo invocation defaults to Interactive + // (real wall-clock deadline preserved) unless the `PHASE_AI_MEASUREMENT` + // harness escape hatch forces it to Measurement too. + let run_context = if batch_games.is_some() || measurement_env { + RunContext::Measurement + } else { + RunContext::Interactive + }; + + Ok(CliArgs { cards_path, feed, seed, @@ -161,36 +283,9 @@ fn main() { action_cap, games_file, batch_games, - }; - let handle = std::thread::Builder::new() - .name("ai-commander-driver".to_string()) - .stack_size(GAME_THREAD_STACK_SIZE) - .spawn(move || run(cli)) - .expect("failed to spawn game-driving thread"); - - // A non-batch (`games_file.is_none()`) game that panics is NOT caught (see - // `run`'s doc) — it unwinds only the spawned thread, which by itself would - // leave the process silently exiting 0. Detect that here and exit 101, the - // same code an uncaught panic on the plain main thread would have produced - // before this thread split existed, so a caller keying off exit status sees - // an unmistakable failure either way. - let exit_code = handle.join().unwrap_or(101); - std::process::exit(exit_code); -} - -/// Bundles the parsed CLI arguments `run` needs. A plain struct (not more -/// positional params on `run`) so the seam between "parse args" (plain main -/// thread) and "drive games" (large-stack spawned thread) stays one value to -/// move across the `thread::spawn` boundary, not eight. -struct CliArgs { - cards_path: String, - feed: String, - seed: u64, - difficulty: AiDifficulty, - seat_difficulty: [Option; 4], - action_cap: usize, - games_file: Option, - batch_games: Option>, + watch_cards, + run_context, + }) } /// Shared immutable inputs for one or more AI-commander game runs. @@ -202,6 +297,8 @@ struct GameRunContext<'a> { action_cap: usize, dump_log_path: Option<&'a str>, dump_actions_path: Option<&'a str>, + watch_cards: &'a HashSet, + run_context: RunContext, } /// Everything that isn't argument parsing: loads the card database and feed @@ -220,6 +317,8 @@ fn run(cli: CliArgs) -> i32 { action_cap, games_file, batch_games, + watch_cards, + run_context, } = cli; let export_path = PathBuf::from(&cards_path).join("card-data.json"); @@ -259,6 +358,16 @@ fn run(cli: CliArgs) -> i32 { None => println!("Seed: {seed} Difficulty: {difficulty:?}"), } println!(); + // D1 must-pass gate for B3: pod-lab's harness greps for this exact + // lowercase literal (not `{run_context:?}`) to distinguish a solo + // Interactive-route game from a Measurement-route one. Appended after + // the blank line that closes the pre-existing pinned preamble (see + // `single_game_stdout_is_deterministic_and_preamble_is_pinned`'s + // `starts_with` assertion), so it never conflicts with that pin. + match run_context { + RunContext::Interactive => println!("ExecutionMode: interactive"), + RunContext::Measurement => println!("ExecutionMode: measurement"), + } let mut deck_lists: Vec = Vec::new(); // Commander names are populated in PlayerDeckList.commander and resolved @@ -355,6 +464,8 @@ fn run(cli: CliArgs) -> i32 { action_cap, dump_log_path: dump_log_path.as_deref(), dump_actions_path: dump_actions_path.as_deref(), + watch_cards: &watch_cards, + run_context, }; match batch_games { @@ -408,6 +519,8 @@ fn play_one_game(context: &GameRunContext<'_>, seed: u64, difficulty: AiDifficul action_cap, dump_log_path, dump_actions_path, + watch_cards, + run_context, } = *context; let mut state = build_game_state(db, payload, seed); @@ -428,7 +541,7 @@ fn play_one_game(context: &GameRunContext<'_>, seed: u64, difficulty: AiDifficul println!(" P{i} difficulty={seat_diff:?}"); ai_configs.insert( PlayerId(i as u8), - create_config_for_players(seat_diff, Platform::Native, 4), + build_seat_config(seat_diff, seed, run_context), ); } println!(); @@ -436,6 +549,13 @@ fn play_one_game(context: &GameRunContext<'_>, seed: u64, difficulty: AiDifficul let start = Instant::now(); let mut game_log: Vec = Vec::new(); let mut actions_log: Vec = Vec::new(); + // pod-lab swap-liveness telemetry (loop-3 Q3(b)): which of `watch_cards`' + // names were ever drawn to hand or cast this game. Names, not `CardId`s — + // `CardId` is assigned per physical-card-object at deck load + // (`deck_loading.rs`), not a stable per-name identity, and every + // `GameObject` already carries its own resolved `name`, so matching on + // name needs no extra database lookup. + let mut cards_seen: HashSet = HashSet::new(); let mut last_turn_reported: u32 = 0; let mut ai_rng = StdRng::seed_from_u64(seed); let ai_session = phase_ai::session::AiSession::arc_from_game(&state); @@ -470,6 +590,10 @@ fn play_one_game(context: &GameRunContext<'_>, seed: u64, difficulty: AiDifficul } } + if !watch_cards.is_empty() { + record_watched_cards(results, watch_cards, &mut cards_seen); + } + if state.turn_number != last_turn_reported { last_turn_reported = state.turn_number; let snapshot: Vec = state @@ -532,6 +656,19 @@ fn play_one_game(context: &GameRunContext<'_>, seed: u64, difficulty: AiDifficul println!("Elapsed: {:.1}s", elapsed.as_secs_f64()); println!("Total actions: {total_actions}"); println!("Turns played: {}", state.turn_number); + // pod-lab swap-liveness telemetry (loop-3 Q3(b)): one line, only when + // `--watch-cards` was given, so a harness that doesn't ask for this pays + // nothing and every existing consumer's line-by-line parse is untouched. + // `PODLAB-TELEM ` is a prefix no other line in this binary's stdout uses, + // and this line does not begin with "Turn ", match `^--- GAME`, or + // contain "Winner: " / "Difficulty: " / "ABORT: hit " / "did NOT reach + // GameOver" (pod-lab's `runner.py`/`mechanisms.py` scan for exactly those + // literals). Cards are sorted for a deterministic, diff-friendly line. + if !watch_cards.is_empty() { + let mut seen: Vec<&str> = cards_seen.iter().map(String::as_str).collect(); + seen.sort_unstable(); + println!("PODLAB-TELEM {}", serde_json::json!({ "cards_seen": seen })); + } println!(); let outcome = classify_run_outcome(aborted, &state.waiting_for); @@ -636,6 +773,38 @@ fn play_one_game(context: &GameRunContext<'_>, seed: u64, difficulty: AiDifficul outcome } +/// pod-lab swap-liveness telemetry (loop-3 Q3(b)): scans one driver-loop +/// batch's `AiActionResult`s for `SpellCast`/`CardDrawn` events naming an +/// object whose CURRENT name (resolved via that action's own `r.state`, not +/// a stale/outer snapshot) is in `watch`, inserting the resolved name into +/// `seen`. Matches on name, not `CardId`: `CardId` is assigned per +/// physical-card-object at deck load (`deck_loading.rs`), not a stable +/// per-name identity, while every `GameObject` already carries its own +/// resolved `name` — no extra database lookup needed. Pure and unit-tested +/// separately from `play_one_game`'s full game-driving loop; the caller +/// skips calling this entirely when `watch` is empty, so a run that doesn't +/// pass `--watch-cards` pays nothing beyond the `is_empty()` check. +fn record_watched_cards( + results: &[phase_ai::auto_play::AiActionResult], + watch: &HashSet, + seen: &mut HashSet, +) { + for r in results { + for event in &r.events { + let object_id = match event { + GameEvent::SpellCast { object_id, .. } => *object_id, + GameEvent::CardDrawn { object_id, .. } => *object_id, + _ => continue, + }; + if let Some(obj) = r.state.objects.get(&object_id) { + if watch.contains(&obj.name) { + seen.insert(obj.name.clone()); + } + } + } + } +} + /// Runs `play` once per entry in `games`, isolating panics per game (Tier 1 /// item 2) so one bad game (this binary has real `unwrap()`/`expect()` calls, /// including deep in the AI search path) can't take down the rest of the @@ -874,11 +1043,56 @@ fn build_game_state(db: &CardDatabase, payload: &DeckPayload, seed: u64) -> Game state } +/// Builds one seat's `AiConfig` for a single game. Single authority for this +/// bin's per-seat AI configuration — `play_one_game` calls it once per seat and +/// the regression test below asserts its one load-bearing invariant. +/// +/// EVERY seat runs in MEASUREMENT mode (`AiConfig::into_measurement`), which +/// disables the wall-clock search deadline (`AI_SEARCH_TIME_BUDGET_MS`, default +/// 1500ms) so search is bounded SOLELY by `max_nodes`/`max_depth`. This is +/// required for reproducibility, not a benchmarking nicety: an interactive +/// (wall-clock-bounded) search truncates to a degraded best-so-far result the +/// moment `Deadline::expired()` fires, and whether it fires on a given decision +/// depends on how fast the process happens to be running at that instant — NOT +/// on `(seed, difficulty, feed)`. A `--games-file` batch process is measurably +/// slower on its Nth game (warmer allocator, more resident state) than a fresh +/// single-game process, so the SAME game played 3rd in a batch could expire the +/// deadline on a mid-game decision that the solo run completed in full, pick a +/// different move, and diverge — the exact cross-game non-determinism the +/// pod-lab equivalence gate caught (a game that wins solo stalling at turn 60 in +/// batch). Measurement mode makes every decision a pure function of the game's +/// inputs, so batch game N is bit-identical to the same game run solo. This +/// mirrors the established `duel_suite::run` batch harness, which builds its +/// config with `.into_measurement(seed)` for the same "eliminate wall-clock +/// flake" reason. `seed` is the per-game seed, itself fully determined by the +/// game's inputs; the value inside `ExecutionMode::Measurement { seed }` only +/// tags the mode — the search's determinization entropy is derived from game +/// state (`search.rs`), not from this seed. +/// +/// `run_context` (see its doc) is a required parameter, not an implicit +/// always-on: `RunContext::Measurement` calls `.into_measurement(seed)` as +/// before; `RunContext::Interactive` leaves the wall-clock deadline in +/// place. `parse_cli` constructs `RunContext::Interactive` for a solo +/// invocation with no measurement override, and `RunContext::Measurement` +/// for `--games-file` batches or the `PHASE_AI_MEASUREMENT` override. +fn build_seat_config(difficulty: AiDifficulty, seed: u64, run_context: RunContext) -> AiConfig { + let config = create_config_for_players(difficulty, Platform::Native, 4); + match run_context { + RunContext::Measurement => config.into_measurement(seed), + RunContext::Interactive => config, + } +} + #[cfg(test)] mod tests { use super::*; use engine::ai_support::candidate_actions; + use engine::game::game_object::GameObject; use engine::types::ability::ChoiceType; + use engine::types::actions::GameAction; + use engine::types::identifiers::{CardId, ObjectId}; + use engine::types::zones::Zone; + use phase_ai::auto_play::AiActionResult; use std::sync::{Arc, Mutex}; struct TempFileGuard(PathBuf); @@ -990,6 +1204,166 @@ mod tests { ); } + /// Regression for the cross-game state-leakage defect (pod-lab equivalence + /// gate; PR phase-rs/phase#6252): a game that won solo stalled at turn 60 + /// when played 3rd in a `--games-file` batch. Root cause was NOT a leaked + /// counter/cache but the interactive wall-clock search deadline + /// (`AI_SEARCH_TIME_BUDGET_MS`): a slower Nth-in-batch process expired it on + /// a mid-game decision the fresh solo process completed, diverging the game. + /// `build_seat_config` fixes this by running every seat in MEASUREMENT mode, + /// which disables the wall-clock deadline (search bounded solely by + /// node/depth — see `search.rs` / `planner::PlannerServices::with_deadline`, + /// both gated on `execution_mode.is_measurement()`), making each decision a + /// pure function of the game's inputs. This asserts the invariant + /// deterministically (no card-data / no real game needed): against the + /// unfixed code (`ExecutionMode::Interactive`) it fails, catching any future + /// regression that drops measurement mode and reintroduces wall-clock flake. + #[test] + fn seat_config_runs_in_measurement_mode_for_batch_reproducibility() { + for difficulty in [ + AiDifficulty::Easy, + AiDifficulty::Medium, + AiDifficulty::Hard, + AiDifficulty::VeryHard, + ] { + let config = build_seat_config(difficulty, 95_000_004, RunContext::Measurement); + assert!( + config.execution_mode.is_measurement(), + "{difficulty:?} seat must run in measurement mode so a batched \ + game is bit-identical to the same game run solo; interactive \ + mode makes search wall-clock-dependent and non-reproducible \ + under batch load" + ); + } + } + + /// Companion to the assertion above: `Interactive` must leave the + /// wall-clock deadline in place (i.e. NOT call `into_measurement`). + /// `parse_cli` constructs `RunContext::Interactive` for the default solo + /// route; pinning both directions here means routing can't silently + /// collapse `Interactive` into `Measurement` (or vice versa) without a + /// red test. + #[test] + fn seat_config_leaves_interactive_mode_wall_clock_bounded() { + for difficulty in [ + AiDifficulty::Easy, + AiDifficulty::Medium, + AiDifficulty::Hard, + AiDifficulty::VeryHard, + ] { + let config = build_seat_config(difficulty, 95_000_004, RunContext::Interactive); + assert!( + !config.execution_mode.is_measurement(), + "{difficulty:?} seat under RunContext::Interactive must NOT run in \ + measurement mode; collapsing it into Measurement would defeat the \ + whole point of having the two variants" + ); + } + } + + /// Building-block test for `record_watched_cards` (loop-3 Q3(b)): proves + /// the matcher (a) recognizes both `SpellCast` and `CardDrawn` events, + /// (b) resolves the watched name via the object's *current* `r.state` + /// rather than any fixed name table, and (c) is selective — an event + /// naming an object outside `watch`, and an event of an unrelated + /// variant entirely, must both be no-ops rather than getting recorded. + #[test] + fn record_watched_cards_matches_spell_cast_and_card_drawn_by_name() { + let mut state = GameState::new(FormatConfig::commander(), 4, 1); + let cast_obj = ObjectId(100); + let drawn_obj = ObjectId(101); + let unwatched_obj = ObjectId(102); + state.objects.insert( + cast_obj, + GameObject::new( + cast_obj, + CardId(100), + PlayerId(0), + "Lightning Bolt".to_string(), + Zone::Stack, + ), + ); + state.objects.insert( + drawn_obj, + GameObject::new( + drawn_obj, + CardId(101), + PlayerId(0), + "Sol Ring".to_string(), + Zone::Hand, + ), + ); + state.objects.insert( + unwatched_obj, + GameObject::new( + unwatched_obj, + CardId(102), + PlayerId(0), + "Forest".to_string(), + Zone::Battlefield, + ), + ); + + let watch: HashSet = ["Lightning Bolt".to_string(), "Sol Ring".to_string()] + .into_iter() + .collect(); + let mut seen: HashSet = HashSet::new(); + + let results = vec![ + AiActionResult { + action: GameAction::PassPriority, + state: state.clone(), + events: vec![GameEvent::SpellCast { + card_id: CardId(100), + controller: PlayerId(0), + object_id: cast_obj, + }], + log_entries: Vec::new(), + }, + AiActionResult { + action: GameAction::PassPriority, + state: state.clone(), + events: vec![ + GameEvent::CardDrawn { + player_id: PlayerId(0), + object_id: drawn_obj, + nth_in_turn: 1, + nth_in_step: 1, + }, + // Drawn but not watched, and an unrelated event variant — + // both must be ignored, not just the watched ones matched. + GameEvent::CardDrawn { + player_id: PlayerId(0), + object_id: unwatched_obj, + nth_in_turn: 2, + nth_in_step: 2, + }, + GameEvent::PriorityPassed { + player_id: PlayerId(0), + }, + ], + log_entries: Vec::new(), + }, + ]; + + record_watched_cards(&results, &watch, &mut seen); + + assert_eq!( + seen, + ["Lightning Bolt".to_string(), "Sol Ring".to_string()] + .into_iter() + .collect::>() + ); + } + + #[test] + fn record_watched_cards_is_a_noop_on_empty_results() { + let watch: HashSet = ["Lightning Bolt".to_string()].into_iter().collect(); + let mut seen: HashSet = HashSet::new(); + record_watched_cards(&[], &watch, &mut seen); + assert!(seen.is_empty()); + } + #[test] fn parse_action_cap_accepts_positive_integer() { assert_eq!(parse_action_cap_checked("50000"), Ok(50000)); @@ -1147,6 +1521,156 @@ mod tests { assert_eq!(*seen.lock().unwrap(), games); } + /// D1 route table: a bare solo invocation with no `--games-file` and no + /// measurement env override resolves to `RunContext::Interactive`. + #[test] + fn parse_cli_solo_invocation_routes_to_interactive() { + let args = vec!["ai-commander".to_string(), "client/public".to_string()]; + let cli = parse_cli(&args, false).expect("solo invocation must parse"); + assert_eq!( + cli.run_context, + RunContext::Interactive, + "a solo (non-games-file) invocation with no measurement env override must route to RunContext::Interactive" + ); + } + + /// D1 route table: `--games-file` invocations must resolve to + /// `RunContext::Measurement`. + #[test] + fn parse_cli_games_file_invocation_routes_to_measurement() { + let path = temp_games_file_path("parse_cli_route_games_file"); + let _guard = TempFileGuard(path.clone()); + std::fs::write( + &path, + "1009,Easy +", + ) + .unwrap(); + let args = vec![ + "ai-commander".to_string(), + "--games-file".to_string(), + path.to_str().unwrap().to_string(), + ]; + let cli = parse_cli(&args, false).expect("games-file invocation must parse"); + assert_eq!(cli.run_context, RunContext::Measurement); + } + + /// D1 route table: the `PHASE_AI_MEASUREMENT` harness escape hatch forces + /// a solo (non-games-file) invocation to `RunContext::Measurement`. + #[test] + fn parse_cli_measurement_env_forces_measurement_on_solo() { + let args = vec!["ai-commander".to_string(), "client/public".to_string()]; + let cli = parse_cli(&args, true).expect("solo invocation must parse"); + assert_eq!(cli.run_context, RunContext::Measurement); + } + + /// F4: the positional `cards_path` scan must not be poisoned by the + /// *value* of any preceding `--flag value` pair (e.g. `--seed 42 + /// some/path` must resolve `cards_path` to `"some/path"`, not `"42"`). + /// Table-driven over every flag that takes a value — including + /// `--games-file` (whose value is a real temp file, since `parse_cli` + /// validates the batch up front), `--watch-cards`, and the seat-override + /// form `--difficulty-p`. + #[test] + fn parse_cli_positional_cards_path_survives_preceding_flag_value() { + let games_path = temp_games_file_path("positional_survival_games"); + let _guard = TempFileGuard(games_path.clone()); + std::fs::write(&games_path, "1009,Easy\n").unwrap(); + let games_path = games_path.to_str().unwrap(); + + let rows: &[&[&str]] = &[ + &["--seed", "42", "some/path"], + &["--feed", "x.json", "some/path"], + &["--action-cap", "5", "some/path"], + &["--difficulty", "Easy", "some/path"], + &["--games-file", games_path, "some/path"], + &["--watch-cards", "Sol Ring,Arcane Signet", "some/path"], + &["--difficulty-p0", "Easy", "some/path"], + ]; + for row in rows { + let mut args = vec!["ai-commander".to_string()]; + args.extend(row.iter().map(|s| s.to_string())); + let cli = parse_cli(&args, false).expect("row must parse"); + assert_eq!( + cli.cards_path, "some/path", + "row {row:?}: positional cards_path must survive a preceding --flag value pair (F4)" + ); + } + } + + /// F4 value-consumption pin: the `--watch-cards` value must reach the + /// consuming set — split on commas, trimmed — and must not disturb the + /// positional `cards_path`. Guards the arm at the consuming loop directly + /// (a row in the positional table alone can't catch the value being + /// dropped or mis-split). + #[test] + fn parse_cli_watch_cards_value_reaches_the_consuming_set() { + let args: Vec = [ + "ai-commander", + "--watch-cards", + "Sol Ring, Arcane Signet", + "some/path", + ] + .iter() + .map(|s| s.to_string()) + .collect(); + let cli = parse_cli(&args, false).expect("watch-cards invocation must parse"); + let expected: HashSet = ["Sol Ring", "Arcane Signet"] + .iter() + .map(|s| s.to_string()) + .collect(); + assert_eq!(cli.watch_cards, expected); + assert_eq!(cli.cards_path, "some/path"); + } + + /// F4 value-consumption pin: the `--games-file` value must reach + /// `CliArgs.games_file` and its parsed batch (validated up front), and + /// must not disturb the positional `cards_path`. + #[test] + fn parse_cli_games_file_value_reaches_the_consuming_option() { + let path = temp_games_file_path("games_file_value_consumption"); + let _guard = TempFileGuard(path.clone()); + std::fs::write(&path, "1009,Easy\n").unwrap(); + let path = path.to_str().unwrap(); + + let args: Vec = ["ai-commander", "--games-file", path, "some/path"] + .iter() + .map(|s| s.to_string()) + .collect(); + let cli = parse_cli(&args, false).expect("games-file invocation must parse"); + assert_eq!(cli.games_file.as_deref(), Some(path)); + assert_eq!( + cli.batch_games, + Some(vec![(1009, AiDifficulty::Easy)]), + "the games-file value must reach the up-front-validated batch" + ); + assert_eq!(cli.cards_path, "some/path"); + } + + /// F4 companion (green today, and a real pin): an *unknown* bare flag + /// (no recognized value-consuming arm) must not eat the following + /// positional arg. Guards against a naive single-loop F4 fix that + /// assumes every `--flag` consumes the next token. + #[test] + fn parse_cli_bare_unknown_flag_does_not_consume_positional_path() { + let args = vec![ + "ai-commander".to_string(), + "--foo".to_string(), + "some/path".to_string(), + ]; + let cli = parse_cli(&args, false).expect("row must parse"); + assert_eq!(cli.cards_path, "some/path"); + } + + /// F4 companion (green today): with no positional arg at all, + /// `cards_path` must default to `client/public`. + #[test] + fn parse_cli_defaults_cards_path_to_client_public_when_no_positional() { + let args = vec!["ai-commander".to_string()]; + let cli = parse_cli(&args, false).expect("bare invocation must parse"); + assert_eq!(cli.cards_path, "client/public"); + } + #[test] fn run_batch_isolated_visits_every_game_exactly_once_when_none_panic() { let games = vec![(10u64, AiDifficulty::Easy), (20u64, AiDifficulty::VeryHard)]; diff --git a/crates/phase-ai/src/bin/ai_duel.rs b/crates/phase-ai/src/bin/ai_duel.rs index 538bb7f444..49a6409fbd 100644 --- a/crates/phase-ai/src/bin/ai_duel.rs +++ b/crates/phase-ai/src/bin/ai_duel.rs @@ -1,3 +1,9 @@ +// pod-lab loop-3 Q5: native-binary throughput lever, gated in Cargo.toml so +// wasm32 builds of this crate's lib (pulled in by engine-wasm/draft-wasm) +// never see it. +#[global_allocator] +static GLOBAL: mimalloc::MiMalloc = mimalloc::MiMalloc; + use std::collections::{HashMap, HashSet}; use std::path::PathBuf; use std::time::{Instant, SystemTime, UNIX_EPOCH}; diff --git a/crates/phase-ai/src/bin/ai_gate.rs b/crates/phase-ai/src/bin/ai_gate.rs index e312589a65..8099c203fe 100644 --- a/crates/phase-ai/src/bin/ai_gate.rs +++ b/crates/phase-ai/src/bin/ai_gate.rs @@ -1,3 +1,9 @@ +// pod-lab loop-3 Q5: native-binary throughput lever, gated in Cargo.toml so +// wasm32 builds of this crate's lib (pulled in by engine-wasm/draft-wasm) +// never see it. +#[global_allocator] +static GLOBAL: mimalloc::MiMalloc = mimalloc::MiMalloc; + use std::fs::File; use std::io::BufWriter; use std::path::{Path, PathBuf}; diff --git a/crates/phase-ai/src/bin/ai_perf_gate.rs b/crates/phase-ai/src/bin/ai_perf_gate.rs index 779c5b002c..0c3458b748 100644 --- a/crates/phase-ai/src/bin/ai_perf_gate.rs +++ b/crates/phase-ai/src/bin/ai_perf_gate.rs @@ -19,6 +19,12 @@ //! child (emit one sample), repro-report (margin gate over saved runs), and //! parent gate (spawn K children, median, compare). +// pod-lab loop-3 Q5: native-binary throughput lever, gated in Cargo.toml so +// wasm32 builds of this crate's lib (pulled in by engine-wasm/draft-wasm) +// never see it. +#[global_allocator] +static GLOBAL: mimalloc::MiMalloc = mimalloc::MiMalloc; + use std::fs::File; use std::io::BufWriter; use std::path::{Path, PathBuf}; @@ -34,6 +40,13 @@ use phase_ai::duel_suite::perf::{ const DEFAULT_BASELINE: &str = "crates/phase-ai/baselines/perf-baseline.json"; const DEFAULT_CURRENT: &str = "target/ai-perf-gate-current.json"; +/// `run_perf_suite`'s AI search recurses deeper than the platform default +/// thread stack on Windows (confirmed: every child overflows immediately +/// after game start under this crate's release profile). Same root cause and +/// fix as `ai_commander.rs`'s `GAME_THREAD_STACK_SIZE` / `duel_suite::run`'s +/// identical spawn — this binary just never got the fix applied. +const PERF_THREAD_STACK_SIZE: usize = 32 << 20; + struct Args { data_root: PathBuf, baseline: PathBuf, @@ -62,9 +75,22 @@ fn main() { // Branch 1 — child: load the DB, emit ONE single-trajectory sample to the // file, exit. Emits NOTHING on stdout (GAP 4) so the parent's stdout stays a - // clean table; diagnostics go to stderr only. + // clean table; diagnostics go to stderr only. Runs on a large-stack thread + // (see `PERF_THREAD_STACK_SIZE`) since the AI search recurses past the + // platform default; an unhandled panic there would otherwise unwind only + // the spawned thread and exit 0 silently, so a join failure is mapped to + // exit 101 (mirrors `ai_commander.rs`'s identical convention). if let Some(sample_path) = &args.emit_sample { - run_child_sample(&args.data_root, sample_path); + let data_root = args.data_root.clone(); + let sample_path = sample_path.clone(); + let handle = std::thread::Builder::new() + .name("ai-perf-gate-sample".to_string()) + .stack_size(PERF_THREAD_STACK_SIZE) + .spawn(move || run_child_sample(&data_root, &sample_path)) + .expect("failed to spawn perf-sample thread"); + if handle.join().is_err() { + std::process::exit(101); + } return; } diff --git a/crates/phase-ai/src/bin/ai_tune.rs b/crates/phase-ai/src/bin/ai_tune.rs index 388caccc2c..9072c48aff 100644 --- a/crates/phase-ai/src/bin/ai_tune.rs +++ b/crates/phase-ai/src/bin/ai_tune.rs @@ -1,3 +1,9 @@ +// pod-lab loop-3 Q5: native-binary throughput lever, gated in Cargo.toml so +// wasm32 builds of this crate's lib (pulled in by engine-wasm/draft-wasm) +// never see it. +#[global_allocator] +static GLOBAL: mimalloc::MiMalloc = mimalloc::MiMalloc; + use std::collections::{HashMap, HashSet}; use std::hash::{DefaultHasher, Hash, Hasher}; use std::panic::AssertUnwindSafe; diff --git a/crates/phase-ai/src/bin/attack_scaling_bench.rs b/crates/phase-ai/src/bin/attack_scaling_bench.rs index c9b5555bdb..5f40d41f00 100644 --- a/crates/phase-ai/src/bin/attack_scaling_bench.rs +++ b/crates/phase-ai/src/bin/attack_scaling_bench.rs @@ -21,6 +21,12 @@ //! the engine very slowly and contends with Tilt, so prefer the debug build //! above unless you specifically need realistic wall-clock numbers. +// pod-lab loop-3 Q5: native-binary throughput lever, gated in Cargo.toml so +// wasm32 builds of this crate's lib (pulled in by engine-wasm/draft-wasm) +// never see it. +#[global_allocator] +static GLOBAL: mimalloc::MiMalloc = mimalloc::MiMalloc; + use std::time::{Duration, Instant}; use engine::game::combat::{ diff --git a/crates/phase-ai/src/bin/combat_priority_bench.rs b/crates/phase-ai/src/bin/combat_priority_bench.rs index cf4d7129b0..0b5691dc47 100644 --- a/crates/phase-ai/src/bin/combat_priority_bench.rs +++ b/crates/phase-ai/src/bin/combat_priority_bench.rs @@ -15,6 +15,12 @@ //! CARGO_TARGET_DIR=/tmp/forge-dbg cargo run \ //! -p phase-ai --bin combat_priority_bench +// pod-lab loop-3 Q5: native-binary throughput lever, gated in Cargo.toml so +// wasm32 builds of this crate's lib (pulled in by engine-wasm/draft-wasm) +// never see it. +#[global_allocator] +static GLOBAL: mimalloc::MiMalloc = mimalloc::MiMalloc; + use std::time::{Duration, Instant}; use engine::ai_support; diff --git a/crates/phase-ai/src/bin/declare_attackers_bench.rs b/crates/phase-ai/src/bin/declare_attackers_bench.rs index 804b8cf0e7..bcdd74ae69 100644 --- a/crates/phase-ai/src/bin/declare_attackers_bench.rs +++ b/crates/phase-ai/src/bin/declare_attackers_bench.rs @@ -23,6 +23,12 @@ //! Counters are profile-independent; only the absolute per-call times inflate //! in a debug build. Prefer debug for fast iteration on the scaling shape. +// pod-lab loop-3 Q5: native-binary throughput lever, gated in Cargo.toml so +// wasm32 builds of this crate's lib (pulled in by engine-wasm/draft-wasm) +// never see it. +#[global_allocator] +static GLOBAL: mimalloc::MiMalloc = mimalloc::MiMalloc; + use std::io::Write; use std::time::Instant; diff --git a/crates/phase-ai/src/bin/legal_actions_bench.rs b/crates/phase-ai/src/bin/legal_actions_bench.rs index 172151be4c..7f82215c48 100644 --- a/crates/phase-ai/src/bin/legal_actions_bench.rs +++ b/crates/phase-ai/src/bin/legal_actions_bench.rs @@ -8,6 +8,12 @@ //! CARGO_TARGET_DIR=/tmp/forge-prof-target cargo run --profile profiling \ //! -p phase-ai --bin legal-actions-bench -- path/to/state.json +// pod-lab loop-3 Q5: native-binary throughput lever, gated in Cargo.toml so +// wasm32 builds of this crate's lib (pulled in by engine-wasm/draft-wasm) +// never see it. +#[global_allocator] +static GLOBAL: mimalloc::MiMalloc = mimalloc::MiMalloc; + use std::fs; use std::time::{Duration, Instant}; diff --git a/crates/phase-ai/src/bin/pass_priority_bench.rs b/crates/phase-ai/src/bin/pass_priority_bench.rs index d86d74f038..eba1ceb62a 100644 --- a/crates/phase-ai/src/bin/pass_priority_bench.rs +++ b/crates/phase-ai/src/bin/pass_priority_bench.rs @@ -16,6 +16,12 @@ //! the engine very slowly and contends with Tilt, so prefer the debug build //! above unless you specifically need realistic wall-clock numbers. +// pod-lab loop-3 Q5: native-binary throughput lever, gated in Cargo.toml so +// wasm32 builds of this crate's lib (pulled in by engine-wasm/draft-wasm) +// never see it. +#[global_allocator] +static GLOBAL: mimalloc::MiMalloc = mimalloc::MiMalloc; + use std::fs; use std::time::Instant; diff --git a/crates/phase-ai/src/bin/resolve_bench.rs b/crates/phase-ai/src/bin/resolve_bench.rs index 7ab1507553..41ff17b781 100644 --- a/crates/phase-ai/src/bin/resolve_bench.rs +++ b/crates/phase-ai/src/bin/resolve_bench.rs @@ -11,6 +11,12 @@ //! RUSTFLAGS="-C debug-assertions=on" CARGO_TARGET_DIR=/tmp/forge-prof-target-dbg \ //! cargo build --profile tool --bin resolve_bench +// pod-lab loop-3 Q5: native-binary throughput lever, gated in Cargo.toml so +// wasm32 builds of this crate's lib (pulled in by engine-wasm/draft-wasm) +// never see it. +#[global_allocator] +static GLOBAL: mimalloc::MiMalloc = mimalloc::MiMalloc; + use std::collections::BTreeMap; use std::fs; use std::time::Instant; diff --git a/crates/phase-ai/src/duel_suite/perf.rs b/crates/phase-ai/src/duel_suite/perf.rs index 56add2f345..cce8414dbf 100644 --- a/crates/phase-ai/src/duel_suite/perf.rs +++ b/crates/phase-ai/src/duel_suite/perf.rs @@ -73,7 +73,7 @@ use super::run::{drive_game, resolve_matchup}; /// the report shape or the counter field set changes (a changed field set is /// self-flagged by [`PerfCounters::from_snapshot`]'s struct destructure — the /// `Removed`/`New` classifications also warn to bump this). -pub const PERF_SCHEMA_VERSION: u32 = 3; // was 2: added SBA snapshot counters +pub const PERF_SCHEMA_VERSION: u32 = 4; // was 3: added spell_keyword_grant_scans counter /// Number of INDEPENDENT cold-process trajectory samples the gate aggregates by /// per-counter median. Independence is why each sample must be its own process diff --git a/crates/phase-ai/tests/ai_commander_batch_equivalence.rs b/crates/phase-ai/tests/ai_commander_batch_equivalence.rs new file mode 100644 index 0000000000..ef8982e5f2 --- /dev/null +++ b/crates/phase-ai/tests/ai_commander_batch_equivalence.rs @@ -0,0 +1,323 @@ +//! Subprocess-level regression tests for `ai-commander`'s `--games-file` batch +//! mode: a batch invocation's per-game output must match the single-game +//! invocation of the same seed+difficulty, byte-for-byte outside the one +//! inherently nondeterministic field (wall-clock elapsed time). These spawn the +//! real `ai-commander` binary rather than calling `run()` in-process, so what is +//! under test is the binary's actual contract with the pod-lab harness — +//! argument parsing, per-game stdout framing, flush timing, process exit — not +//! an internal function's. +//! +//! Every test here is `#[ignore]`d: it loads `client/public/card-data.json` +//! (requires `cargo run --bin card-data-export` or the setup.sh script), which +//! is not available in unit-test CI — the same convention as +//! `greasefang_bounded.rs`/`whitemane_lion_bounded.rs`. Opt in via +//! `cargo test -p phase-ai --test ai_commander_batch_equivalence -- --ignored`. + +use std::path::PathBuf; +use std::process::Command; + +/// Resolves `client/public` the same way `greasefang_bounded.rs` et al. do: a +/// `PHASE_CARDS_PATH` override, else relative to the crate's manifest dir. +fn cards_dir() -> PathBuf { + std::env::var("PHASE_CARDS_PATH") + .map(PathBuf::from) + .unwrap_or_else(|_| { + PathBuf::from(env!("CARGO_MANIFEST_DIR")) + .join("..") + .join("..") + .join("client") + .join("public") + }) +} + +/// Small action cap so these tests run quickly while still exercising +/// several turns. The exact outcome (COMPLETED/ABORT/STALL) doesn't matter +/// for equivalence — only that a given seed+feed+cap combination reaches the +/// SAME outcome deterministically, single-game or batched. +const TEST_ACTION_CAP: &str = "300"; + +/// Runs the real `ai-commander` binary with `cards_dir()` as its positional +/// arg and `args` appended, and returns captured stdout as a `String`. Exit +/// code isn't asserted here: COMPLETED (0)/ABORT (2)/STALL (3) are all +/// legitimate outcomes for a bounded-action-cap test game, and every one of +/// them still prints a full `=== RESULT ===` block — `normalized_result_block` +/// panics if that block is missing, which already catches an actual crash. +/// +/// `measurement` controls whether `PHASE_AI_MEASUREMENT=1` is set on the +/// child process (D1's harness escape hatch, matching the `PHASE_DUMP_*` env +/// convention). This is a process-level knob that `parse_cli` consults via +/// the `measurement_env` parameter, forcing a solo route to +/// `RunContext::Measurement`. +fn run_ai_commander_with_context(args: &[&str], measurement: bool) -> String { + let mut command = Command::new(env!("CARGO_BIN_EXE_ai-commander")); + command.arg(cards_dir()).args(args); + if measurement { + command.env("PHASE_AI_MEASUREMENT", "1"); + } + let output = command.output().expect("spawn ai-commander"); + String::from_utf8(output.stdout).expect("stdout is valid UTF-8") +} + +/// Thin wrapper over [`run_ai_commander_with_context`] for call sites that +/// don't care about run context -- preserves the pre-D1 behavior (always +/// measurement) for them. +fn run_ai_commander(args: &[&str]) -> String { + run_ai_commander_with_context(args, true) +} + +/// Splits `stdout` into one chunk per game. Batch mode anchors on the +/// `--- GAME ` marker: each chunk then runs from one game's own marker up to +/// (not including) the next game's marker or EOF, so it's fully +/// self-contained — critically, a game's marker + per-seat tier echo (all +/// printed BEFORE that game's own "Game started." line) stay attributed to +/// THAT game, not leaked onto the end of the previous one. Single-game mode +/// never prints a marker, so the whole output is one chunk. +fn game_blocks(stdout: &str) -> Vec<&str> { + const MARKER: &str = "--- GAME "; + if !stdout.contains(MARKER) { + return vec![stdout]; + } + let mut starts: Vec = stdout.match_indices(MARKER).map(|(i, _)| i).collect(); + starts.push(stdout.len()); + starts.windows(2).map(|w| &stdout[w[0]..w[1]]).collect() +} + +/// The `=== RESULT ===` epilogue through the end of one game's block (as +/// isolated by `game_blocks`), with the single inherently nondeterministic +/// line (`Elapsed: {:.1}s`, wall-clock) stripped so two separately-timed runs +/// can be compared for equality. +fn normalized_result_block(game_block: &str) -> String { + let start = game_block + .find("=== RESULT ===") + .expect("game block contains a RESULT epilogue"); + game_block[start..] + .lines() + .filter(|line| !line.starts_with("Elapsed:")) + .collect::>() + .join("\n") +} + +/// Higher action cap for the cross-game-leakage regression below: the +/// wall-clock-deadline divergence this guards only surfaces deep into a game +/// (the original repro diverged around turn ~50 / action ~1700), so the tiny +/// `TEST_ACTION_CAP` used by the plumbing tests above would never reach the +/// decisions where an interactive search's budget could straddle. Still bounded +/// so an `#[ignore]` opt-in run stays minutes, not tens of minutes. +const LEAK_REGRESSION_ACTION_CAP: &str = "2000"; + +/// Regression for the cross-game state-leakage defect (pod-lab equivalence +/// gate; PR phase-rs/phase#6252): seed 95000004 won cleanly for P1 run solo but +/// STALLED at turn 60 when played as the 3rd game in a `--games-file` batch +/// (after 2 unrelated games in the same process). Root cause: the AI's +/// interactive search was bounded by a wall-clock deadline +/// (`AI_SEARCH_TIME_BUDGET_MS`), and a warmer/slower Nth-in-batch process +/// expired it on a mid-game decision the fresh solo process completed in full — +/// diverging the game. The fix runs every seat in measurement mode +/// (`build_seat_config`), disabling the wall-clock deadline so search is a pure +/// function of `(seed, difficulty, feed)`. +/// +/// This encodes the repro shape end-to-end: the SAME game, run alone vs. run +/// 3rd after 2 unrelated games, must produce a byte-identical RESULT block. +/// (The deterministic, box-speed-independent catcher for this bug is +/// `ai_commander.rs`'s `seat_config_runs_in_measurement_mode_for_batch_reproducibility` +/// unit test; this is the integration-level forward guard for the whole path.) +#[test] +#[ignore = "loads card-data.json + runs real games; opt in via --ignored"] +fn batched_third_game_matches_same_game_run_alone() { + let target_seed = "95000004"; + let prefix_a = "95000000"; + let prefix_b = "95000001"; + + // The target game run entirely alone (single-game mode), explicitly + // under measurement: this is the re-derived invariant post-D1 -- a + // batch game (always Measurement) must match a solo game run under the + // same PHASE_AI_MEASUREMENT harness escape hatch, not a bare solo run + // (which routes to Interactive by default). + let solo = run_ai_commander_with_context( + &[ + "--seed", + target_seed, + "--difficulty", + "Hard", + "--action-cap", + LEAK_REGRESSION_ACTION_CAP, + ], + true, + ); + + // The SAME target game, but 3rd in a batch process that first played two + // unrelated games — the exact configuration that leaked before the fix. + let pid = std::process::id(); + let games_file_path = std::env::temp_dir().join(format!("ai_commander_leak_repro_{pid}.txt")); + std::fs::write( + &games_file_path, + format!("{prefix_a},Hard\n{prefix_b},Hard\n{target_seed},Hard\n"), + ) + .expect("write games-file"); + let batch = run_ai_commander(&[ + "--games-file", + games_file_path.to_str().unwrap(), + "--action-cap", + LEAK_REGRESSION_ACTION_CAP, + ]); + let _ = std::fs::remove_file(&games_file_path); + + let solo_blocks = game_blocks(&solo); + let batch_blocks = game_blocks(&batch); + assert_eq!( + solo_blocks.len(), + 1, + "single-game must print exactly one game" + ); + assert_eq!( + batch_blocks.len(), + 3, + "batch must print exactly one block per games-file line" + ); + + // Block index 2 is the 3rd (target) game in the batch. + assert_eq!( + normalized_result_block(solo_blocks[0]), + normalized_result_block(batch_blocks[2]), + "the target game played 3rd in a batch must be bit-identical to the \ + same game run alone; a divergence here is the cross-game leak \ + (wall-clock-deadline non-determinism) regressing" + ); +} + +#[test] +#[ignore = "loads card-data.json + runs real games; opt in via --ignored"] +fn batch_output_echoes_seed_and_parsed_difficulty_per_game() { + let pid = std::process::id(); + let games_file_path = std::env::temp_dir().join(format!("ai_commander_echo_games_{pid}.txt")); + std::fs::write(&games_file_path, "9101,Easy\n9102,VeryHard\n").expect("write games-file"); + + let batch = run_ai_commander(&[ + "--games-file", + games_file_path.to_str().unwrap(), + "--action-cap", + TEST_ACTION_CAP, + ]); + let _ = std::fs::remove_file(&games_file_path); + + // Each game's marker must carry BOTH the seed and the difficulty as + // actually parsed from that games-file line — not the process-level + // `--difficulty` (which batch mode ignores). A tier that silently fell + // back to the default would show up here as a wrong `difficulty=` label. + assert!( + batch.contains("--- GAME seed=9101 difficulty=Easy ---"), + "game 1 must echo its parsed seed+difficulty in its marker:\n{batch}" + ); + assert!( + batch.contains("--- GAME seed=9102 difficulty=VeryHard ---"), + "game 2 must echo its parsed seed+difficulty in its marker:\n{batch}" + ); +} + +#[test] +#[ignore = "loads card-data.json + runs real games; opt in via --ignored"] +fn single_game_stdout_is_deterministic_and_preamble_is_pinned() { + let seed = "9201"; + let out1 = run_ai_commander(&[ + "--seed", + seed, + "--difficulty", + "Easy", + "--action-cap", + TEST_ACTION_CAP, + ]); + let out2 = run_ai_commander(&[ + "--seed", + seed, + "--difficulty", + "Easy", + "--action-cap", + TEST_ACTION_CAP, + ]); + + // Two runs of the same seed/feed/difficulty must produce identical + // output modulo wall-clock timing — the process-level half of the same + // property `batched_third_game_matches_same_game_run_alone` asserts + // across the batch boundary. If this one fails too, the divergence is in + // the game itself, not in batch sequencing. + let normalize = |s: &str| { + s.lines() + .filter(|l| !l.contains("elapsed=") && !l.starts_with("Elapsed:")) + .collect::>() + .join("\n") + }; + assert_eq!(normalize(&out1), normalize(&out2)); + + // Pins the exact preamble single-game mode has always printed, in order. + // A reordering (e.g. "Feed:" moving relative to "Seed:.../Difficulty:...") + // would silently break the pod-lab harness's stdout parsing; this fails + // loudly instead. + let expected_preamble = format!( + "=== 4-player Commander AI test ===\n\ + Feed: feeds/mtggoldfish-commander.json\n\ + Seed: {seed} Difficulty: Easy\n\n" + ); + assert!( + out1.starts_with(&expected_preamble), + "single-game preamble format changed:\n{out1}" + ); +} + +/// D1 must-pass gate for B3: a plain single-game invocation (no +/// `--games-file`, no `PHASE_AI_MEASUREMENT` override) must route to +/// `RunContext::Interactive` and print the exact preamble marker +/// `ExecutionMode: interactive` (lowercase literal, not `{:?}`), placed after +/// the blank line that closes the existing pinned preamble (see +/// `single_game_stdout_is_deterministic_and_preamble_is_pinned`'s +/// `expected_preamble` -- that assertion is `starts_with`, so an appended +/// marker line does not conflict with it; that constant must not be edited). +#[test] +#[ignore = "loads card-data.json + runs real games; opt in via --ignored"] +fn single_game_default_route_prints_interactive_marker() { + let out = run_ai_commander_with_context( + &[ + "--seed", + "9301", + "--difficulty", + "Easy", + "--action-cap", + TEST_ACTION_CAP, + ], + false, + ); + assert!( + out.contains("ExecutionMode: interactive"), + "a plain single-game invocation must print the ExecutionMode: interactive marker line: +{out}" + ); +} + +/// D1 must-pass gate for B3: a single-game invocation run under the +/// `PHASE_AI_MEASUREMENT` harness escape hatch must route to +/// `RunContext::Measurement` and print the exact preamble marker +/// `ExecutionMode: measurement` (lowercase literal, not `{:?}`). Sibling of +/// `single_game_default_route_prints_interactive_marker` -- together they +/// pin both `RunContext` variants at the same call site so neither can +/// silently regress. This is pod-lab's exact invocation shape (`runner.py` +/// sets `PHASE_AI_MEASUREMENT=1` on every single-game call per D7), so a +/// missing marker here is a live pod-lab tripwire failure, not a hypothetical. +#[test] +#[ignore = "loads card-data.json + runs real games; opt in via --ignored"] +fn single_game_measurement_env_prints_measurement_marker() { + let out = run_ai_commander_with_context( + &[ + "--seed", + "9301", + "--difficulty", + "Easy", + "--action-cap", + TEST_ACTION_CAP, + ], + true, + ); + assert!( + out.contains("ExecutionMode: measurement"), + "a single-game invocation under PHASE_AI_MEASUREMENT=1 must print the ExecutionMode: measurement marker line: +{out}" + ); +}