feat!: Reduce cronjob write churn by persisting dates separately - #4107
Draft
MajorLift wants to merge 6 commits into
Draft
feat!: Reduce cronjob write churn by persisting dates separately#4107MajorLift wants to merge 6 commits into
MajorLift wants to merge 6 commits into
Conversation
…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
marked this pull request as draft
August 28, 2026 18:10
Codecov Report✅ All modified and coverable lines are covered by tests. 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. 🚀 New features to boost your workflow:
|
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`.
5 tasks
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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
CronjobControllerrescheduling is its most frequent write by a wide margin — a Snap on aPT30Sschedule reschedules every thirty seconds — and it changes exactly one field,events[id].date. It currently goes throughCronjobControllerStateManager.set, which hands the client the whole event map to re-serialise on every tick.setEventDate(id, date)to the state manager interface and routes#schedulethrough it. The controller's in-memory state is updated the same way as before; only the persistence call changes.getInitialState. Nothing in this PR requires them to — a client that keeps writing the whole blob fromsetEventDatebehaves exactly as it does today.setcalls — add, post-execution cleanup, cancel — are structural changes to the event map and are left alone. They fire rarely.BREAKING:
CronjobControllerStateManageris exported, so every implementer must addsetEventDate. The minimal migration keeps existing behaviour: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
CronjobControllerStorageManagerwrites tobrowser.storage.localundertemp-cronjob-storage. BackingsetEventDatewithStorageServicethere is the point of this change, and is one of the writes catalogued in MetaMask/metamask-extension#44802 (flag controller-state writes that bypassPersistenceManager), 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
datemakesDateTime.fromISO(undefined)yieldNaN.#startTimerthen computesNaNmilliseconds; both its guards (ms > DAILY_TIMEOUTandms <= 0) are false againstNaN, so it reachesnew Timer(NaN), whose constructor asserts!Number.isNaN(ms)and throwsTypeError: 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.#rescheduleiterates events in a plainforloop, so every event ordered after the bad one is never scheduled either.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:
dateis 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 adoptingsetEventDateshould keep dates in a store that fails together with the main state.Recovering instead of throwing is tractable —
scheduleandscheduledAtare both immutable and stay in the main state, so a lostdateis 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.persists a reschedule through 'setEventDate', without rewriting all stateassertssetEventDateis called with the event ID andsetis not called at all during a reschedule.#scheduletothis.#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.Mapand merges them ingetInitialState, modelling the split a real client implements. Every existing test exercises that merge path.eslintclean on both changed files.tsc -bproduces no errors insnaps-controllers(17 pre-existing failures insnaps-utils, unrelated).