Terminate the script's process group when a SCRIPT_RUN stage is cancelled - #7320
Open
omlahore wants to merge 1 commit into
Open
Terminate the script's process group when a SCRIPT_RUN stage is cancelled#7320omlahore wants to merge 1 commit into
omlahore wants to merge 1 commit into
Conversation
executeCommand ran /bin/sh with exec.Command and no context, in a goroutine, while the caller only selected on ctx.Done(). On cancel or timeout the stage reported CANCELLED and returned, but the shell and everything it spawned kept running against the cluster. Use exec.CommandContext, put the shell in its own process group with Setpgid, and signal the group rather than the child. CommandContext's default cancel signals only the direct child, and its WaitDelay fallback calls Process.Kill() which is also only the child, so a grandchild that ignores SIGTERM survives either way. Cancel now sends SIGTERM to the group and escalates to SIGKILL after a 2s grace period. Also corrects %w to %v in the exec failure log, which rendered as %!w(*exec.ExitError=...) because StageLogPersister.Errorf does not wrap. Signed-off-by: Om <omlahore47@gmail.com>
✅ Deploy Preview for pipecd-site canceled.
|
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
The current cmd.Cancel implementation can return a spurious error on cancellation races (e.g., ESRCH), which should be handled to avoid incorrect error paths/logging during cancel.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR fixes cancellation behavior for the pipedv1 scriptrun plugin so that cancelling a SCRIPT_RUN (or rollback) stage terminates the entire shell process tree, preventing orphaned commands from continuing to mutate cluster state after the stage is cancelled.
Changes:
- Switches
executeCommandtoexec.CommandContextand runs the shell as a separate process group (Setpgid) so the whole tree can be signaled. - Implements group-wide cancellation: SIGTERM to the process group, with SIGKILL escalation after a short grace period.
- Adds tests to verify the process group is terminated on cancel and that the success path remains fast.
File summaries
| File | Description |
|---|---|
| pkg/app/pipedv1/plugin/scriptrun/plugin.go | Adds context-aware execution and process-group termination logic for cancellation. |
| pkg/app/pipedv1/plugin/scriptrun/plugin_cancel_test.go | Adds regression tests ensuring cancellation kills background descendants and normal execution stays unaffected. |
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+218
to
+223
| cmd.Cancel = func() error { | ||
| pgid := cmd.Process.Pid | ||
| lp.Infof("Cancelling script, sending SIGTERM to process group %d", pgid) | ||
| if err := syscall.Kill(-pgid, syscall.SIGTERM); err != nil { | ||
| return err | ||
| } |
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
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.
Related to #6734, which covers the same pattern in the v0 executors. This PR is the pipedv1 plugin only, to keep it to one concern.
What happens today
executeCommandinpkg/app/pipedv1/plugin/scriptrun/plugin.goruns the user script withexec.Command, taking no context at all, in a goroutine. The caller only selects onctx.Done():So on cancel or timeout the stage reports cancelled and returns, and nothing ever signals the shell. The script keeps running, keeps mutating the cluster, and the goroutine stays parked in
cmd.Run()for as long as it takes.I reproduced it. A script that backgrounds a child, with both trapping SIGTERM:
The change
exec.CommandContext, the shell placed in its own process group withSetpgid, and aCancelthat signals the group rather than the child.The process-group part is the bit that matters, and it is why
CommandContextalone is not enough. Its default cancel signals only the direct child, and itsWaitDelayfallback callsProcess.Kill(), which is also only the child. Either way a grandchild that ignores SIGTERM outlives the stage. SoCancelsends SIGTERM to-pgidand a goroutine escalates to SIGKILL on the group after a grace period, stood down as soon asWaitreturns.The grace period is a named constant rather than an inline literal:
A long
terraform applymay eventually want that configurable. Nothing in the stage config exposes it today, so I left it constant rather than inventing a schema field.Portability
syscall.Setpgidandsyscall.Killare Unix-only, and I did not add a build tag, matching what the repo already does://go:buildfiles anywhere in the treepkg/lifecycle/binary.go:57already callsc.cmd.Process.Signal(syscall.SIGTERM)unguardedGOOS, andMakefiledefaults to the host/bin/sh -l -cSay the word if you would rather have
//go:build unixon it anyway.Tests
plugin_cancel_test.go:TestExecuteCommandKillsProcessGroupOnCancelstarts a script whose backgrounded grandchild traps SIGTERM, cancels the context, and asserts the grandchild is gone. It takes ~2s, which is the grace period, so it is genuinely exercising the SIGKILL escalation and not just the SIGTERM.TestExecuteCommandSucceedsWithoutCancelpins the normal path: a command that exits on its own still returns success and does not wait out the grace period.go build,go vetandgo test ./...all pass in the plugin module.One drive-by
Line 238 was
lp.Errorf("failed to exec command: %w", err).StageLogPersister.Errorfdoes not wrap, so that rendered literally as%!w(*exec.ExitError=&{...}). I noticed it in my own test output and changed it to%v. Say the word if you would rather it were separate.