-
Notifications
You must be signed in to change notification settings - Fork 0
fix(llm): unblock subprocess reads when a worker escapes the process group #508
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,62 @@ | ||
| //go:build !windows | ||
|
|
||
| package llm | ||
|
|
||
| import ( | ||
| "bufio" | ||
| "context" | ||
| "io" | ||
| "os/exec" | ||
| "path/filepath" | ||
| "strings" | ||
| "testing" | ||
| "time" | ||
| ) | ||
|
|
||
| // TestLaunchProcessUnblocksReadOnCanceledContext reproduces the subprocess | ||
| // wedge: a worker that escapes the process group (becomes its own group leader) | ||
| // holds stdout open, so cmd.Cancel's group kill misses it and a consumer's | ||
| // synchronous read of Stdout() never sees EOF. LaunchProcess must force the | ||
| // pipes closed a grace after the context ends so the read unblocks. Without the | ||
| // on-cancel pipe close this test times out. | ||
| func TestLaunchProcessUnblocksReadOnCanceledContext(t *testing.T) { | ||
| if _, err := exec.LookPath("python3"); err != nil { | ||
| t.Skip("python3 required to spawn a process-group-escaping worker") | ||
| } | ||
| prev := subprocessWaitDelay | ||
| subprocessWaitDelay = 100 * time.Millisecond | ||
| defer func() { subprocessWaitDelay = prev }() | ||
|
|
||
| ctx, cancel := context.WithCancel(context.Background()) | ||
| // The worker os.setsid()s out of the process group, prints READY on the | ||
| // inherited stdout, then holds it open. Reading READY proves it has escaped | ||
| // and owns the pipe. The parent sleeps so it (and the group kill) can't | ||
| // close stdout on its own. | ||
| script := `python3 -c 'import os,sys,time; os.setsid(); print("READY"); sys.stdout.flush(); time.sleep(30)' & sleep 20` | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The Python worker's Reply inline to this comment. |
||
| logPath := filepath.Join(t.TempDir(), "subprocess.log") | ||
| p, err := LaunchProcess(ctx, "/bin/sh", []string{"-c", script}, "", nil, 0, logPath, func() error { return nil }, false) | ||
| if err != nil { | ||
| t.Fatalf("LaunchProcess: %v", err) | ||
| } | ||
|
|
||
| rd := bufio.NewReader(p.Stdout()) | ||
| line, err := rd.ReadString('\n') | ||
| if err != nil || !strings.Contains(line, "READY") { | ||
| t.Fatalf("worker did not signal READY: %q err=%v", line, err) | ||
| } | ||
|
|
||
| cancel() // simulate task-timeout / abort; the escaped worker still holds stdout | ||
|
|
||
| done := make(chan struct{}) | ||
| go func() { | ||
| _, _ = io.ReadAll(rd) | ||
| close(done) | ||
| }() | ||
| select { | ||
| case <-done: | ||
| // unblocked as expected | ||
| case <-time.After(5 * time.Second): | ||
| t.Fatal("stdout read wedged after context cancel: pipes were not force-closed") | ||
| } | ||
| _ = p.Command().Wait() | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The goroutine that force-closes
stdout/stderrfires on every context end — including normal successful completion — not only the stuck-pipe scenario it was added for.When a task finishes cleanly the adapter eventually calls the derived
cancel(), which immediately satisfies<-procCtx.Done(). The goroutine then sleepssubprocessWaitDelay(10 s) and closes pipes the process already closed. Every subprocess invocation adds a 10-second goroutine tail, and at the same momentcmd.WaitDelay(set to the same value) triggers Go's exec internals to also close those pipes — so both paths race to close identical*os.Filedescriptors. IndividualClose()errors are suppressed, so there is no crash, but concurrent double-close on the same fd is confusing to future readers, and the always-on goroutine pattern will compound as more subprocess adapter types are added.cmd.WaitDelayand the goroutine sleep also share one variable (subprocessWaitDelay) for two semantically distinct timeouts: the exec-internal backstop forcmd.Waitvs. the grace period before a consumer-sideio.ReadAllis unblocked. Coupling them prevents tuning either independently.Concrete fix: Introduce a
pipesDrainedchannel that the consumer closes after its read loop exits, and gate the force-close goroutine on that signal:Return
pipesDrainedfromLaunchProcessso the adapter can close it after its scan loop. This eliminates the 10-second goroutine tail on the happy path and makes the guard conditional rather than always-on. SplitsubprocessWaitDelayintosubprocessPipeGrace(goroutine) andsubprocessWaitDelay(cmd.WaitDelay) if different tuning is ever needed.Reply inline to this comment.