diff --git a/.agents/skills/verify-session-lifecycle/SKILL.md b/.agents/skills/verify-session-lifecycle/SKILL.md index 0191988c..01a783c3 100644 --- a/.agents/skills/verify-session-lifecycle/SKILL.md +++ b/.agents/skills/verify-session-lifecycle/SKILL.md @@ -30,7 +30,7 @@ The tool is TLC, the TLA+ model checker, pinned to release 1.7.4 by sha256. The | R14 `DestroyOutlivesProgram` | `destroy` is never killed before it reports, also when it runs inside the program's session. | `destroy` prints nothing. With `--force` it kills its own group, and the session reads exited, not destroyed. | | R15 `NoUnrelatedGroupKilled` | No kill reaches a process group that took the program's group id after the program's group ended. | `destroy`, `destroy --force`, a reconcile or the host's shutdown sends SIGKILL to an unrelated group, such as the program of another session. | | R16 `DestroyedSessionStopped` | When the manifest says destroyed, no process of the program's group runs, also none that the program left when it exited on its own, unless the host died before `program.json` named the program. | `destroy` reports `destroyed: true` while a process of the session runs, `list` hides it, and nothing stops it later. | -| R17 `FailedCreateStopped` | Once `create` gave up on its host, the host keeps the session `running` only while `create`'s SIGTERM is on its way to it. | `create` fails with `HOST_UNREACHABLE`, but the session runs, and `list --all` shows it running. | +| R17 `FailedCreateStopped` | Once `create` gave up on its host, the host keeps the session `running` only while `create`'s SIGTERM is on its way to it, or while the host has yet to check for the create-failed marker. | `create` fails with `HOST_UNREACHABLE`, but the session runs, and `list --all` shows it running. | | R18 `DestroyedStaysDestroyed` | Once `destroy` reported success, the manifest says destroyed while the session folder exists, except while a reconcile that wrote over it has yet to write destroyed again. | `destroy` reports `destroyed: true`, and the session then reads `failed`. | | R19 `NoUnrelatedHostKilled` | No kill of `destroy --force` reaches a process that took the host's pid after the host ended. | `destroy --force` sends SIGKILL to an unrelated process that got the crashed host's pid. | @@ -42,12 +42,12 @@ Yes, when the diff changes any of these: - `src/host/lifecycle.ts`: `allocateSession` (and the order of its `launch` call and first manifest write), `launchHost`, `isProcessAlive`, the host proof (`readSessionHost`, `psShowsSessionHost`, `isHostAlive`, `hostMatches`, `killHostBestEffort`), `cliRunsInsideProgramSession` with `ownLinuxSessionId`, `ownProcessGroupId`, `controllingTerminal` and `sharesControllingTerminal`, `reconcileSession`, and `destroySession` with its local `stopLeftoversThenRecordDestroyed` and its helpers `recordDestroyed`, `stopSessionProcesses`, `readSessionManifestOrThrow`, `waitForTerminalManifest`, `waitForProcessesToExit` and `killProcessBestEffort`, and `removeDeadHostSocket`, which `gc` and `destroySession` call. - `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`), `procShowsOwnPidSpace`, 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/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`, the create-failed marker check, `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` 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`. +- `src/storage/manifests.ts` `readValidatedJsonFile`, `replaceFileAtomic`, `replaceFileAtomicSync`, `writeValidatedJsonFileSync`, `writeManifest`, `writeDestroyMarker` and `writeCreateFailedMarker` with `writeEmptyMarker`. - The `SESSION_STATUS_POLICIES` table in `src/protocol/sessionStatusPolicy.ts`. - Any new or removed call to `writeManifest`, `persistManifest`, `replaceFileAtomic`, `writeValidatedJsonFile`, `writeTextFileAtomic`, `manifestPath`, `reconcileSession`, `writeProgramRecord` or `stopProgramGroup` (also through an import alias), or a recursive `rm` (one with `recursive: true`, whatever its argument is named), anywhere in `src/`. - This skill's `model/SessionLifecycle.tla`, `scripts/facts.mjs` and `scripts/check.sh`: an edit to them also needs a new confirmed `coverage.json`. @@ -62,13 +62,13 @@ From the repo root: .agents/skills/verify-session-lifecycle/scripts/check.sh ``` -It needs `node`, Java 11 or newer (`java -version`), and the network once: the first run downloads `tla2tools.jar` 1.7.4 into this skill's `.cache/` folder (git ignores it) and checks its sha256. On this repo it takes about eleven minutes with the default `CHECK_JOBS` of 4. Eight runs explore the whole model, so `CHECK_JOBS=72` starts every run at once and takes about as long as the longest run. `JAVA=/path/to/java` picks another Java. `TLA2TOOLS_JAR=/path/to/tla2tools.jar` uses a jar you already have, which must be the same 1.7.4 build. `CHECK_OUT=` keeps the results in that folder (default: a new temp folder, printed at the end). +It needs `node`, Java 11 or newer (`java -version`), and the network once: the first run downloads `tla2tools.jar` 1.7.4 into this skill's `.cache/` folder (git ignores it) and checks its sha256. On this repo it takes about eleven minutes with the default `CHECK_JOBS` of 4. Ten runs explore more than 20 million distinct states each, so `CHECK_JOBS=75` starts every run at once and takes about as long as the longest run. `JAVA=/path/to/java` picks another Java. `TLA2TOOLS_JAR=/path/to/tla2tools.jar` uses a jar you already have, which must be the same 1.7.4 build. `CHECK_OUT=` keeps the results in that folder (default: a new temp folder, printed at the end). What it does: -1. `scripts/facts.mjs` reads the code with the TypeScript compiler. It finds the thirty-two guards that the rules depend on and the status sets of the policy table, and it hashes each covered function with comments and layout removed. +1. `scripts/facts.mjs` reads the code with the TypeScript compiler. It finds the thirty-three guards that the rules depend on and the status sets of the policy table, and it hashes each covered function with comments and layout removed. 2. It writes one TLC config per run, with those facts as constants. A guard that is not in the code, in the expected place and order, reads FALSE, and the model then runs without it. -3. It runs TLC 72 times and prints one line per run. +3. It runs TLC 75 times and prints one line per run. 4. It compares the hashes and call sites with `coverage.json`. Exit codes: 0 means every run gave the expected result and the covered code matches the sync record. 1 means at least one run gave an unexpected result; the line says `UNEXPECTED`. 3 means every result was as expected, but covered code changed since the sync commit; that is not a pass, so follow "When you change covered code on purpose". 2 means a tool could not run. When both 1 and 3 apply, the exit code is 1. @@ -95,6 +95,9 @@ At the sync commit, the expected results are: | `current-otherspace`, the code as it is with `destroy`, `list` and `gc` in another pid space than the host, every rule | pass | `OtherPidSpace`: no scan and no probe of those commands shows the program's processes, as without `/proc`, with an empty `/proc`, in another pid namespace or on another machine. | | `witness-WitnessUnsharedDestroyCompletes-otherspace` | violated | A plain `destroy` from another pid space still reports success, because it trusts the host's `destroyed` record. | | `mutant-UnsharedGroupReadsUnknown-otherspace`, `mutant-CreateRecordsHost-otherspace` | violated | R16: without the guard, the group reads gone where nothing shows it, and `destroy` of an exited session records destroyed while a process that the program left runs. | +| `mutant-HostChecksCreateFailed` | pass | Redundant off Windows: the SIGTERM of a `create` that gave up stops the host. | +| `current-win32`, the code as it is on Windows, without a token and with a SIGTERM that ends the host at once, R1 to R14 and R17 to R19 | pass | `Win32`: `create` sends the SIGTERM only when the manifest names the program, and that SIGTERM is a host crash. | +| `mutant-HostChecksCreateFailed-win32` | violated | R17: without the marker, a host that the manifest does not name yet keeps serving after `create` gave up. | Read `current` first. The witness and mutant runs use the same facts from the code, so a code change that breaks `current` can also turn them `UNEXPECTED`. @@ -380,6 +383,12 @@ REPLAY_LOG=$LOG REPLAY_HOLD='_host:rename:/session.json:1:9000' NODE_OPTIONS="$H sleep 8; node --import tsx src/cli/main.ts list --all --json # the session is destroyed ``` +### `HostChecksCreateFailed`: on Windows, a host that the manifest does not name yet serves a failed create (R17) + +On Windows, SIGTERM ends the host at once, so the host cannot stop its program. `create` therefore sends it only once the manifest names the program, and before the host's start-up write it sent nothing: a host that started late listened later and served the session, the coder/agent-tty#209 bug in its most likely case (coder/agent-tty#264). + +Now the catch of the readiness wait first writes an empty marker, `/create-failed`, with `writeCreateFailedMarker`, and only then reads the manifest for the platform rule. `runHost` checks for the marker right before its SIGTERM check, before the spawn, and returns without a program when it finds it. It checks again right after its start-up write lands, before it listens, and then calls `initiateShutdown`, which stops the program and records `destroyed`. The host's write comes before its check, and `create`'s marker write comes before its read, so either the host finds the marker or `create` reads the program's pid and sends the SIGTERM. `create` writes the marker on every platform, and off Windows the SIGTERM also stops the host, so `mutant-HostChecksCreateFailed` passes and only `mutant-HostChecksCreateFailed-win32` breaks R17. In the model, `CreateGivesUp` writes the marker, and with `Win32` `CreateStopsHostOnWindows` reads the manifest and ends a named host as a crash. `HostOpensLogAndSpawns` and `HostChecksMarker` are the two checks. The integration test `a create that gives up on Windows, where SIGTERM ends the host at once` runs the Windows path on Linux: a preload makes `create` read `win32` and turns its SIGTERM into a SIGKILL. + ### `FallbackNeedsFolder`: a childPid that gc's removal left stale gets the kill (R15) The model found this race. Without a token, `readProgramGroup` falls back to the childPid of the manifest copy that the caller read before it, as before `program.json` existed. When gc removed the session folder between that read and the `program.json` read, the record read missing too, and the kill used a childPid whose group could have ended and whose id could name an unrelated group by then. @@ -510,7 +519,7 @@ To bring the check back in line after changes that skipped it, use `/maintain-fo ## Scope -The model checks every order of these steps for one session: one `create` that can give up on its host, its host, one `gc` pass over that session (not `--dry-run`), one other command that reconciles twice, one resize, one `destroy` (with or without `--force`, from outside or from inside the program's session), one SIGTERM to the host, a program exit at any time, a process of another user in the program's group that starts and ends at any time, a process that the program leaves when it exits on its own, the host's `host.json` and `program.json` writes, an unrelated group that takes the program's group id once the whole group has ended, an unrelated process that takes the host's pid once the host has ended, the kill timer of the host's shutdown, and a host crash at any time. With a token the model has 24,686,944 distinct states, and without one 24,467,356. The check prints both counts. +The model checks every order of these steps for one session: one `create` that can give up on its host, its host, one `gc` pass over that session (not `--dry-run`), one other command that reconciles twice, one resize, one `destroy` (with or without `--force`, from outside or from inside the program's session), one SIGTERM to the host, a program exit at any time, a process of another user in the program's group that starts and ends at any time, a process that the program leaves when it exits on its own, the host's `host.json` and `program.json` writes, an unrelated group that takes the program's group id once the whole group has ended, an unrelated process that takes the host's pid once the host has ended, the kill timer of the host's shutdown, and a host crash at any time. With a token the model has 27,013,946 distinct states, and without one 26,822,518. The check prints both counts. It assumes: @@ -530,7 +539,7 @@ It assumes: - A killed process stays a zombie until its parent, or init once its parent has died, reaps it, and `kill(-pgid, 0)` still finds a zombie. On Linux, `isProcessGroupAlive` then reads `/proc//stat` of every process and reads the group gone when two scans in a row each show processes of it and only zombies, because a process can fork and end while one scan runs. When a scan shows no process of the group, the probe decides: EPERM reads alive and a found group reads gone. So a group whose last processes are never reaped, as under a container's PID 1 without an init, reads gone. A process whose main thread called `pthread_exit` while another thread runs also shows Z there, so `readProcStat` reads `/proc//task` of every process that shows Z and gives it the state of a thread that is not a zombie. Such a process counts as live in the scan and in the token check, where it can carry the token. On macOS it does not scan, so a zombie still reads alive until launchd reaps it. The model has no zombies: a killed process ends at once. A process that `/proc` refuses to show, as `hidepid=1` (EPERM) or a security module (EACCES) does for other users, reads alive. With `hidepid=2` or `hidepid=4` (`invisible` and `ptraceable` on newer kernels), `/proc` can leave such a process out of its listing, so the probe reads the last `/proc` line of `/proc/self/mountinfo`, skips the scan and reads a group with a zombie alive, as on macOS. With a token, neither a zombie nor a process that `/proc` refuses to show witnesses the token. When no live member is in view, `isProgramGroupAlive` reads the group as `isProcessGroupAlive` does, with the zombie scan, the refused reads and the same `hidepid` check. So under any of those `hidepid` modes, a group that a process of another user took after the program's group ended reads alive, and `destroy` ends with `HOST_TIMEOUT` until that process ends. - 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. +- 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 sends it only once the manifest names the program, and the next reconcile records `failed` and ends the program. Before that write, the host finds the create-failed marker (`HostChecksCreateFailed`). `current-win32` checks this with a SIGTERM that is a host crash and without a token, and keeps the process groups of Linux. A marker write that fails, for example on a full disk, leaves a host that the manifest does not name yet serving on Windows. The model has no such failure. - 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. diff --git a/.agents/skills/verify-session-lifecycle/coverage.json b/.agents/skills/verify-session-lifecycle/coverage.json index 0eab56ff..9275b847 100644 --- a/.agents/skills/verify-session-lifecycle/coverage.json +++ b/.agents/skills/verify-session-lifecycle/coverage.json @@ -1,6 +1,6 @@ { "fingerprints": { - "src/cli/commands/create.ts#runCreateCommand": "635715a49fa0273e", + "src/cli/commands/create.ts#runCreateCommand": "131d2707818ea283", "src/cli/commands/create.ts#waitForReadyHost": "a7fd2ba345b6bfd6", "src/cli/commands/destroy.ts#runDestroyCommand": "ecaa8b1d572da7c6", "src/cli/commands/destroy.ts#rerunInNewSession": "cf32d7d082758522", @@ -85,15 +85,17 @@ "src/storage/manifests.ts#replaceFileAtomicSync": "38bea7cd8e67828f", "src/storage/manifests.ts#writeValidatedJsonFileSync": "4a5d1cc5f63ae8ce", "src/storage/manifests.ts#writeManifest": "1bcc99f09acfc577", - "src/storage/manifests.ts#writeDestroyMarker": "7387a50b983a97c6", + "src/storage/manifests.ts#writeDestroyMarker": "5474dd44808899ec", + "src/storage/manifests.ts#writeCreateFailedMarker": "587c8047e563db4e", + "src/storage/manifests.ts#writeEmptyMarker": "143325689100f36c", "src/protocol/sessionStatusPolicy.ts#SESSION_STATUS_POLICIES": "83827f9f7f6b8e1d", - "src/host/hostMain.ts#runHost(parts)": "37e8ce5dc771d29c", - "src/host/hostMain.ts#runHost(final try)": "16789dd4e2534bc7", + "src/host/hostMain.ts#runHost(parts)": "1d75544221b3c6c1", + "src/host/hostMain.ts#runHost(final try)": "251f1e68a4ec17cc", "src/host/hostMain.ts#runHost>handlers.resize": "d601680963309542", "src/host/hostMain.ts#runHost>handlers.destroy": "ccff44951e711dac", - "skill:model/SessionLifecycle.tla": "7986bb289034aa6c", - "skill:scripts/facts.mjs": "ea6bebdccb255054", - "skill:scripts/check.sh": "66a765237f244773" + "skill:model/SessionLifecycle.tla": "320f9c8a26340aeb", + "skill:scripts/facts.mjs": "0f4e240431fb0fcd", + "skill:scripts/check.sh": "606573a67217656e" }, "callSites": { "writeManifest": [ @@ -110,7 +112,7 @@ "src/host/hostMain.ts#runHost>resize" ], "replaceFileAtomic": [ - "src/storage/manifests.ts#writeDestroyMarker", + "src/storage/manifests.ts#writeEmptyMarker", "src/storage/manifests.ts#writeManifest", "src/storage/manifests.ts#writeTextFileAtomic" ], diff --git a/.agents/skills/verify-session-lifecycle/model/SessionLifecycle.tla b/.agents/skills/verify-session-lifecycle/model/SessionLifecycle.tla index 8c2f76f0..76a11572 100644 --- a/.agents/skills/verify-session-lifecycle/model/SessionLifecycle.tla +++ b/.agents/skills/verify-session-lifecycle/model/SessionLifecycle.tla @@ -96,6 +96,11 @@ CONSTANTS CreateStopsOnGiveUp, \* create.ts: a create that gives up on its \* host sends it SIGTERM, so the host stops \* its program and ends the session + HostChecksCreateFailed, \* create.ts, hostMain.ts: a create that gives + \* up writes the create-failed marker before + \* it reads the manifest, and the host checks + \* for it before the spawn and right after its + \* start-up write, and then ends the session TokenNamesProgram, \* programGroup.ts: captureToken records an \* autogroup only once the program leads its \* own session in an autogroup other than the @@ -127,12 +132,16 @@ CONSTANTS \* processes (no /proc, an empty /proc, \* another pid namespace or another machine), \* and host.json names the host's pid space + Win32, \* the environment: Windows, where SIGTERM ends + \* the host at once, so create sends it only + \* when the manifest names the program MaxListReconciles \* how many times the "list" process reconciles ASSUME MaxListReconciles \in 1..3 ASSUME Token \in BOOLEAN ASSUME Unseen \in BOOLEAN ASSUME OtherPidSpace \in BOOLEAN +ASSUME Win32 \in BOOLEAN ASSUME \A b \in {GcSkipsNonCollectable, GcFinalCheck, ReconcileSkipsTerminal, ReconcileSkipsLiveHost, CreateRecordsHostPid, HostWaitsForManifest, ReconcileRereads, @@ -145,6 +154,7 @@ ASSUME \A b \in {GcSkipsNonCollectable, GcFinalCheck, ReconcileSkipsTerminal, InsideCheckReadsEnv, RecordsProgramSync, ProvesGroupOwnership, DestroyStopsLeftovers, FallbackNeedsFolder, CreateStopsOnGiveUp, + HostChecksCreateFailed, ReconcileRepairsDestroyed, ReconcileRepairsMarked, TokenNamesProgram, ReusedIdReadsGone, ProvesHostPid, CreateRecordsHost, @@ -169,17 +179,21 @@ VARIABLES wasExited, \* ghost: the manifest has shown "exited" wasNamed, \* ghost: a manifest has recorded the program's pid dmark, \* the destroy marker file exists in the session directory + cmark, \* the create-failed marker file exists in it rec, \* program.json names the program with its token startFailed, \* ghost: the host died because its files were missing readRepaired, \* ghost: a reconcile wrote destroyed from its first read \* next to the destroy marker sock, \* the RPC socket file exists (it is outside the directory) - cpc, \* create: "alloc", "write", "launch", "done", "failed" - \* (it gave up on its host) + cpc, \* create: "alloc", "write", "launch", "done", "giveup" + \* (Windows: the marker is written, the manifest read + \* is next), "failed" (it gave up on its host) hostAlive, \* the detached host process is alive hpc, \* host start-up path: "none", "boot", "open", "record" \* (the program runs, program.json is not written yet), - \* "pidwrite", "sockdir", "serving", "idle", "dead" + \* "pidwrite", "markcheck" (the start-up write landed, + \* the marker check is next), "sockdir", "serving", + \* "idle", "dead" hex, \* host exit path: "none", "wait" (the program runs), \* "groupwait" (the program ended during a destroy, and \* the host waits for its group), "exitwrite" (its exit @@ -234,7 +248,7 @@ VARIABLES dkill \* ghost: destroy --force killed a live host diskVars == <> + wasNamed, dmark, cmark>> ghostVars == <> hostVars == <> groupVars == <> + wasNamed, dmark, cmark, ghostVars, sock, hostVars, + shutVars, readerVars, destroyVars, groupVars>> \* allocateSession: writeManifest with status "running". gc skips a directory \* without a manifest, so this write always finds its directory (TypeOK @@ -460,17 +474,38 @@ CreateLaunchesHost == \* runCreateCommand's readiness loop gives up after 50 inspect RPCs that \* fail, which the model lets happen at any step after the start-up, since -\* it has no clock. With CreateStopsOnGiveUp, create then sends its host -\* SIGTERM and reports the error. HostGetsSigterm takes that SIGTERM as the -\* host's one SIGTERM, and HostCrashes covers one that ends a host before it -\* spawns the program. create writes nothing itself: without a token only -\* the host knows which group is the program's. Without the guard, create -\* only reports the error. +\* it has no clock. With HostChecksCreateFailed, create first writes the +\* create-failed marker, an empty file written with no mkdir, so the write +\* fails when gc removed the directory, and create goes on. Then it reads +\* the manifest. With CreateStopsOnGiveUp, create sends its host SIGTERM +\* and reports the error. HostGetsSigterm takes that SIGTERM as the host's +\* one SIGTERM, and HostCrashes covers one that ends a host before it spawns +\* the program. create writes nothing else: without a token only the host +\* knows which group is the program's. Without the guard, create only +\* reports the error. On Linux and macOS the manifest read changes nothing, +\* so the model joins the marker write, the read and the SIGTERM. CreateGivesUp == /\ cpc = "done" + /\ cmark' = (cmark \/ (HostChecksCreateFailed /\ dir)) + /\ cpc' = IF Win32 THEN "giveup" ELSE "failed" + /\ UNCHANGED <> + +\* On Windows the SIGTERM ends the host at once, without its handlers, so +\* create sends it only when the manifest that it reads after the marker +\* write names the program. The next reconcile then records failed and ends +\* the program. +CreateStopsHostOnWindows == + /\ cpc = "giveup" /\ cpc' = "failed" - /\ UNCHANGED <> + /\ IF CreateStopsOnGiveUp /\ mchild /\ hostAlive + THEN /\ HostDies + /\ UNCHANGED <> + ELSE UNCHANGED <> + /\ UNCHANGED <> \* ------------------------------------------------------------------------- \* The host's serial manifest writer (createSerialManifestWriter). Each write @@ -560,12 +595,14 @@ HostReadsManifest == \* handler of the host runs inside this block. HostOpensLogAndSpawns == /\ hostAlive /\ hpc = "open" - /\ IF dir - THEN /\ hpc' = IF RecordsProgramSync THEN "record" ELSE "pidwrite" - /\ hex' = "wait" /\ prog' = TRUE - /\ UNCHANGED <> - ELSE HostFailsToStart /\ UNCHANGED prog + /\ IF ~dir + THEN HostFailsToStart /\ UNCHANGED prog + ELSE IF HostChecksCreateFailed /\ cmark + THEN HostDies /\ UNCHANGED <> + ELSE /\ hpc' = IF RecordsProgramSync THEN "record" ELSE "pidwrite" + /\ hex' = "wait" /\ prog' = TRUE + /\ UNCHANGED <> /\ UNCHANGED <> @@ -592,7 +629,7 @@ HostRecordsProgram == HostWritesPids == /\ hostAlive /\ hpc = "pidwrite" /\ HostWrite("running", FALSE) - /\ hpc' = "sockdir" + /\ hpc' = IF HostChecksCreateFailed THEN "markcheck" ELSE "sockdir" /\ dsnap' = NextTurn(ExitWriteAheadOfDestroying) /\ rec' = (rec \/ (Token /\ ~RecordsProgramSync /\ dir)) /\ UNCHANGED <> +\* Right after the start-up write lands, the host checks for the +\* create-failed marker and shuts down when it finds it, as for a SIGTERM. +HostChecksMarker == + /\ hostAlive /\ hpc = "markcheck" + /\ IF cmark + THEN /\ hpc' = "idle" /\ StartShutdown(WriteQueued) + ELSE /\ hpc' = "sockdir" /\ UNCHANGED <> + /\ UNCHANGED <> + \* `if (!isSessionCommandable(state))` initiateShutdown, else mkdir and \* chmod of the socket directory and rpcServer.listen(), which the host \* repeats when another session's destroy removed that directory. @@ -641,7 +689,9 @@ HostResizeWriteLands == \* calls startShutdown, and it runs only after the synchronous start-up \* block. HostGetsSigterm == - /\ hostAlive /\ ~sig /\ hpc \in {"pidwrite", "sockdir", "serving", "idle"} + /\ ~Win32 + /\ hostAlive /\ ~sig + /\ hpc \in {"pidwrite", "markcheck", "sockdir", "serving", "idle"} /\ sig' = TRUE /\ StartShutdown(WriteQueued) /\ UNCHANGED <> ELSE /\ DestroyEnds("error") /\ UNCHANGED dmark /\ UNCHANGED <> @@ -1331,9 +1381,9 @@ AllFinished == Next == \/ CreateMakesDirectory \/ CreateWritesManifest \/ CreateLaunchesHost - \/ CreateGivesUp + \/ CreateGivesUp \/ CreateStopsHostOnWindows \/ HostReadsManifest \/ HostOpensLogAndSpawns \/ HostRecordsProgram - \/ HostWritesPids + \/ HostWritesPids \/ HostChecksMarker \/ HostListens \/ HostStartsResize \/ HostResizeWriteLands \/ HostGetsSigterm \/ HostWritesDestroying \/ HostClosesSocket \/ HostKillsProgram \/ ProgramExits \/ HostPollsGroup \/ HostWritesExit @@ -1364,7 +1414,9 @@ Spec == Init /\ [][Next]_vars TypeOK == /\ dmark => dir - /\ {dir, mpid, mchild, wasTerminal, wasExited, wasNamed, dmark, startFailed, + /\ cmark => dir + /\ {dir, mpid, mchild, wasTerminal, wasExited, wasNamed, dmark, cmark, + startFailed, readRepaired, sock, hostAlive, prog, foreign, sig, dforce, dinside, dsaw, dkill, rec, left, reused, hitUnrelated, diedUnrecorded, passedReused, hrec, hreused, @@ -1379,7 +1431,7 @@ TypeOK == /\ hsd \in {"none", "write", "wait", "closed"} /\ dsnap \in {"none", "destroying", "destroyed"} /\ (mst # "none") => dir - /\ (dir /\ mst = "none") => cpc \notin {"done", "failed"} + /\ (dir /\ mst = "none") => cpc \notin {"done", "giveup", "failed"} /\ (cpc = "write") => dir /\ gres \in {"pending", "skipped", "removed"} /\ lruns \in 0..MaxListReconciles @@ -1474,10 +1526,13 @@ DestroyedSessionStopped == (mst = "destroyed" /\ ~diedUnrecorded) => ~(GroupAlive \/ left) \* R17: once create gave up, its host keeps the session "running" only -\* while create's SIGTERM is still on its way to it. +\* while create's SIGTERM is still on its way to it, or while the host has +\* yet to check for the create-failed marker. FailedCreateStopped == (cpc = "failed" /\ hostAlive /\ hst = "running") => - (CreateStopsOnGiveUp /\ ~sig) + \/ CreateStopsOnGiveUp /\ ~Win32 /\ ~sig + \/ HostChecksCreateFailed /\ cmark + /\ hpc \in {"boot", "open", "record", "pidwrite", "markcheck"} \* R18: once destroy reported success, the manifest says destroyed while the \* directory exists, except while a reconcile that wrote over it has yet to diff --git a/.agents/skills/verify-session-lifecycle/scripts/check.sh b/.agents/skills/verify-session-lifecycle/scripts/check.sh index 82664fe3..399c57bb 100755 --- a/.agents/skills/verify-session-lifecycle/scripts/check.sh +++ b/.agents/skills/verify-session-lifecycle/scripts/check.sh @@ -122,7 +122,7 @@ TERMINAL=$(node -e ' const c = JSON.parse(require("node:fs").readFileSync(process.argv[1], "utf8")).constants; console.log("{" + c.Terminal.map((s) => JSON.stringify(s)).join(", ") + "}"); ' "$OUT/facts.json") -GUARDS="GcSkipsNonCollectable GcFinalCheck ReconcileSkipsTerminal ReconcileSkipsLiveHost CreateRecordsHostPid HostWaitsForManifest ReconcileRereads HostSerializesWrites HostStopsAfterTerminalWrite ShutdownChecksCommandable DestroyRecordsDestroyed DestroyKeepsExitCode ManifestWriteNeedsFolder KillsProgramGroup ProbeCountsDeniedGroup HostWaitsForGroup ReconcileWaitsForGroup ReRunsInNewSession InsideCheckReadsEnv RecordsProgramSync ProvesGroupOwnership DestroyStopsLeftovers FallbackNeedsFolder ReconcileRepairsDestroyed ReconcileRepairsMarked CreateStopsOnGiveUp TokenNamesProgram ReusedIdReadsGone ProvesHostPid CreateRecordsHost FreezesUnseenGroup UnsharedGroupReadsUnknown" +GUARDS="GcSkipsNonCollectable GcFinalCheck ReconcileSkipsTerminal ReconcileSkipsLiveHost CreateRecordsHostPid HostWaitsForManifest ReconcileRereads HostSerializesWrites HostStopsAfterTerminalWrite ShutdownChecksCommandable DestroyRecordsDestroyed DestroyKeepsExitCode ManifestWriteNeedsFolder KillsProgramGroup ProbeCountsDeniedGroup HostWaitsForGroup ReconcileWaitsForGroup ReRunsInNewSession InsideCheckReadsEnv RecordsProgramSync ProvesGroupOwnership DestroyStopsLeftovers FallbackNeedsFolder ReconcileRepairsDestroyed ReconcileRepairsMarked CreateStopsOnGiveUp HostChecksCreateFailed TokenNamesProgram ReusedIdReadsGone ProvesHostPid CreateRecordsHost FreezesUnseenGroup UnsharedGroupReadsUnknown" # One "Guard=TRUE|FALSE" word per guard, read from the code by facts.mjs. FACTS_LINE=$(node -e ' const c = JSON.parse(require("node:fs").readFileSync(process.argv[1], "utf8")).constants; @@ -149,6 +149,8 @@ TOKEN=TRUE UNSEEN=FALSE # OTHER: whether destroy, list and gc run in another pid space than the host. OTHER=FALSE +# WIN32: whether SIGTERM ends the host at once, as on Windows. +WIN32=FALSE write_run() { local name=$1 expect=$2 invariants=$3 overrides=$4 note=$5 must=${6:-} local dir="$OUT/$name" fact guard value @@ -170,6 +172,7 @@ write_run() { echo " Token = $TOKEN" echo " Unseen = $UNSEEN" echo " OtherPidSpace = $OTHER" + echo " Win32 = $WIN32" echo " MaxListReconciles = 2" echo "SPECIFICATION Spec" echo "CHECK_DEADLOCK TRUE" @@ -204,6 +207,7 @@ write_run mutant-DestroyStopsLeftovers violated DestroyedSessionStopped "Destroy write_run mutant-FallbackNeedsFolder violated NoUnrelatedGroupKilled "FallbackNeedsFolder=FALSE" "without the folder check, a childPid that gc's removal left stale gets the kill" NoUnrelatedGroupKilled write_run mutant-ReconcileRepairsDestroyed violated DestroyedStaysDestroyed "ReconcileRepairsDestroyed=FALSE" "without the marker, a reconcile writes failed over the destroyed that destroy reported" DestroyedStaysDestroyed write_run mutant-CreateStopsOnGiveUp violated FailedCreateStopped "CreateStopsOnGiveUp=FALSE" "without the SIGTERM, a create that gave up leaves its host serving the session" FailedCreateStopped +write_run mutant-HostChecksCreateFailed pass FailedCreateStopped "HostChecksCreateFailed=FALSE" "redundant off Windows: the SIGTERM stops the host" write_run mutant-TokenNamesProgram violated DestroyedSessionStopped "TokenNamesProgram=FALSE" "with the host's autogroup as the token, no member carries it, and destroy records destroyed while the program's group runs" DestroyedSessionStopped write_run mutant-ReusedIdReadsGone pass WitnessWaitPassesReusedId "ReusedIdReadsGone=FALSE" "with a group that took the id read alive, no wait of destroy passes while that group runs" write_run mutant-ProvesHostPid violated NoUnrelatedHostKilled "ProvesHostPid=FALSE" "without the start-up record, destroy --force kills a process that took the host's pid" NoUnrelatedHostKilled @@ -245,6 +249,13 @@ write_run mutant-KillsProgramGroup-no-token violated DestroyedProgramStopped "Ki write_run mutant-HostWaitsForGroup-no-token violated DestroyedProgramStopped "HostWaitsForGroup=FALSE" "without a token, the host records destroyed while a process of another user runs" DestroyedProgramStopped write_run mutant-ReconcileWaitsForGroup-no-token violated LiveGroupStaysNamed "ReconcileWaitsForGroup=FALSE" "without a token, reconcile drops childPid while the group runs" LiveGroupStaysNamed +# Windows: no token, and a SIGTERM ends the host at once, so create sends it +# only when the manifest names the program. +WIN32=TRUE +write_run current-win32 pass "$NO_TOKEN_RULES" "" "the code on Windows: R1 to R14 and R17 to R19 hold" +write_run mutant-HostChecksCreateFailed-win32 violated FailedCreateStopped "HostChecksCreateFailed=FALSE" "without the marker, a host that the manifest does not name yet keeps serving after create gave up" FailedCreateStopped +WIN32=FALSE + run_tlc() { local dir rc=0 dir=$(cd "$OUT/$1" && pwd) diff --git a/.agents/skills/verify-session-lifecycle/scripts/facts.mjs b/.agents/skills/verify-session-lifecycle/scripts/facts.mjs index ae7bdbee..0bd9da5d 100755 --- a/.agents/skills/verify-session-lifecycle/scripts/facts.mjs +++ b/.agents/skills/verify-session-lifecycle/scripts/facts.mjs @@ -256,6 +256,7 @@ function createFacts() { launch >= 0 && launch < write && pidInManifest && onlyThroughCallback, CreateRecordsHost: recordsHost, CreateStopsOnGiveUp: stopsOnGiveUp(create.body), + HostChecksCreateFailed: checksCreateFailed(create.body), }; } @@ -267,26 +268,38 @@ function createFacts() { // 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) { - const stops = body.statements.some((s) => { +// The catch of the try whose only statement is the readiness wait, as +// normalized statements, or null when runCreateCommand has no such try. +function giveUpCatch(body) { + for (const s of body.statements) { if (!ts.isTryStatement(s) || s.catchClause === undefined) { - return false; + continue; } const tried = s.tryBlock.statements.map((t) => normalize(t.getText())); - const st = s.catchClause.block.statements.map((t) => - normalize(t.getText()), - ); - return ( + if ( 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);' - ); - }); + 'result = await waitForReadyHost(socketFile, sessionDirectory, sessionId);' + ) { + return s.catchClause.block.statements.map((t) => normalize(t.getText())); + } + } + return null; +} + +function stopsOnGiveUp(body) { + const st = giveUpCatch(body) ?? []; + const stops = + st.length >= 4 && + st[0].startsWith('invariant(host !== undefined,') && + st + .at(-3) + .startsWith( + 'const manifest = await readManifestIfExists( manifestPath(sessionDirectory), )', + ) && + st.at(-2) === + "if (process.platform !== 'win32' || manifest?.childPid != null) { host.stop(); }" && + st.at(-1) === 'throw withSessionId(error, sessionId);'; const launch = normalize( requireDeclaration('src/host/lifecycle.ts', 'launchHost').getText(), ); @@ -308,6 +321,49 @@ function stopsOnGiveUp(body) { ); } +// The give-up catch writes the create-failed marker right before its manifest +// read, so the read comes after the marker. runHost checks for the marker right +// before its SIGTERM skip, before the spawn, and right after its start-up +// manifest write lands, before the listen, and then shuts down. So a host +// whose start-up write landed before create's read is named by that read, +// and a host whose write landed after it finds the marker. +function checksCreateFailed(body) { + const st = giveUpCatch(body) ?? []; + const marks = + st.length === 5 && + st[1].startsWith( + 'await writeCreateFailedMarker( createFailedMarkerPath(sessionDirectory), sessionId, ).catch(', + ) && + st[2].startsWith('const manifest = await readManifestIfExists('); + const run = requireDeclaration('src/host/hostMain.ts', 'runHost').body; + const check = indexOfStatement( + run, + /^if \(existsSync\(createFailedMarkerPath\(sessDir\)\)\) \{\s*await eventLog\.close\(\);\s*return;\s*\}$/, + ); + const skip = indexOfStatement( + run, + /^if \(sigterm\.received\) \{\s*await eventLog\.close\(\);\s*return;\s*\}$/, + ); + const last = run.statements.at(-1); + const tried = + last !== undefined && ts.isTryStatement(last) + ? last.tryBlock.statements.map((t) => normalize(t.getText())) + : []; + const write = tried.indexOf('await persistManifest();'); + const listen = tried.findIndex((t) => + t.startsWith('rpcListenPromise = listenInSocketDirectory('), + ); + return ( + marks && + check >= 0 && + check === skip - 1 && + write >= 0 && + tried[write + 1] === + 'if (existsSync(createFailedMarkerPath(sessDir))) { await initiateShutdown(); return; }' && + write + 1 < listen + ); +} + // runHost waits for create's manifest, and every host write of session.json // goes through one createSerialManifestWriter. function hostFacts() { @@ -466,9 +522,17 @@ function repairFacts() { normalize(check.thenStatement.getText()), ); }); - const marker = requireDeclaration( + // writeDestroyMarker writes through the helper that it shares with + // writeCreateFailedMarker. + const marker = normalize( + requireDeclaration( + 'src/storage/manifests.ts', + 'writeDestroyMarker', + ).body.getText(), + ); + const emptyMarker = requireDeclaration( 'src/storage/manifests.ts', - 'writeDestroyMarker', + 'writeEmptyMarker', ).getText(); const reconcile = requireDeclaration(lifecycle, 'reconcileSession').body; const marked = indexOfStatement( @@ -490,8 +554,10 @@ function repairFacts() { ReconcileRepairsDestroyed: marks && repairs && - marker.includes("await replaceFileAtomic(path, '')") && - !marker.includes('mkdir'), + marker === + "{ await writeEmptyMarker(path, sessionId, 'destroy marker'); }" && + emptyMarker.includes("await replaceFileAtomic(path, '')") && + !emptyMarker.includes('mkdir'), ReconcileRepairsMarked: marked >= 0 && marked < terminal && @@ -1408,6 +1474,8 @@ const COVERED = [ ['src/storage/manifests.ts', 'writeValidatedJsonFileSync'], ['src/storage/manifests.ts', 'writeManifest'], ['src/storage/manifests.ts', 'writeDestroyMarker'], + ['src/storage/manifests.ts', 'writeCreateFailedMarker'], + ['src/storage/manifests.ts', 'writeEmptyMarker'], ['src/protocol/sessionStatusPolicy.ts', 'SESSION_STATUS_POLICIES'], ]; @@ -1427,6 +1495,7 @@ const RUN_HOST_PARTS = [ /^pty\.onExit\(/, /^process\.on\('SIGTERM'/, /^let onSigterm = /, + /^if \(existsSync\(createFailedMarkerPath\(sessDir\)\)\) \{/, /^if \(sigterm\.received\) \{/, /^invariant\(!sigterm\.received/, /^onSigterm = startShutdown;/, diff --git a/.agents/skills/verify-session-lifecycle/scripts/replay-hold.mjs b/.agents/skills/verify-session-lifecycle/scripts/replay-hold.mjs index 38274dad..bb105e23 100644 --- a/.agents/skills/verify-session-lifecycle/scripts/replay-hold.mjs +++ b/.agents/skills/verify-session-lifecycle/scripts/replay-hold.mjs @@ -8,8 +8,9 @@ // // a word in the process's argv: _host, gc, list, inspect, ... // a node:fs/promises function: rename (matches the target -// path), rm, unlink, writeFile, mkdir (hold before the call), or -// readFile (hold after the read, so the caller gets an old copy) +// path), rm, unlink, writeFile, mkdir, open (hold before the +// call), or readFile (hold after the read, so the caller gets an +// old copy) // hold the nth matching call (1 = the first) in that process // // The detached host inherits NODE_OPTIONS and REPLAY_HOLD from `create`. @@ -21,7 +22,14 @@ import { syncBuiltinESMExports } from 'node:module'; import process from 'node:process'; const HOLD_AFTER = new Set(['readFile']); -const HOLD_BEFORE = new Set(['rename', 'rm', 'unlink', 'writeFile', 'mkdir']); +const HOLD_BEFORE = new Set([ + 'rename', + 'rm', + 'unlink', + 'writeFile', + 'mkdir', + 'open', +]); function log(message) { if (process.env.REPLAY_LOG !== undefined) { diff --git a/docs/USAGE.md b/docs/USAGE.md index bee41ee5..14cd6470 100644 --- a/docs/USAGE.md +++ b/docs/USAGE.md @@ -335,7 +335,9 @@ Once the session reads `destroyed`, a `destroy` of it exits 4 with `SESSION_ALRE 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 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 start-up write, `create` sends nothing. +For that case, on every platform, `create` first writes an empty internal file, `create-failed`, in the session folder, and then reads the manifest. The host checks for that file before it starts the program, and returns then, so no program runs and the next reconcile records the session `failed`. It checks again right after its start-up write, and then ends the session the way a SIGTERM does on Linux: it stops the program and records `destroyed`. +On Windows, a host whose start-up write lands before `create` reads the manifest is named by that read and gets the SIGTERM, and a host whose write lands after it finds the file. When the file write fails, for example on a full disk, a Windows host that starts late can still serve the session. This path is not tested on a real Windows host. On Linux, `destroy` and reconcile never kill a process group that they cannot prove is the session's. When the host starts the program, it records the program's pid, the boot id and the program's autogroup id in an internal file, `program.json`. diff --git a/src/cli/commands/README.md b/src/cli/commands/README.md index 3118c137..5c5a62b6 100644 --- a/src/cli/commands/README.md +++ b/src/cli/commands/README.md @@ -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. 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] +- `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, writes the empty `create-failed` marker with `writeCreateFailedMarker`, logs a failed marker write at debug level, then 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 0e1f00f9..fd9da566 100644 --- a/src/cli/commands/create.ts +++ b/src/cli/commands/create.ts @@ -16,8 +16,12 @@ import { ensureSocketRoot } from '../../host/socketRoot.js'; import { ERROR_CODES, makeCliError } from '../../protocol/errors.js'; import type { SessionRecord } from '../../protocol/schemas.js'; import { upsertHome } from '../../storage/homeRegistry.js'; -import { readManifestIfExists } from '../../storage/manifests.js'; import { + readManifestIfExists, + writeCreateFailedMarker, +} from '../../storage/manifests.js'; +import { + createFailedMarkerPath, manifestPath, sessionDir, socketPath, @@ -266,7 +270,22 @@ export async function runCreateCommand(options: CommandOptions): Promise { // 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. + // So create first writes the create-failed marker, and the host checks + // for it after its start-up write names the program: either the host + // finds the marker, or the manifest read below finds the program. invariant(host !== undefined, 'a session that create polls has a host'); + await writeCreateFailedMarker( + createFailedMarkerPath(sessionDirectory), + sessionId, + ).catch((markerError: unknown) => { + options.context.logger.debug('failed to write the create-failed marker', { + sessionId, + error: + markerError instanceof Error + ? markerError.message + : String(markerError), + }); + }); const manifest = await readManifestIfExists( manifestPath(sessionDirectory), ).catch(() => null); diff --git a/src/config/defaults.ts b/src/config/defaults.ts index 4880c6dd..1d49f88a 100644 --- a/src/config/defaults.ts +++ b/src/config/defaults.ts @@ -18,3 +18,4 @@ export const EVENT_LOG_FILENAME = 'events.jsonl'; export const PROGRAM_RECORD_FILENAME = 'program.json'; export const HOST_RECORD_FILENAME = 'host.json'; export const DESTROY_MARKER_FILENAME = 'destroyed'; +export const CREATE_FAILED_MARKER_FILENAME = 'create-failed'; diff --git a/src/host/README.md b/src/host/README.md index 93b0afff..3f7f249e 100644 --- a/src/host/README.md +++ b/src/host/README.md @@ -80,6 +80,7 @@ - `runHost` spawns the PTY with `createPty` (`src/pty/createPty.ts`), writes input to it, resizes it and kills it on idle timeout and shutdown. It writes `TerminalQueryResponder` replies to the PTY without logging them. [hostMain.ts:runHost] - `runHost` polls `session.json` with `readManifestIfExists` and probes the creator process with `isProcessAlive` until the manifest appears or the creator exits. It writes `session.json` through `createSerialManifestWriter`, creates the socket directory with mode `0o700` and listens with `RpcServer` through `listenInSocketDirectory` and writes snapshot and screenshot artifacts through `src/snapshot/capture.ts` and `src/screenshot/capture.ts`. [hostMain.ts:runHost] - `runHost` registers a `process.on('SIGTERM')` handler that starts shutdown, sends signals to the child with `process.kill` and starts `setInterval` timers for idle and wait polling. [hostMain.ts:runHost] +- `runHost` checks for the `create-failed` marker with `existsSync` before it spawns the program, and returns when it finds it. It checks again right after its start-up manifest write, before it listens, and then shuts down. [hostMain.ts:runHost] - `runHost` rethrows a PTY-ingestion, event-log append, exit-handling or shutdown error in `process.nextTick`. The uncaught exception crashes the host process. [hostMain.ts:rethrowAsync] - `resolveHostRendererName` reads `process.env.AGENT_TTY_HOST_RENDERER` and `process.env.AGENT_TTY_RENDERER`. [hostMain.ts:resolveHostRendererName] - `EventLog.open` opens the file in append mode and runs `chmod 0o600`. `append` writes with `fileHandle.appendFile`, one call at a time. `close` calls `fileHandle.sync()` and then closes the file. [eventLog.ts:EventLog] diff --git a/src/host/hostMain.ts b/src/host/hostMain.ts index 0c9bc07d..82673d85 100644 --- a/src/host/hostMain.ts +++ b/src/host/hostMain.ts @@ -1,3 +1,4 @@ +import { existsSync } from 'node:fs'; import process from 'node:process'; import { setTimeout as delay } from 'node:timers/promises'; @@ -75,6 +76,7 @@ import { captureSnapshotResult } from '../snapshot/capture.js'; import { resolveHome } from '../storage/home.js'; import { readManifestIfExists } from '../storage/manifests.js'; import { + createFailedMarkerPath, eventLogPath, manifestPath, sessionDir, @@ -328,6 +330,11 @@ export async function runHost( }; }); + // A create that gave up before the spawn wrote the create-failed marker. + if (existsSync(createFailedMarkerPath(sessDir))) { + await eventLog.close(); + return; + } if (sigterm.received) { await eventLog.close(); return; @@ -1269,6 +1276,14 @@ export async function runHost( writeProgramRecord(sessDir, program); await persistManifest(); + // That write names the program before this check, and a create that + // gives up writes the marker before it reads the manifest. So the host + // finds the marker, or create finds the program and stops the host. + if (existsSync(createFailedMarkerPath(sessDir))) { + await initiateShutdown(); + return; + } + if (!isSessionCommandable(state)) { await initiateShutdown(); return; diff --git a/src/storage/README.md b/src/storage/README.md index 108bd6a4..b8ac27df 100644 --- a/src/storage/README.md +++ b/src/storage/README.md @@ -9,7 +9,7 @@ - `resolveHome(configuredHome = process.env.AGENT_TTY_HOME)` returns the absolute Home: the configured path, or `os.homedir()/.agent-tty`, passed through `canonicalHomePath`. [home.ts:resolveHome] - `canonicalHomePath(path, realpath = realpathSync)` returns the real path of the deepest ancestor of `path` that resolves, joined with the parts below it. [home.ts:canonicalHomePath] - `ensureHome(configuredHome = process.env.AGENT_TTY_HOME)` resolves the Home, creates it and returns its path. [home.ts:ensureHome] -- `sessionDir(home, sessionId)`, `manifestPath(sessionDir)`, `eventLogPath(sessionDir)`, `programRecordPath(sessionDir)`, `hostRecordPath(sessionDir)` and `destroyMarkerPath(sessionDir)` return `/sessions/`, its `session.json`, its `events.jsonl`, its `program.json`, its `host.json` and its `destroyed` marker. [sessionPaths.ts:sessionDir] [sessionPaths.ts:manifestPath] [sessionPaths.ts:eventLogPath] [sessionPaths.ts:programRecordPath] [sessionPaths.ts:hostRecordPath] [sessionPaths.ts:destroyMarkerPath] +- `sessionDir(home, sessionId)`, `manifestPath(sessionDir)`, `eventLogPath(sessionDir)`, `programRecordPath(sessionDir)`, `hostRecordPath(sessionDir)`, `destroyMarkerPath(sessionDir)` and `createFailedMarkerPath(sessionDir)` return `/sessions/`, its `session.json`, its `events.jsonl`, its `program.json`, its `host.json`, its `destroyed` marker and its `create-failed` marker. [sessionPaths.ts:sessionDir] [sessionPaths.ts:manifestPath] [sessionPaths.ts:eventLogPath] [sessionPaths.ts:programRecordPath] [sessionPaths.ts:hostRecordPath] [sessionPaths.ts:destroyMarkerPath] [sessionPaths.ts:createFailedMarkerPath] - `socketPath(sessionDir)` returns `/<8 hex of sha256(home)>/<12 hex of sha256(sessionId)>`. `socketRootDirectory()` returns the socket root, `/tmp/agent-tty-`, or `/tmp/agent-tty` where `process.getuid` is missing (Windows). `legacySocketPath(socketFile)` returns the same socket under `/tmp/agent-tty`, where hosts of older versions listen, or `null` on Windows and for a path outside the socket root. [sessionPaths.ts:socketPath] [sessionPaths.ts:socketRootDirectory] [sessionPaths.ts:legacySocketPath] - `screenshotFilename`, `snapshotFilename`, `recordingFilename` and `videoFilename` build artifact file names that include the event sequence number, for example `screenshot--.png` and `recording--.cast`. [artifactPaths.ts:screenshotFilename] [artifactPaths.ts:recordingFilename] - `artifactPath(sessionDir, filename)` returns `/artifacts/`. `ensureArtifactsDir(sessionDir)` creates that directory. [artifactPaths.ts:artifactPath] [artifactPaths.ts:ensureArtifactsDir] @@ -37,7 +37,7 @@ - `artifactPath` rejects a file name that is empty or has `/` or `\`, and asserts that the result is directly inside `/artifacts`. File-name builders replace unsafe characters in the profile name with `-`. `screenshotFilename`, `snapshotFilename` and `recordingFilename` require an integer `seq` of at least -1, and `videoFilename` requires a non-negative integer `seq`. [artifactPaths.ts:artifactPath] [artifactPaths.ts:screenshotFilename] [artifactPaths.ts:recordingFilename] - `assertEventLogSize` requires an integer from 0 to `MAX_EVENT_LOG_SIZE`. `readEventLogRecords` checks the size with `stat` before it reads the file. [eventLogCodec.ts:assertEventLogSize] [eventLogCodec.ts:readEventLogRecords] - `parseEventLogContent` and `validateEventRecords` require each record to match `EventRecordSchema`, the first `seq` to be 0, and each next `seq` to be the previous one plus 1. Blank lines are skipped. [eventLogCodec.ts:parseEventLogContent] [eventLogCodec.ts:validateEventRecords] -- Read and write paths in `manifests.ts` must be non-empty and absolute. Invalid JSON and schema failures throw `MANIFEST_VALIDATION_ERROR`. Other read errors throw `STORAGE_READ_ERROR`. Write failures throw `STORAGE_WRITE_ERROR`, except that `writeManifest` and `writeDestroyMarker` throw `SESSION_NOT_FOUND` when the session directory is gone. `writeDestroyMarker` writes an empty file through a temp file and a rename, like `writeManifest`, so it replaces a symlink at that path instead of writing through it, and it creates no folder. [manifests.ts:readValidatedJsonFile] [manifests.ts:writeTextFileAtomic] [manifests.ts:writeManifest] [manifests.ts:writeDestroyMarker] +- Read and write paths in `manifests.ts` must be non-empty and absolute. Invalid JSON and schema failures throw `MANIFEST_VALIDATION_ERROR`. Other read errors throw `STORAGE_READ_ERROR`. Write failures throw `STORAGE_WRITE_ERROR`, except that `writeManifest`, `writeDestroyMarker` and `writeCreateFailedMarker` throw `SESSION_NOT_FOUND` when the session directory is gone. `writeDestroyMarker` and `writeCreateFailedMarker` share `writeEmptyMarker`, which writes an empty file through a temp file and a rename, like `writeManifest`, so it replaces a symlink at that path instead of writing through it, and it creates no folder. [manifests.ts:readValidatedJsonFile] [manifests.ts:writeTextFileAtomic] [manifests.ts:writeManifest] [manifests.ts:writeDestroyMarker] [manifests.ts:writeCreateFailedMarker] - `ArtifactEntrySchema` and `ArtifactManifestSchema` are `.strict()`. `kind` is `screenshot`, `snapshot`, `recording` or `video`. `capturedAtSeq` is an integer of at least -1 (-1 for a capture before the first event). The manifest `version` is the literal `1`. The manifest and entry `sessionId` must equal the session directory name. [artifactManifest.ts:ArtifactEntrySchema] [artifactManifest.ts:ArtifactManifestSchema] - `appendArtifactWithRollback` serializes appends per session directory with an in-process `KeyedSerializer` (from `src/util/keyedSerializer.ts`). `publishArtifactWithRollback` runs its publish step in the same queue, so a failed append cannot delete a file that another capture reused in between. Inside that queue, `withArtifactManifestLock` holds `/artifacts/manifest.json.lock` around the publish step and the manifest read and write, so the host and CLI processes do not drop each other's entries. The lock file is created from a temp file with `createWithoutReplacing` (a `link()`, or an exclusive copy on a filesystem without hard links), with the temp file's timestamps set just before each attempt, and holds `{ version: 1, token, pid, process }`, where `process` is the `captureHost` record or `null`. A writer waits up to 10 s and then throws a retryable `STORAGE_WRITE_ERROR` with `lockPath` and `holderPid`. Other file errors while taking the lock throw `STORAGE_WRITE_ERROR` too (`SESSION_NOT_FOUND` when the folder is gone), and `appendArtifactWithRollback` then removes `rollbackArtifactPath` unless the manifest already lists a file of that name. That check runs without the lock, so a concurrent writer that lists the same file name between the check and the removal is left with a missing file. A lock path that is not a regular file (a FIFO or a symlink) reads as an `unknown` owner, so a writer never blocks on it. It breaks the lock when `judgeHost` says the owner is dead, or when the owner reads `unknown` (a live pid on macOS, another pid namespace, an owner without a process record on Linux, or lock contents that do not parse) and the lock's modification time is more than 60 s away from now, in either direction. To break, a writer first announces itself with `manifest.json.lock.breakers/`, a link of its temp file in a folder that holds only announcements, and lists that folder. So the listing costs the same however many artifacts the session holds. It creates the folder without `recursive`, and the last breaker removes it with `rmdir`, which removes only an empty folder. When another breaker's announcement is there and not stale (by the break-file rule below), it removes its own and waits. It removes stale announcements, whose names no other writer uses. So breakers act one at a time: of two that announce, the one that reads later sees the other. The acting breaker takes `manifest.json.lock.break` the same exclusive way as the lock, which also keeps out older agent-tty versions, re-reads the lock, removes it only when its bytes still equal the ones it judged, and then removes its break file and its announcement. A breaker that saw another one waits a random 0.5 to 1.5 times a backoff of 5 ms that doubles with each such meeting, up to 200 ms. Breakers that keep meeting so spread their attempts until one of them is alone. This makes a meeting less likely each time. It does not rule one out. The break file and the announcement hold the breaker's record. Another writer removes them when `judgeHost` says the breaker is dead, or when the file is more than 60 s old, also while the breaker lives. Release removes the lock only when it holds this writer's token. It tries 3 times. When all fail, the process remembers the lock, and its next acquisition on that session breaks it at once. Until then, other processes wait for it as for a live owner. A writer that found the lock taken, once it holds the lock, removes each break file and announcement that it could remove as above, and each `manifest.json.lock.tmp-*` file whose owner is dead, or reads `unknown` and the file is more than 60 s old. A writer that finds the lock free does not read the artifacts folder, so the temp file of a waiter that died stays until a later writer finds the lock taken. A waiting writer whose temp file another writer removed writes it again before its next attempt. Limits: a writer with an `unknown` owner that stops for over 60 s inside the section loses the lock while it still runs, so two writers can overlap. A breaker stopped for over 60 s while it holds its announcement or the break file loses them the same way. Writers of agent-tty versions without announcements take only the break file, and removing a stale break file reads it and removes it by path in two steps. So an older writer and a newer one that both remove a dead breaker's break file at the same time can still overlap, as two older writers can. A writer with an `unknown` owner that stops for over 60 s between setting its temp file's timestamps and the `link` fails with `STORAGE_WRITE_ERROR` when another writer removed the temp file in that time. [artifactManifest.ts:withArtifactManifestLock] [artifactManifest.ts:appendArtifactWithRollback] [artifactManifest.ts:publishArtifactWithRollback] - `computeArtifactHealth` maps only `MANIFEST_VALIDATION_ERROR` to health `manifest-invalid` and rethrows other errors. It validates its result with `ArtifactHealthSummarySchema`. [artifactHealth.ts:computeArtifactHealth] diff --git a/src/storage/manifests.ts b/src/storage/manifests.ts index 530d43b8..8a986510 100644 --- a/src/storage/manifests.ts +++ b/src/storage/manifests.ts @@ -246,11 +246,14 @@ export async function readManifestIfExists( return readManifestInternal(path, true); } -export async function writeDestroyMarker( +// An empty marker file, written with no mkdir: when the session folder is +// gone, the write fails with SESSION_NOT_FOUND instead of creating it again. +async function writeEmptyMarker( path: string, sessionId: string, + label: string, ): Promise { - assertAbsoluteStoragePath(path, 'destroy marker path'); + assertAbsoluteStoragePath(path, `${label} path`); try { await replaceFileAtomic(path, ''); @@ -264,13 +267,27 @@ export async function writeDestroyMarker( } throw makeCliError(ERROR_CODES.STORAGE_WRITE_ERROR, { - message: `Failed to write the destroy marker at ${path}.`, + message: `Failed to write the ${label} at ${path}.`, details: { path }, cause: error, }); } } +export async function writeDestroyMarker( + path: string, + sessionId: string, +): Promise { + await writeEmptyMarker(path, sessionId, 'destroy marker'); +} + +export async function writeCreateFailedMarker( + path: string, + sessionId: string, +): Promise { + await writeEmptyMarker(path, sessionId, 'create-failed marker'); +} + export async function writeManifest( path: string, record: SessionRecord, diff --git a/src/storage/sessionPaths.ts b/src/storage/sessionPaths.ts index 345b9cec..c007591b 100644 --- a/src/storage/sessionPaths.ts +++ b/src/storage/sessionPaths.ts @@ -3,6 +3,7 @@ import { basename, dirname, isAbsolute, resolve } from 'node:path'; import process from 'node:process'; import { + CREATE_FAILED_MARKER_FILENAME, DESTROY_MARKER_FILENAME, EVENT_LOG_FILENAME, HOST_RECORD_FILENAME, @@ -86,6 +87,12 @@ export function destroyMarkerPath(sessionDirectory: string): string { return childPath(sessionDirectory, DESTROY_MARKER_FILENAME); } +// A create that gave up writes this empty file, so a host that the manifest +// does not name yet stops its session itself. +export function createFailedMarkerPath(sessionDirectory: string): string { + return childPath(sessionDirectory, CREATE_FAILED_MARKER_FILENAME); +} + // One root per uid, so that no other user can create a Home's folder in it. // Windows has no uid and keeps the shared root. export function socketRootDirectory(): string { diff --git a/test/fixtures/fake-win32-create.ts b/test/fixtures/fake-win32-create.ts new file mode 100644 index 00000000..6d2848e8 --- /dev/null +++ b/test/fixtures/fake-win32-create.ts @@ -0,0 +1,15 @@ +// Preload (`--import`) for a `create` process: process.platform reads win32, +// and ChildProcess.kill sends SIGKILL, as TerminateProcess ends a Windows +// process at once without its handlers. The host that create spawns inherits +// NODE_OPTIONS and skips this, so it runs as a Linux host. +import { ChildProcess } from 'node:child_process'; +import process from 'node:process'; + +if (process.argv.includes('create') && !process.argv.includes('_host')) { + Object.defineProperty(process, 'platform', { value: 'win32' }); + const kill = Object.getOwnPropertyDescriptor(ChildProcess.prototype, 'kill') + ?.value as (this: ChildProcess, signal: NodeJS.Signals) => boolean; + ChildProcess.prototype.kill = function killAtOnce(this: ChildProcess) { + return kill.call(this, 'SIGKILL'); + }; +} diff --git a/test/integration/lifecycle.test.ts b/test/integration/lifecycle.test.ts index 8b6cb904..9395091a 100644 --- a/test/integration/lifecycle.test.ts +++ b/test/integration/lifecycle.test.ts @@ -1716,6 +1716,15 @@ describe('lifecycle integration', { timeout: 60_000 }, () => { `--import=${pathToFileURL(resolve('test/fixtures/denied-autogroup.ts')).href}`, ], ], + // create skips the SIGTERM while the manifest names no program, and the + // host finds the create-failed marker after its start-up write. + [ + 'on Windows, where SIGTERM ends the host at once', + [ + `--import=${pathToFileURL(resolve('test/fixtures/denied-autogroup.ts')).href}`, + `--import=${pathToFileURL(resolve('test/fixtures/fake-win32-create.ts')).href}`, + ], + ], ])('a create that gives up %s', (_name, preloads) => { let pgid = 0; @@ -1808,6 +1817,79 @@ describe('lifecycle integration', { timeout: 60_000 }, () => { }, 60_000); }); + // The held event-log open keeps the host between its manifest read and its + // spawn until create has given up. On Windows create sends no SIGTERM while + // the manifest names no program, so only the marker stops the spawn. + it('a create that gives up on Windows before its host spawns leaves no program', async () => { + const holdLog = join(testHome, 'hold.log'); + const release = join(testHome, 'release'); + const creator = spawn( + process.execPath, + [ + '--import', + 'tsx', + './src/cli/main.ts', + 'create', + '--json', + '--', + '/bin/sh', + '-c', + 'trap "" HUP; sleep 300 & wait', + ], + { + stdio: ['ignore', 'pipe', 'ignore'], + env: { + ...process.env, + AGENT_TTY_HOME: testHome, + REPLAY_LOG: holdLog, + REPLAY_RELEASE: release, + REPLAY_HOLD: '_host:open:/events.jsonl:1:60000', + NODE_OPTIONS: [ + process.env.NODE_OPTIONS ?? '', + `--import=${pathToFileURL(resolve('.agents/skills/verify-session-lifecycle/scripts/replay-hold.mjs')).href}`, + `--import=${pathToFileURL(resolve('test/fixtures/fake-win32-create.ts')).href}`, + ].join(' '), + }, + }, + ); + let stdout = ''; + creator.stdout.on('data', (chunk: Buffer) => { + stdout += chunk.toString('utf8'); + }); + const creatorExit = once(creator, 'exit'); + const deadline = Date.now() + 15_000; + while ( + !(await readFile(holdLog, 'utf8').then( + (log) => log.includes('hold open'), + () => false, + )) + ) { + expect(Date.now()).toBeLessThan(deadline); + await sleep(50); + } + const [sessionId] = await readdir(join(testHome, 'sessions')); + const directory = sessionDir(testHome, sessionId as string); + const hostPid = SessionRecordSchema.parse( + JSON.parse(await readFile(manifestPath(directory), 'utf8')), + ).hostPid as number; + + const [code] = (await creatorExit) as [number | null]; + await writeFile(release, ''); + + expect(code).toBe(6); + expect(JSON.parse(stdout)).toMatchObject({ + ok: false, + error: { code: 'HOST_UNREACHABLE', details: { sessionId } }, + }); + const stopDeadline = Date.now() + 20_000; + while (isProcessAlive(hostPid)) { + expect(Date.now()).toBeLessThan(stopDeadline); + await sleep(100); + } + expect(existsSync(programRecordPath(directory))).toBe(false); + expect(inspectSession(testHome, sessionId as string).status).toBe('failed'); + }, 60_000); + // A SIGTERM that the default action would turn into a host death between // the spawn and program.json leaves a program that no file names. describe('a host that gets SIGTERM right after it spawns the program', () => { diff --git a/test/unit/commands/create.test.ts b/test/unit/commands/create.test.ts index 9f6ea63e..c1f5087a 100644 --- a/test/unit/commands/create.test.ts +++ b/test/unit/commands/create.test.ts @@ -16,7 +16,9 @@ const mocks = vi.hoisted(() => ({ stopHost: vi.fn(), sendRpc: vi.fn(), readManifestIfExists: vi.fn(), + writeCreateFailedMarker: vi.fn(), sessionDir: vi.fn(), + createFailedMarkerPath: vi.fn(), manifestPath: vi.fn(), socketPath: vi.fn(), upsertHome: vi.fn(), @@ -46,9 +48,11 @@ vi.mock('../../../src/host/rpcClient.js', () => ({ vi.mock('../../../src/storage/manifests.js', () => ({ readManifestIfExists: mocks.readManifestIfExists, + writeCreateFailedMarker: mocks.writeCreateFailedMarker, })); vi.mock('../../../src/storage/sessionPaths.js', () => ({ + createFailedMarkerPath: mocks.createFailedMarkerPath, sessionDir: mocks.sessionDir, manifestPath: mocks.manifestPath, socketPath: mocks.socketPath, @@ -132,6 +136,10 @@ describe('create command', () => { mocks.socketPath.mockImplementation( (sessionDirectory: string) => `${sessionDirectory}/rpc.sock`, ); + mocks.createFailedMarkerPath.mockImplementation( + (sessionDirectory: string) => `${sessionDirectory}/create-failed`, + ); + mocks.writeCreateFailedMarker.mockResolvedValue(undefined); mocks.upsertHome.mockResolvedValue(undefined); }); @@ -307,6 +315,57 @@ describe('create command', () => { expect(mocks.emitSuccess).not.toHaveBeenCalled(); }); + it('writes the create-failed marker before it reads the manifest for the stop', async () => { + mocks.sendRpc.mockRejectedValue( + makeCliError(ERROR_CODES.HOST_UNREACHABLE, { + message: 'host unreachable', + }), + ); + + await expect( + runCreateCommand({ ...baseOptions, context }), + ).rejects.toMatchObject({ code: 'HOST_UNREACHABLE' }); + + expect(mocks.writeCreateFailedMarker).toHaveBeenCalledOnce(); + expect(mocks.writeCreateFailedMarker).toHaveBeenCalledWith( + '/tmp/agent-tty-home/sessions/session-01/create-failed', + 'session-01', + ); + const markerOrder = + mocks.writeCreateFailedMarker.mock.invocationCallOrder[0] ?? 0; + const lastRead = Math.max( + ...mocks.readManifestIfExists.mock.invocationCallOrder, + ); + expect(markerOrder).toBeGreaterThan(0); + expect(markerOrder).toBeLessThan(lastRead); + expect(mocks.stopHost).toHaveBeenCalledOnce(); + }); + + it('keeps the error and still stops the host when the marker write fails', async () => { + mocks.sendRpc.mockRejectedValue( + makeCliError(ERROR_CODES.HOST_UNREACHABLE, { + message: 'host unreachable', + }), + ); + mocks.writeCreateFailedMarker.mockRejectedValue( + makeCliError(ERROR_CODES.STORAGE_WRITE_ERROR, { message: 'disk full' }), + ); + + await expect( + runCreateCommand({ ...baseOptions, context }), + ).rejects.toMatchObject({ + code: 'HOST_UNREACHABLE', + details: { sessionId: 'session-01' }, + }); + expect(mocks.stopHost).toHaveBeenCalledOnce(); + }); + + it('leaves no create-failed marker for a create that succeeds', async () => { + await runCreateCommand({ ...baseOptions, context }); + + expect(mocks.writeCreateFailedMarker).not.toHaveBeenCalled(); + }); + it('keeps the details of the last error and adds the session id', async () => { mocks.sendRpc.mockRejectedValue( makeCliError(ERROR_CODES.HOST_TIMEOUT, { diff --git a/test/unit/host/hostMarker.test.ts b/test/unit/host/hostMarker.test.ts new file mode 100644 index 00000000..43a90b74 --- /dev/null +++ b/test/unit/host/hostMarker.test.ts @@ -0,0 +1,129 @@ +import { existsSync, readFileSync, writeFileSync } from 'node:fs'; +import { mkdir, mkdtemp, rm } from 'node:fs/promises'; +import { tmpdir } from 'node:os'; +import { join } from 'node:path'; +import process from 'node:process'; +import { setTimeout as delay } from 'node:timers/promises'; + +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; + +import { runHost } from '../../../src/host/hostMain.js'; +import type * as ProgramGroupModule from '../../../src/host/programGroup.js'; +import type { SessionRecord } from '../../../src/protocol/schemas.js'; +import { + readManifest, + writeCreateFailedMarker, + writeManifest, +} from '../../../src/storage/manifests.js'; +import { + createFailedMarkerPath, + manifestPath, + programRecordPath, + sessionDir, + socketPath, +} from '../../../src/storage/sessionPaths.js'; +import { isGroupAlive, killGroupIfAlive } from '../../helpers.js'; + +// Runs after the host's start-up write of program.json, where a create that +// gives up can write its marker. +const hooks = vi.hoisted(() => ({ afterProgramRecord: (): void => {} })); + +vi.mock('../../../src/host/programGroup.js', async (importOriginal) => { + const actual = await importOriginal(); + return { + ...actual, + writeProgramRecord: ( + ...args: Parameters + ): void => { + actual.writeProgramRecord(...args); + hooks.afterProgramRecord(); + }, + }; +}); + +const SESSION_ID = 'session-01'; + +let home = ''; +let directory = ''; +let sigtermListeners: ReadonlySet = new Set(); + +function readProgramGroup(): number { + const { pid } = JSON.parse( + readFileSync(programRecordPath(directory), 'utf8'), + ) as { pid: number }; + expect(pid).toBeGreaterThan(1); + return pid; +} + +beforeEach(async () => { + home = await mkdtemp(join(tmpdir(), 'agent-tty-host-marker-')); + vi.stubEnv('AGENT_TTY_HOME', home); + directory = sessionDir(home, SESSION_ID); + await mkdir(directory, { recursive: true, mode: 0o700 }); + const record: SessionRecord = { + version: 1, + sessionId: SESSION_ID, + createdAt: '2026-11-04T00:00:00.000Z', + updatedAt: '2026-11-04T00:00:00.000Z', + status: 'running', + command: ['/bin/sh', '-c', 'exec sleep 30'], + cwd: home, + cols: 80, + rows: 24, + hostPid: process.pid, + childPid: null, + exitCode: null, + exitSignal: null, + }; + await writeManifest(manifestPath(directory), record); + sigtermListeners = new Set(process.rawListeners('SIGTERM')); +}); + +afterEach(async () => { + hooks.afterProgramRecord = () => {}; + // runHost adds a SIGTERM listener that would outlive the test. + for (const listener of process.rawListeners('SIGTERM')) { + if (!sigtermListeners.has(listener)) { + process.removeListener('SIGTERM', listener); + } + } + if (existsSync(programRecordPath(directory))) { + killGroupIfAlive(readProgramGroup()); + } + vi.unstubAllEnvs(); + await rm(home, { recursive: true, force: true }); +}); + +describe('runHost and the create-failed marker', () => { + it('spawns no program when the marker exists before the spawn', async () => { + await writeCreateFailedMarker( + createFailedMarkerPath(directory), + SESSION_ID, + ); + + await runHost(SESSION_ID, process.pid); + + expect(existsSync(programRecordPath(directory))).toBe(false); + const manifest = await readManifest(manifestPath(directory)); + expect(manifest.status).toBe('running'); + expect(manifest.childPid).toBeNull(); + }); + + it('stops its program and never listens when the marker appears during the start-up write', async () => { + hooks.afterProgramRecord = () => { + writeFileSync(createFailedMarkerPath(directory), ''); + }; + + await runHost(SESSION_ID, process.pid); + + expect(existsSync(socketPath(directory))).toBe(false); + const group = readProgramGroup(); + const deadline = Date.now() + 10_000; + let manifest = await readManifest(manifestPath(directory)); + while (manifest.status !== 'destroyed' || isGroupAlive(group)) { + expect(Date.now()).toBeLessThan(deadline); + await delay(50); + manifest = await readManifest(manifestPath(directory)); + } + }); +}); diff --git a/test/unit/storage/markers.test.ts b/test/unit/storage/markers.test.ts new file mode 100644 index 00000000..8f7624c0 --- /dev/null +++ b/test/unit/storage/markers.test.ts @@ -0,0 +1,64 @@ +import { mkdirSync, readFileSync, writeFileSync } from 'node:fs'; +import { mkdtemp, rm } from 'node:fs/promises'; +import { tmpdir } from 'node:os'; +import { join } from 'node:path'; + +import { afterEach, beforeEach, describe, expect, it } from 'vitest'; + +import { + writeCreateFailedMarker, + writeDestroyMarker, +} from '../../../src/storage/manifests.js'; + +let directory = ''; + +beforeEach(async () => { + directory = await mkdtemp(join(tmpdir(), 'agent-tty-markers-')); +}); + +afterEach(async () => { + await rm(directory, { recursive: true, force: true }); +}); + +describe.each([ + ['writeDestroyMarker', writeDestroyMarker, 'destroyed', 'destroy marker'], + [ + 'writeCreateFailedMarker', + writeCreateFailedMarker, + 'create-failed', + 'create-failed marker', + ], +])('%s', (_name, write, file, label) => { + it('writes an empty file', async () => { + const path = join(directory, file); + + await write(path, 'session-01'); + + expect(readFileSync(path, 'utf8')).toBe(''); + }); + + it('reports SESSION_NOT_FOUND when the session folder is gone', async () => { + const path = join(directory, 'gone', file); + + await expect(write(path, 'session-01')).rejects.toMatchObject({ + code: 'SESSION_NOT_FOUND', + details: { sessionId: 'session-01', markerPath: path }, + }); + }); + + it('reports STORAGE_WRITE_ERROR when the rename fails', async () => { + const path = join(directory, file); + mkdirSync(path); + writeFileSync(join(path, 'keep'), ''); + + await expect(write(path, 'session-01')).rejects.toMatchObject({ + code: 'STORAGE_WRITE_ERROR', + message: `Failed to write the ${label} at ${path}.`, + details: { path }, + }); + }); + + it('rejects a relative path', async () => { + await expect(write(file, 'session-01')).rejects.toThrow(`${label} path`); + }); +});