From e3765a517671495ec871ebfb68c8c8f8148c84d6 Mon Sep 17 00:00:00 2001 From: Raphael Maenle Date: Tue, 9 Jun 2026 14:53:05 +0200 Subject: [PATCH] persistence: multi-version reads across every store MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ProcessJobStore and SubstrateStore already implemented the loader-accepts-any-version-in-[1, kVersion] pattern. The other five stores (ControlJobStore, CarrierStore, LoadPortStore, ExceptionStore, SpoolStore) used strict `header[1] != kVersion` rejection, meaning a future kVersion bump there would silently nuke every persisted record on first replay. That's a footgun the test_persistence_upgrade test already flagged as a tripwire. This commit flips the strict checks to `< 1 || > kVersion`, mirroring PJ + Substrate. No format change (kVersion stays at 1 across the five stores), but: - Future v2 of any store now Just Works: add fields at the end of write_record_, bump kVersion to 2, gate the new reads behind `if (version >= 2)`. Old v1 records on disk continue to replay with the new fields defaulted. - Future versions beyond kVersion still get rejected (downgrade protection — older code can't try to decode trailers it doesn't understand). Comment blocks on each kVersion declaration now describe the upgrade discipline so the next contributor doesn't reinvent it. Test additions: - Positive test that v1 ControlJob records load on current code (will continue to pass when kVersion bumps to 2, proving v1 is still readable) - ExceptionStore rejects a v9 (future) record, matching CJ + Carrier - The existing tripwire tests get retitled from "rejects unknown version" to "rejects a future version" to reflect the new contract README §6 gets honest: every store is now multi-version-aware, not just PJ + Substrate. Co-Authored-By: Claude Opus 4.7 --- README.md | 23 +++++----- include/secsgem/gem/store/carriers.hpp | 21 ++++++--- include/secsgem/gem/store/control_jobs.hpp | 15 +++++-- include/secsgem/gem/store/exceptions.hpp | 10 +++-- include/secsgem/gem/store/spool.hpp | 10 +++-- tests/test_persistence_upgrade.cpp | 50 +++++++++++++++++++--- 6 files changed, 98 insertions(+), 31 deletions(-) diff --git a/README.md b/README.md index 5f36274..ed2f896 100644 --- a/README.md +++ b/README.md @@ -323,17 +323,18 @@ new dictionary loads. Code changes do require rebuild + restart. - **Code upgrades**: deploy to a canary tool first; bake-test for at least a full wafer batch before fleet-wide rollout. - **Schema migrations**: persistence records carry a 1-byte version - stamp after the magic byte. `ProcessJobStore` and `SubstrateStore` - currently implement multi-version reads: code at kVersion=2 still - loads v1 records (the v2 trailer fields default to empty). The - remaining stores (`ControlJobStore`, `CarrierStore`, `LoadPortStore`, - `ExceptionStore`, `SpoolStore`) use strict version equality — a - future kVersion bump there requires adding a parser for the prior - version at the same time, otherwise replay will reject old records. - Tests in `tests/test_persistence_upgrade.cpp` lock down both - contracts and act as a tripwire if a kVersion bumps silently. - Always test the upgrade with a real on-disk journal before fleet - rollout. + stamp after the magic byte. Every store (`ProcessJobStore`, + `SubstrateStore`, `ControlJobStore`, `CarrierStore`, `LoadPortStore`, + `ExceptionStore`, `SpoolStore`) accepts any version in + `[1, kVersion]`: code at kVersion=2 loads both v1 and v2 records + (v1 trailer fields default to empty). Future versions beyond + `kVersion` are rejected so a downgrade can't silently corrupt + data. Upgrade discipline: when adding fields, bump `kVersion` and + gate the new trailer behind `if (version >= N)` in the loader. + Tests in `tests/test_persistence_upgrade.cpp` lock down the + contract and act as a tripwire if a writer bumps `kVersion` + without teaching the loader to handle prior versions. Always + test the upgrade with a real on-disk journal before fleet rollout. ## 7. Integration with the fab stack diff --git a/include/secsgem/gem/store/carriers.hpp b/include/secsgem/gem/store/carriers.hpp index 6a8f96d..375ad72 100644 --- a/include/secsgem/gem/store/carriers.hpp +++ b/include/secsgem/gem/store/carriers.hpp @@ -184,11 +184,15 @@ class CarrierStore { CarrierAccessStatus acc_state; }; - // Carrier record layout, big-endian throughout: - // [u8 magic = 0xC4][u8 version = 1] + // Carrier record (v1), big-endian throughout: + // [u8 magic = 0xC4][u8 version] // [u8 id_state][u8 sm_state][u8 acc_state][u8 port_id] // [u16 slot_count][slot_count × u8 slot.state] // [u16 cid_len][cid_len × u8 carrierid] + // + // Upgrade discipline (mirrors PJ/Substrate): loader accepts any + // version in [1, kVersion]; future field additions go at the end + // and are gated behind `if (version >= N)` blocks. static constexpr uint8_t kMagic = 0xC4; static constexpr uint8_t kVersion = 0x01; @@ -266,7 +270,8 @@ class CarrierStore { if (!in) return std::nullopt; uint8_t header[6]; in.read(reinterpret_cast(header), sizeof(header)); - if (!in || header[0] != kMagic || header[1] != kVersion) return std::nullopt; + if (!in || header[0] != kMagic || + header[1] < 1 || header[1] > kVersion) return std::nullopt; Record r; r.id_state = static_cast(header[2]); r.sm_state = static_cast(header[3]); @@ -418,10 +423,13 @@ class LoadPortStore { LoadPortAssociationStatus as_state; }; - // Load-port record: - // [u8 magic = 0xC5][u8 version = 1] + // Load-port record (v1): + // [u8 magic = 0xC5][u8 version] // [u8 port_id][u8 tx_state][u8 rs_state][u8 as_state] // [u16 cid_len][cid_len × u8 carrierid] + // + // Upgrade discipline: loader accepts any version in [1, kVersion]; + // future fields append at the end behind an `if (version >= N)` gate. static constexpr uint8_t kMagic = 0xC5; static constexpr uint8_t kVersion = 0x01; @@ -492,7 +500,8 @@ class LoadPortStore { if (!in) return std::nullopt; uint8_t header[6]; in.read(reinterpret_cast(header), sizeof(header)); - if (!in || header[0] != kMagic || header[1] != kVersion) return std::nullopt; + if (!in || header[0] != kMagic || + header[1] < 1 || header[1] > kVersion) return std::nullopt; Record r; r.port_id = header[2]; r.tx_state = static_cast(header[3]); diff --git a/include/secsgem/gem/store/control_jobs.hpp b/include/secsgem/gem/store/control_jobs.hpp index 2661d11..6ff849b 100644 --- a/include/secsgem/gem/store/control_jobs.hpp +++ b/include/secsgem/gem/store/control_jobs.hpp @@ -160,10 +160,18 @@ class ControlJobStore { ControlJobState state; }; - // CJ record: - // [u8 magic = 0xC8][u8 version = 1][u8 state] + // CJ record (v1): + // [u8 magic = 0xC8][u8 version][u8 state] // [u16 ctljobid_len][ctljobid_bytes] // [u16 pj_count][repeat: u16 len + bytes] + // + // Upgrade discipline: when adding fields, bump kVersion and append + // the new fields at the end of the on-disk record. The loader + // accepts any version in [1, kVersion]; older records replay with + // the trailing fields defaulted. Future versions read by this + // binary surface as a rejection (so a downgrade can't silently + // corrupt data), but the same code that bumped kVersion to N must + // also gate the new trailer behind `if (version >= N)`. static constexpr uint8_t kMagic = 0xC8; static constexpr uint8_t kVersion = 0x01; @@ -223,7 +231,8 @@ class ControlJobStore { if (!in) return std::nullopt; uint8_t header[3]; in.read(reinterpret_cast(header), sizeof(header)); - if (!in || header[0] != kMagic || header[1] != kVersion) return std::nullopt; + if (!in || header[0] != kMagic || + header[1] < 1 || header[1] > kVersion) return std::nullopt; Record r; r.state = static_cast(header[2]); auto read_str = [&](std::string& out) { diff --git a/include/secsgem/gem/store/exceptions.hpp b/include/secsgem/gem/store/exceptions.hpp index bc8cd12..d72eccd 100644 --- a/include/secsgem/gem/store/exceptions.hpp +++ b/include/secsgem/gem/store/exceptions.hpp @@ -188,12 +188,15 @@ class ExceptionStore { ExceptionState state; }; - // Exception record: - // [u8 magic = 0xC9][u8 version = 1][u8 state] + // Exception record (v1): + // [u8 magic = 0xC9][u8 version][u8 state] // [u32 exid, big-endian] // [u16 extype_len][extype] // [u16 exmessage_len][exmessage] // [u16 recvra_count][repeat: u16 len + bytes] + // + // Upgrade discipline: loader accepts any version in [1, kVersion]; + // future fields append behind an `if (version >= N)` gate. static constexpr uint8_t kMagic = 0xC9; static constexpr uint8_t kVersion = 0x01; @@ -257,7 +260,8 @@ class ExceptionStore { if (!in) return std::nullopt; uint8_t header[3]; in.read(reinterpret_cast(header), sizeof(header)); - if (!in || header[0] != kMagic || header[1] != kVersion) return std::nullopt; + if (!in || header[0] != kMagic || + header[1] < 1 || header[1] > kVersion) return std::nullopt; Record r; r.state = static_cast(header[2]); uint8_t eb[4]; diff --git a/include/secsgem/gem/store/spool.hpp b/include/secsgem/gem/store/spool.hpp index d2d336f..3fb6c7e 100644 --- a/include/secsgem/gem/store/spool.hpp +++ b/include/secsgem/gem/store/spool.hpp @@ -189,14 +189,17 @@ class SpoolStore { std::filesystem::path path; // empty when persistence is disabled }; - // Journal record layout (little-endian-free; all-bytes are explicit): + // Journal record (v1) layout (little-endian-free; all bytes explicit): // [u8 magic = 0xE5] - // [u8 version = 1] + // [u8 version] // [u8 stream] // [u8 function] // [u8 reply_expected] // [u32 body_length, big-endian] // [body_length bytes] + // + // Upgrade discipline: loader accepts any version in [1, kVersion]; + // future fields append behind an `if (version >= N)` gate. static constexpr uint8_t kMagic = 0xE5; static constexpr uint8_t kVersion = 0x01; @@ -241,7 +244,8 @@ class SpoolStore { if (!in) return std::nullopt; uint8_t hdr[5]; in.read(reinterpret_cast(hdr), sizeof(hdr)); - if (!in || hdr[0] != kMagic || hdr[1] != kVersion) return std::nullopt; + if (!in || hdr[0] != kMagic || + hdr[1] < 1 || hdr[1] > kVersion) return std::nullopt; uint8_t blen_be[4]; in.read(reinterpret_cast(blen_be), sizeof(blen_be)); if (!in) return std::nullopt; diff --git a/tests/test_persistence_upgrade.cpp b/tests/test_persistence_upgrade.cpp index 9292cdb..0596faa 100644 --- a/tests/test_persistence_upgrade.cpp +++ b/tests/test_persistence_upgrade.cpp @@ -184,11 +184,12 @@ TEST_CASE("Persistence upgrade: Substrate v1 record loads, history empty") { fs::remove_all(dir); } -// --- Strict-version stores: current behaviour pinned, future bumps -// --- will need to flip these. These tests serve as a tripwire so a -// --- silent kVersion bump in CJ/Carrier/Exception/Spool surfaces. +// --- Multi-version-aware stores: all stores now accept v in [1, kVersion] +// --- following the PJ/Substrate pattern. Future kVersion bumps need +// --- to append fields behind `if (version >= N)` gates; the loaders +// --- already accept the prior versions. These tests pin the contract. -TEST_CASE("Persistence upgrade: ControlJobStore rejects an unknown version") { +TEST_CASE("Persistence upgrade: ControlJobStore rejects a future version") { auto dir = scratch_dir("cj-future"); std::vector rec; rec.push_back(0xC8); @@ -204,7 +205,7 @@ TEST_CASE("Persistence upgrade: ControlJobStore rejects an unknown version") { fs::remove_all(dir); } -TEST_CASE("Persistence upgrade: CarrierStore rejects an unknown version") { +TEST_CASE("Persistence upgrade: CarrierStore rejects a future version") { auto dir = scratch_dir("car-future"); // Carrier records start with magic = 0xC4 — confirm the version // gate kicks in even when magic matches. @@ -221,3 +222,42 @@ TEST_CASE("Persistence upgrade: CarrierStore rejects an unknown version") { CHECK(s.size() == 0); fs::remove_all(dir); } + +TEST_CASE("Persistence upgrade: ControlJobStore loads v1 record") { + // Positive case: v1 record matches the current schema exactly, but + // proves the loader's range check (1 <= version <= kVersion) lets + // the actual current version through. When kVersion bumps to 2, + // *this* test continues to assert that v1 still replays. + auto dir = scratch_dir("cj-v1"); + std::vector rec; + rec.push_back(0xC8); + rec.push_back(0x01); + rec.push_back(static_cast(ControlJobState::Queued)); + put_str(rec, "CJ-V1"); + be16(rec, 1); + put_str(rec, "PJ-V1"); + write_file(dir / "0000000001.cj", rec); + + ControlJobStore s; + s.enable_persistence(dir); + REQUIRE(s.has("CJ-V1")); + auto* cj = s.get("CJ-V1"); + REQUIRE(cj); + CHECK(cj->prjobids == std::vector{"PJ-V1"}); + CHECK(cj->fsm->state() == ControlJobState::Queued); + fs::remove_all(dir); +} + +TEST_CASE("Persistence upgrade: ExceptionStore rejects a future version") { + auto dir = scratch_dir("ex-future"); + std::vector rec; + rec.push_back(0xC9); + rec.push_back(0x09); + for (int i = 0; i < 64; ++i) rec.push_back(0); + write_file(dir / "0000000001.ex", rec); + + ExceptionStore s; + s.enable_persistence(dir); + CHECK(s.size() == 0); + fs::remove_all(dir); +}