mirror of
https://github.com/stablyai/orca.git
synced 2026-10-22 08:03:37 +00:00
fix(daemon): confirm emptiness before it authorizes a kill
Readiness review, sections 02/03/05/06 — no P0 or P1 in any of them. These are the P2s worth taking. The important one: on macOS a terminal contributes exactly one session leader, the login wrapper, because the shell it forks is in the same session and shows S+ rather than Ss. I had assumed the shell counted too. It does not — so a wrapper that looks childless in a single snapshot makes its whole terminal invisible, and that snapshot cannot tell a wrapper whose shell has gone from one whose shell has not yet appeared. Emptiness is the answer that authorizes a kill, so it now costs a second read; 'owns-live-ptys' still needs none. The fixtures said Ss where a real shell says S+, which is why the tests never noticed. Also from that review: a fabricated row was cast to ProcessTableRow to reuse a command-only predicate, which is sound only while that predicate reads nothing else — narrowed to Pick<'command'> so the compiler keeps it honest. The pty:signal listener relied on its provider staying async to convert a routing refusal into a rejection; it is an ipcMain.on listener with nothing above it, so it now catches synchronously too. And the repro's teardown signalled remembered pids a minute after phase 1 waited for them to die — re-verify by tag first, since signalling a recycled pid is the mistake the script exists to study. Co-authored-by: Orca <help@stably.ai>
This commit is contained in:
@@ -453,6 +453,12 @@ function teardown(staged) {
|
||||
staged.daemon.child.unref()
|
||||
}
|
||||
for (const marker of staged.markers ?? []) {
|
||||
// Why re-verify by tag: phase 1 waits for these pids to die, and teardown runs a minute
|
||||
// later. Signalling a remembered pid after that would be signalling whatever the OS has
|
||||
// since recycled it onto — which is the mistake this whole script exists to study.
|
||||
if (!isMarkerAlive(marker)) {
|
||||
continue
|
||||
}
|
||||
for (const pid of [marker.pid, marker.sessionPid]) {
|
||||
try {
|
||||
process.kill(pid, 'SIGKILL')
|
||||
|
||||
@@ -23,6 +23,9 @@ function row(pid: number, ppid: number, overrides: Partial<ProcessTableRow> = {}
|
||||
|
||||
const daemonRow = row(DAEMON_PID, 1, { command: 'daemon-entry.js' })
|
||||
|
||||
// macOS wraps every terminal in login(1); only the wrapper is the session leader.
|
||||
const LOGIN_WRAPPER = '/usr/bin/login -flpq nwparker /bin/bash …'
|
||||
|
||||
function windowsCandidate(pid: number): WindowsProcessCandidate {
|
||||
return { pid, ppid: DAEMON_PID, name: 'bash.exe', command: 'bash', executablePath: '', depth: 1 }
|
||||
}
|
||||
@@ -49,7 +52,7 @@ describe('inspectDaemonPtyOwnership on POSIX', () => {
|
||||
readPosixProcessTable: posixTable([
|
||||
daemonRow,
|
||||
row(101, DAEMON_PID, { command: '/usr/bin/login -flpq nwparker' }),
|
||||
row(202, 101, { command: 'claude' })
|
||||
row(202, 101, { stat: 'S+', command: 'claude' })
|
||||
])
|
||||
})
|
||||
).resolves.toBe('owns-live-ptys')
|
||||
@@ -87,7 +90,7 @@ describe('inspectDaemonPtyOwnership on POSIX', () => {
|
||||
readPosixProcessTable: posixTable([
|
||||
daemonRow,
|
||||
row(101, DAEMON_PID, { stat: 'Z', command: '<defunct>' }),
|
||||
row(202, 101, { command: 'codex' })
|
||||
row(202, 101, { stat: 'Ss', command: 'codex' })
|
||||
])
|
||||
})
|
||||
).resolves.toBe('owns-live-ptys')
|
||||
@@ -408,13 +411,32 @@ describe('inspectDaemonPtyOwnership sampling', () => {
|
||||
).resolves.toBe('owns-live-ptys')
|
||||
})
|
||||
|
||||
it('answers no-live-ptys on the first conclusive read without re-sampling', async () => {
|
||||
it('confirms emptiness with a second read before letting it authorize a kill', async () => {
|
||||
const readPosixProcessTable = vi.fn(async () => [daemonRow])
|
||||
|
||||
await expect(
|
||||
inspectDaemonPtyOwnership(DAEMON_PID, { platform: 'darwin', readPosixProcessTable })
|
||||
).resolves.toBe('no-live-ptys')
|
||||
expect(readPosixProcessTable).toHaveBeenCalledTimes(1)
|
||||
expect(readPosixProcessTable).toHaveBeenCalledTimes(2)
|
||||
})
|
||||
|
||||
it('sees a terminal whose shell had not yet appeared on the first read', async () => {
|
||||
// A terminal contributes exactly one session leader — on macOS the login wrapper — and it
|
||||
// is childless for the moment between forkpty creating it and the shell appearing. One
|
||||
// snapshot cannot tell that from a wrapper whose shell has gone, and guessing wrong here
|
||||
// kills a live terminal.
|
||||
const readPosixProcessTable = vi
|
||||
.fn<() => Promise<ProcessTableRow[]>>()
|
||||
.mockResolvedValueOnce([daemonRow, row(101, DAEMON_PID, { command: LOGIN_WRAPPER })])
|
||||
.mockResolvedValueOnce([
|
||||
daemonRow,
|
||||
row(101, DAEMON_PID, { command: LOGIN_WRAPPER }),
|
||||
row(202, 101, { stat: 'S+', command: '/opt/homebrew/bin/bash --rcfile …' })
|
||||
])
|
||||
|
||||
await expect(
|
||||
inspectDaemonPtyOwnership(DAEMON_PID, { platform: 'darwin', readPosixProcessTable })
|
||||
).resolves.toBe('owns-live-ptys')
|
||||
})
|
||||
|
||||
it('gives up as unknown rather than guessing when every read stays blind', async () => {
|
||||
|
||||
@@ -89,7 +89,7 @@ function programBasename(command: string): string {
|
||||
return base.endsWith('.exe') ? base.slice(0, -'.exe'.length) : base
|
||||
}
|
||||
|
||||
function isDaemonSelfSpawnedPty(row: ProcessTableRow): boolean {
|
||||
function isDaemonSelfSpawnedPty(row: Pick<ProcessTableRow, 'command'>): boolean {
|
||||
const command = row.command.trim().toLowerCase()
|
||||
return DAEMON_SELF_SPAWNED_PTY_COMMANDS.some(({ program, args }) => {
|
||||
const suffix = ` ${args}`
|
||||
@@ -176,9 +176,7 @@ async function probeOnce(
|
||||
if (descendants === null) {
|
||||
return 'unknown'
|
||||
}
|
||||
const hosted = descendants.filter(
|
||||
(row) => !isDaemonSelfSpawnedPty({ command: row.command } as ProcessTableRow)
|
||||
)
|
||||
const hosted = descendants.filter((row) => !isDaemonSelfSpawnedPty(row))
|
||||
return hosted.length > 0 ? 'owns-live-ptys' : 'no-live-ptys'
|
||||
}
|
||||
const deadlineMs = deps.posixDeadlineMs ?? POSIX_OWNERSHIP_PROBE_DEADLINE_MS
|
||||
@@ -220,6 +218,7 @@ export async function inspectDaemonPtyOwnership(
|
||||
deps: DaemonPtyOwnershipDeps = {}
|
||||
): Promise<DaemonPtyOwnership> {
|
||||
const platform = deps.platform ?? process.platform
|
||||
let sawEmpty = false
|
||||
for (let attempt = 0; attempt < PTY_OWNERSHIP_PROBE_ATTEMPTS; attempt++) {
|
||||
let sample: DaemonPtyOwnership
|
||||
try {
|
||||
@@ -227,9 +226,20 @@ export async function inspectDaemonPtyOwnership(
|
||||
} catch {
|
||||
sample = 'unknown'
|
||||
}
|
||||
// Why a second look before accepting emptiness on POSIX: a terminal contributes exactly
|
||||
// one session leader — on macOS the login wrapper — and it is invisible for the moment
|
||||
// between forkpty creating it and the shell appearing beneath it. One snapshot cannot tell
|
||||
// that from a wrapper whose shell has gone. Emptiness authorizes a kill, so it is the
|
||||
// answer worth paying a second read for; 'owns-live-ptys' needs no confirmation.
|
||||
if (sample === 'no-live-ptys' && platform !== 'win32' && !sawEmpty) {
|
||||
sawEmpty = true
|
||||
continue
|
||||
}
|
||||
if (sample !== 'unknown') {
|
||||
return sample
|
||||
}
|
||||
}
|
||||
return 'unknown'
|
||||
// Only reached when every sample was blind, or when the confirming read disagreed with an
|
||||
// emptiness it could not corroborate.
|
||||
return sawEmpty ? 'no-live-ptys' : 'unknown'
|
||||
}
|
||||
|
||||
+9
-3
@@ -7444,9 +7444,15 @@ export function registerPtyHandlers(
|
||||
|
||||
ipcMain.removeAllListeners('pty:signal')
|
||||
ipcMain.on('pty:signal', (_event, args: { id: string; signal: string }) => {
|
||||
tryGetProviderForPty(args.id)
|
||||
?.sendSignal(args.id, args.signal)
|
||||
.catch(() => {})
|
||||
try {
|
||||
// Why try/catch as well as .catch: routing refuses a session whose host is unreachable,
|
||||
// and it may do so synchronously. Nothing sits above an ipcMain.on listener to catch it.
|
||||
tryGetProviderForPty(args.id)
|
||||
?.sendSignal(args.id, args.signal)
|
||||
.catch(() => {})
|
||||
} catch {
|
||||
/* unreachable host — the signal is undeliverable and there is no caller to tell */
|
||||
}
|
||||
})
|
||||
|
||||
ipcMain.removeAllListeners('pty:clearBuffer')
|
||||
|
||||
Reference in New Issue
Block a user