Skip to content

Commit dac14bd

Browse files
committed
llm: IsTerminal must not kill the turn on recoverable failures
IsTerminal was an alias for terminalForFallback, but the two answer different questions. terminalForFallback asks "should the fallback walk stop here?", and several of its cases stop the walk precisely BECAUSE something else recovers them: a context overflow compacts and retries, an incomplete stream retries the same call, an exhausted lane raises the user's fallback-choice card. IsTerminal decides whether a delegated worker's failure ENDS THE TURN, which is strictly narrower. As written, a worker that merely overflowed its context would kill the parent turn instead of compacting — replacing a working recovery with a hard failure, and looking very much like the hang this classification was added to prevent. Shipped in v0.31.0; the new table test fails against that build and passes here.
1 parent efb6ebf commit dac14bd

2 files changed

Lines changed: 65 additions & 1 deletion

File tree

‎internal/llm/policy_test.go‎

Lines changed: 41 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -368,3 +368,44 @@ func TestRequestShapeErrorsAreTerminalButTimingErrorsWalk(t *testing.T) {
368368
})
369369
}
370370
}
371+
372+
// TestIsTerminalExcludesRecoverableCauses: IsTerminal decides whether a failure
373+
// KILLS THE TURN, which is a stricter question than "should the fallback walk
374+
// stop here?" — the question terminalForFallback answers.
375+
//
376+
// Several causes stop the walk precisely because something else recovers them.
377+
// Aliasing the two would make a delegated worker's context overflow kill the
378+
// parent turn instead of compacting and retrying: the recovery would be
379+
// replaced by a hard failure, and the bug would look like the very hang this
380+
// classification exists to prevent.
381+
func TestIsTerminalExcludesRecoverableCauses(t *testing.T) {
382+
shape := fmt.Errorf("gemini stream: %w", &genai.APIError{Code: 400, Message: "malformed"})
383+
384+
for _, tc := range []struct {
385+
name string
386+
err error
387+
want bool
388+
why string
389+
}{
390+
{"malformed request", shape, true, "fails identically on every model"},
391+
{"unauthorized", wire.ErrUnauthorized, true, "needs the user to sign in"},
392+
{"insufficient credit", wire.ErrInsufficientCredit, true, "needs the user to top up"},
393+
{"context overflow", wire.ErrContextOverflow, false, "compact-and-retry recovers it"},
394+
{"incomplete stream", wire.ErrStreamIncomplete, false, "the same call is retried"},
395+
{"rate limited", fmt.Errorf("x: %w", &genai.APIError{Code: 429}), false, "timing, not shape"},
396+
{"provider down", fmt.Errorf("x: %w", &genai.APIError{Code: 503}), false, "infrastructure"},
397+
{"transport failure", errors.New("connection reset"), false, "infrastructure"},
398+
{"nil", nil, false, "no failure at all"},
399+
} {
400+
t.Run(tc.name, func(t *testing.T) {
401+
if got := IsTerminal(tc.err); got != tc.want {
402+
t.Errorf("IsTerminal = %v, want %v — %s", got, tc.want, tc.why)
403+
}
404+
})
405+
}
406+
407+
// The lane-exhausted card is the user's decision, not a turn kill.
408+
if IsTerminal(&provider.ErrLaneExhausted{}) {
409+
t.Error("an exhausted lane raises the fallback-choice card; it must not kill the turn")
410+
}
411+
}

‎internal/llm/recover.go‎

Lines changed: 24 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -41,7 +41,30 @@ const maxModelFallbacks = 2
4141
// thought-signature 400 was retried roughly 150 times over 12 minutes, because
4242
// nothing in the loop could tell "this tool had a bad day" from "this will
4343
// never work".
44-
func IsTerminal(err error) bool { return err != nil && terminalForFallback(err) }
44+
func IsTerminal(err error) bool {
45+
if err == nil {
46+
return false
47+
}
48+
// NOT an alias for terminalForFallback. That answers "should the fallback
49+
// walk stop here?", and several of its cases stop the walk precisely BECAUSE
50+
// something else recovers them: a context overflow compacts and retries, an
51+
// incomplete stream retries the same call, an exhausted lane raises the
52+
// user's fallback-choice card. Killing the turn on those would break the
53+
// recovery instead of surfacing a cause.
54+
for _, recoverable := range []error{
55+
wire.ErrContextOverflow, // compact-and-retry, inside the worker
56+
wire.ErrStreamIncomplete, // same-call transport retry
57+
} {
58+
if errors.Is(err, recoverable) {
59+
return false
60+
}
61+
}
62+
var exh *provider.ErrLaneExhausted
63+
if errors.As(err, &exh) {
64+
return false // raises the fallback-choice card; the user decides
65+
}
66+
return terminalForFallback(err)
67+
}
4568

4669
func terminalForFallback(err error) bool {
4770
for _, sentinel := range []error{

0 commit comments

Comments
 (0)