[Totoro-qaq]

Fixes #5034

Why

ScheduledTaskService.handle_run_completion finalized a run in two transactions: first the scheduled_task_runs row, then the parent task. A crash between them left a once task at running even though its run outcome was already durable, and recovery then cancelled it without consulting the committed outcome, so a successful run could be reported as cancelled.

Review found two further gaps on the same path: recovery ordered occurrences by caller timestamps, which are not a cross-instance recency key, and a late older occurrence (launch lease expired, a newer occurrence admitted and failed, then the older run returned) could overwrite the newer parent result through both recovery and the normal completion callback.

What changed

• Once-task recovery reads the actual latest occurrence and finalizes the parent from its committed outcome (success → completed, failed → failed, interrupted/skipped → cancelled); active occurrences are left to the queue owner. Single-instance startup recovery runs occurrence → parent in order and fails closed before request admission.
• Each new occurrence receives a database-backed per-task occurrence_seq under the existing parent lock. Launch, recovery, completion and queue-failure paths project an occurrence onto the parent only when it is the task's newest sequenced occurrence, so a late older run can no longer replace a newer result.
• An internal launch_accounted marker counts each proven launch exactly once even when a late older launch is fenced, and completion commits the occurrence, its accounting and the eligible parent outcome in one parent-first transaction.
• Migration 0022_scheduled_occurrence_seq (single head, chained after canonical 0019_thread_incarnations). Existing rows are not backfilled. The new fields are internal and omitted from repository/API responses; history display and cross-task FIFO are unchanged.
• Rolling upgrades: upgraded instances project only sequenced occurrences and defer recovery while any occurrence is still live, whichever instance admitted it; rows admitted by pre-upgrade instances are projected by those instances themselves, and the ordering guarantees hold once every Gateway writer runs the upgraded version (README Upgrade Notes).

Surface area

• Frontend UI — page / component / setting / interaction under frontend/
• Backend API — endpoint / SSE event / request-response shape under backend/app (scheduler recovery and completion; Gateway startup fails closed on single-instance recovery failure; migration 0021)
• Agents / LangGraph — agent node, graph wiring, langgraph.json, or prompt change
• Sandbox — docker/ or sandboxed execution
• Skills — change under skills/
• Dependencies — new/upgraded entry in backend/pyproject.toml or frontend/package.json (say what it buys us)
• Default behavior change — once-task recovery finalizes from the committed outcome instead of cancelling; a stale occurrence no longer overwrites a newer parent result
• Docs / tests / CI only — no runtime behavior change

Bug fix verification

• Recovery misreporting a successful run as cancelled: backend/tests/test_scheduler_completion_consistency.py (stuck-parent, skipped, active and race cases). Red before the fix, green here.
• Reversed/equal caller timestamps between occurrences: test_once_recovery_uses_occurrence_order_despite_clock_skew (SQLite) and test_postgres_once_recovery_uses_occurrence_order_despite_clock_skew (PostgreSQL); 16 ids red on timestamp ordering, green with sequence ordering.
• Late older launch overwriting a newer result, and launch counting: backend/tests/test_scheduler_occurrence_ordering.py (red on the sequence-only prototype) and backend/tests/test_scheduler_completion_atomicity.py.
• Pre-upgrade writer rows during a rolling upgrade: test_recovery_lookup_prefers_the_highest_sequence_whenever_one_exists and test_once_recovery_defers_while_any_occurrence_is_live_then_projects_the_sequence_winner (both recovery paths, SQLite and PostgreSQL); the active-row ids were red before the deferral rule, green here.

Validation

Head 4cec5f0e:

