Repository navigation
fix(mcp): Retry rate-limited GETs using Sentry rate-limit reset headers - #1673
Conversation
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>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit a4f9a98. Configure here.
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.
…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.
…uses Sentry's RatelimitMiddleware rejects a 429 in process_request, before the view runs, so replaying one cannot repeat a write. The sentry-api-transport spec now retries 429s for any method, and retries 5xx, timeouts, and network errors only for GET/HEAD/OPTIONS or an explicit idempotent opt-in. The retried statuses are 429, 500, 502, 503, and 504. Other 5xx codes are deterministic. The clock-skew wording now matches the Date-bounded delay from #1673. The long requirements are split so `openspec validate --strict` passes.
BYK
left a comment
There was a problem hiding this comment.
We should then abstract this logic out either into toolkit-core or @sentry/api (ideally) and make it shared across CLI and MCP
| `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 |
There was a problem hiding this comment.
We should link this information to the code or from the code. Ideally the code should have something like:
const RETRIABLES = {
429: true,
500: ["GET", "HEAD", "OPTIONS"],
};or other way around:
const RETRIABLES = {
"GET": [429, 500, 502, 503, 504],
"POST": [429],
}etc. Or some rule. Also ideally these will be Map and Set instances yadda yadda.
There was a problem hiding this comment.
Done in abc41b8. packages/toolkit-core/src/api-retry.ts now has RETRYABLE_STATUSES: ReadonlyMap<number, "any" | "idempotent">, which maps 429 to any and 500/502/503/504 to idempotent. IDEMPOTENT_METHODS is a ReadonlySet. The any/idempotent rationale lives on the map. RetryPolicy lost retryableStatuses, so a product only sets caps and can't drift from the rule. This doc section now shows the same table and links to the file, and the code comment points back here.
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.
|
On sharing this between CLI and MCP: the rule now lives only in I haven't switched the CLI in this PR. #1677 (
|
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.
…reset on 429 (#1677) ## Problem `packages/cli/src/lib/sentry-client.ts` retried **every** HTTP method, including POST/PUT/DELETE, on 408/429/500/502/503/504 and on network errors and timeouts. That can replay writes. A 429 also only got the generic 1s/2s backoff: the client never read Sentry's `X-Sentry-Rate-Limit-Reset` header, and Sentry doesn't send `Retry-After` on API rate limits. ## Behavior changes - **5xx, timeouts and network errors** now retry only for GET, HEAD and OPTIONS, or when a request sets the new `idempotent: true` opt-in on `getSdkConfig` / `ApiRequestOptions`. - **429 retries for any method.** Sentry's rate-limit middleware rejects the request in `process_view`, before the endpoint runs, so nothing was applied. - **Retried statuses stay 408/429/500/502/503/504.** Deterministic 5xx like 501 and 505 are not retried. - **PUT and DELETE are deliberately not retried.** Sentry PUTs have side effects (merge, activity), and a replayed DELETE returns 404. - **The 401-refresh retry still covers every method**, and no longer waits 1s first. - **A timed-out write** now gets a "may still have been applied" hint. - **The 429 wait** is the longer of `X-Sentry-Rate-Limit-Reset` (read only when Remaining is 0) and `Retry-After` (seconds or HTTP date), both measured against the response `Date` to guard against clock skew. Concurrency-limit 429s, or 429s with no hint, use jittered backoff: the upper half of min(1s·2^n, 10s). - **Caps are unchanged:** `MAX_RETRIES` is 2 and each wait is at most 10s. - **New `RateLimitError`** (extends `ApiError`, status 429). It's thrown when the wait would exceed the cap or retries run out. The message is agent-friendly: server detail, then "Retry in N seconds; do not repeat the request before then. Make fewer requests in parallel." - **The policy is pure**, with no CLI imports, in `packages/cli/src/lib/retry-policy.ts`. That way the CLI can switch to the shared `@sentry/toolkit-core/api-retry` from #1673 without a rewrite. ## POST callers given the `idempotent` opt-in Each was checked against its getsentry/sentry handler: - **Chunk uploads** (`chunk-upload.ts`): blobs are stored by SHA-1 (`FileBlob.from_files`). - **Artifact-bundle assemble and `pollAssembly`** (`sourcemaps.ts`). - **DIF assemble** in `debug-files.ts`, `proguard.ts` and `dart-symbols.ts`: the server checks the cached assemble status by checksum. **Not opted in:** preprod assemble (each call creates a new `PreprodArtifact`), Seer `/autofix/`, snapshot create and archive trigger, and SDK and `sentry api` writes. ## Tests - The POST-retry body-cloning tests now go through the 429 and opt-in paths. - New cases: - a rate-limited POST is resent with the same body - a 501 is not retried - a 503 on a non-idempotent method is not retried - the reset-header and `Retry-After` wait, clock skew, and the cap leading to `RateLimitError` - the exact rate-limit message text - `api-client.coverage` used 429 as a sample error and now uses 400. ## Checks - `tsc --noEmit` and `pnpm run lint` pass. - The sentry-client, retry-policy, api-client and api-client.coverage tests pass: 204/204. - Full CLI suite on `bf3b710`: 11,482 passed and 6 failed. The same 6 fail on `main` (`37c7193`) in this sandbox. They're chmod-based permission tests (completions ×2, code scanner, init file-changes, telemetry readonly, upgrade lock), and the sandbox runs with full capabilities, so they can't fail the way they expect. - After the 429-any-method change (`7099e2b`), all other CLI test files that mention 429 pass (1,195), apart from the 2 known sandbox permission failures. ## Relation to #1669 This matches the retry policy in #1669 (task 5.0 and the `sentry-api-transport` spec). Task 5.1 will move this behavior into toolkit-core's shared transport unchanged. <!-- junior-request-attribution:start --> via **BYK**. <!-- junior-request-attribution:end --> <!-- junior-session-footer:start --> <!-- junior-conversation-id:slack%3AC08J1NSPU6S%3A1792772794.175999 --> -- [View Junior Session](https://junior-prod.sentry.dev/conversations/slack%3AC08J1NSPU6S%3A1792772794.175999) [[Sentry]](https://sentry.sentry.io/explore/conversations/slack%3AC08J1NSPU6S%3A1792772794.175999/?project=4510944073809921) <!-- junior-session-footer:end --> --------- Co-authored-by: sentry-junior[bot] <264270552+sentry-junior[bot]@users.noreply.github.com> Co-authored-by: Burak Yigit Kaya <126780+BYK@users.noreply.github.com>
Follow-up to #1677 and #1673, and the last part of #1669 task 5.0. The CLI's interim `retry-policy.ts` duplicated `@sentry/toolkit-core/api-retry`, with small differences between the two. The CLI now uses the shared module, so the CLI and MCP have one rule for which statuses and methods get retried and how long a 429 waits. Each still sets its own limits: the CLI's `RETRY_POLICY` keeps 2 retries, a 1s base and a 10s per-wait cap, and adds a 30s total. Changes in `toolkit-core/src/api-retry.ts`: - New export `isIdempotentRequest(method, idempotent)`. The CLI uses it for the timeout error wording and for `generate-api-schema`'s idempotent-operation filter. - `nextRetryDelayMs` now accepts `status: undefined` for a network error or timeout, which is retried only for idempotent requests. The MCP always passes a status, so its behavior is unchanged. Behavior changes in the CLI, all from taking on the shared rule: - **408 is no longer retried.** The shared list is 429/500/502/503/504, as #1669's 5.0 spec says. Sentry doesn't send 408 itself. - **Clock skew:** with a skewed local clock, the wait is now clamped into the 1s window allowed by the response `Date` header, instead of being measured from `Date`. One `sentry-client` test asserted the old ≥900ms wait; it now asserts that the wait stays inside that window and not the hour of skew. - **Retry-After:** when both `Retry-After` and the rate-limit reset are present, `Retry-After` wins. Before, the CLI took the longer of the two. - **Jitter:** a server-requested wait gets up to `baseDelayMs / 2` of extra delay. Backoff without a server hint works the same as before. Two things stay the same: 401 token refresh still works for any method, and writes that Sentry marks idempotent (#1706) still retry. <!-- junior-request-attribution:start --> via **BYK**. <!-- junior-request-attribution:end --> <!-- junior-session-footer:start --> <!-- junior-conversation-id:slack%3AC08J1NSPU6S%3A1793057920.879309 --> -- [View Junior Session](https://junior-prod.sentry.dev/conversations/slack%3AC08J1NSPU6S%3A1793057920.879309) [[Sentry]](https://sentry.sentry.io/explore/conversations/slack%3AC08J1NSPU6S%3A1793057920.879309/?project=4510944073809921) <!-- junior-session-footer:end --> --------- Co-authored-by: sentry-junior[bot] <264270552+sentry-junior[bot]@users.noreply.github.com> Co-authored-by: BYK <burak.kaya@sentry.io>
Moves the CLI's request loop out of `packages/cli/src/lib/sentry-client.ts` and into a new Workers-safe module, `@sentry/toolkit-core/api-transport`, then switches the CLI over to it. This covers tasks **5.1** and **5.2** of #1669 (`unify-sentry-api-layer`). It builds on the shared retry policy from #1710. ### What's now shared `createApiTransport` returns a fetch-shaped `request()` that owns: - **Auth**: it sets the `Authorization` header from an injected `CredentialProvider`. The provider supplies the current credential, an optional refresh before expiry, and the refresh after a 401. A 401 refreshes once and replays the request for any method. - **Retries** through `api-retry`. A 429 retries for any method. A 500/502/503/504, a timeout, or a network error retries only for GET/HEAD/OPTIONS, or when the request is marked `idempotent`. Waits follow `Retry-After` or `X-Sentry-Rate-Limit-Reset`, under the caller's caps. - **Per-attempt timeouts**: a fixed value or a function of the URL, so the CLI keeps `ENDPOINT_TIMEOUT_OVERRIDES`. - **Body replay** on every attempt, stream bodies included, plus an optional buffered-body mode (`auto` / `buffer` / `stream`). In `buffer` mode, a body that fails mid-read can still be retried. - **Validated redirects**: it follows redirects by hand, sends the credential only to origins the host trusts, and records where the response came from (`getValidatedResponseSource`). - **Cache port** (`TransportCache`): it looks up GETs, revalidates stale entries with their `ETag`, stores successful GETs, and invalidates entries after a successful write. The host plugs in header rules (which may reject a request), its error classes for timeouts, rate limits, and fatal fetch errors (such as TLS failures), a logger, and a span hook. The module uses no Node APIs. ### CLI wiring `sentry-client.ts` now just configures the transport. It passes in its stored credentials and refresh logic, its host-scope and claim checks (`prepareHeaders`), the custom TLS fetch, the disk response cache, its tracing span, and its existing error classes. It keeps `RETRY_POLICY`, `ENDPOINT_TIMEOUT_OVERRIDES`, and the `x-sentry-cli-retry` marker. `getResponseRequestOrigin` and `getResponseCredentialIdentity` now read the transport's provenance. `fetch-input.ts` re-exports `extractInputUrl` from the shared module. ### Behavior changes The goal is the same CLI behavior. One intentional difference: - An unused response body (one dropped before a retry, a refresh, or a redirect hop) is now cancelled without awaiting the cancel. When a stream is teed, cancelling one branch only settles after the other branch is read. Interceptors such as MSW hand out teed streams, so the old awaited cancel could hang a retry indefinitely. A regression test covers this. ### Stacking (#1669) - Done: 5.0 shared retry policy (#1673, #1710). - This PR: 5.1 shared transport and 5.2 the CLI switch. - Next, stacked on this branch and opened after it merges: - 5.3: a request-scoped fresh-read bypass. - 6.1: `fetch` injection for the MCP. `SentryApiService` drops its own retry loop so retries don't stack. The OpenSpec tasks file is unchanged here. ### Verification - `toolkit-core`: 21 files, 269 tests pass, including the new `api-transport.test.ts`. It covers the retry scenarios, 401 refresh, timeouts, fatal errors, header rules, redirects, the cache, and the teed-body regression. - CLI unit tests: 11,735 pass. 6 fail, and those same 6 fail on clean `main` in this sandbox (file-permission tests in completions, telemetry, upgrade, code-scanner, file-changes). - `tsc --noEmit` passes for `toolkit-core` and `cli`. `oxlint`, `oxfmt --check`, and `ast-grep scan` are clean on the changed paths. <!-- junior-request-attribution:start --> via **BYK**. <!-- junior-request-attribution:end --> <!-- junior-session-footer:start --> <!-- junior-conversation-id:agent%3Aa6eddc0161cff61914454da9f74fcdf1 --> -- [View Junior Session in Sentry](https://sentry.sentry.io/explore/conversations/agent%3Aa6eddc0161cff61914454da9f74fcdf1/?project=4510944073809921) <!-- junior-session-footer:end --> Co-authored-by: sentry-junior[bot] <264270552+sentry-junior[bot]@users.noreply.github.com> Co-authored-by: BYK <burak.kaya@sentry.io>
This adds a request-scoped fresh-read bypass to the shared API transport in `@sentry/toolkit-core/api-transport`. It is task **5.3** of #1669 (`unify-sentry-api-layer`) and builds on the shared transport from #1719. ### What's shared `createApiTransport` accepts a new optional `fresh?(): boolean` hook. While the hook returns true: - GETs skip the cache lookup and go to the network. - The response is still stored, so it replaces the cached entry and later reads see the new data. A host backs the hook with request-scoped state, for example `AsyncLocalStorage` around a single MCP tool call that asked for `fresh: true`. That keeps the bypass to one call rather than the whole process. This is the transport side of the MCP's `fresh` input planned in 6.3. ### Behavior changes None for current callers. The hook is optional, and no host sets it yet. The CLI's `--fresh` flag keeps its existing process-wide bypass in `cli/src/lib/response-cache.ts`, with reads skipped and writes kept as before. This PR leaves that unchanged. ### Relation to #1669 - Done: 5.0 shared retry policy (#1673, #1710), and 5.1 + 5.2 shared transport with the CLI switch (#1719). - This PR: 5.3. - Next, stacked on this branch and opened once it merges: 6.1. That adds `fetch` to `McpServerOptions` and routes all `SentryApiService` and `@sentry/api` requests through it. `SentryApiService` drops its own retry loop, so retries happen in the shared transport only. ### Verification - New transport test `skips the cache in a fresh scope and replaces the entry`. It stores v1, then reads in a fresh scope and gets v2 from the network. Once the fresh scope ends, the next read is a cache hit for v2. - `toolkit-core`: 21 files and 270 tests pass. - CLI request, cache and API tests (`test/lib/sentry-client*`, `test/lib/response-cache*`, `test/lib/api`): 49 files and 1,049 tests pass. - `tsc --noEmit` passes for `toolkit-core` and `cli`. - `oxlint`, `oxfmt --check` and `ast-grep scan` are clean on the changed files. <!-- junior-request-attribution:start --> via **BYK**. <!-- junior-request-attribution:end --> <!-- junior-session-footer:start --> <!-- junior-conversation-id:agent%3Aa6eddc0161cff61914454da9f74fcdf1 --> -- [View Junior Session in Sentry](https://sentry.sentry.io/explore/conversations/agent%3Aa6eddc0161cff61914454da9f74fcdf1/?project=4510944073809921) <!-- junior-session-footer:end --> Co-authored-by: sentry-junior[bot] <264270552+sentry-junior[bot]@users.noreply.github.com> Co-authored-by: BYK <burak.kaya@sentry.io>
This PR sends every MCP request to the Sentry API through the shared transport, and moves retries out of `SentryApiService` into it. It is task **6.1** of #1669 (`unify-sentry-api-layer`) and builds on the shared transport (#1719) and the fresh-read hook (#1720). ### What changed - **One transport:** `SentryApiService` sends every request through `createApiTransport` from `@sentry/toolkit-core/api-transport`, using the MCP's existing `REQUEST_RETRY_POLICY`. That includes `@sentry/api` operations, which already go through `request()`. - **One retry layer:** `SentryApiService` no longer has its own retry loop or network-failure bookkeeping. The shared transport retries a 429 for any method, and a 500/502/503/504 or a network failure for GET and HEAD. `SentryApiService` still parses the final error response, so `ApiRateLimitError.retryAfterMs` is still read from `Retry-After` or `X-Sentry-Rate-Limit-Reset`. - **Transport options:** `credentials` and `timeoutMs` on `createApiTransport` are now optional. Without a `CredentialProvider`, the transport keeps the `Authorization` header the request already has, never refreshes, and skips the cache, because cache entries are tied to a credential. The MCP uses it this way, and still sets no timeout, as before. - **Abort reason:** an abort while a request is in flight now rejects with the caller's `signal.reason`, instead of being treated as a retryable network failure. An earlier revision also added a host-supplied `fetch` option to `McpServerOptions` and `ServerContext`. That is gone: per the revised #1669 plan, the shared API layer moves into toolkit-core and then `@sentry/api`, and the CLI passing its fetch to `sentry mcp` is deferred. ### Intentional behavior changes - **`allowStatuses`:** a status a caller lists there is now returned only after the transport has finished retrying it. Before, it came back on the first attempt. Today's callers only list 404 and 400, which are never retried, so they see no difference. - **Abort during a retry wait:** if the caller's signal aborts while the transport is waiting to retry, the abort error is rethrown as-is. It is no longer wrapped as a connection failure. ### Relation to #1669 - Done: 5.0 (#1673, #1710), 5.1 and 5.2 (#1719), 5.3 (#1720). - This PR: 6.1. - Next: consolidate the MCP's `SentryApiService` and the CLI's API modules into one browser-safe API client in toolkit-core, stacked on this PR. ### Verification - New `mcp-core` tests: - A GET that is rate-limited on every attempt reaches the API at most `maxRetries + 1` times. - An in-flight abort rejects with the caller's reason, after one attempt. - New `toolkit-core` tests: - Without credentials, the request's own `Authorization` is kept. - Without credentials, the cache is never consulted. - An in-flight abort keeps the caller's reason. - Suites: | Package | Result | | --- | --- | | `toolkit-core` | 273 pass | | `mcp-core` | 1,899 pass, 6 skipped | | `mcp-server` | 87 pass | | `mcp-cloudflare` | 443 pass | - `tsc --noEmit` passes for `toolkit-core`, `mcp-core`, `mcp-server`, `mcp-cloudflare`, and `cli`. `oxlint` and `oxfmt --check` are clean on the changed files. - Test changes: - Hand-built response mocks in `client.test.ts` gained a `status`, because the transport reads it to decide on a retry. - Header assertions in `client.test.ts` and `mcp-handler.test.ts` now read through `Headers`, because the transport passes a `Headers` object. <!-- junior-request-attribution:start --> via **BYK**. <!-- junior-request-attribution:end --> <!-- junior-session-footer:start --> <!-- junior-conversation-id:agent%3Aa6eddc0161cff61914454da9f74fcdf1 --> -- [View Junior Session in Sentry](https://sentry.sentry.io/explore/conversations/agent%3Aa6eddc0161cff61914454da9f74fcdf1/?project=4510944073809921) <!-- junior-session-footer:end --> --------- Co-authored-by: sentry-junior[bot] <264270552+sentry-junior[bot]@users.noreply.github.com> Co-authored-by: BYK <burak.kaya@sentry.io>

The hosted MCP gets rate limited by the Sentry API a lot. In the last 7 days
sentry/mcp-serverlogged about 32.6Khttp.client429s, nearly all on GETs, usually "Limit is 40 requests in 1 seconds", plus about 72 concurrency-limit hits.SentryApiService.request()only retried 502/503/504, so every 429 failed the tool call.ApiRateLimitError.retryAfterwas read from body fields Sentry never sends, so nothing could say when to try again.Sentry's 429 body is just
{"detail": ...}. The timing comes in headers set bysrc/sentry/middleware/ratelimit.py:X-Sentry-Rate-Limit-Resetis the Unix second when the next window starts, andRemainingis0only when the fixed window is used up. Concurrency 429s carry a reset that doesn't apply to them.This adds
@sentry/toolkit-core/api-retry, a pure retry policy with no Node APIs, so it is safe on Workers. It is task 5.0 in #1669. The MCP adopts it here:idempotentopt-in exists for the CLI's content-addressed uploads, but the MCP doesn't use it.Retry-After(seconds or HTTP-date) if a proxy sent one, otherwise untilX-Sentry-Rate-Limit-Reset. The reset is measured against the responseDatewhen the local clock is off by more than 2s, plus a small jitter. Concurrency 429s and gateway errors use equal-jitter exponential backoff starting at 250ms.ApiRateLimitErrorwith a realretryAfterMs, and its message ends with "Retry in N seconds." When there is no header, it says to wait a few seconds and make fewer parallel requests.signalends the wait immediately.ApiRateLimitError.retryAfter(seconds, from the body) is replaced byretryAfterMs, taken from the headers. Nothing read the old field.The CLI currently has its own interim retry fix in a separate CLI-only PR. It moves onto this shared module in a follow-up after that PR lands. Once the MCP is on the shared transport (#1669 task 6.1),
SentryApiServicedrops this loop so retries don't stack.via @BYK.
--
View Junior Session [Sentry]