Skip to content

agentHost: retroactively recognise unmarked crash orphans - #336806

Open
Sandeep Somavarapu (sandy081) wants to merge 1 commit into
mainfrom
sandy081/agents/suppress-unmarked-orphan-sessions
Open

Sandeep Somavarapu (sandy081) wants to merge 1 commit into
mainfrom
sandy081/agents/suppress-unmarked-orphan-sessions

Conversation

@sandy081

Copy link
Copy Markdown
Member

Follow-up to #336480. Colleagues who hit the phantom-session bug before that fix shipped still see the rows, and always will — #336480 is prevention-only, and a session orphaned before it landed carries no marker for suppression to act on.

Evidence

From a reporter's logs, the row copilotcli:/e50ba1ea-84a3-4728-9093-15c58e9330ae:

  • Restore fails with Provider copilotcli is not ready to open … — the non-provisional branch, which only runs when the session has no marker.
  • Its provider catalog was readable: sessions_v2 import for provider copilotcli … marker set at 15:02:14, and the failure fired at 15:19:54.
  • The row is catalog-served, not a provider fallback: 28 sessions logged is ineligible that run (matching the 27 fallbacks in 651 catalog-served, 27 provider fallback), and this session is not among them.

So the catalog serves stale metadata written at creation for a backing that never materialized, and nothing will ever retract it.

The change

Reconciliation already visits exactly this population. #336434 added the sourceUnresolvable outcome for a session whose provider is registered but cannot describe it, and parks it. At that point this change adds the missing evidence:

If the session's local storage holds no conversation turns, write the durable sessionProvisional: marker retroactively.

The next listing then suppresses it through the path #336480 already ships, which filters the assembled result and so covers catalog-served rows. A pre-fix orphan is converted into exactly the shape the existing fix handles.

Why here rather than in listing: doing this at listing would mean a provider round-trip and a storage open for every catalog-served row — ~651 of them in the logs above — which is precisely the cost the catalog exists to avoid. Reconciliation visits each session once and then parks it, so the probe does not repeat.

Cost: zero extra opens per listing. Reconciliation reuses the database reference it has already acquired; the only new work is one turn-existence query, and only for source-unresolvable sessions.

Why this is safe without a marker

Suppression still requires positive evidence that the session is empty. A session with any conversation turn is never marked, so real user content cannot be hidden. The existing guards are unchanged: the provider's catalog must be established readable before a miss counts as authoritative, a provider that throws or is unavailable is never treated as evidence of absence, and anything uncertain fails open. Under-suppressing costs a junk row; over-suppressing costs a user's work.

Nothing is deleted. The row stays on disk and reappears if the provider ever vouches for it again.

Tests

The repro matches the reported shape — a catalog-served, unmarked orphan with empty local storage — and fails without the fix:

neutralised: 0 passing, 1 failing   (session stayed listed, provisionalMarkers: [])
restored:    1 passing

Guards alongside it: a catalog-served session with turns is never marked or suppressed; an unavailable provider is never treated as evidence; the marked-orphan fast path still works without consulting local storage.

5,422 passing across the agentHost node suites, typecheck and eslint clean.

Scope

Prevention plus retroactive recognition; still no deletion. Affected users need no action — the row disappears on its own once reconciliation has visited it.

During catalog reconciliation, retroactively marks provider-unresolvable sessions as provisional only when their local conversation storage is empty. Listing then reuses the existing provisional suppression path without per-listing storage opens.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Copilot-Session: 26730bc1-5d8d-48f1-a4c6-bde566082230
Copilot AI balanced review requested due to automatic review settings September 18, 2026 17:40

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🟡 Changes recommended

The provisional marker can be reintroduced by stale reconciliation after concurrent mutation or materialization, potentially hiding a real session.

Get a fresh assessment by requesting another Copilot review.

Review effort: Balanced
Findings: 1 High severity · 1 Low severity

Open (2)
What changed in this PR

Retroactively identifies and suppresses legacy crash-orphaned Agent Host sessions when local storage contains no conversation turns.

Changes:

  • Detects persisted and local conversation turns.
  • Marks empty, source-unresolvable sessions as provisional.
  • Adds regression and safety-path coverage.
File Description
sessionDatabase.test.ts Tests conversation-turn detection.
agentService.test.ts Tests orphan recognition and safety guards.
sessionTestHelpers.ts Tracks turns in test databases.
sessionDatabase.ts Adds the turn-existence query.
agentService.ts Mirrors retroactive markers and guards provider availability.
agentHostCatalogReconciliationService.ts Marks empty unresolved sessions as provisional.
sessionDataService.ts Adds the database interface method.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment on lines +368 to +369
await this._catalogDatabase.setSessionProvisional(sessionKey, true);
this._onDidMarkSessionProvisional(sessionKey);
Comment on lines +5411 to +5412
// The empty local store now proves the placeholder is safe to hide
// even before the marker read completes.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants