Skip to content

Commit 30cf59e

Browse files
Merge pull request #5171 from springfall2008/feat/log-redaction-mixin-5169
Move the log-redaction cache into a self-contained mixin (#5169)
2 parents f1a5d83 + 7d17ea1 commit 30cf59e

7 files changed

Lines changed: 393 additions & 82 deletions

File tree

‎apps/predbat/chat_tools.py‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -772,7 +772,7 @@ def set_apps_config(base, key, value, apps_yaml_path=APPS_YAML_PATH, backup_path
772772

773773
base.args[key] = value
774774
# The written key can itself be a credential (or the redact_strings/redact_strings_labelled
775-
# denylists), so log()'s cached redaction pattern (hass.py) must be rebuilt on next use or a
775+
# denylists), so log()'s cached redaction pattern (log_secrets.py) must be rebuilt on next use or a
776776
# newly saved value keeps leaking into the log under the stale pattern until restart (#5053
777777
# review).
778778
base._invalidate_log_secret_pattern()

‎apps/predbat/hass.py‎

Lines changed: 7 additions & 74 deletions
Original file line numberDiff line numberDiff line change
@@ -24,7 +24,8 @@
2424
import os
2525
import subprocess
2626

27-
from utils import collect_log_secret_values, compile_log_secret_pattern, redact_log_line
27+
from log_secrets import LogRedaction
28+
from utils import redact_log_line
2829

2930

3031
def write_git_version_marker():
@@ -183,82 +184,17 @@ async def main():
183184
sys.exit(0)
184185

185186

186-
class Hass:
187+
class Hass(LogRedaction):
187188
"""Standalone mode wrapper emulating the AppDaemon interface.
188189
189190
Enables PredBat to run outside Home Assistant/AppDaemon with YAML
190191
config loading, secret management, log rotation, scheduled callbacks,
191192
and file change detection for development hot-reload.
192-
"""
193-
194-
# Sentinel distinct from None: compile_log_secret_pattern() legitimately returns None when
195-
# there are no secrets configured to redact, so None alone in the cache slot can't tell
196-
# "not built yet" from "built, and there is nothing to redact" - the latter would otherwise
197-
# rebuild (recompute the value set, recompile) on every single log() call instead of caching.
198-
_LOG_SECRET_PATTERN_UNSET = object()
199-
200-
def _invalidate_log_secret_pattern(self):
201-
"""
202-
Mark the cached redaction pattern stale so the next log() call rebuilds it from the
203-
current args/secrets (GH#4770). Every call site that mutates self.args or self.secrets
204-
after startup must call this - see _log_secret_pattern()'s docstring for why a missed
205-
site is a real leak, not just a staleness bug.
206-
"""
207-
with self._log_secret_pattern_lock:
208-
self._log_secret_pattern_cache = self._LOG_SECRET_PATTERN_UNSET
209193
210-
def _log_secret_fingerprint(self):
211-
"""
212-
A cheap value that changes whenever the set of credentials in args/secrets could have.
213-
214-
Deliberately not a hash of every value: this runs under the lock on the way to each
215-
rebuild decision, so it stays O(number of top-level keys). Identity of the args and
216-
secrets mappings plus their sizes catches the shapes a config mutation takes - a key
217-
added or removed (size), and the whole mapping being replaced or rebound (identity), as
218-
web.py's batch editor does via clear()/update() and web_chat.py does by assigning a new
219-
block.
220-
221-
An in-place edit of an existing key that keeps the size the same is NOT caught here, so
222-
the explicit _invalidate_log_secret_pattern() call sites remain load-bearing. This is a
223-
safety net under them, not a replacement: with it, a missed or mis-ordered call site
224-
degrades to "redacted from the next line" instead of "leaks until the next restart",
225-
which is the failure mode successive reviews of this PR kept finding one call site at a
226-
time. GH#5063 tracks removing the contract itself by routing every args mutation through
227-
one setter (#5053 review).
228-
"""
229-
args = getattr(self, "args", None)
230-
secrets = getattr(self, "secrets", None)
231-
return (id(args), len(args) if isinstance(args, dict) else -1, id(secrets), len(secrets) if isinstance(secrets, dict) else -1)
232-
233-
def _log_secret_pattern(self):
234-
"""
235-
Return the cached compiled redaction pattern log() must apply, rebuilding it the first
236-
time it is needed and whenever load_secrets()/apps.yaml load invalidate it (GH#4770).
237-
238-
Cached rather than recomputed on every log() call: log() runs on every log line, while
239-
args/secrets only change on startup and on a config reload, so rebuilding the value set
240-
and recompiling the pattern that rarely - rather than on every call - keeps the
241-
redaction check to a single compiled-regex scan per line on the hot path.
242-
243-
Guarded by a lock, not just the sentinel check: log() runs from component threads as well
244-
as the main thread (create_task()), so two threads can both observe the sentinel and race
245-
to rebuild. Without the lock, a thread that started building from stale args right before
246-
another thread invalidates the cache (a credential just added via set_arg()) can finish
247-
second and overwrite the fresh invalidation with its stale, already-out-of-date pattern -
248-
silently keeping the just-added credential unredacted until something invalidates the
249-
cache again. The lock makes "read sentinel, build, store" one atomic step so a build that
250-
started before an invalidation can never win a race against it.
251-
"""
252-
with self._log_secret_pattern_lock:
253-
fingerprint = self._log_secret_fingerprint()
254-
if self._log_secret_pattern_cache is self._LOG_SECRET_PATTERN_UNSET or fingerprint != self._log_secret_pattern_fingerprint:
255-
args = getattr(self, "args", None)
256-
redact_strings = args.get("redact_strings") if args else None
257-
redact_strings_labelled = args.get("redact_strings_labelled") if args else None
258-
values = collect_log_secret_values(args, getattr(self, "secrets", None), redact_strings, redact_strings_labelled)
259-
self._log_secret_pattern_cache = compile_log_secret_pattern(values)
260-
self._log_secret_pattern_fingerprint = fingerprint
261-
return self._log_secret_pattern_cache
194+
log()'s credential-redaction cache lives in LogRedaction (log_secrets.py) rather
195+
than here, so it carries its own state and needs nothing from this __init__
196+
(GH#5169).
197+
"""
262198

263199
def log(self, msg, quiet=True):
264200
"""
@@ -338,9 +274,6 @@ def __init__(self):
338274
self.threads = []
339275
self.fatal_error = False
340276
self.hass_api_version = 2
341-
self._log_secret_pattern_lock = threading.Lock()
342-
self._log_secret_pattern_cache = self._LOG_SECRET_PATTERN_UNSET
343-
self._log_secret_pattern_fingerprint = None
344277

345278
self.logfile = open("predbat.log", "a")
346279

‎apps/predbat/log_secrets.py‎

Lines changed: 154 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,154 @@
1+
# -----------------------------------------------------------------------------
2+
# Predbat Home Battery System
3+
# Copyright Trefor Southwell 2026 - All Rights Reserved
4+
# This application maybe used for personal use only and not for commercial use
5+
# -----------------------------------------------------------------------------
6+
# fmt off
7+
# pylint: disable=consider-using-f-string
8+
# pylint: disable=line-too-long
9+
# pylint: disable=attribute-defined-outside-init
10+
11+
12+
"""Cached credential-redaction pattern for log(), as a self-contained mixin.
13+
14+
Holds the compiled pattern Hass.log() applies to every line it writes (GH#4770), kept
15+
here rather than on Hass itself so that whatever class provides log() only has to
16+
inherit LogRedaction to get working redaction - with no __init__ of its own to
17+
remember (GH#5169).
18+
"""
19+
20+
import threading
21+
22+
from utils import collect_log_secret_values, compile_log_secret_pattern
23+
24+
25+
class LogRedaction:
26+
"""Mixin providing log()'s cached credential-redaction pattern (GH#4770).
27+
28+
Inheritable on its own terms: all of its state is established on the class rather
29+
than in an __init__, so a class that inherits this mixin has working redaction
30+
whatever its own constructor does - including not calling this one's, since there
31+
isn't one. That matters because the members below are called from files that do not
32+
define the class holding them (userinterface.py, web.py, chat_tools.py all call
33+
_invalidate_log_secret_pattern() on the engine object), so "has the methods but not
34+
the state" was a reachable state as long as exactly one __init__ established it, and
35+
an engine composed from a different hass.py crash-looped on precisely that (GH#5169).
36+
"""
37+
38+
# Sentinel distinct from None: compile_log_secret_pattern() legitimately returns None when
39+
# there are no secrets configured to redact, so None alone in the cache slot can't tell
40+
# "not built yet" from "built, and there is nothing to redact" - the latter would otherwise
41+
# rebuild (recompute the value set, recompile) on every single log() call instead of caching.
42+
_LOG_SECRET_PATTERN_UNSET = object()
43+
44+
# Class attributes, not __init__ assignments: an instance that has never built a pattern
45+
# reads these, and the first build/invalidation shadows them with instance attributes of its
46+
# own, so the cache is per-instance from the moment it holds anything instance-specific
47+
# while still needing no constructor to exist (GH#5169).
48+
_log_secret_pattern_cache = _LOG_SECRET_PATTERN_UNSET
49+
_log_secret_pattern_fingerprint = None
50+
51+
# The lock, unlike the cache, cannot be created on first use: _log_secret_pattern() takes it
52+
# on entry precisely to serialise concurrent first uses, and two threads lazily creating one
53+
# each would each proceed inside their own - the exact interleaving it exists to prevent. A
54+
# class attribute is therefore shared by every instance, which is sound because all it guards
55+
# is the atomicity of one instance's "read sentinel, build, store"; making that atomic
56+
# process-wide is strictly stronger than making it per-instance.
57+
#
58+
# It is acquired on every _log_secret_pattern() call, so on every log line, and held across
59+
# the fingerprint check - a handful of identity and len() reads - on the warm path; only a
60+
# rebuild holds it for longer, and rebuilds happen on a config change. Sharing it does mean a
61+
# rebuild on one instance briefly blocks log() on every other instance of the class, which
62+
# the single-engine production case never notices but a test process holding several engines
63+
# could (#5171 review). Nothing reached under the lock logs - collect_log_secret_values() and
64+
# compile_log_secret_pattern() are pure helpers in utils.py - so there is no re-entrant path
65+
# back into log() to deadlock a non-reentrant Lock, which is what makes sharing it safe
66+
# rather than merely cheap.
67+
_log_secret_pattern_lock = threading.Lock()
68+
69+
def _invalidate_log_secret_pattern(self):
70+
"""
71+
Mark the cached redaction pattern stale so the next log() call rebuilds it from the
72+
current args/secrets (GH#4770). Every call site that mutates self.args or self.secrets
73+
after startup must call this - see _log_secret_pattern()'s docstring for why a missed
74+
site is a real leak, not just a staleness bug.
75+
"""
76+
with self._log_secret_pattern_lock:
77+
self._log_secret_pattern_cache = self._LOG_SECRET_PATTERN_UNSET
78+
79+
def _log_secret_fingerprint(self):
80+
"""
81+
A cheap value that changes whenever the set of credentials in args/secrets could have.
82+
83+
Deliberately not a hash of every value: this runs under the lock on the way to each
84+
rebuild decision, so it stays O(number of top-level keys). Identity of the args and
85+
secrets mappings plus their sizes catches the shapes a config mutation takes - a key
86+
added or removed (size), and the whole mapping being replaced or rebound (identity), as
87+
web.py's batch editor does via clear()/update() and web_chat.py does by assigning a new
88+
block.
89+
90+
The mappings themselves are held here, not their id(): a stored id() is only an integer,
91+
so once the mapping it was taken from is freed CPython is free to hand that same address
92+
to the next allocation, and a rebind to a same-length dict landing there would produce a
93+
fingerprint equal to the stored one - skipping the rebuild and leaving a just-configured
94+
credential unredacted until the next explicit invalidation (#5171 review). Holding the
95+
objects means an address recorded in a live fingerprint cannot be recycled while it is
96+
still recorded, at the cost of keeping a superseded args/secrets mapping reachable until
97+
the next rebuild. They are compared by _log_secret_fingerprint_matches() rather than ==,
98+
because == on two dicts is an O(number of values) comparison that would also report two
99+
distinct-but-equal mappings as unchanged - identity is the question being asked.
100+
101+
An in-place edit of an existing key that keeps the size the same is NOT caught here, so
102+
the explicit _invalidate_log_secret_pattern() call sites remain load-bearing. This is a
103+
safety net under them, not a replacement: with it, a missed or mis-ordered call site
104+
degrades to "redacted from the next line" instead of "leaks until the next restart",
105+
which is the failure mode successive reviews of #5053 kept finding one call site at a
106+
time. GH#5063 tracks removing the contract itself by routing every args mutation through
107+
one setter (#5053 review).
108+
"""
109+
args = getattr(self, "args", None)
110+
secrets = getattr(self, "secrets", None)
111+
return (args, len(args) if isinstance(args, dict) else -1, secrets, len(secrets) if isinstance(secrets, dict) else -1)
112+
113+
@staticmethod
114+
def _log_secret_fingerprint_matches(left, right):
115+
"""
116+
Whether two fingerprints from _log_secret_fingerprint() describe the same args/secrets.
117+
118+
The mapping halves are compared with `is` and only their sizes with ==, so a rebind to an
119+
equal-but-distinct mapping counts as a change (a rebuild) rather than a match. `right` is
120+
None before the first build, which never matches a real fingerprint.
121+
"""
122+
if left is None or right is None:
123+
return left is right
124+
return left[0] is right[0] and left[1] == right[1] and left[2] is right[2] and left[3] == right[3]
125+
126+
def _log_secret_pattern(self):
127+
"""
128+
Return the cached compiled redaction pattern log() must apply, rebuilding it the first
129+
time it is needed and whenever load_secrets()/apps.yaml load invalidate it (GH#4770).
130+
131+
Cached rather than recomputed on every log() call: log() runs on every log line, while
132+
args/secrets only change on startup and on a config reload, so rebuilding the value set
133+
and recompiling the pattern that rarely - rather than on every call - keeps the
134+
redaction check to a single compiled-regex scan per line on the hot path.
135+
136+
Guarded by a lock, not just the sentinel check: log() runs from component threads as well
137+
as the main thread (create_task()), so two threads can both observe the sentinel and race
138+
to rebuild. Without the lock, a thread that started building from stale args right before
139+
another thread invalidates the cache (a credential just added via set_arg()) can finish
140+
second and overwrite the fresh invalidation with its stale, already-out-of-date pattern -
141+
silently keeping the just-added credential unredacted until something invalidates the
142+
cache again. The lock makes "read sentinel, build, store" one atomic step so a build that
143+
started before an invalidation can never win a race against it.
144+
"""
145+
with self._log_secret_pattern_lock:
146+
fingerprint = self._log_secret_fingerprint()
147+
if self._log_secret_pattern_cache is self._LOG_SECRET_PATTERN_UNSET or not self._log_secret_fingerprint_matches(fingerprint, self._log_secret_pattern_fingerprint):
148+
args = getattr(self, "args", None)
149+
redact_strings = args.get("redact_strings") if args else None
150+
redact_strings_labelled = args.get("redact_strings_labelled") if args else None
151+
values = collect_log_secret_values(args, getattr(self, "secrets", None), redact_strings, redact_strings_labelled)
152+
self._log_secret_pattern_cache = compile_log_secret_pattern(values)
153+
self._log_secret_pattern_fingerprint = fingerprint
154+
return self._log_secret_pattern_cache

0 commit comments

Comments
 (0)