From 2eb87b3d5825dde04248b11e05cb5ed097bdd0aa Mon Sep 17 00:00:00 2001 From: Dimitris Marlagkoutsos Date: Tue, 19 May 2026 13:06:50 +0200 Subject: [PATCH] refactor(wallet-cli): Replace { wallet, store } with { wallet, dispose } MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `createWallet` now returns a `dispose` callback that owns the `wallet.destroy()` → `store.close()` ordering and reports per-step failures through the supplied logger. Both daemon-entry cleanup ladders (startup-failure catch and SIGTERM/SIGINT shutdown) call `dispose` instead of inlining the order, so a future third call site or extra teardown step (e.g. SQLite WAL flush) can't drift. The disposer is idempotent across concurrent and sequential calls, closes the store even when destroy rejects, and falls back to `console.error` if no logger (or a throwing one) is supplied. The pre-resolution catch block in `createWallet` was also tightened to route through the same reporter so a `store.close()` throw can't mask the original failure (e.g. bad SRP). Closes #8779 Co-Authored-By: Claude Opus 4.7 (1M context) --- .../src/daemon/daemon-entry.test.ts | 101 ++-------- .../wallet-cli/src/daemon/daemon-entry.ts | 34 +--- .../src/daemon/wallet-factory.test.ts | 174 ++++++++++++++++-- .../wallet-cli/src/daemon/wallet-factory.ts | 105 ++++++++++- 4 files changed, 272 insertions(+), 142 deletions(-) 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); +}