Skip to content

fix(agent-hooks): keep the pane provider-session binding across a transient clear - #12070

Draft
nwparker wants to merge 1 commit into
stablyai:mainfrom
nwparker:nwparker/fix-11546-provider-session-survives-transient-clear
Draft

nwparker wants to merge 1 commit into
stablyai:mainfrom
nwparker:nwparker/fix-11546-provider-session-survives-transient-clear

Conversation

@nwparker

@nwparker nwparker commented Aug 2, 2026 •

Copy link
Copy Markdown
Contributor

Fixes #11546

ELI5

When you chat with an agent in a pane, Orca doesn't keep the conversation in the window — it keeps a little sticky note saying "this pane is conversation #sess-1, whose transcript file lives here". The chat is then read from that transcript file.

Every time an SSH connection dropped, Orca threw away all the sticky notes for that connection — and saved that deletion to disk — betting the remote machine would hand them back on reconnect. If the app restarted before that happened, the terminal came back fine, but the sticky note was gone. The chat had no idea which conversation it was and showed empty history, even though the transcript was still sitting on the remote machine.

The fix keeps the sticky notes in their own section of Orca's saved-state file, copying the same trick an existing "who owns this pane" record already uses to survive disconnects. A dropped connection no longer erases them; only actually closing or retiring the pane does. Old saved-state files still work, new files won't break an older version of Orca, and the note pile is capped and expires after a week.

Root cause

clearStatusEntriesForConnection deletes every status row stamped with the lost connectionId and calls scheduleStatusPersist(). serializeStatusFile() rebuilds entries purely from the live map, so the wipe reaches disk. hydrateLastStatusFromDisk() then finds no row for the pane after a restart.

The status row is the only thing carrying a pane's providerSession — the sessionId + transcriptPath binding that native chat resolves a conversation from. Deleting it is a safe bet while the app keeps running, because reconnect re-publishes the rows. It is not safe across a renderer restart, which is exactly the reported sequence: SshRelaySession.detach()/reconnect() route through teardownProviders('connection_lost') → clearStatusEntriesForConnection, then the app restarts before reconnect completes. PTYs reattach because ownership lives elsewhere; chat resolves sessionId=null and the conversation reads as gone.

authorityCommitments in the same file already solves exactly this problem for pane authority, so this follows that pattern rather than inventing a new one.

Fix

  • New optional providerSessionIdentities section on LastStatusFile, keyed by paneKey: { agent?, sessionKey?, sessionId, transcriptPath?, connectionId, worktreeId?, observedAt }.
  • In-memory retainedProviderSessionsByPaneKey, populated from applyNormalizedStatus — both the full-status and providerSessionOnly branches, so every accepted payload carrying a providerSession seeds it, including the relay ingestRemote path which funnels through the same place.
  • The transient path keeps it. clearStatusEntriesForConnection goes through deleteStatusEntry(..., { preserveAuthority: true }); the binding is only dropped on the non-preserveAuthority branch. Real teardown does drop it: retirePaneAuthority, dropStatusEntriesByTabPrefix, clearPaneState, and clearPaneKeyAliasesForPty when the dying PTY still owns the stable pane key. transferPaneAuthority moves the binding to the new owner key alongside the authority commitment, so it follows a pane rebind instead of being orphaned.
  • Hydrate clears the map first (idempotent, matching the existing hydrate contract), seeds it from any hydrated entry carrying a providerSession — this is what makes an existing on-disk file restore the binding with no migration — then applies the file's own section through a sanitizer reusing normalizeAgentProviderSession (same trust boundary as relay/persisted input) and the existing HYDRATE_MAX_AGE_MS cutoff. The section only wins over an entry-seeded row when its observedAt is not older.
  • getProviderSessionForPane(paneKey) returns the live row's session; else null if a live row exists without one (a live row is the current authority — never resurrect an older conversation over it); else the retained binding.
  • Bounded at 512 entries with insertion-order (least-recently-observed) eviction, same shape as boundPaneKeyAliases, plus the 7-day hydrate TTL.

Deliberately not done: widening isValidPiProviderSessionOnly, and surfacing hydrated identities as providerSessionOnly status rows. Both would risk resurrecting stale status (a phantom "working" badge). Retention is also not gated on the launchTokenHash commitment — agents that never emit a launch token would lose the binding entirely. Pane reuse is handled by the live-row-wins rule plus deletion on real teardown.

Proof

