Skip to content

Commit 7fdff82

Browse files
PlaneInABottlePlaneInABottle
committed
fix(workflows): harden deployment restore flows
1 parent 61c92a4 commit 7fdff82

File tree

9 files changed

+259
-141
lines changed

9 files changed

+259
-141
lines changed

‎apps/sim/app/api/workflows/[id]/deploy/route.test.ts‎

Lines changed: 27 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -254,6 +254,33 @@ describe('Workflow deploy route', () => {
254254
)
255255
})
256256

257+
it('returns success when webhook cleanup throws after undeploy succeeds', async () => {
258+
mockValidateWorkflowAccess.mockResolvedValue({
259+
workflow: { id: 'wf-1', name: 'Test Workflow', workspaceId: 'ws-1' },
260+
auth: {
261+
success: true,
262+
userId: 'api-user',
263+
userName: 'API Key Actor',
264+
userEmail: 'api@example.com',
265+
authType: 'api_key',
266+
},
267+
})
268+
mockUndeployWorkflow.mockResolvedValue({ success: true })
269+
mockCleanupWebhooksForWorkflow.mockRejectedValue(new Error('cleanup failed'))
270+
271+
const req = new NextRequest('http://localhost:3000/api/workflows/wf-1/deploy', {
272+
method: 'DELETE',
273+
headers: { 'x-api-key': 'test-key' },
274+
})
275+
const response = await DELETE(req, { params: Promise.resolve({ id: 'wf-1' }) })
276+
277+
expect(response.status).toBe(200)
278+
const data = await response.json()
279+
expect(data.isDeployed).toBe(false)
280+
expect(mockRemoveMcpToolsForWorkflow).toHaveBeenCalledWith('wf-1', 'req-123')
281+
expect(mockRecordAudit).toHaveBeenCalled()
282+
})
283+
257284
it('checks public API restrictions against hybrid auth userId', async () => {
258285
mockValidateWorkflowAccess.mockResolvedValue({
259286
workflow: { id: 'wf-1', name: 'Test Workflow', workspaceId: 'ws-1' },

‎apps/sim/app/api/workflows/[id]/deploy/route.ts‎

Lines changed: 14 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -373,9 +373,21 @@ export async function DELETE(
373373
return createErrorResponse(result.error || 'Failed to undeploy workflow', 500)
374374
}
375375

376-
await cleanupWebhooksForWorkflow(id, workflowData as Record<string, unknown>, requestId)
376+
try {
377+
await cleanupWebhooksForWorkflow(id, workflowData as Record<string, unknown>, requestId)
378+
} catch (cleanupError) {
379+
logger.error(`[${requestId}] Failed to cleanup webhooks after undeploy for workflow ${id}`, {
380+
error: cleanupError,
381+
})
382+
}
377383

378-
await removeMcpToolsForWorkflow(id, requestId)
384+
try {
385+
await removeMcpToolsForWorkflow(id, requestId)
386+
} catch (cleanupError) {
387+
logger.error(`[${requestId}] Failed to cleanup MCP tools after undeploy for workflow ${id}`, {
388+
error: cleanupError,
389+
})
390+
}
379391

380392
logger.info(`[${requestId}] Workflow undeployed successfully: ${id}`)
381393

‎apps/sim/app/api/workflows/[id]/deployed/route.test.ts‎

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -58,4 +58,19 @@ describe('Workflow deployed-state route', () => {
5858
action: 'read',
5959
})
6060
})
61+
62+
it('returns 500 when deployed-state loading throws', async () => {
63+
mockValidateWorkflowAccess.mockResolvedValue({ workflow: { id: 'wf-1' } })
64+
mockLoadDeployedWorkflowState.mockRejectedValue(new Error('load failed'))
65+
66+
const req = new NextRequest('http://localhost:3000/api/workflows/wf-1/deployed', {
67+
headers: { 'x-api-key': 'test-key' },
68+
})
69+
const response = await GET(req, { params: Promise.resolve({ id: 'wf-1' }) })
70+
71+
expect(response.status).toBe(500)
72+
expect(response.headers.get('Cache-Control')).toBe(
73+
'no-store, no-cache, must-revalidate, max-age=0'
74+
)
75+
})
6176
})

‎apps/sim/app/api/workflows/[id]/deployed/route.ts‎

