Skip to content

Commit f620a1b

Browse files
waleedlatif1claude
andcommitted
fix(mcp): tighten OAuth refresh race and session-error detection
Re-load the OAuth row inside withMcpOauthRefreshLock so concurrent callers observe predecessor-written tokens instead of a stale snapshot loaded before lock acquisition. Without this, the second caller's provider held a rotated-out refresh token and the SDK tripped invalid_grant, forcing reauthorization. Switch isSessionError to match the SDK's typed StreamableHTTPError (code 404/400) instead of substring-checking arbitrary error messages, removing false positives on URLs that happen to contain those digits. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
1 parent c9b183f commit f620a1b

2 files changed

Lines changed: 41 additions & 26 deletions

File tree

apps/sim/app/workspace/[workspaceId]/settings/components/mcp/mcp.tsx

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -491,7 +491,7 @@ export function MCP({ initialServerId }: MCPProps) {
491491
{hasParams && (
492492
<ChevronDown
493493
className={cn(
494-
'mt-0.5 h-[14px] w-[14px] flex-shrink-0 text-[var(--text-muted)] transition-transform duration-200',
494+
'mt-0.5 size-[14px] flex-shrink-0 text-[var(--text-muted)] transition-transform duration-200',
495495
isExpanded && 'rotate-180'
496496
)}
497497
/>

apps/sim/lib/mcp/service.ts

Lines changed: 40 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -3,10 +3,11 @@
33
*/
44

55
import { UnauthorizedError } from '@modelcontextprotocol/sdk/client/auth.js'
6+
import { StreamableHTTPError } from '@modelcontextprotocol/sdk/client/streamableHttp.js'
67
import { db } from '@sim/db'
78
import { mcpServers } from '@sim/db/schema'
89
import { createLogger } from '@sim/logger'
9-
import { getErrorMessage, toError } from '@sim/utils/errors'
10+
import { getErrorMessage } from '@sim/utils/errors'
1011
import { sleep } from '@sim/utils/helpers'
1112
import { and, eq, isNull } from 'drizzle-orm'
1213
import { isTest } from '@/lib/core/config/feature-flags'
@@ -31,7 +32,6 @@ import {
3132
type McpCacheStorageAdapter,
3233
} from '@/lib/mcp/storage'
3334
import {
34-
type McpClientOptions,
3535
McpOauthAuthorizationRequiredError,
3636
type McpServerConfig,
3737
type McpServerStatusConfig,
@@ -181,12 +181,34 @@ class McpService {
181181
allowedOrigins: config.url ? [new URL(config.url).origin] : undefined,
182182
}
183183

184-
let authProvider: McpClientOptions['authProvider']
185-
let rowId: string | undefined
186-
if (config.authType === 'oauth') {
187-
if (!userId || !config.workspaceId) {
188-
throw new Error('OAuth MCP server requires both userId and workspaceId')
189-
}
184+
if (config.authType !== 'oauth') {
185+
const client = new McpClient({
186+
config,
187+
securityPolicy,
188+
resolvedIP: resolvedIP ?? undefined,
189+
})
190+
await client.connect()
191+
return client
192+
}
193+
194+
if (!userId || !config.workspaceId) {
195+
throw new Error('OAuth MCP server requires both userId and workspaceId')
196+
}
197+
198+
const initialRow = await getOrCreateOauthRow({
199+
mcpServerId: config.id,
200+
userId,
201+
workspaceId: config.workspaceId,
202+
})
203+
if (!initialRow.tokens) {
204+
throw new McpOauthAuthorizationRequiredError(config.id, config.name)
205+
}
206+
207+
// Re-read the row inside the lock so concurrent callers observe tokens
208+
// written by a predecessor refresh, rather than the stale snapshot loaded
209+
// before lock acquisition. Without this, the second caller's provider holds
210+
// a rotated-out refresh token and the SDK trips `invalid_grant`.
211+
return withMcpOauthRefreshLock(initialRow.id, async () => {
190212
const row = await getOrCreateOauthRow({
191213
mcpServerId: config.id,
192214
userId,
@@ -195,12 +217,8 @@ class McpService {
195217
if (!row.tokens) {
196218
throw new McpOauthAuthorizationRequiredError(config.id, config.name)
197219
}
198-
rowId = row.id
199220
const preregistered = await loadPreregisteredClient(config.id)
200-
authProvider = new SimMcpOauthProvider({ row, preregistered })
201-
}
202-
203-
const connect = async () => {
221+
const authProvider = new SimMcpOauthProvider({ row, preregistered })
204222
const client = new McpClient({
205223
config,
206224
securityPolicy,
@@ -209,9 +227,7 @@ class McpService {
209227
})
210228
await client.connect()
211229
return client
212-
}
213-
214-
return rowId ? withMcpOauthRefreshLock(rowId, connect) : connect()
230+
})
215231
}
216232

217233
/**
@@ -273,17 +289,16 @@ class McpService {
273289
}
274290

275291
/**
276-
* Check if an error indicates a session-related issue that might be resolved by retry
292+
* Detects an expired or unknown `Mcp-Session-Id` so the caller can retry.
293+
* Per MCP spec, the server returns HTTP 404 for an unknown session id and
294+
* may return 400 when the session header is malformed; the SDK surfaces
295+
* both as `StreamableHTTPError` with a typed numeric `code` field.
277296
*/
278297
private isSessionError(error: unknown): boolean {
279-
const message = toError(error).message
280-
const lowerMessage = message.toLowerCase()
281-
return (
282-
lowerMessage.includes('session') ||
283-
lowerMessage.includes('400') ||
284-
lowerMessage.includes('404') ||
285-
lowerMessage.includes('no valid session')
286-
)
298+
if (error instanceof StreamableHTTPError) {
299+
return error.code === 404 || error.code === 400
300+
}
301+
return false
287302
}
288303

289304
/**

0 commit comments

Comments
 (0)