Repository navigation
Keep the gateway socket alive while the app is backgrounded - #152
Conversation
Minimizing the app let Android freeze or kill the process, stopping the gateway.ping heartbeat so the gateway tore the socket down after its 45s silence deadline — every minimize dropped the connection. Add a specialUse foreground service, started from the app-root lifecycle observer only while DesktopGatewayClients are open and stopped on resume, so Dart timers keep firing in the background and the socket survives minimization. A refused FGS start (Android 12+ OEM limits) stays a silent no-op: the existing reconnect-on-resume loop remains the fallback.
…ce, socket-accurate gating) Deep audit (independent omp -p review + mutation-tested red loops) found: - HIGH: exiting via Back fired start() on inactive but detached never stopped the service — the FGS pinned the process with a permanent notification while the Dart isolate was dead. detached and dispose() now stop unconditionally. - stop() during an in-flight start() was silently dropped (_active set only after the await), leaving the service running in the foreground. start/stop now use a cycle counter; a late start against a newer stop re-stops the native side. - stop() is unconditional: Dart's view of native state can diverge (hot restart, engine reattach); an always-delivered no-op stop is the only recovery path. - The gate counted constructed clients, not live sockets. The turn application controller holds one client per connection for the whole app lifetime, so after the first chat the service armed on every backgrounding with zero traffic. Replaced with hasLiveSocket, which checks for an open or reconnecting socket. - Lifecycle policy moved to ConnectionKeepAliveService.onAppLifecycleState so it is testable without pumping the app. Tests: race, lifecycle-policy, and hasLiveSocket suites added (each red before the fix, mutation-checked after); refused-start/idempotency contracts updated to the unconditional-stop semantics. 1337/1337 pass.
rusty4444
left a comment
rusty4444
left a comment
There was a problem hiding this comment.
Thanks for the careful implementation and the existing race tests, @Thaeland. The current-head PR CI is green, and I ran flutter pub get, full flutter analyze --no-pub, and full flutter test --no-pub locally (all 1,337 existing tests passed). I can't merge this head yet:
-
Reproducible rapid-cycle race (
lib/core/services/connection_keepalive_service.dart:28-49). With nativestartstill pending, background → resume callsstop()→ background callsstart()again. The latter returns because_startingis true; the first start then sees a stale_cycleand stops the native service. The app is backgrounded butisActive == false, precisely when keep-alive is needed. I added a temporary isolated Flutter regression probe for this sequence: it failedExpected: true; Actual: falseon this head. Please retain the latest requested lifecycle state across an in-flight start, with a regression covering background → resume → background and a refused/stale first start. (The probe was removed from my worktree after verification.) -
Foreground-service scope needs a product decision before release.
DesktopGatewayClient.hasLiveSocketincludes an idle connected socket and an indefinitely retrying reconnect timer, so simply leaving any chat can run thisspecialUseservice with a persistent notification for hours, including a prolonged network outage, until the app resumes/detaches. Should background protection be opt-in persistent connectivity or bounded to an active user-visible turn? Please define a stop condition for idle/disconnected work and document the chosen behaviour. The manifest subtype is correctly declared, but Android says special-use cases are reviewed and Google Play's FGS criteria require work to run only as long as necessary and be user-perceptible. I am not asserting Play would reject this APK; I am flagging the policy/battery trade-off for Sam to decide.
I have not changed or pushed your commits. Once the race and service lifetime are resolved, I can re-run the full gates and merge with a true merge commit to preserve authorship.
…light start Rapid minimize -> resume -> minimize while the first native start is still pending lost the keep-alive entirely: the second start() was swallowed by the in-flight guard, and the first start then saw a stale cycle and stopped the service — leaving the app backgrounded with the foreground service down, exactly when it is needed. Dart now records the latest requested lifecycle state (_pendingStart): when an in-flight start lands stale and a background was requested after the racing stop, it retries the start instead of compensating down. stop() clears the retained request, so a later resume still ends the service stopped (pinned by new regression tests for stop-after- pending, stop-racing-the-retry, refused retry, and burst double-start). Also reset _starting in a finally: a malformed native reply raises TypeError out of the typed invoke, and a leaked _starting would wedge the service permanently now that the retry path depends on it. Closes review finding 1 on PR rusty4444#152 (rusty4444's probe: Expected true, Actual false).
Addresses review finding 2: a persistent foreground notification is only defensible while it protects user-visible work. The service now starts only when the app backgrounds with an unresolved chat turn and stops the moment that turn settles while backgrounded — an idle or merely-reconnecting connection never earns it. - GatewayTurnCoordinator/registry: hasActiveTurn aggregate plus a turn-activity listener channel (may-have-changed semantics). - GatewayTurnApplicationController: cross-session aggregation and listener forwarding. - ConnectionKeepAliveService: gate on hasActiveTurn instead of hasLiveSocket; onTurnActivityChanged stops/starts the service the instant turn state flips while backgrounded. - main.dart: wire the activity listener; drop the now-dead DesktopGatewayClient.hasLiveSocket registry and its test. - Notification text and specialUse subtype now describe the bounded, user-started work the service protects.
|
Both findings are addressed on the branch. Finding 1 (rapid-cycle race) — fixed in Finding 2 (foreground-service scope) — reworked in Verification: full suite 1349/1349 green, |
…, pinned races Fresh-context OMP audit over the branch found five items; all are addressed or dispositioned: - ConnectionKeepAliveService.kt: onTaskRemoved now calls stopSelf(). Dart's stop on `detached` is an async platform message that can lose to engine teardown; a swiped task could otherwise orphan the service with a permanent notification and no isolate left to stop it. - Kotlin doc and channel description now state the shipped turn-bounded policy instead of the deleted socket-gating design. - GatewayTurnApplicationController: dropped the write-only _activityForwarderTokens set and corrected the forwarder doc comment. - desktop_gateway_client: empty constructor body (comment moved out). - New pinned scenarios: a new turn starting during an in-flight stop (start must win), listener remove-then-re-add (exactly one forwarder, mutation-checked), and the app-wide-aggregate closure contract that main.dart relies on. Full suite 1351/1351, analyze --fatal-infos clean, compileDebugKotlin green.
|
The fixes also went through a fresh-context deep audit of the full branch diff. It surfaced five items, all now addressed in The one real bug was on the detach path: Dart's stop on The audit also caught a stale Kotlin doc comment still describing the old socket-based gating (a trap for future maintainers), a write-only forwarder-token set in the application controller, and a comment-only constructor body; all cleaned. Three unpinned races are now covered by tests: a new turn starting while a native stop is still in flight (the later start must win), listener remove-then-re-add yielding exactly one session forwarder (mutation-checked — removing the idempotence guard fails the test), and the app-wide-aggregate contract the app-root closure depends on (one session settling must not stop the service while another session's turn is unresolved). One flagged-for-confirmation item was investigated and not counted: a poisoned coordinator retaining a fail-closed state in Verification after the round: full suite 1351/1351, |
Why we bound the keep-alive to active turnsThe review asked us to justify the foreground-service lifetime, so here's how we landed on the boundary. We started from Google's rule: a persistent notification is only defensible while the service is doing work the user can perceive, and it should end when that work ends. So we asked what the unit of user-perceivable work actually is in this app. Three candidates: the connection, the process, the turn. Our first cut gated on a live socket, and that's what failed the review — and it fails Google's test too. We deliberately keep the gateway socket warm across long idle stretches, and an idle socket isn't work the user perceives. A notification saying "keeping the connection alive" while nothing is happening describes our infrastructure, not the user's work. That's exactly the pattern Play rejects, and for most users it would sit in the shade forever. So we picked the turn. When the user sends a message and waits for a reply, that's work they started, can see, and know when it ends — and it's exactly what the service exists to protect: a long reply that outlives minimization should keep streaming. The service now starts only when the app backgrounds with an unresolved turn, and stops the moment that turn settles in the background instead of waiting for resume. The notification reads "Finishing your request" — the work itself, not our plumbing. What we gave up is small. The only cost of ending the service when a turn settles is that the idle warm socket may drop while the app sits backgrounded between turns. We can live with that: the transport reconnects on resume, so the user pays at most one reconnect when they come back — versus a permanent notification and a foreground-priority-pinned process for hours under the socket-based gate. Protecting real work is worth a notification; protecting an idle socket never is. That asymmetry is the whole reason we picked this boundary. We've also folded Google's Android foreground-service rules into the fix itself — the per-turn bound, the work-describing notification, the |
rusty4444
left a comment
rusty4444
left a comment
There was a problem hiding this comment.
Thanks for the thorough follow-up, @Thaeland. The rapid-cycle fix, turn-bounded intent and onTaskRemoved backstop address the previous review. At this head, PR CI is green; I also ran flutter pub get, full flutter analyze --no-pub (no issues), and full flutter test --no-pub (1,351 passed). Two current-head gaps still block a merge:
-
The service does not protect the normal stock-gateway or existing-chat path. The app-root gate reads only
GatewayTurnApplicationController.hasActiveTurn(lib/main.dart:305-310).ChatScreencreates that application session only for local drafts (lib/core/screens/chat_screen.dart:67-72,585-590), and stock gateways fall back to_sendDesktopGatewayMessage(:1716-1729,2597-2611). These user-started turns never enter the controller, so backgrounding reports no active turn and never starts the service. Please cover the actual legacy/stock-gateway turn state (including stored chats), with a regression that backgrounds during one of those turns. -
A disconnected recovery-v2 turn can pin the service without a live socket.
GatewayTurnCoordinator._markTransportLostclears_clientbut retains nonterminal turns in_states(lib/core/services/gateway_turn_coordinator.dart:1311-1353), whilehasActiveTurnis just_states.isNotEmpty(:525-528). Backgrounding then starts/keeps the foreground service (lib/core/services/connection_keepalive_service.dart:102-133), yet the turn can finish server-side with no event to clear the state or stop the notification until the user resumes. Please make the service stop or remain bounded when the socket is lost, and test transport loss during a backgrounded turn. Do not auto-resend an uncertain turn.
I have not changed your branch. Keeping this open under changes requested; no release or merge until these paths are resolved and the gates are rerun.
The turn-bounded keep-alive keyed its active-turn signal on the recovery-v2 coordinator, which stock gateways never enter: every stock send and every stored chat submits through _sendDesktopGatewayMessage, so backgrounding mid-reply started no foreground service on exactly the gateway the app ships against. GatewayTurnApplicationController now tracks legacy sends via beginLegacyTurn/endLegacyTurn tokens (per-send, idempotent release, inert after close), and the chat screen registers the whole legacy send — upload through settle — around its existing try/catch. Regression: a stock-path widget test backgrounds during an in-flight send (held Completer), asserts the service starts, and that the settle while backgrounded ends it; a throwing session factory pins that the stock path never opens a recovery-v2 session.
A disconnected recovery-v2 turn stayed in _states, so a backgrounded app kept the foreground service alive on a dead socket until resume — protecting a connection that can deliver nothing (reconcile runs on resume; nothing auto-resends). hasActiveTurn now requires a live transport, and both transport-loss paths notify even when no state changed (an already-uncertain turn would otherwise pin the service). The turn itself stays journaled for reconcile-on-resume; only the keep-alive signal is bounded. Regression: registry test drives a real socket close and asserts the aggregate drops and the activity listener observes false — the backgrounded-stop half is already pinned in connection_keepalive_service_test (false while backgrounded stops the service), and the main.dart aggregate recompute is pinned in the application-controller suite.
…fy, pinned gaps Independent review of dece19f/74909b7 found the stock-gateway threading was still incomplete and one silent-flip path: - CronScreen built its run-transcript ChatScreen without the application controller, so a follow-up send inside a cron run transcript (an ordinary session on the stock transport) never earned the keep-alive. Threaded through both CronScreen call sites; audited every ChatScreen construction site — all now pass the controller. - GatewayTurnApplicationController.close() cleared legacy turns without notifying, so a backgrounded service started for an in-flight turn could miss its stop through the notify path (latent: today's only caller stops the service first, but the contract must not depend on caller ordering). close() now emits the true→false flip. - Pinned the reviewer's test gaps: close-interaction contract (begin after close inert, end after close never throws, listeners see the flip); a throwing stock-gateway send while backgrounded releases the service; transport loss over an already-uncertain (interrupt-marked) turn still drops the signal — the no-state-change branch the commit-2 notify justified but did not exercise. Mutation-checked: removing the close-notify reddens the close-contract test. Full suite 1356 green, analyze clean.
|
You were right on both counts, and I owe you an apology: I built the keep-alive around the recovery-v2 path without ever stopping to ask whether a stock gateway even walks that road. It doesn't. Every stock send goes through the legacy transport, so what I shipped was protecting a session shape the app never actually uses in production. I anchored on the path I was testing against instead of the gateway the app ships against, and that's on me. I've pushed three fixes:
Full suite is 1356 green with analyze clean, and each new behavior is mutation-checked: I revert the production line and watch its test go red before restoring. Sorry again for making you find the stock-gateway hole. The rule is written down now: features have to earn their keep on the stock transport, not just the recovery path. |
…s before close notification
rusty4444
left a comment
rusty4444
left a comment
There was a problem hiding this comment.
Thanks for the careful follow-up, @Thaeland. The new stock-gateway and stored-chat turn tracking, cron-chat wiring, and transport-loss stop path address the earlier blockers. I added a maintainer follow-up commit for two shutdown edges: native service stop when Back finishes the Activity (in addition to onTaskRemoved), and a true→false controller notification when legacy and recovery turns coexist at close. Your original commits remain intact.
At this head, PR CI passed. Locally: flutter pub get, full flutter analyze --no-pub, 1,357 Flutter tests, and a debug APK build all passed. The combined tree with PR #156 passed analysis and 1,364 tests. Android device lifecycle behaviour is not device-verified by this automation.
Approved for an authorship-preserving merge commit.
|
I confirm that the code and other original material I authored and contributed to rusty4444/hermes-android may be distributed under the MIT License. Copyright in my contributions remains with me. This confirmation does not cover third-party material. |
2 participants
The problem
My Hermes Android app kept dropping the desktop gateway connection every time I minimized it. The reconnect-on-resume logic already existed, but the drop itself was the bug: when Android freezes or kills a cached process, the Dart
gateway.pingheartbeat timer stops with it, and the gateway tears the socket down at its 45-second silence deadline. By the time I came back to the app, the connection was already gone.What I did
I added a client-side keep-alive that keeps the process alive while the app is backgrounded, so the heartbeat timers keep ticking and every socket stays under the silence deadline. The gateway itself is untouched — this rides entirely on stock behavior.
On the Android side there's a new
ConnectionKeepAliveService, aspecialUseforeground service (with the subtype property required on targetSdk 34+) started and stopped over a smallkeepaliveMethodChannel onMainActivity. It usesSTART_NOT_STICKYand a quietIMPORTANCE_LOWnotification channel, and a refused start (Android 12+ background-start limits) returnsfalseso Dart treats it as a silent no-op rather than an error.On the Dart side
ConnectionKeepAliveServiceowns the whole policy in one testable method,onAppLifecycleState(state, hasLiveSocket:), which the app-root lifecycle observer delegates to. The gate isDesktopGatewayClient.hasLiveSocket— a static check over live clients for an open or reconnecting socket. Gating on live sockets rather than constructed clients matters: the turn application controller holds one client per connection for the whole app lifetime, so a client-count gate would arm the service on every backgrounding even with zero traffic. The service starts only when I background with a live or reconnecting socket, and stops on resume, ondetached(after detach no Dart isolate survives to stop it, so an orphaned service would pin the process forever), and indispose().stop()is unconditional so no state divergence can wedge it, and a cycle counter handles a stop landing while a start is still in flight.This works for any number of concurrent sessions: one service pins the process, every session's heartbeat is its own timer inside its own
WsClient, and the gate is ananyover live clients.How I verified it
I ran a deep audit of the feature with an independent reviewer pass alongside my own, and every finding is fixed in the second commit: a back-exit path that orphaned the service with a permanent notification, a missed-stop race that could strand the notification, an unrecoverable stop guard, and the client-count gate described above. Each fix had a failing test written first, and I mutation-checked the key ones (reverting the detached handling turns the suite red). The full suite is 1337/1337 green and
flutter analyze --fatal-infosis clean. I also built and installed the release APK and confirmed in real use that minimizing no longer drops the connection.