Skip to content

Commit 37ab2af

Browse files
swissmoclaude
andcommitted
fix: make filter dedup survive module reloads, not just repeat calls
CodeRabbit follow-up: _add_filter_once's isinstance() check only dedups filters within a single loaded generation of ha_mcp.log_filters. The in-process embedded server purges ha_mcp.* from sys.modules and re-imports fresh on every reinstall (_purge_ha_mcp_modules), so filter_cls is a BRAND NEW class object each reload -- isinstance() against a stale-generation instance never matches, so filters accumulated one more stale instance per reload indefinitely (the actual real-world case for this server, not the same-generation repeat-call case the previous fix covered). Match by (module, qualname) instead, which is stable across reloads of the same module path, and actively replace any stale-generation match rather than just skipping when one is found -- a logger always carries exactly one, current-generation filter of each type. Regression test simulates a stale-generation instance directly (a dynamically-built class sharing the real one's module/qualname) rather than via importlib.reload(): reloading the real module mutates shared global state and was observed to break unrelated sibling test files that import these classes at their own module scope -- caught by running the full affected test set with --maxfail=0 before this commit. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
1 parent 852361a commit 37ab2af

2 files changed

Lines changed: 80 additions & 5 deletions

File tree

src/ha_mcp/log_filters.py

Lines changed: 18 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -150,17 +150,30 @@ def filter(self, record: logging.LogRecord) -> bool:
150150

151151

152152
def _add_filter_once(logger_name: str, filter_cls: type[logging.Filter]) -> None:
153-
"""Attach one ``filter_cls`` instance to ``logger_name``, unless already present.
153+
"""Attach one ``filter_cls`` instance to ``logger_name``, replacing any stale one.
154154
155155
``install_sdk_log_filters()`` can run more than once per process: the
156156
in-process embedded server calls it on every ``_serve()`` reload without
157157
a process restart, and process-wide ``logging`` state (including each
158-
named logger's filter list) persists across those reloads. Without this
159-
guard, filters would accumulate one more redundant instance per reload.
158+
named logger's filter list) persists across those reloads. Each reload
159+
also re-imports this module from scratch (the embedded server purges
160+
``ha_mcp.*`` from ``sys.modules`` before reinstalling -- see
161+
``_purge_ha_mcp_modules``), so ``filter_cls`` is a BRAND NEW class object
162+
each time, even though its name is unchanged. A same-``isinstance``-check
163+
against a filter instance from a PREVIOUS generation would therefore
164+
never match, letting filters accumulate one more stale instance per
165+
reload forever. Match by ``(module, qualname)`` instead -- stable across
166+
reloads of the same module path -- and drop every stale-generation match
167+
before attaching the current one, so a logger never carries more than
168+
one filter of a given conceptual type, and it's always this generation's.
160169
"""
161170
logger = logging.getLogger(logger_name)
162-
if any(isinstance(f, filter_cls) for f in logger.filters):
163-
return
171+
identity = (filter_cls.__module__, filter_cls.__qualname__)
172+
logger.filters[:] = [
173+
f
174+
for f in logger.filters
175+
if (type(f).__module__, type(f).__qualname__) != identity
176+
]
164177
logger.addFilter(filter_cls())
165178

166179

tests/src/unit/test_log_filters_install.py

Lines changed: 62 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,23 @@
1616
}
1717

1818

19+
def _make_stale_generation_filter(real_cls: type[logging.Filter]) -> logging.Filter:
20+
"""Build an instance whose (module, qualname) matches ``real_cls`` but whose
21+
class object is a genuinely DIFFERENT one -- simulating a filter instance
22+
left over from a previous ``ha_mcp.log_filters`` generation after the
23+
in-process embedded server purges and re-imports it (see
24+
``_purge_ha_mcp_modules``), without actually reloading any module.
25+
``importlib.reload()`` mutates the real, shared module in place, which
26+
would corrupt every OTHER test file's already-imported reference to these
27+
same class names for the rest of the pytest session -- this builds an
28+
equivalent stand-in instead.
29+
"""
30+
stale_cls = type(
31+
real_cls.__qualname__, (logging.Filter,), {"__module__": real_cls.__module__}
32+
)
33+
return stale_cls()
34+
35+
1936
class TestInstallSdkLogFiltersIdempotent:
2037
"""The in-process embedded server calls this on every reload without a
2138
process restart, so process-wide logger filter lists persist across
@@ -44,3 +61,48 @@ def test_second_call_does_not_duplicate_filters(self):
4461
assert len(matches) == 1, (
4562
f"{name} must carry exactly one {filter_cls.__name__}, got {len(matches)}"
4663
)
64+
65+
66+
class TestInstallSdkLogFiltersReplacesStaleGeneration:
67+
"""The in-process embedded server purges ``ha_mcp.*`` from ``sys.modules``
68+
and re-imports on every reinstall (see ``_purge_ha_mcp_modules``), so
69+
``ha_mcp.log_filters`` is a fresh module with fresh classes after each
70+
reload -- a stale filter instance from before the reload (a DIFFERENT
71+
class object with the same name) must be replaced, not left to
72+
accumulate alongside the new one."""
73+
74+
def setup_method(self):
75+
"""Snapshot each logger's filters, then plant a stale-generation
76+
stand-in filter on each -- the state a real reload would leave
77+
behind, without actually reloading any module."""
78+
self._saved = {
79+
name: logging.getLogger(name).filters[:] for name in _TARGET_LOGGERS
80+
}
81+
for name, filter_cls in _TARGET_LOGGERS.items():
82+
logging.getLogger(name).filters[:] = [
83+
_make_stale_generation_filter(filter_cls)
84+
]
85+
86+
def teardown_method(self):
87+
"""Restore each target logger's filter list, undoing this test's calls."""
88+
for name, filters in self._saved.items():
89+
logging.getLogger(name).filters[:] = filters
90+
91+
def test_stale_generation_instance_is_replaced_not_duplicated(self):
92+
"""A planted stale-generation filter is replaced, never duplicated."""
93+
stale = {name: logging.getLogger(name).filters[0] for name in _TARGET_LOGGERS}
94+
95+
install_sdk_log_filters()
96+
97+
for name, filter_cls in _TARGET_LOGGERS.items():
98+
filters = logging.getLogger(name).filters
99+
assert len(filters) == 1, (
100+
f"{name} must carry exactly one filter after replacing the "
101+
f"stale generation, got {len(filters)}"
102+
)
103+
assert filters[0] is not stale[name], (
104+
f"{name}'s stale pre-reload filter instance must be replaced, not kept"
105+
)
106+
assert isinstance(filters[0], filter_cls), (
107+
f"{name}'s replacement filter must be the current, real {filter_cls.__name__}"
108+
)

0 commit comments

Comments
 (0)