build_util: the per-script cap kills the process group, not just the child - #257
Merged
Merged
Conversation
…child `subprocess.run(timeout=...)` kills only the direct child. A grandchild — a Popen'd helper, a multiprocessing worker, a JAX compile server — outlives the cap and runs on, holding whatever memory and GPU it had. Over a mega-run of hundreds of scripts those accumulate against every script that follows. Add `run_capped`, a drop-in for the `subprocess.run(..., timeout=...)` calls this module made: same TimeoutExpired and CalledProcessError, same captured output/stderr attributes, so every existing handler — `_timeout_output`, `is_clean_skip_exit`, the ScriptResult/TIMEOUT report paths — is untouched. It runs the child with `start_new_session=True` and SIGKILLs the group on expiry. `kill_group` is public so the workspace `run_smoke.py` runners can import it rather than each growing a copy. Both `execute_notebook` and `execute_script` switch over. Note this is a resource-leak fix, NOT a hang fix: on POSIX `subprocess.run` handles its own timeout with `process.wait()` on the direct child only, so it already returned at the cap. The uncapped runners that hang to the Actions ceiling are a separate problem in the workspace copies, not here. Regression test asserts the grandchild is gone one second after the cap fires; against the previous code it fails `assert 1 == 0` with the TIMEOUT status already correct, isolating the group kill as the thing under test. Verified: tests/test_script_timeout.py 27 passed; full suite 354 passed with the same 14 pre-existing failures main has (missing ipynb-py-convert and image optimisers in this environment). Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01GTPM1RmMvSuMvJkEntAMv8
The docstring cited a satellite repo's issue by name. PyAutoHands is a framework organ and must stay adoptable as a config-diff fork, so repos_sync.py's tenant firewall rejects a new instance fact in organ code — correctly, and growing the allowlist for a comment would be the wrong fix. Rewritten to describe the mechanism rather than the incident: the leak this module actually has (grandchildren outliving the cap) and, separately, the uncapped-runner shape where the same group kill prevents a hang, noted as a workspace-side bug rather than named. No code change. Local check now passes: repos_sync.py --check --only "tenant firewall (organ code)" -> OK Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01GTPM1RmMvSuMvJkEntAMv8
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Leg B of the per-script timeout backport (PyAutoMind
backport_per_script_timeout.md). Merge this first — seven workspace runners import the helper it adds.What
subprocess.run(timeout=...)kills only the direct child. A grandchild — aPopen'd helper, a multiprocessing worker, a JAX compile server — outlives the cap and runs on, holding whatever memory and GPU it had. Over a mega-run of hundreds of scripts those accumulate against every script that follows.Adds
run_capped, a drop-in for thesubprocess.run(..., timeout=...)calls this module made: sameTimeoutExpiredandCalledProcessError, same capturedoutput/stderrattributes, so every existing handler —_timeout_output,is_clean_skip_exit, theScriptResult/TIMEOUT report paths — is untouched. It runs the child withstart_new_session=Trueand SIGKILLs the group on expiry.kill_groupis public so the workspacerun_smoke.pyrunners import it rather than each keeping a copy.Both
execute_notebookandexecute_scriptswitch over.This is a resource-leak fix, not a hang fix
Worth stating plainly, because an earlier draft of the Mind prompt claimed otherwise. On POSIX
subprocess.runhandles its ownTimeoutExpiredwithprocess.kill()thenprocess.wait()on the direct child only — it never re-communicates, so it cannot block on an inherited pipe. Measured: with a 3s cap it raised at 3.0s. What it leaves behind is the grandchild, measured at 1 surviving process running for its full lifetime.The uncapped runners that genuinely hang to the Actions ceiling are a separate problem, in the workspace copies, not here.
Verification
tests/test_script_timeout.py— 27 passed.mainalready has in this environment (missingipynb-py-convertand image optimisers; confirmed identical by re-running on a clean tree).assert 1 == 0with the TIMEOUT status already correct, which isolates the group kill as the thing under test rather than the timer.Generated by Claude Code