diff --git a/.agents/skills/verify-session-lifecycle/SKILL.md b/.agents/skills/verify-session-lifecycle/SKILL.md index 53886fa8..585af229 100644 --- a/.agents/skills/verify-session-lifecycle/SKILL.md +++ b/.agents/skills/verify-session-lifecycle/SKILL.md @@ -44,7 +44,7 @@ Yes, when the diff changes any of these: - `src/host/programGroup.ts`: the token capture (`captureProgram`, `captureToken`, `readAutogroup`, `parseAutogroup`, `parseProcStat`), the `/proc` reads (`readProcFileIfExists`, `readLiveThreadState`, `readProcStat`, `readShownProcStat`, `isRefusedRead`, `readBootId`), the record (`writeProgramRecord`, `readProgramRecord`, `readProvenProgramGroup`, `readProgramGroup`), the verdict (`judgeProgramGroup`, `readGroupMembers`), and the kill and the probe (`stopProgramGroup`, `isProgramGroupAlive`, `processGroupTarget`, `isProcessGroupAlive` with `groupHasLiveProcess` and `procHidesOtherUsers`, `killProcessGroupBestEffort`, `signalProcessGroup`). - `src/host/hostMain.ts`: `waitForManifestFromCreate`, `isSessionCommandable`, and in `runHost` the start-up statements (the manifest wait, the `hostPid === process.pid` check, the `host.json` write with `captureHost` and `writeHostRecord`, `createSerialManifestWriter`, `EventLog.open`, `createPty`, `setChildPid`, `captureProgram`), `hangUpProgram` with its kill timer and the `SHUTDOWN_KILL_GRACE_MS` constant, `startIdlePolling` (the idle time-out), `initiateShutdown`, `startShutdown`, `handlePtyExit` with its wait for the program's group, `pty.onExit`, the `SIGTERM` handler, the `resize` and `destroy` handlers, and the last `try` block (the `program.json` write, the start-up manifest write and `rpcServer.listen`). - `src/host/manifestWriter.ts` `createSerialManifestWriter`. -- `src/cli/commands/gc.ts` `gcSessions`, `src/cli/commands/create.ts` `runCreateCommand`, and `src/cli/commands/destroy.ts` `runDestroyCommand` with `rerunInNewSession`. +- `src/cli/commands/gc.ts` `gcSessions`, `src/cli/commands/create.ts` `runCreateCommand` with `waitForReadyHost`, and `src/cli/commands/destroy.ts` `runDestroyCommand` with `rerunInNewSession`. - `src/host/sessionState.ts` `setDimensions`, `requestDestroy`, `recordDestroyed` and `recordExit`, `src/host/eventLog.ts` `EventLog.open`, `src/host/rpcServer.ts` `listen` and `close`, `src/host/rpcClient.ts` `toTransportCliError` (which socket errors read as `HOST_UNREACHABLE`). - `src/host/hostRecord.ts`: `captureHost`, `writeHostRecord`, `readHostRecord`, `pidExists`, `judgeHost`, `inPidNamespace`, `readTimeNamespace`, `inTimeNamespace`, `readMachineId`, `inHostPidSpace` and `sharesHostPids`. - `src/storage/manifests.ts` `readValidatedJsonFile`, `replaceFileAtomic`, `replaceFileAtomicSync`, `writeValidatedJsonFileSync`, `writeManifest`, `writeDestroyMarker`. @@ -371,7 +371,7 @@ ps -eo pgid=,args= | awk -v g="$pgid" '$1 == g' # prints nothing: the l Without the guard, `runCreateCommand` gave up after 50 failed `inspect` calls, about 6 seconds, and reported `HOST_UNREACHABLE` (exit 6) without stopping anything. A host that started slowly, on a loaded machine or after a pause, then listened later, and the session ran although `create` had failed (coder/agent-tty#209). The `reconcileSession` call and the `HOST_TIMEOUT` error after the loop never ran, because the last attempt rethrew its error, and the reconcile would have skipped a live host anyway. -Now the last failed attempt calls `host.stop()`, which sends the host SIGTERM through the `ChildProcess` handle that `launchHost` returns, and then rethrows the same error, so the error code does not change. The handle sends nothing once the host has exited, so a reused pid never gets the signal. The host stops its program and records `destroyed` once the group is gone, as for any SIGTERM. `create` writes nothing itself: before the host's start-up write names the program, and without a token, no file names a group that a CLI could prove, so a forced destroy from `create` would record `destroyed` while the program runs. `runHost` registers its SIGTERM handler before it waits for the manifest. A SIGTERM before the spawn makes it return without spawning, and one after the spawn waits for the synchronous start-up block, so the default action can no longer end the host between the spawn and `program.json`. In the model, the give-up sends that SIGTERM, which `HostGetsSigterm` takes. The replay holds the host's start-up write past the readiness loop. +Now every error of the readiness wait (`waitForReadyHost`), the last failed attempt included, makes `runCreateCommand` call `host.stop()`, which sends the host SIGTERM through the `ChildProcess` handle that `launchHost` returns. It then rethrows the error with `details.sessionId` added, so the error code does not change. The handle sends nothing once the host has exited, so a reused pid never gets the signal. The host stops its program and records `destroyed` once the group is gone, as for any SIGTERM. `create` writes nothing itself: before the host's start-up write names the program, and without a token, no file names a group that a CLI could prove, so a forced destroy from `create` would record `destroyed` while the program runs. `runHost` registers its SIGTERM handler before it waits for the manifest, and `facts.mjs` checks that order. A SIGTERM before the spawn makes it return without spawning, and one after the spawn waits for the synchronous start-up block, so the default action can no longer end the host between the spawn and `program.json`. In the model, the give-up sends that SIGTERM, which `HostGetsSigterm` takes. The replay holds the host's start-up write past the readiness loop. ```sh LOG=$(mktemp) @@ -525,6 +525,7 @@ It assumes: - A destroy that lands between reconcile's last read and its write is still overwritten with failed, for the time until that reconcile finds the destroy marker and writes destroyed again (`ReconcileRepairsDestroyed`). A `list` or `gc` that reads in that time sees `failed`, a terminal status, so gc can remove the folder as it would remove a destroyed one. A reconcile whose process dies in that time leaves `failed` until the next reconcile reads it: that read finds the marker and writes destroyed (`ReconcileRepairsMarked`). `home list` and the dashboard count a session with the marker as destroyed, also in that time. The model has no crash of a reconciling process, so it checks that the repair at the read breaks no rule, not that it heals the crash. - The idle time-out calls the same `hangUpProgram` as `initiateShutdown`: SIGHUP, then the group kill after `SHUTDOWN_KILL_GRACE_MS`. When the program exits on that SIGHUP, `handlePtyExit` kills the group too, so a process that ignores SIGHUP does not outlive the idle time-out. An idle time-out still ends exited. When the check cannot run (`unknown`), the idle kill sends nothing, and the session ends exited with that process running, as after a program that exits on its own and leaves a process (coder/agent-tty#222). The model leaves both group kills out: they go through `stopProgramGroup` like every kill, and a process that outlives them is one that the program left, which the model keeps after a normal exit anyway. - Windows has no process groups, so there `processGroupTarget` gives the program's pid, and every kill reaches only that process. `destroy` never re-runs on Windows. There SIGTERM ends the host at once, so a `create` that gives up stops its host only once the manifest names the program, and the next reconcile records `failed` and ends the program. Before that write, `create` leaves the host alone. `CreateStopsOnGiveUp` checks the stop for Linux and macOS. +- A host stopped with SIGSTOP, or with a blocked event loop, takes the SIGTERM of a `create` that gave up only when it runs again. Until then its program runs and the session reads `running`, which R17 allows because the SIGTERM is still on its way. A plain `destroy` fails because the host does not answer, and only `destroy --force` ends such a session sooner. - A destroy that the program runs, or that someone types into the program's interactive shell, runs inside the program's session, and `rerunInNewSession` runs it again in a new session. The model lets such a destroy start while the program's leader runs, and assumes that it kept `AGENT_TTY_SESSION_ID`, which every process of the program inherits at spawn. A destroy whose environment lost the variable, or whose `create --env` overrode it, is seen through the id comparison only. On Linux that compares the CLI's session id with `childPid`, so it misses such a destroy only before the host's start-up write. Other POSIX systems compare the process group and the controlling terminal, through `/bin/ps`. A terminal is the controlling terminal of one session and of every process in it, so that also finds a job of an interactive shell, which runs in a group of its own. A process without a controlling terminal is seen only through the group. - The program's processes stay in its process group. A process that leaves the group (`setsid`, or the job control of an interactive shell) is out of reach of the group kill, and it is no witness for the token either. That is case 4 of coder/agent-tty#203, a documented limit with no code change. - A process that the program left when it exited on its own stays in the program's group and session, so it carries the token. `destroy` of the terminal session kills it through `program.json` (case 1 of coder/agent-tty#203, coder/agent-tty#204). Nothing else stops it: the host does not kill at a normal exit, and gc removes `program.json` with the folder, so a process that a session removed by gc left stays unproven. The model drops a process of another user at a normal exit. diff --git a/.agents/skills/verify-session-lifecycle/coverage.json b/.agents/skills/verify-session-lifecycle/coverage.json index 2687578b..ce0072df 100644 --- a/.agents/skills/verify-session-lifecycle/coverage.json +++ b/.agents/skills/verify-session-lifecycle/coverage.json @@ -1,6 +1,7 @@ { "fingerprints": { - "src/cli/commands/create.ts#runCreateCommand": "5135dd8d24063c90", + "src/cli/commands/create.ts#runCreateCommand": "635715a49fa0273e", + "src/cli/commands/create.ts#waitForReadyHost": "a7fd2ba345b6bfd6", "src/cli/commands/destroy.ts#runDestroyCommand": "ecaa8b1d572da7c6", "src/cli/commands/destroy.ts#rerunInNewSession": "cf32d7d082758522", "src/cli/commands/gc.ts#gcSessions": "667e4ebc3ca3029c", @@ -90,7 +91,7 @@ "src/host/hostMain.ts#runHost>handlers.resize": "d601680963309542", "src/host/hostMain.ts#runHost>handlers.destroy": "ccff44951e711dac", "skill:model/SessionLifecycle.tla": "f7270faba013acff", - "skill:scripts/facts.mjs": "6ae4fd3279517f42", + "skill:scripts/facts.mjs": "b336aed63c9e6cc3", "skill:scripts/check.sh": "3a47553300cb217a" }, "callSites": { @@ -126,6 +127,7 @@ "src/cli/commandTarget.ts#resolveCommandTarget", "src/cli/commands/create.ts#readCreateResult", "src/cli/commands/create.ts#runCreateCommand", + "src/cli/commands/create.ts#waitForReadyHost", "src/cli/commands/gc.ts#gcSessions", "src/cli/commands/inspect.ts#runInspectCommand", "src/cli/commands/record-diff.ts#resolveSessionDirectory", diff --git a/.agents/skills/verify-session-lifecycle/scripts/facts.mjs b/.agents/skills/verify-session-lifecycle/scripts/facts.mjs index aaec7eaa..a25d8369 100755 --- a/.agents/skills/verify-session-lifecycle/scripts/facts.mjs +++ b/.agents/skills/verify-session-lifecycle/scripts/facts.mjs @@ -249,45 +249,40 @@ function createFacts() { }; } -// In runCreateCommand's readiness loop, the catch of an unreachable host -// ends with host.stop() right after its last retry and before it rethrows -// the error. Only a win32 create skips the stop while the manifest names -// no program, so on Linux and macOS, which the model covers, it always stops. launchHost's stop sends SIGTERM through the ChildProcess -// handle, and runHost registers its SIGTERM handler before it spawns the -// program and skips the spawn when a SIGTERM came first. +// In runCreateCommand, the readiness wait is the only statement of a try +// whose catch calls host.stop() and then rethrows, so every error of the +// readiness wait stops the host. Only a win32 create skips the stop while the +// manifest names no program, so on Linux and macOS, which the model covers, +// it always stops. launchHost's stop sends SIGTERM through the ChildProcess +// handle, and runHost registers its SIGTERM handler before it waits for the +// manifest, notes a SIGTERM until the spawn, and skips the spawn when a +// SIGTERM came first. function stopsOnGiveUp(body) { - let stops = false; - walk(body, (node) => { - if (!ts.isCatchClause(node)) { - return; - } - const last = node.block.statements.at(-1); - if (last === undefined || normalize(last.getText()) !== 'throw error;') { - return; + const stops = body.statements.some((s) => { + if (!ts.isTryStatement(s) || s.catchClause === undefined) { + return false; } - walk(node.block, (inner) => { - if (!ts.isIfStatement(inner) || !ts.isBlock(inner.thenStatement)) { - return; - } - const st = inner.thenStatement.statements.map((s) => - normalize(s.getText()), - ); - const retry = st.findIndex((s) => - s.startsWith('if (attempt + 1 < READINESS_MAX_ATTEMPTS) {'), - ); - stops ||= - retry >= 0 && - retry === st.length - 3 && - st[retry + 1].startsWith('invariant(host !== undefined,') && - st[retry + 2] === - "if (process.platform !== 'win32' || manifest?.childPid != null) { host.stop(); }"; - }); + const tried = s.tryBlock.statements.map((t) => normalize(t.getText())); + const st = s.catchClause.block.statements.map((t) => + normalize(t.getText()), + ); + return ( + tried.length === 1 && + tried[0] === + 'result = await waitForReadyHost(socketFile, sessionDirectory, sessionId);' && + st.length === 4 && + st[0].startsWith('invariant(host !== undefined,') && + st[2] === + "if (process.platform !== 'win32' || manifest?.childPid != null) { host.stop(); }" && + st[3] === 'throw withSessionId(error, sessionId);' + ); }); const launch = normalize( requireDeclaration('src/host/lifecycle.ts', 'launchHost').getText(), ); const run = requireDeclaration('src/host/hostMain.ts', 'runHost').body; const handler = indexOfStatement(run, /^process\.on\('SIGTERM'/); + const wait = indexOfStatement(run, /= await waitForManifestFromCreate\(/); const skip = indexOfStatement( run, /^if \(sigterm\.received\) \{\s*await eventLog\.close\(\);\s*return;\s*\}$/, @@ -297,7 +292,8 @@ function stopsOnGiveUp(body) { stops && /stop: \(\) => \{ child\.kill\('SIGTERM'\); \}/.test(launch) && handler >= 0 && - handler < skip && + handler < wait && + wait < skip && skip === spawn - 1 ); } @@ -1307,6 +1303,7 @@ function policyFacts() { const COVERED = [ ['src/cli/commands/create.ts', 'runCreateCommand'], + ['src/cli/commands/create.ts', 'waitForReadyHost'], ['src/cli/commands/destroy.ts', 'runDestroyCommand'], ['src/cli/commands/destroy.ts', 'rerunInNewSession'], ['src/cli/commands/gc.ts', 'gcSessions'], diff --git a/docs/USAGE.md b/docs/USAGE.md index 57565482..ec649940 100644 --- a/docs/USAGE.md +++ b/docs/USAGE.md @@ -323,10 +323,16 @@ A reconcile that writes over the record at the same moment finds the file right A `list`, `inspect` or `gc` that reads the session between those two writes sees `failed`, and `home list` and the dashboard count a session that has ended and has that file as destroyed. If the reconciling process is killed between them, the next reconcile finds the file and records the session as `destroyed`. When that repair fails, for example on a full disk, `list` and `inspect` show the session as it reads. If `destroy` fails after it wrote the file, for example because its manifest write fails, the file stays, and the next reconcile records the session as `destroyed`, also when the session had already ended. Nothing deletes the file before `gc` removes the session folder, because a reconcile that read before the destroy can still write after it. A host that died after it recorded the session as ended leaves its socket. `gc`, and a `destroy` of a `destroyed` session, remove that socket when the manifest names no host pid or the host check below reads the host as ended. -When `create` cannot reach its new host within about 6 s, it fails with `HOST_UNREACHABLE` or `HOST_TIMEOUT` and tells the host to stop. +When `create` cannot reach its new host, it fails and tells the host to stop. It fails with `HOST_UNREACHABLE` (exit 6) after about 6 s when nothing listens on the socket, and with `HOST_TIMEOUT` (exit 5) after about 10 s when the host accepts the connection but does not answer. +`create` also tells the host to stop when it fails for another reason after it wrote the session's first manifest, for example when it cannot read that manifest again. Each such error names the session in `details.sessionId`. +When that first manifest write fails, `create` removes the session folder. The host waits for the manifest only while `create` runs, so it then exits without starting the program. On Linux and macOS it sends the host SIGTERM, and the host stops the program the way `destroy` does and records the session as `destroyed` once the program's group has ended, also while its start-up is still slow. +`create` exits before the host has ended the session, so a caller that checks at once can still read `running` or `destroying`. +The host records `destroyed` right after the program exits on the SIGHUP, or about 2 s later, after the shutdown grace, when the program ignores SIGHUP. A host whose start-up write is still slow records it only after that write. +Once the session reads `destroyed`, a `destroy` of it exits 4 with `SESSION_ALREADY_DESTROYED`. A host that has already died, or that gets the SIGTERM before it spawns the program, leaves the session to the reconcile above: the session reads `running` until the next reconcile records it `failed`, and no program runs. When the host cannot end the program's group, for example a process of another user, the session stays `destroying` with the host alive. That host never listened, so a plain `destroy` then fails with `HOST_UNREACHABLE` (exit 6), and only `destroy --force` can end the session. +A host stopped with SIGSTOP, or with a blocked event loop, handles the SIGTERM only when it runs again. Until then the program runs and the session reads `running`. A plain `destroy` fails because the host does not answer, and only `destroy --force` ends the session sooner. On Windows, SIGTERM ends the host at once, so the host cannot stop the program itself. There `create` sends it only once the manifest names the program: the session then reads `running` with a dead host until the next reconcile records it `failed` and ends the program. Before the host's first manifest write, `create` leaves the host alone, as before this change, so a host that starts late can still serve the session. On Linux, `destroy` and reconcile never kill a process group that they cannot prove is the session's. diff --git a/src/cli/commands/README.md b/src/cli/commands/README.md index 7fbc7323..3d6a9fd7 100644 --- a/src/cli/commands/README.md +++ b/src/cli/commands/README.md @@ -36,7 +36,7 @@ This directory holds one handler module per agent-tty CLI command. `src/cli/main - `snapshot` and `screenshot` try the live RPC only when the manifest status is `running`. They fall back to offline replay only on `HOST_UNREACHABLE` and rethrow other errors. [snapshot.ts:runSnapshotCommand] [screenshot.ts:runScreenshotCommand] - `record export` accepts only the formats in `RecordExportFormatSchema` (`asciicast`, `webm`). [record-export.ts:runRecordExportCommand] - `batch` parses the whole plan with `parseBatchPlan` before it resolves the session target. [batch.ts:runBatchCommand] -- `create` polls readiness up to `READINESS_MAX_ATTEMPTS` (50) times, `READINESS_POLL_INTERVAL_MS` (100 ms) apart. [create.ts:runCreateCommand] +- `create` polls readiness up to `READINESS_MAX_ATTEMPTS` (50) times, `READINESS_POLL_INTERVAL_MS` (100 ms) apart. [create.ts:waitForReadyHost] - `doctor` reports the `node-runtime` check as failed when the Node version is outside the `engines.node` range of `package.json`. It keeps running the other checks, reports `ok: false` and sets `process.exitCode = 1`. [doctor.ts:runNodeRuntimeCheck] [doctor.ts:runDoctorCommand] - `dashboard` throws `INVALID_INPUT` unless stdin and stdout are both TTYs. It also throws `INVALID_INPUT` when the libghostty-vt probe reports the renderer unavailable. [dashboard.ts:runDashboardCommand] [src/renderer/readiness.ts:assertDashboardRendererAvailable] - `version` reports the constant `PROTOCOL_VERSION` (`'0.1.0'`). [version.ts:buildVersionResult] @@ -44,7 +44,7 @@ This directory holds one handler module per agent-tty CLI command. `src/cli/main ## Side Effects - Session commands send RPCs over the session socket through `sendRpc`. The method names are `run`, `type`, `paste`, `sendKeys`, `mouse`, `mark`, `resize`, `signal`, `snapshot`, `screenshot`, `wait`, `waitForRender` and `inspect`. `create` also sends `inspect` while it polls. [run.ts:runRunCommand] [send-keys.ts:runSendKeysCommand] [wait.ts:runWaitCommand] [create.ts:runCreateCommand] -- `create` calls `allocateSession` (`src/host/lifecycle.ts`) with a `launch` callback that records the session ID and starts the host process through `launchHost`. If `launchHost` or a later step of `allocateSession` throws, `create` removes the session directory with `fs.rm`. A bad `cwd`, shell path or command segment fails before `allocateSession` creates the directory, so no session directory is left behind. A failure between `mkdir` and the callback, such as a failed `chmod`, leaves the empty session directory in place. A readiness-poll failure rethrows and leaves the session directory in place. It registers the Home through `upsertHome`. [create.ts:runCreateCommand] +- `create` calls `allocateSession` (`src/host/lifecycle.ts`) with a `launch` callback that records the session ID and starts the host process through `launchHost`. If `launchHost` or a later step of `allocateSession` throws, `create` removes the session directory with `fs.rm`. A bad `cwd`, shell path or command segment fails before `allocateSession` creates the directory, so no session directory is left behind. A failure between `mkdir` and the callback, such as a failed `chmod`, leaves the empty session directory in place. Any failure after `allocateSession` returned, a failed readiness poll included, calls the host's `stop()` (on Windows only once the manifest names the program) and leaves the session directory in place. `create` rethrows the error with `details.sessionId` added, and an error that is not a `CliError` becomes `INTERNAL_ERROR`. It registers the Home through `upsertHome`. [create.ts:runCreateCommand] - `gc` reads session directories, calls `reconcileSession` (which can rewrite manifests), deletes collectable session directories with `fs.rm` outside dry-run mode, and deregisters Homes through `forgetHome`. [gc.ts:gcSessions] [gc.ts:runGcCommand] - `inspect` calls `reconcileSession` when the host is unreachable, and for an ended session other than destroyed whose folder holds the destroy marker. When that repair throws, it shows the manifest as it reads. [inspect.ts:runInspectCommand] - `destroy` and `list` call `destroySession` and `listSessions` from `src/host/lifecycle.ts`. [destroy.ts:runDestroyCommand] [list.ts:runListCommand] diff --git a/src/cli/commands/create.ts b/src/cli/commands/create.ts index 230798a3..0e1f00f9 100644 --- a/src/cli/commands/create.ts +++ b/src/cli/commands/create.ts @@ -122,6 +122,57 @@ async function readCreateResult( return toCreateResult(manifest); } +async function waitForReadyHost( + socketFile: string, + sessionDirectory: string, + sessionId: string, +): Promise { + for (let attempt = 0; ; attempt += 1) { + try { + await sendRpc(socketFile, 'inspect', undefined, READINESS_RPC_TIMEOUT_MS); + return await readCreateResult(sessionDirectory, sessionId); + } catch (error) { + if ( + !(error instanceof CliError) || + (error.code !== ERROR_CODES.HOST_UNREACHABLE && + error.code !== ERROR_CODES.HOST_TIMEOUT) + ) { + throw error; + } + const manifest = await readManifestIfExists( + manifestPath(sessionDirectory), + ); + if (manifest?.status === 'exited') { + return toCreateResult(manifest); + } + if (attempt + 1 >= READINESS_MAX_ATTEMPTS) { + throw error; + } + await delay(READINESS_POLL_INTERVAL_MS); + } + } +} + +// Every error of the readiness wait names the session, so a caller can inspect +// or destroy what the failed create left. +function withSessionId(error: unknown, sessionId: string): CliError { + if (error instanceof CliError) { + return new CliError(error.code, error.message, { + retryable: error.retryable, + details: { ...error.details, sessionId }, + result: error.result, + cause: error, + }); + } + + return makeCliError(ERROR_CODES.INTERNAL_ERROR, { + message: + error instanceof Error ? error.message : 'Failed to create session.', + details: { sessionId }, + cause: error, + }); +} + export async function runCreateCommand(options: CommandOptions): Promise { const environment = normalizeCreateEnvironment(options.envEntries); const idleTimeoutMs = @@ -206,53 +257,29 @@ export async function runCreateCommand(options: CommandOptions): Promise { const sessionDirectory = sessionDir(home, sessionId); const socketFile = socketPath(sessionDirectory); - for (let attempt = 0; attempt < READINESS_MAX_ATTEMPTS; attempt += 1) { - try { - await sendRpc(socketFile, 'inspect', undefined, READINESS_RPC_TIMEOUT_MS); - const result = await readCreateResult(sessionDirectory, sessionId); - emitSuccess({ - command: 'create', - json: options.json, - result, - lines: [`Session created: ${sessionId}`], - }); - return; - } catch (error) { - if ( - error instanceof CliError && - (error.code === ERROR_CODES.HOST_UNREACHABLE || - error.code === ERROR_CODES.HOST_TIMEOUT) - ) { - const manifest = await readManifestIfExists( - manifestPath(sessionDirectory), - ); - if (manifest?.status === 'exited') { - emitSuccess({ - command: 'create', - json: options.json, - result: toCreateResult(manifest), - lines: [`Session created: ${sessionId}`], - }); - return; - } - - if (attempt + 1 < READINESS_MAX_ATTEMPTS) { - await delay(READINESS_POLL_INTERVAL_MS); - continue; - } - - // A host that answers too late would serve a session whose create - // failed. The host knows its program's pid also without a session - // token, so it stops the program and ends the session itself. On - // Windows the SIGTERM ends the host at once, and only a program that - // the manifest names can be stopped by a later reconcile. - invariant(host !== undefined, 'a session that create polls has a host'); - if (process.platform !== 'win32' || manifest?.childPid != null) { - host.stop(); - } - } - - throw error; + let result: CreateResult; + try { + result = await waitForReadyHost(socketFile, sessionDirectory, sessionId); + } catch (error) { + // Every error here fails the create, and a host that answers later would + // serve that failed session. The host knows its program's pid also + // without a session token, so it stops the program and ends the session + // itself. On Windows the SIGTERM ends the host at once, and only a + // program that the manifest names can be stopped by a later reconcile. + invariant(host !== undefined, 'a session that create polls has a host'); + const manifest = await readManifestIfExists( + manifestPath(sessionDirectory), + ).catch(() => null); + if (process.platform !== 'win32' || manifest?.childPid != null) { + host.stop(); } + throw withSessionId(error, sessionId); } + + emitSuccess({ + command: 'create', + json: options.json, + result, + lines: [`Session created: ${sessionId}`], + }); } diff --git a/src/host/README.md b/src/host/README.md index 41c4fe21..e5ff579c 100644 --- a/src/host/README.md +++ b/src/host/README.md @@ -13,7 +13,7 @@ - `resolveHostRendererName(input?)` picks the renderer name from the input, then `AGENT_TTY_HOST_RENDERER`, then `AGENT_TTY_RENDERER`, then `DEFAULT_RENDERER_NAME` (from `src/renderer/names.ts`). It throws `INVALID_INPUT` for an unknown renderer name. [hostMain.ts:resolveHostRendererName] - `MAX_CONSECUTIVE_POLL_FAILURES` (10) is the number of failed polls in a row after which a `waitForRender` wait ends as timed out. [hostMain.ts:MAX_CONSECUTIVE_POLL_FAILURES] - `allocateSession(config, launch)` validates an `AllocateConfig`, creates the session directory, calls `launch(sessionId)` and writes the first `running` manifest with the returned PID as `hostPid`. It returns an `AllocateResult` with `sessionId` and `sessionDirectory`. [lifecycle.ts:allocateSession] [lifecycle.ts:AllocateConfig] [lifecycle.ts:AllocateResult] -- `launchHost(config)` spawns the host process for a `LaunchHostConfig` and returns a `LaunchedHost`: its PID and a `stop()` that sends the host SIGTERM through the `ChildProcess` handle, and nothing once the host has exited. `create` calls it from the `launch` callback of `allocateSession`, and calls `stop()` when it gives up on the host. [lifecycle.ts:launchHost] [lifecycle.ts:LaunchHostConfig] [lifecycle.ts:LaunchedHost] [src/cli/commands/create.ts:runCreateCommand] +- `launchHost(config)` spawns the host process for a `LaunchHostConfig` and returns a `LaunchedHost`: its PID and a `stop()` that sends the host SIGTERM through the `ChildProcess` handle, and nothing once the host has exited. `create` calls it from the `launch` callback of `allocateSession`, and calls `stop()` on any error after `allocateSession` returned, such as a give-up on the host. [lifecycle.ts:launchHost] [lifecycle.ts:LaunchHostConfig] [lifecycle.ts:LaunchedHost] [src/cli/commands/create.ts:runCreateCommand] - `removeSocketDirectoryIfEmpty(socketFile)` removes the Home's socket directory with `rmdir`. A directory that still holds a socket (`ENOTEMPTY` or `EEXIST`) or is already gone (`ENOENT`) stays as it is. `destroySession` and `reconcileSession` call it after they unlink a socket. The host's shutdown and `doctor`'s socket check call `removeSocketDirectoryBestEffort`, which logs any other `rmdir` error at debug level instead of throwing it. [lifecycle.ts:removeSocketDirectoryIfEmpty] [lifecycle.ts:removeSocketDirectoryBestEffort] - `ensureSocketRoot(root = socketRootDirectory())` creates the socket root with `mkdir` mode `0o700` and then `chmod 0o700` when it is missing. It throws `STORAGE_WRITE_ERROR`, with the path, the owner's uid and the mode in `details`, unless `lstat` shows a directory of the caller's uid with no group or other mode bit: a symlink, a file, a directory of another uid or a group or other bit fails. A private root without all owner bits, as another `create` shows it while umask took them and its `chmod` is pending, gets `chmod 0o700`. It does nothing where `process.getuid` is missing (Windows). `create` calls it before it allocates a session, because the host's errors reach nobody. [socketRoot.ts:ensureSocketRoot] - `socketRootExists(root = socketRootDirectory())` returns `false` for a missing root and `true` for a private one (a directory of the caller's uid with no group or other bit), after the same `chmod 0o700` of a private root without all owner bits. It throws the same `STORAGE_WRITE_ERROR` for any other root. [socketRoot.ts:socketRootExists] diff --git a/test/integration/lifecycle.test.ts b/test/integration/lifecycle.test.ts index ef1f64fa..39377cd6 100644 --- a/test/integration/lifecycle.test.ts +++ b/test/integration/lifecycle.test.ts @@ -1751,7 +1751,7 @@ describe('lifecycle integration', { timeout: 30000 }, () => { expect(code).toBe(6); expect(JSON.parse(stdout)).toMatchObject({ ok: false, - error: { code: 'HOST_UNREACHABLE' }, + error: { code: 'HOST_UNREACHABLE', details: { sessionId } }, }); // The host hangs the program up, then kills its group after the // shutdown grace, and exits once its held write has landed. diff --git a/test/unit/commands/create.test.ts b/test/unit/commands/create.test.ts index d645aeeb..9f6ea63e 100644 --- a/test/unit/commands/create.test.ts +++ b/test/unit/commands/create.test.ts @@ -293,14 +293,89 @@ describe('create command', () => { mocks.sendRpc.mockRejectedValue(unreachable); mocks.readManifestIfExists.mockResolvedValue(makeManifest()); - await expect(runCreateCommand({ ...baseOptions, context })).rejects.toBe( - unreachable, - ); + await expect( + runCreateCommand({ ...baseOptions, context }), + ).rejects.toMatchObject({ + code: 'HOST_UNREACHABLE', + message: 'host unreachable', + retryable: true, + details: { sessionId: 'session-01' }, + cause: unreachable, + }); expect(mocks.sendRpc).toHaveBeenCalledTimes(50); expect(mocks.stopHost).toHaveBeenCalledOnce(); expect(mocks.emitSuccess).not.toHaveBeenCalled(); }); + it('keeps the details of the last error and adds the session id', async () => { + mocks.sendRpc.mockRejectedValue( + makeCliError(ERROR_CODES.HOST_TIMEOUT, { + message: 'RPC request timed out after 1000ms.', + details: { method: 'inspect', timeoutMs: 1000 }, + }), + ); + + await expect( + runCreateCommand({ ...baseOptions, context }), + ).rejects.toMatchObject({ + code: 'HOST_TIMEOUT', + details: { method: 'inspect', timeoutMs: 1000, sessionId: 'session-01' }, + }); + expect(mocks.stopHost).toHaveBeenCalledOnce(); + }); + + it('stops the host when the manifest is gone after the host answered', async () => { + mocks.readManifestIfExists.mockResolvedValue(null); + + await expect( + runCreateCommand({ ...baseOptions, context }), + ).rejects.toMatchObject({ + code: 'INTERNAL_ERROR', + details: { sessionId: 'session-01' }, + }); + expect(mocks.stopHost).toHaveBeenCalledOnce(); + expect(mocks.emitSuccess).not.toHaveBeenCalled(); + }); + + it('stops the host when the manifest read fails while the host does not answer', async () => { + mocks.sendRpc.mockRejectedValue( + makeCliError(ERROR_CODES.HOST_UNREACHABLE, { + message: 'host unreachable', + }), + ); + mocks.readManifestIfExists.mockRejectedValue( + makeCliError(ERROR_CODES.STORAGE_READ_ERROR, { + message: 'manifest unreadable', + }), + ); + + await expect( + runCreateCommand({ ...baseOptions, context }), + ).rejects.toMatchObject({ + code: 'STORAGE_READ_ERROR', + message: 'manifest unreadable', + details: { sessionId: 'session-01' }, + }); + expect(mocks.sendRpc).toHaveBeenCalledOnce(); + expect(mocks.stopHost).toHaveBeenCalledOnce(); + }); + + it('stops the host on an error that is not a CliError and reports it as INTERNAL_ERROR', async () => { + const failure = new TypeError('socket path broke'); + mocks.sendRpc.mockRejectedValue(failure); + + await expect( + runCreateCommand({ ...baseOptions, context }), + ).rejects.toMatchObject({ + code: 'INTERNAL_ERROR', + message: 'socket path broke', + details: { sessionId: 'session-01' }, + cause: failure, + }); + expect(mocks.sendRpc).toHaveBeenCalledOnce(); + expect(mocks.stopHost).toHaveBeenCalledOnce(); + }); + it('on Windows, stops a host that never answers only once the manifest names its program', async () => { const platform = process.platform; Object.defineProperty(process, 'platform', { value: 'win32' }); @@ -310,26 +385,51 @@ describe('create command', () => { const unreachable = makeCliError(ERROR_CODES.HOST_UNREACHABLE, { message: 'host unreachable', }); + const failure = { code: 'HOST_UNREACHABLE', cause: unreachable }; mocks.sendRpc.mockRejectedValue(unreachable); mocks.readManifestIfExists.mockResolvedValue(makeManifest()); - await expect(runCreateCommand({ ...baseOptions, context })).rejects.toBe( - unreachable, - ); + await expect( + runCreateCommand({ ...baseOptions, context }), + ).rejects.toMatchObject(failure); expect(mocks.stopHost).not.toHaveBeenCalled(); mocks.readManifestIfExists.mockResolvedValue(null); - await expect(runCreateCommand({ ...baseOptions, context })).rejects.toBe( - unreachable, - ); + await expect( + runCreateCommand({ ...baseOptions, context }), + ).rejects.toMatchObject(failure); expect(mocks.stopHost).not.toHaveBeenCalled(); mocks.readManifestIfExists.mockResolvedValue( makeManifest({ childPid: 4321 }), ); - await expect(runCreateCommand({ ...baseOptions, context })).rejects.toBe( - unreachable, + await expect( + runCreateCommand({ ...baseOptions, context }), + ).rejects.toMatchObject(failure); + expect(mocks.stopHost).toHaveBeenCalledOnce(); + }); + + it('on Windows, applies the same rule to an error that is not a readiness error', async () => { + const platform = process.platform; + Object.defineProperty(process, 'platform', { value: 'win32' }); + onTestFinished(() => { + Object.defineProperty(process, 'platform', { value: platform }); + }); + const failure = new TypeError('socket path broke'); + mocks.sendRpc.mockRejectedValue(failure); + + mocks.readManifestIfExists.mockResolvedValue(makeManifest()); + await expect( + runCreateCommand({ ...baseOptions, context }), + ).rejects.toMatchObject({ code: 'INTERNAL_ERROR', cause: failure }); + expect(mocks.stopHost).not.toHaveBeenCalled(); + + mocks.readManifestIfExists.mockResolvedValue( + makeManifest({ childPid: 4321 }), ); + await expect( + runCreateCommand({ ...baseOptions, context }), + ).rejects.toMatchObject({ code: 'INTERNAL_ERROR', cause: failure }); expect(mocks.stopHost).toHaveBeenCalledOnce(); }); diff --git a/test/unit/commands/golden-envelopes.test.ts b/test/unit/commands/golden-envelopes.test.ts index cd03385f..70d0d46a 100644 --- a/test/unit/commands/golden-envelopes.test.ts +++ b/test/unit/commands/golden-envelopes.test.ts @@ -1460,6 +1460,44 @@ describe('JSON envelope contracts', () => { }); }); + it('locks the create error envelope shape after the session was allocated', () => { + const error = makeCliError(ERROR_CODES.HOST_UNREACHABLE, { + message: + 'Failed to reach RPC host at /tmp/agent-tty-1000/0123456789abcdef/rpc.sock.', + details: { + method: 'inspect', + socketPath: '/tmp/agent-tty-1000/0123456789abcdef/rpc.sock', + errno: 'ECONNREFUSED', + sessionId: 'session-01', + }, + }); + + expect( + createErrorEnvelope('create', { + code: error.code, + message: error.message, + retryable: error.retryable, + details: error.details, + }), + ).toEqual({ + ok: false, + command: 'create', + timestamp: LOCKED_TIMESTAMP, + error: { + code: 'HOST_UNREACHABLE', + message: + 'Failed to reach RPC host at /tmp/agent-tty-1000/0123456789abcdef/rpc.sock.', + retryable: true, + details: { + method: 'inspect', + socketPath: '/tmp/agent-tty-1000/0123456789abcdef/rpc.sock', + errno: 'ECONNREFUSED', + sessionId: 'session-01', + }, + }, + }); + }); + it('locks the WAIT_TIMEOUT error envelope shape with the wait result', () => { const error = makeCliError(ERROR_CODES.WAIT_TIMEOUT, { message: 'Wait timed out before its condition was met.', diff --git a/test/unit/host/lifecycle.test.ts b/test/unit/host/lifecycle.test.ts index afe5837a..449f8929 100644 --- a/test/unit/host/lifecycle.test.ts +++ b/test/unit/host/lifecycle.test.ts @@ -99,9 +99,22 @@ const psHooks = vi.hoisted(() => ({ tty: undefined as ((pid: number) => string | null) | undefined, })); +// The last ChildProcess that spawn returned, so a test can watch the host +// that launchHost started. +const spawnHooks = vi.hoisted(() => ({ + last: undefined as unknown, +})); + vi.mock('node:child_process', async () => { const actual = await vi.importActual('node:child_process'); + const spawn = ((...args: unknown[]) => { + const child = ( + actual.spawn as (...all: unknown[]) => ChildProcess.ChildProcess + )(...args); + spawnHooks.last = child; + return child; + }) as typeof actual.spawn; const execFileSync = ((file: string, args: readonly string[], ...rest) => { if (psHooks.tty !== undefined && file === '/bin/ps' && args[1] === 'tty=') { const tty = psHooks.tty(Number(args[3])); @@ -116,7 +129,7 @@ vi.mock('node:child_process', async () => { ...rest, ); }) as typeof actual.execFileSync; - return { ...actual, execFileSync }; + return { ...actual, execFileSync, spawn }; }); vi.mock('node:fs/promises', async () => { @@ -163,10 +176,12 @@ import { destroySession, listSessions, cliRunsInsideProgramSession, + launchHost, reconcileSession, removeSocketDirectoryBestEffort, removeSocketDirectoryIfEmpty, type AllocateConfig, + type LaunchedHost, } from '../../../src/host/lifecycle.js'; import { captureProgram, @@ -366,6 +381,55 @@ describe('allocateSession', () => { }); }); +describe('launchHost', () => { + function launchFixture(source: string): { + host: LaunchedHost; + child: ChildProcess.ChildProcess; + } { + const entrypoint = join(home, 'host.mjs'); + writeFileSync(entrypoint, source); + const cliArgv = process.argv; + process.argv = [...cliArgv.slice(0, 1), entrypoint, ...cliArgv.slice(2)]; + try { + const host = launchHost({ + sessionId: 'session-01', + home, + env: {}, + term: 'xterm-256color', + }); + const child = spawnHooks.last as ChildProcess.ChildProcess; + expect(child.pid).toBe(host.pid); + return { host, child }; + } finally { + process.argv = cliArgv; + } + } + + it('stop sends a running host SIGTERM', async () => { + const { host, child } = launchFixture('setInterval(() => {}, 1000);\n'); + const exit = once(child, 'exit'); + + host.stop(); + + expect(child.killed).toBe(true); + expect(await exit).toEqual([null, 'SIGTERM']); + }); + + it('stop sends nothing once the host has exited', async () => { + const { host, child } = launchFixture('process.exit(0);\n'); + const errors: unknown[] = []; + child.on('error', (error) => errors.push(error)); + expect(await once(child, 'exit')).toEqual([0, null]); + + expect(() => { + host.stop(); + }).not.toThrow(); + + expect(child.killed).toBe(false); + expect(errors).toEqual([]); + }); +}); + describe('reconcileSession', () => { it('keeps an exit that the host records after the read and before the liveness check', async () => { const directory = await seedSession(makeRecord());