From c639d9b6947ab380448287c3fc47018de06b6fc9 Mon Sep 17 00:00:00 2001 From: Tim de Bruijn Date: Fri, 28 Aug 2026 22:56:34 +0200 Subject: [PATCH 1/4] Close the release gate, pin the last three deps, and stop truncating the clock Four findings from the review, all in production code rather than tests. THE RELEASE GATE HAD NO GATE. check_version.sh recognised a release tag as /^v[0-9]/ while release.yml triggers on the far wider "v*", so a tag of vtest, v-wip or a bare v left $tag empty, printed the declared version and exited 0 -- a release built and published with no version check at all. release.yml names its release ${REF_NAME#v}, so vtest would have shipped as "test". The discriminator is now the ref TYPE rather than the shape of the name, which is what it always meant: GITHUB_REF_TYPE=tag. A branch is never read as a version whatever it is called, which is the false positive the old name-shape guard existed to avoid. It also gets a --self-test, run in CI on every push rather than only at tag time -- a broken gate should surface before the release that needs it. The first version of that self-test was itself vacuous: it passed each tag as an ARGUMENT, and every case still passed with the bug reintroduced, because the argument path was never the broken one. It now drives the environment with no argument, exactly as release.yml does, and the three tags I found empirically are the three it fails on. DEPENDENCIES. ArduinoJson and espMqttClient carried carets (^7.4.3, ^1.7.3), which is not a pin: it accepts any later minor. .pio/ is gitignored and there is no lockfile, so the resolved set lived only in whatever machine built last -- and this builds an OTA image, where "the version that shipped" has to be reconstructible from the tag. Both were already resolving to the versions now named, and both are the latest release upstream, so this pins current behaviour rather than changing it. THE CLOCK. main.cpp narrowed nowMs() to uint32 on the way into breadcrumbs::tick(), forty lines below a comment warning that millis() wraps every 49.7 days and that casting a wrapped value to uint64 does not un-wrap it. The record exists to say "this life reached this uptime", so the bridges it was built for -- the long-lived ones -- were exactly the ones it misreported, and it misreported them as a plausible smaller number: a 60-day life read as 10.3 days. Storage now holds SECONDS in the same uint32, which reaches 136 years and keeps the struct at sixteen bytes, so the CRC window and the RTC-RAM layout are untouched. Nothing is lost: tick() only ever wrote once per second. EXTERNAL BEHAVIOUR CHANGE, flagged deliberately. /api/v1/diagnostics still publishes previous_uptime_ms in milliseconds, but the value is now always a whole number of seconds. Accuracy is unchanged -- it was already only sampled once per second -- but a consumer reading the low digits will see zeros where it used to see noise. The field also now carries values past 2^32, and the payload is tested for that. MQTT THROTTLE. PublishThrottle::Sample held its id as a std::string, so every publish rebuilt the whole vector: one allocation per channel, freed on the next, at least once a minute per device for the life of the bridge. measurement.h keeps ids as const char* for exactly that reason -- it records 80-120 short-lived allocations per poll as "a classic fragmentation hazard" -- and this was the one place downstream that put them back on the heap. Every id is an inline constexpr constant with static storage, so the pointer is safe to borrow. The comparison is now an explicit strcmp: it would have silently become a pointer comparison otherwise, which happens to work for interned constants and is not something to depend on. Verified: 1024 native cases, check_layering.sh, the new self-test, and a real waveshare-rs485-can build with no new warnings. --- .github/workflows/ci.yml | 7 ++++ platformio.ini | 15 +++++-- src/device/bridge_info.h | 2 +- src/diagnostics/breadcrumbs.cpp | 16 ++++---- src/diagnostics/breadcrumbs.h | 16 ++++++-- src/main.cpp | 2 +- src/outputs/mqtt/publish_policy.cpp | 3 +- src/outputs/mqtt/publish_policy.h | 12 +++++- test/test_rest/test_main.cpp | 38 +++++++++++++++--- tools/check_version.sh | 62 +++++++++++++++++++++++++++-- 10 files changed, 146 insertions(+), 27 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 2fcf98f..104d4e1 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -90,6 +90,13 @@ jobs: - name: Layering invariants run: bash tools/check_layering.sh + # The release gate had no gate of its own. It recognised a release tag as /^v[0-9]/ while + # release.yml triggers on "v*", so vtest, v-wip and a bare v all built a release with no + # version check at all. Run here, on every push, rather than only at tag time -- a broken + # gate should surface before the release that needs it. + - name: Release version gate self-test + run: bash tools/check_version.sh --self-test + - name: Embedded JS syntax run: python3 tools/check_web_js.py diff --git a/platformio.ini b/platformio.ini index 165f255..ae28a4f 100644 --- a/platformio.ini +++ b/platformio.ini @@ -122,7 +122,7 @@ lib_deps = ; the Home Assistant discovery generator are tested on the host -- which is exactly where ; a wrong value_template or device_class would otherwise stay invisible until Home ; Assistant quietly shows nothing. - bblanchon/ArduinoJson@^7.4.3 + bblanchon/ArduinoJson@7.4.3 ; --------------------------------------------------------------------------------------- ; Target firmware. One environment per supported board (the HELIOGRAPH_BOARD_* flag picks @@ -301,8 +301,17 @@ lib_deps = esp32async/AsyncTCP@3.5.0 https://github.com/ESP32Async/ESPAsyncWebServer#v3.12.0 miq19/eModbus@1.7.4 - bblanchon/ArduinoJson@^7.4.3 - bertmelis/espMqttClient@^1.7.3 + ; EXACT, like everything above it. These two carried carets (^7.4.3, ^1.7.3), which is not + ; a pin: it accepts any later minor. There is no lockfile -- .pio/ is gitignored -- so the + ; resolved set lives only in whatever machine built last, and a CI runner with a cold cache + ; could produce a different firmware from the same commit without anything saying so. That + ; matters more here than in most projects: this builds an OTA image, and "the version that + ; shipped" has to be reconstructible from the tag. + ; + ; Both were already resolving to the versions named here, and both are the latest release + ; upstream (checked 2026-08-28), so this pins the current behaviour rather than changing it. + bblanchon/ArduinoJson@7.4.3 + bertmelis/espMqttClient@1.7.3 ; The board Tim runs in production (16 MB flash, PCF85063 RTC, no relays). [env:waveshare-rs485-can] diff --git a/src/device/bridge_info.h b/src/device/bridge_info.h index d2487eb..1b5495f 100644 --- a/src/device/bridge_info.h +++ b/src/device/bridge_info.h @@ -113,7 +113,7 @@ struct BridgeInfo { /// reason is not here: it is this boot's resetReason, already above. uint32_t bootCount = 0; bool breadcrumbsCold = true; - uint32_t previousUptimeMs = 0; + uint64_t previousUptimeMs = 0; uint32_t previousFirmware = 0; /// The board this firmware is running on. Reported to Home Assistant as the bridge diff --git a/src/diagnostics/breadcrumbs.cpp b/src/diagnostics/breadcrumbs.cpp index e119f6d..f8e2bf7 100644 --- a/src/diagnostics/breadcrumbs.cpp +++ b/src/diagnostics/breadcrumbs.cpp @@ -42,26 +42,28 @@ BootRecord begin(Storage& storage, uint32_t runningFirmware) { // field says what it was running. Its death reason is this boot's // esp_reset_reason(), which the caller already exposes -- nothing to store. record.coldStart = false; - record.previousUptimeMs = storage.heartbeatUptimeMs; + record.previousUptimeMs = static_cast(storage.heartbeatUptimeSeconds) * 1000ULL; record.previousFirmware = storage.runningFirmware; record.bootCount = storage.bootCount + 1; } // Cold path and warm path converge: write this life's record. On cold, bootCount in // the record defaulted to 1. storage.bootCount = record.bootCount; - storage.heartbeatUptimeMs = 0; + storage.heartbeatUptimeSeconds = 0; storage.runningFirmware = runningFirmware; storage.crc = storageCrc(storage); return record; } -void tick(Storage& storage, uint32_t uptimeMs) { - // One write per second of uptime. The comparison also handles the first call (heartbeat - // starts at 0) and is immune to loop() pace. - if (uptimeMs < storage.heartbeatUptimeMs + 1000) { +void tick(Storage& storage, uint64_t uptimeMs) { + // One write per second of uptime, which is also why storing seconds loses nothing. Takes + // the full 64-bit clock: the caller used to narrow it to uint32 here, and casting a + // wrapped value to a wider type does not un-wrap it. + const uint32_t seconds = static_cast(uptimeMs / 1000); + if (seconds == storage.heartbeatUptimeSeconds) { return; } - storage.heartbeatUptimeMs = uptimeMs; + storage.heartbeatUptimeSeconds = seconds; storage.crc = storageCrc(storage); } diff --git a/src/diagnostics/breadcrumbs.h b/src/diagnostics/breadcrumbs.h index b0bea0a..3ac00e0 100644 --- a/src/diagnostics/breadcrumbs.h +++ b/src/diagnostics/breadcrumbs.h @@ -50,7 +50,13 @@ namespace heliograph::breadcrumbs { /// once-per-four-billion event that still only costs one fabricated boot count. struct Storage { uint32_t bootCount; - uint32_t heartbeatUptimeMs; + /// SECONDS, not milliseconds. As milliseconds this wrapped at 49.7 days and the record + /// exists precisely to describe a bridge that had been up a long time: a 60-day life + /// reported ~10 days, which reads as a plausible number rather than as an error. Seconds + /// in the same uint32 reach 136 years and keep this struct at sixteen bytes, so the CRC + /// window and the RTC layout are untouched. Nothing is lost -- tick() only writes once + /// per second, so the millisecond digits were never meaningful. + uint32_t heartbeatUptimeSeconds; /// (major<<16)|(minor<<8)|patch of the image that is running. "The previous life died /// right after an OTA" is the single most valuable thing this record can show. uint32_t runningFirmware; @@ -86,7 +92,11 @@ struct BootRecord { uint32_t bootCount = 1; /// The previous life's last heartbeat: "it had been up this long when it died". /// Meaningless on a cold start; the payload reports it absent then. - uint32_t previousUptimeMs = 0; + /// + /// Still milliseconds, and still what /api/v1/diagnostics publishes as + /// previous_uptime_ms -- the storage changed, the interface did not. 64-bit because a + /// 32-bit millisecond count is the wrap this moved away from. + uint64_t previousUptimeMs = 0; /// The image the previous life was running, same encoding as Storage::runningFirmware. uint32_t previousFirmware = 0; }; @@ -98,6 +108,6 @@ BootRecord begin(Storage& storage, uint32_t runningFirmware); /// Heartbeat: remembers how far this life got. Throttled internally to one write per second /// of uptime -- calling it every loop() pass is fine and expected. -void tick(Storage& storage, uint32_t uptimeMs); +void tick(Storage& storage, uint64_t uptimeMs); } // namespace heliograph::breadcrumbs diff --git a/src/main.cpp b/src/main.cpp index f90206c..481d200 100644 --- a/src/main.cpp +++ b/src/main.cpp @@ -1980,7 +1980,7 @@ void loop() { // Breadcrumb heartbeat: "this life reached this uptime". Throttled inside to one RTC-RAM // write per second. - breadcrumbs::tick(g_breadcrumbStore, static_cast(nowMs())); + breadcrumbs::tick(g_breadcrumbStore, nowMs()); g_wifi.loop(nowMs()); startOutputs(); // no-op until there is a network, and only ever runs once diff --git a/src/outputs/mqtt/publish_policy.cpp b/src/outputs/mqtt/publish_policy.cpp index 7abcd6e..4403630 100644 --- a/src/outputs/mqtt/publish_policy.cpp +++ b/src/outputs/mqtt/publish_policy.cpp @@ -4,6 +4,7 @@ #include #include +#include namespace heliograph::mqtt { @@ -45,7 +46,7 @@ bool PublishThrottle::shouldPublish(const DeviceState& state, uint64_t nowMs) { continue; } const auto it = std::find_if(lastPublished_.begin(), lastPublished_.end(), - [&m](const Sample& s) { return s.id == m.id; }); + [&m](const Sample& s) { return std::strcmp(s.id, m.id) == 0; }); const Sample* previous = it == lastPublished_.end() ? nullptr : &*it; if (previous == nullptr) { return true; // a channel appeared, e.g. a second MPPT was detected diff --git a/src/outputs/mqtt/publish_policy.h b/src/outputs/mqtt/publish_policy.h index e8c3f8a..ab3517f 100644 --- a/src/outputs/mqtt/publish_policy.h +++ b/src/outputs/mqtt/publish_policy.h @@ -50,7 +50,17 @@ class PublishThrottle { double deadbandFor(MeasurementType type) const; struct Sample { - std::string id; + /// Borrowed, not owned -- and safe to borrow: every measurement id is one of the + /// `inline constexpr const char*` constants in measurement.h, so they all have static + /// storage duration and outlive every throttle that points at them. + /// + /// This was a std::string, which rebuilt the whole vector on every publish: one + /// allocation per channel, freed again on the next, at least once a minute per device + /// for the life of the bridge. measurement.h keeps ids as const char* for exactly that + /// reason -- it records 80-120 short-lived allocations per poll as "a classic + /// fragmentation hazard" on a device with no defragmenting allocator -- and this was + /// the one place downstream that put them back on the heap. + const char* id; double value; bool valid; bool stale; diff --git a/test/test_rest/test_main.cpp b/test/test_rest/test_main.cpp index 14a9768..9d987ce 100644 --- a/test/test_rest/test_main.cpp +++ b/test/test_rest/test_main.cpp @@ -2091,14 +2091,15 @@ static void test_breadcrumbs_cold_then_warm() { TEST_ASSERT_EQUAL_UINT32(1, rec.bootCount); // This life runs 65 s; the heartbeat throttles to 1 Hz, so a second tick inside the - // same second must not move it. + // same second must not move it. Reported to the whole second, because that is the rate + // it was ever written at -- the millisecond digits were noise, never accuracy. breadcrumbs::tick(st, 65400); breadcrumbs::tick(st, 65900); auto rec2 = breadcrumbs::begin(st, 0x00001802); // the next boot runs 0.24.2 TEST_ASSERT_FALSE(rec2.coldStart); TEST_ASSERT_EQUAL_UINT32(2, rec2.bootCount); - TEST_ASSERT_EQUAL_UINT32(65400, rec2.previousUptimeMs); + TEST_ASSERT_EQUAL_UINT64(65000, rec2.previousUptimeMs); TEST_ASSERT_EQUAL_UINT32(0x00001801, rec2.previousFirmware); // And the record begin() left behind must itself survive a third boot: count keeps @@ -2106,10 +2107,26 @@ static void test_breadcrumbs_cold_then_warm() { auto rec3 = breadcrumbs::begin(st, 0x00001802); TEST_ASSERT_FALSE(rec3.coldStart); TEST_ASSERT_EQUAL_UINT32(3, rec3.bootCount); - TEST_ASSERT_EQUAL_UINT32(0, rec3.previousUptimeMs); // that life never ticked + TEST_ASSERT_EQUAL_UINT64(0, rec3.previousUptimeMs); // that life never ticked TEST_ASSERT_EQUAL_UINT32(0x00001802, rec3.previousFirmware); } +/// The record exists to describe a bridge that had been up a long time, and as milliseconds in +/// a uint32 it wrapped at 49.7 days -- so the lives it was built for were exactly the ones it +/// misreported, and it misreported them as a plausible smaller number rather than as an error. +static void test_a_life_past_the_32_bit_millisecond_wrap_is_recorded_whole() { + breadcrumbs::Storage st; + std::memset(&st, 0, sizeof st); + breadcrumbs::begin(st, 1); + + constexpr uint64_t kSixtyDaysMs = 60ULL * 86400ULL * 1000ULL; // 5184000000, over 2^32 + breadcrumbs::tick(st, kSixtyDaysMs); + + const auto rec = breadcrumbs::begin(st, 1); + TEST_ASSERT_FALSE(rec.coldStart); + TEST_ASSERT_EQUAL_UINT64(kSixtyDaysMs, rec.previousUptimeMs); +} + /// One flipped bit must read as cold: a torn RTC write may never become an invented past. /// This is the mutation-facing test -- remove the CRC comparison and it fails. static void test_breadcrumbs_corruption_reads_as_cold() { @@ -2117,7 +2134,7 @@ static void test_breadcrumbs_corruption_reads_as_cold() { std::memset(&st, 0, sizeof st); breadcrumbs::begin(st, 1); breadcrumbs::tick(st, 5000); - st.heartbeatUptimeMs ^= 0x4; // torn write, CRC now stale + st.heartbeatUptimeSeconds ^= 0x4; // torn write, CRC now stale auto rec = breadcrumbs::begin(st, 1); TEST_ASSERT_TRUE(rec.coldStart); @@ -2146,13 +2163,21 @@ static void test_breadcrumbs_payload_shapes() { bridge.bootCount = 4; bridge.breadcrumbsCold = false; - bridge.previousUptimeMs = 65400; + bridge.previousUptimeMs = 65000; bridge.previousFirmware = 0x00001801; TEST_ASSERT_TRUE(rest::buildDiagnosticsPayload(d.snapshot(), bridge, json)); doc = parse(json); - TEST_ASSERT_EQUAL_UINT32(65400, doc["previous_uptime_ms"].as()); + TEST_ASSERT_EQUAL_UINT64(65000, doc["previous_uptime_ms"].as()); TEST_ASSERT_EQUAL_STRING("0.24.1", doc["previous_firmware"]); + // And the payload must carry a value past 2^32 without narrowing it on the way out. The + // field is milliseconds and the storage behind it now reaches 136 years, so the JSON is + // the last place left that could still wrap it at 49.7 days. + bridge.previousUptimeMs = 60ULL * 86400ULL * 1000ULL; // 5184000000 + TEST_ASSERT_TRUE(rest::buildDiagnosticsPayload(d.snapshot(), bridge, json)); + TEST_ASSERT_EQUAL_UINT64(5184000000ULL, parse(json)["previous_uptime_ms"].as()); + bridge.previousUptimeMs = 65000; + // A version whose three components are all DIFFERENT and all nonzero. Every firmware this // project has shipped is 0.x, so every stored breadcrumb has a zero major -- which meant the // assertion above held just as well if the major were decoded from the wrong byte. Proved: @@ -3002,6 +3027,7 @@ int main(int, char**) { RUN_TEST(test_poll_duration_payload_absent_until_sampled); RUN_TEST(test_breadcrumbs_cold_then_warm); RUN_TEST(test_breadcrumbs_corruption_reads_as_cold); + RUN_TEST(test_a_life_past_the_32_bit_millisecond_wrap_is_recorded_whole); RUN_TEST(test_breadcrumbs_payload_shapes); RUN_TEST(test_diagnostics_payload_has_no_secrets); RUN_TEST(test_diagnostics_report_stack_marks_and_fragmentation); diff --git a/tools/check_version.sh b/tools/check_version.sh index c9e3602..06cd51e 100755 --- a/tools/check_version.sh +++ b/tools/check_version.sh @@ -41,16 +41,70 @@ declared="$(read_define HELIOGRAPH_VERSION_MAJOR).$(read_define HELIOGRAPH_VERSI # tried to read "main" as a version would fail every pull request for a reason that has nothing # to do with the change. Being wired into the wrong workflow should do nothing, not break it. tag="${1:-}" -if [ -z "$tag" ]; then - case "${GITHUB_REF_NAME:-}" in - v[0-9]*) tag="$GITHUB_REF_NAME" ;; - esac + +if [ "$tag" = "--self-test" ]; then + self_fail=0 + # Driven through the ENVIRONMENT and with no argument, because that is how release.yml + # invokes this (`run: bash tools/check_version.sh`). An earlier version of this self-test + # passed each tag as an argument instead -- and every case still passed with the bug + # reintroduced, because the argument path was never the broken one. The bug was that a ref + # the old guard did not recognise never reached the comparison at all. + expect() { # expect + local want="$1" rt="$2" rn="$3" got=0 + GITHUB_REF_TYPE="$rt" GITHUB_REF_NAME="$rn" "$0" >/dev/null 2>&1 || got=$? + if [ "$got" -ne "$want" ]; then + echo "check_version self-test: ${rt} '${rn}' exited ${got}, expected ${want}" >&2 + self_fail=1 + fi + } + expect 0 tag "v${declared}" + expect 0 tag "v${declared}-rc1" + # Every one of these used to exit 0 WITHOUT CHECKING ANYTHING: the old guard recognised a + # release tag as /^v[0-9]/, so these refs left $tag empty and the script printed the + # declared version and returned success -- while release.yml triggers on the far wider + # "v*" and would have built and published each of them. + expect 1 tag "vtest" + expect 1 tag "v-wip" + expect 1 tag "v" + expect 1 tag "v1.2" + expect 1 tag "v0.0.0" + # A branch must never be read as a version, whatever it is called. This is the false + # positive the old name-shape guard existed to avoid, and it still must not happen. + expect 0 branch "v-something" + expect 0 branch "main" + if [ "$self_fail" -eq 0 ]; then + echo "check_version self-test: OK" + fi + exit "$self_fail" +fi + +# A release is identified by the REF TYPE, not by the shape of the name. GITHUB_REF_NAME holds +# a branch name in ordinary CI, and the old heuristic told the two apart by reading the text -- +# which let through every tag that was not v, unchecked, while release.yml triggers on +# "v*". A tag of vtest, v-wip or a bare v built and published a release with no version check +# at all, and release.yml names the release ${GITHUB_REF_NAME#v}, so vtest shipped as "test". +if [ -z "$tag" ] && [ "${GITHUB_REF_TYPE:-}" = "tag" ]; then + tag="${GITHUB_REF_NAME:-}" fi if [ -z "$tag" ]; then echo "$declared" exit 0 fi +# The tag must BE a version, not merely begin with a v. +if ! printf '%s' "$tag" | grep -qE '^v[0-9]+\.[0-9]+\.[0-9]+(-[A-Za-z0-9.]+)?$'; then + cat >&2 < Date: Sat, 29 Aug 2026 00:19:31 +0200 Subject: [PATCH 2/4] Fall back to the name shape when there is no ref type, rather than to no check The previous commit made GITHUB_REF_TYPE=tag the sole signal that this is a release. That is correct for GitHub Actions -- release.yml runs on ubuntu-latest and its env: block adds BOARDS without displacing the defaults, so the variable is there -- but it made a variable that has never been absent into a single point of failure for the gate, and the failure mode is silence. With no ref type at all, a v-shaped name is now checked by the old heuristic instead of waved through. A branch wrongly read as a version is a loud failure on a pull request; a release that skips its version check is a silent one. Given the choice, be loud. Two more self-test cases cover it, and the whole set still fails on the original bug. --- tools/check_version.sh | 15 +++++++++++++-- 1 file changed, 13 insertions(+), 2 deletions(-) diff --git a/tools/check_version.sh b/tools/check_version.sh index 06cd51e..bfd784a 100755 --- a/tools/check_version.sh +++ b/tools/check_version.sh @@ -72,6 +72,10 @@ if [ "$tag" = "--self-test" ]; then # positive the old name-shape guard existed to avoid, and it still must not happen. expect 0 branch "v-something" expect 0 branch "main" + # And with no ref type at all -- not Actions, or a runner that stopped setting it -- a + # v-shaped name is still checked rather than waved through. + expect 1 "" "vtest" + expect 0 "" "" if [ "$self_fail" -eq 0 ]; then echo "check_version self-test: OK" fi @@ -83,8 +87,15 @@ fi # which let through every tag that was not v, unchecked, while release.yml triggers on # "v*". A tag of vtest, v-wip or a bare v built and published a release with no version check # at all, and release.yml names the release ${GITHUB_REF_NAME#v}, so vtest shipped as "test". -if [ -z "$tag" ] && [ "${GITHUB_REF_TYPE:-}" = "tag" ]; then - tag="${GITHUB_REF_NAME:-}" +if [ -z "$tag" ]; then + case "${GITHUB_REF_TYPE:-}" in + tag) tag="${GITHUB_REF_NAME:-}" ;; + # Unset means this is not a GitHub Actions run -- or a runner that stopped providing + # the variable. Fall back to the old name-shape heuristic rather than to NO CHECK: a + # branch wrongly read as a version is a loud failure on a pull request, and a release + # that skips its version check is a silent one. Given the choice, be loud. + "") case "${GITHUB_REF_NAME:-}" in v*) tag="$GITHUB_REF_NAME" ;; esac ;; + esac fi if [ -z "$tag" ]; then echo "$declared" From ab5fa27b969480e4c4339317705dd1223649a673 Mon Sep 17 00:00:00 2001 From: Tim de Bruijn Date: Sat, 29 Aug 2026 00:24:07 +0200 Subject: [PATCH 3/4] Home Assistant discovery had its own serialiser, without the size check Found while reviewing the branch that hardens the tests around json_limits::finish(). home_assistant_discovery.cpp carried a private serialise() that was finish() with the `needed > maxBytes` check taken out: it caught an overflowed document but let a well-formed one grow to whatever the heap allowed. That was the wrong path to be missing it. Discovery payloads are the LARGEST this device publishes -- every entity repeats the whole device block -- and identity strings are unbounded std::strings filled from whatever a device reports over RS485. So the one output with no ceiling was the one that emits the most bytes, built from the least trusted input. It now calls the shared finish() at kMaxPayloadBytes, the same ceiling the state payloads already use. Over that, the entity is dropped rather than published oversized, which is the convention every other payload here follows. Measured before choosing the ceiling: the largest entity this fixture produces is well under half of it, and the test asserts that headroom rather than a bare "it fits" -- the device block grows when a field is added, and a ceiling sat under by a hair would start dropping entities on the next one. The test asserts the bound through buildDiscoveryEntities rather than beside it, with a device reporting an absurd model name. That is what makes the guard testable at all: removing an upper bound is strictly MORE permissive, so a test that only checks real payloads fit cannot detect its absence. Proven -- unbounded, that fixture publishes 14 oversized entities; bounded, none. Verified: 1025 native cases, check_layering.sh, and a waveshare-rs485-can build. --- src/outputs/mqtt/home_assistant_discovery.cpp | 20 +++++---- test/test_mqtt/test_main.cpp | 42 +++++++++++++++++++ 2 files changed, 53 insertions(+), 9 deletions(-) diff --git a/src/outputs/mqtt/home_assistant_discovery.cpp b/src/outputs/mqtt/home_assistant_discovery.cpp index bb55d22..75b65ff 100644 --- a/src/outputs/mqtt/home_assistant_discovery.cpp +++ b/src/outputs/mqtt/home_assistant_discovery.cpp @@ -8,6 +8,8 @@ #include #include "device/command.h" +#include "json_limits.h" +#include "mqtt_payloads.h" #include "relays/drm.h" namespace heliograph::mqtt { @@ -185,16 +187,16 @@ void addDeviceBlock(JsonObject entity, const BridgeInfo& bridge, const DeviceIde device["via_device"] = bridge.bridgeId; } +// Was a private copy of json_limits::finish() with the size check taken out: it caught an +// overflowed document but let a well-formed one grow to whatever the heap allowed. Every other +// payload on this device is bounded, and discovery payloads are the LARGEST ones -- each +// carries the full device block -- so this was the one path without the guard, on the output +// that publishes the most bytes. +// +// Now the shared one, at the same ceiling the state payloads use. One point of truth: a change +// to how a payload is bounded should not need finding in two places. bool serialise(const JsonDocument& doc, std::string& out) { - if (doc.overflowed()) { - return false; - } - const size_t needed = measureJson(doc); - std::string buffer; - buffer.resize(needed + 1); - buffer.resize(serializeJson(doc, buffer.data(), buffer.size())); - out = std::move(buffer); - return true; + return json_limits::finish(doc, out, kMaxPayloadBytes); } } // namespace diff --git a/test/test_mqtt/test_main.cpp b/test/test_mqtt/test_main.cpp index fb525e6..b1fa898 100644 --- a/test/test_mqtt/test_main.cpp +++ b/test/test_mqtt/test_main.cpp @@ -435,6 +435,47 @@ static void test_writable_driver_lists_its_bounds() { // --- Home Assistant discovery -------------------------------------------------------------- +// Discovery payloads are the largest this device publishes -- every entity repeats the whole +// device block -- and they were the one path that went out unbounded, through a private copy of +// json_limits::finish() with the size check removed. They share the state payloads' ceiling now. +// +// Asserted BOTH ways, because either alone is worthless: real payloads must still be published +// (a ceiling that refuses them breaks Home Assistant discovery outright and silently), and the +// ceiling must actually refuse something (or it is decoration). +static void test_every_discovery_payload_fits_the_ceiling_it_now_shares() { + Rig r; + const auto state = r.poll(); + const auto bridge = makeBridge(); + const MqttTopics topics(kDefaultBaseTopic, bridge.bridgeId); + const auto entities = buildDiscoveryEntities(state, bridge, topics, topics.availability(), + kDefaultDiscoveryPrefix, bridge.bridgeId); + + TEST_ASSERT_TRUE_MESSAGE(entities.size() > 3, "the fixture must produce real entities"); + size_t largest = 0; + for (const auto& e : entities) { + TEST_ASSERT_FALSE_MESSAGE(e.payload.empty(), "an entity was serialised to nothing"); + largest = std::max(largest, e.payload.size()); + } + // Room to spare, not "just fits": the device block grows when a field is added, and a + // ceiling this sits under by a hair would start dropping entities on the next one. + TEST_ASSERT_LESS_THAN_UINT32(kMaxPayloadBytes / 2, largest); + + // AND THE BOUND BITES, through the real builder rather than beside it. Identity strings + // are unbounded std::strings filled from whatever a device reports, and every entity + // repeats the device block -- so a device with an absurd model name is the reachable way + // over the ceiling. Asserting this through buildDiscoveryEntities is what makes the guard + // testable at all: removing an upper bound is strictly MORE permissive, so a test that + // only checks real payloads fit cannot detect its absence. + // + // Over the ceiling the entity is dropped rather than published oversized. That is the + // convention every other payload on this device already follows. + DeviceState huge = r.poll(); + huge.identity.model = std::string(kMaxPayloadBytes, 'M'); + const auto dropped = buildDiscoveryEntities(huge, bridge, topics, topics.availability(), + kDefaultDiscoveryPrefix, bridge.bridgeId); + TEST_ASSERT_EQUAL_UINT32(0, dropped.size()); +} + static void test_discovery_creates_an_entity_per_supported_measurement() { Rig r; const auto state = r.poll(); @@ -1626,6 +1667,7 @@ int main(int, char**) { RUN_TEST(test_capabilities_payload_reports_read_only); RUN_TEST(test_writable_driver_lists_its_bounds); RUN_TEST(test_discovery_creates_an_entity_per_supported_measurement); + RUN_TEST(test_every_discovery_payload_fits_the_ceiling_it_now_shares); RUN_TEST(test_discovery_metadata_matches_the_measurement_type); RUN_TEST(test_value_template_reads_the_right_key); RUN_TEST(test_availability_tracks_the_bridge_not_the_inverter); From 0ee303de54e5a61d1bab5346105cc4eb9bd6a908 Mon Sep 17 00:00:00 2001 From: Tim de Bruijn Date: Sat, 29 Aug 2026 00:43:22 +0200 Subject: [PATCH 4/4] A schema tag, because renaming a field changed what its bytes mean Review found the upgrade path, and it is the worst kind of defect: silent, fleet-wide, and introduced by the fix two commits back. Renaming heartbeatUptimeMs to heartbeatUptimeSeconds changed a field's UNIT without changing the layout. The CRC covers the same twelve bytes it always did, so a record left in RTC RAM by the previous firmware still validates -- and is then read as seconds. RTC_NOINIT survives exactly the reset an OTA performs, so on the first boot into that firmware a bridge that had genuinely been up three days would have published three thousand. Not a plausible-looking error: a thousandfold one, on every device in the fleet, through /api/v1/diagnostics. It self-corrects on the following boot, which is what would have made it hard to believe when someone reported it. storageCrc() now folds a schema tag into the checksum. It is not stored -- it costs no RTC bytes -- and a record written under a different schema simply fails validation and reads as cold. The price is one cold start per device: the boot count restarts and the previous life is reported as unknown, which is the honest answer, because those bytes genuinely cannot be interpreted. Bump the tag whenever a field's meaning, unit or width changes. crc32() becomes crc32Update(), carrying the running value without the final inversion, so the checksum can span the tag and the struct. Also restored a property the same commit had quietly dropped. The old millisecond throttle compared with <, so it could never store a value smaller than the one already there; rewriting it as == gave that up. esp_timer_get_time() does not run backwards, so nothing in the firmware reaches it -- but tick() takes the clock as an argument, so the property can be asserted rather than merely asserted about, and now is. Both are mutation-proven: removing the schema tag fails the upgrade test, and == in place of <= fails the monotonic one. And a comment corrected: Sample::id said every id is one of measurement.h's constants. A few tests pass bare string literals instead. Both have static storage duration so the lifetime argument is unaffected, but the claim as written was not true. Verified: 1027 native cases, check_layering.sh, waveshare-rs485-can builds clean. --- src/diagnostics/breadcrumbs.cpp | 34 +++++++++++++++--- src/outputs/mqtt/publish_policy.h | 8 +++-- test/test_rest/test_main.cpp | 60 +++++++++++++++++++++++++++++++ 3 files changed, 94 insertions(+), 8 deletions(-) diff --git a/src/diagnostics/breadcrumbs.cpp b/src/diagnostics/breadcrumbs.cpp index f8e2bf7..010349c 100644 --- a/src/diagnostics/breadcrumbs.cpp +++ b/src/diagnostics/breadcrumbs.cpp @@ -15,21 +15,41 @@ namespace heliograph::breadcrumbs { namespace { +/// What the twelve checksummed bytes MEAN. Not stored -- it is folded into the CRC, so it +/// costs no RTC bytes and a record written under a different schema simply fails validation. +/// +/// This exists because renaming heartbeatUptimeMs to heartbeatUptimeSeconds changed a field's +/// UNIT without changing the layout, so an old record still CRC'd correctly and was read back +/// as warm: a bridge that had been up three days reported three thousand. That is not a +/// plausible-looking error, it is a thousandfold one, and it would have happened once on every +/// device in the fleet at its first update into that firmware. +/// +/// Bump this whenever a field's meaning, unit or width changes. The cost of bumping is one +/// cold start -- the boot count restarts and the previous life is reported as unknown, which +/// is the honest answer, because the bytes genuinely cannot be interpreted. +inline constexpr uint32_t kStorageSchema = 2; + /// Plain CRC32 (reflected, poly 0xEDB88320), byte at a time, no table. Twelve bytes once a /// second does not justify 1 KB of lookup table in a build where RAM is the scarce resource. -uint32_t crc32(const uint8_t* data, size_t len) { - uint32_t crc = 0xFFFFFFFFu; +/// +/// Takes and returns the RUNNING value, without the final inversion, so a checksum can span +/// more than one buffer -- the schema tag and the struct are two. +uint32_t crc32Update(uint32_t crc, const uint8_t* data, size_t len) { for (size_t i = 0; i < len; ++i) { crc ^= data[i]; for (int b = 0; b < 8; ++b) { crc = (crc >> 1) ^ (0xEDB88320u & (0u - (crc & 1u))); } } - return crc ^ 0xFFFFFFFFu; + return crc; } uint32_t storageCrc(const Storage& s) { - return crc32(reinterpret_cast(&s), offsetof(Storage, crc)); + uint32_t crc = 0xFFFFFFFFu; + crc = crc32Update(crc, reinterpret_cast(&kStorageSchema), + sizeof kStorageSchema); + crc = crc32Update(crc, reinterpret_cast(&s), offsetof(Storage, crc)); + return crc ^ 0xFFFFFFFFu; } } // namespace @@ -60,7 +80,11 @@ void tick(Storage& storage, uint64_t uptimeMs) { // the full 64-bit clock: the caller used to narrow it to uint32 here, and casting a // wrapped value to a wider type does not un-wrap it. const uint32_t seconds = static_cast(uptimeMs / 1000); - if (seconds == storage.heartbeatUptimeSeconds) { + // <=, not !=. The old millisecond comparison was implicitly monotonic -- it could never + // write a value smaller than the one stored -- and an equality check quietly gave that up. + // esp_timer_get_time() does not run backwards, so nothing reaches this today; the property + // is worth keeping anyway, because the field's whole meaning is "how far this life got". + if (seconds <= storage.heartbeatUptimeSeconds) { return; } storage.heartbeatUptimeSeconds = seconds; diff --git a/src/outputs/mqtt/publish_policy.h b/src/outputs/mqtt/publish_policy.h index ab3517f..fb309a3 100644 --- a/src/outputs/mqtt/publish_policy.h +++ b/src/outputs/mqtt/publish_policy.h @@ -50,9 +50,11 @@ class PublishThrottle { double deadbandFor(MeasurementType type) const; struct Sample { - /// Borrowed, not owned -- and safe to borrow: every measurement id is one of the - /// `inline constexpr const char*` constants in measurement.h, so they all have static - /// storage duration and outlive every throttle that points at them. + /// Borrowed, not owned -- and safe to borrow: an id is either one of the + /// `inline constexpr const char*` constants in measurement.h or, in a few tests, a + /// bare string literal. Both have static storage duration, so both outlive every + /// throttle that points at them. What would NOT be safe is an id built at runtime, + /// which nothing does and measurement.h's own contract forbids. /// /// This was a std::string, which rebuilt the whole vector on every publish: one /// allocation per channel, freed again on the next, at least once a minute per device diff --git a/test/test_rest/test_main.cpp b/test/test_rest/test_main.cpp index 9d987ce..09de128 100644 --- a/test/test_rest/test_main.cpp +++ b/test/test_rest/test_main.cpp @@ -5,6 +5,7 @@ #include #include +#include #include #include @@ -2127,6 +2128,63 @@ static void test_a_life_past_the_32_bit_millisecond_wrap_is_recorded_whole() { TEST_ASSERT_EQUAL_UINT64(kSixtyDaysMs, rec.previousUptimeMs); } +/// "How far this life got" can only go up. The millisecond comparison this replaced was +/// implicitly monotonic -- it could never store a value smaller than the one already there -- +/// and rewriting it as an equality check quietly gave that up. esp_timer_get_time() does not +/// run backwards, so nothing in the firmware reaches this; tick() takes the clock as an +/// argument, so the property can be asserted rather than merely asserted ABOUT. +static void test_the_heartbeat_never_moves_backwards() { + breadcrumbs::Storage st; + std::memset(&st, 0, sizeof st); + breadcrumbs::begin(st, 1); + + breadcrumbs::tick(st, 9000); + breadcrumbs::tick(st, 3000); // a clock that went back must not rewrite the high-water mark + + const auto rec = breadcrumbs::begin(st, 1); + TEST_ASSERT_FALSE(rec.coldStart); + TEST_ASSERT_EQUAL_UINT64(9000, rec.previousUptimeMs); +} + +/// The checksum firmware BEFORE the seconds change would have written: the same twelve bytes, +/// with no schema tag folded in. Spelled out rather than calling the current helper, because it +/// is the OTHER system's checksum -- if ours changes again, this must NOT follow it. +static uint32_t legacyStorageCrc(const breadcrumbs::Storage& st) { + const auto* p = reinterpret_cast(&st); + uint32_t crc = 0xFFFFFFFFu; + for (size_t i = 0; i < offsetof(breadcrumbs::Storage, crc); ++i) { + crc ^= p[i]; + for (int b = 0; b < 8; ++b) { + crc = (crc >> 1) ^ (0xEDB88320u & (0u - (crc & 1u))); + } + } + return crc ^ 0xFFFFFFFFu; +} + +/// Renaming heartbeatUptimeMs to heartbeatUptimeSeconds changed a field's UNIT without changing +/// the layout, so a record left in RTC RAM by the previous firmware still checksummed correctly +/// and was read back as warm -- multiplied by a thousand. RTC_NOINIT survives exactly the reset +/// an OTA performs, so this would have happened once on every device in the fleet, and three +/// days of uptime would have been published as three thousand. +/// +/// Folding the schema tag into the CRC makes such a record fail validation instead. One cold +/// start is the price, and it is the honest answer: those bytes cannot be interpreted. +static void test_a_record_written_under_the_millisecond_schema_reads_as_cold() { + breadcrumbs::Storage st; + std::memset(&st, 0, sizeof st); + st.bootCount = 7; + st.runningFirmware = 0x00001801; + st.heartbeatUptimeSeconds = 259200000; // three days, written by firmware that meant ms + st.crc = legacyStorageCrc(st); + + const auto rec = breadcrumbs::begin(st, 0x00001802); + + TEST_ASSERT_TRUE_MESSAGE(rec.coldStart, + "a record whose fields mean something else is not a past"); + TEST_ASSERT_EQUAL_UINT64(0, rec.previousUptimeMs); // not 259200000000 + TEST_ASSERT_EQUAL_UINT32(1, rec.bootCount); +} + /// One flipped bit must read as cold: a torn RTC write may never become an invented past. /// This is the mutation-facing test -- remove the CRC comparison and it fails. static void test_breadcrumbs_corruption_reads_as_cold() { @@ -3028,6 +3086,8 @@ int main(int, char**) { RUN_TEST(test_breadcrumbs_cold_then_warm); RUN_TEST(test_breadcrumbs_corruption_reads_as_cold); RUN_TEST(test_a_life_past_the_32_bit_millisecond_wrap_is_recorded_whole); + RUN_TEST(test_a_record_written_under_the_millisecond_schema_reads_as_cold); + RUN_TEST(test_the_heartbeat_never_moves_backwards); RUN_TEST(test_breadcrumbs_payload_shapes); RUN_TEST(test_diagnostics_payload_has_no_secrets); RUN_TEST(test_diagnostics_report_stack_marks_and_fragmentation);