• Focused SQLite/PostgreSQL scheduler, ordering, completion, migration, bootstrap and guidance tests: 175 passed (PostgreSQL 17 in a disposable container).
• make test-blocking-io: 116 passed. make lint and ruff format --check: pass (1373 files).
• Full offline backend run: 14,448 executed cases pass at 4cec5f0e (14,433 in the uv run pytest full run, followup-full-offline-tests-uvrun.log; the 15 browser/fetch cases that need public DNS were rerun in a terminal with normal networking, 15 passed, user-terminal-baseline15.log), 165 skipped, 3 deselected.
• Twelve controlled real-runtime scenarios (Gateway lifespan, RunManager, workers, LangGraph, checkpoints, SQL repositories; SQLite and PostgreSQL single-/multi-instance; interrupted recovery and late normal completion) pass at 4cec5f0e with a local replay model endpoint and no external model calls. Not exercised: OS process kill, multiple OS processes/Kubernetes.

AI assistance

Tool(s) used: Codex, Claude Code.

How you used it: Source investigation, implementation and regression tests, independent code review of the diff, and local SQLite/PostgreSQL validation with controlled runtime interleavings. The design choices (sequence allocated under the parent lock, parent-write fencing, the launch-accounting marker, the rolling-upgrade fallback) were decided and reviewed by the author.

• I've read and understand every line of this change and take responsibility for it — it's not unreviewed AI output.

🤖 Generated with Claude Code

[CLAassistant]

All committers have signed the CLA.

[Huixin615]

Blocking: reconcile the actual latest run, including skipped.

TERMINAL_RUN_STATUSES already includes skipped, but neither recovery path handles it correctly:

• cancel_stuck_once_tasks reads the latest row but lets skipped fall through to generic cancellation.

• reconcile_stuck_once_tasks filters to success/failed/interrupted before ordering, so it retrieves the latest matching terminal row, not necessarily the task’s latest run. With retained history, it can ignore a newer skipped or active row and apply an older success, incorrectly marking the parent completed.

Please fetch the latest scheduled_task_runs row without a status filter in both paths, then branch on that exact row’s status. skipped should have an explicit parent-state mapping rather than being treated as “no terminal run.” Please also add regression cases with multiple historical runs, including an older success followed by a newer skipped/active row, for both reconciliation paths.

A few test concerns:

1. TestHandleRunCompletionHappyPath never calls ScheduledTaskService.handle_run_completion; it manually performs the two expected repository writes. It would still pass if the real completion hook regressed. Please invoke the production method or remove the claim that this test covers it.

2. The DST tests are unrelated to this fix and call croniter directly rather than DeerFlow’s next_run_at. The spring-forward test also only checks tzinfo is not None twice, which does not prove that a nonexistent local time was normalized to a valid instant. I suggest removing these from this PR or moving them to a focused follow-up with concrete production-level assertions.

3. Please update the relevant scheduler/recovery documentation in backend/AGENTS.md.

@Totoro-qaq Thanks again for your PR, please look at the suggestions.

[Totoro-qaq]

Thanks for the thorough review, @Huixin615. All 4 points addressed:

1. skipped handling — Both recovery paths now explicitly branch on skipped → parent finalised as cancelled (no work performed). Docstring updated.

2. Fetch latest row without status filter — reconcile_stuck_once_tasks query changed from status.in_(("success", "failed", "interrupted")) to unconditional order_by(created_at.desc(), id.desc()).limit(1). Ensures a newer skipped/active row is never skipped over in favour of an older terminal run.

3. Happy-path test — Rewritten to construct a real ScheduledTaskService and call service.handle_run_completion(record) instead of manually performing two repo writes.

4. AGENTS.md — Added one-line note documenting the outcome-aware reconciliation.

Additionally:

• Removed the DST tests from this PR (as suggested) — they call croniter directly rather than DeerFlow's next_run_at.
• Added 4 regression tests for multiple historical runs: older success + newer skipped → cancelled, older success + newer running → generic cancel, for both cancel_stuck_once_tasks and reconcile_stuck_once_tasks.
• Cleaned up duplicate inline imports to top-level.

Local CI gate results (identical commands to CI): make lint clean, make test 12389 passed, PG tests 20 passed, new test file 17 passed.

[willem-bd]

