fix(KNO-13857): reduce websocket reconnect load when connections can't recover#1030
Closed
kylemcd wants to merge 1 commit into
Closed
fix(KNO-13857): reduce websocket reconnect load when connections can't recover#1030kylemcd wants to merge 1 commit into
kylemcd wants to merge 1 commit into
Conversation
…t recover Escalate the Phoenix socket reconnect backoff toward a 10-minute cap (was a fixed 30s cap that Phoenix reset on every successful open), so a client that can never reconnect - e.g. after an API key rotation leaves stale credentials rejected on every upgrade - settles into a slow retry cadence instead of a tight loop. The escalation survives brief "connect then immediately drop" cycles and resets once a connection stays up for 30s. Also: - teardown() always disconnects the socket, even mid-reconnect, so a reauth that replaces the client can't leak a socket that retries forever. - Hidden background tabs stop retrying: a socket mid-reconnect when the page is hidden is disconnected, not just connected ones, and resumes on visible. - Reconnect promptly on the browser `online` event so recovery after a real network drop doesn't wait out the longer backoff.
🦋 Changeset detectedLatest commit: b6bb977 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Member
Author
|
Superseded by #1031 (same changes, correct branch name |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1030 +/- ##
==========================================
+ Coverage 64.49% 64.68% +0.19%
==========================================
Files 212 212
Lines 10217 10275 +58
Branches 1389 1405 +16
==========================================
+ Hits 6589 6646 +57
- Misses 3603 3604 +1
Partials 25 25
|
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
1 participant
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.
Description
Fixes KNO-13857. Follow-up to INC-408, where a customer rotated their public API key while deployed feed clients kept the old one. Each client's socket kept reconnecting with stale credentials, got a 403 on every upgrade, and retried forever — fleet-wide this grew websocket inbound volume ~1000% and saturated the sockets pods.
Browsers don't expose the HTTP status of a failed WS upgrade to JS — Phoenix only sees an abnormal close (1006) and reschedules — so a permanently-failing connection can never "give up" on its own. This PR makes a failing socket get quieter over time and makes background tabs stop entirely, all within
packages/client.What changed (
packages/client/src/api.ts,pageVisibility.ts):reconnectAfterMsnow escalates toward a 10-minute cap (was a fixed 30s cap that Phoenix reset on every successful open). A newtrackConnectionStability()watchessocket.onOpen/onCloseand keeps its ownunstableConnectionCountthat — unlike Phoenix's counter — survives brief "connect then immediately drop" cycles, and resets to 0 only once a connection stays up ≥30s. Identical to today's curve for the first ~6 attempts (1s → 30s), so genuine blips are unaffected.teardown()always disconnects the socket, even mid-reconnect (dropped theisConnected()guard).disconnect()is the only thing that cancels Phoenix's pending reconnect timer, so this stops a reauth (e.g. automatic token refresh) from leaking a socket that retries forever with soon-to-be-stale credentials.PageVisibilityManagernow parks a socket that is mid-reconnect when the page is hidden, not just a connected one, and resumes it when visible. Background zombie tabs drop to zero requests.onlineresume trigger. Reconnects promptly on the browseronlineevent so recovery after a real network drop doesn't wait out the (now longer) backoff.Why "decay to slow" instead of "hard stop after N": a hard cap silently kills realtime forever for users on flaky networks; a slow cadence self-heals after genuine outages while still cutting a dead-credential fleet's request volume by ~40–60× (and to zero for background tabs).
Tunables reviewers may want to weigh in on: the 10-minute terminal cadence (
SOCKET_RECONNECT_MAX_DELAY_MS) and the 30s stability threshold (STABLE_CONNECTION_THRESHOLD_MS).Out of scope (follow-ups, noted on KNO-13857): server-side accept-then-close-1000 on bad credentials (the only thing that helps already-deployed fleets, since client fixes require customers to upgrade + redeploy) and per-API-key connect rate limiting at the ALB.
Checklist
Added coverage in
test/api.test.ts(backoff escalation, ≥30s reset, clean-close no-op, teardown-while-disconnected,onlinesingle reconnect + listener cleanup) andtest/pageVisibility.test.ts(mid-reconnect park/resume, never-activated no-op). Full client suite (582 tests),type:check,lint, andformat:checkall pass.