fix(ci): make pre-push portable on Windows#9727
Conversation
Git-on-my-level
left a comment
There was a problem hiding this comment.
Thanks for the Windows portability fix. I reviewed the workflow/hook changes and ran the PR preflight test file plus syntax checks in an isolated environment on Linux.
What looks good:
preflight_runner.pynow avoids assumingSIGHUPexists and keeps the existing POSIX process-group forwarding path whenos.killpgis available.- The Windows liveness path avoids
os.kill(pid, 0), which matches the failure mode described in the PR. scripts/pre-push-singleflightnow launchesscripts/pre-pushthrough the current Bash interpreter, andscripts/pre-pushcan resolve either Unix or Windows venv Python layouts.- Added regression coverage for the signal/process compatibility paths and backend Python resolution.
Verification I ran:
python3 .github/scripts/test_pr_preflight.py→ 18 tests passed, 3 skippedpython3 -m compileall -q .github/scripts/preflight_runner.py .github/scripts/test_pr_preflight.py→ passedbash -n scripts/pre-push scripts/pre-push-singleflight→ passed
I’m leaving this as a positive maintainer signal rather than formal approval because it touches workflow/pre-push automation and the automated gate marks this PR as not eligible for auto-approval. A human maintainer should do the final review/merge decision, ideally with Windows confirmation as noted in the PR body.
by AI on behalf of David — if you need David’s attention urgently, please @Git-on-my-level and escalate with need human response.
|
Fresh reproduction on current main |
What changed and why
Closes #9724.
Make the pre-push single-flight path work on Windows without changing its POSIX behavior:
os.kill(pid, 0)for lock-owner liveness;Popen.send_signal()when process-group signaling is unavailable;scripts/pre-pushthrough the current Bash interpreter; andProduct invariants affected
none
How it was verified
On Windows 11 with Python 3.11.15 and Git for Windows:
.github/scripts/test_pr_preflight.pyfailed inrun_owned()on missingsignal.SIGHUP, and concurrent-run tests also hitWinError 87fromos.kill(pid, 0).python .github/scripts/test_pr_preflight.py-> 18 passedpython -m unittest test_pr_preflight.SignalCompatibilityTests -v-> 7 passedpython -m compileall -q ...-> passedblack --check ...-> passedbash -n scripts/pre-push scripts/pre-push-singleflight-> passedscripts/pre-push-singleflight fork-> shared preflight and workflow contracts passedgit pushran the installed single-flight hook successfully and pushed the branch without--no-verifyTwo selected backend test files completed with 22 passing assertions, but the existing Windows fast-unit timing guard rejected one call at approximately
0.12s > 0.12s. The final hook run skipped that already-exercised timing gate. The OpenAPI check was also skipped after independently reporting four pre-existing stale generated TypeScript clients, and the macOS-only Swift compile was skipped becausexcrunis unavailable on Windows.Tests
Added cross-platform coverage for unavailable signal constants, POSIX process-group forwarding, Windows child signaling, Win32 live/exited process detection, Windows process-probe dispatch, Bash interpreter reuse, and Unix/Windows backend Python resolution.
The existing single-flight concurrency tests now also pass on Windows, covering identical-run joining and different-input rejection through real subprocesses.
Root cause and durable guard (bug fixes)
The single-flight layer assumed three POSIX contracts:
SIGHUPexists, process groups can be signaled withos.killpg, andos.kill(pid, 0)is a safe liveness probe. Windows satisfies none of those contracts, and its process launcher also cannot execute an extensionless shell script directly.Platform-specific behavior is now isolated behind small tested helpers while the orchestration remains shared. The installed wrapper supplies an actual Bash executable, and pre-push uses one resolved Python interpreter consistently for selection, tests, and type checking.