Skip to content

test(gateway): stop the review-summary tests leaking notification pollers too - #336

Merged
OmarB97 merged 1 commit into
mainfrom
fix/review-summary-poller-leak-fork-20260802
Aug 2, 2026
Merged

test(gateway): stop the review-summary tests leaking notification pollers too#336
OmarB97 merged 1 commit into
mainfrom
fix/review-summary-poller-leak-fork-20260802

Conversation

@OmarB97

@OmarB97 OmarB97 commented Aug 2, 2026

Copy link
Copy Markdown
Owner

What does this PR do?

Finishes the notification-poller leak that #335 started. That PR fixed tests/test_tui_gateway_server.py and explicitly left tests/tui_gateway/test_review_summary_callback.py alone; this closes it.

Three tests there call server._init_session(...), which starts a notification poller. The module's server fixture cleans up with:

mod._sessions.clear()

Clearing the registry does not stop a poller. The loop only breaks on _finalized or its stop event, and both are set by _teardown_session, which the fixture never calls — so each of the three tests leaves a daemon thread polling the process-wide process_registry.completion_queue for the rest of the run. With a plugin that counts live _notification_poller_loop threads, the total climbs 1 → 2 → 3 across the file.

Nothing in this file is queue-sensitive today, which is why #335 could defer it. It is still a leak worth closing: the queue is process-wide, so the blast radius is whatever runs next in the same process, and it is the exact shape that produced the load-dependent failures #335 fixed.

Why it couldn't just use the fixture from #335

reap_notification_pollers does from tui_gateway import server at fixture setup. This module's server fixture must be the first thing in the process to import tui_gateway.server — it imports it inside a patch.dict("sys.modules", ...) that swaps in mocked hermes_constants and hermes_state, and that patch only has any effect on the first import. A fixture that imported the module at setup would silently change what these tests exercise.

So the reaping is split out of reap_notification_pollers into a notification_poller_reaper factory fixture that takes the module you already imported:

def server(notification_poller_reaper):
    with patch.dict("sys.modules", {...}):
        mod = importlib.import_module("tui_gateway.server")
        notification_poller_reaper(mod)     # <- track from here on
        yield mod

reap_notification_pollers is now a two-line wrapper over the same factory, so both paths share one implementation and one teardown contract: set both brakes, join the thread, assert nothing survived. No behavior change for test_tui_gateway_server.py.

Related Issue

Follow-up to #335 (the deferred half, called out in that PR's body).

Type of Change

  • ✅ Tests (adding or improving test coverage)

Changes Made

  • tests/conftest.py — extract notification_poller_reaper, a factory fixture yielding track(server_module); reimplement reap_notification_pollers on top of it. Patching still goes through the shared monkeypatch, so a test patching the same attribute unwinds in the right order and can't leave the wrapper installed.
  • tests/tui_gateway/test_review_summary_callback.py — the server fixture takes notification_poller_reaper and calls it right after its own import, with a comment recording why it can't use the module-wide fixture.

No product code changes. Production still has no equivalent path: every _pop_session_by_id is paired with _teardown_popped_session_teardown_session_finalize_session.

How to Test

pytest tests/tui_gateway/test_review_summary_callback.py -q
  • Before: live _notification_poller_loop threads accumulate 1 → 2 → 3 across the file and all three survive it.
  • After: the count never exceeds the poller belonging to the running test, and the factory's teardown assertion (notification poller thread(s) still running: [...]) fires if reaping ever regresses — the three _init_session tests exercise that path on every run.
  • The import-order property is preserved: the fixture still performs the first tui_gateway.server import inside its patch.dict, and the reaper imports nothing but threading.

Checklist

Code

  • I've read the Contributing Guide
  • My commit messages follow Conventional Commits
  • I searched for existing PRs to make sure this isn't a duplicate
  • My PR contains only changes related to this fix
  • I've run the affected suites and they pass (tests/tui_gateway, tests/test_tui_gateway_server.py, tests/test_profile_isolation_runtime.py)
  • I've added tests for my changes — N/A, this is a test-hygiene fix; the factory's teardown assertion is the guard and the three existing _init_session tests drive it
  • I've tested on my platform: macOS 15 (Darwin 25.6.0)

