Skip to content

Commit 908f1d7

Browse files
andyrewleeclaude
andyrewlee
and
claude
committed
process: add workspace validation, normalized keys, and benign stop error handling
ScriptRunner previously used raw ws.Root as map keys (breaking with symlinks), didn't validate workspace inputs, silently ignored stop failures in nonconcurrent mode, and didn't handle process-already-exited races. Normalize workspace keys via data.NormalizePath, validate workspace fields on all public methods, handle benign stop errors (ESRCH, ECHILD, os.ErrProcessDone) gracefully, and surface non-benign stop failures in nonconcurrent mode instead of silently swallowing them. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
1 parent 5199e2c commit 908f1d7

5 files changed

Lines changed: 237 additions & 8 deletions

File tree

‎internal/process/scripts.go‎

Lines changed: 73 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -3,16 +3,20 @@ package process
33
import (
44
"bytes"
55
"encoding/json"
6+
"errors"
67
"fmt"
78
"os"
89
"os/exec"
910
"path/filepath"
11+
"strings"
1012
"sync"
1113

1214
"github.com/andyrewlee/amux/internal/data"
1315
"github.com/andyrewlee/amux/internal/safego"
1416
)
1517

18+
var killProcessGroupFn = KillProcessGroup
19+
1620
// ScriptType identifies the type of script
1721
type ScriptType string
1822

