Skip to content

test(gateway): stop a leaked notification poller stealing the next test's events - #335

Merged
OmarB97 merged 1 commit into
mainfrom
fix/leaked-notification-poller-tests-fork-20260802
Aug 2, 2026
Merged

test(gateway): stop a leaked notification poller stealing the next test's events#335
OmarB97 merged 1 commit into
mainfrom
fix/leaked-notification-poller-tests-fork-20260802

Conversation

@OmarB97

@OmarB97 OmarB97 commented Aug 2, 2026

Copy link
Copy Markdown
Owner

What does this PR do?

tests/test_tui_gateway_server.py fails intermittently under load, always in the same family:

FAILED test_run_prompt_submit_requeues_all_unstarted_notifications_with_real_threading
FAILED test_run_prompt_submit_requeues_foreign_completion[0]
FAILED test_notification_poller_live_loop_requeues_foreign_completion_for_owner

The cause is a leaked thread, not a product bug. _start_notification_poller spawns a daemon thread that loops until its session is finalized:

while not stop_event.is_set() and not session.get("_finalized"):
    evt = process_registry.completion_queue.get(timeout=0.5)

Four tests in this file register a session straight into server._sessions and pop it again without going through _teardown_session, so _finalized is never set and _notif_stop is never fired — the poller runs for the rest of the pytest process:

  • test_session_resume_profile_uses_profile_db_cwd
  • test_init_session_fires_reset_hook
  • test_session_create_no_race_keeps_worker_alive
  • test_session_create_continues_when_state_db_is_unavailable

By the time the notification tests run, four are live. process_registry.completion_queue is process-wide and the loop re-reads the attribute every iteration, so when a later test monkeypatches an isolated Queue in, the leaked pollers see it too and compete for its events.

Reproduced deterministically — one leaked poller, one poll cycle, and the event a test expected to still be there is gone:

leaked = _sess("leaked-session")
server._sessions["leaked-sid"] = leaked
server._start_notification_poller("leaked-sid", leaked)
server._sessions.pop("leaked-sid", None)      # no _teardown_session -> no _finalized

isolated = queue.Queue(); isolated.put(event)
monkeypatch.setattr(process_registry, "completion_queue", isolated)
time.sleep(0.7)                                # one poll cycle
assert isolated.qsize() == 1                   # FAILS: qsize is 0

Depending on where the leaked poller's own session sits, the event is dropped as unowned, or bounced out and re-queued 0.1s later, or simply held past the test's deadline. All three land as the failures above, which is why they only show up when the machine is busy enough for the poller to win the race.

This is test-only. Production has no equivalent path: every _pop_session_by_id is immediately paired with _teardown_popped_session_teardown_session_finalize_session, which sets _finalized and fires the stop event. No product code is changed here.

Related Issue

No issue — found while verifying #333, where this family failed once in a contended run and again on an unmodified tree at -j 8.

Type of Change

  • ✅ Tests (adding or improving test coverage)

Changes Made

  • tests/conftest.py — new opt-in reap_notification_pollers fixture. It wraps _start_notification_poller, records the stop event, session and thread for each poller a test starts, and at teardown sets both brakes (_finalized and the event) and joins the thread. Joining is the point: setting the event alone still leaves the poller parked in a 0.5s queue.get(), long enough to take one more event off the next test's queue. It asserts nothing survived. Not autouse — it takes the shared monkeypatch so a test patching the same attribute unwinds in the right order, and per tests/conftest.py's own note, intra-file ordering is the test author's job.
  • tests/test_tui_gateway_server.py — opt in via pytestmark, next to the existing _neuter_agent_prewarm_timer fixture that solves the same class of problem (one test's background thread landing in the next test's assertions).
  • tests/test_tui_gateway_server.py — new test_a_started_notification_poller_does_not_outlive_its_test, which leaks a poller on purpose so the reaping path runs on every CI pass. If reaping regresses it fails there by name, instead of resurfacing as a load-dependent flake somewhere else.
  • Corrected the now-stale tolerance comment in test_run_prompt_submit_requeues_all_unstarted_notifications_with_real_threading, which told the next reader that leaked pollers stealing from this queue was legitimate.

tests/tui_gateway/test_review_summary_callback.py leaks three pollers the same way but is deliberately left alone: it has no queue-sensitive tests, and its server fixture must be the first thing in the process to import tui_gateway.server (it imports under a patched sys.modules), which requesting this fixture would break.

How to Test

pytest tests/test_tui_gateway_server.py -q
  • Before: with a plugin that counts live _notification_poller_loop threads, the total climbs 1 → 2 → 3 → 4 across the run, and the file fails intermittently.
  • After: the total never exceeds the poller belonging to the test currently running, and the file passes (390 tests).
  • Break the reaper on purpose (drop the stop.set()) and the new guard test fails in teardown with notification poller thread(s) still running: ['Thread-5 (_notification_poller_loop)'].

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, tests/tools/test_process_registry.py)
  • I've added tests for my changes
  • 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 and comments 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

…st's events

