Skip to content

fix(desktop): retain background turn leases across reconnect - #105985

Open
frizikk wants to merge 1 commit into
NousResearch:mainfrom
frizikk:fix/desktop-background-turn-reconnect
Open

frizikk wants to merge 1 commit into
NousResearch:mainfrom
frizikk:fix/desktop-background-turn-reconnect

Conversation

@frizikk

@frizikk frizikk commented Sep 8, 2026

Copy link
Copy Markdown

What does this PR do?

Keeps a background secondary gateway alive across a transient WebSocket disconnect when its only remaining owner is an active-turn lease.

After a routed prompt.submit ACK, the per-request lease is released but the whole-turn lease must remain. Previously, the secondary's closed/error callback released that last lease, synchronously disposed the route, and set wantOpen = false before reconnect could be scheduled. The background session then received neither replayed outage events nor subsequent completion unless another consumer rescued the route.

Move orphaned-turn cleanup to actual secondary disposal. Transport reconnect retains the existing route and JSON-RPC replay state; explicit disposal disables reconnect and pending material-edit redial before releasing leases. Normal authoritative settlement remains unchanged.

Related Issue

Fixes #105983

Duplicate searches covered issues and open/closed PRs using desktop reconnect, secondary gateway, turn lease, and narrower mechanism terms. Adjacent changes inspected:

No exact competing fix found; mechanism searches repeated before publication.

Type of Change

  • 🐛 Bug fix (non-breaking change that fixes an issue)
  • ✨ New feature (non-breaking change that adds functionality)
  • 🔒 Security fix
  • 📝 Documentation update
  • ✅ Tests (adding or improving test coverage)
  • ♻️ Refactor (no behavior change)
  • 🎯 New skill (bundled or hub)

Changes Made

  • apps/desktop/src/store/gateway.ts: preserve active-turn leases on transport close/error; release them at actual disposal with reentrancy protection and cancelled pending redial.
  • apps/desktop/src/store/gateway-background-reconnect.test.ts: exercise the real request router, registry, HermesGateway and JSON-RPC client through an asynchronous mock WebSocket. Two parameterized behavior contracts cover automatic reconnect/event replay/foreground isolation, plus explicit close/removal/prune/permanent missing-connection cleanup and same-session reuse.
  • apps/desktop/src/store/session-request-router.test.ts: replace the old immediate-disconnect-release expectation with retention until explicit route close. Existing terminal ACK, authoritative settlement and pruning tests remain exercised.

No real remote gateway or model calls; no app/backend restart. Existing backoff policy is unchanged: this is ownership-lifetime repair, not a new retry-budget policy. Explicit teardown and permanent missing-connection failures stop retries, and authoritative session settlement releases an unpinned recovered route.

How to Test

  1. Submit on a non-selected registered remote without prewarming, selecting, or relay-pinning its gateway. Let prompt.submit ACK while the turn runs.
  2. Drop the remote WebSocket, produce an event during the outage, and allow automatic reconnect. The outage event must replay and later completion must reach the background fan-in without changing the foreground gateway.
  3. Explicitly close/remove/prune during reconnect (or remove the connection before the next lookup). There must be no later redial. Reusing the same session/route must acquire a fresh lease and settle normally.

Executed from the repository root with independently installed worktree dependencies:

NODE_ENV=development npm ci --include=dev
NODE_ENV=test NODE_OPTIONS='--max-old-space-size=8192 --localstorage-file=/tmp/hermes-h2-final-focused.json' npm --workspace apps/desktop exec -- vitest run --project ui src/store/gateway src/store/session-request-router.test.ts
NODE_ENV=test npm --workspace apps/desktop run typecheck
NODE_ENV=test npm --workspace apps/desktop exec -- eslint src/store/gateway.ts src/store/gateway-background-reconnect.test.ts src/store/session-request-router.test.ts --max-warnings 0
NODE_ENV=production NODE_OPTIONS='--max-old-space-size=8192' npm --workspace apps/desktop run build
NODE_ENV=test NODE_OPTIONS='--max-old-space-size=8192 --localstorage-file=/tmp/hermes-h2-final-full-storage.json' npm --workspace apps/desktop run test:ui
git diff --check HEAD^ HEAD

Checklist

Code

  • I've read the Contributing Guide and applicable AGENTS.md files.
  • My commit follows Conventional Commits.
  • I searched existing issues and open/closed PRs for duplicates.
  • This PR contains only related code and tests.
  • I've run pytest tests/ -q and all tests pass — not run; renderer-only TypeScript change. Relevant desktop checks were run; full UI has reproduced baseline failures detailed below.
  • I've added behavior tests, including a regression proven red on the pinned base.
  • Tested on Linux 7.2.2-1-cachyos, Node v22.23.2, npm 12.0.2.