Before — new tests against server.ts as it exists on main:

$ npx vitest run --config config/vitest.config.ts src/main/agent-hooks/server.test.ts
   (server.ts restored from origin/main)

     × retains the pane provider-session binding through a transient SSH clear and restart
     × drops the retained provider-session binding on explicit pane retirement
     × drops retained provider-session identities past the hydrate TTL
     × bounds retained provider-session identities

AssertionError: expected [] to deeply equal [ { …(4) } ]
AssertionError: expected +0 to be undefined // Object.is equality

 Test Files  1 failed (1)
      Tests  4 failed | 259 passed (263)

After — this branch:

$ npx vitest run --config config/vitest.config.ts src/main/agent-hooks/server.test.ts

 Test Files  1 passed (1)
      Tests  263 passed (263)

The headline test uses a legacy-shaped on-disk file (no providerSessionIdentities section) to prove existing state hydrates without migration. Its fixture deliberately uses a Windows remote transcript path while the test runs on macOS/Linux, pinning that remote paths are never normalized with local path helpers.

pnpm tc and pnpm lint pass on this branch.

Release-readiness

Scan Result
Backcompat / data loss (mandatory) Version stays at LAST_STATUS_FILE_VERSION = 2; the section is optional. Old file → new code: hydrate reads file.providerSessionIdentities ?? {} and seeds from each hydrated entry's own providerSession, so today's on-disk file restores conversations with no migration. New file → old code: an older build reads only version/entries/authorityCommitments and ignores the extra key, dropping it on its next write — downgrade-safe. This PR reduces data loss; it introduces no new persisted-state failure mode.
Functional correctness Live row always wins over a retained binding, so a reused pane cannot resurrect an older conversation. Binding follows transferPaneAuthority rather than being orphaned. Sanitized through the existing normalizeAgentProviderSession trust boundary.
Cross-platform Pure main-process state plus JSON under <userData>/agent-hooks/last-status.json, path built with the existing join() usage. transcriptPath is stored and returned verbatim, never normalized — for SSH/relay panes it is a remote path in the remote host's syntax.
SSH / relay The targeted path. detach()/reconnect() → teardownProviders('connection_lost') → clearStatusEntriesForConnection now preserves the binding; the shutdown path (clearPtyOwnershipForConnection → pane teardown) still drops it. connectionId is retained so a consumer can tell which host owns the session.
Folder workspace vs worktree Keyed by paneKey, which both workspace kinds use. worktreeId is carried only when the payload had one; nothing assumes a git worktree. Provider-neutral.
Security / supply chain No new dependencies, no shell/exec, no network sink. Persisted input is sanitized on hydrate and bounded (512 entries + 7-day TTL), so a tampered or bloated state file cannot grow memory without limit.

Residual risk

  • Renderer wiring is not in this PR. Native chat still resolves sessionId from the agent-status IPC row (native-chat-pane-resolution.ts). This change makes the binding survive and be resolvable — via the new getProviderSessionForPane and the retained rows now included in getProviderSessionIdentities() — but does not itself change what the renderer asks for. The end-user symptom closes fully once a consumer reads it. Flagging clearly so this isn't mistaken for an end-to-end fix.
  • Retained rows now appear in the provider-session change notification for panes with no live row, flowing into publishProviderSessionChanges → mobile session-tabs invalidation. Net effect is fewer spurious invalidations (a transient clear no longer looks like "session disappeared"), but it is a behavior change on that channel.
  • A pane torn down in a way that bypasses retirePaneAuthority/clearPaneState/dropStatusEntriesByTabPrefix — e.g. an Orca crash before teardown — could keep a stale binding until the 7-day TTL. The live-row-wins rule prevents it from overriding a real session.
  • A user with more than 512 session-bearing panes in one run could lose the oldest bindings before their TTL expires.
  • Not exercised against a real SSH host or the live app; evidence is the main-process unit suite.

Made with Orca 🐋

…nsient clear

Losing an SSH transport calls clearStatusEntriesForConnection, which deletes
every status row stamped with that connection and persists the deletion. The
status row is the only thing carrying a pane's providerSession, so the
pane -> sessionId/transcriptPath binding went with it. That is a safe bet while
the app keeps running, because a reconnect re-publishes the rows; it is not safe
across a renderer restart, which is exactly the reported sequence. Terminals
reattach because PTYs are owned elsewhere, but chat resolves sessionId=null and
the conversation reads as gone even though the transcript is still on the host.

