diff --git a/apps/openant-cli/internal/python/invoke.go b/apps/openant-cli/internal/python/invoke.go index d127e11c..14750fac 100644 --- a/apps/openant-cli/internal/python/invoke.go +++ b/apps/openant-cli/internal/python/invoke.go @@ -3,6 +3,7 @@ package python import ( "bufio" + "context" "encoding/json" "fmt" "io" @@ -16,6 +17,14 @@ import ( "github.com/knostic/open-ant-cli/internal/types" ) +// defaultInvokeTimeout bounds how long Invoke will wait on the Python +// subprocess before the process is killed and Invoke returns. It guards +// against a hung parser (infinite loop, I/O deadlock, pathological repo) +// wedging the CLI forever, which matters most for headless/automated +// callers that cannot deliver a manual Ctrl+C. It is a package var so +// tests can shrink it. +var defaultInvokeTimeout = 30 * time.Minute + // InvokeResult holds the result of a Python CLI invocation. type InvokeResult struct { Envelope types.Envelope @@ -30,7 +39,23 @@ type InvokeResult struct { // - If apiKey is non-empty, it is injected as ANTHROPIC_API_KEY in the subprocess func Invoke(pythonPath string, args []string, workDir string, quiet bool, apiKey string) (*InvokeResult, error) { cmdArgs := append([]string{"-m", "openant"}, args...) - cmd := exec.Command(pythonPath, cmdArgs...) + + // Bound the subprocess with an automatic deadline so a hung parser + // cannot wedge the CLI forever on cmd.Wait(). When the context expires + // CommandContext kills the process. This is the only recovery path for + // headless/automated callers, which never deliver the manual SIGINT the + // signal goroutine below relies on. Mirrors the pattern at cmd/docker.go. + ctx, cancel := context.WithTimeout(context.Background(), defaultInvokeTimeout) + defer cancel() + cmd := exec.CommandContext(ctx, pythonPath, cmdArgs...) + + // Killing the process is not sufficient on its own: a descendant the + // parser spawned can keep the stdout/stderr pipe write-ends open, leaving + // the io.Copy below blocked forever even after the parent is dead. + // WaitDelay tells os/exec to force-close those inherited pipe FDs shortly + // after the context is done, and the explicit read-end close in the + // watchdog goroutine (below) unblocks the in-flight reads. + cmd.WaitDelay = 5 * time.Second if workDir != "" { cmd.Dir = workDir @@ -60,6 +85,23 @@ func Invoke(pythonPath string, args []string, workDir string, quiet bool, apiKey return nil, fmt.Errorf("failed to start Python process: %w", err) } + // Watchdog: when the timeout (or any context cancellation) fires, close + // the pipe read-ends so io.Copy(stdout) and streamStderr(stderr) return + // promptly instead of blocking on a descendant that still holds the + // write-ends open. Without this, the deadline would kill the parser but + // the CLI would still hang in io.Copy. watchdogDone stops the goroutine + // on the normal (non-timeout) exit path. + watchdogDone := make(chan struct{}) + defer close(watchdogDone) + go func() { + select { + case <-ctx.Done(): + _ = stdout.Close() + _ = stderr.Close() + case <-watchdogDone: + } + }() + // Forward SIGINT/SIGTERM to the Python subprocess so Ctrl+C kills it. sigChan := make(chan os.Signal, 1) interrupted := false diff --git a/apps/openant-cli/internal/python/invoke_test.go b/apps/openant-cli/internal/python/invoke_test.go new file mode 100644 index 00000000..25433939 --- /dev/null +++ b/apps/openant-cli/internal/python/invoke_test.go @@ -0,0 +1,64 @@ +package python + +import ( + "os" + "path/filepath" + "runtime" + "testing" + "time" +) + +// writeHangScript creates an executable script that ignores its arguments, +// prints nothing on stdout, and sleeps far longer than any test deadline. +// It stands in for a hung Python parser (infinite loop / I/O deadlock). +func writeHangScript(t *testing.T) string { + t.Helper() + if runtime.GOOS == "windows" { + t.Skip("hang-subprocess test uses a POSIX shell script") + } + dir := t.TempDir() + path := filepath.Join(dir, "hang.sh") + // Sleep well past the test's deadline; never produces stdout. + script := "#!/bin/sh\nsleep 600\n" + if err := os.WriteFile(path, []byte(script), 0o755); err != nil { + t.Fatalf("failed to write hang script: %v", err) + } + return path +} + +// TestInvoke_HangingSubprocessIsBoundedByTimeout asserts that a hung Python +// subprocess must be bounded by an automatic timeout so Invoke returns +// instead of blocking forever on cmd.Wait(). +// +// Pre-fix invoke.go:33 uses exec.Command (no context, no deadline), so +// Invoke blocks on io.Copy/cmd.Wait for the full 600s sleep and this test +// hangs until `go test` kills it — i.e. it does NOT return within the +// bounded window. Post-fix (exec.CommandContext + a default timeout) the +// command is killed at the deadline and Invoke returns promptly. +func TestInvoke_HangingSubprocessIsBoundedByTimeout(t *testing.T) { + hang := writeHangScript(t) + + // Shrink the automatic deadline so the test is fast. The default is + // far larger; this knob is the wiring the fix must expose. + prev := defaultInvokeTimeout + defaultInvokeTimeout = 500 * time.Millisecond + t.Cleanup(func() { defaultInvokeTimeout = prev }) + + // Generous wall-clock budget: comfortably larger than the deadline but + // far smaller than the 600s the subprocess would otherwise sleep. + const budget = 10 * time.Second + + done := make(chan struct{}) + go func() { + defer close(done) + _, _ = Invoke(hang, []string{"parse", "."}, "", true, "") + }() + + select { + case <-done: + // Invoke returned within budget — the timeout bounded the hang. + case <-time.After(budget): + t.Fatalf("Invoke did not return within %v on a hung subprocess; "+ + "expected the automatic timeout (%v) to bound it", budget, defaultInvokeTimeout) + } +}