mirror of
https://github.com/stablyai/orca.git
synced 2026-09-22 08:02:28 +00:00
fix(daemon): let PTY inventory recover from a dead terminal host (#16363)
Worktree removal inventories PTYs through DaemonPtyAdapter.listProcesses. That called ensureConnected bare, so once the terminal-host pipe was dead the removal failed with `connect ENOENT \\?\pipe\orca-terminal-host-...` and stayed broken until the whole app was restarted. spawn already wrapped its work in withDaemonRetry and recovered from exactly this. Inventory did not — so the one path that must not get stuck was the only one that could not heal itself. Both the connect and the listSessions request go inside the retry: a host that dies between them throws the same daemon-gone error, so retrying only the connect would still fail. The reconciliation after the request is deliberately outside it; retrying that would be wrong. Reproduced first, with a real daemon killed mid-test: listProcesses threw DaemonConnectionLostError while a control asserting spawn recovery from the identical kill passed. Both are now regression tests, so the asymmetry cannot come back silently. Co-authored-by: innocarpe <innocarpe@users.noreply.github.com>
This commit is contained in:
@@ -0,0 +1,76 @@
|
||||
/* Inventory after the terminal host dies: worktree removal must not hard-fail. */
|
||||
import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'
|
||||
import { DaemonPtyAdapter } from './daemon-pty-adapter'
|
||||
import { DaemonServer } from './daemon-server'
|
||||
import { createMockSubprocess, startDaemonAdapterHarness } from './daemon-pty-adapter-test-harness'
|
||||
|
||||
describe('inventory after the terminal host dies (#10087)', () => {
|
||||
let harness: Awaited<ReturnType<typeof startDaemonAdapterHarness>>
|
||||
let respawnServer: DaemonServer | undefined
|
||||
|
||||
beforeEach(async () => {
|
||||
harness = await startDaemonAdapterHarness(() => createMockSubprocess())
|
||||
})
|
||||
afterEach(async () => {
|
||||
harness.adapter.dispose()
|
||||
await respawnServer?.shutdown()
|
||||
await harness.server.shutdown().catch(() => {})
|
||||
respawnServer = undefined
|
||||
})
|
||||
|
||||
/** An adapter that can bring the host back, as production does. */
|
||||
const healingAdapter = (): { adapter: DaemonPtyAdapter; respawn: ReturnType<typeof vi.fn> } => {
|
||||
const respawn = vi.fn(async () => {
|
||||
respawnServer = new DaemonServer({
|
||||
socketPath: harness.socketPath,
|
||||
tokenPath: harness.tokenPath,
|
||||
spawnSubprocess: () => createMockSubprocess()
|
||||
})
|
||||
await respawnServer.start()
|
||||
})
|
||||
return {
|
||||
adapter: new DaemonPtyAdapter({
|
||||
socketPath: harness.socketPath,
|
||||
tokenPath: harness.tokenPath,
|
||||
respawn
|
||||
}),
|
||||
respawn
|
||||
}
|
||||
}
|
||||
|
||||
it('lists processes after the host dies instead of failing the removal', async () => {
|
||||
// The reported failure: worktree remove inventories PTYs through
|
||||
// listProcesses, and a dead named pipe surfaced as
|
||||
// `connect ENOENT \\?\pipe\orca-terminal-host-...`, blocking removal until
|
||||
// the whole app was restarted. spawn already recovers from this; inventory
|
||||
// did not, so the destructive path was the one that could not heal.
|
||||
const { adapter, respawn } = healingAdapter()
|
||||
try {
|
||||
await adapter.spawn({ cols: 80, rows: 24 })
|
||||
await harness.server.shutdown()
|
||||
|
||||
const processes = await adapter.listProcesses()
|
||||
|
||||
expect(Array.isArray(processes)).toBe(true)
|
||||
expect(respawn).toHaveBeenCalled()
|
||||
} finally {
|
||||
adapter.dispose()
|
||||
}
|
||||
})
|
||||
|
||||
it('still recovers on spawn, the path that already worked', async () => {
|
||||
// Control: proves the harness really kills the host, so the test above is
|
||||
// not passing because nothing broke.
|
||||
const { adapter, respawn } = healingAdapter()
|
||||
try {
|
||||
await adapter.spawn({ cols: 80, rows: 24 })
|
||||
await harness.server.shutdown()
|
||||
|
||||
await adapter.spawn({ cols: 80, rows: 24 })
|
||||
|
||||
expect(respawn).toHaveBeenCalled()
|
||||
} finally {
|
||||
adapter.dispose()
|
||||
}
|
||||
})
|
||||
})
|
||||
@@ -21,14 +21,27 @@ export abstract class DaemonPtySessionInventory extends DaemonPtyProcessInspecti
|
||||
// be reconciled away below.
|
||||
const preRequestActiveIds = new Set(this.activeSessionIds)
|
||||
try {
|
||||
// Why retry: this inventory is what destructive teardown consults, and a
|
||||
// dead host pipe surfaced as `connect ENOENT \\?\\pipe\\orca-terminal-host-...`
|
||||
// that failed worktree removal until the app was restarted (#10087). Spawn
|
||||
// already recovered from exactly this; inventory did not, so the one path
|
||||
// that must not get stuck was the only one that could not heal.
|
||||
//
|
||||
// Why the request is inside too: a host that dies between connect and
|
||||
// listSessions throws the same daemon-gone error, so retrying only the
|
||||
// connect would still fail. The reconciliation below stays outside --
|
||||
// retrying that would double-apply it.
|
||||
//
|
||||
// Why: connect + listSessions share the caller's one absolute deadline so a
|
||||
// wedged handshake cannot burn the whole teardown budget before the list issues.
|
||||
await this.ensureConnected(opts?.deadlineMs)
|
||||
const result = await this.client.request<ListSessionsResult>(
|
||||
'listSessions',
|
||||
undefined,
|
||||
remainingDaemonRequestTimeoutMs(opts?.deadlineMs)
|
||||
)
|
||||
const result = await this.withDaemonRetry(async () => {
|
||||
await this.ensureConnected(opts?.deadlineMs)
|
||||
return this.client.request<ListSessionsResult>(
|
||||
'listSessions',
|
||||
undefined,
|
||||
remainingDaemonRequestTimeoutMs(opts?.deadlineMs)
|
||||
)
|
||||
})
|
||||
const admission = new PtyProcessListAdmission()
|
||||
const processes: PtyProcessInfo[] = []
|
||||
const aliveSessionIds = new Set<string>()
|
||||
|
||||
Reference in New Issue
Block a user