Skip to content

Commit c4af2ff

Browse files
authored
fix(launcher): repeat the group SIGKILL until reaped so a missed member cannot hold Done open (#1652)
Closes #1649 ## Root cause `TestRetire_LauncherKillReachesDescendants` failure is a real (rare) production slow path, not only a tight test window. A SIGKILL to the launcher's process group can miss a group member that is being forked at that instant (reproduced locally under CPU load: a `sleep 30` orphan survived the group kill, ppid 1, same pgid). That member inherits the launcher's stdout/stderr, so `reap()` blocks in `pumpWG.Wait()` and then in `awaitGroupEmpty` (the exited leader is deliberately kept unreaped while a group member runs). Done stayed open until the member exited: 30 s here, forever for a daemon. The test's `treeAlive` cannot see it (`sleep 30` has no dir in its command line), so only the Done wait failed. ## Fix - `handle.killed` is set when a SIGKILL is sent (Kill, Stop escalation, retirement). - `repeatKillUntilReaped` (started before the pump drain, stopped after the reap) re-sends SIGKILL every 50 ms under `reapMu` only while the leader is unreaped, so the zombie/live leader still pins the pgid (no reuse risk). No-op on Windows (Job Object). - Test: `TestHandle_KillMissedMember_RepeatedUntilGroupEmpty` (fails without the fix); the core test's Done wait widened to 15 s. ## Verification race tests ./internal/upstream/... bare and -tags server, GOOS=windows vet, golangci-lint (only pre-existing govet inline in client_secret_test.go), 40x TestRetire_ under 30 CPU spinners. Cross-reviewed with opencode Sol 6.1: round 1 ISSUES (retry must start before pump drain; fixed), round 2 CLEAN.
1 parent 39d4587 commit c4af2ff

5 files changed

Lines changed: 93 additions & 1 deletion

File tree

‎internal/upstream/core/retire_kill_test.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -119,7 +119,7 @@ func TestRetire_LauncherKillReachesDescendants(t *testing.T) {
119119
"Retire kills the launcher child's whole tree")
120120
select {
121121
case <-h.Done():
122-
case <-time.After(2 * time.Second):
122+
case <-time.After(15 * time.Second):
123123
t.Fatal("the killed launcher child was not reaped")
124124
}
125125
assert.NoError(t, h.Kill(), "Kill after exit is a no-op")

‎internal/upstream/launcher/kill_unix_test.go‎

Lines changed: 34 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -428,3 +428,37 @@ func TestHandle_ObservationFailure_KillNotBlockedAndExitStatusKept(t *testing.T)
428428
<-h3.Done()
429429
require.NoError(t, h3.Wait())
430430
}
431+
432+
// A SIGKILL can miss a group member that was being forked while the signal
433+
// was delivered (#1649). The kill was requested, so the reaper repeats it
434+
// while it keeps the exited leader unreaped, and Done closes instead of
435+
// staying open for as long as the missed member runs.
436+
func TestHandle_KillMissedMember_RepeatedUntilGroupEmpty(t *testing.T) {
437+
dir := t.TempDir()
438+
pidFile := filepath.Join(dir, "descendant.pid")
439+
cmd := exec.Command("/bin/sh", "-c",
440+
`sleep 300 & echo $! > "`+pidFile+`"`)
441+
h, err := Spawn(context.Background(), &Spec{Cmd: cmd, Name: "bg-missed"}, zap.NewNop())
442+
require.NoError(t, err)
443+
hh := h.(*handle)
444+
pid := readPid(t, pidFile)
445+
t.Cleanup(func() { _ = syscall.Kill(pid, syscall.SIGKILL) })
446+
require.Eventually(t, func() bool { return !liveNonZombie(hh.cmd.Process.Pid) },
447+
10*time.Second, 20*time.Millisecond, "the launcher leader did not exit")
448+
select {
449+
case <-h.Done():
450+
t.Fatal("control: Done closed while the descendant runs")
451+
case <-time.After(300 * time.Millisecond):
452+
}
453+
454+
// The member keeps the launcher's output pipes, so the pumps cannot finish
455+
// either. The kill was requested but its signal missed the descendant.
456+
hh.killed.Store(true)
457+
select {
458+
case <-h.Done():
459+
case <-time.After(10 * time.Second):
460+
t.Fatal("a requested kill was not repeated to the member it missed")
461+
}
462+
require.Eventually(t, func() bool { return !liveNonZombie(pid) },
463+
5*time.Second, 20*time.Millisecond, "the missed descendant must be killed")
464+
}

