fix(tests): reap leaked TUI notification pollers between tests - #86427
Merged
Merged
Conversation
test_run_prompt_submit_requeues_all_unstarted_notifications_with_real_threading
failed twice in one hour on CI slices for two UNRELATED PRs (86371,
86374) with `assert set() == {proc_batch_2, proc_batch_3}`. Root cause:
session.init/create tests earlier in the file start real per-session
notification poller daemon threads and never stop them. Those pollers
outlive their test and keep polling the PROCESS-GLOBAL
process_registry.completion_queue, stealing-and-requeuing the target
test's events mid-assertion so its bounded drain loop can starve.
Reproduced: with 30 leaked foreign-session pollers injected via a
sabotage conftest, the target test fails standalone ~1 in 3 runs with
the exact CI assertion; with the reap fixture active it passed 8/8
under the same sabotage.
Fix:
- tui_gateway/server.py: _start_notification_poller registers
(stop_event, thread) in module-level _notification_pollers (pruned of
dead threads on each spawn; threads get a stable
tui-notif-poller-<sid> name for debugging).
- tests/test_tui_gateway_server.py: autouse fixture sets every
registered live poller's stop event after each test and joins them
under ONE shared 3s budget (the poller loop wakes at least every
0.5s), so no poller survives into the next test. No per-thread
timeout, no session-dict mutation — a first draft that mutated
session state and joined per-thread hung the file; full-file runtime
with this version is 15.2s vs 13.2s baseline.
Contributor
૮ >ﻌ< ა ci reviewran on ce6786e — fix(tests): reap leaked TUI notification pollers between tes
|
alt-glitch
added
type/test
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Kills the flaky
test_run_prompt_submit_requeues_all_unstarted_notifications_with_real_threading— it failed on CI twice in one hour on two unrelated PRs (#86371, #86374) withassert set() == {'proc_batch_2', 'proc_batch_3'}while passing locally 14/14.Root cause: session.init/create tests earlier in the file start real per-session notification poller daemon threads and never stop them. Leaked pollers keep polling the process-global
process_registry.completion_queueand steal-and-requeue the target test's events mid-assertion, starving its bounded drain loop.Reproduced deterministically-ish: 30 leaked foreign-session pollers injected via a sabotage conftest → target test fails standalone ~1 in 3 with the exact CI assertion; with the reap fixture active it passed 8/8 under the same sabotage.
Changes
tui_gateway/server.py:_start_notification_poller()registers(stop_event, thread)in a module-level_notification_pollerslist (dead entries pruned on each spawn); poller threads get a stabletui-notif-poller-<sid>name.tests/test_tui_gateway_server.py: autouse fixture sets every registered live poller's stop event after each test and joins under ONE shared 3s budget. No per-thread timeouts, no session-dict mutation (a first draft that did both hung the file).Validation
scripts/run_tests.sh)Infographic