Files
zero/internal/config/command.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

134 lines
3.5 KiB
Go

package config
import (
"bytes"
"encoding/json"
"errors"
"fmt"
"os/exec"
"runtime"
"strings"
"time"
)
const providerCommandTimeout = 5 * time.Second
func LoadProviderCommand(command string) (FileConfig, error) {
stdout, stderr, err := runProviderCommand(command, providerCommandTimeout)
if err != nil {
if errors.Is(err, errProviderCommandTimeout) {
return FileConfig{}, fmt.Errorf("provider command timed out after 5s")
}
return FileConfig{}, fmt.Errorf("provider command failed: %w%s", err, commandOutput(stderr))
}
cfg, err := parseProviderCommandJSON(stdout)
if err != nil {
return FileConfig{}, err
}
if len(cfg.Providers) == 0 {
return FileConfig{}, fmt.Errorf("provider command returned no providers")
}
providers, _, err := normalizeProvidersWithoutModelDefaults(cfg.Providers, cfg.ActiveProvider, map[string]string{})
if err != nil {
return FileConfig{}, err
}
cfg.Providers = providers
return cfg, nil
}
var errProviderCommandTimeout = errors.New("provider command timeout")
func runProviderCommand(command string, timeout time.Duration) ([]byte, []byte, error) {
cmd := shellCommand(command)
// Bound the time Wait spends draining I/O pipes after the process exits,
// in case an orphaned descendant still holds the write ends open.
cmd.WaitDelay = time.Second
var stdout bytes.Buffer
var stderr bytes.Buffer
cmd.Stdout = &stdout
cmd.Stderr = &stderr
proc, err := startCommandProcess(cmd)
if err != nil {
return nil, nil, err
}
defer proc.Close()
done := make(chan error, 1)
go func() {
done <- cmd.Wait()
}()
timer := time.NewTimer(timeout)
defer timer.Stop()
select {
case err := <-done:
if err != nil {
// The shell exited (whether via ErrWaitDelay because a
// background descendant kept the inherited stdout/stderr pipes
// open, or via a nonzero exit status) but a leftover descendant
// may still be running. Terminate is a no-op against an
// already-dead tree, so always call it rather than gating on
// the specific error to avoid leaking that descendant.
proc.Terminate()
}
if errors.Is(err, exec.ErrWaitDelay) {
return stdout.Bytes(), stderr.Bytes(), errProviderCommandTimeout
}
return stdout.Bytes(), stderr.Bytes(), err
case <-timer.C:
proc.Terminate()
<-done
return stdout.Bytes(), stderr.Bytes(), errProviderCommandTimeout
}
}
func shellCommand(command string) *exec.Cmd {
if runtime.GOOS == "windows" {
if strings.HasPrefix(strings.TrimSpace(command), `"`) {
command = "call " + command
}
return exec.Command("cmd", "/C", command)
}
return exec.Command("sh", "-c", command)
}
func commandOutput(stderr []byte) string {
output := strings.TrimSpace(string(stderr))
if output == "" {
return ""
}
return ": " + redactSecrets(output)
}
func parseProviderCommandJSON(data []byte) (FileConfig, error) {
data = []byte(strings.TrimSpace(string(data)))
if len(data) == 0 {
return FileConfig{}, fmt.Errorf("provider command returned empty JSON")
}
var cfg FileConfig
if err := json.Unmarshal(data, &cfg); err == nil && (len(cfg.Providers) > 0 || cfg.ActiveProvider != "" || cfg.MaxTurns > 0) {
if cfg.ActiveProvider == "" && len(cfg.Providers) == 1 {
cfg.ActiveProvider = cfg.Providers[0].Name
}
return cfg, nil
}
var profile ProviderProfile
if err := json.Unmarshal(data, &profile); err != nil {
return FileConfig{}, fmt.Errorf("invalid provider command JSON: %w", err)
}
if profile.Name == "" {
profile.Name = string(ProviderKindOpenAI)
}
return FileConfig{
ActiveProvider: profile.Name,
Providers: []ProviderProfile{profile},
}, nil
}