[#850] Fix EventSource reconnect storm/leak in useStreamEvents - #996
Open
Hollujay wants to merge 2 commits into
Open
[#850] Fix EventSource reconnect storm/leak in useStreamEvents#996Hollujay wants to merge 2 commits into
Hollujay wants to merge 2 commits into
Conversation
Use a stable subscription key derived from sorted streamIds, subscribeToAll, and jwtToken so buildUrl/connect only change when the subscription actually changes — not on every render due to inline array literals or default [] identity. - Derive subscriptionKey as a stable memoized string - Clear pending reconnect timer before scheduling a new one - Cap reconnect attempts at 20 (configurable via constant) Fixes LabsCrypt#850
Author
|
CI failures here are pre-existing and unrelated to this fix:
Local verification (noted in the PR description) passed cleanly: 11/11 tests, |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
useStreamEventswas tearing down and recreating theEventSourceon every render, and since each incoming event re-renders the consumer, the stream ended up reconnecting on every event in a self-sustaining reconnect loop.Root cause
buildUrlwas auseCallbackdepending directly onstreamIds, which is always a fresh array reference every render (both the default[]and every call site passing inline array literals). New reference → newbuildUrl→ newconnect→ mountuseEffect([connect])tears down the oldEventSourceand creates a new one. Incoming events →setEvents→ re-render → cycle repeats.Changes
frontend/src/hooks/useStreamEvents.tsStable subscription identity — A
subscriptionKeystring is derived from sortedstreamIds,subscribeToAll, andjwtTokenviauseMemo.buildUrldepends on this key rather than the raw values, so it only changes when the subscription content changes — inline array identity changes on every render no longer cause a reconnect.Reconnect timer cleared before rescheduling — Both the
connectentry point and theonerrorhandler now clear any pending reconnect timeout before scheduling a new one, preventing duplicate or stale timers.Reconnect attempt cap — Added
MAX_RECONNECT_ATTEMPTS = 20with areconnectAttemptsRefcounter. Incremented on each reconnect attempt; once the cap is exceeded no further reconnection timers are set. The counter resets to0on a successfulonopen.frontend/src/__tests__/useStreamEvents.test.tsxTwo new regression tests:
"creates only one EventSource across multiple re-renders and incoming events" — Renders the hook, re-renders multiple times with the same inline
streamIdsarray, emits events (which triggersetEvents→ consumer re-render), re-renders again. AssertsinstanceCount === 1and the sameMockEventSourceinstance is reused throughout."stops reconnecting after reaching the cap" — Triggers 25 consecutive errors without a successful connection. Asserts at most 21
EventSourceinstances were created (1 initial + 20 capped reconnects).Call sites (no changes needed)
All three call sites pass inline arrays — this is exactly the pattern that was broken before, and the fix handles it transparently because the stable key is derived from array contents, not reference:
NotificationDropdown.tsx—userPublicKeys: [publicKey]dashboard-view.tsx—userPublicKeys: [session.publicKey]stream-details-content.tsx—streamIds: [streamId]Verification
npm test— 11/11 tests pass (9 existing + 2 new)npm run lint— no new warnings or errorsnpx tsc --noEmit— no new type errorsCloses #850