[#850] Fix EventSource reconnect storm/leak in useStreamEvents - #996
Closed
Hollujay wants to merge 6 commits into
Closed
[#850] Fix EventSource reconnect storm/leak in useStreamEvents#996Hollujay wants to merge 6 commits into
Hollujay wants to merge 6 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, |
1. Backend CI Build: Fix TS2532 errors with proper null/undefined checks
- events-wire-format.test.ts: Use nullish coalescing fallback for
HANDLER_READ_FIELDS lookup
- stream.validator.test.ts: Add expect(result.error).toBeDefined()
before accessing issues array
2. Frontend CI Lint: Fix parsing errors and cleanup
- Rename useIncomingStreams.test.ts to .tsx (contains JSX)
- Fix malformed try/catch block indentation in dashboard.ts fetchStreams
- Remove unused useDashboard import from dashboard-view.tsx
- Remove 4 unused eslint-disable no-console directives from logger.ts
3. Soroban Contracts CI: Run cargo fmt --all (rustfmt)
4. Backend npm test: Fix i128 decode error in stream-lifecycle test
- Root cause: nativeToScVal(BigInt) without explicit type creates
non-i128 ScVal; fix uses scvI128 for BigInt override values
- stream.validator.test.ts: assert issues.length > 0 before accessing [0] - useIncomingStreams.test.tsx: add eslint-disable for no-explicit-any in tests
- Frontend: Fix useIncomingStreams test timeout by mocking fetchIncomingStreams with matching stream data so the pollIndexerForWithdraw loop exits on the first attempt instead of timing out after 63s of exponential backoff. This test was previously skipped (`.ts` with JSX couldn't parse); the `.tsx` rename exposed a pre-existing mock bug. - Backend: Add fileParallelism: false to vitest config so integration tests sharing the same database don't clobber each other's cleanup steps (foreign key violations).
- Frontend: Clear setQueryData spy after mutateAsync so the assertion only catches the poll's call, not any optimistic onMutate update. - CI: Add --no-file-parallelism to Backend CI and pr-test-gate workflows instead of baking it into vitest.config.ts so local devs can still run fast parallel tests.
ogazboiz
requested changes
Aug 3, 2026
ogazboiz
left a comment
Contributor
There was a problem hiding this comment.
the hook fix is the right approach: memoized subscriptionKey, clear-before-reschedule, and a reconnect cap all address #850 properly, and the new tests are welcome. but the branch carries a lot that is not the hook:
- rebase onto main (currently conflicting).
- split out or drop the CI workflow edits (--no-file-parallelism in ci.yml and pr-test-gate.yml), contracts test.rs formatting, logger.ts, and dashboard changes. keep the hook fix plus its tests.
- the useIncomingStreams.test.ts to .tsx rename collides with #1070 and #1001, coordinate there.
- confirm green CI post-rebase.
if you want to keep contributing, join us on Telegram: https://t.me/+DOylgFv1jyJlNzM0
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