Skip to content

fix(cron): reconcile stale in-flight claim against executions ledger (t_8b5480b3) - #87259

Closed
sycamoregroupltd wants to merge 1 commit into
NousResearch:mainfrom
sycamoregroupltd:fix/t_8b5480b3-ledger-reconcile
Closed

fix(cron): reconcile stale in-flight claim against executions ledger (t_8b5480b3)#87259
sycamoregroupltd wants to merge 1 commit into
NousResearch:mainfrom
sycamoregroupltd:fix/t_8b5480b3-ledger-reconcile

Conversation

@sycamoregroupltd

Copy link
Copy Markdown
Contributor

Problem

4 recurring no_agent cron jobs (deterministic-verdict-router, kanban-scheduled-wake-scanner, review-required-auto-router, blocked-task-notifier) EAGAIN-failed at 12:50:05 on 2026-08-14 and then recorded zero executions for ~1h47m even after substrate recovery — while 100+ other jobs ran normally. next_run_at kept advancing (jobs stayed due) but _submit_with_guard never dispatched them, and the wedge survived a gateway restart.

Root cause (reviewer-validated)

The t_3778a491 age-based stale-claim sweep (already on main) force-releases an in-memory _running_job_ids claim only once it is older than max(2*interval, 30m). But a leaked claim can be young (inside its allowance) while the durable executions ledger already proves the last run ended. The age bound alone could not see a run the ledger had already finished — so the wedge survived restart until an operator cron resumed the job. This is a persisted-state non-dispatch cause, not just the in-memory leak.

Fix

sweep_stale_inflight now reconciles each in-flight claim against the durable executions ledger (cron/executions.db):

  • If the job's MOST RECENT execution row is terminal (completed/failed/unknown), the run provably ended — the claim is stale by construction regardless of in-memory age, and is force-released so the recurring job re-dispatches on the next tick (no force-run, no cron resume).
  • Persisted-state recovery: the ledger is written by the worker that ran the job and read by ANY ticker process (including one started AFTER the leak), closing the restart-survival gap.
  • A ledger-terminal release is authoritative: it does not write a synthetic mark_job_run(success=False) (the ledger already records the outcome), so an honest completed/ok status is never clobbered.

Verification

  • New TestLedgerTerminalReconciliation (4 tests): young+terminal -> released (clean behavioral RED on main, GREEN here); no-ledger-row -> not released; running-row -> not released; old+terminal -> released once without synthetic failure.
  • Full tests/cron/: 713 passed, 1 skipped, 1 timing-flaky test that passes in isolation (unrelated to this change).

Copilot AI lite review requested due to automatic review settings August 15, 2026 20:50

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

…(t_8b5480b3)

The age-only stale-claim sweep (t_3778a491, already on main) force-releases
an in-memory _running_job_ids claim only once it is older than
max(2*interval, 30m). A leaked claim that is YOUNG (inside its allowance)
while the durable executions ledger already proves the last run ended stays
wedged: the job is returned as due every tick, _submit_with_guard short-
circuits on 'already running', and next_run_at keeps fast-forwarding with no
execution — the exact 2026-08-14 recurring-router incident (t_20e23f84),
which survived a gateway restart because the in-memory age bound alone could
not see a run the ledger had already finished.

sweep_stale_inflight now reconciles each in-flight claim against the durable
executions ledger (cron/executions.db): if the job's MOST RECENT execution
row is terminal (completed/failed/unknown), the run provably ended, so the
claim is stale by construction regardless of its in-memory age and is force-
released. This is a persisted-state recovery path: the ledger is written by
the worker that ran the job and read by ANY ticker process (including one
that started AFTER the leak), so a leaked claim is recoverable without
force-run/resume and without depending on which process holds it in memory.
A ledger-terminal release is authoritative — it does not write a synthetic
mark_job_run failure (the ledger already records the outcome).

Added TestLedgerTerminalReconciliation (4 tests): young+terminal -> released
(RED on main, GREEN here), no-ledger-row -> not released, running-row -> not
released, old+terminal -> released once without synthetic failure.
@sycamoregroupltd
sycamoregroupltd force-pushed the fix/t_8b5480b3-ledger-reconcile branch from 058ef2f to f73b7e5 Compare August 15, 2026 20:51
@alt-glitch alt-glitch added type/bug Something isn't working P1 High — major feature broken, no workaround comp/cron Cron scheduler and job management sweeper:risk-automation Sweeper risk: may affect CI, automerge, label sync, or maintainer automation sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades labels Aug 15, 2026
@Enough1122

Copy link
Copy Markdown
Contributor

AI code review — automated review for reference, author can ignore or act on any point.

fix(cron): reconcile stale in-flight claim against executions ledger (t_8b5480b3)

  1. Double-dispatch window (main concern): the ledger-terminal release fires when fut is None or fut is _FUTURE_PENDING or fut.done() and the job's most recent execution row is terminal. A freshly claimed job is exactly _FUTURE_PENDING at claim registration, and if create_execution writes its row only after the claim (or after the run actually starts), the ledger still shows the previous run's terminal row — the sweep will release the just-taken claim and the job re-dispatches on the next tick, producing two overlapping runs. Suggest gating the ledger-terminal release on the claim being older than a small floor (a few seconds), or binding the claim to the execution id it belongs to, so a new claim is never released on the strength of the previous run's row.
  2. latest_executions(list(_running_job_ids)) is snapshotted once before the loop; the loop iterates due jobs. Claims taken after the snapshot are absent from _latest and simply fall through to the age path — safe, but the ledger check is only as fresh as tick start; fine, just worth being aware of.
  3. The reason is threaded into the stale tuples and the log, but _forced_release_count is shared between ledger-terminal and age releases. If dashboards ever need to distinguish "proven dead by ledger" from "aged out", a separate counter would be cheap to add now that the reason already exists.
  4. Test coverage is strong (young claim released, no-ledger-row not released, running-row not released, old claim released exactly once with no synthetic mark_job_run). The tests mock latest_executions directly; an integration-style test where the ledger is written by a real worker and read by a second ticker would exercise the restart-survival claim the PR describes.

