diff --git a/config/scripts/daemon-replacement-launcher-hold-source-assertions.mjs b/config/scripts/daemon-replacement-launcher-hold-source-assertions.mjs new file mode 100644 index 00000000000..1f2206cf9f6 --- /dev/null +++ b/config/scripts/daemon-replacement-launcher-hold-source-assertions.mjs @@ -0,0 +1,182 @@ +#!/usr/bin/env node +/** + * Static source assertions for the launcher's hold decision. + * + * Split from the repro script it serves: those phases prove behaviour with real processes, + * while these read `daemon-init.ts` to pin the one property real processes cannot reach — + * that the decision is taken, and returns, before anything is killed. daemon-init.ts imports + * electron, so it cannot be executed outside the app. + */ +import { readFileSync } from 'node:fs' +import { join, resolve } from 'node:path' + +const repoRoot = resolve(import.meta.dirname, '..', '..') + +export function stripComments(source) { + // Blanked rather than deleted so offsets and line numbers stay true to the real file. + const blank = (text) => text.replace(/[^\n]/g, ' ') + return source + .replace(/\/\*[\s\S]*?\*\//g, blank) + .replace(/(^|[^:])(\/\/[^\n]*)/g, (_match, prefix, comment) => prefix + blank(comment)) +} + +/** The balanced `{...}` block starting at `braceIndex`, or null if it never closes. */ +function extractBlock(source, braceIndex) { + let depth = 0 + for (let i = braceIndex; i < source.length; i++) { + if (source[i] === '{') { + depth++ + } else if (source[i] === '}') { + depth-- + if (depth === 0) { + return { text: source.slice(braceIndex, i + 1), start: braceIndex, end: i + 1 } + } + } + } + return null +} + +function lineOf(source, index) { + return source.slice(0, index).split('\n').length +} + +function normalize(text) { + return text.replace(/\s+/g, ' ') +} + +/** + * PHASE 3 — the launcher must hold rather than kill. daemon-init.ts imports electron, so it + * cannot be executed here; this reads the source instead, whitespace-tolerantly, and asserts + * the structural properties phase 2's inputs depend on. + */ +export function checkLauncherHoldsOccupiedDaemon({ log, assert }) { + const relativePath = 'src/main/daemon/daemon-init.ts' + const source = stripComments(readFileSync(join(repoRoot, relativePath), 'utf8')) + + // 1. holdIncumbentDaemon() returns a preserved handle in 'held' mode — it does not adopt, + // which a daemon too wedged to answer listSessions could never complete anyway. + const holdDecl = source.match(/const\s+holdIncumbentDaemon\s*=\s*\([^)]*\)[^{]*\{/) + assert(holdDecl !== null, `${relativePath} does not declare holdIncumbentDaemon()`) + const holdBody = extractBlock(source, holdDecl.index + holdDecl[0].length - 1) + assert(holdBody !== null, `could not parse the holdIncumbentDaemon() body in ${relativePath}`) + assert( + /createPreservedDaemonHandle\([^)]*'held'\s*\)/.test(normalize(holdBody.text)), + `holdIncumbentDaemon() does not return createPreservedDaemonHandle(..., 'held'): ${normalize(holdBody.text)}` + ) + log( + `phase 3: ${relativePath}:${lineOf(source, holdDecl.index)} holdIncumbentDaemon() = ${normalize(holdBody.text)}` + ) + + // 2. Process-table evidence is only ever raised from an identity-verified pid — otherwise + // it could describe a recycled pid's children rather than this daemon's terminals. + const verifiedPidCall = source.search(/readVerifiedDaemonPid\s*\(/) + const evidenceCall = source.match(/raiseOccupancyWithProcessEvidence\s*\(([^)]*)\)/) + assert(verifiedPidCall !== -1, `${relativePath} never calls readVerifiedDaemonPid()`) + assert(evidenceCall !== null, `${relativePath} never raises occupancy with process evidence`) + assert( + verifiedPidCall < evidenceCall.index, + `${relativePath} raises occupancy with process evidence before verifying the recorded pid` + ) + // Whatever identifier carries the pid, its declaration must come from the verified read. + const evidencePidName = evidenceCall[1] + .split(',')[1] + ?.trim() + .replace(/[^\w$]/g, '') + assert( + Boolean(evidencePidName), + `could not read the pid argument of raiseOccupancyWithProcessEvidence: ${normalize(evidenceCall[1])}` + ) + const evidencePidDecl = new RegExp( + `const\\s+${evidencePidName}\\b[\\s\\S]{0,400}?readVerifiedDaemonPid\\s*\\(` + ) + assert( + evidencePidDecl.test(source), + `${relativePath} passes '${evidencePidName}' to raiseOccupancyWithProcessEvidence without deriving it from readVerifiedDaemonPid — the evidence could then describe a recycled pid's children` + ) + + // 3. The 'occupied' branch holds and never kills. + const occupiedGuard = source.match(/if\s*\(\s*occupancy\.state\s*===\s*'occupied'\s*\)\s*\{/) + assert(occupiedGuard !== null, `${relativePath} has no 'occupancy.state === occupied' guard`) + const occupiedBlock = extractBlock(source, occupiedGuard.index + occupiedGuard[0].length - 1) + assert(occupiedBlock !== null, `could not parse the occupied branch in ${relativePath}`) + const occupiedLine = lineOf(source, occupiedGuard.index) + assert( + !occupiedBlock.text.includes('killStaleDaemon'), + `${relativePath}:${occupiedLine} calls killStaleDaemon inside the occupied branch` + ) + + // Holding requires BOTH: no hello ever completed, and only the process table could answer. + // A daemon that did complete a hello is adoptable, so it must not be routed to a mode that + // never adopts. + const unverifiableGuard = occupiedBlock.text.match( + /if\s*\(\s*health\s*===\s*'rejected'\s*\|\|\s*occupancy\.liveSessions\s*===\s*null\s*\)\s*\{/ + ) + assert( + unverifiableGuard !== null, + `${relativePath}:${occupiedLine} does not gate the hold on a daemon that cannot be adopted (rejected, or an unverifiable session count)` + ) + const unverifiableBlock = extractBlock( + occupiedBlock.text, + unverifiableGuard.index + unverifiableGuard[0].length - 1 + ) + assert(unverifiableBlock !== null, 'could not parse the liveSessions === null branch') + assert( + normalize(unverifiableBlock.text).includes('return holdIncumbentDaemon()'), + `${relativePath}:${occupiedLine} does not return holdIncumbentDaemon() when the session count came from the process table` + ) + log( + `phase 3: ${relativePath}:${occupiedLine} occupancy.state === 'occupied' + cannot-be-adopted -> return holdIncumbentDaemon(); the branch contains no kill` + ) + + // 3b. The unknown-hold: the protection that no longer depends on any timing budget. An + // unclassifiable daemon is held, not replaced, except where holding is unrecoverable. + const unknownHold = source.match( + /if\s*\(\s*occupancy\.state === 'unknown' &&[\s\S]{0,1500}?return holdIncumbentDaemon\(\)/ + ) + assert( + unknownHold !== null, + `${relativePath} does not hold on occupancy.state === 'unknown' — a daemon we could not classify is being replaced` + ) + assert( + unknownHold[0].includes("health !== 'rejected'"), + `the unknown-hold does not exclude 'rejected', which can never be adopted: ${normalize(unknownHold[0])}` + ) + assert( + unknownHold[0].includes('endpointIsProvenDead'), + `the unknown-hold does not exclude a proven-dead endpoint, so a cold start would be held: ${normalize(unknownHold[0])}` + ) + log( + `phase 3: ${relativePath}:${lineOf(source, unknownHold.index)} occupancy.state === 'unknown' + not-proven-dead + not-rejected -> return holdIncumbentDaemon()` + ) + + // 4. Ordering: every kill on this path is downstream of the occupied branch, so a hold + // returns before any of them can run. + const killCalls = [...source.matchAll(/killStaleDaemon\s*\(/g)].map((match) => match.index) + assert(killCalls.length > 0, `${relativePath} never calls killStaleDaemon()`) + const killsBeforeTheDecision = killCalls.filter( + (index) => index > evidenceCall.index && index < occupiedBlock.end + ) + assert( + killsBeforeTheDecision.length === 0, + `${relativePath} kills at line(s) ${killsBeforeTheDecision.map((i) => lineOf(source, i)).join(', ')}, between resolving occupancy and the hold` + ) + const killsBeforeTheUnknownHold = killCalls.filter( + (index) => index > evidenceCall.index && index < unknownHold.index + ) + assert( + killsBeforeTheUnknownHold.length === 0, + `${relativePath} kills at line(s) ${killsBeforeTheUnknownHold.map((i) => lineOf(source, i)).join(', ')}, before the unknown-hold can return` + ) + const fallThroughKill = killCalls.find((index) => index > unknownHold.index) + assert( + fallThroughKill !== undefined, + `${relativePath} has no killStaleDaemon() after the occupied branch — the replacement path is gone` + ) + const killLines = killCalls.map((index) => lineOf(source, index)).join(', ') + log( + `phase 3: every killStaleDaemon() call site in the file is at line(s) ${killLines} — all downstream of the occupied branch, which returns at line ${lineOf(source, occupiedBlock.start)}` + ) + log( + 'phase 3 RESULT: statically, the failed-health-check path resolves occupancy from a verified pid and returns a held handle before any kill. This proves the source ordering and branch contents; it does NOT execute daemon-init.ts (it imports electron), so the runtime proof stops at the inputs phase 2 produced with real processes.' + ) +} diff --git a/config/scripts/daemon-replacement-live-agent-pty-preservation-repro.mjs b/config/scripts/daemon-replacement-live-agent-pty-preservation-repro.mjs new file mode 100644 index 00000000000..a135c8adc30 --- /dev/null +++ b/config/scripts/daemon-replacement-live-agent-pty-preservation-repro.mjs @@ -0,0 +1,553 @@ +#!/usr/bin/env node +/** + * Regression proof: daemon replacement must not kill live coding-agent terminals. + * + * The protection is no longer a veto inside `killStaleDaemon()` — that was policy + * buried in a mechanism. `killStaleDaemon()` is now purely "make this pid go away" + * and will happily kill a daemon that is hosting live agents. The decision moved + * up to the launcher, ahead of any kill: + * + * readVerifiedDaemonPid() -> which process, identity-verified, is the daemon + * resolveDaemonOccupancy() -> is it hosting work, and how sure are we + * daemon-init.ts -> 'occupied' with an unverifiable count => HOLD + * + * `resolveDaemonOccupancy()` asks the daemon over IPC first (a reply is + * authoritative both ways); only when it cannot answer does it consult the OS + * process table via `inspectDaemonPtyOwnership()`, and that evidence may only + * RAISE the answer to 'occupied' — it can never prove 'empty'. + * + * Three phases, real processes throughout: + * PHASE 1 (the danger is real): a SIGSTOPped daemon owning 2 live agent + * processes presents exactly the launcher's inputs — health 'unreachable', + * an endpoint that is NOT proven dead, no IPC session count. Calling + * killStaleDaemon() directly at that moment kills the daemon and both agents. + * This is what the decision is protecting against, not a bug in the kill. + * PHASE 2 (the decision protects it): same staging, fresh daemon and agents. + * readVerifiedDaemonPid() names the daemon, resolveDaemonOccupancy() returns + * { state: 'occupied', liveSessions: null } — IPC could not answer, the + * process table raised it to occupied — which is the exact input that makes + * the launcher hold. Nothing is signalled: daemon and agents are alive, and + * after SIGCONT the daemon is healthy and reports its 2 sessions again, so + * the wedge was transient and the preserved work was genuinely recoverable. + * PHASE 3 (the launcher actually holds): daemon-init.ts imports electron and + * cannot be executed here, so its failed-health-check branch is verified + * statically — the 'occupied' branch returns holdIncumbentDaemon() and + * contains no kill, and every killStaleDaemon() call sits after it. + * + * SIGSTOP is the faithful stand-in for the wedge: the socket still accepts + * connections while no RPC is ever answered — exactly the "busy machine can time + * out the health check on a live daemon" case daemon-init.ts calls out. + * + * Usage: node config/scripts/daemon-replacement-live-agent-pty-preservation-repro.mjs + */ +import { fork } from 'node:child_process' +import { randomUUID } from 'node:crypto' +import { existsSync, mkdirSync, mkdtempSync, rmSync, writeFileSync } from 'node:fs' +import { tmpdir } from 'node:os' +import { join, resolve } from 'node:path' +import { pathToFileURL } from 'node:url' +import { checkLauncherHoldsOccupiedDaemon } from './daemon-replacement-launcher-hold-source-assertions.mjs' +import { + findTaggedPid, + isMarkerAlive, + verifiedSessionLeaderPid, + isProcessAlive, + processArgs, + processState, + snapshotForeignDaemons, + waitFor +} from './daemon-replacement-process-inspection.mjs' + +const repoRoot = resolve(import.meta.dirname, '..', '..') +const entryPath = join(repoRoot, 'out', 'main', 'daemon-entry.js') +const READY_TIMEOUT_MS = 30_000 +const MARKER_SPAWN_TIMEOUT_MS = 30_000 +const SESSION_COUNT = 2 + +const startedAt = Date.now() +const timeline = [] + +function log(message) { + const elapsed = `+${String(Date.now() - startedAt).padStart(6, ' ')}ms` + timeline.push(`${elapsed} ${message}`) + process.stdout.write(`[daemon-pty-preservation] ${elapsed} ${message}\n`) +} + +function assert(condition, message) { + if (!condition) { + throw new Error(message) + } +} + +/** + * Bundles the real daemon primitives into a loadable ESM module. + * + * Why: the decision primitives live in TypeScript modules that the built + * daemon-entry.js does not re-export. Their import graph is electron-free, so + * esbuild can produce the genuine code — no reimplementation, no drift. + */ +async function loadDaemonPrimitives(scratch) { + const esbuild = await import('esbuild') + const entrySource = join(scratch, 'daemon-primitives-entry.ts') + const bundlePath = join(scratch, 'daemon-primitives.mjs') + const daemonDir = join(repoRoot, 'src', 'main', 'daemon') + writeFileSync( + entrySource, + [ + `export { checkDaemonHealth, killStaleDaemon, readVerifiedDaemonPid } from ${JSON.stringify(join(daemonDir, 'daemon-health'))}`, + `export { resolveDaemonOccupancy } from ${JSON.stringify(join(daemonDir, 'daemon-occupancy'))}`, + `export { endpointIsProvenDead, probeSocketConnect } from ${JSON.stringify(join(daemonDir, 'daemon-endpoint-probe'))}`, + `export { getDaemonPidPath, getDaemonSocketPath, getDaemonTokenPath } from ${JSON.stringify(join(daemonDir, 'daemon-spawner'))}`, + `export { DaemonClient } from ${JSON.stringify(join(daemonDir, 'client'))}`, + '' + ].join('\n') + ) + await esbuild.build({ + entryPoints: [entrySource], + outfile: bundlePath, + bundle: true, + platform: 'node', + format: 'esm', + packages: 'external', + logLevel: 'silent' + }) + return import(pathToFileURL(bundlePath).href) +} + +// Same shape as daemon-occupancy.ts countLiveSessionsOverIpc(): null means "could not answer". +async function countLiveSessionsOverIpc(DaemonClient, socketPath, tokenPath) { + const client = new DaemonClient({ socketPath, tokenPath }) + try { + await client.ensureConnected() + const result = await client.request('listSessions', undefined) + return result.sessions.filter((session) => session.isAlive).length + } catch { + return null + } finally { + client.disconnect() + } +} + +function forkDaemon({ runtimeDir, socketPath, tokenPath, pidPath, launchNonce, logFile }) { + // Argv and spawn options mirror daemon-init.ts createOutOfProcessLauncher(). + const child = fork( + entryPath, + [ + '--socket', + socketPath, + '--token', + tokenPath, + '--pid-record', + pidPath, + '--launch-nonce', + launchNonce, + '--entry-path', + entryPath, + '--app-version', + 'daemon-pty-preservation-repro', + '--spawner-exec-path', + process.execPath, + '--log-file', + logFile + ], + { + cwd: runtimeDir, + detached: true, + stdio: ['ignore', 'ignore', 'pipe', 'ipc'], + env: { + ...process.env, + ELECTRON_RUN_AS_NODE: '1', + ORCA_USER_DATA_PATH: runtimeDir + } + } + ) + let stderr = '' + child.stderr?.on('data', (chunk) => { + stderr += chunk.toString('utf8') + }) + const ready = new Promise((resolveReady, rejectReady) => { + const timer = setTimeout( + () => rejectReady(new Error(`daemon never signaled ready.\nstderr:\n${stderr}`)), + READY_TIMEOUT_MS + ) + child.on('message', (msg) => { + if (msg && typeof msg === 'object' && msg.type === 'ready') { + clearTimeout(timer) + resolveReady() + } + }) + child.on('exit', (code, signal) => { + clearTimeout(timer) + rejectReady(new Error(`daemon exited (code=${code}, signal=${signal}).\nstderr:\n${stderr}`)) + }) + }) + return { child, ready } +} + +async function startMarkerSession(client, phase, index, runtimeDir) { + const tag = `ORCA_LIVE_AGENT_MARKER_P${phase}_${index}_${randomUUID().replaceAll('-', '')}` + const sessionId = `repro-session-${phase}-${index}-${randomUUID()}` + // Long-lived and uniquely identifiable: stands in for a running coding agent. + const command = `exec /bin/sh -c 'while :; do sleep 1; done' ${tag}` + const result = await client.request('createOrAttach', { + sessionId, + cols: 80, + rows: 24, + cwd: runtimeDir, + command, + shellReadySupported: false + }) + if (!Number.isInteger(result.pid) || result.pid <= 0) { + throw new Error(`session ${index} reported no pid: ${JSON.stringify(result)}`) + } + let markerPid = null + await waitFor( + () => (markerPid = findTaggedPid(tag)) !== null, + `agent marker ${index} to start`, + MARKER_SPAWN_TIMEOUT_MS + ) + return { tag, sessionId, pid: markerPid, sessionPid: result.pid } +} + +/** + * Stands up a real daemon with real agent processes, wedges it with SIGSTOP, and replays + * the launcher's decision inputs against it — the state both phases start from. + */ +async function stageWedgedDaemon({ primitives, scratch, phase, registry }) { + const { DaemonClient, checkDaemonHealth, endpointIsProvenDead, probeSocketConnect } = primitives + const runtimeDir = join(scratch, `daemon-phase-${phase}`) + mkdirSync(runtimeDir, { recursive: true }) + const socketPath = primitives.getDaemonSocketPath(runtimeDir) + const tokenPath = primitives.getDaemonTokenPath(runtimeDir) + const pidPath = primitives.getDaemonPidPath(runtimeDir) + log(`phase ${phase}: runtime dir ${runtimeDir} (real userData is untouched)`) + + const daemon = forkDaemon({ + runtimeDir, + socketPath, + tokenPath, + pidPath, + launchNonce: randomUUID(), + logFile: join(scratch, `daemon-phase-${phase}.log`) + }) + const staged = { daemon, markers: [], stopped: false, runtimeDir, socketPath, tokenPath, pidPath } + // Registered before the first await so a mid-staging failure still tears it down. + registry.push(staged) + await daemon.ready + log(`phase ${phase}: daemon ready, pid ${daemon.child.pid}`) + + const client = new DaemonClient({ socketPath, tokenPath }) + await client.ensureConnected() + for (let index = 0; index < SESSION_COUNT; index++) { + staged.markers.push(await startMarkerSession(client, phase, index, runtimeDir)) + } + const liveBefore = await countLiveSessionsOverIpc(DaemonClient, socketPath, tokenPath) + client.disconnect() + for (const marker of staged.markers) { + log( + `phase ${phase}: live agent process pid ${marker.pid} (PTY session leader ${marker.sessionPid}): ${processArgs(marker.pid)}` + ) + } + assert(staged.markers.every(isMarkerAlive), 'agent markers were not alive before the wedge') + log( + `phase ${phase}: ps confirms ${staged.markers.length} live agent processes; daemon reports ${liveBefore} alive` + ) + + process.kill(daemon.child.pid, 'SIGSTOP') + staged.stopped = true + log(`phase ${phase}: SIGSTOP -> daemon ${daemon.child.pid} is ALIVE but cannot service RPCs`) + assert(staged.markers.every(isMarkerAlive), 'the wedge itself killed the agent markers') + log(`phase ${phase}: agent processes unaffected by the wedge — only the daemon is unresponsive`) + + // The launcher's own inputs on the failed-health-check path, via the real primitives. + const health = await checkDaemonHealth(socketPath, tokenPath) + log(`phase ${phase}: checkDaemonHealth() = '${health}' — daemon-init.ts takes the else branch`) + assert(health === 'unreachable', `expected health 'unreachable', got '${health}'`) + + const probe = await probeSocketConnect(socketPath) + log( + `phase ${phase}: probeSocketConnect() = '${probe}', endpointIsProvenDead() = ${endpointIsProvenDead(probe)} — nothing proves the daemon is gone` + ) + assert( + !endpointIsProvenDead(probe), + `the wedged daemon's endpoint was proven dead ('${probe}'); not the modeled failure` + ) + + const ipcCount = await countLiveSessionsOverIpc(DaemonClient, socketPath, tokenPath) + log( + `phase ${phase}: live session count over IPC = ${ipcCount} (null = the daemon could not answer)` + ) + assert(ipcCount === null, 'the wedged daemon answered listSessions; wedge not severe enough') + + return staged +} + +/** + * PHASE 1 — what the decision is protecting against. killStaleDaemon() is now a pure + * mechanism with no opinion about live work, so called at this exact moment it takes the + * daemon and every agent PTY with it. + */ +async function runUnprotectedKillPhase(primitives, scratch, registry) { + const staged = await stageWedgedDaemon({ primitives, scratch, phase: 1, registry }) + log( + 'phase 1: invoking the real killStaleDaemon(runtimeDir, socket, token) directly — no occupancy consulted' + ) + const killOutcome = await primitives.killStaleDaemon( + staged.runtimeDir, + staged.socketPath, + staged.tokenPath + ) + log(`phase 1: killStaleDaemon() = ${JSON.stringify(killOutcome)}`) + staged.stopped = false + assert(killOutcome.killed === true, 'killStaleDaemon() did not kill the wedged daemon') + assert(!isProcessAlive(staged.daemon.child.pid), 'killStaleDaemon() left the daemon alive') + + await waitFor( + () => staged.markers.every((marker) => !isMarkerAlive(marker)), + 'agent processes to die with the killed daemon', + 10_000 + ) + for (const marker of staged.markers) { + log( + `phase 1: agent PTY pid ${marker.pid} is GONE (ps: ${processArgs(marker.pid) ?? 'no such process'})` + ) + } + log( + 'phase 1 RESULT: the danger is real — killStaleDaemon() on a wedged-but-live daemon ends the daemon and every agent with it. There is no fd handoff; only a decision taken BEFORE the kill can save them.' + ) + return staged +} + +/** + * PHASE 2 — the decision the launcher takes instead. Identical staging, but the inputs are + * resolved rather than acted on: readVerifiedDaemonPid() names the process and + * resolveDaemonOccupancy() raises it to 'occupied' from the process table. + */ +async function runOccupancyDecisionPhase(primitives, scratch, registry) { + const staged = await stageWedgedDaemon({ primitives, scratch, phase: 2, registry }) + const daemonPid = staged.daemon.child.pid + + const verifiedPid = await primitives.readVerifiedDaemonPid( + staged.runtimeDir, + staged.socketPath, + staged.tokenPath + ) + log( + `phase 2: readVerifiedDaemonPid() = ${verifiedPid ? `pid ${verifiedPid.pid} (identity verified: cmdline + start time)` : 'null'}` + ) + assert( + verifiedPid?.pid === daemonPid, + `readVerifiedDaemonPid() returned ${JSON.stringify(verifiedPid)}, expected pid ${daemonPid}` + ) + + // Which input answered is readable from the result alone: resolveDaemonOccupancy only ever + // returns a null count when IPC failed and inspectDaemonPtyOwnership() — the OS process + // table, never the socket the daemon already failed to answer — reported 'owns-live-ptys'. + let occupancy = await primitives.resolveDaemonOccupancy({ + socketPath: staged.socketPath, + tokenPath: staged.tokenPath, + recordedPid: verifiedPid.pid + }) + log(`phase 2: resolveDaemonOccupancy() = ${JSON.stringify(occupancy)}`) + // The launcher's grace loop, replayed verbatim: it only re-samples while 'unknown'. + let graceRetry = 0 + while ( + occupancy.state === 'unknown' && + graceRetry < 1 && + !primitives.endpointIsProvenDead(await primitives.probeSocketConnect(staged.socketPath)) + ) { + occupancy = await primitives.resolveDaemonOccupancy({ + socketPath: staged.socketPath, + tokenPath: staged.tokenPath, + recordedPid: verifiedPid.pid + }) + graceRetry++ + } + log( + `phase 2: the launcher makes one patient ask and no retries — what remains after the patient connect is always exactly the request budget, which cannot fund another (ran ${graceRetry})` + ) + assert( + occupancy.state === 'occupied' && occupancy.liveSessions === null, + `expected {state:'occupied',liveSessions:null}, got ${JSON.stringify(occupancy)}` + ) + log( + "phase 2: occupancy is 'occupied' with liveSessions null — IPC could not answer, so the count came from the process table. That exact pair is what makes the launcher hold instead of kill (phase 3)." + ) + + assert(isProcessAlive(daemonPid), 'the daemon died while occupancy was being resolved') + log( + `phase 2: daemon ${daemonPid} is STILL ALIVE (ps stat '${processState(daemonPid)}' — T = stopped, not killed); resolving occupancy signals nothing` + ) + assert(existsSync(staged.pidPath), 'the surviving daemon lost its PID record') + log('phase 2: PID record left intact — no replacement can publish ownership beside it') + + for (const marker of staged.markers) { + assert(isMarkerAlive(marker), `agent PTY pid ${marker.pid} died during the decision`) + log(`phase 2: agent PTY pid ${marker.pid} is ALIVE (ps: ${processArgs(marker.pid)})`) + } + + // Why SIGCONT: a SIGTERM sent to a stopped process stays pending and lands on + // resume. Surviving the resume is the proof that no signal was even queued. + process.kill(daemonPid, 'SIGCONT') + staged.stopped = false + await new Promise((r) => setTimeout(r, 1_000)) + assert(isProcessAlive(daemonPid), 'the daemon died on SIGCONT — a SIGTERM had been queued for it') + log('phase 2: after SIGCONT the daemon is still running — no signal was ever delivered to it') + + const resumedHealth = await primitives.checkDaemonHealth(staged.socketPath, staged.tokenPath) + const resumedSessions = await countLiveSessionsOverIpc( + primitives.DaemonClient, + staged.socketPath, + staged.tokenPath + ) + log( + `phase 2: resumed daemon reports checkDaemonHealth() = '${resumedHealth}', live sessions over IPC = ${resumedSessions}` + ) + assert(resumedHealth === 'healthy', `resumed daemon is not healthy: '${resumedHealth}'`) + assert(resumedSessions === SESSION_COUNT, `resumed daemon lost sessions: ${resumedSessions}`) + for (const marker of staged.markers) { + assert(isMarkerAlive(marker), `agent PTY pid ${marker.pid} died during resume`) + } + const resumedOccupancy = await primitives.resolveDaemonOccupancy({ + socketPath: staged.socketPath, + tokenPath: staged.tokenPath, + recordedPid: verifiedPid.pid + }) + log( + `phase 2: resolveDaemonOccupancy() on the recovered daemon = ${JSON.stringify(resumedOccupancy)} — the count is authoritative again now that IPC answers` + ) + assert( + resumedOccupancy.state === 'occupied' && resumedOccupancy.liveSessions === SESSION_COUNT, + `expected {state:'occupied',liveSessions:${SESSION_COUNT}} after recovery, got ${JSON.stringify(resumedOccupancy)}` + ) + log( + 'phase 2 RESULT: the wedge was transient and the work was genuinely recoverable — the daemon and both agents survived, then came back healthy with all sessions intact' + ) + return staged +} + +function teardown(staged) { + if (!staged) { + return + } + // Why the exit check: phase 1 kills this daemon on purpose, and once Node has reaped the + // child its pid is free for the OS to reuse. Signalling the remembered number after that is + // signalling a stranger. + const daemonChild = staged.daemon?.child + const daemonPid = + daemonChild && daemonChild.exitCode === null && daemonChild.signalCode === null + ? daemonChild.pid + : undefined + if (daemonPid) { + for (const signal of staged.stopped ? ['SIGCONT', 'SIGKILL'] : ['SIGKILL']) { + try { + process.kill(daemonPid, signal) + } catch { + // already gone + } + } + staged.daemon.child.stderr?.destroy() + if (staged.daemon.child.connected) { + staged.daemon.child.disconnect() + } + 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 + } + // The leader is re-read from the live marker rather than remembered: the marker proves its + // own identity by tag, but nothing proved the leader's, and it is the one pid here that + // could have been recycled while its child stayed alive under a new parent. + for (const pid of [marker.pid, verifiedSessionLeaderPid(marker)]) { + if (!pid) { + continue + } + try { + process.kill(pid, 'SIGKILL') + } catch { + // already gone + } + } + } +} + +async function main() { + if (process.platform === 'win32') { + log('SKIP: SIGSTOP is POSIX-only, so a live-but-unresponsive daemon cannot be staged here') + return + } + if (!existsSync(entryPath)) { + throw new Error(`missing ${entryPath} — run \`pnpm run build:electron-vite\` first`) + } + + const scratch = mkdtempSync(join(tmpdir(), 'orca-dpp-')) + const foreignDaemons = snapshotForeignDaemons() + const staged = [] + let verdict = 'FAIL' + + try { + log( + `pre-existing daemons that must survive this run: ${foreignDaemons + .map((d) => `${d.pid}${d.isRealUserDaemon ? ' (real userData daemon)' : ''}`) + .join(', ')}` + ) + const primitives = await loadDaemonPrimitives(scratch) + + log('=== PHASE 1: the danger is real — killStaleDaemon() has no opinion about live work ===') + await runUnprotectedKillPhase(primitives, scratch, staged) + + log('=== PHASE 2: the decision protects it — resolveDaemonOccupancy() on the same wedge ===') + await runOccupancyDecisionPhase(primitives, scratch, staged) + + log('=== PHASE 3: does the launcher actually hold on that verdict? ===') + checkLauncherHoldsOccupiedDaemon({ log, assert }) + + verdict = 'PASS' + } finally { + for (const phase of staged) { + teardown(phase) + } + rmSync(scratch, { recursive: true, force: true }) + + const survivors = foreignDaemons.filter((d) => isProcessAlive(d.pid)) + // Why only the real userData daemon is fatal: orphaned test daemons idle-shut-down or + // death-watch out on their own schedule, so their exit during a 90s run proves nothing. + const realUserDaemons = foreignDaemons.filter((d) => d.isRealUserDaemon) + const harmedRealDaemons = realUserDaemons.filter((d) => !isProcessAlive(d.pid)) + const departed = foreignDaemons.filter((d) => !isProcessAlive(d.pid) && !d.isRealUserDaemon) + const departedNote = + departed.length > 0 + ? ` (unrelated daemons that exited on their own: ${departed.map((d) => d.pid).join(', ')})` + : '' + log( + `cleanup done; pre-existing daemons still running: ${survivors.map((d) => d.pid).join(', ') || 'none'}${departedNote}` + ) + log( + harmedRealDaemons.length > 0 + ? `THE REAL userData DAEMON WAS HARMED: ${harmedRealDaemons.map((d) => d.pid).join(', ')}` + : `real userData daemon untouched: ${realUserDaemons.map((d) => d.pid).join(', ') || 'none running'}` + ) + if (harmedRealDaemons.length > 0) { + verdict = 'FAIL' + } + + process.stdout.write( + `\n[daemon-pty-preservation] TIMELINE\n${timeline.map((line) => ` ${line}`).join('\n')}\n` + ) + process.stdout.write( + verdict === 'PASS' + ? '\n[daemon-pty-preservation] PASS: killStaleDaemon() on a wedged daemon still kills it and every agent PTY with it (phase 1); against the identical wedge resolveDaemonOccupancy() returns { occupied, liveSessions: null } from the process table with the daemon unsignalled, both agents alive, and the daemon recovering healthy with all sessions on SIGCONT (phase 2); and daemon-init.ts returns holdIncumbentDaemon() on that verdict, before any kill (phase 3, static).\n' + : '\n[daemon-pty-preservation] FAIL: live agent PTYs are NOT protected — see the ERROR line and the timeline above.\n' + ) + process.exitCode = verdict === 'PASS' ? 0 : 1 + } +} + +main().catch((error) => { + process.stderr.write(`[daemon-pty-preservation] ERROR: ${error.stack ?? error.message}\n`) + process.exitCode = 1 +}) diff --git a/config/scripts/daemon-replacement-process-inspection.mjs b/config/scripts/daemon-replacement-process-inspection.mjs new file mode 100644 index 00000000000..fed5c49f7ab --- /dev/null +++ b/config/scripts/daemon-replacement-process-inspection.mjs @@ -0,0 +1,136 @@ +#!/usr/bin/env node +/** + * Process-table helpers for the daemon PTY preservation repro: what is alive, what a pid is + * running, and which daemons were already here before the run. Split out so the repro script + * itself stays about the sequence it proves rather than the plumbing it proves it with. + */ +import { execFileSync } from 'node:child_process' + +// Electron's userData path differs per platform, and hardcoding the macOS one meant a real +// daemon could never be recognised on Linux — so the guard that this run harmed nothing was +// inert on exactly the platform where it would go unnoticed. +const REAL_USER_DAEMON_MARKERS = { + darwin: ['Library/Application Support/orca/daemon'], + linux: ['.config/orca/daemon'], + win32: ['AppData/Roaming/orca/daemon', 'AppData\\Roaming\\orca\\daemon'] +} +const REAL_USER_DAEMON_MARKER_LIST = REAL_USER_DAEMON_MARKERS[process.platform] ?? ['orca/daemon'] + +export function processArgs(pid) { + try { + return execFileSync('ps', ['-p', String(pid), '-o', 'args='], { + encoding: 'utf8', + timeout: 5_000 + }).trim() + } catch { + return null + } +} + +export function processState(pid) { + try { + return execFileSync('ps', ['-p', String(pid), '-o', 'stat='], { + encoding: 'utf8', + timeout: 5_000 + }).trim() + } catch { + return null + } +} + +export function isProcessAlive(pid) { + try { + process.kill(pid, 0) + return true + } catch (error) { + return error?.code !== 'ESRCH' + } +} + +// Why scan by tag rather than trust the session pid: macOS wraps the PTY in +// /usr/bin/login for TCC attribution, so the agent process is a descendant of +// the session leader — exactly as a real `claude`/`codex` launch would be. +export function findTaggedPid(tag) { + try { + const output = execFileSync('ps', ['-eo', 'pid=,args='], { + encoding: 'utf8', + timeout: 5_000 + }) + for (const line of output.split('\n')) { + if (line.includes(tag)) { + const pid = Number(line.trim().split(/\s+/, 1)[0]) + if (Number.isInteger(pid) && pid > 0) { + return pid + } + } + } + } catch { + // ps failed; treat as not found. + } + return null +} + +export function isMarkerAlive(marker) { + return processArgs(marker.pid)?.includes(marker.tag) === true +} + +/** + * The session leader of a still-live marker, read now rather than remembered. + * + * Why not trust the pid captured at staging: teardown runs a minute later, and phase 1 has + * deliberately killed things in between. A remembered leader pid may by then belong to whatever + * the OS recycled it onto, and SIGKILLing that is precisely the mistake this script exists to + * demonstrate. Returns null unless the live marker still claims this leader. + */ +export function verifiedSessionLeaderPid(marker) { + if (!isMarkerAlive(marker)) { + return null + } + try { + const ppid = Number( + execFileSync('ps', ['-p', String(marker.pid), '-o', 'ppid='], { + encoding: 'utf8', + timeout: 5_000 + }).trim() + ) + return Number.isInteger(ppid) && ppid === marker.sessionPid ? ppid : null + } catch { + return null + } +} + +// Pre-existing daemons (the user's real one above all) must be untouched by this run. +export function snapshotForeignDaemons() { + const daemons = [] + try { + const output = execFileSync('ps', ['-eo', 'pid=,args='], { encoding: 'utf8', timeout: 5_000 }) + for (const line of output.split('\n')) { + if (!line.includes('daemon-entry.js')) { + continue + } + const pid = Number(line.trim().split(/\s+/, 1)[0]) + if (Number.isInteger(pid) && pid > 0) { + daemons.push({ + pid, + isRealUserDaemon: REAL_USER_DAEMON_MARKER_LIST.some((marker) => line.includes(marker)) + }) + } + } + } catch { + // ps failed; the exit check will report an empty snapshot. + } + return daemons +} + +export async function waitFor(predicate, description, timeoutMs) { + const deadline = Date.now() + timeoutMs + for (;;) { + if (await predicate()) { + return + } + if (Date.now() >= deadline) { + throw new Error(`timed out waiting for ${description}`) + } + await new Promise((r) => setTimeout(r, 200)) + } +} diff --git a/package.json b/package.json index 2f2954020df..6f7be4e6f4b 100644 --- a/package.json +++ b/package.json @@ -26,6 +26,7 @@ "prepare": "husky", "test": "node config/scripts/ensure-native-runtime.mjs --runtime=node && vitest run --config config/vitest.config.ts", "test:repro:remote-agent-session": "pnpm run build:cli && pnpm run build:electron-vite && node config/scripts/remote-agent-session-authority-repro.mjs", + "test:repro:daemon-replacement-live-agent-pty-preservation": "pnpm run build:electron-vite && node config/scripts/daemon-replacement-live-agent-pty-preservation-repro.mjs", "check:reliability-gates": "node config/scripts/check-reliability-gates.mjs", "check:max-lines-ratchet": "node config/scripts/check-max-lines-ratchet.mjs", "check:feature-wall-assets": "node config/scripts/check-feature-wall-assets.mjs", diff --git a/src/main/ai-vault/session-scanner-opencode-sources-wsl-stall.test.ts b/src/main/ai-vault/session-scanner-opencode-sources-wsl-stall.test.ts index 297f4b78b06..accc60d008f 100644 --- a/src/main/ai-vault/session-scanner-opencode-sources-wsl-stall.test.ts +++ b/src/main/ai-vault/session-scanner-opencode-sources-wsl-stall.test.ts @@ -94,9 +94,7 @@ describe('OpenCode source discovery with a stalled WSL data directory', () => { // silent [] here reads as "no OpenCode sessions" on a clean scan. await expect(discoveries).resolves.toHaveLength(1) expect( - issues.some( - (issue) => issue.agent === 'opencode' && issue.path === `${WSL_HOME}/opencode` - ) + issues.some((issue) => issue.agent === 'opencode' && issue.path === `${WSL_HOME}/opencode`) ).toBe(true) } finally { restoreEnv('XDG_DATA_HOME', previousXdg) diff --git a/src/main/daemon/AGENTS.md b/src/main/daemon/AGENTS.md index 8e1dfa6aaea..84b6e7fa20a 100644 --- a/src/main/daemon/AGENTS.md +++ b/src/main/daemon/AGENTS.md @@ -26,6 +26,139 @@ hands → probe once more → `rename` in one syscall → verify we kept it. - **Never collapse "can't tell" into "dead."** Only `connected` means occupied; only `refused`/`missing` prove death. A timeout or `EPERM` proves nothing and must decline — treating it as death deletes an endpoint still serving every terminal on the host. +- **"Can't tell" does not license a kill at launch either.** When the launcher cannot establish + what a health-check-failing daemon is hosting, it holds it in degraded mode rather than + replacing it. Only the daemon itself can prove it is empty, over IPC; the process table may + only ever *raise* a verdict toward "occupied", never lower one toward a kill. + + Two exclusions apply **to that residual only** — not to a daemon already proven occupied: + an endpoint that is proven dead (a cold start has nothing to hold), and `rejected` (it + answered and refused, so it can never be adopted and its sessions can never be reattached). + + A `rejected` daemon that process evidence shows *is* hosting live PTYs is still held, because + the choice there is between unreachable-but-running agents and dead ones. Restart recovers it + at the documented cost. + + The cost is deliberate and known: a wedged-but-empty daemon is no longer replaced at launch, + so #8689 degrades to "restart it from Manage Sessions" instead of being handled automatically. + And an endpoint held by something that accepts connections but never speaks the protocol — a + foreign process, or our own permanently wedged daemon — reads as an incumbent on *every* + launch, so it stays degraded with no auto-recovery. `killStaleDaemon` only kills a process + whose identity matches the pid record, so Restart cannot clear that one; the degraded message + says so and points at quit-and-relaunch. + + Two pieces exist only to keep that cost from growing, and both have been proposed for deletion + on the reasoning that "'unknown' and 'occupied' now behave the same". They do not. The process- + table evidence read is what holds a daemon whose socket entry vanished while it still hosts + agents — the occupied branch has no proven-dead check and the unknown hold does. And the + grace-retry loop is worth *more* since the hold landed, because a counted `occupied` reaches + full adoption where the alternative is a degraded hold. + That was chosen over the alternative, which was killing daemons whose live agents we had + merely failed to observe — unrecoverable, versus one click. + + Three paths still reach a kill, and each is a residual rather than a guarantee. Adversarial + review named all three; none is a regression against the pre-hold behaviour, and none should + be closed by weakening the rules above. + + - **`unknown` + a proven-dead endpoint, when process evidence is unavailable.** The endpoint + probe proves the *entry* is gone, not the *process*; a socket entry can vanish while the + daemon still hosts agents. Evidence covers that on POSIX — it runs for any `unknown`, not + only a live endpoint — so the gap is where evidence cannot answer: the clock is spent, the + pid will not verify, or `ps` is blind. Not reachable on Windows, where a named pipe vanishes + with its process, so a dead endpoint there implies a dead daemon and no agents to lose. + - **`unknown` + `rejected`, when evidence is unavailable.** "Cannot be adopted" is not the + same as "cannot be preserved": its agents keep running even though nothing can ever reattach + to them. Killing is chosen deliberately, because a daemon that can never be adopted and is + never replaced leaves the app permanently degraded with no route back. Reconsider only with + a way for the user to choose. + - **TOCTOU between the verdict and the kill.** The right fix is known and was implemented and + reverted once, deliberately: ask the daemon to retire itself via the existing `shutdownIfIdle` + RPC immediately before the kill, and treat only its own `{retiring: true}` as permission. + The daemon answers that atomically — sole authenticated client, nothing being created or + attached, zero sessions — and closes its listener before acknowledging, so nothing can slip + in behind the proof. A second `listSessions` would only move the race. + + It was reverted because it makes every empty-verdict replacement depend on a new round trip, + and any failure of that round trip must mean hold — which turns a rare race into a new, + common failure mode, and worsens #8689 whenever the call is merely slow. It also changed the + behaviour of two endpoint-identity tests in ways that were not quickly explainable. Land it + on a green base with its own review, not as an addendum. + An `empty` answer can go stale — another Orca + instance may create a session before the ladder runs — and a dead endpoint can be + republished. Nothing revalidates immediately before the kill, and `liveOwnerSurvived` is + 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. + + The owner check covers the operations that can destroy or corrupt a session — write, resize, + shutdown, sendSignal, attach. It does **not** cover `pauseProducer`, `resumeProducer`, + `setPtyBackgrounded`, `clearBuffer`, `closeStartupQueryAuthority`, `acknowledgeDataEvent`, or + the per-session queries, which still route raw. For an unresolved daemon id those reach the + fallback silently: a buffer clear reports success while the daemon's history survives, and + flow control paces a producer that is not the one emitting. Pre-existing and unchanged here. + + Before extending the check to them, note that `acknowledgeDataEvent` is called straight from + an `ipcMain.on` listener and `setPtyBackgrounded` synchronously from a callback, neither with + a boundary — so adding a throwing owner check without changing those call sites converts a + silent misroute into an escaping exception. + + **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 + kill ladder (~11.5s) and the fork (~10s) still have to fit before the 60s fail-open. At 34s + that case cannot arise (34 + 21.5 = 55.5). Raise the budget and it can, and an overrun there + is the worst branch available: daemon killed, replacement forked and then discarded, no + provider installed, Restart broken. The guard is to hold instead of replacing when the + remaining headroom cannot fund the ladder and the fork — safe precisely because that path has + proven the daemon empty, so holding costs no agents. Use `holdIncumbentDaemon()`, not + `preserveDaemon()`, which opens a non-shared 20s handshake and could overrun the deadline it + is meant to respect. + + Why it is unreachable at 34s is structure, not margin, and the distinction is the point: the + hold decoupled long classification from the replace path. A verdict of `empty` means the + daemon *answered*, so it resolved fast by construction; `unknown` + proven-dead means nothing + is listening, so the probe settles in ~500ms and the ladder short-circuits on ESRCH. The path + that actually consumes the budget — a wedge that never answers — now ends in a hold, which + pays neither the ladder nor the fork. The long path and the expensive tail are disjoint. + Raising the budget is what re-couples them, by extending how late an `empty` may legally + arrive (~22s in at 34s; ~32s in at 44s). The raise creates the case; it does not merely + expose it. + + Costing the guard honestly: the launcher closure does not receive the startup abort signal, + but `createOutOfProcessLauncher` is a factory called from inside `initDaemonPtyProvider`, + where `signal` is in scope. A third factory parameter closed over there leaves + `DaemonLauncher`'s call signature — all `DaemonSpawner` knows about — unchanged. One + parameter, not a spawner change. Record alongside it that a closed-over startup signal is + meaningful only for the startup launch: `runRestartDaemon` reuses the same spawner and the + `respawn` closure re-enters the same launcher, and both would read a signal that never + aborts, because `servicesSettled` clears the fail-open timer once init succeeds. That is + correct — later restarts are not under the startup gate — but it reads like a bug without + the sentence. + + Two things erode that margin rather than consume it, and neither is bounded by this budget: + the `health === 'healthy'` branch never consults `classificationRemainingMs()` at all + (`resolveOccupancyOverIpc` passes no `budgetMs`, so it takes the 19s default) and also ends + in a cleanup and a fork; and packaged Windows follows the fork with a daemon-host directory + copy of unbounded size. Both stay under today only because reaching them requires a verdict + that arrives early. + + **Do not try to fix this by tuning the classification budget.** Ten review rounds each found a + different timing band where a bounded classification kills a session an unbounded one keeps. + Matching the old tolerance for a single probe costs more clock than the 60s startup fail-open + leaves once the kill ladder and the fork are paid for. The budget is a latency bound, not a + correctness parameter, and it must stay that way. - **`link` first, never an unconditional `rename`.** `rename` replaces whatever it finds, so it would let a starting daemon destroy a healthy one. `link` fails loudly and forces the liveness question. diff --git a/src/main/daemon/daemon-errors.test.ts b/src/main/daemon/daemon-errors.test.ts index 71374415772..6cb73083bca 100644 --- a/src/main/daemon/daemon-errors.test.ts +++ b/src/main/daemon/daemon-errors.test.ts @@ -4,8 +4,10 @@ import { decodeDaemonResponseError, isDaemonEndpointGoneError, SessionNotFoundError, - TerminalHostGoneError + TerminalHostGoneError, + TerminalSessionOwnerUnverifiedError } from './daemon-errors' +import { isPtyWriteUnavailableError } from '../providers/pty-write-unavailable-error' import { mapRuntimeError } from '../runtime/rpc/errors' function socketError(code: string, syscall: string): Error & { code: string; syscall: string } { @@ -66,3 +68,27 @@ describe('isDaemonEndpointGoneError', () => { expect(response.error).toEqual({ code: 'runtime_error', message: 'terminal_host_gone' }) }) }) + +describe('TerminalSessionOwnerUnverifiedError classification', () => { + const error = new TerminalSessionOwnerUnverifiedError('pty-1') + + it('reads as an unavailable write, so a throw mid-paste reaches the renderer', () => { + // Without this the remaining chunks are dropped with no pty:writeUnavailable, and the pane + // never re-attaches — a silent truncation the user has no way to attribute. + expect(isPtyWriteUnavailableError(error)).toBe(true) + }) + + it('does not read as an already-gone session', () => { + // The reason this is not a SessionNotFoundError: pty.ts's isPtyAlreadyGoneError matches + // /Session not found/i and synthesizes an exit, which would report a session as dead + // precisely when we could not establish that it was. Matching that predicate's shape here + // rather than importing it, because it is private to the IPC layer. + expect(/Session not found/i.test(error.message)).toBe(false) + expect(/Session not found/i.test(new SessionNotFoundError('pty-1').message)).toBe(true) + }) + + it('keeps its own identity for callers that match on it', () => { + expect(error).toBeInstanceOf(TerminalSessionOwnerUnverifiedError) + expect(error.name).toBe('TerminalSessionOwnerUnverifiedError') + }) +}) diff --git a/src/main/daemon/daemon-errors.ts b/src/main/daemon/daemon-errors.ts index d5b7dc2418e..bf324d7f5e9 100644 --- a/src/main/daemon/daemon-errors.ts +++ b/src/main/daemon/daemon-errors.ts @@ -1,3 +1,4 @@ +import { PtyWriteUnavailableError } from '../providers/pty-write-unavailable-error' // Error classes shared across the daemon protocol boundary (client, server, // host). Split from types.ts, which is capped for wire-shape declarations. export class TerminalAttachCanceledError extends Error { @@ -21,7 +22,15 @@ export class SessionNotFoundError extends Error { } } -export class TerminalSessionOwnerUnverifiedError extends Error { +/** + * A PtyWriteUnavailableError so a throw partway through a paste reaches the renderer as + * `pty:writeUnavailable` and the pane re-attaches, instead of the remaining chunks vanishing + * with nothing to explain the gap. + * + * Deliberately not a SessionNotFoundError: that is matched by isPtyAlreadyGoneError and would + * be synthesized into an exit the session never had — the same lie one layer down. + */ +export class TerminalSessionOwnerUnverifiedError extends PtyWriteUnavailableError { constructor(sessionId: string) { super(`Terminal session owner could not be verified: ${sessionId}`) this.name = 'TerminalSessionOwnerUnverifiedError' diff --git a/src/main/daemon/daemon-health.test.ts b/src/main/daemon/daemon-health.test.ts index 7b83b09a9b3..8c220db0d19 100644 --- a/src/main/daemon/daemon-health.test.ts +++ b/src/main/daemon/daemon-health.test.ts @@ -23,6 +23,8 @@ import { } from './daemon-health' import type { SubprocessHandle } from './session' +// Why: the veto's production default is otherwise never exercised — every other test injects it. + function createMockSubprocess(): SubprocessHandle { return { pid: 55555, diff --git a/src/main/daemon/daemon-health.ts b/src/main/daemon/daemon-health.ts index 4efded40d98..1a6e49d42c1 100644 --- a/src/main/daemon/daemon-health.ts +++ b/src/main/daemon/daemon-health.ts @@ -28,7 +28,9 @@ import { type SystemResolverHealthResult } from './types' -const HEALTH_CHECK_TIMEOUT_MS = 3_000 +export const HEALTH_CHECK_TIMEOUT_MS = 3_000 +/** Ceiling on one identity `ps`; the launch budget reserves against it, so it is exported rather than inline. */ +export const PS_IDENTITY_TIMEOUT_MS = 2_000 const RESOLVER_HEALTH_CHECK_TIMEOUT_MS = 3_000 const KILL_WAIT_MS = 3_000 const KILL_POLL_MS = 100 @@ -442,7 +444,7 @@ function getPsProcessIdentity(pid: number): PsProcessIdentity | null { try { const output = execFileSync('ps', ['-p', String(pid), '-o', 'lstart=', '-o', 'command='], { encoding: 'utf8', - timeout: 2_000 + timeout: PS_IDENTITY_TIMEOUT_MS }) // BSD ps formats lstart as a fixed-width 24-character timestamp. const startedAtMs = Date.parse(output.slice(0, 24)) @@ -612,7 +614,7 @@ export async function getDaemonLaunchIdentity( return commandLine.includes(expectedEntryPath) ? 'match' : 'mismatch' } -async function readVerifiedDaemonPid( +export async function readVerifiedDaemonPid( runtimeDir: string, socketPath: string, tokenPath: string, diff --git a/src/main/daemon/daemon-init.test.ts b/src/main/daemon/daemon-init.test.ts index aa9180f598b..51e054f7d14 100644 --- a/src/main/daemon/daemon-init.test.ts +++ b/src/main/daemon/daemon-init.test.ts @@ -1,8 +1,10 @@ /* eslint-disable max-lines -- Why: covers daemon-init's full restart flow (7-step sequence per docs/daemon-staleness-ux.md §Phase 1 + coalescer); one describe block keeps shared mocks in one place. */ import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' +import { WEDGED_DAEMON_CLASSIFICATION_BUDGET_MS } from './daemon-init' +import { OCCUPANCY_CONNECT_BUDGET_MS, OCCUPANCY_REQUEST_BUDGET_MS } from './daemon-occupancy' +import type { DaemonLaunchMode } from './daemon-spawner' import { join } from 'node:path' import { PROTOCOL_VERSION } from './types' -import { WEDGED_DAEMON_GRACE_RETRIES } from './daemon-init' const FAKE_USER_DATA_PATH = '/fake/userData' const FAKE_RUNTIME_DIR = join(FAKE_USER_DATA_PATH, 'daemon') @@ -28,6 +30,8 @@ const { getDaemonLaunchIdentityMock, isDaemonStaleForCurrentBundleMock, killStaleDaemonMock, + readVerifiedDaemonPidMock, + inspectDaemonPtyOwnershipMock, getProcessStartedAtMsMock, parseDaemonPidFileMock, replaceDaemonPidFileMock, @@ -91,6 +95,10 @@ const { const getMacDaemonTccAttributionHealthMock = vi.fn(async () => 'unknown') const getDaemonLaunchIdentityMock = vi.fn(() => 'match') const isDaemonStaleForCurrentBundleMock = vi.fn(() => false) + const inspectDaemonPtyOwnershipMock = vi.fn( + async (): Promise<'owns-live-ptys' | 'no-live-ptys' | 'unknown'> => 'unknown' + ) + const readVerifiedDaemonPidMock = vi.fn(async (): Promise<{ pid: number } | null> => null) const killStaleDaemonMock = vi.fn(async () => ({ killed: true, liveOwnerSurvived: false @@ -129,10 +137,12 @@ const { // Why: every DaemonSpawner pushes here so assertions can check the *same* spawner was reused across restart. const spawnerInstances: MockSpawner[] = [] + // Mirrors DaemonLaunchMode rather than restating one of its members: the type used to omit + // 'held', so no test could describe the launch the hold produces. const ensureRunningOverrides: (() => Promise<{ socketPath: string tokenPath: string - mode?: 'degraded-new-pty-fallback' + mode?: DaemonLaunchMode }>)[] = [] const adoptionLeaseReleases: ReturnType[] = [] const lifecycleLeaseErrors: Error[] = [] @@ -196,6 +206,8 @@ const { getDaemonLaunchIdentityMock, isDaemonStaleForCurrentBundleMock, killStaleDaemonMock, + readVerifiedDaemonPidMock, + inspectDaemonPtyOwnershipMock, getProcessStartedAtMsMock, parseDaemonPidFileMock, replaceDaemonPidFileMock, @@ -265,6 +277,17 @@ vi.mock('electron', () => ({ } })) +// Map the existing boolean socket double onto the canonical three-valued probe: +// present ⇒ something is serving, absent ⇒ positively dead. +vi.mock('./daemon-live-pty-evidence', () => ({ + inspectDaemonPtyOwnership: inspectDaemonPtyOwnershipMock +})) + +vi.mock('./daemon-endpoint-probe', async (importOriginal) => ({ + ...(await importOriginal>()), + probeSocketConnect: async (p: string) => (probeSocketExistsMock(p) ? 'connected' : 'missing') +})) + vi.mock('fs', () => ({ mkdirSync: vi.fn(), existsSync: (p: string) => probeSocketExistsMock(p) || p.includes('.pid'), @@ -289,11 +312,26 @@ vi.mock('./daemon-health', () => ({ healthCheckDaemon: healthCheckDaemonMock, isDaemonStaleForCurrentBundle: isDaemonStaleForCurrentBundleMock, killStaleDaemon: killStaleDaemonMock, + readVerifiedDaemonPid: readVerifiedDaemonPidMock, getProcessStartedAtMs: getProcessStartedAtMsMock, parseDaemonPidFile: parseDaemonPidFileMock })) -vi.mock('./client', () => ({ DaemonClient: daemonClientMock })) +vi.mock('./client', () => ({ + // Mirror ensureConnected onto the bounded variant so every existing double keeps its + // behaviour — including the ones whose whole point is that connecting throws. The budget is + // forwarded rather than dropped so a test can see what the launcher was willing to wait. + DaemonClient: function DaemonClientDouble(...args: unknown[]) { + const instance = (daemonClientMock as unknown as (...a: unknown[]) => Record)( + ...args + ) + if (instance && typeof instance === 'object' && !('ensureConnectedWithin' in instance)) { + instance.ensureConnectedWithin = (budgetMs?: number) => + (instance.ensureConnected as (ms?: number) => unknown)(budgetMs) + } + return instance + } +})) vi.mock('./daemon-lifecycle-event', () => ({ trackDaemonReplaced: trackDaemonReplacedMock, @@ -309,7 +347,7 @@ vi.mock('./daemon-spawner', () => ({ readonly getHandle: ReturnType private socketCounter: number private handle: { - mode?: 'degraded-new-pty-fallback' + mode?: DaemonLaunchMode releaseAdoptionLease?: () => void shutdown: () => Promise } | null @@ -554,6 +592,10 @@ describe('daemon-init: runRestartDaemon (7-step sequence)', () => { afterEach(() => { vi.clearAllMocks() + // Why restore too: tests here spy on Date.now, and clearAllMocks keeps the fake + // implementation. A failure before an inline restore would freeze the clock for every + // later test in the file, turning one red into a cascade. + vi.restoreAllMocks() }) it('re-binds listeners after the first daemon provider is installed', async () => { @@ -717,6 +759,28 @@ describe('daemon-init: runRestartDaemon (7-step sequence)', () => { expect(adapterInstances[1].disconnectOnly).toHaveBeenCalledOnce() }) + it('routes a held daemon through the degraded provider, not a bare adapter', async () => { + // The wiring the hold depends on. Without it a held daemon gets a plain DaemonPtyAdapter: + // every fresh spawn hangs on the wedged socket instead of falling back locally, and + // isDaemonDegraded() — an instanceof check — reports false, so the notice telling the user + // how to recover never renders. + const mod = await importFresh() + ensureRunningOverrides.push(async () => ({ + socketPath: '/fake/held-socket', + tokenPath: '/fake/held-token', + mode: 'held' + })) + + await mod.initDaemonPtyProvider() + + const { DegradedDaemonPtyProvider } = await import('./degraded-daemon-pty-provider') + const provider = mod.getDaemonProvider() + expect(provider).toBeInstanceOf(DegradedDaemonPtyProvider) + expect( + (provider as InstanceType).routesFreshSpawnsToLocalProvider + ).toBe(true) + }) + it('routes fresh PTYs to the local fallback when a preserved daemon cannot spawn new PTYs', async () => { const mod = await importFresh() ensureRunningOverrides.push(async () => ({ @@ -1196,6 +1260,50 @@ describe('daemon-init: runRestartDaemon (7-step sequence)', () => { } }) + it('falls back to killStaleDaemon without the live-PTY veto, so an explicit restart always wins', async () => { + // Why: this is the documented escape hatch (Settings → Manage Sessions → Restart). Opting + // into the veto here would report liveOwnerSurvived and throw, leaving the user no daemon. + probeSocketExistsMock.mockReturnValue(true) + netConnectMock.mockImplementation(() => { + const handlers: Record void)[]> = { connect: [], error: [] } + return { + on(event: string, cb: () => void) { + handlers[event]?.push(cb) + if (event === 'connect') { + queueMicrotask(() => cb()) + } + return this + }, + removeListener(event: string, cb: () => void) { + handlers[event] = handlers[event]?.filter((handler) => handler !== cb) ?? [] + return this + }, + destroy() {} + } + }) + const mod = await importFresh() + daemonClientMock.mockImplementationOnce(function MockWedgedDaemonClient() { + return { + ensureConnected: vi.fn(async () => { + throw new Error('Hello response timed out') + }), + request: vi.fn(), + disconnect: vi.fn() + } + }) + + await expect( + mod.cleanupDaemonForProtocol('/fake/daemon', PROTOCOL_VERSION) + ).resolves.toMatchObject({ cleaned: true }) + + expect(killStaleDaemonMock).toHaveBeenCalledWith( + '/fake/daemon', + `/fake/daemon/daemon-v${PROTOCOL_VERSION}.sock`, + `/fake/daemon/daemon-v${PROTOCOL_VERSION}.token`, + PROTOCOL_VERSION + ) + }) + it('coalesces concurrent restartDaemon() calls so the 7-step sequence runs exactly once', async () => { const mod = await importFresh() await mod.initDaemonPtyProvider() @@ -1729,7 +1837,7 @@ describe('daemon-init: runRestartDaemon (7-step sequence)', () => { '/fake/token', FAKE_DAEMON_ENTRY_PATH ) - expect(requestMock).toHaveBeenCalledWith('listSessions', undefined) + expect(requestMock).toHaveBeenCalledWith('listSessions', undefined, expect.any(Number)) expect(disconnectMock).toHaveBeenCalledOnce() expect(killStaleDaemonMock).not.toHaveBeenCalled() expect(forkMock).not.toHaveBeenCalled() @@ -1763,7 +1871,7 @@ describe('daemon-init: runRestartDaemon (7-step sequence)', () => { await launcher('/fake/socket', '/fake/token') - expect(requestMock).toHaveBeenCalledWith('listSessions', undefined) + expect(requestMock).toHaveBeenCalledWith('listSessions', undefined, expect.any(Number)) expect(disconnectMock).toHaveBeenCalledOnce() expect(killStaleDaemonMock).not.toHaveBeenCalled() expect(forkMock).not.toHaveBeenCalled() @@ -1862,7 +1970,7 @@ describe('daemon-init: runRestartDaemon (7-step sequence)', () => { await launcher('/fake/socket', '/fake/token') expect(getMacDaemonSystemResolverHealthMock).toHaveBeenCalledWith('/fake/socket', '/fake/token') - expect(requestMock).toHaveBeenCalledWith('listSessions', undefined) + expect(requestMock).toHaveBeenCalledWith('listSessions', undefined, expect.any(Number)) expect(disconnectMock).toHaveBeenCalledOnce() expect(getDaemonLaunchIdentityMock).not.toHaveBeenCalled() expect(killStaleDaemonMock).not.toHaveBeenCalled() @@ -1899,7 +2007,7 @@ describe('daemon-init: runRestartDaemon (7-step sequence)', () => { await launcher('/fake/socket', '/fake/token') - expect(requestMock).toHaveBeenCalledWith('listSessions', undefined) + expect(requestMock).toHaveBeenCalledWith('listSessions', undefined, expect.any(Number)) expect(disconnectMock).toHaveBeenCalledOnce() expect(killStaleDaemonMock).not.toHaveBeenCalled() expect(forkMock).not.toHaveBeenCalled() @@ -2840,7 +2948,7 @@ describe('daemon-init: runRestartDaemon (7-step sequence)', () => { await launcher('/fake/socket', '/fake/token') - expect(requestMock).toHaveBeenCalledWith('listSessions', undefined) + expect(requestMock).toHaveBeenCalledWith('listSessions', undefined, expect.any(Number)) expect(disconnectMock).toHaveBeenCalledOnce() expect(killStaleDaemonMock).not.toHaveBeenCalled() expect(forkMock).not.toHaveBeenCalled() @@ -2871,20 +2979,23 @@ describe('daemon-init: runRestartDaemon (7-step sequence)', () => { socketPath: string, tokenPath: string ) => Promise<{ - mode?: 'degraded-new-pty-fallback' + mode?: DaemonLaunchMode shutdown(): Promise }> checkDaemonHealthMock.mockResolvedValueOnce('pty-spawn-unhealthy') const handle = await launcher('/fake/socket', '/fake/token') - expect(requestMock).toHaveBeenCalledWith('listSessions', undefined) + expect(requestMock).toHaveBeenCalledWith('listSessions', undefined, expect.any(Number)) expect(handle.mode).toBe('degraded-new-pty-fallback') expect(killStaleDaemonMock).not.toHaveBeenCalled() expect(forkMock).not.toHaveBeenCalled() }) - it('replaces a health-check-failing daemon when live sessions cannot be verified and the pipe is dead', async () => { + // Note: only the adoption client is wedged here; the session probe answers, so this is the + // verified-zero case and the live-PTY veto stays off. The unverifiable path is covered by + // the grace-retry tests below. + it('replaces a health-check-failing daemon that reports no sessions when the pipe is dead', async () => { const mod = await importFresh() await mod.initDaemonPtyProvider() @@ -2951,6 +3062,8 @@ describe('daemon-init: runRestartDaemon (7-step sequence)', () => { daemonClientMock .mockImplementationOnce(unreachableClient) .mockImplementationOnce(unreachableClient) + // A cold start has no pid record to act on, so nothing is ever killed. + killStaleDaemonMock.mockResolvedValueOnce({ killed: false, liveOwnerSurvived: false }) const launcher = spawnerInstances[0].launcher as ( socketPath: string, @@ -3047,8 +3160,12 @@ describe('daemon-init: runRestartDaemon (7-step sequence)', () => { expect(forkMock).not.toHaveBeenCalled() }) - it('replaces a permanently wedged daemon after the grace window is exhausted (#8689)', async () => { - // Why: a socket that accepts connections but never answers hello was preserved forever (#8689); after grace it must be replaced. + it('holds a permanently wedged daemon rather than killing what it might be hosting', async () => { + // The trade this makes, deliberately: a socket that accepts connections but never answers + // hello can no longer be replaced at launch, so a wedged-but-empty daemon stays until the + // user restarts it (#8689 regresses to degraded mode). The alternative was killing a daemon + // whose live agents we had simply failed to observe, and that loss is unrecoverable while + // this one is one click from repaired. const mod = await importFresh() await mod.initDaemonPtyProvider() @@ -3065,7 +3182,7 @@ describe('daemon-init: runRestartDaemon (7-step sequence)', () => { daemonClientConstructionCount++ return { ensureConnected: vi.fn(async () => { - if (daemonClientConstructionCount <= 2 + WEDGED_DAEMON_GRACE_RETRIES) { + if (daemonClientConstructionCount <= 2) { throw new Error('Hello response timed out') } }), @@ -3104,22 +3221,22 @@ describe('daemon-init: runRestartDaemon (7-step sequence)', () => { try { await launcher('/fake/socket', '/fake/token') - expect(killStaleDaemonMock).toHaveBeenCalledWith( - FAKE_RUNTIME_DIR, - '/fake/socket', - '/fake/token' - ) - expect(forkMock).toHaveBeenCalled() - // The launcher probes the full grace budget: 1 initial probe + WEDGED_DAEMON_GRACE_RETRIES retries. - expect(daemonClientMock).toHaveBeenCalledTimes(3 + WEDGED_DAEMON_GRACE_RETRIES) - // Why: this replace path used to kill the daemon with no log, so a post-hoc - // reader could not tell it apart from an adoption; the verdict must be recorded. - expect(warnSpy).toHaveBeenCalledWith( - expect.stringContaining('Replacing daemon that failed the health check') - ) - expect(warnSpy).toHaveBeenCalledWith( - expect.stringContaining(`graceRetries=${WEDGED_DAEMON_GRACE_RETRIES}`) - ) + expect(killStaleDaemonMock).not.toHaveBeenCalled() + expect(forkMock).not.toHaveBeenCalled() + // 1 adoption + 1 patient probe. No post-fork adoption, because nothing was forked, and + // no retries, because there is no longer a retry loop: what remains after the patient + // connect is always exactly OCCUPANCY_REQUEST_BUDGET_MS, which cannot fund another ask + // at any ceiling. + expect(daemonClientMock).toHaveBeenCalledTimes(2) + // The verdict must still be recorded: holding without a log is indistinguishable from + // a successful adoption to anyone reading the log afterwards. + expect(warnSpy).toHaveBeenCalledWith(expect.stringContaining('holding an unreachable daemon')) + expect(warnSpy).toHaveBeenCalledWith(expect.stringContaining('Manage Sessions')) + // Why the second remedy is asserted: killStaleDaemon only kills a process whose identity + // matches the pid record, so when something other than an Orca daemon holds the endpoint + // a Restart clears nothing and the next launch is identical. Offering only the remedy + // that cannot work is how a user concludes the app is broken. + expect(warnSpy).toHaveBeenCalledWith(expect.stringContaining('quit and relaunch')) } finally { warnSpy.mockRestore() // Restore the answering default: clearAllMocks clears calls not impls, so the throwing impl would leak into later tests. @@ -3127,13 +3244,373 @@ 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. - expect(WEDGED_DAEMON_GRACE_RETRIES).toBeGreaterThanOrEqual(11) + it('holds a wedged daemon that still owns live terminals instead of killing its agents', async () => { + // Why hold and not adopt: a daemon that cannot answer listSessions cannot answer a hello, + // so there is nothing to adopt. The kill must never be attempted — its agents are alive. + const mod = await importFresh() + await mod.initDaemonPtyProvider() + + const answeringDefault = function MockDaemonClient() { + return { + ensureConnected: vi.fn(async () => {}), + request: vi.fn(async () => ({ sessions: [] })), + disconnect: vi.fn() + } + } + daemonClientMock.mockImplementation(function MockWedgedDaemonClient() { + return { + ensureConnected: vi.fn(async () => { + throw new Error('Hello response timed out') + }), + getDaemonIdentity: vi.fn(readLaunchedDaemonIdentity), + request: vi.fn(), + disconnect: vi.fn() + } + }) + // The daemon is identity-verified and its process still owns live terminals. + readVerifiedDaemonPidMock.mockResolvedValue({ pid: 4242 }) + inspectDaemonPtyOwnershipMock.mockResolvedValue('owns-live-ptys') + + const launcher = spawnerInstances[0].launcher as ( + socketPath: string, + tokenPath: string + ) => Promise<{ shutdown(): Promise; mode?: string }> + checkDaemonHealthMock.mockResolvedValueOnce('unreachable') + probeSocketExistsMock.mockReturnValue(true) + netConnectMock.mockImplementation(stubAliveSocketConnect) + + try { + const handle = await launcher('/fake/socket', '/fake/token') + + expect(handle.mode).toBe('held') + expect(killStaleDaemonMock).not.toHaveBeenCalled() + expect(forkMock).not.toHaveBeenCalled() + } finally { + daemonClientMock.mockImplementation(answeringDefault) + readVerifiedDaemonPidMock.mockResolvedValue(null) + inspectDaemonPtyOwnershipMock.mockResolvedValue('unknown') + } }) - it('preserves a daemon that stays wedged until the LAST allowed grace retry', async () => { - // Why: daemon drains only on the last allowed probe (1 + WEDGED_DAEMON_GRACE_RETRIES) — must be preserved, not replaced. + it('adopts a PTY-spawn-unhealthy daemon in degraded mode when it can still answer', async () => { + // Why the count must come from IPC: an answered listSessions is what proves the daemon + // can still complete a handshake, and adoption opens one. + const mod = await importFresh() + await mod.initDaemonPtyProvider() + + const answeringDefault = function MockDaemonClient() { + return { + ensureConnected: vi.fn(async () => {}), + request: vi.fn(async () => ({ sessions: [] })), + disconnect: vi.fn() + } + } + daemonClientMock.mockImplementation(function MockAnsweringDaemonClient() { + return { + ensureConnected: vi.fn(async () => {}), + request: vi.fn(async () => ({ sessions: [{ isAlive: true }] })), + getDaemonIdentity: vi.fn(readLaunchedDaemonIdentity), + disconnect: vi.fn() + } + }) + + const launcher = spawnerInstances[0].launcher as ( + socketPath: string, + tokenPath: string + ) => Promise<{ shutdown(): Promise; mode?: string }> + checkDaemonHealthMock.mockResolvedValueOnce('pty-spawn-unhealthy') + probeSocketExistsMock.mockReturnValue(true) + netConnectMock.mockImplementation(stubAliveSocketConnect) + + try { + const handle = await launcher('/fake/socket', '/fake/token') + + expect(handle.mode).toBe('degraded-new-pty-fallback') + expect(killStaleDaemonMock).not.toHaveBeenCalled() + expect(forkMock).not.toHaveBeenCalled() + } finally { + daemonClientMock.mockImplementation(answeringDefault) + } + }) + + it('holds a PTY-spawn-unhealthy daemon that has since stopped answering', async () => { + // Why: `health` is a reading from before the grace window. If listSessions went + // unanswered across all of it, adoption would open a hello the daemon can no longer + // complete — and that throw costs the app its daemon. + const mod = await importFresh() + await mod.initDaemonPtyProvider() + + const answeringDefault = function MockDaemonClient() { + return { + ensureConnected: vi.fn(async () => {}), + request: vi.fn(async () => ({ sessions: [] })), + disconnect: vi.fn() + } + } + daemonClientMock.mockImplementation(function MockSilentDaemonClient() { + return { + ensureConnected: vi.fn(async () => {}), + request: vi.fn(async () => { + throw new Error('listSessions timed out') + }), + getDaemonIdentity: vi.fn(readLaunchedDaemonIdentity), + disconnect: vi.fn() + } + }) + readVerifiedDaemonPidMock.mockResolvedValue({ pid: 4242 }) + inspectDaemonPtyOwnershipMock.mockResolvedValue('owns-live-ptys') + + const launcher = spawnerInstances[0].launcher as ( + socketPath: string, + tokenPath: string + ) => Promise<{ shutdown(): Promise; mode?: string }> + checkDaemonHealthMock.mockResolvedValueOnce('pty-spawn-unhealthy') + probeSocketExistsMock.mockReturnValue(true) + netConnectMock.mockImplementation(stubAliveSocketConnect) + + try { + const handle = await launcher('/fake/socket', '/fake/token') + + expect(handle.mode).toBe('held') + expect(killStaleDaemonMock).not.toHaveBeenCalled() + expect(forkMock).not.toHaveBeenCalled() + } finally { + daemonClientMock.mockImplementation(answeringDefault) + readVerifiedDaemonPidMock.mockResolvedValue(null) + inspectDaemonPtyOwnershipMock.mockResolvedValue('unknown') + } + }) + + it('keeps a daemon handle when the preserved daemon is too wedged to be adopted', async () => { + // Why: adoption needs a hello, which is exactly what a daemon wedged enough to be + // preserved cannot answer. Throwing here would abort initDaemonPtyProvider, leaving no + // spawner — and restartDaemon() throws without one, so the user loses the documented + // Manage Sessions → Restart remedy on top of having no daemon. + const mod = await importFresh() + await mod.initDaemonPtyProvider() + + const answeringDefault = function MockDaemonClient() { + return { + ensureConnected: vi.fn(async () => {}), + request: vi.fn(async () => ({ sessions: [] })), + disconnect: vi.fn() + } + } + // Permanently wedged: every client, including the adoption client, fails its hello. + daemonClientMock.mockImplementation(function MockWedgedDaemonClient() { + return { + ensureConnected: vi.fn(async () => { + throw new Error('Hello response timed out') + }), + getDaemonIdentity: vi.fn(readLaunchedDaemonIdentity), + request: vi.fn(), + disconnect: vi.fn() + } + }) + killStaleDaemonMock.mockResolvedValueOnce({ killed: false, liveOwnerSurvived: true }) + + const launcher = spawnerInstances[0].launcher as ( + socketPath: string, + tokenPath: string + ) => Promise<{ shutdown(): Promise; mode?: string }> + checkDaemonHealthMock.mockResolvedValueOnce('unreachable') + // The endpoint still listens, so the daemon is wedged rather than gone. + probeSocketExistsMock.mockReturnValue(true) + netConnectMock.mockImplementation(stubAliveSocketConnect) + + try { + const handle = await launcher('/fake/socket', '/fake/token') + + // 'held', not merely degraded: init must not attempt a lease on a daemon whose + // adoption hello just failed — that throw would cost the app its daemon entirely. + expect(handle.mode).toBe('held') + expect(forkMock).not.toHaveBeenCalled() + } finally { + daemonClientMock.mockImplementation(answeringDefault) + } + }) + + it('holds a hello-rejected daemon that owns live terminals rather than adopting it', async () => { + // Why: 'rejected' means it answered and refused the handshake, so adoption can never + // succeed. Falling through to preserveDaemon() would throw and cost the app its daemon + // entirely — and killing it would end agents that are still running. + const mod = await importFresh() + await mod.initDaemonPtyProvider() + + const answeringDefault = function MockDaemonClient() { + return { + ensureConnected: vi.fn(async () => {}), + request: vi.fn(async () => ({ sessions: [] })), + disconnect: vi.fn() + } + } + daemonClientMock.mockImplementation(function MockRejectingDaemonClient() { + return { + ensureConnected: vi.fn(async () => { + throw new Error('hello refused') + }), + getDaemonIdentity: vi.fn(readLaunchedDaemonIdentity), + request: vi.fn(), + disconnect: vi.fn() + } + }) + readVerifiedDaemonPidMock.mockResolvedValue({ pid: 4242 }) + inspectDaemonPtyOwnershipMock.mockResolvedValue('owns-live-ptys') + + const launcher = spawnerInstances[0].launcher as ( + socketPath: string, + tokenPath: string + ) => Promise<{ shutdown(): Promise; mode?: string }> + checkDaemonHealthMock.mockResolvedValueOnce('rejected') + probeSocketExistsMock.mockReturnValue(true) + netConnectMock.mockImplementation(stubAliveSocketConnect) + + try { + const handle = await launcher('/fake/socket', '/fake/token') + + expect(handle.mode).toBe('held') + expect(killStaleDaemonMock).not.toHaveBeenCalled() + expect(forkMock).not.toHaveBeenCalled() + } finally { + daemonClientMock.mockImplementation(answeringDefault) + readVerifiedDaemonPidMock.mockResolvedValue(null) + inspectDaemonPtyOwnershipMock.mockResolvedValue('unknown') + } + }) + + const wedgedClient = function MockWedgedDaemonClient() { + return { + ensureConnected: vi.fn(async () => { + throw new Error('Hello response timed out') + }), + request: vi.fn(), + disconnect: vi.fn() + } + } + const answeringClient = function MockDaemonClient() { + return { + ensureConnected: vi.fn(async () => {}), + request: vi.fn(async () => ({ sessions: [] })), + disconnect: vi.fn() + } + } + + it('still replaces when the endpoint is proven dead, so a cold start is not held', async () => { + // The regression holding most risks: 'unknown' is also what a cold start looks like, since + // nothing answers when nothing is there. Holding then would hand every first launch a + // provider pointed at no daemon. A missing socket is the discriminator. + const mod = await importFresh() + await mod.initDaemonPtyProvider() + daemonClientMock.mockImplementation(wedgedClient) + + const launcher = spawnerInstances[0].launcher as ( + socketPath: string, + tokenPath: string + ) => Promise<{ shutdown(): Promise }> + checkDaemonHealthMock.mockResolvedValueOnce('unreachable') + probeSocketExistsMock.mockReturnValue(false) + // Reaching the fork IS the assertion; throwing there stops before the spawn plumbing. + forkMock.mockImplementationOnce(() => { + throw new Error('reached the replacement fork') + }) + + try { + await expect(launcher('/fake/socket', '/fake/token')).rejects.toThrow( + 'reached the replacement fork' + ) + } finally { + daemonClientMock.mockImplementation(answeringClient) + } + }) + + it('still replaces an unreachable daemon that refused the handshake', async () => { + // 'rejected' answered and refused — bad token or foreign protocol — so it can never be + // adopted and its sessions can never be reattached. Holding one would be permanent + // degradation buying nothing, which is the opposite of the trade holding exists to make. + // Note this needs occupancy to stay 'unknown': a daemon that answers listSessions is + // 'empty' and reaches the replace path without ever consulting the exclusion. + const mod = await importFresh() + await mod.initDaemonPtyProvider() + daemonClientMock.mockImplementation(wedgedClient) + + const launcher = spawnerInstances[0].launcher as ( + socketPath: string, + tokenPath: string + ) => Promise<{ shutdown(): Promise }> + checkDaemonHealthMock.mockResolvedValueOnce('rejected') + probeSocketExistsMock.mockReturnValue(true) + netConnectMock.mockImplementation(stubAliveSocketConnect) + forkMock.mockImplementationOnce(() => { + throw new Error('reached the replacement fork') + }) + + try { + await expect(launcher('/fake/socket', '/fake/token')).rejects.toThrow( + 'reached the replacement fork' + ) + } finally { + daemonClientMock.mockImplementation(answeringClient) + } + }) + + it('spends a patient connect budget on the wedged ask, not the cheap one', async () => { + // Pins the round-11 defect, which shipped green because every other test recomputes the + // budget expression instead of watching the launcher spend it: with the evidence clock + // withheld, max(CONNECT, probeBudget - REQUEST) collapsed to CONNECT and the "patient" ask + // was a cheap ask wearing a comment. Observed here through the client double, which + // forwards the budget rather than dropping it. + const mod = await importFresh() + await mod.initDaemonPtyProvider() + + const connectBudgets: (number | undefined)[] = [] + const answeringDefault = function MockDaemonClient() { + return { + ensureConnected: vi.fn(async () => {}), + request: vi.fn(async () => ({ sessions: [] })), + disconnect: vi.fn() + } + } + daemonClientMock.mockImplementation(function MockWedgedDaemonClient() { + return { + ensureConnected: vi.fn(async (budgetMs?: number) => { + connectBudgets.push(budgetMs) + throw new Error('Hello response timed out') + }), + request: vi.fn(), + disconnect: vi.fn() + } + }) + + const launcher = spawnerInstances[0].launcher as ( + socketPath: string, + tokenPath: string + ) => Promise<{ shutdown(): Promise }> + checkDaemonHealthMock.mockResolvedValueOnce('unreachable') + probeSocketExistsMock.mockReturnValue(true) + netConnectMock.mockImplementation(stubAliveSocketConnect) + + try { + await launcher('/fake/socket', '/fake/token') + + // The first budget is the launcher's own adoption connect; the ask follows it. + const askBudget = connectBudgets[1] + expect(askBudget).toBeGreaterThan(OCCUPANCY_CONNECT_BUDGET_MS) + // And it must still leave the answer something to arrive in. + expect(askBudget).toBeLessThanOrEqual( + WEDGED_DAEMON_CLASSIFICATION_BUDGET_MS - OCCUPANCY_REQUEST_BUDGET_MS + ) + } finally { + daemonClientMock.mockImplementation(answeringDefault) + } + }) + + it('adopts a daemon that drains inside the patient ask, rather than degrading it', async () => { + // The payoff for spending the clock on one tolerant ask instead of many cheap ones. A + // counted answer is the only verdict that reaches preserveDaemon() — full daemon service — + // where 'unknown' would have settled for a degraded hold. This is what a slow-but-alive + // daemon gets back. + const frozenNow = Date.now() + vi.spyOn(Date, 'now').mockReturnValue(frozenNow) const mod = await importFresh() await mod.initDaemonPtyProvider() @@ -3147,7 +3624,8 @@ describe('daemon-init: runRestartDaemon (7-step sequence)', () => { } daemonClientMock.mockImplementation(function MockDaemonClient() { probe += 1 - const drainsNow = probe >= 1 + WEDGED_DAEMON_GRACE_RETRIES + // 1 = the launcher's adoption client; 2 = the patient ask, which is where it drains. + const drainsNow = probe >= 2 return { ensureConnected: vi.fn(async () => { if (!drainsNow) { @@ -3170,10 +3648,13 @@ describe('daemon-init: runRestartDaemon (7-step sequence)', () => { netConnectMock.mockImplementation(stubAliveSocketConnect) try { - await launcher('/fake/socket', '/fake/token') + const handle = (await launcher('/fake/socket', '/fake/token')) as { mode?: string } expect(killStaleDaemonMock).not.toHaveBeenCalled() expect(forkMock).not.toHaveBeenCalled() + // Not 'held': a counted answer is adoptable, and settling for degraded mode here would + // waste the very patience the single ask was widened to buy. + expect(handle.mode).toBeUndefined() } finally { daemonClientMock.mockImplementation(answeringDefault) } @@ -3503,7 +3984,7 @@ describe('daemon-init: runRestartDaemon (7-step sequence)', () => { '/fake/token', '1.2.3' ) - expect(requestMock).toHaveBeenCalledWith('listSessions', undefined) + expect(requestMock).toHaveBeenCalledWith('listSessions', undefined, expect.any(Number)) expect(disconnectMock).toHaveBeenCalledOnce() expect(killStaleDaemonMock).not.toHaveBeenCalled() expect(forkMock).not.toHaveBeenCalled() diff --git a/src/main/daemon/daemon-init.ts b/src/main/daemon/daemon-init.ts index 7b95d4a010c..dbb43ed859a 100644 --- a/src/main/daemon/daemon-init.ts +++ b/src/main/daemon/daemon-init.ts @@ -15,9 +15,18 @@ import { unlinkOwnedDaemonPidFile, type DaemonLauncher, type DaemonPidFile, + type DaemonLaunchMode, type DaemonProcessHandle } from './daemon-spawner' import { DAEMON_EXIT_ENDPOINT_OCCUPIED } from './daemon-endpoint-ownership' +import { endpointIsProvenDead, probeSocketConnect } from './daemon-endpoint-probe' +import { + OCCUPANCY_CONNECT_BUDGET_MS, + OCCUPANCY_REQUEST_BUDGET_MS, + raiseOccupancyWithProcessEvidence, + resolveDaemonOccupancy, + type DaemonOccupancy +} from './daemon-occupancy' import { DaemonPtyAdapter, type DaemonRespawnReason } from './daemon-pty-adapter' import { DaemonPtyRouter } from './daemon-pty-router' import { DaemonClient } from './client' @@ -34,6 +43,7 @@ import { checkDaemonHealth, isDaemonStaleForCurrentBundle, killStaleDaemon, + readVerifiedDaemonPid, parseDaemonPidFile, type MacDaemonTccAttributionHealth } from './daemon-health' @@ -70,8 +80,47 @@ function logDaemonMilestone(event: string, details: Record = {} } } -// Why: extra hello+listSessions probes (~5s each) giving a wedged-but-connectable daemon ~60s grace to answer and keep its live sessions before a permanent wedge (#8689) is replaced; raise only alongside the fail-open cap. -export const WEDGED_DAEMON_GRACE_RETRIES = 11 +/** + * Ceiling on the whole failed-health classification — every probe, the grace window, the + * identity check and the process-table read together — enforced at runtime rather than + * summed by hand. + * + * Why enforced: startup fails open at 60s by abandoning the daemon provider outright, and + * ensureRunning() is not abortable, so overrunning costs the app its daemon *and* still kills + * the incumbent. Four separate reviews found a term missing from the hand-written sum that + * was supposed to prevent that — the launcher's own adoption connect, an identity probe, an + * endpoint probe, a doubled evidence deadline. A budget that has to be remembered is a budget + * that will be wrong, so the code now spends against a clock and stops when it runs out. + * + * The remainder of the fail-open window belongs to what follows a replace verdict: the kill + * ladder and the daemon fork. + */ +export const WEDGED_DAEMON_CLASSIFICATION_BUDGET_MS = 34_000 + +/** + * The clock the identity re-check and process-table read need, checked *after* the probes + * rather than withheld from them. + * + * It used to be a reservation, and that was backwards: withholding twelve seconds starved the + * one probe whose answer can still restore full daemon mode, since only a counted reply reaches + * preserveDaemon(). With the reservation in place the "patient" ask resolved to exactly the + * cheap ask's four seconds. + * + * Do not read the demotion as "the evidence read is cosmetic" — an earlier version of this + * comment said that and it was wrong twice over. The read decides the verdict wherever the + * unknown hold declines to: it has no endpointIsProvenDead check and no health !== 'rejected' + * check, so evidence is what holds a daemon whose socket entry vanished, and what holds a + * hello-rejected daemon that is still hosting agents. Skipping it on a spent clock therefore + * withdraws real protection, not a log line; the gate is set so that only probes which already + * consumed the budget can trigger it. + * + * So the evidence read is opportunistic now. If the probes used the clock, it is skipped and + * the verdict stays 'unknown' — which holds the daemon exactly as an evidence-raised + * 'occupied' would have. Nothing is lost but a more precise log line. + * + * Zero on Windows, which runs neither step. + */ +export const CLASSIFICATION_EVIDENCE_MIN_MS = process.platform === 'win32' ? 0 : 12_000 const DAEMON_SELF_SHUTDOWN_WAIT_MS = 5_000 const DAEMON_CHILD_TERMINATION_GRACE_MS = 5_000 const DAEMON_CHILD_FORCE_EXIT_WAIT_MS = 1_000 @@ -173,27 +222,10 @@ function probeSocket(socketPath: string): Promise { }) } -async function getAliveDaemonSessionCount( - socketPath: string, - tokenPath: string, - protocolVersion = PROTOCOL_VERSION -): Promise { - const client = new DaemonClient({ socketPath, tokenPath, protocolVersion }) - try { - await client.ensureConnected() - const result = await client.request('listSessions', undefined) - return result.sessions.filter((session) => session.isAlive).length - } catch { - return null - } finally { - client.disconnect() - } -} - function createPreservedDaemonHandle( runtimeDir: string, protocolVersion = PROTOCOL_VERSION, - mode?: 'degraded-new-pty-fallback' + mode?: DaemonLaunchMode ): DaemonProcessHandle { const handle: DaemonProcessHandle = { shutdown: async () => { @@ -420,19 +452,30 @@ function isNoSuchProcessError(error: unknown): boolean { return typeof error === 'object' && error !== null && 'code' in error && error.code === 'ESRCH' } +/** How a preserve decision reads in a log line, from either evidence source. */ +function describeOccupancy(occupancy: DaemonOccupancy): string { + if (occupancy.liveSessions === null) { + return 'live session state could not be verified' + } + return `it owns ${occupancy.liveSessions} live session${occupancy.liveSessions === 1 ? '' : 's'}` +} + +/** IPC only: these callers run against a daemon that just answered a health check. */ +function resolveOccupancyOverIpc(socketPath: string, tokenPath: string): Promise { + return resolveDaemonOccupancy({ socketPath, tokenPath, recordedPid: null }) +} + async function shouldPreserveDaemonWithLiveSessions( socketPath: string, tokenPath: string, replacementLabel: string ): Promise { - const liveSessionCount = await getAliveDaemonSessionCount(socketPath, tokenPath) - if (liveSessionCount === 0) { + const occupancy = await resolveOccupancyOverIpc(socketPath, tokenPath) + if (occupancy.state === 'empty') { return false } console.warn( - liveSessionCount === null - ? `[daemon] Preserving daemon ${replacementLabel} because live session state could not be verified` - : `[daemon] Preserving daemon ${replacementLabel} because it owns ${liveSessionCount} live session${liveSessionCount === 1 ? '' : 's'}` + `[daemon] Preserving daemon ${replacementLabel} because ${describeOccupancy(occupancy)}` ) return true } @@ -460,6 +503,9 @@ function createOutOfProcessLauncher( | { reason: Parameters[0] liveSessionCount: number | null + /** Rendered once the outcome is known; only the health-check branch announces. */ + verdict?: string + announce?: boolean } | undefined let confirmedReplacement = false @@ -467,17 +513,35 @@ function createOutOfProcessLauncher( socketPath, tokenPath }) + // Why the clock starts before the adoption connect: that connect is on the classification + // path and uses the non-shared five-seconds-per-step default, so it was the fourth term to + // go missing from the sum this replaces. A ceiling that starts after part of the work is + // the same fiction in a new place. + const classificationDeadline = Date.now() + WEDGED_DAEMON_CLASSIFICATION_BUDGET_MS + const classificationRemainingMs = (): number => Math.max(0, classificationDeadline - Date.now()) try { // Why: acquire the full pair before control-only probes so an expired inherited deadline can't fire in the probe-to-adoption gap. - await adoptionClient.ensureConnected() + // Capped: this only acquires a lease that preserveDaemon() re-establishes anyway, and a + // daemon that accepts the socket but never answers hello would otherwise spend the whole + // classification clock here, leaving nothing for the probes that protect its sessions. + await adoptionClient.ensureConnectedWithin( + Math.min(OCCUPANCY_CONNECT_BUDGET_MS, classificationRemainingMs()) + ) await reconcileDaemonPidOwnership(adoptionClient, pidPath) } catch { adoptionClient.disconnect() adoptionClient = null } - const preserveDaemon = async ( - mode?: 'degraded-new-pty-fallback' - ): Promise => { + /** + * Keep the incumbent without talking to it. No adoption, so no lease: the lease only + * cancels an adoption watchdog, which cannot fire on a daemon that owns sessions. + */ + const holdIncumbentDaemon = (): DaemonProcessHandle => { + adoptionClient?.disconnect() + adoptionClient = null + return createPreservedDaemonHandle(runtimeDir, PROTOCOL_VERSION, 'held') + } + const preserveDaemon = async (mode?: DaemonLaunchMode): Promise => { const connectedClient = adoptionClient ?? undefined adoptionClient = null return holdDaemonAdoptionLease( @@ -494,19 +558,17 @@ function createOutOfProcessLauncher( if (health === 'healthy') { const resolverHealth = await getMacDaemonSystemResolverHealth(socketPath, tokenPath) if (resolverHealth === 'unhealthy') { - const liveSessionCount = await getAliveDaemonSessionCount(socketPath, tokenPath) - if (liveSessionCount !== 0) { + const occupancy = await resolveOccupancyOverIpc(socketPath, tokenPath) + if (occupancy.state !== 'empty') { console.warn( - liveSessionCount === null - ? '[daemon] Preserving daemon with unavailable macOS system resolver because live session state could not be verified' - : `[daemon] Preserving daemon with unavailable macOS system resolver because it owns ${liveSessionCount} live session${liveSessionCount === 1 ? '' : 's'}` + `[daemon] Preserving daemon with unavailable macOS system resolver because ${describeOccupancy(occupancy)}` ) return preserveDaemon() } console.warn('[daemon] Replacing daemon with unavailable macOS system resolver') pendingReplacement = { reason: 'unhealthy_resolver', - liveSessionCount + liveSessionCount: 0 } confirmedReplacement = (await cleanupDaemonForProtocol(runtimeDir, PROTOCOL_VERSION)) .cleaned @@ -558,12 +620,12 @@ function createOutOfProcessLauncher( if (attributionHealth === 'severed') { // Why: replacing with live sessions would kill them; Settings → Developer // Permissions surfaces the Manage Sessions → Restart remedy instead. - const liveSessionCount = await getAliveDaemonSessionCount(socketPath, tokenPath) - if (liveSessionCount === 0) { + const occupancy = await resolveOccupancyOverIpc(socketPath, tokenPath) + if (occupancy.state === 'empty') { console.warn( '[daemon] Replacing daemon whose macOS TCC attribution is severed (spawning app binary no longer exists)' ) - pendingReplacement = { reason: 'severed_tcc_attribution', liveSessionCount } + pendingReplacement = { reason: 'severed_tcc_attribution', liveSessionCount: 0 } confirmedReplacement = ( await cleanupDaemonForProtocol(runtimeDir, PROTOCOL_VERSION) ).cleaned @@ -577,47 +639,141 @@ function createOutOfProcessLauncher( } } } else { - // Why: a busy machine can time out the health check on a live daemon; re-verify with a session list before killing its sessions. - let liveSessionCount = await getAliveDaemonSessionCount(socketPath, tokenPath) - // Why: a wedged-but-connectable daemon (Windows update relaunch) may still own live sessions, so grace-retry before replacing; a permanent wedge (#8689) exhausts the grace, and 'rejected' skips it (handshake refused = never adoptable). - let graceRetry = 0 - while ( - liveSessionCount === null && - health !== 'rejected' && - graceRetry < WEDGED_DAEMON_GRACE_RETRIES && - (await probeSocket(socketPath)) - ) { - liveSessionCount = await getAliveDaemonSessionCount(socketPath, tokenPath) - graceRetry++ - } - if (liveSessionCount !== null && liveSessionCount > 0) { - if (health === 'pty-spawn-unhealthy') { + // Why: a busy machine can time out the health check on a live daemon; re-verify what + // it is hosting before killing its sessions. + // + // No recordedPid: this loop waits for *IPC* to recover, and the process table cannot + // change its answer within a grace window, so scanning it every pass would multiply + // the launch budget for an answer we already have. It is read once, after the wait. + // Every probe spends the shared clock, minus what the evidence read still needs, so no + // probe can eat the reserve however long the daemon takes to answer. + const probeBudgetMs = (): number => classificationRemainingMs() + const askDaemonWhatItHosts = (connectBudgetMs: number): Promise => + resolveDaemonOccupancy({ + socketPath, + tokenPath, + recordedPid: null, + budgetMs: probeBudgetMs(), + connectBudgetMs + }) + // Why the tolerant question goes first: this path is only reached because a 3s health + // check timed out, so a cheap probe re-asks on a stricter budget than the one that + // triaged the daemon here and can only ever agree with it. The patient ask is the only + // one that can disagree, and asking it last meant asking it with the clock already + // spent. It gets every millisecond the answer itself does not still need, and never + // less than the cheap ask would have had. + const patientConnectBudgetMs = (): number => + Math.max(OCCUPANCY_CONNECT_BUDGET_MS, probeBudgetMs() - OCCUPANCY_REQUEST_BUDGET_MS) + let occupancy = await askDaemonWhatItHosts(patientConnectBudgetMs()) + // Why there is no retry loop here any more. Cheap retries used to follow this ask, but + // the arithmetic makes them unreachable at every ceiling, not just this one: + // + // remaining = B - E - max(CONNECT, (B - E) - REQUEST) = REQUEST, whenever B - E > CONNECT + REQUEST + // + // The patient connect takes every millisecond the answer does not need, so what is left + // after it is always exactly OCCUPANCY_REQUEST_BUDGET_MS — never enough to fund another + // ask. Raising the budget donates the increase to the same connect and changes nothing. + // Funding a real retry needs ~71s of classification, kill ladder and fork against a 60s + // fail-open, so the loop cannot be bought back at any price. + // + // One shape did reach it, and the earlier claim that none could was wrong: a connect + // that fails *fast* leaves the budget nearly whole, and while a refused or missing + // endpoint is caught by the proven-dead guard, the EPERM/EMFILE class reads 'unknown' + // and would have passed. Dropping it costs that case a retry — which retrying was never + // going to fix, because an fd-exhausted or permission-denied connect fails the same way + // the second time, and recover() puts the daemon back into full service on the next + // spawn once the condition clears. + // + // Nothing is lost by dropping it: a 4s retry cannot reach a daemon that needs longer + // than 4s to answer, which is the whole wedge population, while this one ask waits ~12s. + // The only case a retry caught and this does not is a daemon that recovers within a few + // seconds of being asked — and DegradedDaemonFreshSpawnRouter.recover() already restores + // it to full daemon service on the next spawn, off the startup clock entirely. + // Do not delete this because 'unknown' and 'occupied' both hold — twice reviewed, twice + // proposed for removal, and it regresses both times. The occupied branch below has no + // endpointIsProvenDead check and the unknown hold does, so this read is the only thing + // standing between a kill and a daemon whose socket entry vanished (a tmp reaper, a + // failed publish) while it still hosts live agents: without it that reads as + // unknown + proven-dead and falls through to killStaleDaemon. + // + // The children scan earns its keep for the same reason in reverse: a verified-live pid + // alone would also hold a *childless* daemon whose socket vanished, which is the one + // #8689 case we can still safely replace. + // + // The evidence that separates a wedged daemon still hosting terminals from one with + // nothing left to lose (#8689). Read once IPC has had its full chance, and only when + // it never answered — both because it costs a process scan, and because identity has + // to be re-verified first: 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' && + process.platform !== 'win32' && + classificationRemainingMs() >= CLASSIFICATION_EVIDENCE_MIN_MS + ? ((await readVerifiedDaemonPid(runtimeDir, socketPath, tokenPath))?.pid ?? null) + : null + occupancy = await raiseOccupancyWithProcessEvidence(occupancy, evidencePid) + if (occupancy.state === 'occupied') { + const owned = + occupancy.liveSessions === null + ? 'live terminal processes' + : `${occupancy.liveSessions} live session${occupancy.liveSessions === 1 ? '' : 's'}` + + // Why this comes first: adoption opens a hello, and neither of these daemons can + // complete one — 'rejected' answered and refused, and a count only the process table + // could supply means nothing answered across the whole grace window. `health` is a + // reading from before that window, so it cannot overrule them. Attempting adoption + // anyway throws, and the throw costs the app its daemon entirely. + if (health === 'rejected' || occupancy.liveSessions === null) { console.warn( - `[daemon] DEGRADED MODE: preserving daemon that failed the PTY spawn health check because it owns ${liveSessionCount} live session${liveSessionCount === 1 ? '' : 's'}. Existing sessions keep working; fresh terminals run on the local provider WITHOUT daemon persistence until you restart the daemon (Manage Sessions → Restart).` + `[daemon] DEGRADED MODE: holding a daemon that cannot be adopted (health=${health}) but still owns ${owned}. Killing it would end them; fresh terminals run on the local provider WITHOUT daemon persistence until it recovers or you restart it (Manage Sessions → Restart).` + ) + return holdIncumbentDaemon() + } + if (health === 'pty-spawn-unhealthy') { + // It answered listSessions just now, so it is adoptable — it simply cannot open + // new PTYs. + console.warn( + `[daemon] DEGRADED MODE: preserving daemon that failed the PTY spawn health check because it owns ${owned}. Existing sessions keep working; fresh terminals run on the local provider WITHOUT daemon persistence until you restart the daemon (Manage Sessions → Restart).` ) return preserveDaemon('degraded-new-pty-fallback') } console.warn( - `[daemon] Preserving daemon that failed the health check because it owns ${liveSessionCount} live session${liveSessionCount === 1 ? '' : 's'}` + `[daemon] Preserving daemon that failed the health check because it owns ${owned}` ) return preserveDaemon() } - // Why: the sibling replace branches announce themselves, but this one used - // to kill a daemon silently — leaving no way to tell a replacement apart - // from an adoption after the fact. A cold start also lands here with - // nothing to replace, so only speak up once something actually answered: - // a probe that returned a count, a socket that survived a grace retry, or - // a refused hello. - if (liveSessionCount !== null || graceRetry > 0 || health === 'rejected') { + // 'unknown' is not permission to kill. Everything above has failed to establish what + // this daemon is hosting, and killing it on that basis is what destroyed live agents. + // Bounded classification cannot be made safe by budgeting — matching main's tolerance + // for one ask costs more clock than the 60s fail-open leaves — so the residual stops + // being lethal instead. Being wrong now costs a degraded session, not an agent. + // + // Two exclusions, both about not holding something that can never be recovered: + // - a proven-dead endpoint is a cold start or a corpse; there is nothing to hold, and + // holding would hand every first launch a provider pointed at no daemon. Read fresh + // here rather than reused, because the daemon can die during the grace window. + // - 'rejected' answered and refused the handshake, so it can never be adopted and its + // sessions can never be reattached. Holding one is permanently degraded for nothing. + if ( + occupancy.state === 'unknown' && + health !== 'rejected' && + !endpointIsProvenDead(await probeSocketConnect(socketPath)) + ) { console.warn( - `[daemon] Replacing daemon that failed the health check (health=${health}, liveSessions=${liveSessionCount ?? 'unverifiable'}, graceRetries=${graceRetry})` + `[daemon] DEGRADED MODE: holding an unreachable daemon (health=${health}); its session state could not be verified, and replacing it would end any terminals it still owns. Fresh terminals run on the local provider WITHOUT daemon persistence until it recovers or you restart it (Manage Sessions → Restart). If a restart does not clear this, something other than an Orca daemon is holding the endpoint — quit and relaunch.` ) + return holdIncumbentDaemon() } - // Why: unlike the log above, telemetry gates on confirmedReplacement below — the - // post-kill truth — so a cold start that killed nothing never reports a replacement. + // Why: a cold start reaches this same fall-through with nothing to replace, so stay + // quiet unless something actually answered — a verified count, a socket that survived + // a grace retry, or a refused hello. + // Why: telemetry gates on confirmedReplacement below — the post-kill truth — so a + // cold start that killed nothing never reports a replacement. pendingReplacement = { reason: 'failed_health_check', - liveSessionCount + liveSessionCount: occupancy.liveSessions, + verdict: `health=${health}, occupancy=${occupancy.state}`, + announce: occupancy.state === 'empty' || health === 'rejected' } } @@ -636,12 +792,25 @@ function createOutOfProcessLauncher( try { return await preserveDaemon('degraded-new-pty-fallback') } catch { + // Why: adoption needs a hello, which is exactly what a daemon wedged enough to be + // preserved cannot answer. A still-listening endpoint means it is wedged, not gone, + // so keep a lease-free handle: the lease only cancels the adoption watchdog, which + // cannot fire on a daemon that still owns sessions, and throwing here would cost the + // app its daemon handle — taking Manage Sessions → Restart down with it. + if (!endpointIsProvenDead(await probeSocketConnect(socketPath))) { + return createPreservedDaemonHandle(runtimeDir, PROTOCOL_VERSION, 'held') + } // It died between the probe and the adoption; the endpoint is genuinely free now. throw new DaemonEndpointOwnershipError( 'Daemon replacement aborted: the existing daemon could not be confirmed stopped' ) } } + if (pendingReplacement?.verdict && (pendingReplacement.announce || killOutcome.killed)) { + console.warn( + `[daemon] Replacing daemon that failed the health check (${pendingReplacement.verdict})` + ) + } confirmedReplacement = killOutcome.killed || confirmedReplacement // Why: rank by how well each reason is evidenced. A confirmed kill whose reason positively // identified the daemon outranks the attribution, so a stale bundle caught here is not billed @@ -912,6 +1081,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. } } @@ -988,12 +1160,17 @@ export async function initDaemonPtyProvider( let routedAdapter: DaemonProvider = newAdapter try { // Why: the launcher's temporary pair closes only after this permanent pair is established, leaving no adoption gap. - await newAdapter.establishLifecycleLease() - releaseDaemonAdoptionLease(newSpawner.getHandle()) + // Why skipped when held: we deliberately never talked to that daemon, so there is no + // handshake to complete and no temporary lease to hand over. Attempting one would throw + // and abort init, leaving the app with no spawner — and restartDaemon() throws without one. + if (launchMode !== 'held') { + await newAdapter.establishLifecycleLease() + releaseDaemonAdoptionLease(newSpawner.getHandle()) + } legacyAdapters = await createLegacyDaemonAdapters(runtimeDir) routedAdapter = - launchMode === 'degraded-new-pty-fallback' + launchMode === 'degraded-new-pty-fallback' || launchMode === 'held' ? new DegradedDaemonPtyProvider({ current: newAdapter, legacy: legacyAdapters, diff --git a/src/main/daemon/daemon-launch-budget.test.ts b/src/main/daemon/daemon-launch-budget.test.ts new file mode 100644 index 00000000000..4726c5b3bb3 --- /dev/null +++ b/src/main/daemon/daemon-launch-budget.test.ts @@ -0,0 +1,90 @@ +import { describe, expect, it } from 'vitest' +import { LOCAL_PTY_STARTUP_FAIL_OPEN_TIMEOUT_MS } from '../startup/first-window-startup-services' +import { + CLASSIFICATION_EVIDENCE_MIN_MS, + WEDGED_DAEMON_CLASSIFICATION_BUDGET_MS +} from './daemon-init' +import { OCCUPANCY_CONNECT_BUDGET_MS, OCCUPANCY_REQUEST_BUDGET_MS } from './daemon-occupancy' +import { + POSIX_OWNERSHIP_PROBE_DEADLINE_MS, + PTY_OWNERSHIP_PROBE_ATTEMPTS +} from './daemon-live-pty-evidence' +import { HEALTH_CHECK_TIMEOUT_MS, PS_IDENTITY_TIMEOUT_MS } from './daemon-health' + +/** + * Kept out of the launcher's own spec because that file mocks daemon-health, which would + * shadow constants this is here to hold to account. + * + * This deliberately asserts one enforced ceiling rather than a sum of the path's parts. The + * sum was the earlier design, and four separate reviews each found a different term missing + * from it — the launcher's own adoption connect, an identity probe, an endpoint probe, an + * evidence deadline applied twice. Every one of them passed this file while the real path + * overran. The launcher now spends against a clock, so the only thing left worth asserting + * is that the clock leaves room for what comes after it. + */ +describe('wedged-daemon classification budget', () => { + it('leaves the kill ladder and the daemon fork room under the startup fail-open', () => { + // Startup abandons the daemon provider entirely at the cap, and ensureRunning() is not + // abortable — so overrunning costs the app its daemon *and* still kills the incumbent. + // What follows a replace verdict is the kill ladder (~11.5s: identity, endpoint probe, + // KILL_WAIT, recheck, another probe, SIGKILL confirm) and the fork's own 10s readiness + // timeout — plus, on packaged Windows, a daemon-host directory copy of unbounded size. + // The margin above 21.5s is what covers that copy. + const afterClassificationMs = + LOCAL_PTY_STARTUP_FAIL_OPEN_TIMEOUT_MS - WEDGED_DAEMON_CLASSIFICATION_BUDGET_MS + + expect(WEDGED_DAEMON_CLASSIFICATION_BUDGET_MS).toBeLessThan( + LOCAL_PTY_STARTUP_FAIL_OPEN_TIMEOUT_MS + ) + expect(afterClassificationMs).toBeGreaterThanOrEqual(22_000) + }) + + it('leaves the evidence read enough clock to be worth attempting', () => { + // Not a reservation: the probes spend first and this is checked afterwards. Assert only + // that the threshold covers what the two steps actually cost, or the launcher would start + // a read it cannot finish. + const evidenceMs = POSIX_OWNERSHIP_PROBE_DEADLINE_MS * PTY_OWNERSHIP_PROBE_ATTEMPTS + + if (process.platform === 'win32') { + // Neither guarded step runs on Windows — there is no session-leader signal to read. + expect(CLASSIFICATION_EVIDENCE_MIN_MS).toBe(0) + } else { + expect(CLASSIFICATION_EVIDENCE_MIN_MS).toBeGreaterThanOrEqual( + PS_IDENTITY_TIMEOUT_MS + evidenceMs + ) + } + }) + + it('only attempts the evidence read while the clock can still finish it', () => { + // The gate is what keeps an opportunistic read from becoming an overrun: the read costs an + // identity ps plus two ownership probes, and it runs after the probes have already spent + // whatever they spent. If the threshold ever drops below that cost, a read started near the + // ceiling finishes past it — and the ceiling is what the kill ladder and fork are sized + // against. + const evidenceCostMs = + PS_IDENTITY_TIMEOUT_MS + POSIX_OWNERSHIP_PROBE_DEADLINE_MS * PTY_OWNERSHIP_PROBE_ATTEMPTS + + if (process.platform !== 'win32') { + expect(CLASSIFICATION_EVIDENCE_MIN_MS).toBeGreaterThanOrEqual(evidenceCostMs) + } + // And the ceiling must still hold if a read starts at the very last moment the gate allows. + expect(WEDGED_DAEMON_CLASSIFICATION_BUDGET_MS).toBeGreaterThanOrEqual( + CLASSIFICATION_EVIDENCE_MIN_MS + ) + }) + + it('gives the patient ask more clock than the cheap ask it replaced', () => { + // Kept as arithmetic, but it is NOT the guard: this restates the expression rather than + // executing it, so it cannot catch the expression being replaced. daemon-init.test.ts + // 'spends a patient connect budget on the wedged ask' watches the launcher actually spend + // it, and is the test that fails when this collapses back to the cheap constant. + const elapsedBeforeAsk = OCCUPANCY_CONNECT_BUDGET_MS + HEALTH_CHECK_TIMEOUT_MS + const probeBudgetMs = WEDGED_DAEMON_CLASSIFICATION_BUDGET_MS - elapsedBeforeAsk + const patientConnectMs = Math.max( + OCCUPANCY_CONNECT_BUDGET_MS, + probeBudgetMs - OCCUPANCY_REQUEST_BUDGET_MS + ) + + expect(patientConnectMs).toBeGreaterThan(OCCUPANCY_CONNECT_BUDGET_MS) + }) +}) diff --git a/src/main/daemon/daemon-live-pty-evidence.test.ts b/src/main/daemon/daemon-live-pty-evidence.test.ts new file mode 100644 index 00000000000..aad786f2cfd --- /dev/null +++ b/src/main/daemon/daemon-live-pty-evidence.test.ts @@ -0,0 +1,437 @@ +import { beforeEach, describe, expect, it, vi } from 'vitest' +import { inspectDaemonPtyOwnership } from './daemon-live-pty-evidence' +import type { ProcessTableRow } from '../../shared/process-table-snapshot' + +const { readFreshProcessTable, readCachedProcessTable } = vi.hoisted(() => ({ + readFreshProcessTable: vi.fn(async () => [] as ProcessTableRow[]), + readCachedProcessTable: vi.fn(async () => [] as ProcessTableRow[]) +})) + +// Spread the original: the Windows enumerator builds its reader from this module too. +vi.mock('../../shared/process-table-snapshot', async (importOriginal) => ({ + ...(await importOriginal>()), + getFreshProcessTableSnapshot: readFreshProcessTable, + getProcessTableSnapshot: readCachedProcessTable +})) + +const DAEMON_PID = 4242 + +function row(pid: number, ppid: number, overrides: Partial = {}): ProcessTableRow { + return { pid, ppid, stat: 'Ss', command: '/bin/bash', ...overrides } +} + +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 posixTable(rows: ProcessTableRow[]): () => Promise { + return async () => rows +} + +describe('inspectDaemonPtyOwnership on POSIX', () => { + it.each(['darwin', 'linux'] as const)('reports live PTY ownership on %s', async (platform) => { + await expect( + inspectDaemonPtyOwnership(DAEMON_PID, { + platform, + readPosixProcessTable: posixTable([daemonRow, row(101, DAEMON_PID)]) + }) + ).resolves.toBe('owns-live-ptys') + }) + + it('counts a grandchild, since macOS wraps every shell in login(1)', async () => { + // daemon -> login(1) -> shell: a direct-children test would miss the agent entirely. + await expect( + inspectDaemonPtyOwnership(DAEMON_PID, { + platform: 'darwin', + readPosixProcessTable: posixTable([ + daemonRow, + row(101, DAEMON_PID, { command: '/usr/bin/login -flpq nwparker' }), + row(202, 101, { stat: 'S+', command: 'claude' }) + ]) + }) + ).resolves.toBe('owns-live-ptys') + }) + + it('reports no live PTYs for an observed root with no descendants', async () => { + await expect( + inspectDaemonPtyOwnership(DAEMON_PID, { + platform: 'darwin', + readPosixProcessTable: posixTable([daemonRow, row(999, 1)]) + }) + ).resolves.toBe('no-live-ptys') + }) + + it('does not count zombies, which a wedged daemon cannot reap', async () => { + // Why this matters: the daemon is wedged precisely because its event loop is blocked, + // so every already-exited agent lingers as . Counting them would read + // "all agents finished" as "agents still running" — correlated with the wedge itself. + await expect( + inspectDaemonPtyOwnership(DAEMON_PID, { + platform: 'darwin', + readPosixProcessTable: posixTable([ + daemonRow, + row(101, DAEMON_PID, { stat: 'Z+', command: '' }), + row(102, DAEMON_PID, { stat: 'Z', command: '' }) + ]) + }) + ).resolves.toBe('no-live-ptys') + }) + + it('still counts a live descendant hidden behind a zombie parent', async () => { + await expect( + inspectDaemonPtyOwnership(DAEMON_PID, { + platform: 'darwin', + readPosixProcessTable: posixTable([ + daemonRow, + row(101, DAEMON_PID, { stat: 'Z', command: '' }), + row(202, 101, { stat: 'Ss', command: 'codex' }) + ]) + }) + ).resolves.toBe('owns-live-ptys') + }) + + it('ignores helpers the daemon forked, which are not session leaders', async () => { + // Why this and not re-sampling: a hung `scutil`, credential helper or PTY-spawn health + // check outlives any sampling gap — often it is *why* the daemon is wedged. Only a PTY + // child is a session leader (forkpty calls setsid), so the flag is the real discriminator. + await expect( + inspectDaemonPtyOwnership(DAEMON_PID, { + platform: 'darwin', + readPosixProcessTable: posixTable([ + daemonRow, + row(101, DAEMON_PID, { stat: 'S', command: '/usr/sbin/scutil --dns' }), + row(102, DAEMON_PID, { stat: 'R+', command: '/bin/sh -c exit 0' }) + ]) + }) + ).resolves.toBe('no-live-ptys') + }) + + it("excludes the daemon's own PTY-spawn probe, which forkpty also makes a session leader", async () => { + // Why the stat flag is not enough: the daemon opens this PTY itself, so a daemon hosting + // zero user terminals would be held forever on the strength of its own stuck health check. + await expect( + inspectDaemonPtyOwnership(DAEMON_PID, { + platform: 'darwin', + readPosixProcessTable: posixTable([ + daemonRow, + row(101, DAEMON_PID, { stat: 'Ss', command: '/bin/sh -c exit 0' }) + ]) + }) + ).resolves.toBe('no-live-ptys') + }) + + it("still counts a real terminal sitting beside the daemon's own probe", async () => { + await expect( + inspectDaemonPtyOwnership(DAEMON_PID, { + platform: 'darwin', + readPosixProcessTable: posixTable([ + daemonRow, + row(101, DAEMON_PID, { stat: 'Ss', command: '/bin/sh -c exit 0' }), + row(202, DAEMON_PID, { stat: 'Ss+', command: 'claude' }) + ]) + }) + ).resolves.toBe('owns-live-ptys') + }) + + it('does not exclude an agent whose command merely contains a probe command', async () => { + // Exact match, not prefix or substring: `sh -c` payloads are user-supplied, and treating one + // as the daemon's own probe discards proof that killing the daemon would end real work. + await expect( + inspectDaemonPtyOwnership(DAEMON_PID, { + platform: 'darwin', + readPosixProcessTable: posixTable([ + daemonRow, + row(101, DAEMON_PID, { stat: 'Ss', command: '/bin/sh -c exit 0 && claude' }) + ]) + }) + ).resolves.toBe('owns-live-ptys') + }) + + it('counts a session leader reached through a non-session-leader hop', async () => { + await expect( + inspectDaemonPtyOwnership(DAEMON_PID, { + platform: 'darwin', + readPosixProcessTable: posixTable([ + daemonRow, + row(101, DAEMON_PID, { stat: 'S', command: 'wrapper' }), + row(202, 101, { stat: 'Ss+', command: 'claude' }) + ]) + }) + ).resolves.toBe('owns-live-ptys') + }) + + it('ignores a login wrapper stranded without its shell (#13764)', async () => { + // Why: the macOS TCC wrapper can outlive the shell it wrapped, leaving a session leader + // hosting nothing. On hosts where those accumulate, counting them would hold a daemon + // whose sessions have all ended — indefinitely, and for no live work. + await expect( + inspectDaemonPtyOwnership(DAEMON_PID, { + platform: 'darwin', + readPosixProcessTable: posixTable([ + daemonRow, + row(101, DAEMON_PID, { command: '/usr/bin/login -flpq nwparker /bin/bash …' }), + row(102, DAEMON_PID, { command: '/usr/bin/login -flpq nwparker /bin/bash …' }) + ]) + }) + ).resolves.toBe('no-live-ptys') + }) + + it('still counts a login wrapper that has its shell', async () => { + await expect( + inspectDaemonPtyOwnership(DAEMON_PID, { + platform: 'darwin', + readPosixProcessTable: posixTable([ + daemonRow, + row(101, DAEMON_PID, { command: '/usr/bin/login -flpq nwparker /bin/bash …' }), + row(202, 101, { command: '/opt/homebrew/bin/bash --rcfile …' }) + ]) + }) + ).resolves.toBe('owns-live-ptys') + }) + + it('reports unknown when the table never contained the daemon', async () => { + // Why: an unobserved root yields the same empty result as a childless one — + // reading that as "empty" authorizes killing a daemon full of live agents. + await expect( + inspectDaemonPtyOwnership(DAEMON_PID, { + platform: 'darwin', + readPosixProcessTable: posixTable([row(999, 1)]) + }) + ).resolves.toBe('unknown') + }) + + it('falls back to the cached table when the uncached read blows its deadline', async () => { + // Why this matters most on the busiest host: every agent pane drives the shared reader on + // its own cadence, so the uncached read queues behind them and can expire on queueing + // alone — going blind exactly where the daemon has the most agents to lose. + await expect( + inspectDaemonPtyOwnership(DAEMON_PID, { + platform: 'darwin', + posixDeadlineMs: 5, + readPosixProcessTable: () => new Promise(() => {}), + readCachedPosixProcessTable: posixTable([daemonRow, row(101, DAEMON_PID)]) + }) + ).resolves.toBe('owns-live-ptys') + }) + + it('reports unknown only when the cached table is blind too', async () => { + await expect( + inspectDaemonPtyOwnership(DAEMON_PID, { + platform: 'darwin', + posixDeadlineMs: 5, + readPosixProcessTable: () => new Promise(() => {}), + readCachedPosixProcessTable: () => new Promise(() => {}) + }) + ).resolves.toBe('unknown') + }) + + it('reports unknown when the process table cannot be read', async () => { + await expect( + inspectDaemonPtyOwnership(DAEMON_PID, { + platform: 'darwin', + readPosixProcessTable: async () => { + throw new Error('ps timed out') + } + }) + ).resolves.toBe('unknown') + }) + + it('tolerates a ppid cycle reachable from the daemon without hanging', async () => { + // Why this shape: `ps` is not atomic, so a re-parented process can appear twice and + // close a loop. The cycle must be reachable from the root or the walk never enters it. + await expect( + inspectDaemonPtyOwnership(DAEMON_PID, { + platform: 'darwin', + readPosixProcessTable: posixTable([ + daemonRow, + row(101, DAEMON_PID), + row(102, 101), + row(101, 102) + ]) + }) + ).resolves.toBe('owns-live-ptys') + }) +}) + +describe('inspectDaemonPtyOwnership on win32', () => { + it('abstains rather than counting descendants it cannot classify', async () => { + // Why no verdict at all: the POSIX signal is that a hosted terminal is a session leader, + // and Windows has no equivalent — so the only available answer was "any descendant", + // which counts the orphaned conpty hosts a wedged daemon cannot reap. Holding on those + // would make a wedged, empty daemon unreplaceable forever. 'unknown' leaves Windows as it + // was before this change instead of trading one failure mode for a worse one. + const readPosixProcessTable = vi.fn(async () => [daemonRow, row(101, DAEMON_PID)]) + + await expect( + inspectDaemonPtyOwnership(DAEMON_PID, { platform: 'win32', readPosixProcessTable }) + ).resolves.toBe('unknown') + expect(readPosixProcessTable).not.toHaveBeenCalled() + }) +}) + +describe('inspectDaemonPtyOwnership login(1) handling', () => { + it('counts a childless login(1) as live work off darwin', async () => { + // Orca only wraps terminals in login(1) on macOS, so elsewhere this pattern is the user's + // own login — and one still prompting for credentials has no child yet. Excluding it there + // discards real work to solve a macOS problem. + const rows = [ + daemonRow, + row(5000, DAEMON_PID, { stat: 'Ss', command: '/usr/bin/login nwparker' }) + ] + + await expect( + inspectDaemonPtyOwnership(DAEMON_PID, { + platform: 'linux', + readPosixProcessTable: async () => rows + }) + ).resolves.toBe('owns-live-ptys') + }) + + it('still excludes a childless login(1) on darwin (#13764)', async () => { + const rows = [ + daemonRow, + row(5000, DAEMON_PID, { stat: 'Ss', command: '/usr/bin/login -pf nwparker /bin/zsh' }) + ] + + await expect( + inspectDaemonPtyOwnership(DAEMON_PID, { + platform: 'darwin', + readPosixProcessTable: async () => rows + }) + ).resolves.toBe('no-live-ptys') + }) +}) + +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. + const readPosixProcessTable = vi + .fn<() => Promise>() + .mockResolvedValueOnce([daemonRow]) + .mockRejectedValueOnce(new Error('ps timed out')) + + await expect( + inspectDaemonPtyOwnership(DAEMON_PID, { + platform: 'darwin', + posixDeadlineMs: 5, + readPosixProcessTable, + readCachedPosixProcessTable: async () => { + throw new Error('cached read blind too') + } + }) + ).resolves.toBe('unknown') + }) + it('takes a conclusive answer on the first read, without re-sampling', async () => { + const readPosixProcessTable = vi.fn(async () => [daemonRow, row(101, DAEMON_PID)]) + + await expect( + inspectDaemonPtyOwnership(DAEMON_PID, { platform: 'darwin', readPosixProcessTable }) + ).resolves.toBe('owns-live-ptys') + expect(readPosixProcessTable).toHaveBeenCalledTimes(1) + }) + + it('retries a blind read, because the load that wedges the daemon also blinds ps', async () => { + const readPosixProcessTable = vi + .fn<() => Promise>() + .mockRejectedValueOnce(new Error('ps timed out')) + .mockResolvedValueOnce([daemonRow, row(101, DAEMON_PID)]) + + await expect( + inspectDaemonPtyOwnership(DAEMON_PID, { platform: 'darwin', readPosixProcessTable }) + ).resolves.toBe('owns-live-ptys') + }) + + it('preserves on a sighting the second read could not contradict', async () => { + // Why: a blind read is not evidence against a live one. Killing agents is unrecoverable. + const readPosixProcessTable = vi + .fn<() => Promise>() + .mockResolvedValueOnce([daemonRow, row(101, DAEMON_PID)]) + .mockRejectedValueOnce(new Error('ps timed out')) + + await expect( + inspectDaemonPtyOwnership(DAEMON_PID, { platform: 'darwin', readPosixProcessTable }) + ).resolves.toBe('owns-live-ptys') + }) + + 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(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>() + .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 () => { + const readPosixProcessTable = vi.fn(async (): Promise => { + throw new Error('ps timed out') + }) + + await expect( + inspectDaemonPtyOwnership(DAEMON_PID, { platform: 'darwin', readPosixProcessTable }) + ).resolves.toBe('unknown') + expect(readPosixProcessTable.mock.calls.length).toBeGreaterThan(1) + }) +}) + +describe('inspectDaemonPtyOwnership POSIX process-table source', () => { + beforeEach(() => { + readFreshProcessTable.mockReset() + readCachedProcessTable.mockReset() + readFreshProcessTable.mockResolvedValue([daemonRow, row(101, DAEMON_PID)]) + readCachedProcessTable.mockResolvedValue([]) + }) + + it('reads an uncached table, since the cached one can predate the PTYs it protects', async () => { + // The 500ms TTL would also hand both samples the same array, collapsing the confirmation. + await expect(inspectDaemonPtyOwnership(DAEMON_PID, { platform: 'darwin' })).resolves.toBe( + 'owns-live-ptys' + ) + expect(readFreshProcessTable).toHaveBeenCalled() + expect(readCachedProcessTable).not.toHaveBeenCalled() + }) +}) diff --git a/src/main/daemon/daemon-live-pty-evidence.ts b/src/main/daemon/daemon-live-pty-evidence.ts new file mode 100644 index 00000000000..7e6c7d7ab8d --- /dev/null +++ b/src/main/daemon/daemon-live-pty-evidence.ts @@ -0,0 +1,260 @@ +import { + getFreshProcessTableSnapshot, + getProcessTableSnapshot, + type ProcessTableRow +} from '../../shared/process-table-snapshot' + +/** + * Out-of-band answer to "is this daemon still hosting running terminals?". + * + * Deliberately never touches the daemon socket: the only caller asks precisely + * because the daemon has already failed to answer over it, and a wedged daemon + * cannot be asked to vouch for its own sessions. Replacing a daemon kills every + * process it hosts, so that decision needs evidence that survives the wedge. + * + * 'unknown' is not "empty" — it means the process table could not be read, or did not + * contain the daemon at all. It is not evidence of absence, and it is deliberately not + * evidence of presence either: the only caller raises to preserve on 'owns-live-ptys' alone. + * + * What 'unknown' costs changed with the launcher: it no longer falls through to a kill, it + * holds the daemon in degraded mode. So this module's job is now to spare the user that + * degradation where it safely can, not to stand between them and a dead agent. + */ +export type DaemonPtyOwnership = 'owns-live-ptys' | 'no-live-ptys' | 'unknown' + +export type DaemonPtyOwnershipDeps = { + platform?: NodeJS.Platform + readPosixProcessTable?: () => Promise + readCachedPosixProcessTable?: () => Promise + posixDeadlineMs?: number +} + +/** + * Why sampled twice: the load that wedges the daemon is the same load that can blind + * the process-table read, so a single blind sample would lose the evidence exactly when + * it matters most. Only blindness is retried — a conclusive answer is taken as given. + * This runs only on the replace path, after ~60s of grace is already spent. + */ +export const PTY_OWNERSHIP_PROBE_ATTEMPTS = 2 + +/** + * 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. + */ +export const POSIX_OWNERSHIP_PROBE_DEADLINE_MS = 4_000 + +/** macos-tcc-login-shell.ts wraps every darwin terminal in this. */ +const MACOS_LOGIN_WRAPPER_PREFIX = '/usr/bin/login ' + +function withDeadline(work: Promise, deadlineMs: number, onDeadline: T): Promise { + return new Promise((resolve) => { + const timer = setTimeout(() => resolve(onDeadline), deadlineMs) + timer.unref?.() + void work.then( + (value) => { + clearTimeout(timer) + resolve(value) + }, + () => { + clearTimeout(timer) + resolve(onDeadline) + } + ) + }) +} + +/** + * The daemon opens PTYs for its own probes, and forkpty makes those session leaders too, so + * process state alone cannot tell them from a hosted terminal. Each is a fixed, argument-less + * command the daemon issues itself, so matching them exactly costs no real terminal. + */ +const DAEMON_SELF_SPAWNED_PTY_COMMANDS = [ + // pty-subprocess.ts checkPtySpawnHealth + { program: 'sh', args: '-c exit 0' } +] + +function isDaemonSelfSpawnedPty(row: Pick): boolean { + const command = row.command.trim() + return DAEMON_SELF_SPAWNED_PTY_COMMANDS.some(({ program, args }) => { + const suffix = ` ${args}` + // Why the argv tail must match exactly: `sh -c` payloads are user-supplied, and treating + // one as the daemon's own probe would discard proof that real work is running. + if (!command.endsWith(suffix)) { + return false + } + // Only the program may vary, and only by path, so compare its trailing segment rather + // than the whole string. + const executable = command.slice(0, command.length - suffix.length) + return (executable.split('/').pop() ?? executable) === program + }) +} + +/** + * A PTY child is a session leader — forkpty() calls setsid() — which the daemon's plain + * subprocesses (a `scutil` resolver probe, a stuck credential helper) never are. + * + * Zombies are excluded for a correlated reason: a wedged daemon cannot reap, so its + * already-exited PTYs linger as and would read as still running. + */ +function isLivePtySessionLeader(row: ProcessTableRow): boolean { + // Lowercase 's' is only ever the session-leader flag; no process state code uses it. + return !row.stat.startsWith('Z') && row.stat.includes('s') && !isDaemonSelfSpawnedPty(row) +} + +/** + * macOS wraps every terminal in `/usr/bin/login` for TCC attribution, and the wrapper can + * outlive the shell it wrapped (#13764) — a session leader hosting nothing. Counting those + * would hold a daemon whose sessions have all ended, on hosts where they accumulate by the + * hundred. A wrapper still doing its job always has the shell it exec'd beneath it. + */ +function isStrandedLoginWrapper( + row: ProcessTableRow, + hasChildren: boolean, + platform: NodeJS.Platform +): boolean { + // Why darwin only: Orca wraps terminals in login(1) for TCC attribution on macOS and nowhere + // else, so off darwin this pattern can only ever be a user's own login(1) — and one that is + // still prompting for credentials has no child yet, which is exactly the shape excluded here. + // Applied POSIX-wide it discarded real work to solve a macOS problem. + return ( + platform === 'darwin' && + !hasChildren && + row.command.trim().startsWith(MACOS_LOGIN_WRAPPER_PREFIX) + ) +} + +function collectLivePtyDescendants( + rows: ProcessTableRow[], + rootPid: number, + platform: NodeJS.Platform +): ProcessTableRow[] { + const childrenByPpid = new Map() + for (const row of rows) { + if (row.pid === rootPid) { + continue + } + const siblings = childrenByPpid.get(row.ppid) + if (siblings) { + siblings.push(row) + } else { + childrenByPpid.set(row.ppid, [row]) + } + } + const visited = new Set([rootPid]) + const queue: number[] = [rootPid] + const live: ProcessTableRow[] = [] + while (queue.length > 0) { + const pid = queue.shift() as number + for (const child of childrenByPpid.get(pid) ?? []) { + if (visited.has(child.pid)) { + continue + } + visited.add(child.pid) + queue.push(child.pid) + if ( + isLivePtySessionLeader(child) && + !isStrandedLoginWrapper(child, childrenByPpid.has(child.pid), platform) + ) { + live.push(child) + } + } + } + return live +} + +/** POSIX only; Windows abstains before this is reached. */ +async function probeOnce( + daemonPid: number, + 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 + const remaining = (): number => Math.max(1, deadline - Date.now()) + const rows = + (await withDeadline( + (deps.readPosixProcessTable ?? getFreshProcessTableSnapshot)(), + remaining(), + null + )) ?? + // Why fall back instead of answering 'unknown': the uncached reader queues behind the + // scans every agent pane already drives, so the busiest host — the one this evidence + // exists to protect — is the likeliest to blow the deadline on queueing alone. A table a + // 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. + (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. + if (rows === null || !rows.some((row) => row.pid === daemonPid)) { + return 'unknown' + } + return collectLivePtyDescendants(rows, daemonPid, deps.platform ?? process.platform).length > 0 + ? 'owns-live-ptys' + : 'no-live-ptys' +} + +/** + * A session-leader descendant is positive proof that killing this daemon would destroy + * running work. Descendants rather than direct children: macOS wraps every shell in + * login(1) for TCC attribution, so the agent is a grandchild at best. + */ +export async function inspectDaemonPtyOwnership( + daemonPid: number, + deps: DaemonPtyOwnershipDeps = {} +): Promise { + const platform = deps.platform ?? process.platform + // Why Windows gets no verdict at all: the POSIX signal is a property only a hosted terminal + // has — forkpty makes it a session leader — and Windows has no equivalent, so the branch + // that lived here could only count descendants. That reads a wedged daemon's orphaned + // conpty hosts as live work, since ClosePseudoConsole runs on the daemon's own JS thread + // and a daemon too wedged to answer is too wedged to reap them. + // + // Abstaining costs Windows nothing it had: this evidence can only ever raise 'unknown' to + // 'occupied', and both already hold the daemon. A verdict here would only let Windows print + // the more accurate of two identical outcomes, which is not worth guessing for. + if (platform === 'win32') { + return 'unknown' + } + let emptyAwaitingConfirmation = false + for (let attempt = 0; attempt < PTY_OWNERSHIP_PROBE_ATTEMPTS; attempt++) { + let sample: DaemonPtyOwnership + try { + // The confirming read must be its own observation, so it is denied the cached table. + sample = await probeOnce(daemonPid, deps, !emptyAwaitingConfirmation) + } catch { + sample = 'unknown' + } + // Why a second look before accepting emptiness: 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' && !emptyAwaitingConfirmation) { + emptyAwaitingConfirmation = true + continue + } + if (sample !== 'unknown') { + return sample + } + } + // Why not 'no-live-ptys' here: reaching this means the confirming read went blind, and a + // blind read cannot corroborate anything. Upgrading an unconfirmed emptiness to a definitive + // one is exactly the "absence of proof is proof of absence" step this module exists to + // refuse — and emptiness is the answer that authorizes a kill. + return 'unknown' +} diff --git a/src/main/daemon/daemon-occupancy.test.ts b/src/main/daemon/daemon-occupancy.test.ts new file mode 100644 index 00000000000..fc3599178eb --- /dev/null +++ b/src/main/daemon/daemon-occupancy.test.ts @@ -0,0 +1,269 @@ +import { describe, expect, it, vi } from 'vitest' +import { + OCCUPANCY_CONNECT_BUDGET_MS, + OCCUPANCY_REQUEST_BUDGET_MS, + raiseOccupancyWithProcessEvidence, + resolveDaemonOccupancy, + type DaemonOccupancy, + type DaemonOccupancyDeps +} from './daemon-occupancy' +import type { inspectDaemonPtyOwnership } from './daemon-live-pty-evidence' + +const SOCKET_PATH = '/tmp/orca-daemon.sock' +const TOKEN_PATH = '/tmp/orca-daemon.token' +const DAEMON_PID = 4242 + +type Ownership = Awaited> + +function ipcAnswers(count: number | null) { + return vi.fn>(async () => count) +} + +function ownershipIs(ownership: Ownership) { + return vi.fn(async () => ownership) +} + +function resolve( + deps: DaemonOccupancyDeps, + recordedPid: number | null = DAEMON_PID +): Promise>> { + return resolveDaemonOccupancy({ + socketPath: SOCKET_PATH, + tokenPath: TOKEN_PATH, + recordedPid, + deps + }) +} + +describe('resolveDaemonOccupancy with a daemon that answered', () => { + it('reports occupied with the counted sessions, without consulting the process table', async () => { + const listSessions = ipcAnswers(3) + const inspectPtyOwnership = ownershipIs('no-live-ptys') + + await expect(resolve({ listSessions, inspectPtyOwnership })).resolves.toEqual({ + state: 'occupied', + liveSessions: 3 + }) + expect(listSessions).toHaveBeenCalledWith( + SOCKET_PATH, + TOKEN_PATH, + expect.any(Number), + expect.any(Number) + ) + // The daemon's own reply is authoritative; process-table evidence could only muddy it. + expect(inspectPtyOwnership).not.toHaveBeenCalled() + }) + + it('reports empty on a count of zero, without consulting the process table', async () => { + // The one state that licenses a kill, and only the daemon itself can establish it. + const listSessions = ipcAnswers(0) + const inspectPtyOwnership = ownershipIs('owns-live-ptys') + + await expect(resolve({ listSessions, inspectPtyOwnership })).resolves.toEqual({ + state: 'empty', + liveSessions: 0 + }) + expect(inspectPtyOwnership).not.toHaveBeenCalled() + }) + + it('reports occupied for a single session', async () => { + await expect( + resolve({ listSessions: ipcAnswers(1), inspectPtyOwnership: ownershipIs('unknown') }) + ).resolves.toEqual({ state: 'occupied', liveSessions: 1 }) + }) +}) + +describe('resolveDaemonOccupancy when the daemon could not answer', () => { + it('raises to occupied on process-table evidence, keyed to the recorded pid', async () => { + const inspectPtyOwnership = ownershipIs('owns-live-ptys') + + await expect(resolve({ listSessions: ipcAnswers(null), inspectPtyOwnership })).resolves.toEqual( + { + state: 'occupied', + liveSessions: null + } + ) + expect(inspectPtyOwnership).toHaveBeenCalledWith(DAEMON_PID) + }) + + it('stays unknown — never empty — when the process table shows no live PTYs', async () => { + // The asymmetry the module exists for: the table may only ever *raise* the answer. + // A daemon too wedged to list its sessions is exactly as likely to be hosting them, + // and ps can miss PTYs it never observed. Reading this as 'empty' would license + // killing live agents unrecoverably; 'unknown' is the residual, not permission. + const inspectPtyOwnership = ownershipIs('no-live-ptys') + + await expect(resolve({ listSessions: ipcAnswers(null), inspectPtyOwnership })).resolves.toEqual( + { + state: 'unknown', + liveSessions: null + } + ) + expect(inspectPtyOwnership).toHaveBeenCalledWith(DAEMON_PID) + }) + + it('stays unknown when the process table could not be read', async () => { + await expect( + resolve({ listSessions: ipcAnswers(null), inspectPtyOwnership: ownershipIs('unknown') }) + ).resolves.toEqual({ state: 'unknown', liveSessions: null }) + }) + + it('stays unknown without inspecting an unverified pid', async () => { + // A pid we could not tie back to this daemon may have been recycled; its children + // would be some other process's, and counting them is evidence about the wrong tree. + const inspectPtyOwnership = ownershipIs('owns-live-ptys') + + await expect( + resolve({ listSessions: ipcAnswers(null), inspectPtyOwnership }, null) + ).resolves.toEqual({ state: 'unknown', liveSessions: null }) + expect(inspectPtyOwnership).not.toHaveBeenCalled() + }) +}) + +describe('resolveDaemonOccupancy budgets', () => { + it('waits longer for an answer than for a handshake', () => { + // Why they differ: a daemon that cannot complete a handshake is wedged and worth + // re-asking cheaply; one that answered the handshake is demonstrably alive, and its + // count settles the question outright. Collapsing both into one tight budget is what + // made a slow-but-answering daemon indistinguishable from a dead one. + expect(OCCUPANCY_REQUEST_BUDGET_MS).toBeGreaterThan(OCCUPANCY_CONNECT_BUDGET_MS) + }) + + it('never spends more than the ceiling the caller handed it', async () => { + const listSessions = vi.fn(async (_socket: string, _token: string, budgetMs: number) => { + expect(budgetMs).toBeLessThanOrEqual(5_000) + return null + }) + + await expect( + resolveDaemonOccupancy({ + socketPath: SOCKET_PATH, + tokenPath: TOKEN_PATH, + recordedPid: null, + budgetMs: 5_000, + deps: { listSessions } + }) + ).resolves.toEqual({ state: 'unknown', liveSessions: null }) + expect(listSessions).toHaveBeenCalledWith(SOCKET_PATH, TOKEN_PATH, 5_000, expect.any(Number)) + }) +}) + +describe('raiseOccupancyWithProcessEvidence', () => { + const UNKNOWN: DaemonOccupancy = { state: 'unknown', liveSessions: null } + + it('raises an unanswered verdict to occupied, with no count to report', async () => { + const inspectPtyOwnership = ownershipIs('owns-live-ptys') + + await expect( + raiseOccupancyWithProcessEvidence(UNKNOWN, DAEMON_PID, { inspectPtyOwnership }) + ).resolves.toEqual({ state: 'occupied', liveSessions: null }) + expect(inspectPtyOwnership).toHaveBeenCalledWith(DAEMON_PID) + }) + + it('leaves an unanswered verdict unknown — never empty — when the table shows no live PTYs', async () => { + // The whole point of the raise-only contract: ps can miss PTYs it never observed, so an + // empty-looking table is not permission to kill a daemon that could not answer for itself. + await expect( + raiseOccupancyWithProcessEvidence(UNKNOWN, DAEMON_PID, { + inspectPtyOwnership: ownershipIs('no-live-ptys') + }) + ).resolves.toEqual(UNKNOWN) + }) + + it('leaves an unanswered verdict unchanged when the table could not be read', async () => { + await expect( + raiseOccupancyWithProcessEvidence(UNKNOWN, DAEMON_PID, { + inspectPtyOwnership: ownershipIs('unknown') + }) + ).resolves.toEqual(UNKNOWN) + }) + + it('does not inspect an unverified pid', async () => { + // A pid we could not tie back to this daemon may have been recycled; its children are + // evidence about the wrong process tree. + const inspectPtyOwnership = ownershipIs('owns-live-ptys') + + await expect( + raiseOccupancyWithProcessEvidence(UNKNOWN, null, { inspectPtyOwnership }) + ).resolves.toEqual(UNKNOWN) + expect(inspectPtyOwnership).not.toHaveBeenCalled() + }) + + it('returns an empty verdict untouched, without consulting the process table', async () => { + // 'empty' came from the daemon itself and is the one state that licenses a kill. Re-asking + // the table could only lower it back to 'unknown', discarding an IPC-proven answer. + const inspectPtyOwnership = ownershipIs('owns-live-ptys') + + await expect( + raiseOccupancyWithProcessEvidence({ state: 'empty', liveSessions: 0 }, DAEMON_PID, { + inspectPtyOwnership + }) + ).resolves.toEqual({ state: 'empty', liveSessions: 0 }) + expect(inspectPtyOwnership).not.toHaveBeenCalled() + }) + + it('keeps the counted sessions of an already-occupied verdict', async () => { + // Raising an answered count to the countless 'occupied' would lose what the daemon reported. + const inspectPtyOwnership = ownershipIs('no-live-ptys') + + await expect( + raiseOccupancyWithProcessEvidence({ state: 'occupied', liveSessions: 3 }, DAEMON_PID, { + inspectPtyOwnership + }) + ).resolves.toEqual({ state: 'occupied', liveSessions: 3 }) + expect(inspectPtyOwnership).not.toHaveBeenCalled() + }) + + it('leaves the verdict unchanged when the inspector throws', async () => { + await expect( + raiseOccupancyWithProcessEvidence(UNKNOWN, DAEMON_PID, { + inspectPtyOwnership: vi.fn(async () => { + throw new Error('process table read exploded') + }) + }) + ).resolves.toEqual(UNKNOWN) + }) +}) + +describe('resolveDaemonOccupancy when an injected dep throws', () => { + it('degrades an inspector rejection to unknown', async () => { + // Why: a question that could not be asked is exactly what the residual is for. Letting + // it escape would route a failed observation into the launch path. + const inspectPtyOwnership = vi.fn(async () => { + throw new Error('process table read exploded') + }) + + await expect(resolve({ listSessions: ipcAnswers(null), inspectPtyOwnership })).resolves.toEqual( + { + state: 'unknown', + liveSessions: null + } + ) + }) + + it('degrades a failing listSessions dep to unknown', async () => { + // countLiveSessionsOverIpc catches internally and returns null; an injected dep is + // not held to that, so the module guards it. + await expect( + resolve({ + listSessions: async () => { + throw new Error('socket vanished') + }, + inspectPtyOwnership: ownershipIs('owns-live-ptys') + }) + ).resolves.toEqual({ state: 'unknown', liveSessions: null }) + }) +}) + +describe('resolveDaemonOccupancy with a nonsense count', () => { + it.each([Number.NaN, -1, 1.5])('refuses to read %p as emptiness', async (counted) => { + // 'empty' is the only verdict that licenses a kill, and `counted > 0` reads every one of + // these as empty. The dep is injectable, so it is reachable without asking the daemon. + await expect( + resolve({ + listSessions: async () => counted, + inspectPtyOwnership: ownershipIs('unknown') + }) + ).resolves.toEqual({ state: 'unknown', liveSessions: null }) + }) +}) diff --git a/src/main/daemon/daemon-occupancy.ts b/src/main/daemon/daemon-occupancy.ts new file mode 100644 index 00000000000..31d4c67b40e --- /dev/null +++ b/src/main/daemon/daemon-occupancy.ts @@ -0,0 +1,151 @@ +import { DaemonClient } from './client' +import { inspectDaemonPtyOwnership } from './daemon-live-pty-evidence' +import { PROTOCOL_VERSION, type ListSessionsResult } from './types' + +/** + * How much work a daemon is hosting, and how sure we are. + * + * The distinction that matters: only the daemon itself can prove it is *empty*. + * The OS process table can prove work exists, but a table that shows nothing may + * simply have failed to observe it — so it may only ever add protection, never + * license a kill. 'unknown' is the residual, and it is not permission. + */ +export type DaemonOccupancy = + | { state: 'occupied'; liveSessions: number | null } + | { state: 'empty'; liveSessions: 0 } + | { state: 'unknown'; liveSessions: null } + +export type DaemonOccupancyDeps = { + listSessions?: ( + socketPath: string, + tokenPath: string, + budgetMs: number, + connectBudgetMs: number + ) => Promise + inspectPtyOwnership?: typeof inspectDaemonPtyOwnership +} + +/** + * Why two budgets, not one: connecting and answering fail for different reasons and deserve + * different patience. A daemon that cannot complete a handshake is wedged, and asking again + * shortly is the cheap way to find out whether it recovers — so connect stays tight and the + * caller retries it. A daemon that *did* answer the handshake is demonstrably alive, and its + * session count is the one thing that settles the question outright, so it is worth waiting + * for. Collapsing both into one tight budget is what made a slow-but-answering daemon + * indistinguishable from a dead one, and got its agents killed. + */ +export const OCCUPANCY_CONNECT_BUDGET_MS = 4_000 +export const OCCUPANCY_REQUEST_BUDGET_MS = 15_000 + +/** + * Live session count over the daemon's own socket; null when it could not answer. + * `budgetMs` caps the whole exchange, so a caller working to a deadline can hand over what + * it has left rather than trusting a constant to still fit. + */ +async function countLiveSessionsOverIpc( + socketPath: string, + tokenPath: string, + budgetMs: number, + connectBudgetMs: number +): Promise { + const client = new DaemonClient({ socketPath, tokenPath, protocolVersion: PROTOCOL_VERSION }) + const deadline = Date.now() + budgetMs + const remaining = (): number => Math.max(1, deadline - Date.now()) + try { + await client.ensureConnectedWithin(Math.min(connectBudgetMs, remaining())) + const result = await client.request( + 'listSessions', + undefined, + Math.min(OCCUPANCY_REQUEST_BUDGET_MS, remaining()) + ) + return result.sessions.filter((session) => session.isAlive).length + } catch { + return null + } finally { + client.disconnect() + } +} + +/** + * Ask the daemon first — a reply is authoritative both ways. Only when it cannot + * answer do we fall back to the process table, and then only to *raise* the answer + * to 'occupied'. A blind or empty-looking table stays 'unknown', because a daemon + * too wedged to list its sessions is exactly as likely to be hosting them. + * + * `recordedPid` must already be identity-verified, or the evidence could describe + * a recycled pid's children rather than this daemon's terminals. + */ +export async function resolveDaemonOccupancy(args: { + socketPath: string + tokenPath: string + recordedPid: number | null + /** Ceiling for the whole resolution; defaults to the connect plus request budgets. */ + budgetMs?: number + /** + * How long to spend on the handshake alone. Defaults to the cheap ask. A caller that has + * budget left and no answer yet should raise it: retrying a four-second handshake twelve + * times cannot reach a daemon that consistently needs five, and this path is only reached + * because a three-second health check already timed out — so re-asking on a stricter budget + * than the one that triaged it here can only ever agree with it. + */ + connectBudgetMs?: number + deps?: DaemonOccupancyDeps +}): Promise { + const { socketPath, tokenPath, recordedPid, deps = {} } = args + const budgetMs = args.budgetMs ?? OCCUPANCY_CONNECT_BUDGET_MS + OCCUPANCY_REQUEST_BUDGET_MS + const connectBudgetMs = args.connectBudgetMs ?? OCCUPANCY_CONNECT_BUDGET_MS + const unknown: DaemonOccupancy = { state: 'unknown', liveSessions: null } + // Why catch rather than let it propagate: 'unknown' is this module's residual, and a + // question that could not be asked is the residual's whole purpose. An escaping throw + // would route a failed observation into the launch path instead. + try { + const counted = await (deps.listSessions ?? countLiveSessionsOverIpc)( + socketPath, + tokenPath, + budgetMs, + connectBudgetMs + ) + if (counted !== null) { + // Why validate a number we just asked for: 'empty' is the one verdict that licenses a + // kill, and `counted > 0` quietly reads NaN, -1 and every other non-count as emptiness. + // The dep is injectable, so that is reachable without the daemon ever being asked. + if (!Number.isInteger(counted) || counted < 0) { + return unknown + } + return counted > 0 + ? { state: 'occupied', liveSessions: counted } + : { state: 'empty', liveSessions: 0 } + } + if (recordedPid === null) { + return unknown + } + const ownership = await (deps.inspectPtyOwnership ?? inspectDaemonPtyOwnership)(recordedPid) + return ownership === 'owns-live-ptys' ? { state: 'occupied', liveSessions: null } : unknown + } catch { + return unknown + } +} + +/** + * Raise an unanswered verdict with out-of-band evidence. It can only ever raise: an + * absent or unreadable process table leaves the verdict exactly as it was, because the + * table can prove work exists and never that it does not. + * + * Separate from the IPC path so a caller waiting for the daemon to recover can re-ask it + * cheaply, and pay for the process table once, after the waiting is done. + */ +export async function raiseOccupancyWithProcessEvidence( + occupancy: DaemonOccupancy, + recordedPid: number | null, + deps: DaemonOccupancyDeps = {} +): Promise { + if (occupancy.state !== 'unknown' || recordedPid === null) { + return occupancy + } + try { + const ownership = await (deps.inspectPtyOwnership ?? inspectDaemonPtyOwnership)(recordedPid) + return ownership === 'owns-live-ptys' ? { state: 'occupied', liveSessions: null } : occupancy + } catch { + return occupancy + } +} diff --git a/src/main/daemon/daemon-self-spawned-pty-inventory.test.ts b/src/main/daemon/daemon-self-spawned-pty-inventory.test.ts new file mode 100644 index 00000000000..562947d3748 --- /dev/null +++ b/src/main/daemon/daemon-self-spawned-pty-inventory.test.ts @@ -0,0 +1,45 @@ +import { describe, expect, it } from 'vitest' +import { readFileSync, readdirSync } from 'node:fs' +import { join } from 'node:path' + +/** + * The evidence module decides whether a daemon still hosts user terminals by looking at its + * process tree, and must discount the PTYs the daemon opens for itself. That exclusion list + * is only safe while it is complete — a self-spawned PTY nobody excluded reads as user work + * and holds a daemon that owns nothing, which is how the list grew a reviewer at a time. + * + * So pin the input instead of the list: every PTY the daemon opens *directly*, enumerated + * from the source. A new spawn site fails this test until someone decides which side it + * belongs on. + * + * Scope, stated so the next reader does not over-trust it: this sees node-pty calls in this + * directory only. The daemon can also open a PTY through a helper binary — the macOS login + * session probe shells out to `expect`, whose own `spawn` forkpty's a `login` wrapper that + * surfaces as a session-leader grandchild (`macos-login-session-pty-probe.ts`). That one is + * caught by the stranded-wrapper filter rather than by this list, and it is the shape a + * future escape will take: indirect, and outside this directory. + */ +const KNOWN_DAEMON_PTY_SPAWN_SITES = [ + // The user's terminal — the thing the evidence exists to protect. + { file: 'pty-subprocess.ts', argv: 'wrapped.file, wrapped.args', hosted: true }, + // checkPtySpawnHealth + { file: 'pty-subprocess.ts', argv: "'/bin/sh', ['-c', 'exit 0']", hosted: false }, + // warmWindowsConptyOnce + { file: 'windows-conpty-warmup.ts', argv: "COMSPEC || 'cmd.exe', ['/c', 'exit']", hosted: false } +] + +describe('daemon self-spawned PTY inventory', () => { + it('has no PTY spawn site the ownership evidence has not accounted for', () => { + const daemonDir = join(import.meta.dirname) + const sites = readdirSync(daemonDir) + .filter((name) => name.endsWith('.ts') && !name.includes('.test.')) + .flatMap((name) => { + const source = readFileSync(join(daemonDir, name), 'utf8') + return [...source.matchAll(/(?:pty\.spawn|spawnPty)\s*\(/g)] + .filter((match) => !/typeof pty\.spawn/.test(source.slice(match.index - 80, match.index))) + .map(() => name) + }) + + expect(sites.sort()).toEqual(KNOWN_DAEMON_PTY_SPAWN_SITES.map((site) => site.file).sort()) + }) +}) diff --git a/src/main/daemon/daemon-session-owner-resolution.test.ts b/src/main/daemon/daemon-session-owner-resolution.test.ts index 55dc6142658..b7765ab8583 100644 --- a/src/main/daemon/daemon-session-owner-resolution.test.ts +++ b/src/main/daemon/daemon-session-owner-resolution.test.ts @@ -21,6 +21,40 @@ function provider( } describe('DaemonSessionOwnerResolver', () => { + it('reattaches a held daemon\u2019s session once the daemon stops being wedged', async () => { + // Pins a promise the degraded notice makes to the user: "reopening a pane retries, and works + // once it does". In held mode discovery ran over the same IPC the daemon was failing, so no + // route was ever recorded — recovery therefore cannot come from a cached route. It has to + // come from the next attach re-inventorying a provider whose failure cooldown has expired. + const session = 'wt-1@@pane-1' + let wedged = true + const daemonInventory = vi.fn(async () => { + if (wedged) { + throw new Error('Hello response timed out') + } + return [{ id: session, cwd: '/repo' }] as PtyProcessInfo[] + }) + const fallback = provider(async () => []) + const daemon = provider(daemonInventory, async (opts) => ({ + id: opts.sessionId as string, + isReattach: true + })) + const resolver = new DaemonSessionOwnerResolver([fallback, daemon], new Map()) + + // While wedged the session cannot be proven to belong to anyone, and the resolver refuses + // rather than letting the fallback answer with a fresh shell. + await expect( + resolver.spawnAttachOnly({ sessionId: session, attachOnly: true } as never) + ).rejects.toBeInstanceOf(TerminalSessionOwnerUnverifiedError) + + wedged = false + await new Promise((resolve) => setTimeout(resolve, 1_100)) // outlast FAILED_PROVIDER_COOLDOWN_MS + + const result = await resolver.spawnAttachOnly({ sessionId: session, attachOnly: true } as never) + expect(result.id).toBe(session) + expect(daemon.spawn).toHaveBeenCalledTimes(1) + }) + it('coalesces complete multi-provider absence without dispatching an attach', async () => { let releaseFallback!: (processes: PtyProcessInfo[]) => void let releaseCurrent!: (processes: PtyProcessInfo[]) => void diff --git a/src/main/daemon/daemon-spawner.ts b/src/main/daemon/daemon-spawner.ts index 6a6d377cc2f..af1f87f1661 100644 --- a/src/main/daemon/daemon-spawner.ts +++ b/src/main/daemon/daemon-spawner.ts @@ -28,8 +28,16 @@ export type DaemonPidFile = { spawnerExecPath?: string } +/** + * 'degraded-new-pty-fallback' — adopted, but it cannot spawn fresh PTYs. + * 'held' — deliberately kept without adopting it, because replacing it might end live work: + * either it demonstrably owns terminals and cannot answer a handshake, or it could not be + * classified at all. Both mean there is no lease to take, and none must be attempted. + */ +export type DaemonLaunchMode = 'degraded-new-pty-fallback' | 'held' + export type DaemonProcessHandle = { - mode?: 'degraded-new-pty-fallback' + mode?: DaemonLaunchMode releaseAdoptionLease?(): void shutdown(): Promise } diff --git a/src/main/daemon/degraded-daemon-fresh-spawn-routing.test.ts b/src/main/daemon/degraded-daemon-fresh-spawn-routing.test.ts new file mode 100644 index 00000000000..6044957696c --- /dev/null +++ b/src/main/daemon/degraded-daemon-fresh-spawn-routing.test.ts @@ -0,0 +1,241 @@ +import { describe, expect, it, vi } from 'vitest' +import { + DEGRADED_DAEMON_RECOVERY_RETRY_MS, + DegradedDaemonFreshSpawnRouter +} from './degraded-daemon-fresh-spawn-routing' +import type { IPtyProvider, PtySpawnResult } from '../providers/types' +import { DaemonProtocolError } from './daemon-errors' + +function provider(id: string, spawn?: IPtyProvider['spawn']): IPtyProvider { + return { + spawn: spawn ?? vi.fn(async () => ({ id: `${id}-pty` }) as PtySpawnResult) + } as unknown as IPtyProvider +} + +function router(opts: { + probe?: (() => Promise) | null + currentSpawn?: IPtyProvider['spawn'] +}) { + const current = provider('current', opts.currentSpawn) + const fallback = provider('fallback') + const sessionProviders = new Map() + return { + current, + fallback, + sessionProviders, + router: new DegradedDaemonFreshSpawnRouter( + current, + fallback, + sessionProviders, + opts.probe === undefined ? async () => true : opts.probe + ) + } +} + +describe('DegradedDaemonFreshSpawnRouter', () => { + it('starts on the fallback, so a held daemon never receives a fresh spawn', () => { + expect(router({}).router.routesToFallback).toBe(true) + }) + + it('promotes fresh spawns back to the daemon once it answers a health check', async () => { + const { router: r } = router({ probe: async () => true }) + + await expect(r.recover()).resolves.toBe(true) + expect(r.routesToFallback).toBeUndefined() + }) + + it('stays on the fallback while the daemon is still unhealthy', async () => { + const { router: r } = router({ probe: async () => false }) + + await expect(r.recover()).resolves.toBe(false) + expect(r.routesToFallback).toBe(true) + }) + + it('routes back to the fallback when a spawn fails after recovery', async () => { + // The defect: recovery was a one-way flip on a two-way condition. A daemon that answers one + // health check and wedges again kept every later fresh spawn pointed at it, and each one + // costs a hello timeout plus a full launcher re-classification — per terminal, for the rest + // of the session. + const wedged = vi.fn(async () => { + throw new DaemonProtocolError('Hello response timed out') + }) + const { router: r } = router({ probe: async () => true, currentSpawn: wedged }) + + await r.recover() + expect(r.routesToFallback).toBeUndefined() + + await expect(r.spawn({ cwd: '/tmp' } as never)).rejects.toThrow('Hello response timed out') + expect(r.routesToFallback).toBe(true) + }) + + it('does not immediately re-promote after routing back', async () => { + // Without re-arming the cooldown the next spawn probes again straight away, and a wedged + // daemon that still passes a cheap health check would be re-promoted into the same failure. + const wedged = vi.fn(async () => { + throw new DaemonProtocolError('Hello response timed out') + }) + const { router: r } = router({ probe: async () => true, currentSpawn: wedged }) + + await r.recover() + await expect(r.spawn({ cwd: '/tmp' } as never)).rejects.toThrow() + + await expect(r.recover()).resolves.toBe(false) + expect(r.routesToFallback).toBe(true) + + const past = vi + .spyOn(Date, 'now') + .mockReturnValue(Date.now() + DEGRADED_DAEMON_RECOVERY_RETRY_MS + 1) + try { + await expect(r.recover()).resolves.toBe(true) + } finally { + past.mockRestore() + } + }) + + it('never lets a retry of a named session be answered by the fallback', async () => { + // The dangerous case `!mapped` could not see: a spawn that names a session may already have + // created it on the daemon and then lost the reply. Demoting on that failure would send the + // retry to the fallback, which answers with a fresh local shell under the same id while the + // agent keeps running on the daemon — the pane binds to the shell and the agent is orphaned. + // That is the symptom this whole change exists to prevent, arriving by another door. + const lostReply = vi.fn(async () => { + throw new DaemonProtocolError('Request createSession timed out after 30000ms') + }) + const { + router: r, + current, + sessionProviders + } = router({ + probe: async () => true, + currentSpawn: lostReply + }) + + await r.recover() + await expect(r.spawn({ cwd: '/tmp', sessionId: 'wt-1@@pane-1' } as never)).rejects.toThrow() + + // The identity sticks to the provider that may already own it... + expect(sessionProviders.get('wt-1@@pane-1')).toBe(current) + // ...and keeps routing there even though the shared route has since demoted, which is the + // property that actually prevents the shadow: the pin outranks the route. + expect(r.routesToFallback).toBe(true) + await expect(r.spawn({ cwd: '/tmp', sessionId: 'wt-1@@pane-1' } as never)).rejects.toThrow( + 'Request createSession timed out after 30000ms' + ) + expect(lostReply).toHaveBeenCalledTimes(2) + }) + + it('demotes for a production-shaped fresh spawn, which always carries an id', async () => { + // The regression this pins: gating demotion on the ABSENCE of a sessionId made it + // unreachable outside tests, because every production fresh spawn mints an id before + // reaching the provider (ipc/pty.ts sets spawnOptions.sessionId). A recovered-then-wedged + // daemon would keep every later terminal pointed at itself, each paying a hello timeout + // plus a full launcher re-classification, and each failing anyway. + const wedged = vi.fn(async () => { + throw new DaemonProtocolError('Hello response timed out') + }) + const { router: r, sessionProviders } = router({ + probe: async () => true, + currentSpawn: wedged + }) + + await r.recover() + expect(r.routesToFallback).toBeUndefined() + + // Exactly what ipc/pty.ts sends for a new terminal: a minted id, and no attachOnly. + await expect(r.spawn({ cwd: '/tmp', sessionId: 'wt-1@@pane-9' } as never)).rejects.toThrow( + 'Hello response timed out' + ) + + expect(r.routesToFallback).toBe(true) + // And correctly does NOT pin: a hello that never completed cannot have created a session, + // so there is nothing on the daemon for a retry to collide with. Pinning here would strand + // later attempts on a host holding nothing of theirs. + expect(sessionProviders.has('wt-1@@pane-9')).toBe(false) + }) + + it('does not demote for an attach that names a session', async () => { + // An attachOnly spawn is not a fresh terminal; its failure says nothing about whether the + // next new terminal should go local, and attaches with an id are routed elsewhere anyway. + const wedged = vi.fn(async () => { + throw new DaemonProtocolError('Hello response timed out') + }) + const { router: r } = router({ probe: async () => true, currentSpawn: wedged }) + + await r.recover() + await expect( + r.spawn({ cwd: '/tmp', sessionId: 'wt-1@@pane-9', attachOnly: true } as never) + ).rejects.toThrow() + + expect(r.routesToFallback).toBeUndefined() + }) + + it('does not demote when the failure says nothing about the daemon', async () => { + // A spawn can fail for reasons that are the caller's, not the host's — an unusable cwd, a + // bad profile. Degrading the whole session's persistence for one of those costs the user + // daemon-backed terminals the daemon would have served perfectly well. + const rejected = vi.fn(async () => { + throw new Error('chdir failed: ENOENT /gone') + }) + const { router: r } = router({ probe: async () => true, currentSpawn: rejected }) + + await r.recover() + await expect(r.spawn({ cwd: '/gone' } as never)).rejects.toThrow('chdir failed') + expect(r.routesToFallback).toBeUndefined() + }) + + it('does not pin a session the daemon cannot have created', async () => { + // The pin exists for a request that was sent and whose answer was lost. A failure that never + // reached the daemon created nothing, so pinning it would strand later attempts on a host + // that has nothing of theirs. + const rejected = vi.fn(async () => { + throw new Error('chdir failed: ENOENT /gone') + }) + const { router: r, sessionProviders } = router({ + probe: async () => true, + currentSpawn: rejected + }) + + await r.recover() + await expect(r.spawn({ cwd: '/gone', sessionId: 'wt-1@@pane-3' } as never)).rejects.toThrow() + expect(sessionProviders.has('wt-1@@pane-3')).toBe(false) + }) + + it('still demotes on an anonymous spawn, which cannot be shadowed', async () => { + // The case demotion exists for: no session identity, so there is nothing a fallback answer + // could shadow, and paying a hello timeout plus a re-classification per terminal is pure loss. + const wedged = vi.fn(async () => { + throw new DaemonProtocolError('Hello response timed out') + }) + const { router: r } = router({ probe: async () => true, currentSpawn: wedged }) + + await r.recover() + await expect(r.spawn({ cwd: '/tmp' } as never)).rejects.toThrow() + expect(r.routesToFallback).toBe(true) + }) + + it('keeps a mapped session on its owner while sparing the next terminal', async () => { + // A mapped id names the provider that actually owns that pty, so it must keep routing there + // however the shared route moves. But its failure is still evidence the daemon is failing, + // and the next *fresh* terminal is a different session that cannot be shadowed by this one — + // so it should not have to discover the same timeout for itself. + const wedged = vi.fn(async () => { + throw new DaemonProtocolError('Hello response timed out') + }) + const { + router: r, + current, + sessionProviders + } = router({ + probe: async () => true, + currentSpawn: wedged + }) + sessionProviders.set('session-1', current) + // Promote first, or the assertion below passes on the constructor's default and proves nothing. + await r.recover() + expect(r.routesToFallback).toBeUndefined() + + await expect(r.spawn({ cwd: '/tmp', sessionId: 'session-1' } as never)).rejects.toThrow() + expect(sessionProviders.get('session-1')).toBe(current) + expect(r.routesToFallback).toBe(true) + }) +}) diff --git a/src/main/daemon/degraded-daemon-fresh-spawn-routing.ts b/src/main/daemon/degraded-daemon-fresh-spawn-routing.ts index b1d4661dc93..6b524219bc2 100644 --- a/src/main/daemon/degraded-daemon-fresh-spawn-routing.ts +++ b/src/main/daemon/degraded-daemon-fresh-spawn-routing.ts @@ -1,4 +1,33 @@ import type { IPtyProvider, PtySpawnOptions, PtySpawnResult } from '../providers/types' +import { isDaemonGoneError } from './daemon-pty-adapter' +import { DaemonProtocolError } from './daemon-errors' + +/** client.ts rejects a sent request with this shape once its budget expires. */ +const REQUEST_TIMED_OUT = /timed out after \d+ms/ + +/** + * Only a daemon that looks unreachable should cost the next terminal its persistence. A spawn + * can fail for reasons that say nothing about the daemon's health — an unusable cwd, a bad + * profile — and demoting on those degrades a session the daemon would have served fine. + */ +function daemonLooksUnreachable(error: unknown): boolean { + return ( + isDaemonGoneError(error) || + (error instanceof DaemonProtocolError && REQUEST_TIMED_OUT.test(error.message)) + ) +} + +/** + * Only a request that was actually sent can hide a session the daemon created before the answer + * was lost. A failure that never reached it cannot have created anything, so pinning that id + * would strand later attempts on a daemon that has nothing of theirs. + */ +function mayHaveCreatedTheSession(error: unknown): boolean { + return ( + error instanceof DaemonProtocolError && + (error.message === 'Connection lost' || REQUEST_TIMED_OUT.test(error.message)) + ) +} export const DEGRADED_DAEMON_RECOVERY_RETRY_MS = 30_000 @@ -68,7 +97,40 @@ export class DegradedDaemonFreshSpawnRouter { async spawn(opts: PtySpawnOptions): Promise { const mapped = opts.sessionId ? this.sessionProviders.get(opts.sessionId) : undefined const target = mapped ?? this.target - const result = await target.spawn(opts) + let result: PtySpawnResult + try { + result = await target.spawn(opts) + } catch (error) { + // Why route back: recovery was a one-way flip on a two-way condition. A daemon that + // answers one health check and wedges again kept every later spawn pointed at it, and a + // spawn there costs a hello timeout plus a full launcher re-classification — per terminal, + // for the rest of the session. Sending the next one to the fallback costs a terminal + // without daemon persistence instead, and the next probe can promote it back. + if (target === this.current) { + // Two independent things, and conflating them cost a fix each way. Pinning protects + // THIS id: the spawn may already have created it on the daemon and lost the reply, so + // letting a retry reach the fallback would answer with a local shell under the same id + // while the original keeps running. Demoting protects the NEXT terminal, which is a + // different session entirely and cannot be shadowed by this one. + if (opts.sessionId && mayHaveCreatedTheSession(error)) { + this.sessionProviders.set(opts.sessionId, target) + } + // Why not `!opts.sessionId`: every production fresh spawn mints an id before it gets + // here (ipc/pty.ts assigns spawnOptions.sessionId), so keying the demotion off its + // absence made the demotion unreachable outside tests — and left every later terminal + // paying a hello timeout plus a full re-classification against a daemon already known + // to be failing. `attachOnly` is the real discriminator: an attach that names a session + // never reaches this router at all. + if (opts.attachOnly !== true && daemonLooksUnreachable(error)) { + this.target = this.fallback + this.retryAfterMs = Date.now() + DEGRADED_DAEMON_RECOVERY_RETRY_MS + console.warn( + '[daemon] Fresh terminals routed back to the local provider: the daemon failed a spawn after recovering' + ) + } + } + throw error + } if (!result.exitedBeforeSpawnReply) { this.sessionProviders.set(result.id, target) } diff --git a/src/main/daemon/degraded-daemon-pty-provider.test.ts b/src/main/daemon/degraded-daemon-pty-provider.test.ts index 1982c4add1b..2f76fff6f7b 100644 --- a/src/main/daemon/degraded-daemon-pty-provider.test.ts +++ b/src/main/daemon/degraded-daemon-pty-provider.test.ts @@ -5,6 +5,7 @@ import type { DaemonPtyAdapter } from './daemon-pty-adapter' import type { IPtyProvider, PtySpawnOptions, PtySpawnResult } from '../providers/types' import type { PtyProcessInspection } from '../providers/pty-process-inspection' import { SessionNotFoundError, TerminalSessionOwnerUnverifiedError } from './daemon-errors' +import { isSshPtyNotFoundError } from '../providers/ssh-pty-errors' type ProviderMock = IPtyProvider & { probePtyLiveness: (id: string) => Promise @@ -663,6 +664,27 @@ describe('DegradedDaemonPtyProvider', () => { expect(provider.hasPty('legacy-session')).toBe(true) }) + it('routes every session operation for a mapped daemon session to that adapter', async () => { + const current = createDaemonAdapter('daemon', ['daemon-session']) + const fallback = createProvider('fallback') + const provider = new DegradedDaemonPtyProvider({ current, legacy: [], fallback }) + await provider.discoverDaemonSessions() + + provider.write('daemon-session', 'ls\n') + provider.resize('daemon-session', 120, 40) + await provider.sendSignal('daemon-session', 'SIGINT') + await provider.shutdown('daemon-session', {}) + + expect(current.write).toHaveBeenCalledWith('daemon-session', 'ls\n') + expect(current.resize).toHaveBeenCalledWith('daemon-session', 120, 40) + expect(current.sendSignal).toHaveBeenCalledWith('daemon-session', 'SIGINT') + expect(current.shutdown).toHaveBeenCalledWith('daemon-session', {}) + expect(fallback.write).not.toHaveBeenCalled() + expect(fallback.resize).not.toHaveBeenCalled() + expect(fallback.sendSignal).not.toHaveBeenCalled() + expect(fallback.shutdown).not.toHaveBeenCalled() + }) + it('keeps an exited legacy daemon poisoning listProcesses after construction', async () => { const current = createDaemonAdapter('daemon', ['current-session']) const legacy = createDaemonAdapter('legacy', ['legacy-session']) @@ -679,6 +701,136 @@ describe('DegradedDaemonPtyProvider', () => { }) }) +describe('DegradedDaemonPtyProvider owner gate against an unanswerable fallback', () => { + // STA-3077 made hasPty three-valued: null now means "this provider cannot answer", where + // before the only answers were yes and no. The owner gate asks the fallback to *prove* it owns + // a session before letting it act, and it must read that new null as "not proven" — otherwise + // the in-process fallback answers for a daemon-owned session, the pane closes, and the agent + // keeps running as an orphan. Nothing else exercises null at this boundary: every other double + // answers false, which makes `!== true` and `=== false` indistinguishable. + it('refuses a mutating operation when the fallback cannot answer for the session', async () => { + const current = createDaemonAdapter('daemon') + const fallback = createProvider('fallback') + fallback.hasPty = vi.fn(() => null) + const provider = new DegradedDaemonPtyProvider({ current, legacy: [], fallback }) + + await expect(provider.shutdown('wt-1@@unanswerable', {})).rejects.toBeInstanceOf( + TerminalSessionOwnerUnverifiedError + ) + expect(fallback.shutdown).not.toHaveBeenCalled() + expect(current.shutdown).not.toHaveBeenCalled() + }) + + it('still lets the fallback act on a session it positively claims', async () => { + const current = createDaemonAdapter('daemon') + const fallback = createProvider('fallback') + fallback.hasPty = vi.fn(() => true) + const provider = new DegradedDaemonPtyProvider({ current, legacy: [], fallback }) + + await provider.shutdown('wt-1@@local', {}) + expect(fallback.shutdown).toHaveBeenCalledWith('wt-1@@local', {}) + }) +}) + +describe('DegradedDaemonPtyProvider with a held daemon', () => { + const HELD_SESSION = 'wt-1@@held-daemon-session' + + /** Held launch mode never connects to the wedged daemon, so discovery maps nothing and + * every daemon-owned id is unrouted — i.e. resolves to the in-process fallback. */ + function createHeldDaemonProvider(): { + current: ReturnType + fallback: ReturnType + provider: DegradedDaemonPtyProvider + } { + const current = createDaemonAdapter('daemon') + const fallback = createProvider('fallback') + return { + current, + fallback, + provider: new DegradedDaemonPtyProvider({ current, legacy: [], fallback }) + } + } + + it('rejects shutdown for a held daemon session instead of reporting a silent success', async () => { + const { current, fallback, provider } = createHeldDaemonProvider() + await provider.discoverDaemonSessions() + + // Why: the fallback's shutdown resolves for ids it never had, so the pane would close + // while the daemon's agent keeps running as an orphan. + await expect(provider.shutdown(HELD_SESSION, {})).rejects.toBeInstanceOf( + TerminalSessionOwnerUnverifiedError + ) + expect(fallback.shutdown).not.toHaveBeenCalled() + expect(current.shutdown).not.toHaveBeenCalled() + }) + + it('does not let the kill path mistake an unreachable owner for an already-gone pty', async () => { + const { provider } = createHeldDaemonProvider() + await provider.discoverDaemonSessions() + // Mirrors pty:kill's isPtyAlreadyGoneError (src/main/ipc/pty.ts), which is not exported: + // any error matching it is swallowed into a synthesized pty:exit and reported as success — + // exactly the orphan-hiding lie this routing exists to prevent. Renaming the thrown error + // back into that shape would silently reintroduce it. + const looksAlreadyGoneToPtyKill = (error: unknown): boolean => + isSshPtyNotFoundError(error) || + /Session not found/i.test(error instanceof Error ? error.message : String(error)) + + const error = await provider.shutdown(HELD_SESSION, {}).catch((err: unknown) => err) + + expect(error).toBeInstanceOf(TerminalSessionOwnerUnverifiedError) + expect(looksAlreadyGoneToPtyKill(error)).toBe(false) + }) + + it('throws on write and resize for a held daemon session instead of swallowing input', async () => { + const { fallback, provider } = createHeldDaemonProvider() + await provider.discoverDaemonSessions() + + // Why: the fallback's write/resize are `ptyProcesses.get(id)?.…` — typing would vanish. + expect(() => provider.write(HELD_SESSION, 'ls\n')).toThrow(TerminalSessionOwnerUnverifiedError) + expect(() => provider.resize(HELD_SESSION, 120, 40)).toThrow( + TerminalSessionOwnerUnverifiedError + ) + expect(fallback.write).not.toHaveBeenCalled() + expect(fallback.resize).not.toHaveBeenCalled() + }) + + it('rejects sendSignal for a held daemon session', async () => { + const { fallback, provider } = createHeldDaemonProvider() + await provider.discoverDaemonSessions() + + await expect(provider.sendSignal(HELD_SESSION, 'SIGINT')).rejects.toBeInstanceOf( + TerminalSessionOwnerUnverifiedError + ) + expect(fallback.sendSignal).not.toHaveBeenCalled() + }) + + it('keeps refusing attach for a held daemon session', async () => { + const { fallback, provider } = createHeldDaemonProvider() + await provider.discoverDaemonSessions() + + await expect(provider.attach(HELD_SESSION)).rejects.toBeInstanceOf(SessionNotFoundError) + expect(fallback.attach).not.toHaveBeenCalled() + }) + + it('still routes every operation for a locally spawned session the fallback owns', async () => { + const { current, fallback, provider } = createHeldDaemonProvider() + await provider.discoverDaemonSessions() + const fresh = await provider.spawn({ cols: 80, rows: 24 }) + + provider.write(fresh.id, 'echo hi\n') + provider.resize(fresh.id, 100, 30) + await expect(provider.sendSignal(fresh.id, 'SIGINT')).resolves.toBeUndefined() + await expect(provider.shutdown(fresh.id, {})).resolves.toBeUndefined() + + expect(fallback.write).toHaveBeenCalledWith(fresh.id, 'echo hi\n') + expect(fallback.resize).toHaveBeenCalledWith(fresh.id, 100, 30) + expect(fallback.sendSignal).toHaveBeenCalledWith(fresh.id, 'SIGINT') + expect(fallback.shutdown).toHaveBeenCalledWith(fresh.id, {}) + expect(current.write).not.toHaveBeenCalled() + expect(current.shutdown).not.toHaveBeenCalled() + }) +}) + // A memoized route outlives the session it was established for: listProcesses // drops ids missing from an authoritative inventory without an exit fanout. So a // mapped owner that cannot answer must stay unknown — coercing it to a liveness diff --git a/src/main/daemon/degraded-daemon-pty-provider.ts b/src/main/daemon/degraded-daemon-pty-provider.ts index e5b67478812..02706432829 100644 --- a/src/main/daemon/degraded-daemon-pty-provider.ts +++ b/src/main/daemon/degraded-daemon-pty-provider.ts @@ -14,6 +14,7 @@ import type { import { adoptOwningProvider, attachDaemonOwnedSession, + ownerForDaemonOwnedOperation, findDaemonAdapter, listProviderSessionIds } from './degraded-daemon-session-routing' @@ -89,6 +90,11 @@ export class DegradedDaemonPtyProvider implements IPtyProvider { attach = (id: string): ReturnType => attachDaemonOwnedSession(this.providerFor(id), this.fallback, id) + /** Routing for anything that changes or feeds a session; see ownerForDaemonOwnedOperation. */ + private ownerFor(id: string): IPtyProvider { + return ownerForDaemonOwnedOperation(this.providerFor(id), this.fallback, id) + } + hasPty(id: string): boolean | null { const mapped = this.sessionProviders.get(id) if (mapped) { @@ -120,11 +126,11 @@ export class DegradedDaemonPtyProvider implements IPtyProvider { )?.providesAgentSessionOwnerListings?.(ptyId) === true write(id: string, data: string): void { - this.providerFor(id).write(id, data) + this.ownerFor(id).write(id, data) } resize(id: string, cols: number, rows: number): void { - this.providerFor(id).resize(id, cols, rows) + this.ownerFor(id).resize(id, cols, rows) } pauseProducer(id: string): void { @@ -143,14 +149,14 @@ export class DegradedDaemonPtyProvider implements IPtyProvider { id: string, opts: { immediate?: boolean; keepHistory?: boolean; deadlineMs?: number } ): Promise { - await this.providerFor(id).shutdown(id, opts) + await this.ownerFor(id).shutdown(id, opts) if (!opts.keepHistory) { this.sessionProviders.delete(id) } } async sendSignal(id: string, signal: string): Promise { - await this.providerFor(id).sendSignal(id, signal) + await this.ownerFor(id).sendSignal(id, signal) } async getCwd(id: string): Promise { diff --git a/src/main/daemon/degraded-daemon-session-routing.ts b/src/main/daemon/degraded-daemon-session-routing.ts index 7fb5a9a00c8..48aa3870baf 100644 --- a/src/main/daemon/degraded-daemon-session-routing.ts +++ b/src/main/daemon/degraded-daemon-session-routing.ts @@ -1,6 +1,6 @@ import type { IPtyProvider } from '../providers/types' import type { DaemonPtyAdapter } from './daemon-pty-adapter' -import { SessionNotFoundError } from './daemon-errors' +import { SessionNotFoundError, TerminalSessionOwnerUnverifiedError } from './daemon-errors' export function listProviderSessionIds( sessionProviders: ReadonlyMap, @@ -26,6 +26,29 @@ export async function attachDaemonOwnedSession( return await owner.attach(sessionId) } +/** + * Session operations that must never be answered by the in-process fallback on another + * provider's behalf. An unknown id resolves to the fallback, whose shutdown returns + * silently and whose write/resize are no-ops — so a daemon-owned session reads as closed + * while its agent keeps running, and typing into it disappears. Route there only when the + * fallback genuinely owns the pty; otherwise say the session cannot be reached. + * + * Why not SessionNotFoundError: pty:kill treats "Session not found" as proof the pty is + * already gone and synthesizes an exit, which is the same lie by another route. This one + * means "still there, we just cannot reach its host", so the kill is reported as failed and + * ownership is kept for a retry. + */ +export function ownerForDaemonOwnedOperation( + owner: IPtyProvider, + fallback: IPtyProvider, + sessionId: string +): IPtyProvider { + if (owner === fallback && fallback.hasPty?.(sessionId) !== true) { + throw new TerminalSessionOwnerUnverifiedError(sessionId) + } + return owner +} + /** Probes providers for an id absent from the routing map and adopts the * first proven owner into the map. */ export function adoptOwningProvider( diff --git a/src/main/ipc/pty.ts b/src/main/ipc/pty.ts index 4b18f55f7e5..e351a2a4191 100644 --- a/src/main/ipc/pty.ts +++ b/src/main/ipc/pty.ts @@ -7667,6 +7667,9 @@ export function registerPtyHandlers( if (isSupersededPtyId(args.id)) { return } + // Routing refuses a session whose host is unreachable, but sendSignal is async everywhere, + // so that refusal arrives as a rejection rather than a throw — and optional chaining + // short-circuits the whole chain when there is no provider at all. tryGetProviderForPty(args.id) ?.sendSignal(args.id, args.signal) .catch(() => {}) diff --git a/src/renderer/src/components/settings/DaemonDegradedNotice.test.tsx b/src/renderer/src/components/settings/DaemonDegradedNotice.test.tsx new file mode 100644 index 00000000000..e922035b351 --- /dev/null +++ b/src/renderer/src/components/settings/DaemonDegradedNotice.test.tsx @@ -0,0 +1,64 @@ +import React from 'react' +import { renderToStaticMarkup } from 'react-dom/server' +import { describe, expect, it, vi } from 'vitest' +import { DaemonDegradedNotice } from './DaemonDegradedNotice' + +function render(props: Partial> = {}): string { + return renderToStaticMarkup( + React.createElement(DaemonDegradedNotice, { + degraded: true, + isBusy: false, + onRestartDaemon: vi.fn(), + ...props + }) + ) +} + +describe('DaemonDegradedNotice', () => { + it('renders nothing when the daemon is healthy', () => { + // The common case by far; a notice that shows up here would train the user to ignore it. + expect(render({ degraded: false })).toBe('') + }) + + it('warns that new terminals will not survive quitting', () => { + // The consequence the user actually needs, not the mechanism. Degraded mode's real cost is + // that a terminal opened now disappears on quit, and nothing else in the app says so. + const html = render() + expect(html).toContain('role="alert"') + expect(html).toMatch(/aren’t being saved/) + expect(html).toMatch(/close when you quit/) + }) + + it('does not claim the held daemon’s terminals still work', () => { + // They do not. Discovery runs over the same IPC the daemon is failing to answer, so its + // sessions are never routed and attach refuses the fallback rather than answering on the + // daemon's behalf (degraded-daemon-session-routing.ts:23). The processes are alive — which + // is the whole point of holding — but unreachable until it responds. + const html = render() + expect(html).toMatch(/can’t reach those terminals until the host responds/) + expect(html).not.toMatch(/already open keep working/) + // And it must not promise automatic recovery: TerminalErrorToast already tells the user to + // "Reopen this pane to retry", because nothing re-attaches a failed pane on its own. + expect(html).not.toMatch(/reconnect on their own/) + expect(html).toMatch(/reopening a pane retries/) + }) + + it('says the restart ends the local terminals too, not just the held ones', () => { + // Restarting is not free twice over: runRestartDaemon kills the daemon's sessions AND calls + // shutdownFallbackSessions() first (daemon-init.ts), so the terminals the notice just told + // the user are running "outside the host" die as well. Naming only half the cost is how a + // user loses work by clicking the button the banner recommended. + expect(render()).toMatch( + /ends every terminal — both the ones it is still holding and the ones running outside it/ + ) + }) + + it('offers the restart action, disabled while another daemon action runs', () => { + // Matched as an attribute, not a substring: the button's utility classes contain + // `disabled:opacity-50`, so a contains-check passes whether or not it is really disabled. + const disabledAttribute = /]*\sdisabled[=>]/ + expect(render()).toContain('Restart host') + expect(render({ isBusy: true })).toMatch(disabledAttribute) + expect(render({ isBusy: false })).not.toMatch(disabledAttribute) + }) +}) diff --git a/src/renderer/src/components/settings/DaemonDegradedNotice.tsx b/src/renderer/src/components/settings/DaemonDegradedNotice.tsx new file mode 100644 index 00000000000..004fa150745 --- /dev/null +++ b/src/renderer/src/components/settings/DaemonDegradedNotice.tsx @@ -0,0 +1,55 @@ +import { TriangleAlert } from 'lucide-react' +import { Button } from '../ui/button' +import { translate } from '@/i18n/i18n' + +/** + * Degraded mode used to be rare and transient, so a console warning was enough. It is now the + * settled outcome for a daemon the launcher could not classify — it holds one rather than + * killing terminals it might still be hosting — which makes it permanent until the user acts. + * + * This surfaces it beside the Restart action that resolves it. It does not surface it anywhere + * a user who has not opened Settings would see; a status-bar indicator is the obvious next step. + */ +export function DaemonDegradedNotice(props: { + degraded: boolean + isBusy: boolean + onRestartDaemon: () => void +}): React.JSX.Element | null { + if (!props.degraded) { + return null + } + + return ( +
+
+ +
+

