-
Notifications
You must be signed in to change notification settings - Fork 89
fix: claim exhausted RRULE schedules and disable them instead of skipping #5634
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -207,7 +207,8 @@ export function claimDueSchedules(now: number): ScheduleJob[] { | |
|
|
||
| const claimed: ScheduleJob[] = []; | ||
| for (const row of candidates) { | ||
| let newNextRunAt: number; | ||
| let newNextRunAt: number | null; | ||
| let exhausted = false; | ||
| try { | ||
| const syntax = (row.scheduleSyntax as ScheduleSyntax) ?? 'cron'; | ||
| newNextRunAt = computeNextRunAtEngine({ | ||
|
|
@@ -216,28 +217,38 @@ export function claimDueSchedules(now: number): ScheduleJob[] { | |
| timezone: row.timezone, | ||
| }); | ||
| } catch { | ||
| // Expression has no future runs — skip | ||
| continue; | ||
| // Finite schedule with no future runs — still claim the current due | ||
| // run but disable the schedule so it doesn't fire again. | ||
| newNextRunAt = null; | ||
| exhausted = true; | ||
|
Comment on lines
219
to
+223
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
This Useful? React with 👍 / 👎. |
||
| } | ||
|
|
||
| // Optimistic lock: only update if nextRunAt hasn't changed | ||
| const updates: Record<string, unknown> = { | ||
| lastRunAt: now, | ||
| updatedAt: now, | ||
| }; | ||
| if (exhausted) { | ||
| updates.nextRunAt = 0; | ||
| updates.enabled = false; | ||
| } else { | ||
| updates.nextRunAt = newNextRunAt!; | ||
| } | ||
|
|
||
| const result = db | ||
| .update(scheduleJobs) | ||
| .set({ | ||
| nextRunAt: newNextRunAt, | ||
| lastRunAt: now, | ||
| updatedAt: now, | ||
| }) | ||
| .set(updates) | ||
| .where(and(eq(scheduleJobs.id, row.id), eq(scheduleJobs.nextRunAt, row.nextRunAt))) | ||
| .run() as unknown as { changes?: number }; | ||
|
|
||
| if ((result.changes ?? 0) === 0) continue; | ||
|
|
||
| claimed.push(parseJobRow({ | ||
| ...row, | ||
| nextRunAt: newNextRunAt, | ||
| nextRunAt: exhausted ? 0 : newNextRunAt!, | ||
| lastRunAt: now, | ||
| updatedAt: now, | ||
| enabled: exhausted ? false : row.enabled, | ||
| })); | ||
| } | ||
| return claimed; | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This test hardcodes
DTSTART:20250101...for aCOUNT=1enabled RRULE, butcreateSchedulecomputesnextRunAtimmediately and throws once that date is in the past, so the test stops before exercisingclaimDueSchedules. As time advances, this regression test becomes invalid/flaky and no longer verifies the intended behavior; use a future date relative toDate.now()(or freeze time) before forcingnext_run_atto past.Useful? React with 👍 / 👎.