diff --git a/src/ReplayReader.h b/src/ReplayReader.h index ce358faa2..c0332d926 100644 --- a/src/ReplayReader.h +++ b/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. diff --git a/src/Version.h b/src/Version.h index a48e034bb..4f513f95d 100644 --- a/src/Version.h +++ b/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 diff --git a/src/ai/echo/BuildingOrder.cpp b/src/ai/echo/BuildingOrder.cpp index 98ab517f7..f3cb8f714 100644 --- a/src/ai/echo/BuildingOrder.cpp +++ b/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(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(id), "id"); stream->writeEnterSection("constraints"); stream->writeUint32(constraints.size(), "size"); diff --git a/src/ai/echo/Construction.h b/src/ai/echo/Construction.h index 85a76ef27..35c6014e2 100644 --- a/src/ai/echo/Construction.h +++ b/src/ai/echo/Construction.h @@ -14,6 +14,8 @@ #include #include +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 > constraints; std::vector > conditions; }; diff --git a/src/ai/echo/EchoSerialization.cpp b/src/ai/echo/EchoSerialization.cpp index 128b1e155..64b29d8e6 100644 --- a/src/ai/echo/EchoSerialization.cpp +++ b/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(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(br.register_building()); stream->readLeaveSection(); } stream->readLeaveSection(); diff --git a/test/EchoBuildingOrderSaveLoadTest.cpp b/test/EchoBuildingOrderSaveLoadTest.cpp new file mode 100644 index 000000000..0927ba15b --- /dev/null +++ b/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 +#include +#include +#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 makeInputStream(const MemoryStreamBackend& written) + { + // BinaryInputStream takes ownership of the backend. + MemoryStreamBackend* copy = new MemoryStreamBackend(written); + copy->seekFromStart(0); + return std::make_unique(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; +} diff --git a/test/EchoBuildingOrderTestStubs.cpp b/test/EchoBuildingOrderTestStubs.cpp new file mode 100644 index 000000000..8f5acf435 --- /dev/null +++ b/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 +#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 *) +{ +} diff --git a/test/README.md b/test/README.md index 88a44aeaa..4b8a8de00 100644 --- a/test/README.md +++ b/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 diff --git a/test/SConstruct b/test/SConstruct index 1f1abb920..ecbf590b3 100644 --- a/test/SConstruct +++ b/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 ) +