Repository navigation
feat(log): make the number of retained log files configurable - #5076
Merged
Merged
Conversation
Adds a log_count apps.yaml setting controlling how many log files to keep, including the live predbat.log. Defaults to 10, which reproduces the previous hard-coded behaviour exactly, and is clamped to 2-100 - a bad value falls back to the default rather than stopping the log working. Rotated logs are now numbered with two digits, predbat.01.log through predbat.99.log, so a directory listing sorts in rotation order. Both spellings are considered at every rotation slot, so an existing set of single-digit files migrates to the padded form as it rotates: no upgrade step, and no run where the previous log is missing. read_predbat_log() and the /debug/log web view both resolve the previous log the same way, preferring the padded name and falling back to the single-digit one. Two things fixed along the way. The web log view hard-coded predbat.1.log, so it would have served nothing once rotation moved to the padded names - it now shares the same helper. And the cleanup only ever removed the single file above the limit, so lowering log_count would strand the surplus ones forever; it now sweeps every slot up to the maximum. Note log_count is an apps.yaml key rather than a CONFIG_ITEMS entity because rotation lives in the standalone Hass wrapper, which reads self.args directly and runs before any HA entity exists. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
Unresolved critical and moderate findings affect parsing, validation, rotation, and startup behavior.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
This PR makes retained Predbat log files configurable, adds padded rotation and legacy migration, and updates log viewers and tests.
Changes:
- Adds
log_countconfiguration, validation metadata, documentation, and template settings. - Updates rotation, cleanup, naming, and legacy-file handling.
- Updates log viewers and adds registered rotation tests.
Review findings:
- Critical (1 vote):
apps/predbat/utils.py:83— non-finite values such as.infcan raiseOverflowError. - Moderate (2 votes):
apps/predbat/config.py:2526— schemamin/maxbounds are not enforced. - Moderate (3 votes):
apps/predbat/hass.py:209— the oldest legacy slot can remain stranded. - Moderate (1 vote):
apps/predbat/hass.py:192— startup rotation may use default settings beforeself.argsis populated. - Nit (2 votes):
apps/predbat/tests/test_log_rotation.py:46— tests duplicate rather than execute the production rotation loop.
File summaries
| File | Description |
|---|---|
docs/apps-yaml.md |
Documents configurable log retention |
apps/predbat/web.py |
Integrates shared previous-log lookup |
apps/predbat/utils.py |
Provides log naming, parsing, and lookup helpers |
apps/predbat/unit_test.py |
Registers log rotation tests |
apps/predbat/tests/test_log_rotation.py |
Tests rotation behavior and migration |
apps/predbat/hass.py |
Implements configurable rotation, migration, and cleanup |
apps/predbat/config/apps.yaml |
Adds the configuration example |
apps/predbat/config.py |
Defines the log_count schema entry |
Review details
Suppressed comments (1)
apps/predbat/hass.py:192
self.argsis still{}here whileHass.__init__()is insideload_apps_yaml(log=self.log); that loader logs before it returns the parsed args. If an existingpredbat.logis already over 10 MB, the startup log call rotates using the default count, so a newly configuredlog_countis ignored for that rotation and a lowered count is not cleaned until a later one.
max_logs = predbat_log_count(self.args) - 1
- Files reviewed: 8/8 changed files
- Comments generated: 4
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
…gration off-by-one Four findings, all confirmed real against the branch code: - predbat_log_count() caught (TypeError, ValueError) but not OverflowError: YAML accepts non-finite scalars (.inf/-.inf), which parse as a float int() cannot convert - every Hass.log() call would raise before rotation instead of falling back to the default as documented. Added to the except tuple; the fallback path is unchanged. - validate_config()'s integer branch declared min/max are read by log_count's own APPS_SCHEMA entry but never enforced - the branch only ever checked type and the "zero" flag. log_count: 1 or 101 passed validation despite the schema promising a range check. Wired min/max into the existing per-item loop, following the same pattern as the zero:False check beside it. - The rotation shift walked slots 1..max_logs-1, one short of max_logs itself. A legacy single-digit log sitting in the oldest kept slot (predbat.9.log with the default log_count=10) is a different filename from what lands there via a two-digit rename, so it was never a rename source and never reached by cleanup either (cleanup starts at max_logs+1) - stranded on disk under the old name forever. The shift now runs through max_logs, so that slot's occupant becomes the rename source once, onto the slot that is about to age out. - test_log_rotation.py's _rotate() was a hand-copy of the loop in hass.py's log(), with a docstring claiming "kept structurally identical so a change to one shows up as a failure here" - backwards, since a change to one does NOT show up in the other; they just silently diverge. This is exactly how the off-by-one shipped with green tests: the copy had the same bug, so it agreed with itself. Extracted the rotation into rotate_predbat_logs() in utils.py, called from both hass.py and the test. New tests: log_count=inf/-inf clamp to the default; a legacy log in the oldest kept slot is migrated rather than stranded; validate_config() rejects log_count outside [2, 100] and accepts the boundary values. Mutation-verified: reverting the off-by-one, the OverflowError catch, or the min/max check each fails its new test. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
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.
(Written by Claude, working with @chalfontchubby.)
What this does
Adds a
log_countapps.yaml setting controlling how many Predbat log files to keep, including the livepredbat.log. Previously this was hard-coded to 9 rotated copies.predbat.logplus01-09).APPS_SCHEMAso apps.yaml validation catches a bad value at load.Rotated logs are now numbered with two digits —
predbat.01.logthroughpredbat.99.log— so a directory listing sorts in rotation order.Migration from single-digit names
Existing installs have
predbat.1.log..predbat.9.log. Both spellings are checked at every rotation slot, and whichever is found is renamed to the two-digit name of the next slot up. So an existing set migrates to the padded form as it rotates: no upgrade step, no manual clean-up, and no run where the previous log goes missing.Both log viewers resolve the previous log through one helper,
predbat_log_file_prev(), which preferspredbat.01.logand falls back topredbat.1.log.Two bugs fixed along the way
The web log view hard-coded
predbat.1.log(web.py), so it would have served nothing at all once rotation moved to the padded names. It now shares the same helper asread_predbat_log().Lowering
log_countwould strand files. The old cleanup only ever removed the single file above the limit, so reducing the setting left the surplus copies on disk forever. The cleanup now sweeps every slot up to the maximum.Design note
log_countis an apps.yaml key rather than aCONFIG_ITEMSentity because rotation lives in the standaloneHasswrapper, which readsself.argsdirectly and runs before any HA entity exists.self.argsis populated inHass.__init__()before any rotation can trigger (rotation needs 10MB of log), so reading it there is safe.Testing
New
log_rotationtest module, 9 cases: default/clamp/fallback behaviour forlog_count, two-digit naming, a normal rotation, single-digit migration, removal past the configured count, stranded-file cleanup when the count is lowered, and the viewer's preference and fallback plus the no-previous-log case.The test mirrors the rotation loop rather than importing it, because
Hass.log()is welded to a live instance with an open file handle. The two were diffed to confirm they match structurally, and the migration path was mutation-tested — removing the legacy name from the walk fails test 4 as intended.Quick suite green; pre-commit clean.