Skip to content

fix: narrow exhaustion catch to no-future-run errors, use future date in test #6278

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.

Sign up for GitHub

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

Merged
siddseethepalli merged 1 commit into from
Feb 21, 2026
Merged

fix: narrow exhaustion catch to no-future-run errors, use future date in test #6278

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 6 additions & 2 deletions assistant/src/__tests__/schedule-store.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -435,8 +435,12 @@ describe('claimDueSchedules', () => {
});

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';
// COUNT=1 means only one occurrence — the DTSTART itself.
// Use a far-future DTSTART so createSchedule succeeds (it validates that at
// least one run exists from now). We then force next_run_at into the past
// via SQL to simulate the schedule becoming due after its only occurrence.
const futureYear = new Date().getFullYear() + 5;
const rrule = `DTSTART:${futureYear}0101T000000Z\nRRULE:FREQ=DAILY;COUNT=1`;
Comment on lines +442 to +443
Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector Bot Feb 21, 2026

Choose a reason for hiding this comment

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

P1 Badge Use past DTSTART when asserting RRULE exhaustion

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 👍 / 👎.

const job = createSchedule({
name: 'Finite RRULE',
cronExpression: rrule,
Expand Down
7 changes: 6 additions & 1 deletion assistant/src/schedule/schedule-store.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -216,7 +216,12 @@ export function claimDueSchedules(now: number): ScheduleJob[] {
expression: row.cronExpression,
timezone: row.timezone,
});
} catch {
} 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;
Comment on lines +219 to +224
Copy link
Copy Markdown
Contributor

@devin-ai-integration devin-ai-integration Bot Feb 21, 2026

Choose a reason for hiding this comment

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

🔴 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.
Open in Devin Review

Was this helpful? React with 👍 or 👎 to provide feedback.

// Finite schedule with no future runs — still claim the current due
// run but disable the schedule so it doesn't fire again.
newNextRunAt = null;
Expand Down
Loading