fix(scheduler): a firing could be lost between rescheduling and dispatch
ci / gates (push) Failing after 6s
ci / rust (push) Skipped
ci / frontend (push) Skipped
ci / e2e (push) Skipped
ci / publish (push) Skipped

`tick` advanced `next_run_at` before dispatching the work, with nothing
recording that the occurrence was owed. A process that died between the two
dropped it silently.

The window is narrower than it first looks — `claim_due` sets `last_run_at`
but does not clear `next_run_at`, so a crash *before* `set_next_run` leaves
the routine due and it re-fires on the next tick. The loss is specifically
between the reschedule and the dispatch. That is tolerable for a message
routine and not tolerable for a scheduled mission, which is why this lands
before mission scheduling does.

`routine_fires` holds one row per (routine, occurrence), claimed before
dispatch and settled after:

- Fresh   — nobody has it; fire.
- Retry   — claimed, never settled: a crash mid-fire. Safe to fire again, as
            no completion was recorded and nothing downstream saw a result.
- Settled — already dispatched; advance the clock and do not run the work.
            This is what keeps a scheduled mission to one container across
            restarts.

A failed dispatch settles terminally rather than staying retryable. Retrying
a persistently failing action every tick is how a broken routine becomes a
denial-of-service against whatever it talks to; the error is kept on the row.

The claim uses `xmax = 0` to distinguish a real insert from a no-op update in
a single statement — `ON CONFLICT DO NOTHING` returns no row at all, so two
schedulers racing one occurrence could both read it as unclaimed.

Also: fan-out capped at 25 per tick with the remainder logged and deferred (a
clock jump or an accidental every-minute cron would otherwise dispatch every
missed occurrence at once — one container each for topology routines), and
`spawn` no longer discards tick errors, so a scheduler that has stopped firing
no longer looks identical to one with nothing to do.

The pre-existing exactly-once test still passes: the claim changes
recoverability, not firing semantics.

Co-Authored-By: Claude Opus 5 <[email protected]>
This commit is contained in:
Omar Sobh
2026-08-01 18:50:11 -07:00
co-authored by Claude Opus 5
parent 9f874bc06a
commit 2c7d619cf0
4 changed files with 303 additions and 7 deletions
+110
View File
@@ -196,3 +196,113 @@ async fn paused_routines_do_not_fire() {
assert_eq!(scheduler.tick(now).await.unwrap(), 0);
}
/// The crash window this exists to close.
///
/// The scheduler advances `next_run_at` before dispatching, so a process that
/// dies between the two used to drop the occurrence with nothing anywhere
/// recording that it was owed. The claim row is what makes that recoverable:
/// a slot left `claimed` is a crash mid-fire, and the next tick retries it.
#[tokio::test]
async fn an_occurrence_claimed_but_never_settled_is_retried() {
let pool = cm_testkit::test_pool().await;
let agent = seeded(&pool).await;
let now = time::OffsetDateTime::now_utc();
let slot = now - time::Duration::minutes(1);
let routine = cm_db::repo::routines::create(
&pool,
agent.id,
"Nightly sweep",
"* * * * *",
json!({"message": "sweep"}),
slot,
)
.await
.unwrap();
use cm_db::repo::routines::FireClaim;
// First claim: nobody has this occurrence.
assert_eq!(
cm_db::repo::routines::claim_fire(&pool, routine.id, slot)
.await
.unwrap(),
FireClaim::Fresh
);
// Simulate a crash: claimed, never settled. The next attempt must be told
// it is safe to retry — no completion was ever recorded, so nothing
// downstream saw a result.
assert_eq!(
cm_db::repo::routines::claim_fire(&pool, routine.id, slot)
.await
.unwrap(),
FireClaim::Retry
);
// Once settled, the same occurrence must never fire again — this is the
// branch that keeps a scheduled mission to one container across restarts.
cm_db::repo::routines::complete_fire(&pool, routine.id, slot, None)
.await
.unwrap();
assert_eq!(
cm_db::repo::routines::claim_fire(&pool, routine.id, slot)
.await
.unwrap(),
FireClaim::Settled
);
// A *different* occurrence of the same routine is independent.
let later = slot + time::Duration::minutes(1);
assert_eq!(
cm_db::repo::routines::claim_fire(&pool, routine.id, later)
.await
.unwrap(),
FireClaim::Fresh
);
}
/// A failed dispatch settles the slot rather than leaving it retryable.
/// Retrying a persistently failing action every tick is how a broken routine
/// becomes a denial-of-service against the thing it talks to.
#[tokio::test]
async fn a_failed_dispatch_is_terminal_for_that_occurrence() {
let pool = cm_testkit::test_pool().await;
let agent = seeded(&pool).await;
let slot = time::OffsetDateTime::now_utc() - time::Duration::minutes(1);
let routine = cm_db::repo::routines::create(
&pool,
agent.id,
"Flaky",
"* * * * *",
json!({"message": "x"}),
slot,
)
.await
.unwrap();
use cm_db::repo::routines::FireClaim;
cm_db::repo::routines::claim_fire(&pool, routine.id, slot)
.await
.unwrap();
cm_db::repo::routines::complete_fire(&pool, routine.id, slot, Some("gateway timed out"))
.await
.unwrap();
assert_eq!(
cm_db::repo::routines::claim_fire(&pool, routine.id, slot)
.await
.unwrap(),
FireClaim::Settled,
"a failed occurrence must not be retried forever"
);
let err: Option<String> =
sqlx::query_scalar("SELECT error FROM routine_fires WHERE routine_id = $1")
.bind(routine.id)
.fetch_one(&pool)
.await
.unwrap();
assert_eq!(err.as_deref(), Some("gateway timed out"));
}