Skip to content

Commit 60866a8

Browse files
icecrasher321claude
andcommitted
fix(analytics): stop PostHog delivery from failing a deploy
`deliverOutboxServerEvent` awaited `client.flush()` before letting the deployment outbox checkpoint advance, so an unreachable PostHog failed the event, retried it, and eventually dead-lettered it — while holding the socket notification, the workspace event, and retired-subscription cleanup behind a third party. `flush()` also drains the whole shared client queue, so an unrelated event's network error surfaced here as a failed deploy. It bought no durability the process did not already have: the outbox handler runs in the long-lived app container, where the client flushes on its own 10s interval and again from the `SIGTERM`/`SIGINT` hook in `instrumentation-node.ts`. The helper had one caller and arrived inside an unrelated squashed PR (#5273) with no rationale, against 161 fire-and-forget `captureServerEvent` call sites. Deleted it and restored `captureServerEvent`, whose contract is already "never throws". `insertId` still collapses retried captures. That contract was untested, which is why this regressed unnoticed, so `server.test.ts` now pins it — spying on the real client, since the lazy `require` defeats `vi.mock` and a disabled client would pass every assertion vacuously. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
1 parent 207a8b2 commit 60866a8

4 files changed

Lines changed: 109 additions & 30 deletions

File tree

Lines changed: 79 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,79 @@
1+
/**
2+
* @vitest-environment node
3+
*/
4+
import type { MockInstance } from 'vitest'
5+
import { beforeAll, beforeEach, describe, expect, it, vi } from 'vitest'
6+
import { captureServerEvent, getPostHogClient } from '@/lib/posthog/server'
7+
8+
/**
9+
* This is the guarantee that keeps analytics off every critical path: callers
10+
* treat `captureServerEvent` as something that cannot fail, and several — the
11+
* deployment outbox among them — would turn a PostHog outage into failed work
12+
* if it ever started throwing.
13+
*
14+
* The client is built through a lazy `require`, which `vi.mock` cannot
15+
* intercept, so this spies on the real one. Its readiness latches at module
16+
* level, hence stubbing the env before the first read and asserting a client
17+
* exists — without that the whole suite would pass on a disabled no-op.
18+
*/
19+
describe('captureServerEvent', () => {
20+
let captureSpy: MockInstance
21+
22+
beforeAll(() => {
23+
vi.stubEnv('NEXT_PUBLIC_POSTHOG_KEY', 'phc_test')
24+
vi.stubEnv('NEXT_PUBLIC_POSTHOG_ENABLED', 'true')
25+
26+
const client = getPostHogClient()
27+
if (!client) throw new Error('expected an enabled PostHog client to spy on')
28+
captureSpy = vi.spyOn(client, 'capture').mockImplementation(() => {})
29+
})
30+
31+
beforeEach(() => {
32+
captureSpy.mockClear()
33+
captureSpy.mockImplementation(() => {})
34+
})
35+
36+
it('swallows a failing client instead of propagating to the caller', () => {
37+
captureSpy.mockImplementation(() => {
38+
throw new Error('PostHog unreachable')
39+
})
40+
41+
expect(() =>
42+
captureServerEvent('user-1', 'workflow_deployed', {
43+
workflow_id: 'workflow-1',
44+
workspace_id: 'workspace-1',
45+
})
46+
).not.toThrow()
47+
expect(captureSpy).toHaveBeenCalledTimes(1)
48+
})
49+
50+
it('captures synchronously, so a caller cannot await delivery', () => {
51+
const result = captureServerEvent('user-1', 'workflow_deployed', {
52+
workflow_id: 'workflow-1',
53+
workspace_id: 'workspace-1',
54+
})
55+
56+
expect(result).toBeUndefined()
57+
expect(captureSpy).toHaveBeenCalledTimes(1)
58+
})
59+
60+
it('forwards insertId as $insert_id so outbox retries collapse', () => {
61+
captureServerEvent(
62+
'user-1',
63+
'workflow_deployed',
64+
{ workflow_id: 'workflow-1', workspace_id: 'workspace-1' },
65+
{ insertId: 'event-1', groups: { workspace: 'workspace-1' } }
66+
)
67+
68+
expect(captureSpy).toHaveBeenCalledWith(
69+
expect.objectContaining({
70+
distinctId: 'user-1',
71+
event: 'workflow_deployed',
72+
properties: expect.objectContaining({
73+
$insert_id: 'event-1',
74+
$groups: { workspace: 'workspace-1' },
75+
}),
76+
})
77+
)
78+
})
79+
})

apps/sim/lib/posthog/server.ts

Lines changed: 0 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -98,22 +98,3 @@ export function captureServerEvent<E extends PostHogEventName>(
9898
logger.warn('Failed to capture PostHog server event', { event, error })
9999
}
100100
}
101-
102-
/** Captures and flushes one outbox event before its durable checkpoint advances. */
103-
export async function deliverOutboxServerEvent<E extends PostHogEventName>(
104-
distinctId: string,
105-
event: E,
106-
properties: PostHogEventMap[E],
107-
options?: CaptureOptions
108-
): Promise<'delivered' | 'skipped'> {
109-
const client = getClient()
110-
if (!client) return 'skipped'
111-
112-
client.capture({
113-
distinctId,
114-
event,
115-
properties: buildCaptureProperties(properties, options),
116-
})
117-
await client.flush()
118-
return 'delivered'
119-
}

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

