fix(cli): bind isolation ownership to process start-time token
A crashed owner's pid can be recycled by an unrelated long-lived process, so kill(pid, 0) succeeds and the leftover sandbox was pinned live forever, unreachable by a non-`--all` clear. The ownership marker now records a process-instance start-time token alongside the pid (Linux /proc/<pid>/stat field 22, other Unix via `ps -o lstart`). A live pid whose current token no longer matches the recorded one is a recycled pid and counts as dead; platforms that can't report a token degrade to the prior pid-only check. Fixes #6761
This commit is contained in:
@@ -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
|
||||
|
||||
|
||||
@@ -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/<pid>/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<string | null> {
|
||||
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<void> {
|
||||
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<boolean> {
|
||||
let decoded: unknown;
|
||||
@@ -51,8 +92,15 @@ export async function hasLiveIsolationOwner(baseDir: string): Promise<boolean> {
|
||||
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;
|
||||
}
|
||||
|
||||
@@ -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<boolean> =>
|
||||
@@ -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);
|
||||
});
|
||||
});
|
||||
|
||||
Reference in New Issue
Block a user