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

src/ReplayReader.h

@@ -23,10 +23,11 @@ static constexpr Uint32 REPLAY_MIN_VALID_ORDERS = 5;
 
 //! Oldest replay format (the VERSION_MINOR the replay was written with) that
 //! the reader still accepts. Replays older than this are rejected: versions 90, 92,
-//! 93, 94 and 95 changed the simulation (weighted pathfinding and diagonal timing,
+//! 93, 94, 95 and 96 changed the simulation (weighted pathfinding and diagonal timing,
 //! hiring-bucket iteration, trapped-colony elimination, fetch-job apportionment,
-//! round-trip routing and hiring), so earlier replays would diverge from what happened.
-static constexpr Uint16 REPLAY_MINIMUM_VERSION_MINOR = 95;
+//! round-trip routing and hiring, Echo building-order ids surviving a load), so
+//! earlier replays would diverge from what happened.
+static constexpr Uint16 REPLAY_MINIMUM_VERSION_MINOR = 96;
 
 /// This class is used for reading replays.
 /// The replay stream is kept open and read every time you do retrieveOrder.

src/Version.h

@@ -6,7 +6,7 @@
 // This is the version of map and savegame format, and all of the recorded data on the server
 #define VERSION_MAJOR 0
 #define MINIMUM_VERSION_MINOR 58
-#define VERSION_MINOR 95
+#define VERSION_MINOR 96
 // version 91 saves the live RNG and routing state for deterministic continuation.
 // version 10 adds script saved in game
 // version 11 the gamesfiles do saves which building has been seen under fog of war.
@@ -102,6 +102,9 @@
 //            rather than refusing it: the simulation changed again
 // version 95 routes and hires by round trip (fetch plus carry) and saves the
 //            round-trip fields with the map runtime state: the simulation changed again
+// version 96 saves AIEcho::Construction::BuildingOrder::id, which was assigned at
+//            runtime and never serialised, so every pending building order restored
+//            from a save carried an uninitialised heap value as its register key
 
 //This must be updated when there are changes to YOG, MapHeader, GameHeader, BasePlayer, BaseTeam,
 //NetMessage, and the likes, in parallel to change of the VERSION_MINOR above

src/ai/echo/BuildingOrder.cpp

