fix: recognize BrokenResourceError, close silent-branch test gap

Addresses two well-researched review findings, both verified directly
against the pinned mcp==1.28.1/anyio==4.10.0 in this repo's venv before
acting on them:

1. _is_only_closed_resource_errors (renamed
   _is_only_disconnect_teardown_errors) only recognized
   anyio.ClosedResourceError, missing its sibling anyio.BrokenResourceError
   -- confirmed a plain Exception subclass, not a subclass of
   ClosedResourceError. Per the reviewer's anyio-internals walkthrough,
   which one surfaces for the exact same disconnect race depends on which
   end of the memory stream closes first (our end closing raises Closed;
   the peer's streamable_http terminate() closing first can wake a parked
   sender into raising Broken instead). This repo already treats the two
   as an equivalent pair for streamable-HTTP memory-stream teardown
   elsewhere (tests/src/haos_runtime.py's transient-error tuple) -- now
   the classifier does too. Not a regression either way: a
   BrokenResourceError-only leaf simply fails closed (stays at ERROR)
   without this, same as before the PR existed.

   Also independently confirmed the reviewer's mcp-version finding while
   verifying this: at the exact pinned mcp==1.28.1, Server._handle_request
   already catches (BrokenResourceError, ClosedResourceError) around
   respond() and drops it at DEBUG (mcp/server/lowlevel/server.py:805-815)
   -- absent at older mcp versions. The live-validated production capture
   this PR's tests describe as "the shape actually logged in production"
   is accurate for whichever mcp version that deployment has (older,
   given third-party deps aren't reinstalled per ha-mcp reload), not a
   version-independent guarantee -- worth keeping in mind if this classifier
   ever needs revisiting again as mcp gets upgraded.

2. No test constructed a genuine ModuleNotFoundError for exactly
   ha_mcp.log_filters to prove the "older ha-mcp" branch in
   _install_log_filters_if_available() actually stays silent -- the
   existing test only exercised the module-present-but-attribute-missing
   ImportError case, and asserted on survival, not on log output. Dropping
   the err.name check would have left the suite green while every older
   install gained a startup warning. Added
   TestInstallLogFiltersIfAvailable: calls the function directly (not
   through the full _serve()/_thread_main harness) with caplog, covering
   all three branches -- genuinely missing module (silent), attribute
   missing (warns), and a different missing dependency inside the import
   (warns, naming it) -- using the same __path__ = [] sys.modules
   technique _stub_ha_mcp_surface already established for this class of
   problem.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

Author: swissmo

src/ha_mcp/log_filters.py

@@ -85,25 +85,40 @@ def filter(self, record: logging.LogRecord) -> bool:
         return True
 
 
-def _is_only_closed_resource_errors(err: BaseException) -> bool:
-    """True if ``err`` is (or an ExceptionGroup wrapping only) ClosedResourceError.
+_DISCONNECT_TEARDOWN_ERRORS = (anyio.ClosedResourceError, anyio.BrokenResourceError)
+
+
+def _is_only_disconnect_teardown_errors(err: BaseException) -> bool:
+    """True if ``err`` is (or an ExceptionGroup wrapping only) a benign
+    streamable-HTTP memory-stream teardown error.
 
     ``mcp.server.lowlevel.server.Server.run()`` dispatches each incoming
-    message via ``anyio.create_task_group().start_soon(...)``, so a
-    ``ClosedResourceError`` raised while delivering one message's response
-    is raised from a task-group child -- anyio always wraps that in an
-    ``ExceptionGroup``, even for a single failure. The exception a "session
-    crashed" log record carries is therefore
-    ``ExceptionGroup(...[ClosedResourceError])``, not a bare
-    ``ClosedResourceError``. Recurse through (possibly nested) groups; any
-    non-``ClosedResourceError`` leaf means this is not the known-benign
-    disconnect race, so the caller should leave the record alone.
+    message via ``anyio.create_task_group().start_soon(...)``, so an error
+    raised while delivering one message's response is raised from a
+    task-group child -- anyio always wraps that in an ``ExceptionGroup``,
+    even for a single failure. The exception a "session crashed" log record
+    carries is therefore ``ExceptionGroup(...[<teardown error>])``, not a
+    bare one.
+
+    Both ``ClosedResourceError`` and ``BrokenResourceError`` -- sibling
+    ``Exception`` subclasses, neither a subclass of the other -- can surface
+    here for the same disconnect race: which one depends on which end
+    closed first (our end closing raises ``Closed``; the peer's
+    ``streamable_http`` ``terminate()`` closing first can wake a parked
+    sender into raising ``Broken`` instead, even though the situation is
+    identical from the caller's perspective). This repo already treats the
+    two as an equivalent pair for streamable-HTTP memory-stream teardown
+    elsewhere -- see ``tests/src/haos_runtime.py``'s transient-error tuple.
+
+    Recurse through (possibly nested) groups; any other leaf type means
+    this is not the known-benign disconnect race, so the caller should
+    leave the record alone.
     """
-    if isinstance(err, anyio.ClosedResourceError):
+    if isinstance(err, _DISCONNECT_TEARDOWN_ERRORS):
         return True
     if isinstance(err, BaseExceptionGroup):
         return bool(err.exceptions) and all(
-            _is_only_closed_resource_errors(sub) for sub in err.exceptions
+            _is_only_disconnect_teardown_errors(sub) for sub in err.exceptions
         )
     return False
 
@@ -118,8 +133,9 @@ class SessionDisconnectLogFilter(logging.Filter):
     timeout -- lets the SDK finish serving the response and tear the
     transport down while ``app.run()`` is still working; the eventual attempt
     to deliver the response then writes into an already-closed memory stream
-    and raises ``anyio.ClosedResourceError`` (wrapped in an ``ExceptionGroup``
-    -- see ``_is_only_closed_resource_errors``). The SDK already catches this
+    and raises ``anyio.ClosedResourceError`` or ``anyio.BrokenResourceError``
+    (wrapped in an ``ExceptionGroup`` -- see
+    ``_is_only_disconnect_teardown_errors``). The SDK already catches this
     (``except Exception: logger.exception(...)`` in both the stateless and
     stateful session runners of mcp/server/streamable_http_manager.py) -- it
     just logs it as an alarming ERROR-level traceback. That's an expected
@@ -135,7 +151,7 @@ def filter(self, record: logging.LogRecord) -> bool:
             return True
 
         err = record.exc_info[1]
-        if err is None or not _is_only_closed_resource_errors(err):
+        if err is None or not _is_only_disconnect_teardown_errors(err):
             return True
 
         record.msg = (

tests/src/unit/test_embedded_server.py

@@ -2694,6 +2694,86 @@ def http_app(self, path, stateless_http):
         assert isinstance(mgr._thread_exc, _StopServe)
 
 
+class TestInstallLogFiltersIfAvailable:
+    """Direct unit coverage of _install_log_filters_if_available()'s branch
+    logic. CodeRabbit review finding: the _serve()-level tests above only
+    assert that serving continues either way, never that the SILENT
+    "older ha-mcp" case and the WARNING cases are actually distinguishable
+    by their log output -- dropping the err.name check entirely would leave
+    the suite green while every older install started logging noise on
+    every startup. These tests call the function directly (no need for the
+    full _stub_ha_mcp_surface/_thread_main harness) and assert on caplog.
+    """
+
+    def setup_method(self):
+        """Snapshot ha_mcp.* sys.modules entries so each test's fakes don't
+        leak into the next -- ha_mcp is genuinely importable in this test
+        process (unlike the fully-stubbed harness the heavier _serve()-level
+        tests above build)."""
+        self._saved = {
+            name: sys.modules.get(name) for name in ("ha_mcp", "ha_mcp.log_filters")
+        }
+
+    def teardown_method(self):
+        """Restore the snapshotted sys.modules entries exactly."""
+        for name, mod in self._saved.items():
+            if mod is None:
+                sys.modules.pop(name, None)
+            else:
+                sys.modules[name] = mod
+
+    def test_genuinely_missing_module_is_silent(self, caplog):
+        """The true 'older ha-mcp, module doesn't exist yet' case: point the
+        faked ha_mcp package's own __path__ at nothing, so Python's real
+        import machinery genuinely cannot find log_filters.py and raises
+        ModuleNotFoundError(name='ha_mcp.log_filters') -- not a hand-built
+        stand-in. This is exactly the branch CodeRabbit found untested."""
+        fake_ha_mcp = ModuleType("ha_mcp")
+        fake_ha_mcp.__path__ = []  # nothing to search -- a genuine "not found"
+        sys.modules["ha_mcp"] = fake_ha_mcp
+        sys.modules.pop("ha_mcp.log_filters", None)
+
+        with caplog.at_level("WARNING", logger=es._LOGGER.name):
+            es._install_log_filters_if_available()
+
+        assert caplog.records == []
+
+    def test_module_present_but_attribute_missing_warns(self, caplog):
+        """The existing ImportError case (module exists, helper attribute
+        doesn't) must actually log a WARNING -- the discriminating half of
+        the same gap: a prior test proved serving continues, not that the
+        output differs from the silent case above."""
+        sys.modules["ha_mcp.log_filters"] = ModuleType("ha_mcp.log_filters")
+
+        with caplog.at_level("WARNING", logger=es._LOGGER.name):
+            es._install_log_filters_if_available()
+
+        assert "Could not install MCP SDK log-noise filters" in caplog.text
+
+    def test_different_missing_dependency_warns_and_names_it(self, monkeypatch, caplog):
+        """A ModuleNotFoundError for anything OTHER than ha_mcp.log_filters
+        itself (e.g. a stale fastmcp/pydantic left over from a previous
+        install -- _purge_ha_mcp_modules deliberately never reinstalls
+        third-party dependencies) is not the older-server case and must
+        warn, naming the actual missing dependency rather than staying
+        silent."""
+        sys.modules.pop("ha_mcp.log_filters", None)
+        real_import = __import__
+
+        def _fake_import(name, *args, **kwargs):
+            if name == "ha_mcp.log_filters":
+                raise ModuleNotFoundError("No module named 'fastmcp'", name="fastmcp")
+            return real_import(name, *args, **kwargs)
+
+        monkeypatch.setattr("builtins.__import__", _fake_import)
+
+        with caplog.at_level("WARNING", logger=es._LOGGER.name):
+            es._install_log_filters_if_available()
+
+        assert "fastmcp" in caplog.text
+        assert "Could not install MCP SDK log-noise filters" in caplog.text
+
+
 # ---------------------------------------------------------------------------
 # start / stop lifecycle + idempotency
 # ---------------------------------------------------------------------------

tests/src/unit/test_session_disconnect_log_filter.py

@@ -7,15 +7,21 @@
 
 from ha_mcp.log_filters import (
     SessionDisconnectLogFilter,
-    _is_only_closed_resource_errors,
+    _is_only_disconnect_teardown_errors,
 )
 
 
 async def _raise_closed_resource_error() -> None:
-    """Raise the exact exception the disconnect race produces."""
+    """Raise one of the two exceptions the disconnect race can produce."""
     raise anyio.ClosedResourceError()
 
 
+async def _raise_broken_resource_error() -> None:
+    """Raise the other exception the same disconnect race can produce --
+    which one depends on which end of the memory stream closed first."""
+    raise anyio.BrokenResourceError()
+
+
 async def _raise_runtime_error() -> None:
     """Raise an unrelated failure, standing in for a real server bug."""
     raise RuntimeError("real bug")
@@ -100,6 +106,20 @@ def test_demotes_stateful_session_crash_from_closed_resource_error(self):
         assert record.levelno == logging.WARNING
         assert record.exc_info is None
 
+    def test_demotes_stateless_session_crash_from_broken_resource_error(self):
+        """The disconnect race's other exception type: which one surfaces
+        depends on which end of the memory stream closed first, but both
+        are the same benign situation."""
+        err = anyio.BrokenResourceError()
+        record = self._make_record(
+            "mcp.server.streamable_http_manager",
+            "Stateless session crashed",
+            err,
+        )
+        assert self.log_filter.filter(record) is True
+        assert record.levelno == logging.WARNING
+        assert record.exc_info is None
+
     async def test_demotes_real_task_group_exception_group(self):
         """The shape actually logged in production: mcp.server.lowlevel.server
         dispatches message handling via anyio.create_task_group().start_soon,
@@ -118,6 +138,21 @@ async def test_demotes_real_task_group_exception_group(self):
         assert record.exc_info is None
         assert "client disconnected before response delivery" in record.getMessage()
 
+    async def test_demotes_real_task_group_exception_group_broken_resource(self):
+        """Same production shape, but with BrokenResourceError as the leaf --
+        the other half of the disconnect-race pair."""
+        caught = await _run_in_task_group(_raise_broken_resource_error)
+        assert isinstance(caught, BaseExceptionGroup)
+
+        record = self._make_record(
+            "mcp.server.streamable_http_manager",
+            "Stateless session crashed",
+            caught,
+        )
+        assert self.log_filter.filter(record) is True
+        assert record.levelno == logging.WARNING
+        assert record.exc_info is None
+
     async def test_leaves_mixed_exception_group_at_error(self):
         """A task group with one ClosedResourceError AND one unrelated failure
         signals a real problem alongside the expected disconnect race -- the
@@ -174,32 +209,58 @@ def test_passes_record_without_exc_info(self):
         assert record.levelno == logging.ERROR
 
 
-class TestIsOnlyClosedResourceErrors:
+class TestIsOnlyDisconnectTeardownErrors:
     """Direct coverage of the recursive classifier the filter relies on."""
 
     def test_bare_closed_resource_error(self):
         """A bare ClosedResourceError matches on its own."""
-        assert _is_only_closed_resource_errors(anyio.ClosedResourceError()) is True
+        assert _is_only_disconnect_teardown_errors(anyio.ClosedResourceError()) is True
+
+    def test_bare_broken_resource_error(self):
+        """A bare BrokenResourceError matches too -- the other half of the
+        disconnect-race pair (sibling Exception subclasses, neither a
+        subclass of the other, so this needs its own explicit check)."""
+        assert _is_only_disconnect_teardown_errors(anyio.BrokenResourceError()) is True
 
     def test_bare_other_exception(self):
         """An unrelated bare exception never matches."""
-        assert _is_only_closed_resource_errors(RuntimeError("x")) is False
+        assert _is_only_disconnect_teardown_errors(RuntimeError("x")) is False
 
     def test_group_of_one_closed_resource_error(self):
         """A single-item group wrapping just the known-benign exception matches."""
         group = ExceptionGroup("eg", [anyio.ClosedResourceError()])
-        assert _is_only_closed_resource_errors(group) is True
+        assert _is_only_disconnect_teardown_errors(group) is True
+
+    def test_group_of_one_broken_resource_error(self):
+        """Same, for the BrokenResourceError half of the pair."""
+        group = ExceptionGroup("eg", [anyio.BrokenResourceError()])
+        assert _is_only_disconnect_teardown_errors(group) is True
+
+    def test_group_mixing_both_benign_types(self):
+        """A group with both ClosedResourceError and BrokenResourceError
+        leaves is still all-benign -- e.g. concurrent responses racing the
+        same teardown from opposite ends."""
+        group = ExceptionGroup(
+            "eg", [anyio.ClosedResourceError(), anyio.BrokenResourceError()]
+        )
+        assert _is_only_disconnect_teardown_errors(group) is True
 
     def test_nested_group_of_closed_resource_errors(self):
         """Nested groups are unwrapped recursively, matching all-benign leaves."""
         inner = ExceptionGroup("inner", [anyio.ClosedResourceError()])
         outer = ExceptionGroup("outer", [inner, anyio.ClosedResourceError()])
-        assert _is_only_closed_resource_errors(outer) is True
+        assert _is_only_disconnect_teardown_errors(outer) is True
+
+    def test_nested_group_with_broken_resource_error_leaf(self):
+        """Nested groups recurse correctly for the BrokenResourceError half too."""
+        inner = ExceptionGroup("inner", [anyio.BrokenResourceError()])
+        outer = ExceptionGroup("outer", [inner, anyio.ClosedResourceError()])
+        assert _is_only_disconnect_teardown_errors(outer) is True
 
     def test_mixed_group_is_rejected(self):
         """A group with even one non-benign leaf must not match."""
         group = ExceptionGroup("eg", [anyio.ClosedResourceError(), RuntimeError("x")])
-        assert _is_only_closed_resource_errors(group) is False
+        assert _is_only_disconnect_teardown_errors(group) is False
 
     def test_empty_group_is_rejected(self):
         """Defensive: an ExceptionGroup always carries at least one exception