mirror of
https://github.com/simstudioai/sim.git
synced 2026-09-24 15:45:35 +08:00
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.
This commit is contained in:
@@ -112,6 +112,7 @@ export {
|
||||
McpOauthRedirectRequiredMock,
|
||||
mcpOauthMock,
|
||||
mcpOauthMockFns,
|
||||
OauthStepTimeoutErrorMock,
|
||||
} from './mcp-oauth.mock'
|
||||
// Permission mocks
|
||||
export { permissionsMock, permissionsMockFns } from './permissions.mock'
|
||||
|
||||
@@ -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()),
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user