Lines changed: 7 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -41,19 +41,13 @@ export async function GET(request: NextRequest, { params }: { params: Promise<{
4141
}
4242
}
4343

44-
let deployedState = null
45-
try {
46-
const data = await loadDeployedWorkflowState(id)
47-
deployedState = {
48-
blocks: data.blocks,
49-
edges: data.edges,
50-
loops: data.loops,
51-
parallels: data.parallels,
52-
variables: data.variables,
53-
}
54-
} catch (error) {
55-
logger.warn(`[${requestId}] Failed to load deployed state for workflow ${id}`, { error })
56-
deployedState = null
44+
const data = await loadDeployedWorkflowState(id)
45+
const deployedState = {
46+
blocks: data.blocks,
47+
edges: data.edges,
48+
loops: data.loops,
49+
parallels: data.parallels,
50+
variables: data.variables,
5751
}
5852

5953
const response = createSuccessResponse({ deployedState })

‎apps/sim/app/api/workflows/[id]/deployments/[version]/revert/route.test.ts‎

Lines changed: 19 additions & 28 deletions
Original file line numberDiff line numberDiff line change
@@ -9,26 +9,18 @@ const {
99
mockDbFrom,
1010
mockDbLimit,
1111
mockDbSelect,
12-
mockDbSet,
13-
mockDbUpdate,
1412
mockDbWhere,
15-
mockDbWhereUpdate,
1613
mockRecordAudit,
17-
mockSaveWorkflowToNormalizedTables,
18-
mockSetWorkflowVariables,
14+
mockRestoreWorkflowDraftState,
1915
mockSyncMcpToolsForWorkflow,
2016
mockValidateWorkflowAccess,
2117
} = vi.hoisted(() => ({
2218
mockDbFrom: vi.fn(),
2319
mockDbLimit: vi.fn(),
2420
mockDbSelect: vi.fn(),
25-
mockDbSet: vi.fn(),
26-
mockDbUpdate: vi.fn(),
2721
mockDbWhere: vi.fn(),
28-
mockDbWhereUpdate: vi.fn(),
2922
mockRecordAudit: vi.fn(),
30-
mockSaveWorkflowToNormalizedTables: vi.fn(),
31-
mockSetWorkflowVariables: vi.fn(),
23+
mockRestoreWorkflowDraftState: vi.fn(),
3224
mockSyncMcpToolsForWorkflow: vi.fn(),
3325
mockValidateWorkflowAccess: vi.fn(),
3426
}))
@@ -64,9 +56,7 @@ vi.mock('@/lib/core/config/env', async (importOriginal) => {
6456
vi.mock('@sim/db', () => ({
6557
db: {
6658
select: mockDbSelect,
67-
update: mockDbUpdate,
6859
},
69-
workflow: { id: 'id' },
7060
workflowDeploymentVersion: {
7161
state: 'state',
7262
workflowId: 'workflowId',
@@ -85,18 +75,13 @@ vi.mock('drizzle-orm', async (importOriginal) => {
8575
})
8676

8777
vi.mock('@/lib/workflows/persistence/utils', () => ({
88-
saveWorkflowToNormalizedTables: (...args: unknown[]) =>
89-
mockSaveWorkflowToNormalizedTables(...args),
78+
restoreWorkflowDraftState: (...args: unknown[]) => mockRestoreWorkflowDraftState(...args),
9079
}))
9180

9281
vi.mock('@/lib/mcp/workflow-mcp-sync', () => ({
9382
syncMcpToolsForWorkflow: (...args: unknown[]) => mockSyncMcpToolsForWorkflow(...args),
9483
}))
9584

96-
vi.mock('@/lib/workflows/utils', () => ({
97-
setWorkflowVariables: (...args: unknown[]) => mockSetWorkflowVariables(...args),
98-
}))
99-
10085
vi.mock('@/lib/audit/log', () => ({
10186
AuditAction: { WORKFLOW_DEPLOYMENT_REVERTED: 'WORKFLOW_DEPLOYMENT_REVERTED' },
10287
AuditResourceType: { WORKFLOW: 'WORKFLOW' },
@@ -121,11 +106,7 @@ describe('Workflow deployment version revert route', () => {
121106
},
122107
},
123108
])
124-
mockDbUpdate.mockReturnValue({ set: mockDbSet })
125-
mockDbSet.mockReturnValue({ where: mockDbWhereUpdate })
126-
mockDbWhereUpdate.mockResolvedValue(undefined)
127-
mockSaveWorkflowToNormalizedTables.mockResolvedValue({ success: true })
128-
mockSetWorkflowVariables.mockResolvedValue(undefined)
109+
mockRestoreWorkflowDraftState.mockResolvedValue({ success: true })
129110
mockFetch.mockResolvedValue({ ok: true })
130111
})
131112

@@ -152,7 +133,7 @@ describe('Workflow deployment version revert route', () => {
152133
requireDeployment: false,
153134
action: 'admin',
154135
})
155-
expect(mockSaveWorkflowToNormalizedTables).toHaveBeenCalled()
136+
expect(mockRestoreWorkflowDraftState).toHaveBeenCalled()
156137
expect(mockSyncMcpToolsForWorkflow).toHaveBeenCalled()
157138
expect(mockRecordAudit).toHaveBeenCalledWith(
158139
expect.objectContaining({
@@ -195,9 +176,14 @@ describe('Workflow deployment version revert route', () => {
195176

196177
await POST(req, { params: Promise.resolve({ id: 'wf-1', version: '3' }) })
197178

198-
expect(mockSetWorkflowVariables).toHaveBeenCalledWith('wf-1', {
199-
var1: { id: 'var1', name: 'API Token', type: 'string', value: 'secret' },
200-
})
179+
expect(mockRestoreWorkflowDraftState).toHaveBeenCalledWith(
180+
expect.objectContaining({
181+
workflowId: 'wf-1',
182+
variables: {
183+
var1: { id: 'var1', name: 'API Token', type: 'string', value: 'secret' },
184+
},
185+
})
186+
)
201187
})
202188

203189
it('defaults variables safely when missing from the deployment snapshot', async () => {
@@ -219,7 +205,12 @@ describe('Workflow deployment version revert route', () => {
219205

220206
await POST(req, { params: Promise.resolve({ id: 'wf-1', version: '3' }) })
221207

222-
expect(mockSetWorkflowVariables).toHaveBeenCalledWith('wf-1', {})
208+
expect(mockRestoreWorkflowDraftState).toHaveBeenCalledWith(
209+
expect.objectContaining({
210+
workflowId: 'wf-1',
211+
variables: {},
212+
})
213+
)
223214
})
224215

225216
it('returns success when MCP sync throws after revert succeeds', async () => {

‎apps/sim/app/api/workflows/[id]/deployments/[version]/revert/route.ts‎

Lines changed: 13 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
import { db, workflow, workflowDeploymentVersion } from '@sim/db'
1+
import { db, workflowDeploymentVersion } from '@sim/db'
22
import { createLogger } from '@sim/logger'
33
import { and, eq } from 'drizzle-orm'
44
import type { NextRequest } from 'next/server'
@@ -7,8 +7,7 @@ import { AuditAction, AuditResourceType, recordAudit } from '@/lib/audit/log'
77
import { env } from '@/lib/core/config/env'
88
import { generateRequestId } from '@/lib/core/utils/request'
99
import { syncMcpToolsForWorkflow } from '@/lib/mcp/workflow-mcp-sync'
10-
import { saveWorkflowToNormalizedTables } from '@/lib/workflows/persistence/utils'
11-
import { setWorkflowVariables } from '@/lib/workflows/utils'
10+
import { restoreWorkflowDraftState } from '@/lib/workflows/persistence/utils'
1211
import { validateWorkflowAccess } from '@/app/api/workflows/middleware'
1312
import { createErrorResponse, createSuccessResponse } from '@/app/api/workflows/utils'
1413

@@ -85,27 +84,23 @@ export async function POST(
8584
return createErrorResponse('Invalid deployed state structure', 500)
8685
}
8786

88-
const saveResult = await saveWorkflowToNormalizedTables(id, {
89-
blocks: deployedState.blocks,
90-
edges: deployedState.edges,
91-
loops: deployedState.loops || {},
92-
parallels: deployedState.parallels || {},
87+
const restoredAt = new Date()
88+
const saveResult = await restoreWorkflowDraftState({
89+
workflowId: id,
90+
state: {
91+
blocks: deployedState.blocks,
92+
edges: deployedState.edges,
93+
loops: deployedState.loops || {},
94+
parallels: deployedState.parallels || {},
95+
},
9396
variables: deployedState.variables || {},
94-
lastSaved: Date.now(),
95-
deploymentStatuses: deployedState.deploymentStatuses || {},
97+
restoredAt,
9698
})
9799

98100
if (!saveResult.success) {
99101
return createErrorResponse(saveResult.error || 'Failed to save deployed state', 500)
100102
}
101103

102-
await setWorkflowVariables(id, deployedState.variables || {})
103-
104-
await db
105-
.update(workflow)
106-
.set({ lastSynced: new Date(), updatedAt: new Date() })
107-
.where(eq(workflow.id, id))
108-
109104
try {
110105
await syncMcpToolsForWorkflow({
111106
workflowId: id,
@@ -150,7 +145,7 @@ export async function POST(
150145

151146
return createSuccessResponse({
152147
message: 'Reverted to deployment version',
153-
lastSaved: Date.now(),
148+
lastSaved: restoredAt.getTime(),
154149
})
155150
} catch (error: any) {
156151
logger.error('Error reverting to deployment version', error)

‎apps/sim/app/api/workflows/middleware.ts‎

Lines changed: 1 addition & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -118,7 +118,7 @@ export async function validateWorkflowAccess(
118118
return { workflow, auth }
119119
}
120120

121-
if (requireDeployment) {
121+
{
122122
const internalSecret = request.headers.get('X-Internal-Secret')
123123
const hasValidInternalSecret =
124124
allowInternalSecret && env.INTERNAL_API_SECRET && internalSecret === env.INTERNAL_API_SECRET
@@ -197,13 +197,6 @@ export async function validateWorkflowAccess(
197197

198198
return { workflow }
199199
}
200-
201-
return {
202-
error: {
203-
message: 'Internal server error',
204-
status: 500,
205-
},
206-
}
207200
} catch (error) {
208201
logger.error('Validation error:', { error })
209202
return {

‎apps/sim/lib/workflows/persistence/utils.test.ts‎

Lines changed: 53 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1165,6 +1165,59 @@ describe('Database Helpers', () => {
11651165
})
11661166
})
11671167

1168+
describe('restoreWorkflowDraftState', () => {
1169+
it('rolls back normalized graph restore when workflow variable update fails', async () => {
1170+
const txDeleteWhere = vi.fn().mockResolvedValue(undefined)
1171+
const txInsertValues = vi.fn().mockResolvedValue(undefined)
1172+
const txUpdateWhere = vi.fn().mockRejectedValue(new Error('workflow update failed'))
1173+
const txUpdateSet = vi.fn().mockReturnValue({ where: txUpdateWhere })
1174+
const tx = {
1175+
delete: vi.fn().mockReturnValue({ where: txDeleteWhere }),
1176+
insert: vi.fn().mockReturnValue({ values: txInsertValues }),
1177+
update: vi.fn().mockReturnValue({ set: txUpdateSet }),
1178+
}
1179+
1180+
mockDb.transaction = vi.fn().mockImplementation(async (callback) => callback(tx))
1181+
1182+
const result = await dbHelpers.restoreWorkflowDraftState({
1183+
workflowId: mockWorkflowId,
1184+
state: {
1185+
blocks: asAppState(mockWorkflowState).blocks,
1186+
edges: asAppState(mockWorkflowState).edges,
1187+
loops: asAppState(mockWorkflowState).loops,
1188+
parallels: asAppState(mockWorkflowState).parallels,
1189+
},
1190+
variables: {
1191+
apiToken: {
1192+
id: 'apiToken',
1193+
name: 'API Token',
1194+
type: 'string',
1195+
value: 'secret',
1196+
},
1197+
},
1198+
restoredAt: new Date('2024-01-01T00:00:00.000Z'),
1199+
})
1200+
1201+
expect(result).toEqual({ success: false, error: 'workflow update failed' })
1202+
expect(mockDb.transaction).toHaveBeenCalledTimes(1)
1203+
expect(tx.delete).toHaveBeenCalledTimes(3)
1204+
expect(tx.insert).toHaveBeenCalledTimes(3)
1205+
expect(tx.update).toHaveBeenCalledWith({})
1206+
expect(txUpdateSet).toHaveBeenCalledWith({
1207+
variables: {
1208+
apiToken: {
1209+
id: 'apiToken',
1210+
name: 'API Token',
1211+
type: 'string',
1212+
value: 'secret',
1213+
},
1214+
},
1215+
lastSynced: new Date('2024-01-01T00:00:00.000Z'),
1216+
updatedAt: new Date('2024-01-01T00:00:00.000Z'),
1217+
})
1218+
})
1219+
})
1220+
11681221
describe('migrateAgentBlocksToMessagesFormat', () => {
11691222
it('should migrate agent block with both systemPrompt and userPrompt', () => {
11701223
const blocks = {

0 commit comments

Comments
 (0)