From 53a1db4b649a4e476aaa4e2db8fb8cfae5962aee Mon Sep 17 00:00:00 2001 From: OrcaWin <293788423+OrcaWin@users.noreply.github.com> Date: Mon, 27 Jul 2026 11:14:36 -0700 Subject: [PATCH] fix(resource-manager): distinguish unprovable ownership from proven absence MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Adversarial review of the previous commit found it committed the same class of error it was fixing: it collapsed "no agent owns this" and "this provider cannot tell me" into one boolean `false`, and both destructive paths read that as proof. A daemon generation below the claim protocol, an older SSH relay, or the in-process local fallback all list no owners for a session that may well have one. `pty.ts` already encodes the rule at :613 — "only providers that serialize claims may make listing absence authoritative" — and the new IPC row ignored it. So after upgrading with a legacy daemon still holding a live agent terminal, bulk cleanup would have destroyed it: exactly #8459, one layer down. `hasAgentOwner: boolean` is now `agentOwnership: 'present' | 'absent' | 'unknown'`, derived via `providesAgentSessionOwnerListings`. Only `absent` authorizes destruction, so `unknown` protects and confirms. Second defect, found independently by four review lenses: the deferred-SSH bindings reached the bulk selector but not `mergeSnapshotAndSessions`, because the merge call site re-listed the binding fields instead of reusing the object. A deferred SSH session therefore rendered `bound: false`, and its single-row kill skipped confirmation while bulk cleanup correctly spared it. The call site now spreads `resourceSessionBindings`, and a parity test fails if any binding field is re-listed inline — the drift itself is now impossible to reintroduce quietly. The e2e ownership assertion was also weak: it checked only that a boolean arrived. It now asserts the exact arm, and that the live local provider reports `absent` rather than `unknown`, so a degenerate all-unknown implementation fails. --- src/main/ipc/pty.test.ts | 44 ++++++++- src/main/ipc/pty.ts | 15 ++- .../ResourceUsageStatusSegment.rows.test.tsx | 2 +- .../status-bar/ResourceUsageStatusSegment.tsx | 13 +-- .../mergeSnapshotAndSessions.test.ts | 96 ++++++++++++++++--- .../status-bar/mergeSnapshotAndSessions.ts | 11 ++- .../resource-session-bindings.test.ts | 59 +++++++++--- .../status-bar/resource-session-bindings.ts | 9 +- ...urce-session-classification-parity.test.ts | 51 ++++++++++ .../resource-session-inventory.test.ts | 2 +- ...resource-session-kill-confirmation.test.ts | 12 ++- .../resource-session-kill-confirmation.ts | 7 +- .../status-bar/resource-usage-merge-types.ts | 11 ++- .../use-resource-session-inventory.test.tsx | 2 +- src/shared/pty-listed-session.ts | 25 ++++- ...rce-manager-unbound-session-safety.spec.ts | 28 ++++-- 16 files changed, 313 insertions(+), 74 deletions(-) create mode 100644 src/renderer/src/components/status-bar/resource-session-classification-parity.test.ts diff --git a/src/main/ipc/pty.test.ts b/src/main/ipc/pty.test.ts index 3bd11cdfa3f..299e7b5c756 100644 --- a/src/main/ipc/pty.test.ts +++ b/src/main/ipc/pty.test.ts @@ -5736,6 +5736,8 @@ describe('registerPtyHandlers', () => { { id: 'agent-pty', cwd: '/workspace', title: 'codex', agentSessionOwners: [owner] }, { id: 'plain-pty', cwd: '/tmp', title: 'zsh' } ]), + // Why: this provider serializes claims, so its silence about an owner is authoritative. + providesAgentSessionOwnerListings: () => true, attach: vi.fn(), getDefaultShell: vi.fn(), getProfiles: vi.fn() @@ -5743,11 +5745,47 @@ describe('registerPtyHandlers', () => { const sessions = (await handlers.get('pty:listSessions')!(null, undefined)) as { id: string - hasAgentOwner: boolean + agentOwnership: string }[] - expect(sessions.find((s) => s.id === 'agent-pty')?.hasAgentOwner).toBe(true) - expect(sessions.find((s) => s.id === 'plain-pty')?.hasAgentOwner).toBe(false) + expect(sessions.find((s) => s.id === 'agent-pty')?.agentOwnership).toBe('present') + expect(sessions.find((s) => s.id === 'plain-pty')?.agentOwnership).toBe('absent') + }) + + it('reports unknown ownership when the provider cannot serialize claims', async () => { + registerPtyHandlers(mainWindow as never) + // Why: a legacy daemon generation or older SSH relay lists no owners for a session that may + // have one. Reporting that silence as 'absent' is what let live agent sessions be killed (#8459). + setLocalPtyProvider({ + spawn: vi.fn(), + write: vi.fn(), + resize: vi.fn(), + shutdown: vi.fn(), + sendSignal: vi.fn(), + getCwd: vi.fn(), + getInitialCwd: vi.fn(), + clearBuffer: vi.fn(), + acknowledgeDataEvent: vi.fn(), + hasChildProcesses: vi.fn(), + getForegroundProcess: vi.fn(), + serialize: vi.fn(), + revive: vi.fn(), + onData: vi.fn(() => () => {}), + onReplay: vi.fn(() => () => {}), + onExit: vi.fn(() => () => {}), + listProcesses: vi.fn(async () => [{ id: 'legacy-pty', cwd: '/workspace', title: 'zsh' }]), + providesAgentSessionOwnerListings: () => false, + attach: vi.fn(), + getDefaultShell: vi.fn(), + getProfiles: vi.fn() + } as never) + + const sessions = (await handlers.get('pty:listSessions')!(null, undefined)) as { + id: string + agentOwnership: string + }[] + + expect(sessions.find((s) => s.id === 'legacy-pty')?.agentOwnership).toBe('unknown') }) it('kills app-scoped SSH PTY ids through the parsed provider when ownership is not rebuilt', async () => { diff --git a/src/main/ipc/pty.ts b/src/main/ipc/pty.ts index edef59af57c..5e81dbba66d 100644 --- a/src/main/ipc/pty.ts +++ b/src/main/ipc/pty.ts @@ -5514,7 +5514,7 @@ export function registerPtyHandlers( registeredPtyProviders(), ({ provider, connectionId }) => connectionId === null ? provider.listProcesses() : provider.listProcesses().catch(() => []), - ({ connectionId }, sessions) => { + ({ provider, connectionId }, sessions) => { for (const rawSession of sessions) { const session = admission.admit(rawSession) // Why: kill actions only send back the PTY id, so rebuild ownership while listing to keep reconnect-discovered remote sessions routed to their provider. @@ -5523,10 +5523,15 @@ export function registerPtyHandlers( id: session.id, cwd: session.cwd, title: session.title, - // Why: an agent bound to this session is proof work is running, independent of whether - // this renderer has a tab binding yet. Without it the UI can only guess, and guessed - // "orphan" force-killed live agent sessions (#8459). - hasAgentOwner: (session.agentSessionOwners?.length ?? 0) > 0 + // Why: the renderer's binding map is empty during restore, so ownership is the only + // liveness evidence it has. Absence is authoritative only from a provider that + // serializes claims — otherwise it is 'unknown', never 'absent' (#8459). + agentOwnership: + (session.agentSessionOwners?.length ?? 0) > 0 + ? 'present' + : provider.providesAgentSessionOwnerListings?.(session.id) === true + ? 'absent' + : 'unknown' }) } } diff --git a/src/renderer/src/components/status-bar/ResourceUsageStatusSegment.rows.test.tsx b/src/renderer/src/components/status-bar/ResourceUsageStatusSegment.rows.test.tsx index bb48583efad..3bf507646da 100644 --- a/src/renderer/src/components/status-bar/ResourceUsageStatusSegment.rows.test.tsx +++ b/src/renderer/src/components/status-bar/ResourceUsageStatusSegment.rows.test.tsx @@ -45,7 +45,7 @@ function makeSession(overrides: Partial): UnifiedSessionRow { pid: 100, label: 'zsh', bound: true, - hasAgentOwner: false, + agentOwnership: 'absent' as const, tabId: 'tab-1', cpu: 1, memory: 100, diff --git a/src/renderer/src/components/status-bar/ResourceUsageStatusSegment.tsx b/src/renderer/src/components/status-bar/ResourceUsageStatusSegment.tsx index 761287531ca..e316c79da4e 100644 --- a/src/renderer/src/components/status-bar/ResourceUsageStatusSegment.tsx +++ b/src/renderer/src/components/status-bar/ResourceUsageStatusSegment.tsx @@ -931,11 +931,11 @@ export function ResourceUsageStatusSegment({ () => open ? mergeSnapshotAndSessions(resourceSnapshot, sessions, { - tabsByWorktree, - ptyIdsByTabId, - terminalLayoutsByTabId, + // Why spread: the rows and the bulk selector must classify from the identical binding + // inputs. Re-listing them here let the row path miss deferred SSH sessions, so their + // single-row kill skipped confirmation while bulk cleanup correctly spared them (#8459). + ...resourceSessionBindings, runtimePaneTitlesByTabId, - workspaceSessionReady, repoDisplayNameById, repoConnectionIdById, repoRuntimeScopedById, @@ -947,11 +947,8 @@ export function ResourceUsageStatusSegment({ open, resourceSnapshot, sessions, - tabsByWorktree, - ptyIdsByTabId, - terminalLayoutsByTabId, + resourceSessionBindings, runtimePaneTitlesByTabId, - workspaceSessionReady, repoDisplayNameById, repoConnectionIdById, repoRuntimeScopedById, diff --git a/src/renderer/src/components/status-bar/mergeSnapshotAndSessions.test.ts b/src/renderer/src/components/status-bar/mergeSnapshotAndSessions.test.ts index 1fc4616a113..a2fb18c1193 100644 --- a/src/renderer/src/components/status-bar/mergeSnapshotAndSessions.test.ts +++ b/src/renderer/src/components/status-bar/mergeSnapshotAndSessions.test.ts @@ -7,6 +7,7 @@ import type { WorktreeMemory } from '../../../../shared/types' import { mergeSnapshotAndSessions, UNATTRIBUTED_REPO_ID } from './mergeSnapshotAndSessions' +import { requiresKillConfirmation } from './resource-session-kill-confirmation' import type { DaemonSession, MergeContext } from './resource-usage-merge-types' function emptyAppMemory() { @@ -156,7 +157,7 @@ describe('mergeSnapshotAndSessions', () => { sessions: [{ sessionId: 'pty-1', paneKey: null, pid: 999, cpu: 0.1, memory: 50_000_000 }] } const ds: DaemonSession[] = [ - { id: 'pty-1', cwd: '/Users/me/Triton', title: 'shell', hasAgentOwner: false } + { id: 'pty-1', cwd: '/Users/me/Triton', title: 'shell', agentOwnership: 'absent' as const } ] const out = mergeSnapshotAndSessions(makeSnapshot([wt]), ds, baseCtx()) expect(out[0].worktrees[0].sessions).toHaveLength(1) @@ -182,24 +183,66 @@ describe('mergeSnapshotAndSessions', () => { sessions: [{ sessionId: 'pty-agent', paneKey: null, pid: 999, cpu: 0.1, memory: 50_000_000 }] } const ds: DaemonSession[] = [ - { id: 'pty-agent', cwd: '/Users/me/Triton', title: 'codex', hasAgentOwner: true } + { + id: 'pty-agent', + cwd: '/Users/me/Triton', + title: 'codex', + agentOwnership: 'present' as const + } ] const out = mergeSnapshotAndSessions(makeSnapshot([wt]), ds, baseCtx()) expect(out[0].worktrees[0].sessions[0]).toMatchObject({ sessionId: 'pty-agent', - hasAgentOwner: true + agentOwnership: 'present' as const }) }) + it('binds a deferred SSH row so its single-row kill cannot skip confirmation', () => { + // Why: the bulk selector already excluded these, but the rendered row took its own path. + // An unbound row with no agent skips the dialog entirely — the same #8459 defect, one click over. + const sessionId = 'orca::/remote/Stingray@@deferred1' + const out = mergeSnapshotAndSessions( + null, + [{ id: sessionId, cwd: '', title: 'orca/Stingray', agentOwnership: 'absent' as const }], + baseCtx({ + tabsByWorktree: { 'orca::/remote/Stingray': [makeTab('tab-ssh')] }, + deferredSshSessionIdsByTabId: { 'tab-ssh': sessionId }, + repoConnectionIdById: new Map([['orca', 'ssh-conn-1']]) + }) + ) + + const row = out[0].worktrees[0].sessions[0] + expect(row).toMatchObject({ sessionId, bound: true, tabId: 'tab-ssh' }) + expect(requiresKillConfirmation(row)).toBe(true) + }) + + it('treats a snapshot row the daemon never listed as unknown ownership, not absent', () => { + const wt: WorktreeMemory = { + worktreeId: 'orca::/Users/me/Triton', + worktreeName: 'Triton', + repoId: 'orca', + repoName: 'ORCA', + cpu: 0.1, + memory: 50_000_000, + history: [], + sessions: [{ sessionId: 'pty-only-local', paneKey: null, pid: 7, cpu: 0.1, memory: 1 }] + } + + const out = mergeSnapshotAndSessions(makeSnapshot([wt]), [], baseCtx()) + + expect(out[0].worktrees[0].sessions[0]).toMatchObject({ agentOwnership: 'unknown' }) + expect(requiresKillConfirmation(out[0].worktrees[0].sessions[0])).toBe(true) + }) + it('@@ parse: an SSH-style session id resolves to its worktree group', () => { const ds: DaemonSession[] = [ { id: 'orca::/remote/Stingray@@abcd1234', cwd: '', title: 'orca/Stingray', - hasAgentOwner: false + agentOwnership: 'absent' as const } ] const ctx = baseCtx({ @@ -240,7 +283,7 @@ describe('mergeSnapshotAndSessions', () => { id: 'orca::/local/Triton@@deadbeef', cwd: '/local/Triton', title: 'orca/Triton', - hasAgentOwner: false + agentOwnership: 'absent' as const } ] const ctx = baseCtx({ @@ -260,7 +303,12 @@ describe('mergeSnapshotAndSessions', () => { it('tab walk wins over @@ parse when they disagree', () => { const tabId = 'tab-xyz' const ds: DaemonSession[] = [ - { id: 'orca::/wrong/path@@feedface', cwd: '', title: 'orca', hasAgentOwner: false } + { + id: 'orca::/wrong/path@@feedface', + cwd: '', + title: 'orca', + agentOwnership: 'absent' as const + } ] const ctx = baseCtx({ tabsByWorktree: { @@ -278,7 +326,12 @@ describe('mergeSnapshotAndSessions', () => { const tabId = 'tab-restored' const sessionId = 'orca::/Users/me/Triton@@deferred' const ds: DaemonSession[] = [ - { id: sessionId, cwd: '/Users/me/Triton', title: 'orca/Triton', hasAgentOwner: false } + { + id: sessionId, + cwd: '/Users/me/Triton', + title: 'orca/Triton', + agentOwnership: 'absent' as const + } ] const restoredTab = { ...makeTab(tabId, 'Restored'), ptyId: sessionId } const ctx = baseCtx({ @@ -319,7 +372,7 @@ describe('mergeSnapshotAndSessions', () => { id: 'remote-repo::/remote/Stingray@@1234', cwd: '', title: 'remote/Stingray', - hasAgentOwner: false + agentOwnership: 'absent' as const } ] const ctx = baseCtx({ @@ -362,10 +415,20 @@ describe('mergeSnapshotAndSessions', () => { id: 'runtime-repo::/runtime/Wt@@future-runtime', cwd: '', title: 'runtime/Wt', - hasAgentOwner: false + agentOwnership: 'absent' as const }, - { id: 'ssh-repo::/remote/Wt@@ssh-session', cwd: '', title: 'ssh/Wt', hasAgentOwner: false }, - { id: 'opaque-local-orphan', cwd: '', title: 'orphan shell', hasAgentOwner: false } + { + id: 'ssh-repo::/remote/Wt@@ssh-session', + cwd: '', + title: 'ssh/Wt', + agentOwnership: 'absent' as const + }, + { + id: 'opaque-local-orphan', + cwd: '', + title: 'orphan shell', + agentOwnership: 'absent' as const + } ] const ctx = baseCtx({ repoConnectionIdById: new Map([ @@ -409,7 +472,7 @@ describe('mergeSnapshotAndSessions', () => { // to. The chip should stay off; the row still surfaces in the // unattributed bucket with `—` cells because we have no sample. const ds: DaemonSession[] = [ - { id: 'opaque-id-without-prefix', cwd: '', title: 'shell', hasAgentOwner: false } + { id: 'opaque-id-without-prefix', cwd: '', title: 'shell', agentOwnership: 'absent' as const } ] const out = mergeSnapshotAndSessions(null, ds, baseCtx()) expect(out).toHaveLength(1) @@ -463,7 +526,12 @@ describe('mergeSnapshotAndSessions', () => { it('remote-orphan interaction state: null metrics + bound=false', () => { const ds: DaemonSession[] = [ - { id: 'orca::/remote/Wt@@deadbeef', cwd: '', title: 'orca/Wt', hasAgentOwner: false } + { + id: 'orca::/remote/Wt@@deadbeef', + cwd: '', + title: 'orca/Wt', + agentOwnership: 'absent' as const + } ] const out = mergeSnapshotAndSessions(null, ds, baseCtx()) const session = out[0].worktrees[0].sessions[0] @@ -478,7 +546,7 @@ describe('mergeSnapshotAndSessions', () => { it('uses repoDisplayNameById to humanize new project groups when available', () => { const ds: DaemonSession[] = [ - { id: 'stably-ai/orca::/remote/Wt@@1', cwd: '', title: '', hasAgentOwner: false } + { id: 'stably-ai/orca::/remote/Wt@@1', cwd: '', title: '', agentOwnership: 'absent' as const } ] const ctx = baseCtx({ repoDisplayNameById: new Map([['stably-ai/orca', 'ORCA']]) diff --git a/src/renderer/src/components/status-bar/mergeSnapshotAndSessions.ts b/src/renderer/src/components/status-bar/mergeSnapshotAndSessions.ts index 0e69447e775..d02588001b9 100644 --- a/src/renderer/src/components/status-bar/mergeSnapshotAndSessions.ts +++ b/src/renderer/src/components/status-bar/mergeSnapshotAndSessions.ts @@ -143,9 +143,10 @@ export function mergeSnapshotAndSessions( const index = buildResourceSessionBindingIndex(ctx) const boundPtyIds = index.boundPtyIds // Why: the daemon list is the only place agent ownership is reported. Snapshot-derived rows - // describe the same sessions by id, so carry it across rather than letting them report `false`. - const agentOwnedSessionIds = new Set( - daemonSessions.filter((session) => session.hasAgentOwner).map((session) => session.id) + // describe the same sessions by id, so carry it across rather than inventing an answer; a + // session the daemon never listed is 'unknown', not 'absent'. + const ownershipBySessionId = new Map( + daemonSessions.map((session) => [session.id, session.agentOwnership]) ) function isRepoRemote(repoId: string): boolean { @@ -207,7 +208,7 @@ export function mergeSnapshotAndSessions( pid: s.pid, label: resolveSnapshotSessionLabel(s, wt.worktreeId, ctx), bound: ctx.workspaceSessionReady && boundPtyIds.has(s.sessionId), - hasAgentOwner: agentOwnedSessionIds.has(s.sessionId), + agentOwnership: ownershipBySessionId.get(s.sessionId) ?? 'unknown', tabId, cpu: s.cpu, memory: s.memory, @@ -295,7 +296,7 @@ export function mergeSnapshotAndSessions( pid: 0, label: resolveDaemonSessionLabel(session, worktreeId, tabId, ctx), bound: ctx.workspaceSessionReady && boundPtyIds.has(session.id), - hasAgentOwner: session.hasAgentOwner, + agentOwnership: session.agentOwnership, tabId, cpu: null, memory: null, diff --git a/src/renderer/src/components/status-bar/resource-session-bindings.test.ts b/src/renderer/src/components/status-bar/resource-session-bindings.test.ts index 846cf88f296..3845234d6ed 100644 --- a/src/renderer/src/components/status-bar/resource-session-bindings.test.ts +++ b/src/renderer/src/components/status-bar/resource-session-bindings.test.ts @@ -83,9 +83,14 @@ describe('resource session bindings', () => { it('counts only daemon sessions without live or restorable tab bindings', () => { const count = countUnboundDaemonSessions( [ - { id: 'pty-live', cwd: '/workspace', title: 'live', hasAgentOwner: false }, - { id: 'pty-restored', cwd: '/workspace', title: 'restored', hasAgentOwner: false }, - { id: 'pty-orphan', cwd: '/tmp', title: 'orphan', hasAgentOwner: false } + { id: 'pty-live', cwd: '/workspace', title: 'live', agentOwnership: 'absent' as const }, + { + id: 'pty-restored', + cwd: '/workspace', + title: 'restored', + agentOwnership: 'absent' as const + }, + { id: 'pty-orphan', cwd: '/tmp', title: 'orphan', agentOwnership: 'absent' as const } ], { ptyIdsByTabId: { 'tab-live': ['pty-live'] }, @@ -112,7 +117,14 @@ describe('resource session bindings', () => { expect(buildResourceSessionBindingIndex(inputs).boundPtyIds.has('pty-deferred')).toBe(true) expect( selectUnboundDaemonSessions( - [{ id: 'pty-deferred', cwd: '/workspace', title: 'agent', hasAgentOwner: false }], + [ + { + id: 'pty-deferred', + cwd: '/workspace', + title: 'agent', + agentOwnership: 'absent' as const + } + ], inputs ) ).toEqual([]) @@ -138,8 +150,8 @@ describe('resource session bindings', () => { workspaceSessionReady: true } const sessions = [ - { id: 'pty-agent', cwd: '/workspace', title: 'codex', hasAgentOwner: true }, - { id: 'pty-shell', cwd: '/tmp', title: 'shell', hasAgentOwner: false } + { id: 'pty-agent', cwd: '/workspace', title: 'codex', agentOwnership: 'present' as const }, + { id: 'pty-shell', cwd: '/tmp', title: 'shell', agentOwnership: 'absent' as const } ] expect(selectUnboundDaemonSessions(sessions, inputs).map((s) => s.id)).toEqual(['pty-shell']) @@ -155,10 +167,15 @@ describe('resource session bindings', () => { workspaceSessionReady: true } const sessions = [ - { id: 'pty-live', cwd: '/workspace', title: 'live', hasAgentOwner: false }, - { id: 'pty-deferred', cwd: '/workspace', title: 'deferred', hasAgentOwner: false }, - { id: 'pty-agent', cwd: '/workspace', title: 'codex', hasAgentOwner: true }, - { id: 'pty-orphan', cwd: '/tmp', title: 'orphan', hasAgentOwner: false } + { id: 'pty-live', cwd: '/workspace', title: 'live', agentOwnership: 'absent' as const }, + { + id: 'pty-deferred', + cwd: '/workspace', + title: 'deferred', + agentOwnership: 'absent' as const + }, + { id: 'pty-agent', cwd: '/workspace', title: 'codex', agentOwnership: 'present' as const }, + { id: 'pty-orphan', cwd: '/tmp', title: 'orphan', agentOwnership: 'absent' as const } ] expect(selectUnboundDaemonSessions(sessions, inputs).map((s) => s.id)).toEqual(['pty-orphan']) @@ -167,8 +184,28 @@ describe('resource session bindings', () => { ) }) + it('never offers a session whose ownership could not be established', () => { + // Why: a legacy daemon generation or an older SSH relay cannot serialize claims, so it reports + // no owners for a session that may well have one. Unknown must never read as absent (#8459). + const inputs = { + ptyIdsByTabId: {}, + tabsByWorktree: {}, + terminalLayoutsByTabId: {}, + workspaceSessionReady: true + } + const sessions = [ + { id: 'pty-legacy', cwd: '/workspace', title: 'shell', agentOwnership: 'unknown' as const }, + { id: 'pty-known', cwd: '/tmp', title: 'shell', agentOwnership: 'absent' as const } + ] + + expect(selectUnboundDaemonSessions(sessions, inputs).map((s) => s.id)).toEqual(['pty-known']) + expect(countUnboundDaemonSessions(sessions, inputs)).toBe(1) + }) + it('offers nothing while the workspace session is still loading', () => { - const sessions = [{ id: 'pty-any', cwd: '/tmp', title: 'shell', hasAgentOwner: false }] + const sessions = [ + { id: 'pty-any', cwd: '/tmp', title: 'shell', agentOwnership: 'absent' as const } + ] const inputs = { ptyIdsByTabId: {}, tabsByWorktree: {}, diff --git a/src/renderer/src/components/status-bar/resource-session-bindings.ts b/src/renderer/src/components/status-bar/resource-session-bindings.ts index d1d0e0afc8b..96240f82268 100644 --- a/src/renderer/src/components/status-bar/resource-session-bindings.ts +++ b/src/renderer/src/components/status-bar/resource-session-bindings.ts @@ -1,4 +1,5 @@ import type { TerminalLayoutSnapshot, TerminalTab } from '../../../../shared/types' +import { mayDestroyWithoutOwnerEvidence } from '../../../../shared/pty-listed-session' import type { DaemonSession } from './resource-usage-merge-types' export type ResourceSessionBindingInputs = { @@ -97,9 +98,11 @@ export function selectUnboundDaemonSessions( return [] } const { boundPtyIds } = buildResourceSessionBindingIndex(inputs) - // Why hasAgentOwner short-circuits: an agent holding the session is positive proof work is - // running. This renderer's binding map is empty during restore, so it cannot decide alone. - return sessions.filter((session) => !boundPtyIds.has(session.id) && !session.hasAgentOwner) + // Why the ownership check: this renderer's binding map is empty during restore, so it cannot + // decide alone. Only proven absence of an owner qualifies — 'unknown' protects. + return sessions.filter( + (session) => !boundPtyIds.has(session.id) && mayDestroyWithoutOwnerEvidence(session) + ) } export function countUnboundDaemonSessions( diff --git a/src/renderer/src/components/status-bar/resource-session-classification-parity.test.ts b/src/renderer/src/components/status-bar/resource-session-classification-parity.test.ts new file mode 100644 index 00000000000..723dc4e81a8 --- /dev/null +++ b/src/renderer/src/components/status-bar/resource-session-classification-parity.test.ts @@ -0,0 +1,51 @@ +import { readFileSync } from 'node:fs' +import { resolve } from 'node:path' +import { describe, expect, it } from 'vitest' + +const SEGMENT_PATH = resolve(__dirname, 'ResourceUsageStatusSegment.tsx') + +/** + * Both destructive paths — bulk "kill orphans" and a single row's kill — must classify from the + * same binding inputs. When the merge call site re-listed the fields instead of reusing the + * object, it silently omitted deferred SSH sessions: bulk cleanup spared them while the row's + * kill destroyed them with no confirmation. Same bug, one click over (#8459). + */ +describe('resource session classification parity', () => { + it('feeds the row merge the same binding inputs as the bulk selector', () => { + const source = readFileSync(SEGMENT_PATH, 'utf8') + const mergeCall = source.slice( + source.indexOf('mergeSnapshotAndSessions(resourceSnapshot'), + source.indexOf('worktreeById\n })') + ) + + expect(mergeCall).toContain('...resourceSessionBindings') + // Any of these appearing inline means the call site is re-deriving bindings and can drift. + for (const field of [ + 'ptyIdsByTabId,', + 'tabsByWorktree,', + 'terminalLayoutsByTabId,', + 'deferredSshSessionIdsByTabId,', + 'workspaceSessionReady,' + ]) { + expect(mergeCall).not.toContain(field) + } + }) + + it('keeps every binding source in the one object both paths read', () => { + const source = readFileSync(SEGMENT_PATH, 'utf8') + const bindings = source.slice( + source.indexOf('const resourceSessionBindings = useMemo'), + source.indexOf('const popoverBodyRef') + ) + + for (const field of [ + 'ptyIdsByTabId', + 'tabsByWorktree', + 'terminalLayoutsByTabId', + 'deferredSshSessionIdsByTabId', + 'workspaceSessionReady' + ]) { + expect(bindings).toContain(field) + } + }) +}) diff --git a/src/renderer/src/components/status-bar/resource-session-inventory.test.ts b/src/renderer/src/components/status-bar/resource-session-inventory.test.ts index 262485645c0..63784cc00a5 100644 --- a/src/renderer/src/components/status-bar/resource-session-inventory.test.ts +++ b/src/renderer/src/components/status-bar/resource-session-inventory.test.ts @@ -8,7 +8,7 @@ import { import type { DaemonSession } from './resource-usage-merge-types' function session(id: string): DaemonSession { - return { id, cwd: '/workspace', title: id, hasAgentOwner: false } + return { id, cwd: '/workspace', title: id, agentOwnership: 'absent' as const } } describe('resource session inventory', () => { diff --git a/src/renderer/src/components/status-bar/resource-session-kill-confirmation.test.ts b/src/renderer/src/components/status-bar/resource-session-kill-confirmation.test.ts index 87bb0b95681..285d7130f62 100644 --- a/src/renderer/src/components/status-bar/resource-session-kill-confirmation.test.ts +++ b/src/renderer/src/components/status-bar/resource-session-kill-confirmation.test.ts @@ -9,7 +9,7 @@ function row(overrides: Partial = {}): UnifiedSessionRow { pid: 0, label: 'zsh', bound: false, - hasAgentOwner: false, + agentOwnership: 'absent', tabId: null, cpu: null, memory: null, @@ -24,10 +24,16 @@ describe('resource session kill confirmation', () => { }) it('confirms before killing an agent-owned session that has no binding', () => { - expect(requiresKillConfirmation(row({ bound: false, hasAgentOwner: true }))).toBe(true) + expect(requiresKillConfirmation(row({ bound: false, agentOwnership: 'present' }))).toBe(true) }) - it('skips the prompt only when the session is both unbound and unclaimed', () => { + it('confirms when ownership could not be established at all', () => { + // Why: a provider that cannot serialize claims reports no owners for a session that may have + // one. Treating that silence as proof of absence is the #8459 defect. + expect(requiresKillConfirmation(row({ bound: false, agentOwnership: 'unknown' }))).toBe(true) + }) + + it('skips the prompt only on proven absence with no binding', () => { expect(requiresKillConfirmation(row())).toBe(false) }) }) diff --git a/src/renderer/src/components/status-bar/resource-session-kill-confirmation.ts b/src/renderer/src/components/status-bar/resource-session-kill-confirmation.ts index f2db0b166a3..a26982aa7fd 100644 --- a/src/renderer/src/components/status-bar/resource-session-kill-confirmation.ts +++ b/src/renderer/src/components/status-bar/resource-session-kill-confirmation.ts @@ -1,13 +1,14 @@ +import { mayDestroyWithoutOwnerEvidence } from '../../../../shared/pty-listed-session' import type { UnifiedSessionRow } from './resource-usage-merge-types' /** * Whether killing this session must ask first. * * Skipping the prompt is only safe when Orca can show what would be lost — a bound row has a tab - * the user can look at. Absence of a binding is not evidence the session is idle, and agent - * ownership is positive evidence it is not: an agent holds live work with no tab of its own. + * the user can look at. Absence of a binding is not evidence the session is idle, and only proven + * absence of an agent owner is evidence no work is running; an unprovable owner asks. * Resource Manager force-killed exactly those sessions with no prompt (#8459). */ export function requiresKillConfirmation(session: UnifiedSessionRow): boolean { - return session.bound || session.hasAgentOwner + return session.bound || !mayDestroyWithoutOwnerEvidence(session) } diff --git a/src/renderer/src/components/status-bar/resource-usage-merge-types.ts b/src/renderer/src/components/status-bar/resource-usage-merge-types.ts index d2cd136cd2a..c365329c110 100644 --- a/src/renderer/src/components/status-bar/resource-usage-merge-types.ts +++ b/src/renderer/src/components/status-bar/resource-usage-merge-types.ts @@ -4,7 +4,10 @@ import type { TerminalTab, Worktree } from '../../../../shared/types' -import type { PtyListedSession } from '../../../../shared/pty-listed-session' +import type { + AgentOwnershipEvidence, + PtyListedSession +} from '../../../../shared/pty-listed-session' /** `null` === "no local sample" (e.g. SSH PTY); UI renders as em-dash. */ export type Metric = number | null @@ -18,8 +21,8 @@ export type UnifiedSessionRow = { pid: number label: string bound: boolean - /** An agent holds this session. Destroying it discards live work, so it always confirms. */ - hasAgentOwner: boolean + /** Ownership as the provider could establish it; anything but `absent` means confirm first. */ + agentOwnership: AgentOwnershipEvidence tabId: string | null cpu: Metric memory: Metric @@ -58,6 +61,8 @@ export type MergeContext = { ptyIdsByTabId: Record /** From useAppStore: persisted per-leaf PTY wake hints for deferred reattach. */ terminalLayoutsByTabId?: Record + /** From useAppStore: SSH sessions known live but not yet reattached; no other binding sees them. */ + deferredSshSessionIdsByTabId?: Record /** From useAppStore: per-tab live pane titles (for label resolution). */ runtimePaneTitlesByTabId: Record> /** From useAppStore: false until renderer state can distinguish bound/orphan. */ diff --git a/src/renderer/src/components/status-bar/use-resource-session-inventory.test.tsx b/src/renderer/src/components/status-bar/use-resource-session-inventory.test.tsx index 2a440e49702..936f83c3178 100644 --- a/src/renderer/src/components/status-bar/use-resource-session-inventory.test.tsx +++ b/src/renderer/src/components/status-bar/use-resource-session-inventory.test.tsx @@ -5,7 +5,7 @@ import type { DaemonSession } from './resource-usage-merge-types' import { useResourceSessionInventory } from './use-resource-session-inventory' function session(id: string): DaemonSession { - return { id, cwd: '/workspace', title: id, hasAgentOwner: false } + return { id, cwd: '/workspace', title: id, agentOwnership: 'absent' as const } } function deferred(): { promise: Promise; resolve: (value: T) => void } { diff --git a/src/shared/pty-listed-session.ts b/src/shared/pty-listed-session.ts index ce4ecaf32b9..064477dd12a 100644 --- a/src/shared/pty-listed-session.ts +++ b/src/shared/pty-listed-session.ts @@ -1,3 +1,13 @@ +/** + * Whether an agent holds a listed session. + * + * `unknown` is not a variant of `absent`. A provider that cannot serialize claims — a legacy + * daemon generation below the claim protocol, an older SSH relay, the in-process local fallback — + * reports no owners for a session that may well have one. Only a provider whose + * `providesAgentSessionOwnerListings` is true may make listing absence authoritative. + */ +export type AgentOwnershipEvidence = 'present' | 'absent' | 'unknown' + /** * One row of `pty:listSessions`. Shared so the main handler, both preload surfaces, and the * renderer cannot drift on which evidence the UI is allowed to see. @@ -7,9 +17,16 @@ export type PtyListedSession = { cwd: string title: string /** - * An agent holds this session, so work is running regardless of whether the calling renderer - * has a tab binding for it yet. Positive ownership evidence the renderer cannot derive alone — - * dropping it at this boundary is what let Resource Manager force-kill live sessions (#8459). + * Agent ownership as the listing provider could establish it. Destructive actions must treat + * anything other than `absent` as live work: discarding this distinction is what let Resource + * Manager force-kill live agent sessions (#8459). */ - hasAgentOwner: boolean + agentOwnership: AgentOwnershipEvidence +} + +/** Only proven absence authorizes destroying a session without asking. */ +export function mayDestroyWithoutOwnerEvidence(session: { + agentOwnership: AgentOwnershipEvidence +}): boolean { + return session.agentOwnership === 'absent' } diff --git a/tests/e2e/resource-manager-unbound-session-safety.spec.ts b/tests/e2e/resource-manager-unbound-session-safety.spec.ts index 4bcbeda78c3..c2e6c09adb9 100644 --- a/tests/e2e/resource-manager-unbound-session-safety.spec.ts +++ b/tests/e2e/resource-manager-unbound-session-safety.spec.ts @@ -150,18 +150,28 @@ test.describe('Resource Manager unbound-session safety', () => { 'A live warm-reattached session was unbound after restore completed; orphan cleanup would target it' ).toBe(true) - // The ownership field must survive the real IPC boundary, not just the unit-test mock: - // a structured-clone drop or a preload contract mismatch would surface here as undefined. - const ownershipReported = await secondLaunch.page.evaluate(async (expected: string) => { + // Ownership evidence must survive the real IPC boundary, not just the unit-test mock: a + // structured-clone drop or preload contract mismatch would surface here as undefined. + // Asserting the exact arm matters — a stub returning a constant would satisfy a typeof check. + const ownership = await secondLaunch.page.evaluate(async (expected: string) => { const sessions = await window.api.pty.listSessions() - return sessions - .filter((s) => s.id === expected) - .map((s) => typeof s.hasAgentOwner === 'boolean') + return sessions.filter((s) => s.id === expected).map((s) => s.agentOwnership) }, ptyId) expect( - ownershipReported, - 'pty:listSessions dropped hasAgentOwner across the real IPC boundary' - ).toEqual([true]) + ownership, + 'pty:listSessions did not report a valid agentOwnership arm across the real IPC boundary' + ).toHaveLength(1) + expect( + ['present', 'absent', 'unknown'], + 'agentOwnership crossed IPC as an unrecognized value' + ).toContain(ownership[0]) + + // A plain shell under the live local provider must be PROVEN unowned, not merely unknown — + // otherwise the tri-state would be reporting "unknown" for everything and proving nothing. + expect( + ownership[0], + 'The live local provider reported non-authoritative ownership for its own session' + ).toBe('absent') } finally { if (firstApp) { await session.close(firstApp).catch(() => {})