mirror of
https://github.com/safishamsi/graphify.git
synced 2026-10-14 07:59:40 +00:00
Kill orphaned workers before the rebuild watchdog exits
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 #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 #3396.
This commit is contained in:
1 parent
c9f9901877
commit
319869e6c1
2 files changed
+61
-2
No files matched your search
+6
-2
@@ -134,7 +134,7 @@ fi
|
||||
# double-quote, $, backtick or backslash characters: it is carried inside a
|
||||
# shell double-quoted `-c "..."` argument (see _detached_launch).
|
||||
_REBUILD_BODY_COMMIT = """\
|
||||
import os, signal, sys, threading
|
||||
import os, signal, sys, threading, multiprocessing
|
||||
from pathlib import Path
|
||||
|
||||
changed_raw = os.environ.get('GRAPHIFY_CHANGED', '')
|
||||
@@ -156,6 +156,8 @@ try:
|
||||
else:
|
||||
def _bail():
|
||||
print(f'[graphify hook] graphify rebuild exceeded {_timeout}s', flush=True)
|
||||
for _child in multiprocessing.active_children():
|
||||
_child.kill()
|
||||
os._exit(1)
|
||||
_watchdog = threading.Timer(_timeout, _bail)
|
||||
_watchdog.daemon = True
|
||||
@@ -191,7 +193,7 @@ except Exception as exc:
|
||||
_REBUILD_BODY_CHECKOUT = """\
|
||||
from graphify.watch import _rebuild_code, _apply_resource_limits
|
||||
from pathlib import Path
|
||||
import os, signal, sys, threading
|
||||
import os, signal, sys, threading, multiprocessing
|
||||
try:
|
||||
_apply_resource_limits()
|
||||
_timeout = int(os.environ.get('GRAPHIFY_REBUILD_TIMEOUT', '600'))
|
||||
@@ -202,6 +204,8 @@ try:
|
||||
else:
|
||||
def _bail():
|
||||
print(f'[graphify] graphify rebuild exceeded {_timeout}s', flush=True)
|
||||
for _child in multiprocessing.active_children():
|
||||
_child.kill()
|
||||
os._exit(1)
|
||||
_watchdog = threading.Timer(_timeout, _bail)
|
||||
_watchdog.daemon = True
|
||||
|
||||
@@ -357,6 +357,61 @@ def test_rebuild_bodies_arm_a_timeout_without_sigalrm(name, body):
|
||||
assert len(prefixes) == 1, f"{name} mixes log prefixes {sorted(prefixes)} (#2148)"
|
||||
|
||||
|
||||
@pytest.mark.parametrize(
|
||||
"name,body",
|
||||
[("post-commit", _REBUILD_BODY_COMMIT), ("post-checkout", _REBUILD_BODY_CHECKOUT)],
|
||||
)
|
||||
def test_rebuild_bodies_kill_children_before_os_exit(name, body):
|
||||
"""os._exit() terminates only the rebuild process itself, not any
|
||||
ProcessPoolExecutor worker it spawned for a large corpus -- os._exit skips
|
||||
every cleanup path, including the pool's own context-manager shutdown, so a
|
||||
worker mid-task at the moment the watchdog fires is orphaned (reparented to
|
||||
PID 1 on POSIX) and keeps running, unsupervised, for as long as whatever it
|
||||
was doing takes. A worker stuck in catastrophic regex backtracking (the
|
||||
exact shape #3341 fixed) has been observed surviving 2.5 days that way. The
|
||||
watchdog owns no reference to the pool object (it fires on a separate timer
|
||||
thread, unrelated to whichever stack frame currently holds the pool), but
|
||||
multiprocessing.active_children() enumerates every live worker process
|
||||
regardless of which thread asks, so the fallback kills them by SIGKILL
|
||||
(not terminate/SIGTERM, which a process stuck in a C-level call like
|
||||
regex backtracking never gets a chance to act on) before exiting itself."""
|
||||
fallbacks = [
|
||||
node.orelse
|
||||
for node in ast.walk(ast.parse(body))
|
||||
if isinstance(node, ast.If) and "'SIGALRM'" in ast.dump(node.test) and node.orelse
|
||||
]
|
||||
assert fallbacks, f"{name} has no else-branch for the missing-SIGALRM case"
|
||||
dumped = "".join(ast.dump(stmt) for stmt in fallbacks[0])
|
||||
assert "attr='active_children'" in dumped, (
|
||||
f"{name} fallback does not enumerate live workers before exiting (#3341 follow-up)"
|
||||
)
|
||||
assert "attr='kill'" in dumped, (
|
||||
f"{name} fallback does not SIGKILL orphaned workers before exiting (#3341 follow-up)"
|
||||
)
|
||||
# The kill loop must run BEFORE os._exit, not after (dead code) or replacing
|
||||
# it (the rebuild process itself still has to exit on timeout). _bail's body
|
||||
# is a straight-line statement list (no branching), so statement POSITION is
|
||||
# execution order -- unlike ast.walk's traversal, which is breadth-first and
|
||||
# would visit os._exit's Call node (a direct child of a top-level Expr)
|
||||
# before a Call nested one level deeper inside the for loop's body, even
|
||||
# though the for loop is written, and runs, first.
|
||||
bail_def = next(
|
||||
n for n in ast.walk(ast.parse(body))
|
||||
if isinstance(n, ast.FunctionDef) and n.name == "_bail"
|
||||
)
|
||||
def _stmt_index_calling(attr: str) -> int:
|
||||
for i, stmt in enumerate(bail_def.body):
|
||||
if any(
|
||||
isinstance(n, ast.Call) and isinstance(n.func, ast.Attribute) and n.func.attr == attr
|
||||
for n in ast.walk(stmt)
|
||||
):
|
||||
return i
|
||||
raise AssertionError(f"{name}: no statement in _bail calls .{attr}(...)")
|
||||
assert _stmt_index_calling("active_children") < _stmt_index_calling("_exit"), (
|
||||
f"{name} kills workers after os._exit instead of before"
|
||||
)
|
||||
|
||||
|
||||
def test_detached_launch_targets_graphify_python():
|
||||
"""The launcher must run via the resolved $GRAPHIFY_PYTHON, not a bare
|
||||
`python`, so it uses the same interpreter the detection block selected."""
|
||||
|
||||
Reference in new issue
Block a user