diff --git a/goodwe_controller/app/clamp.py b/goodwe_controller/app/clamp.py index b20e3e6..d787108 100644 --- a/goodwe_controller/app/clamp.py +++ b/goodwe_controller/app/clamp.py @@ -154,8 +154,9 @@ REASON_CODE = "OUTPUT_CLAMP" # and 2 loop-side ones". On the live install the counter reached 48 loop-side # ENGAGEMENTS in ~25 minutes of ordinary operation - roughly one cycle in six, # not a pinned rail, but far more often than a guard sized for 2-in-60 assumed - -# and at a 30 s re-assert the owner received a persistent notification about it -# all day. +# and the owner received a persistent notification about it all day. ⚠️ NOT +# because of the 30 s re-assert - see the block below; those notifications never +# consulted this constant at all. # # ⚠️ BOTH FIGURES ARE ENGAGEMENT COUNTS; only the regime differs - 2-in-60 was # measured over a §10.3 RUNAWAY, 48-in-305 was ordinary running on the live @@ -210,6 +211,28 @@ REASSERT_S = 150.0 # Consecutive clean cycles before the alarm de-asserts (10.2 hysteresis). CLEAR_CYCLES = 3 +# ⚠️ SAFETY-41. Consecutive ENGAGED cycles after which an episode speaks even if +# the notification budget is spent. THE ESCALATION EXISTS BECAUSE THE RATE LIMIT +# ALONE DELAYS A RUNAWAY, and review measured it: a benign one-cycle clamp +# followed by a continuous runaway held the runaway's first notification for up +# to 140 s - one REASSERT_S minus a cycle. On the live install, where ordinary +# railing spends the budget essentially all the time, a runaway arriving during +# normal operation would be delayed with near-certainty. That is the "benign +# noise suppresses the bug signal" shape this module's docstring is written +# against. +# +# THE DISCRIMINATOR IS RUN LENGTH, and the two populations genuinely differ: the +# live flood was runs of length ONE (engage, de-assert, engage), while a §10.3 +# runaway engages on EVERY cycle. So a sustained run speaks regardless of budget, +# and scattered chatter does not. +# +# Measured, shipped-without vs shipped-with - it costs nothing it was protecting: +# live scatter flood (305 cycles, 1-in-6) 11 -> 11 notifications +# chatter 1-engaged/3-clean over 8000 s 50 -> 50 notifications +# pinned 900 s quarter (floor is 5) 6 -> 6 alarms +# runaway delay after a benign episode 140 s -> 10 s +SUSTAINED_CYCLES = 3 + @dataclass(frozen=True) class ClampResult: @@ -315,6 +338,10 @@ class ClampWatchdog: self.last_alarm: datetime | None = None self.reason = "" self.clean_cycles = 0 + # SAFETY-41. Consecutive engaged cycles, and whether THIS episode has + # spoken yet - the two the escalation clause needs. + self.run_len = 0 + self.spoken = False def note(self, engaged: bool, reason: str, now: datetime) -> bool: """Advance one cycle. True when the caller should raise the alarm. @@ -326,10 +353,12 @@ class ClampWatchdog: self.engagements += 1 self.clean_cycles = 0 self.reason = reason + self.run_len += 1 if not self.active: self.active = True self.since = now self.episodes += 1 + self.spoken = False # ⚠️ SAFETY-41. THE RISING EDGE IS RATE-LIMITED TOO, and it did # not used to be. It fired unconditionally, so a condition that # engages and clears repeatedly raised one notification PER @@ -358,15 +387,27 @@ class ClampWatchdog: if (self.last_alarm is None or (now - self.last_alarm).total_seconds() >= REASSERT_S): self.last_alarm = now + self.spoken = True return True return False + # ⚠️ THE ESCALATION. An episode that has not yet spoken because the + # budget was spent by an EARLIER, unrelated episode speaks anyway + # once it has engaged SUSTAINED_CYCLES times in a row. A runaway + # engages every cycle and reaches it; the scattered one-cycle + # railing that produced the live flood never does. + if not self.spoken and self.run_len >= SUSTAINED_CYCLES: + self.last_alarm = now + self.spoken = True + return True # Still engaged: re-assert on the slow cadence, not every cycle. if (self.last_alarm is None or (now - self.last_alarm).total_seconds() >= REASSERT_S): self.last_alarm = now + self.spoken = True return True return False + self.run_len = 0 if not self.active: return False self.clean_cycles += 1 diff --git a/goodwe_controller/test_clamp.py b/goodwe_controller/test_clamp.py index 8caf7eb..1968880 100644 --- a/goodwe_controller/test_clamp.py +++ b/goodwe_controller/test_clamp.py @@ -78,7 +78,8 @@ import sys from datetime import datetime, timedelta, timezone from app.clamp import ( - CLEAR_CYCLES, REASON_CODE, REASSERT_S, ClampWatchdog, clamp_to_rating, + CLEAR_CYCLES, REASON_CODE, REASSERT_S, SUSTAINED_CYCLES, ClampWatchdog, + clamp_to_rating, is_commandable, ) from app.allocator import Allocation, UnitPlan, plan @@ -296,13 +297,54 @@ check(f"...and the rate honours the same interval the pinned case does: at most check("...while EVERY episode and EVERY engagement is still counted - the " "budget is on notifications, never on evidence", scat.episodes == scat.engagements == SCATTER_CYCLES // SCATTER_ONE_IN + 1) -# ⚠️ THE COUNTERFACTUAL. A runaway that begins from quiet must NOT be delayed by -# a budget spent on an earlier, unrelated episode - so a cold watchdog speaks at -# once, and this is the half that would go red if the rate limit were applied -# without regard to how long it has been silent. -check("...and a rising edge from a COLD watchdog still fires immediately, so a " - "runaway beginning from quiet is never held back", - ClampWatchdog().note(True, "cold runaway", T0) is True) +# ⚠️ THE COUNTERFACTUAL, AND THE FIRST VERSION OF IT COULD NOT FAIL FOR THE +# DEFECT IT NAMED. It read `ClampWatchdog().note(True, ...) is True` - a FRESH +# object, so it only ever exercised the `last_alarm is None` branch, which is the +# trivially-true one. A fresh watchdog has no earlier episode to be held back BY. +# That is the third instance in this file of the family recorded at line 216 and +# at the 224 s sweep, and it was guarding the most safety-critical property in +# the change. Review found it. +# +# The stimulus must therefore SPEND THE BUDGET FIRST: a benign one-cycle clamp, +# a gap long enough to de-assert, then a continuous runaway. Without the +# escalation clause the runaway's first notification is held for up to +# REASSERT_S - measured at 140 s before it was added. +_esc = ClampWatchdog() +_esc.note(True, "benign", T0) # spends the budget +for _i in range(1, 1 + CLEAR_CYCLES + 1): # de-assert + _esc.note(False, "", T0 + timedelta(seconds=5.0 * _i)) +_RUNAWAY_AT = 5.0 * (CLEAR_CYCLES + 2) +_esc_fires = [5.0 * (CLEAR_CYCLES + 2 + _j) + for _j in range(200) + if _esc.note(True, "runaway", + T0 + timedelta(seconds=5.0 * (CLEAR_CYCLES + 2 + _j)))] +_esc_delay = _esc_fires[0] - _RUNAWAY_AT if _esc_fires else None +print(f" budget spent by a benign episode, then a continuous runaway from " + f"t={_RUNAWAY_AT:.0f}s: first alarm at t={_esc_fires[0]:.0f}s " + f"(delay {_esc_delay:.0f}s)") +check(f"⚠️ a SUSTAINED runaway speaks even when an earlier, unrelated episode " + f"spent the budget - it is not held for REASSERT_S ({_esc_delay:.0f}s " + f"delay, escalation is {SUSTAINED_CYCLES} cycles)", + _esc_delay is not None and _esc_delay < REASSERT_S / 2) +check("...and it speaks at the escalation boundary rather than immediately, " + "which is what keeps the scattered flood suppressed", + _esc_delay >= 5.0 * (SUSTAINED_CYCLES - 1)) + +# ⚠️ `last_alarm` SURVIVING THE DE-ASSERT IS NOW LOAD-BEARING, AND NOTHING +# PINNED IT. Review built the mutation: if the de-assert path ALSO refreshed +# `last_alarm = now`, the chatter case collapses from 50 notifications to 1 - the +# alarm never fires again. That is the "alarm that never fires" class, which is a +# different bug from one that fires later, and the suite was green on it. +_starve = ClampWatchdog() +_starve_fires = sum(1 for _i in range(1600) + if _starve.note(_i % 4 == 0, "chatter", + T0 + timedelta(seconds=5.0 * _i))) +print(f" chatter 1-engaged/{CLEAR_CYCLES}-clean over 8000 s: " + f"{_starve.episodes} episodes, {_starve_fires} notification(s)") +check(f"a de-assert does not RESET the notification budget, so repeated chatter " + f"cannot starve the alarm to silence ({_starve_fires} notifications over " + f"8000 s, not 1)", + _starve_fires > 10) # --- THE HYSTERESIS, PINNED AGAINST THE CHATTER DIRECTION -------------------- # ⚠️ REVIEW-FOUND. This used to issue `range(CLEAR_CYCLES - 1)` clean cycles,