Remove the legacy stack the hexagonal slice replaced, now that nothing
assembles it: app/scheduler/service.py (the old orchestration),
app/gateway/routers/scheduled_tasks.py (the old dict-returning router,
no longer mounted), the deerflow/scheduler package (its cron/timezone
rules live in ScheduleSpec), the dict-returning repositories in
persistence/scheduled_task*/sql.py, and the deps.py providers and
app.state wiring that served them. The ORM rows and the
uq_scheduled_task_run_active partial unique index stay -- the table
definitions live with the shared engine/alembic infrastructure and the
schedule adapters are their only readers and writers.
The legacy test suites go with the code they pinned; every scenario has
a counterpart in the new suites. The one suite that guarded semantics
rather than the old implementation -- the real-database dispatch-race
TOCTOU tests -- is migrated to the new stack as
test_schedule_dispatch_race.py, driving ScheduleService over the real
SQL adapters with the same barrier, natural-timing, and index-semantics
cases.
Docs and comments that named the old classes as the current wiring
(backend/AGENTS.md, reload_boundary.py, channel/service comments) now
name the composition-root wiring instead.
created_at was minted twice for one construction: the aggregate's field
default read the clock, then the adapter's add() read it again and stored
its own value -- so the aggregate a caller held disagreed with the stored
row by however long the insert took, and the model docstring pointed at a
write path that no longer exists.
ScheduledTask.create now stamps created_at and updated_at from the
explicit now= it already receives (one clock reading, the rule input),
and the adapter persists the aggregate's instants verbatim on insert --
matching the feedback reference implementation. updated_at's later life
stays deliberately with the adapter's write paths: the CAS methods
(record_launch / record_completion) write the row without ever holding an
aggregate, so only storage can stamp 'last written'. The model docstring
now states that ownership split instead of citing the deleted legacy
repository. Pinned by a domain test on the factory and a contract case
on add() across both implementations.
Move model/errors.py up one level to domain/schedule/exceptions.py, a
sibling of the model, matching the AWS domain layout the spec mandates
(exceptions/ is its own member of the domain folder, not part of the
model) and the feedback reference implementation. Class names keep the
PEP 8 Error suffix. Pure move -- the nine classes are AST-identical to
the originals; imports across the domain, adapters, router, and tests
now take errors from deerflow.domain.schedule.exceptions.
All 102 domain cases were green, and four of them would have stayed green
through the exact regression they were named after.
Assertions that could not fail
------------------------------
`test_a_claimed_task_is_marked_running_before_dispatch` asserted the lease was
released *after* dispatch -- the opposite of the ordering its name and
docstring describe. The claim is what makes a task uneditable while it is
being dispatched, so the only place that ordering is observable is inside the
launch; the launcher double now reads the repository from there.
`test_active_statuses_are_exactly_queued_and_running` restated the constant it
was checking, so editing the constant edits the assertion with it. Replaced by
`is_active` over all six statuses, which also covers `RUNNING` and the two
terminal statuses that had none.
`test_reuse_thread_with_an_empty_thread_falls_back_to_a_fresh_one` compared
its result against `task.thread_id`, which is `None` on the default task -- it
asserted "not None" against a method whose body is `str(uuid.uuid4())`. Now
asserts the fresh-thread semantics it is named for: a real uuid, distinct per
call.
`test_a_task_deleted_mid_flight_is_not_an_error` had no assert at all. That
path does have observable behaviour: the hook writes the run record before it
reads the task, so a task deleted mid-flight must still leave a finalized
record and a freed active slot.
Contracts stated in a docstring and nowhere else
------------------------------------------------
- a cron overlap must not leave `last_error` behind (service.py:455 branches
on it; only the `once` half was covered, so dropping the branch was free)
- a failed launch replaces the launch bookkeeping instead of carrying it over
the way a skip does -- which is what `last_run_id=None` in `_fail` means for
a task that had already run successfully
- `SchedulePolicy`'s defaults are the permissive ones, so a deployment that
configured no policy cannot have a business constraint invented for it
- transitions leave `updated_at` to the repository, rather than becoming a
second source of truth for the same column
ACTIVE_RUN_STATUSES' promised assertion
---------------------------------------
Its docstring says the check that it stays in lockstep with the partial unique
index's predicate "lives in a separate test module rather than the domain
tests". It did not exist, in that module or any other. `test_scheduled_task_
models.py` now reads both dialect predicates off `__table_args__` and compares
the values it extracts against the constant. The domain suite cannot do this
-- it is deliberately dependency-free and cannot import an ORM model -- so the
new domain case names where the other half of the rule lives.
Removed
-------
Three duplicates: an `INTERRUPTED -> CANCELLED` case the parametrize directly
above it already made, a trailing-Z case identical in path to the aware-run_at
case beside it (the `from_primitives` one is the real one, because it parses a
string), and an `ensure_launchable == next_after` case whose value another
case already asserts outright. Their reasoning moved into comments where it
still applies. The second copy of the tautology, in `test_schedule_fakes.py`,
goes with it.
Verification
------------
A green run is not evidence for this kind of change, so every new or rewritten
assertion was checked by mutation -- break the production rule, confirm the
guarding test fails. All nine caught, re-run after `ruff format` to confirm
the reformat did not soften any of them.
Net +207/-31 across four test files; 425 passed, 3 skipped for the schedule
and composition suites.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
A cron task could become permanently unschedulable after a run that failed
fast. Reproduced against the real SQL adapters, and against the legacy path
for contrast -- the legacy code does not have this bug, so the hexagonal
slice introduced it.
The window
----------
`dispatch_task` already documents it: "a fast-failing run can reach
handle_run_completion before this write lands". The hook therefore reads the
task while the dispatch path has not yet written its bookkeeping, and the
snapshot it holds still carries the elapsed `next_run_at` the claim was made
on. `handle_run_completion` then wrote that whole snapshot back through
`save()`, so whichever landed second undid the other.
For a cron task the result is terminal in the worst way. `record_launch`
writes the fresh fire time, bumps `run_count` and clears the claim; the
completion's whole-aggregate write restores the elapsed fire time, rolls
`run_count` back and leaves `status='running'` with `lease_expires_at IS
NULL`. Neither `claim_due` branch matches that shape (one needs `enabled`,
the other needs an expired claim), and `cancel_stuck_once_tasks` only sweeps
`once` rows -- so nothing can ever reach the task again.
The legacy `app/scheduler/service.py` wrote the same outcome field by field
(`update(..., updates={"last_error": ...})`) and never touched scheduling
state, which is why it survives the same interleaving.
The fix
-------
`record_completion` joins `record_launch` as a second deliberately narrow
port method, for the same stated reason `record_launch` is not expressed as
`save(task)`: the two race, so neither may write through the aggregate. They
now own disjoint fields -- the launch owns the schedule, the completion owns
the verdict (terminal status plus `last_error`, with `None` meaning "do not
move the status", i.e. every cron task).
`save()` keeps its documented purpose, the user-initiated whole-aggregate
updates (`update_task` / `pause` / `resume`). `handle_run_completion` still
reads the task first, but only to ask `status_after_completion`, which reads
nothing but the schedule type -- immutable, so that read carries no
time-of-check risk.
Tests
-----
The contract suite missed this because it groups cases by port method:
`record_launch` appears only among its own, never interleaved with `save`,
and `test_protect_terminal_keeps_a_concurrently_finalized_verdict` pins the
mirror-image direction only. The in-memory double replaces the whole row too,
so both implementations were faithfully wrong -- the defect was in the
contract, not either adapter.
Added: `TestRecordCompletion` (5 cases, both implementations), including the
interleaving itself; and `test_a_cron_task_survives_a_launch_write_landing_mid_completion`,
which drives a real `ScheduleService` over a repo double that commits
`record_launch` between the hook's read and its write. Both were watched
failing first -- the service case failing with the actual symptom, a rolled
back `next_run_at`, not a missing method.
Verified end to end against file-backed sqlite with the real service: the
task keeps `enabled`, the concurrent fire time and run count survive, the
verdict is recorded, and the next poll still claims it.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Adopts the layout feedback landed in cb49dd67: secondary adapters live
under `app/adapters/<context>/`, one file per port, the file named after
the port in snake_case with the technology carried by the class name.
`app/infra/` is now gone entirely.
The rename also separates two meanings of "run" that shared one filename
space: `run_sql.py` held `ScheduledRun` (an execution record), while the
`run_launcher.py` still to come deals in Gateway runs.
task_sql.py -> scheduled_task_repository.py
run_sql.py -> scheduled_run_repository.py
spec_mapping.py -> spec_mapping.py (implements no port, so no rename)
Both SQL adapters now carry the `Secondary adapter (owned persistence)`
docstring marker -- this context owns both tables and writes its own
queries. `spec_mapping` says instead that it is a boundary mapping and
names its two callers. The package `__init__.py` is empty, so imports go
through the full path and a class's home file stays unambiguous.
Pure move: every top-level symbol was compared against its pre-move
original by AST dump (docstrings excluded), plus a separate per-class
method-name comparison, since a whole-class dump reports a docstring edit
and a renamed method the same way.
Also repoints three docstrings and one diagram that still named the
deleted `app/infra/` path.
The inner ring is organised per bounded context; the outer ring was not,
so this context's adapters were split across app/infra/persistence/ and
app/infra/schedule/ with no principle separating them — one held the SQL
repositories, the other held the mapping they both consume, purely
because of what technology each touched.
Everything the outer ring provides to the schedule context now lives in
app/infra/schedule/, mirroring domain/schedule/. That also gives the two
remaining adapters an obvious home: the run launcher and the thread
lookup are neither persistence nor mapping, and would have needed a
third rule under the old layout.
Files are moved with git mv so history follows them. app/infra/
persistence/feedback.py stays where it is: it belongs to a separate
migration and moving it here would put that work in this diff. The
package docstring records the asymmetry rather than leaving it to be
rediscovered.
Two secondary adapters plus the spec mapping they share, and the first
real payoff of the port boundary: test_schedule_fakes.py becomes a
contract suite that runs all 31 cases against both the in-memory doubles
and the SQL adapters on a real sqlite file. A rule stated in a port
docstring now has to hold for both, and a divergence is a failure rather
than a surprise in production.
The queries come over unchanged. The claim statement's FOR UPDATE SKIP
LOCKED and both protect_terminal conditional writes are this module's
concurrency contract, not style, and the IntegrityError translation in
add() is what lets the service collapse a lost active-slot race into the
same outcome as its own non-atomic fast path.
Two things the adapters own that the domain deliberately does not. The
claiming process's identity is generated here, because who claimed a
task is an identity rather than a rule and nothing reads it back. And
`Unsupported schedule_type` is raised by the mapping, because
ScheduleSpec only accepts the enum -- structural checks belong to the
boundary, value rules to __post_init__, and both surface as the same
domain error so the router maps one family.
_to_domain introduces a failure mode the legacy repository did not have:
a row whose stored schedule no longer parses. Single-row reads let it
propagate; list reads skip and log, so one corrupt row cannot 500 an
entire listing.
The contract suite reaches past the port in exactly one place -- seeding
a task that already carries a claim, a shape claim_due would never
produce -- and says so where it does.
Four changes, all from the same observation -- the service was expressing
domain concepts in language the domain does not use.
DispatchOutcome replaces the Literal. The caller branches on all four
values and the SKIPPED/CONFLICT distinction is itself a business rule, so
it is domain vocabulary like every other enum in this context; a bare
string was the odd one out.
lease_owner leaves the domain entirely. It was a pure pass-through: the
service held it only to hand it to claim_due, which never reads it back.
Which process claimed a task is an identity, not a rule -- an adapter can
record one for diagnostics without the domain carrying it. lease_seconds
stays, because how long a claim survives genuinely changes recovery
behaviour.
The _UNSET sentinel is gone. It existed for one field: thread_id, the only
update parameter with a meaningful None. But thread_id and context_mode
always move together, so packaging them as ContextChange removes the
ambiguity and lets every other field use plain None for "not supplied" --
which also matches what the HTTP layer already does with exclude_none.
The previous `title: str = _UNSET` annotation was simply untrue.
pause/resume no longer route through a _transition helper taking an
unbound method. Two direct bodies plus a _save that raises on a missing
row read better, and _save documents why this stays a read-modify-write:
pushing "not while running" into a storage predicate would put the rule
beyond a zero-IO test and give it a second home. Closing that window
properly needs optimistic locking, which needs a schema change.
Also cleans three lint findings surfaced by a broader rule set than the
project enables: a regex metacharacter in a pytest match=, an unused
override parameter, and two deliberately-naive datetimes that now say so
with a noqa rather than looking accidental.
Four Protocols the domain declares and the outer ring will implement,
plus the two DTOs that keep infrastructure types out of the inner ring:
- ScheduledTaskRepository / ScheduledRunRepository, exchanging domain
objects rather than the bare dicts the current repositories return
- RunLauncher, whose contract is that only ThreadBusyError or
LaunchFailedError may escape -- that translation is what keeps the run
runtime and the web framework out of the domain
- ThreadLookup, one method rather than the whole thread store
- LaunchedRun and RunOutcome, so the completion path stops taking a
runtime record the purity test would reject
Two deliberate departures from the earlier sketch. There is no Clock
port: `now` is already an explicit parameter throughout, so the domain
never reads a clock and the tests are already deterministic -- adding
one would only create a second source of truth for the same value. And
`record_launch` is not expressed as `save(task)`, because
`protect_terminal` makes it a compare-and-set against a concurrently
finalizing run; a read-modify-write through the aggregate would
reintroduce the race the flag exists to close.
The in-memory doubles model the active-slot rule rather than skipping
it: a double that never refuses a second active run would let the
service's conflict collapse go untested. Their semantics are pinned by
test_schedule_fakes.py, which becomes the contract suite once the SQL
adapters land and both tiers run the same cases.
Concurrency is out of scope for the doubles and says so in their module
docstring -- it stays covered against a real database in
test_scheduled_task_dispatch_race.py.
