Fix a crash rebuilding yesterday's plan from a slot-edge-only status - #4943
Merged
Merged
Conversation
calculate_yesterday() rebuilds each History slot with two walks over the same per-minute status list: one decides *whether* the slot produces a charge or export window, the other decides *where* that window starts and ends. #4872 widened only the first to trust a slot's first/last 5 minutes when they hold one state throughout, leaving the second on the interior minutes alone. A state living solely in a trusted edge was therefore counted by the tally but never seen by the boundary scan, so the window was appended with its bounds still None. in_charge_window() then compared an int against None: TypeError: '>=' not supported between instances of 'int' and 'NoneType' That propagates out of calculate_plan() and kills the whole update_pred() cycle, so a detail of the History view took the plan down with it. The two walks now cover exactly the same trusted minutes, so a counted state always has a start minute by construction. This also makes a short plan_interval_minutes safe: at 10 or below the old interior range was empty, so any activity at all hit this. Fixing that surfaced a second defect in the same loop. The charge/export handoff - whichever side starts second ends the one that started first - also fired when both sides started at the *same* minute, which is exactly what cross-charging is. It closed the export window at its own start minute, so every cross-charging slot rebuilt a zero-width window: present in the list, invisible to in_charge_window(), and worth nothing in the simulated cost. The export half of cross-charging was thus still missing from History long after #4466 restored it, because the test only asserted the window list was non-empty - a list of 60 zero-width windows passed it. An equal start is an overlap, not a handoff, so both sides now run the full slot. Note this changes reported savings for anyone whose inverters cross-charge: those export limits now actually apply in the yesterday simulation. Tests: both edges covered, cross-charging asserted to cover its whole slot and to be visible to the real in_charge_window() lookup, and the existing cross-charging test strengthened to require start < end - the missing assertion that let this hide. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
🟢 Approval recommended
The production fix addresses the reported crash and cross-charging reconstruction defect with targeted regression tests added to prevent recurrence.
Pull request overview
Fixes a calculate_yesterday() crash that could terminate the whole update_pred() cycle when reconstructing yesterday’s plan from History slot statuses, and corrects cross-charging slot reconstruction so export windows have real duration and are visible to in_charge_window().
Changes:
- Align the slot “boundary scan” with the same trusted-minute range used by the dominant-state tally, preventing
{"start": None, "end": None}windows that crashin_charge_window(). - Fix charge/export “handoff” logic so equal start minutes are treated as overlap (cross-charging) rather than ending one side at its own start.
- Add/strengthen regression tests around edge-only states and cross-charging export windows; document the debugging lesson in the triage journal.
File summaries
| File | Description |
|---|---|
| apps/predbat/output.py | Aligns History slot scans with tally trust range and fixes cross-charging handoff to prevent None/zero-width windows. |
| apps/predbat/tests/test_calculate_yesterday.py | Adds new regression tests and strengthens assertions to catch zero-width export windows. |
| .claude/skills/issue-triage/references/debug-journal.md | Adds a debugging note capturing the “two scans must agree” lesson and the cross-charging zero-width pitfall. |
Review details
Suppressed comments (1)
apps/predbat/tests/test_calculate_yesterday.py:1662
- This test docstring says the rebuilt window bounds “lie inside its own slot”, and the loop also carries slot_start, but the assertions never validate slot alignment/range. Adding explicit bounds checks (and using slot_start) would better lock in the intended behaviour and avoid the slot_start variable being effectively unused.
for label, windows, slot_start in (
("charge", captured.get("charge_window_best") or [], charge_slot_start),
("export", captured.get("export_window_best") or [], export_slot_start),
):
if len(windows) != 1:
- Files reviewed: 3/3 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+1188
to
+1193
| # A window is only rebuilt if it covers real time. Asserting the list is merely non-empty | ||
| # let a list of zero-width windows count as a fix for years - see the dedicated test below. | ||
| empty = [window for window in captured["export_window_best"] if window["start"] >= window["end"]] | ||
| if empty: | ||
| print("ERROR: {} rebuilt export windows cover no time at all, e.g. {!r}".format(len(empty), empty[0])) | ||
| failed = True |
springfall2008
deleted the
fix/yesterday-edge-only-window-none-bounds
branch
September 5, 2026 12:55
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The crash
This propagates out of
calculate_plan()and kills the wholeupdate_pred()cycle — a detail of the History view takes the plan down with it. Reported from a user log at 13:30:35, two minutes after #4872 merged.Cause
calculate_yesterday()rebuilds each History slot with two walks over the same per-minute status list:#4872 widened only the first to trust a slot's first/last 5 minutes when they hold one state throughout. The second stayed on
range(edge_minutes, plan_interval_minutes - edge_minutes).So a state living solely in a trusted edge was counted by the tally but never seen by the boundary scan, and the window was appended as
{"start": None, "end": None}.The two walks now cover exactly the same trusted minutes, so a counted state always has a start minute by construction. This also makes a short
plan_interval_minutessafe: at 10 or below the old interior range was empty, so any activity would have hit this.A second defect in the same loop
Fixing the first surfaced this one. The charge/export handoff — whichever side starts second ends the one that started first — also fired when both sides started at the same minute, which is exactly what cross-charging is. It closed the export window at its own start minute.
Every cross-charging slot therefore rebuilt a zero-width window: present in the list, invisible to
in_charge_window()(start <= m < endcan never hold), and worth nothing in the simulated cost. Probed onmain:The export half of cross-charging was thus still missing from History long after #4466 restored it — because
_test_cross_charging_reconstructed_as_both_windowsonly asserted the list was non-empty, and a list of 60 zero-width windows passes that happily.An equal start is an overlap, not a handoff, so both sides now run the full slot.
Important
This changes reported savings for anyone whose inverters cross-charge — those export limits now actually apply in the yesterday simulation. In the direction of being correct, but the numbers will move.
Tests
_test_edge_only_state_still_gets_real_window_bounds— both slot edges, and checks the bounds survive the realin_charge_window()lookup, since that is where the crash landed._test_cross_charging_export_window_covers_the_slot— asserts full-slot coverage, that the export window matches the charge window, and thatin_charge_window()can see it._test_cross_charging_reconstructed_as_both_windowsstrengthened to requirestart < end. This is the assertion whose absence let the zero-width bug hide.Both new tests were confirmed to fail against the unfixed code before the fix went in.
./run_all --quickpasses in full, including thedebug_casesgolden plans (22.66s, no shift)../run_pre_commitclean.🤖 Generated with Claude Code