Skip to content

fix(desktop): require an open socket before publishing a secondary gateway route - #92373

Closed
ygd58 wants to merge 1 commit into
NousResearch:mainfrom
ygd58:fix/desktop-secondary-gateway-open-check
Closed

ygd58 wants to merge 1 commit into
NousResearch:mainfrom
ygd58:fix/desktop-secondary-gateway-open-check

Conversation

@ygd58

@ygd58 ygd58 commented Aug 22, 2026

Copy link
Copy Markdown
Contributor

Fixes #92265 (proposed fix #2; #1 and #4 are separate follow-ups, see below).

Root cause

ensureGatewayForAgent()/ensureGatewayForProfile() decided activation success by checking Boolean(entry.connection) alone. entry.connection is set in openSecondary() before the WebSocket dial completes, so a transient first-dial failure -- caught and left for scheduleReconnect's backoff -- still left entry.connection truthy. Both functions then published the closed socket as the active route, and the next chat RPC failed with "Hermes gateway is not connected".

Fix

Added an isOpen(entry.gateway) check alongside the existing check in both functions, gating both applyActive() (switches g.activeKey, publishes $gateway) and publishActiveConnection() on the socket having actually reached open. A failed first dial now correctly returns false / leaves the previous active route untouched, matching option 3 from the issue's proposed fix.

Not implemented (separate follow-ups): proposed fix #1 (bounded reconnect before returning status) is a larger behavioral change left to a separate PR; proposed fix #4 (Bot Mode's own connection-ID-only guard) is unnecessary at the root now since host.ensureAgent() calls into these now-fixed functions, though additional hardening there may still be worthwhile separately.

Found while verifying

An existing test in gateway-shared-remote.test.ts encoded the exact bug as EXPECTED behavior (asserting setConnection was called once after a single call whose first dial failed). Rewrote it to assert the corrected contract: the failed attempt doesn't publish, and a realistic retry succeeds once the second dial goes through.

Verification

Added a new test file covering both functions. Verified as genuine regressions by reverting both isOpen() checks and confirming 2/4 new tests fail with exactly the reported symptom.

44/44 pass across all 9 gateway-related test files (no regression).

…teway route

Fixes NousResearch#92265 (proposed fix NousResearch#2; #1 and NousResearch#4 are separate follow-ups, see below).

ensureGatewayForAgent() and ensureGatewayForProfile() both decided
whether a secondary activation "succeeded" by checking Boolean(entry.connection)
alone. entry.connection is set in openSecondary() BEFORE the WebSocket
dial completes (`entry.connection = conn` happens ahead of
`await entry.gateway.connect(wsUrl)`), so a transient first-dial
failure -- caught by the surrounding try/catch and left for
scheduleReconnect's backoff retry -- still left entry.connection
truthy. Both functions then treated this as a successful activation:
applyActive() switched g.activeKey and published $gateway to the
closed socket, and publishActiveConnection() pushed the connection
descriptor to the UI. The next chat RPC then failed with "Hermes
gateway is not connected" against a route the user/desktop believed
was live.

Added an isOpen(entry.gateway) check alongside the existing
Boolean(entry.connection) check in both functions' activation/publish
conditions, gating BOTH applyActive() (which switches g.activeKey and
publishes $gateway) and publishActiveConnection() (which pushes the
connection descriptor) on the socket having actually reached 'open'.
A failed first dial now correctly returns false / leaves the previous
active route untouched, matching option 3 from the issue's own
proposed fix ("if both bounded attempts fail, keep the existing
active route") -- the existing scheduleReconnect backoff still owns
recovery for that entry going forward.

Not implemented in this PR (separate, lower-priority follow-ups):
- Proposed fix #1 (one immediate bounded reconnect attempt before
  returning activation status) -- a larger behavioral change with its
  own retry/timing tradeoffs; left to a separate PR.
