Skip to content

Commit 08299db

Browse files
fix: Attach error handler to RPC stream (#3898)
When the execution environment fails on mobile (we still don't know the cause of this) we were seeing uncaught "Premature close" errors, which ends up crashing the app. The issue is that all streams are destroyed when the Snap terminates (as it does if it cannot initialize). Most streams already have error handling setup via `pipeline` and if the Snap in question has already run this applies to the RPC stream too. The problem is that if the Snap never starts, the RPC stream also never has any error handlers attached, so instead it bubbles up the error. This PR fixes that by attaching an error handler to the RPC stream after it has been created. https://consensyssoftware.atlassian.net/browse/WPC-516 <!-- CURSOR_SUMMARY --> --- > [!NOTE] > **Medium Risk** > Touches snap execution stream lifecycle in `AbstractExecutionService`, which is central to snap RPC plumbing; while the change is small, it could affect error propagation/logging during initialization and teardown. > > **Overview** > Prevents uncaught RPC stream errors during snap startup/termination by attaching an `error` listener to the JSON-RPC mux stream as soon as it’s created, filtering out expected `Premature close` noise while logging other failures. > > Adds a regression test that simulates a failed/unreachable execution environment and asserts the request fails cleanly without throwing `Premature close`, and relaxes test utilities to accept any `AbstractExecutionService` instance for injection. > > <sup>Written by [Cursor Bugbot](https://cursor.com/dashboard?tab=bugbot) for commit c8458fa. This will update automatically on new commits. Configure [here](https://cursor.com/dashboard?tab=bugbot).</sup> <!-- /CURSOR_SUMMARY -->
1 parent 5a88d61 commit 08299db

4 files changed

Lines changed: 69 additions & 4 deletions

File tree

packages/snaps-controllers/coverage.json

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
{
2-
"branches": 95.16,
2+
"branches": 94.97,
33
"functions": 98.78,
44
"lines": 98.63,
55
"statements": 98.43

packages/snaps-controllers/src/services/AbstractExecutionService.ts

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -320,6 +320,14 @@ export abstract class AbstractExecutionService<WorkerType>
320320
}
321321
});
322322

323+
// An error handler is not attached to the RPC stream until `setupSnapProvider` is called.
324+
// We must set it up here to prevent errors from bubbling up if the stream is destroyed before then.
325+
rpcStream.on('error', (error) => {
326+
if (error && !error.message?.match('Premature close')) {
327+
logError(`Snap: "${snapId}" - RPC stream failure:`, error);
328+
}
329+
});
330+
323331
const originalWrite = rpcStream.write.bind(rpcStream);
324332

325333
// @ts-expect-error Hack to inspect the messages being written to the stream.

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

Lines changed: 58 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -13,6 +13,7 @@ import type {
1313
CaveatConstraint,
1414
} from '@metamask/permission-controller';
1515
import { SubjectType } from '@metamask/permission-controller';
16+
import type { BasePostMessageStream } from '@metamask/post-message-stream';
1617
import { providerErrors, rpcErrors } from '@metamask/rpc-errors';
1718
import {
1819
WALLET_SNAP_PERMISSION_KEY,
@@ -59,6 +60,7 @@ import {
5960
DEFAULT_ICON_PATH,
6061
TEST_SECRET_RECOVERY_PHRASE_SEED_BYTES,
6162
MOCK_INITIAL_PERMISSIONS,
63+
MockWindowPostMessageStream,
6264
} from '@metamask/snaps-utils/test-utils';
6365
import type { SemVerRange, SemVerVersion, Json } from '@metamask/utils';
6466
import {
@@ -93,8 +95,12 @@ import {
9395
SNAP_APPROVAL_RESULT,
9496
SNAP_APPROVAL_UPDATE,
9597
} from './SnapController';
96-
import { setupMultiplex } from '../services';
97-
import type { NodeThreadExecutionService } from '../services/node';
98+
import { AbstractExecutionService, setupMultiplex } from '../services';
99+
import type {
100+
ExecutionServiceMessenger,
101+
NodeThreadExecutionService,
102+
TerminateJobArgs,
103+
} from '../services/node';
98104
import type { SnapControllerStateWithStorageService } from '../test-utils';
99105
import {
100106
approvalControllerMock,
@@ -1854,6 +1860,56 @@ describe('SnapController', () => {
18541860
snapController.destroy();
18551861
});
18561862

1863+
// This test also ensures that we do not throw "Premature close"
1864+
it('throws if the execution environment fails', async () => {
1865+
const rootMessenger = getControllerMessenger();
1866+
const options = getSnapControllerOptions({
1867+
rootMessenger,
1868+
state: { snaps: getPersistedSnapsState() },
1869+
});
1870+
1871+
class BrickedExecutionService extends AbstractExecutionService<null> {
1872+
constructor(messenger: ExecutionServiceMessenger) {
1873+
super({ messenger, setupSnapProvider: jest.fn(), pingTimeout: 1 });
1874+
}
1875+
1876+
protected async terminateJob(
1877+
_job: TerminateJobArgs<null>,
1878+
): Promise<void> {
1879+
// no-op
1880+
}
1881+
1882+
protected async initEnvStream(
1883+
_snapId: string,
1884+
): Promise<{ worker: null; stream: BasePostMessageStream }> {
1885+
return { worker: null, stream: new MockWindowPostMessageStream() };
1886+
}
1887+
}
1888+
1889+
const [snapController] = await getSnapControllerWithEES(
1890+
options,
1891+
new BrickedExecutionService(getNodeEESMessenger(rootMessenger)),
1892+
);
1893+
1894+
await expect(
1895+
snapController.handleRequest({
1896+
snapId: MOCK_SNAP_ID,
1897+
origin: MOCK_ORIGIN,
1898+
handler: HandlerType.OnRpcRequest,
1899+
request: {
1900+
jsonrpc: '2.0',
1901+
method: 'test',
1902+
params: {},
1903+
id: 1,
1904+
},
1905+
}),
1906+
).rejects.toThrow(
1907+
'The executor for "npm:@metamask/example-snap" was unreachable. The executor did not respond in time.',
1908+
);
1909+
1910+
snapController.destroy();
1911+
});
1912+
18571913
it('throws if unresponsive Snap is terminated while executing', async () => {
18581914
const { manifest, sourceCode, svgIcon } =
18591915
await getMockSnapFilesWithUpdatedChecksum({

packages/snaps-controllers/src/test-utils/controller.tsx

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -68,6 +68,7 @@ import type {
6868
MultichainRouterAllowedActions,
6969
MultichainRouterEvents,
7070
} from '../multichain';
71+
import type { AbstractExecutionService } from '../services';
7172
import type {
7273
AllowedActions,
7374
AllowedEvents,
@@ -691,7 +692,7 @@ export const getSnapController = async (
691692

692693
export const getSnapControllerWithEES = async (
693694
options = getSnapControllerOptions(),
694-
service?: ReturnType<typeof getNodeEES>,
695+
service?: AbstractExecutionService<unknown>,
695696
init = true,
696697
) => {
697698
const _service =

0 commit comments

Comments
 (0)