Repository navigation
fix(terminal): list, stop, open and delete past a terminal-service version that does not answer - #22046
Conversation
…unreachable ones as unverifiable Manage Sessions already listed and killed across every daemon protocol generation, but it presented them as one undifferentiated list and had two places where it asserted more than it observed. Deleted, not guarded: - `manage-sessions-format.ts` mapped `!isAlive` to the word `exited`. The daemon's own producer (`listLiveTerminalHostSessions`) skips every non-alive session and hard-codes `isAlive: true` on the rest, and the adapter filters on `isAlive` again, so that branch could never fire on a real row. Its only possible effect was to name a verdict from client-side bookkeeping. - `pty-management.ts` collected sessions with `Promise.allSettled` and dropped every rejected adapter with `r.status === 'fulfilled' ? r.value : []`. A generation Orca could not reach therefore vanished from the panel, and its sessions were silently uncounted. `listSessions` now returns a per-generation inventory instead of a flat list. A generation that answered is `live` and carries its sessions; one that did not is `unverifiable` and carries no session array at all, because an empty array reads as a counted zero. The vocabulary is the one fixed in docs/reference/ssh-execution-boundary.md, and only those two arms appear because this channel has no refuse or retire signal to report. The panel groups rows under a generation label whenever more than one generation exists, marks which one is current by adapter identity rather than by comparing against PROTOCOL_VERSION, and shows the header total as a lower bound (`3+`) while any generation is uncounted. This makes the accumulation visible. It does not reduce it.
|
Understand this PR’s impact Explore downstream dependencies and potential security impact with Blast Radius. 📝 WalkthroughWalkthroughThe PTY management IPC response now groups sessions by daemon protocol generation. Each generation is marked live or unverifiable when listing fails. The preload API and web stub use the new response shape. The settings UI renders generation headers, incomplete counts, and unreachable-generation notices. Kill operations route through the matching adapter. Tests cover generation metadata, listing failures, session counts, and state formatting. Priority: ➖ Normal Priority: ➖ Normal Estimated code review effort: Merge Risk: 🔵 Low · up to Unreachable generation counts appear in English in non-English settings UI. Localize this label before merge or accept the bounded presentation inconsistency. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 34.78% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 23 functions across 9 files. (1 skipped: 1 unsupported.)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Advanced
Run ID: 34abb4fa-47a6-4cf9-9905-3fdc6f9a0a54
📒 Files selected for processing (10)
src/main/ipc/pty-management.test.tssrc/main/ipc/pty-management.tssrc/preload/api-types.tssrc/preload/api/pty-management-api.tssrc/renderer/src/components/settings/ManageSessionsSection.tsxsrc/renderer/src/components/settings/ManageSessionsTable.tsxsrc/renderer/src/components/settings/manage-sessions-format.test.tssrc/renderer/src/components/settings/manage-sessions-format.tssrc/renderer/src/i18n/locales/en.jsonsrc/renderer/src/web/preload-api/web-terminal-api.ts
Included review availability: Your plan provides up to 10 included reviews per hour; 2 remain after this review.
| export function formatGenerationSessionCount(generation: PtyManagementGeneration): string { | ||
| return generation.contact === 'live' ? String(generation.sessions.length) : 'unverifiable' |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🔎 Supported by static analysis
🏁 Script executed:
sed -n '1,90p' src/renderer/src/components/settings/manage-sessions-format.ts
sed -n '1,85p' src/renderer/src/components/settings/ManageSessionsTable.tsx
sed -n '12010,12035p' src/renderer/src/i18n/locales/en.json
rg -n '"unverifiable"|unverifiable|ManageSessionsTable' src/renderer/src --glob '!components/settings/manage-sessions-format.ts'Repository: stablyai/orca
Length of output: 41858
🏁 Script executed:
#!/bin/bash
set -e
printf '%s\n' '--- ManageSessionsTable locale resources ---'
rg -n -C 8 '"ManageSessionsTable"' src/renderer/src/i18n/locales
printf '%s\n' '--- translation call conventions near settings UI ---'
rg -n -C 3 "translate\('auto\.components\.settings\.ManageSessionsTable|translate\('components\.settings\.ManageSessionsTable|formatGenerationSessionCount" src/renderer/src/components src/renderer/src/i18n --glob '*.ts' --glob '*.tsx' --glob '*.json'
printf '%s\n' '--- translate declaration ---'
rg -n -C 12 "export function translate|function translate|const translate" src/renderer/src/i18nRepository: stablyai/orca
Length of output: 9083
Localize the unverifiable session count at the UI boundary.
formatGenerationSessionCount() returns the English literal unverifiable, and GenerationHeader renders it directly. Non-English locales therefore display this label in English. Keep the semantic value in the formatter, but translate it in GenerationHeader and add the corresponding locale key. Do not return null, because the existing tests require this state to remain distinct from 0.
![pullfrog[bot]](https://avatars.githubusercontent.com/in/1768019?s=60&v=4){kind=small}
There was a problem hiding this comment.
ℹ️ No critical issues — a few minor suggestions inline.
Reviewed changes
- Main-process generation inventory —
getDaemonAdapters()now returns{ adapters, current }(current taken from the router'sgetCurrentAdapter()identity, not aPROTOCOL_VERSIONcomparison), andcollectGenerationsreturns a per-adapterDaemonGenerationInventoryinstead of flatteningallSettledresults, so a generation whose listing rejected is reported asunverifiablerather than silently dropped. listSessionsIPC shape — the channel now resolves to{ generations, degraded };killAll/killOneconsume the same adapter set through a namedreachableSessionsnarrowing.- Removed the dead
!isAlive → 'exited'mapping —listLiveTerminalHostSessionsfilters non-alive sessions and hard-codesisAlive: true, andDaemonPtySessionInventory.listSessionsre-filters onisAlive, so the branch could only name a termination verdict from client bookkeeping. I verified both filters, so the deletion is safe. - Renderer grouping —
ManageSessionsTablegroups rows under aGenerationHeaderwhen more than one generation is present, renders an unverifiable body message, and marks the header total as a lower bound (2+); optimistic kill-all clears only live generations. - Preload/types/web stub —
PtyManagementGenerationmirrors the main type; the web client reports no generations. - Tests — new main-IPC tests for unverifiable reporting and renderer formatter tests, with red-then-green evidence.
I re-ran pnpm tc:node, pnpm tc:web, pnpm run check:code-quality:changed, verify:localization-{extraction,catalog,runtime-catalog}, and the focused suites (23 passed). The channel is Electron IPC only, so the shape change is not on the remote wire.
ℹ️ Nitpicks
ManageSessionsTable.tsx— for a single generation with zero sessions the panel now renders a left-alignedpy-2paragraph where it previously used the centeredpx-3 py-8block, so the "single-generation installs look exactly as they do today" claim is slightly off for the empty case. Consider keeping the centered empty state whengenerations.length === 1.
DeepSeek Flash (free via Pullfrog for OSS) ｜ 𝕏
…fiable' into brennanb2025/manage-sessions-generation-legibility # Conflicts: # src/main/ipc/pty-management.test.ts # src/main/ipc/pty-management.ts # src/preload/api-types.ts # src/preload/api/pty-management-api.ts # src/renderer/src/components/settings/ManageSessionsTable.tsx # src/renderer/src/i18n/locales/en.json
 |
Comments Outside DiffThese findings could not be posted inline.
|
While one old terminal-service version was frozen, every listing that merged all versions waited out the frozen one (30 s) and then failed as a whole, taking the versions that did answer down with it. Adds one per-version listing primitive (live / exited / unverifiable, one absolute deadline) in src/main/daemon. The router and the degraded provider expose it as listProcessesBySource, which also names the ids a silent version was last known to hold (its routes plus the ids this app attached), derived from state already held. listProcesses is now requireComplete(bySource), so every caller not migrated keeps today's fail-closed contract. Manage Sessions lists through it with a 3 s deadline, so a frozen previous version reads unverifiable in 3 s instead of withholding the whole list for 30 s, and a version whose daemon is proven gone reads exited instead of "live with no sessions".
…session
While any old terminal-service version was frozen, every local stop (terminal
stop, worktree stop, tab and split close, mobile close) reported "not
confirmed stopped", even for a terminal of the current version that had
stopped. Each stop proved itself with a census of every version, and the frozen
one failed the census after waiting out its 30 s timeout.
The router and the degraded provider now remember which version they asked to
stop each session (in memory, replaced by the next stop or spawn of that id,
capped), and confirmPtyStopped asks that owner alone: true once it no longer
holds the session, null when it cannot answer. null is never read as an exit.
verifyPtyStopped uses it when the provider has it; single-source providers keep
the process list. The workspace-delete verdict for failed stops asks each id's
owner the same way instead of listing every version.
The observed-exit close test pinned the old census dependency as a known limit
("exact-owner verification across daemon generations is separate work"); it now
expects the stop to confirm, the same as the healthy case.
… does not answer While an old terminal-service version was frozen, deleting any workspace asked for Force Delete: the sweep listed every version at once and failed as a whole. The provider sweep now stops this workspace's terminals from the versions that answered. For a version that did not answer, the ids of this workspace it was last known to hold (its routes plus the ids this app attached) go down the existing per-id stop; that owner cannot confirm, so the delete refuses with the Force Delete hint exactly as it does for a known live terminal today. When the silent version held no known terminal of this workspace, the delete goes ahead and says so: an optional uncheckedTerminalServices field on the removal result, the existing warning for the CLI and mobile, and one non-blocking note on the desktop pointing at Manage Sessions. Missing-worktree reconciliation sweeps the answered versions instead of nothing. The desktop removal slice's preserved-branch bookkeeping moves to its own file to keep the slice under the line limit.
…d and name what did not
Manage Sessions
- Kill all keys every session by its exact identity (version, id, incarnation),
as Kill already does. A version that stops answering mid-kill has its sessions
reported as unverified (never counted killed, never passed to the exact tab
cleanup) and is not polled again; a version that did not answer at all is
counted as not asked. The result toast warns instead of reporting success.
- Kill on a version Orca lost contact with says so, instead of "it may already
be gone".
- When the same id is live in two versions, only the copy this app attached
can open the tab; the other row is not clickable.
- A previous version is shown only while it has sessions or could not answer;
an exited or emptied one disappears. The count slot says "unverifiable" in the
user's language, the lower-bound note is visible text, and each version group
is one table row group with a heading, so columns line up and the single-
version look (border, centred empty state) is back.
Resource Manager
- pty:listSessions returns {sessions, complete, unverifiable} instead of a
bare array, so a partial list can never pass for a whole one. It shows the
sessions that answered plus one line saying an older terminal service did not
answer, and the orphan count reads as a lower bound. The workspace activation
gate treats an incomplete list as it treats a failed one: blocked.
Two real daemon servers (the current version and version 35); version 35 sits behind a byte proxy in its own process, frozen by SIGSTOP of that recorded pid and released and killed by the same pid in teardown. Adapters come from the app-start path (census, router, discovery). Covers: the per-version listing returns the current version within the deadline; a current-version stop confirms; a frozen-version stop never confirms; workspace delete proceeds and names the unchecked version when only current-version terminals are known, refuses with the Force Delete hint when the frozen version holds one of the workspace's terminals, and refuses for a restored pane whose saved id is the only link (the tab-less residual goes ahead and is reported). The runtime is a thin stand-in running the runtime's own sweep loop through stopAndWait. Skipped on Windows (no SIGSTOP).
…elete notes for older terminal services
…untimeEnvironmentId and the test adapter mock
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| const owner = this.owners.get(sessionId) | ||
| const probe = owner | ||
| ? owner.probePtyLiveness | ||
| ? owner.probePtyLiveness(sessionId) | ||
| : Promise.resolve(owner.hasPty?.(sessionId) ?? null) | ||
| : probeWithoutOwner() | ||
| try { | ||
| const live = await withinDeadline(probe, deadlineMs) | ||
| return live === null ? null : !live |
There was a problem hiding this comment.
When two daemon generations hold the same session ID, stopping the routed copy does not stop the other one. This check asks only the recorded owner whether the ID is gone; its answer can mark the session as stopped even while the other generation still holds a live copy. The stop path can then clear the ID’s state and report an exit for a terminal that is still running.
… delete A tab restored as "owner unverified" keeps its saved session id in the workspace session, but nothing else in main names that session: the transport never bound it, so the runtime graph has no id, and the silent version's routes and attached ids do not hold it. Delete went ahead past it. While a daemon version does not answer, the delete sweep now also counts the ids this worktree's saved tabs are bound to (read from the workspace session main already holds) that no answering version listed. They go down the per-id stop, whose owner cannot answer, so the delete refuses with the Force Delete hint. The real-daemon test now reaches those ids only through that evidence: the routed-before-freeze case passes no runtime, and the restored-pane case passes only the saved session binding.
…s listing deadline - pty:listSessions (Resource Manager and the activation gate) asks each daemon version under the 3 s user-facing deadline; a frozen version made it wait the 30 s request timeout on every poll. The real-daemon test times it. - The workspace-delete sweep's listing waits at most 3 s within its budget, so stops it finds still have time before the teardown deadline. - Missing-worktree reconciliation's shared snapshot also memoizes the per-version listing, so N missing worktrees cost one listing again, not N. - A read that throws before returning a promise marks only its own version unverifiable instead of rejecting the whole listing.
…ne, and read a proven-gone owner as absent - The router and degraded provider record the owner only once it is known. A later stop of the same id whose owner lookup fails no longer erases the owner an earlier stop recorded, so that stop can still be confirmed. - The owner's liveness probe takes the caller's deadline, with its 2 s as the floor; a daemon draining a kill no longer reads "not confirmed" at 2 s when the caller allowed longer. - A probe whose daemon is proven gone (the same connect-failure proof the listings use) answers absent instead of unknown; any other failure stays unknown. - Two comments still described the old census check.
… no "older" version, and stay honest at zero - Delete note: one toast id so a batch delete shows one note; skipped where a bulk flow suppresses per-row notes; no longer says "older" (the silent version can be the current one); an "Open Manage Sessions" action reuses the existing handler (now shared as openManageSessions) instead of a written Settings path; for a workspace on a paired runtime host it names that host and offers no action, since this machine's Manage Sessions cannot list it. The CLI sentence names Settings > Terminal > Manage Sessions on that host. - Orphan-directory deletes report the unchecked version the same way as the registered paths. - Manage Sessions: no "(0+)" and no "At least 0"; the note shows only when something was listed, and names sessions. A restored tab that has not re-attached yet opens from the copy its saved incarnation names (or the only listed copy of its id), read from the workspace session main already holds. - Design system: the partial-list icon uses text-status-warning; the row's kill button keeps the ghost variant's colours. - Comment: mobile reads no worktree.rm reply. - All six catalogs updated.
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes
This run re-reviewed the delta since the prior pullfrog review at a3cff6b — the base-branch merge plus seven commits that expanded the PR from a 10-file Manage Sessions change into an 85-file daemon-generation stack. I read the full diff end-to-end and ran the eight focused suites (61 tests, all passing).
- Per-version listing under one deadline — new
daemon-generation-listing.ts(withinDeadline,listPerGeneration,listDaemonProcessesBySource,USER_FACING_DAEMON_LISTING_TIMEOUT_MS),readSessions/readProcesseson the inventory, andlistProcessesBySourceon both the router and the degraded provider. A silent version is reported asunverifiablewith its last-known ids instead of dropping out. - Stop confirmation by the owning version —
confirmPtyStoppedonIPtyProvider,DaemonStoppedSessionOwners,isStoppedPtyLive, and per-owner verification inunstopped-pty-verification, so a stop is confirmed without waiting on an unrelated frozen version. - Workspace delete past a silent version —
uncheckedTerminalServicesthreaded through teardown and removal, surfaced as a renderer toast and folded into the existing CLI/mobile warning wording. - Manage Sessions and Resource Manager show what answered —
collectGenerations/killAllDaemonSessionsreturn per-generation contact and unverified/unreached counts; the renderer groups rows by generation, words an unreachable one, and marks totals as lower bounds. pty:listSessionsshape — now{ sessions, complete, unverifiable }; the activation inventory throws (fail-closed) when incomplete. Channel is Electron IPC only, so this is not a remote-wire change.- Prior review feedback addressed — the count label now renders through
translate(...)with locale entries, and the first-row/border separator was fixed via restoredfirst:border-t-0plus aborder-tgroup separator.
ℹ️ Scope and base branch
The delta is materially larger than what the prior review saw: seven commits turned a focused Manage Sessions fix into an 85-file stack (daemon generation listing, stop confirmation by owning version, workspace delete across silent versions, and the Resource Manager's partial inventory). It also remains based on brennanb2025/r4-d1-census-unverifiable rather than main, so merging carries that branch's census work as well. Nothing here is blocked — surfacing it so the merge order is a deliberate choice.
DeepSeek Flash (free via Pullfrog for OSS) ｜ 𝕏
…ion note The note followed the per-row preserved-branch suppression, so a cleanup batch past a silent terminal-service version reported nothing. It now always shows; its fixed toast id keeps a batch to one note. Other suppression is unchanged. The note's Open Manage Sessions action is now passed in by the removal slice, so the toast module no longer imports the store (that import cycle left the worktree slice undefined when the store loaded through it).
| ) | ||
| for (const listing of silent) { | ||
| const owned = listing.lastKnownIds.filter((id) => id.startsWith(prefix)) | ||
| if (owned.length === 0 && evidence.size === 0) { |
There was a problem hiding this comment.
When more than one daemon generation is unreachable, this check uses session evidence accumulated from earlier generations. If one generation has a saved or last-known session ID, a later generation with no known IDs is not added to the unchecked-service warning. After a Force Delete, the result can omit that later generation even though its terminals were never checked.
…current one With the cap on every version, a current daemon that lists slower than 3 s (right after launch while panes reattach) read as silent: workspace delete skipped its sessions no one else knew about and named it unchecked, and the activation gate blocked. The per-version listing now takes a separate nonCurrentDeadlineMs. The current version (and the in-process provider) keeps the caller's budget: the delete sweep's RPC deadline, and for pty:listSessions the previous unbounded wait. A frozen previous version still ends at 3 s.
The 2 s floor let a confirmation outlive a shorter caller deadline by up to 2 s, which could time out a workspace delete's runtime sweep. The owner probe now uses the caller's remaining time when there is a deadline and keeps its 2 s only when there is none; confirm races the caller's deadline as given. Also corrects the record() comment: only spawn passes no owner now.
…for that session With the same id live in two versions and the tab's own copy in a version that did not answer, the other version's copy counted as "the only listed copy" and opened the tab. Main now passes each saved pane's session id with the incarnation the tab recorded (joined from the workspace session's pane-keyed incarnations and layout bindings). When the tab saved one, only the matching row backs it; the only-copy rule applies only when it saved none.
There was a problem hiding this comment.
ℹ️ No critical issues — two minor notes inline.
Reviewed changes
This run re-reviewed the five commits since the prior pullfrog review at 4739fd6 (98ff01f, b9424e2, 6f96a15, e8bf871, e8f06cb) — the delta that turned the Manage Sessions grouping into a bounded, per-version inventory and reworked the wording. I read the delta and the surrounding code, ran the ten focused suites (75 tests, all passing).
- Bounded the user-facing listings by the 3 s deadline —
pty:listSessions(Resource Manager and the activation gate) and the workspace-delete sweep now cap each per-version listing, and the missing-worktree reconciliation also memoizes the per-source listing, so a frozen version no longer makes every poll or batch delete wait out the 30 s request timeout. A read that throws synchronously before returning a promise now marks only its own version unverifiable. - Kept a stop's recorded owner — the router and degraded provider record the owner only once it is resolved, so a later stop of the same id whose owner lookup fails no longer erases the owner an earlier stop recorded. The owner's liveness probe takes the caller's deadline with the 2 s probe timeout as its floor, and a daemon proven gone now answers
false(absent) instead ofnull. - A saved tab bound to a silent version's session blocks delete —
persistedPaneSessionIdsForWorktreereads the workspace session main already holds, and while a version is silent the sweep counts the ids the worktree's saved tabs are bound to that no answering version listed, so the delete refuses with the Force Delete hint instead of silently proceeding. - A restored tab can open from its saved copy —
attachedis renamedbacksTabandmarkTabBackedSessionsrescues a not-yet-reattached row by its saved incarnation, or by being the only listed copy of its id. - Unchecked-version wording and toast — one toast id so a batch collapses to one note, no longer says "older" (the silent version can be current), stays silent at zero listed, points at Manage Sessions (per-host action), and the CLI sentence names the host; all six catalogs updated.
- Type narrowing — the runtime unregistered-removal
stopPtyscallback may returnvoid, handled with?? {}.
The two inline notes are informational: whether the batch-delete path should still surface the unchecked-version note, and a JSDoc that no longer matches the widened false semantics.
DeepSeek Flash (free via Pullfrog for OSS) ｜ 𝕏
| hasPty?: (id: string) => boolean | ||
| /** Exact provider readback: false only when the provider answered that the PTY is absent. */ | ||
| probePtyLiveness?: (id: string) => Promise<boolean | null> | ||
| probePtyLiveness?: (id: string, opts?: { deadlineMs?: number }) => Promise<boolean | null> |
There was a problem hiding this comment.
The JSDoc on line 144 now understates the contract: probePtyLiveness also answers false when the daemon is proven gone (connectFailureProvesExit), not only when the provider answered that the PTY is absent. Worth folding that second case in so the false / null boundary stays explicit.
There was a problem hiding this comment.
ℹ️ No new issues in the follow-up commit.
Reviewed changes
Re-reviewed the single commit pushed while the previous review was in flight: 505e79d ("a workspace cleanup batch shows the one unchecked-version note").
- Un-gated the unchecked-version note for batches —
remove-worktree.tsno longer skipsshowUncheckedTerminalServicesToastwhensuppressPreservedBranchToastis set, so a multi-select or cleanup batch now surfaces the note; the sharedunchecked-terminal-servicestoast id collapses the batch to one. - Simplified the helper — the toast's second parameter changed from
{ onThisMachine }to an optionalopenManageSessionscallback, which also removes theuseAppStore.getState()read from the toast module. - Tests — a cascade test asserts the batch path reuses the note id, and the toast unit test now passes the callback directly instead of mocking the store/helper.
Verified the two touched suites (27 tests) pass. The JSDoc nit raised on the previous review (pty-provider-contract.ts:144 still says false only means "provider answered absent", while probePtyLiveness also answers false for a proven-gone host) is untouched by this commit and remains the only open item — hence a comment review rather than an approval.
DeepSeek Flash (free via Pullfrog for OSS) ｜ 𝕏
…ent one The same rule as the other listings: a slow current version is never shown as "couldn't reach" because of Orca's own 3 s cap. The list, Kill all's polling and Kill's owner check wait for the current version as before and cap only the others; a frozen previous version still reads unverifiable after about 3 s.
| current: DaemonPtyAdapter | null, | ||
| nonCurrentDeadlineMs: number | ||
| ): number | undefined { | ||
| return adapter === current ? undefined : nonCurrentDeadlineMs |
There was a problem hiding this comment.
When the current daemon accepts a connection but stops answering listing requests, this change removes its three-second deadline. The request then waits for the RPC client's default 30-second timeout. Manage Sessions, status-bar and activation inventories, kill-all polling, and individual session kills are delayed instead of promptly reporting that the daemon could not be reached.
…savedPaneIncarnation The restored-tab rule had its own join of saved pane bindings to incarnations, where the last pane written won. It now reuses savedPaneIncarnation (the join the local stop path already uses), exported from local-pty-shutdown-identity.ts, so there is one definition: two saved panes naming different incarnations for one session id give no saved incarnation.
| registerDaemonManagementHandlers({ | ||
| getSavedIncarnationLookup: () => { | ||
| const session = store.getWorkspaceSession() | ||
| return (sessionId) => savedPaneIncarnation(session, sessionId) |
There was a problem hiding this comment.
Manage Sessions now calls savedPaneIncarnation for each listed session without an attached tab. Each call scans every saved tab and pane. With many saved panes and session rows, a refresh repeats the full scan for every row instead of building the lookup once, making the panel slower as both counts grow.
There was a problem hiding this comment.
Important
The current daemon version is now waited on with no deadline, so a connected-but-unresponsive current daemon stalls every listing — Manage Sessions, the Resource Manager, the activation gate, and kill-all polling — for the RPC client's 30 s default instead of reporting it promptly. One inline note on how to bound it.
Reviewed changes
This run re-reviewed the four commits since the prior pullfrog review at 505e79d (da2e98a, 631ff0e, 24c8e93, 9ad6aa4), read the full diff end-to-end, and ran the seven focused suites plus the real-daemon frozen-version suite (78 tests, all passing).
- Bounded the listing cap to versions other than the current one —
listPerGenerationtakes a per-source deadline, andProcessSourceListingDeadlines.nonCurrentDeadlineMsis threaded through the router, degraded provider,listAnsweredProcesses, the activation/status-barpty:listSessions, kill-all polling, andkillOne; the current version gets no deadline. - Honoured the caller's deadline in stop confirmation —
DaemonStoppedSessionOwners.confirmdrops its 2 s floor and passes the caller's deadline to the owner probe andwithinDeadline;probePtyLivenessuses the remaining budget when given one. - A restored tab's row follows the incarnation it saved —
savedIncarnationBySessionIdmaps each saved pane's session id to its tab-recorded incarnation, andmarkTabBackedSessionsmatches on it instead of any saved incarnation or the only listed copy. - Tests — new focused cases for the slow-current-version deadline, the caller-deadline stop, the multi-version tab attribution, and a real-daemon frozen-previous-version integration suite.
DeepSeek Flash (free via Pullfrog for OSS) ｜ 𝕏
| current: DaemonPtyAdapter | null, | ||
| nonCurrentDeadlineMs: number | ||
| ): number | undefined { | ||
| return adapter === current ? undefined : nonCurrentDeadlineMs |
There was a problem hiding this comment.
The current version now gets no deadline, so it is bounded only by the daemon client's 30 s REQUEST_TIMEOUT_MS. When the current daemon accepts a connection but stops answering listings, Manage Sessions, the Resource Manager, the activation gate, and kill-all polling all stall up to 30 s before the same unverifiable verdict can be reported — where the previous 3 s cap surfaced it promptly.
Technical details
# The current daemon version has no listing deadline
## Affected sites
- `src/main/ipc/pty-management-generations.ts:31` — `generationDeadline` returns `undefined` for `adapter === current`, so `collectGenerations`/`killAllDaemonSessions` call `readSessions(undefined)`.
- `src/main/daemon/daemon-generation-listing.ts:75` — `sourceDeadline(true, …)` returns the caller's `deadlineMs`, which `listAnsweredProcesses` leaves undefined (only `nonCurrentDeadlineMs` is passed).
- `src/main/ipc/pty/ipc/inspect.ts:76` — the local `pty:listSessions` path passes only `nonCurrentDeadlineMs`, so the current version is uncapped there too.
- `src/main/daemon/daemon-pty-session-inventory.ts` `readSessions` → `client.request('listSessions', undefined, undefined)` → default `REQUEST_TIMEOUT_MS = 30_000` (`src/main/daemon/client.ts:30`); `killOne` (`pty-management.ts`) also waits unbounded on the current owner.
## Required outcome
A wedged-but-connected current version must still surface (unverifiable / partial) within a bounded, user-facing time, rather than holding the IPC for the RPC default.
## Suggested approach
Keep a finite cap for the current version (a multiple of `USER_FACING_DAEMON_LISTING_TIMEOUT_MS`), or thread an explicit bound through `collectGenerations`/`listAnsweredProcesses` so the panel decides. Distinguishing "slow" from "unresponsive" needs the cap raised, not removed.
## Open questions for the human
- Is the intent to wait out the current daemon entirely, or only to stop a merely-slow current version from being mislabelled `unverifiable`? A modestly larger finite cap satisfies the latter without the 30 s stall.
There was a problem hiding this comment.
✅ No new issues in the follow-up refactor.
Reviewed changes
Re-reviewed the single commit pushed while the previous review was in flight: ff027d0 ("Manage Sessions reads saved incarnations through savedPaneIncarnation"). It is a refactor that removes a duplicate join; the localPtyShutdownIncarnation behavior is unchanged (it already used the disagree-aware helper).
- Unified the saved-incarnation join — the restored-tab rule now calls the exported
savedPaneIncarnation(local-pty-shutdown-identity.ts:19) instead of its own Map join, so two saved panes naming different incarnations for one session id yield no saved incarnation rather than letting the last write win. - Threaded a lookup instead of a map —
collectGenerations/markTabBackedSessionsnow takesavedIncarnationFor: (sessionId) => string | undefined; theregisterDaemonManagementHandlersdependency is renamedgetSavedIncarnationLookup, andattach-main-window-servicescloses over the current workspace session. - Tests — a new case asserts that two disagreeing saved panes leave both rows non-tab-backed.
DeepSeek Flash (free via Pullfrog for OSS) ｜ 𝕏
The delete paths now read the workspace session for saved tab bindings. The worktrees IPC harness store and the Windows delete test's own store lacked the method, so 30 tests in 6 files failed. The SSH exact-options expectation now includes the (empty) persistedPaneSessionIds.
A hung current daemon read as a partial listing, so Resource Manager lost its "Daemon is not responding" banner, its Restart button and the status-bar warning icon. The partial listing is now only for a silent previous version; when the current version does not answer, pty:listSessions rejects with its error, which the renderer already shows. The activation gate blocks either way.
… "older" nor "mobile" The silent version can be the current one, and mobile reads no worktree.rm reply; two comments and two test names still said otherwise.
There was a problem hiding this comment.
ℹ️ No new issues in the follow-up commits — two prior notes remain open.
Reviewed changes
This run re-reviewed the three commits since the prior pullfrog review at ff027d0 (c8cc942, 63b0670, 482ab79), read the incremental diff and the surrounding listing code, and ran the three touched suites (30 tests, all passing).
- A silent current version now fails
pty:listSessions—listAnsweredProcessesthrows the current version's error instead of returning a partial listing, so the Resource Manager keeps its "not responding" banner, Restart button and status-bar warning, and the activation gate blocks as before.complete: falseis now reserved for a silent previous version. Two tests assert the rejection. - Delete-path test doubles — the worktrees IPC harness
TestStoreand the Windows delete test's store now answergetWorkspaceSession, which the delete paths read for saved tab bindings, and an SSH exact-options expectation gains the emptypersistedPaneSessionIds. - Wording — two comments and two test names no longer call a silent version "older" or imply mobile reads the
worktree.rmreply.
Two prior pullfrog threads remain open and are not restated as new findings: the probePtyLiveness JSDoc at pty-provider-contract.ts:144 still says false only means "provider answered absent", and the current daemon version's listing is still uncapped — this commit fixed the verdict for a silent current version, not the up-to-30 s wait it can take to reach that verdict. Hence a comment review rather than an approval.
DeepSeek Flash (free via Pullfrog for OSS) ｜ 𝕏
… version it never used While a previous version was frozen, activating a newly added worktree opened no terminal: the activation gate blocks on an incomplete listing, though its second-writer worry applies only when the silent version may hold this worktree's session. The scoped listing now names the worktree and is judged with the same evidence the delete sweep uses (silentVersionEvidence, now shared by both): the silent version's last-known ids for the worktree and the worktree's saved tab bindings no answering version listed. With no evidence the listing is complete for that worktree; with evidence the gate still blocks. A silent current version still fails the listing, so the gate keeps blocking.
…ncomplete It said the silent version's sessions "aren't listed", while the popover can still show such a terminal's row from process memory. All six catalogs.
…listing found silent A non-force delete past a frozen version refused only after ~20 s: after the failed stop, the verdict asked the frozen owner again with a fresh 10 s budget. The sweep now hands the verdict the ids its own listing tied to a silent version, and the verdict reads those as unverifiable without probing, so the refusal and its Force Delete hint come about 10 s sooner. It errs only toward refusing. Teardown ordering is unchanged; the kill already queued in a frozen daemon is a known residual for the durable kill order (step 4).
brennanb2025
changed the title
Review status: ready for your decision (not merged, auto-merge off)The problem. When an older version of Orca's background terminal service is still running but not answering (for example frozen), one silent version used to break everything that lists sessions. Manage Sessions silently dropped its sessions after a 30 s wait. Resource Manager showed "sessions unavailable". Every terminal stop said "not confirmed". Deleting any workspace asked for Force Delete. A newly added workspace opened no terminal. #23959 (step 2) made this case more common on purpose, keeping an unreachable version instead of wrongly calling it gone, and it handed this residual to this PR. What changes for the user. Manage Sessions groups sessions by version and shows a silent previous version as "unverifiable" within about 3 s, never as zero or gone. Resource Manager lists what answered, with a "this list may be incomplete" note. Stops are confirmed by the version that owned the session. A workspace with no link to the silent version deletes (with a note and an Open Manage Sessions action) and opens normally. Orca refuses only when the silent version may be running that workspace's terminal. A hung current version keeps today's "not responding" state. Nothing is killed automatically. This PR is step 3 of 4 of STA-2018: #16900 → #23959 → this PR → a durable kill order (not built). Base is #23959's branch. The description now corrects the earlier, disproven pile-up explanation. What the review changed
Live QA on the M4Air (before = #23959 head, after = this PR at 505e79d)
All scenarios passed. A restored tab in the frozen version keeps its session with Retry, and its delete refuses. After the version resumes, the group resolves. The full before/after table and all 16 screenshots are in the description. Deferred or residual (stated in the description)
Verified and not verified
|
| if ( | ||
| forWorktree && | ||
| silentVersionEvidence(listings, forWorktree.worktreeId, forWorktree.persistedPaneSessionIds) | ||
| .length === 0 | ||
| ) { | ||
| return answeredProcesses(listings) | ||
| } |
There was a problem hiding this comment.
If an unreachable previous daemon still holds a worktree session that this app never attached or routed, and no saved pane names it, silentVersionEvidence is empty. This shortcut then marks the scoped list complete despite the unanswered daemon. The activation gate can treat the worktree as empty and start another writer instead of blocking.
| mapWithConcurrency(failedPtyIds, PER_PTY_VERIFY_CONCURRENCY, (ptyId) => | ||
| // Why no probe: this removal already found the owner silent; asking again only waits it out. | ||
| silentEvidence.has(ptyId) | ||
| ? Promise.resolve(null) | ||
| : provider.confirmPtyStopped!(ptyId, { deadlineMs: verifyDeadline }).catch(() => null) |
There was a problem hiding this comment.
If an unreachable generation has a stale record of an ID that the current generation also lists, that ID enters silentEvidence even though an answering owner holds the current copy. When its stop attempt fails, this branch skips the owner’s follow-up check. Even if that owner could confirm the session exited, deletion is refused as unverifiable.
There was a problem hiding this comment.
ℹ️ No new issues in the follow-up commits — two prior notes remain open.
Reviewed changes
This run re-reviewed the three commits since the prior pullfrog review at 482ab79 (a4e5dfc, 4f9a848, ba9ec3a), read the incremental diff and the surrounding listing and teardown code, and ran the five touched suites (43 tests, all passing) plus tc:node and tc:web.
- Scoped listing completeness is now judged per worktree —
PtySessionListScopegains an optionalworktreeId, and the activation inventory sends it.listAnsweredProcessesreturns a complete listing when a silent previous version holds no evidence of the worktree (its last-known ids or saved tab bindings), so activating a newly added worktree is no longer blocked by a version it never used. A silent current version still throws first. - Delete refusal no longer re-probes a version its listing found silent — the evidence ids from
sweepProviderByPrefixare threaded throughkillAllProcessesForWorktreeintoverifyUnstoppedPtys, where an id tied to a silent version resolves tonullinstead of spending the verify budget onconfirmPtyStopped. The verdict staysunverifiable, and a new assertion proves the owner is not asked again. - Resource Manager partial note reworded — the shared i18n key and its inline default now read "…so this list may be incomplete…", translated in all six catalogs, and the key has a single call site.
The two prior pullfrog threads remain open and are not restated as new findings: the current daemon version's listing is still unbounded (up to the client's 30 s default), and the probePtyLiveness JSDoc at pty-provider-contract.ts:145 still describes only the "answered absent" case. Hence a comment review rather than an approval.
DeepSeek Flash (free via Pullfrog for OSS) ｜ 𝕏
ELI5
When Orca updates, the background service that runs your terminals from the previous version can keep running, still holding terminals and agents. Orca talks to every version it finds. Until now, if one of those old versions was running but not answering (for example frozen), Orca either hid its sessions without saying so, or failed outright: the Resource Manager showed an error, stopping a terminal said "not confirmed", and deleting a workspace demanded Force Delete, even when the silent version had nothing to do with what you were doing. After this PR Orca shows what the versions that answered told it, names the one that did not, and only refuses when the silent version may really be running something of yours.
The problem
A "terminal service" here is Orca's background process that keeps terminals alive while the app restarts (internally, the daemon). An app update that changes how the app talks to it starts a new version of the service, and the previous version keeps running for as long as it still holds sessions. So a machine can have two or more versions running at once.
What users saw while one previous version was alive but not answering:
orca terminal stop) reported the terminal as not confirmed stopped, even for a terminal in the current version. Orca confirmed a stop by listing every version's sessions, and one silent version made that whole list fail.orca terminal create, when retrying a create whose reply was lost, reported the runtime unavailable.Why: these callers asked all versions at once and treated the answer as all-or-nothing. If any version failed to answer, the whole answer failed ("fail-closed": refuse to act when you cannot see everything). One caller, Manage Sessions, did the opposite and silently dropped the version that failed. #23959 made this case more common on purpose: it keeps a version it cannot reach in the list instead of wrongly declaring it gone, and it named this as a known limit owned by this PR.
Scope
What changes for the user
orca terminal stop) confirms with the version that owned it, so a current-version terminal reports stopped while an unrelated version is silent. A terminal in the silent version still reads not confirmed, which is true.orca worktree rmprints the same fact in its existing warning. It still refuses and offers Force Delete when the silent version may be running one of this workspace's terminals.The plan (this PR is step 3 of 4)
Old terminal-service versions pile up and keep agents running that no tab shows (STA-2018). The plan:
This PR is stacked on #23959 (base
brennanb2025/r4-d1-census-unverifiable).Correction to the earlier description
An earlier version of this description said old versions pile up because of what happens to their retire-at-quit request on the next startup. That was wrong. Old versions stay because they still hold live sessions that no tab shows, and the main source of those sessions was a startup bug: a 1-second check at startup that failed on a busy Mac dropped a live old version, so its tab took its live agent for dead and started a duplicate, leaving the original running with no tab. #23959 fixes that. This PR does not reduce the pile-up; it makes the remaining old versions visible and stops one silent version from breaking everything else.
How it works
daemon-generation-listing.ts): asks every version at once and returns, per version,livewith its sessions,exited(only with proof), orunverifiable(no answer; loss of contact is never read as exit). Previous versions are capped at 3 seconds, the runtime's existing budget for listing a host's terminals; the current version keeps the caller's own budget, so a slow current version is never read as silent. For a silent version the listing also returns the session ids Orca last knew it held, derived from what the app already tracks; nothing new is stored. The existing all-or-nothing listing is built on top of it and keeps its contract for every caller not migrated here.pty:listSessionsnow returns{sessions, complete, unverifiable}instead of a bare list, so no caller can mistake a partial list for a whole one. A silent current version still failspty:listSessionsas before, which is what keeps the Resource Manager's "not responding" state.daemon-stopped-session-owners.ts,liveness.ts): when Orca stops a session it remembers which version it asked, in memory only (a new stop or spawn of the id replaces it, a replaced version clears it, and the oldest fall off past 512). "Did it stop?" is then asked of that version alone, within the caller's deadline. No answer means unverifiable, never exited.silentVersionEvidence): the session ids that version was last known to hold for this workspace, plus the session ids this workspace's saved tabs are bound to that no answering version listed (a tab restored as "owner unverified" by fix(daemon): keep a previous daemon version the app could not reach instead of reading it as gone #23959 has only this saved binding). Opening a workspace and deleting one both use it.worktree-activation-pty-inventory.ts,inspect.ts): the check now names the workspace, and the listing counts as complete for it unless the rule finds evidence. With evidence it still blocks, as before.worktree-pty-surface-sweeps.ts): stops this workspace's sessions in every version that answered. Evidence ids go through the normal per-session stop, which cannot be confirmed, so the delete refuses with the Force Delete hint exactly as for any known live terminal; the verify step does not ask the silent version again for ids the listing already tied to it. No evidence: the delete proceeds and the result names the version it could not check (uncheckedTerminalServices).What each part of Orca does while a previous version is silent
orca terminal stoporca worktree rm)orca terminal createretry after a lost replyorca terminal listwith fresh liveness,orca worktree psactivity, orphan adoption, legacy worker recovery, mobile tab refreshBehaviour notes
505e79dda3c, and the final head drops the verify step's second wait, which should bring it to about 13 s; not re-measured live).Why this design
Differences from the reference implementations
Compatibility
pty:management:listSessionsreturns per-version groups,pty:management:killAllgains counts,pty:listSessionsreturns{sessions, complete, unverifiable}and accepts an optionalworktreeIdin its scope. These run between main and renderer of the same build; the web client's stubs return the new shapes.worktree.rmaddsuncheckedTerminalServicesand fills the existing optionalwarning. Older clients ignore the field and print the warning as before. No error code changed. The mobile app does not read theworktree.rmreply.Known limits
Visual Proof
Live QA (M4Air)
Two Orca builds on an M4 Air with isolated profiles and plain
sleep 100000shells: a v36 build leaves a terminal in workspacewt-old, a v37 build opens one inwt-cur, then the v36 service is frozen with SIGSTOP. Before = #23959's heada2493914b8c; after = this PR at505e79dda3c. The machine was heavily loaded (load average about 30), so timings run long. Commits after505e79dda3cchanged only the 3-second cap (current version no longer capped), the open-workspace check, the Resource Manager note's wording and the delete's verify step.QA also found that opening a newly added workspace while v36 was frozen opened no terminal. The base has the same block for a version that freezes after startup. Fixed in
a4e5dfc3a41(the open-workspace evidence rule above), covered by unit tests, not re-run live.Two versions answering (scenario 1)
Before: flat Manage Sessions list:
After: grouped by version:
Previous version frozen, Manage Sessions (2a)
Before: v36 session silently missing after ~30 s:
After: unverifiable previous version at ~3 s:
Previous version frozen, Resource Manager (2b)
Before: sessions unavailable banner:
After: answered sessions plus a note, no error (note since reworded):
Delete a workspace whose terminals are in the current version (2c)
Before: refused with Force Delete:
After: deleted with a non-blocking note:
After: the note's action opens Manage Sessions:
Previous version frozen across a restart (5)
Before: delete refused after ~11.6 s:
After: tab keeps its saved session with Retry:
After: delete refused after ~3.4 s:
Same session id in two versions (3)
Before: two identical rows:
After: rows grouped by version:
After: Kill ended exactly the previous-version row:
After the frozen version resumes (4)
After: the unverifiable group resolves:
Testing
Run with
vitest run --config config/vitest.config.tson explicit files.daemon-frozen-previous-version.test.tsruns real terminal-service servers for v35 and the current version. v35 sits behind a proxy process, and freezing that recorded process (SIGSTOP) after startup makes v35 silent the way a stalled service is. Seven cases: Manage Sessions lists the current version and marks v35 unverifiable within the deadline; the Resource Manager listing answers within the deadline and names v35; a current-version stop confirms without waiting on v35; a v35 stop is never confirmed; deleting a workspace with only current-version terminals succeeds and names v35; deleting one whose terminal v35 was last known to hold refuses with the Force Delete hint; deleting one whose only link to v35 is a restored tab's saved session id refuses too, and without that tab the delete proceeds and names v35. Skipped on Windows (no SIGSTOP).Revert checks on that file. Reverting owner-confirmed stops turned 3 cases red, including the current-version-only delete, which then asked for Force Delete. Removing the silent version's evidence from the delete sweep turned both refusal cases red. Removing only the saved-tab evidence turned the restored-tab case red.
Unit tests:
daemon-generation-listing.test.ts(a slow current version is not capped),daemon-stopped-session-owners.test.ts,pty-management.test.ts,pty-management-kill-all.test.ts(a version going silent mid-kill is not counted as killed),pty-activation-inventory-scope.test.ts(a workspace with no evidence reads complete; a silent current version fails as before),worktree-agent-activation-seam.test.ts,worktree-teardown-silent-terminal-service.test.ts(a slow current version is still listed and stopped),unstopped-pty-verification.test.ts(the silent owner is not asked again),worktree-persisted-pane-sessions.test.ts,worktree-rm-pty-waiver.test.ts(CLI warning),ManageSessionsGenerationRows.test.tsx,manage-sessions-format.test.ts,useDaemonActions.test.tsx,unchecked-terminal-services-toast.test.ts,store-worktree-removal-cascade.test.ts.Tests updated to the new rule:
terminal-close-observed-exit.test.ts(a tab close while an unrelated version is gone now closes cleanly), the delete-path store doubles (they now answer the workspace-session read), and the e2e specs and test stubs that readpty:listSessionsas a bare list.Runs: 31 touched test files, 384 tests, all passing at
ff027d05952(including the real-daemon file, 7/7); 541 delete-path tests after the store-double fix; the 5 files covering the last three commits, 43 tests, and the real-daemon file, 7/7, at this head (ba9ec3a1d82).Typecheck:
tc:nodeandtc:webclean at this head;tc:cliclean at9ad6aa46218(nothing the CLI compiles changed since). oxlint, the changed-code design-system check and the localization checks pass. All five translated catalogs carry the new strings.CI, two reds, both pre-existing: (a)
e2e / changed e2e specsfails atremote-agent-session-focus-authority.spec.ts:403, before the lines this PR edits (it only reads.sessionsthere). Reading the code traces it to fix(mobile): stop a created tab from jumping when the host snapshot lands #20069: the host appends the tab for a client without the split-group-placement capability. This headful spec runs only when a PR edits it; the other 8 changed specs pass. (b)persistence (windows-11-arm): a known Bun startup flake inorcad-bun-launcher.integration.test.ts. It passed on three earlier heads of this PR, the same file fails the same way on main, and it has been re-run.Not verified here: Windows and Linux live runs (CI runs the unit tests), SSH hosts live, packaged builds.
I manually tested these changes locally
Automated tests added/updated, or explained why not below
Linked Issue
Part of STA-2018. Sessions already orphaned on users' machines stay until the user ends them in Manage Sessions.
Agent skill upstream boundary
docs/reference/agent-skill-sharing-upstream-boundary.mdand copies or mechanically translates no upstream skill-installer source, tests, fixtures, registry entries, path tables, comments, or documentation.Checklist
N/Awith reasonAuthor: Brennan Benson (@BrennanKB5)