Skip to content

fix(cron): deliver other due jobs while one remains unacked - #100

Merged
adriannoes merged 2 commits into
developmentfrom
fix/cron-due-sibling-starvation
Sep 24, 2026
Merged

adriannoes merged 2 commits into
developmentfrom
fix/cron-due-sibling-starvation

Conversation

@cursor

@cursor cursor Bot commented Sep 23, 2026 •

Copy link
Copy Markdown

Deferred ack left a failed cron job at the head of the due queue. cron_job_get_next_due always returns that earliest row, so every later reminder waits until the stuck job is delivered.

Bug and impact

Two jobs are due. Job A is earlier. Its channel send fails, or agent_run returns an error (send_then_maybe_ack_cron skips cron_ack_delivery). Job A stays next_run <= now. Every poll offers A again and never reaches job B. A permanently broken recipient (missing channel, provider error) stops the rest of the schedule.

Root cause

dcf193b moved delete/reschedule to after a successful delivery, but the poller still selects ORDER BY next_run ASC LIMIT 1 with no way to pass a job that is still inside its re-offer window.

Fix

cron_poll remembers recent offers and, while the earliest job is still inside that window, offers the next due job. If it is the only due job, the existing re-offer wait is unchanged. Ack semantics are unchanged: the stuck job stays due and is retried.

Validation

make test_cron, make test_dispatch, and make test_memory passed. The new regression offers job B immediately while job A is still unacked, and the single-job re-offer wait test still holds.

Open in Web View Automation 

Note

Medium Risk
Changes cron channel polling and due-job selection logic that affects reminder delivery order and timing, though ack/reschedule behavior is preserved and regression tests were added.

Overview
Fixes a case where an unacked earliest due job blocked every later reminder: cron_poll no longer always takes ORDER BY next_run LIMIT 1 when that job is still inside its re-offer window.

The poller now keeps a 16-slot offer table (per-job timeout) and cron_pick_due_row scans due jobs via new cron_job_list_due, preferring jobs that are returnable while the head job is “hot.” If only the earliest job is due, behavior is unchanged—it still waits before re-offering. cron_ack_delivery semantics are unchanged; stuck jobs stay due and retry.

The memory layer adds stable ordering next_run ASC, id ASC, cron_job_get_next_due_after for cursor-style due selection, and rejects ids ≥ 128 chars on create so truncated ids cannot break tie-breaking. Tests cover sibling delivery, a full offer table, and tie ordering.

Reviewed by Cursor Bugbot for commit e09b2e9. Bugbot is set up for automated code reviews on this repo. Configure here.

Deferred ack kept the earliest due row at the head of the queue, so a
failed send or agent run blocked every later reminder until that job
succeeded.
@adriannoes
adriannoes force-pushed the fix/cron-due-sibling-starvation branch from 84a781f to 0e5bc51 Compare September 24, 2026 00:05
@adriannoes
adriannoes marked this pull request as ready for review September 24, 2026 00:05
@adriannoes
adriannoes self-requested a review as a code owner September 24, 2026 00:05

@cursor cursor Bot left a comment

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Cursor Bugbot has reviewed your changes using high effort and found 1 potential issue.

Fix All in Cursor

❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.

Reviewed by Cursor Bugbot for commit 0e5bc51. Configure here.

Comment thread src/tools/cron.c

@cursor cursor Bot left a comment

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Summary

This fixes a real head-of-line block. After a failed delivery the earliest cron row stays next_run <= now, and cron_job_get_next_due used to return only that row, so every later reminder waited on it. The poller now remembers recent offers and walks a (next_run, id) cursor until it finds a due job outside that window. Ack behavior is unchanged, the single-job re-offer wait still holds, and make test_cron passes here (linked with -lm; this image has no libcurl).

The backoff does not survive more than 16 in-flight offers. Past that, a stuck channel tight-loops agent_run. GitHub rejects a changes-requested review from this account on its own PR, so this is a comment. The ring bug should land before merge.

Must Fix

  • src/tools/cron.c: CRON_OFFER_TRACK is 16, and a full ring overwrites the oldest id. cron_offer_is_hot treats a missing id as cold, so the next poll returns that job with no cron_wait_remaining. I drove the built objects with 17 due jobs and an 800ms timeout: the first pass returns j00 through j16 immediately (correct), then the next two polls return j00 and j01 with elapsed 0. The same driver with 2 jobs waits out the timeout on the third poll. POLL_TIMEOUT_MS is 1000 (src/core/main.c), and a systemd cli_poll of /dev/null returns immediately, so this sleep is the only gap between provider calls. Create is uncapped and the list API already returns 64 rows, so a down channel with a backlog of reminders hits this. Reuse a slot only after its recorded age is past the timeout. If every slot is still inside the window, take the existing fallback and wait on the earliest due job.

Should Fix

  • tests/test_cron.c: the new test locks "offer job_b while job_a is hot" and that neither next_run moves. It does not lock the fallback once every due job is hot (the next poll should wait, then return job_a), or the 17-job case above.
  • s_offers is process-global, and neither cron_cleanup nor memory_cleanup clears it. A second scenario in the same process skipped j00 on a fresh database because an earlier poll had marked that id. Later cron poll tests in this binary can inherit a hot id. Zero the table when the channel shuts down, and at the start of tests that call poll.

Nice to Have

  • cron_pick_due_row issues one query per skipped hot row. A single ORDER BY next_run, id scan would do the same walk in memory.
  • cron_job_create still accepts an id longer than 127 bytes. fill_cron_job_row truncates into id[128], the cursor is that prefix, and the monotonicity check then stops the walk, so an overlong head id hides later jobs again. The tool and POST /api/cron already snprintf with %.127s. Reject the long id at insert.
  • cron_job_list is still ORDER BY next_run only, so ties do not match the poller.

Positive Highlights

  • The due-row predicate is bound parameters, and ORDER BY next_run, id matches the cursor (next_run > ? OR (next_run = ? AND id > ?)).
  • The guard in cron_pick_due_row that bails when a fetched row is not strictly after the cursor prevents a repeat row from spinning the walk.
  • The regression checks the actual contract: job_b comes back in well under the 2000ms timeout, and both rows stay at their original next_run.
Open in Web View Automation 

Sent by Cursor Automation: Code review

Comment thread src/tools/cron.c Outdated
Comment thread tests/test_cron.c
A full ring used to drop the oldest id, and the next poll treated that
job as cold, so a down channel tight-looped agent_run. Reuse a slot only
after its recorded window has elapsed, and wait on the earliest due job
when every slot is still hot.
@adriannoes
adriannoes merged commit aa6cd98 into development Sep 24, 2026
2 checks passed
@adriannoes
adriannoes deleted the fix/cron-due-sibling-starvation branch September 24, 2026 00:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant