Desktop activation publishes atomically and fails closed (salvage #82187 store hardening) - #89483
Merged
Merged
Conversation
…mically on a profile switch ensureGatewayProfile used to activate the target gateway and set $activeGatewayProfile while the connection descriptor fetch was still in flight, so during that window $gateway already targeted the new backend while $connection still described the previous one, and any request or plugin mode-listener firing then announced the wrong mode to the new backend. A failed descriptor fetch made the mismatch permanent. prepareGatewayForProfile (new gateway-store seam) opens the socket and returns a synchronous activation thunk without publishing anything; ensureGatewayForProfile now delegates to it. The switch resolves the descriptor and opens the socket first, then flips the active gateway, the profile atom, and $connection in one synchronous frame. A descriptor failure aborts the switch as a unit: nothing is published and every atom still consistently describes the previous profile. The deferred-descriptor test holds the fetch open and asserts the public atoms never disagree, then releases it and asserts all three flipped together; the failure test asserts no partial publication.
`ensureGatewayAgent` is the (connectionId, profile) door the SDK's `ensureAgent`
goes through, and it landed on main after the profile path was made atomic. It
published in the order the profile path used to:
await ensureGatewayForAgent(connection, target) // $gateway flips here
$activeGatewayProfile.set(target)
await syncConnectionToActiveAgent(connection, target) // $connection here
The trailing await is the same mixed-state window: $gateway and
$activeGatewayProfile already name the agent's backend while $connection still
describes the previous one, so any request or plugin mode-listener firing in
that window announces the wrong mode to the new backend.
Both doors now share one seam:
* `prepareGatewayForAgent` mirrors `prepareGatewayForProfile`: dial the socket,
publish nothing, return the synchronous activation thunk. A local/null
connection falls through to the profile seam, so the two paths cannot drift.
`ensureGatewayForAgent` becomes `(await prepareGatewayForAgent(...))()`,
exactly how `ensureGatewayForProfile` relates to its own prepare.
* `syncConnectionToActiveAgent` splits into `resolveConnectionForActiveAgent`,
which resolves only. `ensureGatewayAgent` resolves the descriptor and dials
the socket concurrently, then activates, moves the profile pointer and sets
the descriptor with no awaits between them.
The best-effort contract on this path is unchanged on purpose: a descriptor
lookup that fails still leaves the previous `$connection` in place rather than
aborting the switch, which is what the profile path does instead. That
difference is deliberate and called out for review rather than quietly
harmonised.
Tests: `profile-agent-activation.test.ts` gains
`never publishes the agent gateway before its connection descriptor`, the mirror
of the profile-path test, asserting a pending `getConnectionFor` leaves all
three atoms on the old backend and that they flip together once it resolves. The
existing mutex and resync tests move onto the prepare/publish mocks, which also
repairs them: that file mocked `@/store/gateway` without `prepareGatewayForProfile`,
so its profile-path cases called an undefined mock after the rebase.
… rejects Review caught that the agent path fixed the pending-descriptor race but not the failure path. `resolveConnectionForActiveAgent` caught a `getConnectionFor` rejection and returned null, so `Promise.all` resolved as `[null, activate]` and the switch published anyway: the activation thunk ran and `$activeGatewayProfile` advanced, while only `setConnection` was skipped. That is the same mixed state this PR exists to remove, except it does not close on its own. The pending-descriptor window ends when the descriptor arrives; a failed lookup never arrives, so `$gateway` named the new backend while `$connection` described the old one until an unrelated reconnect or switch happened to repair it. Anything branching on connection mode in between (plugins, `MEDIA:`, `/api/fs/*`, `/api/media`, image attach) saw the pair disagree. Let the rejection propagate, matching `resolveConnectionForProfile`, whose contract is already exactly this: null means "no desktop bridge" and nothing else, and a bridge rejection aborts the whole switch before anything is published. Both doors now fail closed identically, and the caller can retry. The existing "leaves the prior connection intact when the descriptor fetch fails" test asserted the old best-effort behaviour, so it pinned the defect rather than a contract worth keeping. Replaced with a rejected-descriptor test that asserts none of the three atoms moved and the activation thunk was never called. The pending-descriptor case keeps its own separate test, so the success and failure contracts are pinned independently. Also reworded the publication comments: these are sequential atom writes with no asynchronous gap between them, not a transaction, and describing them as one "frame" overstated the guarantee.
The prepare/publish seam removed the *await* between activating the gateway and setting the profile pointer and connection descriptor, but not the *notification* gap. Nanostores drains a store's listeners synchronously inside .set(), so three sequential sets still let a $gateway listener run while $activeGatewayProfile and $connection named the previous backend. That is the same mixed state the seam exists to prevent, just narrowed from an async window to a synchronous one, and it is worse to debug because it is invisible in an await-shaped reading of the code. batch() defers every notification to the end of the callback, so the three become one observable transition on both the profile path and the agent path. Pinned with a test that attaches a real $gateway listener and asserts the companions are already current in the first callback; the mock thunks now publish distinct gateway identities so an out-of-order publication cannot pass unnoticed, and three existing tests assert $gateway is still the ORIGINAL object (by identity) on every path that must publish nothing.
The agent path already declined to publish when applyActive() rejected its activation, but the profile path discarded the same boolean and published unconditionally. applyActive() returns false when its captured epoch has been superseded, which happens whenever a newer switch or a teardown lands while this preparation is still awaiting its route or socket. The result was not a torn publication. batch() makes those writes observer-atomic either way. It was something subtler: ONE complete, internally inconsistent tuple, the CURRENT gateway paired with the stale target's profile pointer and descriptor. Atomicity cannot make a rejected activation correct, so the caller has to decline to publish at all. prepareGatewayForProfile now returns Promise<() => boolean> like its agent counterpart. The primary and shared-primary thunks return applyActive() directly; the secondary thunk reports whether the prepared entry was still current AND the epoch was accepted, keeping the descriptor publish conditional on having a cached connection so an accepted activation with no descriptor still moves the companions. prepareGatewayForAgent's genuinely-local fallthrough now returns the profile thunk unchanged instead of wrapping it to return an unconditional true, which had been reporting a rejected activation to the agent caller as a successful one. Two regressions on the profile door: a superseded activation leaves all three stores on the existing complete route with no subscriber notified at all, and an accepted one still publishes, so a thunk that always reported false could not pass. The mock thunks in profile.test.ts now return true, since a bare vi.fn() returns undefined and would read as "superseded".
The comment above ensureGatewayAgent carried both the old and the new contract on consecutive lines: "a local/null connectionId falls through to the profile path verbatim", immediately contradicted by "only a null connectionId falls through, explicit local is a registry identity". Dropped the stale line. Same wording above prepareGatewayForAgent in gateway.ts, tightened to match what the code actually does: registryBackendScopeKey only collapses to the bare profile key for a null or empty id, so an explicit local id scopes to conn:local::<profile> and stays on the registry route. Comments only, no behavior change. tsc --noEmit, eslint and the three affected suites (43 passed) re-verified. Refs #82140
15 tasks
Contributor
૮ >ﻌ< ა ci reviewran on 48b6179 — docs(desktop): state one fallthrough contract at the agent s
|
alt-glitch
added
type/bug
This was referenced Aug 19, 2026
This was referenced Aug 19, 2026
teknium1
added a commit
that referenced
this pull request
Aug 19, 2026
… mid-dial entries (#89622, #46651) Re-lands the goals of the reverted atomic-publish series (#89483, reverted in #89785) without the fail-closed decline path that killed profile clicks, and fixes the underlying pruner race the original series exposed. Two changes relative to the restored (pre-series) behavior: - Publication is atomic and mode-safe (#46651): the switch resolves the target's connection descriptor CONCURRENTLY with the socket work and publishes $activeGatewayProfile + $connection in one nanostores batch() frame, so no request or plugin mode-listener observes the new gateway beside the previous profile's descriptor. Unlike the reverted series, a failed descriptor lookup fails OPEN — the switch still lands, the previous descriptor stays, boot/reconnect resyncs it later, and the failure is logged instead of swallowed. - The live-work pruner can no longer kill a switch mid-dial (#89622's root race): ensureGatewayForProfile / ensureGatewayForAgent lease their entry (activationLeaseUntil, 30s bound) for the duration of the dial and pruneSecondaryGateways spares leased entries. A switch target is not yet active, has no live sessions and holds no request lease, so any prune recompute during a cold pool spawn used to dispose the dialing entry. Bounded lease: an orphaned one self-expires. The agent door logs a non-landing activation (target removed mid-dial) instead of resolving silently, but never fails the switch closed. Live E2E (Electron over CDP, 3 local profiles, cold + warm): 10 sequential switches, rapid two-click interleave (last click wins), 6-click stress run — all land, overlay always clears, fresh pool backends spawn on cold switches. Tests: gateway-activation-prune-lease suite (mid-dial prune survival on both doors, lease release, lease expiry); the invalidated-registry-identity pin updated for the concurrent (read-only) descriptor probe.
This was referenced Aug 19, 2026
lisajlau
pushed a commit
to lisajlau/hermes-agent
that referenced
this pull request
Aug 20, 2026
… mid-dial entries (NousResearch#89622, NousResearch#46651) Re-lands the goals of the reverted atomic-publish series (NousResearch#89483, reverted in NousResearch#89785) without the fail-closed decline path that killed profile clicks, and fixes the underlying pruner race the original series exposed. Two changes relative to the restored (pre-series) behavior: - Publication is atomic and mode-safe (NousResearch#46651): the switch resolves the target's connection descriptor CONCURRENTLY with the socket work and publishes $activeGatewayProfile + $connection in one nanostores batch() frame, so no request or plugin mode-listener observes the new gateway beside the previous profile's descriptor. Unlike the reverted series, a failed descriptor lookup fails OPEN — the switch still lands, the previous descriptor stays, boot/reconnect resyncs it later, and the failure is logged instead of swallowed. - The live-work pruner can no longer kill a switch mid-dial (NousResearch#89622's root race): ensureGatewayForProfile / ensureGatewayForAgent lease their entry (activationLeaseUntil, 30s bound) for the duration of the dial and pruneSecondaryGateways spares leased entries. A switch target is not yet active, has no live sessions and holds no request lease, so any prune recompute during a cold pool spawn used to dispose the dialing entry. Bounded lease: an orphaned one self-expires. The agent door logs a non-landing activation (target removed mid-dial) instead of resolving silently, but never fails the switch closed. Live E2E (Electron over CDP, 3 local profiles, cold + warm): 10 sequential switches, rapid two-click interleave (last click wins), 6-click stress run — all land, overlay always clears, fresh pool backends spawn on cold switches. Tests: gateway-activation-prune-lease suite (mid-dial prune survival on both doors, lease release, lease expiry); the invalidated-registry-identity pin updated for the concurrent (read-only) descriptor probe.
Contributor
The prepare/publish seam and
|
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
4 participants
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.
Summary
Desktop profile/agent switches now publish gateway, profile, and connection state atomically and fail closed — a rejected descriptor lookup no longer lets the UI advance to a complete-but-false description of the active route. Salvaged from PR #82187 (@jackulau): these seven store-hardening commits were the durable output of @andrexibiza's review of that PR and stand on their own; the connection-mode API they rode alongside was superseded by view-time file-link resolution (#89472).
Changes (all @jackulau's commits, cherry-picked with authorship; clean apply on current main, zero connection-mode coupling)
store/profile.ts/store/gateway.ts: profile switch publishes gateway + profile + connection descriptor in onebatch(); agent activation publishes atomically too; agetConnectionForrejection fails the switch closed (no activation thunk, no store writes) instead of silently returning null; profile publication is guarded on the activation result symmetrically with the agent path.store/profile-agent-activation.test.ts/store/profile.test.ts: regressions observe the stores from inside a gateway listener (real$gatewayupdate), covering torn-publication and fail-open shapes.prepareGatewayForAgentsignature-width refactor + one docs comment.Validation
tsc -p .vitest run src/store/(profile, activation, gateway suites)Credit: @jackulau (author, 7 commits preserved via cherry-pick), @andrexibiza (review that surfaced both bugs).
Infographic