Skip to content

Commit 7819e97

Browse files
CIclaude
andcommitted
fix(octopus): don't keep the grace clock across a restart; judge saved state on the skewed clock (#5316 review)
- The "not charging since" clock is no longer saved. A restart is a stretch with no evidence, after which the running check starts the grace afresh - the car may have charged meanwhile - and restoring it cancelled on the first "off" reading. It also changed with every flap of an unreliable sensor, rewriting the store from the 15 second poll. - Saved times are recorded on the clock_skew axis, so what is still current is now judged on it too rather than on now_utc_real. - A confirmation whose half hour is over is dropped from the saved state. - The storage mocks used by the plan and dynamic load persistence tests move to tests/test_infra.py. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
1 parent 8e91a55 commit 7819e97

4 files changed

Lines changed: 115 additions & 76 deletions

File tree

‎apps/predbat/plan.py‎

Lines changed: 28 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -583,28 +583,39 @@ def dynamic_load_car_target(self, car_n, minute, now, record=True):
583583
def dynamic_load_car_state(self):
584584
"""
585585
The dispatch-check state worth keeping across a restart, in a form storage can hold, keyed by car:
586-
- confirmed: the end of the half hour the car was last seen charging in (dynamic_load_car_strip_from());
586+
- confirmed: the end of the half hour the car was last seen charging in (dynamic_load_car_strip_from()),
587+
while that half hour is still to come;
587588
- run: the dispatch the car is in, with the earliest start seen for it - the start band is timed from it;
588-
- since: when the car was first seen not charging in it - the grace period is timed from it;
589589
- cancelled: the decision so far, which stands while there is no evidence either way.
590590
591-
All times are absolute, as minutes since midnight_utc mean something else after midnight. The rest
592-
of the state (which cars have a sensor, what has been warned about, this cycle's decision) is
593-
rebuilt every cycle.
591+
All times are absolute, as minutes since midnight_utc mean something else after midnight. The "not
592+
charging since" clock is deliberately not kept: a restart is a stretch with no evidence, after which
593+
the running check starts the grace period afresh too (see dynamic_load_car_target()) - the car may
594+
have charged in it - and the clock changes with every flap of an unreliable sensor, which would
595+
rewrite the store every 15 seconds. The rest (which cars have a sensor, what has been warned about,
596+
this cycle's decision) is rebuilt every cycle.
594597
"""
598+
now_minute = self.dynamic_load_car_minute(self.now_utc_real)
599+
confirmed_cars = {car_n: confirmed for car_n, confirmed in self.dynamic_load_car_confirmed.items() if self.dynamic_load_car_minutes_of(confirmed) > now_minute}
595600
cars = {}
596-
for car_n in sorted(set(self.dynamic_load_car_confirmed) | set(self.dynamic_load_car_since) | set(self.dynamic_load_car_run)):
597-
confirmed = self.dynamic_load_car_confirmed.get(car_n)
598-
since = self.dynamic_load_car_since.get(car_n)
601+
for car_n in sorted(set(confirmed_cars) | set(self.dynamic_load_car_run)):
602+
confirmed = confirmed_cars.get(car_n)
599603
run = self.dynamic_load_car_run.get(car_n)
600604
cars[str(car_n)] = {
601605
"confirmed": confirmed.isoformat() if confirmed else None,
602-
"since": since.isoformat() if since else None,
603606
"run": {key: (self.midnight_utc + timedelta(minutes=run[key])).isoformat() for key in ("start", "end")} if run else None,
604607
"cancelled": bool(self.dynamic_load_car_cancelled.get(car_n, False)),
605608
}
606609
return cars
607610

