Skip to content

fix(connection): tolerate a slow-but-alive local backend + heap headroom (#21) - #25

Merged
radroid merged 1 commit into
mainfrom
t3x/issue-21-ws-keepalive-reconnect-heap
Jul 27, 2026
Merged

fix(connection): tolerate a slow-but-alive local backend + heap headroom (#21)#25
radroid merged 1 commit into
mainfrom
t3x/issue-21-ws-keepalive-reconnect-heap

Conversation

@radroid

@radroid radroid commented Jul 27, 2026

Copy link
Copy Markdown
Owner

What & why

Fixes #21 — on a memory-constrained host the desktop app's local ("This device") backend intermittently shows <machine> is not connected / "reconnecting…" and is slow to recover, even though the backend process stays alive. This addresses the app-side robustness gaps.

While mapping the code I confirmed the issue's root-cause analysis with one important refinement: the transport is Effect's RpcServer/RpcClient over WebSocket (not a raw ws server), and the RPC client already sends its own ~5s ping and fails a silent socket on ping-timeout. So client→server dead-socket detection already exists. The genuine, symptom-causing gap is that the reconnect supervisor is too impatient: under host contention the backend answers slowly, and every attempt is abandoned at a fixed 15s budget and then backed off — so a briefly slow but alive backend is repeatedly given up on. That's what this PR fixes.

Changes

1. Client: tolerant / adaptive reconnect + background heartbeat — the core fix

packages/client-runtime/src/connection/supervisor.ts

  • Slow vs. dead. Each attempt records the furthest ConnectionAttemptStage it reached. A setup timeout that reached the backend (opening/synchronizing) is treated as slow, not dead: the next attempt gets an enlarged 45s establishment budget (vs 15s) and retries after a fixed 1s instead of climbing the exponential ladder. A timeout that never reached the backend keeps the existing 15s budget + exponential backoff. Every non-timeout failure (transport/network/etc.) is unchanged.
  • Bounded. The tolerant path is capped at MAX_TOLERANT_TIMEOUTS = 3 consecutive slow timeouts, then falls back to the normal budget + exponential backoff. (The stages are reported before the socket is confirmed open, so a genuinely unreachable endpoint could be misclassified as "slow" — the cap ensures it can't loop on 45s attempts forever and still surfaces backoff.)
  • Background heartbeat. While connected, the supervisor probes the backend every 30s, not only on foreground activation, so a wedged-but-open socket is detected proactively. The existing foreground-probe path was refactored into a shared runLivenessProbe helper — which now also handles a relay credentials-changed that lands mid-probe (previously dropped until the probe finished).

2. Backend: V8 heap headroom

apps/desktop/src/backend/DesktopBackendConfiguration.ts

  • Sets --max-old-space-size=4096 — appended to NODE_OPTIONS for the primary (Electron-run-as-node, which honours the NODE_OPTIONS safelist), preserved if an operator already set one; passed as a V8 execArg for the WSL backend.
  • Honest caveat: the issue's own telemetry (steady-state RSS 400–575 MB, no OOM) shows the backend is not heap-bound today, so this is headroom for transient spikes (large diffs / big file reads), not the primary fix for the reported symptom. It's a ceiling, not a reservation, so it doesn't raise steady-state memory use.

Deliberately deferred (documented, not dropped)

Server-side WS keep-alive / idleTimeout (issue's proposed fix #1). The Effect beta's RpcServer.toHttpEffectWebsocket exposes no idle/ping option, and neither the Bun nor Node platform wrappers surface a WebSocket idleTimeout/ping-interval — a correct server heartbeat would need library/platform plumbing (or a wrapper around the upgraded socket). Given:

  • the RPC client already provides client→server liveness (the 5s pinger), and
  • a healthy client's socket close already tears down the server session,

…the server-side change is low marginal value for this symptom and too risky to land autonomously on the critical transport. The safest future approach is TCP keep-alive on the upgraded socket (Node backend). Happy to do it as a separate PR if you want it.

Tests

  • supervisor.test.ts (+9 tests): enlarged budget after a slow timeout; fast non-escalating retry; a slow backend connecting within the enlarged budget; the opening-stage boundary; the negative branch (a preparing-stage timeout stays on the 15s budget + exponential backoff); the tolerant cap falling back to exponential; the periodic background heartbeat; heartbeat-timeout → reconnect; relay credentials-change during a background probe. Full client-runtime suite: 479 passing.
  • DesktopBackendConfiguration.test.ts: primary NODE_OPTIONS heap flag, WSL execArg, and NODE_OPTIONS empty/append/preserve branches. Desktop backend suite: 34 passing.
  • typecheck + lint clean; formatting applied.

Review

This was put through an adversarial multi-lens review (concurrency, reconnect logic, backend config, test rigor) with per-finding verification. Confirmed findings were addressed: the tolerant-path cap (misclassified-unreachable-as-slow), the mid-probe credentials-change handling, and several test-coverage gaps (negative branch, opening boundary, hermetic NODE_OPTIONS). The "4096 MB is miscalibrated" concern was refuted on verification (it's a ceiling, applied correctly). Ready for your review — I have not merged.

Reviewer notes / decisions to sanity-check

  • Constants: SLOW_CONNECTION_ESTABLISHMENT_TIMEOUT=45s, SLOW_RETRY_DELAY_MS=1s, MAX_TOLERANT_TIMEOUTS=3, CONNECTION_HEARTBEAT_INTERVAL=30s, DESKTOP_BACKEND_MAX_OLD_SPACE_MB=4096.
  • Branch is rebased onto the latest main.

🤖 Generated with Claude Code

Under host memory pressure the local ("This device") backend answers
slowly, and the reconnect supervisor abandoned each attempt at a fixed
15s budget and then backed off — repeatedly giving up on a
briefly-slow-but-alive backend (issue #21).

Client (packages/client-runtime/src/connection/supervisor.ts):
- Classify a setup timeout that reached the backend (stage
  opening/synchronizing) as "slow, not dead": the next attempt gets a
  45s establishment budget and a fast 1s retry instead of climbing the
  exponential ladder. Timeouts that never reached the backend keep the
  existing 15s budget + exponential backoff.
- Bound the tolerant path to MAX_TOLERANT_TIMEOUTS (3) consecutive slow
  timeouts, then fall back to normal backoff — the stages are reported
  before the socket is confirmed open, so a genuinely unreachable
  endpoint must not loop on 45s attempts forever.
- Add a 30s background liveness probe (monitorConnectedLease) so a
  wedged-but-open socket is detected without waiting for foreground
  activation; the foreground probe was refactored into a shared helper
  that also handles a relay credentials-change arriving mid-probe.

Desktop (apps/desktop/src/backend/DesktopBackendConfiguration.ts):
- Give the backend --max-old-space-size=4096 heap headroom (appended to
  NODE_OPTIONS for the Electron-run-as-node primary, passed as a V8
  execArg for WSL).

Server-side WS keep-alive/idleTimeout is deferred (the Effect beta
transport exposes no idle option and the RPC client pinger already
provides client->server liveness) — documented in the PR.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@coderabbitai

coderabbitai Bot commented Jul 27, 2026

Copy link
Copy Markdown

Warning

Review limit reached

@radroid, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 55 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews.

How do review limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window.

Please refer docs for additional details.

Review details
⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: 24fb4df7-2727-409a-a90d-8d568bf1e076

📥 Commits

Reviewing files that changed from the base of the PR and between b7cec63 and 14779d1.

📒 Files selected for processing (4)
  • apps/desktop/src/backend/DesktopBackendConfiguration.test.ts
  • apps/desktop/src/backend/DesktopBackendConfiguration.ts
  • packages/client-runtime/src/connection/supervisor.test.ts
  • packages/client-runtime/src/connection/supervisor.ts
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch t3x/issue-21-ws-keepalive-reconnect-heap

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@radroid

radroid commented Jul 27, 2026

Copy link
Copy Markdown
Owner Author

@coderabbitai review

@coderabbitai

coderabbitai Bot commented Jul 27, 2026

Copy link
Copy Markdown
✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@radroid
radroid merged commit 1e10fc3 into main Jul 27, 2026
2 checks passed
@radroid
radroid deleted the t3x/issue-21-ws-keepalive-reconnect-heap branch July 27, 2026 15:28
radroid added a commit that referenced this pull request Jul 28, 2026
… drop

Follow-up to #25 (closes the last review nit on #21).

The adaptive reconnect budget added in #25 arms a "tolerant" 45s
establishment timeout after a backend-reached slow timeout. Three of the
four run-loop branches that step away from an attempt already reset that
budget, but the `network === "offline"` branch did not. A network drop is
not backend slowness, so carrying the enlarged budget across the outage
let the first attempt after the network returned wait 45s instead of the
normal 15s.

Reset `tolerant`/`consecutiveSlowTimeouts` in the offline branch (keeping
`failureCount` so backoff continuity survives a transient blip) and add a
regression test that arms the tolerant budget, blips offline→online, and
asserts the reconnect times out on the normal 15s budget.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Local "This device" backend drops under host memory pressure; needs WS keepalive + tolerant reconnect + backend heap headroom

1 participant