Skip to content

fix: opinion fetcher perf + websocket safety#604

Merged
realfishsam merged 1 commit into
mainfrom
fix/239-opinion
May 24, 2026
Merged

fix: opinion fetcher perf + websocket safety#604
realfishsam merged 1 commit into
mainfrom
fix/239-opinion

Conversation

@realfishsam
Copy link
Copy Markdown
Contributor

Fixes #239
Fixes #249
Fixes #347

@realfishsam
Copy link
Copy Markdown
Contributor Author

PR Review: VERIFIED

What This Does

Two changes in Opinion exchange code:

  1. fetcher.ts: Replaces allItems = [...allItems, ...list] (O(n^2) spread-copy per page) with allItems.push(...list) (amortized O(1) append). Same pattern as PR perf: replace O(n²) concat with push in kalshi fetcher #603.

  2. websocket.ts: Three improvements:

    • Adds a 30s connection timeout with settled flag to prevent the connection promise from hanging indefinitely if the WS server never responds.
    • Guards open and error handlers against double-settlement via the settled flag, and clears the timeout on settlement.
    • Removes two Map.get()! non-null assertions in watchOrderBook and watchTrades by capturing the result of get() into a local and branching.

Blast Radius

Two files in core/src/exchanges/opinion/. The fetcher change is isolated to a single pagination loop. The websocket changes are confined to the connect() promise lifecycle and the two watch* resolver-push sites.

Findings

  • Connection timeout is correctly implemented. The settled flag prevents both the timer callback and the open/error handlers from double-settling the promise. ws.terminate() is correctly used (hard close, no graceful FIN) since the connection never completed.
  • Non-null assertion removal is clean. The new pattern (const existing = map.get(key); if (existing) { existing.push(...) } else { map.set(key, [...]) }) avoids the intermediate empty-array allocation from the old if (!has) set([]) + get()!.push() pattern.
  • Minor note: The close event handler in connect() does not clear the timeout or check settled. If the WS closes before open fires (e.g., server rejects immediately via close frame without error), the timeout would still fire 30s later and call reject() on an already-resolved promise -- but settled protects against that, so this is safe. The close handler itself does not resolve or reject, it just schedules reconnect, so no double-settlement risk.

Semver Impact

patch -- bug fix (timeout + assertion safety), no API change.

Risk

Low.

@realfishsam realfishsam merged commit 80e950c into main May 24, 2026
9 of 12 checks passed
@realfishsam realfishsam deleted the fix/239-opinion branch May 24, 2026 17:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

1 participant