Thanks — this head cleanly resolves the earlier round: both recovery paths now fetch the latest scheduled_task_runs row without a status filter, skipped gets an explicit parent mapping, multi-history regressions cover older-success/newer-skipped/active for both paths, the happy-path test drives the real handle_run_completion, and backend/AGENTS.md documents the new behaviour. The remaining findings below are minor maintainability items. One doc note (no code change): the PR description still advertises the four DST/croniter regression tests that were removed from this revision — worth trimming that paragraph so the description matches the 17 tests actually shipped.

[willem-bd]

[P1] Refresh the latest run after acquiring the task lock

reconcile_stuck_once_tasks() batch-loads ORM run rows before locking each task. A completion can commit scheduled_task_runs.status = success after _fetch_latest_runs() returns but before that task is finalized. The cached ORM row still says running, so _finalise_once_task_from_run() applies the generic cancellation and recreates the inconsistency this PR is intended to fix.

I reproduced this deterministically by changing the run from running to success immediately after _fetch_latest_runs() returned: reconciliation marked the parent cancelled instead of completed.

Please lock and refresh the latest ScheduledTaskRunRow after acquiring the task lock, likely with SELECT ... FOR UPDATE plus populate_existing, or make the run and parent completion updates atomic. The outcome used for finalization must not come from the pre-lock batch snapshot.

[Totoro-qaq]

Thanks for the sharp P1, @willem-bd — you're right, the pre-lock batch snapshot reintroduced exactly the bug this PR fixes. Fixed.

Root cause of the race
reconcile_stuck_once_tasks() loaded all latest run rows up front (before the loop), then locked each task row and finalized from the cached snapshot. A completion committing scheduled_task_runs.status = success between the batch load and finalization left a stale running row in the session, so _finalise_once_task_from_run() applied the generic cancellation — marking a successful task cancelled again.

Fix

• Reverted the N+1→batch idea on the locked path (correctness beats the micro-opt here). The latest run is now read per task, after session.get(..., with_for_update=True) acquires the parent row lock, via a new _fetch_latest_run().
• Added .execution_options(populate_existing=True) so the read bypasses the session identity map and reflects any concurrently committed status.
• cancel_stuck_once_tasks() now also reads its run per task (fresh), matching the same post-processing read semantics.
• Removed the now-unused func (window-function) import.

Tests
Added two regression tests (test_reconcile_sees_run_committed_before_finalize, test_cancel_sees_run_committed_before_finalize): a separate transaction commits success on the run, then the recovery path must finalize the parent as completed — not cancelled. Suite is now 19 tests, all green.

Local CI gate results (same commands as CI): make lint clean, make test 12389 passed, new test file 19 passed.

One outstanding doc note from your earlier round: the PR description still mentions the DST/croniter tests that were removed. Want me to trim that paragraph, or leave the description as-is?

[Huixin615]

@Totoro-qaq Thanks for the follow-up. The production implementation at the latest head now addresses the previous findings: both paths use the actual latest run, skipped is explicit, the mapping is shared, the real completion hook is exercised, and the pre-lock batch snapshot has been removed.

One remaining test issue:

[P2] The new race tests do not enter the race window

Both test_cancel_sees_run_committed_before_finalize and test_reconcile_sees_run_committed_before_finalize commit status="success" before invoking the recovery method. The previous buggy batch-snapshot implementation would also pass these tests, because its initial batch query would run after that commit and already observe success.

Please make the status transition occur after recovery has observed the stale running state but before parent finalization—for example, by using an event/barrier or wrapping/monkeypatching the per-task fetch so a separate transaction commits success at that exact point. The test should fail against the reverted pre-lock batch implementation and pass against the current implementation.

[WillemJiang]

@Totoro-qaq please take a look at the review comments.

[Totoro-qaq]

Thanks for the precise P2, @Huixin615 — you're right, the previous tests committed success before invoking the recovery method, so the buggy pre-lock batch implementation would also observe the commit and pass. The tests didn't actually enter the race window.

P2 fix

Both race tests now use monkeypatch to intercept _fetch_latest_run at the exact moment the per-task fetch fires. The intercepted function:

1. Commits status="success" in a separate session (simulating the concurrent completion)
2. Calls the real _fetch_latest_run(session, task_id) with populate_existing=True
3. Returns the fresh result

A reverted pre-lock batch implementation never calls _fetch_latest_run — it uses _fetch_latest_runs (batch) instead — so the monkeypatch never fires, the stale cached running row is used, and the test fails (catching the regression). The current post-lock implementation calls _fetch_latest_run, the monkeypatch commits success, and the fresh read picks it up → completed → test passes.

Also fixed a staticmethod() descriptor bug: the monkeypatched function was assigned as a plain function, causing self to be passed as an extra argument. Both the assignment and restoration now use staticmethod() to preserve the correct calling convention.

Verification: lint clean, 19/19 test file passed, full suite 12391 passed / 78 skipped.

[willem-bd]

Requesting changes for the unresolved P1 in the existing inline discussion: recovery must not terminalize a once-task parent while its latest scheduled occurrence is still queued, launching, or running.

The preceding scheduled-run sweep and parent reconciliation are exception-isolated and not atomic. I also reproduced a second interleaving at this head: _fetch_latest_run() returned the latest occurrence as running, a separate completion transaction then committed that row as success, and recovery still committed the parent as cancelled from the stale ORM object. The focused suite does not cover this window because its race transition happens before the production fetch.

Please leave active occurrences unchanged for a later recovery pass, or reconcile/lock the scheduled occurrence, linked durable run, and parent atomically before finalization. Existing inline discussion: #5035 (comment)

Validation: the existing focused suite passes (19 tests), Ruff and formatting pass, and the added post-fetch race reproduction fails with cancelled != completed.

[Totoro-qaq]

Thanks for the deep P1, @willem-bd — you're right, I was still finalizing parents for active occurrences. Fixed.

Root cause

_finalise_once_task_from_run() treated a non-terminal run status (queued/launching/running) the same as "no run row" — both fell into the else branch, which applied generic cancellation. But an active occurrence means the run is still in progress; a concurrent completion may have already committed the linked durable run as success. Finalizing the parent from the stale running occurrence status permanently recreates the bug this PR fixes.

Fix

• _finalise_once_task_from_run() now explicitly checks for active statuses (queued/launching/running) and returns early without touching the parent. Only terminal statuses (success/failed/interrupted/skipped) and truly absent runs trigger finalization.
• Both cancel_stuck_once_tasks() and reconcile_stuck_once_tasks() detect the no-op (active run) via task_row.updated_at unchanged and skip incrementing their counters.
• Updated 3 existing tests to expect the parent is left running (not cancelled) for active occurrences.

Verification: lint clean, 19/19 test file passed, full suite 12391 passed / 78 skipped.

Pushed as 87577981 — PR #5035 updated.

[willem-bd]

This head resolves both prior P1s cleanly: the latest run is now read per task after the parent lock with populate_existing (_fetch_latest_run), and active occurrences (queued/launching/running) are left untouched instead of being generic-cancelled, with the multi-history and monkeypatch race regressions covering both paths. The four items below are follow-ups on the new early-return: the main one is that its "a later recovery pass will finalize it" justification only holds for the multi-instance caller — the single-instance path has no such pass, so a task that hits the branch there stays running until the next restart. Doc note carried over from the earlier round (no code change): the PR description still advertises the removed DST/croniter test section.

[Totoro-qaq]

Thanks for the thorough follow-up, @willem-bd. All four items addressed:

[P2] Single-instance path leaves task permanently "running"
Added cancel_stuck_once_tasks to the single-instance poll loop (run_once), mirroring how _reconcile_active_state re-runs both halves every poll for multi-instance. Stuck once-tasks are now retried on every iteration instead of being left in "running" until the next restart.