`tests/test_tui_gateway_server.py` fails intermittently under load, always in
the same family: `test_run_prompt_submit_requeues_all_unstarted_notifications_
with_real_threading`, `test_run_prompt_submit_requeues_foreign_completion[0]`,
`test_notification_poller_live_loop_requeues_foreign_completion_for_owner`.

`_start_notification_poller` spawns a daemon thread that loops until its
session is finalized. Four tests here register a session straight into
`server._sessions` and pop it again without going through `_teardown_session`,
so `_finalized` is never set and `_notif_stop` is never fired — the poller runs
for the rest of the pytest process:

- test_session_resume_profile_uses_profile_db_cwd
- test_init_session_fires_reset_hook
- test_session_create_no_race_keeps_worker_alive
- test_session_create_continues_when_state_db_is_unavailable

`process_registry.completion_queue` is process-wide and the loop re-reads the
attribute every iteration, so when a later test monkeypatches an isolated Queue
in, all four leaked pollers see it and compete for its events. Reproduced
deterministically with a single leaked poller: put one event on the isolated
queue, sleep one 0.5s poll cycle, and it is gone. Depending on where the leaked
poller's own session sits the event is dropped as unowned, bounced out and
re-queued 0.1s later, or just held past the test's deadline — which is why this
only shows up when the machine is busy enough for the poller to win the race.

Test-only: production has no such path. Every `_pop_session_by_id` is
immediately paired with `_teardown_popped_session` -> `_teardown_session` ->
`_finalize_session`, which sets `_finalized` and fires the stop event.

Add an opt-in `reap_notification_pollers` fixture that records each poller a
test starts and, at teardown, sets both brakes and JOINS the thread — setting
the event alone leaves it parked in a 0.5s `queue.get()`, long enough to take
one more event off the next test's queue. It takes the shared `monkeypatch` so
a test patching the same attribute unwinds in the right order. Not autouse:
`tests/conftest.py` states that intra-file ordering is the test author's job,
and making it global would import `tui_gateway.server` for every test.

`test_a_started_notification_poller_does_not_outlive_its_test` leaks a poller on
purpose so the reaping path runs on every pass; break the reaper and it fails in
teardown by name rather than resurfacing as a flake elsewhere. Also corrects the
tolerance comment in the real-threading test, which told the next reader that
leaked pollers stealing from this queue was legitimate.

`tests/tui_gateway/test_review_summary_callback.py` leaks three the same way but
is left alone: it has no queue-sensitive tests, and its `server` fixture must be
the first importer of `tui_gateway.server` (it imports under a patched
`sys.modules`), which requesting this fixture would break.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@OmarB97
OmarB97 merged commit 4cda854 into main Aug 2, 2026
32 checks passed
OmarB97 added a commit that referenced this pull request Aug 2, 2026
…lers too (#336)

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: Omar Baradei <omar@kostudios.io>
Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
OmarB97 added a commit that referenced this pull request Aug 2, 2026
…rder (#339)

Follow-up to #335. That PR removed the leaked notification pollers that were
rotating the process-wide `completion_queue` under load. This removes the
assumption in `test_run_prompt_submit_requeues_all_unstarted_notifications_
with_real_threading` that made the test sensitive to rotation in the first
place, and turns it into coverage that runs on every pass.

The test hardcoded `proc_batch_1` as the one prompt its agent blocks on, so it
only ever asserted the requeue contract for one starting order. With batch_1
anywhere but first, a *non-blocking* notification is dispatched instead; that
turn finishes, its own post-turn drain dispatches the next, and the cascade
consumes all three — so the queue is empty and the assertion sees `set()`
instead of `{proc_batch_2, proc_batch_3}`. That is the failure #335 diagnosed
from the other end, and it is what the first-attempt traceback showed:
the failure was the final drain assertion, not the `nested_started.wait(5)`
above it, so no timeout needed widening.

The ordering assumption survives #335 — pre-rotating the queue by one on
current main fails 3/3 runs with that exact signature. Nothing rotates it
today, but nothing in the test says it must not.

Block on whichever notification the batch dispatches first and assert the ones
it did not start come back, so the assertion follows the dispatch instead of
predicting it. Parametrize over all three rotations: the contract holds for
each, and the previously-unexercised orders now run every time rather than
waiting for a busy machine. Re-pinning the hold to `proc_batch_1` fails
rotations 1 and 2 while 0 passes, so the guard is engaged rather than
decorative.

Two smaller fixes in the same test:

- The agent's hold was `release_nested.wait(timeout=5)` while the assertion
  window below it is also 5s. Those overlap: if the hold expires first the
  turn ends mid-assert and its post-turn drain consumes the events being
  asserted on. Raised to 60s — the `finally` releases it, so the bound is only
  reached when the test is already failing.
- Added an assertion that exactly one notification turn reached the agent. The
  exact-set check alone passes if the events are consumed and re-queued; this
  is what distinguishes "requeued, unstarted" from "started and put back".

Test-only; no product code changes.

Co-authored-by: Omar Baradei <omar@kostudios.io>
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