‎internal/upstream/launcher/launcher.go‎

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -246,6 +246,12 @@ type handle struct {
246246
reapMu sync.Mutex
247247
reaped bool
248248

249+
// killed is set once a SIGKILL was sent to the group (Unix). A member
250+
// forked while that signal was delivered can be missed by it, so the
251+
// reaper, while it keeps the exited leader unreaped, repeats the kill
252+
// until the leader is reaped (repeatKillUntilReaped).
253+
killed atomic.Bool
254+
249255
stopOnce sync.Once
250256
stopErr error
251257

@@ -356,6 +362,7 @@ func (h *handle) reap() {
356362
// exec.Cmd doc: "it is thus incorrect to call Wait before all reads from the
357363
// pipe have completed." Pumps naturally finish when the child exits and the
358364
// kernel propagates EOF — we don't need Wait() to reach that state.
365+
stopRepeat := h.repeatKillUntilReaped()
359366
h.pumpWG.Wait()
360367

361368
// Reap the leader under reapMu (review round 4). cmd.Wait releases the
@@ -392,6 +399,7 @@ func (h *handle) reap() {
392399
} else {
393400
exitCode, err = h.reapUnobserved()
394401
}
402+
stopRepeat()
395403
if hook := afterReapHook.Load(); hook != nil {
396404
(*hook)(h)
397405
}

‎internal/upstream/launcher/launcher_unix.go‎

Lines changed: 46 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -76,6 +76,9 @@ func (h *handle) signalUnreaped(sig syscall.Signal) error {
7676
if h.reaped {
7777
return nil
7878
}
79+
if sig == syscall.SIGKILL {
80+
h.killed.Store(true)
81+
}
7982
return h.signalLocked(sig)
8083
}
8184

@@ -164,6 +167,49 @@ func (h *handle) awaitGroupEmpty() {
164167
}
165168
}
166169

170+
// repeatKillInterval is how often repeatKillUntilReaped re-sends SIGKILL.
171+
const repeatKillInterval = 50 * time.Millisecond
172+
173+
// repeatKillUntilReaped runs for the whole of reap and returns a stop
174+
// function. Once a SIGKILL was requested (Kill, Stop's escalation or a
175+
// retirement) it repeats the signal to the group until the leader is reaped.
176+
// A member forked while the first signal was delivered is not in the group
177+
// yet when the kernel walks it, so it survives that kill; without a repeat
178+
// the exited leader stays unreaped, and Done stays open, for as long as that
179+
// member runs (forever for a daemon). It must run beside the reaper's pump
180+
// drain too: the missed member holds the output pipes, so the pumps (and
181+
// with them awaitGroupEmpty) cannot finish until it dies. Every signal is
182+
// sent under reapMu with the leader not reaped, so the leader, alive or a
183+
// zombie, still pins the group ID.
184+
func (h *handle) repeatKillUntilReaped() (stop func()) {
185+
quit := make(chan struct{})
186+
finished := make(chan struct{})
187+
go func() {
188+
defer close(finished)
189+
t := time.NewTicker(repeatKillInterval)
190+
defer t.Stop()
191+
for {
192+
select {
193+
case <-quit:
194+
return
195+
case <-t.C:
196+
}
197+
if !h.killed.Load() {
198+
continue
199+
}
200+
h.reapMu.Lock()
201+
if !h.reaped {
202+
_ = h.signalLocked(syscall.SIGKILL)
203+
}
204+
h.reapMu.Unlock()
205+
}
206+
}()
207+
return func() {
208+
close(quit)
209+
<-finished
210+
}
211+
}
212+
167213
// killGroupBeforeReap runs in the reaper under reapMu, after awaitGroupEmpty
168214
// and before cmd.Wait reaps the leader. The zombie leader still holds the
169215
// group ID, so the SIGKILL reaches only what is left of our own group:

‎internal/upstream/launcher/launcher_windows.go‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -133,3 +133,7 @@ func (h *handle) killGroupBeforeReap() {}
133133
// awaitGroupEmpty is a no-op on Windows (no process group; see
134134
// killGroupBeforeReap).
135135
func (h *handle) awaitGroupEmpty() {}
136+
137+
// repeatKillUntilReaped is a no-op on Windows: the Job Object kill reaches
138+
// every member, including ones created while it was delivered.
139+
func (h *handle) repeatKillUntilReaped() (stop func()) { return func() {} }

0 commit comments

Comments
 (0)