Persist the binding in its own last-status section, mirroring how
authorityCommitments already survives the same clear. The transient path keeps
it; only real teardown (retirement, pane/tab removal) drops it, and a pane
rebind moves it alongside the authority commitment.

Existing on-disk files hydrate unchanged: the section is optional, the version is
not bumped, and each hydrated entry's own providerSession seeds the map, so no
migration is needed and an older build ignores the extra key. Retention is capped
at 512 entries with least-recently-observed eviction plus the existing hydrate
TTL.

Fixes stablyai#11546

Co-authored-by: Orca <help@stably.ai>
@greptile-apps

greptile-apps Bot commented Aug 2, 2026 •

Copy link
Copy Markdown

Greptile Summary

This PR fixes the disappearing native-chat history after an SSH reconnect by introducing a new retainedProviderSessionsByPaneKey Map and a corresponding providerSessionIdentities section in the persisted last-status file. The binding now survives a transient connection clear (SSH drop) and is restored on restart, while all explicit teardown paths still drop it correctly.

  • Adds PROVIDER_SESSION_IDENTITIES_MAX = 512 with insertion-order LRU eviction and a 7-day hydrate TTL, mirroring the existing authorityCommitments pattern; the file version stays at 2 with the new section being optional, keeping both forward and backward file compatibility.
  • clearStatusEntriesForConnection now uses preserveAuthority: true which skips the retained-binding delete, while retirePaneAuthority, clearPaneState, dropStatusEntriesByTabPrefix, and transferPaneAuthority all explicitly manage the map so explicit teardown still cleans up correctly.
  • getProviderSessionForPane implements a live-row-wins fallback: returns the live row's session if present, null if a live row exists without a session (preventing stale resurrection), and the retained binding otherwise.

Confidence Score: 4/5

Safe to merge; the change is well-scoped to the main-process state layer, renderer wiring is deliberately deferred to a follow-up PR, and both forward/backward file compatibility are verified by the new tests.

The core fix is solid and the test suite is thorough. There are two narrow gaps: dropStatusEntriesByTabPrefix doesn't scan retainedProviderSessionsByPaneKey when collecting keys to clear, so a pane cleared by an SSH disconnect but not by any other tracked map can leave a stale binding that survives a tab close; and the <= vs < comparison in the hydrate merge gives the persisted section a win on equal timestamps, which is harmless today but slightly contradicts the inline comment. Neither affects the targeted scenario, and the renderer-side wiring is noted as explicitly out of scope.

Files Needing Attention: The hydration merge comparison and the dropStatusEntriesByTabPrefix key-collection gap are both in server.ts; server.test.ts is clean.

Important Files Changed

Filename Overview
src/main/agent-hooks/server.ts Adds a new retainedProviderSessionsByPaneKey Map and providerSessionIdentities disk section to preserve pane→session bindings across transient SSH clears; all teardown paths (retirePaneAuthority, clearPaneState, clearPaneKeyAliasesForPty) correctly delete from the map, but dropStatusEntriesByTabPrefix may miss pane keys that exist only in this map after the live status row was cleared.
src/main/agent-hooks/server.test.ts Adds four new targeted tests: transient-clear-and-restart with a legacy on-disk file, explicit retirement, TTL expiry, and insertion-order eviction at the 512-entry cap; covers both hydration paths and uses a Windows transcript path on a Unix runner to pin cross-platform path verbatim behaviour.

Comments Outside Diff (1)

  1. src/main/agent-hooks/server.ts, line 2316-2385 (link)

    P2 dropStatusEntriesByTabPrefix may not collect retained-only pane keys

    dropStatusEntriesByTabPrefix builds paneKeysToClear by scanning several maps (e.g. lastStatusByPaneKey, runtimeObservedStatusPaneKeys, promptSentDedupeByPaneKey) but does not scan retainedProviderSessionsByPaneKey itself. After clearStatusEntriesForConnection has run — removing the pane from lastStatusByPaneKey and runtimeObservedStatusPaneKeys, and conditionally from promptSentDedupeByPaneKey (only when state was 'done') — the pane key may exist exclusively in retainedProviderSessionsByPaneKey. In that case, a subsequent tab-close calling dropStatusEntriesByTabPrefix will not add it to paneKeysToClear, so the delete on line 2385 is never reached and the stale binding persists until the 7-day TTL.

    The fix is simply to scan retainedProviderSessionsByPaneKey in the collection phase, mirroring how lastStatusByPaneKey is already handled.

Reviews (1): Last reviewed commit: "fix(agent-hooks): keep the pane provider..." | Re-trigger Greptile

Comment on lines +2688 to +2689
// Why: a live row's own binding is newer evidence than the retained section for the same pane.
if (!seeded || seeded.observedAt <= identity.observedAt) {

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 The seeded.observedAt <= identity.observedAt condition uses <=, so when the entry-seeded binding and the persisted section carry the exact same timestamp (the common case after a restart where the disk file was written from that entry), the persisted section silently wins. The comment above says "a live row's own binding is newer evidence," which describes the opposite intention. Using strict < would preserve the entry-derived binding on a tie, which is the semantically safer choice — the entry binding was validated through the full hydration path just moments earlier.

Suggested change
// Why: a live row's own binding is newer evidence than the retained section for the same pane.
if (!seeded || seeded.observedAt <= identity.observedAt) {
// Why: a live row's own binding is newer evidence than the retained section for the same pane.
if (!seeded || seeded.observedAt < identity.observedAt) {

@coderabbitai

coderabbitai Bot commented Aug 2, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

📝 Walkthrough

Walkthrough

The server now retains provider-session identities separately from live status rows. It validates, bounds, hydrates, and serializes these identities with TTL filtering. Live status rows take precedence during lookup and notifications. Retained identities survive transient connection clears and pane transfers, and cleanup paths remove them when authority or status state is retired. Tests cover restart restoration, retirement, expiration, and capacity eviction.

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly and concisely describes retaining pane provider-session bindings across transient clears.
Description check ✅ Passed The description explains the change, testing, compatibility, security, platform behavior, and residual risks, with only minor template omissions.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@coderabbitai coderabbitai Bot 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.

🧹 Nitpick comments (2)
src/main/agent-hooks/server.ts (1)

422-433: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Extract a shared helper for building AgentHookProviderSessionIdentity from a live row.

The same field-mapping logic (sessionId, optional transcriptPath, optional worktreeId) is duplicated in toProviderSessionIdentity, the live branch of getProviderSessionForPane (Lines 750-758), and the live branch of buildStatusChangeNotification (Lines 970-978). A single helper, for example toProviderSessionIdentityFromLive(paneKey, live), would remove the duplication and make it easier to keep the live/retained precedence rule consistent if it changes later.

♻️ Proposed helper extraction
+function toLiveProviderSessionIdentity(
+  paneKey: string,
+  live: EnrichedAgentHookEventPayload & { providerSession: AgentProviderSessionMetadata }
+): AgentHookProviderSessionIdentity {
+  return {
+    paneKey,
+    sessionId: live.providerSession.id,
+    ...(live.providerSession.transcriptPath
+      ? { transcriptPath: live.providerSession.transcriptPath }
+      : {}),
+    ...(live.worktreeId ? { worktreeId: live.worktreeId } : {})
+  }
+}

Also applies to: 750-758, 970-978

src/main/agent-hooks/server.test.ts (1)

6784-6933: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Consider adding coverage for a live row without providerSession after retention.

These four tests cover restore-with-no-live-row, explicit retirement, TTL expiry, and capacity eviction. None cover the scenario flagged in server.ts (getProviderSessionForPane, Lines 739-767): a retained binding exists, then a live status row without providerSession is accepted for the same pane (for example, via ingestTerminalStatus). Add a test asserting the expected outcome for getProviderSessionForPane in that case, once the behavior in server.ts is confirmed.

Do you want me to draft this test once the intended behavior is confirmed?


ℹ️ Review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 1ebc88d7-0288-483f-8319-bfcea7a87f45

📥 Commits

Reviewing files that changed from the base of the PR and between 2f73775 and 71cd3e2.

📒 Files selected for processing (2)
  • src/main/agent-hooks/server.test.ts
  • src/main/agent-hooks/server.ts

@OrcaWin

OrcaWin commented Oct 27, 2026

Copy link
Copy Markdown
Collaborator

Moving this PR to draft because it has been untouched for more than 80 days (latest recorded PR activity or commit timestamp: 2026-08-02T06:47:04Z; checked on 2026-10-27). This keeps the review queue focused on active work while preserving the proposal. Please mark it ready for review when work resumes.

This branch has not been deployed

No deployments
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.

[Bug]: AI conversation context lost on renderer restart — SSH terminal reconnects but chat history is gone

2 participants