mirror of
https://github.com/stablyai/orca.git
synced 2026-10-20 00:03:45 +00:00
fix(resource-manager): distinguish unprovable ownership from proven absence
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.
This commit is contained in:
OrcaWin
@@ -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 () => {
|
||||
|
||||
+10
-5
@@ -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'
|
||||
})
|
||||
}
|
||||
}
|
||||
|
||||
@@ -45,7 +45,7 @@ function makeSession(overrides: Partial<UnifiedSessionRow>): UnifiedSessionRow {
|
||||
pid: 100,
|
||||
label: 'zsh',
|
||||
bound: true,
|
||||
hasAgentOwner: false,
|
||||
agentOwnership: 'absent' as const,
|
||||
tabId: 'tab-1',
|
||||
cpu: 1,
|
||||
memory: 100,
|
||||
|
||||
@@ -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,
|
||||
|
||||
@@ -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<string, string | null>([
|
||||
@@ -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']])
|
||||
|
||||
@@ -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,
|
||||
|
||||
@@ -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: {},
|
||||
|
||||
@@ -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(
|
||||
|
||||
@@ -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)
|
||||
}
|
||||
})
|
||||
})
|
||||
@@ -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', () => {
|
||||
|
||||
@@ -9,7 +9,7 @@ function row(overrides: Partial<UnifiedSessionRow> = {}): 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)
|
||||
})
|
||||
})
|
||||
|
||||
@@ -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)
|
||||
}
|
||||
|
||||
@@ -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<string, string[]>
|
||||
/** From useAppStore: persisted per-leaf PTY wake hints for deferred reattach. */
|
||||
terminalLayoutsByTabId?: Record<string, TerminalLayoutSnapshot>
|
||||
/** From useAppStore: SSH sessions known live but not yet reattached; no other binding sees them. */
|
||||
deferredSshSessionIdsByTabId?: Record<string, string>
|
||||
/** From useAppStore: per-tab live pane titles (for label resolution). */
|
||||
runtimePaneTitlesByTabId: Record<string, Record<number, string>>
|
||||
/** From useAppStore: false until renderer state can distinguish bound/orphan. */
|
||||
|
||||
@@ -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<T>(): { promise: Promise<T>; resolve: (value: T) => void } {
|
||||
|
||||
@@ -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'
|
||||
}
|
||||
|
||||
@@ -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(() => {})
|
||||
|
||||
Reference in New Issue
Block a user