Address PR review feedback (#5754)

Author: j15z

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

@@ -375,6 +375,48 @@ describe('MCP server refresh route', () => {
     }
   )
 
+  it('does not sync published tools after a newer cache invalidation', async () => {
+    const publicationOrder = new Date(initialServer.lastToolsRefresh.getTime() + 30_000)
+    mockDiscoverServerTools.mockResolvedValueOnce({
+      tools: [
+        {
+          name: 'stale-search',
+          description: 'Search tool published before invalidation',
+          inputSchema: {},
+          serverId: 'server-1',
+          serverName: 'OAuth Server',
+        },
+      ],
+      state: 'published',
+      publicationOrder,
+    })
+    const cacheInvalidatedServer = {
+      ...initialServer,
+      lastConnected: publicationOrder,
+      lastToolsRefresh: new Date(publicationOrder.getTime() + 30_000),
+      toolCount: 0,
+    }
+    mockSelect.mockReturnValueOnce(selectRows([cacheInvalidatedServer]))
+
+    const request = new Request('http://localhost/api/mcp/servers/server-1/refresh', {
+      method: 'POST',
+    }) as NextRequest
+    const response = await POST(request, { params: Promise.resolve({ id: 'server-1' }) })
+    const body = await response.json()
+
+    expect(body.data).toEqual(
+      expect.objectContaining({
+        status: 'connected',
+        error: null,
+        toolCount: 0,
+        workflowsUpdated: 0,
+        updatedWorkflowIds: [],
+      })
+    )
+    expect(mockSelect).toHaveBeenCalledTimes(2)
+    expect(mockUpdate).not.toHaveBeenCalled()
+  })
+
   it('preserves a newer successful refresh when discovery is superseded', async () => {
     mockDiscoverServerTools.mockResolvedValueOnce({
       tools: [],

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

@@ -194,6 +194,7 @@ export const POST = withRouteHandler(
         let syncResult: SyncResult = { updatedCount: 0, updatedWorkflowIds: [] }
         let discoveredTools: McpTool[] = []
         let discoveryState: McpServerDiscoveryState | null = null
+        let discoveryPublicationOrder: Date | null = null
         let discoveryError: string | null = null
         let oauthAuthorizationRequired = false
 
@@ -206,6 +207,7 @@ export const POST = withRouteHandler(
           )
           discoveredTools = discovery.tools
           discoveryState = discovery.state
+          discoveryPublicationOrder = discovery.publicationOrder ?? null
           logger.info(
             `[${requestId}] Discovered ${discoveredTools.length} tools from server ${serverId}`
           )
@@ -240,7 +242,12 @@ export const POST = withRouteHandler(
           )
           .limit(1)
 
-        if (discoveryError === null && discoveryState === 'published') {
+        const publicationBaseline = discoveryPublicationOrder ?? server.lastToolsRefresh
+        const newerPublicationWonRace =
+          refreshedServer?.lastToolsRefresh != null &&
+          (publicationBaseline == null || refreshedServer.lastToolsRefresh > publicationBaseline)
+
+        if (discoveryError === null && discoveryState === 'published' && !newerPublicationWonRace) {
           try {
             syncResult = await syncToolSchemasToWorkflows(
               workspaceId,
@@ -265,10 +272,6 @@ export const POST = withRouteHandler(
         let connectionStatus = refreshedServer?.connectionStatus ?? 'error'
         let lastError = refreshedServer ? refreshedServer.lastError : discoveryError
         let toolCount = refreshedServer?.toolCount ?? discoveredTools.length
-        const newerPublicationWonRace =
-          refreshedServer?.lastToolsRefresh != null &&
-          (server.lastToolsRefresh == null ||
-            refreshedServer.lastToolsRefresh > server.lastToolsRefresh)
         const newerSuccessWonRace =
           connectionStatus === 'connected' &&
           newerPublicationWonRace &&

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

@@ -966,6 +966,7 @@ describe('McpService.discoverTools per-server caching', () => {
     ).resolves.toEqual({
       tools: [tool('still-valid', 'mcp-a')],
       state: 'published',
+      publicationOrder: expect.any(Date),
     })
 
     const successfulStatusWrites = mockUpdateSet.mock.calls
@@ -1553,6 +1554,7 @@ describe('McpService.discoverTools per-server caching', () => {
       await expect(discovery).resolves.toEqual({
         tools: [tool('retry-winner', 'mcp-a')],
         state: 'published',
+        publicationOrder: expect.any(Date),
       })
 
       expect(mockCacheAdapter.beginMutation).toHaveBeenCalledTimes(3)

apps/sim/lib/mcp/service.ts

@@ -85,6 +85,7 @@ export type McpServerDiscoveryState =
 export interface McpServerDiscoveryResult {
   tools: McpTool[]
   state: McpServerDiscoveryState
+  publicationOrder?: Date
 }
 
 type DiscoveryOutcome =
@@ -1216,7 +1217,8 @@ class McpService {
             }
             return { tools, state: 'unavailable' }
           }
-          return { tools, state: 'published' }
+          if (!mutation) return { tools, state: 'unavailable' }
+          return { tools, state: 'published', publicationOrder: new Date(mutation.id) }
         } finally {
           await client.disconnect()
         }