fix(desktop): bound the shared resume flight and adopt its straggler - #96523
Open
JoaoMarcos44 wants to merge 1 commit into
Open
fix(desktop): bound the shared resume flight and adopt its straggler#96523JoaoMarcos44 wants to merge 1 commit into
JoaoMarcos44 wants to merge 1 commit into
Conversation
singleFlightSessionResume() shares one promise per stored session id across every surface that can recover a dead runtime, and the slot is only released by that promise's .finally(). An unsettled run() therefore makes the conversation unrecoverable for the life of the window: every later caller joins the dead flight instead of starting its own attempt, and the rejection-driven retry / Retry-UI ladder never arms because nothing ever rejects. The resume RPC itself is bounded (HermesGateway's 30s request budget), but run() bodies resolve the owning profile first, and resolveStoredSession() probes backends sequentially on a cache miss - one 30s getSession() per configured profile. Settlement time therefore scales with profile count, and a probe wedged the way NousResearch#93454 describes never settles at all. Bound the flight with the existing withTimeout() helper. The ceiling is derived from the longest LEGITIMATE settlement rather than picked round - active-profile probe (30s) + one cross-profile probe (30s) + the resume RPC (30s) = 90s - so a slow-but-healthy multi-profile recovery is never aborted while a genuinely wedged one is. A deadline alone would trade a hang for an orphan. withTimeout() does not cancel the work it bounds, and the straggler here is not inert: _claim_or_reuse_live() registers the record before returning, so a late resume has already minted a REAL runtime on the gateway. Dropping it is the per-resume orphan shape this module exists to prevent (NousResearch#91276). Hand a late arrival to the same recovered-runtime cache the drift-abort path uses, so the next resume-shaped action adopts it instead of minting a second one - unless a newer flight already owns the stored id, whose caller will adopt its own result. Six of the seven new tests fail on the prior behavior by timing out, which is the wedge itself. The seventh pins the ceiling against a 89s healthy resume so a future tightening cannot silently start aborting legitimate recoveries. Closes NousResearch#96522 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013XSgsNweukuU4n2WYX19JW
Contributor
fix(desktop): bound the shared resume flight and adopt its straggler — prevents
|
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
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
Closes #96522.
singleFlightSessionResume()shares ONE promise per stored session id across every Desktop surface that can recover a dead runtime, and the slot is released only by that promise's.finally(). An unsettledrun()therefore makes the conversation unrecoverable for the life of the window, and the rejection-driven retry ladder never arms because nothing ever rejects.This PR bounds the flight and decides what the straggler means — because a deadline on its own trades a hang for a stranded runtime.
Root cause
The RPC is already bounded:
HermesGatewaysetsrequestTimeoutMs = DEFAULT_GATEWAY_REQUEST_TIMEOUT_MS(30s). Therun()bodies are not just that RPC — they resolve the owning profile first, andresolveStoredSession()probes backends sequentially on a cache miss, one 30sgetSession()per configured profile. Settlement time scales with profile count; a probe wedged the way #93454 describes never settles at all.Because the map is module-level, a wedge created by any ONE entry point wedges all five:
resolve-target-session.ts:101utils.ts:122(resumeStoredRuntimeSession)submit.ts:592use-session-actions/index.ts:1393use-session-tile-delegate.ts:198The half a deadline does not fix
withTimeout()says it outright:For
session.resumethe straggler is not inert._claim_or_reuse_live()registers the record before returning, so a late resume has already minted a real runtime on the gateway. Dropping it strands that runtime for the reaper AND makes the next recovery mint another — the per-resume orphan shape this module was built to stop (#91276). The module already solved the identical problem for drift-aborts viaregisterRecoveredRuntime()/takeRecoveredRuntime(); nothing wired the timeout path into it.%%{init: {'theme': 'dark', 'themeVariables': { 'primaryColor': '#8b0000', 'mainBkg': '#0a0204', 'primaryTextColor': '#ffccd5', 'primaryBorderColor': '#ff0038', 'lineColor': '#ff0038'}}}%% graph TD A["🩸 Five Recovery Surfaces"] -->|"one shared slot per stored id"| B["🔥 Single Flight"] B --> C["⛓️ Profile Ladder<br/>30s per configured backend"] C --> D["📡 session.resume RPC<br/>30s budget"] B --> E{"⏳ Settlement Ceiling<br/>90s = 30 + 30 + 30"} E -->|"settles in budget"| F["⚔️ Runtime Adopted<br/>slot released"] E -->|"exceeded"| G["💀 Flight Rejected<br/>slot released · retry ladder arms"] G --> H["🕯️ Straggler Still Lands<br/>work was never cancelled"] H -->|"no newer flight owns the id"| I["🔮 Recovered-Runtime Cache<br/>next action adopts it"] H -->|"newer flight owns the id"| J["🚫 Discarded<br/>its caller adopts its own result"] I --> FImplementation
SESSION_RESUME_SETTLEMENT_TIMEOUT_MS = 90_000, derived rather than picked round: active-profile probe (30s, ElectronDEFAULT_FETCH_TIMEOUT_MS) + one cross-profile probe (30s) + the resume RPC (30s). A slow-but-healthy multi-profile recovery is never aborted; past that point the flight has held the slot longer than any single healthy recovery needs.withTimeout(). Its.finally()still frees the slot, so the next caller gets a FRESH attempt and the existing rejection-driven ladder arms.onTimeoutattaches a late-settle handler: a straggler that yields asession_idis handed toregisterRecoveredRuntime(). Guarded two ways — a straggler that rejects minted nothing, and one that lands while a newer flight already owns the stored id is discarded, because that flight's caller will adopt its own result.singleFlightSessionResume()takes an optionaltimeoutMsso tests can pin the boundary; every production caller uses the derived default.Two files,
+167 / −6. No new dependency, no new module, no behavior change on the success path.Test plan
apps/desktop/src/app/session/hooks/use-prompt-actions/single-flight-resume.test.ts— 13 passed (6 existing + 7 new).a never-settling resume rejects at the deadline instead of wedging the slot forevera later caller for the same stored id gets a FRESH attempt, not the dead flighta joiner of an already-wedged flight inherits the same deadlinea slow-but-legitimate resume (profile probe + RPC) still settles inside the ceilinga runtime minted by a timed-out resume is adopted, not stranded on the gatewaya straggler is NOT cached when a newer flight already owns the stored ida straggler that fails minted nothing and caches nothingFalsification: with the production file reverted to
origin/mainand the tests kept, 6 of the 7 fail — by timing out at 5000 ms, which is the wedge itself. The seventh (a slow-but-legitimate resume…) passes on old code because old code has no ceiling at all; it exists so a future tightening of the constant cannot silently start aborting legitimate recoveries.Local run notes
The desktop
vite.config.tsin this checkout imports@rolldown/plugin-babel, which is declared inapps/desktop/package.jsonbut absent from the installed tree, andnpm installrefuses on this machine (EBADENGINE: repo requiresnpm <11.10.0 || >=11.17.0, local is 11.4.1). The suite was therefore run through a throwaway vitest config providing only the@alias — the module under test needs nothing else. That config was deleted and is not part of this diff. Hosted CI remains the authority.Non-duplicate analysis
resumeSessionregression test still applies.tui_gateway). Disjoint file set, disjoint concern — the gateway behaves correctly here; it is the client that abandons a runtime it asked for.Type of change
Checklist