Skip to content

Commit 6998516

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 6998516

2 files changed

Lines changed: 121 additions & 2 deletions

File tree

Lines changed: 107 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,107 @@
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 and deletes webhooks pinned to older deployment versions', 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+
// Stale (older-version) webhook is cleaned up + deleted.
90+
expect(cleanupCalls).toContain('stale-old')
91+
expect(deleteSpy).toHaveBeenCalledTimes(1)
92+
// Null-version webhook is left untouched (matches CLEANUP_INACTIVE semantics).
93+
expect(cleanupCalls).not.toContain('null-version')
94+
})
95+
96+
it('does nothing to other-version webhooks when no deploymentVersionId is given', async () => {
97+
selectRows.current = [
98+
{ id: 'some-version', workflowId: 'wf-1', blockId: 'b1', deploymentVersionId: 'ver-x' },
99+
]
100+
101+
const result = await saveTriggerWebhooksForDeploy(baseInput(undefined))
102+
103+
expect(result.success).toBe(true)
104+
expect(cleanupCalls).toHaveLength(0)
105+
expect(deleteSpy).not.toHaveBeenCalled()
106+
})
107+
})

apps/sim/lib/webhooks/deploy.ts

Lines changed: 14 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -477,12 +477,24 @@ export async function saveTriggerWebhooksForDeploy({
477477
.from(webhook)
478478
.where(and(eq(webhook.workflowId, workflowId), isNull(webhook.archivedAt)))
479479

480-
// Separate webhooks by version: current deployment vs others
480+
// Separate webhooks by version: current deployment vs stale (older) versions.
481481
const existingWebhooks: typeof allWorkflowWebhooks = []
482+
const staleVersionWebhooks: typeof allWorkflowWebhooks = []
482483

483484
for (const wh of allWorkflowWebhooks) {
484485
if (deploymentVersionId && wh.deploymentVersionId === deploymentVersionId) {
485486
existingWebhooks.push(wh)
487+
} else if (
488+
deploymentVersionId &&
489+
wh.deploymentVersionId &&
490+
wh.deploymentVersionId !== deploymentVersionId
491+
) {
492+
// Webhook pinned to a different (older) deployment version. fetchActiveWebhooks
493+
// only polls webhooks whose version matches the active one, so leaving these in
494+
// place strands them as live-but-never-polled orphans. Fold their removal into
495+
// this deploy so a successful run guarantees only the active version has webhooks,
496+
// instead of relying on the separate, best-effort CLEANUP_INACTIVE event.
497+
staleVersionWebhooks.push(wh)
486498
}
487499
}
488500

@@ -508,7 +520,7 @@ export async function saveTriggerWebhooksForDeploy({
508520
}
509521
const webhookConfigs = new Map<string, WebhookConfig>()
510522

511-
const webhooksToDelete: typeof existingWebhooks = []
523+
const webhooksToDelete: typeof existingWebhooks = [...staleVersionWebhooks]
512524
const blocksNeedingWebhook: BlockState[] = []
513525
const blocksNeedingCredentialSetSync: BlockState[] = []
514526

0 commit comments

Comments
 (0)