Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions src/client.ts
Original file line number Diff line number Diff line change
Expand Up @@ -946,9 +946,9 @@ export class OpenAI {
}
}

// If the API asks us to wait a certain amount of time, just do what it
// says, but otherwise calculate a default
if (timeoutMillis === undefined) {
// If the API asks us to wait a certain amount of time (and it's a reasonable amount),
// just do what it says, but otherwise calculate a default
if (!(timeoutMillis && 0 <= timeoutMillis && timeoutMillis < 60 * 1000)) {
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Treat zero Retry-After as valid delay

The new guard uses timeoutMillis && ..., which makes a parsed Retry-After: 0 (or retry-after-ms: 0) look invalid and fall back to exponential backoff instead of retrying immediately. This changes behavior for explicit zero-delay responses and contradicts the intended [0, 60s) acceptance range mentioned in the commit message; a server that sends 0 will now incur an unnecessary client-side delay.

Useful? React with 👍 / 👎.

const maxRetries = options.maxRetries ?? this.maxRetries;
timeoutMillis = this.calculateDefaultRetryTimeoutMillis(retriesRemaining, maxRetries);
}
Expand Down
43 changes: 43 additions & 0 deletions tests/index.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -764,6 +764,49 @@ describe('retries', () => {
expect(count).toEqual(3);
});

test('caps Retry-After at 60s when server returns a large value', async () => {
// Capture delays passed to setTimeout (which sleep() wraps) and fire callbacks
// immediately so the test doesn't actually wait. We set a small client timeout
// (30s) so the per-request abort watchdog can't be confused with the retry sleep.
const realSetTimeout = globalThis.setTimeout;
const sleepDelays: number[] = [];
const setTimeoutSpy = jest
.spyOn(globalThis, 'setTimeout')
.mockImplementation(((cb: (...args: any[]) => void, ms?: number, ...args: any[]) => {
if (typeof ms === 'number') sleepDelays.push(ms);
return realSetTimeout(cb, 0, ...args);
}) as any);

try {
let count = 0;
const testFetch = async (
url: string | URL | Request,
{ signal }: RequestInit = {},
): Promise<Response> => {
if (count++ === 0) {
return new Response(undefined, {
status: 429,
// 600 seconds == 10 minutes; without a cap the SDK would sleep for the full duration.
headers: { 'Retry-After': '600' },
});
}
return new Response(JSON.stringify({ a: 1 }), { headers: { 'Content-Type': 'application/json' } });
};

const client = new OpenAI({ apiKey: 'My API Key', fetch: testFetch, timeout: 30_000 });

expect(await client.request({ path: '/foo', method: 'get' })).toEqual({ a: 1 });
expect(count).toEqual(2);

// With the cap in place the retry should fall back to calculated backoff
// (<= 8s with jitter); without the cap the largest delay would be 600_000ms.
const longestDelay = Math.max(0, ...sleepDelays);
expect(longestDelay).toBeLessThan(60 * 1000);
} finally {
setTimeoutSpy.mockRestore();
}
});

describe('auth', () => {
test('apiKey', async () => {
const client = new OpenAI({
Expand Down