Lines changed: 17 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -75,7 +75,7 @@ vi.mock('@/lib/mcp/server-locks', () => ({
7575
}))
7676

7777
vi.mock('@/lib/posthog/server', () => ({
78-
deliverOutboxServerEvent: mockCaptureServerEvent,
78+
captureServerEvent: mockCaptureServerEvent,
7979
}))
8080

8181
vi.mock('@/lib/mcp/workflow-mcp-sync', () => ({
@@ -212,7 +212,7 @@ describe('versioned deployment preparation outbox', () => {
212212
mockSyncMcpToolsForWorkflow.mockResolvedValue([{ serverId: 'mcp-server-1' }])
213213
mockSetWorkflowMcpTransactionLockTimeout.mockResolvedValue(undefined)
214214
mockEmitWorkflowDeployedEvent.mockResolvedValue(undefined)
215-
mockCaptureServerEvent.mockResolvedValue('delivered')
215+
mockCaptureServerEvent.mockReturnValue(undefined)
216216
mockMarkDeploymentOperationFailed.mockResolvedValue({
217217
success: true,
218218
operation: operation({ status: 'failed' }),
@@ -354,15 +354,20 @@ describe('versioned deployment preparation outbox', () => {
354354
expect(mockActivateDeploymentOperation).not.toHaveBeenCalled()
355355
})
356356

357-
it('does not checkpoint analytics until durable PostHog delivery resolves', async () => {
357+
/**
358+
* Analytics was briefly flushed durably here, which put a deploy's audit
359+
* trail, socket notification, and subscription cleanup behind PostHog and
360+
* retried the event until it dead-lettered. Capture is fire-and-forget
361+
* again: the checkpoint advances on capture, and everything the cutover
362+
* actually owes still runs. `captureServerEvent` swallowing its own
363+
* failures is pinned in `lib/posthog/server.test.ts`.
364+
*/
365+
it('checkpoints analytics on capture and still finishes the deploy', async () => {
358366
mockIsDeploymentOperationCurrent.mockResolvedValue(true)
359-
const active = operation({ status: 'active', completedAt: NOW })
360-
mockGetDeploymentOperation.mockResolvedValue(active)
367+
mockGetDeploymentOperation.mockResolvedValue(operation({ status: 'active', completedAt: NOW }))
361368
queueTableRows(schemaMock.workflow, [
362369
{ id: 'workflow-1', name: 'Workflow', workspaceId: 'workspace-1' },
363370
])
364-
const deliveryFailure = new Error('PostHog flush failed')
365-
mockCaptureServerEvent.mockRejectedValueOnce(deliveryFailure)
366371
const outboxContext = context()
367372

368373
await expect(
@@ -373,13 +378,16 @@ describe('versioned deployment preparation outbox', () => {
373378
},
374379
outboxContext
375380
)
376-
).rejects.toBe(deliveryFailure)
381+
).resolves.toBeUndefined()
377382

378-
expect(outboxContext.checkpointPayload).not.toHaveBeenCalledWith(
383+
expect(mockCaptureServerEvent).toHaveBeenCalledTimes(1)
384+
expect(outboxContext.checkpointPayload).toHaveBeenCalledWith(
379385
expect.objectContaining({
380386
checkpoints: expect.objectContaining({ analyticsCaptured: true }),
381387
})
382388
)
389+
expect(mockEmitWorkflowDeployedEvent).toHaveBeenCalledTimes(1)
390+
expect(mockCleanupRetiredWebhookRegistrations).toHaveBeenCalledTimes(1)
383391
})
384392

385393
it('honors an aborted signal before starting any side effect', async () => {

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

Lines changed: 13 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -22,7 +22,7 @@ import {
2222
removeMcpToolsForWorkflow,
2323
syncMcpToolsForWorkflow,
2424
} from '@/lib/mcp/workflow-mcp-sync'
25-
import { deliverOutboxServerEvent } from '@/lib/posthog/server'
25+
import { captureServerEvent } from '@/lib/posthog/server'
2626
import {
2727
cleanupWebhooksForWorkflow,
2828
prepareStableTriggerWebhooksForDeploy,
@@ -681,12 +681,23 @@ async function emitPostActivationSideEffects(params: {
681681
await params.checkpoint({ auditEmitted: true })
682682
}
683683

684+
/**
685+
* Analytics is fire-and-forget by contract: PostHog being unreachable must
686+
* never fail an activation that is already durable. Awaiting a flush here
687+
* bought no delivery the process does not already have — the client flushes
688+
* on its own interval and again from the `SIGTERM`/`SIGINT` hook in
689+
* `instrumentation-node.ts` — while holding the socket notification, the
690+
* workspace event, and subscription cleanup behind a third party, and
691+
* failing the outbox event until it dead-lettered when that party was down.
692+
* `flush()` also drains the whole shared client queue, so an unrelated
693+
* event's network error surfaced here as a failed deploy.
694+
*/
684695
if (!params.checkpoints.analyticsCaptured) {
685696
params.context.signal.throwIfAborted()
686697
if (params.payload.captureAnalytics !== false) {
687698
const workspaceId = (params.workflow.workspaceId as string) || ''
688699
const isVersionActivation = params.operation.action === 'activate'
689-
await deliverOutboxServerEvent(
700+
captureServerEvent(
690701
params.payload.userId,
691702
isVersionActivation ? 'deployment_version_activated' : 'workflow_deployed',
692703
{

0 commit comments

Comments
 (0)