- Proposed fix NousResearch#4 (Bot Mode's own connection-ID-only guard in
  plugins/hermes-bots/plugin.js) -- host.ensureAgent() calls into the
  now-fixed gateway.ts functions, so this class of bug is already
  closed at the root; Bot Mode's own additional profile/state
  verification may still be worth adding but is a separate, narrower
  hardening pass on top of this fix.

Found and fixed a genuine test-suite inconsistency while verifying:
the existing "refreshes the active connection after a pooled profile
reconnect succeeds" test in gateway-shared-remote.test.ts asserted
setConnection was called once after a SINGLE ensureGatewayForProfile()
call whose first dial failed -- i.e. it encoded the exact bug this
issue reports as the EXPECTED, correct behavior. Rewrote it to assert
the corrected contract: the failed first attempt does not call
setConnection at all, and a realistic retry (calling
ensureGatewayForProfile() again, since g.activeKey correctly never
left the primary after the failed attempt -- ensureActiveGatewayOpen()
is for reconnecting an already-active gateway that went stale, not
retrying an activation that never succeeded) succeeds and publishes
once the second dial goes through.

Added a new test file (gateway-secondary-open-check.test.ts) following
the established mocking pattern from gateway-agent-scope.test.ts,
covering both ensureGatewayForAgent and ensureGatewayForProfile: a
transient first-dial failure does not activate/publish (the exact
reported symptom), and a successful dial still activates/publishes
normally (sanity, no regression to the happy path). Verified as
genuine regressions by reverting both isOpen() checks and confirming
2 of 4 new tests fail with exactly the reported symptom (activated
resolves true / the primary gets replaced despite the failed dial).

44/44 pass across all 9 gateway-related test files (no regression).
@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 Aug 22, 2026
@Enough1122

Copy link
Copy Markdown
Contributor

AI code review — automated review for reference, author can ignore or act on any point.

Reviewed apps/desktop/src/store/gateway.ts (+186/-8) and the new gateway-secondary-open-check.test.ts.

What's good

  • Correct root cause and minimal fix: entry.connection is assigned before the dial resolves in openSecondary, so truthiness of the descriptor could never prove the socket opened. Adding isOpen(entry.gateway) at both publish sites (ensureGatewayForAgent and ensureGatewayForProfile) fixes the class, not just the reported path — and the comment explains why the descriptor exists yet the socket is closed.
  • The regression test is built the right way: its HermesGateway mock propagates the dial outcome (unlike other suites' always-succeeds mocks), letting the test simulate ECONNRESET-before-open and assert the closed gateway is not published while reconnect stays scheduled.
  • Failure mode after the fix degrades correctly: the failed activation is left to scheduleReconnect instead of publishing a route whose next RPC would fail with "gateway is not connected" — trading a wrong-fast switch for an honest retry.

Suggestions

  1. The test asserts the route isn't published on failed first dial; consider also pinning the recovery half — a second call after the mock succeeds should publish — so a future over-tightening of isOpen can't turn this into "never publishes on transient failure".
  2. Nit: both hunks order the new predicate differently relative to applyActive (before in one, after in the other). Behaviorally irrelevant since all terms are cheap booleans, but consistent ordering reads better.
  3. Consider surfacing a one-line UI/console note when activation is suppressed by a not-yet-open socket — silent non-publication plus scheduled reconnect can look like a hang from the user's side.

Precise, well-tested race fix.

@teknium1

Copy link
Copy Markdown
Collaborator

Merged via #95082 at b52b05c — your commit was cherry-picked into the consolidated salvage PR with authorship preserved, so this work carries your name in git history.

Thanks for requiring an open socket before publishing a secondary gateway route — the tightest-scoped fix of the #92265 swarm, which is why it was the one we took.

Closing this original PR now that the consolidated branch has landed on main. Thanks for contributing to the desktop multi-gateway campaign (tracker: #94724).

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.

[Bug]: Desktop can publish a closed secondary gateway after a transient WebSocket reset

4 participants