Skip to content

feat!: Reduce cronjob write churn by persisting dates separately - #4107

Draft
MajorLift wants to merge 6 commits into
mainfrom
feat/cronjob-persist-dates-separately
Draft

feat!: Reduce cronjob write churn by persisting dates separately#4107
MajorLift wants to merge 6 commits into
mainfrom
feat/cronjob-persist-dates-separately

Conversation

@MajorLift

@MajorLift MajorLift commented Aug 28, 2026

Copy link
Copy Markdown
Contributor

Summary

  • CronjobController rescheduling is its most frequent write by a wide margin — a Snap on a PT30S schedule reschedules every thirty seconds — and it changes exactly one field, events[id].date. It currently goes through CronjobControllerStateManager.set, which hands the client the whole event map to re-serialise on every tick.
  • This adds setEventDate(id, date) to the state manager interface and routes #schedule through it. The controller's in-memory state is updated the same way as before; only the persistence call changes.
  • Clients can now store dates apart from the rest of the state and merge them back in getInitialState. Nothing in this PR requires them to — a client that keeps writing the whole blob from setEventDate behaves exactly as it does today.
  • The three remaining set calls — add, post-execution cleanup, cancel — are structural changes to the event map and are left alone. They fire rarely.

BREAKING: CronjobControllerStateManager is exported, so every implementer must add setEventDate. The minimal migration keeps existing behaviour:

setEventDate(id, date) {
  this.set({ ...this.#state, events: { ...this.#state.events, [id]: { ...this.#state.events[id], date } } });
}

Follows on from MetaMask/snaps#3539 (use a custom state manager for the cronjob controller), which introduced this interface as a way to avoid persisting all client state on every cronjob write. This narrows the remaining case that interface did not cover.

Client-side follow-up, tracked separately: the extension's CronjobControllerStorageManager writes to browser.storage.local under temp-cronjob-storage. Backing setEventDate with StorageService there is the point of this change, and is one of the writes catalogued in MetaMask/metamask-extension#44802 (flag controller-state writes that bypass PersistenceManager), under MetaMask/metamask-extension#44253 (Storage Resilience E4: zero controller writes while idle). That work is blocked on this landing and being released.

Known limitation

If a client stores dates where they can be lost independently of the rest of the state, a missing date makes DateTime.fromISO(undefined) yield NaN. #startTimer then computes NaN milliseconds; both its guards (ms > DAILY_TIMEOUT and ms <= 0) are false against NaN, so it reaches new Timer(NaN), whose constructor asserts !Number.isNaN(ms) and throws TypeError: Can't start a timer with NaN time.

The blast radius is wider than the affected event:

  • init() throws, so the client sees an initialization failure.
  • #reschedule iterates events in a plain for loop, so every event ordered after the bad one is never scheduled either.
  • The daily timer is armed before the throw, but its callback is this.#reschedule(); this.#start(); — the throw skips the re-arm, so background scheduling stops for the rest of the session at the first daily tick.

This is unreachable today: date is always written in the same operation as the rest of the event, so it cannot go missing on its own. It becomes reachable only once a client stores dates separately, which is what this PR enables. Until recovery handling lands, a client adopting setEventDate should keep dates in a store that fails together with the main state.

Recovering instead of throwing is tractable — schedule and scheduledAt are both immutable and stay in the main state, so a lost date is reconstructible in every case — and is tracked as a follow-up rather than included here.

Test plan

  • yarn workspace @metamask/snaps-controllers jest src/cronjob/CronjobController.test.ts — 27 passed, 4 snapshots passed. The 26 pre-existing tests pass unchanged, so the refactor is behaviour-preserving.
  • New test persists a reschedule through 'setEventDate', without rewriting all state asserts setEventDate is called with the event ID and set is not called at all during a reschedule.
  • Negative control: reverting #schedule to this.#stateManager.set(nextState) fails that test (Number of calls: 0), and restoring the change passes it — so the assertion is load-bearing rather than vacuous.
  • The mock state manager in the test file now stores dates in a separate Map and merges them in getInitialState, modelling the split a real client implements. Every existing test exercises that merge path.
  • eslint clean on both changed files. tsc -b produces no errors in snaps-controllers (17 pre-existing failures in snaps-utils, unrelated).

…tateManager`

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.
@MajorLift
MajorLift requested a review from a team as a code owner August 28, 2026 18:09
@MajorLift
MajorLift marked this pull request as draft August 28, 2026 18:10
@cursor
cursor Bot requested review from FrederikBolding and Mrtenz August 28, 2026 18:15
@codecov

codecov Bot commented Aug 28, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 98.61%. Comparing base (ff9ec9c) to head (1d4788f).
⚠️ Report is 3 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #4107      +/-   ##
==========================================
+ Coverage   98.59%   98.61%   +0.01%     
==========================================
  Files         429      429              
  Lines       12495    12521      +26     
  Branches     1976     1984       +8     
==========================================
+ Hits        12320    12348      +28     
+ Misses        175      173       -2     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

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.
`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`.
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.
`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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant