diff --git a/graphify/hooks.py b/graphify/hooks.py index a94f932ee..aaae7d67d 100644 --- a/graphify/hooks.py +++ b/graphify/hooks.py @@ -151,7 +151,18 @@ try: _timeout = int(os.environ.get('GRAPHIFY_REBUILD_TIMEOUT', '600')) if _timeout > 0: if hasattr(signal, 'SIGALRM'): - signal.signal(signal.SIGALRM, lambda *_: (_ for _ in ()).throw(TimeoutError(f'graphify rebuild exceeded {_timeout}s'))) + def _sigalrm_bail(*_a): + # Killing here, before the exception unwinds, matters: once + # TimeoutError starts propagating it passes straight through + # the ProcessPoolExecutor with-block's own __exit__, which + # calls shutdown(wait=True) and blocks until every worker + # exits -- forever, for a worker stuck the way #3341 was, + # since the alarm firing never actually stops it. Killing the + # workers first means shutdown has nothing left to wait for. + for _child in multiprocessing.active_children(): + _child.kill() + raise TimeoutError(f'graphify rebuild exceeded {_timeout}s') + signal.signal(signal.SIGALRM, _sigalrm_bail) signal.alarm(_timeout) else: def _bail(): @@ -199,7 +210,18 @@ try: _timeout = int(os.environ.get('GRAPHIFY_REBUILD_TIMEOUT', '600')) if _timeout > 0: if hasattr(signal, 'SIGALRM'): - signal.signal(signal.SIGALRM, lambda *_: (_ for _ in ()).throw(TimeoutError(f'graphify rebuild exceeded {_timeout}s'))) + def _sigalrm_bail(*_a): + # Killing here, before the exception unwinds, matters: once + # TimeoutError starts propagating it passes straight through + # the ProcessPoolExecutor with-block's own __exit__, which + # calls shutdown(wait=True) and blocks until every worker + # exits -- forever, for a worker stuck the way #3341 was, + # since the alarm firing never actually stops it. Killing the + # workers first means shutdown has nothing left to wait for. + for _child in multiprocessing.active_children(): + _child.kill() + raise TimeoutError(f'graphify rebuild exceeded {_timeout}s') + signal.signal(signal.SIGALRM, _sigalrm_bail) signal.alarm(_timeout) else: def _bail(): diff --git a/tests/test_hooks.py b/tests/test_hooks.py index 69e14863a..d8eb4573d 100644 --- a/tests/test_hooks.py +++ b/tests/test_hooks.py @@ -412,6 +412,56 @@ def test_rebuild_bodies_kill_children_before_os_exit(name, body): ) +@pytest.mark.parametrize( + "name,body", + [("post-commit", _REBUILD_BODY_COMMIT), ("post-checkout", _REBUILD_BODY_CHECKOUT)], +) +def test_sigalrm_handler_kills_children_before_raising(name, body): + """The no-SIGALRM fallback killing children before os._exit is not enough: + 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 + (reproduced: a worker in an unconditional loop left the process still + running 15s after a 2s alarm). The except TimeoutError handler is never + reached, so the timeout provides no bound at all in that case. Killing + workers INSIDE the signal handler, before it raises, means shutdown has + nothing left to wait for by the time the exception reaches it.""" + handler_def = next( + n for n in ast.walk(ast.parse(body)) + if isinstance(n, ast.FunctionDef) and n.name == "_sigalrm_bail" + ) + dumped = "".join(ast.dump(stmt) for stmt in handler_def.body) + assert "attr='active_children'" in dumped, ( + f"{name} SIGALRM handler does not enumerate live workers (#3397 follow-up)" + ) + assert "attr='kill'" in dumped, ( + f"{name} SIGALRM handler does not SIGKILL workers (#3397 follow-up)" + ) + + def _stmt_index_calling(attr: str) -> int: + for i, stmt in enumerate(handler_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 _sigalrm_bail calls .{attr}(...)") + kill_idx = _stmt_index_calling("kill") + raise_idx = next( + i for i, stmt in enumerate(handler_def.body) if isinstance(stmt, ast.Raise) + ) + assert kill_idx < raise_idx, ( + f"{name} raises TimeoutError before killing workers, defeating the fix" + ) + # signal.signal must be wired to this handler, not still the old inline + # lambda that only threw the exception. + assert re.search(r"signal\.signal\(signal\.SIGALRM,\s*_sigalrm_bail\)", body), ( + f"{name} does not register _sigalrm_bail as the SIGALRM handler" + ) + + 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."""