fix: narrow exhaustion catch to no-future-run errors, use future date in test#6278
Conversation
… in test Co-Authored-By: Claude <noreply@anthropic.com>
![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: 57b2682274
ℹ️ 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".
| const futureYear = new Date().getFullYear() + 5; | ||
| const rrule = `DTSTART:${futureYear}0101T000000Z\nRRULE:FREQ=DAILY;COUNT=1`; |
There was a problem hiding this comment.
This test setup now contradicts the exhaustion path it is trying to verify: claimDueSchedules recomputes next run from the RRULE expression (assistant/src/schedule/schedule-store.ts, computeNextRunAtEngine call) and computeNextRunAt uses current wall time by default (assistant/src/schedule/recurrence-engine.ts), so a DTSTART 5 years in the future still has an upcoming run and will not hit the 'no upcoming runs' catch branch. Forcing next_run_at to the past in SQL does not change RRULE evaluation, so the schedule remains enabled and this assertion will fail once tests run.
Useful? React with 👍 / 👎.
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| } catch (err) { | ||
| // Only treat "no upcoming runs" as exhaustion — rethrow other failures | ||
| // (e.g. invalid RRULE lines, unsupported syntax) so they surface instead | ||
| // of silently disabling a schedule that has a configuration bug. | ||
| const msg = err instanceof Error ? err.message : String(err); | ||
| if (!msg.includes('no upcoming runs')) throw err; |
There was a problem hiding this comment.
🔴 Rethrowing non-exhaustion errors from claimDueSchedules blocks all other schedules, reminders, and watchers
When claimDueSchedules encounters a schedule with an invalid expression (e.g., corrupted DB data), the new catch block rethrows the error. This causes the entire for loop over candidates to abort, and the exception propagates up to the caller.
Root Cause and Impact
In assistant/src/schedule/scheduler.ts:78, claimDueSchedules(now) is called at the start of runScheduleOnce. If it throws, the entire function aborts — meaning no schedules are processed, no reminders are claimed (scheduler.ts:131), and no watchers run (scheduler.ts:158-164). The outer tick() catch at scheduler.ts:45 logs the error but the entire tick is lost.
Previously, the bare catch {} would silently disable the bad schedule (marking it exhausted) and continue processing the remaining candidates. While that was too aggressive (it hid real config bugs), the new behavior swings too far: a single corrupt schedule poisons the entire scheduler tick.
For example, if a schedule somehow has an invalid RRULE stored in the DB (e.g., missing DTSTART due to a migration bug), computeNextRunAtEngine throws "RRULE expression must include DTSTART...". This doesn't contain "no upcoming runs", so it's rethrown. The for loop at schedule-store.ts:209 aborts immediately, and any schedules after the bad one in the candidates list are never processed. Worse, reminders and watchers in the same tick are also skipped.
Expected behavior: A single bad schedule should not prevent other valid schedules (and reminders/watchers) from being processed. The error should be logged and the bad schedule skipped (or disabled with an error status), while the loop continues.
Actual behavior: The rethrown error aborts the entire claimDueSchedules call and propagates up, blocking all processing for that tick.
Prompt for agents
In assistant/src/schedule/schedule-store.ts, the catch block at lines 219-224 in claimDueSchedules should not rethrow errors for individual schedules, as this aborts processing of all remaining candidates and propagates up to block reminders and watchers in scheduler.ts. Instead, for non-exhaustion errors, log the error and skip the bad schedule (continue to the next candidate). Consider also marking the schedule with an error status so it can be investigated. For example, replace the `throw err` on line 224 with a log statement and `continue` to skip to the next candidate in the for loop. You may need to import the logger at the top of schedule-store.ts.
Was this helpful? React with 👍 or 👎 to provide feedback.
|
Addressed in #6291 |
1 participant
Address Codex review feedback on PR #5634: (1) only treat 'no upcoming runs' errors as exhaustion in the catch block, rethrow other exceptions; (2) use a future DTSTART date in the exhausted RRULE test to prevent createSchedule from rejecting a past date.