diff --git a/README.md b/README.md index 255876e24..5ab2e9d7a 100644 --- a/README.md +++ b/README.md @@ -177,6 +177,15 @@ self-hosted deployment that does run Seer, opt in with `MCP_SKILLS`: } ``` +### Rate Limits + +Tool calls are subject to the Sentry API's rate limits. The MCP server retries +rate-limited requests, and transient Sentry server errors on read requests, +after a short wait. When a limit lasts longer, the tool returns a **Rate +Limited** message that says when to try again. Agents hit the per-second +limits most often by running many Sentry tool calls in parallel, so run them +one at a time if you see this message. + ### MCP Inspector MCP includes an [Inspector](https://modelcontextprotocol.io/docs/tools/inspector), to easily test the service: diff --git a/docs/contributing/api-patterns.md b/docs/contributing/api-patterns.md index 3510c77fd..82f86e5dc 100644 --- a/docs/contributing/api-patterns.md +++ b/docs/contributing/api-patterns.md @@ -73,7 +73,7 @@ fail to resolve for legacy mixed-case project slugs; keep them for display. Organization discovery uses `/api/0/organizations/`; a limit over 100 follows bounded pages through Sentry's Link cursors. Organization, team, and project lists use `@sentry/api` operations to build GET paths and queries. The MCP -client still owns the request transport, token handling, GET retries, error +client still owns the request transport, token handling, retries, error types, and Zod response validation. Public SaaS hosts use `sentry.io` to list organizations across regions. Single-tenant hosts under `*.my.sentry.io` and self-hosted instances keep their configured host for this request. @@ -252,18 +252,35 @@ See error patterns in [common-patterns.md](common-patterns.md#error-handling). ### Automatic Retries -`SentryApiService.request()` transparently retries transient upstream gateway -failures (`502`/`503`/`504`) using exponential backoff. Retries are: - -- **GET-only** — non-idempotent methods (`POST`/`PUT`/`DELETE`) are never - retried, so an automatic retry can never replay a mutation. -- **Gateway-only** — gated on `ApiServerError.isGatewayError()`. Persistent - `500`s, `4xx` client errors, and network/configuration errors fail fast. +`SentryApiService.request()` transparently retries failed requests using the +shared policy in `@sentry/toolkit-core/api-retry`. This page describes the +behavior; the values live only in code, so read them there instead of copying +them here: + +- Which statuses retry, and for which methods: `RETRYABLE_STATUSES` in + [`api-retry.ts`](../../packages/toolkit-core/src/api-retry.ts). In short, a + rate-limited request retries for any method, a transient server error only + for an idempotent one, and other errors never. +- How many retries and how long they may wait: `REQUEST_RETRY_POLICY` in + [`client.ts`](../../packages/mcp-core/src/api-client/client.ts). + +Retries are: + +- **Rate-limit aware** — a `429` waits for its `Retry-After` header, or for + Sentry's `X-Sentry-Rate-Limit-Reset`. Concurrency-limit `429`s and `5xx` + errors use jittered exponential backoff. +- **Capped** — by retry count, single wait, and total wait. A reset further + 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. + 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 as-is from `requestOnce`, never retried. Callers and tools need no changes: a retried request either resolves normally or -throws the same `ApiServerError` it would have thrown without retries. +throws the same error it would have thrown without retries. ## Best Practices diff --git a/packages/mcp-core/src/api-client/client.test.ts b/packages/mcp-core/src/api-client/client.test.ts index 981acad19..a5c1264d2 100644 --- a/packages/mcp-core/src/api-client/client.test.ts +++ b/packages/mcp-core/src/api-client/client.test.ts @@ -11,6 +11,7 @@ import { SentryApiService } from "./client"; import { ApiNotFoundError, ApiPermissionError, + ApiRateLimitError, ApiServerError, ApiValidationError, } from "./errors"; @@ -1442,8 +1443,32 @@ describe("transient 5xx retry", () => { expect(fetchMock).toHaveBeenCalledTimes(3); }); - it("does not retry a non-gateway 500", async () => { - const fetchMock = vi.fn().mockResolvedValue(internalError()); + it("retries a 500 on a GET", async () => { + const fetchMock = vi + .fn() + .mockResolvedValueOnce(internalError()) + .mockResolvedValueOnce(authOk()); + 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 deterministic 5xx such as 501", async () => { + const fetchMock = vi.fn().mockResolvedValue( + new Response(JSON.stringify({ detail: "Not Implemented" }), { + status: 501, + headers: { "Content-Type": "application/json" }, + }), + ); globalThis.fetch = fetchMock; const apiService = new SentryApiService({ @@ -1503,6 +1528,192 @@ describe("transient 5xx retry", () => { }); }); +describe("rate limit retry", () => { + let originalFetch: typeof globalThis.fetch; + const now = Date.UTC(2026, 0, 1, 12, 0, 0, 300); + const nowSeconds = Math.floor(now / 1000); + + // Shaped like Sentry's rate-limit middleware response for a fixed window. + const rateLimited = (resetSeconds: number, headers = {}) => + new Response( + JSON.stringify({ + detail: + "You are attempting to use this endpoint too frequently. Limit is 40 requests in 1 seconds", + }), + { + status: 429, + statusText: "Too Many Requests", + headers: { + "Content-Type": "application/json", + "X-Sentry-Rate-Limit-Remaining": "0", + "X-Sentry-Rate-Limit-Limit": "40", + "X-Sentry-Rate-Limit-Reset": String(resetSeconds), + ...headers, + }, + }, + ); + + const authOk = () => + new Response( + JSON.stringify({ id: "1", name: "Test User", email: "test@example.com" }), + { status: 200, headers: { "Content-Type": "application/json" } }, + ); + + const rawRequest = (apiService: SentryApiService) => + ( + apiService as unknown as { + request: (path: string, options?: RequestInit) => Promise; + } + ).request.bind(apiService); + + const apiService = () => + new SentryApiService({ host: "sentry.io", accessToken: "test-token" }); + + beforeEach(() => { + originalFetch = globalThis.fetch; + vi.useFakeTimers({ now }); + }); + + afterEach(() => { + globalThis.fetch = originalFetch; + vi.useRealTimers(); + }); + + it("waits for the rate-limit reset and retries a GET", async () => { + const fetchMock = vi + .fn() + .mockResolvedValueOnce(rateLimited(nowSeconds + 1)) + .mockResolvedValueOnce(authOk()); + globalThis.fetch = fetchMock; + + const promise = apiService().getAuthenticatedUser(); + // The reset is 700ms away; no retry before then. + await vi.advanceTimersByTimeAsync(699); + expect(fetchMock).toHaveBeenCalledTimes(1); + await vi.runAllTimersAsync(); + + expect((await promise).id).toBe("1"); + expect(fetchMock).toHaveBeenCalledTimes(2); + }); + + it("retries a concurrency-limit 429 with backoff", async () => { + const concurrencyLimited = new Response( + JSON.stringify({ + detail: + "You are attempting to go above the allowed concurrency for this endpoint. Concurrency limit is 25", + }), + { + status: 429, + headers: { + "Content-Type": "application/json", + "X-Sentry-Rate-Limit-Remaining": "30", + "X-Sentry-Rate-Limit-Reset": String(nowSeconds + 1), + "X-Sentry-Rate-Limit-ConcurrentRemaining": "0", + "X-Sentry-Rate-Limit-ConcurrentLimit": "25", + }, + }, + ); + const fetchMock = vi + .fn() + .mockResolvedValueOnce(concurrencyLimited) + .mockResolvedValueOnce(authOk()); + globalThis.fetch = fetchMock; + + const promise = apiService().getAuthenticatedUser(); + // The first backoff step is at most 250ms, well before the window reset. + await vi.advanceTimersByTimeAsync(250); + + expect((await promise).id).toBe("1"); + expect(fetchMock).toHaveBeenCalledTimes(2); + }); + + it("gives up after the retry budget with the delay in the message", async () => { + const fetchMock = vi + .fn() + .mockImplementation(() => rateLimited(Math.floor(Date.now() / 1000) + 1)); + globalThis.fetch = fetchMock; + + const promise = apiService().getAuthenticatedUser(); + promise.catch(() => {}); + await vi.runAllTimersAsync(); + + const error = await promise.catch((e: unknown) => e); + expect(error).toBeInstanceOf(ApiRateLimitError); + expect((error as ApiRateLimitError).toUserMessage()).toMatch( + /Retry in 1 second\.$/, + ); + // Initial attempt + 2 retries. + expect(fetchMock).toHaveBeenCalledTimes(3); + }); + + it("does not wait for a reset beyond the per-wait cap", async () => { + const fetchMock = vi.fn().mockResolvedValue(rateLimited(nowSeconds + 30)); + globalThis.fetch = fetchMock; + + const error = await apiService() + .getAuthenticatedUser() + .catch((e: unknown) => e); + + expect(error).toBeInstanceOf(ApiRateLimitError); + expect((error as ApiRateLimitError).retryAfterMs).toBe(29_700); + expect((error as ApiRateLimitError).toUserMessage()).toBe( + "API error (429): You are attempting to use this endpoint too frequently. Limit is 40 requests in 1 seconds. Retry in 30 seconds.", + ); + expect(fetchMock).toHaveBeenCalledTimes(1); + }); + + it("honors a proxy's Retry-After over the Sentry reset", async () => { + const fetchMock = vi + .fn() + .mockResolvedValue(rateLimited(nowSeconds + 1, { "Retry-After": "120" })); + globalThis.fetch = fetchMock; + + const error = await apiService() + .getAuthenticatedUser() + .catch((e: unknown) => e); + + expect((error as ApiRateLimitError).retryAfterMs).toBe(120_000); + expect(fetchMock).toHaveBeenCalledTimes(1); + }); + + it("retries a rate-limited POST, which Sentry rejected before running it", async () => { + const fetchMock = vi + .fn() + .mockImplementation(() => rateLimited(Math.floor(Date.now() / 1000) + 1)); + globalThis.fetch = fetchMock; + + const promise = apiService().createTeam({ + organizationSlug: "my-org", + name: "My Team", + }); + promise.catch(() => {}); + await vi.runAllTimersAsync(); + + await expect(promise).rejects.toBeInstanceOf(ApiRateLimitError); + expect(fetchMock).toHaveBeenCalledTimes(3); + for (const [, init] of fetchMock.mock.calls) { + expect(init.method).toBe("POST"); + } + }); + + it("stops waiting when the request is aborted", async () => { + const fetchMock = vi.fn().mockResolvedValue(rateLimited(nowSeconds + 1)); + globalThis.fetch = fetchMock; + const controller = new AbortController(); + const reason = new Error("tool call cancelled"); + + const promise = rawRequest(apiService())("/auth/", { + signal: controller.signal, + }); + promise.catch(() => {}); + await vi.advanceTimersByTimeAsync(100); + controller.abort(reason); + + await expect(promise).rejects.toBe(reason); + expect(fetchMock).toHaveBeenCalledTimes(1); + }); +}); + describe("request headers", () => { let originalFetch: typeof globalThis.fetch; diff --git a/packages/mcp-core/src/api-client/client.ts b/packages/mcp-core/src/api-client/client.ts index 09f675447..2f34ca969 100644 --- a/packages/mcp-core/src/api-client/client.ts +++ b/packages/mcp-core/src/api-client/client.ts @@ -11,10 +11,15 @@ import { } from "@sentry/api"; import { buildSentryApiUrl } from "@sentry/toolkit-core/api-request"; import { sentryBearerHeader } from "@sentry/toolkit-core/auth-token"; +import { + nextRetryDelayMs, + parseRetryDelayMs, + type RetryPolicy, + sleep, +} from "@sentry/toolkit-core/api-retry"; import { z } from "zod"; import { DEFAULT_SEARCH_ISSUES_PERIOD } from "../constants"; import { ConfigurationError } from "../errors"; -import { retryWithBackoff } from "../internal/fetch-utils"; import { logIssue, logWarn } from "../telem/logging"; import type { SentryProtocol } from "../types"; import { @@ -47,8 +52,9 @@ import { import { USER_AGENT } from "../version"; import { apiPath } from "./api-path"; import { + ApiError, ApiNotFoundError, - ApiServerError, + ApiRateLimitError, ApiValidationError, createApiError, } from "./errors"; @@ -299,16 +305,22 @@ type RequestOptions = { }; /** - * Automatic retry policy for transient upstream gateway failures (502/503/504). + * Caps for automatic retries. Which statuses and methods are retried is the + * shared rule in `@sentry/toolkit-core/api-retry`. * - * Sentry's edge intermittently returns short-lived gateway errors (e.g. - * "upstream connect error or disconnect/reset before headers") that clear on a - * retry. We retry only idempotent GET requests so an automatic retry can never - * replay a mutation, and keep the budget small so a still-failing tool call - * resolves quickly under the Cloudflare Workers request lifetime. + * Sentry's per-endpoint limits are mostly per second ("Limit is 40 requests + * in 1 seconds"), so a 429's reset is usually under a second away and two + * retries fit in about 2 seconds. A reset further out than `maxDelayMs` means + * a longer window; the tool call fails at once with the delay in its message + * instead of holding the Workers request open. */ -const RETRYABLE_REQUEST_MAX_RETRIES = 2; -const RETRYABLE_REQUEST_INITIAL_DELAY_MS = 250; +const REQUEST_RETRY_POLICY: RetryPolicy = { + maxRetries: 2, + baseDelayMs: 250, + maxDelayMs: 2000, + maxTotalDelayMs: 3000, +}; + const API_MAX_PER_PAGE = 100; const MAX_LIST_PAGES = 50; @@ -759,13 +771,14 @@ export class SentryApiService { /** * Makes an authenticated request to the Sentry API, transparently retrying - * transient upstream gateway failures (502/503/504). + * rate-limited (429) requests and 500/502/503/504 GET and HEAD requests + * under `REQUEST_RETRY_POLICY`. * - * Retries are limited to idempotent GET requests so an automatic retry can - * never replay a mutation, and are gated on `ApiServerError.isGatewayError()` - * so persistent 500s, 4xx client errors, and network/configuration errors - * fail fast without waiting. Callers that opt a 5xx into `allowStatuses` - * receive the raw response from `requestOnce` and are never retried. + * 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 + * a status into `allowStatuses` receive the raw response from `requestOnce` + * and are never retried. Aborting `options.signal` ends any wait. * * @param path API endpoint path (without /api/0 prefix) * @param options Fetch options @@ -779,18 +792,32 @@ export class SentryApiService { options: RequestInit = {}, requestOptions: { host?: string; allowStatuses?: number[] } = {}, ): Promise { - const method = (options.method ?? "GET").toUpperCase(); - const isIdempotent = method === "GET"; + const method = options.method ?? "GET"; + let waitedMs = 0; - return await retryWithBackoff( - () => this.requestOnce(path, options, requestOptions), - { - maxRetries: isIdempotent ? RETRYABLE_REQUEST_MAX_RETRIES : 0, - initialDelay: RETRYABLE_REQUEST_INITIAL_DELAY_MS, - shouldRetry: (error) => - error instanceof ApiServerError && error.isGatewayError(), - }, - ); + for (let attempt = 0; ; attempt++) { + try { + return await this.requestOnce(path, options, requestOptions); + } catch (error) { + if (!(error instanceof ApiError) || error.status === undefined) { + throw error; + } + const delayMs = nextRetryDelayMs({ + policy: REQUEST_RETRY_POLICY, + method, + status: error.status, + serverDelayMs: + error instanceof ApiRateLimitError ? error.retryAfterMs : undefined, + attempt, + waitedMs, + }); + if (delayMs === undefined) { + throw error; + } + await sleep(delayMs, options.signal); + waitedMs += delayMs; + } + } } /** @@ -899,6 +926,7 @@ export class SentryApiService { if (allowStatuses?.includes(response.status)) { return response; } + const retryAfterMs = parseRetryDelayMs(response.status, response.headers); const errorText = await response.text(); let parsed: unknown; try { @@ -923,6 +951,7 @@ export class SentryApiService { response.status, errorText, undefined, + retryAfterMs, ); } logWarn("Failed to parse JSON error response", { @@ -952,6 +981,7 @@ export class SentryApiService { response.status, data.detail, parsed, + retryAfterMs, ); } @@ -978,6 +1008,7 @@ export class SentryApiService { response.status, errorText, undefined, + retryAfterMs, ); } diff --git a/packages/mcp-core/src/api-client/errors.ts b/packages/mcp-core/src/api-client/errors.ts index d80fee97e..a482aab0e 100644 --- a/packages/mcp-core/src/api-client/errors.ts +++ b/packages/mcp-core/src/api-client/errors.ts @@ -195,19 +195,28 @@ export class ApiAuthenticationError extends ApiClientError { /** * Rate limit error (429). - * Includes retry-after information when available. + * `retryAfterMs` comes from the response's `Retry-After` or + * `X-Sentry-Rate-Limit-Reset` header, when it has one. */ export class ApiRateLimitError extends ApiClientError { constructor( message: string, detail?: string, responseBody?: unknown, - public readonly retryAfter?: number, + public readonly retryAfterMs?: number, ) { super(message, 429, detail, responseBody); this.name = "ApiRateLimitError"; Object.setPrototypeOf(this, ApiRateLimitError.prototype); } + + override toUserMessage(): string { + if (this.retryAfterMs === undefined) { + return `${super.toUserMessage()}. Retry in a few seconds.`; + } + const seconds = Math.max(1, Math.ceil(this.retryAfterMs / 1000)); + return `${super.toUserMessage()}. Retry in ${seconds} second${seconds === 1 ? "" : "s"}.`; + } } /** @@ -251,6 +260,7 @@ export function createApiError( status: number, detail?: string, responseBody?: unknown, + retryAfterMs?: number, ): ApiError { // Apply message improvements for known error patterns let improvedMessage = message; @@ -299,18 +309,8 @@ export function createApiError( ); } - case 429: { - // Try to extract retry-after header if available - let retryAfter: number | undefined; - if (responseBody && typeof responseBody === "object") { - if ("retry_after" in responseBody) { - retryAfter = Number(responseBody.retry_after); - } else if ("retryAfter" in responseBody) { - retryAfter = Number(responseBody.retryAfter); - } - } - return new ApiRateLimitError(message, detail, responseBody, retryAfter); - } + case 429: + return new ApiRateLimitError(message, detail, responseBody, retryAfterMs); default: // Generic categorization for other status codes diff --git a/packages/mcp-core/src/internal/error-handling.test.ts b/packages/mcp-core/src/internal/error-handling.test.ts index 6648af8eb..5a7402ccb 100644 --- a/packages/mcp-core/src/internal/error-handling.test.ts +++ b/packages/mcp-core/src/internal/error-handling.test.ts @@ -7,6 +7,7 @@ import { LLMProviderError, } from "../errors"; import { APICallError, RetryError } from "ai"; +import { ApiRateLimitError } from "../api-client"; vi.mock("../telem/logging", () => ({ logIssue: vi.fn(() => "mock-event-id"), @@ -221,6 +222,29 @@ describe("formatErrorForUser", () => { }); }); + describe("ApiRateLimitError", () => { + it("tells the agent when to retry and to stop calling tools in parallel", async () => { + const error = new ApiRateLimitError( + "Limit is 40 requests in 1 seconds", + undefined, + undefined, + 30_000, + ); + + const result = await formatErrorForUser(error, { transport: "http" }); + + expect(result).toBe( + [ + "**Rate Limited**", + "The Sentry API is rate limiting these requests. Short waits are already retried automatically.", + "API error (429): Limit is 40 requests in 1 seconds. Retry in 30 seconds.", + "Call Sentry tools one at a time instead of in parallel, and don't repeat this call before then.", + ].join("\n\n"), + ); + expect(logIssue).not.toHaveBeenCalled(); + }); + }); + describe("UserInputError", () => { const error = new UserInputError("Invalid issue ID format"); diff --git a/packages/mcp-core/src/internal/error-handling.ts b/packages/mcp-core/src/internal/error-handling.ts index d36ec9c65..420eb8cad 100644 --- a/packages/mcp-core/src/internal/error-handling.ts +++ b/packages/mcp-core/src/internal/error-handling.ts @@ -7,6 +7,7 @@ import { import { ApiError, ApiClientError, + ApiRateLimitError, ApiServerError, isApiAuthenticationErrorDeep, } from "../api-client"; @@ -277,6 +278,17 @@ export async function formatErrorForUser( ].join("\n\n"); } + // A 429 isn't an input problem either. Tell the agent when to come back and + // that parallel tool calls are what usually trips the per-second limit. + if (error instanceof ApiRateLimitError) { + return [ + "**Rate Limited**", + "The Sentry API is rate limiting these requests. Short waits are already retried automatically.", + error.toUserMessage(), + "Call Sentry tools one at a time instead of in parallel, and don't repeat this call before then.", + ].join("\n\n"); + } + // Handle ApiClientError (4xx) - user input errors, should NOT be logged to Sentry if (isApiClientError(error)) { const statusText = error.status diff --git a/packages/toolkit-core/README.md b/packages/toolkit-core/README.md index 1919c322e..9c6694bb3 100644 --- a/packages/toolkit-core/README.md +++ b/packages/toolkit-core/README.md @@ -5,7 +5,8 @@ builds bundle this private workspace package into their artifacts. The shared code validates and formats upstream bearer tokens, assembles Sentry API URLs, constructs OAuth device-flow form bodies, classifies RFC 8628 polling -responses, advances retry intervals, recognizes Sentry hostnames and +responses, advances retry intervals, decides when a Sentry API request may +be retried and how long to wait (`api-retry`), recognizes Sentry hostnames and credential-safe SaaS HTTPS origins, encodes API path identifiers, parses Events field expressions the way the Sentry frontend does, parses search queries with the Sentry frontend grammar, resolves Events endpoint sorts so the sort column diff --git a/packages/toolkit-core/package.json b/packages/toolkit-core/package.json index 72ae157d4..6b580fdad 100644 --- a/packages/toolkit-core/package.json +++ b/packages/toolkit-core/package.json @@ -11,6 +11,10 @@ "types": "./src/api-request.ts", "default": "./src/api-request.ts" }, + "./api-retry": { + "types": "./src/api-retry.ts", + "default": "./src/api-retry.ts" + }, "./api-path-segment": { "types": "./src/api-path-segment.ts", "default": "./src/api-path-segment.ts" diff --git a/packages/toolkit-core/src/api-retry.test.ts b/packages/toolkit-core/src/api-retry.test.ts new file mode 100644 index 000000000..6f6998366 --- /dev/null +++ b/packages/toolkit-core/src/api-retry.test.ts @@ -0,0 +1,277 @@ +import { afterEach, describe, expect, it, vi } from "vitest"; +import { + nextRetryDelayMs, + parseRetryDelayMs, + type RetryPolicy, + sleep, +} from "./api-retry.js"; + +const NOW = Date.UTC(2026, 0, 1, 12, 0, 0, 300); +const NOW_SECONDS = Math.floor(NOW / 1000); + +const policy: RetryPolicy = { + maxRetries: 2, + baseDelayMs: 250, + maxDelayMs: 2000, + maxTotalDelayMs: 3000, +}; + +/** Headers from a fixed-window 429, as Sentry's rate-limit middleware sends them. */ +function windowLimited( + resetSeconds: number, + extra: Record = {}, +) { + return new Headers({ + "X-Sentry-Rate-Limit-Remaining": "0", + "X-Sentry-Rate-Limit-Limit": "40", + "X-Sentry-Rate-Limit-Reset": String(resetSeconds), + ...extra, + }); +} + +describe("parseRetryDelayMs", () => { + it("waits until the Sentry window reset", () => { + expect(parseRetryDelayMs(429, windowLimited(NOW_SECONDS + 1), NOW)).toBe( + 700, + ); + }); + + it("measures the reset against the response Date when the local clock is skewed", () => { + const serverNow = NOW + 10 * 60 * 1000; + const headers = windowLimited(Math.floor(serverNow / 1000) + 1, { + Date: new Date(serverNow).toUTCString(), + }); + + expect(parseRetryDelayMs(429, headers, NOW)).toBe(1000); + }); + + it("bounds the reset by the response Date when the local clock lags slightly", () => { + // Local clock 1.5s behind: alone it would put a 1s window 2.2s away. + const headers = windowLimited(NOW_SECONDS + 1, { + Date: new Date(NOW).toUTCString(), + }); + const lagging = NOW - 1500; + + expect(parseRetryDelayMs(429, headers, lagging)).toBe(1000); + expect( + nextRetryDelayMs({ + policy, + method: "GET", + status: 429, + serverDelayMs: parseRetryDelayMs(429, headers, lagging), + attempt: 0, + waitedMs: 0, + random: () => 0, + }), + ).toBe(1000); + }); + + it("bounds the reset by the response Date when the local clock runs ahead", () => { + const headers = windowLimited(NOW_SECONDS + 1, { + Date: new Date(NOW).toUTCString(), + }); + + // 2.5s ahead would put the reset 1.8s in the past; the Date puts it 0-1s + // away, so the estimate is clamped to the lower bound. + expect(parseRetryDelayMs(429, headers, NOW + 2500)).toBe(0); + }); + + it("keeps the local estimate when it agrees with the response Date", () => { + const headers = windowLimited(NOW_SECONDS + 1, { + Date: new Date(NOW).toUTCString(), + }); + + expect(parseRetryDelayMs(429, headers, NOW)).toBe(700); + }); + + it("treats a reset already in the past as no wait", () => { + expect(parseRetryDelayMs(429, windowLimited(NOW_SECONDS - 5), NOW)).toBe(0); + }); + + it("gives no delay for a concurrency-limit 429", () => { + const headers = new Headers({ + "X-Sentry-Rate-Limit-Remaining": "12", + "X-Sentry-Rate-Limit-Reset": String(NOW_SECONDS + 1), + "X-Sentry-Rate-Limit-ConcurrentRemaining": "0", + "X-Sentry-Rate-Limit-ConcurrentLimit": "25", + }); + + expect(parseRetryDelayMs(429, headers, NOW)).toBeUndefined(); + }); + + it("ignores the window reset on other statuses", () => { + expect( + parseRetryDelayMs(503, windowLimited(NOW_SECONDS + 1), NOW), + ).toBeUndefined(); + }); + + it("prefers Retry-After in seconds", () => { + const headers = windowLimited(NOW_SECONDS + 1, { "Retry-After": "3" }); + + expect(parseRetryDelayMs(429, headers, NOW)).toBe(3000); + }); + + it("accepts Retry-After as an HTTP-date on any status", () => { + const headers = new Headers({ + "Retry-After": new Date(NOW + 5300).toUTCString(), + }); + + // HTTP-dates drop milliseconds, so this is 12:00:05.000. + expect(parseRetryDelayMs(503, headers, NOW)).toBe(4700); + }); + + it("returns undefined for missing or malformed headers", () => { + expect(parseRetryDelayMs(429, new Headers(), NOW)).toBeUndefined(); + expect( + parseRetryDelayMs(429, new Headers({ "Retry-After": "soon" }), NOW), + ).toBeUndefined(); + expect( + parseRetryDelayMs( + 429, + new Headers({ + "X-Sentry-Rate-Limit-Remaining": "0", + "X-Sentry-Rate-Limit-Reset": "never", + }), + NOW, + ), + ).toBeUndefined(); + }); +}); + +describe("nextRetryDelayMs", () => { + const base = { + policy, + method: "GET", + status: 429, + serverDelayMs: undefined, + attempt: 0, + waitedMs: 0, + random: () => 0, + }; + + it("waits for the server delay plus jitter of at most half the base step", () => { + expect(nextRetryDelayMs({ ...base, serverDelayMs: 700 })).toBe(700); + expect( + nextRetryDelayMs({ ...base, serverDelayMs: 700, random: () => 1 }), + ).toBe(825); + }); + + it("falls back to equal-jitter exponential backoff", () => { + expect(nextRetryDelayMs({ ...base, attempt: 0 })).toBe(125); + expect(nextRetryDelayMs({ ...base, attempt: 0, random: () => 1 })).toBe( + 250, + ); + expect(nextRetryDelayMs({ ...base, attempt: 1, random: () => 1 })).toBe( + 500, + ); + }); + + it("caps the backoff step at maxDelayMs", () => { + expect( + nextRetryDelayMs({ + ...base, + policy: { ...policy, maxRetries: 10, maxTotalDelayMs: 60_000 }, + attempt: 6, + random: () => 1, + }), + ).toBe(2000); + }); + + it("stops when the server delay exceeds the per-wait cap", () => { + expect( + nextRetryDelayMs({ ...base, serverDelayMs: 30_000 }), + ).toBeUndefined(); + }); + + it("stops when the wait would exceed the total budget", () => { + expect( + nextRetryDelayMs({ + ...base, + attempt: 1, + serverDelayMs: 1000, + waitedMs: 2500, + }), + ).toBeUndefined(); + }); + + it("stops after maxRetries", () => { + expect(nextRetryDelayMs({ ...base, attempt: 2 })).toBeUndefined(); + }); + + it("retries reads in any case", () => { + for (const method of ["GET", "head", "OPTIONS"]) { + expect(nextRetryDelayMs({ ...base, method })).toBe(125); + } + }); + + it("retries a rate-limited write, since Sentry rejects it before the endpoint runs", () => { + for (const method of ["POST", "PUT", "DELETE"]) { + expect(nextRetryDelayMs({ ...base, method, status: 429 })).toBe(125); + } + }); + + it("never retries a write on a 5xx without the idempotent opt-in", () => { + for (const method of ["POST", "PUT", "DELETE"]) { + for (const status of [500, 502, 503, 504]) { + expect(nextRetryDelayMs({ ...base, method, status })).toBeUndefined(); + } + } + expect( + nextRetryDelayMs({ + ...base, + method: "POST", + status: 503, + idempotent: true, + }), + ).toBe(125); + }); + + it("retries a GET on a transient 5xx", () => { + for (const status of [500, 502, 503, 504]) { + expect(nextRetryDelayMs({ ...base, status })).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(); + } + }); +}); + +describe("sleep", () => { + afterEach(() => { + vi.useRealTimers(); + }); + + it("resolves after the delay", async () => { + vi.useFakeTimers(); + const resolved = vi.fn(); + const promise = sleep(1000).then(resolved); + + await vi.advanceTimersByTimeAsync(999); + expect(resolved).not.toHaveBeenCalled(); + await vi.advanceTimersByTimeAsync(1); + await promise; + expect(resolved).toHaveBeenCalled(); + }); + + it("rejects with the abort reason as soon as the signal aborts", async () => { + vi.useFakeTimers(); + const controller = new AbortController(); + const promise = sleep(60_000, controller.signal); + const reason = new Error("cancelled"); + + controller.abort(reason); + + await expect(promise).rejects.toBe(reason); + expect(vi.getTimerCount()).toBe(0); + }); + + it("rejects at once when the signal is already aborted", async () => { + const controller = new AbortController(); + controller.abort(new Error("gone")); + + await expect(sleep(1000, controller.signal)).rejects.toThrow("gone"); + }); +}); diff --git a/packages/toolkit-core/src/api-retry.ts b/packages/toolkit-core/src/api-retry.ts new file mode 100644 index 000000000..d4e667c19 --- /dev/null +++ b/packages/toolkit-core/src/api-retry.ts @@ -0,0 +1,199 @@ +/** + * Retry policy for Sentry API requests. Pure logic over standard Web APIs, so + * it runs in Node and Cloudflare Workers. Each product owns its request loop, + * error classes, and caps; this module decides whether to retry and how long + * to wait. A 401 credential refresh is not a retry here: the transport may + * replay any method once after refreshing, because the endpoint never ran. + */ + +/** Caps for one logical request. All delays are in milliseconds. */ +export type RetryPolicy = { + /** Retries after the first attempt. */ + maxRetries: number; + /** First fallback backoff step; it doubles on each retry. */ + baseDelayMs: number; + /** Longest single wait. A server delay above this is not waited for. */ + maxDelayMs: number; + /** Longest total wait across every retry of the request. */ + maxTotalDelayMs: number; +}; + +type HeaderReader = Pick; + +const IDEMPOTENT_METHODS: ReadonlySet = new Set([ + "GET", + "HEAD", + "OPTIONS", +]); + +/** + * The statuses that are retried, and which requests each may replay. This is + * the one rule for the CLI and MCP; `docs/contributing/api-patterns.md` + * describes it and links here. + * + * - `any`: Sentry's `RatelimitMiddleware` rejects a 429 in `process_request`, + * before the view runs (`sentry/middleware/ratelimit.py`), so replaying any + * method is safe. + * - `idempotent`: the endpoint may have run, so only GET, HEAD, and OPTIONS + * retry, unless the caller marks the request `idempotent`. + * + * Other 5xx codes (501, 505, ...) are deterministic and are not retried. + */ +const RETRYABLE_STATUSES: ReadonlyMap = new Map([ + [429, "any"], + [500, "idempotent"], + [502, "idempotent"], + [503, "idempotent"], + [504, "idempotent"], +]); + +/** 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. */ +function isRetryable( + method: string, + status: number, + idempotent = false, +): boolean { + const replays = RETRYABLE_STATUSES.get(status); + if (replays === undefined) { + return false; + } + return ( + replays === "any" || + idempotent || + IDEMPOTENT_METHODS.has(method.toUpperCase()) + ); +} + +/** + * Milliseconds the response asks the client to wait, or undefined when it + * gives no usable delay. `Retry-After` (seconds or HTTP-date) wins; a proxy + * may add it, but Sentry itself does not. Otherwise a 429 waits for + * `X-Sentry-Rate-Limit-Reset`, the Unix second the next window starts. A + * concurrency-limit 429 is not tied to the window, so it gets no delay here + * and the caller falls back to backoff. + */ +export function parseRetryDelayMs( + status: number, + headers: HeaderReader, + now: number = Date.now(), +): number | undefined { + const retryAfter = headers.get("retry-after")?.trim(); + if (retryAfter) { + if (/^\d+$/.test(retryAfter)) { + return Number(retryAfter) * 1000; + } + const date = Date.parse(retryAfter); + return Number.isNaN(date) ? undefined : delayUntil(date, headers, now); + } + + if (status !== 429 || !isWindowLimited(headers)) { + return undefined; + } + const reset = Number(headers.get("x-sentry-rate-limit-reset")); + if (!Number.isFinite(reset) || reset <= 0) { + return undefined; + } + return delayUntil(reset * 1000, headers, now); +} + +/** + * Delay before the next retry, or undefined to stop and surface the error. + * Stops when the method or status is not retryable, the retries are used up, + * the server delay exceeds `maxDelayMs`, or the wait would exceed + * `maxTotalDelayMs`. + */ +export function nextRetryDelayMs({ + policy, + method, + idempotent, + status, + serverDelayMs, + attempt, + waitedMs, + random = Math.random, +}: { + policy: RetryPolicy; + method: string; + idempotent?: boolean; + status: number; + /** From `parseRetryDelayMs` for the failed response. */ + serverDelayMs: number | undefined; + /** Retries already made for this request, starting at 0. */ + attempt: number; + /** Total time already spent waiting for this request. */ + waitedMs: number; + random?: () => number; +}): number | undefined { + if ( + attempt >= policy.maxRetries || + !isRetryable(method, status, idempotent) + ) { + return undefined; + } + + let delayMs: number; + if (serverDelayMs !== undefined) { + if (serverDelayMs > policy.maxDelayMs) { + return undefined; + } + // Spread clients that share a window so they don't all wake at its start. + delayMs = serverDelayMs + random() * (policy.baseDelayMs / 2); + } else { + // Equal jitter: half the step is fixed, half is random. + const step = Math.min(policy.baseDelayMs * 2 ** attempt, policy.maxDelayMs); + delayMs = step / 2 + random() * (step / 2); + } + + return waitedMs + delayMs > policy.maxTotalDelayMs ? undefined : delayMs; +} + +/** Wait `ms`, or reject with the signal's reason as soon as it aborts. */ +export function sleep(ms: number, signal?: AbortSignal | null): Promise { + return new Promise((resolve, reject) => { + if (signal?.aborted) { + reject(signal.reason); + return; + } + const onAbort = () => { + clearTimeout(timer); + reject(signal?.reason); + }; + const timer = setTimeout(() => { + signal?.removeEventListener("abort", onAbort); + resolve(); + }, ms); + signal?.addEventListener("abort", onAbort, { once: true }); + }); +} + +/** + * Milliseconds until a server-clock `target`. The local clock gives the finest + * estimate, but it may be skewed. The response `Date` bounds the server's + * clock to `[date, date + 1s)`, so the true delay lies in + * `(target - date - 1s, target - date]`; the local estimate is clamped into it. + */ +function delayUntil( + target: number, + headers: HeaderReader, + now: number, +): number { + let delay = target - now; + const date = Date.parse(headers.get("date") ?? ""); + if (!Number.isNaN(date)) { + const latest = target - date; + delay = Math.min(Math.max(delay, latest - DATE_RESOLUTION_MS), latest); + } + return Math.max(0, delay); +} + +/** + * Sentry sends `Remaining: 0` only when the fixed window is exhausted + * (`above_rate_limit_check` in `sentry/ratelimits/utils.py`). Otherwise the + * 429 came from the concurrency limit and the window reset is irrelevant. + */ +function isWindowLimited(headers: HeaderReader): boolean { + return headers.get("x-sentry-rate-limit-remaining") === "0"; +}