fix(tools): bound script/command tool subprocesses with a timeout - #2369
fix(tools): bound script/command tool subprocesses with a timeout#2369abhyudayareddy wants to merge 2 commits into
Conversation
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
Included review availability: Your plan provides up to 8 included reviews per hour; 7 remain after this review. WalkthroughYAML tool subprocesses now use a configurable 300-second timeout. Timed-out process groups are terminated, partial output is preserved, and the result uses exit code ChangesYAML tool subprocess timeout
Estimated code review effort: 3 (Moderate) | ~20 minutes Merge Risk: ⚪ Minimal · up to The change bounds command and script execution, terminates timed-out process groups, and preserves a clear timeout result without introducing a concrete merge-blocking issue; it is merge-ready after normal checks and review. Sequence Diagram(s)sequenceDiagram
participant YAMLTool
participant ProcessGroup
participant TimeoutCleanup
YAMLTool->>ProcessGroup: Start command in a new session
ProcessGroup-->>YAMLTool: Return output and exit code
YAMLTool->>TimeoutCleanup: Handle timeout
TimeoutCleanup->>ProcessGroup: Send SIGKILL to process group
TimeoutCleanup-->>YAMLTool: Return output and exit code 124
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Full details: Linked Issues checkExplanation The changes satisfy issue Full details: Docstring CoverageExplanation No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Analyzed 0 functions across 0 files. (1 skipped: 1 unsupported.) Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
✅ Deploy Preview for holmes-docs ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/core/test_tool_subprocess_timeout.py`:
- Around line 109-110: Update the os.killpg mock in the race test around
tool._invoke so it terminates the real sleep process group before raising
ProcessLookupError, or replace the subprocess with a fake process object that
does not spawn a child. Preserve the test’s race-condition assertion while
ensuring cleanup completes without leaving the sleep process running.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 8da0d099-2fd8-4159-9f66-4e5a8848874a
📒 Files selected for processing (4)
docs/reference/environment-variables.mdholmes/common/env_vars.pyholmes/core/tools.pytests/core/test_tool_subprocess_timeout.py
The race-condition test mocked os.killpg to unconditionally raise ProcessLookupError, which meant the real `sleep 30` process it spawns via _invoke() was never actually killed - it kept running in the background for the rest of its 30s duration on every test run. Have the mock perform the real kill first, then raise, so the process is genuinely reaped while still exercising the code path that must swallow the race-condition error. Flagged by CodeRabbit review on HolmesGPT#2369. Signed-off-by: Abhyuday <abhyudayreddy@gmail.com>
The race-condition test mocked os.killpg to unconditionally raise ProcessLookupError, which meant the real `sleep 30` process it spawns via _invoke() was never actually killed - it kept running in the background for the rest of its 30s duration on every test run. Have the mock perform the real kill first, then raise, so the process is genuinely reaped while still exercising the code path that must swallow the race-condition error. Flagged by CodeRabbit review on HolmesGPT#2369. Signed-off-by: Abhyuday <abhyudayareddy@gmail.com>
97a99d4 to
e7bca26
Compare
|
cc @aantn — you're the primary author of this file, so thought you'd be the right person to take a look whenever you have time. CI is green aside from a pre-existing License Compliance baseline finding unrelated to this change (this diff adds no new dependencies). Thanks! |
|
cc @aantn — checking back in on this one. DCO is green; the tool subprocess timeout fix is still ready for review whenever you have a chance. Thanks! |
Tool.__execute_subprocess() ran every command:/script: toolset tool (e.g. kubectl-based kubernetes/core tools) via subprocess.run() with no timeout. A stalled child process - for example an apiserver connection that accepts the TCP handshake but never responds - blocked the calling thread and leaked the process forever, with no way to recover short of restarting the pod. Switch to subprocess.Popen(start_new_session=True) + communicate(timeout= TOOL_SUBPROCESS_TIMEOUT_SECONDS) (default 300s, configurable). On timeout, kill the whole process group with os.killpg() instead of just the direct shell child, so a stalled grandchild (e.g. kubectl) spawned by the script is also reaped. The tool returns exit code 124, matching GNU timeout, with any output produced before the deadline. Fixes HolmesGPT#2365 Signed-off-by: Abhyuday <abhyudayareddy@gmail.com>
The race-condition test mocked os.killpg to unconditionally raise ProcessLookupError, which meant the real `sleep 30` process it spawns via _invoke() was never actually killed - it kept running in the background for the rest of its 30s duration on every test run. Have the mock perform the real kill first, then raise, so the process is genuinely reaped while still exercising the code path that must swallow the race-condition error. Flagged by CodeRabbit review on HolmesGPT#2369. Signed-off-by: Abhyuday <abhyudayareddy@gmail.com>
e7bca26 to
b936a64
Compare
|
Rebased onto latest The CodeRabbit "kill the process group before raising" note was already handled in |
1 participant
Problem
Tool.__execute_subprocess()(holmes/core/tools.py) is the shared execution path for everycommand:/script:toolset tool, including the built-inkubernetes/coretools (kubernetes_jq_query,kubernetes_tabular_query,kubernetes_count, all of which shell out tokubectl). It ran the command viasubprocess.run()with notimeout=.If the underlying process stalls — e.g.
kubectlhits an apiserver connection that accepts the TCP handshake but never responds —subprocess.run()blocks the calling thread forever and the child process is never reaped. This doesn't require a pod restart to snowball: a single long-lived Holmes pod accumulates one permanently-blocked worker (and orphanedkubectlprocess) per stuck call, and each pins significant CPU.This is not specific to
kubernetes/core—__execute_subprocess()is shared by everyscript:-defined tool across every toolset (e.g.kubevela.yamltoo).Fixes #2365.
Fix
__execute_subprocess()now runs the command viasubprocess.Popen(..., start_new_session=True)+process.communicate(timeout=TOOL_SUBPROCESS_TIMEOUT_SECONDS)(new env var, default300s) instead of an unboundedsubprocess.run().os.killpg(..., SIGKILL), not just the immediatebashchild. This matters because the direct child ofPopenisbash <script>, and the actual offending command (kubectl, etc.) runs as its child — killing only the direct child (what plainsubprocess.run(timeout=...)+.kill()does) leaves that grandchild running independently, which is exactly the failure mode described in the issue.start_new_session=Trueputs the whole tree in its own process group sokillpgreaps it all.os.killpgraisingProcessLookupError) is handled explicitly and does not propagate.124(matching GNUtimeout) plus any output produced before the deadline, so the LLM sees a clear, actionable error instead of the call silently vanishing.TOOL_SUBPROCESS_TIMEOUT_SECONDS(default300) toholmes/common/env_vars.py, following the existingTOOL_MEMORY_LIMIT_MBpattern, and documented it indocs/reference/environment-variables.md.Changes
holmes/common/env_vars.py: newTOOL_SUBPROCESS_TIMEOUT_SECONDSenv var (default 300s).holmes/core/tools.py:Tool.__execute_subprocess()rewritten aroundPopen+communicate(timeout=...); newTool.__kill_process_group_and_collect_output()helper that kills the process group and returns whatever output was buffered.docs/reference/environment-variables.md: reference entry for the new env var.tests/core/test_tool_subprocess_timeout.py: new tests (see below).Test plan
Added
tests/core/test_tool_subprocess_timeout.py, covering:kubectlgrandchild) — the regression test confirms that process is actually killed, not left orphaned.os.killpgrace (process exits right as we try to signal it) is swallowed rather than raised.Validation performed in this environment:
ast.parseon all changed Python files (syntax-valid).ruff checkandruff format --diffwith the pinned versions from.pre-commit-config.yaml(ruff==0.7.2) — clean on all lines touched by this change.isort --profile black --check-only --diff(isort==6.1.0, closest available to the pinned7.0.0for this environment's Python) — the new import is correctly positioned; no changes requested to lines touched by this PR.mypyagainstholmes/core/tools.py/holmes/common/env_vars.py— no new errors introduced by this change (pre-existing unrelated errors elsewhere in the file/codebase are untouched by this diff).litellm/aiohttpenforce this at install time), so I could not runpytest/make test-without-llmdirectly here. To compensate, I extracted the exactPopen/communicate/killpglogic added in this PR into a standalone script (no holmes imports) and exercised it directly — confirmed: fast commands return normally, slow commands are killed at the timeout without blocking, partial output is preserved, and critically, the backgrounded "grandchild" process is killed with the new process-group approach while a side-by-side run of the oldsubprocess.run(timeout=...)pattern left that same grandchild process alive, reproducing the exact bug from the issue. CI will run the full suite including the new test file on a proper Python 3.10+ environment.Example
Summary by CodeRabbit
New Features
Documentation
Bug Fixes