fix(mobile): detect half-open relay sockets with client-side ping/pong - #3231
Open
Dirtywater97 wants to merge 1 commit into
Open
fix(mobile): detect half-open relay sockets with client-side ping/pong#3231Dirtywater97 wants to merge 1 commit into
Dirtywater97 wants to merge 1 commit into
Conversation
The relay pings every 30s and closes after 3 missed pongs, which protects the relay from dead clients but does nothing for the client. Mobile networks silently drop idle TCP flows without sending a FIN, so a dead socket still reports as open: `onDone`/`onError` never fire, the session notifier is never told to reconnect, and the connection can stay dead until some later network event or an app restart. `WebSocketChannel.connect` resolves to a platform-adaptive implementation that exposes no ping configuration. `IOWebSocketChannel` does: an unanswered ping closes the socket with `goingAway`, which reaches the existing `onDone` handler and drives the normal reconnect path, so no additional lifecycle handling is required. The mobile app targets Android and iOS only, so binding to the `dart:io` implementation is safe. `dart:io` never has two pings outstanding, so detection takes up to two intervals — about 40s at 20s, keeping foreground recovery under a minute. The session already disconnects 5s after backgrounding, so the added probe traffic does not run while the app is suspended. The connector is isolated in its own file so `relay_socket.dart` changes by one call site plus an import. Test covers the real failure: a server that completes the WebSocket handshake then goes silent, never answering pings and never sending a close frame. It asserts the socket closes cleanly through `onDone` with close code 1001 rather than erroring. Verified to fail against `WebSocketChannel.connect` (socket stays open indefinitely) and pass with ping/pong enabled. Refs block#3224 Signed-off-by: dirtywater97 <matthew.d.ahern@gmail.com>
ss251
added a commit
to ss251/buzz
that referenced
this pull request
Jul 28, 2026
…relay session Closes gaps 3, 4 and 5 of the mobile relay client's connection resilience (block#3224), all in mobile/lib/shared/relay/relay_session.dart. Gap 1 (stall detection) is PR block#3231's and gap 2 (timeouts) needs a design decision; neither is touched here. Gap 3 — reconnect backoff had no jitter. Each scheduled wait is now randomised by ±20% via `_jitteredDelay`, mirroring the desktop's `jittered_duration` (crates/buzz-acp/src/relay.rs). The ladder position `_reconnectDelayMs` stays un-jittered and jitter is applied per scheduled wait, matching the desktop's split between `backoff_step` and the jitter applied at call time. Without this a fleet of clients reconnects in lockstep after a relay blip. Gap 4 — the backoff ladder reset on connect rather than on stability, so a socket that died seconds after connecting reset to base on every cycle: the doubling never accumulated and a flapping link hammered the relay indefinitely. `_handleConnected` now arms a 60s timer (`_stableConnectionMs`, mirroring the desktop's `STABLE_CONNECTION_SECS`) that resets the ladder only if the connection survives; the timer is cancelled on disconnect, pause and dispose. There are three reset sites and only one changed. The two left resetting immediately are deliberate, and each now says why in a comment: - `_handleConnected` (was relay_session.dart:378) — CHANGED to stability-gated. This is the automatic reconnect path and the only one that can self-spin. - `reconnect()` (:311) — unchanged. Caller-driven, so it cannot loop on its own. - `onAppResumed()` (:344) — unchanged. The preceding disconnect was our own backgrounding, not relay trouble, and the user is looking at the app; making them wait out a ladder the relay never asked for would read as a hang. The desktop has no equivalent of the latter two (no app-backgrounding concept), so desktop parity does not decide them; this is the conservative reading, being the smallest behavioural change that closes the gap. Gap 5 — NOTICE frames were dropped by `_handleMessage`, so the relay's explicit "slow down" was discarded. Since gaps 3+4+5 compound, the client could not hear the rate limit it was provoking. NOTICE is now logged via debugPrint, and a `rate-limited:` notice raises the reconnect backoff floor from the relay's own `retry in Ns` hint: absent or sub-2s hints floor to 5s and the value is clamped to `_maxReconnectDelayMs`, both mirroring the desktop's `set_rate_limit_gate`. It takes the maximum against the current ladder, so overlapping notices can never shorten a longer backoff already in place. Gap 5 deliberately feeds the existing `_reconnectDelayMs` ladder rather than introducing a gate object. PR block#3053 (open, unmerged) adds a RelayRateLimitGate driven by CLOSED frames and rewrites `_handleConnected`; it contains no NOTICE handling and no jitter, so these gaps are unclaimed. Routing NOTICE through the ladder keeps this free of a hard dependency on block#3053 while pointing at the same backpressure concept, so NOTICE can be redirected into that gate in a small follow-up if block#3053 lands. Deviation from the work order worth flagging to review: the order specified ±25% jitter as the desktop's value. The desktop's `jittered_duration` actually uses a factor of [0.8, 1.2) — ±20% — so ±20% is implemented for real parity. Tests: 11 new cases in mobile/test/shared/relay/relay_session_test.dart. Each of the three fixes was negative-controlled by reverting only its lib/ change and confirming the new tests fail — jitter neutralised gives a flat 1000ms, the reset-on-connect restored collapses the ladder to 2000 where 4000 is expected, and the NOTICE case removed leaves the backoff at 1000. `Random` is now injectable through the constructor (alongside the existing `httpClient` and `socketFactory`) so the jitter assertions are deterministic rather than timing dependent. Gate from mobile/: dart format --set-exit-if-changed clean, flutter analyze clean, flutter test 834 passed / 1 skipped. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HU2jkzBjmJSBGz2ciGFdej Signed-off-by: ss251 <ss251@uw.edu>
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.
Fixes gap 1 of #3224 — the one behind the reported symptom.
Problem
The relay pings every 30s and closes after 3 missed pongs (
crates/buzz-relay/src/connection.rs:378-400). That protects the relay from dead clients; it does nothing for the client.Mobile networks — cellular especially — silently drop idle TCP flows without sending a FIN. When that happens the phone's WebSocket never receives a close frame, so neither
onErrornoronDonefires inrelay_socket.dart:84-96,RelaySessionNotifieris never told to reconnect, and the socket stays dead while the UI reports connected. A Play Store user described the result as "constantly having to force quit the app just to see a message."Fix
WebSocketChannel.connectresolves toAdapterWebSocketChannel, which exposes no ping configuration (web_socket_channel/lib/src/channel.dart:107-108).IOWebSocketChanneldoes — and the package is already a direct dependency (mobile/pubspec.yaml:20, locked at 3.0.3):So the missing liveness detection is a constructor away. The unanswered ping produces a normal close, which reaches the existing
onDonehandler and drives the existing reconnect path — no new lifecycle handling.mobile/hasandroid/andios/and no web target, so binding to thedart:ioimplementation is safe. The connector is isolated inrelay_websocket_connector.dartsorelay_socket.dartchanges by one call site plus an import.On the interval
dart:ionever has two pings outstanding: it waits one interval, sends a ping, then allows one more for the pong. Detection therefore takes up to2 × interval— ~40s at 20s, keeping foreground recovery under a minute. Cost is three extra ping/pong exchanges per idle foreground minute; the session already disconnects 5s after backgrounding (relay_session.dart:316-329), so this doesn't run while suspended. 30s would halve the traffic and roughly double time-to-detection — happy to change it if you prefer that trade.Alternative you may prefer
Desktop solved this with a passive watchdog (
relayStallWatchdog.ts) that treats inbound traffic as the liveness signal, chosen deliberately for a Tauri constraint documented at the top of that file. That constraint doesn't apply to Flutter, and active ping/pong is far less code here. But if you'd rather keep the two clients symmetric, say so and I'll port the watchdog shape instead.Testing
New test stands up a raw
ServerSocketthat completes the WebSocket handshake and then goes silent — never answering pings, never sending a close frame. Adart:ioWebSocketserver can't express this because it auto-replies to pings, hence the manual handshake.It asserts the socket closes cleanly through
onDonewith close code 1001, not viaonError— the distinction that matters, since only a clean close exercises the reconnect path.Verified in both directions:
WebSocketChannel.connect: fails — socket stays open indefinitely.Baseline before this change was 823 passed, 1 skipped — the 2 new tests are the only delta, no regressions.
Not included
Gaps 2–5 from #3224 (8s vs 25s timeouts, missing backoff jitter, backoff resetting on connect rather than on stability, dropped
NOTICEframes) are deliberately left out so this stays independently reviewable and revertable. Happy to follow up with those in a separate PR.