Skip to content

fix(cron): only advance next_run_at for jobs that are actually dispatched - #60946

Open
wildnewton wants to merge 2 commits into
NousResearch:mainfrom
wildnewton:fix/scheduler-next-run-at-on-skip
Open

fix(cron): only advance next_run_at for jobs that are actually dispatched#60946
wildnewton wants to merge 2 commits into
NousResearch:mainfrom
wildnewton:fix/scheduler-next-run-at-on-skip

Conversation

@wildnewton

Copy link
Copy Markdown

Problem

When a cron job is silently skipped (not dequeued for execution because it's already running from a previous tick), the scheduler still advances next_run_at via advance_next_run(). This means the job never retries and appears to have run when it hasn't.

Evidence: job 488dec122600 was skipped at 2026-07-08 21:00 and next_run_at advanced to 2026-07-09 without execution.

Root Cause

In tick(), advance_next_run() was called for ALL due jobs BEFORE the in-flight dedup guard (_submit_with_guard). When a job was skipped (already in _running_job_ids), its next_run_at had already been advanced.

Fix

Move advance_next_run() from the pre-dispatch loop into _submit_with_guard, calling it ONLY when the job is successfully claimed for dispatch (not already running). Skipped jobs keep their next_run_at unchanged so they are retried on the next tick.

Changes

  • cron/scheduler.py: Move advance_next_run from pre-dispatch loop into _submit_with_guard, only after successful claim
  • tests/cron/test_scheduler.py: Add two tests:
    • RED: skipped job must NOT have advance_next_run called
    • GREEN: dispatched job still has advance_next_run called (at-most-once semantics preserved)

TDD Commits

  1. test(cron): RED — skipped job still has advance_next_run called
  2. fix(cron): only advance next_run_at for jobs that are actually dispatched

William Niu added 2 commits July 8, 2026 22:10
Add failing test demonstrating that when a due job is skipped by the
in-flight dedup guard (already running from a previous tick), its
next_run_at is still advanced via advance_next_run, causing it to
skip an entire scheduling window without ever executing.

The test asserts advance_next_run is NOT called for skipped jobs.
…ched

Previously, advance_next_run was called for ALL due jobs before the
in-flight dedup guard in tick(). When a job was silently skipped
(already running from a previous tick), its next_run_at was still
advanced, causing it to skip an entire scheduling window without ever
executing.

Now advance_next_run is called only inside _submit_with_guard, after
the job is successfully claimed for dispatch. Skipped jobs keep their
next_run_at unchanged so they are retried on the next tick.

Fixes: job 488dec122600 skipped at 2026-07-08 21:00, next_run_at
advanced to 2026-07-09 without execution.
@alt-glitch alt-glitch added type/bug Something isn't working comp/cron Cron scheduler and job management P2 Medium — degraded but workaround exists labels Jul 8, 2026

@teknium1 teknium1 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.

Thanks for isolating the pre-guard advancement bug; current main still has it at cron/scheduler.py:3554-3560, before the already-running return at cron/scheduler.py:3627-3630.

Problems

  • cron/scheduler.py:2937 advances before pool.submit at :2948. Current main now handles a shutdown race where pool.submit fails and releases the in-memory claim (cron/scheduler.py:3641-3653) so the job stays due. The proposed ordering would still persist an advance when that submission never succeeds, contrary to the PR's "actually dispatched" contract.

Suggested changes

  • Integrate this move with the current guarded submission path so rejected submissions leave next_run_at unchanged without introducing a worker-start race.
  • Add a regression test for a pool.submit shutdown failure asserting advance_next_run was not called, in addition to the already-running case.

Automated hermes-sweeper review.

Comment thread cron/scheduler.py
# Advance next_run_at NOW — this job is being dispatched, so the
# scheduler won't pick it up again until after it completes.
# (mark_job_run overwrites next_run_at on completion.)
advance_next_run(job_id)

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.

advance_next_run still runs before pool.submit. Current main has a shutdown-race path where submission fails and the in-memory claim is released; in that case this persistent advance means a job that was never enqueued is no longer due. Integrate this with the current submit-failure handling and add a regression test.

@teknium1 teknium1 added sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform labels Jul 10, 2026
spfcraze added a commit to spfcraze/hermes-agent that referenced this pull request Aug 1, 2026
The scheduler's pre-dispatch loop called advance_next_run per due job —
one full load_jobs() + one full save_jobs() of the jobs file each — so
N due jobs cost N reads + N writes of the whole file (gateway-restart
catch-up or co-scheduled bursts). advance_next_runs() does one load +
at most one save for the whole due set with identical per-job semantics;
advance_next_run() is now a thin wrapper over it.

Measured (50 due recurring jobs, real jobs file): 107.9 ms -> 2.5 ms
(45x; 50 loads + 50 saves -> 1 + 1).

Tests: batch advances recurring and skips one-shots, single load + save
I/O pin (fails pre-fix — no such function), no save when nothing
advances, and per-job wrapper semantics unchanged. Related: NousResearch#60946 and
NousResearch#75833 both restructure this loop's call site for correctness — neither
addresses the I/O cost, and this batch primitive composes with either
dispatch design; happy to rebase onto whichever lands first.
kshitijk4poor pushed a commit that referenced this pull request Aug 2, 2026
The scheduler's pre-dispatch loop called advance_next_run per due job —
one full load_jobs() + one full save_jobs() of the jobs file each — so
N due jobs cost N reads + N writes of the whole file (gateway-restart
catch-up or co-scheduled bursts). advance_next_runs() does one load +
at most one save for the whole due set with identical per-job semantics;
advance_next_run() is now a thin wrapper over it.

Measured (50 due recurring jobs, real jobs file): 107.9 ms -> 2.5 ms
(45x; 50 loads + 50 saves -> 1 + 1).

Tests: batch advances recurring and skips one-shots, single load + save
I/O pin (fails pre-fix — no such function), no save when nothing
advances, and per-job wrapper semantics unchanged. Related: #60946 and
#75833 both restructure this loop's call site for correctness — neither
addresses the I/O cost, and this batch primitive composes with either
dispatch design; happy to rebase onto whichever lands first.
randlee pushed a commit to randlee/hermes-agent that referenced this pull request Aug 11, 2026
The scheduler's pre-dispatch loop called advance_next_run per due job —
one full load_jobs() + one full save_jobs() of the jobs file each — so
N due jobs cost N reads + N writes of the whole file (gateway-restart
catch-up or co-scheduled bursts). advance_next_runs() does one load +
at most one save for the whole due set with identical per-job semantics;
advance_next_run() is now a thin wrapper over it.

Measured (50 due recurring jobs, real jobs file): 107.9 ms -> 2.5 ms
(45x; 50 loads + 50 saves -> 1 + 1).

Tests: batch advances recurring and skips one-shots, single load + save
I/O pin (fails pre-fix — no such function), no save when nothing
advances, and per-job wrapper semantics unchanged. Related: NousResearch#60946 and
NousResearch#75833 both restructure this loop's call site for correctness — neither
addresses the I/O cost, and this batch primitive composes with either
dispatch design; happy to rebase onto whichever lands first.
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 P2 Medium — degraded but workaround exists sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform 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.

3 participants