diff --git a/packages/wallet-cli/src/daemon/daemon-entry.test.ts b/packages/wallet-cli/src/daemon/daemon-entry.test.ts index 6b81d63da2..8fc4ad35f6 100644 --- a/packages/wallet-cli/src/daemon/daemon-entry.test.ts +++ b/packages/wallet-cli/src/daemon/daemon-entry.test.ts @@ -55,7 +55,7 @@ function enoent(): NodeJS.ErrnoException { } /** - * Create a mock createWallet result with a mocked wallet and store. + * Create a mock createWallet result with a mocked wallet and dispose callback. * * @returns A mock createWallet result. */ @@ -66,9 +66,7 @@ function createMockWallet(): MockCreateWalletResult { state: {}, destroy: jest.fn().mockResolvedValue(undefined), }, - store: { - close: jest.fn(), - }, + dispose: jest.fn().mockResolvedValue(undefined), } as unknown as MockCreateWalletResult; } @@ -306,7 +304,7 @@ describe('daemon-entry', () => { ); }); - it('cleans up wallet, store, and PID file when server fails to start', async () => { + it('disposes the wallet and removes the PID file when server fails to start', async () => { const result = createMockWallet(); mockCreateWallet.mockResolvedValue(result); mockStartRpcSocketServer.mockRejectedValue(new Error('server failed')); @@ -321,8 +319,7 @@ describe('daemon-entry', () => { await importDaemonEntry(); - expect(result.wallet.destroy).toHaveBeenCalled(); - expect(result.store.close).toHaveBeenCalled(); + expect(result.dispose).toHaveBeenCalledTimes(1); expect(mockRm).toHaveBeenCalledWith('/tmp/daemon.pid', { force: true }); expect(process.exitCode).toBe(1); }); @@ -431,37 +428,6 @@ describe('daemon-entry', () => { expect(process.exitCode).toBe(1); }); - it('still cleans up wallet/store when wallet.destroy fails during error cleanup', async () => { - const result = createMockWallet(); - (result.wallet.destroy as jest.Mock).mockRejectedValue( - new Error('destroy failed'), - ); - mockCreateWallet.mockResolvedValue(result); - mockStartRpcSocketServer.mockRejectedValue(new Error('server failed')); - - await importDaemonEntry(); - - expect(result.store.close).toHaveBeenCalled(); - expect(process.exitCode).toBe(1); - }); - - it('logs and continues when store.close throws during error cleanup', async () => { - const result = createMockWallet(); - (result.store.close as jest.Mock).mockImplementation(() => { - throw new Error('close failed'); - }); - mockCreateWallet.mockResolvedValue(result); - mockStartRpcSocketServer.mockRejectedValue(new Error('server failed')); - - await importDaemonEntry(); - - expect(mockAppendFile).toHaveBeenCalledWith( - '/tmp/daemon.log', - expect.stringContaining('store.close() failed during cleanup'), - ); - expect(process.exitCode).toBe(1); - }); - it('logs and continues when ownership-aware PID removal throws during error cleanup', async () => { const result = createMockWallet(); mockCreateWallet.mockResolvedValue(result); @@ -559,8 +525,7 @@ describe('daemon-entry', () => { } expect(handle.close).toHaveBeenCalled(); - expect(result.wallet.destroy).toHaveBeenCalled(); - expect(result.store.close).toHaveBeenCalled(); + expect(result.dispose).toHaveBeenCalledTimes(1); }); it('triggers shutdown when SIGINT handler is called', async () => { @@ -582,11 +547,10 @@ describe('daemon-entry', () => { } expect(handle.close).toHaveBeenCalled(); - expect(result.wallet.destroy).toHaveBeenCalled(); - expect(result.store.close).toHaveBeenCalled(); + expect(result.dispose).toHaveBeenCalledTimes(1); }); - it('shutdown still calls wallet.destroy when handle.close fails', async () => { + it('shutdown still disposes the wallet when handle.close fails', async () => { const result = createMockWallet(); mockCreateWallet.mockResolvedValue(result); const handle = createMockHandle(); @@ -599,55 +563,13 @@ describe('daemon-entry', () => { const onShutdown = callArgs.onShutdown as () => Promise; await onShutdown(); - expect(result.wallet.destroy).toHaveBeenCalled(); + expect(result.dispose).toHaveBeenCalledTimes(1); expect(mockAppendFile).toHaveBeenCalledWith( '/tmp/daemon.log', expect.stringContaining('handle.close() failed'), ); }); - it('shutdown logs wallet.destroy failure', async () => { - const result = createMockWallet(); - (result.wallet.destroy as jest.Mock).mockRejectedValue( - new Error('destroy failed'), - ); - mockCreateWallet.mockResolvedValue(result); - const handle = createMockHandle(); - mockStartRpcSocketServer.mockResolvedValue(handle); - - await importDaemonEntry(); - - const callArgs = mockStartRpcSocketServer.mock.calls[0][0]; - const onShutdown = callArgs.onShutdown as () => Promise; - await onShutdown(); - - expect(mockAppendFile).toHaveBeenCalledWith( - '/tmp/daemon.log', - expect.stringContaining('wallet.destroy() failed'), - ); - }); - - it('shutdown logs store.close failure', async () => { - const result = createMockWallet(); - (result.store.close as jest.Mock).mockImplementation(() => { - throw new Error('close failed'); - }); - mockCreateWallet.mockResolvedValue(result); - const handle = createMockHandle(); - mockStartRpcSocketServer.mockResolvedValue(handle); - - await importDaemonEntry(); - - const callArgs = mockStartRpcSocketServer.mock.calls[0][0]; - const onShutdown = callArgs.onShutdown as () => Promise; - await onShutdown(); - - expect(mockAppendFile).toHaveBeenCalledWith( - '/tmp/daemon.log', - expect.stringContaining('store.close() failed'), - ); - }); - it('handles rm rejection during shutdown cleanup gracefully', async () => { const result = createMockWallet(); mockCreateWallet.mockResolvedValue(result); @@ -675,7 +597,7 @@ describe('daemon-entry', () => { await onShutdown(); expect(handle.close).toHaveBeenCalled(); - expect(result.wallet.destroy).toHaveBeenCalled(); + expect(result.dispose).toHaveBeenCalledTimes(1); expect(mockAppendFile).toHaveBeenCalledWith( '/tmp/daemon.log', expect.stringContaining('Failed to remove socket file'), @@ -692,7 +614,7 @@ describe('daemon-entry', () => { expect(process.exitCode).toBe(1); }); - it('onShutdown closes server and destroys wallet', async () => { + it('onShutdown closes server and disposes the wallet', async () => { const result = createMockWallet(); mockCreateWallet.mockResolvedValue(result); const handle = createMockHandle(); @@ -713,8 +635,7 @@ describe('daemon-entry', () => { await onShutdown(); expect(handle.close).toHaveBeenCalled(); - expect(result.wallet.destroy).toHaveBeenCalled(); - expect(result.store.close).toHaveBeenCalled(); + expect(result.dispose).toHaveBeenCalledTimes(1); expect(mockRm).toHaveBeenCalledWith('/tmp/daemon.pid', { force: true }); }); diff --git a/packages/wallet-cli/src/daemon/daemon-entry.ts b/packages/wallet-cli/src/daemon/daemon-entry.ts index 8a27f7df40..87adf2249c 100644 --- a/packages/wallet-cli/src/daemon/daemon-entry.ts +++ b/packages/wallet-cli/src/daemon/daemon-entry.ts @@ -3,7 +3,6 @@ import type { Wallet } from '@metamask/wallet'; import { mkdirSync } from 'node:fs'; import { appendFile, chmod, readFile, rm, writeFile } from 'node:fs/promises'; -import type { KeyValueStore } from '../persistence/KeyValueStore'; import { pingDaemon } from './daemon-client'; import { getDaemonPaths } from './paths'; import { startRpcSocketServer } from './rpc-socket-server'; @@ -88,11 +87,11 @@ async function main(): Promise { } let wallet: Wallet | undefined; - let store: KeyValueStore | undefined; + let dispose: (() => Promise) | undefined; let handle: RpcSocketServerHandle | undefined; try { - ({ wallet, store } = await createWallet({ + ({ wallet, dispose } = await createWallet({ databasePath: dbPath, infuraProjectId, password, @@ -135,19 +134,8 @@ async function main(): Promise { // synchronously, so this runs before any client can connect. await chmod(socketPath, 0o600); } catch (error) { - if (wallet) { - try { - await wallet.destroy(); - } catch (destroyError) { - log(`wallet.destroy() failed during cleanup: ${String(destroyError)}`); - } - } - if (store) { - try { - store.close(); - } catch (closeError) { - log(`store.close() failed during cleanup: ${String(closeError)}`); - } + if (dispose) { + await dispose(); } // Only remove the PID file if it's still ours (we may have lost the race // and the file now belongs to another daemon). @@ -162,8 +150,7 @@ async function main(): Promise { // Capture the now-resolved bindings so the shutdown closures below have // a stable, non-undefined reference (TS narrowing across closure escape). const activeHandle = handle; - const activeWallet = wallet; - const activeStore = store; + const activeDispose = dispose; log(`Daemon started. Socket: ${socketPath}`); @@ -184,16 +171,7 @@ async function main(): Promise { } catch (closeError) { log(`handle.close() failed: ${String(closeError)}`); } - try { - await activeWallet.destroy(); - } catch (destroyError) { - log(`wallet.destroy() failed: ${String(destroyError)}`); - } - try { - activeStore.close(); - } catch (closeError) { - log(`store.close() failed: ${String(closeError)}`); - } + await activeDispose(); await Promise.all([ removeOwnedPidFile(pidPath, pidFileContents).catch( (rmError: unknown) => { diff --git a/packages/wallet-cli/src/daemon/wallet-factory.test.ts b/packages/wallet-cli/src/daemon/wallet-factory.test.ts index f6f0269ade..31507b5996 100644 --- a/packages/wallet-cli/src/daemon/wallet-factory.test.ts +++ b/packages/wallet-cli/src/daemon/wallet-factory.test.ts @@ -108,11 +108,11 @@ describe('createWallet', () => { ); }); - it('returns the wallet and its backing KeyValueStore', async () => { - const { wallet, store } = await createWallet(CONFIG); + it('returns the wallet alongside a dispose callback', async () => { + const { wallet, dispose } = await createWallet(CONFIG); expect(wallet.messenger).toBe(mockMessenger); - expect(store).toBeInstanceOf(KeyValueStore); - store.close(); + expect(typeof dispose).toBe('function'); + await dispose(); }); it('hydrates the Wallet with state loaded from the store', async () => { @@ -134,7 +134,7 @@ describe('createWallet', () => { }, }); - const { store } = await createWallet(CONFIG); + const { dispose } = await createWallet(CONFIG); const args = MockWallet.mock.calls[0][0]; expect(args.state).toStrictEqual({ @@ -147,7 +147,7 @@ describe('createWallet', () => { }); loadStateSpy.mockRestore(); - store.close(); + await dispose(); }); it('subscribes the store to controller state changes', async () => { @@ -155,17 +155,17 @@ describe('createWallet', () => { .spyOn(persistenceModule, 'subscribeToChanges') .mockReturnValue(() => undefined); - const { wallet, store } = await createWallet(CONFIG); + const { wallet, dispose } = await createWallet(CONFIG); expect(subscribeSpy).toHaveBeenCalledWith( wallet.messenger, wallet.controllerMetadata, - store, + expect.any(KeyValueStore), undefined, ); subscribeSpy.mockRestore(); - store.close(); + await dispose(); }); it('forwards the supplied log callback to subscribeToChanges', async () => { @@ -174,7 +174,7 @@ describe('createWallet', () => { .mockReturnValue(() => undefined); const log = jest.fn(); - const { store } = await createWallet({ ...CONFIG, log }); + const { dispose } = await createWallet({ ...CONFIG, log }); expect(subscribeSpy).toHaveBeenCalledWith( expect.anything(), @@ -184,7 +184,7 @@ describe('createWallet', () => { ); subscribeSpy.mockRestore(); - store.close(); + await dispose(); }); it('skips importing the SRP when the store already contains a KeyringController vault', async () => { @@ -192,11 +192,11 @@ describe('createWallet', () => { KeyringController: { vault: 'encrypted-vault-blob' }, }); - const { store } = await createWallet(CONFIG); + const { dispose } = await createWallet(CONFIG); expect(mockImportSrp).not.toHaveBeenCalled(); - store.close(); + await dispose(); }); it('closes the store and rethrows when state hydration fails', async () => { @@ -251,10 +251,10 @@ describe('createWallet', () => { it('does not remove the database when SRP import succeeds on first run', async () => { const databasePath = tempDbPath('success'); - const { store } = await createWallet({ ...CONFIG, databasePath }); + const { dispose } = await createWallet({ ...CONFIG, databasePath }); expect(mockRm).not.toHaveBeenCalled(); - store.close(); + await dispose(); }); it('does not remove the database when failure occurs on a subsequent run', async () => { @@ -299,9 +299,29 @@ describe('createWallet', () => { destroy, }) as unknown as Wallet, ); + const log = jest.fn(); - await expect(createWallet(CONFIG)).rejects.toThrow(original); + await expect(createWallet({ ...CONFIG, log })).rejects.toThrow(original); expect(destroy).toHaveBeenCalledTimes(1); + expect(log).toHaveBeenCalledWith( + expect.stringContaining( + 'wallet.destroy() failed during first-run cleanup', + ), + ); + }); + + it('tolerates store.close throwing during cleanup and still rethrows the original error', async () => { + const original = new Error('bad SRP'); + mockImportSrp.mockRejectedValue(original); + jest.spyOn(KeyValueStore.prototype, 'close').mockImplementation(() => { + throw new Error('close failed'); + }); + const log = jest.fn(); + + await expect(createWallet({ ...CONFIG, log })).rejects.toThrow(original); + expect(log).toHaveBeenCalledWith( + expect.stringContaining('store.close() failed during first-run cleanup'), + ); }); it('destroys the wallet when subscribeToChanges throws', async () => { @@ -329,4 +349,126 @@ describe('createWallet', () => { await expect(createWallet(CONFIG)).rejects.toThrow(ctorError); expect(closeSpy).toHaveBeenCalled(); }); + + describe('dispose', () => { + it('destroys the wallet before closing the store', async () => { + const closeSpy = jest.spyOn(KeyValueStore.prototype, 'close'); + const { wallet, dispose } = await createWallet(CONFIG); + + const order: string[] = []; + (wallet.destroy as jest.Mock).mockImplementation(async () => { + order.push('destroy'); + }); + closeSpy.mockImplementation(() => { + order.push('close'); + }); + + await dispose(); + + expect(order).toStrictEqual(['destroy', 'close']); + }); + + it('coalesces concurrent calls onto a single teardown', async () => { + const { wallet, dispose } = await createWallet(CONFIG); + const closeSpy = jest.spyOn(KeyValueStore.prototype, 'close'); + + await Promise.all([dispose(), dispose(), dispose()]); + + expect(wallet.destroy).toHaveBeenCalledTimes(1); + expect(closeSpy).toHaveBeenCalledTimes(1); + }); + + it('is idempotent across sequential calls', async () => { + const { wallet, dispose } = await createWallet(CONFIG); + const closeSpy = jest.spyOn(KeyValueStore.prototype, 'close'); + + await dispose(); + await dispose(); + + expect(wallet.destroy).toHaveBeenCalledTimes(1); + expect(closeSpy).toHaveBeenCalledTimes(1); + }); + + it('still closes the store when wallet.destroy rejects', async () => { + jest.spyOn(console, 'error').mockImplementation(() => undefined); + const { wallet, dispose } = await createWallet(CONFIG); + const closeSpy = jest.spyOn(KeyValueStore.prototype, 'close'); + (wallet.destroy as jest.Mock).mockRejectedValue( + new Error('destroy failed'), + ); + + expect(await dispose()).toBeUndefined(); + + expect(closeSpy).toHaveBeenCalledTimes(1); + }); + + it('forwards destroy failures to the supplied log callback', async () => { + const log = jest.fn(); + const { wallet, dispose } = await createWallet({ ...CONFIG, log }); + (wallet.destroy as jest.Mock).mockRejectedValue( + new Error('destroy failed'), + ); + + await dispose(); + + expect(log).toHaveBeenCalledWith( + expect.stringContaining( + 'wallet.destroy() failed: Error: destroy failed', + ), + ); + }); + + it('forwards store.close failures to the supplied log callback', async () => { + const log = jest.fn(); + const { dispose } = await createWallet({ ...CONFIG, log }); + jest.spyOn(KeyValueStore.prototype, 'close').mockImplementation(() => { + throw new Error('close failed'); + }); + + await dispose(); + + expect(log).toHaveBeenCalledWith( + expect.stringContaining('store.close() failed: Error: close failed'), + ); + }); + + it('falls back to console.error when no log callback is supplied', async () => { + const consoleSpy = jest + .spyOn(console, 'error') + .mockImplementation(() => undefined); + const { wallet, dispose } = await createWallet(CONFIG); + (wallet.destroy as jest.Mock).mockRejectedValue( + new Error('destroy failed'), + ); + + await dispose(); + + expect(consoleSpy).toHaveBeenCalledWith( + expect.stringContaining('wallet.destroy() failed'), + ); + + consoleSpy.mockRestore(); + }); + + it('falls back to console.error when the supplied log callback throws', async () => { + const consoleSpy = jest + .spyOn(console, 'error') + .mockImplementation(() => undefined); + const log = jest.fn().mockImplementation(() => { + throw new Error('log explode'); + }); + const { wallet, dispose } = await createWallet({ ...CONFIG, log }); + (wallet.destroy as jest.Mock).mockRejectedValue( + new Error('destroy failed'), + ); + + expect(await dispose()).toBeUndefined(); + + expect(consoleSpy).toHaveBeenCalledWith( + expect.stringContaining('wallet.destroy() failed'), + ); + + consoleSpy.mockRestore(); + }); + }); }); diff --git a/packages/wallet-cli/src/daemon/wallet-factory.ts b/packages/wallet-cli/src/daemon/wallet-factory.ts index 08a95f9833..e5043de6e2 100644 --- a/packages/wallet-cli/src/daemon/wallet-factory.ts +++ b/packages/wallet-cli/src/daemon/wallet-factory.ts @@ -14,7 +14,14 @@ const IN_MEMORY_DATABASE_PATH = ':memory:'; export type CreateWalletResult = { wallet: Wallet; - store: KeyValueStore; + /** + * Tear down the wallet and its backing store in the required order + * (`wallet.destroy()` then `store.close()`). Idempotent — concurrent and + * repeat calls coalesce onto the same teardown promise. Per-step errors are + * forwarded to the `log` callback (or `console.error` if none was supplied), + * so a destroy failure does not prevent the store from being closed. + */ + dispose: () => Promise; }; /** @@ -42,10 +49,11 @@ export type CreateWalletResult = { * @param config.infuraProjectId - The Infura project ID for network access. * @param config.password - The wallet password. * @param config.srp - The secret recovery phrase (BIP-39 mnemonic). - * @param config.log - Optional logger for persistence-write failures. - * @returns The Wallet instance and the underlying KeyValueStore. The caller - * owns the store and must close it after destroying the wallet (closing - * first would cause in-flight persistence writes during teardown to fail). + * @param config.log - Optional logger for persistence-write failures and + * teardown errors surfaced through `dispose`. + * @returns The Wallet instance and a `dispose` callback that owns teardown. + * Call `dispose()` to release resources; it destroys the wallet before + * closing the store so in-flight persistence writes can finish. */ export async function createWallet({ databasePath, @@ -96,12 +104,28 @@ export async function createWallet({ await importSecretRecoveryPhrase(wallet, password, srp); } - return { wallet, store }; + return { wallet, dispose: createDisposer(wallet, store, log) }; } catch (error) { if (wallet) { - await wallet.destroy().catch(() => undefined); + try { + await wallet.destroy(); + } catch (destroyError) { + report( + 'wallet.destroy() failed during first-run cleanup', + destroyError, + log, + ); + } + } + try { + store.close(); + } catch (closeError) { + report( + 'store.close() failed during first-run cleanup', + closeError, + log, + ); } - store.close(); if (wasFirstRun && databasePath !== IN_MEMORY_DATABASE_PATH) { // Best-effort cleanup of the on-disk SQLite files (main, WAL, SHM) so @@ -132,3 +156,68 @@ function hasPersistedKeyring( ): boolean { return typeof state.KeyringController?.vault === 'string'; } + +/** + * Build the single-owner teardown callback returned alongside the Wallet. + * + * Encodes the ordering invariant (`wallet.destroy()` before `store.close()`) + * so call sites can't reintroduce the wrong order. Idempotent via a cached + * promise — repeat or concurrent invocations resolve once teardown finishes. + * Per-step failures are reported but never thrown, so a destroy failure does + * not block the store close. + * + * @param wallet - The wallet to destroy first. + * @param store - The key-value store to close after destroy resolves. + * @param log - Optional logger; falls back to `console.error` when omitted. + * @returns An idempotent teardown callback. + */ +function createDisposer( + wallet: Wallet, + store: KeyValueStore, + log: ((message: string) => void) | undefined, +): () => Promise { + let pending: Promise | undefined; + return async () => { + pending ??= (async (): Promise => { + try { + await wallet.destroy(); + } catch (destroyError) { + report('wallet.destroy() failed', destroyError, log); + } + try { + store.close(); + } catch (closeError) { + report('store.close() failed', closeError, log); + } + })(); + return pending; + }; +} + +/** + * Forward a teardown error to the optional logger, falling back to + * `console.error` so a detached daemon's `stdio: 'ignore'` doesn't silently + * discard the diagnostic in development. A throwing `log` callback also + * falls back to `console.error`, so the disposer's "never throws" contract + * holds even if a consumer wires in a misbehaving logger. + * + * @param prefix - Short label identifying which step failed. + * @param error - The underlying failure. + * @param log - Optional logger callback. + */ +function report( + prefix: string, + error: unknown, + log: ((message: string) => void) | undefined, +): void { + const message = `${prefix}: ${String(error)}`; + if (log) { + try { + log(message); + return; + } catch { + // Fall through to console.error so a buggy logger can't break teardown. + } + } + console.error(message); +}