deer-flow/backend/tests/test_scheduled_occurrence_sequence.py
Totoro cd0e74edaf
fix(scheduler): reconcile stuck once tasks from committed run outcome (#5035)
* fix(scheduler): reconcile stuck once tasks from committed run outcome

Restart recovery (cancel_stuck_once_tasks and the multi-instance
reconcile_stuck_once_tasks) blindly flipped every stuck once-task to
'cancelled'. When handle_run_completion crashed between its two
transactions, a once-task whose run had already committed 'success'
was permanently reported as cancelled.

Both reconciliation paths now read the latest scheduled_task_runs row
without a status filter and finalize the parent to match:
success -> completed (last_error cleared),
failed -> failed with the run's error,
interrupted -> cancelled with the run's error when present,
skipped -> cancelled (no work performed).
Active occurrences (queued/launching/running) are left untouched — a
concurrent completion or a later recovery pass will finalize them once
the run reaches a terminal state.
Tasks without a terminal run row keep the previous generic cancellation.

Review follow-ups (willem-bd / Huixin615):
- Extract _finalise_once_task_from_run() so both recovery paths share one
  outcome mapping (no more drift between single- and multi-instance paths).
  Returns bool (True = finalised, False = active/no-op) for explicit
  counter management at call sites.
- Fix a no-op (`run_row.error or None` -> `run_row.error`) in the skipped
  branch.
- Drop the unused `status` parameter from the test task helpers.
- Use TERMINAL_RUN_STATUSES / ACTIVE_RUN_STATUSES constants (local copies
  to avoid circular import; kept in sync with scheduled_task_runs.sql).
- [P1] Read the latest run AFTER acquiring the parent task row lock, not from
  a pre-lock batch snapshot. The latest-run lookup now runs per task under
  the lock with populate_existing so a concurrently committed status is read
  back fresh.
- [P2] Race tests now use monkeypatch to actually enter the race window:
  _intercepted_fetch commits success in a separate session at the moment the
  per-task fetch fires, so a reverted pre-lock batch implementation fails the
  test, while the current post-lock implementation passes.
- [P1] Do not finalize parent for active occurrences. A non-terminal
  scheduled occurrence means the run is still in progress — the parent must
  be left untouched until the completion path or a later recovery pass
  establishes a terminal outcome.
- [P2] Add cancel_stuck_once_tasks to the single-instance poll loop so
  stuck once-tasks are not left permanently "running" when the startup sweep
  fails (mirrors multi-instance _reconcile_active_state behavior).
- Fix stale docstrings in cancel_stuck_once_tasks and _fetch_latest_run.

Adds regression tests for multiple historical runs (older success +
newer skipped/active) on both paths, monkeypatch-based race tests that
prove a concurrent completion committing success is reflected as
completed, and active-run tests that verify the parent is left
unchanged. Documents the behavior in AGENTS.md.

Fixes #5034

* fix(scheduler): address review comments on completion-consistency fix

- _fetch_latest_run: drop arbitrary id DESC tie-break; order by
  scheduled_for DESC (deterministic recency on schedule position)
- _finalise_once_task_from_run: annotate bool return type
- Centralize TERMINAL/ACTIVE_RUN_STATUSES in scheduled_tasks/model.py;
  stop duplicating them in scheduled_tasks/sql.py and
  scheduled_task_runs/sql.py (removes stale circular-import workaround)
- cancel_stuck_once_tasks: run unconditionally in single-instance poll
  loop (remove try/except swallow)
- tests: pin created_at/scheduled_for in _create_run so recency ordering
  is actually exercised; correct docstrings that described the
  active-occurrence branch as 'generic cancel' instead of 'left unchanged'

* fix(scheduler): correct finalizer return annotation

* fix: order scheduled task runs by creation time

* fix(scheduler): stabilize latest run reconciliation ordering

* fix(scheduler): order latest runs by creation time

* test: update trace scheduler stub

* fix(scheduler): clarify reconciliation diagnostics

Signed-off-by: Totoro-qaq <279883115+Totoro-qaq@users.noreply.github.com>

* fix(scheduler): fail closed on startup recovery

Keep single-instance parent reconciliation at startup so it cannot race manual admission. Propagate recovery failures through the Gateway lifespan before channel startup, preventing a half-started scheduler.

Tests cover both recovery failure stages and a queued occurrence that survives startup before the ordinary poll drain launches it.

Signed-off-by: Totoro-qaq <279883115+Totoro-qaq@users.noreply.github.com>

* fix(scheduler): order occurrences and fence stale parent writes

Allocate per-task occurrence sequences under the parent lock and guard parent projection across launch, recovery, completion, and queue failure paths. Track launch accounting separately so stale occurrences are counted once without replacing newer results. Commit completion and accounting atomically, preserve legacy history, and cover migrations and reordered execution on SQLite and PostgreSQL.

* fix(scheduler): tighten completion projection and launch fencing diagnostics

Share the once-task outcome mapping between completion and both recovery paths, validate the terminal status before opening the completion transaction, leave cron parent status untouched on completion, log the fenced launch update when an occurrence does not belong to the launched run, and drop the README capability line.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>

* fix(scheduler): compare caller time only against unsequenced occurrences

Among sequenced rows the parent-locked occurrence_seq is the only recency key. An unsequenced row can only be legacy history or an admission by a pre-upgrade Gateway writer, so recovery prefers it over the sequence winner only when its caller timestamp is later, which is the previous ordering for that pair. A rolling upgrade therefore degrades to the pre-sequence behaviour instead of ranking every pre-upgrade admission below every sequenced one. Document that boundary instead of requiring every Gateway writer to stop before the upgrade.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>

* fix(scheduler): gate once-task recovery on the same projection rule

Recovery now finalises a once-task parent only from the occurrence that can_project() accepts: the highest sequenced occurrence whenever one exists, or the timestamp-latest row for a task whose history is entirely unsequenced. An unsequenced row admitted by a pre-upgrade writer can no longer cancel a parent whose sequenced occurrence is still live, nor stall finalisation of a parent whose sequenced occurrence already completed. Document that pre-upgrade instances project their own admissions during a rolling upgrade.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>

* fix(scheduler): defer once-task recovery while any occurrence is live

uq_scheduled_task_run_active allows one non-terminal occurrence per task, so a live row is the task's newest admission whatever its caller clock and whether it carries a sequence. Both once-task recovery paths now probe for any active occurrence after the fresh latest-run read and leave the parent untouched while one exists; cancel_stuck_once_tasks also locks the parent row so admission cannot insert a queued occurrence between that probe and the commit. Once no occurrence is live, the sequence winner decides and a terminalised unsequenced row never overrides it.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>

* test(persistence): follow the local head past canonical 0019

Main's forward-revision tests assumed 0019_thread_incarnations was the local chain head. With 0022_scheduled_occurrence_seq chained after it, seed the canonical-0019 shape explicitly, assert the real head where a database is upgraded, derive the 0020 rollback binary's revision set from the ancestors of its head, and step the PostgreSQL restart scenario back to canonical 0019 before the rollback binary restarts.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>

* docs(migrations): describe the chain through 0022_scheduled_occurrence_seq

The rolling-forward section still ended the local chain at canonical 0019; it now names 0022_scheduled_occurrence_seq as the head and lists it among the revisions the 0020 rollback-floor binary does not know.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>

* test(scheduler): accept CI's sync Postgres URL in occurrence fixtures

CI hands over TEST_POSTGRES_URI as postgresql://...?sslmode=disable. The occurrence, ordering and 0022 migration fixtures built async engines from it directly, so SQLAlchemy chose psycopg2, which is not installed. Normalize the scheme to postgresql+asyncpg and drop libpq-only query keys, matching the existing 0019 migration tests.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

* docs(scheduler): keep the backend AGENTS.md chain within its budget

The middlewares guidance chain was already above the hard limit on main, so any added byte in backend/AGENTS.md fails the agent guidance check. Leave backend/AGENTS.md identical to main and record the recovery projection rule in the 0022 migration entry, which already describes the occurrence fields.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

---------

Signed-off-by: Totoro-qaq <279883115+Totoro-qaq@users.noreply.github.com>
Co-authored-by: Totoro-qaq <279883115+Totoro-qaq@users.noreply.github.com>
Co-authored-by: Claude Fable 5.1 <noreply@anthropic.com>
2026-09-12 07:35:07 +08:00

315 lines
15 KiB
Python

"""Database ordering is per task and survives retries independently of caller clocks."""
from __future__ import annotations
import asyncio
import os
import uuid
from datetime import UTC, datetime, timedelta
from unittest.mock import patch
from urllib.parse import parse_qsl, urlencode, urlsplit, urlunsplit
import pytest
import pytest_asyncio
from sqlalchemy import select, text
from sqlalchemy.exc import IntegrityError
from sqlalchemy.ext.asyncio import async_sessionmaker, create_async_engine
import deerflow.persistence.models # noqa: F401
from deerflow.persistence.base import Base
from deerflow.persistence.postgres_schema import build_asyncpg_connect_args
from deerflow.persistence.scheduled_task_runs import ActiveScheduledRunConflict, ScheduledTaskRunRepository
from deerflow.persistence.scheduled_task_runs.model import ScheduledTaskRunRow
from deerflow.persistence.scheduled_tasks import ScheduledTaskRepository
from deerflow.persistence.scheduled_tasks.model import ACTIVE_RUN_STATUSES, ONCE_TASK_STATUS_BY_RUN_STATUS, ScheduledTaskRow
pytestmark = pytest.mark.asyncio
@pytest_asyncio.fixture(params=["sqlite", "postgres"])
async def occurrence_factories(request, tmp_path):
"""Two pools guarantee competing admissions use independent DB connections."""
schema = None
if request.param == "postgres":
uri = os.environ.get("TEST_POSTGRES_URI")
if not uri:
pytest.skip("requires TEST_POSTGRES_URI (real Postgres for occurrence ordering)")
parts = urlsplit(uri)
# CI passes a sync ``postgresql://...?sslmode=disable`` URL; the async
# engine needs the asyncpg driver and rejects libpq-only query keys.
scheme = "postgresql+asyncpg" if parts.scheme in {"postgres", "postgresql"} else parts.scheme
query = urlencode([(key, value) for key, value in parse_qsl(parts.query, keep_blank_values=True) if key not in {"sslmode", "channel_binding"}])
uri = urlunsplit(parts._replace(scheme=scheme, query=query))
schema = f"occurrence_{uuid.uuid4().hex}"
options = {"connect_args": build_asyncpg_connect_args(schema)}
else:
uri = f"sqlite+aiosqlite:///{tmp_path / 'occurrences.db'}"
options = {"connect_args": {"timeout": 30}}
engines = [create_async_engine(uri, **options) for _ in range(2)]
try:
async with engines[0].begin() as connection:
if schema:
await connection.execute(text(f'CREATE SCHEMA "{schema}"'))
await connection.run_sync(Base.metadata.create_all)
yield tuple(async_sessionmaker(engine, expire_on_commit=False) for engine in engines)
finally:
if schema:
async with engines[0].begin() as connection:
await connection.execute(text(f'DROP SCHEMA IF EXISTS "{schema}" CASCADE'))
for engine in engines:
await engine.dispose()
async def _create_task(factory, task_id="task", *, schedule_type="cron"):
spec = {"cron": "* * * * *"} if schedule_type == "cron" else {"run_at": datetime(2026, 7, 15, 12, 0, tzinfo=UTC).isoformat()}
return await ScheduledTaskRepository(factory).create(
task_id=task_id,
user_id="user-1",
thread_id="thread-1",
context_mode="reuse_thread",
assistant_id=None,
title="Occurrence ordering",
prompt="p",
schedule_type=schedule_type,
schedule_spec=spec,
timezone="UTC",
next_run_at=None,
)
async def _create_run(factory, run_id, *, task_id="task", status="success"):
return await ScheduledTaskRunRepository(factory).create(
run_record_id=run_id,
task_id=task_id,
thread_id=f"thread-{run_id}",
scheduled_for=datetime(2026, 7, 15, 12, 0, tzinfo=UTC),
trigger="manual",
status=status,
)
async def _sequence(factory, run_id):
async with factory() as session:
return await session.scalar(select(ScheduledTaskRunRow.occurrence_seq).where(ScheduledTaskRunRow.id == run_id))
async def _high_water_mark(factory, task_id="task"):
async with factory() as session:
return await session.scalar(select(ScheduledTaskRow.last_occurrence_seq).where(ScheduledTaskRow.id == task_id))
async def test_concurrent_allocations_use_distinct_monotonic_sequences(occurrence_factories):
first, second = occurrence_factories
original = await _create_task(first)
ready = [asyncio.Event(), asyncio.Event()]
start = asyncio.Event()
async def admit(factory, lane):
ready[lane].set()
await start.wait()
for index in range(4):
await _create_run(factory, f"run-{lane}-{index}")
admissions = [asyncio.create_task(admit(factory, lane)) for lane, factory in enumerate((first, second))]
await asyncio.gather(*(event.wait() for event in ready))
start.set()
await asyncio.gather(*admissions)
sequences = [await _sequence(first, f"run-{lane}-{index}") for lane in range(2) for index in range(4)]
assert sorted(sequences) == list(range(1, 9))
for lane in range(2):
lane_sequences = sequences[lane * 4 : (lane + 1) * 4]
assert lane_sequences == sorted(lane_sequences)
assert await _high_water_mark(first) == 8
current = await ScheduledTaskRepository(first).get("task", user_id="user-1")
assert current["updated_at"] == original["updated_at"]
assert current["run_count"] == 0
async def test_sequence_allocation_is_independent_per_task(occurrence_factories):
first, second = occurrence_factories
for task_id in ("task-a", "task-b"):
await _create_task(first, task_id)
await _create_run(first, "run-a1", task_id="task-a")
await _create_run(second, "run-a2", task_id="task-a")
await _create_run(second, "run-b1", task_id="task-b")
assert [await _sequence(first, run_id) for run_id in ("run-a1", "run-a2", "run-b1")] == [1, 2, 1]
async def test_active_conflict_rolls_back_sequence_allocation(occurrence_factories):
first, second = occurrence_factories
await _create_task(first)
await _create_run(first, "active", status="queued")
with pytest.raises(ActiveScheduledRunConflict):
await _create_run(second, "rejected", status="queued")
assert await _high_water_mark(first) == 1
assert await _sequence(first, "rejected") is None
await ScheduledTaskRunRepository(first).update_status("active", status="success")
await _create_run(second, "accepted", status="queued")
assert await _sequence(first, "accepted") == 2
@pytest.mark.parametrize("status", ["queued", "success"])
async def test_primary_key_conflict_is_not_an_active_conflict_and_rolls_back(occurrence_factories, status):
first, second = occurrence_factories
await _create_task(first, "task-a")
await _create_task(first, "task-b")
await _create_run(first, "duplicate", task_id="task-a")
with pytest.raises(IntegrityError):
await _create_run(second, "duplicate", task_id="task-b", status=status)
assert await _high_water_mark(first, "task-b") == 0
await _create_run(second, "unique", task_id="task-b", status=status)
assert await _sequence(first, "unique") == 1
async def test_requeue_and_reclaim_preserve_occurrence_sequence(occurrence_factories):
first, second = occurrence_factories
await _create_task(first)
await _create_run(first, "retry", status="queued")
now = datetime(2026, 7, 15, 12, 0, tzinfo=UTC)
repo = ScheduledTaskRunRepository(second)
for attempt in range(2):
claimed = await repo.claim_queued_run("retry", now=now, lease_owner="worker", lease_seconds=60, global_max_concurrent_runs=1)
assert claimed is not None
assert claimed["attempt_count"] == attempt + 1
assert await repo.requeue_claimed_run("retry", lease_owner="worker") is True
assert await _sequence(first, "retry") == 1
assert await _high_water_mark(first) == 1
async def test_internal_sequence_fields_are_absent_from_repository_responses(occurrence_factories):
first, _second = occurrence_factories
created_task = await _create_task(first)
task_repo = ScheduledTaskRepository(first)
run_repo = ScheduledTaskRunRepository(first)
created_run = await _create_run(first, "queued", status="queued")
task_responses = [created_task, await task_repo.get("task", user_id="user-1"), *(await task_repo.list_by_user("user-1"))]
run_responses = [created_run, await run_repo.get_active_run("task"), *(await run_repo.list_by_task("task")), *(await run_repo.list_queued_runs(limit=10))]
for response in task_responses + run_responses:
assert {"last_occurrence_seq", "occurrence_seq", "launch_accounted"}.isdisjoint(response)
assert await _sequence(first, "queued") == 1
async def _insert_unsequenced_run(factory, run_id, *, created_at, status="success"):
"""Insert without the repository: legacy history or a pre-upgrade writer."""
async with factory() as session:
session.add(
ScheduledTaskRunRow(
id=run_id,
task_id="task",
thread_id=f"thread-{run_id}",
scheduled_for=created_at,
created_at=created_at,
trigger="manual",
status=status,
)
)
await session.commit()
async def _latest_run_id(factory):
async with factory() as session:
latest = await ScheduledTaskRepository._fetch_latest_run(session, "task")
assert latest is not None
return latest.id
async def test_recovery_order_keeps_timestamp_fallback_for_legacy_only_history(occurrence_factories):
first, _second = occurrence_factories
await _create_task(first)
now = datetime(2026, 7, 15, 12, 0, tzinfo=UTC)
# Unsequenced history is deliberately not assigned guessed sequence values.
for index in range(2):
await _insert_unsequenced_run(first, f"legacy-{index}", created_at=now + timedelta(days=index))
assert await _latest_run_id(first) == "legacy-1"
assert await _sequence(first, "legacy-0") is None
assert await _sequence(first, "legacy-1") is None
assert await _high_water_mark(first) == 0
@pytest.mark.parametrize("unsequenced_status", ["skipped", "running"])
@pytest.mark.parametrize("unsequenced_offset", [timedelta(days=-365), timedelta(seconds=30)], ids=["legacy-history-is-older", "pre-upgrade-writer-is-newer"])
async def test_recovery_lookup_prefers_the_highest_sequence_whenever_one_exists(occurrence_factories, unsequenced_offset, unsequenced_status):
"""Sequence decides whenever a sequenced row exists.
Reversed caller clocks between sequenced rows do not matter, and an
unsequenced row (legacy history or a pre-upgrade Gateway writer) is not
consulted even when its caller timestamp is later: the lookup returns the
same row ``can_project`` accepts, so recovery cannot act on a row that the
other parent writes would reject.
"""
first, second = occurrence_factories
await _create_task(first)
now = datetime(2026, 7, 15, 12, 0, tzinfo=UTC)
# The later sequence carries the earlier caller clock: sequence still wins.
with patch("deerflow.persistence.scheduled_task_runs.sql.datetime") as clock:
clock.now.return_value = now + timedelta(seconds=30)
await _create_run(first, "older-sequenced")
clock.now.return_value = now
await _create_run(second, "newer-sequenced")
await _insert_unsequenced_run(first, "unsequenced", created_at=now + unsequenced_offset, status=unsequenced_status)
assert [await _sequence(first, run_id) for run_id in ("older-sequenced", "newer-sequenced", "unsequenced")] == [1, 2, None]
assert await _latest_run_id(first) == "newer-sequenced"
@pytest.mark.parametrize("recovery_method", ["cancel_stuck_once_tasks", "reconcile_stuck_once_tasks"])
@pytest.mark.parametrize(
("sequenced_status", "unsequenced_status"),
[("running", "skipped"), ("success", "running"), ("success", "interrupted"), ("failed", "running")],
ids=[
"unsequenced-skipped-while-sequenced-active",
"unsequenced-running-while-sequenced-success",
"unsequenced-interrupted-while-sequenced-success",
"unsequenced-running-while-sequenced-failed",
],
)
async def test_once_recovery_defers_while_any_occurrence_is_live_then_projects_the_sequence_winner(occurrence_factories, recovery_method, sequenced_status, unsequenced_status):
"""Mixed-writer interleavings from review, both recovery paths, both backends.
A live occurrence row is the task's newest admission by construction
(``uq_scheduled_task_run_active``), whatever its caller clock and whether or
not it carries a sequence, so recovery defers while one exists: an
unsequenced ``skipped`` row cannot cancel a parent whose sequenced
occurrence is live, and an unsequenced ``running`` row cannot be skipped
over to finalise the parent from an older sequenced outcome. Once no row is
live, the sequence winner decides and a terminalised unsequenced row never
overrides it.
"""
first, _second = occurrence_factories
await _create_task(first, schedule_type="once")
task_repo = ScheduledTaskRepository(first)
await task_repo.update("task", user_id="user-1", updates={"status": "running"})
await _create_run(first, "sequenced", status=sequenced_status)
# A pre-upgrade node on a skewed clock: no sequence, later caller timestamp.
await _insert_unsequenced_run(first, "unsequenced", created_at=datetime.now(UTC) + timedelta(minutes=5), status=unsequenced_status)
assert await _sequence(first, "sequenced") == 1
assert await _high_water_mark(first) == 1
kwargs = {"error": "interrupted: recovery"}
if recovery_method == "reconcile_stuck_once_tasks":
kwargs["now"] = datetime.now(UTC) + timedelta(minutes=10)
async def recover():
count = await getattr(task_repo, recovery_method)(**kwargs)
task = await task_repo.get_internal("task")
assert task is not None
return count, task
any_live = sequenced_status in ACTIVE_RUN_STATUSES or unsequenced_status in ACTIVE_RUN_STATUSES
sequence_outcome = ONCE_TASK_STATUS_BY_RUN_STATUS.get(sequenced_status, "running")
expected_status, expected_count = ("running", 0) if any_live else (sequence_outcome, 1)
for _ in range(2): # a second pass must not change the outcome
count, task = await recover()
assert task["status"] == expected_status
assert task["last_error"] is None
assert count == expected_count
expected_count = 0
if unsequenced_status in ACTIVE_RUN_STATUSES:
# The pre-upgrade node died and occurrence recovery terminalised its
# row: the sequence winner now decides, not the newer unsequenced row.
assert await ScheduledTaskRunRepository(first).update_status("unsequenced", status="interrupted", error="pre-upgrade node died")
count, task = await recover()
assert task["status"] == sequence_outcome
assert task["last_error"] is None
assert count == 1