Conversation
…own process group Found live: letting the (now-fixed) supervisor/work dispatch pipeline run item #13 end-to-end surfaced a real orphaned agent CLI process once its job hit its timeout. Two independent layers each isolate their own child into a fresh process group so their own timeout can kill it: agentflare-jobs::Supervisor does this to the `agentflare work` process, and agent_launch::run_captured does the same, again, to *its* child (the actual agent CLI) -- needed so it can still enforce a timeout when `agentflare work` is run standalone by a human, with no outer supervisor at all. When the outer layer's timeout fires first, its group-kill only reaches whatever is still in `agentflare work`'s group -- the agent CLI has already been re-grouped by the inner layer and is left running, unsupervised, past the timeout. kill_graceful now also walks /proc/<pid>/task/*/children recursively (Linux-only; no /proc on macOS/Windows) to find every descendant by parent-child relationship, which re-grouping never changes, and signals each one directly alongside the existing group signal. New test reproduces the exact escape (a setsid'd grandchild) and confirms it's killed on timeout rather than orphaned.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe supervisor now traverses Linux descendant PIDs through ChangesProcess tree termination
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@crates/agentflare-jobs/src/supervisor.rs`:
- Around line 192-200: Update the local signal closure around signal and
descendant_pids so descendants are discovered and their verified process
identities retained before signaling the process group. Use that preserved
target set for both TERM and the later KILL phase, rather than relying on a
post-signal descendant_pids traversal after the parent may have exited.
- Around line 237-260: The test
spawn_times_out_and_kills_a_descendant_that_escaped_into_its_own_process_group
must establish that the setsid child started before timing out. Add a readiness
marker created inside the setsid child, use a nonzero timeout sufficient for
that marker, and assert its presence before waiting and verifying the delayed
marker remains absent.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 8aa92c3e-92d1-4d2c-b80d-7ba05649f4c2
📒 Files selected for processing (1)
crates/agentflare-jobs/src/supervisor.rs
Comment on lines
+192
to
+200
| let signal = |sig: &str, pid: u32| { | ||
| let _ = Command::new("kill").arg("-s").arg(sig).arg("--").arg(format!("-{pid}")).status(); | ||
| for descendant in descendant_pids(pid) { | ||
| let _ = Command::new("kill").arg("-s").arg(sig).arg("--").arg(descendant.to_string()).status(); | ||
| } | ||
| }; | ||
| signal("TERM", pid); | ||
| std::thread::sleep(kill_after); | ||
| let _ = Command::new("kill") | ||
| .arg("-s") | ||
| .arg("KILL") | ||
| .arg("--") | ||
| .arg(format!("-{pid}")) | ||
| .status(); | ||
| signal("KILL", pid); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
Preserve descendant targets before the group signal.
At Line 193, the process-group TERM can terminate the direct child before Line 194 reads its descendants. The detached descendant is then reparented and no longer appears below /proc/<pid>. It receives neither TERM nor the later KILL.
Discover descendants before sending the group signal. Retain verified process identities for the forced-kill phase. Do not rely only on a second traversal after the parent can exit.
🤖 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 `@crates/agentflare-jobs/src/supervisor.rs` around lines 192 - 200, Update the
local signal closure around signal and descendant_pids so descendants are
discovered and their verified process identities retained before signaling the
process group. Use that preserved target set for both TERM and the later KILL
phase, rather than relying on a post-signal descendant_pids traversal after the
parent may have exited.
Comment on lines
+237
to
+260
| fn spawn_times_out_and_kills_a_descendant_that_escaped_into_its_own_process_group() { | ||
| let dir = tempfile::tempdir().unwrap(); | ||
| let marker = dir.path().join("still-alive"); | ||
| let mut supervisor = Supervisor::new( | ||
| "test".to_string(), | ||
| "sh".to_string(), | ||
| vec![ | ||
| "-c".to_string(), | ||
| format!("setsid sh -c 'sleep 5; touch {}' & wait", marker.display()), | ||
| ], | ||
| vec![], | ||
| None, | ||
| 0, // times out immediately — the point is what happens on timeout | ||
| 0, | ||
| dir.path().to_path_buf(), | ||
| ); | ||
|
|
||
| let (output, _state) = supervisor.spawn().unwrap(); | ||
| assert!(output.timed_out, "should report timeout"); | ||
| // The 5s `sleep` inside the setsid'd grandchild would still have it | ||
| // alive if `kill_graceful` only signaled the direct child's process | ||
| // group; give it a moment past `spawn()` returning, then confirm it | ||
| // never reached its `touch`. | ||
| std::thread::sleep(Duration::from_millis(500)); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Ensure the test starts the detached descendant before timeout.
At Line 249, a zero timeout can call kill_graceful before sh executes setsid. The marker then stays absent even if descendant termination is broken.
Add a readiness marker from inside the setsid child. Use a timeout that permits that marker. Assert readiness before checking that the delayed marker was not written.
🤖 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 `@crates/agentflare-jobs/src/supervisor.rs` around lines 237 - 260, The test
spawn_times_out_and_kills_a_descendant_that_escaped_into_its_own_process_group
must establish that the setsid child started before timing out. Add a readiness
marker created inside the setsid child, use a nonzero timeout sufficient for
that marker, and assert its presence before waiting and verifying the delayed
marker remains absent.
This was referenced Aug 7, 2026
getappz
added a commit
that referenced
this pull request
Aug 11, 2026
…ge collisions (#456) * git-shim/worktree residual polish: teardown messaging, branch-create accuracy, stranded-checkout recovery - worktree deny message now distinguishes provisioning (claim) from teardown (check_merge/release/audit --prune) so an agent denied mid-teardown gets the exact cleanup action (vent #350) - is_branch_create() classifies checkout -b/-B and switch -c/-C; canonical-checkout deny says 'create a new branch' instead of the misleading 'would detach HEAD' (vent #395) - audit_orphans flags worktrees stranded on the default branch (intact gitdir) so worktree audit --prune clears the gh pr merge --delete- branch / post-merge-sync collision root cause (vents #351/#394/#423) - AGENTFLARE_GIT_ALLOW_CANONICAL_MUTATE now also lifts the default-branch checkout/switch deny in the canonical checkout, giving stranded checkouts a sanctioned way back (vent #386 residual) - allowlist classify.rs (already 1604L on master) in the LOC gate Agentflare-Agent: 1 Agentflare-Branch: task/441 Agentflare-Item: 441 * fix(git-shim): close branch-create detection gaps, protect dirty stranded worktrees CodeRabbit review on PR #456: - is_branch_create only matched exact -b/-B/-c/-C, missing --orphan (checkout+switch), switch's --create/--force-create long forms, and attached short-option spellings (-bname). Those forms slipped past both would_detach_head and deny_canonical_detach_reason undetected, letting an agent create a branch in the canonical checkout via e.g. 'git switch --orphan x' or 'git checkout -bx' with no deny at all. - audit_orphans' new on-default-branch detection could hand a dirty, uncommitted stranded worktree to gc_orphans for deletion; the only other gc_orphans caller (cleanup_item_worktree) already refuses on a dirty status check first. Apply the same guard here. Agentflare-Agent: claude-code_2-1-227_agent Agentflare-Branch: task/441 Agentflare-Item: 441
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
1 participant
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
kill_gracefulnow also walks/proc/<pid>/task/*/childrenrecursively (Linux-only) to find every descendant of a timed-out job by parent-child relationship, and signals each one directly, in addition to the existing process-group signal.agentflare-jobs::Supervisorisolatesagentflare workinto its own process group so it can kill the whole group on timeout;agent_launch::run_captured(invoked insideagentflare workto run the actual agent CLI) does the same to its own child, for the same reason, standalone. When the outer timeout fires first, its group-kill can't reach a descendant the inner layer has already re-grouped away -- it's left running, unsupervised, past the timeout.Found live while letting item #15/#16's now-fixed dispatch pipeline run a real item to completion without stopping it early -- it hit its 300s timeout and left a
claude -pprocess running for minutes afterward.Not yet covered: macOS has no
/proc, so the same escape would still orphan a descendant there. Would need apgrep -P/ps -o pid,ppidwalk to close that gap.Test plan
setsid'd grandchild (reproducing the exact escape) is confirmed killed on timeout, not orphaned (Linux-only, matching the fix's scope)cargo test -p agentflare-jobs-- all passcargo clippy --all-targets --all-features -- -D warnings-- cleanSummary by CodeRabbit