fix(wallet-cli): retry sendCommand only on ECONNREFUSED

packages/wallet-cli/CHANGELOG.md

@@ -21,6 +21,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 ### Changed
 
+- The daemon client (`sendCommand`) now retries only on `ECONNREFUSED`, not `ECONNRESET`, since a reset can drop after the daemon has already acted on a request — re-sending could execute a non-idempotent action (e.g. a transaction broadcast) twice ([#9608](https://github.com/MetaMask/core/pull/9608))
 - `--password` / `MM_WALLET_PASSWORD` is now optional on `mm daemon start`; on subsequent runs, omitting it starts the daemon with a locked keyring, and the persisted vault is auto-unlocked when a password is supplied ([#8821](https://github.com/MetaMask/core/pull/8821))
 - The daemon RPC server now validates `params` against each handler's superstruct before dispatch, returning a `-32602 invalidParams` error on mismatch instead of passing raw params to the handler ([#8846](https://github.com/MetaMask/core/pull/8846))
 - Report daemon socket connection errors consistently across `mm daemon call` and `mm daemon list` ([#9339](https://github.com/MetaMask/core/pull/9339))

packages/wallet-cli/src/daemon/daemon-client.test.ts

@@ -165,21 +165,21 @@ describe('sendCommand', () => {
     expect(socket.destroy).toHaveBeenCalledTimes(2);
   });
 
-  it('retries once on ECONNRESET', async () => {
+  it('does not retry on ECONNRESET, surfacing the error instead', async () => {
+    // ECONNRESET can drop after the daemon has already acted on the request, so
+    // re-sending a non-idempotent request could execute it twice. The error
+    // must surface to the caller rather than trigger a blind resend.
     setupMockSocket();
     mockWriteLine.mockResolvedValue(undefined);
-    mockReadLine
-      .mockRejectedValueOnce(
-        Object.assign(new Error('reset'), { code: 'ECONNRESET' }),
-      )
-      .mockImplementationOnce(respondWithMatchingId());
+    mockReadLine.mockRejectedValue(
+      Object.assign(new Error('reset'), { code: 'ECONNRESET' }),
+    );
 
-    const response = await sendCommand({
-      socketPath: '/tmp/test.sock',
-      method: 'test',
-    });
+    await expect(
+      sendCommand({ socketPath: '/tmp/test.sock', method: 'test' }),
+    ).rejects.toThrow('reset');
 
-    expect(response).toHaveProperty('result');
+    expect(mockReadLine).toHaveBeenCalledTimes(1);
   });
 
   it('does not retry on other errors', async () => {
@@ -286,6 +286,20 @@ describe('pingDaemon', () => {
     });
   });
 
+  it('returns unreachable with reason=refused on ECONNRESET without retrying', async () => {
+    // ECONNRESET is classified as refused but is not retried, so only a single
+    // connection attempt is made.
+    mockConnectionError('ECONNRESET');
+
+    const result = await pingDaemon('/tmp/test.sock');
+    expect(result).toStrictEqual({
+      status: 'unreachable',
+      reason: 'refused',
+      error: expect.any(Error),
+    });
+    expect(mockCreateConnection).toHaveBeenCalledTimes(1);
+  });
+
   it('returns unreachable with reason=permission on EACCES', async () => {
     mockConnectionError('EACCES');
 

packages/wallet-cli/src/daemon/daemon-client.ts

@@ -44,8 +44,12 @@ async function connectSocket(socketPath: string): Promise<Socket> {
  *
  * Opens a connection, writes one JSON-RPC request line, reads one JSON-RPC
  * response line, then closes the connection. Retries once after a short delay
- * on transient connection errors (ECONNREFUSED, ECONNRESET). Verifies that the
- * response `id` matches the outgoing request `id`.
+ * only on `ECONNREFUSED` — the connection was never established, so the daemon
+ * provably never received the request and re-sending is safe. Does not retry
+ * on `ECONNRESET`, which can drop after the daemon has already received and
+ * acted on the request: blindly re-sending could execute a non-idempotent
+ * action (e.g. a transaction broadcast) twice, so it is surfaced to the caller
+ * instead. Verifies that the response `id` matches the outgoing request `id`.
  *
  * @param options - Command options.
  * @param options.socketPath - The Unix socket path.
@@ -91,10 +95,13 @@ export async function sendCommand({
   try {
     return await attempt();
   } catch (error: unknown) {
-    if (
-      !isErrorWithCode(error, 'ECONNREFUSED') &&
-      !isErrorWithCode(error, 'ECONNRESET')
-    ) {
+    // Only retry on ECONNREFUSED: the connection was never established, so the
+    // daemon provably never received the request and re-sending is safe.
+    // ECONNRESET can drop *after* the daemon received and began (or finished)
+    // processing the request, so blindly re-sending a non-idempotent request
+    // (e.g. a transaction broadcast) could execute it twice. Surface it to the
+    // caller instead of retrying.
+    if (!isErrorWithCode(error, 'ECONNREFUSED')) {
       throw error;
     }
     await new Promise((resolve) => setTimeout(resolve, 100));
@@ -105,8 +112,9 @@ export async function sendCommand({
 /**
  * Why an unreachable daemon cannot be queried.
  *
- * - `'refused'`: connection refused after retry (`ECONNREFUSED` / `ECONNRESET`).
- *   Typical of a daemon that has crashed or is mid-restart.
+ * - `'refused'`: the connection could not be completed — `ECONNREFUSED`
+ *   (retried once) or `ECONNRESET` (a mid-request drop, not retried). Typical
+ *   of a daemon that has crashed or is mid-restart.
  * - `'timeout'`: the daemon accepted the connection but did not respond within
  *   the read timeout — most likely wedged on a long-running operation.
  * - `'permission'`: the socket exists but cannot be opened (`EACCES` / `EPERM`).