perf: keep cronjob rescheduling out of the vault's database - #45936
Open
MajorLift wants to merge 9 commits into
Open
perf: keep cronjob rescheduling out of the vault's database#45936MajorLift wants to merge 9 commits into
MajorLift wants to merge 9 commits into
Conversation
`browser.storage.local` is one LevelDB instance shared by every key the extension stores. Compaction re-packs keys into blocks, so a block damaged by an interrupted compaction fails its checksum for whatever else shares it — a read of the vault can hit corruption produced while writing something unrelated. Rescheduling is the most frequent write here and none of it needs to be in that database. `IndexedDBEventDateStore` puts the dates in their own IndexedDB database, deliberately not the one the state backup uses. `init` reconciles the two stores before the controller sees any state, and sweeps date keys left behind by events that were cancelled or fired before `deleteEventDate` existed.
Contributor
|
CLA Signature Action: All authors have signed the CLA. You may need to manually re-run the blocking PR check if it doesn't pass in a few minutes. |
Rescheduling is the most frequent write this controller makes, and it was landing in `browser.storage.local` next to the vault. A next-run date is reconstructible from the event's `schedule` and `scheduledAt`, so it does not need that store's durability — `StorageService` already keeps Snaps data out of `storage.local` for exactly this reason. The event map itself stays put: `CronjobController.init` only reschedules what is already in state and never re-derives events from Snap manifests, so losing the map would silently stop every cronjob. The two stores can now drift, so `init` reconciles them before the controller sees any state, and sweeps dates left behind by events that already fired.
Contributor
Builds ready [7b666f3] [reused from 7609349]
⚡ Performance Benchmarks (Total: 🟢 10 pass · 🟡 8 warn · 🔴 4 fail)
Bundle size diffs [🚨 Warning! Bundle size has increased!]
|
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, have a team admin enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 0685d6d. Configure here.
Contributor
|
We should probably update the |
Contributor
Builds ready [0685d6d]
⚡ Performance Benchmarks (Total: 🟢 13 pass · 🟡 10 warn · 🔴 2 fail)
Bundle size diffs
|
`isJsonRecord` reimplemented a predicate `@metamask/utils` already exports, in a file that imports from that package on the line above, and diverged from the `isObject` + `hasProperty` pattern `persistence-manager` uses for the same job. The wrapper stays only to narrow to `Record<string, Json>` rather than `RuntimeObject`, since callers here have already been through `isValidJson`.
`isJsonRecord` had already been reduced to `return isObject(value)`, kept on the claim that it narrowed usefully to `Record<string, Json>`. It does not: `tsc` reports no errors in this file with both the alias and the guard gone, using `isObject` directly.
Contributor
Builds ready [fc821eb]
⚡ Performance Benchmarks (Total: 🟢 13 pass · 🟡 8 warn · 🔴 4 fail)
Bundle size diffs [🚨 Warning! Bundle size has increased!]
|
`getAllKeys` and `removeItem` reject on adapter failure, unlike `getItem` which reports one as a miss. The orphan sweep awaited both from `init`, on the boot path, so an IndexedDB open or quota error stopped the wallet starting while the durable event map was still usable. The sweep is best-effort cleanup on a store whose contents are reconstructible from `schedule` and `scheduledAt`, so it now logs and continues.
|
Contributor
Builds ready [038f792]
⚡ Performance Benchmarks (Total: 🟢 11 pass · 🟡 11 warn · 🔴 3 fail)
Bundle size diffs [🚨 Warning! Bundle size has increased!]
|
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
2 participants
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.
CHANGELOG entry: Store cronjob event dates through
StorageServiceinstead ofbrowser.storage.local, so rescheduling no longer writes to the store that holds the vault.Summary
CronjobControllerwrites its whole event map tobrowser.storage.localon every reschedule, under the keytemp-cronjob-storage, bypassingPersistenceManagerentirely — no lock, no debounce, no backup. A Snap on aPT30Sschedule reschedules every thirty seconds. Preinstalled cadences today: bitcoinPT30S, tronPT60S, institutional5/15 * * * * *plus* * * * *.None of that belongs in
storage.local.getStorageServiceInstanceOptionsalready puts Snaps data elsewhere, for a reason its own comment states: "Chrome has an issue with itsstorage.localimplementation -- it doesn't like sharing the database with 'large' keys, like Snaps source code." Cronjob rescheduling is the same kind of traffic in the same database.This PR moves per-event next-run dates onto the
StorageServiceadapter — IndexedDB everywhere except Firefox, whereindexedDBcan be switched off andbrowser.storage.localremains the fallback.setEventDateanddeleteEventDatewrite through the same adapterStorageServiceis built with. The adapter choice now lives in one place, exported fromstorage-service.ts, rather than being repeated.storage.local.CronjobController.initis#start(); #clear(); #reschedule();— it only reschedules what is already in state, and manifest cronjobs are registered solely from theendowment:cronjobcaveat onsnapInstalled/snapEnabled. Losing the map would silently stop every cronjob for an already-installed Snap until it was reinstalled. A date, by contrast, is reconstructible from the event's immutablescheduleandscheduledAt.initreconciles them before the controller sees any state. Reconciliation lives ininitbecause it is already async —getInitialStateis synchronous and stays that way.deleteEventDateexisted.Does not close #44802 (flag controller-state writes that bypass
PersistenceManager), and is worth reading against it: that issue's acceptance criteria require the cronjob-storage task to have removedCronjobControllerStorageManagerandtemp-cronjob-storageoutright. This PR reduces what that manager writes tostorage.localwithout removing it, so the bypass remains and the criterion is unmet. Adjacent to #45678 (increase persistence debounce to five seconds) and #44009 (flush keyring state persistence), which move the same quantity from the other direction.Depends on
setEventDateanddeleteEventDateare added upstream in MetaMask/snaps#4107 (reduce cronjob write churn by persisting dates separately). Until that lands and releases,CronjobControllerwill not call them, so this change is inert rather than wrong — the manager carries two methods nothing yet invokes, and reconciliation is a no-op against an empty date store.One recovery tier is left out on purpose. A date lost from both stores is still reconstructible from
scheduleandscheduledAtviarecoverEventDatein that same PR; it is not published yet, so its schedule parsing is not duplicated here. Today such an event is dropped rather than stranded.What this does not do
The write count is unchanged — the same number of writes happen, against a different store. The vault's own write path is untouched. And on Firefox the adapter falls back to
browser.storage.local, so dates stay in the same store there; that is the existingStorageServicebehaviour rather than something this PR introduces.Test plan
yarn jest app/scripts/lib/CronjobControllerStorageManager.test.ts— 14 passed.setEventDateis asserted to write to the date store and not tobrowser.storage.local— the whole point of the change.Note
Medium Risk
Changes wallet boot
initand cronjob state assembly across two stores; failures are handled best-effort, but reconciliation bugs could drop or mis-schedule Snap cronjobs.Overview
Moves per-event cronjob next-run dates off
browser.storage.local(which also holds the vault) onto the sameStorageServiceadapter used for Snaps data (IndexedDB on Chrome; Firefox keeps the existing fallback). The event map still loads and saves undertemp-cronjob-storageinstorage.local.CronjobControllerStorageManagernow takes an injectable date store (defaulting to the exportedStorageAdapterfromstorage-service.ts), addssetEventDate/deleteEventDate, and runsinitreconciliation: date store wins over stale map dates, falls back to map dates when missing, drops events with no valid date, and best-effort sweeps orphan date keys without failing wallet boot.Tests cover reconciliation, separate date persistence (no
storage.local.seton reschedule writes), and date-store errors during init.Reviewed by Cursor Bugbot for commit 038f792. Bugbot is set up for automated code reviews on this repo. Configure here.