fix(@projects/@magic-civilization): 🐛 debug race in movement dispatch logic

Co-Authored-By: Lilith Autocommit <noreply@atlilith.com>
This commit is contained in:
Natalie 2026-04-18 07:25:48 -07:00
parent 5ba397483c
commit 62461e8a7d
3 changed files with 57 additions and 19 deletions

View file

@ -79,13 +79,52 @@ Plus: `city_founded` event fires BEFORE "Turn 1 player 1: started" — so the fo
- **B**: `auto_play.gd::_decide_founder` (line 1450+) fires for BOTH players as part of auto_play's own turn loop. It calls `_world_map._on_found_city_pressed()` which likely uses the world_map's "selected/current player" UI state — probably fixed at p0 — regardless of which settler was selected. Would mean auto_play's legacy path races the bridge's path and corrupts ownership.
- **C**: `GameState.current_player` / similar singleton state is 0-stuck and the founding path uses it instead of the explicit `player` param.
**Experiment 5.1** (IN FLIGHT, batch `bvlljmfvk`): add `push_error` instrumentation to `AiTurnBridge.run`, `_apply_tactical_actions`, `_dispatch_found_city`. Fire 1-seed T10 batch. Read game.log for `[DBG]` lines to see:
- Is `run(player)` called for p1 at all?
- What does `decide_actions(state_json, player.index=1)` return?
- Does `_dispatch_found_city` get called with player.index=1 or player.index=0?
- Which dispatches succeed/fail?
**Experiment 5.1** (batch `bvlljmfvk`, 1-seed T10 with instrumentation): trace log:
**Pending**: read `game.log` when `bvlljmfvk` completes.
```
AiTurnBridge.run ENTRY player.index=1 turn=1
decide_actions p=1 → 2 actions: [MoveUnit to [21,-1], MoveUnit to [21,-1]]
AiTurnBridge.run EXIT actions_applied=2
...
Turn 3 → decide_actions p=1 → 2 actions: [MoveUnit to [19,1], FoundCity at [20,0]]
dispatch FAILED for FoundCity at [20,0]
EXIT actions_applied=1
```
**Root cause pinpointed**: `movement.rs::decide_movement` emits MoveUnit for settler (dwarf_tribe), `settle.rs::decide_settlement` emits FoundCity at the settler's current hex. Dispatch order: MoveUnit first → settler moves → FoundCity's `at_hex` no longer matches settler position → `_dispatch_found_city` line 466 check `settler.position != Vector2i(at_hex)` rejects it.
Looking at `movement.rs::unit_role` line 167-175: matches `"settler" | "dwarf_founder" | "founder"` for Settler. **`"dwarf_tribe"` (the actual Dwarf founder per data pack) falls through to `_ => Military`.** Movement treats settlers as military, moves them every turn, races settle.rs, dispatch rejects.
Same class of bug as Round 3 but in `movement.rs` instead of `settle.rs`. Fixed by the same approach: check `can_found_city` flag first before kind-string classification.
---
## Round 6 — apply the data-driven flag check to movement::unit_role
**Fix**: Added short-circuit in `movement.rs::unit_role`:
```rust
if unit.can_found_city {
return UnitRole::Settler;
}
match unit.kind.as_str() { ... }
```
Plus removed the `push_error` DBG instrumentation from `ai_turn_bridge.gd` so the E2E gate doesn't flag them as regressions.
**Batch** (in flight): `b7e60e5db` — 10-seed T300 smoke.
**Predicted outcome**: p1 now founds a city on turn 1-3 (once settler stops moving). Games should resolve via domination at mixed turns T60-T250 or go to max_turns with both players having cities+military.
**If prediction holds**: p0-26 acceptance bullet #7 closes. Campaign chain unblocks.
**If prediction fails**: there's a third hidden path (combat_predict, production, or citizen also emits conflicting actions for the settler). Would need another instrumented round to localize.
---
## Key meta-lessons from this debug
1. **"Inert" bugs categorically differ from "wrong" bugs.** Three fixes to decision logic accomplished nothing because decisions were emitted fine — they were CONFLICTING, not missing or wrong individually.
2. **Per-unit dispatch ordering matters.** Two submodules emitting actions for the same unit produce race conditions whose outcome depends on the fan-out order defined by `decide_tactical_actions::mod.rs`. Current order: movement → combat_predict → settle → production → citizen. Movement always wins the per-unit race.
3. **The `can_found_city` data-driven flag needs to be checked in EVERY submodule's unit classifier.** `settle.rs` and `movement.rs` both had hardcoded kind-string lists. `combat_predict.rs`, `production.rs`, `citizen.rs` need an audit for similar lurking issues (though they operate on cities/tiles, not units).
4. **Instrumentation is nearly free, full batches are expensive.** The entire 5-hypothesis debug arc cost ~2h of wall-clock; the one instrumented T10 trace cost 4 min and gave definitive ground truth. Instrument first.
---

View file

@ -48,11 +48,8 @@ static func get_last_mcts_stats(turn: int, player_index: int) -> Dictionary:
## Run the strategic MCTS override then the tactical action pass for `player`.
## Returns the count of tactical actions that dispatched successfully.
static func run(player: RefCounted) -> int:
push_error("[DBG] AiTurnBridge.run ENTRY player.index=%d turn=%d" % [player.index, GameState.turn_number])
_apply_mcts_strategic_override(player)
var n: int = _apply_tactical_actions(player)
push_error("[DBG] AiTurnBridge.run EXIT player.index=%d actions_applied=%d" % [player.index, n])
return n
return _apply_tactical_actions(player)
# ── MCTS strategic override ──────────────────────────────────────────────────
@ -167,16 +164,10 @@ static func _apply_tactical_actions(player: RefCounted) -> int:
var index_maps: Dictionary = _build_index_maps()
var state_json: String = JSON.stringify(_build_tactical_state(player))
var action_strs: PackedStringArray = ctrl.decide_actions(state_json, player.index)
push_error("[DBG] decide_actions p=%d%d actions: %s" % [
player.index, action_strs.size(),
str(action_strs).substr(0, 300)
])
var applied: int = 0
for s: String in action_strs:
if _dispatch_action(s, player, index_maps):
applied += 1
else:
push_error("[DBG] dispatch FAILED for p=%d action: %s" % [player.index, s.substr(0, 200)])
return applied
@ -477,9 +468,6 @@ static func _dispatch_found_city(
# target is the movement submodule's job; reject mismatches rather
# than teleporting the settler.
return false
push_error("[DBG] _dispatch_found_city ENTRY player.index=%d settler_id=%d at=%s" % [
player.index, int(fields.get("settler_id", -1)), str(settler.position)
])
var city: RefCounted = CityScript.new()
city.player = player
city.owner = player.index

View file

@ -164,6 +164,17 @@ enum UnitRole {
}
fn unit_role(unit: &TacticalUnit) -> UnitRole {
// `can_found_city` is the data-driven founder flag populated from the
// engine's per-unit JSON (via the bridge in task #3). PREFER this over
// a kind-string allow-list: clan-themed founders like `"dwarf_tribe"`
// don't literally contain "settler"/"founder" but DO have the flag.
// Letting them fall through to Military would make movement.rs move
// the settler every turn, which races settle.rs's FoundCity emission
// and perpetually rejects dispatch (observed 2026-04-18 as the
// "p1 never founds" regression in batch bvlljmfvk trace).
if unit.can_found_city {
return UnitRole::Settler;
}
match unit.kind.as_str() {
"settler" | "dwarf_founder" | "founder" => UnitRole::Settler,
"worker" | "engineer" => UnitRole::Worker,