Stop persisting snaps in the installing state (#1876)

Stops persisting snaps in the installing state, this prevents a bug where snaps could enter an invalid and unusable state if installation was halted unexpectedly.
MetaMask · Oct 20, 2023 · 6d78d2c · 6d78d2c
1 parent 5a700c8
commit 6d78d2c
Show file tree

Hide file tree

Showing 3 changed files with 54 additions and 14 deletions.
diff --git a/packages/snaps-controllers/coverage.json b/packages/snaps-controllers/coverage.json
@@ -1,6 +1,6 @@
 {
   "branches": 89.51,
-  "functions": 96.2,
+  "functions": 96.21,
   "lines": 97.09,
   "statements": 96.76
 }
diff --git a/packages/snaps-controllers/src/snaps/SnapController.test.ts b/packages/snaps-controllers/src/snaps/SnapController.test.ts
@@ -284,7 +284,7 @@ describe('SnapController', () => {
               version: '0.0.1',
               sourceCode: DEFAULT_SNAP_BUNDLE,
               id,
-              status: SnapStatus.Installing,
+              status: SnapStatus.Stopped,
             }),
           ),
         },
@@ -313,6 +313,41 @@ describe('SnapController', () => {
     secondSnapController.destroy();
   });
 
+  it('does not persist snaps in the installing state', async () => {
+    const firstSnapController = getSnapController(
+      getSnapControllerOptions({
+        state: {
+          snaps: getPersistedSnapsState(
+            getPersistedSnapObject({
+              version: '0.0.1',
+              sourceCode: DEFAULT_SNAP_BUNDLE,
+              status: SnapStatus.Installing,
+            }),
+          ),
+        },
+      }),
+    );
+
+    expect(firstSnapController.state.snaps[MOCK_SNAP_ID]).toBeDefined();
+
+    // persist the state somewhere
+    const persistedState = getPersistentState<SnapControllerState>(
+      firstSnapController.state,
+      firstSnapController.metadata,
+    );
+
+    // create a new controller
+    const secondSnapController = getSnapController(
+      getSnapControllerOptions({
+        state: persistedState,
+      }),
+    );
+
+    expect(secondSnapController.state.snaps[MOCK_SNAP_ID]).toBeUndefined();
+    firstSnapController.destroy();
+    secondSnapController.destroy();
+  });
+
   it('handles an error event on the controller messenger', async () => {
     const options = getSnapControllerWithEESOptions({
       state: {

diff --git a/packages/snaps-controllers/src/snaps/SnapController.ts b/packages/snaps-controllers/src/snaps/SnapController.ts
@@ -706,18 +706,23 @@ export class SnapController extends BaseController<
         },
         snaps: {
           persist: (snaps) => {
-            return Object.values(snaps)
-              .map((snap) => {
-                return {
-                  ...snap,
-                  // At the time state is rehydrated, no snap will be running.
-                  status: SnapStatus.Stopped,
-                };
-              })
-              .reduce((memo: Record<ValidatedSnapId, Snap>, snap) => {
-                memo[snap.id] = snap;
-                return memo;
-              }, {});
+            return (
+              Object.values(snaps)
+                // We should not persist snaps that are in the installing state,
+                // since they haven't completed installation and would be unusable
+                .filter((snap) => snap.status !== SnapStatus.Installing)
+                .map((snap) => {
+                  return {
+                    ...snap,
+                    // At the time state is rehydrated, no snap will be running.
+                    status: SnapStatus.Stopped,
+                  };
+                })
+                .reduce((memo: Record<ValidatedSnapId, Snap>, snap) => {
+                  memo[snap.id] = snap;
+                  return memo;
+                }, {})
+            );
           },
           anonymous: false,
         },