Uh oh!
There was an error while loading. Please reload this page.
fix(world-postgres): skip parked runs and dedupe recovery jobs on startup - #3162
fix(world-postgres): skip parked runs and dedupe recovery jobs on startup#3162Mohith26 wants to merge 2 commits into
Conversation
…very Startup recovery previously re-enqueued every pending/running run via the generic reenqueueActiveRuns helper, replaying runs that are durably parked on unresolved hooks or not-yet-due waits, and minting a fresh graphile job key per boot so repeated restarts accumulated duplicate outstanding jobs. Replace it with a Postgres-specific reenqueueRecoverableRuns that: - classifies running runs against persisted state (steps/waits/hooks) and skips runs whose only live state is open hooks and/or waits that are not due yet - their wake-up jobs live durably in the same database and survive restarts - still recovers runs with interrupted (non-terminal) step work, due waits, and unclassifiable runs with no persisted suspension state (fail open) - attaches a stable per-run idempotency key (startup-recovery:<runId>), which the queue uses as the graphile-worker job_key, so repeated boots replace the outstanding recovery job instead of adding another, without suppressing later legitimate wakes Fixesvercel#3119
Add unit tests for reenqueueRecoverableRuns (hook-parked, future-wait and indefinite-wait runs skipped; due waits, interrupted steps, pending and ambiguous runs recovered; stable idempotency key across repeated boots; fail-open on classification errors) and extend the createWorld() startup tests to assert parked runs are skipped end-to-end and that the recovery enqueue uses the stable startup-recovery:<runId> graphile job key on every boot.
🦋 Changeset detectedLatest commit: 9603206 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
@Mohith26 is attempting to deploy a commit to the Vercel Labs Team on Vercel. A member of the Team first needs to authorize it. |
VaguelySerious
commented
Jul 31, 2026
@Mohith26 We require commits to be signed. Could you squash+sign the PR and force-push the branch? Separate review following soon. |
VaguelySerious
left a comment
There was a problem hiding this comment.
AI review: blocking issues found
| runnable.add(runId); | ||
| continue; | ||
| } | ||
| if (openHookRunIds.has(runId) || waitingRunIds.has(runId)) { |
There was a problem hiding this comment.
AI Review: Blocking
This treats "a hook row exists" as "durably parked", but the hooks table carries no per-delivery state: the row is written at hook_created and removed only at hook_disposed (see packages/world-postgres/src/drizzle/schema.ts — no status column). In packages/core/src/runtime/resume-hook.ts the hook_received event is awaited before the workflow re-trigger is enqueued. A crash or lost enqueue between those two writes leaves exactly the state this branch classifies as parked: an undisposed hook row, no runnable step, no due wait — but with a payload already recorded that nothing will ever act on. Nothing else re-drives it, so the run is skipped on every subsequent restart. reenqueueActiveRuns recovered it.
Verified against this branch: run in running, one hook row, no steps/waits → reenqueueActiveRuns enqueues 1, reenqueueRecoverableRuns enqueues 0.
Waits don't have this failure mode because waits.status flips to completed, which lands in the fail-open branch below. Hooks do, because row deletion is the only transition. Issue #3119's own repro describes the parked fixture as having "no received hook delivery" — that precondition isn't checkable from these tables. Classifying from the event log the way openHookAndWaitState() in packages/core/src/runtime.ts does (hook_created without hook_disposed, and no dangling hook_received), or adding a delivered marker to the hook row, would close it.
| await enqueue( | ||
| queueName, | ||
| { runId: run.runId }, | ||
| { idempotencyKey: startupRecoveryIdempotencyKey(run.runId) } |
There was a problem hiding this comment.
AI Review: Blocking
Attaching an idempotency key to a workflow re-trigger message opts it out of the per-run replay serialization. In packages/world-postgres/src/queue.ts, createTaskHandler only consults workflowRunSerializationKey (workflow:${runId}, set for any workflow message without a stepId — i.e. exactly this { runId } payload) inside the if (!idempotencyKey) branch, with the comment "prevent two workflow replays from mutating the same run's event log at the same time". Keyed messages skip straight past it. Startup is precisely when other pending jobs for the same run are also being picked up.
Verified by adapting the existing serializes workflow queue execution for the same runId test in queue.test.ts: two { runId } messages for the same run, the second carrying startup-recovery:<runId> → maxActiveRequests is 2. The unmodified test (no key) asserts 1. Two concurrent replays of one run's event log.
Two further consequences of the same key:
completedMessagesis an in-memory LRU keyed on the idempotency key, so within one process a second recovery attempt for a run is silently dropped rather than executed.- The key propagates into
messageData.idempotencyKeyand is reused asjobKeywhen the handler reschedules (queue.ts~L555). graphile-worker's defaultreplacemode means a lateraddJobwith the same key overwrites the outstanding job'srun_at— so a recovery enqueue and a suspended run's own future wake-up job can clobber each other, in either direction depending on ordering.
If the intent is only "don't accumulate duplicate recovery jobs across restarts", the queue's own per-run serialization already collapses redundant replays, and the event log makes duplicate replays harmless. Worth considering leaving the payload key-less, or threading a job key that doesn't route the message through the idempotency branch.
| * @param enqueue - Queue's enqueue method | ||
| * @param label - Log prefix identifying the world implementation | ||
| * @param namespace - Optional queue namespace. Defaults to WORKFLOW_QUEUE_NAMESPACE. | ||
| */ |
There was a problem hiding this comment.
AI Review: Nit
This doc block documents reenqueueRecoverableRuns (@param runs, @param drizzle, …) but sits immediately above recoverPage's own doc block, so recoverPage ends up with two comments and the exported function with none. Looks like a copy/paste — move it down to the export.
| * and suspending, so it must be replayed to continue; we fail open for | ||
| * this ambiguous case because the enqueue is deduplicated. | ||
| * | ||
| * A run counts as parked (skipped) when its only live state is open hooks |
There was a problem hiding this comment.
AI Review: Note
On the fix as a whole: the problem is real (#3119 — restart storms re-enqueue every suspended run, and duplicate recovery jobs pile up), the page-at-a-time classifier with three batched queries is the right shape, and classifyPageSafe failing open is a good call. The tests are thorough for the classifier itself (22 pass locally).
What makes it risky as written is that both mechanisms are inferred from entity tables that don't carry enough state to distinguish "parked" from "lost wake-up" (see the hooks comment), and the dedupe rides on the queue's idempotency channel, which already has other semantics attached (see the enqueue comment). A narrower first step that only skips runs with a futurewaits.resumeAt — the case where the wake-up job is genuinely persisted in the same database and provably survives the restart — would get most of the benefit with none of the wedge risk, and could be extended to hooks once hook rows can express delivery state.
Fixes#3119, reported by @Jiarui-Ni.
Startup recovery re-enqueued every non-terminal run, including parked runs, and enqueued recovery jobs without a stable job key. Two problems followed: parked runs were woken up when the world restarted, and repeated boots accumulated duplicate recovery jobs for the same run.
Fix: skip parked runs during startup recovery, and enqueue recovery jobs with a stable
startup-recovery:<runId>job key so graphile-worker dedupes them across boots. Includes a changeset (patch for@workflow/world-postgres).All 38 world-postgres tests pass locally; the new parked-run and dedupe tests fail without the fix.