Skip to content

fix(scripts): isolate parallel-runner pytest children from console Ctrl+C on Windows - #945

Open
hashbender wants to merge 1 commit into
mainfrom
mirror/pr-57181
Open

fix(scripts): isolate parallel-runner pytest children from console Ctrl+C on Windows#945
hashbender wants to merge 1 commit into
mainfrom
mirror/pr-57181

Conversation

@hashbender

Copy link
Copy Markdown
Owner

What does this PR do?

Makes full-suite runs of scripts/run_tests_parallel.py survivable on native Windows with the project's Python 3.11 venv.

The runner relied on start_new_session=True to isolate its per-file pytest children — but on Windows that flag only maps to CREATE_NEW_PROCESS_GROUP starting 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 with os.kill(pid, 0) being a GenerateConsoleCtrlEvent broadcast on Windows (bpo-14484) rather than a liveness probe, a single probe anywhere in the run sprayed KeyboardInterrupt into every concurrent child and the runner itself — observed as the runner dying at 100% completion with ~200 collateral KeyboardInterrupt failures per full-suite run.

The fix passes explicit creationflags=CREATE_NEW_PROCESS_GROUP | CREATE_NO_WINDOW on 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), keeping start_new_session=True on POSIX where _kill_tree's killpg cleanup needs it.

It also fixes the one unguarded broadcaster the audit found: tests/tools/test_zombie_process_cleanup.py probed its three spawned sleep(60) children with bare os.kill(pid, 0) on all platforms (now psutil.pid_exists(), per the CONTRIBUTING rule), and its cleanup used os.kill(pid, signal.SIGKILL)signal.SIGKILL doesn't exist on Windows (now portable Popen.kill()). The other os.kill(pid, 0) call sites in tests are already win32-skipped; production call sites were migrated previously.

Related Issue

Fixes NousResearch#57145

Type of Change

  • 🐛 Bug fix (non-breaking change that fixes an issue)

Changes Made

  • scripts/run_tests_parallel.py — platform-split isolation kwargs for the per-file pytest Popen: explicit CREATE_NEW_PROCESS_GROUP | CREATE_NO_WINDOW creationflags on win32, start_new_session=True on POSIX.
  • tests/tools/test_zombie_process_cleanup.py — probe via psutil.pid_exists() instead of os.kill(pid, 0); clean up via Popen.kill() instead of the Windows-nonexistent signal.SIGKILL.
  • tests/test_run_tests_parallel.py — new regression test test_children_spawn_in_their_own_process_group asserting the per-platform Popen isolation kwargs.

How to Test

  1. On native Windows with a Python 3.11 venv, run python scripts/run_tests_parallel.py -q — before this change the runner intermittently died near completion with mass collateral KeyboardInterrupt failures whenever a test performed an os.kill(pid, 0) probe; with it, the run completes.
  2. python -m pytest tests/test_run_tests_parallel.py tests/tools/test_zombie_process_cleanup.py -q on both Windows and POSIX.
  3. python scripts/check-windows-footguns.py --all stays green.

Checklist

Code

  • I've read the Contributing Guide
  • My commit messages follow Conventional Commits (fix(scope):, feat(scope):, etc.)
  • I searched for existing PRs to make sure this isn't a duplicate
  • My PR contains only changes related to this fix/feature (no unrelated commits)
  • I've run pytest tests/ -q and all tests pass — full suite via scripts/run_tests_parallel.py on 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 plain python -m pytest with 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 NousResearch/hermes-agent#57149), tests/tools/test_zombie_process_cleanup.py (13/13).
  • I've added tests for my changes (required for bug fixes, strongly encouraged for features)
  • I've tested on my platform: Windows 11 Pro (native, no WSL), CPython 3.11.15

Documentation & Housekeeping

  • I've updated relevant documentation (README, docs/, docstrings) — the Popen comment block documents the trap in place
  • 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 (Windows, macOS) per the compatibility guide — POSIX behavior is byte-identical (start_new_session=True as before)
  • I've updated tool descriptions/schemas if I changed tool behavior — N/A

Mirror-of: NousResearch#57181
NousResearch#57181

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.

Windows: parallel test runner children share the console process group — one os.kill(pid, 0) probe broadcasts KeyboardInterrupt across the whole run

1 participant