fix(#487): bound bridge/guard/delegate subprocesses with process-group timeouts, across every harness - #529
Merged
Conversation
…p timeouts, across every harness The issue named only Tau + the JS bridge; verified and fixed the same bug class across every subprocess-shelling adapter instead (Tau, Operator, Hermes, JS bridgeInvoker, Pi's runDelegateAgent — Claude Code has no subprocess adapter and was not in scope). Core discovery, live-reproduced before being fixed: a bare single-pid kill (asyncio proc.kill(), Python subprocess.run's own TimeoutExpired handling, Node child.kill()) is insufficient when the spawned command forks a grandchild that inherits its stdout/stderr pipes (a real npx/wrapper behavior) — the grandchild survives the kill and, on the asyncio side, blocks proc.wait()/communicate() for the full original hang duration regardless of the configured timeout. Fixed by giving every spawn its own process group and killing the whole group on timeout (POSIX; Windows keeps the pre-existing single-pid fallback, not a regression). Notable: Operator's own previously-shipped (#391) kill-and-reap helper had this exact bug. An earlier pass had also cleared Hermes as "already safe" via subprocess.run(timeout=...) — live-testing proved that wrong too; subprocess.run's built-in timeout handling has the same limitation. Every Python fix live-verified against the real installed package (tau-coding-agent, operator-use, hermes-agent) with a real forking hung wrapper, and mutation-tested (reverting the fix reproduces a real orphaned process via pgrep). JS-side fix has real node --test coverage (tests/bridge-invoker-timeout-487.test.js), also mutation-tested. Env-tunable timeouts: RSTACK_BRIDGE_TIMEOUT_MS (60s), RSTACK_GUARD_TIMEOUT_MS (15-30s), RSTACK_DELEGATE_TIMEOUT_MS (30min backstop for Pi's delegate spawn, closing the gap where a bridge-invoked sdlc_delegate call from any harness gets no AbortSignal today). bridgeInvoker also gains an optional AbortSignal parameter and a child.on('error', ...) handler (previously entirely absent). Deferred (documented in docs/HARNESS.md): true cross-process AbortSignal propagation through the bridge protocol; Windows process-tree killing beyond the existing single-pid fallback; automated CI-integrated Python tests (none existed before this fix either — Python-side verification remains live/manual per this repo's established precedent). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
Strix is installed on this repository, but we couldn't run this PR security review because this workspace's trial has ended. Add a card to resume code reviews here. |
Contributor
|
Warning Review limit reached
Next review available in: 39 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (6)
📝 WalkthroughWalkthroughThe change adds bounded subprocess execution and process-tree cleanup across bridge invocation, Tau, Hermes, Operator, and Pi. It adds abort handling, timeout configuration, cleanup tests, and documentation. ChangesSubprocess lifecycle controls
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Caller
participant bridgeInvoker
participant BridgeProcess
participant ProcessGroup
Caller->>bridgeInvoker: invoke with timeout or AbortSignal
bridgeInvoker->>BridgeProcess: spawn detached process
alt timeout or abort
bridgeInvoker->>ProcessGroup: terminate process tree
ProcessGroup-->>bridgeInvoker: processes reaped
bridgeInvoker-->>Caller: timeout or cancellation error
else process exits
BridgeProcess-->>bridgeInvoker: exit result
bridgeInvoker-->>Caller: invocation result
end
Possibly related issues
Possibly related PRs
Suggested labels: Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
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 |
PR Summary by QodoFix subprocess hangs via process-group timeouts across all harness adapters
AI Description
Diagram
High-Level Assessment
Files changed (7)
|
Code Review by Qodo
Context used✅ Compliance rules (platform):
300 rules✅ Skills:
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
| try: | ||
| out, err = await _communicate_or_kill(proc, payload, _gate_timeout_s()) | ||
| except asyncio.TimeoutError: | ||
| print(f"[rstack] gate '{gate_name}' timed out after {_gate_timeout_s():.0f}s (RSTACK_GATE_TIMEOUT_MS) — allowing (gates fail open, matching the no-npx behavior)", file=sys.stderr) |
There was a problem hiding this comment.
2. _run_gate uses print 📜 Skill insight ✧ Quality
A new print(...) statement was added, which is considered debug-style logging in source code and can create noisy/unstructured output. Replace it with the project's logging framework (e.g., logging.warning) or remove it if not required.
Agent Prompt
## Issue description
New debug-style logging was added via `print(...)`, which is disallowed by the compliance rule.
## Issue Context
The message is emitted on gate timeout; it should be handled via structured logging (e.g., Python `logging`) rather than `print`.
## Fix Focus Areas
- src/integrations/tau/rstack_sdlc.py[670-674]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
…r leaks Qodo caught 3 real issues in the first cut, all fixed: 1. killpg/getpgid race (Tau/Operator/Hermes): the kill helpers looked up the process group via os.getpgid(proc.pid) at kill time. If the tracked pid had ALREADY exited while its forked descendants kept the group alive, getpgid raises ProcessLookupError and the code skipped killing the still-live group — reintroducing the exact orphan the fix exists to prevent. Not needed: start_new_session=True makes the process its own group leader at spawn time, so its pgid is always simply proc.pid — no lookup, no race. Re-verified live against all 5/3/3 Tau/Operator/Hermes call sites after the fix — still zero orphans. 2. Abort listener outlives the child (Pi's runDelegateAgent): the AbortSignal listener was never removed on normal completion, so a caller aborting after the process had already exited could still fire process.kill(-pid, ...) against a pid that — in the rare reuse case — might belong to an unrelated process by then. Removed on both close and error. 3. Abort listener not removed on timeout (JS bridgeInvoker): the timeout branch set settled=true and rejected without removing the abort listener; the later exit/error handlers short-circuited on settled and never ran their own cleanup either, so the listener stayed attached for the signal's lifetime. Fixed. Also addressed two minor nits: test names now include #487 for traceability, and the two new timeout defaults (bridge 60s, delegate 30min) are now named constants instead of inline literals. Declined: the new stderr diagnostic in Tau's _run_gate uses print() to match this file's own existing convention (already used elsewhere in the same file) — not a regression, no logging framework exists anywhere in this adapter. Full local gate re-verified green (1892/1892 tests, typecheck/lint/ security/validate all clean) and all three Python adapters re-verified live against the real installed packages after this change. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Owner
Author
|
Thanks for the review — addressed the real findings:
Also applied the two minor nits (test names now include #487, timeout defaults are now named constants). Declined: the new Full local gate re-verified green after all fixes. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/integrations/tau/rstack_sdlc.py (1)
663-684: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winCatch spawn failures in
_run_gateto keep the documented fail-open contract.
_run_gatecreates the subprocess at Line 667 outside any exception handler that catchesOSError. Ifasyncio.create_subprocess_execraisesOSError(for example, a permission error or a resource limit hit between theshutil.whichcheck and exec), the exception propagates out of_run_gateand out of_GuardedBuiltinTool.execute()instead of the documented "gates fail open" behavior (see the docstring at Line 661: "Fails OPEN when npx is unreachable")._run_guardin this same file (Lines 617-625) already catchesOSErroraround its equivalent subprocess creation and fails gracefully. Apply the same pattern here.🛡️ Proposed fix to fail open on spawn errors
- proc = await asyncio.create_subprocess_exec( - npx, "--yes", "rstack-agents", "gate", gate_name, "--project", cwd, - stdin=asyncio.subprocess.PIPE, stdout=asyncio.subprocess.PIPE, stderr=asyncio.subprocess.PIPE, - env=os.environ, cwd=cwd, - **_own_process_group_kwargs(), - ) + try: + proc = await asyncio.create_subprocess_exec( + npx, "--yes", "rstack-agents", "gate", gate_name, "--project", cwd, + stdin=asyncio.subprocess.PIPE, stdout=asyncio.subprocess.PIPE, stderr=asyncio.subprocess.PIPE, + env=os.environ, cwd=cwd, + **_own_process_group_kwargs(), + ) + except OSError as exc: + print(f"[rstack] gate '{gate_name}' could not spawn ({exc}) — allowing (gates fail open)", file=sys.stderr) + return True, ""🤖 Prompt for 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. In `@src/integrations/tau/rstack_sdlc.py` around lines 663 - 684, Update `_run_gate` to wrap `asyncio.create_subprocess_exec` in an `OSError` handler, matching the existing `_run_guard` pattern. When subprocess creation fails, return the documented fail-open result `(True, "")` instead of propagating the exception; preserve the existing timeout handling and successful subprocess flow.
🧹 Nitpick comments (1)
src/commands/pipeline-run.js (1)
262-269: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winUse
closeinstead ofexitto avoid resolving with a truncatedstdout.The completion handler settles on
'exit'. Node.js docs state the'exit'event can fire before the child's stdio streams finish delivering buffered data;'close'is the event guaranteed to fire only after stdio streams are fully closed. Since this promise resolves with the accumulatedstdoutbuffer, a race between'exit'and the final'data'chunks could causeresolvePromise(stdout)to run with an incomplete payload.♻️ Proposed fix
- child.on('exit', (code) => { + child.on('close', (code) => { if (settled) return; settled = true; clearTimeout(timer); if (signal) signal.removeEventListener('abort', onAbort); if (code === 0) resolvePromise(stdout); else rejectPromise(new Error(`${toolName} failed (exit ${code}): ${stderr.slice(0, 400)}`)); });Please confirm this matches the intended semantics for downstream consumers of
bridgeInvoker's resolvedstdout(e.g., JSON parsing in callers), since switching events changes exactly when the promise settles relative to stdio completion.🤖 Prompt for 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. In `@src/commands/pipeline-run.js` around lines 262 - 269, Replace the child process completion listener in the bridgeInvoker flow with the `close` event so settlement occurs only after stdout and stderr have fully drained. Preserve the existing `settled` guard, cleanup, exit-code handling, and resolved stdout/rejection behavior.
🤖 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 `@src/commands/pipeline-run.js`:
- Around line 200-213: The process-tree termination logic is duplicated across
killTree and killDelegateTree. Add a shared killProcessTree helper under
src/core/harness/ that preserves the platform-specific process-group kill and
empty-catch behavior, then update src/commands/pipeline-run.js lines 200-213 to
import and use it and replace killDelegateTree in
src/integrations/pi/rstack-sdlc.ts lines 1618-1623 with the same helper.
In `@src/integrations/pi/rstack-sdlc.ts`:
- Around line 1770-1776: Update the processCompletion timeout handling around
killDelegateTree so the timeout callback settles the promise directly and
unconditionally before or alongside terminating proc. Preserve normal
close/error settlement, but ensure a failed or ineffective kill cannot leave
await processCompletion hanging past delegateTimeoutMs().
In `@src/integrations/tau/rstack_sdlc.py`:
- Around line 474-483: Handle subprocess spawn OSError failures in all three
_run_bridge implementations. In src/integrations/tau/rstack_sdlc.py lines
474-483, catch OSError around asyncio.create_subprocess_exec and return
ToolResult.error matching _run_guard; in
src/integrations/operator/rstack_sdlc.py lines 337-345, catch OSError and return
(False, ...); in src/integrations/hermes/rstack_sdlc.py lines 274-282, catch
OSError and return an error string, preserving each adapter’s existing timeout
handling and error format.
---
Outside diff comments:
In `@src/integrations/tau/rstack_sdlc.py`:
- Around line 663-684: Update `_run_gate` to wrap
`asyncio.create_subprocess_exec` in an `OSError` handler, matching the existing
`_run_guard` pattern. When subprocess creation fails, return the documented
fail-open result `(True, "")` instead of propagating the exception; preserve the
existing timeout handling and successful subprocess flow.
---
Nitpick comments:
In `@src/commands/pipeline-run.js`:
- Around line 262-269: Replace the child process completion listener in the
bridgeInvoker flow with the `close` event so settlement occurs only after stdout
and stderr have fully drained. Preserve the existing `settled` guard, cleanup,
exit-code handling, and resolved stdout/rejection behavior.
🪄 Autofix (Beta)
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: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 63b73e4d-b041-4714-a554-eaa85fe835ce
📒 Files selected for processing (7)
docs/HARNESS.mdsrc/commands/pipeline-run.jssrc/integrations/hermes/rstack_sdlc.pysrc/integrations/operator/rstack_sdlc.pysrc/integrations/pi/rstack-sdlc.tssrc/integrations/tau/rstack_sdlc.pytests/bridge-invoker-timeout-487.test.js
… timeout no-hang, OSError handling CodeRabbit caught 3 real issues on the fixup commit, all fixed: 1. killTree (pipeline-run.js) and killDelegateTree (rstack-sdlc.ts) had duplicated the exact same platform-check + negative-pid SIGKILL + empty-catch pattern. Extracted into a shared src/core/harness/process-tree.js killProcessTree, imported by both. 2. Pi's delegate timeout handler only called killDelegateTree and relied entirely on that producing a close/error event to unblock processCompletion — since the kill helper swallows all failures in an empty catch, a stale pid or an ineffective kill would leave `await processCompletion` hanging past the very timeout meant to bound it, silently defeating the #487 backstop. Now calls resolveCode(1) directly and unconditionally in the timeout branch (safe: a native Promise's resolve ignores every call after the first, so this doesn't conflict with a normal close/error settling it first). 3. _run_bridge (all three Python adapters) and Tau's _run_gate only caught the timeout exception around subprocess creation, not OSError — a rare spawn failure (permission error, resource limit hit between the shutil.which check and exec) would propagate an uncaught exception instead of the documented graceful-failure/fail-open contract their sibling _run_guard already has. Added matching OSError handling to all four call sites. Also applied a CodeRabbit nitpick: bridgeInvoker settles on 'close' instead of 'exit' — 'exit' can fire before buffered stdout/stderr 'data' chunks are fully drained, which could resolve the promise with truncated output. Every fix live-verified: all three Python adapters re-verified against the real installed packages (zero orphans across all 11 call sites), plus a new live reproduction of the OSError spawn-failure path (a bad-shebang binary reliably raises OSError with no shell involved) against Tau, Operator, and Hermes. Full local gate green (1892/1892 tests, typecheck/lint/security/ validate all clean). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Owner
Author
|
Addressed CodeRabbit's 3 findings on the previous commit:
Also applied the All fixes live-verified — including a new reproduction of the OSError spawn-failure path (a bad-shebang binary, no shell involved) against Tau/Operator/Hermes — and the full gate is green (1892/1892). |
5 tasks
richard-devbot
pushed a commit
that referenced
this pull request
Aug 1, 2026
… timeouts, PR #529) Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
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
2 participants
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.
Summary
Issue #487 named only "Tau + JS bridge," but per Richardson's explicit direction, this fixes the same bug class across every harness adapter that shells out to a subprocess: Tau, Operator, Hermes (all Python), the JS
bridgeInvoker(the #486 durable worker's engine), and Pi'srunDelegateAgent. Claude Code has no subprocess adapter code and was not in scope.Core discovery (live-reproduced before being fixed): a bare single-pid kill — asyncio
proc.kill(), Pythonsubprocess.run(timeout=...)'s own built-in handling, or Nodechild.kill()— is insufficient when the spawned command forks a grandchild that inherits its stdout/stderr pipes (a realnpx/wrapper-script behavior). The grandchild survives the kill; on the asyncio side this blockedproc.wait()/communicate()for the FULL original hang duration regardless of the configured timeout (reproduced: a 1s timeout took 100s wall-clock against a real forking wrapper). Fixed by giving every spawn its own process group and killing the WHOLE group on timeout/abort (POSIX; Windows keeps the pre-existing single-pid fallback — not a regression, since Windows process-tree killing was never handled here before).Notable findings surfaced by this audit:
subprocess.run(timeout=...); live-testing proved that wrong — its built-in timeout handling has the identical limitation.What changed
_run_bridge,_run_gate,_run_guard,_emit_observation,_fetch_context) — all 5 subprocess call sites._run_bridge,_run_guard,_after_tool_call) — all 3 call sites, including one the original [P2][operator] Confirm operator_use hook API; wire what's possible; make the tier honest #391 fix missed._run_bridge,_run_guard,_observe_hook) — all 3 call sites, replacing baresubprocess.run.bridgeInvoker(src/commands/pipeline-run.js) — adds timeout + process-group kill, an optionalAbortSignal3rd parameter, and achild.on('error', ...)handler (previously entirely absent — a spawn failure like ENOENT could hang the promise forever).runDelegateAgent(src/integrations/pi/rstack-sdlc.ts) — a generous 30-minute backstop timeout, since a bridge-invokedsdlc_delegatecall from any of Tau/Hermes/Operator getssignal === undefinedtoday (confirmed viabin/rstack-bridge.ts).Env-tunable:
RSTACK_BRIDGE_TIMEOUT_MS(60s),RSTACK_GUARD_TIMEOUT_MS(15-30s depending on adapter),RSTACK_DELEGATE_TIMEOUT_MS(30min).Verification
tau-coding-agent,operator-use,hermes-agent, via pip into throwaway venvs) with a real forking hung wrapper standing in for a stallednpx— not just mocked.bridgeInvoker) mutation-tested: reverting the process-group kill was confirmed to reproduce a real orphaned process viapgrep.tests/bridge-invoker-timeout-487.test.js— realnode --testcoverage for the JS side, also mutation-tested.Deferred (documented in
docs/HARNESS.md)AbortSignalpropagation through the bridge protocol.Test plan
npm test— 1892/1892 passnpm run typecheck— 0 errorsnpm run lint— 0 errorsnode scripts/security-audit.mjs— cleannpm run validate— cleantau-coding-agent,operator-use,hermes-agentpackages🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Documentation
Tests