fix(schedule): reconcile interrupted schedule executions - #6780
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
PR SummaryHigh Risk Overview Unknown enqueue/lookup outcomes leave the claim in place instead of advancing cadence. Terminal carriers get a Adds concurrent partial indexes for unreconciled terminal carriers and redacting logs so recovery and cleanup stay index-backed. Database and Trigger.dev recovery remain independent; outstanding schedule jobs must drain before switching providers. Reviewed by Cursor Bugbot for commit 6512dc2. Configure here. |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
Greptile SummaryReworks interrupted schedule execution recovery to reconcile claimed carriers against persisted workflow logs rather than redispatching them.
Confidence Score: 5/5The PR appears safe to merge because no eligible blocking failure remains in the supplied follow-up-review scope. No blocking failure remains.
|
| Filename | Overview |
|---|---|
| apps/sim/app/api/schedules/execute/route.ts | Introduces persisted-log reconciliation, guarded claim restoration, carrier settlement, and retry/defer handling for interrupted schedule occurrences. |
| apps/sim/background/schedule-execution.ts | Exposes and reuses claim-guarded schedule accounting operations required by recovery. |
| apps/sim/app/api/cron/cleanup-stale-executions/route.ts | Excludes active schedule carriers from generic stale cleanup and retains terminal carriers until recovery records reconciliation. |
| apps/sim/lib/workflows/schedules/carrier-metadata.ts | Centralizes reconciliation and irrecoverability metadata markers and matching SQL predicates. |
| packages/db/migrations/0295_chilly_franklin_storm.sql | Adds concurrent partial indexes for stale execution sweeping and unreconciled schedule-carrier recovery. |
| packages/db/schema.ts | Declares the new partial indexes in the Drizzle schema consistently with the migration. |
| apps/sim/app/api/schedules/execute/route.test.ts | Adds extensive coverage for recovery outcomes, claim races, malformed carriers, cancellation, and ambiguous queue acceptance. |
| apps/sim/app/api/cron/cleanup-stale-executions/route.test.ts | Covers status-specific stale-log cleanup and schedule-carrier retention ownership. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
Tick[Schedule cron tick] --> Carrier{Existing carrier?}
Carrier -->|Untouched pending, zero attempts| Execute[Execute occurrence]
Carrier -->|Previously claimed or terminal| Log[Read persisted execution log]
Log --> Outcome{Persisted outcome}
Outcome -->|Completed / paused| Success[Apply claim-guarded success accounting]
Outcome -->|Failed / indeterminate| Failure[Apply claim-guarded failure accounting]
Outcome -->|Cancelled| Cancel[Apply claim-guarded cancellation accounting]
Success --> Mark[Stamp carrier reconciled]
Failure --> Mark
Cancel --> Mark
Mark --> Cleanup[Cleanup after retention window]
Reviews (3): Last reviewed commit: "refactor(schedules): drop vestigial reco..." | Re-trigger Greptile
…dex its scan Follow-up hardening on the schedule recovery reconciliation. Reconciliation settled every carrier it examined, including ones that had already reached a terminal status and recorded their own outcome. Because the normal completion path never stamps the reconciled marker, each successful run's carrier was picked up on the next tick and rewritten: `completedAt` bumped, `error` nulled and `output` replaced with a recovery stub, all of which `GET /api/jobs/[jobId]` surfaces. A completed carrier whose execution log had aged out was additionally flipped to failed. Settle only carriers still in flight; a terminal one is owed schedule accounting and the marker, nothing more. The recovery scan matched no index. There was none on `async_jobs.updated_at`, and the unreconciled-terminal branch tested a jsonb extraction, so the whole OR fell back to a sequential scan and sort of `async_jobs` on every tick. Add the partial index, and spell the branch's status list and metadata key as SQL literals: Postgres cannot prove a parameterised predicate implies a literal index predicate, so a bound key would have left the new index unused. Irrecoverable carrier tombstones were exempted from retention with no secondary expiry, so the one class of row that can never reconcile grew without bound. Give them a longer bounded window instead. `WORKFLOW_EXECUTION_MAX_ROWS_PER_RUN` had become a per-status budget when the stale-execution sweep gained its `redacting` pass, silently doubling the per-run cap. Share the budget across both passes, and add the matching partial indexes so the second pass keeps an index rather than seq-scanning. Also consolidates the carrier metadata keys, their predicates and the jsonb merge into one module -- the two routes were the writer and reader of the same keys with no shared symbol, so a rename type-checked clean while silently breaking retention.
|
Pushed a follow-up commit ( 1. Reconciliation was rewriting carriers that needed no repair. 2. The recovery scan matched no index. There is no index on 3. Irrecoverable tombstones were retained forever. Retention exempted 4. Also consolidated the carrier metadata keys, their predicates and the jsonb merge into Every fix is mutation-tested — I reverted each change and confirmed the tests go red, then green on restore. One assertion did not discriminate on the first pass and was tightened. Two things I deliberately left alone: the at-most-once latency trade (a transient enqueue failure now waits out the reservation TTL rather than taking the fast infra-retry backoff — that's the point of the PR, worth a line in the description), and extracting the reconciliation out of the 1759-line route file, which is real but is a mechanical refactor better done separately. |
… failing live runs Addresses the review findings on the reconciliation hardening. A carrier whose accounting was deferred got no write at all, so it kept its `updatedAt` and stayed at the head of the `updatedAt`-ordered recovery batch on every tick, starving every other claimed carrier and never becoming eligible for retention. This was a regression from the previous commit: settling used to bump the timestamp for every examined row, and skipping the settle for already-terminal carriers removed the bump with it. A payload with no `scheduledFor` can never reconcile, so such a row pinned a batch slot permanently. Bump `updatedAt` unconditionally and keep only the reconciled marker conditional. The stale-execution sweep terminalized `redacting` logs on the execution deadline. That deadline bounds execution, while `redacting` covers payload masking after the run already finished -- so a run that used most of its budget entered redaction with the deadline due, and the sweep failed it five minutes later while the worker was still masking. Schedule recovery then read the log as a failed occurrence and counted a failure that never happened, even though the worker's terminal write later restored `completed`. Sweep `redacting` on the generic stale window only. `getScheduleNextRunAt` falls back to a daily cadence when a schedule has no cron expression. Deployment cannot persist such a schedule, so the branch is unreachable, but this change widened its use from failure recovery to every outcome -- log a warning when it fires rather than silently guessing a cadence. `executionDeadlineAt` was missing from the shared `workflowExecutionLogs` schema mock, so it read as `undefined` and assertions comparing against that column were trivially true. Add it.
|
@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 cb22726. Configure here.
waleedlatif1
changed the title
… gap Follow-ups from a full re-read of the change. No behavior change except the removed dead code paths. The metadata merge stripped a `scheduleRecoveryBlocked` key on every write. That key has never been written by any shipped code -- it appears nowhere in staging and nowhere in history outside this branch -- so the strip guarded against a state that cannot exist, at the cost of an extra jsonb operation and a bind parameter on every reconciliation write. `processScheduleItem` set `carrierObservedOrLookupUncertain` immediately before returning on an ambiguous enqueue. The flag is only read from the surrounding catch block, which a normal return skips, so the assignment was dead and read as though it were load-bearing. Replaced with a comment stating why the occurrence is preserved. The stale-execution sweep carried two near-identical `jsonb_set` templates that differed only in their error expression, kept flat because the test mock renders nested SQL fragments as placeholders. The suite now has a recursive renderer, so the error expression is a named per-status value and there is one `jsonb_set`. Success was the only schedule outcome without a named update builder, which left `executeScheduleJob` using two idioms for the same guarded write and left the update shape untested. Add `buildScheduleSuccessUpdate` beside its cancellation and failure siblings, use it from both call sites, and cover it the way `buildScheduleCancellationUpdate` is covered -- a mutation of its `failedCount` reset previously passed every suite.
|
Did a full line-by-line re-read of the final state. Design verdict below, plus four things I cleaned up in The mechanism is the right one. The question "did this occurrence already run?" has exactly one durable answer in this system — the Two design points I considered and concluded are fine:
What I fixed:
Remaining, not addressed here because they're pre-existing and structural rather than defects: Verification: full suite 28,613 tests — one failure in |
|
@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 6512dc2. Configure here.
2 participants
Summary
Reconcile claimed schedule jobs after outages from persisted workflow execution logs instead of redispatching them. Untouched database jobs still execute only while pending with zero attempts, while claimed jobs are resolved as success, failure, cancellation, pause, or an indeterminate failure. Schedule accounting uses the existing occurrence claim guard, and cleanup leaves active schedule carriers to schedule recovery.
This intentionally keeps database and Trigger.dev recovery independent. Switching queue providers while schedule jobs are outstanding remains unsupported; deployments must drain outstanding schedule jobs before changing providers.
No schema migration, public API change, or new module is introduced.
Type of Change
Testing
Reviewer focus: at-most-once behavior after a carrier is claimed, claim-guarded schedule accounting, cancellation handling, and cleanup ownership.
Checklist
Screenshots/Videos
Not applicable; server-side recovery change.