fix(core): terminalize unfinished tasks when runs fail - #663
Merged
Conversation
🦋 Changeset detectedLatest commit: 5bf15c7 The changes in this PR will be included in the next version bump. This PR includes changesets to release 5 packages
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 |
Contributor
Author
This stack of pull requests is managed by Graphite. Learn more about stacking. |
|
View your CI Pipeline Execution ↗ for commit b4f88e1
💡 Verify your cache is correct by running tasks in a sandbox. Read docs ↗ ☁️ Nx Cloud last updated this comment at |
jumski
force-pushed
the
09-01-issue_645_failed_run_terminalization
branch
from
September 1, 2026 18:42
01e478f to
a90c5e1
Compare
Failed runs archive their queue messages but currently leave queued and started task rows active, allowing stale recovery and callback races to misrepresent or revive terminal work. Mark only unfinished tasks as cancelled before queue archival, preserve completed and genuinely failed outcomes, guard stalled recovery by parent state, and repair historical failed runs. Worker abort behavior and the #621 timeout correction remain separate work. Review round 1 changes: - Add late_callbacks_post_lock_race.test.sql: deterministic two-session dblink test that exercises the post-lock guards the single-session tests cannot reach. A ctrl session holds FOR UPDATE on the run row; late complete_task (single-to-map type-violating completion) and fail_task callbacks block after their pre-lock checks; ctrl then fails the run and commits. Asserts the cancelled task row, runs.failed_at, remaining_steps, event counts, and queue state stay untouched after both callbacks resume. Mutation-checked: removing the post-lock guard fails 8 of 15 asserts. dblink mechanics learned the hard way: pg_stat_activity snapshots are cached per transaction (poll through a dedicated autocommit probe connection), reset_db() does not clear pgflow.workers (cleanup deletes the test worker), and a queue read in this transaction would block the cleanup's drop_queue (read the queue through the probe connection). - cancelled_task_late_callbacks_are_idempotent.test.sql: run_before now includes run_id. Previously the inner run_id resolved to the outer query, making both comparison predicates tautologies instead of binding the idempotency assertions to the target run. - Trim trailing whitespace in 0062_function_requeue_stalled_tasks.sql (source of the migration lint failure) and regenerate the temp migration (repair UPDATE unchanged, rehashed). - edge-worker-integration CI failures on the first run were flake: same job green on main and green on the --failed rerun of this PR's run (33521700558, attempt 2), so no code change. Review round 2 changes: - late_callbacks_post_lock_race.test.sql: the late fail_task() callback now targets single_step, the task the winning boom failure cancels. Targeting boom (the culprit) only exercised the terminal-step guard: boom's step state is 'failed' after the winning failure, so that callback never needed the post-lock failed-run guard. single_step's step state stays 'started' under a failed run, so the post-lock guard is the only thing protecting the cancelled row, which is the regression this test exists to catch. - The wait proof now uses pg_blocking_pids() instead of a co-existence check: each callback backend's blocking chain (recursive, because PostgreSQL queues row-lock waiters FIFO — the second callback's direct blocker is the first callback, not ctrl) must terminate at the ctrl backend before the winning failure commits. Sleep-only/timing-only evidence is gone. - After both callbacks resume: single_step history (status, output, error_message, attempts_count, completed_at, failed_at) asserted unchanged after BOTH late callbacks, plus direct assertions that boom stays genuinely failed with the winning error_message. - Bounded lock_timeout (30s) on the late callback sessions so a stuck setup errors loudly instead of hanging; ctrl keeps its 5s bound; session/worker cleanup unchanged. - Mutation check re-run against the new test: removing only the fail_task post-lock failed-run guard fails exactly 2 of 18 asserts — runs.failed_at wiped to NULL by the guardless run UPDATE (the cancelled task matches no rows in fail_or_retry_task, so maybe_fail_step is empty and run_update takes the ELSE branches) and a duplicate run:failed event (run_update still returns 'failed', so v_run_failed re-fires the event). Guard restored, test green again 10/10 consecutive runs.
jumski
force-pushed
the
09-01-issue_645_failed_run_terminalization
branch
from
September 1, 2026 19:39
a90c5e1 to
165cfa2
Compare
Contributor
Author
Final coordinator review — APPROVEDNo unresolved correctness, concurrency, migration, data-loss, or invariant findings at I independently reviewed the schema and test diff, including:
The final two-session test now proves both late callbacks target the cancelled sibling. Its recursive Independent checks I reran:
CI passes all checks except the intentional |
Failure behavior spans handler retries, skipped steps, failed-run task cancellation, and stalled recovery, but the existing pages explain each path separately. Add a Concepts mental model with D2 state diagrams and cross-link it from build, operations, data-model, and reference pages. Extend the diagram theme for started, skipped, and cancelled task states.
added 2 commits
September 2, 2026 00:51
Explain that completed task output remains, while output and errors reported after cancellation are intentionally not persisted so terminal run history stays unchanged.
…lization Replace 20260901183450_pgflow_temp_failed_run_terminalization.sql with 20260902005317_pgflow_failed_run_terminalization.sql before merging to main: CI blocks temp migrations on main (block-temp-migrations). Regenerated via atlas-migrate-diff from pkgs/core/schemas/*.sql after REMOVE-TEMP + hash reset + supabase reset, then manually re-appended the DATA REPAIR block (atlas diff emits DDL only): the UPDATE terminalizing historical queued/started step_tasks on failed runs to 'cancelled', which the previous commits' temp migration also carried. Verified locally: nx verify-migrations, gen-types, verify-gen-types --skip-nx-cache, test:pgtap, and nx build website all pass; sqruff lint on schemas/ reports zero violations (nx fix-sql fails in this sandbox: sqruff 0.39.0 rejects --force, cosmetic-only).
Contributor
🔍 Preview Deployment: Website✅ Deployment successful! 🔗 Preview URL: https://pr-663.pgflow.pages.dev 📝 Details:
_Last updated: _ |
Contributor
Author
Contributor
🚀 Production Deployment: Website✅ Successfully deployed to production! 🔗 Production URL: https://pgflow.dev 📝 Details:
Deployed at: 2026-09-02T03:11:08+02:00 |
jumski
added a commit
that referenced
this pull request
Sep 2, 2026
## Summary Fix stalled-task recovery to use the effective step timeout instead of always using the flow timeout. This PR stacks directly on #663 (`09-01-issue_645_failed_run_terminalization`) and preserves its run, step-state, and task eligibility guards. ## Root cause `start_tasks()` sets PGMQ visibility from the effective timeout: ```sql coalesce(step.opt_timeout, flow.opt_timeout) + 2 ``` `requeue_stalled_tasks()` used only `flows.opt_timeout`. A short step timeout could therefore make the PGMQ message visible while its task row remained `started` until the longer flow timeout and recovery buffer expired. ## Behavior Recovery now requires: ```sql started_at < now() - (coalesce(step.opt_timeout, flow.opt_timeout) * interval '1 second') - interval '30 seconds' ``` The comparison stays strict. A null step timeout inherits the non-null flow timeout. The PGMQ-only two-second margin is not added to recovery. Adding it again would change the existing 30-second recovery grace to 32 seconds. The 15-second cron cadence can add up to roughly 15 seconds after eligibility. The change preserves: - `run.status = 'started'`, `step_state.status = 'started'`, and `task.status = 'started'`; - `permanently_stalled_at is null` and `FOR UPDATE OF task SKIP LOCKED` behavior; - attempts and requeue counters; - three successful requeues before permanent stall; - immediate visibility through `set_vt_batch(..., 0)`; - archive and permanent-stall behavior. ## Tests Added `effective_step_timeout.test.sql` with deterministic timestamps inside one transaction: - flow 60 / step 5: exactly 35 seconds stays started; 36 seconds requeues; - flow 5 / step 60: 36 seconds stays started; 91 seconds requeues; - flow 5 / null step timeout: 36 seconds requeues through flow fallback. Before the source fix, the focused test failed 5 of 10 assertions for the expected reason. The short step override returned 0 and stayed `started` at 36 seconds. The long step override requeued at 36 seconds, so its later 91-second call returned 0. After the source fix: - focused test: 1 file, 10 tests, pass; - all stalled-recovery tests: 6 files, 54 tests, pass; - full pgTAP: 285 files, 1326 tests, pass. ## Migration and release note Atlas generated `20260901203454_pgflow_temp_effective_step_timeout.sql`. It replaces `pgflow.requeue_stalled_tasks()`, includes the cumulative #645 guards, and performs no backfill. Added a separate patch changeset for `@pgflow/core`. The fixed release group expands the patch at release time. ## Checks - `pnpm nx verify-migrations core --skip-nx-cache` — pass - `pnpm nx gen-types core --skip-nx-cache` — pass; no generated type diff - `pnpm nx verify-gen-types core --skip-nx-cache` — pass - `pnpm nx test:pgtap core --skip-nx-cache` — pass; 285 files, 1326 tests - `pnpm nx test core --skip-nx-cache` — pass - `pnpm nx lint core --skip-nx-cache` — pass; 0 errors and 2 existing type-test warnings - `pnpm nx build core --skip-nx-cache` — pass - `pnpm changeset status` — pass; patch fixed group detected - `git diff --check` — pass `pnpm nx fix-sql core` hit the known Sqruff CLI mismatch: `error: unexpected argument '--force' found`. The direct repository fallback, `sqruff --config=.sqruff fix --parsing-errors pkgs/core/schemas/`, processed 37 files and found nothing to fix. Direct Sqruff lint also passed. Two fresh independent Sol xhigh review rounds returned `APPROVED` with no required findings. ## Out of scope - #656 and execution of the unreferenced `start_tasks()` visibility CTE; - #646 worker-side handler cancellation; - queue identity or per-step queue routing; - changes to #645 cancellation semantics or parent-state guards; - configurable recovery buffers or cron cadence; - migration consolidation and release PR #659. The stack still contains temporary migrations, so the main-targeted temporary-migration check can fail until the settled release sequence consolidates them. This PR does not consolidate the parent migration. Fixes #621
This was referenced Sep 2, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Failed runs archive their PGMQ messages but leave sibling
step_tasksrowsqueuedorstarted, so stale recovery can requeue undispatchable work and late callbacks can misrepresent terminal state (#645).Invariant
Task outcomes stay truthful:
failedcompletedcancelledWhy
cancelledand notfailed/skippedfailedcounts real handler failures; reusing it corrupts failure metrics.skippedis step-skip policy (whenUnmet/whenExhausted); reusing it conflates run failure with explicit skip decisions.cancelledis orchestration state invalidated by the run failure.runs.failed_atis the cancellation time; nocancelled_atcolumn and no reason column were added.Database cancellation does not terminate JavaScript already executing in a worker and does not undo external side effects the handler already performed. Worker-side abort behavior remains out of scope (#646).
Changed failure paths
fail_task()with exhaustedwhen_exhausted = 'fail'— the culprit staysfailed; every remaining queued/started task in the run becomescancelledwithUPDATE ... RETURNINGmessage capture, then archived. Retries before exhaustion are preserved.complete_task()type violation — the directly invalid task is markedfailedfirst (output and[TYPE_VIOLATION]error preserved), then every other queued/started task is cancelled and archived afterward.cascade_resolve_conditions()withwhen_unmet = 'fail'— the condition step and run still fail; unfinished tasks across all independent branches become cancelled, then archived. The run transition is now conditional (WHERE status = 'started') so replayed or concurrent calls cannot duplicate events.fail_task()path — no longer rewrites astarted/cancelledtask tofailed; it only archives any still-active message and returns the row.All three paths reuse the #638/#649 lock-order pattern: update task rows first, capture message IDs with
RETURNING, archive queue rows only afterward.Late-callback and concurrency guards
complete_task()rechecksruns.statusunder theFOR UPDATElock and returns unchanged when the run is failed — closes the race where the pre-lock guard sawstartedbut the run failed while the callback waited.fail_task()captures run status under the existing run/step lock and rechecks it after lock acquisition for the same race.complete_task()andfail_task(); repeated callbacks are idempotent and do not revive cancelled rows, rewrite history, clearruns.failed_at, duplicate events, or drift counters.completed; a genuine failure remainsfailed.Stalled recovery guards
requeue_stalled_tasks()now requiresrun.status = 'started'ANDstep_state.status = 'started'ANDtask.status = 'started'. Genuine recovery, requeue counts, max-requeue and permanent-stall behavior, and the current timeout calculation are unchanged (#621 owns the timeout bug).Migration and backfill
20260901183450_pgflow_temp_failed_run_terminalization.sql(Atlas-generated, temp-prefixed for the stacked #645 → #621 → #656 order):cancelledinstep_tasks.valid_status;queued/startedtasks onfailedruns flip tocancelled; completed/failed outcomes and every history field (attempts, requeue history, worker identity, timestamps, outputs, errors) are preserved.Verified with an upgrade fixture: DB reset through
20260827180017, a failed run holding completed/failed/started/queued tasks, migration applied → completed/failed/cancelled/cancelled with all history fields byte-identical.Tests
fail_task/archive_sibling_map_tasks.test.sql— exhausted map: task 0 failed, task 1 started→cancelled, task 2 queued→cancelled; run/step failed; zero active tasks/messages; all archived.type_violations/cancels_unfinished_tasks.test.sql— invalid task failed with preserved output/error; completed work stays completed; unrelated queued/started tasks cancelled.condition_evaluation/dependent_unmet_fail_archives_active_messages.test.sql— completed trigger task stays completed; condition step fails; independent unfinished work cancelled; replay emits no duplicaterun:failed.complete_task/no_mutations_on_failed_run.test.sql— latecomplete_task()after run failure leaves the task cancelled with history intact.fail_task_when_exhausted/cancelled_task_late_callbacks_are_idempotent.test.sql— repeated latecomplete_task()/fail_task()leave cancelled rows, history, parent state, counters, and events unchanged.fail_task_when_exhausted/late_callbacks_post_lock_race.test.sql— two-session dblink test for the post-lock race: a ctrl session holdsFOR UPDATEon the run row; latecomplete_task()andfail_task()— both aimed at the task the winning failure cancels — block after their pre-lock checks (proven blocked by ctrl viapg_blocking_pids()); ctrl then fails the run and commits. Both callbacks return the cancelled row unchanged: no output, no error, nofailed_at, no movedruns.failed_at, no duplicate events, counters untouched.requeue_stalled_tasks/ignores_terminal_parent_state.test.sql— started run/step recovery still requeues; failed-run and terminal-step stale tasks are ignored.Checks run
pnpm nx test:pgtap core --skip-nx-cache— 284 files, 1316 tests, all passpnpm nx verify-migrations core --skip-nx-cache— passpnpm nx gen-types core --skip-nx-cache/pnpm nx verify-gen-types core --skip-nx-cache— pass (types unchanged; status columns arestring)pnpm nx test core --skip-nx-cache— passpnpm nx build core --skip-nx-cache— passpnpm nx lint core --skip-nx-cache— pass (fix-sqlhit the known sqruff--forceincompatibility; equivalent directsqruff fix schemas/ran clean)pnpm changeset status,git diff --check— cleanOut of scope
#621 (effective-timeout calculation), #656 (queue identity / visibility extension), #646 (worker-side handler cancellation), migration consolidation, manual tasks.
Closes #645.