feat!: Reduce cronjob write churn by persisting dates separately

packages/snaps-controllers/CHANGELOG.md

@@ -7,6 +7,22 @@ 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` 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`.
+  - `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]
 
 ### Added

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
 }

packages/snaps-controllers/src/cronjob/CronjobController.test.ts

@@ -23,16 +23,41 @@ 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`,
+ * `setEventDate` and `deleteEventDate` 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<string, string>();
+
   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);
+    },
+    deleteEventDate: (id) => {
+      dates.delete(id);
+    },
   };
 }
 
@@ -469,6 +494,189 @@ 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('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', async () => {
+    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 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));
+    expect(setEventDate).not.toHaveBeenCalledWith('broken', expect.any(String));
+
+    cronjobController.destroy();
+  });
+
   it('handles the `snapInstalled` event', () => {
     const rootMessenger = getRootCronjobControllerMessenger();
     const controllerMessenger =

packages/snaps-controllers/src/cronjob/CronjobController.ts

@@ -71,6 +71,32 @@ 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;
+
+  /**
+   * 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;
 };
 
@@ -389,11 +415,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,
@@ -411,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;
@@ -465,6 +505,7 @@ export class CronjobController extends BaseController<
       });
 
       this.#stateManager.set(nextState);
+      this.#stateManager.deleteEventDate(event.id);
 
       return;
     }
@@ -488,6 +529,7 @@ export class CronjobController extends BaseController<
     });
 
     this.#stateManager.set(nextState);
+    this.#stateManager.deleteEventDate(id);
   }
 
   /**
@@ -599,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);
     }
   }
 

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,