Skip to content

Commit d7947e8

Browse files
authored
fix(mcp): coordinate OAuth refresh without blocking tools (#7479)
1 parent c8ea980 commit d7947e8

13 files changed

Lines changed: 748 additions & 82 deletions

apps/sim/lib/credentials/application/discover-managed-mcp-tools.test.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -121,7 +121,7 @@ describe('discoverManagedMcpToolsUseCase', () => {
121121
expect(mocks.discoverTools).toHaveBeenCalledWith(
122122
context.mcpServerId,
123123
context.workspaceId,
124-
{},
124+
{ credentialId: context.credentialId, loadProvider: expect.any(Function) },
125125
signal,
126126
{ requireComplete: true }
127127
)

apps/sim/lib/credentials/application/discover-managed-mcp-tools.ts

Lines changed: 9 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -10,7 +10,6 @@ import {
1010
saveManagedMcpToolSnapshot,
1111
} from '@/lib/credentials/managed-mcp'
1212
import { loadManagedMcpAuthProvider } from '@/lib/mcp/application/managed-auth-provider'
13-
import { withMcpOauthRefreshLock } from '@/lib/mcp/oauth'
1413
import { mcpService } from '@/lib/mcp/service'
1514

1615
export interface DiscoverManagedMcpToolsInput {
@@ -35,14 +34,15 @@ export const discoverManagedMcpToolsUseCase = defineAuthorizedWorkspaceUseCase({
3534
async execute({ input, context }) {
3635
input.signal?.throwIfAborted()
3736
const runtime = await loadManagedMcpRuntimeCredential(context.credentialId, context.workspaceId)
38-
const tools = await withMcpOauthRefreshLock(runtime.credentialId, async () =>
39-
mcpService.discoverManagedMcpTools(
40-
runtime.mcpServerId,
41-
runtime.workspaceId,
42-
await loadManagedMcpAuthProvider(runtime.credentialId, runtime.workspaceId),
43-
input.signal,
44-
{ requireComplete: true }
45-
)
37+
const tools = await mcpService.discoverManagedMcpTools(
38+
runtime.mcpServerId,
39+
runtime.workspaceId,
40+
{
41+
credentialId: runtime.credentialId,
42+
loadProvider: () => loadManagedMcpAuthProvider(runtime.credentialId, runtime.workspaceId),
43+
},
44+
input.signal,
45+
{ requireComplete: true }
4646
)
4747
await saveManagedMcpToolSnapshot(
4848
runtime.credentialId,

apps/sim/lib/mcp/application/execute-managed-tool.test.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -186,7 +186,7 @@ describe('executeManagedMcpToolUseCase', () => {
186186
expect(mocks.discoverTools).toHaveBeenCalledWith(
187187
context.mcpServerId,
188188
context.workspaceId,
189-
{},
189+
{ credentialId: context.credentialId, loadProvider: expect.any(Function) },
190190
signal,
191191
{ requireComplete: true }
192192
)

apps/sim/lib/mcp/application/execute-managed-tool.ts

Lines changed: 9 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -17,7 +17,6 @@ import {
1717
validateToolArguments,
1818
} from '@/lib/mcp/application/execute-tool'
1919
import { loadManagedMcpAuthProvider } from '@/lib/mcp/application/managed-auth-provider'
20-
import { withMcpOauthRefreshLock } from '@/lib/mcp/oauth'
2120
import { mcpService } from '@/lib/mcp/service'
2221
import type { McpTool, McpToolCall, McpToolSchema } from '@/lib/mcp/types'
2322

@@ -55,14 +54,15 @@ export const executeManagedMcpToolUseCase = defineAuthorizedWorkspaceUseCase({
5554
async execute({ input, context }): Promise<ExecuteMcpToolResult> {
5655
input.signal?.throwIfAborted()
5756
const runtime = await loadManagedMcpRuntimeCredential(context.credentialId, context.workspaceId)
58-
const tools = await withMcpOauthRefreshLock(runtime.credentialId, async () =>
59-
mcpService.discoverManagedMcpTools(
60-
runtime.mcpServerId,
61-
runtime.workspaceId,
62-
await loadManagedMcpAuthProvider(runtime.credentialId, runtime.workspaceId),
63-
input.signal,
64-
{ requireComplete: true }
65-
)
57+
const tools = await mcpService.discoverManagedMcpTools(
58+
runtime.mcpServerId,
59+
runtime.workspaceId,
60+
{
61+
credentialId: runtime.credentialId,
62+
loadProvider: () => loadManagedMcpAuthProvider(runtime.credentialId, runtime.workspaceId),
63+
},
64+
input.signal,
65+
{ requireComplete: true }
6666
)
6767
await saveManagedMcpToolSnapshot(
6868
runtime.credentialId,

apps/sim/lib/mcp/client.test.ts

Lines changed: 14 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -71,8 +71,12 @@ vi.mock('@/lib/core/execution-limits', () => ({
7171

7272
import { StreamableHTTPClientTransport } from '@modelcontextprotocol/sdk/client/streamableHttp.js'
7373
import { getMaxExecutionTimeout } from '@/lib/core/execution-limits'
74-
import { McpClient } from './client'
75-
import type { McpClientOptions, McpServerConfig } from './types'
74+
import { McpClient } from '@/lib/mcp/client'
75+
import {
76+
type McpClientOptions,
77+
McpOauthAuthorizationRequiredError,
78+
type McpServerConfig,
79+
} from '@/lib/mcp/types'
7680

7781
function createConfig(): McpServerConfig {
7882
return {
@@ -94,6 +98,14 @@ describe('McpClient notification handler', () => {
9498
vi.mocked(getMaxExecutionTimeout).mockReturnValue(30_000)
9599
})
96100

101+
it('preserves authorization-required errors raised by a locked credential reload', async () => {
102+
const error = new McpOauthAuthorizationRequiredError('server-1', 'Test Server')
103+
mockSdkConnect.mockRejectedValueOnce(error)
104+
const client = new McpClient({ config: createConfig() })
105+
await expect(client.connect()).rejects.toBe(error)
106+
expect(client.getStatus().lastError).toBeUndefined()
107+
})
108+
97109
it('fires onToolsChanged when a notification arrives while connected', async () => {
98110
const onToolsChanged = vi.fn()
99111

apps/sim/lib/mcp/client.ts

Lines changed: 24 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -13,6 +13,7 @@ import { getErrorMessage } from '@sim/utils/errors'
1313
import { getMaxExecutionTimeout } from '@/lib/core/execution-limits'
1414
import { getMcpSafeErrorDiagnostics } from '@/lib/mcp/error-diagnostics'
1515
import { McpOauthRedirectRequired } from '@/lib/mcp/oauth'
16+
import { createCoordinatedMcpOauthFetch } from '@/lib/mcp/oauth/coordinated-fetch'
1617
import { createGuardedMcpFetch, createPinnedPrivateMcpFetch } from '@/lib/mcp/pinned-fetch'
1718
import {
1819
type McpClientOptions,
@@ -21,6 +22,7 @@ import {
2122
type McpConsentRequest,
2223
type McpConsentResponse,
2324
McpError,
25+
McpOauthAuthorizationRequiredError,
2426
type McpSecurityPolicy,
2527
type McpServerConfig,
2628
type McpTool,
@@ -47,7 +49,10 @@ function classifyConnectionOutcome(
4749
error: unknown,
4850
authType: McpServerConfig['authType']
4951
): ConnectionOutcome {
50-
if (error instanceof McpOauthRedirectRequired) {
52+
if (
53+
error instanceof McpOauthRedirectRequired ||
54+
error instanceof McpOauthAuthorizationRequiredError
55+
) {
5156
return 'authorization_required'
5257
}
5358
if (error instanceof UnauthorizedError) {
@@ -100,8 +105,15 @@ export class McpClient {
100105
throw new McpError('URL required for Streamable HTTP transport')
101106
}
102107

103-
if (this.config.authType === 'oauth' && this.authProvider == null) {
104-
throw new McpError('OAuth MCP server requires an authProvider')
108+
if (
109+
this.config.authType === 'oauth' &&
110+
this.authProvider == null &&
111+
!options.oauthCredentials
112+
) {
113+
throw new McpError('OAuth MCP server requires OAuth credentials')
114+
}
115+
if (options.oauthCredentials && this.authProvider) {
116+
throw new McpError('OAuth MCP server must use one authentication strategy')
105117
}
106118
const useOauth = this.config.authType === 'oauth'
107119
// `resolvedIP` is null only when the hostname still carries an unresolved env-var
@@ -115,10 +127,18 @@ export class McpClient {
115127
: createGuardedMcpFetch(this.config.url)
116128
: undefined
117129
this.closeGuardedTransport = guarded?.close
130+
const transportFetch =
131+
useOauth && options.oauthCredentials
132+
? createCoordinatedMcpOauthFetch(options.oauthCredentials, {
133+
serverUrl: this.config.url,
134+
fetch: guarded?.fetch ?? fetch,
135+
requestInit: { headers: this.config.headers },
136+
})
137+
: guarded?.fetch
118138
this.transport = new StreamableHTTPClientTransport(new URL(this.config.url), {
119139
authProvider: useOauth ? this.authProvider : undefined,
120140
requestInit: { headers: this.config.headers },
121-
...(guarded ? { fetch: guarded.fetch } : {}),
141+
...(transportFetch ? { fetch: transportFetch } : {}),
122142
})
123143

124144
this.client = new Client(

apps/sim/lib/mcp/connection-manager.test.ts

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -63,6 +63,7 @@ vi.mock('@/lib/mcp/oauth', () => ({
6363
}))
6464

6565
import { McpConnectionManager } from '@/lib/mcp/connection-manager'
66+
import type { McpClientOptions } from '@/lib/mcp/types'
6667

6768
beforeAll(() => {
6869
setEnvFlags({ isTest: false })
@@ -168,6 +169,11 @@ describe('McpConnectionManager', () => {
168169
userId: 'user-1',
169170
workspaceId: 'ws-1',
170171
})
172+
const options: McpClientOptions = MockMcpClientConstructor.mock.calls[0][0]
173+
expect(options.authProvider).toBeUndefined()
174+
expect(options.oauthCredentials?.credentialId).toBe('server-oauth')
175+
await options.oauthCredentials?.loadProvider()
176+
expect(mockGetOrCreateOauthRow).toHaveBeenCalledTimes(2)
171177
})
172178

173179
it('allows a new connect() after a previous one completes', async () => {

apps/sim/lib/mcp/connection-manager.ts

Lines changed: 28 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -16,12 +16,13 @@ import { isTest } from '@/lib/core/config/env-flags'
1616
import { McpClient } from '@/lib/mcp/client'
1717
import { getOrCreateOauthRow, loadPreregisteredClient, SimMcpOauthProvider } from '@/lib/mcp/oauth'
1818
import { mcpPubSub } from '@/lib/mcp/pubsub'
19-
import type {
20-
ManagedConnectionState,
21-
McpClientOptions,
22-
McpServerConfig,
23-
McpToolsChangedCallback,
24-
ToolsChangedEvent,
19+
import {
20+
type ManagedConnectionState,
21+
type McpClientOptions,
22+
McpOauthAuthorizationRequiredError,
23+
type McpServerConfig,
24+
type McpToolsChangedCallback,
25+
type ToolsChangedEvent,
2526
} from '@/lib/mcp/types'
2627

2728
const logger = createLogger('McpConnectionManager')
@@ -137,7 +138,7 @@ export class McpConnectionManager {
137138
this.handleToolsChanged(key)
138139
}
139140

140-
let authProvider: McpClientOptions['authProvider']
141+
let oauthCredentials: McpClientOptions['oauthCredentials']
141142
if (config.authType === 'oauth') {
142143
const row = await getOrCreateOauthRow({
143144
mcpServerId: config.id,
@@ -150,8 +151,25 @@ export class McpConnectionManager {
150151
)
151152
return { supportsListChanged: false }
152153
}
153-
const preregistered = await loadPreregisteredClient(config.id)
154-
authProvider = new SimMcpOauthProvider({ row, preregistered })
154+
oauthCredentials = {
155+
credentialId: config.id,
156+
initialProvider: new SimMcpOauthProvider({
157+
row,
158+
preregistered: await loadPreregisteredClient(config.id),
159+
}),
160+
loadProvider: async () => {
161+
const current = await getOrCreateOauthRow({
162+
mcpServerId: config.id,
163+
userId,
164+
workspaceId,
165+
})
166+
if (!current.tokens) {
167+
throw new McpOauthAuthorizationRequiredError(config.id, config.name)
168+
}
169+
const preregistered = await loadPreregisteredClient(config.id)
170+
return new SimMcpOauthProvider({ row: current, preregistered })
171+
},
172+
}
155173
}
156174

157175
const client = new McpClient({
@@ -163,7 +181,7 @@ export class McpConnectionManager {
163181
},
164182
onToolsChanged,
165183
resolvedIP: resolvedIP ?? undefined,
166-
authProvider,
184+
oauthCredentials,
167185
})
168186

169187
try {

0 commit comments

Comments
 (0)