fix(mcp): bound session disconnect so a wedged host close cannot hang teardown

_disconnect_targets submitted every per-host Target.close() to a
ContextExecutor and used concurrent.futures.wait(..., timeout=45) to bound
the wait -- but the `with ContextExecutor()` block's __exit__ runs
shutdown(wait=True), which re-joins every worker regardless of that timeout.
A paramiko close that wedges (dead peer, no RST) keeps its worker thread
alive, so shutdown(wait=True) blocked forever and close() never returned;
under http this hung the registry idle-sweep awaiting it and leaked the
thread, contradicting the docstring's "bounded wait" promise.

Drop the context-manager exit for the bound: shut the pool down explicitly
with wait=False (cancel_futures=True) after the timed wait, log any host
that overran the budget, and abandon its close (the worker thread leaks, but
teardown always returns). The 45s bound is now a named module constant and
_disconnect_targets takes a `timeout` argument so the behaviour is testable
without a 45s wait.

Adds a revert-verified regression test: a host whose close() blocks forever
no longer stalls _disconnect_targets -- the healthy host is still closed, the
stuck one is logged, and every template's host group is cleared.

Resolves finding openSUSE#10 from the recent code review.

Author: plusky

CHANGELOG.md

@@ -237,6 +237,13 @@ and this project follows [Semantic Versioning](https://semver.org/spec/v2.0.0.ht
   connections — instead of silently overwriting it. Repeated `regenerate`
   cycles no longer leak file descriptors or leave stale arbiter claims that
   could block later slot re-acquisition on the same refhost pool.
+- `mtui-mcp` session teardown (idle-TTL sweep or explicit eviction) is now
+  genuinely time-bounded. A wedged refhost close (a dead peer that never sends
+  an RST can hang paramiko's disconnect forever) no longer blocks the whole
+  disconnect: the stuck close is waited on only up to a fixed budget, then
+  logged and abandoned so `close()` — and the http registry's idle-sweep behind
+  it — always returns. Previously the bound was defeated by the thread pool's
+  context-manager exit, which re-joined every worker regardless of the timeout.
 - An unscoped multi-template fan-out of a host-phase command (e.g. `lock`,
   `run`) no longer fails the whole fan-out — or, for `lock`, pretends to
   succeed — when a loaded template has no connected host. A host-less template

mtui/mcp/session.py

@@ -103,6 +103,14 @@
 #: on ``run`` / ``update`` / ``set_repo`` / ``commit`` etc.).
 DEFAULT_PROGRESS_INTERVAL_SECONDS: float = 10.0
 
+#: Upper bound (seconds) that :meth:`McpSession._disconnect_targets` waits
+#: for the parallel per-host ``Target.close()`` calls to finish before it
+#: gives up and returns. A wedged paramiko close (dead peer, no RST) can
+#: block its worker thread forever; the bound guarantees session teardown
+#: still completes, abandoning the stuck close rather than blocking the
+#: whole disconnect (and, under http, the registry idle-sweep behind it).
+DISCONNECT_TIMEOUT_SECONDS: float = 45.0
+
 
 class _LogCaptureHandler(Handler):
     """Tee ``mtui`` log records emitted *during a command* into its stdout.
@@ -553,14 +561,29 @@ async def close(self) -> None:
             return
         await asyncio.to_thread(self._disconnect_targets)
 
-    def _disconnect_targets(self) -> None:
+    def _disconnect_targets(self, timeout: float = DISCONNECT_TIMEOUT_SECONDS) -> None:
         """Synchronous parallel host-disconnect core for :meth:`close`.
 
         Runs in a worker thread. Closes every loaded template's
         :class:`Target` on its own pool thread (paramiko teardown is
-        blocking) with a bounded wait, then clears each template's host
-        group regardless of individual outcomes. Per-host errors are
-        logged at WARNING, never raised.
+        blocking) with a genuinely bounded wait, then clears each
+        template's host group regardless of individual outcomes.
+        Per-host errors are logged at WARNING, never raised.
+
+        The bound is enforced by shutting the pool down with
+        ``wait=False`` after the timed :func:`concurrent.futures.wait`,
+        *not* by the ``with`` block's exit — ``Executor.__exit__`` calls
+        ``shutdown(wait=True)``, which would re-block on a wedged close
+        (a dead peer with no RST keeps its worker thread alive forever)
+        and defeat the whole point of the timeout. A close that overruns
+        ``timeout`` is logged and abandoned: its worker thread leaks, but
+        ``close()`` — and, under http, the registry idle-sweep awaiting
+        it — always returns. (That worker is a non-daemon pool thread, so
+        a close that stays wedged forever can still delay a *clean*
+        interpreter exit via ``concurrent.futures``' atexit join; that is
+        strictly better than the old behaviour, which blocked ``close()``
+        itself during steady-state operation, and is bounded in practice
+        by the OS TCP timeout.)
         """
         # (HostsGroup, hostname) for every host across every loaded template.
         work = [
@@ -575,9 +598,20 @@ def _close_one(targets, name: str) -> None:
             except Exception as exc:  # noqa: BLE001 - best-effort teardown
                 self.log.warning("error disconnecting host %s: %s", name, exc)
 
-        with ContextExecutor() as executor:
-            futures = [executor.submit(_close_one, t, name) for t, name in work]
-            concurrent.futures.wait(futures, timeout=45)
+        executor = ContextExecutor()
+        try:
+            futures = {executor.submit(_close_one, t, name): name for t, name in work}
+            _done, not_done = concurrent.futures.wait(futures, timeout=timeout)
+            for future in not_done:
+                self.log.warning(
+                    "host %s did not disconnect within %ss; abandoning its close",
+                    futures[future],
+                    timeout,
+                )
+        finally:
+            # Never join a wedged close: wait=False keeps the bound above real,
+            # cancel_futures drops any per-host close still queued behind it.
+            executor.shutdown(wait=False, cancel_futures=True)
 
         for report in self.templates.all():
             report.targets.clear()

tests/test_mcp_session.py

@@ -451,6 +451,56 @@ def test_close_disconnects_every_loaded_templates_hosts(tmp_path: Path) -> None:
     assert other.targets == {}
 
 
+def test_disconnect_targets_bounded_wait_survives_a_wedged_close(
+    tmp_path: Path, caplog: pytest.LogCaptureFixture
+) -> None:
+    """A host whose ``close()`` never returns must not block teardown.
+
+    ``Executor.__exit__`` runs ``shutdown(wait=True)``, so bounding the
+    wait with only the ``with`` block would re-block on a wedged paramiko
+    close and hang ``close()`` (and the http idle-sweep behind it)
+    forever. The explicit ``shutdown(wait=False)`` keeps the timed wait
+    real: teardown returns despite the stuck close, the healthy host is
+    still closed, the stuck one is reported, and every template's host
+    group is cleared.
+    """
+    sess = _make_session(tmp_path)
+
+    release = threading.Event()
+    wedged_host = MagicMock()
+    wedged_host.close.side_effect = lambda: release.wait(30)
+    good_host = MagicMock()
+
+    report = MagicMock()
+    report.id = "SUSE:Maintenance:1:1"
+    report.targets = {"wedged-host": wedged_host, "good-host": good_host}
+    sess.templates.add(report)
+    sess.templates.set_active("SUSE:Maintenance:1:1")
+
+    worker = threading.Thread(target=sess._disconnect_targets, kwargs={"timeout": 0.2})
+    try:
+        with caplog.at_level(logging.WARNING):
+            worker.start()
+            # Generous guard: the fix returns in ~0.2 s; a regression that
+            # relies on the ``with`` exit would still be blocked here.
+            worker.join(timeout=15)
+
+        assert not worker.is_alive()
+        # The healthy host was closed even though a sibling close hung.
+        good_host.close.assert_called_once()
+        # The stuck host was reported, not silently dropped.
+        assert any(
+            "wedged-host" in r.getMessage() and "did not disconnect" in r.getMessage()
+            for r in caplog.records
+        )
+        # Host groups are emptied regardless of the stuck close.
+        assert report.targets == {}
+    finally:
+        # Let the abandoned worker thread finish so it does not linger.
+        release.set()
+        worker.join(timeout=5)
+
+
 # --------------------------------------------------------------------------- #
 # Fan-out dispatch honours the ``template`` parameter (Phase 4)               #
 # --------------------------------------------------------------------------- #