[Suggestion] Use TERMINAL_RUN_STATUSES / ACTIVE_RUN_STATUSES
Defined local frozenset copies in scheduled_tasks/sql.py with a comment explaining the circular import constraint (scheduled_task_runs.sql → scheduled_tasks/model.py → scheduled_tasks/sql.py → back). Kept in sync manually; a dict-keyed mapping was also considered but the explicit elif chain is clearer for the five-way outcome.

[Nit] Stale docstring in cancel_stuck_once_tasks
Updated to say active occurrences are "left untouched" and only absent runs receive generic cancellation.

[Nit] _fetch_latest_run precondition
Softened from "must be called AFTER the parent task row is locked" to "the caller must not hold a pre-lock snapshot" — the single-instance path doesn't lock the parent row.

Bonus: _finalise_once_task_from_run now returns bool
True if the parent was finalised, False for active/no-op. Both callers use the return value for their counters instead of the fragile updated_at comparison.

Verification: lint clean, 19/19 test file passed, full suite 12391 passed / 78 skipped.

Pushed as 23dc63d0 — PR #5035 updated.

[willem-bd]

Thanks — this head resolves both prior P1s: the latest run is now re-read per task after the parent lock with populate_existing (_fetch_latest_run), active occurrences (queued/launching/running) are left untouched instead of generic-cancelled, and the single-instance poll loop now re-runs cancel_stuck_once_tasks so the "a later recovery pass will finalize it" justification actually holds there. The shared _finalise_once_task_from_run helper and its bool return also close out the earlier duplication/no-op-argument nits, and the new NOTE at scheduled_tasks/sql.py:16 is correct — I confirmed the cycle is real (scheduled_task_runs/sql.py imports deerflow.persistence.scheduled_tasks.model, whose package __init__ re-imports scheduled_tasks.sql, the module mid-import).

The items below are follow-ups, none blocking: the multi-history regressions don't actually control the column that decides "latest", the id DESC tie-break is not a recency key, the helper's return annotation contradicts its contract, and several new test docstrings still describe the leave-untouched branch as a generic cancel.

(No code change, carried over: the PR description still advertises the removed DST/croniter test section.)

[Totoro-qaq]

Addressed the follow-up items from the review (head updated to b34b7154):

1. Multi-history regressions now control the column that decides "latest". _create_run in the test helper previously let created_at fall back to the repo default, so the recency ordering was never exercised. It now pins created_at (and scheduled_for) explicitly so the offsets in the older/newer regressions actually drive _fetch_latest_run's ordering.

2. id DESC is no longer used as a recency tie-break. _fetch_latest_run now orders by scheduled_for DESC — a deterministic recency key on the occurrence's schedule position — instead of created_at DESC, id DESC (where id is an opaque run-record string, arbitrary wrt recency).

3. _finalise_once_task_from_run return annotation matches its contract. It now returns bool (was -> None) since it reports whether the parent was finalized vs left for a later pass.

4. Docstrings that described the leave-untouched branch as a "generic cancel" are corrected to state the parent is left unchanged (not cancelled) when the latest occurrence is still active.

5. Constants are now canonical in one place. TERMINAL_RUN_STATUSES / ACTIVE_RUN_STATUSES moved to scheduled_tasks/model.py and imported by both scheduled_tasks/sql.py and scheduled_task_runs/sql.py, removing the local duplication and the stale NOTE about the (real) circular-import workaround.

Also trimmed the PR description to drop the removed DST/croniter test section, matching the 19 tests actually shipped.

Verification: make lint clean; focused suite 36 passed; full make test → 12391 passed, 78 skipped, 0 failures.

Note: CI on this new head is gated by the first-time-contributor "Approve and run workflows" action — could you approve the new check runs so we can confirm backend-unit-tests is green? (The prior failure was test_agent_guidance_check.py::test_repository_guidance_stays_below_soft_budgets_and_avoids_doc_indexes, which passes locally and is unrelated to these changes.)

[Totoro-qaq]

