fix(deployments): fence the cleanup, not the notifications

The resume guard sat at the top of the `active` branch, so it skipped every
post-activation step whenever `isDeploymentOperationCurrent` went false. That
predicate goes false as soon as any newer generation row exists — including one
still `preparing` or already `failed` — and in that window this activation is
still the live cutover. Nothing newer would ever adopt its audit entry,
analytics event, socket notification, or workspace event, so the guard
permanently dropped them and completed the outbox event as if they were owed to
someone else.

Only the two cleanups are generation-fenced. `cleanupInactiveDeploymentsForOperation`
already gated itself on that exact predicate and returned quietly; the retired
webhook cleanup was the one that instead let the store's `assertCurrentOperation`
throw. It now carries the same guard, on the same fence the store asserts —
`deploymentVersionId` and `statuses: ['active']` included, so passing the gate
actually implies passing the assert. The notifications run unconditionally,
still idempotent through their checkpoints.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

Author: icecrasher321

apps/sim/lib/workflows/deployment-outbox.test.ts

@@ -222,6 +222,8 @@ describe('versioned deployment preparation outbox', () => {
   })
 
   it('activates only after every preparation component is ready', async () => {
+    /** Nothing newer has been enqueued, so this deploy owns its generation. */
+    mockIsDeploymentOperationCurrent.mockResolvedValue(true)
     const preparing = operation()
     const webhooksReady = operation({
       componentReadiness: {
@@ -318,11 +320,10 @@ describe('versioned deployment preparation outbox', () => {
     )
 
     /**
-     * The resume still owns the latest generation, so it re-enters
-     * post-activation work and the checkpoints — not the generation gate —
-     * are what must keep analytics from being captured twice.
+     * The resume re-enters post-activation work, so the checkpoints — not the
+     * generation fence — are what must keep analytics from being captured
+     * twice.
      */
-    mockIsDeploymentOperationCurrent.mockResolvedValue(true)
     mockGetDeploymentOperation.mockResolvedValue(active)
     queueTableRows(schemaMock.workflow, [
       { id: 'workflow-1', name: 'Workflow', workspaceId: 'workspace-1' },
@@ -545,7 +546,7 @@ describe('versioned deployment preparation outbox', () => {
    * requeues it. Every resumed attempt then re-fails the same generation
    * fence, so without the guard it exhausts the retry budget and dead-letters.
    */
-  it('skips post-activation work once a newer deploy supersedes an activated attempt', async () => {
+  it('skips the fenced cleanup once a newer deploy supersedes an activated attempt', async () => {
     mockGetDeploymentOperation.mockResolvedValue(operation({ status: 'active', completedAt: NOW }))
     queueTableRows(schemaMock.workflow, [
       { id: 'workflow-1', name: 'Workflow', workspaceId: 'workspace-1' },
@@ -557,11 +558,30 @@ describe('versioned deployment preparation outbox', () => {
     await expect(handler()(payload(), context(new AbortController(), 3))).resolves.toBeUndefined()
 
     expect(mockCleanupRetiredWebhookRegistrations).not.toHaveBeenCalled()
-    expect(mockRecordAudit).not.toHaveBeenCalled()
     expect(mockMarkDeploymentOperationFailed).not.toHaveBeenCalled()
     expect(mockRecordDeploymentOperationRetry).not.toHaveBeenCalled()
   })
 
+  /**
+   * `isDeploymentOperationCurrent` goes false the moment any newer generation
+   * row exists, including one still `preparing` or already `failed`. This
+   * activation is the live cutover in that window and no newer attempt will
+   * adopt its notifications, so the fence must cost it only the cleanup.
+   */
+  it('still notifies when the newer generation has not activated', async () => {
+    mockGetDeploymentOperation.mockResolvedValue(operation({ status: 'active', completedAt: NOW }))
+    queueTableRows(schemaMock.workflow, [
+      { id: 'workflow-1', name: 'Workflow', workspaceId: 'workspace-1' },
+    ])
+
+    await expect(handler()(payload(), context())).resolves.toBeUndefined()
+
+    expect(mockRecordAudit).toHaveBeenCalledTimes(1)
+    expect(mockCaptureServerEvent).toHaveBeenCalledTimes(1)
+    expect(mockEmitWorkflowDeployedEvent).toHaveBeenCalledTimes(1)
+    expect(mockCleanupRetiredWebhookRegistrations).not.toHaveBeenCalled()
+  })
+
   it('resumes post-activation work while the activated attempt is still current', async () => {
     mockIsDeploymentOperationCurrent.mockResolvedValue(true)
     mockGetDeploymentOperation.mockResolvedValue(operation({ status: 'active', completedAt: NOW }))

apps/sim/lib/workflows/deployment-outbox.ts

@@ -330,31 +330,14 @@ async function prepareDeploymentOperation(
 
   if (operation.status === 'active') {
     /**
-     * Resuming an attempt that already activated: every remaining step is
-     * fenced to this generation, so once a newer one exists they can only
-     * fail, identically, on every retry until the event dead-letters. The
-     * terminal short circuit above cannot catch this — a superseded-after-
-     * activation attempt keeps its own `active` status — and the newer
-     * generation adopts the leftover work anyway, retired registrations
-     * included (it collects every retired row below its own fence).
+     * Resuming an attempt that already activated. The terminal short circuit
+     * above cannot catch this case — a superseded-after-activation attempt
+     * keeps its own `active` status — so the generation fence is applied per
+     * step inside {@link runPostActivationWork} rather than here: the
+     * notifications describe a cutover that really happened and stay owed
+     * whatever else has started since, while only the fenced cleanup is
+     * skipped.
      */
-    context.signal.throwIfAborted()
-    const isCurrent = await isDeploymentOperationCurrent({
-      workflowId: payload.workflowId,
-      operationId: payload.operationId,
-      generation: payload.generation,
-    })
-    context.signal.throwIfAborted()
-    if (!isCurrent) {
-      logger.info('Skipping post-activation work for a superseded generation', {
-        workflowId: payload.workflowId,
-        operationId: payload.operationId,
-        generation: payload.generation,
-        errorCode: DEPLOYMENT_ERROR_CODES.operationSuperseded,
-      })
-      return
-    }
-
     await runPostActivationWork({
       payload,
       operation,
@@ -528,6 +511,12 @@ async function prepareDeploymentOperation(
  * timeout its latency burns through — silently cost the deploy its audit
  * trail and left clients on the old version until something else refreshed
  * them. Nothing below depends on the cleanup having run.
+ *
+ * It also decides where the generation fence goes. Both cleanups carry their
+ * own, because only they are fenced; the notifications are not, and gating
+ * them on the same predicate would drop them for good in the window where a
+ * newer generation exists but has not activated — this activation is still
+ * the live one there, and nothing else will emit them.
  */
 async function runPostActivationWork(params: {
   payload: PrepareDeploymentV2Payload
@@ -598,13 +587,35 @@ async function cleanupRetiredWebhooksForOperation(params: {
   context: OutboxEventContext
 }): Promise<void> {
   params.context.signal.throwIfAborted()
-  await cleanupRetiredWebhookRegistrationsAfterActivation({
-    fence: {
+  const fence = {
+    workflowId: params.payload.workflowId,
+    operationId: params.payload.operationId,
+    generation: params.payload.generation,
+    deploymentVersionId: params.payload.deploymentVersionId,
+  }
+
+  /**
+   * Gated exactly like {@link cleanupInactiveDeploymentsForOperation} below,
+   * and on the same predicate the store asserts internally — the store throws
+   * where this returns, so a superseded attempt would otherwise fail here
+   * identically on every retry until the event dead-lettered. Skipping loses
+   * nothing: a newer generation collects every retired row below its own
+   * fence, this one included.
+   */
+  const isCurrent = await isDeploymentOperationCurrent({ ...fence, statuses: ['active'] })
+  params.context.signal.throwIfAborted()
+  if (!isCurrent) {
+    logger.info('Skipping retired webhook cleanup for a superseded generation', {
       workflowId: params.payload.workflowId,
       operationId: params.payload.operationId,
       generation: params.payload.generation,
-      deploymentVersionId: params.payload.deploymentVersionId,
-    },
+      errorCode: DEPLOYMENT_ERROR_CODES.operationSuperseded,
+    })
+    return
+  }
+
+  await cleanupRetiredWebhookRegistrationsAfterActivation({
+    fence,
     workflow: params.workflow,
     requestId: params.payload.requestId,
     signal: params.context.signal,