mirror of
https://github.com/stablyai/orca.git
synced 2026-10-23 08:03:39 +00:00
fix(daemon): bound the POSIX evidence read and re-verify the pid it describes
Round-six follow-ups, none destructive. The process-table read had a deadline on Windows but not on POSIX, where the shared reader's ps timeout does not cover queueing behind an in-flight scan — so the one step that runs after the grace window could still outlast it. The pid handed to that read was verified before the grace window, which is long enough for the daemon to die and its pid to be recycled onto a shell with children. Verify it where it is used instead, and only when there is still something to raise: the common case now skips the identity probe altogether, which also takes a few seconds off the worst-case launch. Two renderer call sites killed PTYs without handling rejection. That was harmless while an unreachable session was answered by a silent no-op; now that it honestly rejects, pane teardown and repo removal would log an unhandled rejection every time — exactly when the daemon is already sick. Deliberately not taken from that review: giving the endpoint-occupied catch the same held fallback as the failed-health path. That path arrives with occupancy unknown or empty, so holding there would swallow a real launch failure to protect nothing. Splits the repro script, which had grown past the line limit, into the sequence it proves and the two things it proves it with: process-table inspection, and the static assertions on the launcher's hold decision. Co-authored-by: Orca <help@stably.ai>
This commit is contained in:
@@ -3444,7 +3444,9 @@ describe('daemon-init: runRestartDaemon (7-step sequence)', () => {
|
||||
})
|
||||
|
||||
it('grace budget is generous enough to ride out a ~60s transient wedge', () => {
|
||||
// Why: each probe waits the client's 5s hello timeout, so 1 + 11 probes ≈ 60s of drain grace; don't cut without telemetry.
|
||||
// Why still pinned: the wall-clock ceiling binds first, so this is an upper bound on
|
||||
// retries rather than the real budget — but lowering it silently shortens the drain
|
||||
// window for a daemon whose process table cannot be read, which has no other protection.
|
||||
expect(WEDGED_DAEMON_GRACE_RETRIES).toBeGreaterThanOrEqual(11)
|
||||
})
|
||||
|
||||
|
||||
@@ -598,7 +598,6 @@ function createOutOfProcessLauncher(
|
||||
} else {
|
||||
// Why: a busy machine can time out the health check on a live daemon; re-verify what
|
||||
// it is hosting before killing its sessions.
|
||||
const verifiedPid = await readVerifiedDaemonPid(runtimeDir, socketPath, tokenPath)
|
||||
// Why recordedPid is withheld here: the loop below is waiting for *IPC* to recover,
|
||||
// and the process table cannot change its answer within a grace window. Scanning it
|
||||
// every pass would multiply the launch budget for an answer we already have.
|
||||
@@ -618,7 +617,13 @@ function createOutOfProcessLauncher(
|
||||
}
|
||||
// One scan, once IPC has had its full chance: this is the evidence that separates a
|
||||
// wedged daemon still hosting terminals from one with nothing left to lose (#8689).
|
||||
occupancy = await raiseOccupancyWithProcessEvidence(occupancy, verifiedPid?.pid ?? null)
|
||||
// Why re-verify: the grace window is long enough for the daemon to die and its pid to
|
||||
// be recycled, and the evidence would then describe a stranger's children.
|
||||
const evidencePid =
|
||||
occupancy.state === 'unknown'
|
||||
? ((await readVerifiedDaemonPid(runtimeDir, socketPath, tokenPath))?.pid ?? null)
|
||||
: null
|
||||
occupancy = await raiseOccupancyWithProcessEvidence(occupancy, evidencePid)
|
||||
if (occupancy.state === 'occupied') {
|
||||
const owned =
|
||||
occupancy.liveSessions === null
|
||||
@@ -966,6 +971,9 @@ function createOutOfProcessLauncher(
|
||||
try {
|
||||
return await preserveDaemon('degraded-new-pty-fallback')
|
||||
} catch {
|
||||
// Why not hold here, unlike the failed-health path: that one declined to kill
|
||||
// because it had proof of live work. This one arrives with occupancy unknown or
|
||||
// empty, so holding would swallow a real launch failure to protect nothing.
|
||||
// It stopped answering between the probe and the adoption; report the launch failure.
|
||||
}
|
||||
}
|
||||
|
||||
@@ -25,6 +25,7 @@ export type DaemonPtyOwnershipDeps = {
|
||||
readPosixProcessTable?: () => Promise<ProcessTableRow[]>
|
||||
queryWindowsDescendants?: typeof queryWindowsProcessDescendants
|
||||
windowsDeadlineMs?: number
|
||||
posixDeadlineMs?: number
|
||||
}
|
||||
|
||||
/**
|
||||
@@ -42,6 +43,12 @@ const PTY_OWNERSHIP_PROBE_ATTEMPTS = 2
|
||||
*/
|
||||
const WINDOWS_OWNERSHIP_PROBE_DEADLINE_MS = 6_000
|
||||
|
||||
/**
|
||||
* POSIX needs its own ceiling for the same reason: the shared reader's `ps` timeout does not
|
||||
* cover queueing behind an in-flight scan, and this runs on a launch that fails open.
|
||||
*/
|
||||
const POSIX_OWNERSHIP_PROBE_DEADLINE_MS = 4_000
|
||||
|
||||
function withDeadline<T>(work: Promise<T>, deadlineMs: number, onDeadline: T): Promise<T> {
|
||||
return new Promise<T>((resolve) => {
|
||||
const timer = setTimeout(() => resolve(onDeadline), deadlineMs)
|
||||
@@ -156,10 +163,15 @@ async function probeOnce(
|
||||
)
|
||||
return hosted.length > 0 ? 'owns-live-ptys' : 'no-live-ptys'
|
||||
}
|
||||
const rows = await (deps.readPosixProcessTable ?? getFreshProcessTableSnapshot)()
|
||||
const rows = await withDeadline(
|
||||
(deps.readPosixProcessTable ?? getFreshProcessTableSnapshot)(),
|
||||
deps.posixDeadlineMs ?? POSIX_OWNERSHIP_PROBE_DEADLINE_MS,
|
||||
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.
|
||||
if (!rows.some((row) => row.pid === daemonPid)) {
|
||||
// never examined. Only a root we actually observed can prove emptiness — and a read
|
||||
// that blew its deadline saw nothing at all.
|
||||
if (rows === null || !rows.some((row) => row.pid === daemonPid)) {
|
||||
return 'unknown'
|
||||
}
|
||||
return collectLivePtyDescendants(rows, daemonPid).length > 0 ? 'owns-live-ptys' : 'no-live-ptys'
|
||||
|
||||
@@ -1027,7 +1027,9 @@ export function createIpcPtyTransport(opts: IpcPtyTransportOptions = {}): PtyTra
|
||||
inputWriteQueue.clear()
|
||||
if (ptyId) {
|
||||
const id = ptyId
|
||||
window.api.pty.kill(id)
|
||||
// Why swallow: an unreachable terminal host rejects rather than pretending the
|
||||
// session closed, and teardown must still run — but nothing here can act on it.
|
||||
void Promise.resolve(window.api.pty.kill(id)).catch(() => {})
|
||||
connected = false
|
||||
ptyId = null
|
||||
unregisterPtyHandlers(id)
|
||||
|
||||
@@ -3443,7 +3443,9 @@ export const createRepoSlice: StateCreator<AppState, [], [], RepoSlice> = (set,
|
||||
killedTabIds.add(tab.id)
|
||||
for (const ptyId of get().ptyIdsByTabId[tab.id] ?? []) {
|
||||
if (!ptyId.startsWith('remote:')) {
|
||||
window.api.pty.kill(ptyId)
|
||||
// Why swallow: an unreachable terminal host rejects instead of reporting a
|
||||
// close it did not perform; removal proceeds either way.
|
||||
void Promise.resolve(window.api.pty.kill(ptyId)).catch(() => {})
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user