From e6a73687ddca5ff732f64c6ab589b632b26411ef Mon Sep 17 00:00:00 2001 From: "google-labs-jules[bot]" <161369871+google-labs-jules[bot]@users.noreply.github.com> Date: Sat, 14 Feb 2026 13:22:43 +0000 Subject: [PATCH] fix: resolve NostrClient initialization race condition Co-authored-by: PR0M3TH3AN <53631862+PR0M3TH3AN@users.noreply.github.com> --- ...13-28-00_race-condition-agent_completed.md | 15 ++++ ...weekly-race-condition-report-2026-02-14.md | 44 +++++++++++ js/nostr/client.js | 73 ++++++++++--------- tests/race/nostr-client-init-race.test.mjs | 59 +++++++++++++++ 4 files changed, 158 insertions(+), 33 deletions(-) create mode 100644 docs/agents/task-logs/weekly/2026-02-14_13-28-00_race-condition-agent_completed.md create mode 100644 docs/race-condition-reports/weekly-race-condition-report-2026-02-14.md create mode 100644 tests/race/nostr-client-init-race.test.mjs diff --git a/docs/agents/task-logs/weekly/2026-02-14_13-28-00_race-condition-agent_completed.md b/docs/agents/task-logs/weekly/2026-02-14_13-28-00_race-condition-agent_completed.md new file mode 100644 index 00000000..d1d615b3 --- /dev/null +++ b/docs/agents/task-logs/weekly/2026-02-14_13-28-00_race-condition-agent_completed.md @@ -0,0 +1,15 @@ +# Weekly Agent Task Complete: Race Condition Audit + +- **Agent**: race-condition-agent +- **Date**: 2026-02-14 +- **Cadence**: Weekly + +## Summary +Audited `js/nostr/client.js` and identified a critical race condition in the initialization logic where `isInitialized` was set prematurely. Implemented a fix using the `initPromise` pattern and validated it with a new reproduction test. + +## Artifacts +- **Report**: [docs/race-condition-reports/weekly-race-condition-report-2026-02-14.md](../../race-condition-reports/weekly-race-condition-report-2026-02-14.md) +- **Reproduction Test**: [tests/race/nostr-client-init-race.test.mjs](../../../../tests/race/nostr-client-init-race.test.mjs) + +## Next Steps +- Monitor Sentry for `Cannot read properties of null` errors related to pool access. diff --git a/docs/race-condition-reports/weekly-race-condition-report-2026-02-14.md b/docs/race-condition-reports/weekly-race-condition-report-2026-02-14.md new file mode 100644 index 00000000..4a50ea2c --- /dev/null +++ b/docs/race-condition-reports/weekly-race-condition-report-2026-02-14.md @@ -0,0 +1,44 @@ +# Weekly Race Condition Report: 2026-02-14 + +## Audit Overview +- **Agent**: bitvid-race-condition-agent +- **Focus Areas**: `js/nostr/client.js`, `js/nostr/managers/ConnectionManager.js`, `js/nostr/videoEventBuffer.js` +- **Cadence**: Weekly + +## Executive Summary +One critical race condition was identified in the `NostrClient` initialization logic. The client flagged itself as `isInitialized = true` synchronously at the start of the `init()` method, before the connection pool was actually ready. This allowed concurrent calls (or UI components checking the flag) to access the uninitialized pool, causing crashes (`TypeError: Cannot read properties of null`). + +## Findings + +### 1. Critical: NostrClient Initialization Race +- **Location**: `js/nostr/client.js` +- **Severity**: Critical +- **Description**: The `init()` method set `this.isInitialized = true` immediately to prevent re-entrant calls. However, `ensurePool()` and `connectToRelays()` are asynchronous. Any component checking `client.isInitialized` during this window would proceed to use the client and crash. +- **Interleaving**: + 1. Caller A calls `client.init()`. + 2. `client.isInitialized` becomes `true`. + 3. Caller A awaits `restoreLocalData()` (async). + 4. Caller B checks `client.isInitialized` -> `true`. + 5. Caller B calls `client.subscribeVideos()`. + 6. `subscribeVideos` calls `this.pool.sub`. + 7. `this.pool` is `null` (because Caller A hasn't reached `ensurePool` yet). + 8. Crash. + +- **Fix Implemented**: + - Introduced `this.initPromise` to track the ongoing initialization. + - `init()` now returns the existing promise if called concurrently. + - `this.isInitialized` is only set to `true` **after** the pool is ready and relays are connected. + - Fix validated with a new reproduction test `tests/race/nostr-client-init-race.test.mjs`. + +## Other Observations + +### VideoEventBuffer (Low Risk) +- `js/nostr/videoEventBuffer.js` uses a debounce timer (`scheduleFlush`) and checks `document.hidden` to prevent UI thrashing. +- A potential "render race" exists where `flush()` triggers `onVideo` before NIP-71 metadata is fully hydrated. This is an intended trade-off for responsiveness (Stale-While-Revalidate pattern) and not a data corruption bug. + +### ConnectionManager (Safe) +- `js/nostr/managers/ConnectionManager.js` properly guards `ensurePool` with `this.poolPromise`, which prevented double-instantiation but didn't protect the `NostrClient` wrapper from the race described above. + +## Next Steps +- **Next Audit**: Review `js/nostr/nip07Permissions.js` and `js/nostr/nip46Client.js` for signer negotiation races. +- **Monitoring**: Watch for `Cannot read properties of null` errors in Sentry related to `pool.sub`. diff --git a/js/nostr/client.js b/js/nostr/client.js index 9f8d43d3..1846b5ec 100644 --- a/js/nostr/client.js +++ b/js/nostr/client.js @@ -570,6 +570,7 @@ export class NostrClient { this.dmDecryptor = null; this.dmDecryptorPromise = null; this.isInitialized = false; + this.initPromise = null; } get pubkey() { return this.signerManager.pubkey; } @@ -1020,43 +1021,49 @@ export class NostrClient { * @returns {Promise} Resolves when the relay pool is initialized and connections are attempted. */ async init() { - if (this.isInitialized) { - return; - } - this.isInitialized = true; - - devLogger.log("Connecting to relays..."); - - // 1. Restore cache for immediate UI render (Stale-While-Revalidate) - await this.restoreLocalData(); - - try { - this.scheduleStoredRemoteSignerRestore(); - } catch (error) { - devLogger.warn("[nostr] Failed to schedule remote signer restoration:", error); + if (this.initPromise) { + return this.initPromise; } - try { - await this.ensurePool(); - const results = await this.connectToRelays(); - const successfulRelays = results - .filter((r) => r.success) - .map((r) => r.url); - if (successfulRelays.length === 0) { - userLogger.warn( - "[nostr] No relays connected during init. Retrying in the background.", - ); - this.scheduleRelayReconnect({ reason: "initial-connect-failed" }); - } else { - this.resetRelayReconnectState(); - devLogger.log( - `Connected to ${successfulRelays.length} relay(s)`, + this.initPromise = (async () => { + devLogger.log("Connecting to relays..."); + + // 1. Restore cache for immediate UI render (Stale-While-Revalidate) + await this.restoreLocalData(); + + try { + this.scheduleStoredRemoteSignerRestore(); + } catch (error) { + devLogger.warn( + "[nostr] Failed to schedule remote signer restoration:", + error ); } - } catch (err) { - userLogger.error("Nostr init failed:", err); - throw err; - } + + try { + await this.ensurePool(); + const results = await this.connectToRelays(); + const successfulRelays = results + .filter((r) => r.success) + .map((r) => r.url); + if (successfulRelays.length === 0) { + userLogger.warn( + "[nostr] No relays connected during init. Retrying in the background." + ); + this.scheduleRelayReconnect({ reason: "initial-connect-failed" }); + } else { + this.resetRelayReconnectState(); + devLogger.log(`Connected to ${successfulRelays.length} relay(s)`); + } + this.isInitialized = true; + } catch (err) { + userLogger.error("Nostr init failed:", err); + this.initPromise = null; + throw err; + } + })(); + + return this.initPromise; } /** diff --git a/tests/race/nostr-client-init-race.test.mjs b/tests/race/nostr-client-init-race.test.mjs new file mode 100644 index 00000000..6248270f --- /dev/null +++ b/tests/race/nostr-client-init-race.test.mjs @@ -0,0 +1,59 @@ + +import "../test-helpers/setup-localstorage.mjs"; +import assert from "node:assert/strict"; +import test from "node:test"; +import { NostrClient } from "../../js/nostr/client.js"; + +// Mock global.window if missing +if (typeof globalThis.window === "undefined") { + globalThis.window = {}; +} + +test("NostrClient initialization race condition: concurrent init() calls", async (t) => { + const client = new NostrClient(); + + // Mock ensurePool to simulate a slow initialization + let poolReady = false; + + // We mock connectionManager.ensurePool via the instance + client.connectionManager.ensurePool = async () => { + // Delay to simulate async work + await new Promise(resolve => setTimeout(resolve, 50)); + + // Create a fake pool + const fakePool = { + sub: () => ({ on: () => {}, unsub: () => {} }), + get: async () => null, + list: async () => [], + ensureRelay: async () => {}, + }; + client.connectionManager.pool = fakePool; + poolReady = true; + return fakePool; + }; + + // Also mock connectToRelays to avoid network calls + client.connectToRelays = async () => []; + client.restoreLocalData = async () => true; + + // ACT: Call init() (Caller A) + const initPromiseA = client.init(); + + // ACT: Call init() again immediately (Caller B) + const initPromiseB = client.init(); + + // ASSERT: Caller B should receive a promise that resolves when initialization is complete. + // In the buggy version, initPromiseB is undefined because it returns early. + assert.ok(initPromiseB instanceof Promise, "Second init() call should return a Promise, not undefined"); + + await initPromiseB; + + assert.equal(poolReady, true, "Pool should be ready after awaiting second init() call"); + + // Verify we can use the client + try { + client.subscribeVideos(() => {}); + } catch (e) { + assert.fail("Should be able to subscribe after awaiting init()"); + } +});