611+
def dynamic_load_car_minutes_of(self, when):
612+
"""
613+
A time saved by the dispatch check as minutes since midnight_utc, the axis of dynamic_load_car_minute():
614+
the times are built from that axis, so it includes clock_skew and must be compared on it, not with
615+
now_utc_real.
616+
"""
617+
return (when - self.midnight_utc).total_seconds() / 60
618+
608619
def dynamic_load_car_save(self):
609620
"""
610621
Save the dispatch-check state (see dynamic_load_car_state()) to storage when it has changed since
@@ -635,9 +646,9 @@ def dynamic_load_car_load(self):
635646
"""
636647
Restore the dispatch-check state saved by dynamic_load_car_save(), at start up.
637648
638-
Only what is still current is taken: a confirmed half hour that has not ended yet, and the run,
639-
grace clock and decision of a dispatch that has not ended yet. Anything older belongs to a dispatch
640-
the car has since left, where the running check would have dropped it too.
649+
Only what is still current is taken: a confirmed half hour that has not ended yet, and the run and
650+
decision of a dispatch that has not ended yet. Anything older belongs to a dispatch the car has since
651+
left, where the running check would have dropped it too.
641652
"""
642653
storage = self.components.get_component("storage") if self.components else None
643654
if not storage or self.midnight_utc is None:
@@ -653,27 +664,23 @@ def dynamic_load_car_load(self):
653664
if not isinstance(cars, dict):
654665
return
655666

656-
now = self.now_utc_real
667+
now_minute = self.dynamic_load_car_minute(self.now_utc_real)
657668
restored = []
658669
for key, car in cars.items():
659670
try:
660671
car_n = int(key)
661672
confirmed = datetime.fromisoformat(car["confirmed"]) if car.get("confirmed") else None
662-
since = datetime.fromisoformat(car["since"]) if car.get("since") else None
663-
run = {name: datetime.fromisoformat(car["run"][name]) for name in ("start", "end")} if car.get("run") else None
664-
confirmed_current = confirmed is not None and confirmed > now
665-
run_current = run is not None and run["end"] > now
666-
run_minutes = {name: (run[name] - self.midnight_utc).total_seconds() / 60 for name in ("start", "end")} if run_current else None
673+
run = {name: self.dynamic_load_car_minutes_of(datetime.fromisoformat(car["run"][name])) for name in ("start", "end")} if car.get("run") else None
674+
confirmed_current = confirmed is not None and self.dynamic_load_car_minutes_of(confirmed) > now_minute
675+
run_current = run is not None and run["end"] > now_minute
667676
except (ValueError, TypeError, KeyError, AttributeError):
668677
continue
669678
if confirmed_current:
670679
self.dynamic_load_car_confirmed[car_n] = confirmed
671680
if run_current:
672681
# Whole minutes stay ints, as the slots they are compared with are
673-
self.dynamic_load_car_run[car_n] = {name: int(value) if value == int(value) else value for name, value in run_minutes.items()}
682+
self.dynamic_load_car_run[car_n] = {name: int(value) if value == int(value) else value for name, value in run.items()}
674683
self.dynamic_load_car_cancelled[car_n] = bool(car.get("cancelled", False))
675-
if since is not None:
676-
self.dynamic_load_car_since[car_n] = since
677684
if confirmed_current or run_current:
678685
restored.append(car_n)
679686
if restored:

‎apps/predbat/tests/test_dynamic_load_car.py‎

Lines changed: 49 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -25,8 +25,7 @@
2525
import tempfile
2626
from datetime import timedelta
2727

28-
from tests.test_infra import run_async
29-
from tests.test_plan_persistence import _MockComponents, _make_storage
28+
from tests.test_infra import run_async, MockStorageComponents, make_test_storage
3029

3130
TIME_FORMAT = "%Y-%m-%dT%H:%M:%S%z"
3231
SENSOR = "binary_sensor.car_charging_now_test"
@@ -1105,14 +1104,14 @@ def _restart(my_predbat):
11051104
def _run_persistence(my_predbat):
11061105
"""
11071106
The dispatch-check state is saved to storage whenever it changes and restored after a restart, so a
1108-
restart part-way through a dispatch neither loses the kept half hour (GH#5316) nor restarts the grace
1109-
period, the start band or the cancellation.
1107+
restart part-way through a dispatch neither loses the kept half hour (GH#5316) nor restarts the start
1108+
band or the cancellation. The grace period does start again, as after any stretch with no evidence.
11101109
"""
11111110
failed = False
11121111
saved_components = my_predbat.components
11131112
tmpdir = tempfile.mkdtemp()
11141113
try:
1115-
storage = _make_storage(my_predbat, tmpdir)
1114+
storage = make_test_storage(my_predbat, tmpdir)
11161115
saves = []
11171116
backend_save = storage.backend.save
11181117

@@ -1124,9 +1123,9 @@ async def counting_save(module, filename, data, **kwargs):
11241123
storage.backend.save = counting_save
11251124
slots = [{"start": 390, "end": 450, "kwh": 3.0, "octopus": True}]
11261125

1127-
print("Test 49: the confirmed half hour, the grace clock and the dispatch run survive a restart")
1126+
print("Test 49: the confirmed half hour and the dispatch run survive a restart, the grace clock starts again")
11281127
_reset(my_predbat)
1129-
my_predbat.components = _MockComponents(storage)
1128+
my_predbat.components = MockStorageComponents(storage)
11301129
_sensor(my_predbat, "on")
11311130
_cycle(my_predbat, 395, slots=slots)
11321131
_sensor(my_predbat, "off")
@@ -1136,29 +1135,38 @@ async def counting_save(module, filename, data, **kwargs):
11361135
_at(my_predbat, 403)
11371136
_restart(my_predbat)
11381137
failed |= _check("t49 confirmed restored", my_predbat.dynamic_load_car_confirmed.get(0) == my_predbat.midnight_utc + timedelta(minutes=420), "confirmed {}".format(my_predbat.dynamic_load_car_confirmed))
1139-
failed |= _check("t49 grace clock restored", my_predbat.dynamic_load_car_since.get(0) == since, "since {}".format(my_predbat.dynamic_load_car_since))
1138+
failed |= _check("t49 grace clock not restored", my_predbat.dynamic_load_car_since.get(0) is None, "since {}".format(my_predbat.dynamic_load_car_since))
11401139
failed |= _check("t49 run restored", my_predbat.dynamic_load_car_run.get(0) == {"start": 390, "end": 450}, "run {}".format(my_predbat.dynamic_load_car_run))
11411140
failed |= _check("t49 still trusted", my_predbat.dynamic_load_car_cancelled.get(0) is False, "cancelled {}".format(my_predbat.dynamic_load_car_cancelled))
1142-
# The grace runs from the restored clock, not from the restart
1141+
# The car may have charged while Predbat was down, so the grace runs from the first reading after the restart
11431142
changed = _cycle(my_predbat, 404, 30, slots=slots)
1144-
failed |= _check("t49 cancelled on the restored clock", changed and my_predbat.dynamic_load_car_cancelled.get(0), "changed {}".format(changed))
1143+
failed |= _check("t49 not cancelled on the old clock", not changed and not my_predbat.dynamic_load_car_cancelled.get(0), "changed {}".format(changed))
1144+
changed = _cycle(my_predbat, 406, 30, slots=slots)
1145+
failed |= _check("t49 cancelled after a fresh grace", changed and my_predbat.dynamic_load_car_cancelled.get(0), "changed {}".format(changed))
11451146
failed |= _check("t49 strip from 07:00", my_predbat.dynamic_load_car_strip_from(0) == 420, "strip from {}".format(my_predbat.dynamic_load_car_strip_from(0)))
11461147

11471148
print("Test 50: a cancellation survives a restart, with no evidence either way after it")
11481149
_sensor(my_predbat, "unavailable")
1149-
_at(my_predbat, 406)
1150+
_at(my_predbat, 407)
11501151
_restart(my_predbat)
11511152
failed |= _check("t50 cancelled restored", my_predbat.dynamic_load_car_cancelled.get(0) is True, "cancelled {}".format(my_predbat.dynamic_load_car_cancelled))
1152-
_cycle(my_predbat, 407, slots=slots)
1153+
_cycle(my_predbat, 408, slots=slots)
11531154
failed |= _check("t50 still cancelled", my_predbat.dynamic_load_car_cancelled.get(0) and _kwh(my_predbat) == [0], "kwh {}".format(_kwh(my_predbat)))
11541155

1155-
print("Test 51: state is only written when it changes")
1156+
print("Test 51: state is only written when it changes - not for a sensor flapping between unavailable and off")
11561157
count = len(saves)
1157-
_cycle(my_predbat, 408, slots=slots)
1158+
_cycle(my_predbat, 409, slots=slots)
11581159
my_predbat.dynamic_load_car_poll(my_predbat.now_utc_real)
11591160
failed |= _check("t51 no write when unchanged", len(saves) == count, "saves {} was {}".format(len(saves), count))
1161+
for step, state in enumerate(["off", "unavailable", "off", "unavailable"]):
1162+
_sensor(my_predbat, state)
1163+
my_predbat.dynamic_load_car_poll(my_predbat.midnight_utc + timedelta(minutes=409, seconds=15 * (step + 1)))
1164+
failed |= _check("t51 no write for a flapping sensor", len(saves) == count, "saves {} was {}".format(len(saves), count))
11601165

1161-
print("Test 52: state from a dispatch that is over is not restored")
1166+
print("Test 52: state from a dispatch that is over is not restored, and a confirmation is not kept once its half hour is over")
1167+
_cycle(my_predbat, 425, slots=slots)
1168+
saved = run_async(storage.load("predbat", "dynamic_load_car"))
1169+
failed |= _check("t52 expired confirmation not saved", saved["cars"]["0"]["confirmed"] is None and saved["cars"]["0"]["run"] is not None, "saved {}".format(saved))
11621170
_at(my_predbat, 455)
11631171
_restart(my_predbat)
11641172
failed |= _check(
@@ -1167,22 +1175,42 @@ async def counting_save(module, filename, data, **kwargs):
11671175
"confirmed {} cancelled {} run {}".format(my_predbat.dynamic_load_car_confirmed, my_predbat.dynamic_load_car_cancelled, my_predbat.dynamic_load_car_run),
11681176
)
11691177

1170-
print("Test 53: a comparison run, a car with no dispatch and a missing or broken store write and restore nothing")
1178+
print("Test 53: what is still current is judged on the clock_skew clock the times were recorded on")
1179+
saved_skew = my_predbat.args.get("clock_skew", None)
1180+
try:
1181+
_reset(my_predbat)
1182+
my_predbat.components = MockStorageComponents(storage)
1183+
_sensor(my_predbat, "on")
1184+
# With the clock 5 minutes fast, 06:52 real is 06:57 on the plan's clock
1185+
my_predbat.args["clock_skew"] = 5
1186+
_cycle(my_predbat, 387, slots=slots)
1187+
failed |= _check("t53 confirmed 06:30-07:00 on the skewed clock", my_predbat.dynamic_load_car_confirmed.get(0) == my_predbat.midnight_utc + timedelta(minutes=420), "confirmed {}".format(my_predbat.dynamic_load_car_confirmed))
1188+
# Real 06:57 is 07:02 on the plan's clock: that half hour is over
1189+
_at(my_predbat, 417)
1190+
_restart(my_predbat)
1191+
failed |= _check("t53 over on the skewed clock is not restored", my_predbat.dynamic_load_car_confirmed.get(0) is None, "confirmed {}".format(my_predbat.dynamic_load_car_confirmed))
1192+
finally:
1193+
if saved_skew is None:
1194+
my_predbat.args.pop("clock_skew", None)
1195+
else:
1196+
my_predbat.args["clock_skew"] = saved_skew
1197+
1198+
print("Test 54: a comparison run, a car with no dispatch and a missing or broken store write and restore nothing")
11711199
tmpdir2 = tempfile.mkdtemp()
11721200
try:
1173-
storage2 = _make_storage(my_predbat, tmpdir2)
1201+
storage2 = make_test_storage(my_predbat, tmpdir2)
11741202
_reset(my_predbat)
1175-
my_predbat.components = _MockComponents(storage2)
1203+
my_predbat.components = MockStorageComponents(storage2)
11761204
_sensor(my_predbat, "on")
11771205
_cycle(my_predbat, 395, slots=slots, save=False)
11781206
_cycle(my_predbat, 395, slots=[])
1179-
failed |= _check("t53 nothing written", run_async(storage2.load("predbat", "dynamic_load_car")) is None)
1207+
failed |= _check("t54 nothing written", run_async(storage2.load("predbat", "dynamic_load_car")) is None)
11801208
run_async(storage2.save("predbat", "dynamic_load_car", ["not", "a", "dict"], format="json"))
11811209
_restart(my_predbat)
11821210
run_async(storage2.save("predbat", "dynamic_load_car", {"cars": {"x": 1, "0": {"confirmed": "junk", "run": {"start": "junk"}}}}, format="json"))
11831211
_restart(my_predbat)
1184-
failed |= _check("t53 junk ignored", not my_predbat.dynamic_load_car_confirmed and not my_predbat.dynamic_load_car_run, "confirmed {} run {}".format(my_predbat.dynamic_load_car_confirmed, my_predbat.dynamic_load_car_run))
1185-
for components in (None, _MockComponents(None)):
1212+
failed |= _check("t54 junk ignored", not my_predbat.dynamic_load_car_confirmed and not my_predbat.dynamic_load_car_run, "confirmed {} run {}".format(my_predbat.dynamic_load_car_confirmed, my_predbat.dynamic_load_car_run))
1213+
for components in (None, MockStorageComponents(None)):
11861214
my_predbat.components = components
11871215
_cycle(my_predbat, 395, slots=slots)
11881216
_restart(my_predbat)

‎apps/predbat/tests/test_infra.py‎

Lines changed: 26 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -50,6 +50,32 @@ def run_async(coro):
5050
return loop.run_until_complete(coro)
5151

5252

53+
class MockStorageComponents:
54+
"""
55+
Minimal components mock returning a pre-configured storage component, for tests of code that saves
56+
and loads through Storage.
57+
"""
58+
59+
def __init__(self, storage):
60+
"""Initialise with a storage instance (may be None to simulate unavailable)."""
61+
self._storage = storage
62+
63+
def get_component(self, name):
64+
"""Return the mocked storage for 'storage', None for everything else."""
65+
if name == "storage":
66+
return self._storage
67+
return None
68+
69+
70+
def make_test_storage(predbat, tmpdir):
71+
"""Create a StorageComponent backed by a real local-file backend in tmpdir."""
72+
from storage import StorageComponent, StorageLocalFiles
73+
74+
storage = StorageComponent(predbat)
75+
storage.backend = StorageLocalFiles(tmpdir, predbat.log)
76+
return storage
77+
78+
5379
def create_aiohttp_mock_response(status=200, json_data=None, json_exception=None):
5480
"""Create a mock aiohttp response object"""
5581
mock_response = MagicMock()

0 commit comments

Comments
 (0)