Skip to content

Commit f75e0e0

Browse files
committed
fix(webhooks): clean up stale deployment-version webhooks during deploy
saveTriggerWebhooksForDeploy only inspected webhooks matching the version being deployed; webhooks pinned to older deployment versions were left untouched. Removing them was delegated to a separate, best-effort CLEANUP_INACTIVE outbox event that dead-letters silently on failure. When that event didn't complete, old-version webhooks lingered as is_active orphans that fetchActiveWebhooks skips (version mismatch), so they silently stopped polling. Fold old-version webhook cleanup into the same registration pass: collect webhooks pinned to a different (non-null) deployment version and route them through the existing external-teardown + delete path, so a successful deploy guarantees only the active version has webhooks. Null-version webhooks are left untouched, matching CLEANUP_INACTIVE semantics.
1 parent 3e03f8c commit f75e0e0

2 files changed

Lines changed: 125 additions & 3 deletions

File tree

Lines changed: 105 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,105 @@
1+
/**
2+
* @vitest-environment node
3+
*/
4+
import { beforeEach, describe, expect, it, vi } from 'vitest'
5+
6+
const { selectRows, deleteSpy, cleanupCalls, cleanupSpy } = vi.hoisted(() => ({
7+
selectRows: { current: [] as Array<Record<string, unknown>> },
8+
deleteSpy: vi.fn(() => ({ where: vi.fn(() => Promise.resolve(undefined)) })),
9+
cleanupCalls: [] as string[],
10+
cleanupSpy: vi.fn(async (wh: { id: string }) => {
11+
cleanupCalls.push(wh.id)
12+
}),
13+
}))
14+
15+
vi.mock('@sim/db', () => ({
16+
db: {
17+
select: () => ({ from: () => ({ where: () => Promise.resolve(selectRows.current) }) }),
18+
delete: deleteSpy,
19+
transaction: async (cb: (tx: unknown) => Promise<unknown>) =>
20+
cb({ insert: () => ({ values: () => Promise.resolve(undefined) }) }),
21+
},
22+
}))
23+
vi.mock('@sim/db/schema', () => ({
24+
account: {},
25+
credentialSetMember: {},
26+
webhook: { id: 'webhook.id', workflowId: 'webhook.workflowId' },
27+
workflowDeploymentVersion: {},
28+
}))
29+
vi.mock('drizzle-orm', () => ({
30+
and: vi.fn(),
31+
eq: vi.fn((field: unknown, value: unknown) => ({ field, value })),
32+
inArray: vi.fn(),
33+
isNotNull: vi.fn(),
34+
isNull: vi.fn(),
35+
or: vi.fn(),
36+
}))
37+
vi.mock('@/lib/webhooks/provider-subscriptions', () => ({
38+
cleanupExternalWebhook: cleanupSpy,
39+
createExternalWebhookSubscription: vi.fn(),
40+
shouldRecreateExternalWebhookSubscription: vi.fn(() => false),
41+
}))
42+
vi.mock('@/lib/webhooks/providers', () => ({ getProviderHandler: vi.fn() }))
43+
vi.mock('@/lib/webhooks/utils.server', () => ({
44+
findConflictingWebhookPathOwner: vi.fn(),
45+
syncWebhooksForCredentialSet: vi.fn(),
46+
}))
47+
vi.mock('@/lib/webhooks/pending-verification', () => ({
48+
PendingWebhookVerificationTracker: class {
49+
async register() {}
50+
async clearAll() {}
51+
},
52+
}))
53+
vi.mock('@/lib/oauth', () => ({ getProviderIdFromServiceId: vi.fn() }))
54+
vi.mock('@/lib/workflows/subblocks/visibility', () => ({ buildCanonicalIndex: vi.fn() }))
55+
vi.mock('@/blocks', () => ({ getBlock: vi.fn() }))
56+
vi.mock('@/triggers', () => ({ getTrigger: vi.fn(), isTriggerValid: vi.fn(() => false) }))
57+
vi.mock('@/triggers/constants', () => ({ SYSTEM_SUBBLOCK_IDS: new Set<string>() }))
58+
59+
import { saveTriggerWebhooksForDeploy } from '@/lib/webhooks/deploy'
60+
61+
function baseInput(deploymentVersionId?: string) {
62+
return {
63+
request: {} as never,
64+
workflowId: 'wf-1',
65+
workflow: {},
66+
userId: 'user-1',
67+
blocks: {},
68+
requestId: 'req-1',
69+
deploymentVersionId,
70+
}
71+
}
72+
73+
describe('saveTriggerWebhooksForDeploy — stale deployment-version cleanup', () => {
74+
beforeEach(() => {
75+
vi.clearAllMocks()
76+
cleanupCalls.length = 0
77+
selectRows.current = []
78+
})
79+
80+
it('tears down older-version webhooks while preserving null-version webhooks', async () => {
81+
selectRows.current = [
82+
{ id: 'stale-old', workflowId: 'wf-1', blockId: 'b1', deploymentVersionId: 'ver-old' },
83+
{ id: 'null-version', workflowId: 'wf-1', blockId: 'b2', deploymentVersionId: null },
84+
]
85+
86+
const result = await saveTriggerWebhooksForDeploy(baseInput('ver-active'))
87+
88+
expect(result.success).toBe(true)
89+
expect(cleanupCalls).toContain('stale-old')
90+
expect(deleteSpy).toHaveBeenCalledTimes(1)
91+
expect(cleanupCalls).not.toContain('null-version')
92+
})
93+
94+
it('does nothing to other-version webhooks when no deploymentVersionId is given', async () => {
95+
selectRows.current = [
96+
{ id: 'some-version', workflowId: 'wf-1', blockId: 'b1', deploymentVersionId: 'ver-x' },
97+
]
98+
99+
const result = await saveTriggerWebhooksForDeploy(baseInput(undefined))
100+
101+
expect(result.success).toBe(true)
102+
expect(cleanupCalls).toHaveLength(0)
103+
expect(deleteSpy).not.toHaveBeenCalled()
104+
})
105+
})

