From dc07ea7fdc6de06981ec6b582071d96efc5499b3 Mon Sep 17 00:00:00 2001 From: piekstra Date: Mon, 31 Aug 2026 09:58:21 -0400 Subject: [PATCH 1/6] fix(llmadapters): fall back to foreground when a Claude background job fails The Claude job-service transport is the default for reviewer tasks, and this file already documents it as the fragile one: sessions die mid-turn without writing a state file, and a job can enter a terminal state having never run the task at all. Observed in production: a job reported `blocked` because the prompt file it was told to read was gone, on four consecutive runs of the same review, always taking one reviewer with it. Losing a reviewer is worse than it looks from inside the adapter. A review that reports no findings because a reviewer never ran is not the same as a review that found nothing, and a caller gating on coverage cannot tell them apart, so one flaky job service withholds approval on an unrelated change. Background failures that mean "the transport never ran the task" are now marked with ErrClaudeBGTransport, and such a task is retried once on the foreground transport, which this file already describes as fully cr-owned. A job that DID complete and wrote nothing usable is a model failure, is not marked, and is not retried, since retrying would only repeat it. When the retry fails too, the original transport failure is what surfaces: the retry failing says nothing new, and burying the cause under it is how a job-service problem gets read as a reviewer problem. CR_CLAUDE_FOREGROUND is unchanged and still selects foreground outright. --- internal/llmadapters/subprocess.go | 95 ++++++++++++++++++++++--- internal/llmadapters/subprocess_test.go | 61 ++++++++++++++++ 2 files changed, 145 insertions(+), 11 deletions(-) diff --git a/internal/llmadapters/subprocess.go b/internal/llmadapters/subprocess.go index eaa7cb6..34b823e 100644 --- a/internal/llmadapters/subprocess.go +++ b/internal/llmadapters/subprocess.go @@ -14,6 +14,7 @@ import ( "regexp" "strconv" "strings" + "sync" "time" "github.com/open-cli-collective/codereview-cli/internal/llm" @@ -68,6 +69,14 @@ const ( claudeBGStaleJobAge = 24 * time.Hour ) +// ErrClaudeBGTransport marks a failure of the Claude job-service transport +// itself: the job never ran the task to completion because it was blocked, +// stopped, failed to register, or lost the scratch it was told to read. It +// says nothing about the review, so the same request can be retried on +// another transport. A job that DID complete and wrote nothing usable is a +// model failure and is not marked, since retrying would only repeat it. +var ErrClaudeBGTransport = errors.New("llm subprocess: Claude background transport failed") + var ( claudeBGJobIDDirectRE = regexp.MustCompile(`backgrounded\s+.\s+([A-Za-z0-9_-]+)`) claudeBGJobIDAttachRE = regexp.MustCompile(`\bclaude\s+attach\s+([A-Za-z0-9_-]+)\b`) @@ -169,10 +178,7 @@ func (a *SubprocessAdapter) Start(ctx context.Context, req Request) (Stream, err return nil, err } if a.kind == subprocessClaude { - if claudeForegroundEnabled(a.env) { - return a.startClaudeForeground(ctx, req, "") - } - return a.startClaudeBG(ctx, req, "") + return a.startClaude(ctx, req, "") } if a.kind == subprocessCodex && !a.allowBestEffortNoTools { return nil, fmt.Errorf("%w: codex_cli requires AllowBestEffortNoTools until Codex exposes an all-tools-disabled flag", ErrUnsafeSubprocessConfig) @@ -344,6 +350,76 @@ func buildClaudeForegroundArgs(req Request, scratch string, resumeSessionID stri return append(args, "--", claudeBGPositionalPrompt(scratch)) } +// startClaude picks the transport for a Claude task. Background mode is the +// default and foreground is the documented-sturdier fallback, so a background +// job that fails without producing a result is retried once in foreground +// rather than failing the task. +// +// Without that retry, one flaky job service costs a whole reviewer: a review +// that reports no findings because a reviewer never ran is not the same as a +// review that found nothing, and callers that gate on coverage cannot tell +// them apart from the outside. +func (a *SubprocessAdapter) startClaude(ctx context.Context, req Request, resumeSessionID string) (Stream, error) { + if claudeForegroundEnabled(a.env) { + return a.startClaudeForeground(ctx, req, resumeSessionID) + } + bg, err := a.startClaudeBG(ctx, req, resumeSessionID) + if err != nil { + // The launch itself failed, so there is no job to wait on and the + // fallback is immediate. + return a.startClaudeForeground(ctx, req, resumeSessionID) + } + return &claudeFallbackStream{adapter: a, req: req, resume: resumeSessionID, primary: bg}, nil +} + +// claudeFallbackStream waits on the background job and, when the job service +// fails it, runs the same request again in foreground. +type claudeFallbackStream struct { + adapter *SubprocessAdapter + req Request + resume string + primary Stream + + mu sync.Mutex + fallback Stream +} + +func (s *claudeFallbackStream) SessionID() string { + s.mu.Lock() + fallback := s.fallback + s.mu.Unlock() + if fallback != nil { + if id := fallback.SessionID(); id != "" { + return id + } + } + return s.primary.SessionID() +} + +func (s *claudeFallbackStream) Wait(ctx context.Context) (Response, error) { + response, err := s.primary.Wait(ctx) + if err == nil || ctx.Err() != nil || !errors.Is(err, ErrClaudeBGTransport) { + return response, err + } + fallback, startErr := s.adapter.startClaudeForeground(ctx, s.req, s.resume) + if startErr != nil { + // The original failure is the one worth reporting: the fallback not + // starting is a second symptom, not the cause. + return response, err + } + s.mu.Lock() + s.fallback = fallback + s.mu.Unlock() + fallbackResponse, fallbackErr := fallback.Wait(ctx) + if fallbackErr != nil { + // Report what actually went wrong first. The retry failing too says + // nothing new, and burying the transport failure under it is how a + // job-service problem gets read as a reviewer problem. + return response, err + } + return fallbackResponse, nil +} + // startClaudeForeground runs a Claude task as a plain foreground child in // headless print mode. Same scratch, prompt-file, env, and result-file // contract as background mode — only the transport differs. @@ -587,10 +663,7 @@ func (a *SubprocessAdapter) Resume(ctx context.Context, sessionID string, req Re // through optional resume state without special-casing the first run. return a.Start(ctx, req) } - if claudeForegroundEnabled(a.env) { - return a.startClaudeForeground(ctx, req, sessionID) - } - return a.startClaudeBG(ctx, req, sessionID) + return a.startClaude(ctx, req, sessionID) case subprocessCodex: if !a.allowBestEffortNoTools { return nil, fmt.Errorf("%w: codex_cli requires AllowBestEffortNoTools until Codex exposes an all-tools-disabled flag", ErrUnsafeSubprocessConfig) @@ -950,7 +1023,7 @@ func (s *subprocessStream) runClaudeBG(ctx context.Context, adapter *SubprocessA } else { jobID = extractClaudeBGJobID(string(launchStdout)) if jobID == "" { - result.err = errors.New("llm subprocess: could not parse Claude background job id") + result.err = fmt.Errorf("%w: could not parse Claude background job id", ErrClaudeBGTransport) } } @@ -1317,7 +1390,7 @@ func (a *SubprocessAdapter) waitForClaudeBGResult(ctx context.Context, jobID str sessionID, state = a.waitForClaudeBGSessionID(ctx, jobID, state) } if sessionID == "" { - return Response{}, "", fmt.Errorf("llm subprocess: Claude background job completed without session id: %s", claudeBGStateDetail(state)) + return Response{}, "", fmt.Errorf("%w: job completed without session id: %s", ErrClaudeBGTransport, claudeBGStateDetail(state)) } output, err := readFirstNonEmptyFile(resultPaths) if err != nil { @@ -1348,7 +1421,7 @@ func (a *SubprocessAdapter) waitForClaudeBGState(ctx context.Context, jobID stri return state, nil } detail := claudeBGStateDetail(state) - jobErr := fmt.Errorf("llm subprocess: Claude background job %s: %s", stateName, detail) + jobErr := fmt.Errorf("%w: job %s: %s", ErrClaudeBGTransport, stateName, detail) return state, classifyCLIDetail(jobErr, detail) } } diff --git a/internal/llmadapters/subprocess_test.go b/internal/llmadapters/subprocess_test.go index ccb38f6..4686027 100644 --- a/internal/llmadapters/subprocess_test.go +++ b/internal/llmadapters/subprocess_test.go @@ -1635,6 +1635,9 @@ func runClaudeBGHelper(mode string, args []string) { case "bg-blocked": state = map[string]any{"state": "blocked", "detail": "permission needed"} writeResult = false + case "bg-blocked-foreground-recovers": + state = map[string]any{"state": "blocked", "detail": "prompt file no longer exists"} + writeResult = false case "bg-failed": state = map[string]any{"state": "failed", "error": "model failed"} writeResult = false @@ -1969,6 +1972,11 @@ func runClaudeForegroundHelper(mode string, args []string) { _ = os.WriteFile(filepath.Join(scratch, claudeBGResultFilename), []byte(`{"ok":true}`), 0o600) } fmt.Println(`{"type":"result","subtype":"success","is_error":false,"result":"wrote the result file","session_id":"fg-session-1","total_cost_usd":0.1234,"usage":{"input_tokens":11,"output_tokens":22,"cache_read_input_tokens":33,"cache_creation_input_tokens":44,"speed":"standard"}}`) + case "bg-blocked-foreground-recovers": + if scratch != "" { + _ = os.WriteFile(filepath.Join(scratch, claudeBGResultFilename), []byte(`{"recovered":true}`), 0o600) + } + fmt.Println(`{"type":"result","subtype":"success","is_error":false,"result":"recovered","session_id":"fg-recovered"}`) case "foreground-no-result": fmt.Println(`{"type":"result","subtype":"success","is_error":false,"result":"forgot the file","session_id":"fg-session-2"}`) case "foreground-fail": @@ -2158,3 +2166,56 @@ func TestClaudeTransportXORValidation(t *testing.T) { } } } + +// A job service that refuses the task must not cost the whole reviewer: the +// review it would have produced is still available on the other transport, +// and a caller gating on coverage cannot tell "reviewer found nothing" from +// "reviewer never ran". +func TestSubprocessClaudeBackgroundFailureFallsBackToForeground(t *testing.T) { + tempDir := t.TempDir() + recordPath := filepath.Join(tempDir, "records.jsonl") + configDir := filepath.Join(tempDir, "claude") + adapter := newClaudeHelperAdapter("bg-blocked-foreground-recovers", recordPath, configDir, 5*time.Second) + + stream, err := adapter.Start(context.Background(), Request{ + Model: "claude-sonnet-5", + Prompt: "prompt", + LogPath: filepath.Join(tempDir, "events.log"), + }) + if err != nil { + t.Fatalf("Start: %v", err) + } + response, err := stream.Wait(context.Background()) + if err != nil { + t.Fatalf("Wait: %v", err) + } + if string(response.StructuredOutput) != `{"recovered":true}` { + t.Fatalf("StructuredOutput = %s, want the foreground result", response.StructuredOutput) + } + if stream.SessionID() != "fg-recovered" { + t.Fatalf("SessionID = %q, want the foreground session", stream.SessionID()) + } +} + +// The fallback is for the transport, not for the model: a job that ran and +// wrote nothing usable would only repeat itself, and the original failure is +// what the caller has to see. +func TestSubprocessClaudeBackgroundModelFailureIsNotRetried(t *testing.T) { + tempDir := t.TempDir() + recordPath := filepath.Join(tempDir, "records.jsonl") + configDir := filepath.Join(tempDir, "claude") + adapter := newClaudeHelperAdapter("bg-empty-result", recordPath, configDir, 5*time.Second) + + stream, err := adapter.Start(context.Background(), Request{ + Model: "claude-sonnet-5", + Prompt: "prompt", + LogPath: filepath.Join(tempDir, "events.log"), + }) + if err != nil { + t.Fatalf("Start: %v", err) + } + if _, err := stream.Wait(context.Background()); err == nil || + !strings.Contains(err.Error(), "empty result file") { + t.Fatalf("Wait error = %v, want the original empty-result failure", err) + } +} From d3def239dd0fef9b87b5122750d3ea8e6e3a1389 Mon Sep 17 00:00:00 2001 From: piekstra Date: Mon, 31 Aug 2026 14:58:32 -0400 Subject: [PATCH 2/6] fix(llmadapters): retry only transport failures, and never at the cost of evidence MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review findings. A job that ran to completion and wrote its result is finished, whatever its state file says about session ids. The session-id check ran before the result read, so such a job was marked as a transport failure and re-run end to end, spending a second model run on output already on disk — the opposite of this change's own rule that a completed job is not retried. The result is read first now, and an empty session id is returned as empty, which is the position the foreground transport already takes on the same condition. The launch-side fallback retried on any error, so a configuration failure in background arg construction was reported as a foreground problem. It now applies the same ErrClaudeBGTransport test the wait side does, and joins the original error when the fallback launch fails too. The retry no longer truncates the log of the failure it is retrying. Task logs are opened with os.Create, so a shared path erased the background attempt — including the banner and blocked-state detail that identify this failure mode, and the only account of the error still reported when the retry also fails. The retry writes to a sibling path instead. Both attempts now share one task deadline rather than each starting a full window, so the Timeout contract still bounds a task rather than an attempt. SessionID reports the stream that produced the response and nothing else. Pairing a foreground response with the background job's session sent a later resume at a session the transport had already lost. Tests: the states table now asserts launch counts per case, so a retry that stops happening, or one that starts happening for a model failure, fails the test rather than passing on an unhandled helper mode; a completed job with a result and no session id is not retried; a fallback keeps both transports' logs. --- internal/llmadapters/subprocess.go | 123 +++++++++++++++++++----- internal/llmadapters/subprocess_test.go | 105 ++++++++++++++++++-- 2 files changed, 199 insertions(+), 29 deletions(-) diff --git a/internal/llmadapters/subprocess.go b/internal/llmadapters/subprocess.go index 34b823e..c6cd1d2 100644 --- a/internal/llmadapters/subprocess.go +++ b/internal/llmadapters/subprocess.go @@ -365,33 +365,76 @@ func (a *SubprocessAdapter) startClaude(ctx context.Context, req Request, resume } bg, err := a.startClaudeBG(ctx, req, resumeSessionID) if err != nil { - // The launch itself failed, so there is no job to wait on and the - // fallback is immediate. - return a.startClaudeForeground(ctx, req, resumeSessionID) + // A launch that failed on configuration would fail the same way in + // foreground, and reporting it as a foreground problem hides where it + // came from. Only a transport-shaped launch failure is retried, the + // same rule Wait applies. + if !errors.Is(err, ErrClaudeBGTransport) { + return nil, err + } + fallback, fallbackErr := a.startClaudeForeground(ctx, req, resumeSessionID) + if fallbackErr != nil { + return nil, errors.Join(err, fallbackErr) + } + return fallback, nil + } + return &claudeFallbackStream{ + adapter: a, + req: req, + resume: resumeSessionID, + primary: bg, + // One budget for the task, not one per attempt: the Timeout contract + // bounds a task, and a retry that started its own full window would + // make a Claude task take twice as long as callers size for. + deadline: taskDeadline(ctx, a.timeout), + }, nil +} + +// taskDeadline is when this task's whole budget runs out, so a retry gets +// what is left rather than a fresh window. Zero means unbounded. +func taskDeadline(ctx context.Context, timeout time.Duration) time.Time { + deadlines := make([]time.Time, 0, 2) + if timeout > 0 { + deadlines = append(deadlines, time.Now().Add(timeout)) } - return &claudeFallbackStream{adapter: a, req: req, resume: resumeSessionID, primary: bg}, nil + if ctxDeadline, ok := ctx.Deadline(); ok { + deadlines = append(deadlines, ctxDeadline) + } + earliest := time.Time{} + for _, d := range deadlines { + if earliest.IsZero() || d.Before(earliest) { + earliest = d + } + } + return earliest } // claudeFallbackStream waits on the background job and, when the job service // fails it, runs the same request again in foreground. type claudeFallbackStream struct { - adapter *SubprocessAdapter - req Request - resume string - primary Stream - - mu sync.Mutex + adapter *SubprocessAdapter + req Request + resume string + primary Stream + deadline time.Time + + mu sync.Mutex + // fallback is the retry once started; winner is the stream whose response + // Wait returned. Until then the primary is the only one that has run. fallback Stream + winner Stream } +// SessionID reports the session of the stream that produced the response, and +// nothing else. Reporting the background job's session alongside a foreground +// response would pair a live result with a session that is gone, and a resume +// against it spends an attempt discovering that. func (s *claudeFallbackStream) SessionID() string { s.mu.Lock() - fallback := s.fallback + winner := s.winner s.mu.Unlock() - if fallback != nil { - if id := fallback.SessionID(); id != "" { - return id - } + if winner != nil { + return winner.SessionID() } return s.primary.SessionID() } @@ -401,7 +444,16 @@ func (s *claudeFallbackStream) Wait(ctx context.Context) (Response, error) { if err == nil || ctx.Err() != nil || !errors.Is(err, ErrClaudeBGTransport) { return response, err } - fallback, startErr := s.adapter.startClaudeForeground(ctx, s.req, s.resume) + + retryCtx := ctx + if !s.deadline.IsZero() { + var cancel context.CancelFunc + retryCtx, cancel = context.WithDeadline(ctx, s.deadline) + defer cancel() + } + req := s.req + req.LogPath = foregroundRetryLogPath(s.req.LogPath) + fallback, startErr := s.adapter.startClaudeForeground(retryCtx, req, s.resume) if startErr != nil { // The original failure is the one worth reporting: the fallback not // starting is a second symptom, not the cause. @@ -410,16 +462,33 @@ func (s *claudeFallbackStream) Wait(ctx context.Context) (Response, error) { s.mu.Lock() s.fallback = fallback s.mu.Unlock() - fallbackResponse, fallbackErr := fallback.Wait(ctx) + fallbackResponse, fallbackErr := fallback.Wait(retryCtx) if fallbackErr != nil { // Report what actually went wrong first. The retry failing too says // nothing new, and burying the transport failure under it is how a - // job-service problem gets read as a reviewer problem. + // job-service problem gets read as a reviewer problem. The primary's + // log still describes the error being returned. return response, err } + s.mu.Lock() + s.winner = fallback + s.mu.Unlock() return fallbackResponse, nil } +// foregroundRetryLogPath is where the retry writes. Task logs are opened with +// os.Create, so reusing the primary's path would truncate the record of the +// failure being retried — the one artifact that explains why a retry happened +// at all, and the only account of the error this stream still reports when the +// retry fails too. An empty path stays empty: no log was wanted. +func foregroundRetryLogPath(primary string) string { + if strings.TrimSpace(primary) == "" { + return primary + } + ext := filepath.Ext(primary) + return strings.TrimSuffix(primary, ext) + ".foreground" + ext +} + // startClaudeForeground runs a Claude task as a plain foreground child in // headless print mode. Same scratch, prompt-file, env, and result-file // contract as background mode — only the transport differs. @@ -1389,14 +1458,22 @@ func (a *SubprocessAdapter) waitForClaudeBGResult(ctx context.Context, jobID str if sessionID == "" { sessionID, state = a.waitForClaudeBGSessionID(ctx, jobID, state) } + // The result first, and the session id second. A job that ran to + // completion and wrote its result is done, and re-running the whole task + // because its state file never carried a session id would spend a second + // model run on output already sitting on disk. An empty session id costs + // a later resume, which callers already handle, and the foreground + // transport takes the same position on the same condition. + output, err := readFirstNonEmptyFile(resultPaths) + if err == nil { + return Response{StructuredOutput: output, Usage: claudeBGTranscriptUsage(state)}, sessionID, nil + } if sessionID == "" { + // Neither a result nor a session: the job left nothing behind, which + // is the transport failing rather than the model. return Response{}, "", fmt.Errorf("%w: job completed without session id: %s", ErrClaudeBGTransport, claudeBGStateDetail(state)) } - output, err := readFirstNonEmptyFile(resultPaths) - if err != nil { - return Response{}, sessionID, err - } - return Response{StructuredOutput: output, Usage: claudeBGTranscriptUsage(state)}, sessionID, nil + return Response{}, sessionID, err } func (a *SubprocessAdapter) waitForClaudeBGState(ctx context.Context, jobID string, resultPaths []string) (map[string]any, error) { diff --git a/internal/llmadapters/subprocess_test.go b/internal/llmadapters/subprocess_test.go index 4686027..a16dd86 100644 --- a/internal/llmadapters/subprocess_test.go +++ b/internal/llmadapters/subprocess_test.go @@ -353,14 +353,19 @@ func TestSubprocessClaudeBackgroundStatesAndCleanup(t *testing.T) { wantStop bool timeout time.Duration wantRawResult bool + // wantRetry marks the states startClaude treats as transport + // failures. Asserting the launch count is what keeps this table + // honest: without it a retry that stopped happening, or one that + // started happening for a model failure, would both still pass. + wantRetry bool }{ {name: "idle result", mode: "bg-idle-result", wantOutput: `{"idle":true}`, wantSession: "session-idle"}, {name: "invalid json is returned raw", mode: "bg-invalid-json", wantOutput: `not-json`, wantSession: "session-invalid", wantRawResult: true}, - {name: "blocked", mode: "bg-blocked", wantErr: "blocked: permission needed", wantStop: true}, - {name: "failed", mode: "bg-failed", wantErr: "failed: model failed", wantStop: true}, - {name: "waiting", mode: "bg-waiting", wantErr: "waiting: waiting for input", wantStop: true}, - {name: "stopped", mode: "bg-stopped", wantErr: "stopped: stopped by user", wantStop: true}, - {name: "stop fails still removes", mode: "bg-stop-fails", wantErr: "blocked: stop will fail", wantStop: true}, + {name: "blocked", mode: "bg-blocked", wantErr: "blocked: permission needed", wantStop: true, wantRetry: true}, + {name: "failed", mode: "bg-failed", wantErr: "failed: model failed", wantStop: true, wantRetry: true}, + {name: "waiting", mode: "bg-waiting", wantErr: "waiting: waiting for input", wantStop: true, wantRetry: true}, + {name: "stopped", mode: "bg-stopped", wantErr: "stopped: stopped by user", wantStop: true, wantRetry: true}, + {name: "stop fails still removes", mode: "bg-stop-fails", wantErr: "blocked: stop will fail", wantStop: true, wantRetry: true}, {name: "missing result", mode: "bg-missing-result", wantErr: "completed without writing result file", wantSession: "session-missing", wantStop: true}, {name: "empty result", mode: "bg-empty-result", wantErr: "empty result file", wantSession: "session-empty", wantStop: true}, {name: "timeout", mode: "bg-running", wantErrIs: context.DeadlineExceeded, wantStop: true, timeout: 50 * time.Millisecond}, @@ -391,7 +396,9 @@ func TestSubprocessClaudeBackgroundStatesAndCleanup(t *testing.T) { if tt.wantSession != "" && stream.SessionID() != tt.wantSession { t.Fatalf("SessionID = %q, want %q", stream.SessionID(), tt.wantSession) } - assertClaudeCleanup(t, readHelperRecords(t, recordPath), "job-1", tt.wantStop, configDir) + records := readHelperRecords(t, recordPath) + assertClaudeCleanup(t, records, "job-1", tt.wantStop, configDir) + assertClaudeLaunchCount(t, records, tt.wantRetry) return } if err != nil { @@ -1638,6 +1645,9 @@ func runClaudeBGHelper(mode string, args []string) { case "bg-blocked-foreground-recovers": state = map[string]any{"state": "blocked", "detail": "prompt file no longer exists"} writeResult = false + case "bg-result-without-session": + state = map[string]any{"state": "done"} + result = `{"done":true}` case "bg-failed": state = map[string]any{"state": "failed", "error": "model failed"} writeResult = false @@ -1728,6 +1738,28 @@ func readHelperRecord(t *testing.T, path string) helperRecord { return records[0] } +// assertClaudeLaunchCount checks how many task launches the helper saw: one +// for the background job, plus one more when the failure was transport-shaped +// and the task was retried in foreground. Control calls (stop, rm, agents) are +// not launches and are excluded. +func assertClaudeLaunchCount(t *testing.T, records []helperRecord, wantRetry bool) { + t.Helper() + launches := 0 + for _, record := range records { + if containsFlag(record.AdapterArgs, "--bg") || + (containsFlag(record.AdapterArgs, "-p") && flagValue(record.AdapterArgs, "--output-format") == "json") { + launches++ + } + } + want := 1 + if wantRetry { + want = 2 + } + if launches != want { + t.Fatalf("task launches = %d, want %d (retry expected: %v)", launches, want, wantRetry) + } +} + func readHelperRecords(t *testing.T, path string) []helperRecord { t.Helper() // #nosec G304 -- test reads helper output from a t.TempDir path. @@ -2219,3 +2251,64 @@ func TestSubprocessClaudeBackgroundModelFailureIsNotRetried(t *testing.T) { t.Fatalf("Wait error = %v, want the original empty-result failure", err) } } + +// A job that ran to completion and wrote its result is finished, whatever its +// state file says about session ids. Re-running it would spend a second model +// run on output already on disk. +func TestSubprocessClaudeBackgroundResultWithoutSessionIsNotRetried(t *testing.T) { + tempDir := t.TempDir() + recordPath := filepath.Join(tempDir, "records.jsonl") + configDir := filepath.Join(tempDir, "claude") + adapter := newClaudeHelperAdapter("bg-result-without-session", recordPath, configDir, 5*time.Second) + + stream, err := adapter.Start(context.Background(), Request{Prompt: "prompt"}) + if err != nil { + t.Fatalf("Start: %v", err) + } + response, err := stream.Wait(context.Background()) + if err != nil { + t.Fatalf("Wait: %v", err) + } + if string(response.StructuredOutput) != `{"done":true}` { + t.Fatalf("StructuredOutput = %s, want the background result", response.StructuredOutput) + } + if stream.SessionID() != "" { + t.Fatalf("SessionID = %q, want empty rather than invented", stream.SessionID()) + } + assertClaudeLaunchCount(t, readHelperRecords(t, recordPath), false) +} + +// The retry must not erase the record of the failure it is retrying: task +// logs are opened with os.Create, so a shared path would truncate the one +// artifact that explains why a retry happened. +func TestSubprocessClaudeFallbackKeepsBothTransportLogs(t *testing.T) { + tempDir := t.TempDir() + recordPath := filepath.Join(tempDir, "records.jsonl") + configDir := filepath.Join(tempDir, "claude") + logPath := filepath.Join(tempDir, "events.log") + adapter := newClaudeHelperAdapter("bg-blocked-foreground-recovers", recordPath, configDir, 5*time.Second) + + stream, err := adapter.Start(context.Background(), Request{Prompt: "prompt", LogPath: logPath}) + if err != nil { + t.Fatalf("Start: %v", err) + } + if _, err := stream.Wait(context.Background()); err != nil { + t.Fatalf("Wait: %v", err) + } + + primary, err := os.ReadFile(logPath) // #nosec G304 -- t.TempDir path + if err != nil { + t.Fatalf("read primary log: %v", err) + } + if !strings.Contains(string(primary), "backgrounded") { + t.Fatalf("primary log lost the background attempt:\n%s", primary) + } + retryPath := filepath.Join(tempDir, "events.foreground.log") + retry, err := os.ReadFile(retryPath) // #nosec G304 -- t.TempDir path + if err != nil { + t.Fatalf("read retry log: %v", err) + } + if !strings.Contains(string(retry), "fg-recovered") { + t.Fatalf("retry log missing the foreground attempt:\n%s", retry) + } +} From 2335eeeac765467b20a1db9da45011cc970e4e29 Mon Sep 17 00:00:00 2001 From: piekstra Date: Mon, 31 Aug 2026 17:09:21 -0400 Subject: [PATCH 3/6] fix(llmadapters): drop the unreachable launch retry, and tell an empty result from a missing one Review findings. The launch-side fallback could never run: nothing on the launch path wraps ErrClaudeBGTransport, so the branch was dead, and a future change marking a launch failure would have landed there silently, reusing the primary log path and reintroducing the truncation the sibling path was added to prevent. A launch fails on configuration, a missing binary, or a scratch dir that could not be made, and foreground fails the same way, so the error is returned. An empty result file with no session id was being marked as a transport failure and retried, which the stated rule excludes: the job ran, and running it again repeats it. readFirstNonEmptyFile now returns errClaudeBGEmptyResult or errClaudeBGMissingResult, and only the missing case can mean the transport left nothing behind. The two are distinguished by sentinel rather than by message text. The fallback field was write-only once SessionID started reading the winning stream, so it is gone. Tests: taskDeadline covers its four branches (no bound, timeout only, context only, and the earlier of both), and a spent budget is shown to launch no retry at all rather than starting a fresh window. --- internal/llmadapters/subprocess.go | 46 +++++++------- internal/llmadapters/subprocess_test.go | 81 +++++++++++++++++++++++++ 2 files changed, 104 insertions(+), 23 deletions(-) diff --git a/internal/llmadapters/subprocess.go b/internal/llmadapters/subprocess.go index c6cd1d2..f1baf2a 100644 --- a/internal/llmadapters/subprocess.go +++ b/internal/llmadapters/subprocess.go @@ -365,18 +365,11 @@ func (a *SubprocessAdapter) startClaude(ctx context.Context, req Request, resume } bg, err := a.startClaudeBG(ctx, req, resumeSessionID) if err != nil { - // A launch that failed on configuration would fail the same way in - // foreground, and reporting it as a foreground problem hides where it - // came from. Only a transport-shaped launch failure is retried, the - // same rule Wait applies. - if !errors.Is(err, ErrClaudeBGTransport) { - return nil, err - } - fallback, fallbackErr := a.startClaudeForeground(ctx, req, resumeSessionID) - if fallbackErr != nil { - return nil, errors.Join(err, fallbackErr) - } - return fallback, nil + // Not retried: a launch fails on configuration, a missing binary, or + // a scratch dir that could not be made, and foreground would fail on + // the same thing. The retry is for a job service that accepted the + // task and then did not run it, which is a failure of the wait. + return nil, err } return &claudeFallbackStream{ adapter: a, @@ -419,10 +412,9 @@ type claudeFallbackStream struct { deadline time.Time mu sync.Mutex - // fallback is the retry once started; winner is the stream whose response - // Wait returned. Until then the primary is the only one that has run. - fallback Stream - winner Stream + // winner is the stream whose response Wait returned. Until then the + // primary is the only one that has run. + winner Stream } // SessionID reports the session of the stream that produced the response, and @@ -459,9 +451,6 @@ func (s *claudeFallbackStream) Wait(ctx context.Context) (Response, error) { // starting is a second symptom, not the cause. return response, err } - s.mu.Lock() - s.fallback = fallback - s.mu.Unlock() fallbackResponse, fallbackErr := fallback.Wait(retryCtx) if fallbackErr != nil { // Report what actually went wrong first. The retry failing too says @@ -1468,9 +1457,11 @@ func (a *SubprocessAdapter) waitForClaudeBGResult(ctx context.Context, jobID str if err == nil { return Response{StructuredOutput: output, Usage: claudeBGTranscriptUsage(state)}, sessionID, nil } - if sessionID == "" { + if sessionID == "" && errors.Is(err, errClaudeBGMissingResult) { // Neither a result nor a session: the job left nothing behind, which - // is the transport failing rather than the model. + // is the transport failing rather than the model. An empty result + // file is the other case, and falls through as the model failure it + // is: the job ran, and running it again would only repeat it. return Response{}, "", fmt.Errorf("%w: job completed without session id: %s", ErrClaudeBGTransport, claudeBGStateDetail(state)) } return Response{}, sessionID, err @@ -1595,6 +1586,15 @@ func anyNonEmptyFile(paths []string) bool { return false } +// errClaudeBGEmptyResult and errClaudeBGMissingResult separate "the job wrote +// nothing usable" from "the job wrote nothing at all". Only the second can +// mean the transport never ran the task; the first is a job that did run, and +// callers must not branch on the message text to tell them apart. +var ( + errClaudeBGEmptyResult = errors.New("llm subprocess: Claude background job wrote an empty result file") + errClaudeBGMissingResult = errors.New("llm subprocess: Claude background job completed without writing result file") +) + func readFirstNonEmptyFile(paths []string) ([]byte, error) { for _, path := range paths { // #nosec G304 -- result paths are adapter-owned scratch/job tmp paths. @@ -1603,11 +1603,11 @@ func readFirstNonEmptyFile(paths []string) ([]byte, error) { continue } if len(strings.TrimSpace(string(data))) == 0 { - return nil, errors.New("llm subprocess: Claude background job wrote an empty result file") + return nil, errClaudeBGEmptyResult } return data, nil } - return nil, errors.New("llm subprocess: Claude background job completed without writing result file") + return nil, errClaudeBGMissingResult } func claudeBGStateDetail(state map[string]any) string { diff --git a/internal/llmadapters/subprocess_test.go b/internal/llmadapters/subprocess_test.go index a16dd86..b43aae6 100644 --- a/internal/llmadapters/subprocess_test.go +++ b/internal/llmadapters/subprocess_test.go @@ -2312,3 +2312,84 @@ func TestSubprocessClaudeFallbackKeepsBothTransportLogs(t *testing.T) { t.Fatalf("retry log missing the foreground attempt:\n%s", retry) } } + +// One budget for the task: whichever bound runs out first is the one a retry +// inherits, and no bound at all stays unbounded. +func TestTaskDeadline(t *testing.T) { + now := time.Now() + ctxSoon, cancelSoon := context.WithDeadline(context.Background(), now.Add(time.Minute)) + defer cancelSoon() + ctxLate, cancelLate := context.WithDeadline(context.Background(), now.Add(time.Hour)) + defer cancelLate() + + for _, tt := range []struct { + name string + ctx context.Context + timeout time.Duration + want time.Duration // 0 means "no deadline" + }{ + {name: "no bound at all", ctx: context.Background(), want: 0}, + {name: "timeout only", ctx: context.Background(), timeout: 30 * time.Minute, want: 30 * time.Minute}, + {name: "context only", ctx: ctxSoon, want: time.Minute}, + {name: "context is earlier", ctx: ctxSoon, timeout: time.Hour, want: time.Minute}, + {name: "timeout is earlier", ctx: ctxLate, timeout: time.Minute, want: time.Minute}, + } { + t.Run(tt.name, func(t *testing.T) { + got := taskDeadline(tt.ctx, tt.timeout) + if tt.want == 0 { + if !got.IsZero() { + t.Fatalf("deadline = %v, want none", got) + } + return + } + if got.IsZero() { + t.Fatalf("deadline = none, want about %v out", tt.want) + } + // The clock moves between the call and the assertion, so compare + // the remaining window rather than an exact instant. + if diff := time.Until(got) - tt.want; diff > time.Second || diff < -time.Second { + t.Fatalf("deadline is %v out, want about %v", time.Until(got), tt.want) + } + }) + } +} + +// The retry runs under what is left of the task's window, not a fresh one. A +// task whose budget is already spent must not start a second attempt at all. +func TestSubprocessClaudeFallbackInheritsTheTaskBudget(t *testing.T) { + tempDir := t.TempDir() + recordPath := filepath.Join(tempDir, "records.jsonl") + configDir := filepath.Join(tempDir, "claude") + adapter := newClaudeHelperAdapter("bg-blocked-foreground-recovers", recordPath, configDir, 5*time.Second) + + primaryErr := fmt.Errorf("%w: job blocked: prompt file no longer exists", ErrClaudeBGTransport) + stream := &claudeFallbackStream{ + adapter: adapter, + req: Request{Prompt: "prompt"}, + primary: stubStream{sessionID: "bg-session", err: primaryErr}, + // Spent: with a fresh window the retry would run and succeed, and the + // helper would record a launch. + deadline: time.Now().Add(-time.Second), + } + + if _, err := stream.Wait(context.Background()); !errors.Is(err, ErrClaudeBGTransport) { + t.Fatalf("Wait error = %v, want the primary transport failure", err) + } + if stream.SessionID() != "bg-session" { + t.Fatalf("SessionID = %q, want the primary's", stream.SessionID()) + } + if _, err := os.Stat(recordPath); err == nil { + t.Fatalf("a spent budget must not launch a retry: %v", readHelperRecords(t, recordPath)) + } +} + +// stubStream stands in for a transport that has already run. +type stubStream struct { + sessionID string + response Response + err error +} + +func (s stubStream) SessionID() string { return s.sessionID } + +func (s stubStream) Wait(context.Context) (Response, error) { return s.response, s.err } From 900c1ab922953004065334355b24d92a1213fe89 Mon Sep 17 00:00:00 2001 From: piekstra Date: Mon, 31 Aug 2026 17:14:15 -0400 Subject: [PATCH 4/6] fix(llmadapters): name the result-file sentinels for the file, not the transport Both transports read result files through readFirstNonEmptyFile, so sentinels named and documented for the background one made the foreground path report "foreground task completed without a result file: ...background job completed without writing result file". They now describe the file ("no result file", "result file is empty") and each call site supplies its own prefix, which is what both already did. The rule about which case can mean the transport never ran the task moved to the branch that applies it. --- internal/llmadapters/subprocess.go | 28 +++++++++++++------------ internal/llmadapters/subprocess_test.go | 6 +++--- 2 files changed, 18 insertions(+), 16 deletions(-) diff --git a/internal/llmadapters/subprocess.go b/internal/llmadapters/subprocess.go index f1baf2a..126f8c1 100644 --- a/internal/llmadapters/subprocess.go +++ b/internal/llmadapters/subprocess.go @@ -1457,14 +1457,15 @@ func (a *SubprocessAdapter) waitForClaudeBGResult(ctx context.Context, jobID str if err == nil { return Response{StructuredOutput: output, Usage: claudeBGTranscriptUsage(state)}, sessionID, nil } - if sessionID == "" && errors.Is(err, errClaudeBGMissingResult) { + if sessionID == "" && errors.Is(err, errClaudeMissingResultFile) { // Neither a result nor a session: the job left nothing behind, which - // is the transport failing rather than the model. An empty result - // file is the other case, and falls through as the model failure it - // is: the job ran, and running it again would only repeat it. + // is the only shape here that can mean the transport never ran the + // task. An empty result file is the other case, and falls through as + // the model failure it is: the job ran, and running it again would + // only repeat it. return Response{}, "", fmt.Errorf("%w: job completed without session id: %s", ErrClaudeBGTransport, claudeBGStateDetail(state)) } - return Response{}, sessionID, err + return Response{}, sessionID, fmt.Errorf("llm subprocess: Claude background job: %w", err) } func (a *SubprocessAdapter) waitForClaudeBGState(ctx context.Context, jobID string, resultPaths []string) (map[string]any, error) { @@ -1586,13 +1587,14 @@ func anyNonEmptyFile(paths []string) bool { return false } -// errClaudeBGEmptyResult and errClaudeBGMissingResult separate "the job wrote -// nothing usable" from "the job wrote nothing at all". Only the second can -// mean the transport never ran the task; the first is a job that did run, and -// callers must not branch on the message text to tell them apart. +// errClaudeEmptyResultFile and errClaudeMissingResultFile separate "the run +// wrote nothing usable" from "the run wrote nothing at all". Both transports +// read result files through this helper and each supplies its own prefix, so +// these describe the file and not who was reading it. Callers tell the two +// apart with errors.Is rather than by message text. var ( - errClaudeBGEmptyResult = errors.New("llm subprocess: Claude background job wrote an empty result file") - errClaudeBGMissingResult = errors.New("llm subprocess: Claude background job completed without writing result file") + errClaudeEmptyResultFile = errors.New("result file is empty") + errClaudeMissingResultFile = errors.New("no result file") ) func readFirstNonEmptyFile(paths []string) ([]byte, error) { @@ -1603,11 +1605,11 @@ func readFirstNonEmptyFile(paths []string) ([]byte, error) { continue } if len(strings.TrimSpace(string(data))) == 0 { - return nil, errClaudeBGEmptyResult + return nil, errClaudeEmptyResultFile } return data, nil } - return nil, errClaudeBGMissingResult + return nil, errClaudeMissingResultFile } func claudeBGStateDetail(state map[string]any) string { diff --git a/internal/llmadapters/subprocess_test.go b/internal/llmadapters/subprocess_test.go index b43aae6..7202f11 100644 --- a/internal/llmadapters/subprocess_test.go +++ b/internal/llmadapters/subprocess_test.go @@ -366,8 +366,8 @@ func TestSubprocessClaudeBackgroundStatesAndCleanup(t *testing.T) { {name: "waiting", mode: "bg-waiting", wantErr: "waiting: waiting for input", wantStop: true, wantRetry: true}, {name: "stopped", mode: "bg-stopped", wantErr: "stopped: stopped by user", wantStop: true, wantRetry: true}, {name: "stop fails still removes", mode: "bg-stop-fails", wantErr: "blocked: stop will fail", wantStop: true, wantRetry: true}, - {name: "missing result", mode: "bg-missing-result", wantErr: "completed without writing result file", wantSession: "session-missing", wantStop: true}, - {name: "empty result", mode: "bg-empty-result", wantErr: "empty result file", wantSession: "session-empty", wantStop: true}, + {name: "missing result", mode: "bg-missing-result", wantErr: "background job: no result file", wantSession: "session-missing", wantStop: true}, + {name: "empty result", mode: "bg-empty-result", wantErr: "background job: result file is empty", wantSession: "session-empty", wantStop: true}, {name: "timeout", mode: "bg-running", wantErrIs: context.DeadlineExceeded, wantStop: true, timeout: 50 * time.Millisecond}, } { t.Run(tt.name, func(t *testing.T) { @@ -2247,7 +2247,7 @@ func TestSubprocessClaudeBackgroundModelFailureIsNotRetried(t *testing.T) { t.Fatalf("Start: %v", err) } if _, err := stream.Wait(context.Background()); err == nil || - !strings.Contains(err.Error(), "empty result file") { + !strings.Contains(err.Error(), "result file is empty") { t.Fatalf("Wait error = %v, want the original empty-result failure", err) } } From 60ada627c9c75233da878d1368ca95a0fe3888b3 Mon Sep 17 00:00:00 2001 From: piekstra Date: Mon, 31 Aug 2026 17:21:28 -0400 Subject: [PATCH 5/6] test(llmadapters): cover the job that leaves nothing behind The one branch that marks a background failure as ErrClaudeBGTransport had no test: the table's missing-result case carries a session id and takes the non-transport path, the empty-result case carries the other sentinel, and the result-without-session case is the opposite combination. A regression that stopped marking this case, or started marking one with a session id, passed everything. The new case is a job that finishes with neither a session id nor a result, and it asserts both the sentinel and that the retry was attempted. The session-id grace is an adapter field defaulting to the existing constant, so the test drives that path without waiting out a ten-second window. Verified the test fails with the branch disabled. --- internal/llmadapters/subprocess.go | 10 +++++++- internal/llmadapters/subprocess_test.go | 32 +++++++++++++++++++++++++ 2 files changed, 41 insertions(+), 1 deletion(-) diff --git a/internal/llmadapters/subprocess.go b/internal/llmadapters/subprocess.go index 126f8c1..29a0acb 100644 --- a/internal/llmadapters/subprocess.go +++ b/internal/llmadapters/subprocess.go @@ -101,6 +101,10 @@ type SubprocessAdapter struct { scratchDirFactory ScratchDirFactory allowBestEffortNoTools bool fastModeModels []string + // sessionIDGrace is how long a completed job is given to publish a + // session id. A field rather than the constant so a test can exercise + // the paths behind it without waiting out the real grace window. + sessionIDGrace time.Duration } // Claude CLI and Codex CLI share this concrete implementation. @@ -1509,7 +1513,11 @@ func (a *SubprocessAdapter) waitForClaudeBGState(ctx context.Context, jobID stri func (a *SubprocessAdapter) waitForClaudeBGSessionID(ctx context.Context, jobID string, lastState map[string]any) (string, map[string]any) { statePath := filepath.Join(claudeConfigDirFromEnv(a.env), "jobs", jobID, "state.json") - deadline := time.NewTimer(claudeBGSessionIDGrace) + grace := a.sessionIDGrace + if grace <= 0 { + grace = claudeBGSessionIDGrace + } + deadline := time.NewTimer(grace) defer deadline.Stop() ticker := time.NewTicker(claudeBGPollInterval) defer ticker.Stop() diff --git a/internal/llmadapters/subprocess_test.go b/internal/llmadapters/subprocess_test.go index 7202f11..1c4944b 100644 --- a/internal/llmadapters/subprocess_test.go +++ b/internal/llmadapters/subprocess_test.go @@ -1648,6 +1648,9 @@ func runClaudeBGHelper(mode string, args []string) { case "bg-result-without-session": state = map[string]any{"state": "done"} result = `{"done":true}` + case "bg-nothing-left-behind": + state = map[string]any{"state": "done"} + writeResult = false case "bg-failed": state = map[string]any{"state": "failed", "error": "model failed"} writeResult = false @@ -2393,3 +2396,32 @@ type stubStream struct { func (s stubStream) SessionID() string { return s.sessionID } func (s stubStream) Wait(context.Context) (Response, error) { return s.response, s.err } + +// The one shape that can mean the job service accepted the task and never ran +// it: a job that finished with neither a session id nor a result. It is the +// case the transport marking exists for, so it has to be retried. +func TestSubprocessClaudeBackgroundNothingLeftBehindIsRetried(t *testing.T) { + tempDir := t.TempDir() + recordPath := filepath.Join(tempDir, "records.jsonl") + configDir := filepath.Join(tempDir, "claude") + adapter := newClaudeHelperAdapter("bg-nothing-left-behind", recordPath, configDir, 5*time.Second) + // The job is already done, so waiting out the real grace window would + // only make the test slow. + adapter.sessionIDGrace = 20 * time.Millisecond + + stream, err := adapter.Start(context.Background(), Request{Prompt: "prompt"}) + if err != nil { + t.Fatalf("Start: %v", err) + } + _, err = stream.Wait(context.Background()) + if err == nil { + t.Fatal("Wait: want a failure, the job produced nothing") + } + if !errors.Is(err, ErrClaudeBGTransport) { + t.Fatalf("Wait error = %v, want ErrClaudeBGTransport", err) + } + // The foreground helper has no case for this mode, so the retry fails + // too and the primary error is what surfaces — but it must have been + // attempted. + assertClaudeLaunchCount(t, readHelperRecords(t, recordPath), true) +} From 5afb105ea4f7db3c3d578ac2b52bc8e9665729d5 Mon Sep 17 00:00:00 2001 From: piekstra Date: Mon, 31 Aug 2026 17:26:47 -0400 Subject: [PATCH 6/6] refactor(llmadapters): take the session-id grace through the constructor The field was the one adapter dependency written directly by tests, which routed around the unexported SubprocessOptions seam that commandArgsPrefix already uses, and it resolved its default at the use site rather than once in the constructor the way timeout does, so the stored value was not the value in effect. It is an unexported option now, defaulted in newSubprocessAdapter, and the wait reads the field directly. --- internal/llmadapters/subprocess.go | 18 +++++++++++------- internal/llmadapters/subprocess_test.go | 14 ++++++++++++-- 2 files changed, 23 insertions(+), 9 deletions(-) diff --git a/internal/llmadapters/subprocess.go b/internal/llmadapters/subprocess.go index 29a0acb..c32b5a3 100644 --- a/internal/llmadapters/subprocess.go +++ b/internal/llmadapters/subprocess.go @@ -42,6 +42,10 @@ type SubprocessOptions struct { AllowBestEffortNoTools bool FastModeModels []string commandArgsPrefix []string + // sessionIDGrace overrides how long a completed job is given to publish + // a session id. Unexported beside commandArgsPrefix: the seam exists for + // tests, which would otherwise wait out the real window. + sessionIDGrace time.Duration } // defaultLLMTaskTimeout bounds a single LLM task when the caller does not set @@ -102,8 +106,7 @@ type SubprocessAdapter struct { allowBestEffortNoTools bool fastModeModels []string // sessionIDGrace is how long a completed job is given to publish a - // session id. A field rather than the constant so a test can exercise - // the paths behind it without waiting out the real grace window. + // session id, already resolved to the value in effect. sessionIDGrace time.Duration } @@ -133,6 +136,10 @@ func newSubprocessAdapter(kind subprocessKind, defaultCommand string, opts Subpr if timeout == 0 { timeout = defaultLLMTaskTimeout } + sessionIDGrace := opts.sessionIDGrace + if sessionIDGrace <= 0 { + sessionIDGrace = claudeBGSessionIDGrace + } return &SubprocessAdapter{ kind: kind, command: command, @@ -142,6 +149,7 @@ func newSubprocessAdapter(kind subprocessKind, defaultCommand string, opts Subpr scratchDirFactory: factory, allowBestEffortNoTools: opts.AllowBestEffortNoTools, fastModeModels: append([]string(nil), opts.FastModeModels...), + sessionIDGrace: sessionIDGrace, } } @@ -1513,11 +1521,7 @@ func (a *SubprocessAdapter) waitForClaudeBGState(ctx context.Context, jobID stri func (a *SubprocessAdapter) waitForClaudeBGSessionID(ctx context.Context, jobID string, lastState map[string]any) (string, map[string]any) { statePath := filepath.Join(claudeConfigDirFromEnv(a.env), "jobs", jobID, "state.json") - grace := a.sessionIDGrace - if grace <= 0 { - grace = claudeBGSessionIDGrace - } - deadline := time.NewTimer(grace) + deadline := time.NewTimer(a.sessionIDGrace) defer deadline.Stop() ticker := time.NewTicker(claudeBGPollInterval) defer ticker.Stop() diff --git a/internal/llmadapters/subprocess_test.go b/internal/llmadapters/subprocess_test.go index 1c4944b..f94baaa 100644 --- a/internal/llmadapters/subprocess_test.go +++ b/internal/llmadapters/subprocess_test.go @@ -1557,6 +1557,16 @@ func newClaudeHelperAdapter(mode string, recordPath string, configDir string, ti return newClaudeHelperAdapterWithEnv(mode, recordPath, configDir, timeout) } +func newClaudeHelperAdapterWithGrace(mode string, recordPath string, configDir string, timeout time.Duration, grace time.Duration) *SubprocessAdapter { + return NewClaudeCLIAdapter(SubprocessOptions{ + Command: os.Args[0], + commandArgsPrefix: helperPrefix(), + Env: helperClaudeEnv(mode, recordPath, configDir), + Timeout: timeout, + sessionIDGrace: grace, + }) +} + func newClaudeHelperAdapterWithEnv(mode string, recordPath string, configDir string, timeout time.Duration, extraEnv ...string) *SubprocessAdapter { return NewClaudeCLIAdapter(SubprocessOptions{ Command: os.Args[0], @@ -2404,10 +2414,10 @@ func TestSubprocessClaudeBackgroundNothingLeftBehindIsRetried(t *testing.T) { tempDir := t.TempDir() recordPath := filepath.Join(tempDir, "records.jsonl") configDir := filepath.Join(tempDir, "claude") - adapter := newClaudeHelperAdapter("bg-nothing-left-behind", recordPath, configDir, 5*time.Second) // The job is already done, so waiting out the real grace window would // only make the test slow. - adapter.sessionIDGrace = 20 * time.Millisecond + adapter := newClaudeHelperAdapterWithGrace( + "bg-nothing-left-behind", recordPath, configDir, 5*time.Second, 20*time.Millisecond) stream, err := adapter.Start(context.Background(), Request{Prompt: "prompt"}) if err != nil {