fix(scripts): isolate parallel-runner pytest children from console Ctrl+C on Windows - #57181
Open
lEWFkRAD wants to merge 1 commit into
Open
fix(scripts): isolate parallel-runner pytest children from console Ctrl+C on Windows#57181lEWFkRAD wants to merge 1 commit into
lEWFkRAD wants to merge 1 commit into
Conversation
…rl+C on Windows scripts/run_tests_parallel.py relied on start_new_session=True for child isolation, but on Windows that flag only maps to CREATE_NEW_PROCESS_GROUP in CPython 3.12+ — on 3.11 it is silently ignored, so every per-file pytest child shared the runner's console process group. os.kill(pid, 0) on Windows is not a no-op: it routes through GenerateConsoleCtrlEvent (bpo-14484), so a single liveness probe during a test broadcast KeyboardInterrupt to every concurrent child AND the runner itself (observed: runner died at 100% completion with ~200 collateral KeyboardInterrupt failures per full-suite run). - Pass creationflags=CREATE_NEW_PROCESS_GROUP | CREATE_NO_WINDOW explicitly on win32 (each child is its own ctrl-event group root, on its own invisible console); keep start_new_session=True on POSIX. - Fix the one unguarded os.kill(pid, 0) prober that fires on Windows: tests/tools/test_zombie_process_cleanup.py now probes via psutil.pid_exists() and cleans up via Popen.kill() (the old cleanup used signal.SIGKILL, which does not exist on Windows). - Add a regression test asserting the Popen isolation kwargs per platform. Found during the PR NousResearch#57066 / issue NousResearch#57068 triage. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
13 tasks
This was referenced Jul 3, 2026
Contributor
|
Thanks for tracing the Windows process-group failure; current Problems
Suggested changes
Automated hermes-sweeper review. |
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
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.
What does this PR do?
Makes full-suite runs of
scripts/run_tests_parallel.pysurvivable on native Windows with the project's Python 3.11 venv.The runner relied on
start_new_session=Trueto isolate its per-file pytest children — but on Windows that flag only maps toCREATE_NEW_PROCESS_GROUPstarting with CPython 3.12 (the comment at the Popen call already admitted this). On 3.11 it is silently ignored, so every child shared the runner's console process group. Combined withos.kill(pid, 0)being aGenerateConsoleCtrlEventbroadcast on Windows (bpo-14484) rather than a liveness probe, a single probe anywhere in the run sprayedKeyboardInterruptinto every concurrent child and the runner itself — observed as the runner dying at 100% completion with ~200 collateralKeyboardInterruptfailures per full-suite run.The fix passes explicit
creationflags=CREATE_NEW_PROCESS_GROUP | CREATE_NO_WINDOWon win32 (each child becomes its own ctrl-event group root on its own invisible console — ctrl-event broadcasts can no longer fan out across children, and per-child conhost flashes disappear as a bonus), keepingstart_new_session=Trueon POSIX where_kill_tree'skillpgcleanup needs it.It also fixes the one unguarded broadcaster the audit found:
tests/tools/test_zombie_process_cleanup.pyprobed its three spawnedsleep(60)children with bareos.kill(pid, 0)on all platforms (nowpsutil.pid_exists(), per the CONTRIBUTING rule), and its cleanup usedos.kill(pid, signal.SIGKILL)—signal.SIGKILLdoesn't exist on Windows (now portablePopen.kill()). The otheros.kill(pid, 0)call sites in tests are already win32-skipped; production call sites were migrated previously.Related Issue
Fixes #57145
Type of Change
Changes Made
scripts/run_tests_parallel.py— platform-split isolation kwargs for the per-file pytest Popen: explicitCREATE_NEW_PROCESS_GROUP | CREATE_NO_WINDOWcreationflags on win32,start_new_session=Trueon POSIX.tests/tools/test_zombie_process_cleanup.py— probe viapsutil.pid_exists()instead ofos.kill(pid, 0); clean up viaPopen.kill()instead of the Windows-nonexistentsignal.SIGKILL.tests/test_run_tests_parallel.py— new regression testtest_children_spawn_in_their_own_process_groupasserting the per-platform Popen isolation kwargs.How to Test
python scripts/run_tests_parallel.py -q— before this change the runner intermittently died near completion with mass collateralKeyboardInterruptfailures whenever a test performed anos.kill(pid, 0)probe; with it, the run completes.python -m pytest tests/test_run_tests_parallel.py tests/tools/test_zombie_process_cleanup.py -qon both Windows and POSIX.python scripts/check-windows-footguns.py --allstays green.Checklist
Code
fix(scope):,feat(scope):, etc.)pytest tests/ -qand all tests pass — full suite viascripts/run_tests_parallel.pyon native Windows now completes end-to-end (before this fix the runner itself died near 100% with ~200 collateral KeyboardInterrupt failures). The remaining failures are pre-existing native-Windows failures unrelated to this change: spot-checked files (e.g.tests/tools/test_file_tools.py,tests/tools/test_local_background_child_hang.py) reproduce identical failure counts under plainpython -m pytestwith no runner involved. Targeted suites for the touched files are green:tests/test_run_tests_parallel.py(all but one pre-existing Windows failure tracked in run_tests_parallel.py on Windows: ':' path-list split breaks drive letters; per-file progress lines lost to UnicodeEncodeError on piped stdout #57149),tests/tools/test_zombie_process_cleanup.py(13/13).Documentation & Housekeeping
docs/, docstrings) — the Popen comment block documents the trap in placecli-config.yaml.exampleif I added/changed config keys — N/ACONTRIBUTING.mdorAGENTS.mdif I changed architecture or workflows — N/Astart_new_session=Trueas before)