From a89b243b35ea7dfd8edf2669ace796533c6da892 Mon Sep 17 00:00:00 2001 From: Waleed Latif Date: Wed, 22 Jul 2026 18:12:40 -0700 Subject: [PATCH 1/5] fix(mcp): bound and retry OAuth start so a transient stall recovers instead of a blank popup MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- apps/sim/app/api/mcp/oauth/callback/route.ts | 42 +--------- .../sim/app/api/mcp/oauth/start/route.test.ts | 30 +++++++ apps/sim/app/api/mcp/oauth/start/route.ts | 80 ++++++++++++++----- apps/sim/lib/mcp/oauth/index.ts | 1 + apps/sim/lib/mcp/oauth/timed-step.ts | 45 +++++++++++ packages/testing/src/mocks/index.ts | 1 + packages/testing/src/mocks/mcp-oauth.mock.ts | 14 ++++ 7 files changed, 153 insertions(+), 60 deletions(-) create mode 100644 apps/sim/lib/mcp/oauth/timed-step.ts diff --git a/apps/sim/app/api/mcp/oauth/callback/route.ts b/apps/sim/app/api/mcp/oauth/callback/route.ts index 79b7058ddea..9d63191fb32 100644 --- a/apps/sim/app/api/mcp/oauth/callback/route.ts +++ b/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(step: string, ms: number, fn: () => Promise): Promise { - const start = Date.now() - logger.info(`OAuth callback step start: ${step}`) - const work = Promise.resolve(fn()) - work.catch(() => {}) - let timer: ReturnType | undefined - try { - const value = await Promise.race([ - work, - new Promise((_, 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, '&') diff --git a/apps/sim/app/api/mcp/oauth/start/route.test.ts b/apps/sim/app/api/mcp/oauth/start/route.test.ts index 6b0df17c66e..eb639f1c997 100644 --- a/apps/sim/app/api/mcp/oauth/start/route.test.ts +++ b/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,35 @@ describe('MCP OAuth start route', () => { ) }) + it('retries mcpAuthGuarded once on a transient stall and succeeds on the fresh attempt', async () => { + mcpOauthMockFns.mockMcpAuthGuarded + .mockRejectedValueOnce(new OauthStepTimeoutErrorMock('mcpAuthGuarded (attempt 1)', 12_000)) + .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(2) + expect(response.status).toBe(200) + expect(body).toEqual({ status: 'redirect', authorizationUrl: 'https://mcp.exa.ai/authorize' }) + }) + + it('does not retry a non-timeout error (e.g. redirect success on the first attempt)', 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' + ) + + await GET(request) + + expect(mcpOauthMockFns.mockMcpAuthGuarded).toHaveBeenCalledTimes(1) + }) + 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' diff --git a/apps/sim/app/api/mcp/oauth/start/route.ts b/apps/sim/app/api/mcp/oauth/start/route.ts index 5bc08af493f..46c5db0a666 100644 --- a/apps/sim/app/api/mcp/oauth/start/route.ts +++ b/apps/sim/app/api/mcp/oauth/start/route.ts @@ -3,6 +3,7 @@ import { db } from '@sim/db' import { mcpServers } from '@sim/db/schema' import { createLogger } from '@sim/logger' import { getErrorMessage, toError } from '@sim/utils/errors' +import { sleep } from '@sim/utils/helpers' import { and, eq, isNull } from 'drizzle-orm' import type { NextRequest } from 'next/server' import { NextResponse } from 'next/server' @@ -16,14 +17,26 @@ 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 +/** + * OAuth discovery + DCR occasionally hits the transient headers-then-stalled-body class we've + * documented for CDN-fronted MCP hosts — a per-connection stall a fresh attempt dodges. Bound + * each attempt tightly and retry once so an intermittent stall recovers automatically instead + * of hanging the browser popup to the client's timeout. Two 12s attempts stay under the + * client's 30s `/oauth/start` deadline. + */ +const MCP_AUTH_ATTEMPT_MS = 12_000 +const MCP_AUTH_MAX_ATTEMPTS = 2 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 +94,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', 15_000, () => + 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 +123,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 +137,13 @@ export const GET = withRouteHandler( throw e } - const row = await getOrCreateOauthRow({ - mcpServerId: server.id, - userId, - workspaceId, - }) + const row = await timedStep('getOrCreateOauthRow', 15_000, () => + getOrCreateOauthRow({ + mcpServerId: server.id, + userId, + workspaceId, + }) + ) const hasActiveFlow = !!row.state && !!row.stateCreatedAt && @@ -140,13 +159,34 @@ export const GET = withRouteHandler( await setOauthRowUser(row.id, userId) row.userId = userId } - const preregistered = await loadPreregisteredClient(server.id) + const preregistered = await timedStep('loadPreregisteredClient', 15_000, () => + 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. Each attempt is bounded (labeled in + // logs); a per-connection transient stall on the first attempt is retried on a fresh + // one. `McpOauthRedirectRequired` is the SUCCESS signal — rethrow it immediately so the + // outer catch returns the authorize URL; only a bounded timeout is retried. + let result: Awaited> | undefined + for (let attempt = 1; attempt <= MCP_AUTH_MAX_ATTEMPTS; attempt++) { + try { + result = await timedStep( + `mcpAuthGuarded (attempt ${attempt})`, + MCP_AUTH_ATTEMPT_MS, + () => mcpAuthGuarded(provider, { serverUrl }) + ) + break + } catch (attemptError) { + if (attemptError instanceof OauthStepTimeoutError && attempt < MCP_AUTH_MAX_ATTEMPTS) { + logger.warn(`MCP OAuth start stalled for server ${serverId}, retrying`) + await sleep(250) + continue + } + throw attemptError + } + } if (result === 'AUTHORIZED') { return NextResponse.json({ status: 'already_authorized' }) } diff --git a/apps/sim/lib/mcp/oauth/index.ts b/apps/sim/lib/mcp/oauth/index.ts index 5237b22fe16..7e89f72d7c3 100644 --- a/apps/sim/lib/mcp/oauth/index.ts +++ b/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' diff --git a/apps/sim/lib/mcp/oauth/timed-step.ts b/apps/sim/lib/mcp/oauth/timed-step.ts new file mode 100644 index 00000000000..4429969b798 --- /dev/null +++ b/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(step: string, ms: number, fn: () => Promise): Promise { + const start = Date.now() + logger.info(`OAuth step start: ${step}`) + const work = Promise.resolve(fn()) + work.catch(() => {}) + let timer: ReturnType | undefined + try { + const value = await Promise.race([ + work, + new Promise((_, 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) + } + } +} diff --git a/packages/testing/src/mocks/index.ts b/packages/testing/src/mocks/index.ts index e3f6c2d6f62..296c9796a5f 100644 --- a/packages/testing/src/mocks/index.ts +++ b/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' diff --git a/packages/testing/src/mocks/mcp-oauth.mock.ts b/packages/testing/src/mocks/mcp-oauth.mock.ts index 81dee8de9c4..e5318930a60 100644 --- a/packages/testing/src/mocks/mcp-oauth.mock.ts +++ b/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: + () => + (_step: string, _ms: number, fn: () => Promise): Promise => + Promise.resolve(fn()), } From cf5563ea6c15515548647ee3d00c163a462e390e Mon Sep 17 00:00:00 2001 From: Waleed Latif Date: Wed, 22 Jul 2026 18:21:45 -0700 Subject: [PATCH 2/5] fix(mcp): drop the unsafe OAuth-start retry; fail fast without error-logging success MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- .../sim/app/api/mcp/oauth/start/route.test.ts | 23 +++--- apps/sim/app/api/mcp/oauth/start/route.ts | 77 ++++++++++--------- 2 files changed, 53 insertions(+), 47 deletions(-) diff --git a/apps/sim/app/api/mcp/oauth/start/route.test.ts b/apps/sim/app/api/mcp/oauth/start/route.test.ts index eb639f1c997..78140325edf 100644 --- a/apps/sim/app/api/mcp/oauth/start/route.test.ts +++ b/apps/sim/app/api/mcp/oauth/start/route.test.ts @@ -87,23 +87,23 @@ describe('MCP OAuth start route', () => { ) }) - it('retries mcpAuthGuarded once on a transient stall and succeeds on the fresh attempt', async () => { - mcpOauthMockFns.mockMcpAuthGuarded - .mockRejectedValueOnce(new OauthStepTimeoutErrorMock('mcpAuthGuarded (attempt 1)', 12_000)) - .mockRejectedValueOnce(new McpOauthRedirectRequiredMock('https://mcp.exa.ai/authorize')) + 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) - const body = await response.json() - expect(mcpOauthMockFns.mockMcpAuthGuarded).toHaveBeenCalledTimes(2) - expect(response.status).toBe(200) - expect(body).toEqual({ status: 'redirect', authorizationUrl: 'https://mcp.exa.ai/authorize' }) + expect(mcpOauthMockFns.mockMcpAuthGuarded).toHaveBeenCalledTimes(1) + expect(response.status).toBe(504) }) - it('does not retry a non-timeout error (e.g. redirect success on the first attempt)', async () => { + it('returns the authorize URL without error-logging the success redirect throw', async () => { mcpOauthMockFns.mockMcpAuthGuarded.mockRejectedValueOnce( new McpOauthRedirectRequiredMock('https://mcp.exa.ai/authorize') ) @@ -111,9 +111,12 @@ describe('MCP OAuth start route', () => { 'http://localhost:3000/api/mcp/oauth/start?workspaceId=workspace-1&serverId=server-1' ) - await GET(request) + 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 () => { diff --git a/apps/sim/app/api/mcp/oauth/start/route.ts b/apps/sim/app/api/mcp/oauth/start/route.ts index 46c5db0a666..ec60106355c 100644 --- a/apps/sim/app/api/mcp/oauth/start/route.ts +++ b/apps/sim/app/api/mcp/oauth/start/route.ts @@ -3,7 +3,6 @@ import { db } from '@sim/db' import { mcpServers } from '@sim/db/schema' import { createLogger } from '@sim/logger' import { getErrorMessage, toError } from '@sim/utils/errors' -import { sleep } from '@sim/utils/helpers' import { and, eq, isNull } from 'drizzle-orm' import type { NextRequest } from 'next/server' import { NextResponse } from 'next/server' @@ -29,14 +28,17 @@ const logger = createLogger('McpOauthStartAPI') const timedStep = makeTimedStep(logger) const OAUTH_START_TTL_MS = 10 * 60 * 1000 /** - * OAuth discovery + DCR occasionally hits the transient headers-then-stalled-body class we've - * documented for CDN-fronted MCP hosts — a per-connection stall a fresh attempt dodges. Bound - * each attempt tightly and retry once so an intermittent stall recovers automatically instead - * of hanging the browser popup to the client's timeout. Two 12s attempts stay under the - * client's 30s `/oauth/start` deadline. + * Per-step budgets, kept small so the whole request stays under the client's 30s `/oauth/start` + * deadline even in the worst case (5+5+5 DB + 12 auth = 27s). 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 MCP_AUTH_ATTEMPT_MS = 12_000 -const MCP_AUTH_MAX_ATTEMPTS = 2 +const DB_STEP_MS = 5_000 +const MCP_AUTH_STEP_MS = 12_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." @@ -96,7 +98,7 @@ export const GET = withRouteHandler( const { serverId } = parsed.data.query logger.info(`Starting MCP OAuth flow for server ${serverId}`) - const [server] = await timedStep('loadServer', 15_000, () => + const [server] = await timedStep('loadServer', DB_STEP_MS, () => db .select() .from(mcpServers) @@ -137,7 +139,7 @@ export const GET = withRouteHandler( throw e } - const row = await timedStep('getOrCreateOauthRow', 15_000, () => + const row = await timedStep('getOrCreateOauthRow', DB_STEP_MS, () => getOrCreateOauthRow({ mcpServerId: server.id, userId, @@ -159,35 +161,35 @@ export const GET = withRouteHandler( await setOauthRowUser(row.id, userId) row.userId = userId } - const preregistered = await timedStep('loadPreregisteredClient', 15_000, () => + const preregistered = await timedStep('loadPreregisteredClient', DB_STEP_MS, () => loadPreregisteredClient(server.id) ) const provider = new SimMcpOauthProvider({ row, preregistered }) try { - // OAuth discovery + DCR through the guarded fetch. Each attempt is bounded (labeled in - // logs); a per-connection transient stall on the first attempt is retried on a fresh - // one. `McpOauthRedirectRequired` is the SUCCESS signal — rethrow it immediately so the - // outer catch returns the authorize URL; only a bounded timeout is retried. - let result: Awaited> | undefined - for (let attempt = 1; attempt <= MCP_AUTH_MAX_ATTEMPTS; attempt++) { + // 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 { - result = await timedStep( - `mcpAuthGuarded (attempt ${attempt})`, - MCP_AUTH_ATTEMPT_MS, - () => mcpAuthGuarded(provider, { serverUrl }) - ) - break - } catch (attemptError) { - if (attemptError instanceof OauthStepTimeoutError && attempt < MCP_AUTH_MAX_ATTEMPTS) { - logger.warn(`MCP OAuth start stalled for server ${serverId}, retrying`) - await sleep(250) - continue + 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 attemptError + throw e } + }) + if (authOutcome.kind === 'redirect') { + logger.info(`OAuth redirect for server ${serverId}`) + return NextResponse.json({ + status: 'redirect', + authorizationUrl: authOutcome.authorizationUrl, + }) } - if (result === 'AUTHORIZED') { + if (authOutcome.value === 'AUTHORIZED') { return NextResponse.json({ status: 'already_authorized' }) } return createMcpErrorResponse( @@ -196,16 +198,17 @@ 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) } + if (e instanceof OauthStepTimeoutError) { + logger.warn(`MCP OAuth start stalled for server ${serverId}`) + return createMcpErrorResponse( + e, + 'Authorization is taking too long — please try again.', + 504 + ) + } throw e } } catch (error) { From d013b032773e6af3bf2d6001905e9b4cb71a6623 Mon Sep 17 00:00:00 2001 From: Waleed Latif Date: Wed, 22 Jul 2026 18:30:10 -0700 Subject: [PATCH 3/5] fix(mcp): route all bounded-step timeouts to the 504 handler MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- .../sim/app/api/mcp/oauth/start/route.test.ts | 16 ++++++++++++++ apps/sim/app/api/mcp/oauth/start/route.ts | 22 ++++++++++++------- 2 files changed, 30 insertions(+), 8 deletions(-) diff --git a/apps/sim/app/api/mcp/oauth/start/route.test.ts b/apps/sim/app/api/mcp/oauth/start/route.test.ts index 78140325edf..6806f1ffe91 100644 --- a/apps/sim/app/api/mcp/oauth/start/route.test.ts +++ b/apps/sim/app/api/mcp/oauth/start/route.test.ts @@ -103,6 +103,22 @@ describe('MCP OAuth start route', () => { 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') diff --git a/apps/sim/app/api/mcp/oauth/start/route.ts b/apps/sim/app/api/mcp/oauth/start/route.ts index ec60106355c..cb1c1e90039 100644 --- a/apps/sim/app/api/mcp/oauth/start/route.ts +++ b/apps/sim/app/api/mcp/oauth/start/route.ts @@ -201,17 +201,23 @@ export const GET = withRouteHandler( if (isDynamicClientRegistrationUnsupported(e)) { return createMcpErrorResponse(toError(e), DCR_UNSUPPORTED_MESSAGE, 422) } - if (e instanceof OauthStepTimeoutError) { - logger.warn(`MCP OAuth start stalled for server ${serverId}`) - return createMcpErrorResponse( - e, - 'Authorization is taking too long — please try again.', - 504 - ) - } 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. From 43dcd07382df1335bfb17bc9522acbc89e472434 Mon Sep 17 00:00:00 2001 From: Waleed Latif Date: Wed, 22 Jul 2026 18:37:21 -0700 Subject: [PATCH 4/5] 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. --- apps/sim/app/api/mcp/oauth/start/route.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/apps/sim/app/api/mcp/oauth/start/route.ts b/apps/sim/app/api/mcp/oauth/start/route.ts index cb1c1e90039..dffa469b8c6 100644 --- a/apps/sim/app/api/mcp/oauth/start/route.ts +++ b/apps/sim/app/api/mcp/oauth/start/route.ts @@ -158,7 +158,7 @@ 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 timedStep('loadPreregisteredClient', DB_STEP_MS, () => From d793545d463ac48e548e25432b5990181a0417a6 Mon Sep 17 00:00:00 2001 From: Waleed Latif Date: Wed, 22 Jul 2026 18:43:43 -0700 Subject: [PATCH 5/5] 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. --- apps/sim/app/api/mcp/oauth/start/route.ts | 20 +++++++++++--------- 1 file changed, 11 insertions(+), 9 deletions(-) diff --git a/apps/sim/app/api/mcp/oauth/start/route.ts b/apps/sim/app/api/mcp/oauth/start/route.ts index dffa469b8c6..3a936167650 100644 --- a/apps/sim/app/api/mcp/oauth/start/route.ts +++ b/apps/sim/app/api/mcp/oauth/start/route.ts @@ -29,16 +29,18 @@ 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 (5+5+5 DB + 12 auth = 27s). 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. + * 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 = 5_000 -const MCP_AUTH_STEP_MS = 12_000 +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."