Save the Echo AI's building-order id (savegame-resumed AI games were not reproducible)

[Giszmo]

Summary

AIEcho::Construction::BuildingOrder::id is assigned at runtime (Echo::add_building_order → BuildingRegister::register_building) and is the key the order is known by in BuildingRegister::pending_buildings. BuildingOrder::save() and load() never touched it, and the constructor load() uses initialised nothing, so every pending Echo/Nicowar building order restored from a savegame carried whatever was in the heap. Echo::update_building_orders then used that value as a map key (br.issue_order) and passed it to AssignWorkers.

Consequences on master:

• A game resumed from a save with AI players is not reproducible run to run. Eight Nicowars on Playground resumed from a tick-25000 save and run to 27000 settle on one of two outcomes (checksums 9421f63c / 6e93bf5b, 502 vs 510 deliveries), roughly coin-flipped. setarch -R makes it stable; with ASLR off, MALLOC_PERTURB_=85 alone flips it, and so does enabling GLOB2_CHECKSUM_SIDECAR, which only moves the heap.
• Any multiplayer game loaded from a save with AI players can desync without packet loss or version mismatch.
• Every A/B measured from a warm save (all of our recent pathfinding and hiring beds) had this noise underneath it.

Memcheck on a symbolised build names it: uninitialised value read in BuildingRegister::issue_order (BuildingRegister.cpp:105), origin the operator new in Echo::load (EchoSerialization.cpp:65).

A game played from the start was never affected; the id is only garbage on the load path.

Change

• Serialise id in BuildingOrder::save() / load(), read when versionMinor >= 96.
• Default the member to -1, so an order constructed and never registered is still readable.
• Saves written before 96 do not carry the field: Echo::load hands those orders a fresh br.register_building() rather than a sentinel, because the value has to be a real register key. BuildingRegister is loaded earlier in the same function, so the registration is valid at that point.
• VERSION_MINOR 95 → 96, REPLAY_MINIMUM_VERSION_MINOR 96. MINIMUM_VERSION_MINOR (58) is unchanged; old saves still load.

Known limitation of the legacy path, deliberate: for a pre-96 save, the register's old pending_buildings entry for a not-yet-issued order stays behind under its old key (those entries are never aged out in BuildingRegister::tick). It is one orphaned map entry per pending order at load time, never read again. Issued orders age out or get found on their own.

Verification

• Memcheck: 24 "conditional jump depends on uninitialised value" reports in the first 40 ticks → 0.
• Stability: 6/6 identical runs (checkSum 9421f63c, ASLR left on, fresh HOME per run) on the save that coin-flipped on master. It settles on the value master reaches with ASLR off.
• With the fix, the six-seed eight-Nicowar bed (tick 25000 → 40000) is identical across MALLOC_PERTURB_ 0 / 85 / 170 on every seed; on master seed 7 spread 3322/3322/3289.
• Backward compatibility: the version-84 and version-88 legacy save fixtures still load and match their expected output; both predate the field and exercise the register_building() fallback.
• Green: TestsRunner (187), savegame-safety, buffered-file, trapped-unit, hiring-bucket, resource-fetch-target, round-trip-hunger-gate, inn-swap, team-stats.

Not claimed: this does not change the result of savegame-safety-test on master (it passes there), so it is not evidence about #232. Re-running #232 on top of this is worthwhile, but the two are not shown to be the same bug.

Found and fixed by Carol while trying to make a hiring A/B reproduce; opened by Bob on Leo's request.

🤖 Generated with Claude Code

[genixpro]

Reviewed and verified locally (macOS/arm64, clang). Approving; I've pushed the one thing the PR was missing rather than asking for a round trip.

What I checked, beyond the description

• The bug is real and its blast radius is as described. On master BuildingOrder::save() writes building_type, number_of_workers, constraints and conditions and never the id, and Construction.h declared a bare int id;. The garbage reaches br.issue_order((*i)->id, …) as a map key (Echo.cpp:161), two AssignWorkers(…, (*i)->id) orders (:166, :172), and previous_building_id (:177).
• The legacy fallback is sound, which is the part that could have gone wrong. A fresh register_building() during load would be dangerous if the counter were reset, since a new id could overwrite a restored pending_buildings entry. It can't: BuildingRegister::load restores building_id from the stream (BuildingRegister.cpp:159) and Echo::load calls br.load() before the building_orders loop, so the counter is already at the saved high-water mark. It's a pure monotonic counter walked in index order, so every client loading the same save derives identical ids — no new desync.
• The version gate reads the right number. Player::load → ai->load(stream, versionMinor) (Player.cpp:134) → AI::load → aiImplementation->load(…, versionMinor) → Echo::load → BuildingOrder::load all thread the savegame's VERSION_MINOR. Echo::load also runs a signature_check after the section, so a wrong gate fails loudly rather than corrupting silently.
• Save backcompat holds. MINIMUM_VERSION_MINOR stays 58 and pre-96 saves load through the fallback.
• The orphaned register entry is accurately characterised as harmless. register_building() inserts with AI_ECHO_PENDING_NOT_ISSUED and tick() only ages entries whose ticks field differs from it, so the stale entry does persist. The failure mode that would make that matter — previous_building_id stuck pending and blocking new orders at Echo.cpp:156 — can't occur: it isn't serialised (resets to -1 on load) and is only ever set to an issued order, which does age out.

Added in 7a2a4f4: the missing regression test

The verification here was all prose in the description, which per CLAUDE.md isn't evidence a reviewer can check, and every comparable save/load fix in this tree ships a fixture. test/EchoBuildingOrderSaveLoadTest.cpp pins the version-96 round trip, the -1 sentinel surviving the Uint32 on the wire, and the pre-96 layout leaving the sentinel with every following field still aligned.

Negative control: removing the two serialisation lines from BuildingOrder.cpp fails exactly the roundTrip: id preserved assertion and leaves the other eleven passing, so it tests the fix and not the scaffolding.

Local results on the pushed head: TestsRunner OK (187 tests), and all 20 standalone ./*Test / ./*Harness binaries pass. The binary is named *Test, so the existing "Build and run the tests" step picks it up with no workflow change. BuildingOrder::save/load, id and the default constructor are private, so the test reaches them via a friend declaration — the same way GradientBFSTest reaches AIEcho::Gradients::Gradient.

Two non-blocking notes

1. Version 96 is contested three ways. Let a cached route field notice the ground moved, wherever it moved #232 and Read one worker level where hiring read harvest and the menu read build #256 both also claim 96 (Hire a fetcher for one delivery at a time #254 → 97, Put the team's stocked markets on the fetch gradients #257 → 98). Only one can have it, and as a pure bugfix and the base of that whole stack this is the right one to take it — merging this first means the others rebase onto a settled number instead of onto each other's guesses.
2. One correction to the description's framing. "A game played from the start was never affected" is true of live simulation but reads as though replays were safe. They weren't: ReplayWriter embeds a full game state via gui.save(buffer, "replayHeader") (ReplayWriter.cpp:77) and ReplayReader loads it back, so starting a replay is a load and goes through the garbage-id path. That makes REPLAY_MINIMUM_VERSION_MINOR = 96 necessary rather than merely cautious — a v95 replay carries no id field, takes the fallback, and can diverge from its recorded orders. Correct call, under-argued.

Not covered by me: Windows/MinGW and a cross-architecture checksum comparison; CI covers the former.