Background: escalate process termination from SIGTERM to SIGKILL#134
Conversation
terminateProcess (POSIX) sent only SIGTERM, so a background process that traps or ignores it kept running after a Kill/KillRunning — a leak, e.g. on Ctrl+C shutdown. It now sends SIGTERM (letting the process clean up), polls liveness, and force-kills with SIGKILL if the process is still alive after a grace period. - internal/background/process_posix.go: SIGTERM -> poll (signal 0) until a 3s grace deadline -> SIGKILL; treats an already-exited process as success (ESRCH / ErrProcessDone). Grace/poll are vars so tests run fast. Windows already force-kills via taskkill /F, so it is unchanged. Tests: a SIGTERM-ignoring process (trap, with a readiness handshake to avoid signalling before the trap installs) is killed only after the grace period; a well-behaved process dies on SIGTERM well before escalation; an already-exited pid is a no-op. -race + full-suite + GOOS=windows build green; no new deps.
Zero automated PR reviewVerdict: No blockers found Blockers
Validation
ScopeHead: This deterministic review checks validation status and basic diff hygiene. A human reviewer still owns product judgment and design quality. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
WalkthroughThe PR changes POSIX termination from a single SIGTERM send to a two-phase flow: send SIGTERM, poll liveness for a configurable grace period (signal 0), treat already-exited errors as success, and escalate to SIGKILL if still alive. Tests cover escalation, graceful termination, and already-exited no-op behavior. Manager.Kill now persists an optimistic killed mark and can roll it back on failed kill; a race test was added. ChangesGraceful Process Termination with SIGKILL Escalation
Sequence DiagramsequenceDiagram
participant Caller
participant terminateProcess
participant TargetProcess as Target OS Process
Caller->>terminateProcess: terminateProcess(pid)
terminateProcess->>TargetProcess: send SIGTERM
loop poll until grace period expires
terminateProcess->>TargetProcess: signal 0 (liveness check)
alt process exited
TargetProcess-->>terminateProcess: no longer signallable
terminateProcess->>Caller: return success
else still alive
terminateProcess->>terminateProcess: wait poll interval
end
end
alt grace expired and still alive
terminateProcess->>TargetProcess: send SIGKILL
TargetProcess->>terminateProcess: forced termination
end
terminateProcess->>Caller: return (success if gone, error otherwise)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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: 1
🧹 Nitpick comments (1)
internal/background/process_posix_test.go (1)
27-27: ⚡ Quick winUse
exec.CommandContextwith test-scoped timeouts for subprocesses.Using plain
exec.Commandcan leave hanging children when setup/handshake paths stall.CommandContextaligns with the lint rule and makes these tests self-bounding.Suggested pattern
+ctx, cancel := context.WithTimeout(t.Context(), 5*time.Second) +t.Cleanup(cancel) -cmd := exec.Command("sh", "-c", "...") +cmd := exec.CommandContext(ctx, "sh", "-c", "...")Also applies to: 64-64, 87-87
🤖 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 `@internal/background/process_posix_test.go` at line 27, Replace uses of exec.Command (e.g., the creation of cmd := exec.Command("sh", "-c", "trap '' TERM; echo ready; while true; do sleep 0.2; done") and the other similar invocations) with exec.CommandContext using a test-scoped context with a reasonable timeout (context.WithTimeout(ctx, ...)) so subprocesses are automatically killed when the test deadline is reached; update the test helper to create and defer cancel on the context, pass that context into exec.CommandContext when constructing cmd, and ensure any waits/handshake still use the same context to avoid leaked children.
🤖 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 `@internal/background/process_posix_test.go`:
- Around line 64-83: TestTerminateProcessGracefulSIGTERM currently only checks
timing but not which signal killed the child; change the goroutine that calls
cmd.Wait() to send its error/result on a channel (e.g., errCh) and after the
process finishes inspect cmd.ProcessState or the returned error (type
*exec.ExitError) to obtain the syscall.WaitStatus and assert that
WaitStatus.Signal() == syscall.SIGTERM (use syscall.SIGTERM symbol) in addition
to the existing timing assertions; keep the use of terminateProcess and
terminationGracePeriod and fail the test if the observed signal is not SIGTERM.
---
Nitpick comments:
In `@internal/background/process_posix_test.go`:
- Line 27: Replace uses of exec.Command (e.g., the creation of cmd :=
exec.Command("sh", "-c", "trap '' TERM; echo ready; while true; do sleep 0.2;
done") and the other similar invocations) with exec.CommandContext using a
test-scoped context with a reasonable timeout (context.WithTimeout(ctx, ...)) so
subprocesses are automatically killed when the test deadline is reached; update
the test helper to create and defer cancel on the context, pass that context
into exec.CommandContext when constructing cmd, and ensure any waits/handshake
still use the same context to avoid leaked children.
🪄 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: e5fff2eb-52f6-4f64-b12c-426ff0b82dce
📒 Files selected for processing (2)
internal/background/process_posix.gointernal/background/process_posix_test.go
The graceful and escalation tests now check the actual signal that killed the process (via WaitStatus.Signal()), not just timing — so a regression to an immediate SIGKILL (or to never escalating) is caught. Graceful path must be SIGTERM; the SIGTERM-ignoring process must be SIGKILL.
|
Fixed: both tests now assert the actual terminating signal via |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
cat internal/background/process_posix.go
echo "---FILE_BOUNDARY---"
cat internal/background/process_posix_test.goLength of output: 5909
1.
|
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/background/process_posix_test.go (1)
75-80:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAvoid
sh -cin the graceful-path test.
terminateProcess(cmd.Process.Pid)only targets the shell PID here. If/bin/shdoes notexecsleep,cmd.Wait()can report a SIGTERM-killed shell while the childsleep 30keeps running, so this test can pass while still leaking a process. Spawnsleepdirectly so the PID under test is the long-lived process.Suggested change
- cmd := exec.Command("sh", "-c", "sleep 30") // default disposition: SIGTERM kills it + cmd := exec.Command("sleep", "30")🤖 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 `@internal/background/process_posix_test.go` around lines 75 - 80, The test spawns a shell so terminateProcess(cmd.Process.Pid) may only kill the shell PID and leak the actual sleep child; replace the sh -c invocation with a direct sleep process so the PID under test is the long-lived process (i.e., create the command using the direct binary and args rather than "sh -c"), keep the same start/Wait logic (cmd.Start(), goroutine sending cmd.Wait() to done) and ensure terminateProcess is called against cmd.Process.Pid so the test verifies the child is terminated rather than the shell.
🤖 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.
Outside diff comments:
In `@internal/background/process_posix_test.go`:
- Around line 75-80: The test spawns a shell so
terminateProcess(cmd.Process.Pid) may only kill the shell PID and leak the
actual sleep child; replace the sh -c invocation with a direct sleep process so
the PID under test is the long-lived process (i.e., create the command using the
direct binary and args rather than "sh -c"), keep the same start/Wait logic
(cmd.Start(), goroutine sending cmd.Wait() to done) and ensure terminateProcess
is called against cmd.Process.Pid so the test verifies the child is terminated
rather than the shell.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: d8b5f83a-31d4-4a31-9145-93b227ba0017
📒 Files selected for processing (1)
internal/background/process_posix_test.go
Vasanthdev2004
left a comment
Vasanthdev2004
left a comment
There was a problem hiding this comment.
Verdict: Request changes
I found one lifecycle blocker on the current head 73154c4c2a0c.
[P1] Preserve killed status when POSIX stop waits for the child to exit
terminateProcess now waits until processAlive is false before returning. For background specialists, launchBackgroundProcess also has a command.Wait() goroutine that calls manager.MarkExited(...). On a normal SIGTERM stop, that waiter can reap the process and mark the task error with exit code -1 before Manager.Kill reaches markKilledIfStillRunning. Because markKilledIfStillRunning no-ops once the task is no longer running, the persisted task can end up as error even though TaskStop returned status: killed.
Path: internal/background/process_posix.go:39 waits for exit, while internal/specialist/exec.go:652 reaps and calls the onExit status update before internal/background/manager.go:273 marks killed.
Smallest fix: make user-initiated stop intent win over the waiter before/while waiting for process exit, or keep SIGTERM return non-blocking and do SIGKILL escalation separately. Please add a POSIX lifecycle regression test around launchBackgroundProcess/Manager.Kill (not only terminateProcess) that asserts the final task status remains killed after the waiter runs.
Validation I ran locally:
git diff --check origin/main...HEADgo test ./internal/background -count=1go test ./internal/specialist -count=1
Note: I’m on Windows, so the new //go:build !windows POSIX signal tests are not executed locally; the CI Ubuntu/macOS smoke jobs are the platform evidence for those tests.
…erminate Vasanthdev P1: terminateProcess now blocks until the child exits, so the background Wait-goroutine can reap the process and MarkExited the task to 'error' BEFORE Kill marked it 'killed' — leaving a user-initiated stop persisted as 'error' even though TaskStop returned killed. Manager.Kill now records the kill intent (markKilledIfStillRunning) BEFORE terminating. MarkExited only acts on a running task, so once the task is killed the waiter's MarkExited is a no-op and the status stays killed. If terminateProcess fails, restoreRunningAfterFailedKill reverts the optimistic mark so a still-running task isn't falsely reported killed (preserves TestManagerDoesNotMarkKilledWhenKillFails). Adds TestManagerKillStaysKilledWhenExitWaiterReapsDuringTerminate (cross-platform: a KillProcess that simulates the waiter reaping mid-terminate; asserts final status stays killed). build/-race/full-suite + GOOS=windows green.
|
Fixed the P1 lifecycle race:
Added |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
# Find and read all relevant background package files
fd . internal/background --type f | sort | xargs -I{} sh -c 'echo "=== {} ==="; cat "{}"'Length of output: 37586
1.
|
Vasanthdev2004
left a comment
Vasanthdev2004
left a comment
There was a problem hiding this comment.
Rereviewed PR #134 on current head 39cbd06bef12.
Verdict: APPROVED
The previous blocker is fixed. Manager.Kill now records user kill intent before the blocking POSIX terminate path, so the background Wait() callback cannot clobber a user-initiated stop into error. The new regression test simulates the waiter calling MarkExited while termination is in progress and asserts the final task state remains killed.
Validation I ran locally:
git diff --check origin/main...HEADgo test ./internal/background -count=1go test ./internal/specialist -count=1go test ./...go run ./cmd/zero-release buildgo run ./cmd/zero-release smoke
Note: I’m on Windows, so the POSIX signal-specific tests remain CI-covered rather than locally executed here.
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 `@internal/background/manager.go`:
- Around line 275-283: markKilledIfStillRunning currently returns nil even when
it didn't apply the optimistic "killed" mark, causing Kill to proceed to
killProcess and potentially signal a stale PID; change markKilledIfStillRunning
to return a distinct indicator (e.g. (bool, error) or a sentinel error) that
conveys whether the optimistic mark was actually set, then update the caller in
Kill (the block using markKilledIfStillRunning, killProcess, and
restoreRunningAfterFailedKill) to only call killProcess when the mark was
applied and to return early (nil) when it wasn't; ensure
restoreRunningAfterFailedKill is only invoked when killProcess was attempted and
failed.
- Around line 279-282: The rollback path currently swallows persistence failures
when undoing an optimistic kill (called via
manager.restoreRunningAfterFailedKill after killProcess fails), so if
persistTaskLocked fails the task can remain incorrectly marked killed; change
restoreRunningAfterFailedKill to return an error (or return the
persistTaskLocked error) and update the callers in the kill error paths (the
block that calls manager.restoreRunningAfterFailedKill at the failing-kill
branch and the similar block around lines 352-366) to check that return value
and propagate a combined/wrapped error (e.g., include both the original
killProcess error and any persistTaskLocked error) instead of ignoring the
persistence failure. Ensure you reference and modify
manager.restoreRunningAfterFailedKill, killProcess, and persistTaskLocked
accordingly so rollback persistence failures are surfaced to callers.
🪄 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: cdfafefe-6263-41ba-ad2d-c002e17de4a7
📒 Files selected for processing (2)
internal/background/manager.gointernal/background/manager_test.go
Vasanthdev2004
left a comment
Vasanthdev2004
left a comment
There was a problem hiding this comment.
Updated rereview for PR #134 on current head 39cbd06bef12.
Verdict: CHANGES_REQUESTED
The original stop-vs-waiter race is fixed, but CodeRabbit's two current-head findings are valid and should be addressed before merge:
-
[P1]
Killcan still signal after the optimistic killed mark was not applied.
markKilledIfStillRunningreturnsnilboth when it successfully marks the task and when the task is no longer running.Killthen proceeds tokillProcess(pid)either way. In the exit/reuse race, that can still signal a stale PID. MakemarkKilledIfStillRunningreturn whether it actually marked the task, and only callkillProcesswhen the mark was applied. -
[P2] Rollback persistence failures are swallowed.
IfkillProcessfails after the optimistic mark andrestoreRunningAfterFailedKillcannot persist the rollback, callers only see the kill error while the task may remain stored askilled. HaverestoreRunningAfterFailedKillreturn an error and join/wrap it with the original kill error.
Validation I ran locally before this verdict:
git diff --check origin/main...HEADgo test ./internal/background -count=1go test ./internal/specialist -count=1go test ./...go run ./cmd/zero-release buildgo run ./cmd/zero-release smoke
So the build/test surface is green, but the manager edge cases need a small fix.
Two follow-ups to the kill-ordering fix (CodeRabbit): 1. markKilledIfStillRunning now returns whether it actually marked. If the task exited (or was reused) between the running check and the mark, Kill returns without calling killProcess — so it never signals a possibly-stale pid in the exit/reuse race. 2. restoreRunningAfterFailedKill returns its persistence error; on a failed terminate, Kill joins the kill error and the rollback error (errors.Join) instead of swallowing a failed rollback that would leave the task wrongly marked killed. Adds TestMarkKilledIfStillRunningReportsWhetherItMarked. build/-race/full-suite + GOOS=windows green.
|
Two follow-ups fixed:
Added |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
fd . internal/background --type f | sort | xargs -I{} sh -c 'echo "=== {} ==="; cat "{}"'Length of output: 39192
Two items have now been flagged in both previous rounds and are still not addressed: 1.
|
Vasanthdev2004
left a comment
Vasanthdev2004
left a comment
There was a problem hiding this comment.
Rereviewed PR #134 on current head 3312ffa9d5f2.
Verdict: APPROVED
The prior manager edge cases are fixed. markKilledIfStillRunning now reports whether the optimistic killed mark was actually applied, so Kill does not signal a possibly stale PID when the task is no longer running. The failed-kill rollback now returns and surfaces persistence failures instead of silently leaving the task marked killed. CodeRabbit has also approved this head.
Validation I ran locally:
git diff --check origin/main...HEADgo test ./internal/background -count=1go test ./internal/specialist -count=1go test ./...go run ./cmd/zero-release buildgo run ./cmd/zero-release smoke
Note: I’m on Windows, so the POSIX signal-specific tests are CI-covered rather than locally executed here.
2 participants
Background: escalate process termination from SIGTERM to SIGKILL
terminateProcess(POSIX) sent only SIGTERM, so a background process that traps or ignores it kept running afterKill/KillRunning— a process leak (notably on a Ctrl+C shutdown that kills running tasks).What changed
internal/background/process_posix.go— SIGTERM (so the process can flush/clean up) → poll liveness with signal 0 until a 3s grace deadline → SIGKILL if still alive. An already-exited process (ESRCH /ErrProcessDone) is treated as success. Grace/poll are package vars so tests run fast.taskkill /T /F.Why
A force-kill that can't actually force-kill isn't one. This guarantees a killed task is gone.
Testing
trap '' TERM+ a readiness handshake so we don't signal before the trap installs) is killed only after the grace period — proving SIGTERM is tried first, then escalated.-race+ full suite +GOOS=windowsbuild green. No new deps.Summary by CodeRabbit
Bug Fixes
Tests