From 90bf89413dea677a460b4358a85ce58e3bc4c2c1 Mon Sep 17 00:00:00 2001 From: Tamas Date: Wed, 25 Feb 2026 09:12:03 +0100 Subject: [PATCH] fix: SessionStore race conditions and async initialization (WAPI-1118) Replace fire-and-forget garbage collection with an async factory method (SessionStore.create) that completes GC before the store is usable. Add mutex protection around master list read-modify-write operations to prevent concurrent session set/delete from corrupting the list. BREAKING: SessionStore constructor is now private. Use await SessionStore.create(kvstore) instead of new SessionStore(kvstore). --- .../src/end-to-end.integration.test.ts | 8 +-- apps/load-tests/src/client/session-pair.ts | 4 +- .../src/scenarios/async-delivery-helpers.ts | 6 +- apps/rn-demo/context/WalletContext.tsx | 2 +- apps/web-demo/src/components/BasicDemo.tsx | 4 +- .../src/components/MetaMaskMobileDemo.tsx | 2 +- apps/web-demo/src/components/TrustedDemo.tsx | 4 +- .../web-demo/src/components/UntrustedDemo.tsx | 4 +- packages/core/CHANGELOG.md | 2 + packages/core/package.json | 1 + .../core/src/base-client.integration.test.ts | 6 +- packages/core/src/session-store/index.test.ts | 60 +++++++++++++------ packages/core/src/session-store/index.ts | 35 +++++++---- .../src/client.integration.test.ts | 2 +- .../src/client.integration.test.ts | 4 +- yarn.lock | 10 ++++ 16 files changed, 102 insertions(+), 52 deletions(-) diff --git a/apps/integration-tests/src/end-to-end.integration.test.ts b/apps/integration-tests/src/end-to-end.integration.test.ts index 33dd482..eb98ba6 100644 --- a/apps/integration-tests/src/end-to-end.integration.test.ts +++ b/apps/integration-tests/src/end-to-end.integration.test.ts @@ -94,8 +94,8 @@ t.describe("E2E Integration Test", () => { t.beforeEach(async () => { dappKvStore = new InMemoryKVStore(); walletKvStore = new InMemoryKVStore(); - dappSessionStore = new SessionStore(dappKvStore); - walletSessionStore = new SessionStore(walletKvStore); + dappSessionStore = await SessionStore.create(dappKvStore); + walletSessionStore = await SessionStore.create(walletKvStore); const dappTransport = await WebSocketTransport.create({ url: RELAY_URL, kvstore: dappKvStore, websocket: WebSocket }); const walletTransport = await WebSocketTransport.create({ url: RELAY_URL, kvstore: walletKvStore, websocket: WebSocket }); @@ -251,8 +251,8 @@ t.describe("E2E Integration Test via Proxy", () => { dappKvStore = new InMemoryKVStore(); walletKvStore = new InMemoryKVStore(); - dappSessionStore = new SessionStore(dappKvStore); - walletSessionStore = new SessionStore(walletKvStore); + dappSessionStore = await SessionStore.create(dappKvStore); + walletSessionStore = await SessionStore.create(walletKvStore); // DApp connects directly, Wallet connects through proxy const dappTransport = await WebSocketTransport.create({ url: RELAY_URL, kvstore: dappKvStore, websocket: WebSocket }); diff --git a/apps/load-tests/src/client/session-pair.ts b/apps/load-tests/src/client/session-pair.ts index 3d441cf..dc6fe6a 100644 --- a/apps/load-tests/src/client/session-pair.ts +++ b/apps/load-tests/src/client/session-pair.ts @@ -93,8 +93,8 @@ export async function createSessionPair(options: CreateSessionPairOptions): Prom // Create isolated stores for each client const dappKvStore = new InMemoryKVStore(); const walletKvStore = new InMemoryKVStore(); - const dappSessionStore = new SessionStore(dappKvStore); - const walletSessionStore = new SessionStore(walletKvStore); + const dappSessionStore = await SessionStore.create(dappKvStore); + const walletSessionStore = await SessionStore.create(walletKvStore); // Use MockKeyManager - no real crypto (see key-manager.ts for rationale) const keyManager = new MockKeyManager(); diff --git a/apps/load-tests/src/scenarios/async-delivery-helpers.ts b/apps/load-tests/src/scenarios/async-delivery-helpers.ts index cdd1d74..ab69627 100644 --- a/apps/load-tests/src/scenarios/async-delivery-helpers.ts +++ b/apps/load-tests/src/scenarios/async-delivery-helpers.ts @@ -32,8 +32,8 @@ export interface AsyncDeliverySession { export async function setupAsyncDeliverySession(url: string, timeoutMs = 60000): Promise { const dappKvStore = new InMemoryKVStore(); const walletKvStore = new InMemoryKVStore(); - const dappSessionStore = new SessionStore(dappKvStore); - const walletSessionStore = new SessionStore(walletKvStore); + const dappSessionStore = await SessionStore.create(dappKvStore); + const walletSessionStore = await SessionStore.create(walletKvStore); const keyManager = new MockKeyManager(); let dappClient: DappClient | null = null; @@ -93,7 +93,7 @@ export async function setupAsyncDeliverySession(url: string, timeoutMs = 60000): export async function testAsyncRecovery(session: AsyncDeliverySession, timeoutMs = 30000): Promise { const { sessionId, url, dappClient, walletKvStore } = session; const keyManager = new MockKeyManager(); - const walletSessionStore = new SessionStore(walletKvStore); + const walletSessionStore = await SessionStore.create(walletKvStore); let walletClient: WalletClient | null = null; let timeoutHandle: ReturnType | null = null; diff --git a/apps/rn-demo/context/WalletContext.tsx b/apps/rn-demo/context/WalletContext.tsx index 7df8494..75990e0 100644 --- a/apps/rn-demo/context/WalletContext.tsx +++ b/apps/rn-demo/context/WalletContext.tsx @@ -62,7 +62,7 @@ export function WalletProvider({ children }: { children: ReactNode }) { setIsInitializing(true); const kvstore = new AsyncStorageKVStore("wallet-"); - const sessionstore = new SessionStore(kvstore); + const sessionstore = await SessionStore.create(kvstore); manager = new SessionManager(sessionstore, RELAY_URL); setSessionManager(manager); diff --git a/apps/web-demo/src/components/BasicDemo.tsx b/apps/web-demo/src/components/BasicDemo.tsx index 4ab562a..e3dffc2 100644 --- a/apps/web-demo/src/components/BasicDemo.tsx +++ b/apps/web-demo/src/components/BasicDemo.tsx @@ -27,8 +27,8 @@ export default function BasicDemo() { const dappKvStore = new LocalStorageKVStore("dapp-"); const walletKvStore = new LocalStorageKVStore("wallet-"); - const dappSessionStore = new SessionStore(dappKvStore); - const walletSessionStore = new SessionStore(walletKvStore); + const dappSessionStore = await SessionStore.create(dappKvStore); + const walletSessionStore = await SessionStore.create(walletKvStore); // Create transports (in a real app, these would be separate instances) const dappTransport = await WebSocketTransport.create({ diff --git a/apps/web-demo/src/components/MetaMaskMobileDemo.tsx b/apps/web-demo/src/components/MetaMaskMobileDemo.tsx index 77c8b2c..f2e6f40 100644 --- a/apps/web-demo/src/components/MetaMaskMobileDemo.tsx +++ b/apps/web-demo/src/components/MetaMaskMobileDemo.tsx @@ -178,7 +178,7 @@ export default function MetaMaskMobileDemo() { addDappLog("system", "Creating dApp client..."); const dappKvStore = new LocalStorageKVStore("metamask-mobile-demo-dapp/"); - const sessionStore = new SessionStore(dappKvStore); + const sessionStore = await SessionStore.create(dappKvStore); setDappSessionStore(sessionStore); const dappTransport = await WebSocketTransport.create({ diff --git a/apps/web-demo/src/components/TrustedDemo.tsx b/apps/web-demo/src/components/TrustedDemo.tsx index 00e68fd..3ce17fa 100644 --- a/apps/web-demo/src/components/TrustedDemo.tsx +++ b/apps/web-demo/src/components/TrustedDemo.tsx @@ -172,7 +172,7 @@ export default function TrustedDemo() { addDappLog("system", "Creating dApp client..."); const dappKvStore = new LocalStorageKVStore("trusted-demo-dapp-"); - const dappSessionStore = new SessionStore(dappKvStore); + const dappSessionStore = await SessionStore.create(dappKvStore); const dappTransport = await WebSocketTransport.create({ url: RELAY_URL, @@ -316,7 +316,7 @@ export default function TrustedDemo() { addWalletLog("system", "Creating wallet client..."); const walletKvStore = new LocalStorageKVStore("trusted-demo-wallet-"); - const walletSessionStore = new SessionStore(walletKvStore); + const walletSessionStore = await SessionStore.create(walletKvStore); const walletTransport = await WebSocketTransport.create({ url: RELAY_URL, diff --git a/apps/web-demo/src/components/UntrustedDemo.tsx b/apps/web-demo/src/components/UntrustedDemo.tsx index ec3a132..4630b53 100644 --- a/apps/web-demo/src/components/UntrustedDemo.tsx +++ b/apps/web-demo/src/components/UntrustedDemo.tsx @@ -177,7 +177,7 @@ export default function UntrustedDemo() { addDappLog("system", "Creating dApp client..."); const dappKvStore = new LocalStorageKVStore("untrusted-demo-dapp-"); - const dappSessionStore = new SessionStore(dappKvStore); + const dappSessionStore = await SessionStore.create(dappKvStore); const dappTransport = await WebSocketTransport.create({ url: RELAY_URL, @@ -350,7 +350,7 @@ export default function UntrustedDemo() { addWalletLog("system", "Creating wallet client..."); const walletKvStore = new LocalStorageKVStore("untrusted-demo-wallet-"); - const walletSessionStore = new SessionStore(walletKvStore); + const walletSessionStore = await SessionStore.create(walletKvStore); const walletTransport = await WebSocketTransport.create({ url: RELAY_URL, diff --git a/packages/core/CHANGELOG.md b/packages/core/CHANGELOG.md index 516e9d7..cfd009b 100644 --- a/packages/core/CHANGELOG.md +++ b/packages/core/CHANGELOG.md @@ -10,9 +10,11 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Changed - Add `validatePeerKey` method to `IKeyManager` interface for peer public key validation at handshake and resume time ([#70](https://github.com/MetaMask/mobile-wallet-protocol/pull/70)) +- **BREAKING:** `SessionStore` constructor is now private; use `await SessionStore.create(kvstore)` ([#71](https://github.com/MetaMask/mobile-wallet-protocol/pull/71)) ### Fixed +- Fix `SessionStore` race conditions and fire-and-forget garbage collection ([#71](https://github.com/MetaMask/mobile-wallet-protocol/pull/71)) - Guard against `NaN` in session expiry timestamps ([#70](https://github.com/MetaMask/mobile-wallet-protocol/pull/70)) ## [0.3.1] diff --git a/packages/core/package.json b/packages/core/package.json index c446388..075a38e 100644 --- a/packages/core/package.json +++ b/packages/core/package.json @@ -28,6 +28,7 @@ "registry": "https://registry.npmjs.org/" }, "dependencies": { + "async-mutex": "^0.5.0", "centrifuge": "^5.3.5", "eventemitter3": "^5.0.1", "uuid": "^11.1.0" diff --git a/packages/core/src/base-client.integration.test.ts b/packages/core/src/base-client.integration.test.ts index ef0273a..99af4a9 100644 --- a/packages/core/src/base-client.integration.test.ts +++ b/packages/core/src/base-client.integration.test.ts @@ -101,8 +101,8 @@ t.describe("BaseClient", () => { const kvstoreA = new InMemoryKVStore(); const kvstoreB = new InMemoryKVStore(); - sessionStoreA = new SessionStore(kvstoreA); - sessionStoreB = new SessionStore(kvstoreB); + sessionStoreA = await SessionStore.create(kvstoreA); + sessionStoreB = await SessionStore.create(kvstoreB); const transportA = await WebSocketTransport.create({ url: WEBSOCKET_URL, kvstore: kvstoreA, websocket: WebSocket }); const transportB = await WebSocketTransport.create({ url: WEBSOCKET_URL, kvstore: kvstoreB, websocket: WebSocket }); @@ -292,7 +292,7 @@ t.describe("BaseClient", () => { // 3. Create a third client to publish garbage data const kvstoreC = new InMemoryKVStore(); const transportC = await WebSocketTransport.create({ url: WEBSOCKET_URL, kvstore: kvstoreC, websocket: WebSocket }); - const clientC = new TestClient(transportC, new KeyManager(), new SessionStore(kvstoreC)); + const clientC = new TestClient(transportC, new KeyManager(), await SessionStore.create(kvstoreC)); await clientC["transport"].connect(); // 4. Listen for error events on client B diff --git a/packages/core/src/session-store/index.test.ts b/packages/core/src/session-store/index.test.ts index 094516a..165603a 100644 --- a/packages/core/src/session-store/index.test.ts +++ b/packages/core/src/session-store/index.test.ts @@ -41,9 +41,9 @@ t.describe("SessionStore", () => { expiresAt, }); - t.beforeEach(() => { + t.beforeEach(async () => { kvstore = new MockKVStore(); - sessionstore = new SessionStore(kvstore); + sessionstore = await SessionStore.create(kvstore); }); t.test("should set and get a session", async () => { @@ -135,25 +135,47 @@ t.describe("SessionStore", () => { t.expect(retrieved).toBeNull(); }); - t.test("should garbage collect expired sessions", async () => { - const valid = createSession("valid", Date.now() + 10000); - const expired = createSession("expired", Date.now() - 10000); - - // Manually set sessions in the kvstore - await sessionstore.set(valid); - // Manually setting an expired session by bypassing the `set` method's check - const expiredKey = `session:${expired.id}`; - await kvstore.set(expiredKey, JSON.stringify(expired)); - await (kvstore as MockKVStore)["store"].set("sessions:master-list", JSON.stringify(["valid", "expired"])); - - // Manually trigger garbage collection - await (sessionstore as any).garbageCollect(); - - const list = await sessionstore.list(); + t.test("should complete GC before the first public method returns", async () => { + const freshKvstore = new MockKVStore(); + const validSession = createSession("valid", Date.now() + 10000); + const expiredSession = createSession("expired", Date.now() - 10000); + + // Seed store with one valid and one expired session + await freshKvstore.set( + "session:valid", + JSON.stringify({ + ...validSession, + keyPair: { publicKeyB64: Buffer.from(validSession.keyPair.publicKey).toString("base64"), privateKeyB64: Buffer.from(validSession.keyPair.privateKey).toString("base64") }, + theirPublicKeyB64: Buffer.from(validSession.theirPublicKey).toString("base64"), + }), + ); + await freshKvstore.set( + "session:expired", + JSON.stringify({ + ...expiredSession, + keyPair: { + publicKeyB64: Buffer.from(expiredSession.keyPair.publicKey).toString("base64"), + privateKeyB64: Buffer.from(expiredSession.keyPair.privateKey).toString("base64"), + }, + theirPublicKeyB64: Buffer.from(expiredSession.theirPublicKey).toString("base64"), + }), + ); + await freshKvstore.set("sessions:master-list", JSON.stringify(["valid", "expired"])); + + const store = await SessionStore.create(freshKvstore); + const list = await store.list(); t.expect(list).toHaveLength(1); t.expect(list[0].id).toBe("valid"); + }); + + t.test("should not lose entries when multiple sessions are set concurrently", async () => { + const promises = []; + for (let i = 0; i < 10; i++) { + promises.push(sessionstore.set(createSession(`concurrent-${i}`, Date.now() + 10000))); + } + await Promise.all(promises); - const rawExpired = await kvstore.get(expiredKey); - t.expect(rawExpired).toBeNull(); + const list = await sessionstore.list(); + t.expect(list).toHaveLength(10); }); }); diff --git a/packages/core/src/session-store/index.ts b/packages/core/src/session-store/index.ts index a50bda6..b428572 100644 --- a/packages/core/src/session-store/index.ts +++ b/packages/core/src/session-store/index.ts @@ -1,3 +1,4 @@ +import { Mutex } from "async-mutex"; import { ErrorCode, SessionError } from "../domain/errors"; import type { IKVStore } from "../domain/kv-store"; import type { Session } from "../domain/session"; @@ -31,10 +32,20 @@ export class SessionStore implements ISessionStore { private static readonly MASTER_LIST_KEY = "sessions:master-list"; private readonly kvstore: IKVStore; + private readonly mutex = new Mutex(); - constructor(kvstore: IKVStore) { + /** + * Creates a new SessionStore instance and runs initial garbage collection. + * Use this instead of the constructor to ensure GC completes before use. + */ + static async create(kvstore: IKVStore): Promise { + const store = new SessionStore(kvstore); + await store.garbageCollect(); + return store; + } + + private constructor(kvstore: IKVStore) { this.kvstore = kvstore; - this.garbageCollect(); // Run garbage collection once on startup } /** @@ -169,11 +180,13 @@ export class SessionStore implements ISessionStore { * @param id - The ID of the session to add. */ private async addToMasterList(id: string): Promise { - const list = await this.getMasterList(); - if (!list.includes(id)) { - list.push(id); - await this.kvstore.set(SessionStore.MASTER_LIST_KEY, JSON.stringify(list)); - } + await this.mutex.runExclusive(async () => { + const list = await this.getMasterList(); + if (!list.includes(id)) { + list.push(id); + await this.kvstore.set(SessionStore.MASTER_LIST_KEY, JSON.stringify(list)); + } + }); } /** @@ -181,8 +194,10 @@ export class SessionStore implements ISessionStore { * @param id - The ID of the session to remove. */ private async removeFromMasterList(id: string): Promise { - const list = await this.getMasterList(); - const filtered = list.filter((sessionId) => sessionId !== id); - await this.kvstore.set(SessionStore.MASTER_LIST_KEY, JSON.stringify(filtered)); + await this.mutex.runExclusive(async () => { + const list = await this.getMasterList(); + const filtered = list.filter((sessionId) => sessionId !== id); + await this.kvstore.set(SessionStore.MASTER_LIST_KEY, JSON.stringify(filtered)); + }); } } diff --git a/packages/dapp-client/src/client.integration.test.ts b/packages/dapp-client/src/client.integration.test.ts index 3d064ff..1182ecf 100644 --- a/packages/dapp-client/src/client.integration.test.ts +++ b/packages/dapp-client/src/client.integration.test.ts @@ -48,7 +48,7 @@ t.describe("DappClient Integration Tests", () => { t.beforeEach(async () => { const dappKvStore = new InMemoryKVStore(); - const dappSessionStore = new SessionStore(dappKvStore); + const dappSessionStore = await SessionStore.create(dappKvStore); const dappTransport = await WebSocketTransport.create({ url: RELAY_URL, kvstore: dappKvStore, websocket: WebSocket }); dappClient = new DappClient({ transport: dappTransport, sessionstore: dappSessionStore, keymanager: new KeyManager() }); }); diff --git a/packages/wallet-client/src/client.integration.test.ts b/packages/wallet-client/src/client.integration.test.ts index 08d63b9..ed2206b 100644 --- a/packages/wallet-client/src/client.integration.test.ts +++ b/packages/wallet-client/src/client.integration.test.ts @@ -51,7 +51,7 @@ t.describe("WalletClient Integration Tests", () => { t.beforeEach(async () => { const walletKvStore = new InMemoryKVStore(); - const walletSessionStore = new SessionStore(walletKvStore); + const walletSessionStore = await SessionStore.create(walletKvStore); const walletTransport = await WebSocketTransport.create({ url: RELAY_URL, kvstore: walletKvStore, websocket: WebSocket }); walletClient = new WalletClient({ transport: walletTransport, sessionstore: walletSessionStore, keymanager: new KeyManager() }); @@ -77,7 +77,7 @@ t.describe("WalletClient Integration Tests", () => { // Create fresh client for second test to avoid state issues const walletKvStore2 = new InMemoryKVStore(); - const walletSessionStore2 = new SessionStore(walletKvStore2); + const walletSessionStore2 = await SessionStore.create(walletKvStore2); const walletTransport2 = await WebSocketTransport.create({ url: RELAY_URL, kvstore: walletKvStore2, websocket: WebSocket }); const walletClient2 = new WalletClient({ transport: walletTransport2, sessionstore: walletSessionStore2, keymanager: new KeyManager() }); diff --git a/yarn.lock b/yarn.lock index 1289ad8..5c7b9b6 100644 --- a/yarn.lock +++ b/yarn.lock @@ -2764,6 +2764,7 @@ __metadata: "@metamask/auto-changelog": ^5.0.2 "@types/uuid": ^10.0.0 "@types/ws": ^8.18.1 + async-mutex: ^0.5.0 centrifuge: ^5.3.5 eciesjs: ^0.4.15 eventemitter3: ^5.0.1 @@ -5421,6 +5422,15 @@ __metadata: languageName: node linkType: hard +"async-mutex@npm:^0.5.0": + version: 0.5.0 + resolution: "async-mutex@npm:0.5.0" + dependencies: + tslib: ^2.4.0 + checksum: be1587f4875f3bb15e34e9fcce82eac2966daef4432c8d0046e61947fb9a1b95405284601bc7ce4869319249bc07c75100880191db6af11d1498931ac2a2f9ea + languageName: node + linkType: hard + "asynckit@npm:^0.4.0": version: 0.4.0 resolution: "asynckit@npm:0.4.0"