Repository navigation
fix(state): back off when the connection closes before connecting - #7605
Conversation
A transport closed before the namespace connected (for example, a CONNECT packet over the server or a proxy message size limit) emitted "disconnect", and the handler reconnected at once with no error. Oversized browser storage made the page reconnect hundreds of times per second with no toast (reflex-dev#7486). Such a close now counts as a failed attempt: it records a connection error and retries with the incremental backoff. The backoff read its attempt count from inside the setConnectErrors updater. React runs that updater lazily when the component has pending work, so the delay was often 0 ms. The count now lives in the connect() closure. Fixes reflex-dev#7486
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
[Medium impact] The PR appears safe to merge; no new actionable issues were found. SummaryAdds growing retry delays when a connection closes before it finishes connecting.
FarhanAliRaza manually resolved the earlier duration-comment thread without explanation. Reviews (4) · Last reviewed commit: "Merge branch 'main' into farhan/7486-rec..." · Reviewed by Greptile |
There was a problem hiding this comment.
All reported issues were addressed across 4 files
Reply with feedback, questions, or to request a fix.
View guided diff | Re-trigger cubic
Merging this PR will not alter performance
|
A transport error before the namespace connects emits connect_error and then disconnect. The disconnect reconnected at once and skipped the backoff, so a polling CONNECT rejected with an HTTP error (for example a proxy 413) still looped. The disconnect now waits for the pending retry. A connect cancels the pending retry, so a stale timer cannot reconnect after a later intentional disconnect. The integration test now asserts that at least three retries happen with growing gaps, instead of only an upper bound after a fixed 3 s wait.
masenf
left a comment
There was a problem hiding this comment.
might need to rerun ci, looks like it was a flakeeeeeeeeeeeeeeeeeeeeeeee
Port upstream's reconnect back-off (reflex-dev#7605) to the transport abstraction: retryConnect keeps the branch's addConnectError helper and caps the attempt count at MAX_CONNECT_ERRORS. Socket.IO reports a transport closed before the namespace connected as a disconnect, which now retries with back-off; the plain WebSocket reports it as connect_error, so it gets the same back-off. The state.js back-off tests run over both transports, with the fake socket reporting a close before connecting the way each transport does, and read connect errors through upstream's lazily applied updaters.
Port upstream's EventNamespace changes to the moved namespace modules: the trusted proxy hops for the client IP (reflex-dev#7614) apply in build_static_router_data, and the cancellation of a disconnected socket's page-load chains (reflex-dev#7415) runs in BaseEventNamespace.handle_disconnect, so both transports get them. The branch already decoded non-UTF-8 header values (reflex-dev#7602). Their tests move from tests/units/test_app.py to tests/units/test_event_namespace.py, and the transport benchmark's app double gains the event processor attribute the disconnect reads. Port the oversized hydrate check of reflex-dev#7551 to the Socket.IO transport only: state.js wraps the socket.io encoder where it creates the Socket.IO socket, since the plain WebSocket has no engine.io encoder. Its state.js tests run over Socket.IO, whose fake socket now encodes CONNECT packets, and the reconnect back-off of reflex-dev#7605 stays in place for both transports. uv.lock is upstream's, relocked for the branch's dependencies.
Summary
Fixes #7486. A page with more than 1 MB of browser storage reconnected hundreds of times per second, showed no error, and never hydrated. The cause is the reconnect logic, not the payload size:
REFLEX_SOCKET_MAX_HTTP_BUFFER_SIZE), socket.io-client emitsdisconnect("transport close"). The namespace never connected. Thedisconnecthandler reconnected at once and recorded no error. Now, whenwait_connectis still set, the close counts as a failed attempt: it records a connection error and retries with the incremental backoff (theconnect_errorpath).setConnectErrorsupdater. React runs that updater lazily when the component has pending work, so a retry can get a 0 ms delay. In a live app this hit the first retry in 1 of 21 measured retries. Change 1 alone stops the loop. The count now lives in theconnect()closure. It resets on connect and is capped at 10, as before.connect_errorand thendisconnect. Thedisconnectreconnected at once and skipped the backoff. For example, a polling CONNECT that a proxy rejects with 413 looped every ~25 ms. Thedisconnectnow waits for the pending retry. A connect cancels the pending retry.This also bounds limits that the client cannot know, for example a proxy that closes the socket on large frames.
Tests
state_js.test.mjs: a transport that closes before CONNECT retries with 200 ms, then 400 ms, and records an error. A transport error (connect_errorthendisconnect) schedules one retry and no immediate reconnect. A connect cancels the pending retry. The harness now appliessetConnectErrorsupdaters only on render, as React does with pending work. A connection that drops after it connected still reconnects at once.test_hydration_storage.py::test_refused_hydration_retries_with_backoff: the real server limit (1.1MLocalStoragevalue) and a proxy limit (a route that closes frames over 100 kB). Both show the connection toast, retry at least three times with growing gaps of more than 150 ms, and hydrate after the storage is cleared. Both cases fail onmain.Relation to #7551
#7551 checks the CONNECT packet size on the client. That check does not cover the proxy case, so the loop stays there. A size check can still follow as an optional, clearer message for the known server limit.