fix(mcp): bound and retry OAuth start so a transient stall recovers instead of a blank popup (#5874)

* fix(mcp): bound and retry OAuth start so a transient stall recovers instead 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.

* fix(mcp): drop the unsafe OAuth-start retry; fail fast without error-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.

* fix(mcp): route all bounded-step timeouts to the 504 handler

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.

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

* fix(mcp): shrink OAuth-start step budgets to fit the 30s client deadline 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.

Author: waleedlatif1

apps/sim/app/api/mcp/oauth/callback/route.ts

@@ -16,55 +16,17 @@ import {
   loadOauthRowByState,
   loadPreregisteredClient,
   type McpOauthCallbackReason,
+  makeTimedStep,
   mcpAuthGuarded,
   SimMcpOauthProvider,
 } from '@/lib/mcp/oauth'
 import { mcpService } from '@/lib/mcp/service'
 
 const logger = createLogger('McpOauthCallbackAPI')
+const timedStep = makeTimedStep(logger)
 
 export const dynamic = 'force-dynamic'
 
-class OauthCallbackStepTimeout extends Error {
-  constructor(step: string, ms: number) {
-    super(`MCP OAuth callback step "${step}" did not settle within ${ms}ms`)
-    this.name = 'OauthCallbackStepTimeout'
-  }
-}
-
-/**
- * Times and bounds one awaited step of the callback so a stalled operation
- * surfaces as a labeled, logged error instead of hanging the request forever.
- * The losing promise is not cancelled (a wedged DB/socket op can't be), so it
- * settles in the background with its rejection swallowed; the point is that the
- * request stops waiting on it and the logs name the exact step that stalled.
- */
-async function timedStep<T>(step: string, ms: number, fn: () => Promise<T>): Promise<T> {
-  const start = Date.now()
-  logger.info(`OAuth callback step start: ${step}`)
-  const work = Promise.resolve(fn())
-  work.catch(() => {})
-  let timer: ReturnType<typeof setTimeout> | undefined
-  try {
-    const value = await Promise.race([
-      work,
-      new Promise<never>((_, reject) => {
-        timer = setTimeout(() => reject(new OauthCallbackStepTimeout(step, ms)), ms)
-        timer.unref?.()
-      }),
-    ])
-    logger.info(`OAuth callback step done: ${step} (${Date.now() - start}ms)`)
-    return value
-  } catch (error) {
-    logger.error(`OAuth callback step failed: ${step} (${Date.now() - start}ms)`, {
-      error: toError(error).message,
-    })
-    throw error
-  } finally {
-    clearTimeout(timer)
-  }
-}
-
 function escapeHtml(value: string): string {
   return value
     .replace(/&/g, '&amp;')

apps/sim/app/api/mcp/oauth/start/route.test.ts

@@ -7,6 +7,7 @@ import {
   McpOauthRedirectRequiredMock,
   mcpOauthMock,
   mcpOauthMockFns,
+  OauthStepTimeoutErrorMock,
   permissionsMock,
   permissionsMockFns,
   resetDbChainMock,
@@ -75,6 +76,54 @@ describe('MCP OAuth start route', () => {
     )
   })
 
+  it('returns 504 (not a retry) when the auth step times out', async () => {
+    // The stall is intentionally NOT auto-retried — a lingering attempt shares the OAuth row and
+    // could corrupt the retry's PKCE/state. The bounded step fails fast; the user re-clicks.
+    mcpOauthMockFns.mockMcpAuthGuarded.mockImplementationOnce(() => {
+      throw new OauthStepTimeoutErrorMock('mcpAuthGuarded', 12_000)
+    })
+    const request = new NextRequest(
+      'http://localhost:3000/api/mcp/oauth/start?workspaceId=workspace-1&serverId=server-1'
+    )
+
+    const response = await GET(request)
+
+    expect(mcpOauthMockFns.mockMcpAuthGuarded).toHaveBeenCalledTimes(1)
+    expect(response.status).toBe(504)
+  })
+
+  it('returns 504 (not a generic 500) when a DB step times out', async () => {
+    // DB-step timeouts are bounded too; their OauthStepTimeoutError must reach the same
+    // 504 handler, not fall through to the generic 500.
+    mcpOauthMockFns.mockGetOrCreateOauthRow.mockImplementationOnce(() => {
+      throw new OauthStepTimeoutErrorMock('getOrCreateOauthRow', 5_000)
+    })
+    const request = new NextRequest(
+      'http://localhost:3000/api/mcp/oauth/start?workspaceId=workspace-1&serverId=server-1'
+    )
+
+    const response = await GET(request)
+
+    expect(response.status).toBe(504)
+    expect(mcpOauthMockFns.mockMcpAuthGuarded).not.toHaveBeenCalled()
+  })
+
+  it('returns the authorize URL without error-logging the success redirect throw', async () => {
+    mcpOauthMockFns.mockMcpAuthGuarded.mockRejectedValueOnce(
+      new McpOauthRedirectRequiredMock('https://mcp.exa.ai/authorize')
+    )
+    const request = new NextRequest(
+      'http://localhost:3000/api/mcp/oauth/start?workspaceId=workspace-1&serverId=server-1'
+    )
+
+    const response = await GET(request)
+    const body = await response.json()
+
+    expect(mcpOauthMockFns.mockMcpAuthGuarded).toHaveBeenCalledTimes(1)
+    expect(response.status).toBe(200)
+    expect(body).toEqual({ status: 'redirect', authorizationUrl: 'https://mcp.exa.ai/authorize' })
+  })
+
   it('requires workspace write permission via MCP auth middleware', async () => {
     const request = new NextRequest(
       'http://localhost:3000/api/mcp/oauth/start?workspaceId=workspace-1&serverId=server-1'

apps/sim/app/api/mcp/oauth/start/route.ts

@@ -16,14 +16,31 @@ import {
   loadPreregisteredClient,
   McpOauthInsecureUrlError,
   McpOauthRedirectRequired,
+  makeTimedStep,
   mcpAuthGuarded,
+  OauthStepTimeoutError,
   SimMcpOauthProvider,
   setOauthRowUser,
 } from '@/lib/mcp/oauth'
 import { createMcpErrorResponse } from '@/lib/mcp/utils'
 
 const logger = createLogger('McpOauthStartAPI')
+const timedStep = makeTimedStep(logger)
 const OAUTH_START_TTL_MS = 10 * 60 * 1000
+/**
+ * Per-step budgets, kept small so the whole request stays under the client's 30s `/oauth/start`
+ * deadline even in the worst case: up to four bounded DB steps (loadServer, getOrCreateOauthRow,
+ * setOauthRowUser, loadPreregisteredClient) + the auth step = 4×4 + 10 = 26s, leaving margin for
+ * middleware and network. OAuth discovery + DCR occasionally hits the transient
+ * headers-then-stalled-body class documented for CDN-fronted MCP hosts; the bound turns that into
+ * a fast, labeled failure so the popup closes with a clear error and the user can retry (a fresh
+ * click = a fresh connection that dodges the per-connection stall) rather than the popup hanging
+ * blank. We deliberately do NOT auto-retry here: `timedStep` can't cancel a wedged attempt, and a
+ * lingering first attempt sharing this server's OAuth row could overwrite the retry's PKCE
+ * verifier / state and break the callback.
+ */
+const DB_STEP_MS = 4_000
+const MCP_AUTH_STEP_MS = 10_000
 const MAX_SURFACED_ERROR_LENGTH = 250
 const DCR_UNSUPPORTED_MESSAGE =
   "This server doesn't support automatic OAuth client registration. Add a pre-registered OAuth client ID and secret, or configure a token instead."
@@ -81,18 +98,21 @@ export const GET = withRouteHandler(
       const parsed = await parseRequest(startMcpOauthContract, request, {})
       if (!parsed.success) return parsed.response
       const { serverId } = parsed.data.query
+      logger.info(`Starting MCP OAuth flow for server ${serverId}`)
 
-      const [server] = await db
-        .select()
-        .from(mcpServers)
-        .where(
-          and(
-            eq(mcpServers.id, serverId),
-            eq(mcpServers.workspaceId, workspaceId),
-            isNull(mcpServers.deletedAt)
+      const [server] = await timedStep('loadServer', DB_STEP_MS, () =>
+        db
+          .select()
+          .from(mcpServers)
+          .where(
+            and(
+              eq(mcpServers.id, serverId),
+              eq(mcpServers.workspaceId, workspaceId),
+              isNull(mcpServers.deletedAt)
+            )
           )
-        )
-        .limit(1)
+          .limit(1)
+      )
 
       if (!server) {
         return createMcpErrorResponse(new Error('Server not found'), 'Server not found', 404)
@@ -107,8 +127,9 @@ export const GET = withRouteHandler(
       if (!server.url) {
         return createMcpErrorResponse(new Error('Server has no URL'), 'Missing server URL', 400)
       }
+      const serverUrl = server.url
       try {
-        assertSafeOauthServerUrl(server.url)
+        assertSafeOauthServerUrl(serverUrl)
       } catch (e) {
         if (e instanceof McpOauthInsecureUrlError) {
           return createMcpErrorResponse(
@@ -120,11 +141,13 @@ export const GET = withRouteHandler(
         throw e
       }
 
-      const row = await getOrCreateOauthRow({
-        mcpServerId: server.id,
-        userId,
-        workspaceId,
-      })
+      const row = await timedStep('getOrCreateOauthRow', DB_STEP_MS, () =>
+        getOrCreateOauthRow({
+          mcpServerId: server.id,
+          userId,
+          workspaceId,
+        })
+      )
       const hasActiveFlow =
         !!row.state &&
         !!row.stateCreatedAt &&
@@ -137,17 +160,38 @@ export const GET = withRouteHandler(
         )
       }
       if (row.userId !== userId) {
-        await setOauthRowUser(row.id, userId)
+        await timedStep('setOauthRowUser', DB_STEP_MS, () => setOauthRowUser(row.id, userId))
         row.userId = userId
       }
-      const preregistered = await loadPreregisteredClient(server.id)
+      const preregistered = await timedStep('loadPreregisteredClient', DB_STEP_MS, () =>
+        loadPreregisteredClient(server.id)
+      )
       const provider = new SimMcpOauthProvider({ row, preregistered })
 
       try {
-        const result = await mcpAuthGuarded(provider, {
-          serverUrl: server.url,
+        // OAuth discovery + DCR through the guarded fetch, bounded so a transient stall fails
+        // fast with a labeled log instead of hanging the popup. `McpOauthRedirectRequired` is
+        // the SUCCESS signal (a throw carrying the authorize URL), so we catch it INSIDE the
+        // bounded step and return it as a normal value — otherwise timedStep would error-log
+        // every successful authorize. Only a real error or a timeout escapes as a throw.
+        const authOutcome = await timedStep('mcpAuthGuarded', MCP_AUTH_STEP_MS, async () => {
+          try {
+            return { kind: 'result' as const, value: await mcpAuthGuarded(provider, { serverUrl }) }
+          } catch (e) {
+            if (e instanceof McpOauthRedirectRequired) {
+              return { kind: 'redirect' as const, authorizationUrl: e.authorizationUrl }
+            }
+            throw e
+          }
         })
-        if (result === 'AUTHORIZED') {
+        if (authOutcome.kind === 'redirect') {
+          logger.info(`OAuth redirect for server ${serverId}`)
+          return NextResponse.json({
+            status: 'redirect',
+            authorizationUrl: authOutcome.authorizationUrl,
+          })
+        }
+        if (authOutcome.value === 'AUTHORIZED') {
           return NextResponse.json({ status: 'already_authorized' })
         }
         return createMcpErrorResponse(
@@ -156,19 +200,26 @@ export const GET = withRouteHandler(
           500
         )
       } catch (e) {
-        if (e instanceof McpOauthRedirectRequired) {
-          logger.info(`OAuth redirect for server ${serverId}`)
-          return NextResponse.json({
-            status: 'redirect',
-            authorizationUrl: e.authorizationUrl,
-          })
-        }
         if (isDynamicClientRegistrationUnsupported(e)) {
           return createMcpErrorResponse(toError(e), DCR_UNSUPPORTED_MESSAGE, 422)
         }
         throw e
       }
     } catch (error) {
+      // Any bounded step timing out (DB reads or the auth step) is a stall, not a bug —
+      // surface it as a fast 504 so the popup closes with a clear "try again" rather than a
+      // generic 500. A fresh retry is a clean flow: the callback correlates on the `state`
+      // nonce, so even if a lingering timed-out attempt later overwrites the row's state, the
+      // user's authorize URL (carrying the fresh nonce) simply fails `invalid_state` — a clean
+      // retry, never silent corruption.
+      if (error instanceof OauthStepTimeoutError) {
+        logger.warn('MCP OAuth start stalled')
+        return createMcpErrorResponse(
+          error,
+          'Authorization is taking too long — please try again.',
+          504
+        )
+      }
       logger.error('Error starting MCP OAuth flow:', error)
       // Only surface OAuth-flow errors verbatim; everything else (DB, decryption,
       // network) gets a generic message to avoid leaking internal details.

apps/sim/lib/mcp/oauth/index.ts

@@ -28,4 +28,5 @@ export {
   setOauthRowUser,
   withMcpOauthRefreshLock,
 } from './storage'
+export { makeTimedStep, OauthStepTimeoutError } from './timed-step'
 export { assertSafeOauthServerUrl, McpOauthInsecureUrlError } from './url-validation'

apps/sim/lib/mcp/oauth/timed-step.ts

@@ -0,0 +1,45 @@
+import type { Logger } from '@sim/logger'
+import { toError } from '@sim/utils/errors'
+
+/** Thrown when a `timedStep`-bounded operation doesn't settle within its budget. */
+export class OauthStepTimeoutError extends Error {
+  constructor(step: string, ms: number) {
+    super(`MCP OAuth step "${step}" did not settle within ${ms}ms`)
+    this.name = 'OauthStepTimeoutError'
+  }
+}
+
+/**
+ * Times and bounds one awaited step of an OAuth route so a stalled operation surfaces
+ * as a labeled, logged error instead of hanging the request (and the browser popup
+ * waiting on it) forever. The losing promise is not cancelled — a wedged DB/socket op
+ * can't be — so it settles in the background with its rejection swallowed; the point is
+ * that the request stops waiting on it and the logs name the exact step that stalled.
+ */
+export function makeTimedStep(logger: Logger) {
+  return async function timedStep<T>(step: string, ms: number, fn: () => Promise<T>): Promise<T> {
+    const start = Date.now()
+    logger.info(`OAuth step start: ${step}`)
+    const work = Promise.resolve(fn())
+    work.catch(() => {})
+    let timer: ReturnType<typeof setTimeout> | undefined
+    try {
+      const value = await Promise.race([
+        work,
+        new Promise<never>((_, reject) => {
+          timer = setTimeout(() => reject(new OauthStepTimeoutError(step, ms)), ms)
+          timer.unref?.()
+        }),
+      ])
+      logger.info(`OAuth step done: ${step} (${Date.now() - start}ms)`)
+      return value
+    } catch (error) {
+      logger.error(`OAuth step failed: ${step} (${Date.now() - start}ms)`, {
+        error: toError(error).message,
+      })
+      throw error
+    } finally {
+      clearTimeout(timer)
+    }
+  }
+}

packages/testing/src/mocks/index.ts

@@ -112,6 +112,7 @@ export {
   McpOauthRedirectRequiredMock,
   mcpOauthMock,
   mcpOauthMockFns,
+  OauthStepTimeoutErrorMock,
 } from './mcp-oauth.mock'
 // Permission mocks
 export { permissionsMock, permissionsMockFns } from './permissions.mock'

packages/testing/src/mocks/mcp-oauth.mock.ts

@@ -44,6 +44,13 @@ export class McpOauthInsecureUrlErrorMock extends Error {
   }
 }
 
+export class OauthStepTimeoutErrorMock extends Error {
+  constructor(step: string, ms: number) {
+    super(`MCP OAuth step "${step}" did not settle within ${ms}ms`)
+    this.name = 'OauthStepTimeoutErrorMock'
+  }
+}
+
 /**
  * Returns the provider config back as the constructed instance, matching the
  * original identity passthrough. Declared as a named function (not an arrow) so
@@ -82,5 +89,12 @@ export const mcpOauthMock = {
   withMcpOauthRefreshLock: mcpOauthMockFns.mockWithMcpOauthRefreshLock,
   McpOauthRedirectRequired: McpOauthRedirectRequiredMock,
   McpOauthInsecureUrlError: McpOauthInsecureUrlErrorMock,
+  OauthStepTimeoutError: OauthStepTimeoutErrorMock,
   SimMcpOauthProvider: vi.fn().mockImplementation(buildSimMcpOauthProvider),
+  // Pass-through: run the step immediately, no bounding, so route tests exercise real behavior.
+  // Wrap in Promise.resolve like the real helper so a mock returning a non-promise still chains.
+  makeTimedStep:
+    () =>
+    <T>(_step: string, _ms: number, fn: () => Promise<T>): Promise<T> =>
+      Promise.resolve(fn()),
 }