fix(mcp): bound and retry OAuth start so a transient stall recovers instead of a blank popup by waleedlatif1 · Pull Request #5874 · simstudioai/sim

waleedlatif1 · 2026-07-23T01:13:03Z

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):

The provider is healthy and fast — planetscale returns 401+OAuth in <800ms, the full discovery chain resolves in <300ms, DNS <10ms, and DCR registration returns 201 in 98ms.
Our SSRF-guarded OAuth fetch is fast too — 120/120 legs clean hammering planetscale's discovery + DCR from staging.
/oauth/start uses local AES encryption (no KMS/network) and the Redis refresh lock is not in the start path — both ruled out.
So the intermittent hang is the same transient headers-then-stalled-body class we've documented for CDN-fronted MCP hosts (rare, per-connection; a burst of 120 legs can miss it) — and unlike the callback route, /oauth/start had 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):

Bound every /oauth/start step with a shared timedStep helper (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.
Retry mcpAuthGuarded once 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

Bug fix (resilience — auto-recovery + fast bounded failure + diagnosability)

Testing

2 new start-route tests: retry-on-timeout succeeds on the fresh attempt; a non-timeout (redirect success) is never retried. 472 MCP tests green; oauth suites green; tsc (my files) + biome clean.

Checklist

Code follows project style guidelines
Self-reviewed my changes
Tests added/updated and passing
No new warnings introduced
I confirm that I have read and agree to the terms outlined in the Contributor License Agreement (CLA)

…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.

vercel · 2026-07-23T01:13:09Z

The latest updates on your projects. Learn more about Vercel for GitHub.

1 Skipped Deployment

Project	Deployment	Actions	Updated (UTC)
docs	Skipped		Jul 23, 2026 1:43am

cursor · 2026-07-23T01:13:14Z

PR Summary

Medium Risk
Touches OAuth start/callback request handling and failure modes (504 vs hang); bounded steps cannot cancel in-flight work, so behavior on slow-but-eventual responses depends on the new timeouts.

Overview
Adds per-step timeouts on /api/mcp/oauth/start so stalled DB or OAuth discovery no longer leaves the authorize popup hanging until the client’s ~30s deadline. DB work is capped at 4s per step; mcpAuthGuarded at 10s, with budgets chosen so worst-case work stays under that client limit.

The timedStep / OauthStepTimeoutError logic is moved intolib/mcp/oauth/timed-step.ts and reused by the callback route (local duplicate removed). OauthStepTimeoutError is mapped to 504 with a user-facing “try again” message; there is no server-side auto-retry on timeout (tests assert a single mcpAuthGuarded call) to avoid PKCE/state races on the shared OAuth row.

McpOauthRedirectRequired (successful redirect) is caught inside the bounded auth step and returned as a normal outcome so timedStep does not log it as a failure. Route tests cover auth/DB timeouts, redirect success, and testing mocks export makeTimedStep as a pass-through.

^{Reviewed by Cursor Bugbot for commit d793545. Configure here.}

greptile-apps · 2026-07-23T01:15:13Z

Greptile Summary

This PR bounds OAuth startup work so stalled provider calls return a clear error instead of leaving the authorization popup blank. The main changes are:

Extracts a shared timed-step helper for OAuth routes.
Adds bounded database and authentication steps to the start route.
Removes the unsafe automatic retry and returns 504 on timeout.
Adds tests for timeout handling and successful redirects.

Confidence Score: 5/5

The latest fixes look safe to merge.

The automatic retry that could overlap shared OAuth state has been removed.
All four route-local database steps are included in the updated budget.
No additional issue meets the scope for this follow-up review.

Important Files Changed

Filename	Overview
apps/sim/app/api/mcp/oauth/start/route.ts	Bounds OAuth startup steps, removes automatic retries, and maps timeout errors to 504 responses.
apps/sim/lib/mcp/oauth/timed-step.ts	Introduces the shared timeout and step-logging helper used by both OAuth routes.
apps/sim/app/api/mcp/oauth/callback/route.ts	Replaces the callback route's local timeout implementation with the shared helper.
apps/sim/app/api/mcp/oauth/start/route.test.ts	Adds coverage for timeout responses and successful authorization redirects.

_{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.

waleedlatif1 · 2026-07-23T01:22:11Z

@greptile

waleedlatif1 · 2026-07-23T01:22:12Z

@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.

waleedlatif1 · 2026-07-23T01:30:16Z

@greptile

waleedlatif1 · 2026-07-23T01:30:17Z

@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.

waleedlatif1 · 2026-07-23T01:37:25Z

@greptile

waleedlatif1 · 2026-07-23T01:37:26Z

@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.

waleedlatif1 · 2026-07-23T01:43:49Z

@greptile

waleedlatif1 · 2026-07-23T01:43:50Z

@cursor review

cursor

✅ 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.}

cursorBot reviewed Jul 23, 2026
View reviewed changes

Comment threadapps/sim/app/api/mcp/oauth/start/route.ts
Comment threadapps/sim/app/api/mcp/oauth/start/route.ts Outdated

greptile-appsBot reviewed Jul 23, 2026
View reviewed changes

Comment threadapps/sim/app/api/mcp/oauth/start/route.ts Outdated
Comment threadapps/sim/app/api/mcp/oauth/start/route.ts Outdated

vercelBottemporarily deployed to Preview July 23, 2026 01:21 Inactive

greptile-appsBot reviewed Jul 23, 2026
View reviewed changes

Comment threadapps/sim/app/api/mcp/oauth/start/route.ts Outdated

cursorBot reviewed Jul 23, 2026
View reviewed changes

Comment threadapps/sim/app/api/mcp/oauth/start/route.ts Outdated

vercelBottemporarily deployed to Preview July 23, 2026 01:30 Inactive

cursorBot reviewed Jul 23, 2026
View reviewed changes

Comment threadapps/sim/app/api/mcp/oauth/start/route.ts

fix(mcp): bound the setOauthRowUser write too so no step escapes the …
43dcd07
…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.

vercelBottemporarily deployed to Preview July 23, 2026 01:37 Inactive

cursorBot reviewed Jul 23, 2026
View reviewed changes

Comment threadapps/sim/app/api/mcp/oauth/start/route.ts Outdated

greptile-appsBot reviewed Jul 23, 2026
View reviewed changes

Comment threadapps/sim/app/api/mcp/oauth/start/route.ts Outdated

vercelBottemporarily deployed to Preview July 23, 2026 01:43 Inactive

cursorBot reviewed Jul 23, 2026
View reviewed changes

waleedlatif1 merged commit 227cd65 into stagingJul 23, 2026
20 checks passed

waleedlatif1 deleted the fix/mcp-oauth-start-bound-steps branch July 23, 2026 03:31

waleedlatif1 mentioned this pull request Jul 23, 2026
v0.7.42: event-driven discovery for MCP, nuqs adoption for deeplinks, test speedup, org session policies, proxy URL for API calls #5870
Merged

Uh oh!

Conversation

waleedlatif1 commented Jul 23, 2026

Summary

Type of Change

Testing

Checklist

Uh oh!

vercelBot commented Jul 23, 2026• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cursorBot commented Jul 23, 2026• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Summary

Uh oh!

Uh oh!

Uh oh!

greptile-appsBot commented Jul 23, 2026• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Greptile Summary

Confidence Score: 5/5

Important Files Changed

Uh oh!

Uh oh!

Uh oh!

waleedlatif1 commented Jul 23, 2026

Uh oh!

waleedlatif1 commented Jul 23, 2026

Uh oh!

Uh oh!

Uh oh!

waleedlatif1 commented Jul 23, 2026

Uh oh!

waleedlatif1 commented Jul 23, 2026

Uh oh!

Uh oh!

waleedlatif1 commented Jul 23, 2026

Uh oh!

waleedlatif1 commented Jul 23, 2026

Uh oh!

Uh oh!

Uh oh!

waleedlatif1 commented Jul 23, 2026

Uh oh!

waleedlatif1 commented Jul 23, 2026

Uh oh!

cursorBot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

vercelBot commented Jul 23, 2026•
edited
Loading

cursorBot commented Jul 23, 2026•
edited
Loading

greptile-appsBot commented Jul 23, 2026•
edited
Loading