@@ -27,6 +27,9 @@ bool BuildingOrder::load(GAGCore::InputStream *stream, Player *player, Sint32 ve
 
 	building_type=stream->readUint32("building_type");
 	number_of_workers=stream->readUint32("number_of_workers");
+	// Saves older than 96 do not carry it; Echo::load registers a fresh one.
+	if (versionMinor>=96)
+		id=static_cast<int>(stream->readUint32("id"));
 
 	stream->readEnterSection("constraints");
 	Uint32 size = stream->readUint32("size");
@@ -62,6 +65,7 @@ void BuildingOrder::save(GAGCore::OutputStream *stream)
 
 	stream->writeUint32(building_type, "building_type");
 	stream->writeUint32(number_of_workers, "number_of_workers");
+	stream->writeUint32(static_cast<Uint32>(id), "id");
 
 	stream->writeEnterSection("constraints");
 	stream->writeUint32(constraints.size(), "size");

src/ai/echo/Construction.h

@@ -14,6 +14,8 @@
 #include <vector>
 #include <boost/logic/tribool.hpp>
 
+class EchoBuildingOrderSaveLoadTest;
+
 namespace AIEcho
 {
 	class Echo;
@@ -236,6 +238,7 @@ namespace AIEcho
 			void add_condition(Conditions::Condition* condition);
 		private:
 			friend class AIEcho::Echo;
+			friend class ::EchoBuildingOrderSaveLoadTest;
 			BuildingOrder() {}
 			bool load(GAGCore::InputStream *stream, Player *player, Sint32 versionMinor);
 			void save(GAGCore::OutputStream *stream);
@@ -248,7 +251,11 @@ namespace AIEcho
 			int get_number_of_workers() const { return number_of_workers; }
 			int building_type;
 			int number_of_workers;
-			int id;
+			/// Assigned by Echo::add_building_order from BuildingRegister, and the key
+			/// this order is known by in BuildingRegister::pending_buildings. Defaulted
+			/// so an order that is constructed and never registered is still readable;
+			/// load() leaves it at -1 for saves written before it was serialised.
+			int id = -1;
 			std::vector<std::shared_ptr<Constraint> > constraints;
 			std::vector<std::shared_ptr<Conditions::Condition> > conditions;
 		};

src/ai/echo/EchoSerialization.cpp

@@ -64,6 +64,12 @@ bool Echo::load(GAGCore::InputStream *stream, Player *player, Sint32 versionMino
 		stream->readEnterSection(buildingIndex);
 		building_orders[buildingIndex]=std::shared_ptr<BuildingOrder>(new BuildingOrder);
 		building_orders[buildingIndex]->load(stream, player, versionMinor);
+		// A save from before the id was serialised leaves it at -1. Hand out a
+		// fresh registration rather than a sentinel: the id is used as a
+		// BuildingRegister map key and passed to AssignWorkers, so it has to be
+		// a real one. br is already loaded at this point.
+		if (building_orders[buildingIndex]->id < 0)
+			building_orders[buildingIndex]->id = static_cast<int>(br.register_building());
 		stream->readLeaveSection();
 	}
 	stream->readLeaveSection();

test/EchoBuildingOrderSaveLoadTest.cpp

@@ -0,0 +1,147 @@
+// SPDX-License-Identifier: GPL-3.0-or-later
+// Copyright (C) 2026 glob2 contributors
+
+// Regression harness for AIEcho::Construction::BuildingOrder::id serialization.
+// The id is handed out at runtime by Echo::add_building_order (via
+// BuildingRegister::register_building) and is the key the order is known by in
+// BuildingRegister::pending_buildings, but save()/load() never moved it and the
+// member had no initialiser. Every pending building order restored from a save
+// therefore carried an uninitialised heap value, which Echo::update_building_orders
+// then used as a map key for issue_order() and passed to AssignWorkers -- so an
+// AI game resumed from a save was not reproducible run to run, and a resumed
+// multiplayer game could desync without packet loss or a version mismatch.
+//
+// Version 96 serialises the field. Saves older than that do not carry it and
+// load() leaves the member at -1, the sentinel Echo::load keys its
+// fresh-registration fallback on (a real BuildingRegister key is required, so a
+// sentinel must never reach issue_order).
+//
+// Exercised here:
+//   1. version-96 round trip -> id preserved, every other field intact
+//   2. an unregistered order (id still -1) survives the Uint32 on the wire as -1
+//      rather than coming back as 4294967295 and being treated as a real key
+//   3. pre-96 stream (no id on the wire) -> id left at the -1 sentinel, and
+//      every following field still decodes from the right offset
+//
+// Links libgagserver.a for BinaryStream + MemoryStreamBackend.
+
+#include <cstdio>
+#include <memory>
+#include <SDL.h>
+#include "BinaryStream.h"
+#include "StreamBackend.h"
+#include "Version.h"
+#include "echo/Echo.h"
+
+// SHA1 is wired in this project by direct .c-into-.cpp inclusion (see
+// YOGServerPasswordRegistry.cpp); the .h has no extern "C" wrapper, so
+// libgagserver.a's BinaryOutputStream references C++-mangled SHA1 names.
+// Same trick as BulletSaveLoadTest.cpp to satisfy the linker.
+#include "../gnupg/sha1.c"
+
+using namespace GAGCore;
+using AIEcho::Construction::BuildingOrder;
+
+// BuildingOrder::save/load, the id member and the default constructor are all
+// private and friended to this class (src/ai/echo/Construction.h), the same way
+// GradientBFSTest reaches AIEcho::Gradients::Gradient.
+class EchoBuildingOrderSaveLoadTest
+{
+public:
+	static int failures;
+
+	static void check(bool ok, const char* what)
+	{
+		std::printf("%s: %s\n", ok ? "PASS" : "FAIL", what);
+		if (!ok)
+			++failures;
+	}
+
+	static std::unique_ptr<BinaryInputStream> makeInputStream(const MemoryStreamBackend& written)
+	{
+		// BinaryInputStream takes ownership of the backend.
+		MemoryStreamBackend* copy = new MemoryStreamBackend(written);
+		copy->seekFromStart(0);
+		return std::make_unique<BinaryInputStream>(copy);
+	}
+
+	// A constraint-free, condition-free order: load() then never reaches the
+	// Constraint/Condition factories, so the fixture needs no Player.
+	static void testRoundTripCurrentVersion()
+	{
+		BuildingOrder original(5, 3);
+		original.id = 4242;
+
+		MemoryStreamBackend* backend = new MemoryStreamBackend;
+		BinaryOutputStream ostream(backend);
+		original.save(&ostream);
+
+		auto istream = makeInputStream(*backend);
+		BuildingOrder loaded;
+		loaded.load(istream.get(), NULL, VERSION_MINOR);
+
+		check(loaded.id == 4242, "roundTrip: id preserved");
+		check(loaded.building_type == 5, "roundTrip: building_type preserved");
+		check(loaded.number_of_workers == 3, "roundTrip: number_of_workers preserved");
+		check(loaded.constraints.empty(), "roundTrip: constraint list decodes");
+		check(loaded.conditions.empty(), "roundTrip: condition list decodes");
+	}
+
+	// An order constructed but never registered keeps id == -1. It goes onto the
+	// wire as a Uint32, so this pins that it comes back as -1 (and therefore
+	// takes the fallback) rather than as a huge positive key.
+	static void testUnregisteredOrderRoundTrips()
+	{
+		BuildingOrder original(7, 1);
+
+		MemoryStreamBackend* backend = new MemoryStreamBackend;
+		BinaryOutputStream ostream(backend);
+		original.save(&ostream);
+
+		auto istream = makeInputStream(*backend);
+		BuildingOrder loaded;
+		loaded.load(istream.get(), NULL, VERSION_MINOR);
+
+		check(loaded.id == -1, "unregistered: id round-trips as -1 through Uint32");
+		check(loaded.building_type == 7, "unregistered: building_type preserved");
+	}
+
+	// Hand-write the version-95 wire layout: no id after number_of_workers.
+	static void testPre96StreamLeavesSentinel()
+	{
+		MemoryStreamBackend* backend = new MemoryStreamBackend;
+		BinaryOutputStream ostream(backend);
+		ostream.writeEnterSection("BuildingOrder");
+		ostream.writeUint32(5, "building_type");
+		ostream.writeUint32(3, "number_of_workers");
+		ostream.writeEnterSection("constraints");
+		ostream.writeUint32(0, "size");
+		ostream.writeLeaveSection();
+		ostream.writeEnterSection("conditions");
+		ostream.writeUint32(0, "size");
+		ostream.writeLeaveSection();
+		ostream.writeLeaveSection();
+
+		auto istream = makeInputStream(*backend);
+		BuildingOrder loaded;
+		loaded.load(istream.get(), NULL, 95);
+
+		check(loaded.id == -1, "pre96: id left at the -1 sentinel for Echo::load to replace");
+		check(loaded.building_type == 5, "pre96: building_type still aligned");
+		check(loaded.number_of_workers == 3, "pre96: number_of_workers still aligned");
+		check(loaded.constraints.empty(), "pre96: constraint list still aligned");
+		check(loaded.conditions.empty(), "pre96: condition list still aligned");
+	}
+};
+
+int EchoBuildingOrderSaveLoadTest::failures = 0;
+
+int main(int /*argc*/, char* /*argv*/[])
+{
+	EchoBuildingOrderSaveLoadTest::testRoundTripCurrentVersion();
+	EchoBuildingOrderSaveLoadTest::testUnregisteredOrderRoundTrips();
+	EchoBuildingOrderSaveLoadTest::testPre96StreamLeavesSentinel();
+	const int failures = EchoBuildingOrderSaveLoadTest::failures;
+	std::printf(failures == 0 ? "ALL PASS\n" : "FAILURES: %d\n", failures);
+	return failures == 0 ? 0 : 1;
+}

test/EchoBuildingOrderTestStubs.cpp

@@ -0,0 +1,76 @@
+// SPDX-License-Identifier: GPL-3.0-or-later
+// Copyright (C) 2026 glob2 contributors
+
+// Stubs for symbols referenced by BuildingOrder.o that
+// EchoBuildingOrderSaveLoadTest does not exercise. Only save() and load() are
+// under test; find_location(), passes_conditions() and queue_gradients() live in
+// the same translation unit, so the linker still has to resolve what they call --
+// globalContainer and the building type tables, Map's placement predicate,
+// FlagMap, GradientManager, and the Constraint / Condition factories. Pulling in
+// the real definitions would drag in most of the game.
+//
+// None of these are called at runtime by the test. The Constraint / Condition
+// factories are the closest call: BuildingOrder::load() and save() do reach them,
+// but only once per element of the constraint and condition vectors, and every
+// fixture in the test uses an order with both lists empty.
+
+#include <string>
+#include "echo/Echo.h"
+#include "GlobalContainer.h"
+#include "BuildingType.h"
+#include "IntBuildingType.h"
+#include "Map.h"
+
+GlobalContainer *globalContainer = NULL;
+
+BuildingType *BuildingsTypes::getByType(const std::string &, int, bool)
+{
+	return NULL;
+}
+
+const std::string &IntBuildingType::typeFromShortNumber(int)
+{
+	static const std::string none;
+	return none;
+}
+
+bool Map::isHardSpaceForBuilding(int, int, int, int) const
+{
+	return false;
+}
+
+int AIEcho::Construction::FlagMap::get_flag(int, int)
+{
+	return 0;
+}
+
+void AIEcho::Gradients::GradientManager::queue_gradient(const AIEcho::Gradients::GradientInfo &)
+{
+}
+
+bool AIEcho::Gradients::GradientManager::is_updated(const AIEcho::Gradients::GradientInfo &)
+{
+	return false;
+}
+
+AIEcho::Conditions::Condition *AIEcho::Conditions::Condition::load_condition(
+	GAGCore::InputStream *, Player *, Sint32)
+{
+	return NULL;
+}
+
+void AIEcho::Conditions::Condition::save_condition(
+	AIEcho::Conditions::Condition *, GAGCore::OutputStream *)
+{
+}
+
+AIEcho::Construction::Constraint *AIEcho::Construction::Constraint::load_constraint(
+	GAGCore::InputStream *, Player *, Sint32)
+{
+	return NULL;
+}
+
+void AIEcho::Construction::Constraint::save_constraint(
+	AIEcho::Construction::Constraint *, GAGCore::OutputStream *)
+{
+}

test/README.md

@@ -321,6 +321,36 @@ python3 test/run-savegame-safety-tests.py --check-preferences build/src/HiringBu
 
 The harness runs headlessly in disposable profile directories in Linux and Windows CI.
 
+## Echo building-order id save compatibility
+
+`EchoBuildingOrderSaveLoadTest` covers `AIEcho::Construction::BuildingOrder::id`,
+the `BuildingRegister` key handed out at runtime by `Echo::add_building_order`.
+`save()` and `load()` never moved the field and the member had no initialiser, so
+every pending building order restored from a save carried an uninitialised heap
+value into `BuildingRegister::issue_order` and `AssignWorkers`: an AI game resumed
+from a save was not reproducible run to run, and a resumed multiplayer game could
+desync without packet loss or a version mismatch. Version 96 serialises the field.
+Older saves do not carry it and load leaves the member at `-1`, the sentinel
+`Echo::load` replaces with a fresh `register_building()` key.
+
+The fixture checks the version-96 round trip, that an unregistered order's `-1`
+survives the `Uint32` on the wire rather than returning as a huge positive key,
+and that a pre-96 stream leaves the sentinel with every following field still
+decoding from the right offset. `BuildingOrder.cpp` is linked against
+`EchoBuildingOrderTestStubs.cpp`, which satisfies the `find_location` /
+`passes_conditions` link surface (`globalContainer`, `BuildingsTypes`, `Map`,
+`FlagMap`, `GradientManager`, and the `Constraint` / `Condition` factories) that
+a constraint-free order never reaches at runtime. It needs no profile or display:
+
+```sh
+cd test
+scons -j8 EchoBuildingOrderSaveLoadTest
+./EchoBuildingOrderSaveLoadTest
+```
+
+Linux CI runs it through this directory's "Build and run the tests" step, which
+executes `./TestsRunner` and then every `./*Harness` and `./*Test` binary.
+
 ### Native main Settings redesign
 
 Build `scons -j6 release=1 settings-tests speed-tests` and run

test/SConstruct

@@ -465,3 +465,21 @@ replay_objs = [
 ]
 wcdecode_env.Program( target = 'ReplayStepCounterTest', source = replay_objs )
 
+
+# Standalone regression harness for AIEcho::Construction::BuildingOrder::id
+# serialization. The id is a BuildingRegister key handed out at runtime by
+# Echo::add_building_order, but save()/load() never moved it and the member had
+# no initialiser, so every pending building order restored from a save carried
+# an uninitialised heap value into issue_order() and AssignWorkers -- an AI game
+# resumed from a save was not reproducible, and a resumed multiplayer game could
+# desync. Checks the version-96 round trip, the -1 sentinel surviving the Uint32
+# on the wire, and the pre-96 layout. Reuses the wcdecode env's libgagserver.a
+# link surface. Distinct object name for BuildingOrder.cpp because the main
+# build compiles it with different flags.
+echo_order_objs = [
+  wcdecode_env.Object('EchoBuildingOrderSaveLoadTest.o', 'EchoBuildingOrderSaveLoadTest.cpp'),
+  wcdecode_env.Object('BuildingOrder-saveload.o', '../src/ai/echo/BuildingOrder.cpp'),
+  wcdecode_env.Object('EchoBuildingOrderTestStubs.o', 'EchoBuildingOrderTestStubs.cpp'),
+]
+wcdecode_env.Program( target = 'EchoBuildingOrderSaveLoadTest', source = echo_order_objs )
+