Move the log-redaction cache into a self-contained mixin (#5169)

[springfall2008]

This is an automated draft PR generated from issue #5169 — a maintainer should review it before merging.

Fixes #5169

Summary

Hass held log()'s credential-redaction cache (GH#4770/#5053), but its three members are called from files that do not define the class holding them — userinterface.py's set_arg()/auto_config(), web.py's batch apps.yaml editor and chat_tools.py all call self._invalidate_log_secret_pattern() on the engine object — while the state they need (_log_secret_pattern_lock, _log_secret_pattern_cache, _log_secret_pattern_fingerprint) was established by exactly one __init__. "Has the methods but not the state" was therefore a reachable state that raises AttributeError on first use.

The sentinel and the three methods move verbatim into LogRedaction in a new apps/predbat/log_secrets.py; Hass becomes class Hass(LogRedaction) and __init__ loses three lines. No behaviour changes: the same methods, the same caching, the same fingerprint safety net.

State lives on the class rather than in an __init__, so nothing has to be remembered by whatever inherits the mixin. Following the design note on the triage comment, the lock is not lazily initialised — _log_secret_pattern() takes it on entry precisely to serialise concurrent first uses, and two threads each creating one lazily would each proceed inside their own, which is the interleaving the lock exists to prevent. It is a class attribute, shared by every instance: all it guards is the atomicity of one instance's "read sentinel, build, store", so making that atomic process-wide is strictly stronger than per-instance, and it is only held for any length of time during a rebuild (a config change) - it is, as the #5171 review notes, acquired on every log line, across a fingerprint check of four identity/len() reads. Sharing it means a rebuild on one instance briefly blocks log() on the others; nothing reached under it logs, so there is no re-entrant path back into log() to deadlock a non-reentrant Lock. The cache and fingerprint are class attributes too, but only as defaults to read — the first build or invalidation shadows them with instance attributes, so no instance ever sees another's secrets.

This complements #5063 (which removes the call-site contract) rather than overlapping it.

Testing

• cd coverage && ./run_pre_commit — passed, including the quick suite (All tests passed (4 slow tests skipped)), which covers secrets, web_apps_edit, chat_tools and web_chat — the four modules that poke at this cache.

• tools/triage_test.sh secrets — fails without the fix, passes with it. With the source change stashed (test file kept), the new test_log_redaction_mixin_needs_no_init reports exactly the failure from the issue:

  ERROR: redaction needed state that only Hass.__init__ establishes:
    'EngineWithItsOwnInit' object has no attribute '_log_secret_pattern_lock'
  ERROR: the redaction mixin is not importable on its own: No module named 'log_secrets'
  

  With the change restored, the whole module passes (**** Test secrets PASSED ****).

The new test asserts both halves of what the mixin is for: a Hass subclass that never calls Hass.__init__ still redacts (rather than raising on the first invalidation), and LogRedaction is usable with no Hass anywhere, each instance's pattern covering its own args only.

test_log_secret_pattern_build_is_not_racy is updated to monkeypatch collect_log_secret_values in log_secrets's namespace rather than hass's — that is where _log_secret_pattern() now looks the name up, and patching the old target would have silently stopped pausing the build, leaving the test green without exercising the race at all. It still passes, which is the evidence that the shared class-level lock serialises a build against a concurrent invalidation exactly as the per-instance one did.

Notes

• Four one-line pointer updates are included beyond the move itself (userinterface.py, web.py, chat_tools.py, utils.py): each is a comment saying the cache lives in hass.py, which the move makes untrue. One docstring reference to "successive reviews of this PR" is reworded to name fix(security): redact secrets at log-write time, not just at serve time (#4770) #5053, since it now sits in a file a different PR created.
• Blast radius: impact({target: "_log_secret_pattern", direction: "upstream"}) reports CRITICAL — its only direct caller is Hass.log(), which every execution flow reaches. That reflects log()'s reach, not this change's: the methods keep their names and semantics and Hass still exposes all three by inheritance. The only namespace change is that hass.collect_log_secret_values/hass.compile_log_secret_pattern no longer exist; a repo-wide grep found one referrer, the monkeypatch above. detect_changes() reports only the redaction path plus the comment-only touches.
• apps/predbat/log_secrets.py needs no install-list entry: download.py installs apps/predbat/* and coverage/standalone_ha symlinks *.py, both globs.
• Not done deliberately: the module is not added to the core-module tables in CLAUDE.md/AGENTS.md, which list the orchestrator's major subsystems rather than small helpers.

🤖 Generated with Claude Code

Review round (7d17ea1)

Four inline comments on the first round, all replied to in their threads:

• Fingerprint could be fooled by address reuse (fixed) - _log_secret_fingerprint() recorded id(args)/id(secrets), so a freed mapping's address handed to a same-length replacement compared equal and skipped the rebuild, leaving a just-configured credential unredacted until the next explicit invalidation. It now holds the mappings themselves, compared by _log_secret_fingerprint_matches() using is. Carried over from hass.py rather than introduced here, but the move re-anchored it.
• Shared class-level lock (comment corrected, lock kept) - the comment claimed it was not on the per-log-line path, which was wrong; it now states the real cost and records that nothing under the lock logs, so sharing it cannot deadlock.
• Cache-hit path uncovered (fixed) - test_log_secret_pattern_cache_is_reused() counts rebuilds, so a regression that recompiled on every log line can no longer pass the suite silently.
• Only AttributeError caught in the new test (fixed) - any other failure is now a reported test failure rather than a traceback that aborts the whole run.

test_log_secret_fingerprint_is_not_fooled_by_address_reuse() covers the first of those. It asserts the stored fingerprint is among gc.get_referrers() of the superseded mapping rather than trying to provoke a real address collision: a probe that frees the mapping and allocates same-shaped dicts looking for the old address was tried first and passed with and without the fix (64 decoy allocations never landed on the freed address), so it proved nothing. The referrer assertion fails against the reverted id()-based fingerprint and passes with the fix - verified both ways.

Gates after the fixes: ./run_pre_commit clean apart from pre-existing markdownlint findings in working-tree files this PR does not touch, tools/triage_test.sh secrets passes, and ./run_all --quick passes (4 slow tests skipped).

[Copilot]

Copilot review overview

🟢 Approval recommended

No unresolved blocking issues were identified.

Review effort: Lite
Findings: None

What changed in this PR

Moves log-redaction caching into a reusable LogRedaction mixin, independent of Hass.__init__.

Changes:

• Added self-contained cache, locking, and fingerprint logic.
• Updated Hass inheritance and documentation references.
• Expanded tests for caching, concurrency, and fingerprint safety.

File	Description
apps/​predbat/​web.py	Updated cache ownership documentation
apps/​predbat/​utils.py	Updated cache documentation
apps/​predbat/​userinterface.py	Updated cache ownership documentation
apps/​predbat/​tests/​test_secrets.py	Expanded redaction and concurrency tests
apps/​predbat/​log_secrets.py	Added redaction mixin
apps/​predbat/​hass.py	Inherits the mixin and removes duplicated setup
apps/​predbat/​chat_tools.py	Updated cache ownership documentation

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.