From a1d4f99725d2d15422f1534e8ec329f8f218e234 Mon Sep 17 00:00:00 2001 From: PierrunoYT Date: Tue, 14 Jul 2026 22:48:33 +0200 Subject: [PATCH 1/3] 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 --- internal/config/command.go | 8 ++- internal/config/command_test.go | 37 ++++++++++++- internal/config/process_posix.go | 18 ++++-- internal/config/process_posix_test.go | 10 ++++ internal/config/process_windows.go | 73 ++++++++++++++++++++++++- internal/config/process_windows_test.go | 20 +++++++ 6 files changed, 156 insertions(+), 10 deletions(-) create mode 100644 internal/config/process_posix_test.go create mode 100644 internal/config/process_windows_test.go diff --git a/internal/config/command.go b/internal/config/command.go index ba4094ee1..4422d01b5 100644 --- a/internal/config/command.go +++ b/internal/config/command.go @@ -43,6 +43,9 @@ var errProviderCommandTimeout = errors.New("provider command timeout") func runProviderCommand(command string, timeout time.Duration) ([]byte, []byte, error) { cmd := shellCommand(command) configureCommandProcess(cmd) + // 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 @@ -53,6 +56,9 @@ func runProviderCommand(command string, timeout time.Duration) ([]byte, []byte, return nil, nil, err } + proc := attachCommandProcess(cmd) + defer proc.Close() + done := make(chan error, 1) go func() { done <- cmd.Wait() @@ -65,7 +71,7 @@ func runProviderCommand(command string, timeout time.Duration) ([]byte, []byte, case err := <-done: return stdout.Bytes(), stderr.Bytes(), err case <-timer.C: - terminateCommandProcess(cmd) + proc.Terminate() <-done return stdout.Bytes(), stderr.Bytes(), errProviderCommandTimeout } diff --git a/internal/config/command_test.go b/internal/config/command_test.go index 5127424c3..d201c811b 100644 --- a/internal/config/command_test.go +++ b/internal/config/command_test.go @@ -4,6 +4,7 @@ import ( "os" "path/filepath" "runtime" + "strconv" "strings" "testing" "time" @@ -67,7 +68,8 @@ func TestLoadProviderCommandFailureIncludesExitAndRedactsOutput(t *testing.T) { } func TestLoadProviderCommandTimeout(t *testing.T) { - command := writeCommand(t, commandScript{SleepSeconds: 10}) + pidFile := filepath.Join(t.TempDir(), "sleep.pid") + command := writeCommand(t, commandScript{SleepSeconds: 10, PidFile: pidFile}) start := time.Now() _, err := LoadProviderCommand(command) @@ -85,6 +87,29 @@ func TestLoadProviderCommandTimeout(t *testing.T) { if elapsed > maxElapsed { t.Fatalf("timeout returned after %s, want roughly 5s", elapsed) } + assertProcessTerminated(t, pidFile) +} + +func assertProcessTerminated(t *testing.T, pidFile string) { + t.Helper() + + data, err := os.ReadFile(pidFile) + if err != nil { + t.Fatalf("read sleeper pid file: %v", err) + } + pid, err := strconv.Atoi(strings.TrimSpace(string(data))) + if err != nil { + t.Fatalf("parse sleeper pid %q: %v", data, err) + } + + deadline := time.Now().Add(3 * time.Second) + for time.Now().Before(deadline) { + if !processAlive(pid) { + return + } + time.Sleep(100 * time.Millisecond) + } + t.Fatalf("sleeper process %d still alive after timeout", pid) } func TestLoadProviderCommandInvalidJSON(t *testing.T) { @@ -118,6 +143,7 @@ type commandScript struct { Stderr string ExitCode int SleepSeconds int + PidFile string } func writeCommand(t *testing.T, script commandScript) string { @@ -128,7 +154,11 @@ func writeCommand(t *testing.T, script commandScript) string { path := filepath.Join(dir, "provider.cmd") lines := []string{"@echo off"} if script.SleepSeconds > 0 { - lines = append(lines, "powershell -NoProfile -Command \"Start-Sleep -Seconds "+itoa(script.SleepSeconds)+"\"") + sleep := "Start-Sleep -Seconds " + itoa(script.SleepSeconds) + if script.PidFile != "" { + sleep = "Set-Content -Path '" + script.PidFile + "' -Value $PID -Encoding Ascii; " + sleep + } + lines = append(lines, "powershell -NoProfile -Command \""+sleep+"\"") } if script.Stdout != "" { lines = append(lines, "echo "+script.Stdout) @@ -146,6 +176,9 @@ func writeCommand(t *testing.T, script commandScript) string { path := filepath.Join(dir, "provider.sh") lines := []string{"#!/bin/sh"} if script.SleepSeconds > 0 { + if script.PidFile != "" { + lines = append(lines, "echo $$ > '"+script.PidFile+"'") + } lines = append(lines, "sleep "+itoa(script.SleepSeconds)) } if script.Stdout != "" { diff --git a/internal/config/process_posix.go b/internal/config/process_posix.go index 501b07536..7138ef03f 100644 --- a/internal/config/process_posix.go +++ b/internal/config/process_posix.go @@ -11,10 +11,20 @@ func configureCommandProcess(cmd *exec.Cmd) { cmd.SysProcAttr = &syscall.SysProcAttr{Setpgid: true} } -func terminateCommandProcess(cmd *exec.Cmd) { - if cmd.Process == nil { +type commandProcess struct { + cmd *exec.Cmd +} + +func attachCommandProcess(cmd *exec.Cmd) *commandProcess { + return &commandProcess{cmd: cmd} +} + +func (p *commandProcess) Terminate() { + if p.cmd.Process == nil { return } - _ = syscall.Kill(-cmd.Process.Pid, syscall.SIGKILL) - _ = cmd.Process.Kill() + _ = syscall.Kill(-p.cmd.Process.Pid, syscall.SIGKILL) + _ = p.cmd.Process.Kill() } + +func (p *commandProcess) Close() {} diff --git a/internal/config/process_posix_test.go b/internal/config/process_posix_test.go new file mode 100644 index 000000000..dd5ccb65d --- /dev/null +++ b/internal/config/process_posix_test.go @@ -0,0 +1,10 @@ +//go:build !windows + +package config + +import "syscall" + +func processAlive(pid int) bool { + err := syscall.Kill(pid, 0) + return err == nil || err == syscall.EPERM +} diff --git a/internal/config/process_windows.go b/internal/config/process_windows.go index 2e358887d..49d193b43 100644 --- a/internal/config/process_windows.go +++ b/internal/config/process_windows.go @@ -7,17 +7,84 @@ import ( "os/exec" "path/filepath" "strconv" + "unsafe" + + "golang.org/x/sys/windows" ) func configureCommandProcess(cmd *exec.Cmd) {} -func terminateCommandProcess(cmd *exec.Cmd) { +// commandProcess tracks a started provider command so its entire process +// tree can be terminated on timeout. It prefers a job object: taskkill /T +// walks the tree by parent PID in user space and can miss descendants, +// letting them run to completion while Wait blocks on inherited pipes. +type commandProcess struct { + cmd *exec.Cmd + job windows.Handle +} + +func attachCommandProcess(cmd *exec.Cmd) *commandProcess { + proc := &commandProcess{cmd: cmd} if cmd.Process == nil { + return proc + } + job, err := createKillOnCloseJob() + if err != nil { + return proc + } + handle, err := windows.OpenProcess(windows.PROCESS_SET_QUOTA|windows.PROCESS_TERMINATE, false, uint32(cmd.Process.Pid)) + if err != nil { + _ = windows.CloseHandle(job) + return proc + } + defer func() { _ = windows.CloseHandle(handle) }() + if err := windows.AssignProcessToJobObject(job, handle); err != nil { + _ = windows.CloseHandle(job) + return proc + } + proc.job = job + return proc +} + +func createKillOnCloseJob() (windows.Handle, error) { + job, err := windows.CreateJobObject(nil, nil) + if err != nil { + return 0, err + } + info := windows.JOBOBJECT_EXTENDED_LIMIT_INFORMATION{ + BasicLimitInformation: windows.JOBOBJECT_BASIC_LIMIT_INFORMATION{ + LimitFlags: windows.JOB_OBJECT_LIMIT_KILL_ON_JOB_CLOSE, + }, + } + if _, err := windows.SetInformationJobObject(job, windows.JobObjectExtendedLimitInformation, uintptr(unsafe.Pointer(&info)), uint32(unsafe.Sizeof(info))); err != nil { + _ = windows.CloseHandle(job) + return 0, err + } + return job, nil +} + +func (p *commandProcess) Terminate() { + if p.job != 0 { + _ = windows.TerminateJobObject(p.job, 1) + } + if p.cmd.Process == nil { return } + // Fallback for descendants spawned before the job assignment or when + // job creation failed. taskkill := taskkillPath() - _ = exec.Command(taskkill, "/T", "/F", "/PID", strconv.Itoa(cmd.Process.Pid)).Run() - _ = cmd.Process.Kill() + _ = exec.Command(taskkill, "/T", "/F", "/PID", strconv.Itoa(p.cmd.Process.Pid)).Run() + _ = p.cmd.Process.Kill() +} + +// Close releases the job handle; KILL_ON_JOB_CLOSE reaps any process still +// assigned to the job. +func (p *commandProcess) Close() { + if p.job == 0 { + return + } + _ = windows.CloseHandle(p.job) + p.job = 0 } func taskkillPath() string { diff --git a/internal/config/process_windows_test.go b/internal/config/process_windows_test.go new file mode 100644 index 000000000..658f3f2a5 --- /dev/null +++ b/internal/config/process_windows_test.go @@ -0,0 +1,20 @@ +//go:build windows + +package config + +import "golang.org/x/sys/windows" + +func processAlive(pid int) bool { + handle, err := windows.OpenProcess(windows.PROCESS_QUERY_LIMITED_INFORMATION, false, uint32(pid)) + if err != nil { + return false + } + defer func() { _ = windows.CloseHandle(handle) }() + + var code uint32 + if err := windows.GetExitCodeProcess(handle, &code); err != nil { + return false + } + const stillActive = 259 // STATUS_PENDING + return code == stillActive +} From c847946aa618a2d1446c48b14f1822aec293ac9b Mon Sep 17 00:00:00 2001 From: PierrunoYT Date: Wed, 15 Jul 2026 04:07:46 +0200 Subject: [PATCH 2/3] 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 --- internal/config/command.go | 8 ++++ internal/config/command_test.go | 60 ++++++++++++++++++++++++- internal/config/process_windows.go | 54 +++++++++++++++------- internal/config/process_windows_test.go | 2 +- 4 files changed, 105 insertions(+), 19 deletions(-) diff --git a/internal/config/command.go b/internal/config/command.go index 4422d01b5..2f01c2d83 100644 --- a/internal/config/command.go +++ b/internal/config/command.go @@ -69,6 +69,14 @@ func runProviderCommand(command string, timeout time.Duration) ([]byte, []byte, select { case err := <-done: + if errors.Is(err, exec.ErrWaitDelay) { + // The process exited but a background descendant kept the + // inherited stdout/stderr pipes open past WaitDelay. Treat this + // like a timeout so the tree is actually terminated instead of + // leaking that descendant. + proc.Terminate() + return stdout.Bytes(), stderr.Bytes(), errProviderCommandTimeout + } return stdout.Bytes(), stderr.Bytes(), err case <-timer.C: proc.Terminate() diff --git a/internal/config/command_test.go b/internal/config/command_test.go index d201c811b..b8113c39f 100644 --- a/internal/config/command_test.go +++ b/internal/config/command_test.go @@ -90,6 +90,34 @@ func TestLoadProviderCommandTimeout(t *testing.T) { assertProcessTerminated(t, pidFile) } +// TestLoadProviderCommandTerminatesBackgroundChild covers a command that +// exits immediately but leaves a detached child holding the inherited +// stdout/stderr pipes open (e.g. `sleep 600 & exit`). cmd.Wait() only +// unblocks once WaitDelay elapses, and that path must still terminate the +// leftover child instead of just returning and leaking it. +func TestLoadProviderCommandTerminatesBackgroundChild(t *testing.T) { + pidFile := filepath.Join(t.TempDir(), "bg.pid") + command := writeCommand(t, commandScript{ + Stdout: `{"name":"cmd","provider":"openai","apiKey":"sk-command","model":"gpt-command"}`, + BackgroundSleepSeconds: 10, + BackgroundPidFile: pidFile, + }) + + start := time.Now() + _, err := LoadProviderCommand(command) + elapsed := time.Since(start) + if err == nil { + t.Fatal("LoadProviderCommand() error = nil, want error from WaitDelay-bounded wait") + } + if !strings.Contains(err.Error(), "timed out after 5s") { + t.Fatalf("error = %q, want timeout", err.Error()) + } + if elapsed > 4*time.Second { + t.Fatalf("returned after %s, want well under the 5s provider-command timeout since WaitDelay (1s) should trigger termination", elapsed) + } + assertProcessTerminated(t, pidFile) +} + func assertProcessTerminated(t *testing.T, pidFile string) { t.Helper() @@ -144,6 +172,13 @@ type commandScript struct { ExitCode int SleepSeconds int PidFile string + + // BackgroundSleepSeconds, if set, spawns a detached child that keeps the + // inherited stdout/stderr handles open well after the script itself + // exits, simulating a `sleep 600 & exit` style command. BackgroundPidFile + // records the detached child's PID. + BackgroundSleepSeconds int + BackgroundPidFile string } func writeCommand(t *testing.T, script commandScript) string { @@ -156,7 +191,7 @@ func writeCommand(t *testing.T, script commandScript) string { if script.SleepSeconds > 0 { sleep := "Start-Sleep -Seconds " + itoa(script.SleepSeconds) if script.PidFile != "" { - sleep = "Set-Content -Path '" + script.PidFile + "' -Value $PID -Encoding Ascii; " + sleep + sleep = "Set-Content -Path '" + psSingleQuote(script.PidFile) + "' -Value $PID -Encoding Ascii; " + sleep } lines = append(lines, "powershell -NoProfile -Command \""+sleep+"\"") } @@ -166,6 +201,10 @@ func writeCommand(t *testing.T, script commandScript) string { if script.Stderr != "" { lines = append(lines, "echo "+script.Stderr+" 1>&2") } + if script.BackgroundSleepSeconds > 0 { + bgSleep := "Set-Content -Path '" + psSingleQuote(script.BackgroundPidFile) + "' -Value $PID -Encoding Ascii; Start-Sleep -Seconds " + itoa(script.BackgroundSleepSeconds) + lines = append(lines, "start /B powershell -NoProfile -Command \""+bgSleep+"\"") + } lines = append(lines, "exit /b "+itoa(script.ExitCode)) if err := os.WriteFile(path, []byte(strings.Join(lines, "\r\n")), 0o700); err != nil { t.Fatalf("write command: %v", err) @@ -177,7 +216,7 @@ func writeCommand(t *testing.T, script commandScript) string { lines := []string{"#!/bin/sh"} if script.SleepSeconds > 0 { if script.PidFile != "" { - lines = append(lines, "echo $$ > '"+script.PidFile+"'") + lines = append(lines, "echo $$ > '"+shSingleQuote(script.PidFile)+"'") } lines = append(lines, "sleep "+itoa(script.SleepSeconds)) } @@ -187,6 +226,10 @@ func writeCommand(t *testing.T, script commandScript) string { if script.Stderr != "" { lines = append(lines, "printf '%s\\n' '"+script.Stderr+"' >&2") } + if script.BackgroundSleepSeconds > 0 { + lines = append(lines, "sleep "+itoa(script.BackgroundSleepSeconds)+" &") + lines = append(lines, "echo $! > '"+shSingleQuote(script.BackgroundPidFile)+"'") + } lines = append(lines, "exit "+itoa(script.ExitCode)) if err := os.WriteFile(path, []byte(strings.Join(lines, "\n")), 0o700); err != nil { t.Fatalf("write command: %v", err) @@ -194,6 +237,19 @@ func writeCommand(t *testing.T, script commandScript) string { return path } +// psSingleQuote escapes a value for interpolation inside a PowerShell +// single-quoted string literal, where a literal quote is written as ''. +func psSingleQuote(value string) string { + return strings.ReplaceAll(value, "'", "''") +} + +// shSingleQuote escapes a value for interpolation inside a POSIX shell +// single-quoted string literal, where a literal quote must close the +// quoted section, emit an escaped quote, then reopen it. +func shSingleQuote(value string) string { + return strings.ReplaceAll(value, "'", `'\''`) +} + func itoa(value int) string { if value == 0 { return "0" diff --git a/internal/config/process_windows.go b/internal/config/process_windows.go index 49d193b43..f9a13547b 100644 --- a/internal/config/process_windows.go +++ b/internal/config/process_windows.go @@ -7,12 +7,20 @@ import ( "os/exec" "path/filepath" "strconv" + "syscall" "unsafe" "golang.org/x/sys/windows" ) -func configureCommandProcess(cmd *exec.Cmd) {} +// configureCommandProcess starts the process suspended so it can be +// assigned to the job object before its main thread (and therefore any +// code it runs) executes. Without this, a fast command can spawn and +// detach a descendant before AssignProcessToJobObject runs, letting that +// descendant escape the job and survive termination. +func configureCommandProcess(cmd *exec.Cmd) { + cmd.SysProcAttr = &syscall.SysProcAttr{CreationFlags: windows.CREATE_SUSPENDED} +} // commandProcess tracks a started provider command so its entire process // tree can be terminated on timeout. It prefers a job object: taskkill /T @@ -28,7 +36,11 @@ func attachCommandProcess(cmd *exec.Cmd) *commandProcess { if cmd.Process == nil { return proc } - job, err := createKillOnCloseJob() + // The main thread is suspended (see configureCommandProcess); resume it + // once we're done attaching, however that turns out. + defer resumeMainThread(cmd.Process.Pid) + + job, err := windows.CreateJobObject(nil, nil) if err != nil { return proc } @@ -46,21 +58,28 @@ func attachCommandProcess(cmd *exec.Cmd) *commandProcess { return proc } -func createKillOnCloseJob() (windows.Handle, error) { - job, err := windows.CreateJobObject(nil, nil) +// resumeMainThread resumes the (assumed suspended) primary thread of pid. +// It's a no-op if the thread can't be found or is already running. +func resumeMainThread(pid int) { + snapshot, err := windows.CreateToolhelp32Snapshot(windows.TH32CS_SNAPTHREAD, 0) if err != nil { - return 0, err - } - info := windows.JOBOBJECT_EXTENDED_LIMIT_INFORMATION{ - BasicLimitInformation: windows.JOBOBJECT_BASIC_LIMIT_INFORMATION{ - LimitFlags: windows.JOB_OBJECT_LIMIT_KILL_ON_JOB_CLOSE, - }, + return } - if _, err := windows.SetInformationJobObject(job, windows.JobObjectExtendedLimitInformation, uintptr(unsafe.Pointer(&info)), uint32(unsafe.Sizeof(info))); err != nil { - _ = windows.CloseHandle(job) - return 0, err + defer func() { _ = windows.CloseHandle(snapshot) }() + + var entry windows.ThreadEntry32 + entry.Size = uint32(unsafe.Sizeof(entry)) + for err := windows.Thread32First(snapshot, &entry); err == nil; err = windows.Thread32Next(snapshot, &entry) { + if entry.OwnerProcessID != uint32(pid) { + continue + } + thread, err := windows.OpenThread(windows.THREAD_SUSPEND_RESUME, false, entry.ThreadID) + if err != nil { + continue + } + _, _ = windows.ResumeThread(thread) + _ = windows.CloseHandle(thread) } - return job, nil } func (p *commandProcess) Terminate() { @@ -77,8 +96,11 @@ func (p *commandProcess) Terminate() { _ = p.cmd.Process.Kill() } -// Close releases the job handle; KILL_ON_JOB_CLOSE reaps any process still -// assigned to the job. +// Close releases the job handle without touching any still-running +// descendants: the job carries no KILL_ON_JOB_CLOSE limit, so on the +// success path a provider command's detached helpers keep running exactly +// as they did before job objects were introduced. Descendant termination +// happens explicitly via Terminate, called only on timeout/error. func (p *commandProcess) Close() { if p.job == 0 { return diff --git a/internal/config/process_windows_test.go b/internal/config/process_windows_test.go index 658f3f2a5..2b14a1f7c 100644 --- a/internal/config/process_windows_test.go +++ b/internal/config/process_windows_test.go @@ -15,6 +15,6 @@ func processAlive(pid int) bool { if err := windows.GetExitCodeProcess(handle, &code); err != nil { return false } - const stillActive = 259 // STATUS_PENDING + const stillActive = 259 // STILL_ACTIVE return code == stillActive } From f9da403be8ebb363540626254e51c2ba1cb2f727 Mon Sep 17 00:00:00 2001 From: PierrunoYT Date: Wed, 15 Jul 2026 04:16:04 +0200 Subject: [PATCH 3/3] 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. --- internal/config/command_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/config/command_test.go b/internal/config/command_test.go index b8113c39f..3ddbab695 100644 --- a/internal/config/command_test.go +++ b/internal/config/command_test.go @@ -238,7 +238,7 @@ func writeCommand(t *testing.T, script commandScript) string { } // psSingleQuote escapes a value for interpolation inside a PowerShell -// single-quoted string literal, where a literal quote is written as ''. +// single-quoted string literal, where a literal quote is doubled. func psSingleQuote(value string) string { return strings.ReplaceAll(value, "'", "''") }