From 459bcd78ea6ff78a504774d7c8b27ff74a473daa Mon Sep 17 00:00:00 2001 From: Jeff Watts <186512915+lEWFkRAD@users.noreply.github.com> Date: Thu, 2 Jul 2026 08:47:47 -0400 Subject: [PATCH 1/2] fix(tests): make tui_gateway server tests order-independent across files Running any combination of tui_gateway test files in one pytest process (e.g. `pytest tests/tui_gateway/ tests/test_tui_gateway_server.py`) failed ~10 tests that all pass per-file. The canonical runner spawns one process per file, so CI never sees these, but direct multi-file pytest invocations do. Four leak vectors, all in test fixtures: 1. RPC-registry stubs: test_protocol.py and test_inline_rpc_gil_starvation.py replace handlers in the shared module-level `_methods` dict ("slash.exec" -> raising stub, "prompt.submit" -> canned response) and never restore them, so later files hit RuntimeError("kaboom") on session.compress or empty slash completions. The `server` fixtures now snapshot `_methods` right after import and restore it in place. 2. sys.modules mocks active during test bodies: the `server` fixtures kept `patch.dict("sys.modules", {"hermes_constants": MagicMock(...)})` open across the yield. Any module first imported inside a test body (e.g. hermes_cli.active_sessions in the lease tests) permanently bound the mocked get_hermes_home -> fixed shared "/tmp/hermes_test" path, so unreleased active-session leases accumulated in one shared registry across files (observed: 7 stale entries under a fresh tmp HERMES_HOME). The mocks are now scoped to the import statement only; once the module is cached they were inert anyway. 3. hermes_state.DEFAULT_DB_PATH captured at import: the constant is computed at module import, which usually happens at collection time -- BEFORE the hermetic env fixture ever runs -- so every argless SessionDB() in the whole run opened the developer's REAL state.db, reading real sessions into assertions and persisting test rows into the real profile (found s1/s2/s3 rows from test_projects_rpc.py in a live state.db). _hermetic_environment now re-pins the constant to the per-test home, promoting the workaround that several test files already carried locally into an invariant. 4. test_undo_command.py teardown did `_methods.clear()` + importlib.reload(): the reload re-registers atexit hooks (duplicate ThreadPoolExecutor shutdowns race the stderr buffer at interpreter exit -> Fatal Python error: _enter_buffered_busy) and re-captures _hermes_home against the test's soon-deleted tmpdir. Replaced with in-place snapshot/restore. A new autouse fixture in tests/conftest.py backstops the rest of the module state: it snapshots/restores `_methods`, the config cache and the DB handle, clears the session/pending/child-mirror registries (releasing leftover active-session leases so the file-based registry cannot pin the session cap), and resets a leaked context-local hermes-home override. It is a near no-op (one sys.modules lookup) while tui_gateway.server has not been imported. Verified: the previously failing combinations plus the full tui_gateway set (584 tests) pass in a single process in both orders, and every touched file still passes standalone. Co-Authored-By: Claude Fable 5 --- tests/conftest.py | 117 ++++++++++++++++++ tests/tui_gateway/test_compaction_status.py | 5 +- tests/tui_gateway/test_goal_command.py | 25 ++-- .../test_inline_rpc_gil_starvation.py | 20 ++- tests/tui_gateway/test_moa_reference_emit.py | 7 +- tests/tui_gateway/test_protocol.py | 37 ++++-- .../test_review_summary_callback.py | 25 ++-- .../tui_gateway/test_subagent_child_mirror.py | 15 ++- tests/tui_gateway/test_undo_command.py | 27 ++-- 9 files changed, 218 insertions(+), 60 deletions(-) diff --git a/tests/conftest.py b/tests/conftest.py index bdef8bae944e..b7533187d117 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -360,6 +360,20 @@ def _hermetic_environment(tmp_path, monkeypatch): (fake_hermes_home / "skills").mkdir() monkeypatch.setenv("HERMES_HOME", str(fake_hermes_home)) + # 3b. hermes_state computes ``DEFAULT_DB_PATH = get_hermes_home() / "state.db"`` + # at import time. When the module is first imported at collection (any + # test file with a top-level ``from hermes_state import ...``) that + # happens BEFORE this fixture ever runs, so every argless + # ``SessionDB()`` in every test opens the developer's REAL state.db — + # reading real sessions into assertions and writing test rows into the + # real profile. Re-pin the constant to this test's home. (Several test + # files already do this locally; this makes it an invariant.) + hermes_state_mod = sys.modules.get("hermes_state") + if hermes_state_mod is not None and hasattr(hermes_state_mod, "DEFAULT_DB_PATH"): + monkeypatch.setattr( + hermes_state_mod, "DEFAULT_DB_PATH", fake_hermes_home / "state.db" + ) + # 4. Deterministic locale / timezone / hashseed. CI runs in UTC with # C.UTF-8 locale; local dev often doesn't. Pin everything. monkeypatch.setenv("TZ", "UTC") @@ -419,6 +433,109 @@ def _isolate_hermes_home(_hermetic_environment): # approvals from one test's session into another's. +# ── tui_gateway.server shared-module state isolation ─────────────────────── +# +# ``tui_gateway.server`` registers its RPC handlers in a module-level +# ``_methods`` dict at import time and keeps per-session state in module +# globals (sessions, child-run registry, config cache, DB handle). The +# canonical per-file process isolation above hides any leakage, but a direct +# multi-file invocation (``pytest tests/tui_gateway/ tests/test_tui_gateway_server.py``, +# or plain ``pytest tests/``) shares one interpreter: a test that stubs +# ``_methods["slash.exec"]`` or leaves an active-session lease behind breaks +# unrelated tests in later files. This fixture snapshots the cheap-to-copy +# globals before each test and restores them after, so any file combination +# is order-independent. It is a near no-op (one sys.modules lookup) while +# the module has not been imported. +# +# The case this cannot cover — the module is first imported *during* a test +# that also mutates ``_methods`` — is handled by the importing files' own +# ``server`` fixtures (tests/tui_gateway/test_protocol.py and friends), which +# snapshot immediately after the import. + +_TUI_SERVER_MODULE = "tui_gateway.server" + + +def _release_tui_server_leases(sessions: dict) -> None: + """Release active-session leases held by leftover server sessions. + + Clearing ``_sessions`` without releasing leaks the lease's registry + entry; with the test process alive it is never pruned, so later tests + hit a phantom 'active session limit' against stale entries. + """ + for session in list(sessions.values()): + lease = session.get("active_session_lease") if isinstance(session, dict) else None + if lease is not None: + try: + lease.release() + except Exception: + pass + + +@pytest.fixture(autouse=True) +def _reset_tui_gateway_server_state(): + mod = sys.modules.get(_TUI_SERVER_MODULE) + snapshot = None + if mod is not None: + snapshot = { + "methods": dict(mod._methods), + "cfg": (mod._cfg_cache, mod._cfg_mtime, mod._cfg_path), + "db": (mod._db, mod._db_error), + "real_stdout": mod._real_stdout, + } + + yield + + mod = sys.modules.get(_TUI_SERVER_MODULE) + if mod is None: + return + + # This finalizer can run before the test's own monkeypatch undo, so a + # global may still be replaced with a non-dict test double — skip those + # (monkeypatch restores the real, pre-test object afterwards anyway). + sessions = mod._sessions + if isinstance(sessions, dict): + _release_tui_server_leases(sessions) + sessions.clear() + for name in ( + "_pending", + "_pending_prompt_payloads", + "_answers", + "_child_mirrors", + "_active_child_runs", + ): + obj = getattr(mod, name, None) + if isinstance(obj, dict): + obj.clear() + + if snapshot is not None: + mod._methods.clear() + mod._methods.update(snapshot["methods"]) + mod._cfg_cache, mod._cfg_mtime, mod._cfg_path = snapshot["cfg"] + mod._db, mod._db_error = snapshot["db"] + mod._real_stdout = snapshot["real_stdout"] + else: + # First imported during this test — reset to import-time defaults + # for the globals we could not snapshot (``_methods`` is left to + # the importing file's fixture, see block comment above). + mod._cfg_cache = None + mod._cfg_mtime = None + mod._cfg_path = None + mod._db = None + mod._db_error = None + + # A leaked context-local Hermes home override redirects every later + # ``get_hermes_home()`` call (active-session registry, config paths) + # to a stale per-test tmpdir. Force the main-thread ContextVar back + # to its default. + try: + from hermes_constants import get_hermes_home_override, set_hermes_home_override + + if get_hermes_home_override() is not None: + set_hermes_home_override(None) + except Exception: + pass + + @pytest.fixture() def tmp_dir(tmp_path): """Provide a temporary directory that is cleaned up automatically.""" diff --git a/tests/tui_gateway/test_compaction_status.py b/tests/tui_gateway/test_compaction_status.py index 0e98bde18547..a9cc7b693362 100644 --- a/tests/tui_gateway/test_compaction_status.py +++ b/tests/tui_gateway/test_compaction_status.py @@ -17,6 +17,8 @@ @pytest.fixture() def server(): + # Mocks are scoped to the initial import only (see + # tests/tui_gateway/test_protocol.py for the rationale). with patch.dict( "sys.modules", { @@ -28,7 +30,8 @@ def server(): "hermes_state": MagicMock(), }, ): - yield importlib.import_module("tui_gateway.server") + mod = importlib.import_module("tui_gateway.server") + yield mod def _capture(server, monkeypatch): diff --git a/tests/tui_gateway/test_goal_command.py b/tests/tui_gateway/test_goal_command.py index 11ceadb58af5..2152c13aafee 100644 --- a/tests/tui_gateway/test_goal_command.py +++ b/tests/tui_gateway/test_goal_command.py @@ -35,6 +35,8 @@ def hermes_home(tmp_path, monkeypatch): @pytest.fixture() def server(hermes_home): + # Mocks are scoped to the initial import only (see + # tests/tui_gateway/test_protocol.py for the rationale). with patch.dict( "sys.modules", { @@ -43,18 +45,17 @@ def server(hermes_home): }, ): mod = importlib.import_module("tui_gateway.server") - yield mod - # Reset module-level session state without re-importing. importlib.reload - # would re-register the module's atexit hooks (ThreadPoolExecutor - # shutdown, _shutdown_sessions); the duplicates race the stderr - # buffer at interpreter shutdown and surface as Fatal Python error: - # _enter_buffered_busy. Clearing the per-session dicts gives the - # next test a clean slate; _methods is NOT cleared because it's - # populated at module import time and re-registration only happens - # via reload (which we don't do). - mod._sessions.clear() - mod._pending.clear() - mod._answers.clear() + + yield mod + # Reset module-level session state without re-importing. importlib.reload + # would re-register the module's atexit hooks (ThreadPoolExecutor + # shutdown, _shutdown_sessions); the duplicates race the stderr + # buffer at interpreter shutdown and surface as Fatal Python error: + # _enter_buffered_busy. Clearing the per-session dicts gives the + # next test a clean slate. + mod._sessions.clear() + mod._pending.clear() + mod._answers.clear() @pytest.fixture() diff --git a/tests/tui_gateway/test_inline_rpc_gil_starvation.py b/tests/tui_gateway/test_inline_rpc_gil_starvation.py index 80244b71a731..fc056aa1b63f 100644 --- a/tests/tui_gateway/test_inline_rpc_gil_starvation.py +++ b/tests/tui_gateway/test_inline_rpc_gil_starvation.py @@ -34,6 +34,9 @@ def _restore_stdout(): @pytest.fixture() def server(): + # Mocks are scoped to the initial import only — keeping them active for + # the whole test would poison modules first imported inside test bodies + # (see tests/tui_gateway/test_protocol.py for the full rationale). with patch.dict("sys.modules", { "hermes_constants": MagicMock(get_hermes_home=MagicMock(return_value="/tmp/hermes_test")), "hermes_cli.env_loader": MagicMock(), @@ -42,10 +45,19 @@ def server(): }): import importlib mod = importlib.import_module("tui_gateway.server") - yield mod - mod._sessions.clear() - mod._pending.clear() - mod._answers.clear() + + # Tests below stub handlers ("session.list", "prompt.submit", ...) in + # the module-level _methods dict shared with every other test file in + # the process — snapshot and restore it around each test. + methods = dict(mod._methods) + real_stdout = mod._real_stdout + yield mod + mod._methods.clear() + mod._methods.update(methods) + mod._real_stdout = real_stdout + mod._sessions.clear() + mod._pending.clear() + mod._answers.clear() @pytest.fixture() diff --git a/tests/tui_gateway/test_moa_reference_emit.py b/tests/tui_gateway/test_moa_reference_emit.py index 161e69bd0fea..a063c5f63506 100644 --- a/tests/tui_gateway/test_moa_reference_emit.py +++ b/tests/tui_gateway/test_moa_reference_emit.py @@ -16,6 +16,8 @@ @pytest.fixture() def server(): + # Mocks are scoped to the initial import only (see + # tests/tui_gateway/test_protocol.py for the rationale). with patch.dict( "sys.modules", { @@ -30,8 +32,9 @@ def server(): import importlib mod = importlib.import_module("tui_gateway.server") - yield mod - mod._sessions.clear() + + yield mod + mod._sessions.clear() @pytest.fixture() diff --git a/tests/tui_gateway/test_protocol.py b/tests/tui_gateway/test_protocol.py index 274ad8906be4..9183f9ec8778 100644 --- a/tests/tui_gateway/test_protocol.py +++ b/tests/tui_gateway/test_protocol.py @@ -21,6 +21,12 @@ def _restore_stdout(): @pytest.fixture() def server(): + # The sys.modules mocks only need to cover the *initial* import — once + # tui_gateway.server is cached, they are inert. Keeping them active for + # the whole test poisons any module first imported inside a test body: + # e.g. hermes_cli.active_sessions would bind the mocked get_hermes_home + # (a fixed shared path) forever, leaking active-session registry entries + # across every later test in the process. Scope the patch to the import. with patch.dict("sys.modules", { "hermes_constants": MagicMock(get_hermes_home=MagicMock(return_value="/tmp/hermes_test")), "hermes_cli.env_loader": MagicMock(), @@ -29,18 +35,25 @@ def server(): }): import importlib mod = importlib.import_module("tui_gateway.server") - yield mod - # Reset module-level session state without re-importing. importlib.reload - # would re-register the module's atexit hooks (ThreadPoolExecutor - # shutdown, _shutdown_sessions); the duplicates race the stderr - # buffer at interpreter shutdown and surface as Fatal Python error: - # _enter_buffered_busy. Clearing the per-session dicts gives the - # next test a clean slate; _methods is NOT cleared because it's - # populated at module import time and re-registration only happens - # via reload (which we don't do). - mod._sessions.clear() - mod._pending.clear() - mod._answers.clear() + + # Snapshot the RPC registry: several tests below stub handlers + # ("slash.exec", "fast.ping", ...) directly in the module-level dict, + # which is shared with every other test file in the process. + methods = dict(mod._methods) + real_stdout = mod._real_stdout + yield mod + # Reset module-level state without re-importing. importlib.reload + # would re-register the module's atexit hooks (ThreadPoolExecutor + # shutdown, _shutdown_sessions); the duplicates race the stderr + # buffer at interpreter shutdown and surface as Fatal Python error: + # _enter_buffered_busy. Restoring the dicts in place gives the next + # test a clean slate. + mod._methods.clear() + mod._methods.update(methods) + mod._real_stdout = real_stdout + mod._sessions.clear() + mod._pending.clear() + mod._answers.clear() @pytest.fixture() diff --git a/tests/tui_gateway/test_review_summary_callback.py b/tests/tui_gateway/test_review_summary_callback.py index 6ca17889d64c..67cb5ba0af1c 100644 --- a/tests/tui_gateway/test_review_summary_callback.py +++ b/tests/tui_gateway/test_review_summary_callback.py @@ -18,6 +18,8 @@ @pytest.fixture() def server(): + # Mocks are scoped to the initial import only (see + # tests/tui_gateway/test_protocol.py for the rationale). with patch.dict( "sys.modules", { @@ -32,18 +34,17 @@ def server(): import importlib mod = importlib.import_module("tui_gateway.server") - yield mod - # Reset module-level session state without re-importing. importlib.reload - # would re-register the module's atexit hooks (ThreadPoolExecutor - # shutdown, _shutdown_sessions); the duplicates race the stderr - # buffer at interpreter shutdown and surface as Fatal Python error: - # _enter_buffered_busy. Clearing the per-session dicts gives the - # next test a clean slate; _methods is NOT cleared because it's - # populated at module import time and re-registration only happens - # via reload (which we don't do). - mod._sessions.clear() - mod._pending.clear() - mod._answers.clear() + + yield mod + # Reset module-level session state without re-importing. importlib.reload + # would re-register the module's atexit hooks (ThreadPoolExecutor + # shutdown, _shutdown_sessions); the duplicates race the stderr + # buffer at interpreter shutdown and surface as Fatal Python error: + # _enter_buffered_busy. Clearing the per-session dicts gives the + # next test a clean slate. + mod._sessions.clear() + mod._pending.clear() + mod._answers.clear() def test_init_session_attaches_background_review_callback(server, monkeypatch): diff --git a/tests/tui_gateway/test_subagent_child_mirror.py b/tests/tui_gateway/test_subagent_child_mirror.py index 1f7e7df0c528..81a3f29c85ba 100644 --- a/tests/tui_gateway/test_subagent_child_mirror.py +++ b/tests/tui_gateway/test_subagent_child_mirror.py @@ -17,6 +17,8 @@ @pytest.fixture() def server(): + # Mocks are scoped to the initial import only (see + # tests/tui_gateway/test_protocol.py for the rationale). with patch.dict( "sys.modules", { @@ -31,12 +33,13 @@ def server(): import importlib mod = importlib.import_module("tui_gateway.server") - yield mod - mod._sessions.clear() - mod._pending.clear() - mod._answers.clear() - mod._child_mirrors.clear() - mod._active_child_runs.clear() + + yield mod + mod._sessions.clear() + mod._pending.clear() + mod._answers.clear() + mod._child_mirrors.clear() + mod._active_child_runs.clear() @pytest.fixture() diff --git a/tests/tui_gateway/test_undo_command.py b/tests/tui_gateway/test_undo_command.py index cd0f7e5c4e0b..9440a9c03d04 100644 --- a/tests/tui_gateway/test_undo_command.py +++ b/tests/tui_gateway/test_undo_command.py @@ -34,6 +34,8 @@ def hermes_home(tmp_path, monkeypatch): @pytest.fixture() def server(hermes_home): + # Mocks are scoped to the initial import only (see + # tests/tui_gateway/test_protocol.py for the rationale). with patch.dict( "sys.modules", { @@ -42,17 +44,20 @@ def server(hermes_home): }, ): mod = importlib.import_module("tui_gateway.server") - yield mod - # Reset module-level session state without re-importing. importlib.reload - # would re-register the module's atexit hooks; duplicated hooks race the - # stderr buffer at interpreter shutdown (Fatal Python error: - # _enter_buffered_busy) — same class as PR #34217. - mod._sessions.clear() - mod._pending.clear() - mod._answers.clear() - # NOTE: _methods is intentionally NOT cleared — it's populated at import - # time and would only repopulate via reload. - mod._db = None + + methods = dict(mod._methods) + yield mod + # Restore in place instead of clear+reload: importlib.reload + # re-registers atexit hooks (duplicate ThreadPoolExecutor shutdowns + # race the stderr buffer at interpreter exit — same class as PR #34217) + # and re-captures module-level paths like _hermes_home against this + # test's soon-deleted tmpdir, breaking later files in the same process. + mod._methods.clear() + mod._methods.update(methods) + mod._sessions.clear() + mod._pending.clear() + mod._answers.clear() + mod._db = None @pytest.fixture() From 602a6d3cd2154a3ab82e8747168317f7357690ac Mon Sep 17 00:00:00 2001 From: Jeff Watts <186512915+lEWFkRAD@users.noreply.github.com> Date: Wed, 15 Jul 2026 09:59:40 -0400 Subject: [PATCH 2/2] test(tui): fully tear down leaked fixture sessions --- tests/conftest.py | 24 +++++++++----------- tests/tui_gateway/test_protocol.py | 35 +++++++++++++++++++++++++++++- 2 files changed, 44 insertions(+), 15 deletions(-) diff --git a/tests/conftest.py b/tests/conftest.py index b7533187d117..dfb37e7f5506 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -455,20 +455,17 @@ def _isolate_hermes_home(_hermetic_environment): _TUI_SERVER_MODULE = "tui_gateway.server" -def _release_tui_server_leases(sessions: dict) -> None: - """Release active-session leases held by leftover server sessions. +def _teardown_tui_server_sessions(mod) -> None: + """Close leftover sessions through the production teardown boundary. - Clearing ``_sessions`` without releasing leaks the lease's registry - entry; with the test process alive it is never pruned, so later tests - hit a phantom 'active session limit' against stale entries. + Besides returning active-session leases, this finalizes the session, + unregisters notification state, and closes its agent and slash worker. """ - for session in list(sessions.values()): - lease = session.get("active_session_lease") if isinstance(session, dict) else None - if lease is not None: - try: - lease.release() - except Exception: - pass + sessions = getattr(mod, "_sessions", None) + if not isinstance(sessions, dict): + return + for sid in list(sessions): + mod._close_session_by_id(sid, end_reason="test_cleanup") @pytest.fixture(autouse=True) @@ -494,8 +491,7 @@ def _reset_tui_gateway_server_state(): # (monkeypatch restores the real, pre-test object afterwards anyway). sessions = mod._sessions if isinstance(sessions, dict): - _release_tui_server_leases(sessions) - sessions.clear() + _teardown_tui_server_sessions(mod) for name in ( "_pending", "_pending_prompt_payloads", diff --git a/tests/tui_gateway/test_protocol.py b/tests/tui_gateway/test_protocol.py index 9183f9ec8778..27c053ed8f75 100644 --- a/tests/tui_gateway/test_protocol.py +++ b/tests/tui_gateway/test_protocol.py @@ -51,11 +51,44 @@ def server(): mod._methods.clear() mod._methods.update(methods) mod._real_stdout = real_stdout - mod._sessions.clear() + for sid in list(mod._sessions): + mod._close_session_by_id(sid, end_reason="test_cleanup") mod._pending.clear() mod._answers.clear() +def test_shared_fixture_cleanup_uses_full_session_teardown(server, monkeypatch): + """The cross-file autouse cleanup must close every retained resource.""" + from tests import conftest + + closed = {"worker": 0, "agent": 0, "lease": 0} + + class _Closable: + def __init__(self, key): + self.key = key + + def close(self): + closed[self.key] += 1 + + class _Lease: + def release(self): + closed["lease"] += 1 + + monkeypatch.setattr(server, "_get_db", lambda: None) + server._sessions["leaked"] = { + "session_key": "leaked", + "agent": _Closable("agent"), + "slash_worker": _Closable("worker"), + "active_session_lease": _Lease(), + "history": [], + } + + conftest._teardown_tui_server_sessions(server) + + assert server._sessions == {} + assert closed == {"worker": 1, "agent": 1, "lease": 1} + + @pytest.fixture() def capture(server): """Redirect server's real stdout to a StringIO and return (server, buf)."""