Documentation & Housekeeping

  • I've updated relevant documentation (README, docs/, docstrings) — N/A (test-only; the fixture docstrings carry the rationale)
  • I've updated cli-config.yaml.example if I added/changed config keys — N/A
  • I've updated CONTRIBUTING.md or AGENTS.md if I changed architecture or workflows — N/A
  • I've considered cross-platform impact — N/A (stdlib threading only)
  • I've updated tool descriptions/schemas if I changed tool behavior — N/A

…lers too

Finishes what #335 started. That PR fixed `tests/test_tui_gateway_server.py`
and explicitly deferred `tests/tui_gateway/test_review_summary_callback.py`.

Three tests there call `_init_session`, which starts a notification poller. The
module's `server` fixture cleans up with `mod._sessions.clear()`, and clearing
the registry does not stop a poller: the loop only breaks on `_finalized` or
its stop event, both of which only `_teardown_session` sets. So each of the
three leaves a daemon thread polling the process-wide
`process_registry.completion_queue` for the rest of the run — with a plugin
counting live `_notification_poller_loop` threads, the total climbs 1 -> 2 -> 3
across the file.

Nothing in this file is queue-sensitive today, which is why #335 could defer
it, but the queue is process-wide so the blast radius is whatever runs next in
the same process — the exact shape that produced the load-dependent failures
#335 fixed.

It cannot simply use `reap_notification_pollers`: that fixture does
`from tui_gateway import server` at setup, and this module's fixture has to be
the process's FIRST importer of it. It imports inside a `patch.dict` that swaps
in mocked `hermes_constants` / `hermes_state`, and that patch only affects the
first import — so a fixture importing at setup would silently change what these
tests exercise.

Split the reaping out of `reap_notification_pollers` into a
`notification_poller_reaper` factory that takes the module you already
imported; this file calls it right after its own import.
`reap_notification_pollers` becomes a thin wrapper over the same factory, so
both paths share one implementation and one teardown contract: set both brakes,
join the thread, assert nothing survived. No behavior change for
`test_tui_gateway_server.py`, and no product code changes.

Verified by breaking the reaper on purpose: exactly the three `_init_session`
tests then error in teardown with "notification poller thread(s) still
running", so the reaping is engaged here rather than a silent no-op.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@OmarB97
OmarB97 merged commit 77f7e2b into main Aug 2, 2026
35 checks passed
OmarB97 pushed a commit that referenced this pull request Aug 2, 2026
…anch

main advanced while this branch was in flight, and #338 ("wire the local
stream stale ceiling the config already promises") landed in the same
stale-timeout logic. Three conflicts, none of them a behavior collision:

* agent/chat_completion_helpers.py — docstring only. Both sides appended to
  the same paragraph in _dflash_prefill_scaled_timeout; main's text is a
  superset of ours, adding a cross-reference to _generic_local_stale_timeout.
  Took main's. Verified that function exists in the merged tree (line 630) and
  that it really is widened by the per-1k prefill term, so the reference it
  adds is accurate rather than aspirational.

* website/docs/user-guide/configuration.md — same sentence on both sides,
  differing only in the example provider name. Took main's `my-local-lane`:
  it is the entry actually used in cli-config.yaml.example, whereas
  `ai-router` appeared nowhere else in the docs.

* tests/agent/test_local_stream_timeout.py — a tail add/add. Our side
  contributes no lines at the conflict point; main appends
  TestGenericLocalStreamStaleCeiling. Kept both bodies by dropping the
  markers, so our first-chunk resolver tests above the seam and main's new
  class below it both survive.

No production logic was resolved away — the only code hunk was a docstring.

Verified on the merge result, not on either parent:
  pytest tests/hermes_cli/test_timeouts.py \
         tests/agent/test_request_prefix_stability.py \
         tests/run_agent/test_provider_parity.py \
         tests/agent/test_local_stream_timeout.py \
         tests/agent/test_reasoning_stale_timeout_floor.py \
         tests/agent/test_stream_read_timeout_floor.py -q
  343 passed

Merged rather than rebased on purpose: another session is active on this
branch with uncommitted work, and a force-push would have destroyed it.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant