diff --git a/src/main/daemon/AGENTS.md b/src/main/daemon/AGENTS.md index 050a355fdfd..2c5f555be0a 100644 --- a/src/main/daemon/AGENTS.md +++ b/src/main/daemon/AGENTS.md @@ -89,6 +89,19 @@ hands → probe once more → `rename` in one syscall → verify we kept it. read only afterwards. Pre-existing, and narrowed by this change rather than widened: the window now opens only after the daemon has itself reported zero sessions. + Known limits of the process-table evidence, none of which can license a kill on their own — + each only fails to *raise* a verdict, so the cost is a hold not taken: + + - A PTY whose session leader has exited leaves its still-running child reparented outside the + daemon's descendant tree. The walk cannot see it, so a daemon with real work can read as + childless. + - On Windows the evidence abstains entirely. A daemon that closed its listener but is still + draining sessions therefore has no protection from the endpoint-dead path. + - The self-spawned-probe exclusion matches an exact argv (`sh -c exit 0`). A hosted session + leader whose executable basename is `sh` and whose command is exactly that would be + discarded. Contrived — `exit 0` returns immediately — but it is executable identity the + match cannot establish. + **If you ever raise the classification budget, gate the replace path on headroom first.** The budget serves two verdicts with opposite time-costs: reaching "don't kill" slowly is free, because the daemon survives however long it took, while reaching `empty` slowly is not — the diff --git a/src/main/daemon/daemon-live-pty-evidence.test.ts b/src/main/daemon/daemon-live-pty-evidence.test.ts index 9a2ce8df9bb..d1d12f9bee2 100644 --- a/src/main/daemon/daemon-live-pty-evidence.test.ts +++ b/src/main/daemon/daemon-live-pty-evidence.test.ts @@ -271,6 +271,29 @@ describe('inspectDaemonPtyOwnership on win32', () => { }) describe('inspectDaemonPtyOwnership sampling', () => { + it('will not confirm emptiness from the cached table the first read already used', async () => { + // Two agreeing samples are only worth more than one if they are two observations. When the + // fresh read is slow — the busy host this evidence exists for — both attempts fell through + // to the same TTL-cached snapshot, so a login(1) wrapper photographed before its shell + // appeared could be 'confirmed' empty by a second look at the same photograph. + const readCachedPosixProcessTable = vi.fn<() => Promise>(async () => [ + daemonRow + ]) + + await expect( + inspectDaemonPtyOwnership(DAEMON_PID, { + platform: 'darwin', + posixDeadlineMs: 5, + // Never settles inside the deadline, so every attempt reaches for the cache. + readPosixProcessTable: () => new Promise(() => {}), + readCachedPosixProcessTable + }) + ).resolves.toBe('unknown') + + // Only the first sample may be served from the cache; the confirming one must not be. + expect(readCachedPosixProcessTable).toHaveBeenCalledTimes(1) + }) + it('will not let a blind confirming read turn emptiness into a verdict', async () => { // Emptiness authorizes a kill, so it needs corroboration. A read that saw nothing at all // corroborates nothing — treating it as agreement is the step this module refuses. diff --git a/src/main/daemon/daemon-live-pty-evidence.ts b/src/main/daemon/daemon-live-pty-evidence.ts index 205d503bb1b..e77e47c2710 100644 --- a/src/main/daemon/daemon-live-pty-evidence.ts +++ b/src/main/daemon/daemon-live-pty-evidence.ts @@ -149,7 +149,15 @@ function collectLivePtyDescendants(rows: ProcessTableRow[], rootPid: number): Pr /** POSIX only; Windows abstains before this is reached. */ async function probeOnce( daemonPid: number, - deps: DaemonPtyOwnershipDeps + deps: DaemonPtyOwnershipDeps, + /** + * The cached table may stand in for a slow read when the answer we are protecting is + * 'owns-live-ptys', but never when confirming emptiness: a confirmation drawn from the same + * TTL-cached snapshot as the sample it confirms is one observation counted twice, and the + * window it is meant to exclude — a login(1) wrapper whose shell has not appeared yet — is + * shorter than the cache. Denied there, a slow read answers 'unknown', which holds. + */ + allowCachedFallback = true ): Promise { const deadlineMs = deps.posixDeadlineMs ?? POSIX_OWNERSHIP_PROBE_DEADLINE_MS const deadline = Date.now() + deadlineMs @@ -166,11 +174,13 @@ async function probeOnce( // few hundred milliseconds old still shows whether this daemon has children, and going // blind here gets them killed. It shares the attempt's budget rather than doubling it, // so an attempt costs what the launch budget was told it costs. - (await withDeadline( - (deps.readCachedPosixProcessTable ?? getProcessTableSnapshot)(), - remaining(), - null - )) + (allowCachedFallback + ? await withDeadline( + (deps.readCachedPosixProcessTable ?? getProcessTableSnapshot)(), + remaining(), + null + ) + : null) // Why: a walk that never saw the root reports zero descendants for a process it // never examined. Only a root we actually observed can prove emptiness — and a read // that blew its deadline saw nothing at all. @@ -206,7 +216,8 @@ export async function inspectDaemonPtyOwnership( for (let attempt = 0; attempt < PTY_OWNERSHIP_PROBE_ATTEMPTS; attempt++) { let sample: DaemonPtyOwnership try { - sample = await probeOnce(daemonPid, deps) + // The confirming read must be its own observation, so it is denied the cached table. + sample = await probeOnce(daemonPid, deps, !emptyAwaitingConfirmation) } catch { sample = 'unknown' }