feat: add/improve Sentry instrumentation for storage related error handling (#39113)

gauthierpetetin · web-flow · commit a200b56c4fbe · 2026-01-16T18:25:41.000Z
## Description This PR is a follow-up PR for #39010 It adds Sentry instrumentation to monitor storage-related errors and enables error reporting even during database corruption scenarios. ### Context Firefox users occasionally experience database corruption that prevents MetaMask from reading or writing to `storage.local`. This manifests as `"Error: An unexpected error occurred"`. We have workarounds in place (vault recovery flow, storage error toast), but we lacked visibility into how often these occur in production. ### Changes ### 1. Added Sentry Tags and Fingerprints - Added `persistence.error` tags to `captureException` calls: - `get-failed` - when `storage.local.get()` fails - `set-failed` - when `storage.local.set()` fails - `persist-failed` - when IndexedDB backup fails - `backup-db-open-failed` - when backup database can't be opened - Added custom fingerprints to prevent Sentry's deduplication from dropping events with the same underlying error message (e.g., Firefox's generic "An unexpected error occurred") ### 2. Enabled Sentry Reporting During Storage Corruption - Added `MetaMetricsController` to the list of backed-up controllers in IndexedDB - Added `getBackupState` hook to allow Sentry to check MetaMetrics consent from backup when primary storage is unavailable - This ensures we can report errors to Sentry even during database corruption scenarios (when primary storage is unreadable) ### 3. Added Recovery Tracking for Temporary Failures - Added `captureMessage` calls to track when storage operations recover after a temporary failure: - `set-recovered` - when `storage.local.set()` succeeds after a previous failure - `persist-recovered` - when IndexedDB backup succeeds after a previous failure - This helps answer the question: "Do set calls ever fail and then succeed in the same session?" - Uses `persistence.event` tag (vs `persistence.error`) to distinguish recovery events from error events ### 4. Fixed State Persistence in Backup Code Path - Fixed bug where backup code wrote unfiltered controller state (including non-persistent properties like `KeyringController.encryptionKey`) to storage - Now uses `deriveStateFromMetadata()` to filter to only `persist: true` properties, matching the normal `stateChange` behavior ## **Changelog** CHANGELOG entry: Adds Sentry instrumentation to monitor storage-related errors and enables error reporting even during database corruption scenarios. ## **Related issues** None ## **Manual testing steps** Same as in this PR: #39010 ## **Screenshots/Recordings** ### **Before** Nothing visible in Sentry ### **After** <img width="1426" height="328" alt="Screenshot 2026-01-08 at 22 24 27" src="https://github.com/user-attachments/assets/eee8d9fd-2fb6-40f3-9268-5c267a4028dd" /> ## **Pre-merge author checklist** - [x] I've followed [MetaMask Contributor Docs](https://github.com/MetaMask/contributor-docs) and [MetaMask Extension Coding Standards](https://github.com/MetaMask/metamask-extension/blob/main/.github/guidelines/CODING_GUIDELINES.md). - [x] I've completed the PR template to the best of my ability - [ ] I've included tests if applicable - [x] I've documented my code using [JSDoc](https://jsdoc.app/) format if applicable - [x] I've applied the right labels on the PR (see [labeling guidelines](https://github.com/MetaMask/metamask-extension/blob/main/.github/guidelines/LABELING_GUIDELINES.md)). Not required for external contributors. ## **Pre-merge reviewer checklist** - [ ] I've manually tested the PR (e.g. pull and build branch, run the app, test code being changed). - [ ] I confirm that this PR addresses all acceptance criteria described in the ticket it closes and includes the necessary testing evidence such as recordings and or screenshots.  --- > [!NOTE] > **Sentry instrumentation & recovery** > > - Add `persistence.error` tags and custom fingerprints to `captureException` for storage paths: `get-failed`, `set-failed`, `set-backup-failed`, `persist-failed`, `persist-backup-failed`, and `backup-db-open-failed`. > - Emit `captureMessage` recovery signals with `persistence.event` tags: `set-recovered` and `persist-recovered` when writes/backups succeed after a prior failure. > - Log & Sentry-capture `storage.local.get` errors without immediately throwing; throw `PersistenceError` with backup payload when vault is missing and a backup exists. > > **Backup & consent fallback** > > - Add `MetaMetricsController` to `backedUpStateKeys` and `Backup`; update `getBackup()` accordingly. > - In `background.js` split-state `stateChange`, use `deriveStateFromMetadata(..., 'persist')` to only back up persistent controller fields; throw if metadata missing. > - New `stateHooks.getBackupState()` to read IndexedDB backup; `setupSentry.getMetaMetricsEnabled` falls back to backup state when primary storage retrieval fails; add `getMetaMetricsEnabledFromBackupState`. > > **PersistenceManager robustness** > > - Differentiate storage.local vs IndexedDB backup failures; add Sentry tags/fingerprints per failure type and recovery notifications; notify UI via `setOnSetFailed` when writes fail. > - Handle Firefox private-browsing IndexedDB `InvalidStateError` with tagged Sentry capture and graceful degradation. > > **Tests** > > - Update `persistence-manager.test.ts` to verify new tags/fingerprints, recovery `captureMessage` events, backup DB open error handling, and repeated failure/success behavior. > > <sup>Written by [Cursor Bugbot](https://cursor.com/dashboard?tab=bugbot) for commit 1ca4d4d. This will update automatically on new commits. Configure [here](https://cursor.com/dashboard?tab=bugbot).</sup>
diff --git a/app/scripts/lib/setup-initial-state-hooks.js b/app/scripts/lib/setup-initial-state-hooks.js
@@ -27,6 +27,16 @@ globalThis.stateHooks.getPersistedState = async function () {
   return await sentryLocalStore.get({ validateVault: false });
 };
 
+/**
+ * Get the backup state from IndexedDB.
+ * This is used as a fallback when primary storage is unavailable.
+ *
+ * @returns The backup state, or null if unavailable.
+ */
+globalThis.stateHooks.getBackupState = async function () {
+  return await sentryLocalStore.getBackup();
+};
+
 const persistedStateMask = {
   data: SENTRY_BACKGROUND_STATE,
   meta: {
diff --git a/app/scripts/lib/setupSentry.js b/app/scripts/lib/setupSentry.js
@@ -205,6 +205,16 @@ function getMetaMetricsEnabledFromPersistedState(persistedState) {
   );
 }
 
+/**
+ * Returns whether MetaMetrics is enabled, given the backup state.
+ *
+ * @param {unknown} backupState - Backup state from IndexedDB
+ * @returns `true` if MetaMetrics is enabled in the backup, `false` otherwise.
+ */
+function getMetaMetricsEnabledFromBackupState(backupState) {
+  return Boolean(backupState?.MetaMetricsController?.participateInMetaMetrics);
+}
+
 function getSentryEnvironment() {
   if (METAMASK_BUILD_TYPE === 'main') {
     return METAMASK_ENVIRONMENT;
@@ -265,8 +275,15 @@ async function getMetaMetricsEnabled() {
     const persistedState = await globalThis.stateHooks.getPersistedState();
     return getMetaMetricsEnabledFromPersistedState(persistedState);
   } catch (error) {
-    log('Error retrieving persisted state', error);
-    return false;
+    log('Error retrieving persisted state, falling back to backup', error);
+    // Primary storage failed (e.g., database corruption) - check the backup
+    try {
+      const backupState = await globalThis.stateHooks.getBackupState();
+      return getMetaMetricsEnabledFromBackupState(backupState);
+    } catch (backupError) {
+      log('Error retrieving backup state', backupError);
+      return false;
+    }
   }
 }
 
diff --git a/app/scripts/lib/stores/persistence-manager.test.ts b/app/scripts/lib/stores/persistence-manager.test.ts
@@ -3,7 +3,10 @@
 import 'navigator.locks';
 import log from 'loglevel';
 
-import { captureException } from '../../../../shared/lib/sentry';
+import {
+  captureException,
+  captureMessage,
+} from '../../../../shared/lib/sentry';
 import { MISSING_VAULT_ERROR } from '../../../../shared/constants/errors';
 import { PersistenceManager } from './persistence-manager';
 import ExtensionStore from './extension-store';
@@ -32,8 +35,10 @@ jest.mock('loglevel', () => ({
 }));
 jest.mock('../../../../shared/lib/sentry', () => ({
   captureException: jest.fn(),
+  captureMessage: jest.fn(),
 }));
 const mockedCaptureException = jest.mocked(captureException);
+const mockedCaptureMessage = jest.mocked(captureMessage);
 
 describe('PersistenceManager', () => {
   let manager: PersistenceManager;
@@ -78,7 +83,10 @@ describe('PersistenceManager', () => {
       mockStoreSet.mockRejectedValueOnce(error);
 
       await manager.set({ appState: { broken: true } });
-      expect(mockedCaptureException).toHaveBeenCalledWith(error);
+      expect(mockedCaptureException).toHaveBeenCalledWith(error, {
+        tags: { 'persistence.error': 'set-failed' },
+        fingerprint: ['persistence-error', 'set-failed'],
+      });
       expect(log.error).toHaveBeenCalledWith(
         'error setting state in local store:',
         error,
@@ -116,6 +124,43 @@ describe('PersistenceManager', () => {
 
       expect(mockedCaptureException).toHaveBeenCalledTimes(2);
     });
+
+    it('tracks recovery with captureMessage when store.set fails then succeeds', async () => {
+      manager.setMetadata({ version: 17 });
+
+      const error = new Error('store.set error');
+      mockStoreSet.mockRejectedValueOnce(error);
+
+      // First set fails
+      await manager.set({ appState: { broken: true } });
+
+      expect(mockedCaptureException).toHaveBeenCalledTimes(1);
+      expect(mockedCaptureMessage).not.toHaveBeenCalled();
+
+      // Second set succeeds - should trigger recovery tracking
+      mockStoreSet.mockResolvedValueOnce(undefined);
+      await manager.set({ appState: { fixed: true } });
+
+      expect(mockedCaptureMessage).toHaveBeenCalledTimes(1);
+      expect(mockedCaptureMessage).toHaveBeenCalledWith(
+        'Data persistence recovered after temporary failure',
+        {
+          level: 'info',
+          tags: { 'persistence.event': 'set-recovered' },
+          fingerprint: ['persistence-event', 'set-recovered'],
+        },
+      );
+    });
+
+    it('does not track recovery if set never failed', async () => {
+      manager.setMetadata({ version: 17 });
+
+      // Set succeeds without prior failure
+      mockStoreSet.mockResolvedValueOnce(undefined);
+      await manager.set({ appState: { working: true } });
+
+      expect(mockedCaptureMessage).not.toHaveBeenCalled();
+    });
   });
 
   describe('get', () => {
@@ -264,7 +309,10 @@ describe('PersistenceManager', () => {
 
       await manager.persist();
 
-      expect(mockedCaptureException).toHaveBeenCalledWith(error);
+      expect(mockedCaptureException).toHaveBeenCalledWith(error, {
+        tags: { 'persistence.error': 'persist-failed' },
+        fingerprint: ['persistence-error', 'persist-failed'],
+      });
       expect(log.error).toHaveBeenCalledWith(
         'error setting state in local store:',
         error,
@@ -423,7 +471,10 @@ describe('PersistenceManager', () => {
       // returns `undefined`
       expect(await brokenManager.getBackup()).toBeUndefined();
 
-      expect(mockedCaptureException).toHaveBeenCalledWith(domException);
+      expect(mockedCaptureException).toHaveBeenCalledWith(domException, {
+        tags: { 'persistence.error': 'backup-db-open-failed' },
+        fingerprint: ['persistence-error', 'backup-db-open-failed'],
+      });
       expect(consoleWarnSpy).toHaveBeenCalledWith(
         'Could not open backup database; automatic vault recovery will not be available.',
       );
diff --git a/app/scripts/lib/stores/persistence-manager.ts b/app/scripts/lib/stores/persistence-manager.ts
@@ -1,7 +1,10 @@
 import log from 'loglevel';
 import { isEmpty } from 'lodash';
 import { RuntimeObject, hasProperty, isObject } from '@metamask/utils';
-import { captureException } from '../../../../shared/lib/sentry';
+import {
+  captureException,
+  captureMessage,
+} from '../../../../shared/lib/sentry';
 import { MISSING_VAULT_ERROR } from '../../../../shared/constants/errors';
 import { getManifestFlags } from '../../../../shared/lib/manifestFlags';
 import { IndexedDBStore } from './indexeddb-store';
@@ -22,12 +25,16 @@ export type Backup = {
   // TODO: Fix in https://github.com/MetaMask/metamask-extension/issues/31860
   // eslint-disable-next-line @typescript-eslint/naming-convention
   AppMetadataController?: unknown;
+  // TODO: Fix in https://github.com/MetaMask/metamask-extension/issues/31860
+  // eslint-disable-next-line @typescript-eslint/naming-convention
+  MetaMetricsController?: unknown;
   meta?: MetaData;
 };
 
 export const backedUpStateKeys = [
   'KeyringController',
   'AppMetadataController',
+  'MetaMetricsController',
 ] as const;
 
 export type BackedUpStateKey = (typeof backedUpStateKeys)[number];
@@ -237,7 +244,13 @@ export class PersistenceManager {
           error.message ===
             'A mutation operation was attempted on a database that did not allow mutations.'
         ) {
-          captureException(error);
+          // Custom fingerprint prevents Sentry's deduplication from dropping
+          // this event when other persistence errors with the same underlying
+          // error message (e.g., "An unexpected error occurred") are reported.
+          captureException(error, {
+            tags: { 'persistence.error': 'backup-db-open-failed' },
+            fingerprint: ['persistence-error', 'backup-db-open-failed'],
+          });
           console.warn(
             'Could not open backup database; automatic vault recovery will not be available.',
           );
@@ -326,6 +339,8 @@ export class PersistenceManager {
       { mode: 'exclusive', signal: abortController.signal },
       async () => {
         this.#currentLockAbortController = undefined;
+        // Track which operation failed to use the correct Sentry tag
+        let backupFailed = false;
         try {
           // atomically set all the keys
           await this.#localStore.set({
@@ -339,19 +354,44 @@ export class PersistenceManager {
             const stringifiedBackup = JSON.stringify(backup);
             // and the backup has changed
             if (this.#backup !== stringifiedBackup) {
-              // save it to the backup DB
-              await this.#backupDb?.set(backup);
-              this.#backup = stringifiedBackup;
+              // save it to the backup DB - wrapped in try-catch to differentiate
+              // backup failures from storage.local failures in Sentry
+              try {
+                await this.#backupDb?.set(backup);
+                this.#backup = stringifiedBackup;
+              } catch (backupErr) {
+                backupFailed = true;
+                throw backupErr;
+              }
             }
           }
 
           if (this.#dataPersistenceFailing) {
             this.#dataPersistenceFailing = false;
+            // Track recovery to understand how often failures are temporary.
+            // This helps answer: "Do set calls ever fail and then succeed in the same session?"
+            captureMessage(
+              'Data persistence recovered after temporary failure',
+              {
+                level: 'info',
+                tags: { 'persistence.event': 'set-recovered' },
+                fingerprint: ['persistence-event', 'set-recovered'],
+              },
+            );
           }
         } catch (err) {
           if (!this.#dataPersistenceFailing) {
             this.#dataPersistenceFailing = true;
-            captureException(err);
+            // Use different tags to differentiate storage.local vs IndexedDB backup failures.
+            const tag = backupFailed ? 'set-backup-failed' : 'set-failed';
+
+            // Custom fingerprint prevents Sentry's deduplication from dropping
+            // this event when other persistence errors with the same underlying
+            // error message (e.g., "An unexpected error occurred") are reported.
+            captureException(err, {
+              tags: { 'persistence.error': tag },
+              fingerprint: ['persistence-error', tag],
+            });
           }
           this.#notifySetFailed();
           log.error('error setting state in local store:', err);
@@ -413,6 +453,8 @@ export class PersistenceManager {
       { mode: 'exclusive', signal: abortController.signal },
       async () => {
         this.#currentLockAbortController = undefined;
+        // Track which operation failed to use the correct Sentry tag
+        let backupFailed = false;
         try {
           const clone = structuredClone(this.#pendingPairs);
           // reset the pendingPairs
@@ -441,17 +483,44 @@ export class PersistenceManager {
           }
           if (hasVault(partialState)) {
             const backup = makeBackup(partialState, meta);
-            // save it to the backup DB
-            await this.#backupDb?.set(backup);
+            // save it to the backup DB - wrapped in try-catch to differentiate
+            // backup failures from storage.local failures in Sentry
+            try {
+              await this.#backupDb?.set(backup);
+            } catch (backupErr) {
+              backupFailed = true;
+              throw backupErr;
+            }
           }
 
           if (this.#dataPersistenceFailing) {
             this.#dataPersistenceFailing = false;
+            // Track recovery to understand how often failures are temporary.
+            // This helps answer: "Do set calls ever fail and then succeed in the same session?"
+            captureMessage(
+              'Data persistence recovered after temporary failure',
+              {
+                level: 'info',
+                tags: { 'persistence.event': 'persist-recovered' },
+                fingerprint: ['persistence-event', 'persist-recovered'],
+              },
+            );
           }
         } catch (err) {
           if (!this.#dataPersistenceFailing) {
             this.#dataPersistenceFailing = true;
-            captureException(err);
+            // Use different tags to differentiate storage.local vs IndexedDB backup failures.
+            const tag = backupFailed
+              ? 'persist-backup-failed'
+              : 'persist-failed';
+
+            // Custom fingerprint prevents Sentry's deduplication from dropping
+            // this event when other persistence errors with the same underlying
+            // error message (e.g., "An unexpected error occurred") are reported.
+            captureException(err, {
+              tags: { 'persistence.error': tag },
+              fingerprint: ['persistence-error', tag],
+            });
           }
           this.#notifySetFailed();
           log.error('error setting state in local store:', err);
@@ -493,12 +562,19 @@ export class PersistenceManager {
           ])
           .catch((error: Error): [Error, undefined] => [error, undefined]);
 
-        // Log the error if one occurred, but don't throw yet
+        // Log and capture the error if one occurred, but don't throw yet
         if (localStoreError) {
           log.error(
             'Error retrieving the current state of the local store:',
             localStoreError,
           );
+          // Custom fingerprint prevents Sentry's deduplication from dropping
+          // this event when other persistence errors with the same underlying
+          // error message (e.g., "An unexpected error occurred") are reported.
+          captureException(localStoreError, {
+            tags: { 'persistence.error': 'get-failed' },
+            fingerprint: ['persistence-error', 'get-failed'],
+          });
         }
 
         if (validateVault) {
@@ -601,12 +677,16 @@ export class PersistenceManager {
     if (!backupDb) {
       return undefined;
     }
-    const [KeyringController, AppMetadataController, meta] = await backupDb.get(
-      [...backedUpStateKeys, `meta`],
-    );
+    const [
+      KeyringController,
+      AppMetadataController,
+      MetaMetricsController,
+      meta,
+    ] = await backupDb.get([...backedUpStateKeys, `meta`]);
     return {
       KeyringController,
       AppMetadataController,
+      MetaMetricsController,
       meta: meta as MetaData | undefined,
     };
   }
