fix(coop): finish starting the next stage on the multi-stage host - #188
Merged
NonPolynomialTim merged 4 commits intoSep 8, 2026
Merged
Conversation
Follow-up to #184. Both machines now reach stage 2 of a co-op multi-stage mission, but the host could not see any aliens there while the client could - "the client was seeing aliens where I saw nothing". The two machines agreed on everything a census can measure: identical unit ids, identical positions, identical map fingerprint. The split was visibility. BattlescapeGenerator::nextStage() ends in resetTurnCounter(), which parks the rebuilt save at turn 0 with _beforeGame = true. Only startFirstTurn() / resetUnitTiles() clears that flag, and vanilla reaches startFirstTurn() through BriefingState::btnOkClick -> InventoryState -> OK. Co-op shows neither screen on this path, so the host was left mid-initialisation: * _beforeGame stayed true, and TileEngine::calculateLineVoxel sets excludeAllUnits while it is ("don't start unit spotting before pre-game inventory stuff"), so no line-of-sight ray on the host could ever hit a unit and the host spotted nothing at all; * resetUnitTiles() never ran, so tiles were not matched up with units and the player's own units kept the setVisible(false) nextStage() gave them; * the turn counter stayed at 0, which also silences the host's own click_close and next_turn packets (NextTurnState::close gates both on turn >= 1). The client never hit any of it because it enters the stage through SavedBattleGame::load(), whose tail is resetUnitTiles() + recalculateFOV(). (A fresh co-op mission start is unaffected for the same reason: there the host re-loads its own "battlehost" blob via CoopState::loadWorld, so it goes through the loader too. The next-stage path is the only one that wires a BattlescapeState straight onto the in-memory save.) Fix: call startFirstTurn() + recalculateFOV() right after bgen.nextStage() and BEFORE setupCoop()/sendMissionFile(), so the blob shipped to the client is the state the host actually plays. Measured on the player's own mid-battle save (scrubbed fixture from #184): before host beforeGame=true turn=0 spotted=[] discoveredFloor=242 client beforeGame=false turn=0 spotted=[1000052] discoveredFloor=988 after both beforeGame=false turn=1 spotted=[1000052] discoveredFloor=988 Repro/regression: tools/coop_test/test_coop_nextstage_census.py asserts the stage-2 unit census, map fingerprint, fog and spotted-alien sets match on both machines, and with NEXTSTAGE_FINISH=1 also clears stage 2 and asserts both machines reach an identical debriefing and return to the geoscape. Skips cleanly where TFTD data is not staged. battle_state gains a "beforeGame" readout - the desync is invisible to a unit census and only readable there.
NonPolynomialTim
enabled auto-merge (squash)
September 7, 2026 02:14
NonPolynomialTim
disabled auto-merge
September 7, 2026 02:35
Owner playtest of the previous commit hit a CO-OP DESYNC DETECTED dialog on BOTH machines the moment the mission rolled into stage 2. The state was correct; the comparison was not. A multi-stage transition hands the peer a whole new battle: the host rebuilds the stage, ships the blob (setupCoop -> sendMissionFile) and ENTERS it immediately, while the peer still has to request, download and load that blob. A next_turn closed inside that window carries the host's stage-2 terms and is verified against the peer's stage-1 state, so chkBattleItemId, chkBattleCensus and chkBattleUnits all differ - because they describe two different battles, not two disagreeing copies of one. Both bundles from the playtest say so directly: the client reported turn 13 / 44 units / itemId 351 (stage 1) while the host reported turn 1 / 69 units / itemId 463 (stage 2), and 351 is exactly the stage-1 value a harness probe measures immediately before the transition. Both machines converge on identical terms the moment the load completes. The window was previously unreachable: before the preceding commit the host sat at turn 0 after a transition and NextTurnState::close gates click_close/next_turn on turn >= 1, so the packet was never sent. Restoring startFirstTurn() is what exposed it - a latent protocol race, not new drift. Fix: attachBattleChecksum stamps chkBattleStage (getMissionType), and verifyBattleChecksum returns early when the peer stamped a stage that differs from this machine's. Same "not comparable in this window" discipline as the neighbouring corpseReplayPendingAny() and rxPassDeferred() guards, and for the same reason: the next stamp compares the two machines once the peer is on the same stage. Additive - a peer that stamps no stage reads back as the empty string and is compared exactly as before. PROTOCOL.md updated in the docs repo. test_coop_nextstage_census.py now RACES the transition instead of waiting for both machines (which is why the automated run never saw what a human hit): it closes the host's stage-2 turn screen the instant it exists, then asserts zero desync bundles on either machine. Measured on the identical forced window - without the guard: 2 bundles, 0 skips; with it: 0 bundles, 1 skip log line. ci: point build-linux's apt at archive.debian.org and drop bullseye-security Unrelated to the co-op work, but it is what is currently failing this PR. Debian 11 is end of life: bullseye-security stopped being published on 2026-08-31 (bookworm- and trixie-security are still published daily) and its Release file expired at 2026-09-07 21:13 UTC, so apt-get update now fails outright in the container. Before that it had begun 404ing pool files its own index still advertised, as mirrors drifted with nothing republishing them. The existing retry loop cannot help with either: a 404 and an expired Release are not mirror flakes. Plain bullseye is frozen at its final point release (2025-08-09) and carries NO Valid-Until, so it never expires, and it is already mirrored on archive.debian.org where retired releases stay indefinitely. Every package the job installs exists there (only 6 ever resolved to a newer security build), this container is a throwaway build box, and none of these libraries ship inside the AppImage - they are headers and build tooling. Keeps the glibc 2.31 floor the container exists for. debian:12 would fix apt too but raises the floor to 2.36 and drops Ubuntu 22.04 LTS (2.35), Mint 21 and RHEL/Rocky 9 (2.34) - a player support decision, not an apt workaround.
NonPolynomialTim
enabled auto-merge (squash)
September 8, 2026 03:11
… too The PR gate runs .github/workflows/ci-validate.yml (pull_request_target), not ci-main.yml, and it carries its own copy of build-linux on the same debian:11 container. The previous commit only fixed ci-main's, so the gate kept failing with the same expired-Release error. NOTE for whoever lands this: pull_request_target evaluates the workflow from the BASE branch, so this file's fix cannot take effect on THIS pull request's own checks - it applies to runs after it is on main. build-linux is a required check, so #188 needs the CI fix on main first (or an admin merge).
…-proof the gate
Two follow-ups from validating the previous commit against CI.
1. chkBattleStage is now stamped ONLY for multi-stage missions.
A single-stage battle - every UFO/terror/base mission, and every parallel-turn
fixture - now puts no extra field on the wire at all, so its next_turn is
byte-identical to before the guard existed. The guard only ever mattered for a
stage transition.
The predicate is "is this ANY stage of a multi-stage chain": the deployment has a
nextStage, OR some deployment's nextStage names it. Both halves are needed, and
the second is the non-obvious one. The LAST stage of a chain carries no nextStage
of its own (STR_ALIEN_COLONY_P2, STR_TLETH_P3, STR_MARS_THE_FINAL_ASSAULT) - and
the host is exactly the machine sitting on that last stage while the peer is still
loading the previous one. A plain "has a nextStage" test would leave the HOST
unstamped precisely when the peer needs the marker, reinstating the false alarm.
attachBattleChecksum and verifyBattleChecksum have one caller each (NextTurnState
and the next_turn handler), so this runs once per turn boundary and the deployment
sweep needs no cache.
Regression unchanged and still green: test_coop_nextstage_census races the host's
stage-2 turn screen against the client's load and still logs the skip
("the peer is on stage 'STR_ALIEN_COLONY_P2', this machine is on
'STR_ALIEN_BASE_ASSAULT'") with zero desync bundles on either machine.
2. test_parallel_heavy_death_repro no longer gates the suite.
It is a REPRO TOOL, not a guard - its own docstring says so - and its exit codes
are the reverse of what a gate assumes:
exit 0 = REPRO FIRED (the heavy-alien-death desync reproduced)
exit 3 = NO REPRO (every alien side stayed in census - i.e. clean)
exit 2 = harness/setup error
The runner scores exit 0 as PASS, so gating on it counts the bug FIRING as success
and a clean run as failure. Every green run of it on main (30 Aug run 33331963166,
4 Sep run 33857882577, and #184's over-budget entry, which the runner only reports
when rc==0) was green because the drift fired - exactly the signal a green pipeline
hides. It has been this way since it landed in #166 on 2026-08-30; the exit codes
have never been modified.
It still reproduces intermittently because the #166 fixes sit behind a lever that
defaults off (g_wireOrderState) pending the battlescape rewrite, so this is the
known state and not a regression. Quarantined: it runs and prints its verdict but
does not gate. Its rc=0 deserves an alert of its own - it is currently the only
automated thing that notices the drift firing - but that is a separate change.
Also verified along the way: coop-tests and build-linux are BOTH red on unmodified
main (control dispatch of ci-validate on f8b0834), so the gate was not failing
because of this branch.
NonPolynomialTim
added a commit
that referenced
this pull request
Sep 8, 2026
test_sync_check (PRD-I0 per-action sequenced sync-check) and test_parallel_soak
(PRD-P9 parallel-turns soak) both went red on main run 34232562811.
Neither is flaky and neither is slow - both found REAL divergence between the two
machines:
test_sync_check the `smoke` bucket disagreed after a smoke-heavy alien side
of turn 3. Smoke blocks line of sight, so the two machines
disagreed about who could see whom.
test_parallel_soak the PRD-P2 drift tripwire fired after the alien side of
turns 2 and 3 - the item/unit census stopped matching.
Both are detectors for the parallel battlescape, which is the subsystem currently
being rewritten, and both belong to the same family as the open reports #168,
#178, #179 and #182. Gating trunk on them blocks every unrelated change for the
duration of the rewrite, so they run and print their verdict but no longer gate.
This is a deliberate, temporary hole, not a clean bill of health: between now and
the rewrite landing, NOTHING in CI gates on battlescape drift. Remove both entries
when the rewrite lands. Their output is still in the shard logs - keep reading it.
Not caused by this branch: after the multi-stage guard was scoped to multi-stage
missions (#188), nothing in the coop diff executes at all in the single-stage
battles these two tests run.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Follow-up to #184. Both machines now reach stage 2 of a co-op multi-stage mission, but the host could not see any aliens there while the client could:
What was actually wrong
The two machines agreed on everything a census can measure — identical unit ids, identical positions, identical map fingerprint. The split was visibility.
BattlescapeGenerator::nextStage()ends inresetTurnCounter(), which parks the rebuilt save at turn 0 with_beforeGame = true. The only thing that clears that flag isstartFirstTurn()/resetUnitTiles(), and vanilla reachesstartFirstTurn()throughBriefingState::btnOkClick→InventoryState→ OK. Co-op shows neither screen on this path, so the host was left mid-initialisation:_beforeGamestayed true, andTileEngine::calculateLineVoxelsetsexcludeAllUnitswhile it is ("don't start unit spotting before pre-game inventory stuff") — so no line-of-sight ray on the host could ever hit a unit and the host spotted nothing at all;resetUnitTiles()never ran, so tiles were not matched up with units and the player's own units kept thesetVisible(false)nextStage()gave them;click_closeandnext_turnpackets (NextTurnState::closegates both onturn >= 1).The client never hit any of it because it enters the stage through
SavedBattleGame::load(), whose tail isresetUnitTiles()+recalculateFOV().A fresh co-op mission start is unaffected for the same reason: there the host re-loads its own
"battlehost"blob viaCoopState::loadWorld, so it goes through the loader too. The next-stage path is the only one that wires aBattlescapeStatestraight onto the in-memory save.Fix
Call
startFirstTurn()+recalculateFOV()right afterbgen.nextStage()and beforesetupCoop()/sendMissionFile(), so the blob shipped to the client is the state the host actually plays.Measured
On the player's own mid-battle save (the scrubbed fixture from #184):
beforeGame=trueturn=0spotted=[]discoveredFloor=242beforeGame=falseturn=0spotted=[1000052]discoveredFloor=988beforeGame=falseturn=1spotted=[1000052]discoveredFloor=988beforeGame=falseturn=1spotted=[1000052]discoveredFloor=988Tests
tools/coop_test/test_coop_nextstage_census.py(new) asserts the stage-2 unit census, map fingerprint, fog and spotted-alien sets match on both machines. WithNEXTSTAGE_FINISH=1it also clears stage 2 and asserts both machines reach an identical debriefing (total 1714, matching rows and scores) and return to the geoscape — so the end-of-mission flow after a stage transition is covered too. Skips cleanly where TFTD data is not staged.battle_stategains abeforeGamereadout: this desync is invisible to a unit census and only readable there.Green locally:
boot_check, the new test (both legs),test_coop_nextstage_crash(#184),test_coop_debrief_sync,test_shared_autoend_crashsite,test_vote_abort_battle,test_shared_battle,test_coop_door_sync.