From bf3b710bae8007439ef8692dc05443800e86ef3b Mon Sep 17 00:00:00 2001 From: "sentry-junior[bot]" <264270552+sentry-junior[bot]@users.noreply.github.com> Date: Mon, 26 Oct 2026 23:42:22 +0000 Subject: [PATCH 1/4] fix(cli): Don't auto-retry non-idempotent requests; honor rate-limit reset on 429 The authenticated fetch retried every method on 408/429/5xx, network errors, and timeouts, so a POST that reached the server could run up to three times (for example, three Seer autofix runs). On 429 it ignored X-Sentry-Rate-Limit-Reset and Retry-After and backed off blindly. Automatic retries now apply only to GET/HEAD/OPTIONS, or to requests that opt in with `idempotent: true`. Chunk uploads (stored by SHA-1) and artifact-bundle/DIF assemble calls (state keyed by checksum) opt in. The 401 token-refresh retry still applies to every method. On 429 the client waits for the server-requested delay (rate-limit reset measured against the response Date header, or Retry-After), capped at 10s. Concurrency-limit 429s and responses without a hint use jittered exponential backoff. When the wait exceeds the cap, retries run out, or the method can't be retried, it throws RateLimitError with the time to wait. The policy lives in a pure retry-policy.ts so it can move to toolkit-core unchanged. Co-Authored-By: Burak Yigit Kaya <126780+BYK@users.noreply.github.com> --- packages/cli/src/lib/api/chunk-upload.ts | 8 +- packages/cli/src/lib/api/dart-symbols.ts | 3 + packages/cli/src/lib/api/debug-files.ts | 8 +- packages/cli/src/lib/api/infrastructure.ts | 7 + packages/cli/src/lib/api/proguard.ts | 3 + packages/cli/src/lib/api/sourcemaps.ts | 2 + packages/cli/src/lib/errors.ts | 27 ++ packages/cli/src/lib/retry-policy.ts | 152 +++++++++ packages/cli/src/lib/sentry-client.ts | 149 ++++++--- .../cli/test/lib/api-client.coverage.test.ts | 10 +- packages/cli/test/lib/retry-policy.test.ts | 163 ++++++++++ packages/cli/test/lib/sentry-client.test.ts | 289 +++++++++++++++++- 12 files changed, 759 insertions(+), 62 deletions(-) create mode 100644 packages/cli/src/lib/retry-policy.ts create mode 100644 packages/cli/test/lib/retry-policy.test.ts diff --git a/packages/cli/src/lib/api/chunk-upload.ts b/packages/cli/src/lib/api/chunk-upload.ts index 11f7ddd6b..a96ecfc56 100644 --- a/packages/cli/src/lib/api/chunk-upload.ts +++ b/packages/cli/src/lib/api/chunk-upload.ts @@ -395,7 +395,8 @@ export async function uploadMissingChunks(params: { } const limit = pLimit(serverOptions.concurrency); - const { fetch: authFetch } = getSdkConfig(regionUrl); + // Chunks are stored by SHA-1, so a replayed upload is a no-op. + const { fetch: authFetch } = getSdkConfig(regionUrl, { idempotent: true }); await Promise.all( missingChunks.map((chunk) => @@ -487,7 +488,8 @@ export async function uploadMissingBufferChunks(params: { } const limit = pLimit(serverOptions.concurrency); - const { fetch: authFetch } = getSdkConfig(regionUrl); + // Chunks are stored by SHA-1, so a replayed upload is a no-op. + const { fetch: authFetch } = getSdkConfig(regionUrl, { idempotent: true }); await Promise.all( missingChunks.map((chunk) => @@ -550,6 +552,8 @@ export async function pollAssembly(params: { method: "POST", body, schema, + // Assembly state is keyed by checksum; re-sending only polls it. + idempotent: true, }, ); diff --git a/packages/cli/src/lib/api/dart-symbols.ts b/packages/cli/src/lib/api/dart-symbols.ts index 6ca5212e8..eb7858a80 100644 --- a/packages/cli/src/lib/api/dart-symbols.ts +++ b/packages/cli/src/lib/api/dart-symbols.ts @@ -159,6 +159,8 @@ export async function uploadDartSymbolMap( method: "POST", body: assembleBody, schema: DifAssembleResponseSchema, + // Assembly state is keyed by checksum; re-sending only polls it. + idempotent: true, }, ); @@ -194,6 +196,7 @@ export async function uploadDartSymbolMap( method: "POST", body: assembleBody, schema: DifAssembleResponseSchema, + idempotent: true, }, ); diff --git a/packages/cli/src/lib/api/debug-files.ts b/packages/cli/src/lib/api/debug-files.ts index 35cfaafa7..b66494da5 100644 --- a/packages/cli/src/lib/api/debug-files.ts +++ b/packages/cli/src/lib/api/debug-files.ts @@ -272,10 +272,16 @@ async function postAssemble( endpoint: string, body: unknown, ): Promise { + // Assembly state is keyed by checksum; re-sending only polls it. const { data } = await apiRequestToRegion( regionUrl, endpoint, - { method: "POST", body, schema: DifAssembleResponseSchema }, + { + method: "POST", + body, + schema: DifAssembleResponseSchema, + idempotent: true, + }, ); return data; } diff --git a/packages/cli/src/lib/api/infrastructure.ts b/packages/cli/src/lib/api/infrastructure.ts index 1699031c0..ece5742a2 100644 --- a/packages/cli/src/lib/api/infrastructure.ts +++ b/packages/cli/src/lib/api/infrastructure.ts @@ -205,6 +205,11 @@ export type ApiRequestOptions = { credential?: CredentialContext; /** Manually validate each redirect before forwarding credentials. */ validatedRedirects?: boolean; + /** + * Retry transient failures even for writes. Only for requests the server + * dedupes (see `idempotent` in sentry-client.ts). + */ + idempotent?: boolean; }; /** @@ -774,11 +779,13 @@ export async function apiRequestToRegion( schema, credential, validatedRedirects, + idempotent, } = options; const config = getSdkConfig(regionUrl, { credential, validatedRedirects, bufferResponseBody: true, + idempotent, }); const endpointWithParams = appendSearchParams(endpoint, params); diff --git a/packages/cli/src/lib/api/proguard.ts b/packages/cli/src/lib/api/proguard.ts index 5f2147488..5cb1a1d99 100644 --- a/packages/cli/src/lib/api/proguard.ts +++ b/packages/cli/src/lib/api/proguard.ts @@ -175,6 +175,8 @@ export async function uploadProguardMappings( method: "POST", body: assembleBody, schema: DifAssembleResponseSchema, + // Assembly state is keyed by checksum; re-sending only polls it. + idempotent: true, }, ); @@ -235,6 +237,7 @@ async function pollDifAssembly(params: { method: "POST", body, schema: DifAssembleResponseSchema, + idempotent: true, }, ); diff --git a/packages/cli/src/lib/api/sourcemaps.ts b/packages/cli/src/lib/api/sourcemaps.ts index 67425a1ad..2421bbffb 100644 --- a/packages/cli/src/lib/api/sourcemaps.ts +++ b/packages/cli/src/lib/api/sourcemaps.ts @@ -340,6 +340,8 @@ async function uploadArtifactBundle(opts: { method: "POST", body: assembleBody, schema: AssembleResponseSchema, + // Assembly state is keyed by checksum; re-sending only polls it. + idempotent: true, }, ); diff --git a/packages/cli/src/lib/errors.ts b/packages/cli/src/lib/errors.ts index 171b3e676..54fa0d6bc 100644 --- a/packages/cli/src/lib/errors.ts +++ b/packages/cli/src/lib/errors.ts @@ -212,6 +212,33 @@ export class ApiError extends CliError { } } +/** + * The Sentry API rejected a request with 429 and the CLI did not (or could + * no longer) wait it out. + * + * @param endpoint - API path that was rate limited + * @param retryAfterMs - Server-requested wait, when known + * @param serverDetail - The API's `detail` message, when present + */ +export class RateLimitError extends ApiError { + readonly retryAfterMs?: number; + + constructor(endpoint: string, retryAfterMs?: number, serverDetail?: string) { + const when = + retryAfterMs === undefined + ? "Wait a few seconds and try again." + : `Try again in ${Math.max(1, Math.ceil(retryAfterMs / 1000))}s.`; + super( + "Rate limited by the Sentry API", + 429, + serverDetail ? `${serverDetail}\n ${when}` : when, + endpoint, + ); + this.name = "RateLimitError"; + this.retryAfterMs = retryAfterMs; + } +} + export type AuthErrorReason = "not_authenticated" | "expired" | "invalid"; /** Options for AuthError */ diff --git a/packages/cli/src/lib/retry-policy.ts b/packages/cli/src/lib/retry-policy.ts new file mode 100644 index 000000000..4f0349bc1 --- /dev/null +++ b/packages/cli/src/lib/retry-policy.ts @@ -0,0 +1,152 @@ +/** + * Retry policy for Sentry API requests. + * + * Decides whether a failed request may be replayed automatically and how + * long to wait first. Kept pure (no CLI imports, no I/O) so it can be swapped + * for the shared toolkit-core policy without touching callers. + * + * Only idempotent methods are replayed by default. A transient status, a + * network error, or a timeout after a POST does not tell us whether the + * server already applied it, so replaying could duplicate the write. + * PUT and DELETE are excluded too: Sentry uses PUT for partial updates with + * side effects (issue merges, activity, notifications), and a replayed + * DELETE turns a successful delete into a 404. Callers that know a write is + * safe to replay (content-addressed uploads) opt in per request. + */ + +/** HTTP statuses that indicate a transient failure worth retrying. */ +const RETRYABLE_STATUS_CODES: ReadonlySet = new Set([ + 408, 429, 500, 502, 503, 504, +]); + +/** Methods that are safe to replay without an explicit opt-in. */ +const IDEMPOTENT_METHODS: ReadonlySet = new Set([ + "GET", + "HEAD", + "OPTIONS", +]); + +/** First fallback backoff step; doubles per attempt up to the cap. */ +const BASE_BACKOFF_MS = 1000; + +/** + * Whether a request may be replayed after a transient status, network + * error, or timeout. + * + * @param method - HTTP method (any case) + * @param idempotent - Caller asserts the request is safe to replay + */ +export function isRetryableMethod(method: string, idempotent = false): boolean { + return idempotent || IDEMPOTENT_METHODS.has(method.toUpperCase()); +} + +/** + * Jittered exponential backoff: a random delay in the upper half of + * `min(1s * 2^attempt, maxDelayMs)`, so parallel requests spread out + * without retrying sooner than half the nominal step. + */ +export function backoffDelayMs( + attempt: number, + maxDelayMs: number, + random: () => number = Math.random, +): number { + const ceiling = Math.min(BASE_BACKOFF_MS * 2 ** attempt, maxDelayMs); + return Math.round(ceiling / 2 + random() * (ceiling / 2)); +} + +function parseRetryAfterMs(value: string, referenceMs: number): number { + const trimmed = value.trim(); + if (/^\d+$/.test(trimmed)) { + return Number(trimmed) * 1000; + } + return Date.parse(trimmed) - referenceMs; +} + +/** + * Delay requested by the server before retrying, in milliseconds. + * + * Reads `Retry-After` (delta seconds or HTTP date) on any status, and on 429 + * also `X-Sentry-Rate-Limit-Reset` (Unix seconds when the window resets). + * The reset header is ignored when `X-Sentry-Rate-Limit-Remaining` is + * non-zero: that 429 came from the concurrency limit, where the window reset + * is unrelated and jittered backoff is the right wait. + * + * Absolute times are measured against the response `Date` header when + * present, so a skewed local clock cannot turn a 1s window into a long wait + * or an immediate retry. Times already in the past are dropped. + * + * @returns The longest requested wait, or undefined when the server gave + * no usable hint + */ +export function serverRetryDelayMs( + status: number, + headers: Headers, + nowMs: number, +): number | undefined { + const serverDate = Date.parse(headers.get("date") ?? ""); + const referenceMs = Number.isNaN(serverDate) ? nowMs : serverDate; + const candidates: number[] = []; + + const retryAfter = headers.get("retry-after"); + if (retryAfter) { + candidates.push(parseRetryAfterMs(retryAfter, referenceMs)); + } + + const remaining = headers.get("x-sentry-rate-limit-remaining"); + const reset = headers.get("x-sentry-rate-limit-reset"); + if ( + status === 429 && + reset && + (remaining === null || Number(remaining) <= 0) + ) { + candidates.push(Number(reset) * 1000 - referenceMs); + } + + const valid = candidates.filter((ms) => Number.isFinite(ms) && ms >= 0); + return valid.length > 0 ? Math.max(...valid) : undefined; +} + +/** Outcome of {@link decideStatusRetry}. */ +export type RetryDecision = + | { retry: true; delayMs: number } + | { + retry: false; + /** Server-requested wait, when known; tells the user when to retry. */ + retryAfterMs?: number; + }; + +/** + * Decide whether to retry a response with the given status. + * + * Retries only retryable statuses on retryable methods, while attempts + * remain, and only when the server-requested wait fits within `maxDelayMs`. + * Without a server hint, waits a jittered exponential backoff. + */ +export function decideStatusRetry(params: { + status: number; + headers: Headers; + methodRetryable: boolean; + attempt: number; + maxRetries: number; + maxDelayMs: number; + nowMs: number; + random?: () => number; +}): RetryDecision { + const { status, headers, methodRetryable, attempt, maxRetries, maxDelayMs } = + params; + if (!RETRYABLE_STATUS_CODES.has(status)) { + return { retry: false }; + } + const serverDelay = serverRetryDelayMs(status, headers, params.nowMs); + if ( + !methodRetryable || + attempt >= maxRetries || + (serverDelay !== undefined && serverDelay > maxDelayMs) + ) { + return { retry: false, retryAfterMs: serverDelay }; + } + return { + retry: true, + delayMs: serverDelay ?? backoffDelayMs(attempt, maxDelayMs, params.random), + }; +} diff --git a/packages/cli/src/lib/sentry-client.ts b/packages/cli/src/lib/sentry-client.ts index fae7b82ae..de8229fdd 100644 --- a/packages/cli/src/lib/sentry-client.ts +++ b/packages/cli/src/lib/sentry-client.ts @@ -41,6 +41,7 @@ import { ApiError, HostScopeError, MalformedAuthTokenError, + RateLimitError, TimeoutError, } from "./errors.js"; import { extractInputUrl } from "./fetch-input.js"; @@ -51,6 +52,11 @@ import { invalidateCachedResponsesMatching, storeCachedResponse, } from "./response-cache.js"; +import { + backoffDelayMs, + decideStatusRetry, + isRetryableMethod, +} from "./retry-policy.js"; import { normalizeHttpOrigin, normalizeOrigin } from "./sentry-urls.js"; import { withTracingSpan } from "./telemetry.js"; import { parseSntrysClaim } from "./token-claims.js"; @@ -82,12 +88,14 @@ const ENDPOINT_TIMEOUT_OVERRIDES: TimeoutOverride[] = [ /** Maximum retry attempts for failed requests */ const MAX_RETRIES = 2; -/** Maximum backoff delay between retries in milliseconds */ +/** + * Longest wait before a retry, in milliseconds. Caps the jittered backoff + * and the server-requested wait (`Retry-After`, rate-limit reset); a longer + * server-requested wait is not retried, so it surfaces instead of hanging. Sentry's API rate-limit + * windows are usually one second, so honored waits are typically short. + */ const MAX_BACKOFF_MS = 10_000; -/** HTTP status codes that trigger automatic retry */ -const RETRYABLE_STATUS_CODES = [408, 429, 500, 502, 503, 504]; - /** Header to mark requests as retries, preventing infinite token refresh loops */ const RETRY_MARKER_HEADER = "x-sentry-cli-retry"; @@ -146,11 +154,6 @@ export function getResponseCredentialIdentity( return responseCredentialIdentities.get(response); } -/** Calculate exponential backoff delay, capped at MAX_BACKOFF_MS */ -function backoffDelay(attempt: number): number { - return Math.min(1000 * 2 ** attempt, MAX_BACKOFF_MS); -} - /** Check if an error is a user-initiated abort */ function isUserAbort(error: unknown, signal?: AbortSignal | null): boolean { return ( @@ -389,48 +392,93 @@ async function fetchWithTimeout({ /** Result of a single fetch attempt - drives the retry loop */ type AttemptResult = | { action: "done"; response: Response } - | { action: "retry" } + | { action: "retry"; delayMs: number } | { action: "throw"; error: unknown }; +/** Per-attempt retry inputs. */ +type AttemptRetryState = { + attempt: number; + /** Method is idempotent or the caller opted in; see retry-policy.ts. */ + methodRetryable: boolean; + /** Request path, for error messages. */ + urlPath: string; +}; + +/** Best-effort `detail` from a Sentry error body. */ +async function readErrorDetail( + response: Response, +): Promise { + try { + const parsed = JSON.parse(await response.text()) as { detail?: unknown }; + return typeof parsed.detail === "string" ? parsed.detail : undefined; + } catch (error) { + log.debug("Failed to read rate-limit response detail", error); + return; + } +} + /** * Decide what to do with a successful HTTP response. - * Returns 'done' for final responses, 'retry' for retryable errors and 401s. - * Only refresh credentials when another attempt can use them. + * + * A 401 refreshes the token and retries for any method: the server rejected + * the request before processing it. Other transient statuses retry only per + * {@link decideStatusRetry}. A 429 that is not retried throws + * {@link RateLimitError} so the user learns when to try again. */ async function handleResponse( response: Response, headers: Headers, credential: CredentialContext, - isLastAttempt: boolean, + { attempt, methodRetryable, urlPath }: AttemptRetryState, ): Promise { - if (response.status === 401 && !isLastAttempt) { + if (response.status === 401 && attempt < MAX_RETRIES) { const refreshed = await handleUnauthorized(headers, credential); if (refreshed) { await cancelResponseBody(response); - return { action: "retry" }; + return { action: "retry", delayMs: 0 }; } return { action: "done", response }; } - if (RETRYABLE_STATUS_CODES.includes(response.status) && !isLastAttempt) { + const decision = decideStatusRetry({ + status: response.status, + headers: response.headers, + methodRetryable, + attempt, + maxRetries: MAX_RETRIES, + maxDelayMs: MAX_BACKOFF_MS, + nowMs: Date.now(), + }); + if (decision.retry) { await cancelResponseBody(response); - return { action: "retry" }; + return { action: "retry", delayMs: decision.delayMs }; + } + if (response.status === 429) { + return { + action: "throw", + error: new RateLimitError( + urlPath, + decision.retryAfterMs, + await readErrorDetail(response), + ), + }; } - return { action: "done", response }; } /** - * Decide what to do with a fetch error. User aborts and an internal - * timeout on the last attempt throw immediately; TLS cert errors throw - * immediately with actionable guidance (retrying is pointless); other - * errors retry until the last attempt, then propagate. + * Decide what to do with a fetch error. User aborts throw immediately; TLS + * cert errors throw immediately with actionable guidance (retrying is + * pointless). Network errors and internal timeouts retry only for retryable + * methods with attempts left: a write may have reached the server, so + * replaying it could apply it twice. */ function handleFetchError( error: unknown, signal: AbortSignal | undefined | null, - isLastAttempt: boolean, + { attempt, methodRetryable }: AttemptRetryState, ): AttemptResult { + const canRetry = methodRetryable && attempt < MAX_RETRIES; if (error instanceof HostScopeError) { return { action: "throw", error }; } @@ -450,7 +498,7 @@ function handleFetchError( }; } - if (isInternalTimeout(error) && isLastAttempt) { + if (isInternalTimeout(error) && !canRetry) { const timeoutMs = (error as { timeoutMs?: number }).timeoutMs ?? REQUEST_TIMEOUT_MS; const seconds = Math.round(timeoutMs / 1000); @@ -458,14 +506,16 @@ function handleFetchError( action: "throw", error: new TimeoutError( `Request timed out after ${seconds}s.`, - "The Sentry API did not respond in time. Try again; if the problem persists, check https://status.sentry.io.", + methodRetryable + ? "The Sentry API did not respond in time. Try again; if the problem persists, check https://status.sentry.io." + : "The Sentry API did not respond in time. The change may still have been applied; check before trying again. If the problem persists, check https://status.sentry.io.", ), }; } - if (isLastAttempt) { + if (!canRetry) { return { action: "throw", error }; } - return { action: "retry" }; + return { action: "retry", delayMs: backoffDelayMs(attempt, MAX_BACKOFF_MS) }; } /** Extract the URL pathname for span naming */ @@ -609,8 +659,8 @@ async function buildAttemptFactory( /** * Authenticate and execute a request with retry logic. * - * Refreshes the auth token, then retries the request up to `MAX_RETRIES` times - * with exponential backoff on transient errors. + * Refreshes the auth token, then retries the request up to `MAX_RETRIES` + * times on transient errors (retry-policy.ts decides which and how long). */ type RetryContext = { input: Request | string | URL; @@ -620,6 +670,7 @@ type RetryContext = { credential: CredentialContext; validatedRedirects: boolean; responseBodyMode: ResponseBodyMode; + idempotent: boolean; }; async function fetchWithRetry({ @@ -630,6 +681,7 @@ async function fetchWithRetry({ credential, validatedRedirects, responseBodyMode, + idempotent, }: RetryContext): Promise { const shouldRefresh = Boolean( credential.refreshable && @@ -650,16 +702,17 @@ async function fetchWithRetry({ const headers = prepareHeaders(input, init, effectiveCredential); const attemptFactory = await buildAttemptFactory(input, init); const timeoutMs = resolveTimeoutMs(fullUrl); + const methodRetryable = isRetryableMethod(method, idempotent); + const urlPath = extractUrlPath(fullUrl); for (let attempt = 0; attempt <= MAX_RETRIES; attempt++) { - const isLastAttempt = attempt === MAX_RETRIES; const { input: attemptInput, init: attemptInit } = attemptFactory(); const result = await executeAttempt({ input: attemptInput, init: attemptInit, headers, credential: effectiveCredential, - isLastAttempt, + retryState: { attempt, methodRetryable, urlPath }, timeoutMs, validatedRedirects, responseBodyMode, @@ -687,11 +740,10 @@ async function fetchWithRetry({ throw result.error; } - const delay = backoffDelay(attempt); log.debug( - `${method} ${new URL(fullUrl).pathname} → retry ${attempt + 1}/${MAX_RETRIES} after ${delay}ms`, + `${method} ${urlPath} → retry ${attempt + 1}/${MAX_RETRIES} after ${result.delayMs}ms`, ); - await sleepMs(delay); + await sleepMs(result.delayMs); } // Unreachable: the last attempt always returns 'done' or 'throw' @@ -705,9 +757,12 @@ async function fetchWithRetry({ * - **Response caching** for GET requests (checked before hitting the network) * - Auth token injection (Bearer token) * - Request timeout via AbortController - * - Automatic retry on transient HTTP errors (408, 429, 5xx) - * - 401 handling: force-refreshes the token and retries once - * - Exponential backoff between retries + * - Automatic retry on transient HTTP errors (408, 429, 5xx), network errors, + * and timeouts — for GET/HEAD/OPTIONS, or any method with `idempotent` + * - 429 handling: waits for the server's rate-limit reset when it fits the + * cap, otherwise throws {@link RateLimitError} + * - 401 handling: force-refreshes the token and retries once (any method) + * - Jittered exponential backoff between retries * - User-Agent header for API analytics * - Automatic HTTP span tracing for every request * @@ -722,6 +777,12 @@ type AuthenticatedFetchOptions = { validatedRedirects?: boolean; /** Complete successful GET bodies inside the retry/timeout budget. */ bufferResponseBody?: boolean; + /** + * Caller asserts that replaying the request is harmless, so transient + * failures retry for any method. Only for writes the server dedupes, such + * as content-addressed chunk uploads and checksum-keyed assemble calls. + */ + idempotent?: boolean; }; function createAuthenticatedFetch( @@ -787,6 +848,7 @@ function createAuthenticatedFetch( fullUrl, credential, validatedRedirects: options.validatedRedirects === true, + idempotent: options.idempotent === true, responseBodyMode: method.toUpperCase() !== "GET" ? "stream" @@ -817,7 +879,7 @@ type ExecuteAttemptArgs = { init: RequestInit | undefined; headers: Headers; credential: CredentialContext; - isLastAttempt: boolean; + retryState: AttemptRetryState; timeoutMs: number; validatedRedirects: boolean; responseBodyMode: ResponseBodyMode; @@ -947,7 +1009,7 @@ async function fetchFollowingValidatedRedirects({ responseBodyMode, }: Omit< ExecuteAttemptArgs, - "isLastAttempt" | "validatedRedirects" + "retryState" | "validatedRedirects" >): Promise { const original = new Request(input, init); const visited = new Set([redirectLoopKey(new URL(original.url))]); @@ -1010,7 +1072,7 @@ async function executeAttempt({ init, headers, credential, - isLastAttempt, + retryState, timeoutMs, validatedRedirects, responseBodyMode, @@ -1041,11 +1103,11 @@ async function executeAttempt({ responseBodyMode, }); } catch (error) { - return handleFetchError(error, signal, isLastAttempt); + return handleFetchError(error, signal, retryState); } // Outside the try: response handling errors must not be treated as fetch // failures (and retried). - return await handleResponse(response, headers, credential, isLastAttempt); + return await handleResponse(response, headers, credential, retryState); } /** Singleton authenticated fetch instance - reused across all requests */ @@ -1061,7 +1123,8 @@ function getAuthenticatedFetch( if ( options.credential || options.validatedRedirects || - options.bufferResponseBody + options.bufferResponseBody || + options.idempotent ) { return createAuthenticatedFetch(options) as unknown as typeof fetch; } diff --git a/packages/cli/test/lib/api-client.coverage.test.ts b/packages/cli/test/lib/api-client.coverage.test.ts index 9c53edd89..a37b63eaf 100644 --- a/packages/cli/test/lib/api-client.coverage.test.ts +++ b/packages/cli/test/lib/api-client.coverage.test.ts @@ -1417,9 +1417,9 @@ describe("infrastructure.ts", () => { test("parses JSON error detail from non-ok response", async () => { globalThis.fetch = mockFetch( async () => - new Response(JSON.stringify({ detail: "Rate limited" }), { - status: 429, - statusText: "Too Many Requests", + new Response(JSON.stringify({ detail: "Invalid query" }), { + status: 400, + statusText: "Bad Request", headers: { "Content-Type": "application/json" }, }), ); @@ -1430,8 +1430,8 @@ describe("infrastructure.ts", () => { } catch (error) { expect(error).toBeInstanceOf(ApiError); const apiErr = error as ApiError; - expect(apiErr.status).toBe(429); - expect(apiErr.detail).toBe("Rate limited"); + expect(apiErr.status).toBe(400); + expect(apiErr.detail).toBe("Invalid query"); } }); diff --git a/packages/cli/test/lib/retry-policy.test.ts b/packages/cli/test/lib/retry-policy.test.ts new file mode 100644 index 000000000..67701b3de --- /dev/null +++ b/packages/cli/test/lib/retry-policy.test.ts @@ -0,0 +1,163 @@ +import { describe, expect, test } from "vitest"; +import { + backoffDelayMs, + decideStatusRetry, + isRetryableMethod, + serverRetryDelayMs, +} from "../../src/lib/retry-policy.js"; + +const NOW = Date.parse("2026-01-01T00:00:00Z"); +const NOW_SECONDS = NOW / 1000; + +describe("isRetryableMethod", () => { + test("retries only safe methods by default", () => { + expect(isRetryableMethod("GET")).toBe(true); + expect(isRetryableMethod("head")).toBe(true); + expect(isRetryableMethod("OPTIONS")).toBe(true); + for (const method of ["POST", "PUT", "PATCH", "DELETE"]) { + expect(isRetryableMethod(method)).toBe(false); + } + }); + + test("retries any method when the caller opts in", () => { + expect(isRetryableMethod("POST", true)).toBe(true); + }); +}); + +describe("backoffDelayMs", () => { + test("jitters within the upper half of the capped step", () => { + expect(backoffDelayMs(0, 10_000, () => 0)).toBe(500); + expect(backoffDelayMs(0, 10_000, () => 1)).toBe(1000); + expect(backoffDelayMs(1, 10_000, () => 1)).toBe(2000); + expect(backoffDelayMs(10, 10_000, () => 0)).toBe(5000); + expect(backoffDelayMs(10, 10_000, () => 1)).toBe(10_000); + }); +}); + +describe("serverRetryDelayMs", () => { + test("reads Retry-After seconds", () => { + const headers = new Headers({ "Retry-After": "3" }); + expect(serverRetryDelayMs(503, headers, NOW)).toBe(3000); + }); + + test("reads a Retry-After HTTP date against the server Date header", () => { + const headers = new Headers({ + Date: new Date(NOW).toUTCString(), + "Retry-After": new Date(NOW + 2000).toUTCString(), + }); + // Local clock is an hour ahead; the server Date keeps the wait at 2s. + expect(serverRetryDelayMs(429, headers, NOW + 3_600_000)).toBe(2000); + }); + + test("reads X-Sentry-Rate-Limit-Reset when the window is exhausted", () => { + const headers = new Headers({ + "X-Sentry-Rate-Limit-Remaining": "0", + "X-Sentry-Rate-Limit-Reset": String(NOW_SECONDS + 1), + }); + expect(serverRetryDelayMs(429, headers, NOW)).toBe(1000); + }); + + test("guards the reset against a skewed local clock", () => { + const headers = new Headers({ + Date: new Date(NOW).toUTCString(), + "X-Sentry-Rate-Limit-Remaining": "0", + "X-Sentry-Rate-Limit-Reset": String(NOW_SECONDS + 1), + }); + expect(serverRetryDelayMs(429, headers, NOW - 3_600_000)).toBe(1000); + expect(serverRetryDelayMs(429, headers, NOW + 3_600_000)).toBe(1000); + }); + + test("ignores the reset for concurrency-limit 429s", () => { + const headers = new Headers({ + "X-Sentry-Rate-Limit-Remaining": "12", + "X-Sentry-Rate-Limit-ConcurrentRemaining": "0", + "X-Sentry-Rate-Limit-Reset": String(NOW_SECONDS + 60), + }); + expect(serverRetryDelayMs(429, headers, NOW)).toBeUndefined(); + }); + + test("ignores the reset on non-429 statuses", () => { + const headers = new Headers({ + "X-Sentry-Rate-Limit-Remaining": "0", + "X-Sentry-Rate-Limit-Reset": String(NOW_SECONDS + 5), + }); + expect(serverRetryDelayMs(503, headers, NOW)).toBeUndefined(); + }); + + test("drops past and malformed values", () => { + const headers = new Headers({ + "Retry-After": "soon", + "X-Sentry-Rate-Limit-Remaining": "0", + "X-Sentry-Rate-Limit-Reset": String(NOW_SECONDS - 5), + }); + expect(serverRetryDelayMs(429, headers, NOW)).toBeUndefined(); + }); + + test("uses the longest of Retry-After and the reset", () => { + const headers = new Headers({ + "Retry-After": "4", + "X-Sentry-Rate-Limit-Remaining": "0", + "X-Sentry-Rate-Limit-Reset": String(NOW_SECONDS + 1), + }); + expect(serverRetryDelayMs(429, headers, NOW)).toBe(4000); + }); +}); + +describe("decideStatusRetry", () => { + const base = { + headers: new Headers(), + methodRetryable: true, + attempt: 0, + maxRetries: 2, + maxDelayMs: 10_000, + nowMs: NOW, + random: () => 1, + }; + + test("does not retry non-transient statuses", () => { + expect(decideStatusRetry({ ...base, status: 404 })).toEqual({ + retry: false, + }); + }); + + test("backs off without a server hint", () => { + expect(decideStatusRetry({ ...base, status: 503 })).toEqual({ + retry: true, + delayMs: 1000, + }); + }); + + test("waits for the server-requested delay", () => { + const headers = new Headers({ "Retry-After": "2" }); + expect(decideStatusRetry({ ...base, status: 429, headers })).toEqual({ + retry: true, + delayMs: 2000, + }); + }); + + test("gives up when the server-requested delay exceeds the cap", () => { + const headers = new Headers({ "Retry-After": "60" }); + expect(decideStatusRetry({ ...base, status: 429, headers })).toEqual({ + retry: false, + retryAfterMs: 60_000, + }); + }); + + test("does not retry non-idempotent methods", () => { + const headers = new Headers({ "Retry-After": "1" }); + expect( + decideStatusRetry({ + ...base, + status: 429, + headers, + methodRetryable: false, + }), + ).toEqual({ retry: false, retryAfterMs: 1000 }); + }); + + test("stops when retries are exhausted", () => { + expect(decideStatusRetry({ ...base, status: 502, attempt: 2 })).toEqual({ + retry: false, + }); + }); +}); diff --git a/packages/cli/test/lib/sentry-client.test.ts b/packages/cli/test/lib/sentry-client.test.ts index 89df80c7d..b658da10f 100644 --- a/packages/cli/test/lib/sentry-client.test.ts +++ b/packages/cli/test/lib/sentry-client.test.ts @@ -5,7 +5,7 @@ import { afterEach, beforeEach, describe, expect, test } from "vitest"; import { setAuthToken } from "../../src/lib/db/auth.js"; -import { TimeoutError } from "../../src/lib/errors.js"; +import { RateLimitError, TimeoutError } from "../../src/lib/errors.js"; import { __injectTimeoutOverrideForTests, __resolveRequestTimeoutMsForTests, @@ -35,8 +35,23 @@ function getAuthenticatedFetch(): typeof fetch { return getSdkConfig(REGION_URL).fetch as typeof fetch; } +/** Fetch for callers that opted in to replaying writes (chunk uploads). */ +function getIdempotentFetch(): typeof fetch { + return getSdkConfig(REGION_URL, { idempotent: true }).fetch as typeof fetch; +} + +function jsonResponse( + body: unknown, + init: { status?: number; headers?: Record } = {}, +): Response { + return new Response(JSON.stringify(body), { + status: init.status, + headers: { "Content-Type": "application/json", ...init.headers }, + }); +} + describe("fetchWithRetry / buildAttemptFactory", () => { - test("retries a POST with a string body without re-consuming the body", async () => { + test("retries an opted-in POST with a string body without re-consuming the body", async () => { const marker = "__test_string_body__"; const seen: string[] = []; let callCount = 0; @@ -60,7 +75,7 @@ describe("fetchWithRetry / buildAttemptFactory", () => { }); }); - const authFetch = getAuthenticatedFetch(); + const authFetch = getIdempotentFetch(); const res = await authFetch( `${REGION_URL}/api/0/${marker}/organizations/`, { @@ -78,7 +93,7 @@ describe("fetchWithRetry / buildAttemptFactory", () => { ]); }); - test("retries a POST built from a Request object without consuming its body", async () => { + test("retries an opted-in POST built from a Request object without consuming its body", async () => { // SDK path: @sentry/api hands us a Request as the sole argument. // Pre-fix: attempt 2 saw a consumed body stream. const marker = "__test_request_body__"; @@ -101,14 +116,14 @@ describe("fetchWithRetry / buildAttemptFactory", () => { }); }); - const payload = JSON.stringify({ stopping_point: "root_cause" }); - const request = new Request(`${REGION_URL}/api/0/${marker}/autofix/`, { + const payload = JSON.stringify({ checksum: "abc", chunks: ["abc"] }); + const request = new Request(`${REGION_URL}/api/0/${marker}/assemble/`, { method: "POST", headers: { "Content-Type": "application/json" }, body: payload, }); - const authFetch = getAuthenticatedFetch(); + const authFetch = getIdempotentFetch(); const res = await authFetch(request); expect(res.status).toBe(200); @@ -116,7 +131,7 @@ describe("fetchWithRetry / buildAttemptFactory", () => { expect(seen).toEqual([payload, payload]); }); - test("retries with a ReadableStream body by materializing once", async () => { + test("retries an opted-in ReadableStream body by materializing once", async () => { // Streams are consumed on first read; without up-front materialization // attempt 2 would see an undefined body. const marker = "__test_stream_body__"; @@ -151,7 +166,7 @@ describe("fetchWithRetry / buildAttemptFactory", () => { }, }); - const authFetch = getAuthenticatedFetch(); + const authFetch = getIdempotentFetch(); // Node's fetch requires duplex: "half" for streamed bodies; Bun accepts // it as a no-op. const res = await authFetch(`${REGION_URL}/api/0/${marker}/streamed/`, { @@ -165,7 +180,7 @@ describe("fetchWithRetry / buildAttemptFactory", () => { expect(seen).toEqual(["streamed-body", "streamed-body"]); }); - test("retries a FormData body without losing the multipart boundary", async () => { + test("retries an opted-in FormData body without losing the multipart boundary", async () => { // Regression for a Cursor Bugbot finding: materializing FormData to // an ArrayBuffer drops the auto-negotiated // `Content-Type: multipart/form-data; boundary=...` header that @@ -197,7 +212,7 @@ describe("fetchWithRetry / buildAttemptFactory", () => { "chunk.bin", ); - const authFetch = getAuthenticatedFetch(); + const authFetch = getIdempotentFetch(); const res = await authFetch(`${REGION_URL}/api/0/${marker}/chunks/`, { method: "POST", body: form, @@ -222,6 +237,223 @@ describe("fetchWithRetry / buildAttemptFactory", () => { }); }); +describe("non-idempotent requests", () => { + test("does not retry a POST on a transient status", async () => { + let calls = 0; + globalThis.fetch = mockFetch(async () => { + calls += 1; + return new Response("busy", { status: 503 }); + }); + + const res = await getAuthenticatedFetch()( + `${REGION_URL}/api/0/organizations/acme/issues/1/autofix/`, + { method: "POST", body: "{}" }, + ); + + expect(res.status).toBe(503); + expect(calls).toBe(1); + }); + + test("does not retry a POST after a network error", async () => { + let calls = 0; + globalThis.fetch = mockFetch(async () => { + calls += 1; + throw new TypeError("fetch failed"); + }); + + await expect( + getAuthenticatedFetch()(`${REGION_URL}/api/0/projects/acme/`, { + method: "POST", + body: "{}", + }), + ).rejects.toThrow("fetch failed"); + expect(calls).toBe(1); + }); + + test("still retries a GET on a transient status", async () => { + let calls = 0; + globalThis.fetch = mockFetch(async () => { + calls += 1; + return calls === 1 + ? new Response("busy", { status: 502 }) + : jsonResponse({ ok: true }); + }); + + const res = await getAuthenticatedFetch()( + `${REGION_URL}/api/0/organizations/`, + ); + + expect(res.status).toBe(200); + expect(calls).toBe(2); + }); + + test("replays a POST Request body after a 401 token refresh", async () => { + // The server rejected the request before processing it, so the refresh + // retry applies to every method. + process.env.SENTRY_CLIENT_ID = "test-client-id"; + setAuthToken("initial-token", 3600, "test-refresh-token"); + const seen: { auth: string | null; body: string }[] = []; + globalThis.fetch = mockFetch(async (input, init) => { + const req = new Request(input as Request, init); + if (req.url.includes("/oauth/token/")) { + return jsonResponse({ + access_token: "refreshed-token", + token_type: "Bearer", + expires_in: 3600, + refresh_token: "new-refresh-token", + }); + } + seen.push({ + auth: req.headers.get("authorization"), + body: await req.text(), + }); + return seen.length === 1 + ? jsonResponse({ detail: "Unauthorized" }, { status: 401 }) + : jsonResponse({ ok: true }); + }); + + const payload = JSON.stringify({ step: "root_cause" }); + const res = await getAuthenticatedFetch()( + new Request(`${REGION_URL}/api/0/organizations/acme/issues/1/autofix/`, { + method: "POST", + headers: { "Content-Type": "application/json" }, + body: payload, + }), + ); + + expect(res.status).toBe(200); + expect(seen).toEqual([ + { auth: "Bearer initial-token", body: payload }, + { auth: "Bearer refreshed-token", body: payload }, + ]); + }); +}); + +describe("429 rate limiting", () => { + test("waits for X-Sentry-Rate-Limit-Reset, then retries a GET", async () => { + let calls = 0; + const started = Date.now(); + globalThis.fetch = mockFetch(async () => { + calls += 1; + if (calls > 1) { + return jsonResponse({ ok: true }); + } + // Server clock an hour behind ours: the Date header keeps the wait at 1s. + const serverNow = Math.floor(Date.now() / 1000) - 3600; + return jsonResponse( + { detail: "Limit is 40 requests in 1 seconds" }, + { + status: 429, + headers: { + Date: new Date(serverNow * 1000).toUTCString(), + "X-Sentry-Rate-Limit-Remaining": "0", + "X-Sentry-Rate-Limit-Reset": String(serverNow + 1), + }, + }, + ); + }); + + const res = await getAuthenticatedFetch()( + `${REGION_URL}/api/0/organizations/`, + ); + + expect(res.status).toBe(200); + expect(calls).toBe(2); + expect(Date.now() - started).toBeGreaterThanOrEqual(900); + }); + + test("throws RateLimitError with the wait when it exceeds the cap", async () => { + let calls = 0; + globalThis.fetch = mockFetch(async () => { + calls += 1; + return jsonResponse( + { detail: "Limit is 100 requests in 60 seconds" }, + { status: 429, headers: { "Retry-After": "45" } }, + ); + }); + + const error = await getAuthenticatedFetch()( + `${REGION_URL}/api/0/organizations/`, + ).catch((err: unknown) => err); + + expect(calls).toBe(1); + expect(error).toBeInstanceOf(RateLimitError); + const rateLimit = error as RateLimitError; + expect(rateLimit.status).toBe(429); + expect(rateLimit.retryAfterMs).toBe(45_000); + expect(rateLimit.endpoint).toBe("/api/0/organizations/"); + expect(rateLimit.format()).toContain("Limit is 100 requests in 60 seconds"); + expect(rateLimit.format()).toContain("Try again in 45s."); + }); + + test("throws RateLimitError once retries are exhausted", async () => { + let calls = 0; + globalThis.fetch = mockFetch(async () => { + calls += 1; + return jsonResponse( + { detail: "slow down" }, + { status: 429, headers: { "Retry-After": "0" } }, + ); + }); + + await expect( + getAuthenticatedFetch()(`${REGION_URL}/api/0/organizations/`), + ).rejects.toBeInstanceOf(RateLimitError); + // MAX_RETRIES = 2 → attempts 0, 1, 2. + expect(calls).toBe(3); + }); + + test("backs off on concurrency-limit 429s instead of waiting for the reset", async () => { + let calls = 0; + globalThis.fetch = mockFetch(async () => { + calls += 1; + if (calls > 1) { + return jsonResponse({ ok: true }); + } + return jsonResponse( + { detail: "Too many concurrent requests" }, + { + status: 429, + headers: { + "X-Sentry-Rate-Limit-Remaining": "30", + "X-Sentry-Rate-Limit-ConcurrentRemaining": "0", + "X-Sentry-Rate-Limit-Reset": String( + Math.floor(Date.now() / 1000) + 3600, + ), + }, + }, + ); + }); + + const res = await getAuthenticatedFetch()( + `${REGION_URL}/api/0/organizations/`, + ); + + expect(res.status).toBe(200); + expect(calls).toBe(2); + }); + + test("does not retry a POST on 429", async () => { + let calls = 0; + globalThis.fetch = mockFetch(async () => { + calls += 1; + return jsonResponse( + { detail: "slow down" }, + { status: 429, headers: { "Retry-After": "1" } }, + ); + }); + + const error = await getAuthenticatedFetch()( + `${REGION_URL}/api/0/organizations/acme/issues/1/autofix/`, + { method: "POST", body: "{}" }, + ).catch((err: unknown) => err); + + expect(calls).toBe(1); + expect(error).toBeInstanceOf(RateLimitError); + expect((error as RateLimitError).retryAfterMs).toBe(1000); + }); +}); + describe("fetchWithTimeout internal timeout classification", () => { test("surfaces TimeoutError on the last attempt when our own timeout fires", async () => { // Inject a tiny timeout so we don't wait 30 s for the default to fire. @@ -271,6 +503,41 @@ describe("fetchWithTimeout internal timeout classification", () => { restore(); } }); + + test("does not retry a timed-out POST and warns it may have applied", async () => { + const marker = "___post-timeout-test___"; + const restore = __injectTimeoutOverrideForTests({ + pattern: new RegExp(`/${marker}/`), + timeoutMs: 50, + }); + + try { + let calls = 0; + globalThis.fetch = mockFetch(async (_input, init) => { + calls += 1; + return await new Promise((_resolve, reject) => { + init?.signal?.addEventListener( + "abort", + () => reject(new DOMException("aborted", "AbortError")), + { once: true }, + ); + }); + }); + + const error = await getAuthenticatedFetch()(`${REGION_URL}/${marker}/`, { + method: "POST", + body: "{}", + }).catch((err: unknown) => err); + + expect(error).toBeInstanceOf(TimeoutError); + expect((error as TimeoutError).hint).toContain( + "may still have been applied", + ); + expect(calls).toBe(1); + } finally { + restore(); + } + }); }); describe("user abort passthrough", () => { From 7099e2bb000fc8e31c1a2e95bbebb7ae920a3817 Mon Sep 17 00:00:00 2001 From: "sentry-junior[bot]" <264270552+sentry-junior[bot]@users.noreply.github.com> Date: Tue, 27 Oct 2026 09:31:18 +0000 Subject: [PATCH 2/4] fix(cli): Retry 429 for any method; make RateLimitError actionable Sentry's rate-limit middleware rejects a request in process_view, before the endpoint runs, so a 429 never applied the write. Retry 429 for every method. 5xx, timeouts, and network errors stay limited to GET/HEAD/OPTIONS and the `idempotent` opt-in. Retried statuses stay 408/429/500/502/503/504; 501, 505 and other deterministic 5xx are not retried. RateLimitError now tells callers (including agents) to retry in N seconds, or a few seconds without a hint, not to repeat the request before then, and to make fewer requests in parallel. --- packages/cli/src/lib/errors.ts | 14 +++-- packages/cli/src/lib/retry-policy.ts | 18 ++++-- packages/cli/src/lib/sentry-client.ts | 5 +- packages/cli/test/lib/retry-policy.test.ts | 24 +++++-- packages/cli/test/lib/sentry-client.test.ts | 69 ++++++++++++++++----- 5 files changed, 96 insertions(+), 34 deletions(-) diff --git a/packages/cli/src/lib/errors.ts b/packages/cli/src/lib/errors.ts index 54fa0d6bc..57773585e 100644 --- a/packages/cli/src/lib/errors.ts +++ b/packages/cli/src/lib/errors.ts @@ -213,8 +213,8 @@ export class ApiError extends CliError { } /** - * The Sentry API rejected a request with 429 and the CLI did not (or could - * no longer) wait it out. + * The Sentry API rejected a request with 429 and the CLI could not wait it + * out (the wait exceeds the cap, or retries ran out). * * @param endpoint - API path that was rate limited * @param retryAfterMs - Server-requested wait, when known @@ -224,14 +224,16 @@ export class RateLimitError extends ApiError { readonly retryAfterMs?: number; constructor(endpoint: string, retryAfterMs?: number, serverDetail?: string) { - const when = + const seconds = Math.max(1, Math.ceil((retryAfterMs ?? 0) / 1000)); + const wait = retryAfterMs === undefined - ? "Wait a few seconds and try again." - : `Try again in ${Math.max(1, Math.ceil(retryAfterMs / 1000))}s.`; + ? "a few seconds" + : `${seconds} second${seconds === 1 ? "" : "s"}`; + const advice = `Retry in ${wait}; do not repeat the request before then. Make fewer requests in parallel.`; super( "Rate limited by the Sentry API", 429, - serverDetail ? `${serverDetail}\n ${when}` : when, + serverDetail ? `${serverDetail}\n ${advice}` : advice, endpoint, ); this.name = "RateLimitError"; diff --git a/packages/cli/src/lib/retry-policy.ts b/packages/cli/src/lib/retry-policy.ts index 4f0349bc1..976e8a9ac 100644 --- a/packages/cli/src/lib/retry-policy.ts +++ b/packages/cli/src/lib/retry-policy.ts @@ -12,9 +12,16 @@ * side effects (issue merges, activity, notifications), and a replayed * DELETE turns a successful delete into a 404. Callers that know a write is * safe to replay (content-addressed uploads) opt in per request. + * + * A 429 is the exception and retries for any method: Sentry's rate-limit + * middleware rejects the request before the view runs, so nothing was + * applied. */ -/** HTTP statuses that indicate a transient failure worth retrying. */ +/** + * HTTP statuses that indicate a transient failure worth retrying. Not a + * blanket 5xx: 501, 505 and similar are deterministic. + */ const RETRYABLE_STATUS_CODES: ReadonlySet = new Set([ 408, 429, 500, 502, 503, 504, ]); @@ -118,9 +125,10 @@ export type RetryDecision = /** * Decide whether to retry a response with the given status. * - * Retries only retryable statuses on retryable methods, while attempts - * remain, and only when the server-requested wait fits within `maxDelayMs`. - * Without a server hint, waits a jittered exponential backoff. + * Retries retryable statuses while attempts remain and the server-requested + * wait fits within `maxDelayMs`. A 429 retries for any method; other + * statuses only when `methodRetryable`. Without a server hint, waits a + * jittered exponential backoff. */ export function decideStatusRetry(params: { status: number; @@ -139,7 +147,7 @@ export function decideStatusRetry(params: { } const serverDelay = serverRetryDelayMs(status, headers, params.nowMs); if ( - !methodRetryable || + (!methodRetryable && status !== 429) || attempt >= maxRetries || (serverDelay !== undefined && serverDelay > maxDelayMs) ) { diff --git a/packages/cli/src/lib/sentry-client.ts b/packages/cli/src/lib/sentry-client.ts index de8229fdd..6ef6b2198 100644 --- a/packages/cli/src/lib/sentry-client.ts +++ b/packages/cli/src/lib/sentry-client.ts @@ -759,8 +759,9 @@ async function fetchWithRetry({ * - Request timeout via AbortController * - Automatic retry on transient HTTP errors (408, 429, 5xx), network errors, * and timeouts — for GET/HEAD/OPTIONS, or any method with `idempotent` - * - 429 handling: waits for the server's rate-limit reset when it fits the - * cap, otherwise throws {@link RateLimitError} + * - 429 handling (any method; rejected before processing): waits for the + * server's rate-limit reset when it fits the cap, otherwise throws + * {@link RateLimitError} * - 401 handling: force-refreshes the token and retries once (any method) * - Jittered exponential backoff between retries * - User-Agent header for API analytics diff --git a/packages/cli/test/lib/retry-policy.test.ts b/packages/cli/test/lib/retry-policy.test.ts index 67701b3de..3e3722be2 100644 --- a/packages/cli/test/lib/retry-policy.test.ts +++ b/packages/cli/test/lib/retry-policy.test.ts @@ -115,9 +115,11 @@ describe("decideStatusRetry", () => { }; test("does not retry non-transient statuses", () => { - expect(decideStatusRetry({ ...base, status: 404 })).toEqual({ - retry: false, - }); + for (const status of [404, 501, 505]) { + expect(decideStatusRetry({ ...base, status })).toEqual({ + retry: false, + }); + } }); test("backs off without a server hint", () => { @@ -143,18 +145,30 @@ describe("decideStatusRetry", () => { }); }); - test("does not retry non-idempotent methods", () => { + test("does not retry non-idempotent methods on 5xx", () => { const headers = new Headers({ "Retry-After": "1" }); expect( decideStatusRetry({ ...base, - status: 429, + status: 503, headers, methodRetryable: false, }), ).toEqual({ retry: false, retryAfterMs: 1000 }); }); + test("retries 429 for non-idempotent methods", () => { + const headers = new Headers({ "Retry-After": "1" }); + expect( + decideStatusRetry({ + ...base, + status: 429, + headers, + methodRetryable: false, + }), + ).toEqual({ retry: true, delayMs: 1000 }); + }); + test("stops when retries are exhausted", () => { expect(decideStatusRetry({ ...base, status: 502, attempt: 2 })).toEqual({ retry: false, diff --git a/packages/cli/test/lib/sentry-client.test.ts b/packages/cli/test/lib/sentry-client.test.ts index b658da10f..3b20cfca0 100644 --- a/packages/cli/test/lib/sentry-client.test.ts +++ b/packages/cli/test/lib/sentry-client.test.ts @@ -270,6 +270,21 @@ describe("non-idempotent requests", () => { expect(calls).toBe(1); }); + test("does not retry deterministic 5xx statuses", async () => { + let calls = 0; + globalThis.fetch = mockFetch(async () => { + calls += 1; + return new Response("nope", { status: 501 }); + }); + + const res = await getAuthenticatedFetch()( + `${REGION_URL}/api/0/organizations/`, + ); + + expect(res.status).toBe(501); + expect(calls).toBe(1); + }); + test("still retries a GET on a transient status", async () => { let calls = 0; globalThis.fetch = mockFetch(async () => { @@ -383,7 +398,9 @@ describe("429 rate limiting", () => { expect(rateLimit.retryAfterMs).toBe(45_000); expect(rateLimit.endpoint).toBe("/api/0/organizations/"); expect(rateLimit.format()).toContain("Limit is 100 requests in 60 seconds"); - expect(rateLimit.format()).toContain("Try again in 45s."); + expect(rateLimit.format()).toContain( + "Retry in 45 seconds; do not repeat the request before then. Make fewer requests in parallel.", + ); }); test("throws RateLimitError once retries are exhausted", async () => { @@ -396,9 +413,12 @@ describe("429 rate limiting", () => { ); }); - await expect( - getAuthenticatedFetch()(`${REGION_URL}/api/0/organizations/`), - ).rejects.toBeInstanceOf(RateLimitError); + const error = await getAuthenticatedFetch()( + `${REGION_URL}/api/0/organizations/`, + ).catch((err: unknown) => err); + + expect(error).toBeInstanceOf(RateLimitError); + expect((error as RateLimitError).format()).toContain("Retry in 1 second;"); // MAX_RETRIES = 2 → attempts 0, 1, 2. expect(calls).toBe(3); }); @@ -433,24 +453,41 @@ describe("429 rate limiting", () => { expect(calls).toBe(2); }); - test("does not retry a POST on 429", async () => { - let calls = 0; - globalThis.fetch = mockFetch(async () => { - calls += 1; - return jsonResponse( - { detail: "slow down" }, - { status: 429, headers: { "Retry-After": "1" } }, - ); + test("retries a POST on 429, since the request was rejected before processing", async () => { + const bodies: string[] = []; + globalThis.fetch = mockFetch(async (input, init) => { + bodies.push(await new Request(input as string, init).text()); + return bodies.length === 1 + ? jsonResponse( + { detail: "slow down" }, + { status: 429, headers: { "Retry-After": "0" } }, + ) + : jsonResponse({ ok: true }); }); - const error = await getAuthenticatedFetch()( + const res = await getAuthenticatedFetch()( `${REGION_URL}/api/0/organizations/acme/issues/1/autofix/`, - { method: "POST", body: "{}" }, + { method: "POST", body: '{"step":"root_cause"}' }, + ); + + expect(res.status).toBe(200); + expect(bodies).toEqual(['{"step":"root_cause"}', '{"step":"root_cause"}']); + }); + + test("says to retry in a few seconds when the 429 has no wait hint", async () => { + globalThis.fetch = mockFetch(async () => + jsonResponse({ detail: "slow down" }, { status: 429 }), + ); + + const error = await getAuthenticatedFetch()( + `${REGION_URL}/api/0/organizations/`, ).catch((err: unknown) => err); - expect(calls).toBe(1); expect(error).toBeInstanceOf(RateLimitError); - expect((error as RateLimitError).retryAfterMs).toBe(1000); + expect((error as RateLimitError).retryAfterMs).toBeUndefined(); + expect((error as RateLimitError).format()).toContain( + "Retry in a few seconds; do not repeat the request before then.", + ); }); }); From 6c071b676a70434d3007a5f2270b783d2f5fb376 Mon Sep 17 00:00:00 2001 From: "sentry-junior[bot]" <264270552+sentry-junior[bot]@users.noreply.github.com> Date: Tue, 27 Oct 2026 15:18:47 +0000 Subject: [PATCH 3/4] fix(cli): Opt preprod build assemble into transport retries The assemble loop already re-sends the identical body to poll, and the server reuses the in-progress PreprodArtifact row via get_or_create, so retrying a transient 5xx/timeout is no riskier than the next poll. --- packages/cli/src/lib/api/preprod-artifacts.ts | 4 ++++ packages/cli/test/lib/api/preprod-artifacts.test.ts | 2 ++ 2 files changed, 6 insertions(+) diff --git a/packages/cli/src/lib/api/preprod-artifacts.ts b/packages/cli/src/lib/api/preprod-artifacts.ts index 70a2730f7..c96176c97 100644 --- a/packages/cli/src/lib/api/preprod-artifacts.ts +++ b/packages/cli/src/lib/api/preprod-artifacts.ts @@ -273,6 +273,10 @@ export async function uploadBuild( method: "POST", body, schema: BuildAssembleResponseSchema, + // This loop already re-sends the identical body to poll, and the server + // reuses the in-progress artifact row (get_or_create), so a transport + // retry is no riskier than the next poll. + idempotent: true, }); if (data.state === "error") { diff --git a/packages/cli/test/lib/api/preprod-artifacts.test.ts b/packages/cli/test/lib/api/preprod-artifacts.test.ts index 165098c2c..dca4acca5 100644 --- a/packages/cli/test/lib/api/preprod-artifacts.test.ts +++ b/packages/cli/test/lib/api/preprod-artifacts.test.ts @@ -270,6 +270,8 @@ describe("uploadBuild", () => { expect(body.build_configuration).toBe("Release"); expect(body.release_notes).toBe("notes"); expect(body.install_groups).toEqual(["qa", "beta"]); + // Opted into transport retries: the poll loop re-sends the same body anyway. + expect(call?.[2]?.idempotent).toBe(true); // No missing chunks on the first (and only) response. expect(uploadMissingChunksMock).not.toHaveBeenCalled(); }); From 8e1c9a9cf6ea3b91294618113e402670cf72fb0f Mon Sep 17 00:00:00 2001 From: "sentry-junior[bot]" <264270552+sentry-junior[bot]@users.noreply.github.com> Date: Tue, 27 Oct 2026 15:23:46 +0000 Subject: [PATCH 4/4] fix(cli): Give RateLimitError its own exit code; cite RFC 9110 for retry methods - RateLimitError now exits with EXIT.RATE_LIMITED (32) so scripts and agents can back off without parsing stderr. Documented in exit-codes.md. - Rename IDEMPOTENT_METHODS to SAFE_METHODS and reference RFC 9110 9.2.1/9.2.2; note the retryable status list mirrors common client defaults since HTTP defines none. --- apps/cli-docs/src/content/docs/exit-codes.md | 1 + packages/cli/src/lib/errors.ts | 4 ++++ packages/cli/src/lib/retry-policy.ts | 23 +++++++++++--------- packages/cli/test/lib/errors.test.ts | 7 ++++++ 4 files changed, 25 insertions(+), 10 deletions(-) diff --git a/apps/cli-docs/src/content/docs/exit-codes.md b/apps/cli-docs/src/content/docs/exit-codes.md index a7c56051b..0c85b0a25 100644 --- a/apps/cli-docs/src/content/docs/exit-codes.md +++ b/apps/cli-docs/src/content/docs/exit-codes.md @@ -35,6 +35,7 @@ react to failure categories without parsing stderr. | 23 | Not Found | A user-provided identifier could not be resolved | | 30 | API Error | Sentry API returned an error response | | 31 | Timeout | Operation exceeded its time limit | +| 32 | Rate Limited | Sentry API rate limit hit and the CLI could not wait it out — wait for the time in the error message, then retry with fewer parallel requests | | 40 | Seer Not Enabled | Seer is not enabled for the organization | | 41 | Seer No Budget | Seer requires a paid plan | | 42 | AI Disabled | AI features disabled by organization admin | diff --git a/packages/cli/src/lib/errors.ts b/packages/cli/src/lib/errors.ts index 57773585e..71fb08bd2 100644 --- a/packages/cli/src/lib/errors.ts +++ b/packages/cli/src/lib/errors.ts @@ -71,6 +71,8 @@ export const EXIT = { API: 30, /** Operation timed out */ TIMEOUT: 31, + /** Rate limited by the Sentry API — wait before retrying */ + RATE_LIMITED: 32, // 40–49: Feature / billing (HTTP 402/451 family) /** Seer not enabled for the organization */ @@ -221,6 +223,8 @@ export class ApiError extends CliError { * @param serverDetail - The API's `detail` message, when present */ export class RateLimitError extends ApiError { + /** Distinct exit code so scripts and agents can back off without parsing stderr. */ + override readonly exitCode: number = EXIT.RATE_LIMITED; readonly retryAfterMs?: number; constructor(endpoint: string, retryAfterMs?: number, serverDetail?: string) { diff --git a/packages/cli/src/lib/retry-policy.ts b/packages/cli/src/lib/retry-policy.ts index 976e8a9ac..aeebfbde9 100644 --- a/packages/cli/src/lib/retry-policy.ts +++ b/packages/cli/src/lib/retry-policy.ts @@ -5,7 +5,7 @@ * long to wait first. Kept pure (no CLI imports, no I/O) so it can be swapped * for the shared toolkit-core policy without touching callers. * - * Only idempotent methods are replayed by default. A transient status, a + * Only safe methods (RFC 9110 §9.2.1) are replayed by default. A transient status, a * network error, or a timeout after a POST does not tell us whether the * server already applied it, so replaying could duplicate the write. * PUT and DELETE are excluded too: Sentry uses PUT for partial updates with @@ -19,19 +19,22 @@ */ /** - * HTTP statuses that indicate a transient failure worth retrying. Not a - * blanket 5xx: 501, 505 and similar are deterministic. + * HTTP statuses that indicate a transient failure worth retrying. HTTP has no + * normative "retryable" list; this matches common client defaults (undici's + * RetryAgent, ky) plus 408. Not a blanket 5xx: 501, 505 and similar are + * deterministic. */ const RETRYABLE_STATUS_CODES: ReadonlySet = new Set([ 408, 429, 500, 502, 503, 504, ]); -/** Methods that are safe to replay without an explicit opt-in. */ -const IDEMPOTENT_METHODS: ReadonlySet = new Set([ - "GET", - "HEAD", - "OPTIONS", -]); +/** + * RFC 9110 §9.2.1 safe methods (TRACE omitted, the CLI never sends it). + * Deliberately narrower than the §9.2.2 idempotent set, which also includes + * PUT and DELETE; see the module comment for why Sentry's aren't safe to + * replay. + */ +const SAFE_METHODS: ReadonlySet = new Set(["GET", "HEAD", "OPTIONS"]); /** First fallback backoff step; doubles per attempt up to the cap. */ const BASE_BACKOFF_MS = 1000; @@ -44,7 +47,7 @@ const BASE_BACKOFF_MS = 1000; * @param idempotent - Caller asserts the request is safe to replay */ export function isRetryableMethod(method: string, idempotent = false): boolean { - return idempotent || IDEMPOTENT_METHODS.has(method.toUpperCase()); + return idempotent || SAFE_METHODS.has(method.toUpperCase()); } /** diff --git a/packages/cli/test/lib/errors.test.ts b/packages/cli/test/lib/errors.test.ts index 376893d58..6806cc158 100644 --- a/packages/cli/test/lib/errors.test.ts +++ b/packages/cli/test/lib/errors.test.ts @@ -16,6 +16,7 @@ import { isSearchQueryParseError, isUserError, OutputError, + RateLimitError, ResolutionError, SeerError, stringifyUnknown, @@ -721,6 +722,12 @@ describe("exit codes", () => { expect(new ApiError("fail", 500).exitCode).toBe(EXIT.API); }); + test("RateLimitError has exit code RATE_LIMITED", () => { + const err = new RateLimitError("/api/0/organizations/", 2000); + expect(err.exitCode).toBe(EXIT.RATE_LIMITED); + expect(err.status).toBe(429); + }); + test("ConfigError has exit code CONFIG", () => { expect(new ConfigError("bad config").exitCode).toBe(EXIT.CONFIG); });