fix(deployments): stop superseded activations from dead-lettering - #6522
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
PR SummaryMedium Risk Overview Fenced cleanup is now a no-op when a newer generation owns the workflow ( Post-activation order is reversed via shared Analytics is fire-and-forget again: Reviewed by Cursor Bugbot for commit def9273. Configure here. |
Greptile SummaryThe PR prevents superseded deployment activations from repeatedly failing generation-fenced cleanup and moves durable cutover notifications ahead of failure-prone external cleanup.
Confidence Score: 5/5The PR appears safe to merge. No blocking failure remains.
|
| Filename | Overview |
|---|---|
| apps/sim/lib/workflows/deployment-outbox.ts | Centralizes post-activation work, emits checkpointed notifications before cleanup, and skips generation-fenced cleanup for superseded activations. |
| apps/sim/lib/workflows/deployment-outbox.test.ts | Adds regression coverage for superseded resumes, current-generation resumes, notification ordering, and fire-and-forget analytics. |
| apps/sim/lib/posthog/server.ts | Removes the outbox-specific flush helper in favor of the existing non-throwing synchronous capture contract. |
| apps/sim/lib/posthog/server.test.ts | Pins synchronous capture, error swallowing, and PostHog insert-ID forwarding. |
| apps/sim/lib/workflows/deployment-lifecycle.ts | Adds a log-oriented error-code identity for benign operation supersession. |
| apps/sim/lib/webhooks/registration-store.test.ts | Adds coverage showing a stale deployment cannot write registrations while the succeeding generation can prepare and activate them. |
Sequence Diagram
sequenceDiagram
participant O as Deployment Outbox
participant N as Post-activation Notifications
participant G as Generation Fence
participant W as Webhook Cleanup
O->>N: Emit checkpointed cutover side effects
N-->>O: Audit, analytics, socket, workspace event
O->>G: Is this active operation current?
alt Current generation
G-->>O: Yes
O->>W: Retire previous webhook registrations
else Superseded generation
G-->>O: No
O-->>O: Complete cleanup as a no-op
end
Reviews (4): Last reviewed commit: "fix(deployments): fence the cleanup, not..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
|
@cursor review |
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit 8fc4c4c. Configure here.
29 workflow.deployment.prepare.v2 events dead-lettered with "Webhook registration operation is stale", every one at attempts = max_attempts. A full retry budget means the failure is deterministic, which rules out the preparation path: an attempt superseded while preparing is marked superseded, so its next attempt short-circuits at the top of the handler and completes. The branch a retry re-enters is the other one. isTerminalNonActiveOperation covers failed and superseded but not active, so an attempt that activated and was then superseded by the next deploy keeps its own active status, re-enters post-activation work on every retry, and re-fails the same generation fence until the event dies. The fence it fails is correct — it takes the same workflow row lock the generation bump takes, and compares generations exactly — so nothing about the detection is racy; only the reaction to it was wrong. Reaching it needs a handler timeout, which parks the row for the 10-minute reaper instead of the 2s/4s/8s backoff, opening a window wide enough for a redeploy to land. Gate the resume branch on the operation still owning the current generation, matching the sibling cleanup that already does this, and complete the event as a no-op when it does not. The newer generation adopts the leftover work anyway: it collects every retired registration below its own fence. Also reverse the post-activation order. The audit entry, analytics event, socket notification, and workspace event describe a cutover that is already durable, and each is separately checkpointed, but they ran behind retiring the previous generation's external subscriptions — one provider call per retired row, and by far the most failure-prone step there. A single flaky provider silently cost the deploy its audit trail and left clients on the old version until something else refreshed them. Both call sites now share one helper so the order cannot drift apart again. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
8fc4c4c to
c6c5f56
Compare
icecrasher321
changed the title
|
Dropped the billing reconciler from this PR — the root cause it backstopped is already fixed by #6510, and the three affected users need a one-time backfill rather than a permanent sweep. This PR is now deployment-only: 4 files, +212/-33. |
|
@cursor review |
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit c6c5f56. Configure here.
…eneration Two tests that staging added alongside durable PostHog delivery drive the `operation.status === 'active'` resume branch this PR now gates, and neither overrides `mockIsDeploymentOperationCurrent` — the suite's `beforeEach` defaults it to `false`. Under the new gate they read as superseded, so the handler short-circuits: the delivery test sees a resolve where it asserts a rejection, and the unconsumed workflow row leaks into the next test. Both are still-current resumes by intent, so they say so explicitly. The checkpoints, not the generation gate, remain what keeps analytics from being captured twice. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
bugbot run |
`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>
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>
|
@cursor review |
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit def9273. Configure here.
1 participant
What
29
workflow.deployment.prepare.v2outbox events dead-lettered withWebhook registration operation is stale, spanning 2026-07-21 → 2026-08-09 across 13 workflows. Every one sat atattempts = max_attempts.That full retry budget is the tell: the failure is deterministic, which rules out the preparation path. An attempt superseded while preparing is marked
superseded, so its next attempt short-circuits at the top of the handler and completes — it would die atattempts = 1, never 4.The branch a retry re-enters is the other one.
isTerminalNonActiveOperationcoversfailedandsupersededbut notactive, so an attempt that activated and was then superseded by the next deploy keeps its ownactivestatus, re-enters post-activation work on every retry, and re-fails the same generation fence until the event dies.The fence it fails is correct — it takes the same workflow row lock the generation bump takes, and compares generations exactly. Nothing about the detection is racy; only the reaction to it was wrong. Reaching it at all needs a handler timeout, which parks the row for the 10-minute reaper instead of the 2s/4s/8s backoff, opening a window wide enough for a redeploy to land. That is why the same workflow dominates both this class and the
Outbox handler timed out after 90000msclass.Changes
Gate the resume branch on still owning the current generation, matching the sibling cleanup that already does this, and complete the event as a no-op when it does not. The newer generation adopts the leftover work anyway — it collects every retired registration below its own fence, so nothing leaks.
Reverse the post-activation order. The audit entry, analytics event, socket notification, and workspace event describe a cutover that is already durable, and each is separately checkpointed — but they ran behind retiring the previous generation's external subscriptions, one provider call per retired row and by far the most failure-prone step there. A single flaky provider silently cost the deploy its audit trail and left clients on the old version until something else refreshed them. Both call sites now share one helper so the order cannot drift apart again.
Notes
runInTxhelper to module scope for a new describe block.🤖 Generated with Claude Code