One final cleanup push: 45558b43 changes only _finalise_once_task_from_run()'s return annotation from None to bool, matching its documented contract and both callers.

Verification on this exact change:

• annotation assertion: RED (None) -> GREEN (bool)
• uv run pytest tests/test_scheduler_completion_consistency.py -q: 19 passed
• make lint: ruff check passed; 1,235 files already formatted
• make test with localhost excluded from the local proxy: 12,388 passed, 81 skipped, 0 failed

One remaining CI/documentation question: the requested one-line backend/AGENTS.md note now makes the synthetic merge exceed the repository's soft guidance budget (28,902 > 28,672 bytes). This was the sole failure in the previous Unit Tests run; the rest of that run passed. I have not relocated the guidance to a different scoped AGENTS.md, because I do not want to choose project guidance ownership as an external contributor. Would you prefer me to fold it into the existing scheduled-task paragraph in backend/AGENTS.md, or drop the note from this PR?

[willem-bd]

This head cleanly resolves both prior P1s: the latest run is re-read per task after the parent lock (_fetch_latest_run with populate_existing), active occurrences are left untouched instead of generic-cancelled, and the single-instance poll loop now re-runs cancel_stuck_once_tasks so the "a later recovery pass will finalize it" justification holds there. The canonical status constants in scheduled_tasks/model.py also close out the duplication/NOTE round, and the test helper's created_at write-back makes the multi-history ordering real. Three follow-ups below — the first is the substantive one: the ordering key behind latest run changed this revision from created_at DESC, id DESC to scheduled_for DESC with no tie-break, and scheduled_for is not a recency key.

[Totoro-qaq]

Rebased onto current main (0d492530) after #5216 landed; no scheduler logic changed since 50f54f20 (now 99450ebd), head is cd75cbb9.

• 0022_scheduled_occurrence_seq now chains after canonical 0019_thread_incarnations, so the tree keeps a single head and a database at canonical 0019 validates the fixed floor and then upgrades to 0022 as intended.
• Main's forward-revision tests assumed canonical 0019 was the local head. cd75cbb9 makes them follow the real head: the canonical-0019 shape is seeded explicitly, upgraded databases are asserted against _get_head_revision(), the simulated 0020 rollback binary's revision set is derived from the ancestors of its own head, and the PostgreSQL restart scenario steps the schema back to canonical 0019 before the rollback binary restarts. The batch-acceptance guard regression keeps main's expectation unchanged.

Validation at cd75cbb9: 290 focused SQLite/PostgreSQL tests (forward-revision compatibility incl. the PostgreSQL restart case, migrations 0019/0021/0022, bootstrap, scheduler ordering/completion/lifespan/trace/guidance), 119 blocking-I/O tests, make lint and ruff format --check pass; the twelve real-runtime scenarios pass with the local replay endpoint. Full offline backend run: 14,595 passed in the sandboxed uv run pytest run plus the 15 browser/fetch cases that need public DNS rerun in a terminal with normal networking (15 passed, user-terminal-baseline15-p4.log); 14,610 executed cases pass, 178 skipped, 3 deselected.

[Huixin615]

One non-blocking documentation nit: persistence/migrations/AGENTS.md still describes the local chain as ending at 0019_thread_incarnations, while the current head is now 0022_scheduled_occurrence_seq

[Totoro-qaq]

@Huixin615 Thanks, fixed in cbf46fcb: the rolling-forward section now describes the chain through 0022_scheduled_occurrence_seq as the current head and lists it among the revisions the 0020 rollback-floor binary does not know. Documentation only; no code change since cd75cbb9.

[Totoro-qaq]

Both CI failures on 7f7ccc1 came from this PR's own test setup and docs, not scheduler logic, and are fixed in aa3fe16: the new Postgres fixtures now accept CI's postgresql://...?sslmode=disable URL (normalized to asyncpg, like the 0019 migration tests), and backend/AGENTS.md is back to main's size so the middlewares guidance chain no longer grows. Could the workflows be approved again?