Repository navigation
fix(daemon): launcher.go call sites still use fragile Getpgid rediscovery path #861
Description
Context
Follow-up from review of PR #774, which introduced TerminateProcessGroup/terminateOwnedProcess specifically to avoid the fragile Getpgid rediscovery in TerminateProcessTree (documented at internal/execution/process_unix.go:62-69: on Darwin, Getpgid can return ESRCH once an unreaped group leader has exited, even though live descendants remain in the group it configured, causing TerminateProcessTree to silently fall back to signalling only the dead leader PID and leaving descendants running).
Two call sites still hold an *exec.Cmd that went through background.ConfigureChildProcessGroup (so launch-time group identity is known) but call background.TerminateProcess(pid), which routes through the fragile rediscovery path instead of the stronger primitive:
internal/daemon/launcher.go:77(execWorker.Kill)internal/daemon/launcher.go:163(cmd.Cancel)
internal/specialist/exec.go:493 is correctly left alone since it only has a bare PID (no launch-time group knowledge to leverage).
Proposed fix
Since both call sites hold the *exec.Cmd, they can call background.TerminateCommand(cmd) (or a comparable helper that uses terminateOwnedProcess) instead of background.TerminateProcess(cmd.Process.Pid), so they take the launch-time-invariant path rather than rediscovery.
Not blocking; pre-existing behavior, same shape as the daemon-start path #774 fixed.
Activity
fix(daemon): kill launcher workers via launch-time process group
Vasanthdev2004 commentedon Sep 8, 2026
Labelled and approved. I checked the open PRs that touch the daemon (#927, #987) and neither reaches these two call sites, so nothing is in flight for this. The fix is what the issue already prescribes: the two sites hold an
*exec.Cmdthat went throughConfigureChildProcessGroup, so they should call the group primitive rather thanTerminateProcess(pid)and its rediscovery path. A regression that makes the leader exit unreaped before termination, then asserts the descendants are gone, is the test that proves it.Vasanthdev2004 commentedon Sep 12, 2026
Back in the pool, and still valid on current main.
This came up in an audit of open issues whose only linked pull request had closed, which made them look covered to a query while nobody was actually working them. The earlier attempt closed without landing, so this is open for a fresh one.
I verified the problem still exists rather than assuming it, and the audit verdict was independently re-checked before I posted this. The label is already on, so a PR implementing it can merge on review.
fix(daemon): stop worker process groups by their launch-time identity
fix(daemon): stop worker process groups by their launch-time identity (…