Skip to content

Commit 389e265

Browse files
committed
refactor(session/retry): single-pass classify(); policy() no longer double-calls transportMessage
policy() previously called retryable(error) (which internally called transportMessage) and then called transportMessage(error) again on the next line to decide whether to apply TRANSPORT_RETRY_CAP. Consolidates to a single classify() returning { message, isTransport? }; retryable() becomes a thin wrapper kept to avoid churning the existing retry.test.ts suite. Preserves legacy cap behavior for plain-text errors whose message matches BOTH a rate-limit phrase AND a transport pattern: classify() still picks the rate-limit message text but sets isTransport so policy() applies TRANSPORT_RETRY_CAP as before. Adds retry-classification.test.ts with 7 cases including the mixed rate-limit + ETIMEDOUT regression test. Addresses audit finding F5 (Opus diamond review, 2026-04-22).
1 parent 257bdb2 commit 389e265

2 files changed

Lines changed: 95 additions & 14 deletions

File tree

packages/opencode/src/session/retry.ts

Lines changed: 29 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -62,18 +62,29 @@ export function delay(attempt: number, error?: MessageV2.APIError) {
6262
return cap(Math.min(RETRY_INITIAL_DELAY * Math.pow(RETRY_BACKOFF_FACTOR, attempt - 1), RETRY_MAX_DELAY_NO_HEADERS))
6363
}
6464