apps/sim/lib/webhooks/deploy.ts

Lines changed: 20 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -452,7 +452,18 @@ async function syncCredentialSetWebhooks(params: {
452452

453453
/**
454454
* Saves trigger webhook configurations as part of workflow deployment.
455-
* Uses delete + create approach for changed/deleted webhooks.
455+
* Uses a delete + create approach for changed/deleted webhooks.
456+
*
457+
* Also removes webhooks pinned to an older (non-active) deployment version of this
458+
* workflow. `fetchActiveWebhooks` only polls webhooks whose version matches the active
459+
* one, so leaving stale-version rows in place strands them as live-but-never-polled
460+
* orphans. Folding their teardown into this pass guarantees a successful deploy leaves
461+
* only the active version's webhooks, rather than relying on the separate, best-effort
462+
* `CLEANUP_INACTIVE` outbox event. Webhooks with a null version are left untouched, the
463+
* same scope `CLEANUP_INACTIVE` uses.
464+
*
465+
* The sole caller only invokes this for the active deployment version (it skips the sync
466+
* when the version is no longer active), so non-matching rows are always genuinely stale.
456467
*/
457468
export async function saveTriggerWebhooksForDeploy({
458469
request,
@@ -477,12 +488,18 @@ export async function saveTriggerWebhooksForDeploy({
477488
.from(webhook)
478489
.where(and(eq(webhook.workflowId, workflowId), isNull(webhook.archivedAt)))
479490

480-
// Separate webhooks by version: current deployment vs others
481491
const existingWebhooks: typeof allWorkflowWebhooks = []
492+
const staleVersionWebhooks: typeof allWorkflowWebhooks = []
482493

483494
for (const wh of allWorkflowWebhooks) {
484495
if (deploymentVersionId && wh.deploymentVersionId === deploymentVersionId) {
485496
existingWebhooks.push(wh)
497+
} else if (
498+
deploymentVersionId &&
499+
wh.deploymentVersionId &&
500+
wh.deploymentVersionId !== deploymentVersionId
501+
) {
502+
staleVersionWebhooks.push(wh)
486503
}
487504
}
488505

@@ -508,7 +525,7 @@ export async function saveTriggerWebhooksForDeploy({
508525
}
509526
const webhookConfigs = new Map<string, WebhookConfig>()
510527

511-
const webhooksToDelete: typeof existingWebhooks = []
528+
const webhooksToDelete: typeof existingWebhooks = [...staleVersionWebhooks]
512529
const blocksNeedingWebhook: BlockState[] = []
513530
const blocksNeedingCredentialSetSync: BlockState[] = []
514531

0 commit comments

Comments
 (0)