diff --git a/tests/test_tui_gateway_server.py b/tests/test_tui_gateway_server.py index a25d713b80a..477ca21d2cf 100644 --- a/tests/test_tui_gateway_server.py +++ b/tests/test_tui_gateway_server.py @@ -949,6 +949,37 @@ def test_ws_orphan_reap_closes_worker_when_session_stays_detached(monkeypatch): server._sessions.pop("orphan-sid", None) +def test_finalize_session_closes_slash_worker(monkeypatch): + """_finalize_session closes the slash_worker subprocess itself. + + Regression for #38095: the worker cleanup used to live only in the + callers (_teardown_session / _shutdown_sessions), so any code path that + finalized a session without going through them leaked the worker. Folding + close() into the single _finalized-guarded chokepoint makes the cleanup + defense-in-depth and idempotent. + """ + closed = {"count": 0} + + class _FakeWorker: + def close(self): + closed["count"] += 1 + + monkeypatch.setattr(server, "_notify_session_boundary", lambda *a, **k: None) + monkeypatch.setattr(server, "_get_db", lambda: None) + + session = _session(slash_worker=_FakeWorker()) + + server._finalize_session(session) + assert closed["count"] == 1 + assert session.get("_finalized") is True + + # Idempotent: a second finalize (or a follow-up teardown) must not + # re-close the worker — the _finalized guard short-circuits. + server._finalize_session(session) + server._teardown_session(session) + assert closed["count"] == 1 + + def test_ws_orphan_reap_spares_reattached_session(monkeypatch): """A session that rebinds a live transport is NOT considered orphaned.""" diff --git a/tui_gateway/server.py b/tui_gateway/server.py index 4dd9592a77b..c0885e661c2 100644 --- a/tui_gateway/server.py +++ b/tui_gateway/server.py @@ -348,14 +348,31 @@ def _finalize_session(session: dict | None, end_reason: str = "tui_close") -> No except Exception: pass + # Close the slash-worker subprocess as part of finalize itself, not just + # in the callers. Defense-in-depth: every session-end path goes through + # _finalize_session (it's the single ``_finalized``-guarded chokepoint), so + # folding worker cleanup in here means a future code path that calls + # _finalize_session directly — without the surrounding _teardown_session / + # _shutdown_sessions worker.close() — can't reintroduce the #38095 leak. + # Idempotent: _SlashWorker.close() is poll()-guarded, so the explicit + # close() still in those callers is harmless. + try: + worker = session.get("slash_worker") + if worker: + worker.close() + except Exception: + pass + def _teardown_session(session: dict | None) -> None: """Fully tear down a session: finalize, unregister, close agent + worker. - Shared by ``session.close`` and the orphaned-WS-session reaper so the - slash-worker subprocess is always closed exactly once via the same path. - Idempotent: the ``_finalized`` guard in ``_finalize_session`` and the - ``poll()`` guard in ``_SlashWorker.close`` make repeat calls harmless. + Shared by ``session.close`` and the orphaned-WS-session reaper. The + slash-worker subprocess is closed inside ``_finalize_session`` (the single + finalize chokepoint); this still unregisters the approval notifier and + closes the in-process agent. Idempotent: the ``_finalized`` guard in + ``_finalize_session`` and the ``poll()`` guard in ``_SlashWorker.close`` + make repeat calls harmless. """ if not session: return @@ -372,12 +389,6 @@ def _teardown_session(session: dict | None) -> None: agent.close() except Exception: pass - try: - worker = session.get("slash_worker") - if worker: - worker.close() - except Exception: - pass def _ws_session_is_orphaned(session: dict | None) -> bool: @@ -425,13 +436,8 @@ def _shutdown_sessions() -> None: with _sessions_lock: snapshot = list(_sessions.values()) for session in snapshot: + # _finalize_session closes the slash-worker subprocess too. _finalize_session(session, end_reason="tui_shutdown") - try: - worker = session.get("slash_worker") - if worker: - worker.close() - except Exception: - pass atexit.register(_shutdown_sessions)