Skip to content

fix(ci): make pre-push single-flight runner start on hosts without POSIX signals (#9724)#9730

Open
kodjima33 wants to merge 1 commit into
mainfrom
watchdog/issue-9724-preflight-runner-windows-signals
Open

fix(ci): make pre-push single-flight runner start on hosts without POSIX signals (#9724)#9730
kodjima33 wants to merge 1 commit into
mainfrom
watchdog/issue-9724-preflight-runner-windows-signals

Conversation

@kodjima33

@kodjima33 kodjima33 commented Jul 14, 2026

Copy link
Copy Markdown
Collaborator

Fixes #9724.

Bug

On Windows, every git push fails before any pre-push check runs:

AttributeError: module 'signal' has no attribute 'SIGHUP'

scripts/pre-push-singleflight routes through .github/scripts/preflight_runner.py, whose run_owned() built its handler map from (signal.SIGINT, signal.SIGTERM, signal.SIGHUP) unconditionally. Windows Python does not define SIGHUP, so the wrapper exits 1 and Git rejects the push.

Root cause / failure class

The runner assumes POSIX process APIs are always present. Fixing only the signal tuple would leave two more landmines on the same code path, so this PR closes the class:

  1. Signal registrationowned_signals() registers only signals the host actually defines, and picks up SIGBREAK where it exists (Windows' console-interrupt signal).
  2. Signal forwardingsignal_child() uses os.killpg where process groups exist (POSIX children get their own session via start_new_session), and falls back to child.terminate() elsewhere.
  3. Liveness probeprocess_exists() used os.kill(pid, 0). On Windows os.kill does not send a signal; it calls TerminateProcess. The moment the runner became reachable on Windows, the stale-lock probe would have killed the live lock holder. Windows now probes with OpenProcess/GetExitCodeProcess (access-denied ⇒ alive, mirroring the POSIX PermissionError branch).

Regression test

.github/scripts/test_preflight_runner.py (9 tests) exercises signal selection against Windows- and POSIX-shaped signal modules, forwarding on hosts with and without process groups, and asserts process_exists never reaches os.kill on Windows — no real signal needed to interrupt the test process, per the issue. Registered in .github/checks-manifest.yaml (preflight-runner-portability, local + ci lanes), so it is diff-scoped-selected whenever the runner changes.

Verification

  • python3 .github/scripts/test_preflight_runner.py → 9 tests OK
  • python3 .github/scripts/test_run_checks.py → 9 tests OK (manifest contract guard)
  • make preflight on this branch → 9 checks passed, with preflight-runner-portability SELECTED by the diff
  • Real run: preflight_runner.py --name wd -- sh -c 'echo "==> phase-one"; echo hello; exit 3' streamed the phase line, wrote result.json, exited 3
  • A/B against HEAD: SIGINT to the runner kills the child process group in both the old and the new code — POSIX forwarding behavior is unchanged
  • Old expression against a Windows-shaped signal module raises AttributeError: 'SIGHUP'; owned_signals() returns (SIGINT, SIGTERM, SIGBREAK)

Not verified on a real Windows host (no Windows machine available) — the Windows paths are covered by the hermetic tests above and by CPython's documented os.kill/OpenProcess semantics.

🤖 automated by hourly watchdog; opened for review, not merged.

Review in cubic

…SIX signals (#9724)

run_owned() built its handler map from (SIGINT, SIGTERM, SIGHUP) unconditionally.
Windows Python has no SIGHUP, so every git push died with AttributeError before the
pre-push checks started. Close the whole class of POSIX-only process APIs in this file:

- owned_signals() registers only signals the host defines (and picks up SIGBREAK).
- signal_child() forwards via os.killpg where process groups exist, else terminate().
- process_exists() no longer probes with os.kill(pid, 0) on Windows, where os.kill
  TerminateProcess()es the target — a liveness probe would have killed the lock holder
  the moment the runner became reachable on Windows.

Verification:
- python3 .github/scripts/test_preflight_runner.py -> 9 tests OK
- python3 .github/scripts/test_run_checks.py -> 9 tests OK (manifest contract)
- make preflight -> 8 checks passed
- Real run: `preflight_runner.py --name wd -- sh -c 'echo "==> phase-one"; exit 3'`
  streamed the phase, wrote result.json, exited 3.
- A/B against HEAD: SIGINT to the runner kills the child process group in both the
  old and new code -> POSIX forwarding is unchanged.
- Old expression vs a Windows-shaped signal module raises AttributeError: 'SIGHUP';
  owned_signals() returns (SIGINT, SIGTERM, SIGBREAK).

@tianmind-studio tianmind-studio left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Blocking on a real Windows host: bash scripts/pre-push-singleflight fork still exits before any pre-push check starts. After the signal fix succeeds, the wrapper hands the extensionless Bash script scripts/pre-push directly to Python's subprocess.Popen; Windows CreateProcess cannot launch it, and CPython 3.11.15 raises FileNotFoundError: [WinError 2] at run_owned.

Please pass the active Bash executable explicitly (for example, -- "$BASH" scripts/pre-push "$@" in scripts/pre-push-singleflight) and cover that wrapper contract. As written, this changes the first Windows failure but does not restore git push.

@tianmind-studio

Copy link
Copy Markdown
Contributor

Thanks for the signal/process work here. I reproduced the next Windows failure on this head and opened #9804 directly against your branch; it passes the active Bash executable to the runner, makes the existing POSIX simulations runnable on Windows, and adds the wrapper to the portability check trigger.

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.

Pre-push single-flight runner crashes on Windows without SIGHUP

2 participants