From 04cc811b4ca0746826f2aaeddd1f31357e9a3878 Mon Sep 17 00:00:00 2001 From: ChuckBuilds Date: Wed, 19 Aug 2026 09:39:58 -0400 Subject: [PATCH] fix: harden the health-state repair and confirm journald took effect Second review round; all three findings were valid and two were bugs in the repair added last commit. The repair could raise out of itself. An unhashable circuit_state (a list or dict on disk) hit `value in {...}` and raised TypeError -- from the code whose whole job is to stop a malformed record crashing the caller. It now requires a str before the membership test. bool is a subclass of int, so True passed the timestamp check and then compared as 1.0: enough to expire a cooldown the instant the breaker opened, while False would stop the elapsed check firing at all. Timestamps now exclude bool explicitly. The regression test for the original crash was seeded with a record that *contained* circuit_state, so it passed against the old raw-return behaviour too -- the counters are read with .get(), so circuit_state is the only field whose absence used to raise. Reseeded to omit it, and it now fails against raw-return as intended. journald: drop-ins apply in lexical order, so a local file sorting after ledmatrix-persistent.conf still wins and writing ours proves nothing. The effective Storage is re-read afterwards and a warning naming the diagnostic command is printed if persistence is still not active, rather than reporting a success that was not verified. Full suite 2934 passed, same single pre-existing tmpfs failure. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01STMbQE4YctTacQXfbYqKuW --- first_time_install.sh | 15 +++++++++++++++ src/plugin_system/plugin_health.py | 14 ++++++++++++-- test/test_plugin_health.py | 28 ++++++++++++++++++++++++++-- 3 files changed, 53 insertions(+), 4 deletions(-) diff --git a/first_time_install.sh b/first_time_install.sh index 471f0516..1a25bb25 100644 --- a/first_time_install.sh +++ b/first_time_install.sh @@ -1759,6 +1759,21 @@ else mkdir -p /var/log/journal systemd-tmpfiles --create --prefix /var/log/journal >/dev/null 2>&1 || true systemctl restart systemd-journald >/dev/null 2>&1 || true + + # Drop-ins are applied in lexical order, so a locally added file that sorts + # after ledmatrix-persistent.conf (zz-local.conf and friends) still wins. + # Writing the file is not evidence it took effect -- re-read and say so + # plainly rather than reporting success we cannot confirm. + journald_now="$(journald_effective | grep -E '^[[:space:]]*Storage=' | tail -n1 | cut -d= -f2 | tr -d '[:space:]')" + if [ "$journald_now" = "persistent" ]; then + echo " Persistent journald storage active" + else + echo " WARNING: journald storage is still '${journald_now:-unset}' after" + echo " writing /etc/systemd/journald.conf.d/ledmatrix-persistent.conf." + echo " Another drop-in that sorts later is overriding it. Check:" + echo " systemd-analyze cat-config systemd/journald.conf | grep -n Storage=" + echo " Logs will not survive a reboot until that is resolved." + fi fi # Ensure dtparam=audio=off in config.txt (idempotent) diff --git a/src/plugin_system/plugin_health.py b/src/plugin_system/plugin_health.py index 39b5be3e..d9b4f2c5 100644 --- a/src/plugin_system/plugin_health.py +++ b/src/plugin_system/plugin_health.py @@ -139,9 +139,19 @@ class PluginHealthTracker: if field in cls._COUNTER_FIELDS: ok = isinstance(value, int) and not isinstance(value, bool) and value >= 0 elif field in cls._TIMESTAMP_FIELDS: - ok = value is None or isinstance(value, (int, float)) + # bool is a subclass of int, so True would pass as a timestamp + # and then compare as 1.0 -- expiring a cooldown the instant it + # opens, or (False) making the elapsed check never fire. + ok = value is None or ( + isinstance(value, (int, float)) and not isinstance(value, bool) + ) elif field == 'circuit_state': - ok = value in {member.value for member in CircuitState} + # Membership first requires the value to be hashable: a list or + # dict here would raise TypeError out of the repair itself, + # which is the crash this whole path exists to prevent. + ok = isinstance(value, str) and value in { + member.value for member in CircuitState + } else: # last_error ok = value is None or isinstance(value, str) if ok: diff --git a/test/test_plugin_health.py b/test/test_plugin_health.py index 4b1a7636..f418061c 100644 --- a/test/test_plugin_health.py +++ b/test/test_plugin_health.py @@ -161,7 +161,31 @@ def test_newer_fields_are_carried_through(): def test_recording_against_a_repaired_state_does_not_raise(): - """The actual failure: record_failure indexing a field that was not there.""" - tracker = _tracker_reading({"circuit_state": "closed"}) + """The actual failure: record_failure indexing a field that was not there. + + The seed deliberately omits circuit_state. Seeding a record that *has* it + would pass against the old raw-return behaviour too -- the counters are + read with .get(), so circuit_state is the only field whose absence used to + raise. + """ + tracker = _tracker_reading({"total_failures": 2, "total_successes": 1}) tracker.record_failure("p", Exception("boom")) tracker.record_success("p") + + +def test_unhashable_or_boolean_values_are_repaired(): + """Values that break the repair itself rather than a later caller. + + An unhashable circuit_state raises TypeError inside a set membership test, + and bool is a subclass of int, so True would pass as a timestamp and then + compare as 1.0 -- expiring a cooldown the moment it opens. + """ + for bad_state in ({"circuit_state": []}, {"circuit_state": {}}): + state = _tracker_reading(bad_state).get_health_state("p") + assert state["circuit_state"] == CircuitState.CLOSED.value + + state = _tracker_reading({ + "circuit_opened_time": True, "last_success_time": False, + }).get_health_state("p") + assert state["circuit_opened_time"] is None + assert state["last_success_time"] is None