Skip to content

fix: gracefully terminate none-runtime process group on cancel#132

Open
zpzjzj wants to merge 1 commit into
mainfrom
fix/none-runtime-graceful-termination
Open

fix: gracefully terminate none-runtime process group on cancel#132
zpzjzj wants to merge 1 commit into
mainfrom
fix/none-runtime-graceful-termination

Conversation

@zpzjzj

@zpzjzj zpzjzj commented Jul 6, 2026

Copy link
Copy Markdown
Collaborator

Summary

The none runtime already starts commands in their own process group and kills the whole group on cancellation, but it did so with an immediate SIGKILL. Children never got a chance to run trap handlers, release locks, or clean up — the exact failure mode the issue describes, where a helper holding a lock left stale state that stalled a later command.

This adds a graceful escalation path:

  • On cancel/timeout, the process group is signaled with SIGTERM first, giving children a chance to clean up.
  • If the group is still alive after a short grace (noneExecKillGrace = 1s), it escalates to SIGKILL.
  • The grace fits inside the existing 2s WaitDelay, so a timed-out Exec still returns within its deadline and preserves the context.DeadlineExceeded / exit-code -1 contract.
  • Whole-group semantics (negative PID) and the single-process fallback are kept. Escalation runs out of band (goroutine) so cmd.Cancel returns immediately and os/exec can start its WaitDelay timer; a done channel closed once Wait returns stops the escalation timer before the PID can be recycled.
  • No-op on non-Unix platforms (none_exec_other.go, guarded by the same build tags).

Tests

  • TestNoneRuntime_ExecTerminatesGracefullyThenEscalates — a SIGTERM trap writes a marker before exit, proving the graceful path runs cleanup rather than an immediate SIGKILL (verified: fails under immediate-SIGKILL, passes with SIGTERM-first).
  • TestNoneRuntime_ExecStaleLockDoesNotHangNextCommand — the issue's lock-file regression: a first command times out while a background child records its PID as a lock; after the timeout the child's group is gone, and a second command sees a dead PID (kill -0) and does not hang.
  • Existing TestNoneRuntime_ExecKillsDescendantsOnTimeout and the deadline/cancel contract tests still pass.

go build ./..., go vet ./..., gofmt -l clean, and go test -race ./... green.

Fixes #120

🤖 Generated with Claude Code

Timed-out or canceled `none` runtime commands previously had their whole
process group hit with an immediate SIGKILL. Children got no chance to run
trap handlers, release locks, or clean up — so a helper holding a lock could
leave stale state that stalled a later command until a long timeout.

Cancellation now signals the group with SIGTERM first, then escalates to
SIGKILL after a short grace (noneExecKillGrace = 1s) if the group has not
exited. The grace fits inside the existing 2s WaitDelay, so a timed-out Exec
still returns within its deadline and preserves the context.DeadlineExceeded
/ exit-code -1 contract. Whole-group semantics (negative PID) and the
single-process fallback are kept; escalation runs out of band so Cancel
returns immediately, and a done channel (closed once Wait returns) stops the
timer before the PID can be recycled. No-op on non-Unix platforms.

Adds a SIGTERM-trap test proving the graceful path runs cleanup before
teardown, and a lock-file regression test proving a stale child is gone after
timeout so a second command does not hang on it.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@zpzjzj zpzjzj requested a review from hittyt as a code owner July 6, 2026 08:42
@zpzjzj zpzjzj requested review from jwx0925 and roark47 July 6, 2026 10:03
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.

Clean up process groups on none runtime timeout or cancellation

1 participant