Skip to content

Commit f9dce8a

Browse files
committed
fix(providers): restore status-based retries to the OpenAI Responses path
`/v1/responses` posted through the OpenAI SDK until 1933e1a (#3135) moved it onto raw `fetch`, which silently dropped the SDK's `maxRetries: 2`. The 16 other providers that construct an SDK client still retry; only openai and azure-openai, which share this core, retried nothing. Restores 2 retries (3 attempts) on 408/409/429/5xx using `backoffWithJitter` and `parseRetryAfter`, preferring OpenAI's `retry-after-ms` over `Retry-After`. The loop sits in the shared request helper so the streaming paths are covered too — a refused request yields no body, so no stream bytes were consumed and no response was created server-side. Aborts, other 4xx, and the body-read deadline stay non-retryable. `/v1/responses` ignores `Idempotency-Key`, so a retry after a response already exists would generate and bill a second one; the body stall is exactly that case.
1 parent 06264c3 commit f9dce8a

2 files changed

Lines changed: 389 additions & 5 deletions

File tree

Lines changed: 270 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,270 @@
1+
/**
2+
* @vitest-environment node
3+
*
4+
* `/v1/responses` is posted with raw `fetch`, which dropped the OpenAI SDK's own
5+
* `maxRetries: 2` when this path moved off the SDK. These cover the restored
6+
* status-based retries — and, just as importantly, the classes that must stay
7+
* non-retryable: a caller abort, and a stalled body, which arrives only after a
8+
* response already exists and would therefore be billed twice.
9+
*/
10+
import { beforeEach, describe, expect, it, vi } from 'vitest'
11+
import { executeResponsesProviderRequest } from '@/providers/openai/core'
12+
import type { ProviderRequest } from '@/providers/types'
13+
14+
vi.mock('@/providers', () => ({ MAX_TOOL_ITERATIONS: 5 }))
15+
16+
vi.mock('@/providers/utils', () => ({
17+
isFunctionToolCall: () => false,
18+
calculateCost: () => ({ input: 0, output: 0, total: 0 }),
19+
sumToolCosts: () => 0,
20+
enforceStrictSchema: (schema: unknown) => schema,
21+
prepareToolExecution: () => ({ toolParams: {}, executionParams: {} }),
22+
prepareToolsWithUsageControl: (tools: unknown[]) => ({
23+
tools,
24+
toolChoice: undefined,
25+
forcedTools: [],
26+
hasFilteredTools: false,
27+
}),
28+
trackForcedToolUsage: () => ({ hasUsedForcedTool: false, usedForcedTools: [] }),
29+
supportsReasoningEffort: () => false,
30+
}))
31+
32+
vi.mock('@/tools', () => ({ executeTool: vi.fn() }))
33+
34+
const COMPLETED_RESPONSE = {
35+
id: 'resp_1',
36+
status: 'completed',
37+
output: [{ type: 'message', role: 'assistant', content: [{ type: 'output_text', text: 'ok' }] }],
38+
usage: { input_tokens: 1, output_tokens: 1, total_tokens: 2 },
39+
}
40+
41+
function okResponse() {
42+
return {
43+
ok: true,
44+
status: 200,
45+
headers: new Headers(),
46+
json: () => Promise.resolve(COMPLETED_RESPONSE),
47+
}
48+
}
49+
50+
function errorResponse(status: number, headers: Record<string, string> = {}) {
51+
return {
52+
ok: false,
53+
status,
54+
headers: new Headers(headers),
55+
text: () => Promise.resolve(JSON.stringify({ error: { message: `boom ${status}` } })),
56+
}
57+
}
58+
59+
/**
60+
* Exactly what the runtime raises when a fetch deadline fires: a `DOMException`,
61+
* NOT a plain `Error`. `DOMException.message` is a readonly getter, so a plain
62+
* `Error` here would not exercise the real failure shape.
63+
*/
64+
function timeoutError() {
65+
return new DOMException('The operation timed out.', 'TimeoutError')
66+
}
67+
68+
/** A 200 whose body never settles until the request's signal aborts. */
69+
function stallingBody(init: RequestInit) {
70+
return {
71+
ok: true,
72+
status: 200,
73+
headers: new Headers(),
74+
json: () =>
75+
new Promise((_resolve, reject) => {
76+
if (init.signal?.aborted) {
77+
reject(timeoutError())
78+
return
79+
}
80+
init.signal?.addEventListener('abort', () => reject(timeoutError()), { once: true })
81+
}),
82+
}
83+
}
84+
85+
describe('OpenAI Responses status retries', () => {
86+
const logger = { info: vi.fn(), warn: vi.fn(), error: vi.fn(), debug: vi.fn() } as never
87+
88+
beforeEach(() => vi.clearAllMocks())
89+
90+
function run(fetchMock: unknown, request: Partial<ProviderRequest> = {}) {
91+
return executeResponsesProviderRequest(
92+
{
93+
apiKey: 'k',
94+
model: 'gpt-5.5',
95+
messages: [{ role: 'user', content: 'hi' }],
96+
workflowId: 'wf_1',
97+
blockId: 'blk_1',
98+
executionId: 'exec_1',
99+
...request,
100+
},
101+
{
102+
providerId: 'openai',
103+
providerLabel: 'OpenAI',
104+
modelName: 'gpt-5.5',
105+
endpoint: 'https://api.openai.com/v1/responses',
106+
headers: { Authorization: 'Bearer k' },
107+
logger,
108+
fetch: fetchMock as typeof fetch,
109+
}
110+
)
111+
}
112+
113+
/** Drives a run to completion under fake timers so backoff costs no wall time. */
114+
async function runWithTimers(fetchMock: unknown, request: Partial<ProviderRequest> = {}) {
115+
vi.useFakeTimers()
116+
try {
117+
const promise = run(fetchMock, request).catch((error: unknown) => error)
118+
await vi.advanceTimersByTimeAsync(120_000)
119+
return await promise
120+
} finally {
121+
vi.useRealTimers()
122+
}
123+
}
124+
125+
it('retries a 429 and then succeeds', async () => {
126+
const fetchMock = vi
127+
.fn()
128+
.mockResolvedValueOnce(errorResponse(429))
129+
.mockResolvedValueOnce(okResponse())
130+
131+
const result = await runWithTimers(fetchMock)
132+
133+
expect(fetchMock).toHaveBeenCalledTimes(2)
134+
expect(result).toMatchObject({ content: 'ok' })
135+
})
136+
137+
it('retries a 500 and then succeeds', async () => {
138+
const fetchMock = vi
139+
.fn()
140+
.mockResolvedValueOnce(errorResponse(500))
141+
.mockResolvedValueOnce(okResponse())
142+
143+
const result = await runWithTimers(fetchMock)
144+
145+
expect(fetchMock).toHaveBeenCalledTimes(2)
146+
expect(result).toMatchObject({ content: 'ok' })
147+
})
148+
149+
it('logs each retry with the attempt, status, delay and correlation ids', async () => {
150+
const fetchMock = vi
151+
.fn()
152+
.mockResolvedValueOnce(errorResponse(503))
153+
.mockResolvedValueOnce(okResponse())
154+
155+
await runWithTimers(fetchMock)
156+
157+
expect(logger.warn).toHaveBeenCalledWith(
158+
expect.stringContaining('retryable status'),
159+
expect.objectContaining({
160+
attempt: 1,
161+
status: 503,
162+
delayMs: expect.any(Number),
163+
workflowId: 'wf_1',
164+
blockId: 'blk_1',
165+
executionId: 'exec_1',
166+
})
167+
)
168+
})
169+
170+
it('does not retry a 400', async () => {
171+
const fetchMock = vi.fn().mockResolvedValue(errorResponse(400))
172+
173+
const error = await runWithTimers(fetchMock)
174+
175+
expect(fetchMock).toHaveBeenCalledTimes(1)
176+
expect((error as Error).message).toContain('boom 400')
177+
})
178+
179+
it('does not retry a 401', async () => {
180+
const fetchMock = vi.fn().mockResolvedValue(errorResponse(401))
181+
182+
const error = await runWithTimers(fetchMock)
183+
184+
expect(fetchMock).toHaveBeenCalledTimes(1)
185+
expect((error as Error).message).toContain('boom 401')
186+
})
187+
188+
it('gives up after the maximum attempts and surfaces the final error', async () => {
189+
const fetchMock = vi.fn().mockResolvedValue(errorResponse(429))
190+
191+
const error = await runWithTimers(fetchMock)
192+
193+
expect(fetchMock).toHaveBeenCalledTimes(3)
194+
expect((error as Error).message).toContain('OpenAI API error (429): boom 429')
195+
})
196+
197+
it('honours Retry-After before re-sending', async () => {
198+
const fetchMock = vi
199+
.fn()
200+
.mockResolvedValueOnce(errorResponse(429, { 'retry-after': '5' }))
201+
.mockResolvedValueOnce(okResponse())
202+
203+
vi.useFakeTimers()
204+
try {
205+
const promise = run(fetchMock).catch((error: unknown) => error)
206+
207+
await vi.advanceTimersByTimeAsync(4_000)
208+
expect(fetchMock).toHaveBeenCalledTimes(1)
209+
210+
await vi.advanceTimersByTimeAsync(1_500)
211+
expect(fetchMock).toHaveBeenCalledTimes(2)
212+
213+
await vi.advanceTimersByTimeAsync(1_000)
214+
await promise
215+
} finally {
216+
vi.useRealTimers()
217+
}
218+
})
219+
220+
it('prefers retry-after-ms over Retry-After', async () => {
221+
const fetchMock = vi
222+
.fn()
223+
.mockResolvedValueOnce(errorResponse(429, { 'retry-after-ms': '8000', 'retry-after': '1' }))
224+
.mockResolvedValueOnce(okResponse())
225+
226+
vi.useFakeTimers()
227+
try {
228+
const promise = run(fetchMock).catch((error: unknown) => error)
229+
230+
await vi.advanceTimersByTimeAsync(7_000)
231+
expect(fetchMock).toHaveBeenCalledTimes(1)
232+
233+
await vi.advanceTimersByTimeAsync(1_500)
234+
expect(fetchMock).toHaveBeenCalledTimes(2)
235+
236+
await vi.advanceTimersByTimeAsync(1_000)
237+
await promise
238+
} finally {
239+
vi.useRealTimers()
240+
}
241+
})
242+
243+
it('does not retry when the caller aborts', async () => {
244+
const caller = new AbortController()
245+
const fetchMock = vi.fn().mockImplementation(() => {
246+
caller.abort(new DOMException('workflow cancelled', 'AbortError'))
247+
return Promise.resolve(errorResponse(429))
248+
})
249+
250+
await runWithTimers(fetchMock, { abortSignal: caller.signal })
251+
252+
expect(fetchMock).toHaveBeenCalledTimes(1)
253+
})
254+
255+
/**
256+
* The double-billing guard. A stalled body means the response was created and
257+
* billed server-side; `/v1/responses` ignores `Idempotency-Key`, so re-sending
258+
* would generate and bill a second one.
259+
*/
260+
it('does not retry a body stall', async () => {
261+
const fetchMock = vi
262+
.fn()
263+
.mockImplementation((_url: string, init: RequestInit) => Promise.resolve(stallingBody(init)))
264+
265+
const error = await runWithTimers(fetchMock)
266+
267+
expect(fetchMock).toHaveBeenCalledTimes(1)
268+
expect((error as Error).message).toContain('phase=reading-response-body')
269+
})
270+
})

0 commit comments

Comments
 (0)