diff --git a/packages/coding-agent/CHANGELOG.md b/packages/coding-agent/CHANGELOG.md index d71c4c22c..b8a86c626 100644 --- a/packages/coding-agent/CHANGELOG.md +++ b/packages/coding-agent/CHANGELOG.md @@ -4,7 +4,7 @@ ### Fixed -- Fixed `omp worktree clear` (without `--all`) deleting task-isolation sandboxes owned by subagents that are still running, discarding their uncaptured work; the `no live task owns it` reason was set from the mere presence of the `m` mount dir with no ownership check. `ensureIsolation` now writes a pid-stamped ownership marker into each sandbox and `worktree list`/`clear` report a sandbox as `live` (never removed without `--all`) while its owning process is alive, reclaiming only crashed leftovers ([#6761](https://github.com/can1357/oh-my-pi/issues/6761)). +- Fixed `omp worktree clear` (without `--all`) deleting task-isolation sandboxes owned by subagents that are still running, discarding their uncaptured work; the `no live task owns it` reason was set from the mere presence of the `m` mount dir with no ownership check. `ensureIsolation` now writes an ownership marker (pid plus a process start-time token that survives pid reuse) into each sandbox, and `worktree list`/`clear` report a sandbox as `live` (never removed without `--all`) while its owning process is alive, reclaiming only crashed leftovers ([#6761](https://github.com/can1357/oh-my-pi/issues/6761)). ## [17.1.5] - 2026-07-27 diff --git a/packages/coding-agent/src/task/isolation-ownership.ts b/packages/coding-agent/src/task/isolation-ownership.ts index c77485b86..01adcb4f9 100644 --- a/packages/coding-agent/src/task/isolation-ownership.ts +++ b/packages/coding-agent/src/task/isolation-ownership.ts @@ -7,6 +7,7 @@ * subagent's sandbox from a crashed run's leftover instead of deleting both. */ import * as path from "node:path"; +import { $ } from "bun"; /** Marker file written into a task-isolation base dir identifying its owner. */ export const ISOLATION_OWNER_FILE = ".omp-isolation-owner.json"; @@ -17,6 +18,43 @@ export interface IsolationOwner { pid: number; /** Task id the sandbox was materialised for. */ id: string; + /** + * Process-instance start-time token for {@link pid}, when the OS can report + * it. Distinguishes the owning process from an unrelated process that later + * inherits a recycled pid, so a crashed sandbox is never pinned live. + */ + startToken?: string; +} + +/** + * Boot-stable start-time token for `pid`, or `null` when the process is gone or + * the platform cannot report it. Read from the same source on write and + * validate so an exact string compare rejects a recycled pid. + * + * Linux reads `/proc//stat` field 22 (start time in clock ticks since + * boot); other Unixes shell out to `ps -o lstart`. Platforms that report + * neither (e.g. Windows) yield `null`, degrading to a pid-only liveness check. + */ +async function processStartToken(pid: number): Promise { + if (process.platform === "linux") { + let stat: string; + try { + stat = await Bun.file(`/proc/${pid}/stat`).text(); + } catch { + return null; + } + // The comm field (2) may embed spaces and parens, so parse the numeric + // fields after the final ')'. `starttime` is field 22 overall, i.e. the + // 20th token once `pid` and `(comm)` are dropped. + const commEnd = stat.lastIndexOf(")"); + if (commEnd < 0) return null; + const starttime = stat.slice(commEnd + 2).split(" ")[19]; + return starttime && starttime.length > 0 ? starttime : null; + } + const res = await $`ps -o lstart= -p ${pid}`.quiet().nothrow(); + if (res.exitCode !== 0) return null; + const started = res.text().trim(); + return started.length > 0 ? started : null; } /** @@ -26,7 +64,8 @@ export interface IsolationOwner { * `omp worktree clear` never sees an owner-less sandbox mid-creation. */ export async function writeIsolationOwner(baseDir: string, id: string): Promise { - const owner: IsolationOwner = { pid: process.pid, id }; + const startToken = await processStartToken(process.pid); + const owner: IsolationOwner = { pid: process.pid, id, ...(startToken ? { startToken } : {}) }; await Bun.write(path.join(baseDir, ISOLATION_OWNER_FILE), JSON.stringify(owner)); } @@ -37,7 +76,9 @@ export async function writeIsolationOwner(baseDir: string, id: string): Promise< * sandbox from before markers existed, both safe to reclaim. `process.kill(pid, * 0)` can fail with `EPERM` even when the process is alive, so only an explicit * `ESRCH` ("no such process") counts as dead; any other error is treated as - * alive to avoid deleting a sandbox that is actually in use. + * alive to avoid deleting a sandbox that is actually in use. When the marker + * carries a {@link IsolationOwner.startToken}, a live pid whose current token no + * longer matches is a recycled pid — a different process — and counts as dead. */ export async function hasLiveIsolationOwner(baseDir: string): Promise { let decoded: unknown; @@ -51,8 +92,15 @@ export async function hasLiveIsolationOwner(baseDir: string): Promise { if (typeof pid !== "number" || !Number.isInteger(pid) || pid <= 0) return false; try { process.kill(pid, 0); - return true; } catch (err) { - return (err as NodeJS.ErrnoException).code !== "ESRCH"; + if ((err as NodeJS.ErrnoException).code === "ESRCH") return false; } + // The pid is live (or unknowable via EPERM). Reject a recycled pid: if the + // marker pinned the owner's start-time token, the process wearing that pid + // now must still present the same token. + if ("startToken" in decoded && typeof decoded.startToken === "string" && decoded.startToken.length > 0) { + const current = await processStartToken(pid); + if (current !== null && current !== decoded.startToken) return false; + } + return true; } diff --git a/packages/coding-agent/test/cli/worktree-clear-isolation.test.ts b/packages/coding-agent/test/cli/worktree-clear-isolation.test.ts index 2c03a5387..15ed54df5 100644 --- a/packages/coding-agent/test/cli/worktree-clear-isolation.test.ts +++ b/packages/coding-agent/test/cli/worktree-clear-isolation.test.ts @@ -63,6 +63,14 @@ describe("worktree clear task-isolation ownership", () => { await fs.mkdir(pending, { recursive: true }); await writeIsolationOwner(pending, "pend0005"); + // Recycled pid: the crashed owner's pid was reassigned to this live test + // process, but the recorded start-time token no longer matches. + const recycled = await makeSandbox("trecyc06"); + await Bun.write( + path.join(recycled, ISOLATION_OWNER_FILE), + JSON.stringify({ pid: process.pid, id: "recyc06", startToken: "not-the-current-token" }), + ); + await clearWorktrees({ all: false, dryRun: false, json: true }); const exists = async (p: string): Promise => @@ -75,5 +83,6 @@ describe("worktree clear task-isolation ownership", () => { expect(await exists(orphan)).toBe(false); expect(await exists(corrupt)).toBe(false); expect(await exists(pending)).toBe(true); + expect(await exists(recycled)).toBe(false); }); });