From bc15cc5951c9dc0539676fd52151c07ce83e7f28 Mon Sep 17 00:00:00 2001 From: Jongsun Suh Date: Fri, 28 Aug 2026 13:02:02 -0400 Subject: [PATCH 1/6] Persist cronjob reschedules via `setEventDate` on `CronjobControllerStateManager` MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Rescheduling is the most frequent write `CronjobController` makes — a Snap on a `PT30S` schedule reschedules every thirty seconds — and it changes exactly one field. It went through `set`, which hands the client the entire event map to re-serialise on every tick. Routing it through a dedicated `setEventDate` lets a client store dates apart from the rest of the state. This is breaking: `CronjobControllerStateManager` is exported, so every implementer must add the method. --- .../src/cronjob/CronjobController.test.ts | 79 ++++++++++++++++++- .../src/cronjob/CronjobController.ts | 18 ++++- 2 files changed, 93 insertions(+), 4 deletions(-) diff --git a/packages/snaps-controllers/src/cronjob/CronjobController.test.ts b/packages/snaps-controllers/src/cronjob/CronjobController.test.ts index 3ec745717c..3a55d4abea 100644 --- a/packages/snaps-controllers/src/cronjob/CronjobController.test.ts +++ b/packages/snaps-controllers/src/cronjob/CronjobController.test.ts @@ -23,16 +23,38 @@ const MOCK_VERSION = '1.0.0' as SemVerVersion; /** * Get a mock state manager for the `CronjobController`. * - * @returns A state manager object with `get` and `set` methods. + * @returns A state manager object with `getInitialState`, `set` and + * `setEventDate` methods. */ function getMockStateManager(): CronjobControllerStateManager { let state: CronjobControllerState | undefined; + // Dates are stored apart from the rest of the state, mirroring how a client + // is expected to implement this, and merged back on read. + const dates = new Map(); + return { - getInitialState: () => state, + getInitialState: () => { + if (!state) { + return undefined; + } + + return { + ...state, + events: Object.fromEntries( + Object.entries(state.events).map(([id, event]) => [ + id, + { ...event, date: dates.get(id) ?? event.date }, + ]), + ), + }; + }, set: (newState) => { state = newState; }, + setEventDate: (id, date) => { + dates.set(id, date); + }, }; } @@ -469,6 +491,59 @@ describe('CronjobController', () => { cronjobController.destroy(); }); + it('persists a reschedule through `setEventDate`, without rewriting all state', async () => { + const rootMessenger = getRootCronjobControllerMessenger(); + const controllerMessenger = + getRestrictedCronjobControllerMessenger(rootMessenger); + + const handleRequest = jest.fn().mockResolvedValue(undefined); + rootMessenger.registerActionHandler( + 'SnapController:handleRequest', + handleRequest, + ); + + const stateManager = getMockStateManager(); + const set = jest.spyOn(stateManager, 'set'); + const setEventDate = jest.spyOn(stateManager, 'setEventDate'); + + const cronjobController = new CronjobController({ + messenger: controllerMessenger, + stateManager, + state: { + events: { + [`cronjob-${MOCK_SNAP_ID}-0`]: { + id: `cronjob-${MOCK_SNAP_ID}-0`, + snapId: MOCK_SNAP_ID, + date: new Date('2022-01-01T00:00Z').toISOString(), + scheduledAt: new Date('2022-01-01T00:00Z').toISOString(), + schedule: 'PT25H', + recurring: true, + request: { + method: 'exampleMethod', + params: ['p1'], + }, + }, + }, + }, + }); + + cronjobController.init(); + + await new Promise((resolve) => originalProcessNextTick(resolve)); + expect(handleRequest).toHaveBeenCalledTimes(1); + + // Firing the event reschedules it, which is the write this change is + // about: one date, not the whole event map. + expect(setEventDate).toHaveBeenCalledWith( + `cronjob-${MOCK_SNAP_ID}-0`, + expect.any(String), + ); + + expect(set).not.toHaveBeenCalled(); + + cronjobController.destroy(); + }); + it('handles the `snapInstalled` event', () => { const rootMessenger = getRootCronjobControllerMessenger(); const controllerMessenger = diff --git a/packages/snaps-controllers/src/cronjob/CronjobController.ts b/packages/snaps-controllers/src/cronjob/CronjobController.ts index 8a8ad0ca60..cf92079d5f 100644 --- a/packages/snaps-controllers/src/cronjob/CronjobController.ts +++ b/packages/snaps-controllers/src/cronjob/CronjobController.ts @@ -71,6 +71,20 @@ export const DAILY_TIMEOUT = inMilliseconds(24, Duration.Hour); export type CronjobControllerStateManager = { set(state: CronjobControllerState): void; + + /** + * Persist a single event's next execution date. + * + * Rescheduling is by far the most frequent write this controller makes — a + * snap with a `PT30S` schedule reschedules every thirty seconds — and it + * changes one field. Routing it here lets an implementation store dates + * separately instead of re-serialising every event on each tick. + * + * @param id - The ID of the event. + * @param date - The next execution date, as an ISO 8601 string. + */ + setEventDate(id: string, date: string): void; + getInitialState(): CronjobControllerState | undefined; }; @@ -389,11 +403,11 @@ export class CronjobController extends BaseController< } const date = getExecutionDate(event.schedule); - const { nextState } = this.update((state) => { + this.update((state) => { state.events[event.id].date = date; }); - this.#stateManager.set(nextState); + this.#stateManager.setEventDate(event.id, date); this.#startTimer({ ...event, From 3d5019409cae41715d1a6d44c1a4111f83cfdd19 Mon Sep 17 00:00:00 2001 From: Jongsun Suh Date: Fri, 28 Aug 2026 14:10:48 -0400 Subject: [PATCH 2/6] Add changelog entry for `setEventDate` --- packages/snaps-controllers/CHANGELOG.md | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/packages/snaps-controllers/CHANGELOG.md b/packages/snaps-controllers/CHANGELOG.md index e9c77b0cd4..90936d413b 100644 --- a/packages/snaps-controllers/CHANGELOG.md +++ b/packages/snaps-controllers/CHANGELOG.md @@ -7,6 +7,12 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +### Changed + +- **BREAKING:** Add `setEventDate` to `CronjobControllerStateManager` ([#4107](https://github.com/MetaMask/snaps/pull/4107)) + - `CronjobController` now persists a rescheduled event's next execution date through `setEventDate(id, date)` rather than passing the entire state to `set`. Rescheduling is the controller's most frequent write and changes only this field, so clients may now store dates separately and merge them back in `getInitialState`. + - Implementers of `CronjobControllerStateManager` must add the method. Delegating to `set` with the date applied preserves existing behaviour. + ## [21.1.0] ### Added From e10d833808201473a76512e47b890069367af4f4 Mon Sep 17 00:00:00 2001 From: Jongsun Suh Date: Mon, 31 Aug 2026 12:47:42 -0400 Subject: [PATCH 3/6] Make a lost background-event date recoverable rather than fatal Splitting dates out of the main state introduces a failure the controller could not previously have: the date goes missing while the event survives. `DateTime.fromISO(undefined)` yields NaN, which passed both bounds checks in `#startTimer` and reached `new Timer(NaN)`, whose constructor throws. That threw out of `#reschedule`'s loop, so every event ordered behind the bad one was never scheduled, and the daily timer's re-arm was skipped with it. `recoverEventDate` reconstructs the date from `schedule` and `scheduledAt`, both written once at creation and never mutated. It is deliberately not `getExecutionDate`, which is impure for durations and throws for an absolute date already past. `deleteEventDate` closes the other half: nothing told a client storing dates separately that an event was gone, so every cancelled or completed event left an orphaned key behind. --- packages/snaps-controllers/CHANGELOG.md | 14 +- .../src/cronjob/CronjobController.test.ts | 132 +++++++++++++++++- .../src/cronjob/CronjobController.ts | 47 ++++++- .../snaps-controllers/src/cronjob/index.ts | 1 + .../snaps-controllers/src/cronjob/utils.ts | 68 +++++++++ 5 files changed, 253 insertions(+), 9 deletions(-) diff --git a/packages/snaps-controllers/CHANGELOG.md b/packages/snaps-controllers/CHANGELOG.md index 90936d413b..c5fdecdca7 100644 --- a/packages/snaps-controllers/CHANGELOG.md +++ b/packages/snaps-controllers/CHANGELOG.md @@ -7,11 +7,21 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +### Added + +- Add `recoverEventDate` for reconstructing a background event's next execution date ([#4107](https://github.com/MetaMask/snaps/pull/4107)) + - A client that stores dates separately can lose one without losing the event. `schedule` and `scheduledAt` are written once when the event is added and never mutated, so the date is reconstructible from them. + - Unlike `getExecutionDate`, this function is pure and total: it anchors a non-recurring event's duration on `scheduledAt` rather than on the current time, and returns `undefined` instead of throwing when the schedule cannot be parsed. + ### Changed -- **BREAKING:** Add `setEventDate` to `CronjobControllerStateManager` ([#4107](https://github.com/MetaMask/snaps/pull/4107)) +- **BREAKING:** Add `setEventDate` and `deleteEventDate` to `CronjobControllerStateManager` ([#4107](https://github.com/MetaMask/snaps/pull/4107)) - `CronjobController` now persists a rescheduled event's next execution date through `setEventDate(id, date)` rather than passing the entire state to `set`. Rescheduling is the controller's most frequent write and changes only this field, so clients may now store dates separately and merge them back in `getInitialState`. - - Implementers of `CronjobControllerStateManager` must add the method. Delegating to `set` with the date applied preserves existing behaviour. + - `deleteEventDate(id)` is called when an event is cancelled or when a non-recurring event fires. Without it a client storing dates separately has no signal that an event is gone, and accumulates one orphaned key per completed event. + - Implementers of `CronjobControllerStateManager` must add both methods. Delegating each to `set` with the change applied preserves existing behaviour. +- `CronjobController` no longer stops scheduling every remaining event when one event has an unusable date ([#4107](https://github.com/MetaMask/snaps/pull/4107)) + - A date that cannot be parsed yields `NaN` milliseconds, which passed both bounds checks in the timer setup and reached the `Timer` constructor, throwing. That threw out of the rescheduling loop, so every event ordered after the offending one was never scheduled, and the daily timer was not re-armed. + - Such an event is now reported and skipped individually, and the timer setup rejects an unusable date with a message naming the event. ## [21.1.0] diff --git a/packages/snaps-controllers/src/cronjob/CronjobController.test.ts b/packages/snaps-controllers/src/cronjob/CronjobController.test.ts index 3a55d4abea..222b1b0c0f 100644 --- a/packages/snaps-controllers/src/cronjob/CronjobController.test.ts +++ b/packages/snaps-controllers/src/cronjob/CronjobController.test.ts @@ -23,8 +23,8 @@ const MOCK_VERSION = '1.0.0' as SemVerVersion; /** * Get a mock state manager for the `CronjobController`. * - * @returns A state manager object with `getInitialState`, `set` and - * `setEventDate` methods. + * @returns A state manager object with `getInitialState`, `set`, + * `setEventDate` and `deleteEventDate` methods. */ function getMockStateManager(): CronjobControllerStateManager { let state: CronjobControllerState | undefined; @@ -55,6 +55,9 @@ function getMockStateManager(): CronjobControllerStateManager { setEventDate: (id, date) => { dates.set(id, date); }, + deleteEventDate: (id) => { + dates.delete(id); + }, }; } @@ -544,6 +547,131 @@ describe('CronjobController', () => { cronjobController.destroy(); }); + it('removes the persisted date when a one-shot event fires', async () => { + const rootMessenger = getRootCronjobControllerMessenger(); + const controllerMessenger = + getRestrictedCronjobControllerMessenger(rootMessenger); + + rootMessenger.registerActionHandler( + 'SnapController:handleRequest', + jest.fn().mockResolvedValue(undefined), + ); + + const stateManager = getMockStateManager(); + const deleteEventDate = jest.spyOn(stateManager, 'deleteEventDate'); + + const cronjobController = new CronjobController({ + messenger: controllerMessenger, + stateManager, + state: { + events: { + foo: { + id: 'foo', + snapId: MOCK_SNAP_ID, + date: new Date('2022-01-01T00:00Z').toISOString(), + scheduledAt: new Date('2022-01-01T00:00Z').toISOString(), + schedule: '2022-01-01T00:00Z', + recurring: false, + request: { method: 'exampleMethod', params: [] }, + }, + }, + }, + }); + + cronjobController.init(); + await new Promise((resolve) => originalProcessNextTick(resolve)); + + // Without this the date store keeps a key for every event that has already + // fired, growing without bound — which would undo the point of storing + // dates separately in the first place. + expect(deleteEventDate).toHaveBeenCalledWith('foo'); + + cronjobController.destroy(); + }); + + it('removes the persisted date when an event is cancelled', () => { + const rootMessenger = getRootCronjobControllerMessenger(); + const controllerMessenger = + getRestrictedCronjobControllerMessenger(rootMessenger); + + const stateManager = getMockStateManager(); + const deleteEventDate = jest.spyOn(stateManager, 'deleteEventDate'); + + const cronjobController = new CronjobController({ + messenger: controllerMessenger, + stateManager, + }); + + const id = cronjobController.schedule({ + snapId: MOCK_SNAP_ID, + schedule: new Date(Date.now() + inMilliseconds(1, Duration.Hour)) + .toISOString() + .replace(/\.\d{3}/u, ''), + request: { method: 'exampleMethod', params: [] }, + }); + + cronjobController.cancel(MOCK_SNAP_ID, id); + + expect(deleteEventDate).toHaveBeenCalledWith(id); + + cronjobController.destroy(); + }); + + it('schedules the remaining events when one of them has an unusable date', () => { + const rootMessenger = getRootCronjobControllerMessenger(); + const controllerMessenger = + getRestrictedCronjobControllerMessenger(rootMessenger); + + rootMessenger.registerActionHandler( + 'SnapController:handleRequest', + jest.fn().mockResolvedValue(undefined), + ); + + jest.spyOn(console, 'error').mockImplementation(); + + const stateManager = getMockStateManager(); + const setEventDate = jest.spyOn(stateManager, 'setEventDate'); + + const cronjobController = new CronjobController({ + messenger: controllerMessenger, + stateManager, + state: { + events: { + // Ordered first on purpose: before the loop caught its own errors, + // this one threw out of `init` and every event behind it was never + // scheduled at all. + broken: { + id: 'broken', + snapId: MOCK_SNAP_ID, + date: undefined as unknown as string, + scheduledAt: new Date('2022-01-01T00:00Z').toISOString(), + schedule: 'PT30S', + recurring: true, + request: { method: 'brokenMethod', params: [] }, + }, + healthy: { + id: 'healthy', + snapId: MOCK_SNAP_ID, + date: new Date('2022-01-01T00:00Z').toISOString(), + scheduledAt: new Date('2022-01-01T00:00Z').toISOString(), + schedule: 'PT25H', + recurring: true, + request: { method: 'healthyMethod', params: [] }, + }, + }, + }, + }); + + expect(() => cronjobController.init()).not.toThrow(); + + // The past-dated healthy event executes immediately and reschedules, which + // is only reachable if the loop survived the broken event before it. + expect(setEventDate).toHaveBeenCalledWith('healthy', expect.any(String)); + expect(setEventDate).not.toHaveBeenCalledWith('broken', expect.any(String)); + + cronjobController.destroy(); + }); + it('handles the `snapInstalled` event', () => { const rootMessenger = getRootCronjobControllerMessenger(); const controllerMessenger = diff --git a/packages/snaps-controllers/src/cronjob/CronjobController.ts b/packages/snaps-controllers/src/cronjob/CronjobController.ts index cf92079d5f..7e5958eecf 100644 --- a/packages/snaps-controllers/src/cronjob/CronjobController.ts +++ b/packages/snaps-controllers/src/cronjob/CronjobController.ts @@ -85,6 +85,18 @@ export type CronjobControllerStateManager = { */ setEventDate(id: string, date: string): void; + /** + * Remove an event's persisted date. + * + * An implementation that stores dates apart from the rest of the state has + * no other signal that an event is gone: removing it from `events` says + * nothing about the separate date. Without this the date store grows without + * bound, one orphaned key per event that is cancelled or fires once. + * + * @param id - The ID of the event whose date should be removed. + */ + deleteEventDate(id: string): void; + getInitialState(): CronjobControllerState | undefined; }; @@ -425,6 +437,20 @@ export class CronjobController extends BaseController< const ms = DateTime.fromISO(event.date, { setZone: true }).toMillis() - Date.now(); + // Every comparison against NaN is false, so an unparseable date would fall + // through both guards below and reach `new Timer(NaN)`, which throws. That + // throw escapes `#reschedule`'s loop and strands every event behind this + // one, so a single bad date takes down all scheduling rather than itself. + // A client is expected to repair dates before handing state over; this is + // the backstop for one that does not. + if (Number.isNaN(ms)) { + throw new Error( + `Background event "${event.id}" has an unusable date: "${String( + event.date, + )}".`, + ); + } + // We don't schedule this job yet as it is too far in the future. if (ms > DAILY_TIMEOUT) { return; @@ -479,6 +505,7 @@ export class CronjobController extends BaseController< }); this.#stateManager.set(nextState); + this.#stateManager.deleteEventDate(event.id); return; } @@ -502,6 +529,7 @@ export class CronjobController extends BaseController< }); this.#stateManager.set(nextState); + this.#stateManager.deleteEventDate(id); } /** @@ -613,12 +641,21 @@ export class CronjobController extends BaseController< // If the event is recurring and the date is in the past, execute it // immediately. - if (event.recurring && eventDate <= now) { - this.#execute(event); - continue; + try { + if (event.recurring && eventDate <= now) { + this.#execute(event); + continue; + } + + this.#schedule(event, false); + } catch (error) { + // One unschedulable event must not strand the others. Without this the + // loop aborts on the first throw, every event after it in iteration + // order is silently never scheduled, and — because the daily timer's + // callback is `#reschedule(); #start();` — the re-arm is skipped too, + // so scheduling stops for the rest of the session. + logError(`Failed to schedule background event "${event.id}".`, error); } - - this.#schedule(event, false); } } diff --git a/packages/snaps-controllers/src/cronjob/index.ts b/packages/snaps-controllers/src/cronjob/index.ts index c87ea0c970..a51838f707 100644 --- a/packages/snaps-controllers/src/cronjob/index.ts +++ b/packages/snaps-controllers/src/cronjob/index.ts @@ -9,6 +9,7 @@ export type { CronjobControllerStateManager, } from './CronjobController'; export { CronjobController } from './CronjobController'; +export { recoverEventDate } from './utils'; export type { CronjobControllerInitAction, CronjobControllerScheduleAction, diff --git a/packages/snaps-controllers/src/cronjob/utils.ts b/packages/snaps-controllers/src/cronjob/utils.ts index 1d61fa7f9c..76a2daa4f3 100644 --- a/packages/snaps-controllers/src/cronjob/utils.ts +++ b/packages/snaps-controllers/src/cronjob/utils.ts @@ -85,3 +85,71 @@ export function getExecutionDate(schedule: string) { ); } } + +/** + * Recover an event's next execution date when the stored date is missing. + * + * This is deliberately NOT `getExecutionDate`. That function is impure for + * durations — it returns `now + duration`, so calling it on every read would + * push a `PT30S` event forever into the future and it would never fire — and + * it throws for an absolute date that has already passed. Recovery needs the + * opposite of both: anchor on `scheduledAt` rather than on now, and return + * `undefined` rather than throw, so an unrecoverable event can be cancelled + * instead of taking the caller down with it. + * + * Recovery is possible at all because `schedule` and `scheduledAt` are written + * once when the event is added and never mutated afterwards. A client that + * stores dates separately can lose the date without losing either of them. + * + * @param event - The event whose date is missing. + * @param event.schedule - The cron expression, ISO 8601 duration, or ISO 8601 + * date that defines the event's schedule. + * @param event.scheduledAt - The ISO 8601 date at which the event was added. + * @param event.recurring - Whether the event repeats. + * @returns The recovered ISO 8601 date, or `undefined` if the schedule cannot + * be parsed. + */ +export function recoverEventDate({ + schedule, + scheduledAt, + recurring, +}: { + schedule: string; + scheduledAt: string; + recurring: boolean; +}): string | undefined { + // An absolute date is its own answer, whether or not it has passed. A past + // date means the event was due while the date was missing, and the caller + // already executes past-due events on startup. + const absolute = DateTime.fromISO(schedule, { setZone: true }); + if (absolute.isValid) { + return absolute.toUTC().startOf('second').toISO({ + suppressMilliseconds: true, + }); + } + + const duration = Duration.fromISO(schedule); + if (duration.isValid) { + // A one-shot's original date is exactly reconstructible. A recurring one's + // is not — `scheduledAt` is the creation time and never moves, so after N + // intervals it is long stale — but a recurring event only needs a valid + // next date, and losing at most one interval of phase is harmless. + const anchor = recurring + ? DateTime.now() + : DateTime.fromISO(scheduledAt, { setZone: true }); + + if (!anchor.isValid) { + return undefined; + } + + return anchor.toUTC().plus(getDuration(duration)).toISO(); + } + + try { + const parsed = parseExpression(schedule, { utc: true }); + const next = DateTime.fromJSDate(parsed.next().toDate()); + return next.isValid ? next.toUTC().toISO() : undefined; + } catch { + return undefined; + } +} From 3f546b7400ae439e66ba39f6ca6556824e68a470 Mon Sep 17 00:00:00 2001 From: Jongsun Suh Date: Mon, 31 Aug 2026 12:53:29 -0400 Subject: [PATCH 4/6] Reject an empty schedule in `recoverEventDate` instead of recovering it MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `cron-parser` accepts `''` and whitespace and reads them as `* * * * *`, so an event whose schedule did not survive storage was recovered as a once-a-minute job forever rather than being reported unrecoverable. That is the outcome the `undefined` return exists to enable, and recovery is where it bites: it runs on whatever came back from disk, not on a schedule validated at creation. Also drops an unreachable branch — `toISO()` already returns null for an invalid `DateTime`, so coalescing is equivalent to testing `isValid`. --- .../src/cronjob/utils.test.ts | 302 +++++++++++++++++- .../snaps-controllers/src/cronjob/utils.ts | 14 +- 2 files changed, 314 insertions(+), 2 deletions(-) diff --git a/packages/snaps-controllers/src/cronjob/utils.test.ts b/packages/snaps-controllers/src/cronjob/utils.test.ts index ad7e149d96..d13a772124 100644 --- a/packages/snaps-controllers/src/cronjob/utils.test.ts +++ b/packages/snaps-controllers/src/cronjob/utils.test.ts @@ -1,4 +1,8 @@ -import { getCronjobSpecificationSchedule, getExecutionDate } from './utils'; +import { + getCronjobSpecificationSchedule, + getExecutionDate, + recoverEventDate, +} from './utils'; jest.useFakeTimers(); jest.setSystemTime(1747994147500); @@ -74,3 +78,299 @@ describe('getExecutionDate', () => { ).toThrow('Cannot schedule an event in the past.'); }); }); + +describe('recoverEventDate', () => { + it('returns an absolute ISO 8601 date', () => { + expect( + recoverEventDate({ + schedule: '2025-05-24T09:55:47Z', + scheduledAt: '2025-05-23T09:00:00.000Z', + recurring: false, + }), + ).toBe('2025-05-24T09:55:47Z'); + + expect( + recoverEventDate({ + schedule: '2025-05-24T09:55:47+00:00', + scheduledAt: '2025-05-23T09:00:00.000Z', + recurring: false, + }), + ).toBe('2025-05-24T09:55:47Z'); + + expect( + recoverEventDate({ + schedule: '2025-05-24T09:55:47+01:00', + scheduledAt: '2025-05-23T09:00:00.000Z', + recurring: false, + }), + ).toBe('2025-05-24T08:55:47Z'); + }); + + it('truncates an absolute ISO 8601 date to the second', () => { + expect( + recoverEventDate({ + schedule: '2025-05-24T09:55:47.999Z', + scheduledAt: '2025-05-23T09:00:00.000Z', + recurring: false, + }), + ).toBe('2025-05-24T09:55:47Z'); + }); + + it('ignores `scheduledAt` and `recurring` for an absolute ISO 8601 date', () => { + expect( + recoverEventDate({ + schedule: '2025-05-24T09:55:47Z', + scheduledAt: 'invalid', + recurring: true, + }), + ).toBe('2025-05-24T09:55:47Z'); + }); + + it('returns an absolute ISO 8601 date in the past without throwing', () => { + expect(() => + recoverEventDate({ + schedule: '2020-01-01T00:00:00Z', + scheduledAt: '2019-12-01T00:00:00.000Z', + recurring: false, + }), + ).not.toThrow(); + + expect( + recoverEventDate({ + schedule: '2020-01-01T00:00:00Z', + scheduledAt: '2019-12-01T00:00:00.000Z', + recurring: false, + }), + ).toBe('2020-01-01T00:00:00Z'); + + expect( + recoverEventDate({ + schedule: new Date(Date.now() - 100).toISOString(), + scheduledAt: '2025-05-23T09:00:00.000Z', + recurring: false, + }), + ).toBe('2025-05-23T09:55:47Z'); + }); + + it('anchors an ISO 8601 duration on `scheduledAt` for a one-shot event', () => { + expect( + recoverEventDate({ + schedule: 'PT1H', + scheduledAt: '2025-05-23T09:00:00.000Z', + recurring: false, + }), + ).toBe('2025-05-23T10:00:00.000Z'); + + expect( + recoverEventDate({ + schedule: 'P1Y', + scheduledAt: '2025-05-23T09:00:00.000Z', + recurring: false, + }), + ).toBe('2026-05-23T09:00:00.000Z'); + + expect( + recoverEventDate({ + schedule: 'PT1H', + scheduledAt: '2025-05-23T11:00:00+02:00', + recurring: false, + }), + ).toBe('2025-05-23T10:00:00.000Z'); + }); + + it('preserves the milliseconds of `scheduledAt` for a one-shot event', () => { + expect( + recoverEventDate({ + schedule: 'PT1H', + scheduledAt: '2025-05-23T09:00:00.123Z', + recurring: false, + }), + ).toBe('2025-05-23T10:00:00.123Z'); + }); + + it('returns a date in the past for an overdue one-shot event', () => { + expect( + recoverEventDate({ + schedule: 'PT1H', + scheduledAt: '2020-01-01T00:00:00Z', + recurring: false, + }), + ).toBe('2020-01-01T01:00:00.000Z'); + }); + + it('anchors an ISO 8601 duration on the current time for a recurring event', () => { + expect( + recoverEventDate({ + schedule: 'PT1H', + scheduledAt: '2025-05-23T09:00:00.000Z', + recurring: true, + }), + ).toBe('2025-05-23T10:55:47.500Z'); + + expect( + recoverEventDate({ + schedule: 'P1Y', + scheduledAt: '2025-05-23T09:00:00.000Z', + recurring: true, + }), + ).toBe('2026-05-23T09:55:47.500Z'); + }); + + it('ignores an unusable `scheduledAt` for a recurring event', () => { + expect( + recoverEventDate({ + schedule: 'PT1H', + scheduledAt: 'invalid', + recurring: true, + }), + ).toBe('2025-05-23T10:55:47.500Z'); + }); + + it('rounds a duration of less than one second up to one second', () => { + expect( + recoverEventDate({ + schedule: 'PT0S', + scheduledAt: '2025-05-23T09:00:00.000Z', + recurring: false, + }), + ).toBe('2025-05-23T09:00:01.000Z'); + + expect( + recoverEventDate({ + schedule: 'PT0.5S', + scheduledAt: '2025-05-23T09:00:00.000Z', + recurring: false, + }), + ).toBe('2025-05-23T09:00:01.000Z'); + + expect( + recoverEventDate({ + schedule: 'PT0S', + scheduledAt: '2025-05-23T09:00:00.000Z', + recurring: true, + }), + ).toBe('2025-05-23T09:55:48.500Z'); + }); + + it('parses a cron expression', () => { + expect( + recoverEventDate({ + schedule: '0 0 * * *', + scheduledAt: '2025-05-23T09:00:00.000Z', + recurring: true, + }), + ).toBe('2025-05-24T00:00:00.000Z'); + + expect( + recoverEventDate({ + schedule: '*/5 * * * *', + scheduledAt: '2025-05-23T09:00:00.000Z', + recurring: true, + }), + ).toBe('2025-05-23T10:00:00.000Z'); + + expect( + recoverEventDate({ + schedule: '0 0 1 1 *', + scheduledAt: '2025-05-23T09:00:00.000Z', + recurring: true, + }), + ).toBe('2026-01-01T00:00:00.000Z'); + }); + + it('ignores `scheduledAt` for a cron expression', () => { + expect( + recoverEventDate({ + schedule: '0 0 * * *', + scheduledAt: 'invalid', + recurring: false, + }), + ).toBe('2025-05-24T00:00:00.000Z'); + }); + + it('returns `undefined` for an unparseable schedule', () => { + expect( + recoverEventDate({ + schedule: 'invalid', + scheduledAt: '2025-05-23T09:00:00.000Z', + recurring: false, + }), + ).toBeUndefined(); + + expect( + recoverEventDate({ + schedule: '2025-05-23T09:55:47Z+01:00', + scheduledAt: '2025-05-23T09:00:00.000Z', + recurring: false, + }), + ).toBeUndefined(); + + expect( + recoverEventDate({ + schedule: 'P1Y2M3D4H', + scheduledAt: '2025-05-23T09:00:00.000Z', + recurring: false, + }), + ).toBeUndefined(); + + expect( + recoverEventDate({ + schedule: '100 * * * * *', + scheduledAt: '2025-05-23T09:00:00.000Z', + recurring: false, + }), + ).toBeUndefined(); + + expect( + recoverEventDate({ + schedule: '0 0 30 2 *', + scheduledAt: '2025-05-23T09:00:00.000Z', + recurring: false, + }), + ).toBeUndefined(); + }); + + it('does not throw for an unparseable schedule', () => { + expect(() => + recoverEventDate({ + schedule: 'invalid', + scheduledAt: 'invalid', + recurring: false, + }), + ).not.toThrow(); + }); + + it('returns `undefined` when a duration cannot be anchored', () => { + expect( + recoverEventDate({ + schedule: 'PT1H', + scheduledAt: 'invalid', + recurring: false, + }), + ).toBeUndefined(); + + expect( + recoverEventDate({ + schedule: 'PT1H', + scheduledAt: '', + recurring: false, + }), + ).toBeUndefined(); + }); + + it.each(['', ' ', '\t'])( + 'returns undefined for the empty schedule %j', + (schedule) => { + // `cron-parser` accepts these and reads them as `* * * * *`. Recovering + // such an event would resurrect it as a once-a-minute job forever + // instead of reporting it unrecoverable. + expect( + recoverEventDate({ + schedule, + scheduledAt: '2025-05-23T09:55:47.500Z', + recurring: true, + }), + ).toBeUndefined(); + }, + ); +}); diff --git a/packages/snaps-controllers/src/cronjob/utils.ts b/packages/snaps-controllers/src/cronjob/utils.ts index 76a2daa4f3..5a71d08779 100644 --- a/packages/snaps-controllers/src/cronjob/utils.ts +++ b/packages/snaps-controllers/src/cronjob/utils.ts @@ -118,6 +118,16 @@ export function recoverEventDate({ scheduledAt: string; recurring: boolean; }): string | undefined { + // `cron-parser` accepts an empty or whitespace-only expression and treats it + // as `* * * * *`, so without this a schedule that did not survive storage + // would be "recovered" as firing every minute forever, rather than being + // reported unrecoverable so the caller can cancel it. This matters here more + // than at scheduling time: recovery runs on whatever came back from disk, + // not on a schedule that was validated when the event was created. + if (typeof schedule !== 'string' || schedule.trim() === '') { + return undefined; + } + // An absolute date is its own answer, whether or not it has passed. A past // date means the event was due while the date was missing, and the caller // already executes past-due events on startup. @@ -148,7 +158,9 @@ export function recoverEventDate({ try { const parsed = parseExpression(schedule, { utc: true }); const next = DateTime.fromJSDate(parsed.next().toDate()); - return next.isValid ? next.toUTC().toISO() : undefined; + // `toISO()` already returns null for an invalid DateTime, so coalescing is + // equivalent to testing `isValid` and leaves no unreachable branch behind. + return next.toUTC().toISO() ?? undefined; } catch { return undefined; } From 4756afc20e5de21722778ec225a8da503b2dc76b Mon Sep 17 00:00:00 2001 From: Jongsun Suh Date: Tue, 1 Sep 2026 10:33:11 -0400 Subject: [PATCH 5/6] Settle the executing event before destroying the controller in a test The healthy event in the unusable-date test executes asynchronously, so its promise outlived `destroy` and rescheduled against a torn-down controller. Locally that passes; under CI's parallel workers it leaves the worker unable to exit, which the retry wrapper reports as a failed job. --- .../src/cronjob/CronjobController.test.ts | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/packages/snaps-controllers/src/cronjob/CronjobController.test.ts b/packages/snaps-controllers/src/cronjob/CronjobController.test.ts index 222b1b0c0f..752d8ad363 100644 --- a/packages/snaps-controllers/src/cronjob/CronjobController.test.ts +++ b/packages/snaps-controllers/src/cronjob/CronjobController.test.ts @@ -617,7 +617,7 @@ describe('CronjobController', () => { cronjobController.destroy(); }); - it('schedules the remaining events when one of them has an unusable date', () => { + it('schedules the remaining events when one of them has an unusable date', async () => { const rootMessenger = getRootCronjobControllerMessenger(); const controllerMessenger = getRestrictedCronjobControllerMessenger(rootMessenger); @@ -664,6 +664,11 @@ describe('CronjobController', () => { expect(() => cronjobController.init()).not.toThrow(); + // The healthy event executes asynchronously; without settling it here its + // promise outlives `destroy` and reschedules against a torn-down + // controller, which leaves the jest worker unable to exit. + await new Promise((resolve) => originalProcessNextTick(resolve)); + // The past-dated healthy event executes immediately and reschedules, which // is only reachable if the loop survived the broken event before it. expect(setEventDate).toHaveBeenCalledWith('healthy', expect.any(String)); From 1d4788f425c5272861e3214f5f806305756f1c60 Mon Sep 17 00:00:00 2001 From: Jongsun Suh Date: Tue, 1 Sep 2026 10:56:26 -0400 Subject: [PATCH 6/6] Ratchet `snaps-controllers` coverage for the new tests `test:post` rewrites `coverage.json` whenever a metric rises by at least 0.3%, and CI's clean-working-directory check fails on the resulting diff. The new cronjob tests move lines and statements past that threshold. --- packages/snaps-controllers/coverage.json | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/snaps-controllers/coverage.json b/packages/snaps-controllers/coverage.json index d3637a7234..d82efd58e1 100644 --- a/packages/snaps-controllers/coverage.json +++ b/packages/snaps-controllers/coverage.json @@ -1,6 +1,6 @@ { "branches": 94.97, "functions": 98.78, - "lines": 98.45, - "statements": 98.18 + "lines": 98.76, + "statements": 98.49 }