fix: claim exhausted RRULE schedules and disable them instead of skipping#5634
Conversation
…ping Co-Authored-By: Claude <noreply@anthropic.com>
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: cbe8c31775
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| } 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; |
There was a problem hiding this comment.
This catch now treats every computeNextRunAtEngine failure as an exhausted schedule and disables the row, but computeNextRunAtEngine also throws for non-exhaustion problems (for example invalid RRULE lines or unsupported syntax in recurrence-engine.ts). In those cases we would silently mark the job disabled and still run its payload once, which hides data issues and permanently turns schedules off instead of surfacing the real error path. Please only apply the disable-and-claim behavior for explicit "no upcoming runs" failures and rethrow other exceptions.
Useful? React with 👍 / 👎.
|
|
||
| test('claims exhausted RRULE schedule and disables it', () => { | ||
| // COUNT=1 means only one occurrence — the DTSTART itself | ||
| const rrule = 'DTSTART:20250101T000000Z\nRRULE:FREQ=DAILY;COUNT=1'; |
There was a problem hiding this comment.
This test hardcodes DTSTART:20250101... for a COUNT=1 enabled RRULE, but createSchedule computes nextRunAt immediately and throws once that date is in the past, so the test stops before exercising claimDueSchedules. As time advances, this regression test becomes invalid/flaky and no longer verifies the intended behavior; use a future date relative to Date.now() (or freeze time) before forcing next_run_at to past.
Useful? React with 👍 / 👎.
|
Addressed in #6278 |
1 participant
Address reviewer feedback on PR #5516.
When a finite RRULE schedule (e.g. COUNT=1) is exhausted, computeNextRunAtEngine throws because there are no future occurrences. Previously, the catch block in claimDueSchedules would skip the job entirely, meaning the current due run was never claimed/executed and the row remained perpetually due.
Now, exhausted schedules are still claimed (so the due run executes) but are disabled (enabled=false, nextRunAt=0) so they don't fire again.
Adds a test covering the exhausted RRULE claim + disable behavior.