agentHost: recognise a crash-orphaned provisional session - #336480
Open
Sandeep Somavarapu (sandy081) wants to merge 3 commits into
Open
Sandeep Somavarapu (sandy081) wants to merge 3 commits into
Sandeep Somavarapu (sandy081) wants to merge 3 commits into
Conversation
A session whose provider defers its backing until the first send is registered durably at creation, but nothing exists on the provider side until it materializes. `createSession` records only in-memory state for that fact, and `isIdleProvisionalSession` reports `false` for a session it no longer tracks, so a crash before materialization leaves a registration the guard stops hiding. The registration is not stale metadata the host could repair: it durably records a pointer to an SDK session that was never created. The host even persists the backing id at creation (`_persistDefaultChatBacking`), so the identity survives intact — there is simply nothing on the provider side for it to name. No later run can resolve it, so it surfaces as a row that cannot be opened and reports a transient-sounding failure forever. Record the provisional fact durably instead, as host-owned metadata rather than a `registration_source` value, so an older build — which casts that column straight to its union and would classify an unknown value as external — keeps reading these sessions unchanged. The marker is cleared when the session materializes. Listing then drops a session only when it is *both* still marked provisional and undescribable by its provider. Requiring both means a promotion write that never landed cannot hide real content, and a provider that is merely unavailable cannot hide anything: that combination is the protection #331721 added, kept intact here. Restore reports the same combination as authoritative absence rather than a retryable failure, because waiting cannot make such a session resolvable. The marker is mirrored in memory and consulted synchronously while listing: that path's phase ordering drives provider catalog migration, and an added read there changes when those passes run. This prevents new orphans; it does not clean up ones already on disk, which carry `source: 'explicit'` and no marker. Deleting such a session already works and needs no provider metadata. Co-authored-by: Copilot <[email protected]>
The transient restore failure read "Provider <id> could not describe <uri> yet". The "yet" carried the whole meaning: it was the only hint that the failure might resolve on its own, and it left the user with no idea whether to wait, retry, or give up. Say what the state actually is — the provider is not ready — and what to do about it. The classification is deliberately unchanged: this branch is still chosen whenever the session is known to the registry or the catalog was unreadable, because narrowing it would turn a provider that is merely unavailable into a sticky "not found", which is the regression #331721 exists to prevent. Co-authored-by: Copilot <[email protected]>
Copilot started reviewing on behalf of
Sandeep Somavarapu (sandy081)
September 16, 2026 20:38
View session
roblourens
previously approved these changes
Sep 16, 2026
Contributor
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Registration and marking are not atomic, while stale markers can incorrectly hide materialized sessions during provider outages.
Get a fresh assessment by requesting another Copilot review.
Review effort: Balanced
Findings: 2
Open (2)
What changed in this PR
Adds durable tracking for provisional Agent Host sessions to suppress crash-orphaned entries and improve restore errors.
Changes:
- Persists and clears provisional-session markers.
- Filters unmaterialized sessions during listing and restoration.
- Adds regression coverage and clearer provider-readiness messaging.
| File | Description |
|---|---|
agentHostDatabase.ts |
Stores provisional markers. |
agentSessionRegistry.ts |
Exposes marker operations. |
agentService.ts |
Manages markers, filtering, and errors. |
agentService.test.ts |
Adds crash-recovery regression tests. |
agentSessionRegistry.test.ts |
Updates the database test double. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+1656
to
+1663
| const agent = this._providerService.getProvider(entry.provider); | ||
| if (!agent) { | ||
| return; | ||
| } | ||
| try { | ||
| if (!await this._registeredSessionMetadata(agent, entry.session, entry.external, entry)) { | ||
| unmaterialized.add(key); | ||
| } |
Comment on lines
+3920
to
+3922
| await this._retryRegistryMutation( | ||
| () => this._setSessionProvisional(session, true), | ||
| `provisional marking for ${session.toString()}`, |
`whenDeferredWorkSettled()` awaited only the deferred-work chain, but the provisional marker read is started fire-and-forget at construction and never joins it. A caller that waited for work to settle could therefore observe a listing computed before any marker was known — the orphan still visible — which made suppression look ordering-dependent and let a test pass on load timing rather than on the behaviour it asserts. Join the marker load there. Listing itself is untouched: it still reads the in-memory mirror synchronously and skips the filter when the mirror is empty, so the phase ordering that drives provider catalog migration is unchanged. Early listings still fail open, which is the intended direction — a listing that cannot yet classify a session shows it rather than hiding it. The window is bounded by the marker read alone: a computation still running when the markers land consults the mirror after its provider phase, and a settled one is evicted from the in-flight map, so the next listing recomputes. The added test models a slow read, since the in-memory double always wins that race and a test written against it would pass either way. Co-authored-by: Copilot <[email protected]>
roblourens
approved these changes
Sep 16, 2026
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.
Fixes a session that can never be opened after a crash, reported by roblourens after his OS crashed:
What actually happened
A new chat reserves a backend session before the user sends anything. That reservation is durably registered, and — this is the part worth being precise about — its provider data, including the SDK backing id, is persisted too. The identity survived the crash intact.
What it recorded was a pointer to an SDK session that was never created:
_reserveChatBackingonly mints a UUID, and the real SDK session first exists at_materialize. So the host durably recorded a promise that was never fulfilled.That matters because the widely-repeated explanation — "creation state is in-memory only until materialization" — is wrong, and it points at the wrong fix. No recovery path can exist for these rows, which is why
yetwas categorically wrong rather than merely imprecise, and why suppressing them is correct rather than a workaround.The row stayed visible because the guard that hides idle provisional sessions,
isIdleProvisionalSession, is in-memory: after a restart there is no tracked state, so it returnsfalseand stops hiding the registration (#321269).The fix
Record the provisional fact durably, as a host-owned
sessionProvisional:metadata key, and clear it at materialization. Listing then drops a session only when both hold:Both clauses are required, and the second is what protects real user content: clearing the marker is best-effort, so a materialized session whose clear never landed keeps a stale marker — and stays visible, because its provider vouches for it. An unregistered or erroring provider is likewise never treated as evidence of absence.
Rejected: deferring durable registration until materialization
The obvious fix — don't register until the session materializes — was tested and discarded. Probed by execution, a materialized session whose registry row is missing does not recover after a restart, even when the provider relists it under the correct URI:
The registry is the sole durable index. Deferring its write trades a cosmetic empty row for permanent silent loss of a session with real user content, in exactly the crash window this bug occupies.
Scope: prevention only
Existing orphaned rows are not cleaned up; they stay until deleted. Deleting one works today —
_doDisposeSessionneeds no provider metadata, and the SDK call is skipped when there is no live backing — so anyone hitting this has a working recovery path now and need not wait for this change. Nothing already on disk can carry the marker, so a retroactive sweep would need a different, evidence-based discriminator; that deserves its own design rather than being smuggled in here.Second commit: the error message
yettold the user to wait for something that could never happen. Now: "Provider X is not ready to open Y; it may still be starting, so try again shortly".Wording only. The
catalogReadable && !knownToRegistryclassification is deliberately untouched — turning borderline cases into stickyAHP_SESSION_NOT_FOUNDis the regression #331721 exists to prevent.Implementation note
The suppression check cannot
awaitinside_computeSessions. Doing so shifts when_retryInitialProviderMigrationsInBackgroundfires and produces a second provider-absent exclusion pass, breakingprovider-absent stale exclusion withholds migration completion. That is why the marker is mirrored in memory and read synchronously, with the call site skipping the await entirely when nothing is provisional. It looks like gratuitous complexity otherwise, and simplifying it reintroduces the regression.Independent verification
The fix was verified by a second implementation built against the same behaviour, not a re-reading of this code. Both orphan shapes were run with the session catalog on and off:
Session was never created on the backendSuppression holds with the catalog disabled, which is true by construction: the check runs outside any catalog-mode branch. The fix is therefore at the registration layer and is unaffected by the rollback switch — it is not a narrowing of what the catalog will show.
The vouches-while-live shape is the valuable half: a catalog row written from real provider metadata that later outlives the provider's memory. That is the reported shape, and it was the route least covered by the tests in this PR.
The inverse-orphan case — a materialized session with real content — was probed under four interleavings (clean, stale marker, stale marker with a throwing provider, stale marker with no provider). All keep the session visible. No interleaving was found that hides real content.
If you write your own probe: vouch via
getChatMetadataSuppression consults
getChatMetadata. A double that vouches only throughgetSessionMetadata/listSessionsis not vouching on the path under test, and will appear to show real content being wrongly hidden. That false positive occurred during verification before being traced to the mock.Nothing here required instrumenting inside
_computeSessions: the behaviour is observable fromlistSessions()pluslistProvisionalSessions().A rejected robustness change, recorded so it is not re-added
The marker mirror loads asynchronously while listing reads it synchronously, so a listing during that window fails open. Two mitigations were considered; only one was shipped.
whenDeferredWorkSettled()now joins the marker load, so a settled listing cannot miss suppression. This was the load-bearing half — the observed "still visible when settled" behaviour was a settle-semantics gap, not a caching one.Re-verified against a 250 ms delayed marker read:
visibleWhenSettledflippedtrue → false, whilevisibleOnFirstListingWithSlowReadstayedtrue(fail-open, by design) andvisibleAfterLoadCompleteswas alreadyfalsebeforehand. The change moved exactly the one cell it was meant to move.An epoch bump on load completion was implemented and then dropped as a no-op:
_inFlightListSessionsonclear(), so the next listing recomputes regardless._sameSessionRegistrationsalso reports a change. Markers are not registrations, so that branch never fires for this case.No test could be written that fails without it. Shipping it would have added a dead line plus an unfalsifiable test, both of which would read as covering the race. The loader carries a comment explaining this so it is not re-added.
Validation
The new test reproduces the reported symptom exactly when the fix is neutralised:
With the fix:
listed: [],restoreCode: -32001,saysYet: false. Falsifiability was verified twice — on the initial shape and again after the design changed.Three regression guards alongside it: a merely-unavailable provider stays listed; a provisional session the provider can describe stays listed (the failed-promotion case); deletion clears the marker.
5,389 passing across the agentHost node suites, typecheck and lint clean.