Skip to content

Commit 7903661

Browse files
authored
fix(core): terminalize unfinished tasks when runs fail (#663)
## Summary Failed runs archive their PGMQ messages but leave sibling `step_tasks` rows `queued` or `started`, so stale recovery can requeue undispatchable work and late callbacks can misrepresent terminal state (#645). ## Invariant ```text runs.status = 'failed' implies no step_tasks row for that run has status IN ('queued', 'started') ``` Task outcomes stay truthful: - task that caused the failure → `failed` - task completed before the failure → `completed` - unfinished queued or started task → `cancelled` ## Why `cancelled` and not `failed`/`skipped` - `failed` counts real handler failures; reusing it corrupts failure metrics. - `skipped` is step-skip policy (`whenUnmet`/`whenExhausted`); reusing it conflates run failure with explicit skip decisions. - `cancelled` is orchestration state invalidated by the run failure. - `runs.failed_at` is the cancellation time; no `cancelled_at` column 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 1. **`fail_task()` with exhausted `when_exhausted = 'fail'`** — the culprit stays `failed`; every remaining queued/started task in the run becomes `cancelled` with `UPDATE ... RETURNING` message capture, then archived. Retries before exhaustion are preserved. 2. **`complete_task()` type violation** — the directly invalid task is marked `failed` first (output and `[TYPE_VIOLATION]` error preserved), then every other queued/started task is cancelled and archived afterward. 3. **`cascade_resolve_conditions()` with `when_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. 4. **Already-failed-run `fail_task()` path** — no longer rewrites a `started`/`cancelled` task to `failed`; 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()` rechecks `runs.status` under the `FOR UPDATE` lock and returns unchanged when the run is failed — closes the race where the pre-lock guard saw `started` but 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. - Cancellation wins over late `complete_task()` and `fail_task()`; repeated callbacks are idempotent and do not revive cancelled rows, rewrite history, clear `runs.failed_at`, duplicate events, or drift counters. - A committed completed task remains `completed`; a genuine failure remains `failed`. ## Stalled recovery guards `requeue_stalled_tasks()` now requires `run.status = 'started'` AND `step_state.status = 'started'` AND `task.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): 1. allows `cancelled` in `step_tasks.valid_status`; 2. recreates the four changed functions; 3. repairs historical rows — only `queued`/`started` tasks on `failed` runs flip to `cancelled`; 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 duplicate `run:failed`. - `complete_task/no_mutations_on_failed_run.test.sql` — late `complete_task()` after run failure leaves the task cancelled with history intact. - `fail_task_when_exhausted/cancelled_task_late_callbacks_are_idempotent.test.sql` — repeated late `complete_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 holds `FOR UPDATE` on the run row; late `complete_task()` and `fail_task()` — both aimed at the task the winning failure cancels — block after their pre-lock checks (proven blocked by ctrl via `pg_blocking_pids()`); ctrl then fails the run and commits. Both callbacks return the cancelled row unchanged: no output, no error, no `failed_at`, no moved `runs.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. - Each failure-path regression installs the #638 queue-table trigger that fails deterministically if archival ever precedes task terminalization. ## Checks run - `pnpm nx test:pgtap core --skip-nx-cache` — 284 files, 1316 tests, all pass - `pnpm nx verify-migrations core --skip-nx-cache` — pass - `pnpm nx gen-types core --skip-nx-cache` / `pnpm nx verify-gen-types core --skip-nx-cache` — pass (types unchanged; status columns are `string`) - `pnpm nx test core --skip-nx-cache` — pass - `pnpm nx build core --skip-nx-cache` — pass - `pnpm nx lint core --skip-nx-cache` — pass (`fix-sql` hit the known sqruff `--force` incompatibility; equivalent direct `sqruff fix schemas/` ran clean) - `pnpm changeset status`, `git diff --check` — clean ## Out of scope #621 (effective-timeout calculation), #656 (queue identity / visibility extension), #646 (worker-side handler cancellation), migration consolidation, manual tasks. Closes #645.
1 parent d79a175 commit 7903661

29 files changed

Lines changed: 2706 additions & 211 deletions
Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
1+
---
2+
"@pgflow/core": patch
3+
---
4+
5+
Mark unfinished tasks as cancelled when their run fails, prevent late callbacks and stalled recovery from reviving them, and repair active tasks on historical failed runs.

‎NOMENCLATURE_GUIDE.md‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -151,6 +151,7 @@ Slugs are unique text identifiers with specific rules:
151151
- `completed` - Task completed successfully
152152
- `failed` - Task failed (may be retried or permanent)
153153
- `skipped` - Task was cancelled because its parent step was skipped
154+
- `cancelled` - Task was cancelled because its run failed before the task finished; `runs.failed_at` is the cancellation time
154155

155156
## Configuration Terms
156157

‎pkgs/core/README.md‎

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -348,9 +348,12 @@ The system handles failures by:
348348
- Marking the step as 'failed'
349349
- Marking the run as 'failed'
350350
- Archiving the message in PGMQ
351-
- **Archiving all queued messages for the failed run** (preventing orphaned messages)
351+
- **Marking every remaining queued or started task in the run as 'cancelled'** and archiving their messages (the failed-run invariant: a failed run has no task rows left in `queued` or `started`)
352352
4. Additional failure handling:
353-
- **No retries on already-failed runs** - tasks are immediately marked as failed
353+
- **No retries on already-failed runs** - late `fail_task()` callbacks keep cancelled tasks cancelled
354+
- **Cancellation wins over late callbacks** - late `complete_task()` or `fail_task()` cannot revive or rewrite a cancelled task; a committed completed task stays completed
355+
- **`cancelled` differs from `failed` and `skipped`** - `failed` counts real handler failures, `skipped` is step-skip policy, `cancelled` is orchestration state invalidated by the run failure; `runs.failed_at` is the cancellation time
356+
- **Database cancellation does not terminate handlers** - JavaScript already executing in a worker is not forcibly stopped and external side effects are not undone
354357
- **Graceful type constraint violations** - handled without exceptions when single steps feed map steps
355358
- **Stores invalid output on type violations** - captures the output that caused the violation for debugging
356359
- **Performance-optimized message archiving** using indexed queries

‎pkgs/core/schemas/0060_tables_runtime.sql‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -104,7 +104,7 @@ create table pgflow.step_tasks (
104104
foreign key (run_id, step_slug)
105105
references pgflow.step_states(run_id, step_slug),
106106
constraint valid_status check (
107-
status in ('queued', 'started', 'completed', 'failed', 'skipped')
107+
status in ('queued', 'started', 'completed', 'failed', 'skipped', 'cancelled')
108108
),
109109
constraint output_valid_only_for_completed check (
110110
output is null or status in ('completed', 'failed')

‎pkgs/core/schemas/0062_function_requeue_stalled_tasks.sql‎

Lines changed: 7 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -13,6 +13,8 @@ begin
1313
-- Find and requeue stalled tasks (where started_at > timeout + 30s buffer)
1414
-- Tasks with requeued_count >= max_requeues will have their message archived
1515
-- but status left as 'started' for easy identification via requeued_count column
16+
-- Eligibility requires the parent run AND parent step to still be 'started':
17+
-- stale rows on failed runs or terminal steps must not be revived (#645).
1618
with stalled_tasks as (
1719
select
1820
st.run_id,
@@ -24,8 +26,11 @@ begin
2426
f.opt_timeout
2527
from pgflow.step_tasks st
2628
join pgflow.runs r on r.run_id = st.run_id
29+
join pgflow.step_states ss on ss.run_id = st.run_id and ss.step_slug = st.step_slug
2730
join pgflow.flows f on f.flow_slug = r.flow_slug
2831
where st.status = 'started'
32+
and r.status = 'started'
33+
and ss.status = 'started'
2934
and st.permanently_stalled_at is null
3035
and st.started_at < now() - (f.opt_timeout * interval '1 second') - interval '30 seconds'
3136
for update of st skip locked
@@ -84,9 +89,9 @@ begin
8489
_vr as (select count(*) from visibility_reset),
8590
-- Force execution of mark_permanently_stalled CTE
8691
_mps as (select count(*) from mark_permanently_stalled),
87-
-- Force execution of archived CTE
92+
-- Force execution of archived CTE
8893
_ar as (select count(*) from archived)
89-
select count(*) into result_count
94+
select count(*) into result_count
9095
from requeued, _vr, _mps, _ar;
9196

9297
return result_count;

‎pkgs/core/schemas/0100_function_cascade_resolve_conditions.sql‎

Lines changed: 63 additions & 42 deletions
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,9 @@ DECLARE
1717
v_iteration_count int := 0;
1818
v_max_iterations int := 50;
1919
v_processed_count int;
20+
v_run_transitioned boolean;
21+
v_flow_slug text;
22+
v_cancelled_message_ids bigint[];
2023
BEGIN
2124
-- ==========================================
2225
-- GUARD: Early return if run is already terminal
@@ -108,54 +111,72 @@ BEGIN
108111
-- Note: Cannot use "v_first_fail IS NOT NULL" because records with NULL fields
109112
-- evaluate to NULL in IS NOT NULL checks. Use FOUND instead.
110113
IF FOUND THEN
111-
UPDATE pgflow.step_states
112-
SET status = 'failed',
113-
failed_at = now(),
114-
error_message = 'Condition not met'
115-
WHERE pgflow.step_states.run_id = cascade_resolve_conditions.run_id
116-
AND pgflow.step_states.step_slug = v_first_fail.step_slug;
117-
114+
-- Fail the run only if it is still started. The conditional UPDATE takes
115+
-- the run row lock and rechecks status atomically, so replayed or
116+
-- concurrent calls cannot duplicate the terminal transition or its events.
118117
UPDATE pgflow.runs
119118
SET status = 'failed',
120119
failed_at = now()
121-
WHERE pgflow.runs.run_id = cascade_resolve_conditions.run_id;
120+
WHERE pgflow.runs.run_id = cascade_resolve_conditions.run_id
121+
AND pgflow.runs.status = 'started'
122+
RETURNING true INTO v_run_transitioned;
123+
124+
IF v_run_transitioned THEN
125+
UPDATE pgflow.step_states
126+
SET status = 'failed',
127+
failed_at = now(),
128+
error_message = 'Condition not met'
129+
WHERE pgflow.step_states.run_id = cascade_resolve_conditions.run_id
130+
AND pgflow.step_states.step_slug = v_first_fail.step_slug;
131+
132+
PERFORM realtime.send(
133+
jsonb_build_object(
134+
'event_type', 'step:failed',
135+
'run_id', cascade_resolve_conditions.run_id,
136+
'step_slug', v_first_fail.step_slug,
137+
'status', 'failed',
138+
'error_message', 'Condition not met',
139+
'failed_at', now()
140+
),
141+
concat('step:', v_first_fail.step_slug, ':failed'),
142+
concat('pgflow:run:', cascade_resolve_conditions.run_id),
143+
false
144+
);
122145

123-
PERFORM realtime.send(
124-
jsonb_build_object(
125-
'event_type', 'step:failed',
126-
'run_id', cascade_resolve_conditions.run_id,
127-
'step_slug', v_first_fail.step_slug,
128-
'status', 'failed',
129-
'error_message', 'Condition not met',
130-
'failed_at', now()
131-
),
132-
concat('step:', v_first_fail.step_slug, ':failed'),
133-
concat('pgflow:run:', cascade_resolve_conditions.run_id),
134-
false
135-
);
146+
PERFORM realtime.send(
147+
jsonb_build_object(
148+
'event_type', 'run:failed',
149+
'run_id', cascade_resolve_conditions.run_id,
150+
'flow_slug', v_first_fail.flow_slug,
151+
'status', 'failed',
152+
'error_message', 'Condition not met',
153+
'failed_at', now()
154+
),
155+
'run:failed',
156+
concat('pgflow:run:', cascade_resolve_conditions.run_id),
157+
false
158+
);
136159

137-
PERFORM realtime.send(
138-
jsonb_build_object(
139-
'event_type', 'run:failed',
140-
'run_id', cascade_resolve_conditions.run_id,
141-
'flow_slug', v_first_fail.flow_slug,
142-
'status', 'failed',
143-
'error_message', 'Condition not met',
144-
'failed_at', now()
145-
),
146-
'run:failed',
147-
concat('pgflow:run:', cascade_resolve_conditions.run_id),
148-
false
149-
);
160+
-- Terminalize every unfinished task across all branches as cancelled,
161+
-- capturing their message ids for archival below. Lock-order invariant:
162+
-- always lock/update step_tasks before PGMQ queue rows.
163+
WITH cancelled_tasks AS (
164+
UPDATE pgflow.step_tasks AS task
165+
SET status = 'cancelled'
166+
WHERE task.run_id = cascade_resolve_conditions.run_id
167+
AND task.status IN ('queued', 'started')
168+
RETURNING task.message_id
169+
)
170+
SELECT ARRAY_AGG(ct.message_id) INTO v_cancelled_message_ids
171+
FROM cancelled_tasks ct
172+
WHERE ct.message_id IS NOT NULL;
150173

151-
PERFORM pgmq.archive(r.flow_slug, ARRAY_AGG(st.message_id))
152-
FROM pgflow.step_tasks st
153-
JOIN pgflow.runs r ON st.run_id = r.run_id
154-
WHERE st.run_id = cascade_resolve_conditions.run_id
155-
AND st.status IN ('queued', 'started')
156-
AND st.message_id IS NOT NULL
157-
GROUP BY r.flow_slug
158-
HAVING COUNT(st.message_id) > 0;
174+
-- Archive the cancelled task messages captured above (only after their
175+
-- task rows are terminalized)
176+
IF v_cancelled_message_ids IS NOT NULL THEN
177+
PERFORM pgmq.archive(v_first_fail.flow_slug, v_cancelled_message_ids);
178+
END IF;
179+
END IF;
159180

160181
RETURN false;
161182
END IF;

‎pkgs/core/schemas/0100_function_complete_task.sql‎

Lines changed: 66 additions & 33 deletions
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,7 @@ declare
1414
v_dependent_map_slug text;
1515
v_run_record pgflow.runs%ROWTYPE;
1616
v_step_record pgflow.step_states%ROWTYPE;
17+
v_violation_archived_ids bigint[];
1718
begin
1819

1920
-- ==========================================
@@ -40,6 +41,27 @@ WHERE pgflow.step_states.run_id = complete_task.run_id
4041
AND pgflow.step_states.step_slug = complete_task.step_slug
4142
FOR UPDATE;
4243

44+
-- ==========================================
45+
-- GUARD: Run failed while this callback waited for the lock
46+
-- ==========================================
47+
-- The failed-run guard above ran before the failure committed. Recheck under
48+
-- lock so cancellation wins: archived message stays archived, task row keeps
49+
-- its terminal status, and no events or counters are emitted.
50+
IF v_run_record.status = 'failed' THEN
51+
-- Archive the task message if present (no-op when already archived)
52+
PERFORM pgflow._archive_task_message(
53+
complete_task.run_id,
54+
complete_task.step_slug,
55+
complete_task.task_index
56+
);
57+
-- Return the current task row without any mutations
58+
RETURN QUERY SELECT * FROM pgflow.step_tasks
59+
WHERE pgflow.step_tasks.run_id = complete_task.run_id
60+
AND pgflow.step_tasks.step_slug = complete_task.step_slug
61+
AND pgflow.step_tasks.task_index = complete_task.task_index;
62+
RETURN;
63+
END IF;
64+
4365
-- ==========================================
4466
-- GUARD: Late callback - step not started
4567
-- ==========================================
@@ -84,6 +106,20 @@ LIMIT 1;
84106

85107
-- Handle type violation if detected
86108
IF v_dependent_map_slug IS NOT NULL THEN
109+
-- Mark current task as failed FIRST and store the output that caused the
110+
-- violation, so the task row is terminal before any queue row is touched.
111+
UPDATE pgflow.step_tasks
112+
SET status = 'failed',
113+
failed_at = now(),
114+
output = complete_task.output, -- Store the output that caused the violation
115+
error_message = '[TYPE_VIOLATION] Produced ' ||
116+
CASE WHEN complete_task.output IS NULL THEN 'null'
117+
ELSE jsonb_typeof(complete_task.output) END ||
118+
' instead of array'
119+
WHERE pgflow.step_tasks.run_id = complete_task.run_id
120+
AND pgflow.step_tasks.step_slug = complete_task.step_slug
121+
AND pgflow.step_tasks.task_index = complete_task.task_index;
122+
87123
-- Mark run as failed immediately
88124
UPDATE pgflow.runs
89125
SET status = 'failed',
@@ -105,30 +141,6 @@ IF v_dependent_map_slug IS NOT NULL THEN
105141
false
106142
);
107143

108-
-- Archive all active messages (both queued and started) to prevent orphaned messages
109-
PERFORM pgmq.archive(
110-
v_run_record.flow_slug,
111-
array_agg(st.message_id)
112-
)
113-
FROM pgflow.step_tasks st
114-
WHERE st.run_id = complete_task.run_id
115-
AND st.status IN ('queued', 'started')
116-
AND st.message_id IS NOT NULL
117-
HAVING count(*) > 0; -- Only call archive if there are messages to archive
118-
119-
-- Mark current task as failed and store the output
120-
UPDATE pgflow.step_tasks
121-
SET status = 'failed',
122-
failed_at = now(),
123-
output = complete_task.output, -- Store the output that caused the violation
124-
error_message = '[TYPE_VIOLATION] Produced ' ||
125-
CASE WHEN complete_task.output IS NULL THEN 'null'
126-
ELSE jsonb_typeof(complete_task.output) END ||
127-
' instead of array'
128-
WHERE pgflow.step_tasks.run_id = complete_task.run_id
129-
AND pgflow.step_tasks.step_slug = complete_task.step_slug
130-
AND pgflow.step_tasks.task_index = complete_task.task_index;
131-
132144
-- Mark step state as failed
133145
UPDATE pgflow.step_states
134146
SET status = 'failed',
@@ -159,16 +171,37 @@ IF v_dependent_map_slug IS NOT NULL THEN
159171
false
160172
);
161173

162-
-- Archive the current task's message (it was started, now failed)
163-
PERFORM pgmq.archive(
164-
v_run_record.flow_slug,
165-
st.message_id -- Single message, use scalar form
174+
-- Terminalize every other unfinished task as cancelled, capturing their
175+
-- message ids for archival below. Lock-order invariant: always lock/update
176+
-- step_tasks before PGMQ queue rows. The culprit task is already terminal
177+
-- (failed above), so it is excluded from the cancellation set.
178+
WITH cancelled_tasks AS (
179+
UPDATE pgflow.step_tasks AS task
180+
SET status = 'cancelled'
181+
WHERE task.run_id = complete_task.run_id
182+
AND task.status IN ('queued', 'started')
183+
RETURNING task.message_id
184+
),
185+
culprit_task AS (
186+
-- Terminal culprit row: safe to read for its message id after terminalization
187+
SELECT st.message_id
188+
FROM pgflow.step_tasks st
189+
WHERE st.run_id = complete_task.run_id
190+
AND st.step_slug = complete_task.step_slug
191+
AND st.task_index = complete_task.task_index
192+
AND st.message_id IS NOT NULL
166193
)
167-
FROM pgflow.step_tasks st
168-
WHERE st.run_id = complete_task.run_id
169-
AND st.step_slug = complete_task.step_slug
170-
AND st.task_index = complete_task.task_index
171-
AND st.message_id IS NOT NULL;
194+
SELECT ARRAY_AGG(ids.message_id) INTO v_violation_archived_ids
195+
FROM (
196+
SELECT message_id FROM culprit_task
197+
UNION ALL
198+
SELECT message_id FROM cancelled_tasks WHERE message_id IS NOT NULL
199+
) ids;
200+
201+
-- Archive the culprit and cancelled task messages (only after their task rows are terminalized)
202+
IF v_violation_archived_ids IS NOT NULL THEN
203+
PERFORM pgmq.archive(v_run_record.flow_slug, v_violation_archived_ids);
204+
END IF;
172205

173206
-- Return the failed task row (API contract: always return task row)
174207
RETURN QUERY

0 commit comments

Comments
 (0)