diff --git a/docs/contributing/api-patterns.md b/docs/contributing/api-patterns.md index 106b6a53e..01d1b07f5 100644 --- a/docs/contributing/api-patterns.md +++ b/docs/contributing/api-patterns.md @@ -280,7 +280,11 @@ Retries are: out than the single-wait cap fails at once with an `ApiRateLimitError` whose `retryAfterMs` and user message say when to retry. - **Abortable** — aborting the request's `signal` ends the wait. -- **Fail-fast otherwise** — network and configuration errors are not retried. +- **Network failures retry for idempotent requests** — when `fetch` itself + fails (DNS, connection refused or reset, socket timeout), only GET, HEAD, + and OPTIONS retry, since a write may already have reached Sentry. +- **Fail-fast otherwise** — configuration errors (such as a malformed token) + are not retried. A rate-limited tool call returns a **Rate Limited** message with the retry time, not an input error. - **Bypassed by `allowStatuses`** — a status the caller opts into is returned @@ -289,6 +293,12 @@ Retries are: Callers and tools need no changes: a retried request either resolves normally or throws the same error it would have thrown without retries. +The CLI's transport (`packages/cli/src/lib/sentry-client.ts`) follows the same +shared rule. Only the caps differ (`RETRY_POLICY`): the hosted MCP keeps its +total wait short so a Workers request isn't held open, while the CLI can wait +longer. The CLI also applies its own per-request timeout, which counts as a +network failure. + ## Best Practices 1. **Always use context helper** when in tools/prompts diff --git a/packages/cli/script/generate-api-schema.ts b/packages/cli/script/generate-api-schema.ts index cf128099d..49ec12ecc 100644 --- a/packages/cli/script/generate-api-schema.ts +++ b/packages/cli/script/generate-api-schema.ts @@ -61,7 +61,7 @@ export type { ApiEndpoint } from "../src/lib/api-schema.js"; import type { ApiEndpoint } from "../src/lib/api-schema.js"; import type { IdempotentOperation } from "../src/lib/idempotent-operations.js"; -import { isRetryableMethod } from "../src/lib/retry-policy.js"; +import { isIdempotentRequest } from "@sentry/toolkit-core/api-retry"; // --------------------------------------------------------------------------- // OpenAPI Types (minimal subset we need) @@ -244,7 +244,7 @@ console.log( // Safe methods already retry, so only writes need listing; that keeps the // file the CLI loads on every request tiny. const idempotentOperations: IdempotentOperation[] = operations - .filter((op) => op.idempotent && !isRetryableMethod(op.method)) + .filter((op) => op.idempotent && !isIdempotentRequest(op.method)) .map((op) => ({ method: op.method, path: op.path })) .sort( (a, b) => a.path.localeCompare(b.path) || a.method.localeCompare(b.method), diff --git a/packages/cli/src/lib/retry-policy.ts b/packages/cli/src/lib/retry-policy.ts deleted file mode 100644 index bb9ec4c36..000000000 --- a/packages/cli/src/lib/retry-policy.ts +++ /dev/null @@ -1,165 +0,0 @@ -/** - * 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. Writes Sentry marks - * idempotent (`x-sentry-idempotent`, see idempotent-operations.ts) replay, - * and callers that know an unmarked write is safe to replay 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 ffc891d6a..426eaa9d5 100644 --- a/packages/cli/src/lib/sentry-client.ts +++ b/packages/cli/src/lib/sentry-client.ts @@ -11,6 +11,12 @@ import { setTimeout as sleepMs } from "node:timers/promises"; import type { SentryClient } from "@sentry/api"; import { getTraceData } from "@sentry/node"; +import { + isIdempotentRequest, + nextRetryDelayMs, + parseRetryDelayMs, + type RetryPolicy, +} from "@sentry/toolkit-core/api-retry"; import { computeInvalidationPrefixes } from "@sentry/toolkit-core/response-cache"; import { formatAuthHeader, @@ -55,11 +61,6 @@ import { lookupCachedResponse, storeCachedResponse, } from "./response-cache.js"; -import { - backoffDelayMs, - decideStatusRetry, - isRetryableMethod, -} from "./retry-policy.js"; import { normalizeHttpOrigin, normalizeOrigin } from "./sentry-urls.js"; import { isManualHttpTracing, withTracingSpan } from "./telemetry.js"; import { parseSntrysClaim } from "./token-claims.js"; @@ -88,16 +89,22 @@ const ENDPOINT_TIMEOUT_OVERRIDES: TimeoutOverride[] = [ { pattern: /\/autofix\/?(?:\?|$)/, timeoutMs: 120_000 }, ]; -/** Maximum retry attempts for failed requests */ -const MAX_RETRIES = 2; - /** - * 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. + * Caps for automatic retries. Which statuses and methods retry is the shared + * rule in `@sentry/toolkit-core/api-retry`. + * + * `maxDelayMs` 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. The per-wait + * cap binds first; the total only bounds the worst case. */ -const MAX_BACKOFF_MS = 10_000; +const RETRY_POLICY: RetryPolicy = { + maxRetries: 2, + baseDelayMs: 1000, + maxDelayMs: 10_000, + maxTotalDelayMs: 30_000, +}; /** Header to mark requests as retries, preventing infinite token refresh loops */ const RETRY_MARKER_HEADER = "x-sentry-cli-retry"; @@ -398,13 +405,34 @@ type AttemptResult = /** Per-attempt retry inputs. */ type AttemptRetryState = { + method: string; + /** Sentry marks the operation idempotent, or the caller opted in. */ + idempotent: boolean; + /** Retries already made for this request, starting at 0. */ attempt: number; - /** Method is idempotent or the caller opted in; see retry-policy.ts. */ - methodRetryable: boolean; + /** Total time already spent waiting between attempts. */ + waitedMs: number; /** Request path, for error messages. */ urlPath: string; }; +/** Delay before retrying after `status` (undefined for a network error or timeout). */ +function retryDelayMs( + { method, idempotent, attempt, waitedMs }: AttemptRetryState, + status: number | undefined, + serverDelayMs: number | undefined, +): number | undefined { + return nextRetryDelayMs({ + policy: RETRY_POLICY, + method, + idempotent, + status, + serverDelayMs, + attempt, + waitedMs, + }); +} + /** Best-effort `detail` from a Sentry error body. */ async function readErrorDetail( response: Response, @@ -422,17 +450,17 @@ async function readErrorDetail( * Decide what to do with a successful HTTP response. * * 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 + * the request before processing it. Other transient statuses retry per the + * shared policy in `@sentry/toolkit-core/api-retry`. 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, - { attempt, methodRetryable, urlPath }: AttemptRetryState, + retryState: AttemptRetryState, ): Promise { - if (response.status === 401 && attempt < MAX_RETRIES) { + if (response.status === 401 && retryState.attempt < RETRY_POLICY.maxRetries) { const refreshed = await handleUnauthorized(headers, credential); if (refreshed) { await cancelResponseBody(response); @@ -441,25 +469,18 @@ async function handleResponse( return { action: "done", response }; } - const decision = decideStatusRetry({ - status: response.status, - headers: response.headers, - methodRetryable, - attempt, - maxRetries: MAX_RETRIES, - maxDelayMs: MAX_BACKOFF_MS, - nowMs: Date.now(), - }); - if (decision.retry) { + const serverDelayMs = parseRetryDelayMs(response.status, response.headers); + const delayMs = retryDelayMs(retryState, response.status, serverDelayMs); + if (delayMs !== undefined) { await cancelResponseBody(response); - return { action: "retry", delayMs: decision.delayMs }; + return { action: "retry", delayMs }; } if (response.status === 429) { return { action: "throw", error: new RateLimitError( - urlPath, - decision.retryAfterMs, + retryState.urlPath, + serverDelayMs, await readErrorDetail(response), ), }; @@ -477,9 +498,8 @@ async function handleResponse( function handleFetchError( error: unknown, signal: AbortSignal | undefined | null, - { attempt, methodRetryable }: AttemptRetryState, + retryState: AttemptRetryState, ): AttemptResult { - const canRetry = methodRetryable && attempt < MAX_RETRIES; if (error instanceof HostScopeError) { return { action: "throw", error }; } @@ -499,7 +519,8 @@ function handleFetchError( }; } - if (isInternalTimeout(error) && !canRetry) { + const delayMs = retryDelayMs(retryState, undefined, undefined); + if (isInternalTimeout(error) && delayMs === undefined) { const timeoutMs = (error as { timeoutMs?: number }).timeoutMs ?? REQUEST_TIMEOUT_MS; const seconds = Math.round(timeoutMs / 1000); @@ -507,16 +528,16 @@ function handleFetchError( action: "throw", error: new TimeoutError( `Request timed out after ${seconds}s.`, - methodRetryable + isIdempotentRequest(retryState.method, retryState.idempotent) ? "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 (!canRetry) { + if (delayMs === undefined) { return { action: "throw", error }; } - return { action: "retry", delayMs: backoffDelayMs(attempt, MAX_BACKOFF_MS) }; + return { action: "retry", delayMs }; } /** Extract the URL pathname for logging and cache keys */ @@ -660,8 +681,9 @@ async function buildAttemptFactory( /** * Authenticate and execute a request with retry logic. * - * Refreshes the auth token, then retries the request up to `MAX_RETRIES` - * times on transient errors (retry-policy.ts decides which and how long). + * Refreshes the auth token, then retries the request up to + * `RETRY_POLICY.maxRetries` times on transient errors + * (`@sentry/toolkit-core/api-retry` decides which and how long). */ type RetryContext = { input: Request | string | URL; @@ -709,20 +731,25 @@ async function fetchWithRetry({ } const attemptFactory = await buildAttemptFactory(input, init); const timeoutMs = resolveTimeoutMs(fullUrl); - const methodRetryable = isRetryableMethod( - method, - idempotent || isIdempotentOperation(method, fullUrl), - ); + const operationIdempotent = + idempotent || isIdempotentOperation(method, fullUrl); const urlPath = extractUrlPath(fullUrl); + let waitedMs = 0; - for (let attempt = 0; attempt <= MAX_RETRIES; attempt++) { + for (let attempt = 0; attempt <= RETRY_POLICY.maxRetries; attempt++) { const { input: attemptInput, init: attemptInit } = attemptFactory(); const result = await executeAttempt({ input: attemptInput, init: attemptInit, headers, credential: effectiveCredential, - retryState: { attempt, methodRetryable, urlPath }, + retryState: { + method, + idempotent: operationIdempotent, + attempt, + waitedMs, + urlPath, + }, timeoutMs, validatedRedirects, responseBodyMode, @@ -751,9 +778,10 @@ async function fetchWithRetry({ } log.debug( - `${method} ${urlPath} → retry ${attempt + 1}/${MAX_RETRIES} after ${result.delayMs}ms`, + `${method} ${urlPath} → retry ${attempt + 1}/${RETRY_POLICY.maxRetries} after ${Math.round(result.delayMs)}ms`, ); await sleepMs(result.delayMs); + waitedMs += result.delayMs; } // Unreachable: the last attempt always returns 'done' or 'throw' @@ -767,7 +795,7 @@ 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), network errors, + * - Automatic retry on transient HTTP errors (429, 500, 502-504), network errors, * and timeouts — for GET/HEAD/OPTIONS, operations Sentry marks idempotent * (see idempotent-operations.ts), or any method with `idempotent` * - 429 handling (any method; rejected before processing): waits for the diff --git a/packages/cli/test/lib/retry-policy.test.ts b/packages/cli/test/lib/retry-policy.test.ts deleted file mode 100644 index 3e3722be2..000000000 --- a/packages/cli/test/lib/retry-policy.test.ts +++ /dev/null @@ -1,177 +0,0 @@ -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 b86907027..e87956a84 100644 --- a/packages/cli/test/lib/sentry-client.test.ts +++ b/packages/cli/test/lib/sentry-client.test.ts @@ -399,7 +399,8 @@ describe("429 rate limiting", () => { if (calls > 1) { return jsonResponse({ ok: true }); } - // Server clock an hour behind ours: the Date header keeps the wait at 1s. + // Server clock an hour behind ours: the Date header bounds the wait to + // the 1s window (plus jitter) instead of an hour. const serverNow = Math.floor(Date.now() / 1000) - 3600; return jsonResponse( { detail: "Limit is 40 requests in 1 seconds" }, @@ -420,7 +421,7 @@ describe("429 rate limiting", () => { expect(res.status).toBe(200); expect(calls).toBe(2); - expect(Date.now() - started).toBeGreaterThanOrEqual(900); + expect(Date.now() - started).toBeLessThan(2000); }); test("throws RateLimitError with the wait when it exceeds the cap", async () => { @@ -465,7 +466,7 @@ describe("429 rate limiting", () => { expect(error).toBeInstanceOf(RateLimitError); expect((error as RateLimitError).format()).toContain("Retry in 1 second;"); - // MAX_RETRIES = 2 → attempts 0, 1, 2. + // RETRY_POLICY.maxRetries = 2 → attempts 0, 1, 2. expect(calls).toBe(3); }); @@ -580,7 +581,7 @@ describe("fetchWithTimeout internal timeout classification", () => { expect((thrown as TimeoutError).message).toContain( "Request timed out after", ); - // MAX_RETRIES = 2 → attempts 0, 1, 2. + // RETRY_POLICY.maxRetries = 2 → attempts 0, 1, 2. expect(calls).toBe(3); } finally { restore(); diff --git a/packages/mcp-core/src/api-client/client.test.ts b/packages/mcp-core/src/api-client/client.test.ts index a5c1264d2..db2a4f9a6 100644 --- a/packages/mcp-core/src/api-client/client.test.ts +++ b/packages/mcp-core/src/api-client/client.test.ts @@ -1528,6 +1528,89 @@ describe("transient 5xx retry", () => { }); }); +describe("network failure retry", () => { + let originalFetch: typeof globalThis.fetch; + + const connectionReset = () => { + const error = new Error("fetch failed"); + error.cause = new Error("read ECONNRESET"); + return error; + }; + + beforeEach(() => { + originalFetch = globalThis.fetch; + vi.useFakeTimers(); + }); + + afterEach(() => { + globalThis.fetch = originalFetch; + vi.useRealTimers(); + }); + + it("retries a GET after a network failure and succeeds", async () => { + const fetchMock = vi + .fn() + .mockRejectedValueOnce(connectionReset()) + .mockResolvedValueOnce( + new Response( + JSON.stringify({ + id: "1", + name: "Test User", + email: "t@example.com", + }), + { status: 200, headers: { "Content-Type": "application/json" } }, + ), + ); + globalThis.fetch = fetchMock; + + const apiService = new SentryApiService({ + host: "sentry.io", + accessToken: "test-token", + }); + + const promise = apiService.getAuthenticatedUser(); + await vi.runAllTimersAsync(); + + expect((await promise).id).toBe("1"); + expect(fetchMock).toHaveBeenCalledTimes(2); + }); + + it("does not retry a POST after a network failure", async () => { + const fetchMock = vi.fn().mockRejectedValue(connectionReset()); + globalThis.fetch = fetchMock; + + const apiService = new SentryApiService({ + host: "sentry.io", + accessToken: "test-token", + }); + + const promise = apiService.createTeam({ + organizationSlug: "my-org", + name: "My Team", + }); + promise.catch(() => {}); + await vi.runAllTimersAsync(); + + await expect(promise).rejects.toThrow(/Connection reset/); + expect(fetchMock).toHaveBeenCalledTimes(1); + }); + + it("does not retry a malformed token, which fails before fetch", async () => { + const fetchMock = vi.fn(); + globalThis.fetch = fetchMock; + + const apiService = new SentryApiService({ + host: "sentry.io", + accessToken: "bad\ntoken", + }); + + await expect(apiService.getAuthenticatedUser()).rejects.toThrow( + /Malformed authentication token/, + ); + expect(fetchMock).not.toHaveBeenCalled(); + }); +}); + describe("rate limit retry", () => { let originalFetch: typeof globalThis.fetch; const now = Date.UTC(2026, 0, 1, 12, 0, 0, 300); diff --git a/packages/mcp-core/src/api-client/client.ts b/packages/mcp-core/src/api-client/client.ts index 1184f3b8c..0545e7d7e 100644 --- a/packages/mcp-core/src/api-client/client.ts +++ b/packages/mcp-core/src/api-client/client.ts @@ -321,6 +321,13 @@ const REQUEST_RETRY_POLICY: RetryPolicy = { maxTotalDelayMs: 3000, }; +/** + * Errors thrown because `fetch` itself failed (DNS, connection refused or + * reset, socket timeout), as opposed to configuration checks before the + * request. The shared policy retries these for idempotent requests only. + */ +const fetchFailures = new WeakSet(); + const API_MAX_PER_PAGE = 100; const MAX_LIST_PAGES = 50; @@ -771,12 +778,13 @@ export class SentryApiService { /** * Makes an authenticated request to the Sentry API, transparently retrying - * rate-limited (429) requests and 500/502/503/504 GET and HEAD requests - * under `REQUEST_RETRY_POLICY`. + * rate-limited (429) requests, and GET and HEAD requests that hit a + * 500/502/503/504 or a network failure, under `REQUEST_RETRY_POLICY`. * * A 429 waits for its `Retry-After` or `X-Sentry-Rate-Limit-Reset` delay; - * everything else uses jittered backoff. Writes that hit a 5xx, other 4xx - * errors, and network/configuration errors fail fast. Callers that opt + * everything else uses jittered backoff. Writes that hit a 5xx or a network + * failure, other 4xx errors, and configuration checks before the request + * fail fast. Callers that opt * a status into `allowStatuses` receive the raw response from `requestOnce` * and are never retried. Aborting `options.signal` ends any wait. * @@ -799,13 +807,18 @@ export class SentryApiService { try { return await this.requestOnce(path, options, requestOptions); } catch (error) { - if (!(error instanceof ApiError) || error.status === undefined) { + const status = error instanceof ApiError ? error.status : undefined; + const networkFailure = + error instanceof Error && + fetchFailures.has(error) && + !options.signal?.aborted; + if (status === undefined && !networkFailure) { throw error; } const delayMs = nextRetryDelayMs({ policy: REQUEST_RETRY_POLICY, method, - status: error.status, + status, serverDelayMs: error instanceof ApiRateLimitError ? error.retryAfterMs : undefined, attempt, @@ -915,10 +928,16 @@ export class SentryApiService { errorCode === "ETIMEDOUT" || errorMessage.includes("Connect Timeout Error") ) { - throw new ConfigurationError(friendlyMessage, { cause: error }); + const configError = new ConfigurationError(friendlyMessage, { + cause: error, + }); + fetchFailures.add(configError); + throw configError; } - throw new Error(friendlyMessage, { cause: error }); + const networkError = new Error(friendlyMessage, { cause: error }); + fetchFailures.add(networkError); + throw networkError; } // Handle error responses generically diff --git a/packages/toolkit-core/src/api-retry.test.ts b/packages/toolkit-core/src/api-retry.test.ts index 6f6998366..99071ecff 100644 --- a/packages/toolkit-core/src/api-retry.test.ts +++ b/packages/toolkit-core/src/api-retry.test.ts @@ -1,5 +1,6 @@ import { afterEach, describe, expect, it, vi } from "vitest"; import { + isIdempotentRequest, nextRetryDelayMs, parseRetryDelayMs, type RetryPolicy, @@ -232,6 +233,21 @@ describe("nextRetryDelayMs", () => { } }); + it("retries a network error or timeout only for idempotent requests", () => { + expect(nextRetryDelayMs({ ...base, status: undefined })).toBe(125); + expect( + nextRetryDelayMs({ ...base, method: "POST", status: undefined }), + ).toBeUndefined(); + expect( + nextRetryDelayMs({ + ...base, + method: "POST", + status: undefined, + idempotent: true, + }), + ).toBe(125); + }); + it("does not retry deterministic 5xx codes or other 4xx", () => { for (const status of [400, 404, 501, 505]) { expect(nextRetryDelayMs({ ...base, status })).toBeUndefined(); @@ -239,6 +255,21 @@ describe("nextRetryDelayMs", () => { }); }); +describe("isIdempotentRequest", () => { + it("replays only GET, HEAD, and OPTIONS by default", () => { + for (const method of ["GET", "head", "OPTIONS"]) { + expect(isIdempotentRequest(method)).toBe(true); + } + for (const method of ["POST", "PUT", "PATCH", "DELETE"]) { + expect(isIdempotentRequest(method)).toBe(false); + } + }); + + it("replays any method when the caller opts in", () => { + expect(isIdempotentRequest("POST", true)).toBe(true); + }); +}); + describe("sleep", () => { afterEach(() => { vi.useRealTimers(); diff --git a/packages/toolkit-core/src/api-retry.ts b/packages/toolkit-core/src/api-retry.ts index d4e667c19..ce6c618b6 100644 --- a/packages/toolkit-core/src/api-retry.ts +++ b/packages/toolkit-core/src/api-retry.ts @@ -50,21 +50,34 @@ const RETRYABLE_STATUSES: ReadonlyMap = new Map([ /** The response `Date` header truncates server time to whole seconds. */ const DATE_RESOLUTION_MS = 1000; -/** Whether `RETRYABLE_STATUSES` lets this failed request be sent again. */ +/** + * Whether a request may be replayed after the endpoint may have run: a + * transient 5xx, a network error, or a timeout. True for GET, HEAD, and + * OPTIONS, or when the caller marks the request `idempotent`. + */ +export function isIdempotentRequest( + method: string, + idempotent = false, +): boolean { + return idempotent || IDEMPOTENT_METHODS.has(method.toUpperCase()); +} + +/** + * Whether `RETRYABLE_STATUSES` lets this failed request be sent again. With + * no status (a network error or timeout), the endpoint may have run, so only + * idempotent requests replay. + */ function isRetryable( method: string, - status: number, + status: number | undefined, idempotent = false, ): boolean { - const replays = RETRYABLE_STATUSES.get(status); + const replays = + status === undefined ? "idempotent" : RETRYABLE_STATUSES.get(status); if (replays === undefined) { return false; } - return ( - replays === "any" || - idempotent || - IDEMPOTENT_METHODS.has(method.toUpperCase()) - ); + return replays === "any" || isIdempotentRequest(method, idempotent); } /** @@ -118,7 +131,8 @@ export function nextRetryDelayMs({ policy: RetryPolicy; method: string; idempotent?: boolean; - status: number; + /** Failed response status, or undefined for a network error or timeout. */ + status: number | undefined; /** From `parseRetryDelayMs` for the failed response. */ serverDelayMs: number | undefined; /** Retries already made for this request, starting at 0. */