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/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/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/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..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 */ @@ -212,6 +214,37 @@ export class ApiError extends CliError { } } +/** + * 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 + * @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) { + const seconds = Math.max(1, Math.ceil((retryAfterMs ?? 0) / 1000)); + const wait = + retryAfterMs === undefined + ? "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 ${advice}` : advice, + 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..aeebfbde9 --- /dev/null +++ b/packages/cli/src/lib/retry-policy.ts @@ -0,0 +1,163 @@ +/** + * 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 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 + * 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 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, +]); + +/** + * 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; + +/** + * 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 || SAFE_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 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; + 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 && status !== 429) || + 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..6ef6b2198 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,13 @@ 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 (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 * - Automatic HTTP span tracing for every request * @@ -722,6 +778,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 +849,7 @@ function createAuthenticatedFetch( fullUrl, credential, validatedRedirects: options.validatedRedirects === true, + idempotent: options.idempotent === true, responseBodyMode: method.toUpperCase() !== "GET" ? "stream" @@ -817,7 +880,7 @@ type ExecuteAttemptArgs = { init: RequestInit | undefined; headers: Headers; credential: CredentialContext; - isLastAttempt: boolean; + retryState: AttemptRetryState; timeoutMs: number; validatedRedirects: boolean; responseBodyMode: ResponseBodyMode; @@ -947,7 +1010,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 +1073,7 @@ async function executeAttempt({ init, headers, credential, - isLastAttempt, + retryState, timeoutMs, validatedRedirects, responseBodyMode, @@ -1041,11 +1104,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 +1124,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/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(); }); 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); }); 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..3e3722be2 --- /dev/null +++ b/packages/cli/test/lib/retry-policy.test.ts @@ -0,0 +1,177 @@ +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", () => { + for (const status of [404, 501, 505]) { + expect(decideStatusRetry({ ...base, status })).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 on 5xx", () => { + const headers = new Headers({ "Retry-After": "1" }); + expect( + decideStatusRetry({ + ...base, + 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 89df80c7d..3b20cfca0 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,260 @@ 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("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 () => { + 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( + "Retry in 45 seconds; do not repeat the request before then. Make fewer requests in parallel.", + ); + }); + + 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" } }, + ); + }); + + 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); + }); + + 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("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 res = await getAuthenticatedFetch()( + `${REGION_URL}/api/0/organizations/acme/issues/1/autofix/`, + { 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(error).toBeInstanceOf(RateLimitError); + expect((error as RateLimitError).retryAfterMs).toBeUndefined(); + expect((error as RateLimitError).format()).toContain( + "Retry in a few seconds; do not repeat the request before then.", + ); + }); +}); + 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 +540,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", () => {