Documentation & Housekeeping

  • Documentation: N/A, no new public behavior/configuration surface.
  • cli-config.yaml.example: N/A, no configuration changes.
  • CONTRIBUTING.md / AGENTS.md: N/A, no architecture/workflow changes.
  • Cross-platform impact considered: browser WebSocket/registry ownership only, no OS-specific APIs; macOS/Windows execution not performed.
  • Tool descriptions/schemas: N/A.

For New Skills

N/A.

Screenshots / Logs

Pinned base: b2aa855b626ff8688eb34b95c60ee8b6a4af3679.
Local commit: 29859645ab6ce656406282f4289d7bb16aec16f1.

  • Before implementation: original 3-case integration probe had 1 failed / 2 passed. Sole-turn-lease disconnect failed automatic redial, route retention, outage replay and completion delivery; retained and uninterrupted controls passed.
  • Final focused gateway/router suite: 137 passed, 16 files, including all 8 new integration cases.
  • Renderer/Electron/E2E typechecks: PASS.
  • Changed-file ESLint with zero warning budget: PASS.
  • Production renderer/Electron build and assert-dist-built: PASS. Vite reports native-config/dynamic-import/plugin timing warnings; build succeeds.
  • Diff whitespace check: PASS; commit worktree clean.
  • Final full UI suite: 6 failed / 7361 passed, 4 failed / 752 passed files.
  • Independently installed clean pinned-base worktree: 6 failed / 7353 passed, 4 failed / 751 passed files. All six failing test titles match exactly:
    • voice-prefs.test.ts: desktop toggle across refreshes; legacy preference migration.
    • billing/index.test.tsx: auto-refill bounds; polling/settled buy controls.
    • fallback-model.test.ts: localized omitted-character count (5,000 expectation).
    • use-prompt-actions/utils.test.ts: locale-specific thousands separators.
  • npm install reported 6 dependency advisories (2 moderate, 4 high); no dependency or lockfile changes made.

Local evidence: /tmp/hermes-h2-red.log, /tmp/hermes-h2-final-focused.log, /tmp/hermes-h2-final-typecheck.log, /tmp/hermes-h2-lint.log, /tmp/hermes-h2-build.log, /tmp/hermes-h2-final-full-ui.log, /tmp/hermes-h2-baseline-full-ui.log, /tmp/hermes-h2-duplicates.json, /tmp/hermes-h2-adjacent-prs.json.

@alt-glitch alt-glitch added type/bug Something isn't working P2 Medium — degraded but workaround exists comp/desktop Electron desktop app (apps/desktop/*) sweeper:risk-session-state Sweeper risk: may lose/corrupt/mis-associate session or context state labels Sep 8, 2026
@Enough1122

Copy link
Copy Markdown

AI code review — automated review for reference; please use your judgment.

PR 105985 — fix(desktop): retain background turn leases across reconnect

Transport drops no longer settle running turns: the whole-turn lease now keeps the route alive so reconnect can replay missed events (session.events.since) and deliver the terminal event. Lease release moved to explicit disposal.

  • apps/desktop/src/store/gateway.ts:322-333 — closed/error no longer calls releaseTurnLeasesForScope; reconnect is scheduled while wantOpen. Correct — the old code orphaned the turn.
  • gateway.ts:337-351 — disposeSecondary early-returns when already closed (idempotent) and releases leases on real teardown, with a re-entrancy note. The stale-closure reuse hazard (release fn bound to a disposed entry) is addressed by acquiring a fresh lease on resubmit — covered by the second test. Good.
  • gateway-background-reconnect.test.ts:179-261 — 4-way matrix (close / error+close / retained control / uninterrupted) plus cleanup-path coverage (close/remove/prune/missing-connection). Thorough; backoff-capped redial and replay assertions included.

Non-blocking: a dropped route is now held open (with backoff redial) until explicit disposal or terminal settlement — bounded by existing backoff caps, acceptable. No concerns.

@OutThisLife
OutThisLife force-pushed the fix/desktop-background-turn-reconnect branch from 04c9c9e to 27d5f8a Compare September 30, 2026 02:49

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

comp/desktop Electron desktop app (apps/desktop/*) P2 Medium — degraded but workaround exists sweeper:risk-session-state Sweeper risk: may lose/corrupt/mis-associate session or context state type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Desktop: sole background turn lease is lost on WebSocket disconnect, preventing reconnect

3 participants