Skip to content

fix(daemon): keep a previous daemon version the app could not reach instead of reading it as gone - #23959

Open
brennanb2025 wants to merge 10 commits into
brennanb2025/coercion-contract-design-r1from
brennanb2025/r4-d1-census-unverifiable
Open

brennanb2025 wants to merge 10 commits into
brennanb2025/coercion-contract-design-r1from
brennanb2025/r4-d1-census-unverifiable

Conversation

@brennanb2025

@brennanb2025 brennanb2025 commented Sep 30, 2026 •

Copy link
Copy Markdown
Contributor
Files Added Deleted Net
Test 11 $\color{#1a7f37}{\Huge{\mathbf{+}}}$​768 $\color{#cf222e}{\Huge{\mathbf{−}}}$​31 $\color{#1a7f37}{\Huge{\mathbf{+}}}$​737
Prod 26 $\color{#1a7f37}{\Huge{\mathbf{+}}}$​311 $\color{#cf222e}{\Huge{\mathbf{−}}}$​129 $\color{#1a7f37}{\Huge{\mathbf{+}}}$​182

ELI5

When Orca starts on a busy Mac, it can take more than a second to say hello to the terminal service left over from the previous Orca version. Today Orca then decides that old service is gone, tells your agent tab its conversation is dead, and quietly starts a second copy of the same conversation while the original keeps running where nobody can see it. After this PR Orca only calls the old service gone when it can prove it. A hello that arrived during the stall now counts, so the tab simply reconnects. If Orca really cannot tell yet, the tab keeps its saved session and shows the existing "Orca left the saved session unchanged … Click Retry" message instead of starting a second copy.

The problem

What users see: after Orca updates, agent sessions from the previous version keep running invisibly and pile up (one report: ~46 hidden Claude processes against 8 visible sessions and ~20 GB of swap). Worse, on main a restored agent tab can start a second copy of the same agent conversation (claude --resume of the same transcript) while the original keeps running with no tab.

Why it happens: at startup Orca checks each previous terminal-service version (daemon v1…v35) with a 1-second connection check. The check returned the same "no" for three different facts: the socket file is gone, the connection was refused, or the 1-second timer fired first. A "no" dropped that version from the version list for the whole run (daemon-legacy-adapters.ts).

Measured trigger: the kernel accepts a Unix-socket connection even from a daemon that is stopped, so a busy daemon does not cause this. What causes it is the app's own main thread stalling about a second across the check (a loaded Mac at boot). When the stall ends, Node runs expired timers before it looks at finished connections, so the timer wins against a connection that already succeeded.

What happened next: session lookup counted only the versions it still had, called its answer complete, and answered "absent". The tab took that as proof the agent had exited (hostExitConfirmed: true), dropped its saved session, and started a fresh one with the pane's resume command. If every previous version was dropped, the bare current daemon answered "Session not found" for another version's session, with the same result. The original agent ran on with no tab, and it kept its old daemon alive across every later update.

A second way the same mistake happened: on macOS a live daemon that is not accepting connections refuses every connection once 128 are queued (measured, and reproduced by a positive-control test here). main read any refused connection as "that daemon exited", both in startup cleanup and when a tab reattaches (stable-owner.ts), so a frozen daemon could also trigger the second copy.

What changes for the user

  • On a slow startup (the common case) the tab now reattaches to the agent that was already running, with no message. Before, it started a second copy.
  • If the old version really cannot be reached (for example it is frozen), the tab shows the existing "Orca left the saved session unchanged. Click Retry…" message and keeps its saved session. Retry reconnects once the old version answers. "Open a new terminal" always works.
  • Panes whose sessions live in a version that did answer still reattach normally, even while another version is unreachable. They are matched by their saved session identity, so one unreachable version does not put every restored pane into "unverified".
  • Closing a tab (or one pane of a split) still ends its session while another version is unreachable, as long as the session lives in a version that answers. Closing a tab whose session sits in the unreachable version no longer counts the session as ended; the close still happens immediately, and the session itself cannot be ended until its version answers (step 4 below handles that).
  • In Settings → Terminal → Manage Sessions, Kill now ends exactly the row you clicked. After the bug above, the same session id could appear twice (the original in the old version, the copy in the current one), and Kill ended whichever came first, which could be the agent in your open tab. The two rows still look the same until step 3 (fix(terminal): list, stop, open and delete past a terminal-service version that does not answer #22046) labels each version.
  • Nothing is killed automatically. Sessions that were already orphaned before this PR stay until you end them.

The plan (this PR is step 2 of 4)

  1. Preserve legacy daemon ownership when liveness is unavailable #16900: keep an old version's pid and token files when its state cannot be told, so its sessions stay reachable.
  2. This PR: stop reading "could not reach it" as "it is gone", so Orca stops creating new orphans at startup.
  3. fix(terminal): list, stop, open and delete past a terminal-service version that does not answer #22046: show old versions' sessions in Manage Sessions, grouped by version, with an unreachable version shown as unverifiable rather than empty.
  4. Retry a tab-close kill that could not be delivered, later, when that version answers (a durable kill order). Not opened yet.

This PR is stacked on #16900 (base brennanb2025/coercion-contract-design-r1).

How it works

  • The stall itself (probeSocketConnect): when the 1-second timer fires, the check reports the timeout one event-loop turn later. A connection that finished during a stall is processed first, so it counts as connected. This removes the common trigger at its source.
  • One probe answers live / unverifiable / exited: probeDaemonEndpoint (daemon-endpoint-verdict.ts). It reuses the existing connect classifier, and every caller of the old yes/no probeDaemonSocket now uses it. The old probe is deleted.
  • exited requires real proof: the endpoint is missing, or the connection was refused and the recorded daemon process is gone. "Gone" means ESRCH from the signal-0 check, no pid record (a daemon removes its own record as it exits), or a record that says the daemon started before this machine last booted. A refusal while the recorded process runs, a timeout, EPERM without the boot proof, or an unreadable record all read as unverifiable.
  • Census (daemon-legacy-adapters.ts): versions that are live or unverifiable stay in the list, so the existing resolver answers "unknown", which produces the shipped owner-unverified state. Only exited versions are dropped, and only they go through Preserve legacy daemon ownership when liveness is unavailable #16900's file reclaim. Each version kept is logged as [daemon] Keeping previous daemon vNN unverified: <reason>, and each pane left unverified as [daemon] Keeping pane session <id> unverified: no daemon proved its owner, so we can count how often this happens.
  • Exact saved identity (daemon-session-owner-resolution.ts): a single exact match on the pane's saved incarnation (a random UUID per session) in a version that answered is accepted even when another version did not answer. A saved incarnation that is out of date can fail to match but can never match the wrong session. Sessions from before incarnations existed (protocol v1–v25) stay unverified, the cautious answer.
  • Attach step (daemon-pty-session-spawn.ts): a refused or missing endpoint maps to "the terminal daemon exited" only when the probe proves it, and to owner-unverified otherwise. The unproven check in stable-owner.ts is removed.
  • Previous versions re-derived on every listing (daemon-pty-session-inventory.ts): a version that exits mid-run lists as empty, based on proof, and nothing is stored. So one that goes away cannot leave lookups stuck on "unknown" for the rest of the run.
  • Kills and writes (daemon-pty-router.ts): a session with no route is resolved to its owner, no longer sent to the current daemon, whose "Session not found" read as "already gone". Local kills (tab close, pane close, runtime and CLI kills) pass the pane's saved incarnation, so a session in a version that answered is found even while another version is unreachable; SSH kills are unchanged. Absent from every version means gone; unknown throws an error that does not mean absence. A settled write (orchestration) to a session no version can be shown to own is refused (provider_unavailable), and a plain keystroke write to a session with no route is dropped, instead of either being handed to the current daemon.
  • Other readers that took a timeout as "gone": startup cleanup (cleanupDaemonForProtocol) no longer deletes an old pre-v24 daemon's pid record or skips teardown after a timed-out check; the self-shutdown wait (waitForDaemonEndpointExit) no longer takes a timeout as exit; the health-check grace retry keeps waiting while the check times out, instead of moving on to killing a daemon whose session count is unknown (a refusal from a live process still reaches the existing kill quickly, as before). The launcher's last-chance adoption still adopts only a daemon that answers.
  • Manage Sessions: Kill sends the clicked row's session id, protocol version and incarnation, confirms that exact session in that version's listing, then kills it.

What this deletes

  • probeDaemonSocket (the yes/no probe) and its separate timeout constant.
  • The continue that turned a missed check into "that version is gone".
  • isDaemonEndpointGoneError as a verdict. It is renamed isDaemonEndpointConnectRejection, is now only a first filter, and the probe decides.
  • The router's fallback to the current daemon for kills and writes.
  • Manage Sessions' "kill the first session with this id".

Traced along the way (the "any other reattach error" path)

The renderer (reattach-result-handler.ts, reached from deferred-session-reattach-connect.ts) answers any local reattach error other than owner-unverified by clearing the pane's saved session and starting over with the resume command. Errors from main arrive there as "no result": ipc-pty-connect.ts routes them to onError and returns undefined.

For a single-pane tab, main puts the binding back (workspace-session-terminal-binding-replay.ts), so the next attempt reattaches again rather than duplicating. For a split tab, the partial layout is written without that leaf's binding and nothing restores it for local panes, so the next spawn has no owner and starts fresh, orphaning the original.

So that path makes the same mistake whenever the error is "could not reach it" (a timeout or a dropped connection). This PR fixes it at the source: an attach that never heard the owner answer now reports owner-unverified, which the renderer already handles by keeping the binding and offering Retry.

What still reaches that renderer path unchanged: a proven "daemon exited", ownership races (terminal_pane_owner_changed / _conflict / _host_mismatch), local history I/O errors, and renderer-side exceptions in the .catch in deferred-session-reattach-connect.ts.

Why this design

  • It fixes the reading of a failed connection, which is where the bug is, instead of adding a guard beside it: one probe with three answers, every caller migrated, the old yes/no probe deleted.
  • It stores nothing. Whether an old version is gone is re-derived on every listing and every attach, so a wrong answer cannot latch for the rest of the run.
  • It reuses the shipped owner-unverified state and its Retry button, so there is no new UI and no new recovery path.
  • It is app-only, so it works against every daemon already running on users' machines.

Differences from the reference implementations

  • No automatic cleanup of orphaned sessions. Intended. Some persistent-terminal designs end sessions that have been detached for a long time, or reap sessions that have no ownership record. Orca's orphans include agents in the middle of real work (the STA-2018 reports include one with 6h31m of CPU). Orca promises agents keep running while Orca is closed, and Orca has no durable ownership record covering every kind of owner. Whether Orca should offer a one-confirmation cleanup is a separate product decision.
  • A refused connection is not enough to call a daemon gone. Intended. Other designs treat a refused socket as a dead daemon. On macOS a live daemon with a full accept queue also refuses (reproduced by a positive-control test here), and reading that as death is what started the second copy.
  • Orca still connects to previous versions. Intended. That is what keeps tabs working across an update. This PR only changes how Orca reads a failed connection.
  • A failed tab-close kill to an unreachable version is not retried yet. Temporary. Step 4 (durable kill order) removes this.
  • Manage Sessions' identity check lists the session, then kills it, as two steps. The daemon's kill request takes no incarnation at any shipped version. Intended for this PR, since the fix must work against unmodified old daemons. The window between the two steps is small, because a closed tab's session ids are never reused.
  • Proof of exit uses the signal-0 check and the boot time, not the process start time. Intended. On macOS the repo's validated proof list accepts only pid_missing, a start-time comparison there has one-second resolution against the daemon's own reported start (a slow-starting live daemon could read as "different process", which is this very bug), and a start-time read on Windows would fork PowerShell. The cost is below under "Known limits".

Known limits (all fail toward "unverifiable", never toward a second copy)

  • Listings while a version is unverifiable. Temporary, owned by step 3. Listing every version's sessions fails while one version cannot be verified, as it already did on main for a version that answered at startup and froze later. While that lasts, the Resource Manager shows its sessions error, a workspace delete asks for Force Delete, and the orca terminal create retry check reports the runtime unavailable. With the stall fix and the boot check, this now needs an old daemon that is genuinely alive but not answering. Step 3 (fix(terminal): list, stop, open and delete past a terminal-service version that does not answer #22046) lists what answered and marks the unverifiable version instead.
  • A crashed old daemon whose pid number now belongs to another process, without a reboot in between, stays unverifiable until that process exits (after a reboot, the boot check proves it gone).
  • macOS clock steps. A large forward wall-clock step (for example resuming a suspended VM, or correcting a clock that was hours behind) moves the machine's recorded boot time forward. A live daemon that is also refusing connections at that moment could then read as exited. Both conditions are needed; sleep does not cause it (measured).
  • Pane-less writes: orchestration's settled write to a session with no route is refused while any version is unverifiable (its callers hold no saved identity), instead of guessing the current daemon.

Scope, platforms, mixed versions

  • App-only. No daemon change, no wire change, no protocol bump. It works against every shipped daemon v1–v35, because it only changes how the app reads a connection result.
  • Windows. A missing named pipe (ENOENT) is the only exit evidence from the endpoint. Busy pipes, EPERM and timeouts are unverifiable. No process enumeration and no PowerShell: the probe uses process.kill(pid, 0) and os.uptime(). Nothing in docs/reference/windows-daemon-host-relocation.md is touched. An unverifiable version keeps its host folder pinned until it answers or exits.
  • Linux. A full accept queue returns EAGAIN, which reads as unverifiable.
  • SSH. Relay-backed terminals are unchanged, and SSH kills do not carry the new identity. A paired remote host runs this same code for its own daemon, so the decision stays on the host that runs the terminals. The owner-unverified marker is an already-shipped string.
  • Folder workspaces. Daemon sessions are not tied to git. Nothing here assumes a worktree.
  • Cost. When a version stays unreachable, startup waits up to the existing 2-second session-lookup deadline for it once, and lookups for panes with no route retry it at most once a second. A kill or settled write for a session with no route now waits for that owner lookup (up to 2 s) instead of going straight to the current daemon. In the common case (a startup stall) the version answers straight away.
  • Behavior change to state. A pane whose session really died while Orca was closed, at a moment when an unrelated old version still cannot be reached, now shows Retry instead of a fresh shell. This needs a version that stays unreachable after startup. It is the correct cost of not guessing.
  • Attach error replies. An attach that reaches a daemon but gets an error reply back (for example a rejected hello) now reads as owner-unverified and shows Retry, instead of clearing the pane's saved session and starting fresh (which left the original running with no tab). "Open a new terminal" still works.

Not in this PR (follow-ups)

  • Temporary: durable, retried kill orders for local daemon sessions (step 4).
  • Temporary: the degraded-mode provider (degraded-daemon-pty-provider.ts) still sends kills for a session with no route to its local fallback provider, and resize and signals for a session with no route still go to the current daemon. Same class of mistake, rare.
  • Temporary: the daemon's endpoint publisher (daemon code) and the app's killStaleDaemon still take a refusal as proof of death (endpointIsProvenDead). That affects replacing the current daemon, not the census. src/main/daemon/AGENTS.md now warns that a refusal alone is not proof outside the publisher.

Visual Proof

Live run on a Mac (M4 Air) with an isolated profile: a v36 build holds a terminal running sleep 100000, then a v37 build of each branch starts with a deliberate 1.5-second main-thread stall across the version check. The shell prints its own pid so you can tell the original from a copy.

Before (base): the tab comes back as a NEW shell (pid 80695) while the original sleep keeps running in the old version with no tab.

Base: after the stall the tab shows a new shell

After: the same stall, and the tab reattaches to the ORIGINAL shell (pid 69964) with its sleep still running. One session.

PR: the tab reattaches to the original shell

After, old version frozen (SIGSTOP): the tab keeps its saved session and offers Retry instead of starting a copy.

PR: owner-unverified message with Retry

After Retry (old version resumed): the original scrollback and running sleep are back.

PR: Retry restores the original session

Manage Sessions with the duplicate the base build created (two rows, same id). Before: Kill on the second row (the old-version orphan) killed the first row instead, which was the open tab's own session, and the tab closed.

Base: two identical rows

Base: after Kill on row 2 the open tab closed

After: Kill on the second row ended exactly that session; the tab's own session stays and the tab keeps working.

PR: one row left after Kill

PR: the tab stays on its own session

Testing

Real sockets, no Electron, run with vitest run --config config/vitest.config.ts:

  • daemon-legacy-census-unreachable.test.ts runs real daemon servers for v35 and the current version. A previous app run leaves a live session in v35, and the connection check to v35 is forced to time out. On the base (Preserve legacy daemon ownership when liveness is unavailable #16900's head, e6b3d2392b3), five of its cases fail: the restore starts a second copy under the same id (with and without another previous version), still starts fresh when v35 answers after the stall, attaches a pane saved against the orphan to the other copy, and reads closing the stuck pane as "already gone". It also covers closing a current-daemon session that was never reattached while v35 is unreachable: killed with its saved identity, refused without one.

  • daemon-endpoint-probe.test.ts: a real listener, the main thread blocked across the probe's timer; the connection that finished during the block wins. Fails without the timer change.

  • daemon-endpoint-full-accept-queue.test.ts starts a child listener that blocks its own event loop with a backlog of 1. Positive control: macOS returns ECONNREFUSED for it. It is kept as unverifiable; once the process is killed it reads as exited; and a killed listener whose record names a live process reads as exited only when the record predates this boot.

  • daemon-endpoint-verdict.test.ts: the probe's results, and the mapping from attach failures to answers (timeouts and dropped connections become owner-unverified; "absent" and "killed" pass through).

  • pty-kill-saved-pane-incarnation.test.ts: tab close, runtime kill and stop-and-wait pass the saved identity; panes that disagree pass none.

  • Cleanup, grace retry and self-shutdown wait: daemon-init-restart-sequence.test.ts and daemon-init-wedged-daemon-grace.test.ts. Manage Sessions: pty-management.test.ts.

  • Revert checks. Every behavior change was undone on its own and its tests went red. The per-version log line and the Manage Sessions row key have no test.

  • Tests updated to the new rule (each one pinned the old behavior): "does not trust persisted incarnation proof from an incomplete inventory" (owner resolution), "pins colliding unmapped legacy ids falling through to the current daemon" (router), a timed-out probe returning cleaned: false (restart sequence). The router's test double no longer counts every listed session as attached, which had hidden the tab-close case above.

  • Live QA on a Mac (M4 Air), isolated profile, plain sleep shells: the screenshots above.

  • Not verified here: Windows and Linux runs (CI covers the unit tests), SSH and WSL, packaged builds, and how often this happens in the field. The new log lines make that countable.

  • I manually tested these changes locally

  • Automated tests added/updated, or explained why not below

Linked Issue

Part of STA-2018. Orphans already on users' machines stay until the user ends them.

Checklist

  • I explained what changed and why (ELI5, the user-facing before/after, the mechanism, and why over the alternatives)
  • Before/after screenshots or videos attached for UI changes, or N/A with reason
  • Self-reviewed for correctness, security, and performance
  • Cross-platform, SSH/remote, and path/shortcut impact considered (or N/A)

Author: Brennan Benson (@BrennanKB5)

…nstead of reading it as gone

At startup a previous daemon version that missed a 1-second connection check
was dropped from the version list for the whole run. Session lookup then
answered "absent", the restored tab certified its live agent dead, dropped its
saved session and resumed a second copy of the same conversation on top.

One endpoint probe now answers live / unverifiable / exited and every caller
uses it; the yes/no probe is deleted. "Exited" needs the endpoint gone, or a
refusal while the recorded daemon process is gone: macOS refuses a live
listener whose accept queue is full. The census keeps unverifiable versions
(and logs each), so a restored pane shows the shipped "left the saved session
unchanged, Retry" state instead of starting over. An exact match on the saved
session identity still resolves panes whose session lives in a version that
answered.

Also:
- attach-only failures that never heard the owner answer (timeouts, lost
  connections, refusals without proof of exit) now read as owner-unverified,
  so the renderer keeps the pane's binding instead of starting a fresh session;
- a version that exits mid-run lists as empty, re-derived on every listing;
- kills and writes for a session with no route resolve its owner instead of
  asking the current daemon, whose "not found" read as already gone;
- Manage Sessions kills the clicked row by protocol version and incarnation;
- cleanup, the replacement health-check grace and the self-shutdown wait no
  longer read a timed-out probe as the daemon exiting.

Part of STA-2018.
@brennanb2025
brennanb2025 marked this pull request as ready for review September 30, 2026 00:53
@greptile-apps

greptile-apps Bot commented Sep 30, 2026 •

Copy link
Copy Markdown

RetriggerConfidence Score: 2/5

[Medium risk] Refines daemon endpoint liveness detection logic.

The PR does not appear safe to merge while the three outstanding previous findings remain.

Findings

  1. P1 Live pane input is dropped ▶
  2. P1 Clock jump hides live daemon ▶
  3. P2 Daemon rejections become retry prompts ▶

Summary

This PR distinguishes unreachable previous daemons from exited ones, preserves uncertain session ownership during restoration, and routes session stops by saved identity. Since the previous review, it also sends a pending split-pane stop before retiring the saved binding.

Reviews (3) · Last reviewed commit: "fix(terminal): stop a closed split's ses..."

Comment on lines 94 to +95
write(id: string, data: string): boolean {
return this.adapterFor(id).write(id, data)
return this.knownOwnerFor(id)?.write(id, data) ?? false

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P1 Live pane input is dropped When the same session ID is live in the current and a legacy daemon, a later complete inventory removes its cached route. This write then finds two possible owners and returns false, even for an already attached pane. The input path does not surface that failure, so the user’s keystrokes silently disappear. Preserve or recover the pane’s established owner before refusing its input.

Knowledge Base Used:

Comment on lines +70 to +73
function isContactFailure(error: unknown): boolean {
return (
error instanceof DaemonProtocolError ||
isDaemonGoneError(error) ||

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 Daemon rejections become retry prompts A daemon can explicitly reject an attach, for example with “Invalid token” during the hello exchange. That rejection is a DaemonProtocolError, so this check treats it as lost contact. The pane then shows owner-unverified and offers Retry instead of explaining the rejection; retrying with the unchanged token cannot fix it. Keep daemon-answered errors distinct from timeouts and connection loss.

Knowledge Base Used: Terminal runtime and interaction

@pullfrog pullfrog 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.

ℹ️ No critical issues — one cross-cutting tradeoff worth confirming.

Reviewed changes

  • Three-valued endpoint probe (daemon-endpoint-verdict.ts): probeDaemonEndpoint returns live / unverifiable / exited, proving exited only on a missing endpoint or a refusal while the recorded process is gone; attachOnlyFailure maps attach errors to TerminalHostGoneError (proven) or TerminalSessionOwnerUnverifiedError (uncontactable). The old yes/no probeDaemonSocket is deleted and isDaemonEndpointGoneError is demoted to isDaemonEndpointConnectRejection.
  • Census keeps unverifiable versions (daemon-legacy-adapters.ts): only exited versions are dropped and reclaimed; unverifiable ones are retained and logged.
  • All app-side readers follow the rule: attach (daemon-pty-session-spawn.ts), inventory (daemon-pty-session-inventory.ts), router kill/write (daemon-pty-router.ts), self-shutdown wait (daemon-protocol-cleanup.ts), replacement grace (daemon-replacement-preflight.ts), adoption (daemon-out-of-process-launcher.ts), and the removal of the local translation in stable-owner.ts.
  • Exact saved identity (daemon-session-owner-resolution.ts): an exact persisted-incarnation match in a version that answered is accepted even when the inventory is incomplete.
  • Manage Sessions kill by clicked row (pty-management.ts, renderer): killOne now takes {sessionId, protocolVersion, incarnationId?}.

I traced the two riskiest cross-cutting surfaces and found them safe: the removed stable-owner.ts translation still classifies correctly on the paired-runtime path (classification runs host-side in attachOnlyFailure before the RPC, and the renderer keys off the terminal_pane_owner_unverified message), and the removed current-daemon fallback in router.write/shutdown does not drop input for a routed pane.

ℹ️ Keeping an unverifiable version makes every aggregate daemon listing fail closed

DaemonPtyRouter.listProcesses joins its adapters with Promise.all, and an unverifiable adapter's listProcesses rethrows rather than returning [] (daemon-pty-session-inventory.ts only treats a proven exit as empty). So while any retained version is unverifiable, the router's whole listing rejects. That is the deliberate fail-closed contract for stop/liveness flows, but the census change widens its blast radius: a version that previously would have been dropped now sits in the router and fails those flows until it answers or is proven exited.

Technical details
# Keeping an unverifiable previous version fails closed every aggregate daemon listing

## Affected sites
- `src/main/daemon/daemon-pty-session-inventory.ts:97-104` — `listProcesses` rethrows unless `connectFailureProvesExit` returns true (status `exited`); `unverifiable` rethrows.
- `src/main/daemon/daemon-pty-router.ts:215-222` — `listProcesses` is `Promise.all(allAdapters().map(...))`, so one unverifiable adapter rejects the whole call.
- `src/main/daemon/daemon-legacy-adapters.ts:52-64` — the census now keeps an `unverifiable` version.
- `src/main/runtime/unstopped-pty-verification.ts:38-58` — a rejected listing becomes `status: 'unverifiable'`, which blocks worktree removal (`force` required).
- `src/main/ipc/pty/runtime/kill.ts:279-297` — `verifyPtyStopped` rejection marks the PTY liveness unverifiable and returns `false` for the close.
- `src/main/daemon/daemon-provider-state.ts:99-116` — `listLiveDaemonPtyIds` returns `null` (via `allSettled`), so startup Codex pane reconciliation is skipped.
- `src/main/daemon/daemon-pty-router.ts:61-64` — `supportsAgentSessionClaims()` returns `every(...)`, so a retained old (pre-v26) version flips it false.

## Required outcome
Decide whether this widened fail-closed surface is acceptable for the duration a version is
unverifiable. In the target scenario (a startup stall) it is transient because the version soon
answers; it is persistent only for a version kept unverifiable by a recycled-stale-pid record, which
the PR body accepts. If intended, a line in the PR body / `daemon/AGENTS.md` would make the tradeoff
explicit; if not, the listing needs to distinguish "a version I could not ask" from "a version that
answered and holds nothing".

## Open questions for the human
- Is blocking worktree removal (until Force) while an unrelated previous version is unverifiable the
  intended cost, or should the per-adapter listing tolerate an unverifiable peer for non-destructive
  reads?
- Should `supportsAgentSessionClaims()` treat an unverifiable legacy adapter as absent rather than as
  a veto?

Pullfrog  | Fix it ➔ | View workflow run | Using DeepSeek Flash (free via Pullfrog for OSS) | 𝕏

…ous version cannot be reached

A tab not reattached this run has no route, so its close resolved the owner
without an identity and was refused whenever any previous version was kept
unverifiable, even when the session lived in a version that answered. Local
kills now carry this run's incarnation, else the pane's saved one, and the
router resolves the owner by it.
…win over the probe timer

After a stall, the event loop runs expired timers before polling I/O, so the
1 s check reported a timeout for an endpoint that had already accepted. The
timeout is now reported one turn later, after that poll.
…dates this boot

A crashed version leaves its socket file and pid record behind. Once the
recorded pid names another live process, or one this user may not signal,
the version stayed unverifiable on every launch until that process exited.
A record started before the last boot cannot be that process.
Comment on lines +104 to +106
if (liveness.status === 'live' && startedBeforeThisBoot(record.startedAtMs)) {
// Why: whatever answers to that pid now, it is not a process recorded before the last boot.
return { status: 'exited' }

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P1 Clock jump hides live daemon If the wall clock jumps forward by more than five minutes after a daemon writes its PID record, and that live daemon's full accept queue refuses connections, this check can report that it exited. Startup then drops the daemon version from its census, so a restored pane can treat its running agent as gone and start a second copy.

Knowledge Base Used: Terminal runtime and interaction

Comment thread src/main/daemon/daemon-pty-router.ts Resolved

@pullfrog pullfrog 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.

✅ No new issues found.

Reviewed changes

Since the prior pullfrog review at e20f86a4, eight commits land on this branch:

  • Saved-identity shutdown routing (78b3730962): local-pty-shutdown-identity.ts resolves the pane's saved incarnation when ptyIncarnationById has none, and it is threaded as expectedIncarnationId through shutdownProviderAndDetectExit → DaemonPtyRouter.shutdown → ownerFor(id, expectedIncarnationId) → DaemonSessionOwnerResolver.resolve. A tab close can now reach the version that owns the session even while another version stays unlistable.
  • Exact-incarnation match without a complete inventory (daemon-session-owner-resolution.ts): an exact persisted-incarnation match in a version that answered is accepted even when another version stayed silent, on the argument that incarnations are random per session (session.ts:26).
  • Probe timeout yields to a completed connect (daemon-endpoint-probe.ts): the timer now schedules its unknown result one event-loop turn later, so a connect the kernel completed during a main-thread stall is observed before settle('unknown').
  • Boot-time exit proof (daemon-endpoint-verdict.ts): a refused connection with a live recorded pid is read as exited when the record's startedAtMs predates the current boot by more than the 5-minute tolerance, proving the pid was recycled.
  • Unverified-pane log (stable-owner.ts): a pane left owner-unverified now logs [daemon] Keeping pane session <id> unverified.

I traced the shutdown routing end to end (renderer pty:kill → stopRendererOwnedPtyProcess → shutdownProviderAndDetectExit → router ownerFor), confirmed the passed expectedIncarnationId rides through opts to provider selection while the live ptyIncarnationById value still filters observed exits, and ran the changed suites locally (daemon-endpoint-probe, daemon-endpoint-verdict, daemon-legacy-census-unreachable, pty-kill-saved-pane-incarnation, daemon-pty-router, pty-management): 6 files, 89 tests pass.

ℹ️ PR description still states an exit-proof rule the boot-time commit narrows

The PR body's "Differences from the reference implementations" bullet ("Proof of exit uses only the signal-0 check (ESRCH), not the process start time… a stale pid record whose number now belongs to an unrelated process keeps that version unverifiable") and the Windows bullet ("A missing named pipe (ENOENT) is the only exit evidence… the probe uses process.kill(pid, 0)") no longer match the code. Commit 9a678369cd adds a second proof: a live recorded pid whose startedAtMs predates the current boot now reads as exited. Worth updating those paragraphs so the documented cost matches the heuristic that replaced part of it.

Technical details
# PR body exit-proof description predates the boot-time heuristic

## Affected sites
- `src/main/daemon/daemon-endpoint-verdict.ts:104-115` — `recordedDaemonVerdict` returns `{ status: 'exited' }` when `liveness.status === 'live'` and `startedBeforeThisBoot(record.startedAtMs)` is true; this is a new exit proof beyond the signal-0 check the description names.
- PR body sections "Differences from the reference implementations" (last bullet) and "Scope, platforms, mixed versions → Windows" — still assert signal-0 `ESRCH` / missing endpoint as the only exit evidence.

## Required outcome
The description's exit-proof and stale-pid-cost claims match the code after commit `9a678369cd`.

## Open questions for the human
- Does the Windows note need its own wording (e.g. a recycled pid record predating boot now also proves exit)?

Pullfrog  | View workflow run | Using DeepSeek Flash (free via Pullfrog for OSS) | 𝕏

…retires the saved binding

Closing a split pane that never reconnected this run sent the pane-close intent
first, so main deleted the pane's saved incarnation before the stop read it and
refused the stop while a previous daemon version was unverifiable, leaving the
session running with no pane. The stop is now sent first, as whole-tab close
already does.

@pullfrog pullfrog 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.

✅ No new issues found.

Reviewed changes

Since the prior pullfrog review at 23298572fa, one commit lands on this branch:

  • Stop a closed split before its close intent retires the saved binding (use-terminal-pane-close-actions.ts, a2493914b8): the split-pane close path now issues retireUnboundIpcTerminalPane (which dispatches pty:kill) ahead of commitTerminalSurfaceClose, matching the tab-close order in terminal-tab-close.ts:39 vs :251. Main's kill handler reads localPtyShutdownIncarnation from the saved binding before its first await (renderer-kill.ts:98-100), so the closeTerminalSurface IPC that deletes that binding must not be dispatched first.
  • New regression test (pending-split-close.test.ts): asserts window.api.pty.kill's invocation order precedes closeTerminalSurface. I reverted just the ordering and confirmed the test fails, then passes with the commit — it is real coverage, not theatre.
  • Comment only (renderer-kill.ts): the "read before any await" rationale now names the tab or pane close intent.

Pullfrog  | View workflow run | Using DeepSeek Flash (free via Pullfrog for OSS) | 𝕏

@brennanb2025

Copy link
Copy Markdown
Contributor Author

Review status: ready for your merge decision (not merged)

The problem. On a slow startup (the app's main thread stalls about a second while it checks a terminal service left over from the previous Orca version), Orca decided that version's sessions were gone. The tab marked the running agent dead and started a second copy of the same conversation while the original kept running unseen, pinning the old service forever. Manage Sessions' Kill could also end the wrong one of the two identical rows this left behind, including the open tab's own session.

What changes for you. After a slow startup the tab reconnects to the agent that was already running. If the old version really cannot be reached, the tab shows the existing "Orca left the saved session unchanged … Retry" message, and Retry recovers once it answers. No second copy is started. Kill in Manage Sessions ends exactly the row you clicked. Closing a tab or a pane still ends its session while another old version is unreachable. Nothing is killed automatically; orphans from before this PR stay until you end them.

Fixed during review (on top of the original commit).

  • Closing a tab or split pane that had not reconnected yet left its session running with no tab whenever any old version was unreachable, even when the session lived in a version that answered. Local kills now carry the pane's saved session identity (78b37309625, a2493914b8c).
  • The stall itself is now handled at the source: the connection check reports its timeout one event-loop turn later, so a connection that finished during the stall counts. Live QA confirms the stall now reads as connected (68a839a2cfb).
  • A crashed old version whose pid record outlived a reboot stayed "unverifiable" on every launch, which kept session listings failing. A record that predates the current boot now proves the process gone (9a678369cd4).
  • A log line for each pane left unverified (13bf68f3587), a corrected code comment (78f928272af), and three CI gate fixes (9a8742d7518, 76c79e8f1e5, 23298572fa1).
  • PR body corrected where it overclaimed (the health-check retry, test coverage, write behavior) and now lists the known limits.

Deferred (stated in the body).

  • While an old version is genuinely alive but not answering, listing every version's sessions fails, as it already did on main for a version that froze after startup: the Resource Manager shows its sessions error and a workspace delete asks for Force Delete. Temporary, owned by step 3 (fix(terminal): list, stop, open and delete past a terminal-service version that does not answer #22046).
  • A crashed old version whose pid was reused without a reboot stays unverifiable until that process exits. On macOS a large forward clock step (VM resume) while an old daemon is also refusing connections could read it as exited. Both are rare and stated.
  • A route-less stop's owner lookup uses its own 2-second deadline and does not follow a shorter caller deadline (not proven to matter).
  • A failed tab-close kill to an unreachable version is not retried yet (step 4). Resize and signals for a session with no route, and the degraded-mode provider's kills, still go to the current or local provider.
  • Nits: the owner-unverified toast shows raw text on restart; a settled write to a session no version holds re-lists every daemon each time.

Verified.

  • Live on a Mac (M4 Air), isolated profile, plain sleep shells, a deliberate 1.5 s stall. Base: the tab came back as a new shell while the original ran on, and Kill on the old-version row killed the tab's own session. PR (23298572fa1): silent reattach to the original shell; with the old service frozen, Retry message then Retry restored the original scrollback; Kill ended exactly the clicked row.

Base: after the stall the tab is a new shell

PR: the tab reattaches to the original shell

PR: Retry message while the old version is frozen

PR: Kill ended only the clicked row

  • Two review rounds plus an architecture challenge (kept the design, no split) and a readiness checklist (pass, no P0/P1/P2). Real-socket tests for each fix fail when the fix is reverted. Typecheck clean (node and web). CI at a2493914b8c: 41 pass, 16 skipped, 0 failing.
  • No open PR already implements this fix, and main still has the bug.

Not verified. Windows and Linux beyond CI unit tests, SSH and WSL, packaged builds, the path where the check still times out at the new head (the stall now always reads as connected), and closing a tab while two sessions share its id.

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.

1 participant