Skip to content

fix(worktree): actually kill run_output_timeout's child on timeout - #181

Merged
getappz merged 3 commits into
masterfrom
task/45
Jul 14, 2026
Merged

fix(worktree): actually kill run_output_timeout's child on timeout#181
getappz merged 3 commits into
masterfrom
task/45

Conversation

@getappz

@getappz getappz commented Jul 14, 2026

Copy link
Copy Markdown
Owner

Follow-up to item #45's review (subprocess timeout for push_and_open_pr, stacked on #180 / task/44 since both touch worktree.rs).

Found: run_output_timeout returned promptly on timeout but never killed the child — it moved the Child into a background thread that just blocked on wait_with_output() forever. The process (and any grandchild, e.g. a stuck git credential helper) kept running unbounded; a long-lived server would leak one thread + process per timeout.

Fixed by putting the child in its own process group (Unix) and try_wait-polling with a deadline; on timeout the whole group is killed via the existing kill_tree helper (already used for headless agent runs) and reaped, instead of just walking away. Stdout/stderr are drained on separate threads throughout so a chatty child can't deadlock the wait loop.

New regression test spawns a command that outlives the timeout and would write a marker file if left running to completion, then asserts the marker never appears — directly distinguishes "killed" from "abandoned but still running".

431/431 tests pass (was 430; +1), clippy clean (-D warnings -A unsafe_code -A clippy::pedantic), fmt clean.

Summary by CodeRabbit

  • Bug Fixes

    • Added hard time limits to repository push and pull request creation to prevent indefinite hangs.
    • Timed-out commands are terminated and won’t continue running in the background.
    • Improved failure reporting by surfacing stderr output when push or pull request creation fails.
  • Tests

    • Added coverage to verify timed-out commands are killed (the process does not finish later on its own).

@coderabbitai

coderabbitai Bot commented Jul 14, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: e6f63a69-ff7e-447c-8f5a-f67f7d3b92a1

📥 Commits

Reviewing files that changed from the base of the PR and between 6a3dbe2 and 7f25484.

📒 Files selected for processing (2)
  • src/agent_launch.rs
  • src/worktree.rs
🚧 Files skipped from review as they are similar to previous changes (2)
  • src/agent_launch.rs
  • src/worktree.rs

📝 Walkthrough

Walkthrough

The change adds bounded process execution with concurrent output draining and process-tree termination, then applies it to Git push and pull-request creation commands with soft failure handling and timeout coverage.

Changes

Process timeout handling

Layer / File(s) Summary
Deadline-based process runner
src/agent_launch.rs, src/worktree.rs
kill_tree is made crate-visible, and run_output_timeout adds deadline polling, concurrent stdout/stderr draining, and process-group termination on Unix.
Push and PR creation integration
src/worktree.rs
git push and gh pr create use explicit timeouts, print failures, and return None; a test verifies timed-out children are killed.

Estimated code review effort: 4 (Complex) | ~45 minutes

Sequence Diagram(s)

sequenceDiagram
  participant push_and_open_pr
  participant run_output_timeout
  participant ChildProcess
  participant kill_tree
  push_and_open_pr->>run_output_timeout: run git push or gh pr create
  run_output_timeout->>ChildProcess: spawn and drain output
  run_output_timeout->>ChildProcess: poll until deadline
  run_output_timeout->>kill_tree: terminate process tree on timeout
  run_output_timeout-->>push_and_open_pr: return output or error
Loading

Possibly related PRs

  • getappz/agentflare#155: Both changes use process-tree termination for headless or timed-out execution paths.

Suggested labels: rust

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly and concisely summarizes the main change: killing timed-out worktree subprocesses.
Description check ✅ Passed It explains the fix, rationale, and validation, though it doesn't strictly follow the repository's template headings.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch task/45

Comment @coderabbitai help to get the list of available commands.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 `@src/worktree.rs`:
- Around line 190-249: Update run_output_timeout to configure the spawned
Command with Stdio::null() for stdin, matching agent_launch::run_captured, while
preserving the existing piped stdout and stderr handling.
🪄 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: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: 23dde5df-a77d-4a87-a1fd-e2f4eeaa52c2

📥 Commits

Reviewing files that changed from the base of the PR and between a51ab13 and ab2540a.

📒 Files selected for processing (2)
  • src/agent_launch.rs
  • src/worktree.rs

Comment thread src/worktree.rs
@getappz

getappz commented Jul 14, 2026

Copy link
Copy Markdown
Owner Author

@coderabbitai review

@coderabbitai

coderabbitai Bot commented Jul 14, 2026

Copy link
Copy Markdown
✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

getappz added a commit that referenced this pull request Jul 14, 2026
git/gh were inheriting the MCP server's own stdin, which is the live
JSON-RPC channel under stdio transport -- a credential prompt or any
other stdin read in the child could consume or interfere with MCP
traffic. Matches agent_launch::run_captured, which already nulls
stdin for the same reason.

Found by CodeRabbit on PR #181.
Base automatically changed from task/44 to master July 14, 2026 08:29
getappz added 3 commits July 14, 2026 14:15
… timeout instead of abandoning it

Previously the timed-out child was moved into a background thread that
just blocked on wait_with_output() forever; the outer call returned on
schedule but the process (and, more importantly, any grandchild like a
stuck git credential helper) kept running unbounded. Now the child is
placed in its own process group (Unix) and try_wait-polled with a
deadline; on timeout the whole group is killed via the same kill_tree
helper already used for headless agent runs, then reaped.

Regression test spawns a command that outlives the timeout and would
write a marker file if left running to completion, then asserts the
marker never appears.
git/gh were inheriting the MCP server's own stdin, which is the live
JSON-RPC channel under stdio transport -- a credential prompt or any
other stdin read in the child could consume or interfere with MCP
traffic. Matches agent_launch::run_captured, which already nulls
stdin for the same reason.

Found by CodeRabbit on PR #181.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant