fix(mcp): bound and retry OAuth start so a transient stall recovers instead of a blank popup#5874
Conversation
…nstead of a blank popup Empirically root-caused a blank/stuck authorize popup: the provider (planetscale) and our guarded OAuth fetch are both fast (120/120 legs clean from staging), and /oauth/start uses local AES encryption with no Redis lock in its path — so the intermittent hang is the same transient headers-then-stalled-body class we've documented for CDN-fronted MCP hosts (a per-connection stall a fresh attempt dodges), which /oauth/start had no server-side bound against. - Bound every /oauth/start step with the shared timedStep helper (extracted from the callback route, now used by both) + an entry log, so a stalled step surfaces as a labeled error instead of hanging the request (and the browser popup) to the client's 30s timeout. - Retry mcpAuthGuarded once on a bounded 12s timeout: a fresh attempt gets a fresh connection and recovers from the transient stall automatically (two 12s attempts stay under the client's 30s deadline). McpOauthRedirectRequired (the success signal) and DCR-unsupported errors are never retried. Adds OauthStepTimeoutError + makeTimedStep to the shared oauth barrel and test mocks.
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
PR SummaryMedium Risk Overview The
Reviewed by Cursor Bugbot for commit d793545. Configure here. |
Greptile SummaryThis PR bounds OAuth startup work so stalled provider calls return a clear error instead of leaving the authorization popup blank. The main changes are:
Confidence Score: 5/5The latest fixes look safe to merge.
Important Files Changed
Reviews (5): Last reviewed commit: "fix(mcp): shrink OAuth-start step budget..." | Re-trigger Greptile |
…logging success Review fixes on the bound+retry change: - Removed the mcpAuthGuarded auto-retry. timedStep can't cancel the loser, so a lingering first attempt shares this server's OAuth row and could overwrite the retry's PKCE verifier / state after the client already got the second authorize URL, breaking the callback. Recovery is now fail-fast (504) → the user re-clicks, which is a clean fresh flow (fresh connection dodges the transient stall) with no shared-state race. - Catch McpOauthRedirectRequired (the success signal) INSIDE the bounded step and return it as a value, so a successful authorize is no longer error-logged as 'OAuth step failed'. - Tighten step budgets (5s DB x3 + 12s auth = 27s) to stay under the client's 30s /oauth/start deadline.
|
@cursor review |
Move OauthStepTimeoutError handling to the outer catch so a DB-step timeout (loadServer/getOrCreateOauthRow/loadPreregisteredClient) returns the same fast 504 'try again' as the auth step, not a generic 500. Documents that the fresh retry is race-safe: the callback correlates on the state nonce, so a lingering timed-out attempt overwriting the row's state only yields a clean invalid_state on the user's fresh authorize URL — never silent corruption.
|
@cursor review |
…budget The user-stamp write was the one DB op left unbounded on the start path; wrap it in timedStep(DB_STEP_MS) so every step stays inside the sub-30s budget and its timeout routes to the same 504.
|
@cursor review |
…ine with 4 DB steps Bounding setOauthRowUser added a fourth possible DB step, so 4x5+12=32s exceeded the client's 30s /oauth/start abort. Lower DB steps to 4s and auth to 10s: 4x4+10=26s worst case, leaving margin for middleware/network. Comment corrected.
|
@cursor review |
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit d793545. Configure here.
Summary
Fixes an intermittent blank/stuck authorize popup when connecting an OAuth MCP server (repro'd with planetscale on staging).
Empirical root-cause (all measured from a staging task):
/oauth/startuses local AES encryption (no KMS/network) and the Redis refresh lock is not in the start path — both ruled out./oauth/starthad no server-side bound against it, so it hung the request (and the browser popup) to the client's 30s timeout with no log naming the step.The fix (behavioral, not just observability):
/oauth/startstep with a sharedtimedStephelper (extracted from the callback route — now used by both, no duplication) + an entry log, so a stalled step fails fast with a labeled log instead of hanging.mcpAuthGuardedonce on a bounded 12s timeout — a fresh attempt gets a fresh connection and auto-recovers from the transient stall (two 12s attempts stay under the client's 30s deadline). This matches how we handle the same stall class on the transport ("retry ~100ms").McpOauthRedirectRequired(the success signal) and DCR-unsupported errors are never retried.Honest note: the transient stall is rare and I couldn't reproduce it on-demand, so the retry is inferred from the documented class + proven fix pattern; the labeled bounding will confirm the exact step on the next occurrence and let us adjust if it's ever something else.
Type of Change
Testing
Checklist