mirror of
https://github.com/stablyai/orca.git
synced 2026-10-21 16:03:26 +00:00
fix(daemon): stop the emptiness confirmation reading the same snapshot twice
The second sample exists because one snapshot cannot tell a login(1) wrapper whose shell has not appeared yet from a wrapper whose shell has gone. But when the fresh read misses its deadline — the busy host this evidence exists to protect — both attempts fell through to the same TTL-cached table. Two agreeing samples, one observation, and the window being excluded is shorter than the cache. The confirming read is now denied the cached fallback. If it cannot get a fresh table it answers 'unknown', which holds. The first sample keeps the fallback, because there the cache protects the answer worth protecting: a stale table still shows that a daemon has children, and going blind there is what got them killed. Also records three limits of this evidence that review surfaced and that no code change should paper over — a reparented orphan outside the descendant tree, the Windows abstention, and an argv match that cannot establish executable identity. Each only fails to raise a verdict, so each costs a hold not taken rather than a kill licensed.
This commit is contained in:
@@ -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
|
||||
|
||||
@@ -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<ProcessTableRow[]>>(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<ProcessTableRow[]>(() => {}),
|
||||
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.
|
||||
|
||||
@@ -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<DaemonPtyOwnership> {
|
||||
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'
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user