Files
zero/internal/config/process_posix.go
PierrunoYTandClaude Sonnet 5 5c58256abc fix(config): kill provider-command process tree via job object on Windows (#690)
* fix(config): kill provider-command process tree via job object on Windows

taskkill /T walks the process tree by parent PID in user space and can
miss descendants, leaving the sleeping child holding the inherited
stdout/stderr pipes so cmd.Wait() blocked until natural completion
(~10s instead of the 5s timeout).

Assign the started command to a job object with
JOB_OBJECT_LIMIT_KILL_ON_JOB_CLOSE and terminate the job on timeout so
the kernel kills the whole tree atomically; taskkill remains as a
fallback. Also set cmd.WaitDelay so Wait can never hang on pipes held
by an escaped orphan, and add a regression assertion that the sleeping
child is actually gone after the timeout returns.

Fixes #683

Co-Authored-By: Claude Fable 5 <[email protected]>

* fix(config): harden provider-command tree termination

Address code review on PR #690: terminate the process tree when
cmd.Wait() returns exec.ErrWaitDelay (a background descendant kept
inherited pipes open, previously left running); start the Windows
process suspended and assign it to the job object before resuming its
main thread, closing the window where a fast child could escape the
job; drop JOB_OBJECT_LIMIT_KILL_ON_JOB_CLOSE so a successful command's
detached helpers survive Close() instead of being reaped; escape
single quotes in generated PID-file paths; and correct a misnamed
Windows exit-code constant in a comment.

Co-Authored-By: Claude Sonnet 5 <[email protected]>

* fix(config): avoid gofmt smart-quote rewrite in doc comment

gofmt's doc-comment formatter rewrites a trailing '' into a curly
closing quote, which CI's gofmt -l check flags as unformatted. Reword
to avoid the pattern instead of embedding a raw quote pair.

* fix(config): terminate leaked descendants on nonzero exit, avoid PID reuse

Terminate the process tree on any error returned from cmd.Wait, not only

exec.ErrWaitDelay. Go only returns ErrWaitDelay when the shell itself

exited successfully; a nonzero exit (e.g. sleep 600 & exit 7) yields the

bare *ExitError instead, so a detached background child that held the

inherited pipes open was never terminated on that path.

Also stop falling through to the taskkill/PID fallback once a job handle

successfully contained the tree: by the time Terminate runs on the

ErrWaitDelay path, cmd.Wait has already reaped the root process, so its

PID may have been reused by an unrelated process, and taskkill /T /PID

against a reused PID would force-kill the wrong tree.

Addresses PR #690 review feedback.

* fix(config): surface suspended-thread resume failures instead of hanging

resumeMainThread now reports whether it actually resumed at least one

thread. If Toolhelp/OpenThread/ResumeThread all fail, the launched

process would otherwise sit suspended forever, never exit on its own,

and get misreported as a plain 5s provider-command timeout instead of

the real resume failure. attachCommandProcess now terminates the

process immediately in that case.

Also documents the known, non-regressive limitation that the taskkill

fallback (used only when job creation/assignment itself fails) can

still miss a descendant that has reparented away from the tree it

walks by PID.

Addresses remaining PR #690 review feedback; PR description corrected

separately to drop the outdated JOB_OBJECT_LIMIT_KILL_ON_JOB_CLOSE claim.

* fix(config): guard taskkill with retained process identity

* fix(config): retain POSIX process group identity

* test(tui): match provider wizard shortcut

* test(tui): pin credential-step env for deterministic footer assertion

The credential step's footer only shows the /→ shortcut once a key
is present, whether typed or inherited from the provider's
AuthEnvVars. The previous commit changed the assertion to
"Enter/→ continue" without accounting for that, so the test only
passed by coincidence when ANTHROPIC_API_KEY happened to be set in the
environment — failing deterministically on a clean CI runner (as
smoke reported on all three OSes). Pin the env var empty and assert
the no-key footer text so the test is correct regardless of ambient
environment.

Co-Authored-By: Claude Sonnet 5 <[email protected]>

* fix(config): preserve provider process attributes

* test(config): keep SysProcAttr coverage portable

---------

Co-authored-by: Claude Fable 5 <[email protected]>
2026-07-24 09:03:59 +08:00

62 lines
1.4 KiB
Go

//go:build !windows
package config
import (
"io"
"os/exec"
"syscall"
)
type commandProcess struct {
groupID int
anchor *exec.Cmd
anchorInput io.WriteCloser
}
// startCommandProcess retains a live process-group leader until cleanup. The
// provider shell may be reaped before Wait finishes draining descendant-held
// pipes, so its PID cannot safely identify the group after Wait returns.
func startCommandProcess(cmd *exec.Cmd) (*commandProcess, error) {
anchor := exec.Command("sh", "-c", "read _")
anchor.SysProcAttr = &syscall.SysProcAttr{Setpgid: true}
anchorInput, err := anchor.StdinPipe()
if err != nil {
return nil, err
}
if err := anchor.Start(); err != nil {
_ = anchorInput.Close()
return nil, err
}
proc := &commandProcess{groupID: anchor.Process.Pid, anchor: anchor, anchorInput: anchorInput}
if cmd.SysProcAttr == nil {
cmd.SysProcAttr = &syscall.SysProcAttr{}
}
cmd.SysProcAttr.Setpgid = true
cmd.SysProcAttr.Pgid = proc.groupID
if err := cmd.Start(); err != nil {
proc.Close()
return nil, err
}
return proc, nil
}
func (p *commandProcess) Terminate() {
if p.groupID != 0 {
_ = syscall.Kill(-p.groupID, syscall.SIGKILL)
}
}
func (p *commandProcess) Close() {
if p.anchorInput != nil {
_ = p.anchorInput.Close()
p.anchorInput = nil
}
if p.anchor != nil {
_ = p.anchor.Wait()
p.anchor = nil
}
p.groupID = 0
}