65-
export function retryable(error: Err) {
65+
// Branch order matches legacy retryable(): ContextOverflow -> APIError -> plain-text
66+
// rate-limit -> transport -> JSON. An error message like "rate limit exceeded
67+
// (ETIMEDOUT during retry)" must stay classified as rate-limit (not transport)
68+
// for message semantics, but we still honor TRANSPORT_RETRY_CAP via isTransport
69+
// when the message also matches a transport pattern.
70+
export function classify(error: Err) {
6671
// context overflow errors should not be retried
6772
if (MessageV2.ContextOverflowError.isInstance(error)) return undefined
6873
if (MessageV2.APIError.isInstance(error)) {
6974
const status = error.data.statusCode
7075
// 5xx errors are transient server failures and should always be retried,
7176
// even when the provider SDK doesn't explicitly mark them as retryable.
7277
if (!error.data.isRetryable && !(status !== undefined && status >= 500)) return undefined
73-
if (error.data.responseBody?.includes("FreeUsageLimitError")) return GO_UPSELL_MESSAGE
74-
return error.data.message.includes("Overloaded") ? "Provider is overloaded" : error.data.message
78+
if (error.data.responseBody?.includes("FreeUsageLimitError")) {
79+
return { message: GO_UPSELL_MESSAGE }
80+
}
81+
return {
82+
message: error.data.message.includes("Overloaded") ? "Provider is overloaded" : error.data.message,
83+
}
7584
}
7685

86+
const transport = transportMessage(error)
87+
7788
// Check for rate limit patterns in plain text error messages
7889
const msg = error.data?.message
7990
if (typeof msg === "string") {
@@ -83,12 +94,12 @@ export function retryable(error: Err) {
8394
lower.includes("rate limit") ||
8495
lower.includes("too many requests")
8596
) {
86-
return msg
97+
if (transport) return { message: msg, isTransport: true as const }
98+
return { message: msg }
8799
}
88100
}
89101

90-
const transport = transportMessage(error)
91-
if (transport) return transport
102+
if (transport) return { message: transport, isTransport: true as const }
92103

93104
const json = iife(() => {
94105
try {
@@ -106,34 +117,38 @@ export function retryable(error: Err) {
106117
const code = typeof json.code === "string" ? json.code : ""
107118

108119
if (json.type === "error" && json.error?.type === "too_many_requests") {
109-
return "Too Many Requests"
120+
return { message: "Too Many Requests" }
110121
}
111122
if (code.includes("exhausted") || code.includes("unavailable")) {
112-
return "Provider is overloaded"
123+
return { message: "Provider is overloaded" }
113124
}
114125
if (json.type === "error" && typeof json.error?.code === "string" && json.error.code.includes("rate_limit")) {
115-
return "Rate Limited"
126+
return { message: "Rate Limited" }
116127
}
117128
return undefined
118129
}
119130

131+
// Kept to avoid churning the existing retry.test.ts suite. Prefer classify() in new code.
132+
export function retryable(error: Err) {
133+
return classify(error)?.message
134+
}
135+
120136
export function policy(opts: {
121137
parse: (error: unknown) => Err
122138
set: (input: { attempt: number; message: string; next: number }) => Effect.Effect<void>
123139
}) {
124140
return Schedule.fromStepWithMetadata(
125141
Effect.succeed((meta: Schedule.InputMetadata<unknown>) => {
126142
const error = opts.parse(meta.input)
127-
const message = retryable(error)
128-
const transport = transportMessage(error)
129-
if (!message) return Cause.done(meta.attempt)
130-
if (transport && !MessageV2.APIError.isInstance(error) && meta.attempt > TRANSPORT_RETRY_CAP) {
143+
const c = classify(error)
144+
if (!c) return Cause.done(meta.attempt)
145+
if (c.isTransport && !MessageV2.APIError.isInstance(error) && meta.attempt > TRANSPORT_RETRY_CAP) {
131146
return Cause.done(meta.attempt)
132147
}
133148
return Effect.gen(function* () {
134149
const wait = delay(meta.attempt, MessageV2.APIError.isInstance(error) ? error : undefined)
135150
const now = yield* Clock.currentTimeMillis
136-
yield* opts.set({ attempt: meta.attempt, message, next: now + wait })
151+
yield* opts.set({ attempt: meta.attempt, message: c.message, next: now + wait })
137152
return [meta.attempt, Duration.millis(wait)] as [number, Duration.Duration]
138153
})
139154
}),
Lines changed: 66 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,66 @@
1+
import { describe, expect, test } from "bun:test"
2+
import { MessageV2 } from "../../src/session/message-v2"
3+
import { SessionRetry } from "../../src/session/retry"
4+
5+
describe("SessionRetry.classify", () => {
6+
test("returns undefined for non-retryable context overflow", () => {
7+
const err = new MessageV2.ContextOverflowError({ message: "too long" }).toObject()
8+
expect(SessionRetry.classify(err)).toBeUndefined()
9+
})
10+
11+
test("returns transport classification for SSEStallError", () => {
12+
const err = new MessageV2.SSEStallError({ message: "SSE read timed out after 120000ms" }).toObject()
13+
const out = SessionRetry.classify(err)
14+
expect(out).toEqual({ message: "SSE read timed out after 120000ms", isTransport: true })
15+
})
16+
17+
test("returns non-transport classification for APIError 500", () => {
18+
const err = new MessageV2.APIError({
19+
message: "upstream exploded",
20+
statusCode: 503,
21+
isRetryable: false,
22+
}).toObject()
23+
const out = SessionRetry.classify(err)
24+
expect(out?.message).toBe("upstream exploded")
25+
expect(out?.isTransport).toBeFalsy()
26+
})
27+
28+
test("retryable() still returns a string for transport errors (legacy API)", () => {
29+
const err = new MessageV2.SSEStallError({ message: "SSE read timed out after 120000ms" }).toObject()
30+
expect(SessionRetry.retryable(err)).toBe("SSE read timed out after 120000ms")
31+
})
32+
33+
test("classifies plain-text ETIMEDOUT transport error as isTransport", () => {
34+
const err = {
35+
_tag: "Error",
36+
name: "UnknownError",
37+
data: { message: "connect ETIMEDOUT 1.2.3.4:443" },
38+
} as unknown as Parameters<typeof SessionRetry.classify>[0]
39+
const out = SessionRetry.classify(err)
40+
expect(out).toEqual({ message: "connect ETIMEDOUT 1.2.3.4:443", isTransport: true })
41+
})
42+
43+
test("mixed rate-limit + transport message keeps rate-limit message AND transport cap", () => {
44+
// Regression: legacy policy() treated any message matching a TRANSPORT_PATTERN
45+
// as transport for cap purposes, even when the rate-limit branch picked the
46+
// message. Preserve that cap behavior via isTransport.
47+
const err = {
48+
_tag: "Error",
49+
name: "UnknownError",
50+
data: { message: "rate limit exceeded (ETIMEDOUT during retry)" },
51+
} as unknown as Parameters<typeof SessionRetry.classify>[0]
52+
const out = SessionRetry.classify(err)
53+
expect(out?.message).toBe("rate limit exceeded (ETIMEDOUT during retry)")
54+
expect(out?.isTransport).toBe(true)
55+
})
56+
57+
test("APIError with overloaded message returns non-transport", () => {
58+
const err = new MessageV2.APIError({
59+
message: "Overloaded",
60+
statusCode: 529,
61+
isRetryable: true,
62+
}).toObject()
63+
const out = SessionRetry.classify(err)
64+
expect(out).toEqual({ message: "Provider is overloaded" })
65+
})
66+
})

0 commit comments

Comments
 (0)