@@ -24,6 +28,44 @@ const (
2428

2529
const configFilename = "workspaces.json"
2630

31+
func scriptWorkspaceKey(ws *data.Workspace) string {
32+
return data.NormalizePath(ws.Root)
33+
}
34+
35+
func validateScriptWorkspace(ws *data.Workspace) error {
36+
if ws == nil {
37+
return errors.New("workspace is required")
38+
}
39+
if strings.TrimSpace(ws.Repo) == "" {
40+
return errors.New("workspace repo is required")
41+
}
42+
if strings.TrimSpace(ws.Root) == "" {
43+
return errors.New("workspace root is required")
44+
}
45+
return nil
46+
}
47+
48+
func isBenignStopError(err error) bool {
49+
if err == nil {
50+
return true
51+
}
52+
if errors.Is(err, os.ErrProcessDone) {
53+
return true
54+
}
55+
if isTypedProcessGoneError(err) {
56+
return true
57+
}
58+
msg := err.Error()
59+
return strings.Contains(msg, "process already finished") ||
60+
strings.Contains(msg, "no such process")
61+
}
62+
63+
func (r *ScriptRunner) clearRunningEntry(key string) {
64+
r.mu.Lock()
65+
delete(r.running, key)
66+
r.mu.Unlock()
67+
}
68+
2769
// WorkspaceConfig holds per-project workspace configuration
2870
type WorkspaceConfig struct {
2971
SetupWorkspace []string `json:"setup-workspace"`
@@ -74,6 +116,9 @@ func (r *ScriptRunner) LoadConfig(repoPath string) (*WorkspaceConfig, error) {
74116

75117
// RunSetup runs the setup scripts for a workspace
76118
func (r *ScriptRunner) RunSetup(ws *data.Workspace) error {
119+
if err := validateScriptWorkspace(ws); err != nil {
120+
return err
121+
}
77122
config, err := r.LoadConfig(ws.Repo)
78123
if err != nil {
79124
return err
@@ -101,6 +146,10 @@ func (r *ScriptRunner) RunSetup(ws *data.Workspace) error {
101146

102147
// RunScript runs a script for a workspace
103148
func (r *ScriptRunner) RunScript(ws *data.Workspace, scriptType ScriptType) (*exec.Cmd, error) {
149+
if err := validateScriptWorkspace(ws); err != nil {
150+
return nil, err
151+
}
152+
104153
config, err := r.LoadConfig(ws.Repo)
105154
if err != nil {
106155
return nil, err
@@ -126,7 +175,9 @@ func (r *ScriptRunner) RunScript(ws *data.Workspace, scriptType ScriptType) (*ex
126175

127176
// Check for existing process in non-concurrent mode
128177
if ws.ScriptMode == "nonconcurrent" {
129-
_ = r.Stop(ws)
178+
if err := r.Stop(ws); !isBenignStopError(err) {
179+
return nil, err
180+
}
130181
}
131182

132183
env := r.envBuilder.BuildEnv(ws)
@@ -141,17 +192,17 @@ func (r *ScriptRunner) RunScript(ws *data.Workspace, scriptType ScriptType) (*ex
141192
}
142193

143194
running := &runningScript{cmd: cmd}
144-
root := ws.Root
195+
key := scriptWorkspaceKey(ws)
145196
r.mu.Lock()
146-
r.running[root] = running
197+
r.running[key] = running
147198
r.mu.Unlock()
148199

149200
// Monitor in background
150201
safego.Go("process.script_wait", func() {
151202
_ = cmd.Wait()
152203
r.mu.Lock()
153-
if current, ok := r.running[root]; ok && current == running {
154-
delete(r.running, root)
204+
if current, ok := r.running[key]; ok && current == running {
205+
delete(r.running, key)
155206
}
156207
r.mu.Unlock()
157208
})
@@ -161,26 +212,40 @@ func (r *ScriptRunner) RunScript(ws *data.Workspace, scriptType ScriptType) (*ex
161212

162213
// Stop stops the running script for a workspace
163214
func (r *ScriptRunner) Stop(ws *data.Workspace) error {
215+
if err := validateScriptWorkspace(ws); err != nil {
216+
return err
217+
}
218+
219+
key := scriptWorkspaceKey(ws)
164220
r.mu.Lock()
165-
running, ok := r.running[ws.Root]
221+
running, ok := r.running[key]
166222
r.mu.Unlock()
167223

168224
if !ok {
169225
return nil
170226
}
171227

172228
if running.cmd != nil && running.cmd.Process != nil {
173-
return KillProcessGroup(running.cmd.Process.Pid, KillOptions{})
229+
err := killProcessGroupFn(running.cmd.Process.Pid, KillOptions{})
230+
if isBenignStopError(err) {
231+
r.clearRunningEntry(key)
232+
return nil
233+
}
234+
return err
174235
}
175236

176237
return nil
177238
}
178239

179240
// IsRunning checks if a script is running for a workspace
180241
func (r *ScriptRunner) IsRunning(ws *data.Workspace) bool {
242+
if validateScriptWorkspace(ws) != nil {
243+
return false
244+
}
245+
key := scriptWorkspaceKey(ws)
181246
r.mu.Lock()
182247
defer r.mu.Unlock()
183-
_, ok := r.running[ws.Root]
248+
_, ok := r.running[key]
184249
return ok
185250
}
186251

‎internal/process/scripts_stop_error_unix_test.go‎

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,17 @@
1+
//go:build !windows
2+
3+
package process
4+
5+
import (
6+
"syscall"
7+
"testing"
8+
)
9+
10+
func TestIsBenignStopErrorRecognizesTypedProcessGone(t *testing.T) {
11+
if !isBenignStopError(syscall.ESRCH) {
12+
t.Fatal("ESRCH should be benign")
13+
}
14+
if !isBenignStopError(syscall.ECHILD) {
15+
t.Fatal("ECHILD should be benign")
16+
}
17+
}

‎internal/process/scripts_test.go‎

Lines changed: 127 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
package process
22

33
import (
4+
"errors"
45
"os"
56
"path/filepath"
67
"strings"
@@ -221,6 +222,132 @@ func TestScriptRunnerStop(t *testing.T) {
221222
}
222223
}
223224

225+
func TestScriptRunnerWorkspaceValidation(t *testing.T) {
226+
runner := NewScriptRunner(6200, 10)
227+
228+
// nil workspace
229+
if err := runner.RunSetup(nil); err == nil {
230+
t.Fatal("expected error for nil workspace")
231+
}
232+
if _, err := runner.RunScript(nil, ScriptRun); err == nil {
233+
t.Fatal("expected error for nil workspace")
234+
}
235+
if err := runner.Stop(nil); err == nil {
236+
t.Fatal("expected error for nil workspace")
237+
}
238+
if runner.IsRunning(nil) {
239+
t.Fatal("expected false for nil workspace")
240+
}
241+
242+
// empty repo
243+
ws := &data.Workspace{Repo: "", Root: "/some/root"}
244+
if err := runner.RunSetup(ws); err == nil {
245+
t.Fatal("expected error for empty repo")
246+
}
247+
248+
// empty root
249+
ws = &data.Workspace{Repo: "/some/repo", Root: ""}
250+
if err := runner.RunSetup(ws); err == nil {
251+
t.Fatal("expected error for empty root")
252+
}
253+
}
254+
255+
func TestScriptRunnerUsesNormalizedWorkspaceKey(t *testing.T) {
256+
repo := t.TempDir()
257+
wsRoot := t.TempDir()
258+
259+
writeWorkspaceConfig(t, repo, `{"run": "sleep 5"}`)
260+
261+
runner := NewScriptRunner(6200, 10)
262+
ws1 := &data.Workspace{Repo: repo, Root: wsRoot}
263+
264+
if _, err := runner.RunScript(ws1, ScriptRun); err != nil {
265+
t.Fatalf("RunScript() error = %v", err)
266+
}
267+
268+
// Check via a workspace with an equivalent but non-identical path
269+
// (trailing slash variation, which filepath.Clean normalizes)
270+
ws2 := &data.Workspace{Repo: repo, Root: wsRoot + "/"}
271+
if !runner.IsRunning(ws2) {
272+
t.Fatal("expected script to be running via normalized key")
273+
}
274+
275+
// Clean up
276+
_ = runner.Stop(ws1)
277+
}
278+
279+
func TestScriptRunnerRunScriptNonconcurrentStopFailure(t *testing.T) {
280+
repo := t.TempDir()
281+
wsRoot := t.TempDir()
282+
283+
writeWorkspaceConfig(t, repo, `{"run": "sleep 5"}`)
284+
285+
runner := NewScriptRunner(6200, 10)
286+
ws := &data.Workspace{Repo: repo, Root: wsRoot, ScriptMode: "nonconcurrent"}
287+
288+
// Start a script first
289+
if _, err := runner.RunScript(ws, ScriptRun); err != nil {
290+
t.Fatalf("RunScript() error = %v", err)
291+
}
292+
time.Sleep(50 * time.Millisecond)
293+
294+
// Inject a non-benign stop error
295+
origKill := killProcessGroupFn
296+
t.Cleanup(func() { killProcessGroupFn = origKill })
297+
killProcessGroupFn = func(pid int, opts KillOptions) error {
298+
return errors.New("permission denied")
299+
}
300+
301+
// Second run in nonconcurrent mode should fail because stop fails
302+
_, err := runner.RunScript(ws, ScriptRun)
303+
if err == nil {
304+
t.Fatal("expected error from non-benign stop failure")
305+
}
306+
if !strings.Contains(err.Error(), "permission denied") {
307+
t.Fatalf("expected permission denied error, got: %v", err)
308+
}
309+
310+
// Clean up with original kill
311+
killProcessGroupFn = origKill
312+
_ = runner.Stop(ws)
313+
}
314+
315+
func TestScriptRunnerRunScriptNonconcurrentIgnoresBenignStopRace(t *testing.T) {
316+
repo := t.TempDir()
317+
wsRoot := t.TempDir()
318+
319+
writeWorkspaceConfig(t, repo, `{"run": "sleep 5"}`)
320+
321+
runner := NewScriptRunner(6200, 10)
322+
ws := &data.Workspace{Repo: repo, Root: wsRoot, ScriptMode: "nonconcurrent"}
323+
324+
// Start a script first
325+
if _, err := runner.RunScript(ws, ScriptRun); err != nil {
326+
t.Fatalf("RunScript() error = %v", err)
327+
}
328+
time.Sleep(50 * time.Millisecond)
329+
330+
// Inject a benign "process already finished" error
331+
origKill := killProcessGroupFn
332+
t.Cleanup(func() { killProcessGroupFn = origKill })
333+
killProcessGroupFn = func(pid int, opts KillOptions) error {
334+
return errors.New("process already finished")
335+
}
336+
337+
// Second run should succeed because benign stop errors are ignored
338+
cmd, err := runner.RunScript(ws, ScriptRun)
339+
if err != nil {
340+
t.Fatalf("expected success, got error: %v", err)
341+
}
342+
if cmd == nil {
343+
t.Fatal("expected non-nil cmd")
344+
}
345+
346+
// Clean up
347+
killProcessGroupFn = origKill
348+
_ = runner.Stop(ws)
349+
}
350+
224351
func waitForFile(path string, timeout time.Duration) error {
225352
deadline := time.After(timeout)
226353
for {

‎internal/process/stop_errors_unix.go‎

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,12 @@
1+
//go:build !windows
2+
3+
package process
4+
5+
import (
6+
"errors"
7+
"syscall"
8+
)
9+
10+
func isTypedProcessGoneError(err error) bool {
11+
return errors.Is(err, syscall.ESRCH) || errors.Is(err, syscall.ECHILD)
12+
}

‎internal/process/stop_errors_windows.go‎

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,8 @@
1+
//go:build windows
2+
3+
package process
4+
5+
func isTypedProcessGoneError(err error) bool {
6+
_ = err
7+
return false
8+
}

0 commit comments

Comments
 (0)