Skip to content

Commit 99e235a

Browse files
committed
refactor(mcp): drop the ordered-publication consistency layer for the minimal fix set
A four-angle LOC review (correctness, overengineering, dead-code, intent) found the fencing-token / discoveryRevision / conditional-publication apparatus and its 5 bounded-retry loops (~700 lines) to be correct but disproportionate to a low-contention, self-healing per-workspace-per-server discovery path — and it introduced two regressions: failing closed on a transient Redis blip (blocking DB status writes) and clearCache writing the DB for every server in the workspace. Revert service.ts, the storage adapters, the refresh route, and types to the production-tested base (which keeps the simple lastConnected<=discoveryStartedAt ordering guard and plain cache set/delete), and keep only the genuinely-good, decoupled fixes: authType-aware UnauthorizedError classification, OAuth-pending UX labels, DCR->422, credential-safe error diagnostics, and HTTP/2 negotiation.
1 parent b9dfe48 commit 99e235a

11 files changed

Lines changed: 334 additions & 2514 deletions

File tree

apps/sim/app/api/mcp/servers/[id]/refresh/route.test.ts

Lines changed: 32 additions & 309 deletions
Large diffs are not rendered by default.

apps/sim/app/api/mcp/servers/[id]/refresh/route.ts

Lines changed: 38 additions & 78 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,3 @@
1-
import { UnauthorizedError } from '@modelcontextprotocol/sdk/client/auth.js'
21
import { db } from '@sim/db'
32
import { mcpServers, workflow, workflowBlocks } from '@sim/db/schema'
43
import { createLogger } from '@sim/logger'
@@ -10,12 +9,8 @@ import { mcpServerIdParamsSchema } from '@/lib/api/contracts/mcp'
109
import { validationErrorResponse } from '@/lib/api/server'
1110
import { withRouteHandler } from '@/lib/core/utils/with-route-handler'
1211
import { withMcpAuth } from '@/lib/mcp/middleware'
13-
import { type McpServerDiscoveryState, mcpService } from '@/lib/mcp/service'
14-
import {
15-
McpOauthAuthorizationRequiredError,
16-
type McpTool,
17-
type McpToolSchema,
18-
} from '@/lib/mcp/types'
12+
import { mcpService } from '@/lib/mcp/service'
13+
import type { McpTool, McpToolSchema } from '@/lib/mcp/types'
1914
import {
2015
categorizeError,
2116
createMcpErrorResponse,
@@ -193,71 +188,34 @@ export const POST = withRouteHandler(
193188

194189
let syncResult: SyncResult = { updatedCount: 0, updatedWorkflowIds: [] }
195190
let discoveredTools: McpTool[] = []
196-
let discoveryState: McpServerDiscoveryState | null = null
197-
let discoveryPublicationOrder: Date | null = null
198191
let discoveryError: string | null = null
199-
let oauthAuthorizationRequired = false
192+
const discoveryStartedAt = new Date()
200193

201194
try {
202-
const discovery = await mcpService.discoverServerToolsWithMetadata(
195+
discoveredTools = await mcpService.discoverServerTools(
203196
userId,
204197
serverId,
205198
workspaceId,
206199
true
207200
)
208-
discoveredTools = discovery.tools
209-
discoveryState = discovery.state
210-
discoveryPublicationOrder = discovery.publicationOrder ?? null
211201
logger.info(
212202
`[${requestId}] Discovered ${discoveredTools.length} tools from server ${serverId}`
213203
)
214204
} catch (error) {
215-
oauthAuthorizationRequired =
216-
server.authType === 'oauth' &&
217-
(error instanceof McpOauthAuthorizationRequiredError ||
218-
error instanceof UnauthorizedError)
219205
discoveryError = truncate(categorizeError(error).message, 200, '')
220206
logger.warn(`[${requestId}] Failed to connect to server ${serverId}`, {
221207
error: discoveryError,
222208
})
223209
}
224210

225-
const [refreshedServer] = await db
226-
.select({
227-
name: mcpServers.name,
228-
url: mcpServers.url,
229-
connectionStatus: mcpServers.connectionStatus,
230-
lastConnected: mcpServers.lastConnected,
231-
lastToolsRefresh: mcpServers.lastToolsRefresh,
232-
lastError: mcpServers.lastError,
233-
toolCount: mcpServers.toolCount,
234-
})
235-
.from(mcpServers)
236-
.where(
237-
and(
238-
eq(mcpServers.id, serverId),
239-
eq(mcpServers.workspaceId, workspaceId),
240-
isNull(mcpServers.deletedAt)
241-
)
242-
)
243-
.limit(1)
244-
245-
const publicationBaseline = discoveryPublicationOrder ?? server.lastToolsRefresh
246-
const newerPublicationWonRace =
247-
refreshedServer?.lastToolsRefresh != null &&
248-
(publicationBaseline == null || refreshedServer.lastToolsRefresh > publicationBaseline)
249-
250-
if (discoveryError === null && discoveryState === 'published' && !newerPublicationWonRace) {
211+
if (discoveryError === null) {
251212
try {
252213
syncResult = await syncToolSchemasToWorkflows(
253214
workspaceId,
254215
serverId,
255216
discoveredTools,
256217
requestId,
257-
{
258-
url: refreshedServer?.url ?? undefined,
259-
name: refreshedServer?.name ?? undefined,
260-
}
218+
{ url: server.url ?? undefined, name: server.name ?? undefined }
261219
)
262220
} catch (error) {
263221
// Discovery already persisted status and cached tools; a workflow-sync
@@ -269,43 +227,45 @@ export const POST = withRouteHandler(
269227
}
270228
}
271229

230+
const now = new Date()
231+
232+
const [refreshedServer] = await db
233+
.update(mcpServers)
234+
.set({
235+
lastToolsRefresh: now,
236+
updatedAt: now,
237+
})
238+
.where(
239+
and(
240+
eq(mcpServers.id, serverId),
241+
eq(mcpServers.workspaceId, workspaceId),
242+
isNull(mcpServers.deletedAt)
243+
)
244+
)
245+
.returning({
246+
connectionStatus: mcpServers.connectionStatus,
247+
lastConnected: mcpServers.lastConnected,
248+
lastError: mcpServers.lastError,
249+
toolCount: mcpServers.toolCount,
250+
})
251+
272252
let connectionStatus = refreshedServer?.connectionStatus ?? 'error'
273253
let lastError = refreshedServer ? refreshedServer.lastError : discoveryError
274-
let toolCount = refreshedServer?.toolCount ?? discoveredTools.length
275-
const newerSuccessWonRace =
276-
connectionStatus === 'connected' &&
277-
newerPublicationWonRace &&
278-
refreshedServer.lastConnected != null &&
279-
(server.lastConnected == null || refreshedServer.lastConnected > server.lastConnected)
280-
281-
if (discoveryState === 'superseded' && !newerSuccessWonRace) {
282-
connectionStatus = 'disconnected'
283-
lastError = 'Tool discovery was superseded by a newer refresh. Please retry.'
284-
toolCount = 0
285-
} else if (
286-
discoveryError === null &&
287-
!newerPublicationWonRace &&
288-
(discoveryState === 'unavailable' || discoveryState === 'winner-cache')
289-
) {
290-
connectionStatus = 'connected'
291-
lastError = null
292-
toolCount = discoveredTools.length
293-
}
254+
const toolCount = refreshedServer?.toolCount ?? discoveredTools.length
294255

295256
if (discoveryError !== null && connectionStatus === 'connected') {
257+
const newerSuccessWonRace =
258+
refreshedServer?.lastConnected != null &&
259+
refreshedServer.lastConnected > discoveryStartedAt
260+
296261
if (!newerSuccessWonRace) {
297262
connectionStatus = 'disconnected'
298-
lastError = oauthAuthorizationRequired ? null : discoveryError
299-
toolCount = 0
263+
lastError = discoveryError
300264
}
301-
} else if (
302-
discoveryError !== null &&
303-
connectionStatus === 'disconnected' &&
304-
lastError === null &&
305-
!oauthAuthorizationRequired
306-
) {
307-
lastError = discoveryError
308-
toolCount = 0
265+
}
266+
267+
if (connectionStatus === 'connected') {
268+
await mcpService.clearCache(workspaceId)
309269
}
310270

311271
return createMcpSuccessResponse({

0 commit comments

Comments
 (0)