From 451ec1d84a91d2df480d0529c7abd7082a12757b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Baldur=20P=C3=A1ll=20H=C3=B3lmgeirsson?= Date: Fri, 2 Oct 2026 19:54:18 +0000 Subject: [PATCH] fix(cleanup): keep a runtime-done session live while its process runs The runtime has been seen reporting `state: "done"` for a session whose pid was still running with its cwd in the worktree, so `cleanup` swept the worktree as ordinary stale work. A `done` is now believed only when no process is running behind it: where the reported pid is a positive integer and `kill(pid, 0)` finds it, the `done` is dropped when the runtime listing is read, and the session is weighed like any other live one. Idle still does not hold `cleanup` back (#89); busy, unrecognised or no status does. `list --agents` no longer marks such a session `[done]`. Closes #73 --- .changeset/stale-pids-linger.md | 5 + README.md | 5 +- context/executors.md | 2 +- context/verify.md | 2 +- docs/src/app/docs/commands/cleanup/page.mdx | 4 +- docs/src/app/docs/commands/list/page.mdx | 8 +- skills/worktree/SKILL.md | 2 +- src/lib/agent.test.ts | 160 +++++++++++++++++++- src/lib/agent.ts | 36 ++++- src/lib/cli.test.ts | 61 ++++++++ src/lib/cli.ts | 31 ++++ src/lib/git.ts | 4 +- src/test-setup.ts | 7 + 13 files changed, 312 insertions(+), 15 deletions(-) create mode 100644 .changeset/stale-pids-linger.md diff --git a/.changeset/stale-pids-linger.md b/.changeset/stale-pids-linger.md new file mode 100644 index 0000000..5338f96 --- /dev/null +++ b/.changeset/stale-pids-linger.md @@ -0,0 +1,5 @@ +--- +"@northguild/worktree": patch +--- + +`worktree cleanup` no longer removes a worktree whose agent session the runtime reports as done while its process is still running. Such a session now counts as live, so it holds the worktree back unless it is idle, and `worktree list --agents` no longer marks it `[done]`. diff --git a/README.md b/README.md index acfe220..f86e538 100644 --- a/README.md +++ b/README.md @@ -160,7 +160,7 @@ To name the agent session living in each worktree: worktree list --agents ``` -Sessions are found from two places, joined on the directory they run in: `herdr agent list` when `herdr` is installed, and the runtime's own ` agents --json`, where the program is `agent.command`'s first word or else `herdr.agent`. A finished session shows `[done]`, unless Herdr still shows it in a pane. With neither source available the list simply shows no sessions. +Sessions are found from two places, joined on the directory they run in: `herdr agent list` when `herdr` is installed, and the runtime's own ` agents --json`, where the program is `agent.command`'s first word or else `herdr.agent`. A finished session shows `[done]`, unless Herdr still shows it in a pane or its process is still running. With neither source available the list simply shows no sessions. ### Reopen a worktree in your editor @@ -360,7 +360,8 @@ receipt**: check it, not just the exit code, when a brief matters. They are joined on the directory each session runs in, compared as real paths, and a Herdr entry is named by the runtime session with the same session id. A session without a `pid` is kept. `live` is `false` for a -session the runtime reports as finished (`[done]` in the text list), unless Herdr still shows it in a pane. With +session the runtime reports as finished (`[done]` in the text list), unless Herdr still shows it in a pane or the +process behind it is still running. With neither source, the result is no agents and no error. `cleanup` holds a worktree back for any session that is still `live` and not just idle; a finished or idle one does not hold it back. A session counts as idle when the runtime's `status` or Herdr's status is `idle` (or Herdr's `done`) and none of the statuses it has says otherwise. ### A coordinator driving worker agents diff --git a/context/executors.md b/context/executors.md index 95e5eb2..61822db 100644 --- a/context/executors.md +++ b/context/executors.md @@ -115,7 +115,7 @@ always did. Sessions are found from two sources joined on the real path of their list` when `herdr` is on PATH, and the runtime's ` agents --json`, where the program is `agent.command`'s head or else `herdr.agent`. So a repository with only `herdr.agent` set is covered, and a Herdr entry is named by the runtime session it matches. What comes back per worktree is a session name -(a pid only where the runtime has one), and markers for live, interactive, waiting and `[done]` (a runtime-finished session that Herdr still shows in a pane stays live). With +(a pid only where the runtime has one), and markers for live, interactive, waiting and `[done]` (a runtime-finished session stays live when Herdr still shows it in a pane or its process is still running). With neither source available the answer is no sessions, not an error — which is "no answer", not "nobody is there"; see the next paragraph. diff --git a/context/verify.md b/context/verify.md index aaea322..c4d04d0 100644 --- a/context/verify.md +++ b/context/verify.md @@ -66,7 +66,7 @@ interactive shell. If a console assertion ever starts failing only on someone's first. `pnpm test` covers `src/` and `scripts/`; `pnpm docs:test` covers the `docs` workspace including the Cloudflare Worker. -CI runs both. As of 2026-10-01 that is 960 and 66 tests respectively — a snapshot for recognising a suite +CI runs both. As of 2026-10-02 that is 987 and 68 tests respectively — a snapshot for recognising a suite that did not run, not a figure to assert against. It goes stale on any commit that adds a test, so correct it in passing rather than treating a mismatch as a failure. diff --git a/docs/src/app/docs/commands/cleanup/page.mdx b/docs/src/app/docs/commands/cleanup/page.mdx index cad0c09..4da651a 100644 --- a/docs/src/app/docs/commands/cleanup/page.mdx +++ b/docs/src/app/docs/commands/cleanup/page.mdx @@ -118,7 +118,9 @@ and deleting it out from under either is the failure this check exists to preven A session that has finished does not hold anything back — the worktree goes back to being ordinary stale work. A session the runtime calls finished but Herdr still shows in a pane is judged by -what Herdr says about it. +what Herdr says about it, and one whose process is still running is judged as any live session is, by +its status below. The runtime has been seen to report a session finished while its process was still +working in the worktree. Nor does a session that is only idle, such as an agent pane left open at its prompt in a merged worktree. The runtime's `idle` and Herdr's `idle` both count, and so does Herdr's `done`, a finished diff --git a/docs/src/app/docs/commands/list/page.mdx b/docs/src/app/docs/commands/list/page.mdx index 7c4ac50..a8f5244 100644 --- a/docs/src/app/docs/commands/list/page.mdx +++ b/docs/src/app/docs/commands/list/page.mdx @@ -50,9 +50,11 @@ not list is named `@`, for example `claude@w5:p1`. - **`[waiting]`** — a background agent that is live but not progressing, so it is probably waiting on you. - **`[done]`** — a session the runtime reports as finished. It no longer holds anything back, so - `cleanup` treats the worktree as ordinary stale work. The exception is a session Herdr still shows in - a pane: that one is not marked `[done]`, and it holds the worktree back only while Herdr reports it as - anything other than idle or done. + `cleanup` treats the worktree as ordinary stale work. The exceptions are a session Herdr still shows in + a pane and one whose process is still running, which the runtime has been seen to report as finished: + those are not marked `[done]`. A session Herdr shows holds the worktree back only while Herdr reports it + as anything other than idle or done; a running one is `[waiting]` when it is idle and otherwise holds + the worktree back. - **`[interactive]`** — somebody's own terminal session, not an agent this tool dispatched. It is listed because `cleanup` will refuse to remove a worktree somebody is working in, and this is where you find out which those are. One that is only idle does diff --git a/skills/worktree/SKILL.md b/skills/worktree/SKILL.md index becb83c..ec15abc 100644 --- a/skills/worktree/SKILL.md +++ b/skills/worktree/SKILL.md @@ -280,7 +280,7 @@ worktree list --agents worktree list -a # alias # Sessions come from `herdr agent list` (when herdr is on PATH) and from # ` agents --json`, joined on the real -# path of their directory. A finished session shows [done] (unless Herdr still shows it in a pane); with no source +# path of their directory. A finished session shows [done] (unless Herdr still shows it in a pane or its process is still running); with no source # available there are simply no sessions. # Reopen a worktree in your editor diff --git a/src/lib/agent.test.ts b/src/lib/agent.test.ts index 13ed290..5c5c73a 100644 --- a/src/lib/agent.test.ts +++ b/src/lib/agent.test.ts @@ -1,7 +1,12 @@ import { mkdtempSync, realpathSync, rmSync, symlinkSync } from "node:fs"; import { tmpdir } from "node:os"; import { join } from "node:path"; -import { expectCommands, mockRun, mockRunCapturing } from "../test-setup.js"; +import { + expectCommands, + mockIsProcessRunning, + mockRun, + mockRunCapturing, +} from "../test-setup.js"; import { findSessionForPath, getAgentSessions, @@ -656,6 +661,159 @@ describe("getAgentSessions across Herdr and the runtime", () => { }); }); +// #73: the runtime has been seen reporting `done` for a session whose process +// was still running in the worktree. The probe is the mocked cli helper, so no +// result here depends on the process table of the machine running the suite. +describe("a runtime done state with a process behind it", () => { + beforeEach(() => { + vi.clearAllMocks(); + mockConfig({ "agent.command": "claude --bg" }); + vi.spyOn(cli, "commandExists").mockResolvedValue(false); + }); + + async function collect(...entries: Record[]) { + expectCommands("claude agents --json"); + mockRun.mockResolvedValueOnce(JSON.stringify(entries)); + return getAgentSessions(); + } + + const done = (overrides: Record = {}) => + runtimeEntry({ kind: "background", state: "done", ...overrides }); + + it("is live, waiting and not blocking when the pid is running and the status is idle", async () => { + mockIsProcessRunning.mockReturnValue(true); + + const [session] = await collect(done({ status: "idle" })); + + expect(mockIsProcessRunning).toHaveBeenCalledWith(9187); + expect(isSessionLive(session as AgentSession)).toBe(true); + expect(isSessionWaiting(session as AgentSession)).toBe(true); + expect(isSessionBlocking(session as AgentSession)).toBe(false); + }); + + it.each([ + ["busy", { status: "busy" }], + ["working", { status: "working" }], + ["unrecognised", { status: "dreaming" }], + ["null", { status: null }], + ["absent", { status: undefined }], + ])("is live and blocking when the pid is running and the status is %s", async (_label, overrides) => { + mockIsProcessRunning.mockReturnValue(true); + + const [session] = await collect(done(overrides)); + + expect(isSessionLive(session as AgentSession)).toBe(true); + expect(isSessionBlocking(session as AgentSession)).toBe(true); + }); + + it("stays finished when the pid is not running", async () => { + mockIsProcessRunning.mockReturnValue(false); + + const [session] = await collect(done({ status: "idle" })); + + expect(mockIsProcessRunning).toHaveBeenCalledWith(9187); + expect(isSessionLive(session as AgentSession)).toBe(false); + expect(isSessionWaiting(session as AgentSession)).toBe(false); + expect(isSessionBlocking(session as AgentSession)).toBe(false); + }); + + it("stays finished when there is no pid, without probing", async () => { + mockIsProcessRunning.mockReturnValue(true); + const { pid: _pid, ...withoutPid } = done(); + + const [session] = await collect(withoutPid); + + expect(mockIsProcessRunning).not.toHaveBeenCalled(); + expect(isSessionLive(session as AgentSession)).toBe(false); + }); + + it.each([ + 0, + -1, + 1.5, + "9187", + ])("treats pid %j as no pid: done stands and nothing is probed", async (pid) => { + mockIsProcessRunning.mockReturnValue(true); + + const [session] = await collect(done({ pid })); + + expect(mockIsProcessRunning).not.toHaveBeenCalled(); + expect(isSessionLive(session as AgentSession)).toBe(false); + }); + + it("does not probe a session that is not done", async () => { + await collect( + runtimeEntry({ kind: "background", state: "working" }), + runtimeEntry({ kind: "interactive" }), + ); + + expect(mockIsProcessRunning).not.toHaveBeenCalled(); + }); + + it("blocks the worktree through findSessionForPath, and a working sibling still wins", async () => { + mockIsProcessRunning.mockImplementation((pid: number) => pid === 9187); + + const sessions = await collect( + done({ pid: 9187, name: "stale-done", status: "busy" }), + done({ pid: 4, name: "really-done", status: "idle" }), + ); + const found = findSessionForPath(sessions, worktreePath); + + expect(found?.name).toBe("stale-done"); + expect(isSessionBlocking(found as AgentSession)).toBe(true); + + const withWorker = await collect( + done({ pid: 9187, name: "stale-done", status: "idle" }), + runtimeEntry({ + kind: "background", + name: "worker", + pid: 5, + state: "working", + }), + ); + expect(findSessionForPath(withWorker, worktreePath)?.name).toBe("worker"); + }); + + it("leaves a finished session with no running process out of the way of an idle live one", async () => { + mockIsProcessRunning.mockImplementation((pid: number) => pid === 9187); + + const sessions = await collect( + done({ pid: 4, name: "gone" }), + done({ pid: 9187, name: "stale-done", status: "idle" }), + ); + + expect(findSessionForPath(sessions, worktreePath)?.name).toBe("stale-done"); + }); + + it("still joins to Herdr as before: the done is dropped and both signals are weighed", async () => { + mockIsProcessRunning.mockReturnValue(true); + mockConfig({ "herdr.agent": "claude" }); + vi.spyOn(cli, "commandExists").mockResolvedValue(true); + expectCommands("claude agents --json"); + mockRun.mockResolvedValueOnce(JSON.stringify([done({ status: "idle" })])); + mockHerdrAgents([herdrAgent({ agent_status: "working" })]); + + const [session] = await getAgentSessions(); + + expect(session?.herdrAgent).toBe("wA:p1"); + expect(isSessionLive(session as AgentSession)).toBe(true); + expect(isSessionBlocking(session as AgentSession)).toBe(true); + }); + + it("joined to Herdr, stays live even when the pid is not running", async () => { + mockIsProcessRunning.mockReturnValue(false); + mockConfig({ "herdr.agent": "claude" }); + vi.spyOn(cli, "commandExists").mockResolvedValue(true); + expectCommands("claude agents --json"); + mockRun.mockResolvedValueOnce(JSON.stringify([done()])); + mockHerdrAgents([herdrAgent()]); + + const [session] = await getAgentSessions(); + + expect(isSessionLive(session as AgentSession)).toBe(true); + }); +}); + describe("findSessionForPath", () => { it("matches a session whose cwd is the worktree itself", () => { const target = session(); diff --git a/src/lib/agent.ts b/src/lib/agent.ts index 7638420..9d9dfa0 100644 --- a/src/lib/agent.ts +++ b/src/lib/agent.ts @@ -5,7 +5,7 @@ import { isHerdrInstalled, listHerdrAgents, } from "../integrations/herdr.js"; -import { run } from "./cli.js"; +import { isProcessRunning, run } from "./cli.js"; import { gitGetConfigValue } from "./git.js"; import type { AgentSession } from "./types.js"; import { splitCommandValue } from "./utils.js"; @@ -42,6 +42,30 @@ function readOptionalString( return typeof value === "string" ? value : undefined; } +// A runtime `done` is believed only when no process is behind it (#73). The +// runtime has been seen reporting `done` for a session whose process was still +// running with its cwd in the worktree, so a `done` that carries a running pid is +// dropped, as `joinSessions` drops one Herdr still shows: the contrary evidence +// wins in the safe direction, and the session is then weighed like any other +// live one — idle does not hold `cleanup` back, anything else does. The pid is +// another program's JSON, so only a positive integer is probed; 0 and negatives +// address process groups, and anything else is no pid. No pid, an unusable one, +// or one that is gone leaves `done` standing. The probe lives here, where +// sessions are collected, so the predicates below stay pure. +function withoutStaleDone(session: AgentSession): AgentSession { + const { pid } = session; + if ( + session.state !== "done" || + pid === undefined || + !Number.isSafeInteger(pid) || + pid <= 0 || + !isProcessRunning(pid) + ) { + return session; + } + return { ...session, state: undefined }; +} + // Only `cwd` is load-bearing: it is the join key, so an entry without one // cannot match a worktree. `name` and `pid` are rendering fields, and dropping // an entry over one would yield no session where one exists — the direction @@ -126,7 +150,8 @@ async function listRuntimeSessions(): Promise { } return parsed .map(toAgentSession) - .filter((session): session is AgentSession => session !== undefined); + .filter((session): session is AgentSession => session !== undefined) + .map(withoutStaleDone); } catch { return []; } @@ -174,7 +199,9 @@ function herdrOnlySession(entry: HerdrAgentEntry): AgentSession { // beside the runtime's own so `isSessionBlocking` weighs both, except that a // runtime `done` is dropped: Herdr still shows the session in a pane, and a live // process has been seen reported `done` by the runtime, so the contrary evidence -// wins in the safe direction. A Herdr entry nothing matched stands on its own. +// wins in the safe direction. A `done` whose own pid is still running is already +// dropped when the runtime listing is read (#73); Herdr covers the one with no +// pid to probe. A Herdr entry nothing matched stands on its own. // Every cwd is already a real path. function joinSessions( runtime: AgentSession[], @@ -270,7 +297,8 @@ export function findSessionForPath( // Fails safe: an absent or unrecognised state counts as live, so a session whose // shape we no longer recognise blocks removal rather than being ignored. That is // also what makes an interactive session live, since those carry no state at -// all. See D6 and §4.1. +// all. See D6 and §4.1. A runtime `done` reaches here believed only when no +// process is running behind it, and a Herdr-joined one not at all (#73, #70). export function isSessionLive(session: AgentSession): boolean { return session.state !== "done"; } diff --git a/src/lib/cli.test.ts b/src/lib/cli.test.ts index 6044281..7d63cd1 100644 --- a/src/lib/cli.test.ts +++ b/src/lib/cli.test.ts @@ -11,6 +11,7 @@ import { tmpdir } from "node:os"; import { basename, delimiter, dirname, join } from "node:path"; import { commandExists, + isProcessRunning, KILL_GRACE_MS, run, runCapturing, @@ -461,3 +462,63 @@ describe("env option", () => { await expect(run(node, ["-e", printVar])).resolves.toBe(""); }); }); + +// #73. process.kill is spied wherever the answer matters, so nothing here +// depends on which pids exist on the machine, and no signal is ever delivered. +describe("isProcessRunning", () => { + afterEach(() => { + vi.restoreAllMocks(); + }); + + function killFailsWith(code: string) { + return vi.spyOn(process, "kill").mockImplementation(() => { + throw Object.assign(new Error(code), { code }); + }); + } + + it("probes with signal 0 and reports a pid that exists as running", () => { + const kill = vi.spyOn(process, "kill").mockReturnValue(true); + + expect(isProcessRunning(4242)).toBe(true); + expect(kill).toHaveBeenCalledWith(4242, 0); + }); + + it("reports the current process as running, for real", () => { + expect(isProcessRunning(process.pid)).toBe(true); + }); + + it("reports ESRCH as not running", () => { + killFailsWith("ESRCH"); + + expect(isProcessRunning(4242)).toBe(false); + }); + + it("reports EPERM as running: the process exists, under another user", () => { + killFailsWith("EPERM"); + + expect(isProcessRunning(4242)).toBe(true); + }); + + it("fails safe, as running, on an error it does not understand", () => { + killFailsWith("EINVAL"); + expect(isProcessRunning(4242)).toBe(true); + + vi.spyOn(process, "kill").mockImplementation(() => { + throw "not an error"; + }); + expect(isProcessRunning(4242)).toBe(true); + }); + + it.each([ + 0, + -1, + 1.5, + Number.NaN, + Number.POSITIVE_INFINITY, + ])("never signals %s: it addresses a group or is no pid", (pid) => { + const kill = vi.spyOn(process, "kill").mockReturnValue(true); + + expect(isProcessRunning(pid)).toBe(false); + expect(kill).not.toHaveBeenCalled(); + }); +}); diff --git a/src/lib/cli.ts b/src/lib/cli.ts index 9b485ef..d1ef6bd 100644 --- a/src/lib/cli.ts +++ b/src/lib/cli.ts @@ -243,3 +243,34 @@ export function spawnDetached( child.on("error", (error: Error) => onError?.(error)); child.unref(); } + +/** + * Whether a process with this pid exists, probed with signal 0: the kernel + * checks the pid and the permission to signal it, and delivers nothing. + * + * Only a positive integer is a pid. 0 and negative values address process + * groups in `kill(2)`, and a non-integer is not a pid at all, so none of them + * is ever passed to it — they are reported as not running, because there is no + * process to ask about. The pid usually comes from another program's JSON, so + * the check is here rather than left to the caller. + * + * ESRCH is the one answer that means "no such process". EPERM means it exists + * but belongs to another user, which is running. Any other error is not + * understood, and a caller that is deciding whether something may be deleted + * has to fail in the safe direction, so it counts as running too. + */ +export function isProcessRunning(pid: number): boolean { + if (!Number.isSafeInteger(pid) || pid <= 0) { + return false; + } + try { + process.kill(pid, 0); + return true; + } catch (error) { + return !( + error instanceof Error && + "code" in error && + error.code === "ESRCH" + ); + } +} diff --git a/src/lib/git.ts b/src/lib/git.ts index e7e8d75..151283a 100644 --- a/src/lib/git.ts +++ b/src/lib/git.ts @@ -253,7 +253,9 @@ export async function gitGetWorktrees({ // terminal, which D5 treats alike. Fails safe on an absent marker: an entry that // records a session without saying whether it finished counts as live, so a // hand-built entry and a runtime that renamed its state field both block removal -// rather than being waved through. See AGENT-MODE-PLAN §3 D5/D6. +// rather than being waved through. A runtime `done` only marks a session +// finished when no process is running behind it (#73). See AGENT-MODE-PLAN §3 +// D5/D6. // // A session that is only idle does not count (#89): it holds nothing back, and // `cleanup` closes the pane with the worktree. `blocks` fails the same way diff --git a/src/test-setup.ts b/src/test-setup.ts index cf7eb94..b5ed960 100644 --- a/src/test-setup.ts +++ b/src/test-setup.ts @@ -11,6 +11,7 @@ const mockRun: ReturnType = vi.fn(); const mockRunCapturing: ReturnType = vi.fn(); const mockSpawnDetached: ReturnType = vi.fn(); const mockRunStreaming: ReturnType = vi.fn(); +const mockIsProcessRunning: ReturnType = vi.fn(); let expectedCommands: string[] = []; // This factory replaces the whole module, so anything cli.js exports has to be @@ -21,6 +22,7 @@ vi.mock("./lib/cli.js", () => ({ commandExists: vi.fn().mockResolvedValue(true), runCapturing: mockRunCapturing, runStreaming: mockRunStreaming, + isProcessRunning: mockIsProcessRunning, })); // A run() call reads as its argv joined, with the cwd appended when one is @@ -54,6 +56,10 @@ beforeEach(() => { // promise, and fails somewhere unrelated to what it is testing. mockRunCapturing.mockResolvedValue({ stdout: "", stderr: "", exitCode: 0 }); mockRunStreaming.mockResolvedValue({ exitCode: 0 }); + // No pid is running unless a test says so. The fixtures carry arbitrary pids + // such as 9187, and whether one is alive on the machine running the suite must + // never change a result, so the real process table is never consulted. + mockIsProcessRunning.mockReturnValue(false); expectedCommands = []; }); @@ -136,6 +142,7 @@ export { captureOutput, expectCommands, loadConfig, + mockIsProcessRunning, mockRun, mockRunCapturing, mockRunStreaming,