Skip to content

Commit a79e677

Browse files
committed
Terminate the script's process group when a stage is cancelled
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>
1 parent 0dd3280 commit a79e677

2 files changed

Lines changed: 158 additions & 7 deletions

File tree

pkg/app/pipedv1/plugin/scriptrun/plugin.go

Lines changed: 45 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -22,6 +22,8 @@ import (
2222
"os/exec"
2323
"strconv"
2424
"strings"
25+
"syscall"
26+
"time"
2527

2628
sdk "github.com/pipe-cd/piped-plugin-sdk-go"
2729
)
@@ -30,7 +32,13 @@ const (
3032
stageScriptRun = "SCRIPT_RUN"
3133
stageScriptRunRollback = "SCRIPT_RUN_ROLLBACK"
3234
metadataKeyPrefix = "started-"
33-
nonEmptyValue = "_"
35+
36+
// commandTerminationGracePeriod is how long the script's process group gets
37+
// after SIGTERM before it is SIGKILLed. Long-running commands such as
38+
// terraform apply may want this configurable; it is a constant for now
39+
// because nothing in the stage config exposes it yet.
40+
commandTerminationGracePeriod = 2 * time.Second
41+
nonEmptyValue = "_"
3442
)
3543

3644
type ContextInfo struct {
@@ -110,7 +118,7 @@ func executeScriptRun(ctx context.Context, request sdk.ExecuteStageRequest[struc
110118
}
111119
c := make(chan sdk.StageStatus, 1)
112120
go func() {
113-
c <- executeCommand(opts.Run, opts.Env, request, lp)
121+
c <- executeCommand(ctx, opts.Run, opts.Env, request, lp)
114122
}()
115123
select {
116124
case result := <-c:
@@ -143,7 +151,7 @@ func executeRollback(ctx context.Context, request sdk.ExecuteStageRequest[struct
143151
}
144152
c := make(chan sdk.StageStatus, 1)
145153
go func() {
146-
c <- executeCommand(opts.OnRollback, opts.Env, request, lp)
154+
c <- executeCommand(ctx, opts.OnRollback, opts.Env, request, lp)
147155
}()
148156
select {
149157
case result := <-c:
@@ -158,7 +166,7 @@ func executeRollback(ctx context.Context, request sdk.ExecuteStageRequest[struct
158166
func (p *plugin) FetchDefinedStages() []string {
159167
return []string{stageScriptRun, stageScriptRunRollback}
160168
}
161-
func executeCommand(commands string, customEnv map[string]string, request sdk.ExecuteStageRequest[struct{}], lp sdk.StageLogPersister) sdk.StageStatus {
169+
func executeCommand(ctx context.Context, commands string, customEnv map[string]string, request sdk.ExecuteStageRequest[struct{}], lp sdk.StageLogPersister) sdk.StageStatus {
162170
lp.Infof("Running commands...")
163171
for _, v := range strings.Split(commands, "\n") {
164172
if v != "" {
@@ -191,13 +199,43 @@ func executeCommand(commands string, customEnv map[string]string, request sdk.Ex
191199
envs = append(envs, key+"="+value)
192200
}
193201

194-
cmd := exec.Command("/bin/sh", "-l", "-c", commands)
202+
cmd := exec.CommandContext(ctx, "/bin/sh", "-l", "-c", commands)
195203
cmd.Env = append(os.Environ(), envs...)
196204
cmd.Dir = request.TargetDeploymentSource.ApplicationDirectory
197205
cmd.Stdout = lp
198206
cmd.Stderr = lp
199-
if err := cmd.Run(); err != nil {
200-
lp.Errorf("failed to exec command: %w", err)
207+
208+
// Run the shell as its own process group leader so the whole tree can be
209+
// signalled. Without this, cancelling the stage kills /bin/sh and leaves
210+
// whatever it spawned still running against the cluster.
211+
cmd.SysProcAttr = &syscall.SysProcAttr{Setpgid: true}
212+
213+
// exec.CommandContext's default cancel only signals the direct child, and
214+
// its WaitDelay fallback calls Process.Kill(), which is also just the
215+
// child. Signal the group instead, then escalate to SIGKILL on the group so
216+
// a grandchild that ignores SIGTERM cannot outlive the stage.
217+
stopEscalation := make(chan struct{})
218+
cmd.Cancel = func() error {
219+
pgid := cmd.Process.Pid
220+
lp.Infof("Cancelling script, sending SIGTERM to process group %d", pgid)
221+
if err := syscall.Kill(-pgid, syscall.SIGTERM); err != nil {
222+
return err
223+
}
224+
go func() {
225+
select {
226+
case <-stopEscalation:
227+
case <-time.After(commandTerminationGracePeriod):
228+
lp.Infof("Script did not exit within %s, sending SIGKILL to process group %d", commandTerminationGracePeriod, pgid)
229+
_ = syscall.Kill(-pgid, syscall.SIGKILL)
230+
}
231+
}()
232+
return nil
233+
}
234+
235+
err = cmd.Run()
236+
close(stopEscalation)
237+
if err != nil {
238+
lp.Errorf("failed to exec command: %v", err)
201239
return sdk.StageStatusFailure
202240
} else {
203241
return sdk.StageStatusSuccess
Lines changed: 113 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,113 @@
1+
// Copyright 2026 The PipeCD Authors.
2+
//
3+
// Licensed under the Apache License, Version 2.0 (the "License");
4+
// you may not use this file except in compliance with the License.
5+
// You may obtain a copy of the License at
6+
//
7+
// http://www.apache.org/licenses/LICENSE-2.0
8+
//
9+
// Unless required by applicable law or agreed to in writing, software
10+
// distributed under the License is distributed on an "AS IS" BASIS,
11+
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
12+
// See the License for the specific language governing permissions and
13+
// limitations under the License.
14+
15+
package main
16+
17+
import (
18+
"context"
19+
"fmt"
20+
"os"
21+
"path/filepath"
22+
"syscall"
23+
"testing"
24+
"time"
25+
26+
sdk "github.com/pipe-cd/piped-plugin-sdk-go"
27+
"github.com/pipe-cd/piped-plugin-sdk-go/logpersister/logpersistertest"
28+
"github.com/stretchr/testify/assert"
29+
"github.com/stretchr/testify/require"
30+
)
31+
32+
// alive reports whether pid is still running. Signal 0 performs the permission
33+
// and existence checks without delivering anything.
34+
func alive(pid int) bool {
35+
return syscall.Kill(pid, 0) == nil
36+
}
37+
38+
func waitGone(pid int, d time.Duration) bool {
39+
deadline := time.Now().Add(d)
40+
for time.Now().Before(deadline) {
41+
if !alive(pid) {
42+
return true
43+
}
44+
time.Sleep(10 * time.Millisecond)
45+
}
46+
return !alive(pid)
47+
}
48+
49+
// A cancelled stage must take the whole process tree with it. The script here
50+
// backgrounds a child that traps SIGTERM and keeps running, which is what a
51+
// naive kill of /bin/sh alone would leave behind holding cluster state.
52+
func TestExecuteCommandKillsProcessGroupOnCancel(t *testing.T) {
53+
t.Parallel()
54+
55+
dir := t.TempDir()
56+
pidFile := filepath.Join(dir, "child.pid")
57+
58+
script := fmt.Sprintf(`
59+
trap '' TERM
60+
( trap '' TERM; echo $$ > %q; while true; do sleep 0.05; done ) &
61+
while true; do sleep 0.05; done
62+
`, pidFile)
63+
64+
ctx, cancel := context.WithCancel(context.Background())
65+
66+
done := make(chan sdk.StageStatus, 1)
67+
go func() {
68+
done <- executeCommand(ctx, script, nil, sdk.ExecuteStageRequest[struct{}]{
69+
StageName: stageScriptRun,
70+
Deployment: sdk.Deployment{ID: "deployment-1", ApplicationID: "app-1"},
71+
}, logpersistertest.NewTestLogPersister(t))
72+
}()
73+
74+
// Wait for the grandchild to record its pid.
75+
var childPID int
76+
require.Eventually(t, func() bool {
77+
b, err := os.ReadFile(pidFile)
78+
if err != nil {
79+
return false
80+
}
81+
_, err = fmt.Sscanf(string(b), "%d", &childPID)
82+
return err == nil && childPID > 0
83+
}, 10*time.Second, 20*time.Millisecond, "grandchild never started")
84+
85+
require.True(t, alive(childPID), "grandchild should be running before cancel")
86+
87+
cancel()
88+
89+
select {
90+
case <-done:
91+
case <-time.After(30 * time.Second):
92+
t.Fatal("executeCommand did not return after cancel")
93+
}
94+
95+
assert.True(t, waitGone(childPID, 15*time.Second),
96+
"grandchild %d survived cancellation; the process group was not terminated", childPID)
97+
}
98+
99+
// The normal path must be unaffected: a command that finishes on its own still
100+
// reports success and does not wait out the grace period.
101+
func TestExecuteCommandSucceedsWithoutCancel(t *testing.T) {
102+
t.Parallel()
103+
104+
start := time.Now()
105+
status := executeCommand(context.Background(), "echo hello", nil, sdk.ExecuteStageRequest[struct{}]{
106+
StageName: stageScriptRun,
107+
Deployment: sdk.Deployment{ID: "deployment-1", ApplicationID: "app-1"},
108+
}, logpersistertest.NewTestLogPersister(t))
109+
110+
assert.Equal(t, sdk.StageStatusSuccess, status)
111+
assert.Less(t, time.Since(start), commandTerminationGracePeriod,
112+
"a command that exits on its own must not wait for the termination grace period")
113+
}

0 commit comments

Comments
 (0)