fix(#850): EventSource reconnect storm + backoff timing - #1170
Merged
Conversation
- Memoize subscription key so identical streamIds/token don't trigger spurious reconnects on every render. - Clear any pending reconnect timeout at the start of connect() to prevent overlapping connections. - Cap reconnect attempts at 20 to stop infinite reconnect loops. - Combined with an independent backoff-timing fix already on main (precompute delay before scheduling, not after).
# Conflicts: # frontend/src/hooks/useStreamEvents.ts
4 tasks
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.
Closes #850
Summary
Fixes the EventSource reconnect storm in
useStreamEvents— supersedes #996 with a scoped-down diff per @ogazboiz's review. This PR touches only the two files directly related to the hook fix:frontend/src/hooks/useStreamEvents.tsfrontend/src/__tests__/useStreamEvents.test.tsxNo CI workflow edits, formatting changes, logger/dashboard changes, or backend test tweaks are included — those were dropped from #996 as unrelated scope creep.
What changed
Reconnect storm fix:
streamIds+subscribeToAll+jwtToken) sobuildUrldoesn't get a new reference on every render, which previously caused the effect to reconnect even when the actual subscription hadn't changed.connect()now clears any pending reconnect timeout at the start, preventing overlapping/duplicate connections.MAX_RECONNECT_ATTEMPTScap (20) so a persistently failing connection stops retrying instead of looping forever.Combined with an independent fix already on
main:While this branch was open,
mainpicked up a separate fix for the backoff-delay timing (precomputing the capped delay and the next backoff value before schedulingsetTimeout, rather than updating it afterconnectRef.current()already ran). Both fixes touch the sameconnect()/onerrorblock, so this PR merges them together rather than one overwriting the other — seeuseStreamEvents.tsfor the combined logic.Testing
useStreamEvents.test.tsxnow includes all 14 relevant tests: the original hook tests,main's 3 backoff-timing tests (exponential growth, cap atmaxRetryDelay, reset after successful reconnect), and this PR's 2 storm-prevention tests (single EventSource instance across re-renders, stops reconnecting after hitting the cap).tsc --noEmitthat this change introduces zero new type errors compared to a cleanmaincheckout.Notes for reviewers
useIncomingStreams.test.ts → .tsxrename from the original [#850] Fix EventSource reconnect storm/leak in useStreamEvents #996 is not included here — it already landed onmainvia another PR, so re-including it would have re-created the collision.