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

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

@@ -9,6 +9,7 @@ import {
  McpOauthRedirectRequiredMock,
  mcpOauthMock,
  mcpOauthMockFns,
+ OauthStepTimeoutErrorMock,
  permissionsMock,
  permissionsMockFns,
  resetDbChainMock,
@@ -86,6 +87,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

@@ -96,6 +96,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()),
 }