+ {translate( + 'auto.components.settings.DaemonDegradedNotice.title', + 'New terminals aren’t being saved' + )} +

+

+ {translate( + 'auto.components.settings.DaemonDegradedNotice.body', + 'The terminal host stopped responding. Orca kept it rather than ending anything it might still be hosting, but it can’t reach those terminals until the host responds again — reopening a pane retries, and works once it does. New terminals open outside the host and close when you quit Orca. Restarting the host usually clears this, and ends every terminal — both the ones it is still holding and the ones running outside it.' + )} +

+
+
+ +
+ ) +} diff --git a/src/renderer/src/components/settings/ManageSessionsSection.tsx b/src/renderer/src/components/settings/ManageSessionsSection.tsx index 73256687792..40b837344a9 100644 --- a/src/renderer/src/components/settings/ManageSessionsSection.tsx +++ b/src/renderer/src/components/settings/ManageSessionsSection.tsx @@ -8,6 +8,7 @@ import { activateAndRevealWorktree } from '@/lib/worktree-activation' import { activateTabAndFocusPane } from '@/lib/activate-tab-and-focus-pane' import { useDaemonActions, DaemonActionDialog } from '../shared/useDaemonActions' import { ManageSessionKillDialog } from './ManageSessionKillDialog' +import { DaemonDegradedNotice } from './DaemonDegradedNotice' import { ManageSessionsTable } from './ManageSessionsTable' import { notifyDaemonSessionInventoryInvalidated } from '../status-bar/daemon-session-inventory-invalidation' import { @@ -20,6 +21,7 @@ type ConfirmKind = 'killOne' export function ManageSessionsSection(): React.JSX.Element { const [sessions, setSessions] = useState([]) + const [isDaemonDegraded, setIsDaemonDegraded] = useState(false) const [isRefreshing, setIsRefreshing] = useState(true) const [hasLoadedOnce, setHasLoadedOnce] = useState(false) const [pendingKillSession, setPendingKillSession] = useState(null) @@ -81,6 +83,7 @@ export function ManageSessionsSection(): React.JSX.Element { if (!isMounted.current || mutationInFlight.current) { return result.sessions } + setIsDaemonDegraded(result.degraded === true) setSessions(result.sessions) return result.sessions } catch (err) { @@ -109,6 +112,19 @@ export function ManageSessionsSection(): React.JSX.Element { void refresh() }, [refresh]) + // Why refetch on focus: the degraded flag is computed in the main process and never pushed. + // DegradedDaemonFreshSpawnRouter.recover() clears it the moment the daemon answers a health + // check, so a banner rendered at mount can outlive the condition — and it arms a Restart that + // ends every live session. Matches TerminalTccAttributionNotice, which refetches for the same + // reason: a daemon restart or drain changes the verdict without a pane remount. + useEffect(() => { + const onFocus = (): void => { + void refresh() + } + window.addEventListener('focus', onFocus) + return () => window.removeEventListener('focus', onFocus) + }, [refresh]) + const sessionCount = sessions.length const daemonActions = useDaemonActions({ @@ -218,6 +234,11 @@ export function ManageSessionsSection(): React.JSX.Element { showManageSessionsButton={false} refreshRevision={attributionRefreshRevision} /> + daemonActions.setPending('restart')} + /> {}) connected = false ptyId = null unregisterPtyHandlers(id) diff --git a/src/renderer/src/i18n/locales/en.json b/src/renderer/src/i18n/locales/en.json index 0b4b0981957..ef59f068538 100644 --- a/src/renderer/src/i18n/locales/en.json +++ b/src/renderer/src/i18n/locales/en.json @@ -10676,6 +10676,11 @@ "disconnectFailed": "Could not remove the saved Bitbucket credential." } } + }, + "DaemonDegradedNotice": { + "title": "New terminals aren’t being saved", + "body": "The terminal host stopped responding. Orca kept it rather than ending anything it might still be hosting, but it can’t reach those terminals until the host responds again — reopening a pane retries, and works once it does. New terminals open outside the host and close when you quit Orca. Restarting the host usually clears this, and ends every terminal — both the ones it is still holding and the ones running outside it.", + "action": "Restart host" } }, "right": { diff --git a/src/renderer/src/lib/setup-script-prompt.test.ts b/src/renderer/src/lib/setup-script-prompt.test.ts index 605142bc154..3ce55848f1c 100644 --- a/src/renderer/src/lib/setup-script-prompt.test.ts +++ b/src/renderer/src/lib/setup-script-prompt.test.ts @@ -220,10 +220,7 @@ describe('setup script prompt inspection', () => { getSetupScriptPromptDismissalKey(remoteIdentity) ] expect( - filterSetupScriptPromptDismissalsToValidRepos( - input, - new Set([localIdentity, remoteIdentity]) - ) + filterSetupScriptPromptDismissalsToValidRepos(input, new Set([localIdentity, remoteIdentity])) ).toBe(input) }) diff --git a/src/renderer/src/store/slices/github.ts b/src/renderer/src/store/slices/github.ts index 24501bc23e9..299e3e21bbb 100644 --- a/src/renderer/src/store/slices/github.ts +++ b/src/renderer/src/store/slices/github.ts @@ -2787,9 +2787,7 @@ export const createGitHubSlice: StateCreator = (s ...(errorForCache ? { error: - errorUnchanged && previousError !== undefined - ? previousError - : errorForCache + errorUnchanged && previousError !== undefined ? previousError : errorForCache } : {}), ...(nextFellBack ? { issueSourceFellBack: true } : {}) diff --git a/src/renderer/src/store/slices/repos-refresh-identity.test.ts b/src/renderer/src/store/slices/repos-refresh-identity.test.ts index 193bd045ede..779f9e8d04e 100644 --- a/src/renderer/src/store/slices/repos-refresh-identity.test.ts +++ b/src/renderer/src/store/slices/repos-refresh-identity.test.ts @@ -421,9 +421,11 @@ describe('SSH readoption catalog identity', () => { expect(oldSetup).toBeDefined() expect(newSetup).toBeDefined() - store.getState().recordSshRepoReadoptions([ - { oldTargetId: 'ssh-old', newTargetId: 'ssh-new', repoIds: [repo.id] } - ]) + store + .getState() + .recordSshRepoReadoptions([ + { oldTargetId: 'ssh-old', newTargetId: 'ssh-new', repoIds: [repo.id] } + ]) const next = store.getState().projectHostSetups expect(next).not.toBe(setups) @@ -435,4 +437,3 @@ describe('SSH readoption catalog identity', () => { expect(store.getState().pendingSshRepoReadoptions).toEqual([]) }) }) - diff --git a/src/renderer/src/store/slices/repos.ts b/src/renderer/src/store/slices/repos.ts index 4020b199b04..5a76fbf6963 100644 --- a/src/renderer/src/store/slices/repos.ts +++ b/src/renderer/src/store/slices/repos.ts @@ -3571,7 +3571,9 @@ export const createRepoSlice: StateCreator = (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(() => {}) } } } diff --git a/tests/e2e/helpers/remote-pane-launch-transcript.ts b/tests/e2e/helpers/remote-pane-launch-transcript.ts index 9bab298e604..94918b1b69b 100644 --- a/tests/e2e/helpers/remote-pane-launch-transcript.ts +++ b/tests/e2e/helpers/remote-pane-launch-transcript.ts @@ -58,10 +58,7 @@ export function readRemotePaneLaunchTranscript(target: DockerSshRelayTarget): st } /** The pids the host launched a shell for under one pane key. */ -export function readRemotePaneLaunchPids( - target: DockerSshRelayTarget, - paneKey: string -): number[] { +export function readRemotePaneLaunchPids(target: DockerSshRelayTarget, paneKey: string): number[] { return readRemotePaneLaunchTranscript(target) .filter((line) => line.split('\t')[0] === paneKey) .map((line) => Number(line.split('\t')[1]))