fix(llm): unblock subprocess reads when a worker escapes the process group

[piekstra]

Problem

A cr review can wedge indefinitely at 0% CPU (holding the ledger) partway through. It reproduces reliably on slower (Opus) reviews and not on fast ones.

Root cause

When a reviewer task hits its per-task timeout, its context is canceled and cmd.Cancel SIGKILLs the claude child's process group. But claude's node workers make themselves process-group leaders (pgid == pid), so they escape the group kill and survive, holding the subprocess stdout/stderr pipes open.

LaunchProcess consumers then read stdout synchronously (scanStdout / io.ReadAll) before cmd.Wait() — deliberately, so no output is lost. With the pipe held open by the escaped worker, that read never sees EOF, so cmd.Wait() is never reached and the run hangs forever (until some external timeout kills it). Fast reviews that finish before the per-task timeout never trigger the cancel path, so they're unaffected.

Fix

In LaunchProcess (shared by every subprocess adapter), once the process context ends (task timeout, abort, or normal completion), give the pipes a short grace to EOF cleanly and then force stdout/stderr closed, so a pending synchronous read unblocks and the task fails cleanly instead of wedging. Also set cmd.WaitDelay as a backstop for cmd.Wait itself.

The synchronous stdout drain is unchanged, so no output is lost on the happy path (the force-close only fires after the context ends).

Verification

• New regression test (TestLaunchProcessUnblocksReadOnCanceledContext): spawns a worker that os.setsid()s out of the group and holds stdout, then cancels the context. It times out (wedges) without this change and passes with it.
• go test ./internal/llm/ ./internal/llmadapters/, make lint, make build all green.

[piekstra-dev]

Automated PR Review

Reviewed commit: 042304242e4e
Profile: reviewer - Posting as: piekstra-dev

Summary

Reviewer	Findings
go:implementation-tests	1
structure:repo-health	1

go:implementation-tests (1 finding)

Nits - internal/llm/subprocess_launch_unix_test.go:35

The Python worker's time.sleep(30) far outlasts the 5-second test deadline. Because the worker has called os.setsid() it escapes the process-group kill and becomes an orphan reparented to init; it will run for up to 30 seconds after the test exits. Across repeated or parallel test runs this accumulates stray Python processes. A 3–5 second sleep is enough to hold the pipe open through the 5-second test timeout and the 100 ms grace period without leaving long-lived orphans. Change the script to time.sleep(5) (or similar) to keep the scenario valid while bounding the orphan lifetime.

structure:repo-health (1 finding)

Minor - internal/llm/subprocess.go:120

The goroutine that force-closes stdout/stderr fires 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 sleeps subprocessWaitDelay (10 s) and closes pipes the process already closed. Every subprocess invocation adds a 10-second goroutine tail, and at the same moment cmd.WaitDelay (set to the same value) triggers Go's exec internals to also close those pipes — so both paths race to close identical *os.File descriptors. Individual Close() 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.WaitDelay and the goroutine sleep also share one variable (subprocessWaitDelay) for two semantically distinct timeouts: the exec-internal backstop for cmd.Wait vs. the grace period before a consumer-side io.ReadAll is unblocked. Coupling them prevents tuning either independently.

Concrete fix: Introduce a pipesDrained channel that the consumer closes after its read loop exits, and gate the force-close goroutine on that signal:

pipesDrained := make(chan struct{})
go func() {
    select {
    case <-pipesDrained:
        // consumer finished normally; nothing to force-close
    case <-procCtx.Done():
        time.Sleep(subprocessWaitDelay)
        _ = stdout.Close()
        _ = stderr.Close()
    }
}()

Return pipesDrained from LaunchProcess so 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. Split subprocessWaitDelay into subprocessPipeGrace (goroutine) and subprocessWaitDelay (cmd.WaitDelay) if different tuning is ever needed.

Reviewer Coverage

Reviewer	Status	Inspected	Skipped	Constraints
go:implementation-tests	complete_broad	internal/llm/subprocess.go, internal/llm/subprocess_launch_unix_test.go	unavailable	unavailable
structure:repo-health	complete_broad	internal/llm/subprocess.go	unavailable	unavailable

0 PR discussion threads considered. 0 summarized; 0 resolved.

---

Completed in 4m 32s | ~$0.67 (est.) | claude-sonnet-4-6 | cr 0.10.255

Field	Value
Model	claude-sonnet-4-6
Reviewers	go:implementation-tests, structure:repo-health
Engine	claude_cli · claude-sonnet-4-6
Reviewed by	cr · piekstra-dev
Duration	4m 32s wall · 7m 11s compute
Cost	~$0.67 (est.)
Tokens	25 in / 19.9k out

Per-workstream usage

Workstream	Model	In	Out	Cache read	Cache create	Cost	Duration
orchestrator-selection	claude-sonnet-4-6	7	755	25.3k	9.4k	~$0.05 (est.)	27s
go:implementation-tests	claude-sonnet-4-6	6	11.6k	51.6k	31.8k	~$0.31 (est.)	3m 38s
structure:repo-health	claude-sonnet-4-6	7	7.1k	75.7k	27.0k	~$0.23 (est.)	2m 25s
orchestrator-rollup	claude-sonnet-4-6	5	456	40.1k	16.6k	~$0.08 (est.)	41s

[piekstra-dev]

The Python worker's time.sleep(30) far outlasts the 5-second test deadline. Because the worker has called os.setsid() it escapes the process-group kill and becomes an orphan reparented to init; it will run for up to 30 seconds after the test exits. Across repeated or parallel test runs this accumulates stray Python processes. A 3–5 second sleep is enough to hold the pipe open through the 5-second test timeout and the 100 ms grace period without leaving long-lived orphans. Change the script to time.sleep(5) (or similar) to keep the scenario valid while bounding the orphan lifetime.

Reply inline to this comment.

[piekstra-dev]

The goroutine that force-closes stdout/stderr fires 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 sleeps subprocessWaitDelay (10 s) and closes pipes the process already closed. Every subprocess invocation adds a 10-second goroutine tail, and at the same moment cmd.WaitDelay (set to the same value) triggers Go's exec internals to also close those pipes — so both paths race to close identical *os.File descriptors. Individual Close() 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.WaitDelay and the goroutine sleep also share one variable (subprocessWaitDelay) for two semantically distinct timeouts: the exec-internal backstop for cmd.Wait vs. the grace period before a consumer-side io.ReadAll is unblocked. Coupling them prevents tuning either independently.

Concrete fix: Introduce a pipesDrained channel that the consumer closes after its read loop exits, and gate the force-close goroutine on that signal:

pipesDrained := make(chan struct{})
go func() {
    select {
    case <-pipesDrained:
        // consumer finished normally; nothing to force-close
    case <-procCtx.Done():
        time.Sleep(subprocessWaitDelay)
        _ = stdout.Close()
        _ = stderr.Close()
    }
}()

Return pipesDrained from LaunchProcess so 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. Split subprocessWaitDelay into subprocessPipeGrace (goroutine) and subprocessWaitDelay (cmd.WaitDelay) if different tuning is ever needed.

Reply inline to this comment.