From f8ce6aa77d6255f3427a5ff5eb25f50b0efaa9b9 Mon Sep 17 00:00:00 2001 From: cryptotavares Date: Fri, 30 Oct 2026 01:34:53 +0000 Subject: [PATCH] fix: resend a CLI request only when the daemon refused the connection isTransientError matched String(error), and Node fetch errors stringify as 'TypeError: fetch failed', so every network failure was resent up to 3 times, including resets after the daemon got the request (a click could run twice). Read error.cause.code and retry only ECONNREFUSED; any other connection failure fails at once with MM_DAEMON_CONNECTION_LOST. --- CHANGELOG.md | 1 + SKILL.md | 2 +- src/cli/mm.test.ts | 190 ++++++++++++++++++++++++++++++++++----------- src/cli/mm.ts | 79 ++++++++++--------- 4 files changed, 190 insertions(+), 82 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 4caf36c..445cfdc 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -21,6 +21,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Fixed +- `mm` resends a request only when the daemon refused the connection (`ECONNREFUSED`, read from Node's `error.cause.code`). It no longer resends after a reset, broken pipe or socket error, which can come after the daemon got the request (a resent `click` could tap twice); those fail at once with `MM_DAEMON_CONNECTION_LOST` (exit 3). Before, every network failure matched `fetch failed` and was resent up to 3 times. - Dispose session-owned drivers after consumer cleanup, including cleanup errors, forced relaunch, failed launch, daemon shutdown, and server startup rollback. Retain failed disposals in one server-owned lifecycle, so the next cleanup or daemon stop retries them. ## [0.12.0] diff --git a/SKILL.md b/SKILL.md index 0abff88..6231e84 100644 --- a/SKILL.md +++ b/SKILL.md @@ -761,7 +761,7 @@ Use `error.code` to decide what to do: | `MM_DEVICE_NOT_AVAILABLE` | Mobile-only tool invoked without a mobile driver | Launch a mobile (iOS/Android) session first | | `MM_TOOL_NOT_SUPPORTED_ON_PLATFORM` | Tool gated off the active platform | Use the tool on its supported platform (browser vs mobile) | | `MM_DAEMON_NOT_RUNNING` | No daemon, or it refused the connection | Run `mm launch` | -| `MM_DAEMON_CONNECTION_LOST` | Connection failed after the request was sent | The action may have run: `mm describe-screen` before you retry | +| `MM_DAEMON_CONNECTION_LOST` | Connection failed after the request was sent (never resent by `mm`) | The action may have run: `mm describe-screen` before you retry | | `MM_REQUEST_TIMEOUT` | The CLI stopped waiting for the daemon | Action state unknown: `mm describe-screen` before you retry | | `VALIDATION_ERROR` | Input failed the tool schema | Fix the arguments (exit 2) | diff --git a/src/cli/mm.test.ts b/src/cli/mm.test.ts index d3074b1..7a2ec28 100644 --- a/src/cli/mm.test.ts +++ b/src/cli/mm.test.ts @@ -26,6 +26,7 @@ import { resolveWithinFromArgs, getPositionalTarget, getPositionalArgs, + isConnectionError, isTransientError, parseIntFlag, parseStringFlag, @@ -551,29 +552,61 @@ describe('getNetworkErrorCode', () => { }); describe('isTransientError', () => { - it('returns true for ECONNREFUSED', () => { - expect(isTransientError(new Error('ECONNREFUSED'))).toBe(true); + const fetchError = (cause: unknown): TypeError => + new TypeError('fetch failed', { cause }); + + it('returns true for a refused connection (Node fetch cause.code)', () => { + expect(isTransientError(fetchError({ code: 'ECONNREFUSED' }))).toBe(true); }); - it('returns true for ECONNRESET', () => { - expect(isTransientError(new Error('ECONNRESET'))).toBe(true); + it('returns true when every address of an AggregateError refused', () => { + expect( + isTransientError( + fetchError({ + errors: [{ code: 'ECONNREFUSED' }, { code: 'ECONNREFUSED' }], + }), + ), + ).toBe(true); }); - it('returns true for EPIPE', () => { - expect(isTransientError(new Error('EPIPE'))).toBe(true); + it('returns false when one address of an AggregateError did not refuse', () => { + expect( + isTransientError( + fetchError({ + errors: [{ code: 'ECONNREFUSED' }, { code: 'ECONNRESET' }], + }), + ), + ).toBe(false); }); - it('returns true for UND_ERR_SOCKET', () => { - expect(isTransientError(new Error('UND_ERR_SOCKET'))).toBe(true); + it.each([['ECONNRESET'], ['EPIPE'], ['UND_ERR_SOCKET']])( + 'returns false for %s (the daemon may have got the request)', + (code) => { + expect(isTransientError(fetchError({ code }))).toBe(false); + expect(isTransientError(new Error(code))).toBe(false); + }, + ); + + it('returns false for a bare fetch failed and other errors', () => { + expect(isTransientError(new TypeError('fetch failed'))).toBe(false); + expect(isTransientError(new Error('timeout'))).toBe(false); + expect(isTransientError(new Error('404 not found'))).toBe(false); }); +}); - it('returns true for fetch failed', () => { - expect(isTransientError(new Error('fetch failed'))).toBe(true); +describe('isConnectionError', () => { + it('returns true for network failures', () => { + expect( + isConnectionError( + new TypeError('fetch failed', { cause: { code: 'ECONNRESET' } }), + ), + ).toBe(true); + expect(isConnectionError(new TypeError('fetch failed'))).toBe(true); + expect(isConnectionError(new TypeError('terminated'))).toBe(true); }); it('returns false for other errors', () => { - expect(isTransientError(new Error('timeout'))).toBe(false); - expect(isTransientError(new Error('404 not found'))).toBe(false); + expect(isConnectionError(new SyntaxError('Unexpected token'))).toBe(false); }); }); @@ -1502,12 +1535,14 @@ describe('sendRequest', () => { expectFailure('Tool failed', 1); }); - it('retries transient errors', async () => { + it('retries a refused connection', async () => { let attempts = 0; vi.spyOn(globalThis, 'fetch').mockImplementation(async () => { attempts += 1; if (attempts <= 2) { - throw new Error('ECONNREFUSED'); + throw new TypeError('fetch failed', { + cause: { code: 'ECONNREFUSED' }, + }); } return { ok: true, @@ -1520,15 +1555,6 @@ describe('sendRequest', () => { expect(attempts).toBe(3); }); - it('exits after max retries for transient errors', async () => { - vi.spyOn(globalThis, 'fetch').mockRejectedValue(new Error('ECONNREFUSED')); - - await expect( - sendRequest(3000, 'GET', '/health', null), - ).rejects.toThrowError('process.exit'); - expectFailure(expect.stringContaining('ECONNREFUSED'), 3); - }); - it('reports a refused connection as MM_DAEMON_NOT_RUNNING, exit 3', async () => { vi.spyOn(globalThis, 'fetch').mockRejectedValue( new TypeError('fetch failed', { cause: { code: 'ECONNREFUSED' } }), @@ -1538,7 +1564,10 @@ describe('sendRequest', () => { sendRequest(3000, 'GET', '/health', null), ).rejects.toThrowError('process.exit'); - const line = expectFailure('fetch failed', 3); + const line = expectFailure( + 'daemon refused the connection on port 3000 (ECONNREFUSED) after 4 attempts', + 3, + ); expect(line.error.code).toBe('MM_DAEMON_NOT_RUNNING'); expect(fetch).toHaveBeenCalledTimes(4); }); @@ -1552,9 +1581,99 @@ describe('sendRequest', () => { sendRequest(3000, 'POST', '/tool/click', {}), ).rejects.toThrowError('process.exit'); - const line = expectFailure(expect.stringContaining('fetch failed'), 3); + const line = expectFailure( + 'connection to the daemon failed (ECONNRESET): fetch failed; the action may have run, check state', + 3, + ); expect(line.error.code).toBe('MM_DAEMON_CONNECTION_LOST'); expect(line.error.hint).toContain('may have run'); + expect(line.error.retryable).toBe(false); + // Never resent: the daemon may already have tapped. + expect(fetch).toHaveBeenCalledTimes(1); + }); + + it.each([['EPIPE'], ['UND_ERR_SOCKET']])( + 'does not resend after %s', + async (code) => { + vi.spyOn(globalThis, 'fetch').mockRejectedValue( + new TypeError('fetch failed', { cause: { code } }), + ); + + await expect( + sendRequest(3000, 'POST', '/tool/click', {}), + ).rejects.toThrowError('process.exit'); + + expect(expectFailure(expect.stringContaining(code), 3).error.code).toBe( + 'MM_DAEMON_CONNECTION_LOST', + ); + expect(fetch).toHaveBeenCalledTimes(1); + }, + ); + + it('does not resend a bare fetch failed without a code', async () => { + vi.spyOn(globalThis, 'fetch').mockRejectedValue( + new TypeError('fetch failed'), + ); + + await expect( + sendRequest(3000, 'POST', '/tool/click', {}), + ).rejects.toThrowError('process.exit'); + + expect( + expectFailure( + 'connection to the daemon failed: fetch failed; the action may have run, check state', + 3, + ).error.code, + ).toBe('MM_DAEMON_CONNECTION_LOST'); + expect(fetch).toHaveBeenCalledTimes(1); + }); + + it('waits longer before each resend of a refused request', async () => { + const sleeps: number[] = []; + const realSetTimeout = globalThis.setTimeout; + vi.spyOn(globalThis, 'setTimeout').mockImplementation((( + fn: () => void, + ms?: number, + ) => { + if (ms !== undefined && ms < 1000) { + sleeps.push(ms); + fn(); + return 0 as unknown as ReturnType; + } + return realSetTimeout(fn, ms); + }) as typeof setTimeout); + vi.spyOn(globalThis, 'fetch').mockRejectedValue( + new TypeError('fetch failed', { cause: { code: 'ECONNREFUSED' } }), + ); + + await expect( + sendRequest(3000, 'GET', '/health', null), + ).rejects.toThrowError('process.exit'); + + expect(sleeps).toStrictEqual([200, 400, 600]); + }); + + it('retries a refused connection against a closed port, then fails clearly', async () => { + const { createServer } = await import('node:net'); + const server = createServer(); + const port = await new Promise((resolve) => { + server.listen(0, '127.0.0.1', () => { + resolve((server.address() as { port: number }).port); + }); + }); + await new Promise((resolve) => server.close(() => resolve())); + const fetchSpy = vi.spyOn(globalThis, 'fetch'); + + await expect( + sendRequest(port, 'GET', '/health', null), + ).rejects.toThrowError('process.exit'); + + expect(fetchSpy).toHaveBeenCalledTimes(4); + const line = expectFailure( + `daemon refused the connection on port ${port} (ECONNREFUSED) after 4 attempts`, + 3, + ); + expect(line.error.code).toBe('MM_DAEMON_NOT_RUNNING'); }); it('exits immediately for non-transient errors', async () => { @@ -1738,27 +1857,6 @@ describe('sendRequest', () => { expectFailure('Request failed', 1); }); - it('reaches the final fallback after repeated transient failures when exit does not throw', async () => { - vi.spyOn(globalThis, 'fetch').mockRejectedValue(new Error('ECONNREFUSED')); - exitSpy.mockRestore(); - exitSpy = vi.spyOn(process, 'exit').mockImplementation((() => { - return undefined as never; - }) as never); - - await sendRequest(3000, 'GET', '/health', null); - - // exit does not throw here, so every attempt reaches fail(); the last - // line is the final fallback. - const lines = failureLines(); - expect(lines[lines.length - 1].error).toStrictEqual({ - code: 'MM_DAEMON_NOT_RUNNING', - message: expect.stringContaining('request failed after 4 attempts'), - hint: 'Run `mm launch` to start.', - retryable: false, - }); - expect(process.exit).toHaveBeenLastCalledWith(3); - }); - it('merges observations into object result', async () => { vi.spyOn(globalThis, 'fetch').mockResolvedValue({ ok: true, diff --git a/src/cli/mm.ts b/src/cli/mm.ts index 6836c03..679c0e2 100644 --- a/src/cli/mm.ts +++ b/src/cli/mm.ts @@ -1169,21 +1169,39 @@ export async function routeMockNetworkCommand( } /** - * Checks whether a fetch error is transient and worth retrying. - * Only network-level failures are retried — HTTP responses (even errors) are not. + * Checks whether a failed request is safe to send again: only a refused + * connection (`ECONNREFUSED`), because the daemon never accepted the request. + * A reset, broken pipe or socket error can come after the daemon got the + * request, so a resend could run a click twice; those are not retried. + * HTTP responses (even errors) are never retried. * * @param error - The caught error from a fetch attempt. - * @returns Whether the error is transient. + * @returns Whether the request can be sent again. */ export function isTransientError(error: unknown): boolean { + const inner = (error as { cause?: { errors?: unknown } } | null | undefined) + ?.cause?.errors; + // `localhost` can resolve to several addresses; Node then reports an + // AggregateError. Retry only when every address refused. + if (Array.isArray(inner) && inner.length > 0) { + return inner.every((item) => getNetworkErrorCode(item) === 'ECONNREFUSED'); + } + return getNetworkErrorCode(error) === 'ECONNREFUSED'; +} + +/** + * Checks whether an error is a network failure of the request to the daemon + * (as opposed to a bad response body or a bug). + * + * @param error - The caught error from a fetch attempt. + * @returns Whether the connection failed. + */ +export function isConnectionError(error: unknown): boolean { + if (getNetworkErrorCode(error) !== undefined) { + return true; + } const message = String(error); - return ( - message.includes('ECONNREFUSED') || - message.includes('ECONNRESET') || - message.includes('EPIPE') || - message.includes('UND_ERR_SOCKET') || - message.includes('fetch failed') - ); + return message.includes('fetch failed') || message.includes('terminated'); } /** @@ -1258,8 +1276,10 @@ export async function daemonFetch( /** * Sends an HTTP request to the daemon and prints the response. - * Retries transient network errors (ECONNREFUSED, ECONNRESET, etc.) - * with linear backoff up to SEND_MAX_RETRIES times. + * Retries only a refused connection (ECONNREFUSED: the daemon never got the + * request), with linear backoff up to SEND_MAX_RETRIES times. Any other + * connection failure fails at once with MM_DAEMON_CONNECTION_LOST, because + * the daemon may already have run the action. * * @param port - The daemon HTTP server port. * @param method - The HTTP method to use. @@ -1340,24 +1360,28 @@ export async function sendRequest( }); } - if (isTransientError(error) && attempt < SEND_MAX_RETRIES) { + if (isTransientError(error)) { + // Refused: the daemon never got the request, so a resend is safe. lastError = error; continue; } const errorText = error instanceof Error ? error.message : String(error); - if (isTransientError(error)) { - failConnection(error, errorText); + if (isConnectionError(error)) { + const code = getNetworkErrorCode(error); + fail( + ErrorCodes.MM_DAEMON_CONNECTION_LOST, + `connection to the daemon failed${code ? ` (${code})` : ''}: ${errorText}; the action may have run, check state`, + ); } fail(ErrorCodes.MM_INTERNAL_ERROR, errorText); } } - const lastErrorText = - lastError instanceof Error ? lastError.message : String(lastError); - failConnection( - lastError, - `request failed after ${SEND_MAX_RETRIES + 1} attempts: ${lastErrorText}`, + const code = getNetworkErrorCode(lastError) ?? 'ECONNREFUSED'; + fail( + ErrorCodes.MM_DAEMON_NOT_RUNNING, + `daemon refused the connection on port ${port} (${code}) after ${SEND_MAX_RETRIES + 1} attempts`, ); } @@ -1382,21 +1406,6 @@ export function getNetworkErrorCode(error: unknown): string | undefined { )?.[1]; } -/** - * Ends the CLI after a failed connection to the daemon: refused means no - * daemon listens (`MM_DAEMON_NOT_RUNNING`); any other failure can come after - * the daemon got the request (`MM_DAEMON_CONNECTION_LOST`). - * - * @param error - The network error. - * @param message - The message to print. - * @returns Never; the process exits. - */ -function failConnection(error: unknown, message: string): never { - return getNetworkErrorCode(error) === 'ECONNREFUSED' - ? fail(ErrorCodes.MM_DAEMON_NOT_RUNNING, message) - : fail(ErrorCodes.MM_DAEMON_CONNECTION_LOST, message); -} - /** * Discovers a running daemon or auto-starts one for eligible commands. *