Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -486,9 +486,13 @@ describe('DaemonPtyAdapter (IPtyProvider)', () => {
const probeAdapter = new DaemonPtyAdapter({ socketPath, tokenPath, protocolVersion })
;(
probeAdapter as unknown as {
client: { request: ReturnType<typeof vi.fn>; disconnect: ReturnType<typeof vi.fn> }
client: {
request: ReturnType<typeof vi.fn>
disconnect: ReturnType<typeof vi.fn>
isConnected: () => boolean
}
}
).client = { request, disconnect: vi.fn() }
).client = { request, disconnect: vi.fn(), isConnected: () => true }
return probeAdapter
}

Expand Down
45 changes: 45 additions & 0 deletions src/main/daemon/daemon-pty-adapter-unattached-presence.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,45 @@
/* A fresh adapter (new app process) answering presence for a session an older process spawned. */
import { afterEach, beforeEach, describe, expect, it } from 'vitest'
import { rmSync } from 'node:fs'
import { DaemonPtyAdapter } from './daemon-pty-adapter'
import { resolveProviderPtyPresence } from '../ipc/pty/provider/liveness'
import { createMockSubprocess, startDaemonAdapterHarness } from './daemon-pty-adapter-test-harness'

describe('DaemonPtyAdapter presence for a session this process never attached', () => {
let harness: Awaited<ReturnType<typeof startDaemonAdapterHarness>>
let restoredAdapter: DaemonPtyAdapter

beforeEach(async () => {
harness = await startDaemonAdapterHarness(() => createMockSubprocess())
restoredAdapter = new DaemonPtyAdapter({
socketPath: harness.socketPath,
tokenPath: harness.tokenPath
})
})
afterEach(async () => {
restoredAdapter.dispose()
harness.adapter.dispose()
await harness.server.shutdown()
rmSync(harness.dir, { recursive: true, force: true })
})

it('reads the daemon instead of trusting its own empty cache', async () => {
const { id } = await harness.adapter.spawn({ cols: 80, rows: 24 })

// The cache is what `hasPty` alone answers; it is empty because nothing in this process
// attached the session. The daemon still owns a live process under that id.
expect(restoredAdapter.hasPty(id)).toBe(false)
await expect(resolveProviderPtyPresence(restoredAdapter, id)).resolves.toBe(true)
})

it('still answers absent for an id the daemon never had', async () => {
await expect(resolveProviderPtyPresence(restoredAdapter, 'never-spawned')).resolves.toBe(false)
})

it('answers absent after the session exits', async () => {
const { id } = await harness.adapter.spawn({ cols: 80, rows: 24 })
await harness.adapter.shutdown(id, { immediate: true })

await expect(resolveProviderPtyPresence(restoredAdapter, id)).resolves.toBe(false)
})
})
5 changes: 5 additions & 0 deletions src/main/daemon/daemon-pty-session-control.ts
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,11 @@ export abstract class DaemonPtySessionControl extends DaemonPtySessionInput {

async probePtyLiveness(id: string): Promise<boolean | null> {
try {
// Why: a fresh adapter (new app process) has never connected; without this the readback
// answers `null` for every session an older process spawned, and the cache miss stands.
if (!this.client.isConnected()) {
await this.ensureConnected(Date.now() + LIVENESS_PROBE_TIMEOUT_MS)
}
if (!this.getSizeUnsupported && this.protocolVersion >= GET_SIZE_PROTOCOL_VERSION) {
try {
const result = await this.client.request<{ size: { cols: number; rows: number } | null }>(
Expand Down
105 changes: 105 additions & 0 deletions src/main/ipc/pty-has-pty-cache-miss-presence.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,105 @@
import { describe, expect, it, vi } from 'vitest'
import { setupPtyIpcSuite } from './pty-ipc-test-harness'
import { registerPtyHandlers } from './pty'

vi.mock('electron', () => import('./pty-ipc-mock-registry').then((m) => m.electronModuleMock()))
vi.mock('fs', () => import('./pty-ipc-mock-registry').then((m) => m.fsModuleMock()))
vi.mock('node-pty', () => import('./pty-ipc-mock-registry').then((m) => m.nodePtyModuleMock()))
vi.mock('node:child_process', async (importOriginal) =>
(await import('./pty-ipc-mock-registry')).childProcessModuleMock(await importOriginal())
)
vi.mock('../opencode/hook-service', () =>
import('./pty-ipc-mock-registry').then((m) => m.openCodeHookServiceModuleMock())
)
vi.mock('../mimo/hook-service', () =>
import('./pty-ipc-mock-registry').then((m) => m.mimoHookServiceModuleMock())
)
vi.mock('../agent-hooks/server', () =>
import('./pty-ipc-mock-registry').then((m) => m.agentHookServerModuleMock())
)
vi.mock('../pi/titlebar-extension-service', () =>
import('./pty-ipc-mock-registry').then((m) => m.piTitlebarExtensionModuleMock())
)
vi.mock('../pwsh', () => import('./pty-ipc-mock-registry').then((m) => m.pwshModuleMock()))
vi.mock('../wsl', async (importOriginal) =>
(await import('./pty-ipc-mock-registry')).wslModuleMock(await importOriginal())
)
vi.mock('../telemetry/client', () =>
import('./pty-ipc-mock-registry').then((m) => m.telemetryClientModuleMock())
)
vi.mock('../telemetry/classify-error', () =>
import('./pty-ipc-mock-registry').then((m) => m.classifyErrorModuleMock())
)
vi.mock('../cli/linux-terminal-orca-cli-shim', () =>
import('./pty-ipc-mock-registry').then((m) => m.linuxCliShimModuleMock())
)
vi.mock('../memory/pty-registry', () =>
import('./pty-ipc-mock-registry').then((m) => m.ptyRegistryModuleMock())
)
vi.mock('../agent-hooks/migration-unsupported-pty-state', () =>
import('./pty-ipc-mock-registry').then((m) => m.migrationUnsupportedPtyModuleMock())
)
vi.mock('../codex/codex-pane-account-registry', () =>
import('./pty-ipc-mock-registry').then((m) => m.codexPaneAccountRegistryModuleMock())
)
vi.mock('../codex/codex-state-db-backfill-recovery', () =>
import('./pty-ipc-mock-registry').then((m) => m.codexBackfillRecoveryModuleMock())
)

// A retained orchestration worker's pane re-attaches renderer-only after an app restart, so the
// daemon adapter's `hasPty` cache never learns the session. The renderer's visibility reconciler
// closes a pane on exactly the `false` that cache miss used to produce, even though the daemon
// still runs the process (Slack thread "disappearing orchestrator-worker tabs", #16904 regression).
describe('pty:hasPty on a daemon-adapter cache miss', () => {
const { handlers, mainWindow, installDaemonTestProvider } = setupPtyIpcSuite()

it('asks the provider readback before answering absent', async () => {
const probePtyLiveness = vi.fn(async (id: string) => id === 'retained-worker-pty')
installDaemonTestProvider({ hasPty: () => false, probePtyLiveness })
registerPtyHandlers(mainWindow as never)

await expect(handlers.get('pty:hasPty')!(null, { id: 'retained-worker-pty' })).resolves.toBe(
true
)
expect(probePtyLiveness).toHaveBeenCalledWith('retained-worker-pty')
})

it('keeps the readback verdict when the daemon really has no such session', async () => {
installDaemonTestProvider({
hasPty: () => false,
probePtyLiveness: vi.fn(async () => false)
})
registerPtyHandlers(mainWindow as never)

await expect(handlers.get('pty:hasPty')!(null, { id: 'reaped-pty' })).resolves.toBe(false)
})

it('answers unverifiable when the readback cannot reach the daemon', async () => {
installDaemonTestProvider({
hasPty: () => false,
probePtyLiveness: vi.fn(async () => null)
})
registerPtyHandlers(mainWindow as never)

await expect(handlers.get('pty:hasPty')!(null, { id: 'unreachable-pty' })).resolves.toBe(null)
})

it('does not spend a readback on a cache hit', async () => {
const probePtyLiveness = vi.fn(async () => false)
installDaemonTestProvider({ hasPty: (id: string) => id === 'attached-pty', probePtyLiveness })
registerPtyHandlers(mainWindow as never)

await expect(handlers.get('pty:hasPty')!(null, { id: 'attached-pty' })).resolves.toBe(true)
expect(probePtyLiveness).not.toHaveBeenCalled()
})

it('keeps the sole in-process provider authoritative when it has no readback', async () => {
// The in-process LocalPtyProvider is its own only owner (#12393): its cache IS the process table.
installDaemonTestProvider({ hasPty: () => false })
registerPtyHandlers(mainWindow as never)

await expect(handlers.get('pty:hasPty')!(null, { id: 'never-spawned-pty' })).resolves.toBe(
false
)
})
})
8 changes: 5 additions & 3 deletions src/main/ipc/pty/ipc/inspect.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import {
} from '../../../providers/pty-process-list-admission'
import type { PtyListedSession } from '../../../../shared/pty-listed-session'
import { ptyOwnership } from '../provider/ownership-state'
import { resolveProviderPtyPresence } from '../provider/liveness'
import {
getProviderForPty,
hasPtyProviderForInspection,
Expand Down Expand Up @@ -137,13 +138,14 @@ export function installPtyInspectIpcHandlers(deps: {
const provider = parsedSshId
? sshProviders.get(parsedSshId.connectionId)
: tryGetProviderForPty(args.id)
if (!provider?.hasPty) {
if (!provider) {
return null
}
try {
return provider.hasPty(args.id)
// Why: liveness is only allowed to close panes on an authoritative false, and the daemon
// adapter's `hasPty` is a cache of what THIS process attached, not what the daemon runs.
return await resolveProviderPtyPresence(provider, args.id)
} catch {
// Why: liveness is only allowed to close panes on an authoritative false.
return null
}
})
Expand Down
19 changes: 19 additions & 0 deletions src/main/ipc/pty/provider/liveness.ts
Original file line number Diff line number Diff line change
Expand Up @@ -104,6 +104,25 @@ export function delay(ms: number): Promise<void> {
})
}

/**
* The presence answer a renderer may tear a pane down on. `hasPty` on the daemon adapter is an
* in-process cache of the sessions THIS process attached; a pane that attached renderer-only (a
* retained orchestration worker after an app restart) never warms it, so a cache miss is not an
* observation. Only the owner's readback may answer `false` (docs/reference/ssh-execution-boundary.md).
*/
export async function resolveProviderPtyPresence(
provider: IPtyProvider,
ptyId: string
): Promise<boolean | null> {
if (!provider.hasPty) {
return null
}
if (provider.hasPty(ptyId)) {
return true
}
return provider.probePtyLiveness ? await provider.probePtyLiveness(ptyId) : false
}

export async function isProviderPtyLive(
provider: IPtyProvider,
ptyId: string,
Expand Down
Loading
Loading