Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
31 changes: 31 additions & 0 deletions tests/test_tui_gateway_server.py
Original file line number Diff line number Diff line change
Expand Up @@ -949,6 +949,37 @@ def close(self):
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."""

Expand Down
38 changes: 22 additions & 16 deletions tui_gateway/server.py
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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:
Expand Down Expand Up @@ -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)
Expand Down
Loading