diff --git a/.github/workflows/lint-build-test.yml b/.github/workflows/lint-build-test.yml index 306446104fb..30f4ebc0fec 100644 --- a/.github/workflows/lint-build-test.yml +++ b/.github/workflows/lint-build-test.yml @@ -222,3 +222,31 @@ jobs: echo "Working tree dirty at end of job" exit 1 fi + + # The wallet-cli daemon e2e spawns the BUILT `mm` CLI and the native + # better-sqlite3 addon as real child processes, so it needs its dependency + # subtree built first and cannot run in the per-package `test-*` matrix above. + test-wallet-cli-e2e: + name: Test wallet-cli daemon e2e (${{ matrix.node-version }}) + runs-on: ubuntu-latest + needs: prepare + strategy: + matrix: + node-version: [20.x, 22.x, 24.x] + steps: + - name: Checkout and setup environment + uses: MetaMask/action-checkout-and-setup@v3 + with: + is-high-risk-environment: false + persist-credentials: false + node-version: ${{ matrix.node-version }} + - name: Build wallet-cli and its dependencies + run: yarn workspaces foreach --topological-dev --recursive --from '@metamask/wallet-cli' run build + - run: yarn workspace @metamask/wallet-cli run test:e2e + - name: Require clean working directory + shell: bash + run: | + if ! git diff --exit-code; then + echo "Working tree dirty at end of job" + exit 1 + fi diff --git a/eslint.config.mjs b/eslint.config.mjs index 3cab21fa9c7..c87252b6894 100644 --- a/eslint.config.mjs +++ b/eslint.config.mjs @@ -321,6 +321,7 @@ const config = createConfig([ { files: [ 'packages/wallet-cli/src/**/*.test.{js,ts}', + 'packages/wallet-cli/tests/**/*.{js,ts}', 'packages/platform-api-docs/**/*.{js,ts}', ], rules: { diff --git a/packages/wallet-cli/README.md b/packages/wallet-cli/README.md index 29ac82275b1..ef6c44b847e 100644 --- a/packages/wallet-cli/README.md +++ b/packages/wallet-cli/README.md @@ -55,6 +55,12 @@ Or invoke `prebuild-install` directly from the monorepo root (where `better-sqli cd node_modules/better-sqlite3 && node ../.bin/prebuild-install ``` +## Testing + +Unit tests run with `yarn workspace @metamask/wallet-cli test`. + +The subprocess end-to-end suite (in `tests/`) spawns the built `mm` CLI and the native `better-sqlite3` addon as real processes, so it is kept out of the unit run and its coverage gate. Build the workspace dependencies first (`yarn build` from the repo root), then run it with `yarn workspace @metamask/wallet-cli test:e2e`. + ## Contributing This package is part of a monorepo. Instructions for contributing can be found in the [monorepo README](https://github.com/MetaMask/core#readme). diff --git a/packages/wallet-cli/jest.config.e2e.js b/packages/wallet-cli/jest.config.e2e.js new file mode 100644 index 00000000000..aff6a9372c3 --- /dev/null +++ b/packages/wallet-cli/jest.config.e2e.js @@ -0,0 +1,30 @@ +/* + * Jest configuration for the subprocess e2e suite in `tests/`. + * + * Kept separate from `jest.config.js` because these suites spawn the BUILT `mm` + * CLI and the native `better-sqlite3` addon as real child processes: they must + * stay out of the fast unit `test` run and must not be held to that run's + * 100%-coverage gate (subprocess work is invisible to in-process coverage). + * Run it with `yarn test:e2e`. + */ + +const merge = require('deepmerge'); + +const baseConfig = require('../../jest.config.packages'); + +module.exports = merge(baseConfig, { + displayName: 'wallet-cli:e2e', + + // Every test under `tests/` is a subprocess e2e; the default config runs + // everything in `src/`. + roots: ['/tests'], + + // Coverage is meaningless here — the work happens in spawned processes — so + // collecting it would only report the e2e harness as uncovered source. + collectCoverage: false, + + // The CLI runs in a normal Node process with the Web Crypto globals, so this + // suite needs neither the `jest.environment.js` polyfill nor any coverage + // threshold. + testEnvironment: 'node', +}); diff --git a/packages/wallet-cli/jest.config.js b/packages/wallet-cli/jest.config.js index bc5f44e07f4..568fedaec47 100644 --- a/packages/wallet-cli/jest.config.js +++ b/packages/wallet-cli/jest.config.js @@ -24,6 +24,11 @@ module.exports = merge(baseConfig, { // production code's test infrastructure, not production code itself. coveragePathIgnorePatterns: ['.*/src/test/.*'], + // The subprocess e2e suite lives in `tests/` and has its own config + // (`jest.config.e2e.js`, run via `yarn test:e2e`); it spawns the built CLI + // and must not run in the fast unit suite. + testPathIgnorePatterns: ['/node_modules/', '/tests/'], + // An object that configures minimum threshold enforcement for coverage results coverageThreshold: { global: { diff --git a/packages/wallet-cli/package.json b/packages/wallet-cli/package.json index aaa4f06e1c6..a1123ccb00d 100644 --- a/packages/wallet-cli/package.json +++ b/packages/wallet-cli/package.json @@ -39,6 +39,7 @@ "test:prepare": "./scripts/install-binaries.sh", "test": "yarn test:prepare && NODE_OPTIONS=--experimental-vm-modules jest --reporters=jest-silent-reporter", "test:clean": "NODE_OPTIONS=--experimental-vm-modules jest --clearCache", + "test:e2e": "yarn test:prepare && NODE_OPTIONS=--experimental-vm-modules jest --config jest.config.e2e.js", "test:verbose": "NODE_OPTIONS=--experimental-vm-modules jest --verbose", "test:watch": "NODE_OPTIONS=--experimental-vm-modules jest --watch" }, diff --git a/packages/wallet-cli/src/daemon/daemon-spawn.test.ts b/packages/wallet-cli/src/daemon/daemon-spawn.test.ts index 353dbf20328..7d884b7f2ee 100644 --- a/packages/wallet-cli/src/daemon/daemon-spawn.test.ts +++ b/packages/wallet-cli/src/daemon/daemon-spawn.test.ts @@ -1,21 +1,31 @@ import { spawn } from 'node:child_process'; -import { existsSync } from 'node:fs'; +import type { ChildProcess } from 'node:child_process'; +import { closeSync, existsSync, openSync } from 'node:fs'; import { pingDaemon } from './daemon-client'; import { ensureDaemon } from './daemon-spawn'; +import { ensureOwnerOnlyDirectory } from './data-dir'; import { getDaemonPaths } from './paths'; import type { DaemonSpawnConfig } from './types'; jest.mock('node:child_process'); jest.mock('node:fs'); jest.mock('./daemon-client'); +jest.mock('./data-dir'); jest.mock('./paths'); const mockSpawn = jest.mocked(spawn); const mockExistsSync = jest.mocked(existsSync); +const mockOpenSync = jest.mocked(openSync); +const mockCloseSync = jest.mocked(closeSync); +const mockEnsureOwnerOnlyDirectory = jest.mocked(ensureOwnerOnlyDirectory); const mockPingDaemon = jest.mocked(pingDaemon); const mockGetDaemonPaths = jest.mocked(getDaemonPaths); +// Arbitrary file descriptor handed back by the mocked `openSync` so tests can +// assert it is wired into the child's stdio and later closed in the parent. +const LOG_FILE_DESCRIPTOR = 7; + const CONFIG: DaemonSpawnConfig = { dataDir: '/tmp/data', infuraProjectId: 'test-key', @@ -65,7 +75,7 @@ function setupSpawnMock(): SpawnMock { listeners.get('exit')?.(code, signal); }, }; - mockSpawn.mockReturnValue(result as never); + mockSpawn.mockReturnValue(result as unknown as ChildProcess); return result; } @@ -79,6 +89,8 @@ describe('ensureDaemon', () => { logPath: '/tmp/test.log', dbPath: '/tmp/wallet.db', }); + mockOpenSync.mockReturnValue(LOG_FILE_DESCRIPTOR); + mockEnsureOwnerOnlyDirectory.mockResolvedValue(undefined); setupSpawnMock(); }); @@ -128,7 +140,7 @@ describe('ensureDaemon', () => { ['/pkg/dist/daemon/daemon-entry.mjs'], expect.objectContaining({ detached: true, - stdio: 'ignore', + stdio: ['ignore', 'ignore', LOG_FILE_DESCRIPTOR], env: expect.objectContaining({ MM_DAEMON_DATA_DIR: '/tmp/data', MM_DAEMON_SOCKET_PATH: '/tmp/test.sock', @@ -141,6 +153,57 @@ describe('ensureDaemon', () => { ); }); + it('redirects the daemon stderr to its log file and closes the parent file descriptor', async () => { + mockPingDaemon + .mockResolvedValueOnce(ABSENT) + .mockResolvedValueOnce(RESPONSIVE); + mockExistsSync.mockReturnValue(true); + + await ensureDaemon(CONFIG); + + expect(mockOpenSync).toHaveBeenCalledWith('/tmp/test.log', 'a'); + const spawnOptions = mockSpawn.mock.calls[0][2] as { stdio: unknown }; + expect(spawnOptions.stdio).toStrictEqual([ + 'ignore', + 'ignore', + LOG_FILE_DESCRIPTOR, + ]); + expect(mockCloseSync).toHaveBeenCalledWith(LOG_FILE_DESCRIPTOR); + }); + + it('propagates a log-file open failure without spawning', async () => { + mockPingDaemon.mockResolvedValue(ABSENT); + mockExistsSync.mockReturnValue(true); + mockOpenSync.mockImplementation(() => { + throw Object.assign(new Error('EACCES'), { code: 'EACCES' }); + }); + + await expect(ensureDaemon(CONFIG)).rejects.toThrow('EACCES'); + expect(mockSpawn).not.toHaveBeenCalled(); + }); + + it('creates the data directory before opening the log file', async () => { + mockPingDaemon + .mockResolvedValueOnce(ABSENT) + .mockResolvedValueOnce(RESPONSIVE); + mockExistsSync.mockReturnValue(true); + // The log lives inside the data dir, so the dir must be created first (else + // openSync ENOENTs on a fresh dir). + const order: string[] = []; + mockEnsureOwnerOnlyDirectory.mockImplementation(async () => { + order.push('ensureDir'); + }); + mockOpenSync.mockImplementation(() => { + order.push('openLog'); + return LOG_FILE_DESCRIPTOR; + }); + + await ensureDaemon(CONFIG); + + expect(mockEnsureOwnerOnlyDirectory).toHaveBeenCalledWith('/tmp/data'); + expect(order).toStrictEqual(['ensureDir', 'openLog']); + }); + it('returns started when the spawned daemon becomes responsive', async () => { mockPingDaemon .mockResolvedValueOnce(ABSENT) @@ -226,7 +289,10 @@ describe('ensureDaemon', () => { } }, ); - mockSpawn.mockReturnValue({ unref: jest.fn(), on } as never); + mockSpawn.mockReturnValue({ + unref: jest.fn(), + on, + } as unknown as ChildProcess); jest.useFakeTimers(); const promise = ensureDaemon(CONFIG); @@ -267,7 +333,10 @@ describe('ensureDaemon', () => { } }, ); - mockSpawn.mockReturnValue({ unref: jest.fn(), on } as never); + mockSpawn.mockReturnValue({ + unref: jest.fn(), + on, + } as unknown as ChildProcess); jest.useFakeTimers(); const promise = ensureDaemon(CONFIG); @@ -283,6 +352,35 @@ describe('ensureDaemon', () => { expect((thrownError as Error).message).toContain('/tmp/test.log'); }); + it('reports the spawn error when the child both errors and exits', async () => { + mockPingDaemon.mockResolvedValue(ABSENT); + mockExistsSync.mockReturnValue(true); + const on = jest.fn( + (event: string, handler: (...args: unknown[]) => void) => { + if (event === 'error') { + handler(new Error('spawn ENOENT')); + } + if (event === 'exit') { + handler(1, null); + } + }, + ); + mockSpawn.mockReturnValue({ + unref: jest.fn(), + on, + } as unknown as ChildProcess); + + jest.useFakeTimers(); + const promise = ensureDaemon(CONFIG); + const rejection = promise.catch((thrown: unknown) => thrown); + await jest.advanceTimersByTimeAsync(200); + + const thrownError = await rejection; + expect((thrownError as Error).message).toContain( + 'Failed to spawn daemon process', + ); + }); + it('writes spawn errors to stderr', async () => { mockPingDaemon .mockResolvedValueOnce(ABSENT) @@ -297,7 +395,10 @@ describe('ensureDaemon', () => { } }, ); - mockSpawn.mockReturnValue({ unref: jest.fn(), on } as never); + mockSpawn.mockReturnValue({ + unref: jest.fn(), + on, + } as unknown as ChildProcess); await ensureDaemon(CONFIG); errorHandler?.(new Error('spawn ENOENT')); diff --git a/packages/wallet-cli/src/daemon/daemon-spawn.ts b/packages/wallet-cli/src/daemon/daemon-spawn.ts index ff6f429d0ad..5f740943f17 100644 --- a/packages/wallet-cli/src/daemon/daemon-spawn.ts +++ b/packages/wallet-cli/src/daemon/daemon-spawn.ts @@ -1,8 +1,9 @@ import { spawn } from 'node:child_process'; -import { existsSync } from 'node:fs'; +import { closeSync, existsSync, openSync } from 'node:fs'; import { join } from 'node:path'; import { pingDaemon } from './daemon-client'; +import { ensureOwnerOnlyDirectory } from './data-dir'; import { getDaemonPaths } from './paths'; import type { DaemonSpawnConfig } from './types'; @@ -38,34 +39,52 @@ export type EnsureDaemonResult = { export async function ensureDaemon( config: DaemonSpawnConfig, ): Promise { - const { socketPath } = getDaemonPaths(config.dataDir); + const { socketPath, logPath } = getDaemonPaths(config.dataDir); const initialPing = await pingDaemon(socketPath); - if (initialPing.status === 'responsive') { - return { state: 'already-running', socketPath }; - } - if (initialPing.status === 'unreachable') { - if (initialPing.reason === 'permission') { + switch (initialPing.status) { + case 'responsive': + return { state: 'already-running', socketPath }; + case 'unreachable': + if (initialPing.reason === 'permission') { + throw new Error( + `Refusing to start: the socket at ${socketPath} is owned by another user. ` + + `Choose a different data directory (MM_DAEMON_DATA_DIR) or remove the socket manually. ` + + `(${initialPing.error.message})`, + ); + } throw new Error( - `Refusing to start: the socket at ${socketPath} is owned by another user. ` + - `Choose a different data directory (MM_DAEMON_DATA_DIR) or remove the socket manually. ` + + `Refusing to start: a daemon socket already exists at ${socketPath} but is unresponsive. ` + + `Run \`mm daemon stop\` (or \`mm daemon purge\`) before starting a new daemon. ` + `(${initialPing.error.message})`, ); + case 'absent': + break; + /* istanbul ignore next -- exhaustiveness guard; unreachable for the current PingResult union */ + default: { + const exhaustiveCheck: never = initialPing; + throw new Error( + `Unexpected daemon ping status: ${String(exhaustiveCheck)}`, + ); } - throw new Error( - `Refusing to start: a daemon socket already exists at ${socketPath} but is unresponsive. ` + - `Run \`mm daemon stop\` (or \`mm daemon purge\`) before starting a new daemon. ` + - `(${initialPing.error.message})`, - ); } process.stderr.write('Starting daemon...\n'); const { entryPath, args } = resolveEntryPoint(config.packageRoot); + // Create the data directory before opening the log file inside it. The daemon + // entry also does this, but only after spawn — opening the log first would + // ENOENT on a fresh data directory. + await ensureOwnerOnlyDirectory(config.dataDir); + + // Redirect the detached daemon's stderr to its log file rather than + // discarding it, so a crash after startup stays diagnosable. stdout stays + // ignored — structured status goes through the file logger. + const logFileDescriptor = openSync(logPath, 'a'); const child = spawn(process.execPath, [...args, entryPath], { detached: true, - stdio: 'ignore', + stdio: ['ignore', 'ignore', logFileDescriptor], env: { ...process.env, MM_DAEMON_DATA_DIR: config.dataDir, @@ -75,36 +94,45 @@ export async function ensureDaemon( MM_WALLET_SRP: config.srp, }, }); + // The child dup'd the file descriptor into its stderr, so drop the parent's + // copy. Safe on the success path: `spawn` reports failures via the 'error' + // event, not a synchronous throw. + closeSync(logFileDescriptor); + + type StartupOutcome = + | { kind: 'pending' } + | { kind: 'error'; error: Error } + | { kind: 'exited'; code: number | null; signal: NodeJS.Signals | null }; - type ExitInfo = { code: number | null; signal: NodeJS.Signals | null }; - const exitInfo: { value: ExitInfo | null } = { value: null }; // A failed spawn (bad interpreter, EACCES, ENOENT) emits 'error' and may - // never emit 'exit'. Capture it so the readiness loop can surface the real - // cause immediately instead of hanging for the full timeout. - const spawnError: { value: Error | null } = { value: null }; + // never emit 'exit', so 'error' is recorded first and not overwritten by a + // later 'exit' — the loop surfaces the real cause instead of hanging. + const outcome: { current: StartupOutcome } = { current: { kind: 'pending' } }; child.on('error', (error: Error) => { process.stderr.write(`Failed to spawn daemon process: ${String(error)}\n`); - spawnError.value = error; + outcome.current = { kind: 'error', error }; }); child.on('exit', (code, signal) => { - exitInfo.value = { code, signal }; + if (outcome.current.kind === 'pending') { + outcome.current = { kind: 'exited', code, signal }; + } }); child.unref(); for (let i = 0; i < MAX_POLLS; i++) { await new Promise((resolve) => setTimeout(resolve, POLL_INTERVAL_MS)); - if (spawnError.value !== null) { + const settled = outcome.current; + if (settled.kind === 'error') { throw new Error( - `Failed to spawn daemon process: ${spawnError.value.message}. ` + - `Check the daemon log at ${getDaemonPaths(config.dataDir).logPath}.`, + `Failed to spawn daemon process: ${settled.error.message}. ` + + `Check the daemon log at ${logPath}.`, ); } - if (exitInfo.value !== null) { - const { code, signal } = exitInfo.value; + if (settled.kind === 'exited') { throw new Error( - `Daemon process exited during startup (code=${String(code)}, signal=${String(signal)}). ` + - `Check the daemon log at ${getDaemonPaths(config.dataDir).logPath}.`, + `Daemon process exited during startup (code=${String(settled.code)}, signal=${String(settled.signal)}). ` + + `Check the daemon log at ${logPath}.`, ); } const ping = await pingDaemon(socketPath); diff --git a/packages/wallet-cli/src/daemon/socket-line.test.ts b/packages/wallet-cli/src/daemon/socket-line.test.ts index 0fc460f765a..acb02e4a234 100644 --- a/packages/wallet-cli/src/daemon/socket-line.test.ts +++ b/packages/wallet-cli/src/daemon/socket-line.test.ts @@ -38,6 +38,23 @@ describe('writeLine', () => { await expect(writeLine(socket, 'hello')).rejects.toThrow('write failed'); }); + it('keeps an error listener for the trailing event a failed write also emits', async () => { + const socket = createMockSocket(); + const writeError = Object.assign(new Error('EPIPE'), { code: 'EPIPE' }); + (socket.write as jest.Mock).mockImplementation( + (_data: string, callback: (e?: Error) => void) => callback(writeError), + ); + + await expect(writeLine(socket, 'hello')).rejects.toThrow('EPIPE'); + + // A failed write also emits a trailing 'error' event; a listener must + // survive to handle it, or Node crashes with "Unhandled 'error' event". + expect(socket.listenerCount('error')).toBe(1); + expect(() => socket.emit('error', writeError)).not.toThrow(); + // Once it fires, the listener detaches itself. + expect(socket.listenerCount('error')).toBe(0); + }); + it('rejects when the socket emits an error before the write completes', async () => { const socket = createMockSocket(); // Never invoke the write callback; the failure arrives via the 'error' event. @@ -106,6 +123,20 @@ describe('readLine', () => { ); }); + it('settles once and cleans up when end is followed by close', async () => { + const socket = createMockSocket(); + const promise = readLine(socket); + + socket.emit('end'); + socket.emit('close'); + + await expect(promise).rejects.toThrow( + 'Socket closed before response received', + ); + expect(socket.listenerCount('end')).toBe(0); + expect(socket.listenerCount('close')).toBe(0); + }); + it('rejects after timeout when no complete line received', async () => { jest.useFakeTimers(); const socket = createMockSocket(); diff --git a/packages/wallet-cli/src/daemon/socket-line.ts b/packages/wallet-cli/src/daemon/socket-line.ts index 6959a792b76..e4c00505890 100644 --- a/packages/wallet-cli/src/daemon/socket-line.ts +++ b/packages/wallet-cli/src/daemon/socket-line.ts @@ -18,12 +18,17 @@ export async function writeLine(socket: Socket, line: string): Promise { socket.once('error', onError); socket.write(`${line}\n`, (error) => { - socket.removeListener('error', onError); if (error) { + // A failed write (e.g. EPIPE) is delivered BOTH here AND as a later + // 'error' event. Leave `onError` attached to handle that event — + // detaching here would let Node treat it as unhandled and crash the + // process. This reject settles the promise; `onError` detaches itself + // when/if the event arrives. reject(error); - } else { - resolve(); + return; } + socket.removeListener('error', onError); + resolve(); }); }); } diff --git a/packages/wallet-cli/src/daemon/stop-daemon.test.ts b/packages/wallet-cli/src/daemon/stop-daemon.test.ts index 5c45f09c2bb..daaa96cd5cf 100644 --- a/packages/wallet-cli/src/daemon/stop-daemon.test.ts +++ b/packages/wallet-cli/src/daemon/stop-daemon.test.ts @@ -50,6 +50,39 @@ describe('stopDaemon', () => { expect(mockSendSignal).not.toHaveBeenCalled(); }); + it('cleans up a stale socket and PID file when connections are refused and the process is dead', async () => { + mockReadPidFile.mockResolvedValue(123); + mockPingDaemon.mockResolvedValue(UNREACHABLE); + mockIsProcessAlive.mockReturnValue(false); + + const result = await stopDaemon('/tmp/test.sock', '/tmp/test.pid'); + + // A crashed daemon leaves a refused socket and PID file behind; the daemon + // is gone, so report success and clear both. + expect(result).toBe(true); + expect(mockRm).toHaveBeenCalledWith('/tmp/test.pid', { force: true }); + expect(mockRm).toHaveBeenCalledWith('/tmp/test.sock', { force: true }); + // The recorded PID is dead, so never signal it — it may have been recycled. + expect(mockSendSignal).not.toHaveBeenCalled(); + expect(mockSendCommand).not.toHaveBeenCalled(); + }); + + it('does not delete the socket or report success when the socket is unreachable for a non-refused reason and the process is dead', async () => { + mockReadPidFile.mockResolvedValue(123); + mockPingDaemon.mockResolvedValue({ + status: 'unreachable', + reason: 'permission', + error: new Error('EACCES'), + }); + mockIsProcessAlive.mockReturnValue(false); + + const result = await stopDaemon('/tmp/test.sock', '/tmp/test.pid'); + + expect(result).toBe(false); + expect(mockRm).not.toHaveBeenCalled(); + expect(mockSendSignal).not.toHaveBeenCalled(); + }); + it('signals the recorded PID when the socket is absent but the process is still alive', async () => { mockReadPidFile.mockResolvedValue(123); mockPingDaemon.mockResolvedValue(ABSENT); @@ -116,6 +149,7 @@ describe('stopDaemon', () => { expect(result).toBe(true); expect(mockSendSignal).toHaveBeenCalledWith(123, 'SIGTERM'); + expect(mockRm).toHaveBeenCalledWith('/tmp/test.sock', { force: true }); }); it('falls through to SIGTERM when graceful shutdown times out', async () => { @@ -170,6 +204,7 @@ describe('stopDaemon', () => { const result = await stopDaemon('/tmp/test.sock', '/tmp/test.pid'); expect(result).toBe(true); expect(mockSendSignal).toHaveBeenCalledWith(123, 'SIGKILL'); + expect(mockRm).toHaveBeenCalledWith('/tmp/test.sock', { force: true }); }); it('returns false when all strategies fail', async () => { diff --git a/packages/wallet-cli/src/daemon/stop-daemon.ts b/packages/wallet-cli/src/daemon/stop-daemon.ts index f62e8d77744..3ffcdde8392 100644 --- a/packages/wallet-cli/src/daemon/stop-daemon.ts +++ b/packages/wallet-cli/src/daemon/stop-daemon.ts @@ -4,15 +4,25 @@ import { pingDaemon, sendCommand } from './daemon-client'; import { isProcessAlive, readPidFile, sendSignal, waitFor } from './utils'; /** - * Stop the daemon via a `shutdown` RPC call. Falls back to PID + SIGTERM if - * the socket is unresponsive, and escalates to SIGKILL if SIGTERM is ignored. + * Stop the daemon, preferring a graceful shutdown. * - * Signals are sent when EITHER the socket was observed (`responsive` or - * `unreachable`) OR the recorded PID is still alive on its own. The - * socket-absent + alive-PID branch trades a small risk of signalling a + * Resolution order when a live daemon is present: + * 1. If the socket is responsive, request a graceful `shutdown` over it. + * 2. If the recorded PID is still alive, escalate to SIGTERM. + * 3. ...then SIGKILL. + * + * Signals (steps 2-3) are only ever sent against a PID that is observed alive. + * The socket-absent + alive-PID branch trades a small risk of signalling a * recycled PID for the larger risk of leaving an orphan daemon holding the - * SQLite database — which `daemon purge` would otherwise wipe out from - * under it. + * SQLite database — which `daemon purge` would otherwise wipe out from under + * it. + * + * When the socket is NOT responsive AND the recorded PID is dead (or there is + * no PID file), there is no live daemon: a lingering socket or PID file is + * stale leftovers from a daemon that already exited — typically one that + * crashed without running its own cleanup. Those files are removed and the + * stop is reported as successful, rather than failing on a daemon that is + * already gone. * * @param socketPath - The daemon socket path. * @param pidPath - The daemon PID file path. @@ -30,9 +40,15 @@ export async function stopDaemon( ping.status === 'responsive' || ping.status === 'unreachable'; const processAlive = pid !== undefined && isProcessAlive(pid); - if (!socketObserved && !processAlive) { - // No live daemon evidence. Just remove the stale PID file if any. + // Only `absent` and `refused` prove no live daemon; `permission`/`timeout`/ + // `protocol` may be a wedged or foreign daemon, so those fall through. + const socketProvenGone = + ping.status === 'absent' || + (ping.status === 'unreachable' && ping.reason === 'refused'); + + if (socketProvenGone && !processAlive) { await cleanupFile(pidPath, 'PID file', log); + await cleanupFile(socketPath, 'socket file', log); return true; } diff --git a/packages/wallet-cli/src/daemon/wallet-factory.e2e.test.ts b/packages/wallet-cli/src/daemon/wallet-factory-integration.test.ts similarity index 100% rename from packages/wallet-cli/src/daemon/wallet-factory.e2e.test.ts rename to packages/wallet-cli/src/daemon/wallet-factory-integration.test.ts diff --git a/packages/wallet-cli/tests/lifecycle.e2e.test.ts b/packages/wallet-cli/tests/lifecycle.e2e.test.ts new file mode 100644 index 00000000000..9db668dfc7c --- /dev/null +++ b/packages/wallet-cli/tests/lifecycle.e2e.test.ts @@ -0,0 +1,267 @@ +import { spawn } from 'node:child_process'; +import { mkdtemp, readFile, rm, stat } from 'node:fs/promises'; +import { tmpdir } from 'node:os'; +import { join } from 'node:path'; + +import { getDaemonPaths } from '../src/daemon/paths'; +import { isProcessAlive, readPidFile } from '../src/daemon/utils'; + +// Subprocess-level lifecycle test for the `mm daemon` command suite. Unlike the +// in-process suites (`socket-integration.test.ts` exercises the transport in +// the test realm; `wallet-factory-integration.test.ts` constructs a real `Wallet` +// in-process), this spawns the BUILT `mm` CLI as a child process against a temp +// data directory and drives the real `start → call → status/stop/purge` +// lifecycle over the Unix socket. It needs `dist/` and the native +// `better-sqlite3` addon, so it runs only via `yarn test:e2e` (its own jest +// config), excluded from the fast unit `test` run and its 100%-coverage gate. +// +// Offline-safe: the daemon's startup neither fetches feature flags +// (RemoteFeatureFlagController only fetches in `updateRemoteFeatureFlags`) nor +// looks up the network (NetworkController's `init` is synchronous), and the +// only action called here, `KeyringController:getState`, is local. + +// A valid 12-word BIP-39 mnemonic — the same fixtures the in-process e2e uses. +const TEST_SRP = 'test test test test test test test test test test test ball'; +const TEST_PASSWORD = 'testpass'; +// NetworkController requires a project ID but is never reached over the network +// here, so any well-formed-looking value works. +const TEST_INFURA_PROJECT_ID = '00000000000000000000000000000000'; + +const ADDRESS_REGEX = /^0x[0-9a-fA-F]{40}$/u; + +const BIN_PATH = join(__dirname, '..', 'bin', 'run.mjs'); + +// Each step (spawn the CLI, construct a real Wallet, run PBKDF2 key derivation +// for the first-run SRP import) is slow; give the whole lifecycle room. +const STEP_TIMEOUT_MS = 60_000; + +type RunResult = { code: number | null; stdout: string; stderr: string }; + +/** + * Run the built `mm` CLI as a child process and capture its output. + * + * `NODE_OPTIONS` is stripped from the child environment so the parent jest + * run's `--experimental-vm-modules` flag does not leak an ExperimentalWarning + * onto the CLI's stderr. + * + * @param args - CLI arguments (e.g. `['daemon', 'start']`). + * @param dataDir - Data directory to point the CLI at (via `MM_DATA_DIR`). + * @returns The exit code and captured stdout/stderr. + */ +async function runMm(args: string[], dataDir: string): Promise { + const env = { ...process.env }; + delete env.NODE_OPTIONS; + + return new Promise((resolve, reject) => { + const child = spawn(process.execPath, [BIN_PATH, ...args], { + env: { + ...env, + MM_DATA_DIR: dataDir, + INFURA_PROJECT_ID: TEST_INFURA_PROJECT_ID, + MM_WALLET_PASSWORD: TEST_PASSWORD, + MM_WALLET_SRP: TEST_SRP, + }, + stdio: ['ignore', 'pipe', 'pipe'], + }); + + let stdout = ''; + let stderr = ''; + child.stdout.on('data', (chunk) => (stdout += chunk.toString())); + child.stderr.on('data', (chunk) => (stderr += chunk.toString())); + child.on('error', reject); + child.on('close', (code) => resolve({ code, stdout, stderr })); + }); +} + +/** + * Assert that a CLI invocation exited 0. + * + * On failure this throws an error embedding the captured stdout/stderr AND the + * daemon's own log file. + * + * @param step - Human-readable label for the CLI step (e.g. `daemon stop`). + * @param result - The captured run result. + * @param dataDir - Data directory the daemon is using (to locate its log). + */ +async function expectSuccessfulRun( + step: string, + result: RunResult, + dataDir: string, +): Promise { + if (result.code === 0) { + return; + } + const { logPath } = getDaemonPaths(dataDir); + const daemonLog = await readFile(logPath, 'utf-8').catch( + (error: unknown) => ``, + ); + throw new Error( + `Expected \`mm ${step}\` to exit 0 but it exited ${String(result.code)}.\n` + + `=== stdout ===\n${result.stdout}\n` + + `=== stderr ===\n${result.stderr}\n` + + `=== ${logPath} ===\n${daemonLog}\n`, + ); +} + +/** + * Call a messenger action on the running daemon and parse its JSON result. + * + * @param action - The messenger action name. + * @param dataDir - Data directory the daemon is using. + * @returns The parsed result object. + */ +async function callAction( + action: string, + dataDir: string, +): Promise> { + const result = await runMm(['daemon', 'call', action], dataDir); + await expectSuccessfulRun(`daemon call ${action}`, result, dataDir); + return JSON.parse(result.stdout.trim()); +} + +/** + * Whether a path exists, without throwing on absence. + * + * @param path - The path to check. + * @returns True if `stat` succeeds. + */ +async function exists(path: string): Promise { + try { + await stat(path); + return true; + } catch { + return false; + } +} + +/** + * Guarantee no daemon is left running and remove the temp data directory, + * regardless of how a test ended. Kills by the recorded PID directly (rather + * than going through `mm daemon stop`) so a wedged daemon cannot block cleanup. + * + * @param dataDir - The temp data directory to tear down. + */ +async function cleanup(dataDir: string): Promise { + const { pidPath } = getDaemonPaths(dataDir); + const pid = await readPidFile(pidPath).catch(() => undefined); + if (pid !== undefined && isProcessAlive(pid)) { + try { + process.kill(pid, 'SIGKILL'); + } catch { + // Already gone — nothing to clean up. + } + } + await rm(dataDir, { recursive: true, force: true }); +} + +describe('mm daemon lifecycle (subprocess e2e)', () => { + let dataDir: string; + + beforeEach(async () => { + dataDir = await mkdtemp(join(tmpdir(), 'mm-e2e-')); + }); + + afterEach(async () => { + await cleanup(dataDir); + }); + + it( + 'starts, reports already-running, answers call & status, then stops', + async () => { + const start = await runMm(['daemon', 'start'], dataDir); + await expectSuccessfulRun('daemon start', start, dataDir); + expect(start.stdout).toMatch(/Daemon running\. Socket:/u); + + // A second start finds the responsive daemon and leaves it untouched. + const restart = await runMm(['daemon', 'start'], dataDir); + await expectSuccessfulRun('daemon start (restart)', restart, dataDir); + expect(restart.stdout).toMatch(/already running/iu); + + // First run imports the SRP, so the wallet is unlocked and exposes the + // derived account. + const keyringState = await callAction( + 'KeyringController:getState', + dataDir, + ); + expect(keyringState.isUnlocked).toBe(true); + const keyrings = keyringState.keyrings as { accounts: string[] }[]; + expect(keyrings[0]?.accounts[0]).toMatch(ADDRESS_REGEX); + + const status = await runMm(['daemon', 'status'], dataDir); + await expectSuccessfulRun('daemon status', status, dataDir); + expect(status.stdout).toMatch( + /Daemon is running\. PID: \d+, Uptime: \d+s/u, + ); + + // The socket holds an unlocked wallet and the data dir holds the vault, so + // both must be owner-only (the only access-control boundary). The low 3 + // octal digits of `mode` are the permission bits. + const paths = getDaemonPaths(dataDir); + const socketMode = (await stat(paths.socketPath)).mode + .toString(8) + .slice(-3); + const dirMode = (await stat(dataDir)).mode.toString(8).slice(-3); + expect(socketMode).toBe('600'); + expect(dirMode).toBe('700'); + + const stop = await runMm(['daemon', 'stop'], dataDir); + await expectSuccessfulRun('daemon stop', stop, dataDir); + + const statusAfterStop = await runMm(['daemon', 'status'], dataDir); + await expectSuccessfulRun( + 'daemon status (after stop)', + statusAfterStop, + dataDir, + ); + expect(statusAfterStop.stdout).toMatch(/not running/iu); + }, + STEP_TIMEOUT_MS, + ); + + it( + 'resumes the persisted vault across a restart instead of re-importing', + async () => { + await runMm(['daemon', 'start'], dataDir); + const firstRun = await callAction('KeyringController:getState', dataDir); + expect(firstRun.isUnlocked).toBe(true); + const firstKeyrings = firstRun.keyrings as { accounts: string[] }[]; + expect(firstKeyrings[0]?.accounts[0]).toMatch(ADDRESS_REGEX); + + await runMm(['daemon', 'stop'], dataDir); + + // The on-disk database survives the stop. + const { dbPath } = getDaemonPaths(dataDir); + expect(await exists(dbPath)).toBe(true); + + await runMm(['daemon', 'start'], dataDir); + + // On the second start the persisted vault is found, so first-run SRP + // import is skipped and the wallet resumes LOCKED — a re-import would + // have left it unlocked. This is the observable signature of the + // `hasPersistedKeyring` resume path. + const resumed = await callAction('KeyringController:getState', dataDir); + expect(resumed.isUnlocked).toBe(false); + expect(typeof resumed.vault).toBe('string'); + + await runMm(['daemon', 'stop'], dataDir); + }, + STEP_TIMEOUT_MS, + ); + + it( + 'purges all daemon state', + async () => { + await runMm(['daemon', 'start'], dataDir); + + const purge = await runMm(['daemon', 'purge', '--force'], dataDir); + await expectSuccessfulRun('daemon purge --force', purge, dataDir); + expect(purge.stdout).toMatch(/All daemon state deleted/u); + + const paths = getDaemonPaths(dataDir); + expect(await exists(paths.dbPath)).toBe(false); + expect(await exists(paths.socketPath)).toBe(false); + expect(await exists(paths.pidPath)).toBe(false); + }, + STEP_TIMEOUT_MS, + ); +}); diff --git a/packages/wallet-cli/tsconfig.json b/packages/wallet-cli/tsconfig.json index 7a5498120f9..5f3efa9c6fe 100644 --- a/packages/wallet-cli/tsconfig.json +++ b/packages/wallet-cli/tsconfig.json @@ -1,9 +1,7 @@ { "extends": "../../tsconfig.packages.json", "compilerOptions": { - "baseUrl": "./", - "outDir": "./dist", - "rootDir": "./src" + "baseUrl": "./" }, "references": [ { "path": "../base-controller/tsconfig.json" }, @@ -11,5 +9,5 @@ { "path": "../storage-service/tsconfig.json" }, { "path": "../wallet/tsconfig.json" } ], - "include": ["../../types", "./bin", "./src"] + "include": ["../../types", "./bin", "./src", "./tests"] }