diff --git a/apps/sim/lib/posthog/server.test.ts b/apps/sim/lib/posthog/server.test.ts new file mode 100644 index 00000000000..ec472a5afcf --- /dev/null +++ b/apps/sim/lib/posthog/server.test.ts @@ -0,0 +1,79 @@ +/** + * @vitest-environment node + */ +import type { MockInstance } from 'vitest' +import { beforeAll, beforeEach, describe, expect, it, vi } from 'vitest' +import { captureServerEvent, getPostHogClient } from '@/lib/posthog/server' + +/** + * This is the guarantee that keeps analytics off every critical path: callers + * treat `captureServerEvent` as something that cannot fail, and several — the + * deployment outbox among them — would turn a PostHog outage into failed work + * if it ever started throwing. + * + * The client is built through a lazy `require`, which `vi.mock` cannot + * intercept, so this spies on the real one. Its readiness latches at module + * level, hence stubbing the env before the first read and asserting a client + * exists — without that the whole suite would pass on a disabled no-op. + */ +describe('captureServerEvent', () => { + let captureSpy: MockInstance + + beforeAll(() => { + vi.stubEnv('NEXT_PUBLIC_POSTHOG_KEY', 'phc_test') + vi.stubEnv('NEXT_PUBLIC_POSTHOG_ENABLED', 'true') + + const client = getPostHogClient() + if (!client) throw new Error('expected an enabled PostHog client to spy on') + captureSpy = vi.spyOn(client, 'capture').mockImplementation(() => {}) + }) + + beforeEach(() => { + captureSpy.mockClear() + captureSpy.mockImplementation(() => {}) + }) + + it('swallows a failing client instead of propagating to the caller', () => { + captureSpy.mockImplementation(() => { + throw new Error('PostHog unreachable') + }) + + expect(() => + captureServerEvent('user-1', 'workflow_deployed', { + workflow_id: 'workflow-1', + workspace_id: 'workspace-1', + }) + ).not.toThrow() + expect(captureSpy).toHaveBeenCalledTimes(1) + }) + + it('captures synchronously, so a caller cannot await delivery', () => { + const result = captureServerEvent('user-1', 'workflow_deployed', { + workflow_id: 'workflow-1', + workspace_id: 'workspace-1', + }) + + expect(result).toBeUndefined() + expect(captureSpy).toHaveBeenCalledTimes(1) + }) + + it('forwards insertId as $insert_id so outbox retries collapse', () => { + captureServerEvent( + 'user-1', + 'workflow_deployed', + { workflow_id: 'workflow-1', workspace_id: 'workspace-1' }, + { insertId: 'event-1', groups: { workspace: 'workspace-1' } } + ) + + expect(captureSpy).toHaveBeenCalledWith( + expect.objectContaining({ + distinctId: 'user-1', + event: 'workflow_deployed', + properties: expect.objectContaining({ + $insert_id: 'event-1', + $groups: { workspace: 'workspace-1' }, + }), + }) + ) + }) +}) diff --git a/apps/sim/lib/posthog/server.ts b/apps/sim/lib/posthog/server.ts index 18274456081..f2e5a828fa1 100644 --- a/apps/sim/lib/posthog/server.ts +++ b/apps/sim/lib/posthog/server.ts @@ -98,22 +98,3 @@ export function captureServerEvent( logger.warn('Failed to capture PostHog server event', { event, error }) } } - -/** Captures and flushes one outbox event before its durable checkpoint advances. */ -export async function deliverOutboxServerEvent( - distinctId: string, - event: E, - properties: PostHogEventMap[E], - options?: CaptureOptions -): Promise<'delivered' | 'skipped'> { - const client = getClient() - if (!client) return 'skipped' - - client.capture({ - distinctId, - event, - properties: buildCaptureProperties(properties, options), - }) - await client.flush() - return 'delivered' -} diff --git a/apps/sim/lib/webhooks/registration-store.test.ts b/apps/sim/lib/webhooks/registration-store.test.ts index 5a0641f3304..b54f67d131b 100644 --- a/apps/sim/lib/webhooks/registration-store.test.ts +++ b/apps/sim/lib/webhooks/registration-store.test.ts @@ -63,6 +63,14 @@ const FENCE: WebhookRegistrationOperationFence = { deploymentVersionId: 'version-3', } +/** The redeploy that lands seconds after {@link FENCE} and supersedes it. */ +const NEXT_FENCE: WebhookRegistrationOperationFence = { + workflowId: 'workflow-1', + operationId: 'operation-2', + generation: 4, + deploymentVersionId: 'version-4', +} + interface UpdateCall { payload: Record condition: Condition @@ -125,6 +133,15 @@ function createTx(selectResults: unknown[][]) { return { tx: tx as unknown as DbOrTx, updates, inserts, updateResults } } +/** Routes `db.transaction` at a queue-driven tx so store writes are observable. */ +function runInTx(selectResults: unknown[][]) { + const harness = createTx(selectResults) + dbChainMockFns.transaction.mockImplementation( + async (callback: (tx: DbOrTx) => Promise) => callback(harness.tx) + ) + return harness +} + function activeRow(overrides: Record = {}) { return { id: 'wh-active', @@ -224,14 +241,6 @@ describe('prepareWebhookRegistrationIntents', () => { }) }) - function runInTx(selectResults: unknown[][]) { - const harness = createTx(selectResults) - dbChainMockFns.transaction.mockImplementation( - async (callback: (tx: DbOrTx) => Promise) => callback(harness.tx) - ) - return harness - } - const desired = { blockId: 'block-1', provider: 'slack', @@ -341,3 +350,67 @@ describe('prepareWebhookRegistrationIntents', () => { expect(updates).toHaveLength(0) }) }) + +describe('redeploys racing within seconds', () => { + beforeEach(() => { + vi.clearAllMocks() + resetDbChainMock() + mockClaimWebhookPath.mockResolvedValue('hooks/a') + dbChainMockFns.transaction.mockImplementation(async () => { + throw new Error('db.transaction not configured for this test') + }) + }) + + const desired = { + blockId: 'block-1', + provider: 'slack', + path: 'hooks/a', + routingKey: null, + providerConfig: { url: 'https://example.test' }, + configFingerprint: 'fp-new', + } + + it('no-ops the superseded attempt and still lands the newer registration', async () => { + mockIsDeploymentOperationCurrent.mockResolvedValue(false) + const superseded = runInTx([[{ id: 'workflow-1' }]]) + + await expect( + prepareWebhookRegistrationIntents({ fence: FENCE, desired: [desired] }) + ).rejects.toBeInstanceOf(StaleWebhookRegistrationOperationError) + expect(superseded.inserts).toHaveLength(0) + expect(superseded.updates).toHaveLength(0) + expect(mockClaimWebhookPath).not.toHaveBeenCalled() + + mockIsDeploymentOperationCurrent.mockResolvedValue(true) + const winner = runInTx([[{ id: 'workflow-1' }], [], [activeRow()], [], []]) + + const work = await prepareWebhookRegistrationIntents({ fence: NEXT_FENCE, desired: [desired] }) + + expect(mockClaimWebhookPath).toHaveBeenCalledWith(expect.anything(), { + path: 'hooks/a', + workflowId: 'workflow-1', + generation: 4, + }) + expect(work.candidates).toHaveLength(1) + expect(winner.inserts).toHaveLength(1) + expect(winner.inserts[0].values).toEqual( + expect.objectContaining({ + registrationStatus: 'candidate', + registrationGeneration: 4, + deploymentVersionId: 'version-4', + }) + ) + + const activation = createTx([[{ id: 'workflow-1' }], [], []]) + await activateWebhookRegistrations(activation.tx, NEXT_FENCE) + + expect(activation.updates[1].payload).toEqual( + expect.objectContaining({ + registrationStatus: 'active', + deploymentVersionId: 'version-4', + isActive: true, + archivedAt: null, + }) + ) + }) +}) diff --git a/apps/sim/lib/workflows/deployment-lifecycle.ts b/apps/sim/lib/workflows/deployment-lifecycle.ts index 1ed98bf590b..d4ed7f51c7b 100644 --- a/apps/sim/lib/workflows/deployment-lifecycle.ts +++ b/apps/sim/lib/workflows/deployment-lifecycle.ts @@ -129,6 +129,13 @@ export function parseDeploymentReadiness(value: unknown): DeploymentReadiness | export const DEPLOYMENT_ERROR_CODES = { webhookPathConflict: 'webhook_path_conflict', invalidTriggerConfiguration: 'invalid_trigger_configuration', + /** + * A newer generation took over the workflow while this attempt was running. + * Never a failure — the newer attempt owns the outcome — so it is neither + * persisted on the operation nor counted as non-retryable; it exists to give + * the benign hand-off a greppable identity in logs. + */ + operationSuperseded: 'deployment_operation_superseded', } as const const NON_RETRYABLE_DEPLOYMENT_ERROR_CODES = new Set([ diff --git a/apps/sim/lib/workflows/deployment-outbox.test.ts b/apps/sim/lib/workflows/deployment-outbox.test.ts index eee9c74f65a..4483575af30 100644 --- a/apps/sim/lib/workflows/deployment-outbox.test.ts +++ b/apps/sim/lib/workflows/deployment-outbox.test.ts @@ -75,7 +75,7 @@ vi.mock('@/lib/mcp/server-locks', () => ({ })) vi.mock('@/lib/posthog/server', () => ({ - deliverOutboxServerEvent: mockCaptureServerEvent, + captureServerEvent: mockCaptureServerEvent, })) vi.mock('@/lib/mcp/workflow-mcp-sync', () => ({ @@ -212,7 +212,7 @@ describe('versioned deployment preparation outbox', () => { mockSyncMcpToolsForWorkflow.mockResolvedValue([{ serverId: 'mcp-server-1' }]) mockSetWorkflowMcpTransactionLockTimeout.mockResolvedValue(undefined) mockEmitWorkflowDeployedEvent.mockResolvedValue(undefined) - mockCaptureServerEvent.mockResolvedValue('delivered') + mockCaptureServerEvent.mockReturnValue(undefined) mockMarkDeploymentOperationFailed.mockResolvedValue({ success: true, operation: operation({ status: 'failed' }), @@ -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: { @@ -317,6 +319,11 @@ describe('versioned deployment preparation outbox', () => { mockActivateDeploymentOperation.mock.invocationCallOrder[0] ) + /** + * The resume re-enters post-activation work, so the checkpoints — not the + * generation fence — are what must keep analytics from being captured + * twice. + */ mockGetDeploymentOperation.mockResolvedValue(active) queueTableRows(schemaMock.workflow, [ { id: 'workflow-1', name: 'Workflow', workspaceId: 'workspace-1' }, @@ -348,14 +355,20 @@ describe('versioned deployment preparation outbox', () => { expect(mockActivateDeploymentOperation).not.toHaveBeenCalled() }) - it('does not checkpoint analytics until durable PostHog delivery resolves', async () => { - const active = operation({ status: 'active', completedAt: NOW }) - mockGetDeploymentOperation.mockResolvedValue(active) + /** + * Analytics was briefly flushed durably here, which put a deploy's audit + * trail, socket notification, and subscription cleanup behind PostHog and + * retried the event until it dead-lettered. Capture is fire-and-forget + * again: the checkpoint advances on capture, and everything the cutover + * actually owes still runs. `captureServerEvent` swallowing its own + * failures is pinned in `lib/posthog/server.test.ts`. + */ + it('checkpoints analytics on capture and still finishes the deploy', async () => { + mockIsDeploymentOperationCurrent.mockResolvedValue(true) + mockGetDeploymentOperation.mockResolvedValue(operation({ status: 'active', completedAt: NOW })) queueTableRows(schemaMock.workflow, [ { id: 'workflow-1', name: 'Workflow', workspaceId: 'workspace-1' }, ]) - const deliveryFailure = new Error('PostHog flush failed') - mockCaptureServerEvent.mockRejectedValueOnce(deliveryFailure) const outboxContext = context() await expect( @@ -366,13 +379,16 @@ describe('versioned deployment preparation outbox', () => { }, outboxContext ) - ).rejects.toBe(deliveryFailure) + ).resolves.toBeUndefined() - expect(outboxContext.checkpointPayload).not.toHaveBeenCalledWith( + expect(mockCaptureServerEvent).toHaveBeenCalledTimes(1) + expect(outboxContext.checkpointPayload).toHaveBeenCalledWith( expect.objectContaining({ checkpoints: expect.objectContaining({ analyticsCaptured: true }), }) ) + expect(mockEmitWorkflowDeployedEvent).toHaveBeenCalledTimes(1) + expect(mockCleanupRetiredWebhookRegistrations).toHaveBeenCalledTimes(1) }) it('honors an aborted signal before starting any side effect', async () => { @@ -524,6 +540,85 @@ describe('versioned deployment preparation outbox', () => { expect(mockActivateDeploymentOperation).not.toHaveBeenCalled() }) + /** + * The production shape: an attempt activates, its post-activation phase is + * interrupted (handler timeout), and a redeploy lands before the reaper + * 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 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' }, + ]) + mockCleanupRetiredWebhookRegistrations.mockRejectedValue( + new Error('Webhook registration operation is stale') + ) + + await expect(handler()(payload(), context(new AbortController(), 3))).resolves.toBeUndefined() + + expect(mockCleanupRetiredWebhookRegistrations).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 })) + queueTableRows(schemaMock.workflow, [ + { id: 'workflow-1', name: 'Workflow', workspaceId: 'workspace-1' }, + ]) + + await handler()(payload(), context()) + + expect(mockCleanupRetiredWebhookRegistrations).toHaveBeenCalledTimes(1) + expect(mockRecordAudit).toHaveBeenCalledTimes(1) + expect(mockEmitWorkflowDeployedEvent).toHaveBeenCalledTimes(1) + }) + + /** + * Retiring the previous generation's provider subscriptions is the slowest + * step after cutover; a deploy that already went live must not lose its + * audit trail or its "deployment changed" notification when that step fails. + */ + it('records and notifies an activated deploy before retiring old subscriptions', async () => { + mockIsDeploymentOperationCurrent.mockResolvedValue(true) + mockGetDeploymentOperation.mockResolvedValue(operation({ status: 'active', completedAt: NOW })) + queueTableRows(schemaMock.workflow, [ + { id: 'workflow-1', name: 'Workflow', workspaceId: 'workspace-1' }, + ]) + mockCleanupRetiredWebhookRegistrations.mockRejectedValue(new Error('provider unavailable')) + + await expect(handler()(payload(), context())).rejects.toThrow('provider unavailable') + + expect(mockRecordAudit).toHaveBeenCalledTimes(1) + expect(mockCaptureServerEvent).toHaveBeenCalledTimes(1) + expect(mockEmitWorkflowDeployedEvent).toHaveBeenCalledTimes(1) + expect(mockRecordAudit.mock.invocationCallOrder[0]).toBeLessThan( + mockCleanupRetiredWebhookRegistrations.mock.invocationCallOrder[0] + ) + }) + it('keeps v1 cleanup from deleting a candidate owned by the current v2 operation', async () => { queueTableRows(schemaMock.workflow, [ { id: 'workflow-1', name: 'Workflow', workspaceId: 'workspace-1' }, diff --git a/apps/sim/lib/workflows/deployment-outbox.ts b/apps/sim/lib/workflows/deployment-outbox.ts index eff2d1db51c..62875063496 100644 --- a/apps/sim/lib/workflows/deployment-outbox.ts +++ b/apps/sim/lib/workflows/deployment-outbox.ts @@ -22,7 +22,7 @@ import { removeMcpToolsForWorkflow, syncMcpToolsForWorkflow, } from '@/lib/mcp/workflow-mcp-sync' -import { deliverOutboxServerEvent } from '@/lib/posthog/server' +import { captureServerEvent } from '@/lib/posthog/server' import { cleanupWebhooksForWorkflow, prepareStableTriggerWebhooksForDeploy, @@ -329,19 +329,16 @@ async function prepareDeploymentOperation( } if (operation.status === 'active') { - await cleanupRetiredWebhooksForOperation({ - payload, - workflow: workflowRecord as Record, - context, - }) - await cleanupInactiveDeploymentsForOperation({ - payload, - workflow: workflowRecord as Record, - checkpoints, - checkpoint, - context, - }) - await emitPostActivationSideEffects({ + /** + * 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. + */ + await runPostActivationWork({ payload, operation, workflow: workflowRecord as Record, @@ -491,19 +488,7 @@ async function prepareDeploymentOperation( notifyMcpToolServers(affectedMcpServers) context.signal.throwIfAborted() - await cleanupRetiredWebhooksForOperation({ - payload, - workflow: workflowRecord as Record, - context, - }) - await cleanupInactiveDeploymentsForOperation({ - payload, - workflow: workflowRecord as Record, - checkpoints, - checkpoint, - context, - }) - await emitPostActivationSideEffects({ + await runPostActivationWork({ payload, operation, workflow: workflowRecord as Record, @@ -513,6 +498,49 @@ async function prepareDeploymentOperation( }) } +/** + * Runs everything that follows a committed cutover — notifications first. + * + * The ordering is load-bearing. The audit entry, analytics event, socket + * notification, and workspace event all describe an activation that is + * already durable, and each is individually checkpointed. Retiring the + * previous generation's external subscriptions is best-effort cleanup that + * makes one provider call per retired row and is by far the slowest, most + * failure-prone step here. Running cleanup first put every one of those + * notifications behind it, so a single flaky provider — or the handler + * 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 + operation: WorkflowDeploymentOperation + workflow: Record + checkpoints: DeploymentPreparationCheckpoints + checkpoint: (patch: Partial) => Promise + context: OutboxEventContext +}): Promise { + await emitPostActivationSideEffects(params) + await cleanupRetiredWebhooksForOperation({ + payload: params.payload, + workflow: params.workflow, + context: params.context, + }) + await cleanupInactiveDeploymentsForOperation({ + payload: params.payload, + workflow: params.workflow, + checkpoints: params.checkpoints, + checkpoint: params.checkpoint, + context: params.context, + }) +} + async function prepareReadinessComponent(params: { payload: PrepareDeploymentV2Payload operation: WorkflowDeploymentOperation @@ -559,13 +587,35 @@ async function cleanupRetiredWebhooksForOperation(params: { context: OutboxEventContext }): Promise { 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, @@ -642,12 +692,23 @@ async function emitPostActivationSideEffects(params: { await params.checkpoint({ auditEmitted: true }) } + /** + * Analytics is fire-and-forget by contract: PostHog being unreachable must + * never fail an activation that is already durable. Awaiting a flush here + * bought no delivery the process does not already have — the client flushes + * on its own interval and again from the `SIGTERM`/`SIGINT` hook in + * `instrumentation-node.ts` — while holding the socket notification, the + * workspace event, and subscription cleanup behind a third party, and + * failing the outbox event until it dead-lettered when that party was down. + * `flush()` also drains the whole shared client queue, so an unrelated + * event's network error surfaced here as a failed deploy. + */ if (!params.checkpoints.analyticsCaptured) { params.context.signal.throwIfAborted() if (params.payload.captureAnalytics !== false) { const workspaceId = (params.workflow.workspaceId as string) || '' const isVersionActivation = params.operation.action === 'activate' - await deliverOutboxServerEvent( + captureServerEvent( params.payload.userId, isVersionActivation ? 'deployment_version_activated' : 'workflow_deployed', {