From 66ba6bcdb3685da084002c6f605985857c7423a8 Mon Sep 17 00:00:00 2001 From: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com> Date: Mon, 19 Oct 2026 18:27:00 +0000 Subject: [PATCH 1/3] feat(sdk): retry control-plane requests only on Should-Retry: true Co-Authored-By: mish@e2b.dev --- .changeset/should-retry-header.md | 9 + packages/js-sdk/src/connectionConfig.ts | 19 +- packages/js-sdk/src/retry.ts | 74 +--- packages/js-sdk/tests/client.test.ts | 9 +- packages/js-sdk/tests/retry.test.ts | 280 ++++++------- packages/python-sdk/e2b/connection_config.py | 16 +- packages/python-sdk/e2b/retry.py | 99 +---- .../tests/test_api_client_transport.py | 2 + .../tests/test_rate_limit_retry_transport.py | 393 +++++++----------- 9 files changed, 340 insertions(+), 561 deletions(-) create mode 100644 .changeset/should-retry-header.md diff --git a/.changeset/should-retry-header.md b/.changeset/should-retry-header.md new file mode 100644 index 0000000000..72db32484e --- /dev/null +++ b/.changeset/should-retry-header.md @@ -0,0 +1,9 @@ +--- +'e2b': minor +'@e2b/python-sdk': minor +--- + +Retry control-plane API errors only when the response carries `Should-Retry: true` (honoring `Retry-After`, otherwise exponential backoff) + +- Removes the client-side retry allowlists (status codes, methods, replayable operations) +- Responses without `Should-Retry: true` (including 429/502/503) and network errors after the request was sent are no longer retried; connection failures still are diff --git a/packages/js-sdk/src/connectionConfig.ts b/packages/js-sdk/src/connectionConfig.ts index 6be84b7ffb..dbfdb1666b 100644 --- a/packages/js-sdk/src/connectionConfig.ts +++ b/packages/js-sdk/src/connectionConfig.ts @@ -100,21 +100,16 @@ export interface ConnectionOpts { */ requestTimeoutMs?: number /** - * Number of control-plane API retries after a 429, 502 or 503 response or - * a failure to establish the connection (refused, DNS, unreachable host). - * Any other network error (dropped connection, opaque browser/Workers - * `TypeError`) is retried too. A 502 and those network errors are not - * retried for operations that create a resource (e.g. sandbox creation) or - * append to one (secret update), as the server may already have processed - * them. - * A 429 is retried only with a valid, non-negative integer delta-seconds - * `Retry-After` header (HTTP-date and malformed values are not retried). - * 502 and 503 honor such a `Retry-After` when present; otherwise they and - * connection failures use exponential backoff with jitter starting at - * 100 ms (capped at 10 s). + * Number of control-plane API retries for responses marked `Should-Retry: true`. + * Waits for a valid `Retry-After` delta-seconds value, otherwise exponential + * backoff with jitter from 100 ms to 10 s. Also retries failures to establish + * the connection; errors after the request is sent are not retried. + * A retry is skipped when its wait would exhaust the request timeout. * Retry waits use a 60-second total limit when request timeouts are disabled. + * Set to `0` to disable retries. Defaults to 3 retries. * * @default 3 + * @throws InvalidArgumentError if the value is negative or not an integer */ retries?: number /** diff --git a/packages/js-sdk/src/retry.ts b/packages/js-sdk/src/retry.ts index 903edce8bb..3d803493a9 100644 --- a/packages/js-sdk/src/retry.ts +++ b/packages/js-sdk/src/retry.ts @@ -1,12 +1,12 @@ import { InvalidArgumentError } from './errors' import { isReadableStreamLike } from './is' +// Retries API responses only when the server marks them `Should-Retry: true`. const MAX_RETRY_AFTER_SECONDS = 2_147_483 const MAX_RETRY_WAIT_WITHOUT_TIMEOUT_MS = 60_000 const BACKOFF_BASE_MS = 100 const BACKOFF_MAX_MS = 10_000 const BACKOFF_JITTER_MIN = 0.5 -const RETRYABLE_STATUSES = new Set([429, 502, 503]) // Error codes raised while establishing the connection, before any request // bytes are written: Node/undici (`ECONNREFUSED`, `ENOTFOUND`, ...), Bun // (`ConnectionRefused`). Post-write failures such as `ECONNRESET` are @@ -29,25 +29,6 @@ const DENO_CONNECTION_ERROR = /client error \(Connect\)/ // Chrome `Failed to fetch`, Firefox `NetworkError when attempting to fetch resource.`, Safari `Load failed` const OPAQUE_NETWORK_ERROR = /^(Failed to fetch|Load failed|NetworkError when attempting to fetch resource\.?)$/ -// GET/PUT/PATCH/DELETE are idempotent by HTTP semantics. A POST is retried -// after a 502 or a network error that may have occurred once the request was -// written only if it is listed here: a replay is a no-op or fails with a -// 404/409 the SDK already tolerates. POSTs that mint a resource without a -// client-supplied idempotency key (sandbox/fork/snapshot/API-key/volume/secret/ -// webhook creation) or append to one (secret update) must stay off the list — -// a replay could create a duplicate — and so does any new POST until it is -// reviewed. Connection-establishment failures -// are retried for every operation regardless: the request never left. -const REPLAYABLE_METHODS = new Set(['GET', 'PUT', 'PATCH', 'DELETE']) -const REPLAYABLE_OPERATIONS: [method: string, path: RegExp][] = [ - ['POST', /^\/sandboxes\/[^/]+\/(pause|resume|timeout|refreshes)$/], - ['POST', /^\/(v2\/)?sandboxes\/[^/]+\/connect$/], - ['POST', /^\/v3\/templates$/], - ['POST', /^\/v2\/templates\/[^/]+\/builds\/[^/]+$/], - ['POST', /^\/templates\/tags$/], - ['POST', /^\/nodes\/[^/]+$/], - ['POST', /^\/admin\/teams\/[^/]+\/(sandboxes\/kill|builds\/cancel)$/], -] export function resolveRetries(retries: number): number { if (!Number.isInteger(retries) || retries < 0) { @@ -104,31 +85,12 @@ export function isNetworkError(error: unknown): boolean { } /** - * Whether `request` may be sent again after a 502 or a network error that may - * have occurred once the request was written (see `REPLAYABLE_OPERATIONS`). + * Whether a `fetch` rejection is a connection-establishment failure. Aborts + * and failures after the request may have been sent are not retried. */ -export function isReplayable(request: Request): boolean { - if (REPLAYABLE_METHODS.has(request.method)) return true - const { pathname } = new URL(request.url) - return REPLAYABLE_OPERATIONS.some( - ([method, path]) => request.method === method && path.test(pathname) - ) -} - -/** - * Whether a `fetch` rejection may be retried. Connection-establishment - * failures are always retried: the request never left. For a replayable - * request any other network error is retried as well — a connection dropped - * mid-request, or the opaque `TypeError` browsers and Cloudflare Workers raise - * for every network failure. Aborts are never retried. - */ -export function isRetryableFetchError( - error: unknown, - replayable: boolean -): boolean { +export function isRetryableFetchError(error: unknown): boolean { if (error instanceof DOMException) return false - if (isConnectionError(error)) return true - return replayable && error instanceof Error + return isConnectionError(error) } type RetryDependencies = { @@ -160,34 +122,25 @@ function backoffMs(attempt: number, random: () => number): number { ) } -/** - * Delay before the next attempt, or `undefined` when the response is not - * retried: `Retry-After` when the server sends a usable one, otherwise - * exponential backoff with jitter for 502/503. A 429 without `Retry-After` - * is not retried, nor is a 502 for a non-replayable request: a gateway may - * answer 502 after the backend already processed it. - */ +/** Delay before the next attempt, or `undefined` unless `Should-Retry` is true. */ function retryDelayMs( response: Response, attempt: number, - replayable: boolean, random: () => number ): number | undefined { - if (!RETRYABLE_STATUSES.has(response.status)) return undefined - if (response.status === 502 && !replayable) return undefined + if (response.headers.get('Should-Retry')?.trim().toLowerCase() !== 'true') { + return undefined + } const retryAfter = parseRetryAfter(response.headers.get('Retry-After')) if (retryAfter !== undefined) return retryAfter * 1000 - if (response.status === 429) return undefined return backoffMs(attempt, random) } /** - * Retry requests after a 429 carrying `Retry-After`, a 503 or — for replayable - * requests ({@link isReplayable}) — a 502 (using `Retry-After` when present, - * exponential backoff otherwise), or a network failure (exponential backoff; - * see {@link isRetryableFetchError}). + * Retries API responses marked `Should-Retry: true` after `Retry-After` or + * exponential backoff, and retries connection-establishment failures. */ export function withRetry( fetchImpl: typeof fetch, @@ -216,7 +169,6 @@ export function withRetry( : new Request(input, init) const deadline = monotonic() + (requestTimeoutMs || MAX_RETRY_WAIT_WITHOUT_TIMEOUT_MS) - const replayable = isReplayable(request) for (let attempt = 0; ; attempt++) { let response: Response @@ -228,7 +180,7 @@ export function withRetry( if ( attempt === retries || request.signal.aborted || - !isRetryableFetchError(error, replayable) + !isRetryableFetchError(error) ) { throw error } @@ -241,7 +193,7 @@ export function withRetry( } if (attempt === retries) return response - const delayMs = retryDelayMs(response, attempt, replayable, random) + const delayMs = retryDelayMs(response, attempt, random) if (delayMs === undefined || monotonic() + delayMs >= deadline) { return response } diff --git a/packages/js-sdk/tests/client.test.ts b/packages/js-sdk/tests/client.test.ts index 4c96db88bc..7f4b0499a6 100644 --- a/packages/js-sdk/tests/client.test.ts +++ b/packages/js-sdk/tests/client.test.ts @@ -176,7 +176,7 @@ test('client retries rate-limited control-plane requests', async () => { if (attempts === 1) { return new HttpResponse(null, { status: 429, - headers: { 'Retry-After': '0' }, + headers: { 'Should-Retry': 'true', 'Retry-After': '0' }, }) } return HttpResponse.json([]) @@ -200,7 +200,7 @@ test('client replays serialized control-plane JSON after a rate limit', async () if (bodies.length === 1) { return new HttpResponse(null, { status: 429, - headers: { 'Retry-After': '0' }, + headers: { 'Should-Retry': 'true', 'Retry-After': '0' }, }) } return HttpResponse.json(sandboxResponse) @@ -223,7 +223,10 @@ test('client retries sandbox creation after a 503', async () => { if (attempts === 1) { return HttpResponse.json( { code: 503, message: 'not enough capacity' }, - { status: 503, headers: { 'Retry-After': '0' } } + { + status: 503, + headers: { 'Should-Retry': 'true', 'Retry-After': '0' }, + } ) } return HttpResponse.json(sandboxResponse) diff --git a/packages/js-sdk/tests/retry.test.ts b/packages/js-sdk/tests/retry.test.ts index 7f43e9f9e7..cc30c5cedc 100644 --- a/packages/js-sdk/tests/retry.test.ts +++ b/packages/js-sdk/tests/retry.test.ts @@ -2,7 +2,6 @@ import { describe, expect, test, vi } from 'vitest' import { isConnectionError, - isReplayable, isRetryableFetchError, parseRetryAfter, resolveRetries, @@ -46,7 +45,7 @@ test('retries a buffered request and cancels the intermediate response', async ( const bodies: string[] = [] const rateLimited = new Response('rate limited', { status: 429, - headers: { 'Retry-After': '2' }, + headers: { 'Should-Retry': 'true', 'Retry-After': '2' }, }) const fetchImpl = vi.fn(async (input: RequestInfo | URL) => { bodies.push(await (input as Request).text()) @@ -89,7 +88,7 @@ test.each(['', '{"templateID":"base"}'])( bodies.push(await attempt.text()) return new Response(null, { status: attempts.length < 3 ? 429 : 200, - headers: { 'Retry-After': '0' }, + headers: { 'Should-Retry': 'true', 'Retry-After': '0' }, }) }) as typeof fetch @@ -115,7 +114,7 @@ test('returns the final 429 after exhausting retries', async () => { const fetchImpl = vi.fn(async () => { return new Response(null, { status: 429, - headers: { 'Retry-After': '0' }, + headers: { 'Should-Retry': 'true', 'Retry-After': '0' }, }) }) as typeof fetch const fetchWithRetry = withRetry(fetchImpl, 2, 10_000, { @@ -133,7 +132,7 @@ test('propagates 429 when Retry-After exceeds the request timeout', async () => const fetchImpl = vi.fn(async () => { return new Response(null, { status: 429, - headers: { 'Retry-After': '2' }, + headers: { 'Should-Retry': 'true', 'Retry-After': '2' }, }) }) as typeof fetch const sleep = vi.fn(async () => {}) @@ -154,7 +153,7 @@ test('bounds retry waits when the request timeout is disabled', async () => { async () => new Response(null, { status: 429, - headers: { 'Retry-After': '60' }, + headers: { 'Should-Retry': 'true', 'Retry-After': '60' }, }) ) as typeof fetch const sleep = vi.fn(async () => {}) @@ -187,13 +186,42 @@ test('propagates 429 without Retry-After as is', async () => { expect(sleep).not.toHaveBeenCalled() }) +test.each([ + { status: 503, headers: { 'Should-Retry': 'false' } }, + { status: 429, headers: { 'Retry-After': '2' } }, + { status: 502, headers: {} }, + { status: 503, headers: {} }, +])( + 'does not retry an unmarked $status response', + async ({ status, headers }) => { + const failed = new Response('error', { status, headers }) + const fetchImpl = vi.fn(async () => failed) as typeof fetch + const sleep = vi.fn(async () => {}) + const fetchWithRetry = withRetry(fetchImpl, 3, 10_000, { + monotonic: () => 0, + sleep, + }) + + const response = await fetchWithRetry('https://api.e2b.test/resource') + + expect(response).toBe(failed) + expect(response.bodyUsed).toBe(false) + expect(fetchImpl).toHaveBeenCalledOnce() + expect(sleep).not.toHaveBeenCalled() + } +) + test.each([502, 503])( 'retries a %s with exponential backoff and jitter', async (status) => { const statuses = [status, status, status, 200] - const fetchImpl = vi.fn( - async () => new Response(null, { status: statuses.shift() }) - ) as typeof fetch + const fetchImpl = vi.fn(async () => { + const responseStatus = statuses.shift()! + return new Response(null, { + status: responseStatus, + headers: responseStatus === 200 ? {} : { 'Should-Retry': 'true' }, + }) + }) as typeof fetch const sleep = vi.fn(async () => {}) const random = vi .fn<() => number>() @@ -214,34 +242,67 @@ test.each([502, 503])( } ) +test.each(['TRUE', ' true '])( + 'retries a response with Should-Retry: %j', + async (shouldRetry) => { + const fetchImpl = vi + .fn() + .mockResolvedValueOnce( + new Response(null, { + status: 503, + headers: { 'Should-Retry': shouldRetry }, + }) + ) + .mockResolvedValueOnce(new Response('ok')) as typeof fetch + const sleep = vi.fn(async () => {}) + + const response = await withRetry(fetchImpl, 1, 10_000, { + monotonic: () => 0, + sleep, + })('https://api.e2b.test/resource') + + expect(response.status).toBe(200) + expect(fetchImpl).toHaveBeenCalledTimes(2) + } +) + test.each([ - ['POST', '/sandboxes', 1], - ['POST', '/secrets/secret-1', 1], - ['POST', '/sandboxes/sbx-1/pause', 2], - ['GET', '/sandboxes', 2], -])('a 502 for %s %s is attempted %i time(s)', async (method, path, calls) => { - const statuses = [502, 200] - const fetchImpl = vi.fn( - async () => new Response(null, { status: statuses.shift() }) - ) as typeof fetch - const fetchWithRetry = withRetry(fetchImpl, 3, 60_000, { - monotonic: () => 0, - sleep: async () => {}, - }) + ['POST', '/v2/sandboxes', 500], + ['GET', '/sandboxes', 503], +])( + 'retries a %s for %s when marked by the server', + async (method, path, status) => { + const statuses = [status, 200] + const fetchImpl = vi.fn(async () => { + const responseStatus = statuses.shift()! + return new Response(null, { + status: responseStatus, + headers: responseStatus === 200 ? {} : { 'Should-Retry': 'true' }, + }) + }) as typeof fetch + const fetchWithRetry = withRetry(fetchImpl, 3, 60_000, { + monotonic: () => 0, + sleep: async () => {}, + }) - const response = await fetchWithRetry(`https://api.e2b.test${path}`, { - method, - }) + const response = await fetchWithRetry(`https://api.e2b.test${path}`, { + method, + }) - expect(response.status).toBe(calls === 1 ? 502 : 200) - expect(fetchImpl).toHaveBeenCalledTimes(calls) -}) + expect(response.status).toBe(200) + expect(fetchImpl).toHaveBeenCalledTimes(2) + } +) -test('retries a 503 for a non-replayable POST', async () => { +test('retries a POST when the server marks a 503', async () => { const statuses = [503, 200] - const fetchImpl = vi.fn( - async () => new Response(null, { status: statuses.shift() }) - ) as typeof fetch + const fetchImpl = vi.fn(async () => { + const responseStatus = statuses.shift()! + return new Response(null, { + status: responseStatus, + headers: responseStatus === 200 ? {} : { 'Should-Retry': 'true' }, + }) + }) as typeof fetch const fetchWithRetry = withRetry(fetchImpl, 3, 60_000, { monotonic: () => 0, sleep: async () => {}, @@ -257,7 +318,11 @@ test('retries a 503 for a non-replayable POST', async () => { test('caps the backoff for long retry sequences', async () => { const fetchImpl = vi.fn( - async () => new Response(null, { status: 503 }) + async () => + new Response(null, { + status: 503, + headers: { 'Should-Retry': 'true' }, + }) ) as typeof fetch const sleep = vi.fn(async () => {}) const fetchWithRetry = withRetry(fetchImpl, 8, 0, { @@ -279,7 +344,7 @@ test('prefers Retry-After over backoff for a 503', async () => { async () => new Response(null, { status: statuses.shift(), - headers: { 'Retry-After': '3' }, + headers: { 'Should-Retry': 'true', 'Retry-After': '3' }, }) ) as typeof fetch const sleep = vi.fn(async () => {}) @@ -299,7 +364,11 @@ test('prefers Retry-After over backoff for a 503', async () => { test('returns the final 503 after exhausting retries', async () => { const fetchImpl = vi.fn( - async () => new Response('busy', { status: 503 }) + async () => + new Response('busy', { + status: 503, + headers: { 'Should-Retry': 'true' }, + }) ) as typeof fetch const fetchWithRetry = withRetry(fetchImpl, 2, 10_000, { monotonic: () => 0, @@ -315,7 +384,11 @@ test('returns the final 503 after exhausting retries', async () => { test('propagates 502 when the backoff would exceed the request timeout', async () => { const fetchImpl = vi.fn( - async () => new Response(null, { status: 502 }) + async () => + new Response(null, { + status: 502, + headers: { 'Should-Retry': 'true' }, + }) ) as typeof fetch const sleep = vi.fn(async () => {}) const fetchWithRetry = withRetry(fetchImpl, 3, 80, { @@ -351,7 +424,7 @@ test.each([400, 404, 500, 504])('does not retry a %s', async (status) => { test('aborting during Retry-After sleep rejects promptly', async () => { const rateLimited = new Response('rate limited', { status: 429, - headers: { 'Retry-After': '1' }, + headers: { 'Should-Retry': 'true', 'Retry-After': '1' }, }) const fetchImpl = vi.fn(async () => rateLimited) as typeof fetch const fetchWithRetry = withRetry(fetchImpl, 1, 10_000) @@ -372,7 +445,13 @@ test('aborting during Retry-After sleep rejects promptly', async () => { test.each([0, 3])( 'retries=%s passes a streaming request through unchanged', async (retries) => { - const fetchImpl = vi.fn(async () => new Response('ok')) as typeof fetch + const fetchImpl = vi.fn( + async () => + new Response('unavailable', { + status: 503, + headers: { 'Should-Retry': 'true' }, + }) + ) as typeof fetch const fetchWithRetry = withRetry(fetchImpl, retries, 10_000) const body = new ReadableStream() const init = { @@ -381,8 +460,10 @@ test.each([0, 3])( duplex: 'half' as const, } - await fetchWithRetry('https://api.e2b.test/resource', init) + const response = await fetchWithRetry('https://api.e2b.test/resource', init) + expect(response.status).toBe(503) + expect(fetchImpl).toHaveBeenCalledOnce() expect(fetchImpl).toHaveBeenCalledWith( 'https://api.e2b.test/resource', init @@ -444,9 +525,7 @@ const connectFailures = { ), } -// Connection dropped after the request was (at least partially) written: -// the server may have processed it, so these are replayed only for -// operations that are safe to replay. +// Connection dropped after the request may have been written. const terminated = { Node: new TypeError('terminated'), 'Node reset': new TypeError('fetch failed', { @@ -524,84 +603,25 @@ describe('isConnectionError', () => { }) describe('isRetryableFetchError', () => { - test.each(Object.entries(connectFailures))( - 'retries %s for any operation', - (_, error) => { - expect(isRetryableFetchError(error, true)).toBe(true) - expect(isRetryableFetchError(error, false)).toBe(true) - } - ) + test.each(Object.entries(connectFailures))('retries %s', (_, error) => { + expect(isRetryableFetchError(error)).toBe(true) + }) test.each([...Object.entries(terminated), ...Object.entries(opaque)])( - 'retries %s only for a replayable operation', + 'does not retry %s', (_, error) => { - expect(isRetryableFetchError(error, true)).toBe(true) - expect(isRetryableFetchError(error, false)).toBe(false) + expect(isRetryableFetchError(error)).toBe(false) } ) test.each([...Object.entries(aborts), ['non-error', 'ECONNREFUSED']])( 'never retries %s', (_, error) => { - expect(isRetryableFetchError(error, true)).toBe(false) - expect(isRetryableFetchError(error, false)).toBe(false) + expect(isRetryableFetchError(error)).toBe(false) } ) }) -const nonReplayable = [ - ['POST', '/sandboxes'], - ['POST', '/v2/sandboxes'], - ['POST', '/sandboxes/sbx-1/fork'], - ['POST', '/sandboxes/sbx-1/snapshots'], - ['POST', '/api-keys'], - ['POST', '/admin/teams/team-1/api-keys'], - ['POST', '/volumes'], - ['POST', '/secrets'], - ['POST', '/secrets/secret-1'], - ['POST', '/events/webhooks'], - // POSTs not on the allowlist: unknown and near-miss paths - ['POST', '/sandboxes/sbx-1/pause/extra'], - ['POST', '/sandboxes/a/b/pause'], - ['POST', '/templates/tags/extra'], - ['POST', '/new-resources'], -] - -const replayable = [ - ['GET', '/sandboxes'], - ['GET', '/sandboxes/sbx-1'], - ['DELETE', '/sandboxes/sbx-1'], - ['POST', '/sandboxes/sbx-1/pause'], - ['POST', '/sandboxes/sbx-1/resume'], - ['POST', '/sandboxes/sbx-1/connect'], - ['POST', '/v2/sandboxes/sbx-1/connect'], - ['POST', '/sandboxes/sbx-1/timeout'], - ['POST', '/sandboxes/sbx-1/refreshes'], - ['PUT', '/sandboxes/sbx-1/network'], - ['PATCH', '/templates/tpl-1'], - ['POST', '/v3/templates'], - ['POST', '/v2/templates/tpl-1/builds/build-1'], - ['POST', '/templates/tags'], - ['DELETE', '/templates/tags'], - ['POST', '/nodes/node-1'], - ['POST', '/admin/teams/team-1/sandboxes/kill'], - ['POST', '/admin/teams/team-1/builds/cancel'], - ['DELETE', '/volumes/vol-1'], - ['PATCH', '/events/webhooks/hook-1'], -] - -describe('isReplayable', () => { - test.each(nonReplayable)('%s %s is not replayable', (method, path) => { - const request = new Request(`https://api.e2b.test${path}?x=1`, { method }) - expect(isReplayable(request)).toBe(false) - }) - - test.each(replayable)('%s %s is replayable', (method, path) => { - const request = new Request(`https://api.e2b.test${path}?x=1`, { method }) - expect(isReplayable(request)).toBe(true) - }) -}) - test('retries a connection failure with exponential backoff', async () => { const outcomes = [ () => Promise.reject(connectError('ECONNREFUSED')), @@ -666,43 +686,10 @@ test('rethrows a connection failure when the backoff would exceed the request ti }) test.each([ - ['GET', '/sandboxes'], - ['POST', '/sandboxes/sbx-1/pause'], - ['DELETE', '/sandboxes/sbx-1'], -])('retries an opaque network error for %s %s', async (method, path) => { - const outcomes = [ - () => Promise.reject(opaque.Chrome), - () => Promise.reject(terminated['Node reset']), - () => Promise.resolve(new Response('ok')), - ] - const fetchImpl = vi.fn(() => outcomes.shift()!()) as typeof fetch - const sleep = vi.fn(async () => {}) - const fetchWithRetry = withRetry(fetchImpl, 3, 60_000, { - monotonic: () => 0, - sleep, - random: () => 1, - }) - - const response = await fetchWithRetry(`https://api.e2b.test${path}`, { - method, - }) - - expect(response.status).toBe(200) - expect(fetchImpl).toHaveBeenCalledTimes(3) - expect(sleep.mock.calls.map(([delayMs]) => delayMs)).toEqual([100, 200]) -}) - -test.each([ - ['opaque network error', opaque.Chrome, 'POST', '/sandboxes'], - ['dropped connection', terminated['Node reset'], 'POST', '/v2/sandboxes'], - [ - 'dropped connection', - terminated['Cloudflare Workers'], - 'POST', - '/sandboxes/sbx-1/fork', - ], - ['abort', aborts.abort, 'GET', '/sandboxes'], -])('does not retry %s for %s %s', async (_, error, method, path) => { + ['opaque browser error', opaque.Chrome], + ['post-write connection reset', terminated['Node reset']], + ['abort', aborts.abort], +])('does not retry a POST after %s', async (_, error) => { const fetchImpl = vi.fn(async () => { throw error }) as typeof fetch @@ -712,7 +699,10 @@ test.each([ }) await expect( - fetchWithRetry(`https://api.e2b.test${path}`, { method }) + fetchWithRetry('https://api.e2b.test/v2/sandboxes', { + method: 'POST', + body: 'payload', + }) ).rejects.toBe(error) expect(fetchImpl).toHaveBeenCalledOnce() }) diff --git a/packages/python-sdk/e2b/connection_config.py b/packages/python-sdk/e2b/connection_config.py index 439df3eaab..1129692e56 100644 --- a/packages/python-sdk/e2b/connection_config.py +++ b/packages/python-sdk/e2b/connection_config.py @@ -78,18 +78,10 @@ class ApiParams(TypedDict, total=False): """Timeout for the request in **seconds**, defaults to 60 seconds.""" retries: Optional[int] - """Number of control-plane HTTP retries after a 429, 502 or 503 response or - a network error once the request was written (dropped connection). A 502 - and such network errors are not retried for operations that create a - resource (e.g. sandbox creation) or append to one (secret update), as the - server may already have processed them; failures to - establish the connection are retried separately for every operation, see - ``E2B_CONNECTION_RETRIES``. - A 429 is retried only with a valid, non-negative integer delta-seconds - ``Retry-After`` header (HTTP-date and malformed values are not retried). - 502 and 503 honor such a ``Retry-After`` when present; otherwise they and - network errors use exponential backoff with jitter starting at 0.1 seconds - (capped at 10 seconds). + """Number of control-plane retries for responses marked ``Should-Retry: true``. + Waits for valid ``Retry-After`` delta-seconds, otherwise exponential backoff + with jitter from 0.1 to 10 seconds. Connection failures are retried + separately; see ``E2B_CONNECTION_RETRIES``. A retry is skipped when its wait would exhaust the request timeout. Retry waits use a 60-second total limit when request timeouts are disabled. Set to ``0`` to disable retries. Defaults to 3 retries. diff --git a/packages/python-sdk/e2b/retry.py b/packages/python-sdk/e2b/retry.py index 50510fccfa..f76b99bb96 100644 --- a/packages/python-sdk/e2b/retry.py +++ b/packages/python-sdk/e2b/retry.py @@ -1,8 +1,9 @@ +"""Retry API responses marked by the server and leave connection retries below.""" + import asyncio import random -import re import time -from typing import Awaitable, Callable, List, Optional, Tuple +from typing import Awaitable, Callable, Optional import httpx @@ -13,34 +14,6 @@ BACKOFF_BASE_SECONDS = 0.1 BACKOFF_MAX_SECONDS = 10.0 BACKOFF_JITTER_MIN = 0.5 -RETRYABLE_STATUSES = frozenset({429, 502, 503}) - -# GET/PUT/PATCH/DELETE are idempotent by HTTP semantics. A POST is retried -# after a 502 or a network error that may have occurred once the request was -# written only if it is listed here: a replay is a no-op or fails with a -# 404/409 the SDK already tolerates. POSTs that mint a resource without a -# client-supplied idempotency key (sandbox/fork/snapshot/API-key/volume/secret/ -# webhook creation) or append to one (secret update) must stay off the list — -# a replay could create a duplicate — and so does any new POST until it is -# reviewed. (Connection-establishment -# failures are retried for every operation by ``ConnectionRetryTransport`` -# underneath: the request never left.) -REPLAYABLE_METHODS = frozenset({"GET", "PUT", "PATCH", "DELETE"}) -REPLAYABLE_OPERATIONS: List[Tuple[str, re.Pattern[str]]] = [ - ("POST", re.compile(r"^/sandboxes/[^/]+/(pause|resume|timeout|refreshes)$")), - ("POST", re.compile(r"^/(v2/)?sandboxes/[^/]+/connect$")), - ("POST", re.compile(r"^/v3/templates$")), - ("POST", re.compile(r"^/v2/templates/[^/]+/builds/[^/]+$")), - ("POST", re.compile(r"^/templates/tags$")), - ("POST", re.compile(r"^/nodes/[^/]+$")), - ("POST", re.compile(r"^/admin/teams/[^/]+/(sandboxes/kill|builds/cancel)$")), -] - -# Raised by the pyqwest httpx adapter once the request was (at least partially) -# written: the connection dropped while sending it or awaiting the response. -# ``httpx.ConnectError``/``ConnectTimeout`` are excluded — the connection layer -# already retries those — as are timeouts, which exhaust the request's budget. -NETWORK_ERRORS = (httpx.ReadError, httpx.WriteError, httpx.RemoteProtocolError) def resolve_max_retries(retries: int) -> int: @@ -62,18 +35,6 @@ def parse_retry_after(value: Optional[str]) -> Optional[int]: return delay if delay <= MAX_RETRY_AFTER_SECONDS else None -def is_replayable(request: httpx.Request) -> bool: - """Whether ``request`` may be sent again after a 502 or a network error - that may have occurred once it was written (see ``REPLAYABLE_OPERATIONS``).""" - if request.method in REPLAYABLE_METHODS: - return True - path = request.url.path - return any( - request.method == method and pattern.match(path) - for method, pattern in REPLAYABLE_OPERATIONS - ) - - def _backoff_delay(attempt: int, random_: Callable[[], float]) -> float: backoff = min(BACKOFF_BASE_SECONDS * 2**attempt, BACKOFF_MAX_SECONDS) return backoff * (BACKOFF_JITTER_MIN + random_() * (1 - BACKOFF_JITTER_MIN)) @@ -82,24 +43,15 @@ def _backoff_delay(attempt: int, random_: Callable[[], float]) -> float: def _retry_delay( response: httpx.Response, attempt: int, - replayable: bool, random_: Callable[[], float], ) -> Optional[float]: - """Delay before the next attempt, or ``None`` when the response is not - retried: ``Retry-After`` when the server sends a usable one, otherwise - exponential backoff with jitter for 502/503. A 429 without ``Retry-After`` - is not retried, nor is a 502 for a non-replayable request: a gateway may - answer 502 after the backend already processed it.""" - if response.status_code not in RETRYABLE_STATUSES: - return None - if response.status_code == 502 and not replayable: + """Return a delay only when the API marks the response ``Should-Retry``.""" + if response.headers.get("Should-Retry", "").strip().lower() != "true": return None retry_after = parse_retry_after(response.headers.get("Retry-After")) if retry_after is not None: return float(retry_after) - if response.status_code == 429: - return None return _backoff_delay(attempt, random_) @@ -139,10 +91,7 @@ def _request_deadline(request: httpx.Request, monotonic: Callable[[], float]) -> class RetryableTransport(httpx.BaseTransport): - """Retry requests after a 429 carrying ``Retry-After`` or a 503, and — for - replayable operations (``is_replayable``) — after a 502 (using - ``Retry-After`` when present, exponential backoff otherwise) or a network - error once the request was written.""" + """Retry API responses marked ``Should-Retry: true`` using server delay or backoff.""" def __init__( self, @@ -167,7 +116,6 @@ def handle_request(self, request: httpx.Request) -> httpx.Response: request.read() deadline = _request_deadline(request, self._monotonic) - replayable = is_replayable(request) for attempt in range(self.retries + 1): remaining_timeout = None if attempt > 0: @@ -178,22 +126,13 @@ def handle_request(self, request: httpx.Request) -> httpx.Response: "Increase `request_timeout` or lower `retries`.", request=request, ) - try: - response = self.transport.handle_request( - _copy_request(request, remaining_timeout) - ) - except NETWORK_ERRORS: - if attempt == self.retries or not replayable: - raise - delay = _backoff_delay(attempt, self._random) - if self._monotonic() + delay >= deadline: - raise - self._sleep(delay) - continue + response = self.transport.handle_request( + _copy_request(request, remaining_timeout) + ) if attempt == self.retries: return response - delay = _retry_delay(response, attempt, replayable, self._random) + delay = _retry_delay(response, attempt, self._random) if delay is None or self._monotonic() + delay >= deadline: return response @@ -233,7 +172,6 @@ async def handle_async_request(self, request: httpx.Request) -> httpx.Response: await request.aread() deadline = _request_deadline(request, self._monotonic) - replayable = is_replayable(request) for attempt in range(self.retries + 1): remaining_timeout = None if attempt > 0: @@ -244,22 +182,13 @@ async def handle_async_request(self, request: httpx.Request) -> httpx.Response: "Increase `request_timeout` or lower `retries`.", request=request, ) - try: - response = await self.transport.handle_async_request( - _copy_request(request, remaining_timeout) - ) - except NETWORK_ERRORS: - if attempt == self.retries or not replayable: - raise - delay = _backoff_delay(attempt, self._random) - if self._monotonic() + delay >= deadline: - raise - await self._sleep(delay) - continue + response = await self.transport.handle_async_request( + _copy_request(request, remaining_timeout) + ) if attempt == self.retries: return response - delay = _retry_delay(response, attempt, replayable, self._random) + delay = _retry_delay(response, attempt, self._random) if delay is None or self._monotonic() + delay >= deadline: return response diff --git a/packages/python-sdk/tests/test_api_client_transport.py b/packages/python-sdk/tests/test_api_client_transport.py index 6b9b744b9d..c29d0cce78 100644 --- a/packages/python-sdk/tests/test_api_client_transport.py +++ b/packages/python-sdk/tests/test_api_client_transport.py @@ -543,6 +543,7 @@ def do_GET(self): type(self).rate_limit_requests += 1 if type(self).rate_limit_requests == 1: self.send_response(429) + self.send_header("Should-Retry", "true") self.send_header("Retry-After", "0") self.send_header("Content-Length", "0") self.end_headers() @@ -566,6 +567,7 @@ def do_POST(self): type(self).rate_limit_bodies.append(received) if len(type(self).rate_limit_bodies) == 1: self.send_response(429) + self.send_header("Should-Retry", "true") self.send_header("Retry-After", "0") self.send_header("Content-Length", "0") self.end_headers() diff --git a/packages/python-sdk/tests/test_rate_limit_retry_transport.py b/packages/python-sdk/tests/test_rate_limit_retry_transport.py index c4699a8b72..9513562c78 100644 --- a/packages/python-sdk/tests/test_rate_limit_retry_transport.py +++ b/packages/python-sdk/tests/test_rate_limit_retry_transport.py @@ -4,15 +4,15 @@ from e2b.retry import ( AsyncRetryableTransport, RetryableTransport, - is_replayable, parse_retry_after, ) class FakeTransport(httpx.BaseTransport): - def __init__(self, outcomes, retry_after="0"): + def __init__(self, outcomes, retry_after="0", should_retry="false"): self.outcomes = iter(outcomes) self.retry_after = retry_after + self.should_retry = should_retry self.requests = [] self.responses = [] self.closed = False @@ -22,9 +22,14 @@ def handle_request(self, request): outcome = next(self.outcomes) if isinstance(outcome, Exception): raise outcome + headers = {} + if outcome >= 400 and self.should_retry is not None: + headers["Should-Retry"] = self.should_retry + if outcome >= 400 and self.retry_after is not None: + headers["Retry-After"] = self.retry_after response = httpx.Response( outcome, - headers={"Retry-After": self.retry_after} if self.retry_after else {}, + headers=headers, content=b"response", request=request, ) @@ -36,9 +41,10 @@ def close(self): class FakeAsyncTransport(httpx.AsyncBaseTransport): - def __init__(self, outcomes, retry_after="0"): + def __init__(self, outcomes, retry_after="0", should_retry="false"): self.outcomes = iter(outcomes) self.retry_after = retry_after + self.should_retry = should_retry self.requests = [] self.responses = [] self.closed = False @@ -48,9 +54,14 @@ async def handle_async_request(self, request): outcome = next(self.outcomes) if isinstance(outcome, Exception): raise outcome + headers = {} + if outcome >= 400 and self.should_retry is not None: + headers["Should-Retry"] = self.should_retry + if outcome >= 400 and self.retry_after is not None: + headers["Retry-After"] = self.retry_after response = httpx.Response( outcome, - headers={"Retry-After": self.retry_after} if self.retry_after else {}, + headers=headers, content=b"response", request=request, ) @@ -75,7 +86,7 @@ def sleep(self, delay): def test_retries_rate_limit_after_server_delay_and_replays_body(): - inner = FakeTransport([429, 200], retry_after="2") + inner = FakeTransport([429, 200], retry_after="2", should_retry="true") sleeps = [] request = httpx.Request("POST", "https://api.test", content=b"payload") @@ -90,7 +101,7 @@ def test_retries_rate_limit_after_server_delay_and_replays_body(): def test_exhaustion_returns_final_rate_limit_response(): - inner = FakeTransport([429, 429, 429]) + inner = FakeTransport([429, 429, 429], should_retry="true") response = RetryableTransport( inner, retries=2, sleep=lambda _: None @@ -103,15 +114,17 @@ def test_exhaustion_returns_final_rate_limit_response(): @pytest.mark.parametrize("retry_after", ["soon", "-1", "+1", "1.5"]) -def test_does_not_retry_without_delta_seconds(retry_after): - inner = FakeTransport([429], retry_after=retry_after) +def test_invalid_retry_after_falls_back_to_backoff(retry_after): + inner = FakeTransport([503, 200], retry_after=retry_after, should_retry="true") + sleeps = [] - response = RetryableTransport(inner, retries=3).handle_request( - httpx.Request("GET", "https://api.test") - ) + response = RetryableTransport( + inner, retries=3, sleep=sleeps.append, random_=lambda: 0.0 + ).handle_request(httpx.Request("GET", "https://api.test")) - assert response.status_code == 429 - assert len(inner.requests) == 1 + assert response.status_code == 200 + assert len(inner.requests) == 2 + assert sleeps == pytest.approx([0.05]) def test_429_without_retry_after_is_propagated_as_is(): @@ -126,7 +139,7 @@ def test_429_without_retry_after_is_propagated_as_is(): def test_retry_after_exceeding_timeout_is_propagated_as_is(): - inner = FakeTransport([429], retry_after="4") + inner = FakeTransport([429], retry_after="4", should_retry="true") sleeps = [] request = httpx.Request( "GET", @@ -144,7 +157,7 @@ def test_retry_after_exceeding_timeout_is_propagated_as_is(): def test_retry_wait_is_bounded_when_request_timeout_is_disabled(): - inner = FakeTransport([429], retry_after="60") + inner = FakeTransport([429], retry_after="60", should_retry="true") sleeps = [] request = httpx.Request( "GET", @@ -163,9 +176,17 @@ def test_retry_wait_is_bounded_when_request_timeout_is_disabled(): assert sleeps == [] -@pytest.mark.parametrize("status", [400, 404, 500, 504]) -def test_does_not_retry_other_status_codes(status): - inner = FakeTransport([status], retry_after="0") +@pytest.mark.parametrize( + ("status", "retry_after", "should_retry"), + [ + (503, "0", "false"), + (429, "2", None), + (502, "0", None), + (503, "0", None), + ], +) +def test_does_not_retry_unmarked_responses(status, retry_after, should_retry): + inner = FakeTransport([status], retry_after=retry_after, should_retry=should_retry) response = RetryableTransport(inner, retries=3).handle_request( httpx.Request("GET", "https://api.test") @@ -177,7 +198,9 @@ def test_does_not_retry_other_status_codes(status): @pytest.mark.parametrize("status", [502, 503]) def test_retries_unavailable_status_with_exponential_backoff_and_jitter(status): - inner = FakeTransport([status, status, status, 200], retry_after=None) + inner = FakeTransport( + [status, status, status, 200], retry_after=None, should_retry="true" + ) sleeps = [] randoms = iter([0.0, 1.0, 0.5]) request = httpx.Request( @@ -198,39 +221,46 @@ def test_retries_unavailable_status_with_exponential_backoff_and_jitter(status): assert all(item.is_closed for item in inner.responses[:-1]) -@pytest.mark.parametrize( - ("method", "path", "attempts"), - [ - ("POST", "/sandboxes", 1), - ("POST", "/secrets/secret-1", 1), - ("POST", "/sandboxes/sbx-1/pause", 2), - ("GET", "/sandboxes", 2), - ], -) -def test_bad_gateway_is_retried_only_for_replayable_operations(method, path, attempts): - inner = FakeTransport([502, 200], retry_after=None) +def test_retries_post_500_when_marked_by_server(): + inner = FakeTransport([500, 200], retry_after=None, should_retry="true") response = RetryableTransport( inner, retries=3, sleep=lambda _: None, monotonic=lambda: 0.0 - ).handle_request(httpx.Request(method, f"https://api.test{path}", content=b"p")) + ).handle_request( + httpx.Request("POST", "https://api.test/v2/sandboxes", content=b"p") + ) - assert response.status_code == (502 if attempts == 1 else 200) - assert len(inner.requests) == attempts + assert response.status_code == 200 + assert len(inner.requests) == 2 -def test_service_unavailable_is_retried_for_non_replayable_operation(): - inner = FakeTransport([503, 200], retry_after=None) +def test_retries_post_503_when_marked_by_server(): + inner = FakeTransport([503, 200], retry_after=None, should_retry="true") response = RetryableTransport( inner, retries=3, sleep=lambda _: None, monotonic=lambda: 0.0 - ).handle_request(httpx.Request("POST", "https://api.test/sandboxes", content=b"p")) + ).handle_request( + httpx.Request("POST", "https://api.test/v2/sandboxes", content=b"p") + ) + + assert response.status_code == 200 + assert len(inner.requests) == 2 + + +@pytest.mark.parametrize("should_retry", ["TRUE", " true "]) +def test_should_retry_header_is_case_and_whitespace_insensitive(should_retry): + inner = FakeTransport([503, 200], should_retry=should_retry) + + response = RetryableTransport( + inner, retries=1, sleep=lambda _: None, monotonic=lambda: 0.0 + ).handle_request(httpx.Request("GET", "https://api.test")) assert response.status_code == 200 assert len(inner.requests) == 2 def test_backoff_is_capped_for_long_retry_sequences(): - inner = FakeTransport([503] * 9, retry_after=None) + inner = FakeTransport([503] * 9, retry_after=None, should_retry="true") sleeps = [] request = httpx.Request( "GET", @@ -250,7 +280,7 @@ def test_backoff_is_capped_for_long_retry_sequences(): def test_unavailable_status_prefers_retry_after_over_backoff(): - inner = FakeTransport([503, 200], retry_after="3") + inner = FakeTransport([503, 200], retry_after="3", should_retry="true") sleeps = [] def random_(): @@ -265,7 +295,7 @@ def random_(): def test_unavailable_status_exhaustion_returns_final_response(): - inner = FakeTransport([503, 503, 503], retry_after=None) + inner = FakeTransport([503, 503, 503], retry_after=None, should_retry="true") response = RetryableTransport( inner, retries=2, sleep=lambda _: None, random_=lambda: 0.0 @@ -277,7 +307,7 @@ def test_unavailable_status_exhaustion_returns_final_response(): def test_backoff_exceeding_timeout_is_propagated_as_is(): - inner = FakeTransport([502], retry_after=None) + inner = FakeTransport([502], retry_after=None, should_retry="true") sleeps = [] request = httpx.Request( "GET", @@ -298,7 +328,7 @@ def test_backoff_exceeding_timeout_is_propagated_as_is(): def test_does_not_retry_when_cumulative_wait_reaches_request_timeout(): - inner = FakeTransport([429, 429], retry_after="2") + inner = FakeTransport([429, 429], retry_after="2", should_retry="true") clock = FakeClock() request = httpx.Request( "GET", @@ -321,7 +351,7 @@ def test_does_not_retry_when_cumulative_wait_reaches_request_timeout(): def test_timeout_after_retry_wait_names_configuration_options(): - inner = FakeTransport([429]) + inner = FakeTransport([429], should_retry="true") clock = FakeClock() def sleep(delay): @@ -353,7 +383,7 @@ def handle_request(self, request): clock.now = 2.0 return response - inner = AdvancingTransport([429, 200]) + inner = AdvancingTransport([429, 200], should_retry="true") request = httpx.Request( "GET", "https://api.test", @@ -381,7 +411,7 @@ def handle_request(self, request): clock.now = 2.0 return response - inner = AdvancingTransport([429, 200]) + inner = AdvancingTransport([429, 200], should_retry="true") request = httpx.Request( "GET", "https://api.test", @@ -417,7 +447,7 @@ class Stream(httpx.SyncByteStream): def __iter__(self): yield b"payload" - inner = FakeTransport([429]) + inner = FakeTransport([429], should_retry="true") request = httpx.Request("POST", "https://api.test", content=Stream()) response = RetryableTransport(inner, retries=3).handle_request(request) @@ -434,95 +464,6 @@ def test_close_leaves_cached_transport_open(): assert not inner.closed -NON_REPLAYABLE = [ - ("POST", "/sandboxes"), - ("POST", "/v2/sandboxes"), - ("POST", "/sandboxes/sbx-1/fork"), - ("POST", "/sandboxes/sbx-1/snapshots"), - ("POST", "/api-keys"), - ("POST", "/admin/teams/team-1/api-keys"), - ("POST", "/volumes"), - ("POST", "/secrets"), - ("POST", "/secrets/secret-1"), - ("POST", "/events/webhooks"), - # POSTs not on the allowlist: unknown and near-miss paths - ("POST", "/sandboxes/sbx-1/pause/extra"), - ("POST", "/sandboxes/a/b/pause"), - ("POST", "/templates/tags/extra"), - ("POST", "/new-resources"), -] - -REPLAYABLE = [ - ("GET", "/sandboxes"), - ("GET", "/sandboxes/sbx-1"), - ("DELETE", "/sandboxes/sbx-1"), - ("POST", "/sandboxes/sbx-1/pause"), - ("POST", "/sandboxes/sbx-1/resume"), - ("POST", "/sandboxes/sbx-1/connect"), - ("POST", "/v2/sandboxes/sbx-1/connect"), - ("POST", "/sandboxes/sbx-1/timeout"), - ("POST", "/sandboxes/sbx-1/refreshes"), - ("PUT", "/sandboxes/sbx-1/network"), - ("PATCH", "/templates/tpl-1"), - ("POST", "/v3/templates"), - ("POST", "/v2/templates/tpl-1/builds/build-1"), - ("POST", "/templates/tags"), - ("DELETE", "/templates/tags"), - ("POST", "/nodes/node-1"), - ("POST", "/admin/teams/team-1/sandboxes/kill"), - ("POST", "/admin/teams/team-1/builds/cancel"), - ("DELETE", "/volumes/vol-1"), - ("PATCH", "/events/webhooks/hook-1"), -] - -NETWORK_ERRORS = [ - httpx.ReadError("connection reset"), - httpx.WriteError("broken pipe"), - httpx.RemoteProtocolError("stream reset"), -] - - -@pytest.mark.parametrize(("method", "path"), NON_REPLAYABLE) -def test_unlisted_posts_are_not_replayable(method, path): - assert not is_replayable(httpx.Request(method, f"https://api.test{path}?x=1")) - - -@pytest.mark.parametrize(("method", "path"), REPLAYABLE) -def test_other_operations_are_replayable(method, path): - assert is_replayable(httpx.Request(method, f"https://api.test{path}?x=1")) - - -@pytest.mark.parametrize(("method", "path"), REPLAYABLE[:5]) -def test_retries_network_error_for_replayable_operation(method, path): - inner = FakeTransport([NETWORK_ERRORS[0], NETWORK_ERRORS[2], 200]) - sleeps = [] - - response = RetryableTransport( - inner, - retries=3, - sleep=sleeps.append, - monotonic=lambda: 0.0, - random_=lambda: 1.0, - ).handle_request(httpx.Request(method, f"https://api.test{path}", content=b"p")) - - assert response.status_code == 200 - assert sleeps == pytest.approx([0.1, 0.2]) - assert [item.content for item in inner.requests] == [b"p"] * 3 - - -@pytest.mark.parametrize("error", NETWORK_ERRORS) -@pytest.mark.parametrize(("method", "path"), NON_REPLAYABLE[:3]) -def test_does_not_retry_network_error_for_non_replayable_operation(error, method, path): - inner = FakeTransport([error, 200]) - - with pytest.raises(type(error)): - RetryableTransport(inner, retries=3, sleep=lambda _: None).handle_request( - httpx.Request(method, f"https://api.test{path}", content=b"p") - ) - - assert len(inner.requests) == 1 - - @pytest.mark.parametrize( "error", [ @@ -543,43 +484,9 @@ def test_leaves_connect_errors_and_timeouts_to_the_layers_around(error): assert len(inner.requests) == 1 -def test_rethrows_network_error_after_exhausting_retries(): - inner = FakeTransport([httpx.ReadError("reset")] * 3) - - with pytest.raises(httpx.ReadError): - RetryableTransport( - inner, retries=2, sleep=lambda _: None, monotonic=lambda: 0.0 - ).handle_request(httpx.Request("GET", "https://api.test/sandboxes")) - - assert len(inner.requests) == 3 - - -def test_rethrows_network_error_when_backoff_would_exceed_request_timeout(): - inner = FakeTransport([httpx.ReadError("reset"), 200]) - sleeps = [] - - with pytest.raises(httpx.ReadError): - RetryableTransport( - inner, - retries=3, - sleep=sleeps.append, - monotonic=lambda: 0.0, - random_=lambda: 1.0, - ).handle_request( - httpx.Request( - "GET", - "https://api.test/sandboxes", - extensions={"timeout": {"connect": 0.08, "read": 0.08}}, - ) - ) - - assert len(inner.requests) == 1 - assert sleeps == [] - - @pytest.mark.asyncio async def test_async_retries_rate_limit_and_replays_body(): - inner = FakeAsyncTransport([429, 200], retry_after="2") + inner = FakeAsyncTransport([429, 200], retry_after="2", should_retry="true") sleeps = [] async def sleep(delay): @@ -599,7 +506,7 @@ async def sleep(delay): @pytest.mark.asyncio async def test_async_does_not_retry_when_wait_reaches_request_timeout(): - inner = FakeAsyncTransport([429], retry_after="3") + inner = FakeAsyncTransport([429], retry_after="3", should_retry="true") sleeps = [] async def sleep(delay): @@ -622,7 +529,7 @@ async def sleep(delay): @pytest.mark.asyncio async def test_async_timeout_after_retry_wait_names_configuration_options(): - inner = FakeAsyncTransport([429]) + inner = FakeAsyncTransport([429], should_retry="true") clock = FakeClock() async def sleep(delay): @@ -646,7 +553,7 @@ async def sleep(delay): @pytest.mark.asyncio async def test_async_retries_unavailable_status_with_backoff(): - inner = FakeAsyncTransport([503, 502, 200], retry_after=None) + inner = FakeAsyncTransport([503, 502, 200], retry_after=None, should_retry="true") sleeps = [] randoms = iter([0.0, 1.0]) @@ -672,49 +579,57 @@ async def sleep(delay): @pytest.mark.asyncio -@pytest.mark.parametrize( - ("method", "path", "attempts"), - [ - ("POST", "/sandboxes", 1), - ("POST", "/secrets/secret-1", 1), - ("POST", "/sandboxes/sbx-1/pause", 2), - ("GET", "/sandboxes", 2), - ], -) -async def test_async_bad_gateway_is_retried_only_for_replayable_operations( - method, path, attempts -): - inner = FakeAsyncTransport([502, 200], retry_after=None) +@pytest.mark.parametrize("status", [500, 503]) +async def test_async_retries_post_when_marked_by_server(status): + inner = FakeAsyncTransport([status, 200], retry_after=None, should_retry="true") async def sleep(_): pass response = await AsyncRetryableTransport( - inner, retries=3, sleep=sleep, monotonic=lambda: 0.0 + inner, retries=1, sleep=sleep, monotonic=lambda: 0.0 ).handle_async_request( - httpx.Request(method, f"https://api.test{path}", content=b"p") + httpx.Request("POST", "https://api.test/v2/sandboxes", content=b"payload") ) - assert response.status_code == (502 if attempts == 1 else 200) - assert len(inner.requests) == attempts + assert response.status_code == 200 + assert len(inner.requests) == 2 @pytest.mark.asyncio -@pytest.mark.parametrize("status", [400, 404, 500, 504]) -async def test_async_does_not_retry_other_status_codes(status): - inner = FakeAsyncTransport([status], retry_after="0") +@pytest.mark.parametrize("should_retry", ["TRUE", " true "]) +async def test_async_should_retry_header_is_case_and_whitespace_insensitive( + should_retry, +): + inner = FakeAsyncTransport([503, 200], should_retry=should_retry) - response = await AsyncRetryableTransport(inner, retries=3).handle_async_request( - httpx.Request("GET", "https://api.test") - ) + async def sleep(_): + pass - assert response is inner.responses[0] - assert len(inner.requests) == 1 + response = await AsyncRetryableTransport( + inner, retries=1, sleep=sleep, monotonic=lambda: 0.0 + ).handle_async_request(httpx.Request("GET", "https://api.test")) + + assert response.status_code == 200 + assert len(inner.requests) == 2 @pytest.mark.asyncio -async def test_async_429_without_retry_after_is_propagated_as_is(): - inner = FakeAsyncTransport([429], retry_after=None) +@pytest.mark.parametrize( + ("status", "retry_after", "should_retry"), + [ + (503, "0", "false"), + (429, "2", None), + (502, "0", None), + (503, "0", None), + ], +) +async def test_async_does_not_retry_unmarked_responses( + status, retry_after, should_retry +): + inner = FakeAsyncTransport( + [status], retry_after=retry_after, should_retry=should_retry + ) response = await AsyncRetryableTransport(inner, retries=3).handle_async_request( httpx.Request("GET", "https://api.test") @@ -726,7 +641,7 @@ async def test_async_429_without_retry_after_is_propagated_as_is(): @pytest.mark.asyncio async def test_async_retry_after_exceeding_timeout_is_propagated_as_is(): - inner = FakeAsyncTransport([429], retry_after="4") + inner = FakeAsyncTransport([429], retry_after="4", should_retry="true") sleeps = [] async def sleep(delay): @@ -749,7 +664,7 @@ async def sleep(delay): @pytest.mark.asyncio async def test_async_retry_wait_is_bounded_when_request_timeout_is_disabled(): - inner = FakeAsyncTransport([429], retry_after="60") + inner = FakeAsyncTransport([429], retry_after="60", should_retry="true") sleeps = [] async def sleep(delay): @@ -788,7 +703,7 @@ async def handle_async_request(self, request): clock.now = 2.0 return response - inner = AdvancingTransport([429, 200]) + inner = AdvancingTransport([429, 200], should_retry="true") async def sleep(delay): clock.sleep(delay) @@ -821,7 +736,7 @@ async def handle_async_request(self, request): clock.now = 2.0 return response - inner = AdvancingTransport([429, 200]) + inner = AdvancingTransport([429, 200], should_retry="true") async def sleep(delay): clock.sleep(delay) @@ -857,7 +772,7 @@ class Stream(httpx.AsyncByteStream): async def __aiter__(self): yield b"payload" - inner = FakeAsyncTransport([429]) + inner = FakeAsyncTransport([429], should_retry="true") request = httpx.Request("POST", "https://api.test", content=Stream()) response = await AsyncRetryableTransport(inner, retries=3).handle_async_request( @@ -877,36 +792,43 @@ async def test_async_close_leaves_cached_transport_open(): assert not inner.closed +@pytest.mark.parametrize( + "error", + [httpx.ReadError("connection reset"), httpx.RemoteProtocolError("stream reset")], +) +def test_does_not_retry_post_write_network_errors(error): + inner = FakeTransport([error, 200]) + + with pytest.raises(type(error)): + RetryableTransport(inner, retries=3, sleep=lambda _: None).handle_request( + httpx.Request("POST", "https://api.test/v2/sandboxes", content=b"payload") + ) + + assert len(inner.requests) == 1 + + @pytest.mark.asyncio -@pytest.mark.parametrize(("method", "path"), REPLAYABLE[:5]) -async def test_async_retries_network_error_for_replayable_operation(method, path): - inner = FakeAsyncTransport([NETWORK_ERRORS[0], NETWORK_ERRORS[2], 200]) - sleeps = [] +async def test_async_exhaustion_returns_final_retryable_response(): + inner = FakeAsyncTransport([503, 503, 503], should_retry="true") - async def sleep(delay): - sleeps.append(delay) + async def sleep(_): + pass response = await AsyncRetryableTransport( - inner, - retries=3, - sleep=sleep, - monotonic=lambda: 0.0, - random_=lambda: 1.0, - ).handle_async_request( - httpx.Request(method, f"https://api.test{path}", content=b"p") - ) + inner, retries=2, sleep=sleep + ).handle_async_request(httpx.Request("GET", "https://api.test")) - assert response.status_code == 200 - assert sleeps == pytest.approx([0.1, 0.2]) - assert [item.content for item in inner.requests] == [b"p"] * 3 + assert response is inner.responses[-1] + assert len(inner.requests) == 3 + assert response.content == b"response" @pytest.mark.asyncio -@pytest.mark.parametrize("error", NETWORK_ERRORS) -@pytest.mark.parametrize(("method", "path"), NON_REPLAYABLE[:3]) -async def test_async_does_not_retry_network_error_for_non_replayable_operation( - error, method, path -): +@pytest.mark.parametrize( + "error", + [httpx.ReadError("connection reset"), httpx.RemoteProtocolError("stream reset")], +) +async def test_async_does_not_retry_post_write_network_errors(error): inner = FakeAsyncTransport([error, 200]) async def sleep(_): @@ -916,27 +838,12 @@ async def sleep(_): await AsyncRetryableTransport( inner, retries=3, sleep=sleep ).handle_async_request( - httpx.Request(method, f"https://api.test{path}", content=b"p") + httpx.Request("POST", "https://api.test/v2/sandboxes", content=b"payload") ) assert len(inner.requests) == 1 -@pytest.mark.asyncio -async def test_async_rethrows_network_error_after_exhausting_retries(): - inner = FakeAsyncTransport([httpx.ReadError("reset")] * 3) - - async def sleep(_): - pass - - with pytest.raises(httpx.ReadError): - await AsyncRetryableTransport( - inner, retries=2, sleep=sleep, monotonic=lambda: 0.0 - ).handle_async_request(httpx.Request("GET", "https://api.test/sandboxes")) - - assert len(inner.requests) == 3 - - @pytest.mark.parametrize( ("value", "expected"), [ From 6d4629d1fb1f8d66d6480cbd03f3b431c51d2a0a Mon Sep 17 00:00:00 2001 From: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com> Date: Mon, 19 Oct 2026 19:14:20 +0000 Subject: [PATCH 2/3] docs(python-sdk): note post-send network errors are not retried Co-Authored-By: mish@e2b.dev --- packages/python-sdk/e2b/connection_config.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/packages/python-sdk/e2b/connection_config.py b/packages/python-sdk/e2b/connection_config.py index 1129692e56..2064285be3 100644 --- a/packages/python-sdk/e2b/connection_config.py +++ b/packages/python-sdk/e2b/connection_config.py @@ -81,7 +81,8 @@ class ApiParams(TypedDict, total=False): """Number of control-plane retries for responses marked ``Should-Retry: true``. Waits for valid ``Retry-After`` delta-seconds, otherwise exponential backoff with jitter from 0.1 to 10 seconds. Connection failures are retried - separately; see ``E2B_CONNECTION_RETRIES``. + separately; see ``E2B_CONNECTION_RETRIES``. Network errors after the + request is sent (e.g. a dropped connection) are not retried. A retry is skipped when its wait would exhaust the request timeout. Retry waits use a 60-second total limit when request timeouts are disabled. Set to ``0`` to disable retries. Defaults to 3 retries. From 310b2b7a2a3c7b2888fbc229d9857b9f76d86411 Mon Sep 17 00:00:00 2001 From: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com> Date: Thu, 22 Oct 2026 13:38:01 +0000 Subject: [PATCH 3/3] feat(sdk): fall back to retrying 429/502/503/504 when Should-Retry is absent Co-Authored-By: mish@e2b.dev --- .changeset/should-retry-header.md | 6 +- packages/js-sdk/src/connectionConfig.ts | 10 +- packages/js-sdk/src/retry.ts | 24 ++- packages/js-sdk/tests/retry.test.ts | 86 +++++++-- packages/python-sdk/e2b/connection_config.py | 11 +- packages/python-sdk/e2b/retry.py | 18 +- .../tests/test_rate_limit_retry_transport.py | 171 +++++++++++++++--- 7 files changed, 263 insertions(+), 63 deletions(-) diff --git a/.changeset/should-retry-header.md b/.changeset/should-retry-header.md index 72db32484e..f25369feb7 100644 --- a/.changeset/should-retry-header.md +++ b/.changeset/should-retry-header.md @@ -3,7 +3,7 @@ '@e2b/python-sdk': minor --- -Retry control-plane API errors only when the response carries `Should-Retry: true` (honoring `Retry-After`, otherwise exponential backoff) +Retry control-plane API errors marked `Should-Retry: true`, or 429/502/503/504 responses without the header (e.g. from a load balancer); `Should-Retry: false` is never retried. -- Removes the client-side retry allowlists (status codes, methods, replayable operations) -- Responses without `Should-Retry: true` (including 429/502/503) and network errors after the request was sent are no longer retried; connection failures still are +- Removes method and replayable-operation retry allowlists +- Post-send network errors are no longer retried; connection failures still are diff --git a/packages/js-sdk/src/connectionConfig.ts b/packages/js-sdk/src/connectionConfig.ts index dbfdb1666b..0cb5ea3ba1 100644 --- a/packages/js-sdk/src/connectionConfig.ts +++ b/packages/js-sdk/src/connectionConfig.ts @@ -100,10 +100,12 @@ export interface ConnectionOpts { */ requestTimeoutMs?: number /** - * Number of control-plane API retries for responses marked `Should-Retry: true`. - * Waits for a valid `Retry-After` delta-seconds value, otherwise exponential - * backoff with jitter from 100 ms to 10 s. Also retries failures to establish - * the connection; errors after the request is sent are not retried. + * Number of control-plane API retries for responses marked `Should-Retry: true`, + * or 429/502/503/504 responses without that header. `Should-Retry: false` is + * never retried. Waits for a valid `Retry-After` delta-seconds value, otherwise + * exponential backoff with jitter from 100 ms to 10 s. Also retries failures + * to establish the connection; network errors after the request is sent are + * not retried. * A retry is skipped when its wait would exhaust the request timeout. * Retry waits use a 60-second total limit when request timeouts are disabled. * Set to `0` to disable retries. Defaults to 3 retries. diff --git a/packages/js-sdk/src/retry.ts b/packages/js-sdk/src/retry.ts index 3d803493a9..ead92931ee 100644 --- a/packages/js-sdk/src/retry.ts +++ b/packages/js-sdk/src/retry.ts @@ -1,12 +1,14 @@ import { InvalidArgumentError } from './errors' import { isReadableStreamLike } from './is' -// Retries API responses only when the server marks them `Should-Retry: true`. +// Retries API responses marked `Should-Retry: true`, or selected statuses +// when the header is absent. const MAX_RETRY_AFTER_SECONDS = 2_147_483 const MAX_RETRY_WAIT_WITHOUT_TIMEOUT_MS = 60_000 const BACKOFF_BASE_MS = 100 const BACKOFF_MAX_MS = 10_000 const BACKOFF_JITTER_MIN = 0.5 +const RETRYABLE_STATUSES = new Set([429, 502, 503, 504]) // Error codes raised while establishing the connection, before any request // bytes are written: Node/undici (`ECONNREFUSED`, `ENOTFOUND`, ...), Bun // (`ConnectionRefused`). Post-write failures such as `ECONNRESET` are @@ -122,15 +124,22 @@ function backoffMs(attempt: number, random: () => number): number { ) } -/** Delay before the next attempt, or `undefined` unless `Should-Retry` is true. */ +/** + * Delay before the next attempt, or `undefined` unless `Should-Retry` is true + * or the header is absent for a retryable status. A present header is + * authoritative, including explicit `false`. + */ function retryDelayMs( response: Response, attempt: number, random: () => number ): number | undefined { - if (response.headers.get('Should-Retry')?.trim().toLowerCase() !== 'true') { - return undefined - } + const shouldRetry = response.headers.get('Should-Retry') + const retryable = + shouldRetry === null + ? RETRYABLE_STATUSES.has(response.status) + : shouldRetry.trim().toLowerCase() === 'true' + if (!retryable) return undefined const retryAfter = parseRetryAfter(response.headers.get('Retry-After')) if (retryAfter !== undefined) return retryAfter * 1000 @@ -139,8 +148,9 @@ function retryDelayMs( } /** - * Retries API responses marked `Should-Retry: true` after `Retry-After` or - * exponential backoff, and retries connection-establishment failures. + * Retries API responses marked `Should-Retry: true`, or 429/502/503/504 + * responses without the header, after `Retry-After` or exponential backoff. + * Also retries connection-establishment failures. */ export function withRetry( fetchImpl: typeof fetch, diff --git a/packages/js-sdk/tests/retry.test.ts b/packages/js-sdk/tests/retry.test.ts index cc30c5cedc..88aa092351 100644 --- a/packages/js-sdk/tests/retry.test.ts +++ b/packages/js-sdk/tests/retry.test.ts @@ -169,30 +169,34 @@ test('bounds retry waits when the request timeout is disabled', async () => { expect(sleep).not.toHaveBeenCalled() }) -test('propagates 429 without Retry-After as is', async () => { +test('retries 429 without Should-Retry or Retry-After with backoff', async () => { const rateLimited = new Response('rate limited', { status: 429 }) - const fetchImpl = vi.fn(async () => rateLimited) as typeof fetch + const responses = [rateLimited, new Response('ok')] + const fetchImpl = vi.fn(async () => responses.shift()!) as typeof fetch const sleep = vi.fn(async () => {}) + const random = vi.fn(() => 0) const fetchWithRetry = withRetry(fetchImpl, 2, 10_000, { monotonic: () => 0, sleep, + random, }) const response = await fetchWithRetry('https://api.e2b.test/resource') - expect(response).toBe(rateLimited) - expect(response.bodyUsed).toBe(false) - expect(fetchImpl).toHaveBeenCalledOnce() - expect(sleep).not.toHaveBeenCalled() + expect(response.status).toBe(200) + expect(rateLimited.bodyUsed).toBe(true) + expect(fetchImpl).toHaveBeenCalledTimes(2) + expect(sleep).toHaveBeenCalledWith(50, expect.any(AbortSignal)) + expect(random).toHaveBeenCalledOnce() }) test.each([ { status: 503, headers: { 'Should-Retry': 'false' } }, - { status: 429, headers: { 'Retry-After': '2' } }, - { status: 502, headers: {} }, - { status: 503, headers: {} }, + { status: 503, headers: { 'Should-Retry': 'maybe' } }, + { status: 503, headers: { 'Should-Retry': '' } }, + { status: 500, headers: {} }, ])( - 'does not retry an unmarked $status response', + 'does not retry a $status response with headers $headers', async ({ status, headers }) => { const failed = new Response('error', { status, headers }) const fetchImpl = vi.fn(async () => failed) as typeof fetch @@ -211,6 +215,47 @@ test.each([ } ) +test.each([502, 503, 504])( + 'retries a %s without Should-Retry', + async (status) => { + const responses = [ + new Response(null, { status, headers: { 'Retry-After': '0' } }), + new Response('ok'), + ] + const fetchImpl = vi.fn(async () => responses.shift()!) as typeof fetch + const sleep = vi.fn(async () => {}) + const fetchWithRetry = withRetry(fetchImpl, 1, 10_000, { + monotonic: () => 0, + sleep, + }) + + const response = await fetchWithRetry('https://api.e2b.test/resource') + + expect(response.status).toBe(200) + expect(fetchImpl).toHaveBeenCalledTimes(2) + expect(sleep).toHaveBeenCalledWith(0, expect.any(AbortSignal)) + } +) + +test('uses Retry-After on an absent-header 429 fallback', async () => { + const responses = [ + new Response(null, { status: 429, headers: { 'Retry-After': '2' } }), + new Response('ok'), + ] + const fetchImpl = vi.fn(async () => responses.shift()!) as typeof fetch + const sleep = vi.fn(async () => {}) + const fetchWithRetry = withRetry(fetchImpl, 1, 10_000, { + monotonic: () => 0, + sleep, + }) + + const response = await fetchWithRetry('https://api.e2b.test/resource') + + expect(response.status).toBe(200) + expect(fetchImpl).toHaveBeenCalledTimes(2) + expect(sleep).toHaveBeenCalledWith(2_000, expect.any(AbortSignal)) +}) + test.each([502, 503])( 'retries a %s with exponential backoff and jitter', async (status) => { @@ -316,6 +361,25 @@ test('retries a POST when the server marks a 503', async () => { expect(fetchImpl).toHaveBeenCalledTimes(2) }) +test('retries a POST 502 without Should-Retry', async () => { + const responses = [ + new Response(null, { status: 502, headers: { 'Retry-After': '0' } }), + new Response('ok'), + ] + const fetchImpl = vi.fn(async () => responses.shift()!) as typeof fetch + const fetchWithRetry = withRetry(fetchImpl, 1, 10_000, { + monotonic: () => 0, + sleep: async () => {}, + }) + + const response = await fetchWithRetry('https://api.e2b.test/v2/sandboxes', { + method: 'POST', + }) + + expect(response.status).toBe(200) + expect(fetchImpl).toHaveBeenCalledTimes(2) +}) + test('caps the backoff for long retry sequences', async () => { const fetchImpl = vi.fn( async () => @@ -404,7 +468,7 @@ test('propagates 502 when the backoff would exceed the request timeout', async ( expect(sleep).not.toHaveBeenCalled() }) -test.each([400, 404, 500, 504])('does not retry a %s', async (status) => { +test.each([400, 404])('does not retry a %s', async (status) => { const failed = new Response('error', { status }) const fetchImpl = vi.fn(async () => failed) as typeof fetch const sleep = vi.fn(async () => {}) diff --git a/packages/python-sdk/e2b/connection_config.py b/packages/python-sdk/e2b/connection_config.py index 2064285be3..c818d12317 100644 --- a/packages/python-sdk/e2b/connection_config.py +++ b/packages/python-sdk/e2b/connection_config.py @@ -78,11 +78,12 @@ class ApiParams(TypedDict, total=False): """Timeout for the request in **seconds**, defaults to 60 seconds.""" retries: Optional[int] - """Number of control-plane retries for responses marked ``Should-Retry: true``. - Waits for valid ``Retry-After`` delta-seconds, otherwise exponential backoff - with jitter from 0.1 to 10 seconds. Connection failures are retried - separately; see ``E2B_CONNECTION_RETRIES``. Network errors after the - request is sent (e.g. a dropped connection) are not retried. + """Number of control-plane retries for responses marked ``Should-Retry: true``, + or 429/502/503/504 responses without that header. ``Should-Retry: false`` is + never retried. Waits for valid ``Retry-After`` delta-seconds, otherwise + exponential backoff with jitter from 0.1 to 10 seconds. Connection failures + are retried separately; see ``E2B_CONNECTION_RETRIES``. Network errors after + the request is sent (e.g. a dropped connection) are not retried. A retry is skipped when its wait would exhaust the request timeout. Retry waits use a 60-second total limit when request timeouts are disabled. Set to ``0`` to disable retries. Defaults to 3 retries. diff --git a/packages/python-sdk/e2b/retry.py b/packages/python-sdk/e2b/retry.py index f76b99bb96..d1b5985f5f 100644 --- a/packages/python-sdk/e2b/retry.py +++ b/packages/python-sdk/e2b/retry.py @@ -1,4 +1,4 @@ -"""Retry API responses marked by the server and leave connection retries below.""" +"""Retry API responses marked by the server or selected missing-header statuses.""" import asyncio import random @@ -14,6 +14,7 @@ BACKOFF_BASE_SECONDS = 0.1 BACKOFF_MAX_SECONDS = 10.0 BACKOFF_JITTER_MIN = 0.5 +RETRYABLE_STATUSES = frozenset({429, 502, 503, 504}) def resolve_max_retries(retries: int) -> int: @@ -45,8 +46,15 @@ def _retry_delay( attempt: int, random_: Callable[[], float], ) -> Optional[float]: - """Return a delay only when the API marks the response ``Should-Retry``.""" - if response.headers.get("Should-Retry", "").strip().lower() != "true": + """Return a delay for marked responses or retryable statuses without a header. + + A present header is authoritative, including explicit ``false``. + """ + should_retry = response.headers.get("Should-Retry") + if should_retry is None: + if response.status_code not in RETRYABLE_STATUSES: + return None + elif should_retry.strip().lower() != "true": return None retry_after = parse_retry_after(response.headers.get("Retry-After")) @@ -91,7 +99,7 @@ def _request_deadline(request: httpx.Request, monotonic: Callable[[], float]) -> class RetryableTransport(httpx.BaseTransport): - """Retry API responses marked ``Should-Retry: true`` using server delay or backoff.""" + """Retry marked API responses or 429/502/503/504 responses without a header.""" def __init__( self, @@ -147,7 +155,7 @@ def close(self) -> None: class AsyncRetryableTransport(httpx.AsyncBaseTransport): - """Async counterpart of :class:`RetryableTransport`.""" + """Async transport with the same response retry policy as RetryableTransport.""" def __init__( self, diff --git a/packages/python-sdk/tests/test_rate_limit_retry_transport.py b/packages/python-sdk/tests/test_rate_limit_retry_transport.py index 9513562c78..aa3a7786d0 100644 --- a/packages/python-sdk/tests/test_rate_limit_retry_transport.py +++ b/packages/python-sdk/tests/test_rate_limit_retry_transport.py @@ -127,15 +127,17 @@ def test_invalid_retry_after_falls_back_to_backoff(retry_after): assert sleeps == pytest.approx([0.05]) -def test_429_without_retry_after_is_propagated_as_is(): - inner = FakeTransport([429], retry_after=None) +def test_429_without_header_or_retry_after_uses_backoff(): + inner = FakeTransport([429, 200], retry_after=None, should_retry=None) + sleeps = [] - response = RetryableTransport(inner, retries=3).handle_request( - httpx.Request("GET", "https://api.test") - ) + response = RetryableTransport( + inner, retries=3, sleep=sleeps.append, random_=lambda: 0.0 + ).handle_request(httpx.Request("GET", "https://api.test")) - assert response is inner.responses[0] - assert len(inner.requests) == 1 + assert response.status_code == 200 + assert len(inner.requests) == 2 + assert sleeps == pytest.approx([0.05]) def test_retry_after_exceeding_timeout_is_propagated_as_is(): @@ -176,17 +178,44 @@ def test_retry_wait_is_bounded_when_request_timeout_is_disabled(): assert sleeps == [] +@pytest.mark.parametrize("status", [429, 502, 503, 504]) +def test_retries_retryable_status_when_header_is_absent(status): + inner = FakeTransport([status, 200], retry_after="0", should_retry=None) + sleeps = [] + + response = RetryableTransport( + inner, retries=1, sleep=sleeps.append, monotonic=lambda: 0.0 + ).handle_request(httpx.Request("GET", "https://api.test")) + + assert response.status_code == 200 + assert len(inner.requests) == 2 + assert sleeps == [0] + + +def test_absent_header_429_uses_retry_after(): + inner = FakeTransport([429, 200], retry_after="2", should_retry=None) + sleeps = [] + + response = RetryableTransport( + inner, retries=1, sleep=sleeps.append, monotonic=lambda: 0.0 + ).handle_request(httpx.Request("GET", "https://api.test")) + + assert response.status_code == 200 + assert len(inner.requests) == 2 + assert sleeps == [2] + + @pytest.mark.parametrize( - ("status", "retry_after", "should_retry"), + ("status", "should_retry"), [ - (503, "0", "false"), - (429, "2", None), - (502, "0", None), - (503, "0", None), + (503, "false"), + (503, "maybe"), + (503, ""), + (500, None), ], ) -def test_does_not_retry_unmarked_responses(status, retry_after, should_retry): - inner = FakeTransport([status], retry_after=retry_after, should_retry=should_retry) +def test_does_not_retry_explicit_denial_or_non_fallback_status(status, should_retry): + inner = FakeTransport([status], retry_after="0", should_retry=should_retry) response = RetryableTransport(inner, retries=3).handle_request( httpx.Request("GET", "https://api.test") @@ -247,6 +276,19 @@ def test_retries_post_503_when_marked_by_server(): assert len(inner.requests) == 2 +def test_retries_post_502_without_should_retry_header(): + inner = FakeTransport([502, 200], retry_after="0", should_retry=None) + + response = RetryableTransport( + inner, retries=1, sleep=lambda _: None, monotonic=lambda: 0.0 + ).handle_request( + httpx.Request("POST", "https://api.test/v2/sandboxes", content=b"payload") + ) + + assert response.status_code == 200 + assert len(inner.requests) == 2 + + @pytest.mark.parametrize("should_retry", ["TRUE", " true "]) def test_should_retry_header_is_case_and_whitespace_insensitive(should_retry): inner = FakeTransport([503, 200], should_retry=should_retry) @@ -504,6 +546,44 @@ async def sleep(delay): assert inner.responses[0].is_closed +@pytest.mark.asyncio +async def test_async_429_without_header_or_retry_after_uses_backoff(): + inner = FakeAsyncTransport([429, 200], retry_after=None, should_retry=None) + sleeps = [] + + async def sleep(delay): + sleeps.append(delay) + + response = await AsyncRetryableTransport( + inner, + retries=1, + sleep=sleep, + monotonic=lambda: 0.0, + random_=lambda: 0.0, + ).handle_async_request(httpx.Request("GET", "https://api.test")) + + assert response.status_code == 200 + assert len(inner.requests) == 2 + assert sleeps == pytest.approx([0.05]) + + +@pytest.mark.asyncio +async def test_async_absent_header_429_uses_retry_after(): + inner = FakeAsyncTransport([429, 200], retry_after="2", should_retry=None) + sleeps = [] + + async def sleep(delay): + sleeps.append(delay) + + response = await AsyncRetryableTransport( + inner, retries=1, sleep=sleep, monotonic=lambda: 0.0 + ).handle_async_request(httpx.Request("GET", "https://api.test")) + + assert response.status_code == 200 + assert len(inner.requests) == 2 + assert sleeps == [2] + + @pytest.mark.asyncio async def test_async_does_not_retry_when_wait_reaches_request_timeout(): inner = FakeAsyncTransport([429], retry_after="3", should_retry="true") @@ -553,9 +633,11 @@ async def sleep(delay): @pytest.mark.asyncio async def test_async_retries_unavailable_status_with_backoff(): - inner = FakeAsyncTransport([503, 502, 200], retry_after=None, should_retry="true") + inner = FakeAsyncTransport( + [503, 502, 504, 200], retry_after=None, should_retry=None + ) sleeps = [] - randoms = iter([0.0, 1.0]) + randoms = iter([0.0, 1.0, 0.5]) async def sleep(delay): sleeps.append(delay) @@ -573,8 +655,8 @@ async def sleep(delay): ) assert response.status_code == 200 - assert sleeps == pytest.approx([0.05, 0.2]) - assert [item.content for item in inner.requests] == [b"payload"] * 3 + assert sleeps == pytest.approx([0.05, 0.2, 0.3]) + assert [item.content for item in inner.requests] == [b"payload"] * 4 assert all(item.is_closed for item in inner.responses[:-1]) @@ -596,6 +678,23 @@ async def sleep(_): assert len(inner.requests) == 2 +@pytest.mark.asyncio +async def test_async_retries_post_502_without_should_retry_header(): + inner = FakeAsyncTransport([502, 200], retry_after="0", should_retry=None) + + async def sleep(_): + pass + + response = await AsyncRetryableTransport( + inner, retries=1, sleep=sleep, monotonic=lambda: 0.0 + ).handle_async_request( + httpx.Request("POST", "https://api.test/v2/sandboxes", content=b"payload") + ) + + assert response.status_code == 200 + assert len(inner.requests) == 2 + + @pytest.mark.asyncio @pytest.mark.parametrize("should_retry", ["TRUE", " true "]) async def test_async_should_retry_header_is_case_and_whitespace_insensitive( @@ -614,22 +713,38 @@ async def sleep(_): assert len(inner.requests) == 2 +@pytest.mark.asyncio +@pytest.mark.parametrize("status", [429, 502, 503, 504]) +async def test_async_retries_retryable_status_when_header_is_absent(status): + inner = FakeAsyncTransport([status, 200], retry_after="0", should_retry=None) + sleeps = [] + + async def sleep(delay): + sleeps.append(delay) + + response = await AsyncRetryableTransport( + inner, retries=1, sleep=sleep, monotonic=lambda: 0.0 + ).handle_async_request(httpx.Request("GET", "https://api.test")) + + assert response.status_code == 200 + assert len(inner.requests) == 2 + assert sleeps == [0] + + @pytest.mark.asyncio @pytest.mark.parametrize( - ("status", "retry_after", "should_retry"), + ("status", "should_retry"), [ - (503, "0", "false"), - (429, "2", None), - (502, "0", None), - (503, "0", None), + (503, "false"), + (503, "maybe"), + (503, ""), + (500, None), ], ) -async def test_async_does_not_retry_unmarked_responses( - status, retry_after, should_retry +async def test_async_does_not_retry_explicit_denial_or_non_fallback_status( + status, should_retry ): - inner = FakeAsyncTransport( - [status], retry_after=retry_after, should_retry=should_retry - ) + inner = FakeAsyncTransport([status], retry_after="0", should_retry=should_retry) response = await AsyncRetryableTransport(inner, retries=3).handle_async_request( httpx.Request("GET", "https://api.test")