Skip to content

Commit 8dc843a

Browse files
j15zj15z
committed
Address PR review feedback (#5754)
- Retry failure and OAuth status publication across metadata-only races - Keep losing winner-cache refreshes from syncing workflows
1 parent 3296f1f commit 8dc843a

4 files changed

Lines changed: 282 additions & 2 deletions

File tree

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

Lines changed: 34 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -277,6 +277,40 @@ describe('MCP server refresh route', () => {
277277
)
278278
})
279279

280+
it('returns the winning cached tools without syncing workflows from the losing refresh', async () => {
281+
mockDiscoverServerTools.mockResolvedValueOnce({
282+
tools: [
283+
{
284+
name: 'winner-search',
285+
description: 'Search tool from the winning refresh',
286+
inputSchema: {},
287+
serverId: 'server-1',
288+
serverName: 'OAuth Server',
289+
},
290+
],
291+
state: 'winner-cache',
292+
})
293+
mockSelect.mockReturnValueOnce(selectRows([persistedServer]))
294+
295+
const request = new Request('http://localhost/api/mcp/servers/server-1/refresh', {
296+
method: 'POST',
297+
}) as NextRequest
298+
const response = await POST(request, { params: Promise.resolve({ id: 'server-1' }) })
299+
const body = await response.json()
300+
301+
expect(body.data).toEqual(
302+
expect.objectContaining({
303+
status: 'connected',
304+
error: null,
305+
toolCount: 1,
306+
workflowsUpdated: 0,
307+
updatedWorkflowIds: [],
308+
})
309+
)
310+
expect(mockSelect).toHaveBeenCalledTimes(2)
311+
expect(mockUpdate).not.toHaveBeenCalled()
312+
})
313+
280314
it('fails closed without syncing workflows when discovery is superseded', async () => {
281315
mockDiscoverServerTools.mockResolvedValueOnce({
282316
tools: [],

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

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -240,7 +240,7 @@ export const POST = withRouteHandler(
240240
)
241241
.limit(1)
242242

243-
if (discoveryError === null && discoveryState !== 'superseded') {
243+
if (discoveryError === null && discoveryState === 'published') {
244244
try {
245245
syncResult = await syncToolSchemasToWorkflows(
246246
workspaceId,

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

Lines changed: 172 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -904,6 +904,178 @@ describe('McpService.discoverTools per-server caching', () => {
904904
)
905905
})
906906

907+
it('publishes a failed discovery after repeated metadata-only renames win the database CAS', async () => {
908+
const serverKey = `workspace:${WORKSPACE_ID}:server:mcp-a`
909+
const failureKey = `${serverKey}:failure`
910+
const original = dbRow('mcp-a', 'A', {
911+
statusConfig: { consecutiveFailures: 0, lastSuccessfulDiscovery: null },
912+
})
913+
const renamed = dbRow('mcp-a', 'Renamed A', {
914+
description: 'Updated display-only description',
915+
updatedAt: new Date('2026-01-01T00:00:01Z'),
916+
statusConfig: { consecutiveFailures: 0, lastSuccessfulDiscovery: null },
917+
})
918+
const renamedAgain = dbRow('mcp-a', 'Renamed A Again', {
919+
description: 'Another display-only description',
920+
updatedAt: new Date('2026-01-01T00:00:02Z'),
921+
statusConfig: { consecutiveFailures: 0, lastSuccessfulDiscovery: null },
922+
})
923+
mockGetWorkspaceServersRows
924+
.mockResolvedValueOnce([original])
925+
.mockResolvedValueOnce([renamed])
926+
.mockResolvedValueOnce([renamed])
927+
.mockResolvedValueOnce([renamed])
928+
.mockResolvedValueOnce([renamed])
929+
.mockResolvedValueOnce([renamedAgain])
930+
.mockResolvedValueOnce([renamedAgain])
931+
.mockResolvedValueOnce([renamedAgain])
932+
.mockResolvedValueOnce([renamedAgain])
933+
.mockResolvedValue([renamedAgain])
934+
mockUpdateReturning
935+
.mockResolvedValueOnce([])
936+
.mockResolvedValueOnce([])
937+
.mockResolvedValueOnce([])
938+
.mockResolvedValueOnce([])
939+
.mockResolvedValueOnce([])
940+
.mockResolvedValueOnce([])
941+
.mockResolvedValueOnce([{ id: 'mcp-a' }])
942+
mockListTools.mockRejectedValueOnce(new Error('Permanent discovery failure'))
943+
cacheStore.set(serverKey, {
944+
tools: [tool('previous-tool', 'mcp-a')],
945+
expiry: Date.now() + 60_000,
946+
})
947+
948+
await expect(
949+
mcpService.discoverServerTools(USER_ID, 'mcp-a', WORKSPACE_ID, true)
950+
).rejects.toThrow('Permanent discovery failure')
951+
952+
const failureStatusWrites = mockUpdateSet.mock.calls
953+
.map(([update]) => update)
954+
.filter((update) => update.lastError === 'Connection failed')
955+
expect(failureStatusWrites).toHaveLength(7)
956+
expect(failureStatusWrites.at(-1)).toEqual(
957+
expect.objectContaining({ connectionStatus: 'disconnected', toolCount: 0 })
958+
)
959+
expect(cacheStore.has(serverKey)).toBe(false)
960+
expect(cacheStore.has(failureKey)).toBe(true)
961+
expect(mockCacheAdapter.applyMutationIfCurrent).toHaveBeenCalledWith(
962+
serverKey,
963+
expect.any(Number),
964+
null,
965+
[]
966+
)
967+
})
968+
969+
it('publishes OAuth pending after repeated metadata-only renames win the database CAS', async () => {
970+
const serverKey = `workspace:${WORKSPACE_ID}:server:mcp-a`
971+
const original = dbRow('mcp-a', 'A')
972+
const renamed = dbRow('mcp-a', 'Renamed A', {
973+
description: 'Updated display-only description',
974+
updatedAt: new Date('2026-01-01T00:00:01Z'),
975+
})
976+
const renamedAgain = dbRow('mcp-a', 'Renamed A Again', {
977+
description: 'Another display-only description',
978+
updatedAt: new Date('2026-01-01T00:00:02Z'),
979+
})
980+
mockGetWorkspaceServersRows
981+
.mockResolvedValueOnce([original])
982+
.mockResolvedValueOnce([renamed])
983+
.mockResolvedValueOnce([renamedAgain])
984+
.mockResolvedValue([renamedAgain])
985+
mockUpdateReturning
986+
.mockResolvedValueOnce([])
987+
.mockResolvedValueOnce([])
988+
.mockResolvedValueOnce([{ id: 'mcp-a' }])
989+
mockListTools.mockRejectedValueOnce(new McpOauthAuthorizationRequiredError('mcp-a', 'A'))
990+
cacheStore.set(serverKey, {
991+
tools: [tool('previous-tool', 'mcp-a')],
992+
expiry: Date.now() + 60_000,
993+
})
994+
995+
await expect(
996+
mcpService.discoverServerTools(USER_ID, 'mcp-a', WORKSPACE_ID, true)
997+
).rejects.toThrow('OAuth authorization required')
998+
999+
const oauthStatusWrites = mockUpdateSet.mock.calls
1000+
.map(([update]) => update)
1001+
.filter((update) => update.connectionStatus === 'disconnected' && update.lastError === null)
1002+
expect(oauthStatusWrites).toHaveLength(3)
1003+
expect(cacheStore.has(serverKey)).toBe(false)
1004+
expect(mockCacheAdapter.applyMutationIfCurrent).toHaveBeenCalledWith(
1005+
serverKey,
1006+
expect.any(Number),
1007+
null,
1008+
[]
1009+
)
1010+
})
1011+
1012+
it('does not retry failed status publication after a connection-config edit', async () => {
1013+
const serverKey = `workspace:${WORKSPACE_ID}:server:mcp-a`
1014+
const failureKey = `${serverKey}:failure`
1015+
const original = dbRow('mcp-a', 'A', {
1016+
statusConfig: { consecutiveFailures: 0, lastSuccessfulDiscovery: null },
1017+
})
1018+
const reconfigured = dbRow('mcp-a', 'A', {
1019+
url: 'https://changed-config.example.com/mcp',
1020+
updatedAt: new Date('2026-01-01T00:00:01Z'),
1021+
statusConfig: { consecutiveFailures: 0, lastSuccessfulDiscovery: null },
1022+
})
1023+
mockGetWorkspaceServersRows
1024+
.mockResolvedValueOnce([original])
1025+
.mockResolvedValueOnce([original])
1026+
.mockResolvedValueOnce([original])
1027+
.mockResolvedValueOnce([original])
1028+
.mockResolvedValue([reconfigured])
1029+
mockUpdateReturning.mockResolvedValue([])
1030+
mockListTools.mockRejectedValueOnce(new Error('Permanent discovery failure'))
1031+
1032+
await expect(
1033+
mcpService.discoverServerTools(USER_ID, 'mcp-a', WORKSPACE_ID, true)
1034+
).rejects.toThrow('Permanent discovery failure')
1035+
1036+
const failureStatusWrites = mockUpdateSet.mock.calls
1037+
.map(([update]) => update)
1038+
.filter((update) => update.lastError === 'Connection failed')
1039+
expect(failureStatusWrites).toHaveLength(3)
1040+
expect(cacheStore.has(failureKey)).toBe(false)
1041+
expect(mockCacheAdapter.applyMutationIfCurrent).toHaveBeenLastCalledWith(
1042+
serverKey,
1043+
expect.any(Number),
1044+
null,
1045+
[failureKey]
1046+
)
1047+
})
1048+
1049+
it('does not retry OAuth status publication after a connection-config edit', async () => {
1050+
const serverKey = `workspace:${WORKSPACE_ID}:server:mcp-a`
1051+
const original = dbRow('mcp-a', 'A')
1052+
const reconfigured = dbRow('mcp-a', 'A', {
1053+
url: 'https://changed-config.example.com/mcp',
1054+
updatedAt: new Date('2026-01-01T00:00:01Z'),
1055+
})
1056+
mockGetWorkspaceServersRows
1057+
.mockResolvedValueOnce([original])
1058+
.mockResolvedValueOnce([reconfigured])
1059+
.mockResolvedValue([reconfigured])
1060+
mockUpdateReturning.mockResolvedValue([])
1061+
mockListTools.mockRejectedValueOnce(new McpOauthAuthorizationRequiredError('mcp-a', 'A'))
1062+
1063+
await expect(
1064+
mcpService.discoverServerTools(USER_ID, 'mcp-a', WORKSPACE_ID, true)
1065+
).rejects.toThrow('OAuth authorization required')
1066+
1067+
const oauthStatusWrites = mockUpdateSet.mock.calls
1068+
.map(([update]) => update)
1069+
.filter((update) => update.connectionStatus === 'disconnected' && update.lastError === null)
1070+
expect(oauthStatusWrites).toHaveLength(1)
1071+
expect(mockCacheAdapter.applyMutationIfCurrent).toHaveBeenLastCalledWith(
1072+
serverKey,
1073+
expect.any(Number),
1074+
null,
1075+
[]
1076+
)
1077+
})
1078+
9071079
it('supersedes an older discovery before it can publish status', async () => {
9081080
vi.useFakeTimers({ toFake: ['Date'] })
9091081
try {

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

Lines changed: 75 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -69,6 +69,7 @@ export function getTimestampMillisecondBounds(timestamp: string): {
6969
const FAILURE_CACHE_SENTINEL: McpTool[] = []
7070
const CACHE_MUTATION_BEGIN_ATTEMPTS = 2
7171
const STATUS_UPDATE_CAS_ATTEMPTS = 3
72+
const STATUS_METADATA_RACE_RETRY_ATTEMPTS = 3
7273

7374
type CacheMutationResult = 'applied' | 'superseded' | 'unavailable'
7475
type SuccessfulPublicationResult = 'published' | Exclude<CacheMutationResult, 'applied'>
@@ -813,6 +814,49 @@ class McpService {
813814
return 'unavailable'
814815
}
815816

817+
private async getCurrentConfigForOwnedDiscoveryMutation(
818+
workspaceId: string,
819+
config: McpServerConfig,
820+
mutation: CacheMutation
821+
): Promise<McpServerConfig | null> {
822+
const ownership = await this.applyServerCacheMutation(
823+
workspaceId,
824+
config.id,
825+
mutation,
826+
null,
827+
[]
828+
)
829+
if (ownership !== 'applied') return null
830+
831+
const currentConfig = await this.getServerConfig(config.id, workspaceId)
832+
if (
833+
!currentConfig ||
834+
!config.discoveryRevision ||
835+
currentConfig.discoveryRevision !== config.discoveryRevision
836+
) {
837+
return null
838+
}
839+
return currentConfig
840+
}
841+
842+
private async retryStatusPublicationAfterMetadataRace(
843+
workspaceId: string,
844+
config: McpServerConfig,
845+
mutation: CacheMutation,
846+
publishStatus: (configUpdatedAt: string) => Promise<boolean>
847+
): Promise<boolean> {
848+
for (let attempt = 0; attempt < STATUS_METADATA_RACE_RETRY_ATTEMPTS; attempt++) {
849+
const currentConfig = await this.getCurrentConfigForOwnedDiscoveryMutation(
850+
workspaceId,
851+
config,
852+
mutation
853+
)
854+
if (!currentConfig) return false
855+
if (await publishStatus(currentConfig.updatedAt!)) return true
856+
}
857+
return false
858+
}
859+
816860
private async publishFailedDiscovery(
817861
workspaceId: string,
818862
config: McpServerConfig,
@@ -843,6 +887,24 @@ class McpService {
843887
})
844888
if (statusApplied) return true
845889

890+
// A metadata-only edit can advance updatedAt without changing anything
891+
// that affects discovery. Keep the failed publication only while this
892+
// mutation still owns the cache and the connection configuration is
893+
// unchanged, then retry the status CAS against the row's current token.
894+
const retriedStatusApplied = await this.retryStatusPublicationAfterMetadataRace(
895+
workspaceId,
896+
config,
897+
mutation,
898+
(configUpdatedAt) =>
899+
this.updateServerStatus(config.id, workspaceId, {
900+
outcome: 'failed',
901+
error: message,
902+
configUpdatedAt,
903+
publicationOrder: new Date(mutation.id),
904+
})
905+
)
906+
if (retriedStatusApplied) return true
907+
846908
// Do not leave a negative-cache entry for a failure that lost the
847909
// database publication CAS.
848910
await this.applyServerCacheMutation(workspaceId, config.id, mutation, null, [
@@ -865,12 +927,24 @@ class McpService {
865927
)
866928
if (cacheApplied !== 'applied' || !mutation) return false
867929

868-
return this.markServerOauthPending(
930+
const statusApplied = await this.markServerOauthPending(
869931
config.id,
870932
workspaceId,
871933
config.updatedAt!,
872934
new Date(mutation.id)
873935
)
936+
if (statusApplied) return true
937+
938+
// Metadata-only edits share discovery state with the original row. Retry
939+
// only while this mutation still owns the cache and the discovery-relevant
940+
// configuration has not changed.
941+
return this.retryStatusPublicationAfterMetadataRace(
942+
workspaceId,
943+
config,
944+
mutation,
945+
(configUpdatedAt) =>
946+
this.markServerOauthPending(config.id, workspaceId, configUpdatedAt, new Date(mutation.id))
947+
)
874948
}
875949

876950
async discoverTools(

0 commit comments

Comments
 (0)