diff --git a/.github/workflows/check-mqtt-preset-parity.yml b/.github/workflows/check-mqtt-preset-parity.yml index e1039a40..7b7d663d 100644 --- a/.github/workflows/check-mqtt-preset-parity.yml +++ b/.github/workflows/check-mqtt-preset-parity.yml @@ -1,7 +1,11 @@ name: Check MQTT Preset Name Parity -# Ensures observer-firmware and observer-firmware-dev share the same built-in -# MQTT preset *names* (config details may differ). See scripts/check_mqtt_preset_parity.py. +# Ensures observer-firmware and observer-firmware-dev ship an identical +# src/helpers/MQTTPresets.h. A slot's preset is stored in /mqtt.json by name, and +# firmware that does not know a name repairs it to "none" and writes the file +# back, so a node that rolls back between channels loses that slot for good. +# Behaviour that differs per channel belongs in MQTTPresetPolicy.h, which is not +# compared. See scripts/check_mqtt_preset_parity.py. permissions: contents: read @@ -81,9 +85,9 @@ jobs: - name: Self-test checker run: python3 scripts/check_mqtt_preset_parity.py --self-test - - name: Compare preset names + - name: Compare preset tables run: | - python3 scripts/check_mqtt_preset_parity.py \ + python3 scripts/check_mqtt_preset_parity.py --exact \ /tmp/preset-parity/prod.h \ /tmp/preset-parity/dev.h \ --label-a observer-firmware \ diff --git a/scripts/check_mqtt_preset_parity.py b/scripts/check_mqtt_preset_parity.py index d213a258..039bc6ee 100755 --- a/scripts/check_mqtt_preset_parity.py +++ b/scripts/check_mqtt_preset_parity.py @@ -1,13 +1,25 @@ #!/usr/bin/env python3 -"""Compare MQTT built-in preset *names* between two MQTTPresets.h files. +"""Compare the MQTT broker preset table between two MQTTPresets.h files. -Only the first string field of each ``MQTT_PRESETS`` entry is compared (set -equality, case-sensitive). URL, auth, CA, keepalive, and credentials are -ignored so channel branches may diverge on config details without failing CI. +With ``--exact`` (what CI uses) the two files must be byte-identical. The table +is fleet state, not just a build detail: a slot's preset is stored in +/mqtt.json by NAME, and firmware that does not know a name does not merely +ignore it — MQTTPrefsSerializer repairs it to "none" and the repaired file is +written back to flash. A node that rolls back from one channel to the other +therefore loses that slot permanently. Byte equality also catches URL, CA and +credential drift, which a name-only check passes silently even though the two +channels are meant to dial the same brokers. + +Channel-specific behaviour belongs in MQTTPresetPolicy.h, which is not compared, +so this file can stay identical while the channels differ elsewhere. + +Without ``--exact`` only the first string field of each ``MQTT_PRESETS`` entry +is compared (set equality, case-sensitive). That is the older, weaker check, +kept for ad-hoc use. Usage:: - python3 scripts/check_mqtt_preset_parity.py FILE_A FILE_B \\ + python3 scripts/check_mqtt_preset_parity.py FILE_A FILE_B --exact \\ --label-a observer-firmware --label-b observer-firmware-dev python3 scripts/check_mqtt_preset_parity.py --self-test @@ -137,6 +149,49 @@ def compare( ] +def compare_exact( + path_a: Path, + path_b: Path, + *, + label_a: str, + label_b: str, +) -> list[str]: + """Return error lines if the two files differ byte for byte.""" + + data_a = path_a.read_bytes() + data_b = path_b.read_bytes() + if data_a == data_b: + return [] + + import difflib + + errors = [f"{label_a} and {label_b} do not match byte for byte."] + try: + diff = list( + difflib.unified_diff( + data_a.decode("utf-8").splitlines(), + data_b.decode("utf-8").splitlines(), + fromfile=label_a, + tofile=label_b, + lineterm="", + n=1, + ) + ) + except UnicodeDecodeError: + errors.append("(binary difference; cannot render a text diff)") + return errors + + # Enough to identify the drift without pasting the whole table into a log. + errors.extend(diff[:60]) + if len(diff) > 60: + errors.append(f"... {len(diff) - 60} more diff line(s)") + errors.append( + "Preset rows must be identical on both channels. Behaviour that differs " + "per channel belongs in MQTTPresetPolicy.h." + ) + return errors + + def load_names(path: Path) -> set[str]: text = path.read_text(encoding="utf-8") names, _ = parse_presets(text, source=str(path)) @@ -237,6 +292,26 @@ static const MQTTPresetDef MQTT_PRESETS[MQTT_PRESET_COUNT] = {{ print("self-test failed: URL scheme was treated as a comment.", file=sys.stderr) return 1 + # --exact: identical bytes pass, any drift fails — including a change + # that a name-only comparison waves through. + if compare_exact(equal_a, equal_a, label_a="a", label_b="a-copy"): + print("self-test failed: a file was not equal to itself.", file=sys.stderr) + return 1 + if compare(load_names(equal_a), load_names(equal_b), label_a="a", label_b="b"): + print("self-test failed: fixture assumption broken.", file=sys.stderr) + return 1 + exact_errs = compare_exact(equal_a, equal_b, label_a="a", label_b="b") + if not exact_errs or "byte for byte" not in exact_errs[0]: + print( + "self-test failed: --exact accepted files that differ only in " + f"config: {exact_errs!r}", + file=sys.stderr, + ) + return 1 + if not any("MQTTPresetPolicy.h" in line for line in exact_errs): + print("self-test failed: --exact failure omitted the remedy.", file=sys.stderr) + return 1 + # Silence unused path in fixture layout. dupes.write_text("// unused\n", encoding="utf-8") @@ -250,6 +325,11 @@ def main(argv: list[str] | None = None) -> int: parser.add_argument("file_b", type=Path, nargs="?", help="Second MQTTPresets.h (e.g. dev)") parser.add_argument("--label-a", default="file_a", help="Label for file_a in reports") parser.add_argument("--label-b", default="file_b", help="Label for file_b in reports") + parser.add_argument( + "--exact", + action="store_true", + help="Require the two files to be byte-identical (what CI enforces)", + ) parser.add_argument("--self-test", action="store_true") args = parser.parse_args(argv) @@ -259,6 +339,23 @@ def main(argv: list[str] | None = None) -> int: if args.file_a is None or args.file_b is None: parser.error("FILE_A and FILE_B are required unless --self-test is set") + if args.exact: + try: + errors = compare_exact( + args.file_a, args.file_b, label_a=args.label_a, label_b=args.label_b + ) + except OSError as error: + print(f"MQTT preset parity check could not run: {error}", file=sys.stderr) + return 2 + if errors: + print("MQTT preset parity check failed:", *errors, sep="\n ", file=sys.stderr) + return 1 + print( + f"MQTT preset parity check passed: {args.label_a} and {args.label_b} " + "have identical preset tables." + ) + return 0 + try: names_a = load_names(args.file_a) names_b = load_names(args.file_b) diff --git a/src/helpers/MQTTPresetPolicy.h b/src/helpers/MQTTPresetPolicy.h new file mode 100644 index 00000000..a5a8ddce --- /dev/null +++ b/src/helpers/MQTTPresetPolicy.h @@ -0,0 +1,34 @@ +#pragma once + +#include "MQTTPresets.h" + +#include + +// Policy that keys off a preset rather than describing one. +// +// MQTTPresets.h is data: it is the broker table, and it is kept byte-identical +// between the observer-firmware and observer-firmware-dev channels so a node +// that rolls back cannot meet a preset name its firmware does not know. An +// unknown name is not merely ignored — MQTTPrefsSerializer repairs it to "none" +// and the repaired /mqtt.json is written back, so the operator's slot is gone +// for good. Channel-specific behaviour therefore lives here instead, where the +// two channels are free to differ. + +// True when the broker tears down a live session once its JWT passes exp, so the +// renewal must proactively bounce the connection to present a fresh token. +// +// Default true, because getting this wrong the safe way costs a re-handshake and +// getting it wrong the unsafe way costs an outage. waev is the exception: its +// operator confirmed (2026-08-11) that their servers do not disconnect on expiry, +// so a live session there needs only its credentials refreshed for the next +// reconnect. waev is also the only preset with a short token_lifetime, so it was +// the only one bouncing often — every ~47 min, and each bounce's re-handshake can +// cost ~10 KB of contiguous internal DRAM on a non-PSRAM board. +// +// Keyed by name rather than a struct field on purpose: adding a field would mean +// re-ordering a dozen positional initialisers in the table, where a mistake is +// silent. +static inline bool mqttPresetEnforcesTokenExp(const MQTTPresetDef* preset) { + if (!preset || !preset->name) return true; // custom/audience slots: assume enforced + return strcmp(preset->name, "waev") != 0; +} diff --git a/src/helpers/MQTTPresets.h b/src/helpers/MQTTPresets.h index 8b9d4b1f..5537b031 100644 --- a/src/helpers/MQTTPresets.h +++ b/src/helpers/MQTTPresets.h @@ -47,24 +47,6 @@ struct MQTTPresetDef { // Braces match topic placeholders ({device}/{iata}); never send this string to the broker. static const char MQTT_USERPASS_USERNAME_PUBKEY[] = "{pubkey}"; -// True when the broker tears down a live session once its JWT passes exp, so the -// renewal must proactively bounce the connection to present a fresh token. -// -// Default true, because getting this wrong the safe way costs a re-handshake and -// getting it wrong the unsafe way costs an outage. waev is the exception: its -// operator confirmed (2026-08-11) that their servers do not disconnect on expiry, -// so a live session there needs only its credentials refreshed for the next -// reconnect. waev is also the only preset with a short token_lifetime, so it was -// the only one bouncing often — every ~47 min, and each bounce's re-handshake can -// cost ~10 KB of contiguous internal DRAM on a non-PSRAM board. -// -// Keyed by name rather than a struct field on purpose: adding a field would mean -// re-ordering a dozen positional initialisers below, where a mistake is silent. -static inline bool mqttPresetEnforcesTokenExp(const MQTTPresetDef* preset) { - if (!preset || !preset->name) return true; // custom/audience slots: assume enforced - return strcmp(preset->name, "waev") != 0; -} - static inline bool mqttPresetUsesDevicePubkeyUsername(const MQTTPresetDef* preset) { return preset && preset->auth_type == MQTT_AUTH_USERPASS && preset->userpass_username && diff --git a/src/helpers/bridges/MQTTBridge.h b/src/helpers/bridges/MQTTBridge.h index 7b662da6..1126da0b 100644 --- a/src/helpers/bridges/MQTTBridge.h +++ b/src/helpers/bridges/MQTTBridge.h @@ -10,6 +10,7 @@ #include "helpers/JWTHelper.h" #include "helpers/MQTTPacketFilter.h" #include "helpers/MQTTPresets.h" +#include "helpers/MQTTPresetPolicy.h" #include "helpers/MQTTLifecycle.h" #include "helpers/AlertFaultPolicy.h" #include "helpers/MQTTEffectiveConfig.h"