fix: gracefully handle errors for multiple failing requests (#2346)

Fixes a problem where if multiple simultaneous requests were sent to a
Snap that resulted in an error, termination would be attempted multiple
times causing unintentional errors to be surfaced to the caller.

This is fixed by making sure `stopSnap` is only called once even if we
have multiple pending requests for the Snap.

packages/snaps-controllers/coverage.json

@@ -1,6 +1,6 @@
 {
-  "branches": 91.47,
+  "branches": 91.49,
   "functions": 96.61,
-  "lines": 97.86,
+  "lines": 97.87,
   "statements": 97.53
 }

packages/snaps-controllers/src/snaps/SnapController.test.ts

@@ -1567,6 +1567,74 @@ describe('SnapController', () => {
     snapController.destroy();
   });
 
+  it('gracefully throws for multiple failing requests', async () => {
+    const sourceCode = `
+    module.exports.onRpcRequest = async () => snap.request({ method: 'snap_dialog', params: null });
+    `;
+
+    const options = getSnapControllerWithEESOptions({
+      environmentEndowmentPermissions: [SnapEndowments.EthereumProvider],
+      idleTimeCheckInterval: 30000,
+      maxIdleTime: 160000,
+      state: {
+        snaps: getPersistedSnapsState(
+          getPersistedSnapObject({
+            sourceCode,
+            manifest: getSnapManifest({
+              shasum: await getSnapChecksum(getMockSnapFiles({ sourceCode })),
+            }),
+          }),
+        ),
+      },
+    });
+
+    const { rootMessenger } = options;
+    const [snapController, service] = getSnapControllerWithEES(options);
+    const snap = snapController.getExpect(MOCK_SNAP_ID);
+
+    rootMessenger.registerActionHandler(
+      'PermissionController:hasPermission',
+      () => true,
+    );
+
+    const results = (await Promise.allSettled([
+      snapController.handleRequest({
+        snapId: snap.id,
+        origin: 'foo.com',
+        handler: HandlerType.OnRpcRequest,
+        request: {
+          jsonrpc: '2.0',
+          method: 'test',
+          params: {},
+          id: 1,
+        },
+      }),
+      snapController.handleRequest({
+        snapId: snap.id,
+        origin: 'foo.com',
+        handler: HandlerType.OnRpcRequest,
+        request: {
+          jsonrpc: '2.0',
+          method: 'test',
+          params: {},
+          id: 1,
+        },
+      }),
+    ])) as PromiseRejectedResult[];
+
+    expect(results[0].status).toBe('rejected');
+    expect(results[0].reason.message).toBe(
+      "'args.params' must be an object or array if provided.",
+    );
+    expect(results[1].status).toBe('rejected');
+    expect(results[1].reason.message).toBe(
+      "'args.params' must be an object or array if provided.",
+    );
+
+    snapController.destroy();
+    await service.terminateAllSnaps();
+  });
+
   it('does not kill snaps with open sessions', async () => {
     const sourceCode = `
       module.exports.onRpcRequest = () => 'foo bar';

packages/snaps-controllers/src/snaps/SnapController.ts

@@ -233,6 +233,11 @@ export interface SnapRuntimeData {
    * Cached encryption salt used for state encryption.
    */
   encryptionSalt: string | null;
+
+  /**
+   * A boolean flag to determine whether the Snap is currently being stopped.
+   */
+  stopping: boolean;
 }
 
 export type SnapError = {
@@ -1438,16 +1443,26 @@ export class SnapController extends BaseController<
       throw new Error(`The snap "${snapId}" is not running.`);
     }
 
-    // Reset request tracking
-    runtime.lastRequest = null;
-    runtime.pendingInboundRequests = [];
-    runtime.pendingOutboundRequests = 0;
+    // No-op if the Snap is already stopping.
+    if (runtime.stopping) {
+      return;
+    }
+
+    // Flag that the Snap is actively stopping, this prevents other calls to stopSnap
+    // while we are handling termination of the Snap
+    runtime.stopping = true;
+
     try {
       if (this.isRunning(snapId)) {
         this.#closeAllConnections?.(snapId);
         await this.#terminateSnap(snapId);
       }
     } finally {
+      // Reset request tracking
+      runtime.lastRequest = null;
+      runtime.pendingInboundRequests = [];
+      runtime.pendingOutboundRequests = 0;
+      runtime.stopping = false;
       if (this.isRunning(snapId)) {
         this.#transition(snapId, statusEvent);
       }
@@ -3385,6 +3400,7 @@ export class SnapController extends BaseController<
       pendingInboundRequests: [],
       pendingOutboundRequests: 0,
       interpreter,
+      stopping: false,
     });
   }