fix(llm): default per-task timeout for subprocess LLM adapters#509
Conversation
The claude/codex/pi adapters were constructed (app/runtime.go) without a Timeout, so every LLM task ran under a plain cancel context with no deadline. One hung CLI worker or a background job that never reaches a terminal state therefore hung the entire review indefinitely — observed repeatedly in production as reviews that write artifacts for a few minutes, then sit at 0% CPU forever until some external supervisor kills them, with no attributable error. Default SubprocessOptions.Timeout (and PiRPCOptions.Timeout) to 10 minutes when unset. Healthy reviewer tasks finish in a few minutes, so this converts "one worker hung" into a timely, clearly-attributed task failure that composes with the pipe-teardown fix (#508): deadline fires -> context ends -> pipes force-closed -> the task fails cleanly instead of wedging. Negative values opt out explicitly; explicit values are respected unchanged.
|
Primary evidence from a live production hang (captured while this PR was open — the direct review of this very PR hung, and a SIGQUIT goroutine dump names the exact path this PR bounds): A claude background job never reached a terminal state, and |
piekstra-dev
left a comment
There was a problem hiding this comment.
Automated PR Review
Reviewed commit: 0a35c092a6fb
Profile: reviewer - Posting as: piekstra-dev
Summary
| Reviewer | Findings |
|---|---|
| go:implementation-tests | 1 |
| structure:repo-health | 0 |
go:implementation-tests (1 finding)
Nits - internal/llmadapters/pi_rpc.go:52
PiRPCOptions.Timeoutis the only exported field inPiRPCOptionswhose zero/negative semantics differ from what a reader would assume (zero → 10-minute default, negative → no bound), yet the field has no doc comment while the equivalentSubprocessOptions.Timeoutfield carries a thorough explanation of both conventions. A caller reading onlyPiRPCOptionshas no in-source signal that passing-1opts out of the deadline or that omitting the field applies a default. Suggested fix: add the same explanatory comment toPiRPCOptions.Timeoutas exists onSubprocessOptions.Timeout, i.e. "Zero applies defaultLLMTaskTimeout; negative disables the bound explicitly." The implementation and tests are otherwise correct.
Reviewer Coverage
| Reviewer | Status | Inspected | Skipped | Constraints |
|---|---|---|---|---|
| go:implementation-tests | complete_broad | internal/llmadapters/pi_rpc.go, internal/llmadapters/subprocess.go, internal/llmadapters/subprocess_test.go | unavailable | unavailable |
| structure:repo-health | complete_broad | internal/llmadapters/pi_rpc.go, internal/llmadapters/subprocess.go | unavailable | unavailable |
0 PR discussion threads considered. 0 summarized; 0 resolved.
Completed in 3m 47s | ~$0.90 (est.) | claude-sonnet-4-6 | cr 0.10.256
| 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 | 3m 47s wall · 5m 34s compute |
| Cost | ~$0.90 (est.) |
| Tokens | 24 in / 13.7k out |
Per-workstream usage
| Workstream | Model | In | Out | Cache read | Cache create | Cost | Duration |
|---|---|---|---|---|---|---|---|
| orchestrator-selection | claude-sonnet-4-6 | 4 | 696 | 8.7k | 14.4k | ~$0.07 (est.) | 24s |
| go:implementation-tests | claude-sonnet-4-6 | 8 | 5.4k | 180.6k | 69.4k | ~$0.40 (est.) | 2m 13s |
| structure:repo-health | claude-sonnet-4-6 | 7 | 6.7k | 126.1k | 51.3k | ~$0.33 (est.) | 2m 31s |
| orchestrator-rollup | claude-sonnet-4-6 | 5 | 999 | 26.5k | 23.8k | ~$0.11 (est.) | 24s |
| @@ -52,11 +52,17 @@ func NewPiRPCAdapter(opts PiRPCOptions) *PiRPCAdapter { | |||
| if factory == nil { | |||
There was a problem hiding this comment.
File-level note: internal/llmadapters/pi_rpc.go
PiRPCOptions.Timeout is the only exported field in PiRPCOptions whose zero/negative semantics differ from what a reader would assume (zero → 10-minute default, negative → no bound), yet the field has no doc comment while the equivalent SubprocessOptions.Timeout field carries a thorough explanation of both conventions. A caller reading only PiRPCOptions has no in-source signal that passing -1 opts out of the deadline or that omitting the field applies a default. Suggested fix: add the same explanatory comment to PiRPCOptions.Timeout as exists on SubprocessOptions.Timeout, i.e. "Zero applies defaultLLMTaskTimeout; negative disables the bound explicitly." The implementation and tests are otherwise correct.
Reply inline to this comment.
Problem
The subprocess LLM adapters are constructed in
app/runtime.gowith noTimeout, so every LLM task runs undercontext.WithCancel— no deadline at all. One hung CLI worker (or a background job that never reaches a terminal state) hangs the entire review indefinitely: artifacts get written for a few minutes, then the run sits at 0% CPU forever with no attributable error, until some external supervisor kills it. Observed repeatedly in production today.Fix
Default
SubprocessOptions.Timeout/PiRPCOptions.Timeoutto 10 minutes when unset (healthy reviewer tasks finish in a few minutes; mirrorsdefaultAPIClientTimeout). Negative opts out explicitly; explicit values are unchanged.This composes with #508: the deadline ends the task context → the pipe-teardown fix closes the pipes → the task fails cleanly and quickly with a clear reason instead of wedging.
Verification
go test ./internal/llmadapters/ ./internal/llm/,make lint,go build ./...all green.