Kill orphaned workers before the rebuild watchdog exits - #3397
Kill orphaned workers before the rebuild watchdog exits#3397ayushcodes10 wants to merge 2 commits into
Conversation
os._exit(1) terminates only the rebuild process itself, not any ProcessPoolExecutor worker it spawned for a large corpus. It skips every cleanup path, including the pool's own context-manager shutdown, so a worker still running at the moment the watchdog fires is orphaned, reparented to PID 1 on POSIX, and keeps going unsupervised for as long as whatever it was doing takes. A worker stuck in catastrophic regex backtracking, the exact shape Graphify-Labs#3341 fixed, was observed surviving 2.5 days that way on macOS. extract.py already documents this risk for its own 1 worker special case, but that only covers max_workers == 1, not a real multi worker pool. The watchdog fires on a separate timer thread with no reference to the pool object, but multiprocessing.active_children() enumerates every live worker process regardless of which thread asks, so the fallback kills them before exiting itself. Kill, not terminate: a worker stuck in a C level call like regex backtracking never gets a chance to act on a signal a Python level handler would need to catch. Fixes Graphify-Labs#3396.
|
@safishamsi opened this against #3396, filed separately from @Yo-TR's comment on #3341 about an orphaned ProcessPoolExecutor worker surviving 2.5 days after the rebuild watchdog's os._exit. Verified the kill mechanism directly against a worker stuck in the exact reported failure mode (catastrophic regex backtracking, a C-level call SIGTERM can't preempt but SIGKILL can). Happy to address any feedback. |
![graphify-labs[bot]](https://avatars.githubusercontent.com/in/4356270?s=60&v=4){kind=small}
There was a problem hiding this comment.
Graphify reviewed this change.
Worth a look — the grounded gate found no coupling regressions or blocking issues, but 2 advisory finding(s) below merit a look before merge.
Graphify review — findings
Adds a watchdog cleanup step to both the post-commit and post-checkout rebuild hook bodies so that when the timeout fires without SIGALRM, _bail now SIGKILLs every live multiprocessing.active_children() worker before calling os._exit, preventing orphaned pool workers (e.g. one stuck in regex backtracking) from surviving the rebuild. Adds test_rebuild_bodies_kill_children_before_os_exit to assert each fallback branch enumerates and kills workers, and that the kill loop is positioned ahead of the os._exit call rather than after it.
Worth a look
- SIGALRM timeout path still exits without killing worker processes —
graphify/hooks.py:156· Escalate · high- agreed by 2 of 2 members but NOT verified (no proof, no reproducing execution) — consensus is not a verdict; needs human review
- Timeout child cleanup only covers the no-SIGALRM path —
graphify/hooks.py:156· Escalate · medium- agreed by 2 of 2 members but NOT verified (no proof, no reproducing execution) — consensus is not a verdict; needs human review
Analysis details — impact, health, verification
Impact & health
Graphify review
Impact — 365 functions depend on the 194 functions this change touches.
Health — this change adds coupling hotspots:
- new:
dispatch_command()— 2 callers, 124 callees - new:
install()— 33 callers, 7 callees - new:
dispatch_install_cli()— 2 callers, 31 callees - new:
status()— 8 callers, 6 callees - new:
uninstall()— 9 callers, 5 callees - new:
uninstall_all()— 2 callers, 13 callees - new:
test_poisoned_manifest_is_healed()— 0 callers, 6 callees
Verification — 365 functions in the blast radius were not formally verified this run (proofs are advisory here).
Gate & verification
graphify gate
PASS — objectively clean (no health regressions, tests not run — proofs not run this pass (advisory)). Grounded, not self-assessed.
Advisory (not blocking):
- verification_scope: 201 function(s) in the blast radius were not formally verified this run
· 7 more finding(s) on lines outside this diff (see the check run).
The no-SIGALRM fallback killing workers before os._exit does not cover the primary path: on POSIX, where SIGALRM is available, a TimeoutError raised while the main thread is waiting inside the ProcessPoolExecutor with-block propagates straight through that block's own __exit__, which calls shutdown(wait=True) and blocks until every worker exits. Forever, for a worker stuck in a C-level call the alarm firing does nothing to stop, the exact shape Graphify-Labs#3341 was. The except TimeoutError handler that would call sys.exit is never reached, so the configured timeout bounds nothing in that case. Reproduced directly: a worker in an unconditional loop, a SIGALRM after 2s, and the process was still running 15s later. Kills any live multiprocessing.active_children() inside the signal handler itself, before it raises, so by the time the exception reaches shutdown(wait=True) there is nothing left to wait for. Moved the handler from an inline lambda (which cannot contain a statement, only expressions) to a named function, matching the no-SIGALRM fallback's own style. Fixes Graphify-Labs#3396.
|
Investigated the bot's two findings here and both were correct, this exposed something more serious than the original scope: the SIGALRM path (the primary one, used on macOS/Linux) has the same orphaning problem the no-SIGALRM fallback had, just manifesting differently. Reproduced directly: a worker stuck in an unconditional loop, SIGALRM firing after 2s, process still running 15s later, because pool.shutdown(wait=True) in the with-block's own exit blocks before the except TimeoutError handler ever gets control. Pushed a fix that kills workers inside the SIGALRM handler itself, before it raises, verified against the same repro (now exits cleanly at 2s). Added a matching regression test. |
There was a problem hiding this comment.
Graphify reviewed this change.
Looks safe to merge — no coupling regressions and no blocking issues, checked against the code graph (not a self-assessment).
Graphify review — findings
Kills leftover ProcessPoolExecutor workers on rebuild timeout instead of letting them orphan and run indefinitely: the SIGALRM handler (_sigalrm_bail) now SIGKILLs every multiprocessing.active_children() before raising TimeoutError, so the pool's shutdown(wait=True) on __exit__ has nothing left to block on. The no-SIGALRM _watchdog fallback does the same before its os._exit(1). Adds AST-level tests asserting both hook bodies enumerate and kill workers, and that the kill happens before the raise/exit rather than after.
No blocking issues surfaced. 2 lower-confidence candidates did not survive cross-model review.
Analysis details — impact, health, verification
Impact & health
Graphify review
Impact — 368 functions depend on the 197 functions this change touches.
Health — this change adds coupling hotspots:
- new:
dispatch_command()— 2 callers, 124 callees - new:
install()— 33 callers, 7 callees - new:
dispatch_install_cli()— 2 callers, 31 callees - new:
status()— 8 callers, 6 callees - new:
uninstall()— 9 callers, 5 callees - new:
uninstall_all()— 2 callers, 13 callees - new:
test_poisoned_manifest_is_healed()— 0 callers, 6 callees
Verification — 368 functions in the blast radius were not formally verified this run (proofs are advisory here).
Gate & verification
graphify gate
PASS — objectively clean (no health regressions, tests not run — proofs not run this pass (advisory)). Grounded, not self-assessed.
Advisory (not blocking):
- verification_scope: 204 function(s) in the blast radius were not formally verified this run
· 7 more finding(s) on lines outside this diff (see the check run).
1 participant
What
os._exit(1), used by the git-hook rebuild watchdog's SIGALRM-less fallback, terminates only the rebuild process itself — it skips every cleanup path, including aProcessPoolExecutor's own context-manager shutdown. A worker still running at the moment the watchdog fires is orphaned (reparented to PID 1 on POSIX) and keeps going unsupervised for as long as whatever it was doing takes.extract.pyalready documents this exact risk for its own 1-worker special case (max_workers == 1deliberately falls back to sequential extraction in-process, specifically to avoid this), but that only covers that one case — a real multi-worker pool spawned for a larger corpus was not covered.Real-world trigger
Reported on #3341 (a ReDoS in the old TS import-type normalizer, since fixed by #3210): a worker stuck in catastrophic regex backtracking survived as an orphan pinning a CPU core for 2.5 days on macOS after the parent rebuild process exited.
Fix
The watchdog fires on a separate timer thread with no reference to the pool object (it's a local variable deep in an unrelated call stack), but
multiprocessing.active_children()enumerates every live worker process regardless of which thread asks. Kill them before exiting.Verified
.kill()(SIGKILL) actually terminates a worker stuck in exactly the reported failure mode — a process executing a long C-levelre.match()call never returns control to the interpreter to check for a Python-level signal, so.terminate()(SIGTERM) would not have worked here; SIGKILL is enforced by the kernel unconditionally:Added an AST-based test (matching this file's existing convention for the embedded, shell-quoted hook bodies) verifying both rebuild bodies enumerate and kill live children in a statement that runs before
os._exit, not after.Fixes #3396.