Repository navigation
fix(sentry): group limiter degradation by failure mode, not by route (#6454) - #6458
Merged
Merged
Conversation
…6454) `logRateLimitDegraded` fingerprinted on the raw `stage`, and `stage` embeds the caller: `checkEndpointRateLimit:/api/market/v1/list-market-quotes`, `checkScopedRateLimit:/api/skills/fetch-agentskills`. Sentry groups by fingerprint, so the token meant to consolidate grouping was fragmenting it — one issue per route for a single Redis slowdown. `checkEndpointRateLimit` covers 213 gateway routes, so one sustained incident can open up to 213 issues. This is the exact trap `_sentry-common.js`'s own fingerprint doc warns about: "use when the error message contains a high-cardinality token ... that would otherwise split one logical issue into many." Observed: a 2026-08-11 triage sweep found seven unresolved issues plus two ignored ones for this single handled condition. Three of them (WORLDMONITOR-YY/YX/YZ) were created within two minutes of each other, sharing a release and differing only by which route was being limited. Fix: collapse the stage to its low-cardinality head for grouping only. checkEndpointRateLimit:/api/market/v1/list-market-quotes -> checkEndpointRateLimit checkScopedRateLimit:/api/skills/fetch-agentskills -> checkScopedRateLimit checkScopedRateLimit:/api/x:missing-config -> checkScopedRateLimit:missing-config checkRateLimit:timeout -> checkRateLimit:timeout Failure-mode suffixes are a closed set and keep their own issues — a slow Redis and an unconfigured Redis are different problems. The full stage stays a TAG, and the Sentry dedup key stays the full stage too, so an incident still emits one event per affected route; that is what keeps the per-route breakdown meaningful inside the single grouped issue. Extracted as a pure `rateLimitFingerprintStage()` in both mirrors rather than inlined, so the mapping is unit-testable instead of only observable through a live Sentry capture. The pre-existing source-regex guard now also asserts the raw stage is NOT used as a fingerprint, so the regression cannot come back silently. Not in scope: WORLDMONITOR-R2 looks like this cluster but is not — it is a Convex transport timeout from api/user-prefs with its own deliberate fingerprint. Left alone. Grouping changes for NEW events only; the existing issues will not retroactively merge. Resolve them after deploy and let the next incident open the single issue. Verified: typecheck:api, lint:boundaries, lint:rate-limit-policies, biome, rate-limit (56), edge-functions/docs-mcp/mcp-proxy (384 total), and api _rate-limit/wm-session/analytics-health (75) all pass. Mutation-proved both layers: restoring the raw-stage fingerprint turns the source guard red, and neutralising the collapse turns three unit tests red including the mirror- agreement check. Closes #6454 Claude-Session: https://claude.ai/code/session_016vZfnBuYuBiwAZE1DADrqz
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
koala73
added a commit
that referenced
this pull request
Sep 11, 2026
## Summary A live Pro-MCP failure condition currently reads as drained when it is not. On 2026-09-11, `Pro MCP token validation temporarily unavailable` was firing in two Sentry groups at the same time: WORLDMONITOR-ZR, resolved 2026-09-04 and quiet since, and WORLDMONITOR-T8, which took three events that same day. Anyone looking at ZR concludes the condition stopped. It has not. The cause is that `api/mcp/error-fingerprint.ts` was only ever wired into `dispatch.ts`. The two captures in `validateProMcpAuthorization` fall back to Sentry's default stack grouping, and on the minified edge bundle every frame is an anonymous `(vc/edge/function` with no source map — so they collapse into the T8 catch-all, exactly the pathology that helper's own docstring was written to close. This also means PR #7601 does not do what its commit message says. That PR downgraded the transient arm to `warning` so a sustained Convex outage would "still escalate by volume." Volume split across two groups, one of them a permanently noisy catch-all, escalates nothing. A 25-event T8 sample held seven distinct messages across three steps and two routes. Both arms now carry an explicit fingerprint and stay separable from each other: `threw` for the defect arm, `transient` for the fail-soft arm. That preserves a distinction the surrounding comment already protects — a thrown validator is an unexpected defect, not the fail-soft path. Using distinct discriminators rather than leaning on the error name means the two arms cannot merge even when the thrown error is a plain `Error`. ## What a reviewer should expect after deploy Sentry applies grouping at ingest, so the existing T8 and ZR groups are untouched. New transient-verdict events will land in a **new** group rather than in T8. That new issue appearing is the fix working, not a regression. ZR stays resolved and should remain so, since its own fingerprint no longer receives traffic. Tags and levels are unchanged on both paths, so existing `step:pro-token-validate` queries keep working. ## Validation Written test-first against the real code path, not a source-text pin. The test activates a throwaway DSN before importing the module, stubs `fetch`, and asserts on the delivered Sentry envelope. - Failing first: both arms delivered no `fingerprint` at all (`actual: undefined`). The message and `level: warning` assertions passed in the same run, so the harness was provably exercising the real capture rather than silently no-opping. - After the fix: 3/3 green. - Neighbouring suites: 137 pass across `mcp-pro-token-fingerprint`, `mcp-auth-entitlement-and-limits`, `mcp-error-fingerprint` and `pro-mcp-token`; a further 206 across `mcp`, `mcp-user-key-auth` and `mcp-billing-denial`. The pre-existing WORLDMONITOR-ZR level test still passes. - Mutation-tested per guard: dropping either fingerprint reds two tests; giving both arms the same discriminator reds one. - Lint clean on both changed files. That mutation pass found a real weakness in my own test. The separability assertion used `notDeepEqual` alone, which is satisfied by `undefined !== [...]`, so it passed vacuously when a fingerprint was missing entirely. It now asserts both fingerprints are present before comparing them, and only then does the mutation red it. `npm run typecheck` reports one error in `src/services/webmcp.ts`. It is pre-existing and unrelated — I confirmed it reproduces identically on a clean tree with this change reverted. ## Related - Sentry: WORLDMONITOR-ZR, WORLDMONITOR-T8 - Prior art for this pattern: #6409, #6458, #7228 - Level downgrade this restores the intent of: #7601 --- [](https://github.com/EveryInc/compound-engineering-plugin) 
This branch was successfully deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #6454.
The bug
logRateLimitDegradedfingerprinted on the rawstage, andstageembeds the caller:Sentry groups by fingerprint, so the token meant to consolidate grouping was fragmenting it — one issue per route for a single Redis slowdown.
checkEndpointRateLimitcovers 213 gateway routes, so one sustained incident can open up to 213 issues.It is the exact trap
api/_sentry-common.js's own fingerprint doc warns about:Evidence
A triage sweep on 2026-08-11 found seven unresolved issues plus two ignored ones for this single handled condition. Three of them —
WORLDMONITOR-YY,YX,YZ— were created within two minutes of each other, sharing releaseff93f2137cf9...and differing only by which route was being limited.That trio ruled out per-deploy line drift as the cause and pointed straight at the fingerprint.
The fix
Collapse the stage to its low-cardinality head, for grouping only:
checkEndpointRateLimit:/api/market/v1/list-market-quotescheckEndpointRateLimitcheckScopedRateLimit:/api/skills/fetch-agentskillscheckScopedRateLimitcheckScopedRateLimit:/api/x:missing-configcheckScopedRateLimit:missing-configcheckRateLimit:timeoutcheckRateLimit:timeoutFailure-mode suffixes (
missing-config,timeout) are a closed set and keep their own issues — a slow Redis and an unconfigured Redis are different problems and must not merge.Two deliberate non-changes:
Extracted as a pure
rateLimitFingerprintStage()in both mirrors rather than inlined, so the mapping is unit-testable instead of only observable through a live Sentry capture.Testing
Both layers mutation-proved:
fingerprint: [..., stage]return raw stage)The pre-existing source-regex guard now also asserts the raw stage is not used as a fingerprint (
assert.doesNotMatch), so the regression cannot come back silently — the previous guard only pinned the old literal and would have gone green on any replacement.Not in scope
WORLDMONITOR-R2looks like this cluster but is not:route=api/user-prefs,surface=api,error_shape=transport_timeout,convex_fn=userPreferences:getPreferences. It is a Convex transport timeout with its own deliberate fingerprint atapi/user-prefs.ts:608and is correctly separate. Left alone.After merge
Grouping changes for new events only — the seven existing issues will not retroactively merge. Resolve them once this deploys and let the next incident open the single grouped issue.
Type of change
Affected areas
/api/*)server/_shared/rate-limit.tsand itsapi/_rate-limit.jsmirrorChecklist
npm run typecheck:api)