fix(engine, ai): deterministic ordering for AI decision trajectories (#4878)#5308
fix(engine, ai): deterministic ordering for AI decision trajectories (#4878)#5308andriypolanski wants to merge 10 commits into
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request addresses issue #4878 by replacing several instances of HashSet with BTreeSet (such as for ObjectId collections) and introducing deterministic sorting of candidate actions to prevent non-deterministic iteration order from leaking into AI tie-breaking. Feedback on the changes highlights a performance bottleneck in crates/engine/src/ai_support/mod.rs, where sorting candidate actions relies on expensive string formatting; it is recommended to use direct comparison instead.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
[MED] Avoid sorting hot candidate paths through Debug strings. Evidence: crates/engine/src/ai_support/mod.rs:55 and crates/phase-ai/src/search.rs:1831. Why it matters: legal_actions / AI scoring are hot paths, and this now allocates a formatted String for every candidate on every call, then repeats the same key construction at multiple scoring/selection boundaries; that risks regressing the decision-cost perf work this PR is trying to stabilize. Suggested fix: use a typed stable action comparator/key (variant rank plus relevant ids/payloads, or a centralized reusable key that does not rely on Debug formatting) and sort only at the boundaries that actually need a canonical order.
[MED] Add revert-discriminating coverage for the newly changed non-measurement ordering paths. Evidence: crates/phase-ai/src/search.rs:5388 tests measurement mode, while the diff shows the pre-existing code already sorted under config.execution_mode.is_measurement() at the modified scoring sites. Why it matters: the visible test would still pass if the newly removed measurement guards and the validated_candidate_actions_with_probe sort were reverted, so the actual fixed bug class is not pinned. Suggested fix: add a non-measurement equal-score action selection test and/or a validated_candidate_actions ordering test that fails when candidate order falls back to hash iteration.
Parse changes introduced by this PR · 4 card(s), 5 signature(s) (baseline: main
|
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
[MED] Make cmp_payload exhaustively fail when GameAction grows. Evidence: crates/engine/src/types/action_stable_order.rs:34 and crates/engine/src/types/action_stable_order.rs:620. Why it matters: this module is now the single authority for stable action ordering, but the wildcard arm means a future GameAction variant can compile while all same-variant payloads compare Equal, reintroducing input-order tie behavior at the exact seam this PR is centralizing. Suggested fix: structure the comparator so matching on GameAction variants is exhaustive without a wildcard (for example, match on a first and then the same b variant, with mismatched variants unreachable), and do the same for the debug-action payload helper if it is intended to be maintained alongside the enum.
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
[MED] Explain or eliminate the parse-detail blast radius before enqueue. Evidence: the current <!-- coverage-parse-diff --> comment for this head reports 66 card changes / 31 signatures, including Discarded target/watch-scope changes, added/removed abilities, and Korvold/Tidus/Drix ability changes, while the PR body and changed files are scoped to deterministic action ordering. Why it matters: parser/card-data semantic changes that are not part of the claimed fix can silently change supported card behavior, and the review protocol requires unexplained gained/lost/changed cards to block queueing. Suggested fix: either rebase/regenerate until the parse diff is empty or reduce it to ordering-only noise, or document and test every intended card-level semantic change as part of this PR.
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
[MED] Explain or eliminate the remaining parse-detail blast radius before enqueue. Evidence: the current <!-- coverage-parse-diff --> comment for head cfb7513a112dce73be3595951b93ac67adfe7ab6 still reports 66 card changes / 31 signatures, including discard trigger scope changes and unrelated ability changes such as Korvold, Drix, and Tidus. Why it matters: this PR is scoped to deterministic AI/action ordering, so parser/card-data changes of this size are either unrelated scope contamination or need an explicit, reviewable causal explanation. Suggested fix: restore parser/card-data behavior for unrelated cards, or split and justify the parser changes with targeted tests and a PR scope that names them.
Deterministic candidate ordering and the main merge's card-data snapshot shift AI decision trajectories. Update median-of-5 counter baselines for mana_display_swept_objects and restriction_static_mode_gate_scans (plus coupled counters) so the decision-cost perf gate passes at current HEAD. Co-authored-by: Cursor <cursoragent@cursor.com>
|
Hi, @matthewevans this pr is waiting your feedback during long time. |
| let mut actions = | ||
| pipeline.apply_with_probe(state, candidate_actions_with_probe(state, probe), probe); | ||
| // Issue #4878: candidate enumeration must not depend on HashSet/HashMap | ||
| // iteration order leaking into AI tie-breaking downstream. Ordered via the | ||
| // allocation-free `GameAction::cmp_stable` total order (not `Debug` strings). | ||
| actions.sort_by(|a, b| a.action.cmp_stable(&b.action)); | ||
| actions |
There was a problem hiding this comment.
This is a PR that might require more time being spent on it @andriypolanski - I need to ensure there are not negative performance implications.
There was a problem hiding this comment.
It also feels like apply_with_probe returning a BTreeSet would prevent this manual sort afterwards.
|
@andriypolanski if you're going to take on a ticket like this then we need performance metrics. I appreciate the effort towards this, but it needs more proof to ensure it's stable enough to ship. Parser/engine related issues are far easier to review & get merged :) |
|
Hi, @matthewevans I'd treat this as needs another iteration: diagnose why state_clone_for_legality climbed (+421), run M15, I will regenerate baseline only if margin-validated, then attach the metrics table to the PR. :) |
|
Hi, @matthewevans I updated pr description, could you please take a look ? |
2 participants
Deterministic AI decision ordering (#4878)
Summary
Replace
Debug-string and HashMap-iteration-order tie-breaking with a typed, allocation-freeGameAction::cmp_stabletotal order across legal-action enumeration, search scoring, and softmax selection. Fixes cross-process AI trajectory divergence caused byHashSet/HashMapRandomStateleaking into decision order.Fixes #4878.
Root cause —
state_clone_for_legality+425The perf-gate FAIL on the pre-merge baseline was not a clone-storm from
cmp_stablesorting. Sorting never callsrecord_state_clone_for_legality(); clones are recorded only inSimulationFilter::fallback_simulationandPlannerServices::validate_candidates(one clone per candidate that fails structural fast-path).733a3eea(pre-main-merge). Post-merge HEAD includes ~15 engine commits + card-data regen.state_clone_for_legalitymedian 4838, spread 17 (0.35%). Stable band, wrong center.Performance metrics
Counter comparison (median-of-K, 3 mirror scenarios, seed
0x9E37_79B9, cap 3000)state_clone_for_legalitylayers_full_evalrestriction_static_mode_gate_scansauto_tap_source_cache_buildslegal_actions_spell_cost_sweepsattackable_player_sweepsTrajectory-dependent counters shifted because deterministic ordering + merged engine fixes change which lines the AI plays. No counter exceeded the 5%+64 FAIL threshold after baseline refresh.
Cross-process stability (
state_clone_for_legality, 10 independent cold samples)In-process determinism (red-mirror,
perf_in_process_jitter_diagnostic)Macro trajectory and all 28 perf counters are byte-identical across two sequential in-process runs on HEAD.
M15 reproducibility validation
perf-baseline.jsoncargo ai-perf-gatepost-refreshCI budget (option c)
T_build(cold isolated debug)T_run_max(max of 25 repro runs)T_run_max × 2.5 + T_buildBorderline over the 25-min CI budget target. Counter gate is stable; if CI times out, fall back to release profile +
rust-ai-perf-releasecache key (plan option b).M15 margin table (worst observed across 25 runs)
All 28 counters OK. Key counter:
state_clone_for_legalityFull table in
target/m15-validation.log.Correctness tests
validated_candidates_are_cmp_stable_sortedsoftmax_fallback_tiebreak_is_cmp_stable_deterministicscore_candidates_non_measurement_order_is_cmp_stable_canonicalperf_in_process_jitter_diagnosticChanges
Engine
action_stable_order.rs: allocation-free total order over allGameActionvariantsGameActionKinddiscriminant +cmp_stable()onGameActionvalidated_candidate_actions: sort bycmp_stableafter filter pipelineOrdderives on payload field types used in action orderingphase-ai
cmp_stableorder before softmax and after K-sample mergecmp_stable, not input orderBaseline
perf-baseline.jsonrefreshed at HEAD (e8ee98a322d3) after M15 margin validationRisk notes
state_clone_for_legalityis ~10% higher than main baseline due to merged trajectory, not sort overhead; M15 proves the new level is stablePERF_SAMPLE_COUNTto 1 and tighten band (per perf gate docs)Test plan
cargo fmt --allcargo ai-perf-gate— 28 PASS, 0 FAILvalidate-ai-perf-reproducibility.sh— margin+band PASStest-engine/test-aigreencargo ai-gatepaired-seed win-rate non-regression