@kshitijk4poor

Copy link
Copy Markdown
Collaborator

Salvaged via #88343 — your commit was cherry-picked with authorship preserved (you'll show as the author on main once it merges). One safety follow-up on top: the ledger-terminal release now verifies the terminal row belongs to THIS claim (claimed_at >= _running_since) — for a recurring job the latest terminal row is usually the previous run's, and releasing on it during the try_register→create_execution window (or before the worker's finally runs) would double-dispatch the job concurrently. Missing claimed_at fails closed to the age bound.

Thanks for the ledger-reconciliation idea — it closes the young-wedged-claim gap the age sweep couldn't!

kshitijk4poor added a commit that referenced this pull request Aug 17, 2026
Follow-ups on the #87259 salvage:

- cron/scheduler.py: the ledger-terminal reconciliation now requires the
  terminal execution row's claimed_at to be >= the in-memory claim's
  registration time (_running_since). Without this, the latest terminal
  row for a recurring job is usually the PREVIOUS run's outcome — a fresh
  claim in the try_register_running_job -> create_execution window (or a
  finished run whose worker finally block hasn't released yet) would be
  force-released and the job double-dispatched. Unparseable/missing
  claimed_at fails closed to the age-based bound.
- cron/scheduler.py: take the _running_job_ids snapshot for the ledger
  query under _running_lock — list() over a set concurrently mutated by
  try_register/release_running_job can raise RuntimeError.
- tests: existing reconciliation tests updated to the claimed_at contract;
  two new race-guard tests (previous-run terminal row never releases a
  fresh claim; missing claimed_at fails closed). Mutation-verified:
  removing the ownership guard fails both.
lisajlau pushed a commit to lisajlau/hermes-agent that referenced this pull request Aug 20, 2026
Follow-ups on the NousResearch#87259 salvage:

- cron/scheduler.py: the ledger-terminal reconciliation now requires the
  terminal execution row's claimed_at to be >= the in-memory claim's
  registration time (_running_since). Without this, the latest terminal
  row for a recurring job is usually the PREVIOUS run's outcome — a fresh
  claim in the try_register_running_job -> create_execution window (or a
  finished run whose worker finally block hasn't released yet) would be
  force-released and the job double-dispatched. Unparseable/missing
  claimed_at fails closed to the age-based bound.
- cron/scheduler.py: take the _running_job_ids snapshot for the ledger
  query under _running_lock — list() over a set concurrently mutated by
  try_register/release_running_job can raise RuntimeError.
- tests: existing reconciliation tests updated to the claimed_at contract;
  two new race-guard tests (previous-run terminal row never releases a
  fresh claim; missing claimed_at fails closed). Mutation-verified:
  removing the ownership guard fails both.
prmartinow pushed a commit to prmartinow/hermes-agent that referenced this pull request Aug 26, 2026
Follow-ups on the NousResearch#87259 salvage:

- cron/scheduler.py: the ledger-terminal reconciliation now requires the
  terminal execution row's claimed_at to be >= the in-memory claim's
  registration time (_running_since). Without this, the latest terminal
  row for a recurring job is usually the PREVIOUS run's outcome — a fresh
  claim in the try_register_running_job -> create_execution window (or a
  finished run whose worker finally block hasn't released yet) would be
  force-released and the job double-dispatched. Unparseable/missing
  claimed_at fails closed to the age-based bound.
- cron/scheduler.py: take the _running_job_ids snapshot for the ledger
  query under _running_lock — list() over a set concurrently mutated by
  try_register/release_running_job can raise RuntimeError.
- tests: existing reconciliation tests updated to the claimed_at contract;
  two new race-guard tests (previous-run terminal row never releases a
  fresh claim; missing claimed_at fails closed). Mutation-verified:
  removing the ownership guard fails both.
bobaba76 pushed a commit to bobaba76/hermes-agent that referenced this pull request Aug 27, 2026
Follow-ups on the NousResearch#87259 salvage:

- cron/scheduler.py: the ledger-terminal reconciliation now requires the
  terminal execution row's claimed_at to be >= the in-memory claim's
  registration time (_running_since). Without this, the latest terminal
  row for a recurring job is usually the PREVIOUS run's outcome — a fresh
  claim in the try_register_running_job -> create_execution window (or a
  finished run whose worker finally block hasn't released yet) would be
  force-released and the job double-dispatched. Unparseable/missing
  claimed_at fails closed to the age-based bound.
- cron/scheduler.py: take the _running_job_ids snapshot for the ledger
  query under _running_lock — list() over a set concurrently mutated by
  try_register/release_running_job can raise RuntimeError.
- tests: existing reconciliation tests updated to the claimed_at contract;
  two new race-guard tests (previous-run terminal row never releases a
  fresh claim; missing claimed_at fails closed). Mutation-verified:
  removing the ownership guard fails both.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

comp/cron Cron scheduler and job management P1 High — major feature broken, no workaround sweeper:risk-automation Sweeper risk: may affect CI, automerge, label sync, or maintainer automation sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants