From a4f9a98e9165799119e19091f5758d8934ca1bdf Mon Sep 17 00:00:00 2001 From: "sentry-junior[bot]" <264270552+sentry-junior[bot]@users.noreply.github.com> Date: Mon, 26 Oct 2026 23:45:11 +0000 Subject: [PATCH 1/5] fix(mcp): Retry rate-limited GETs using Sentry rate-limit reset headers The hosted MCP saw about 32,600 Sentry API 429s in 7 days, nearly all on GETs ("Limit is 40 requests in 1 seconds"). request() only retried 502/503/504, so every one failed the tool call, and ApiRateLimitError's retryAfter read body fields Sentry never sends. Add a pure retry policy in toolkit-core/api-retry (OpenSpec task 5.0 in #1669). It retries only GET/HEAD/OPTIONS unless a request opts in as idempotent, waits for Retry-After or X-Sentry-Rate-Limit-Reset with a Date-header clock-skew guard and per-wait/total caps, falls back to jittered backoff for concurrency 429s and gateway errors, and sleeps abortably. SentryApiService.request() now retries 429 and 502/503/504 on reads, at most 2 times, 2s per wait, and 3s in total. A reset further out fails at once with an ApiRateLimitError whose retryAfterMs and message say when to retry. Writes are still never retried. Co-Authored-By: Burak Yigit Kaya <126780+BYK@users.noreply.github.com> --- docs/contributing/api-patterns.md | 26 +- .../mcp-core/src/api-client/client.test.ts | 182 ++++++++++++++ packages/mcp-core/src/api-client/client.ts | 86 +++++-- packages/mcp-core/src/api-client/errors.ts | 28 +-- packages/toolkit-core/README.md | 3 +- packages/toolkit-core/package.json | 4 + packages/toolkit-core/src/api-retry.test.ts | 232 ++++++++++++++++++ packages/toolkit-core/src/api-retry.ts | 164 +++++++++++++ 8 files changed, 674 insertions(+), 51 deletions(-) create mode 100644 packages/toolkit-core/src/api-retry.test.ts create mode 100644 packages/toolkit-core/src/api-retry.ts diff --git a/docs/contributing/api-patterns.md b/docs/contributing/api-patterns.md index 3510c77fd..37a9138f6 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,26 @@ 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 rate limits (`429`) and +transient upstream gateway failures (`502`/`503`/`504`), using the shared +policy in `@sentry/toolkit-core/api-retry`. Retries are: + +- **GET and HEAD only** — writes (`POST`/`PUT`/`DELETE`) are never retried, so + an automatic retry can never replay a mutation. +- **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 gateway + errors use jittered exponential backoff. +- **Capped** — at most 2 retries, 2 seconds per wait, and 3 seconds in total. + A longer reset fails at once with an `ApiRateLimitError` whose + `retryAfterMs` and user message say when to retry. +- **Abortable** — aborting the request's `signal` ends the wait. +- **Status-limited** — persistent `500`s, other `4xx` errors, and + network/configuration errors fail fast. - **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..3f42e15ec 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"; @@ -1503,6 +1504,187 @@ 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("does not retry a rate-limited POST", async () => { + const fetchMock = vi.fn().mockResolvedValue(rateLimited(nowSeconds + 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(1); + }); + + 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..b41a70d6e 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,23 @@ type RequestOptions = { }; /** - * Automatic retry policy for transient upstream gateway failures (502/503/504). + * Automatic retries for rate limits (429) and transient gateway failures + * (502/503/504) on GET and HEAD requests. Writes are never retried. * - * 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. Edge gateway errors ("upstream + * connect error ... reset before headers") clear on a quick retry. */ -const RETRYABLE_REQUEST_MAX_RETRIES = 2; -const RETRYABLE_REQUEST_INITIAL_DELAY_MS = 250; +const REQUEST_RETRY_POLICY: RetryPolicy = { + maxRetries: 2, + retryableStatuses: [429, 502, 503, 504], + baseDelayMs: 250, + maxDelayMs: 2000, + maxTotalDelayMs: 3000, +}; const API_MAX_PER_PAGE = 100; const MAX_LIST_PAGES = 50; @@ -759,13 +772,14 @@ export class SentryApiService { /** * Makes an authenticated request to the Sentry API, transparently retrying - * transient upstream gateway failures (502/503/504). + * rate-limited (429) and gateway-failed (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, persistent 500s, 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 +793,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 +927,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 +952,7 @@ export class SentryApiService { response.status, errorText, undefined, + retryAfterMs, ); } logWarn("Failed to parse JSON error response", { @@ -952,6 +982,7 @@ export class SentryApiService { response.status, data.detail, parsed, + retryAfterMs, ); } @@ -978,6 +1009,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..eeb94c17d 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()}. Wait a few seconds before retrying, and make fewer requests in parallel.`; + } + 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/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..206d4e43c --- /dev/null +++ b/packages/toolkit-core/src/api-retry.test.ts @@ -0,0 +1,232 @@ +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, + retryableStatuses: [429, 502, 503, 504], + 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("ignores a Date header within its one-second resolution", () => { + 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("never retries a write without the idempotent opt-in", () => { + for (const method of ["PUT", "DELETE"]) { + expect(nextRetryDelayMs({ ...base, method })).toBeUndefined(); + } + for (const status of [429, 502, 503, 504]) { + expect( + nextRetryDelayMs({ ...base, method: "POST", status }), + ).toBeUndefined(); + } + expect( + nextRetryDelayMs({ ...base, method: "POST", idempotent: true }), + ).toBe(125); + }); + + it("does not retry statuses outside the policy", () => { + expect(nextRetryDelayMs({ ...base, status: 500 })).toBeUndefined(); + expect(nextRetryDelayMs({ ...base, status: 404 })).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..f309787d9 --- /dev/null +++ b/packages/toolkit-core/src/api-retry.ts @@ -0,0 +1,164 @@ +/** + * 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; + retryableStatuses: readonly 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 = new Set(["GET", "HEAD", "OPTIONS"]); + +/** + * The response `Date` header has one-second resolution, so smaller + * disagreements with the local clock are noise, not skew. + */ +const CLOCK_SKEW_TOLERANCE_MS = 2000; + +/** + * Whether a failed request may be sent again. Writes are only retried when + * the caller says replaying them is safe, such as a content-addressed upload. + */ +function isRetryableMethod(method: string, idempotent = false): boolean { + return 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 reference = serverNow(headers, now); + + 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 : Math.max(0, date - reference); + } + + 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 Math.max(0, reset * 1000 - reference); +} + +/** + * 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 || + !isRetryableMethod(method, idempotent) || + !policy.retryableStatuses.includes(status) + ) { + 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 }); + }); +} + +/** + * The window reset is server time. Trust the local clock unless it disagrees + * with the response `Date` by more than the header's resolution. + */ +function serverNow(headers: HeaderReader, now: number): number { + const date = Date.parse(headers.get("date") ?? ""); + if (Number.isNaN(date) || Math.abs(now - date) <= CLOCK_SKEW_TOLERANCE_MS) { + return now; + } + return date; +} + +/** + * 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"; +} From 513c6ca1a75a873a72f6b17e9ca3f674ac8b74fc Mon Sep 17 00:00:00 2001 From: "sentry-junior[bot]" <264270552+sentry-junior[bot]@users.noreply.github.com> Date: Mon, 26 Oct 2026 23:56:18 +0000 Subject: [PATCH 2/5] fix(toolkit-core): Bound the rate-limit reset by the response Date A local clock up to 2s behind the response Date was treated as in sync, so a 1s Sentry window could look 2s+ away and exceed the MCP's 2s per-wait cap, dropping a retry that would have succeeded. Clamp the local estimate into the range the Date header allows instead of switching clocks at a threshold. --- packages/toolkit-core/src/api-retry.test.ts | 33 ++++++++++++++++++++- packages/toolkit-core/src/api-retry.ts | 33 +++++++++++---------- 2 files changed, 50 insertions(+), 16 deletions(-) diff --git a/packages/toolkit-core/src/api-retry.test.ts b/packages/toolkit-core/src/api-retry.test.ts index 206d4e43c..09a617143 100644 --- a/packages/toolkit-core/src/api-retry.test.ts +++ b/packages/toolkit-core/src/api-retry.test.ts @@ -46,7 +46,38 @@ describe("parseRetryDelayMs", () => { expect(parseRetryDelayMs(429, headers, NOW)).toBe(1000); }); - it("ignores a Date header within its one-second resolution", () => { + 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(), }); diff --git a/packages/toolkit-core/src/api-retry.ts b/packages/toolkit-core/src/api-retry.ts index f309787d9..806a9faa5 100644 --- a/packages/toolkit-core/src/api-retry.ts +++ b/packages/toolkit-core/src/api-retry.ts @@ -23,11 +23,8 @@ type HeaderReader = Pick; const IDEMPOTENT_METHODS = new Set(["GET", "HEAD", "OPTIONS"]); -/** - * The response `Date` header has one-second resolution, so smaller - * disagreements with the local clock are noise, not skew. - */ -const CLOCK_SKEW_TOLERANCE_MS = 2000; +/** The response `Date` header truncates server time to whole seconds. */ +const DATE_RESOLUTION_MS = 1000; /** * Whether a failed request may be sent again. Writes are only retried when @@ -50,15 +47,13 @@ export function parseRetryDelayMs( headers: HeaderReader, now: number = Date.now(), ): number | undefined { - const reference = serverNow(headers, now); - 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 : Math.max(0, date - reference); + return Number.isNaN(date) ? undefined : delayUntil(date, headers, now); } if (status !== 429 || !isWindowLimited(headers)) { @@ -68,7 +63,7 @@ export function parseRetryDelayMs( if (!Number.isFinite(reset) || reset <= 0) { return undefined; } - return Math.max(0, reset * 1000 - reference); + return delayUntil(reset * 1000, headers, now); } /** @@ -143,15 +138,23 @@ export function sleep(ms: number, signal?: AbortSignal | null): Promise { } /** - * The window reset is server time. Trust the local clock unless it disagrees - * with the response `Date` by more than the header's resolution. + * 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 serverNow(headers: HeaderReader, now: number): number { +function delayUntil( + target: number, + headers: HeaderReader, + now: number, +): number { + let delay = target - now; const date = Date.parse(headers.get("date") ?? ""); - if (Number.isNaN(date) || Math.abs(now - date) <= CLOCK_SKEW_TOLERANCE_MS) { - return now; + if (!Number.isNaN(date)) { + const latest = target - date; + delay = Math.min(Math.max(delay, latest - DATE_RESOLUTION_MS), latest); } - return date; + return Math.max(0, delay); } /** From 613babb77a2af222f29178f7cc2f65ff2a6825c3 Mon Sep 17 00:00:00 2001 From: "sentry-junior[bot]" <264270552+sentry-junior[bot]@users.noreply.github.com> Date: Tue, 27 Oct 2026 09:26:42 +0000 Subject: [PATCH 3/5] fix(mcp): Retry rate-limited writes and 500s, and tell agents how to back off Sentry's rate-limit middleware rejects a 429 in process_request, before the endpoint runs, so replaying it is safe for any method. Retry 429s for writes too; 5xx responses still retry only for GET/HEAD since the endpoint may have run. Add 500 to the MCP's retryable statuses; 501, 505, and other 5xx codes stay out because they are deterministic. Format rate-limit tool errors as "Rate Limited" instead of "Input Error", with the retry time and advice to stop calling tools in parallel. --- docs/contributing/api-patterns.md | 16 ++++---- .../mcp-core/src/api-client/client.test.ts | 39 ++++++++++++++++--- packages/mcp-core/src/api-client/client.ts | 20 ++++++---- packages/mcp-core/src/api-client/errors.ts | 2 +- .../src/internal/error-handling.test.ts | 24 ++++++++++++ .../mcp-core/src/internal/error-handling.ts | 12 ++++++ packages/toolkit-core/src/api-retry.test.ts | 24 ++++++++---- packages/toolkit-core/src/api-retry.ts | 19 ++++++--- 8 files changed, 122 insertions(+), 34 deletions(-) diff --git a/docs/contributing/api-patterns.md b/docs/contributing/api-patterns.md index 37a9138f6..5d7b0f81f 100644 --- a/docs/contributing/api-patterns.md +++ b/docs/contributing/api-patterns.md @@ -253,20 +253,22 @@ See error patterns in [common-patterns.md](common-patterns.md#error-handling). ### Automatic Retries `SentryApiService.request()` transparently retries rate limits (`429`) and -transient upstream gateway failures (`502`/`503`/`504`), using the shared -policy in `@sentry/toolkit-core/api-retry`. Retries are: +transient server failures (`500`/`502`/`503`/`504`), using the shared policy in +`@sentry/toolkit-core/api-retry`. Retries are: -- **GET and HEAD only** — writes (`POST`/`PUT`/`DELETE`) are never retried, so - an automatic retry can never replay a mutation. +- **Safe for writes only on 429** — Sentry's rate-limit middleware rejects a + request before the endpoint runs, so a `429` is retried for any method. A + `5xx` may have run the endpoint, so only GET and HEAD retry on one. - **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 gateway + Sentry's `X-Sentry-Rate-Limit-Reset`. Concurrency-limit `429`s and `5xx` errors use jittered exponential backoff. - **Capped** — at most 2 retries, 2 seconds per wait, and 3 seconds in total. A longer reset fails at once with an `ApiRateLimitError` whose `retryAfterMs` and user message say when to retry. - **Abortable** — aborting the request's `signal` ends the wait. -- **Status-limited** — persistent `500`s, other `4xx` errors, and - network/configuration errors fail fast. +- **Status-limited** — deterministic `5xx` codes such as `501`, other `4xx` + errors, and network/configuration errors fail fast. 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. diff --git a/packages/mcp-core/src/api-client/client.test.ts b/packages/mcp-core/src/api-client/client.test.ts index 3f42e15ec..a5c1264d2 100644 --- a/packages/mcp-core/src/api-client/client.test.ts +++ b/packages/mcp-core/src/api-client/client.test.ts @@ -1443,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({ @@ -1652,8 +1676,10 @@ describe("rate limit retry", () => { expect(fetchMock).toHaveBeenCalledTimes(1); }); - it("does not retry a rate-limited POST", async () => { - const fetchMock = vi.fn().mockResolvedValue(rateLimited(nowSeconds + 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({ @@ -1664,7 +1690,10 @@ describe("rate limit retry", () => { await vi.runAllTimersAsync(); await expect(promise).rejects.toBeInstanceOf(ApiRateLimitError); - expect(fetchMock).toHaveBeenCalledTimes(1); + 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 () => { diff --git a/packages/mcp-core/src/api-client/client.ts b/packages/mcp-core/src/api-client/client.ts index b41a70d6e..f90694014 100644 --- a/packages/mcp-core/src/api-client/client.ts +++ b/packages/mcp-core/src/api-client/client.ts @@ -305,23 +305,27 @@ type RequestOptions = { }; /** - * Automatic retries for rate limits (429) and transient gateway failures - * (502/503/504) on GET and HEAD requests. Writes are never retried. + * Automatic retries for rate limits (429) on any method, and for transient + * server failures (500/502/503/504) on GET and HEAD requests only. + * + * Sentry rejects a rate-limited request before the endpoint runs, so a 429 is + * safe to replay even for a write. A 5xx may have run the endpoint, so writes + * are not retried on one. Other 5xx codes (501, 505, ...) are deterministic. * * 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. Edge gateway errors ("upstream - * connect error ... reset before headers") clear on a quick retry. + * instead of holding the Workers request open. */ const REQUEST_RETRY_POLICY: RetryPolicy = { maxRetries: 2, - retryableStatuses: [429, 502, 503, 504], + retryableStatuses: [429, 500, 502, 503, 504], baseDelayMs: 250, maxDelayMs: 2000, maxTotalDelayMs: 3000, }; + const API_MAX_PER_PAGE = 100; const MAX_LIST_PAGES = 50; @@ -772,12 +776,12 @@ export class SentryApiService { /** * Makes an authenticated request to the Sentry API, transparently retrying - * rate-limited (429) and gateway-failed (502/503/504) GET and HEAD requests + * rate-limited (429) requests and 500/502/503/504 GET and HEAD requests * under `REQUEST_RETRY_POLICY`. * * A 429 waits for its `Retry-After` or `X-Sentry-Rate-Limit-Reset` delay; - * everything else uses jittered backoff. Writes, persistent 500s, other - * 4xx errors, and network/configuration errors fail fast. Callers that opt + * 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. * diff --git a/packages/mcp-core/src/api-client/errors.ts b/packages/mcp-core/src/api-client/errors.ts index eeb94c17d..a482aab0e 100644 --- a/packages/mcp-core/src/api-client/errors.ts +++ b/packages/mcp-core/src/api-client/errors.ts @@ -212,7 +212,7 @@ export class ApiRateLimitError extends ApiClientError { override toUserMessage(): string { if (this.retryAfterMs === undefined) { - return `${super.toUserMessage()}. Wait a few seconds before retrying, and make fewer requests in parallel.`; + 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"}.`; 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/src/api-retry.test.ts b/packages/toolkit-core/src/api-retry.test.ts index 09a617143..000e83336 100644 --- a/packages/toolkit-core/src/api-retry.test.ts +++ b/packages/toolkit-core/src/api-retry.test.ts @@ -205,17 +205,25 @@ describe("nextRetryDelayMs", () => { } }); - it("never retries a write without the idempotent opt-in", () => { - for (const method of ["PUT", "DELETE"]) { - expect(nextRetryDelayMs({ ...base, method })).toBeUndefined(); + 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); } - for (const status of [429, 502, 503, 504]) { - expect( - nextRetryDelayMs({ ...base, method: "POST", status }), - ).toBeUndefined(); + }); + + it("never retries a write on a 5xx without the idempotent opt-in", () => { + for (const method of ["POST", "PUT", "DELETE"]) { + for (const status of [502, 503, 504]) { + expect(nextRetryDelayMs({ ...base, method, status })).toBeUndefined(); + } } expect( - nextRetryDelayMs({ ...base, method: "POST", idempotent: true }), + nextRetryDelayMs({ + ...base, + method: "POST", + status: 503, + idempotent: true, + }), ).toBe(125); }); diff --git a/packages/toolkit-core/src/api-retry.ts b/packages/toolkit-core/src/api-retry.ts index 806a9faa5..5c9792fd1 100644 --- a/packages/toolkit-core/src/api-retry.ts +++ b/packages/toolkit-core/src/api-retry.ts @@ -27,11 +27,20 @@ const IDEMPOTENT_METHODS = new Set(["GET", "HEAD", "OPTIONS"]); const DATE_RESOLUTION_MS = 1000; /** - * Whether a failed request may be sent again. Writes are only retried when - * the caller says replaying them is safe, such as a content-addressed upload. + * Whether a failed request may be sent again. A 429 is safe for any method: + * Sentry's rate-limit middleware rejects the request in `process_request`, + * before the endpoint runs (`sentry/middleware/ratelimit.py`). Other failures + * may have run the endpoint, so writes are only retried when the caller says + * replaying them is safe, such as a content-addressed upload. */ -function isRetryableMethod(method: string, idempotent = false): boolean { - return idempotent || IDEMPOTENT_METHODS.has(method.toUpperCase()); +function isRetryableMethod( + method: string, + status: number, + idempotent = false, +): boolean { + return ( + status === 429 || idempotent || IDEMPOTENT_METHODS.has(method.toUpperCase()) + ); } /** @@ -96,7 +105,7 @@ export function nextRetryDelayMs({ }): number | undefined { if ( attempt >= policy.maxRetries || - !isRetryableMethod(method, idempotent) || + !isRetryableMethod(method, status, idempotent) || !policy.retryableStatuses.includes(status) ) { return undefined; From abc41b8d479f76c25e9592725c88ef1d76e2cdc2 Mon Sep 17 00:00:00 2001 From: "sentry-junior[bot]" <264270552+sentry-junior[bot]@users.noreply.github.com> Date: Tue, 27 Oct 2026 15:20:24 +0000 Subject: [PATCH 4/5] refactor(toolkit-core): Define retried statuses and methods in one table Replace the per-product retryableStatuses list and the 429 special case with RETRYABLE_STATUSES, a Map from status to the requests it may replay ("any" for 429, "idempotent" for 500/502/503/504). Products now set only caps, so the CLI and MCP share one rule. The contributor docs show the table and link to the code. --- docs/contributing/api-patterns.md | 24 ++++++---- packages/mcp-core/src/api-client/client.ts | 9 +--- packages/toolkit-core/src/api-retry.test.ts | 16 ++++--- packages/toolkit-core/src/api-retry.ts | 49 +++++++++++++++------ 4 files changed, 64 insertions(+), 34 deletions(-) diff --git a/docs/contributing/api-patterns.md b/docs/contributing/api-patterns.md index 5d7b0f81f..b85600636 100644 --- a/docs/contributing/api-patterns.md +++ b/docs/contributing/api-patterns.md @@ -252,13 +252,19 @@ See error patterns in [common-patterns.md](common-patterns.md#error-handling). ### Automatic Retries -`SentryApiService.request()` transparently retries rate limits (`429`) and -transient server failures (`500`/`502`/`503`/`504`), using the shared policy in -`@sentry/toolkit-core/api-retry`. Retries are: +`SentryApiService.request()` transparently retries failed requests using the +shared policy in `@sentry/toolkit-core/api-retry`. Which statuses retry, and +for which methods, is defined once in `RETRYABLE_STATUSES` in +[`api-retry.ts`](../../packages/toolkit-core/src/api-retry.ts): + +| Status | Retried for | +| ------------------------- | --------------------------------------------------- | +| `429` | Any method. Sentry rejects it before the view runs. | +| `500`/`502`/`503`/`504` | GET, HEAD, and OPTIONS, or an `idempotent` request. | +| Anything else, e.g. `501` | Never. These are deterministic. | + +Retries are: -- **Safe for writes only on 429** — Sentry's rate-limit middleware rejects a - request before the endpoint runs, so a `429` is retried for any method. A - `5xx` may have run the endpoint, so only GET and HEAD retry on one. - **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. @@ -266,9 +272,9 @@ transient server failures (`500`/`502`/`503`/`504`), using the shared policy in A longer reset fails at once with an `ApiRateLimitError` whose `retryAfterMs` and user message say when to retry. - **Abortable** — aborting the request's `signal` ends the wait. -- **Status-limited** — deterministic `5xx` codes such as `501`, other `4xx` - errors, and network/configuration errors fail fast. A rate-limited tool call - returns a **Rate Limited** message with the retry time, not an input error. +- **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. diff --git a/packages/mcp-core/src/api-client/client.ts b/packages/mcp-core/src/api-client/client.ts index f90694014..2f34ca969 100644 --- a/packages/mcp-core/src/api-client/client.ts +++ b/packages/mcp-core/src/api-client/client.ts @@ -305,12 +305,8 @@ type RequestOptions = { }; /** - * Automatic retries for rate limits (429) on any method, and for transient - * server failures (500/502/503/504) on GET and HEAD requests only. - * - * Sentry rejects a rate-limited request before the endpoint runs, so a 429 is - * safe to replay even for a write. A 5xx may have run the endpoint, so writes - * are not retried on one. Other 5xx codes (501, 505, ...) are deterministic. + * Caps for automatic retries. Which statuses and methods are retried is the + * shared rule in `@sentry/toolkit-core/api-retry`. * * 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 @@ -320,7 +316,6 @@ type RequestOptions = { */ const REQUEST_RETRY_POLICY: RetryPolicy = { maxRetries: 2, - retryableStatuses: [429, 500, 502, 503, 504], baseDelayMs: 250, maxDelayMs: 2000, maxTotalDelayMs: 3000, diff --git a/packages/toolkit-core/src/api-retry.test.ts b/packages/toolkit-core/src/api-retry.test.ts index 000e83336..6f6998366 100644 --- a/packages/toolkit-core/src/api-retry.test.ts +++ b/packages/toolkit-core/src/api-retry.test.ts @@ -11,7 +11,6 @@ const NOW_SECONDS = Math.floor(NOW / 1000); const policy: RetryPolicy = { maxRetries: 2, - retryableStatuses: [429, 502, 503, 504], baseDelayMs: 250, maxDelayMs: 2000, maxTotalDelayMs: 3000, @@ -213,7 +212,7 @@ describe("nextRetryDelayMs", () => { it("never retries a write on a 5xx without the idempotent opt-in", () => { for (const method of ["POST", "PUT", "DELETE"]) { - for (const status of [502, 503, 504]) { + for (const status of [500, 502, 503, 504]) { expect(nextRetryDelayMs({ ...base, method, status })).toBeUndefined(); } } @@ -227,9 +226,16 @@ describe("nextRetryDelayMs", () => { ).toBe(125); }); - it("does not retry statuses outside the policy", () => { - expect(nextRetryDelayMs({ ...base, status: 500 })).toBeUndefined(); - expect(nextRetryDelayMs({ ...base, status: 404 })).toBeUndefined(); + 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(); + } }); }); diff --git a/packages/toolkit-core/src/api-retry.ts b/packages/toolkit-core/src/api-retry.ts index 5c9792fd1..d4e667c19 100644 --- a/packages/toolkit-core/src/api-retry.ts +++ b/packages/toolkit-core/src/api-retry.ts @@ -10,7 +10,6 @@ export type RetryPolicy = { /** Retries after the first attempt. */ maxRetries: number; - retryableStatuses: readonly number[]; /** First fallback backoff step; it doubles on each retry. */ baseDelayMs: number; /** Longest single wait. A server delay above this is not waited for. */ @@ -21,25 +20,50 @@ export type RetryPolicy = { type HeaderReader = Pick; -const IDEMPOTENT_METHODS = new Set(["GET", "HEAD", "OPTIONS"]); +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 a failed request may be sent again. A 429 is safe for any method: - * Sentry's rate-limit middleware rejects the request in `process_request`, - * before the endpoint runs (`sentry/middleware/ratelimit.py`). Other failures - * may have run the endpoint, so writes are only retried when the caller says - * replaying them is safe, such as a content-addressed upload. - */ -function isRetryableMethod( +/** 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 ( - status === 429 || idempotent || IDEMPOTENT_METHODS.has(method.toUpperCase()) + replays === "any" || + idempotent || + IDEMPOTENT_METHODS.has(method.toUpperCase()) ); } @@ -105,8 +129,7 @@ export function nextRetryDelayMs({ }): number | undefined { if ( attempt >= policy.maxRetries || - !isRetryableMethod(method, status, idempotent) || - !policy.retryableStatuses.includes(status) + !isRetryable(method, status, idempotent) ) { return undefined; } From 6c2938379510d3806a91abd8285c0f401bcb8253 Mon Sep 17 00:00:00 2001 From: "sentry-junior[bot]" <264270552+sentry-junior[bot]@users.noreply.github.com> Date: Tue, 27 Oct 2026 15:27:09 +0000 Subject: [PATCH 5/5] docs(mcp): Link retry rules to code and document rate limits for users The contributor table and caps copied RETRYABLE_STATUSES and REQUEST_RETRY_POLICY by hand, so they could drift. Point to the code instead, and add a short user-facing note on rate limits to the README. --- README.md | 9 +++++++++ docs/contributing/api-patterns.md | 21 +++++++++++---------- 2 files changed, 20 insertions(+), 10 deletions(-) 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 b85600636..82f86e5dc 100644 --- a/docs/contributing/api-patterns.md +++ b/docs/contributing/api-patterns.md @@ -253,23 +253,24 @@ See error patterns in [common-patterns.md](common-patterns.md#error-handling). ### Automatic Retries `SentryApiService.request()` transparently retries failed requests using the -shared policy in `@sentry/toolkit-core/api-retry`. Which statuses retry, and -for which methods, is defined once in `RETRYABLE_STATUSES` in -[`api-retry.ts`](../../packages/toolkit-core/src/api-retry.ts): +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: -| Status | Retried for | -| ------------------------- | --------------------------------------------------- | -| `429` | Any method. Sentry rejects it before the view runs. | -| `500`/`502`/`503`/`504` | GET, HEAD, and OPTIONS, or an `idempotent` request. | -| Anything else, e.g. `501` | Never. These are deterministic. | +- 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** — at most 2 retries, 2 seconds per wait, and 3 seconds in total. - A longer reset fails at once with an `ApiRateLimitError` whose +- **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.