fix(cli): preserve live task-isolation sandboxes on worktree clear
`omp worktree clear` (without `--all`) removed every task-isolation dir under the worktree base, including sandboxes owned by subagents running right now, and the "no live task owns it" reason was asserted from the mere presence of the `m` mount dir with no ownership check. `ensureIsolation` now stamps each sandbox base dir with a pid-bearing ownership marker before the backend materialises `m`, and the worktree scanner classifies a sandbox as live while its owning process is alive, so `clear` reclaims only crashed leftovers. Fixes #6761
This commit is contained in:
@@ -2,6 +2,10 @@
|
||||
|
||||
## [Unreleased]
|
||||
|
||||
### 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)).
|
||||
|
||||
## [17.1.5] - 2026-07-27
|
||||
|
||||
### Added
|
||||
|
||||
@@ -8,8 +8,10 @@
|
||||
* `<parent-repo>/.git/worktrees/<name>/`.
|
||||
* - **Task-isolation dirs** (`task/worktree.ts`): a wrapper dir with a
|
||||
* compact `m` subdir mounted/cloned by `natives.isoStart`. Legacy `merged`
|
||||
* subdirs are still recognized. These are ephemeral; `ensureIsolation`
|
||||
* removes the base before re-creating it, so leftovers are crashed runs.
|
||||
* subdirs are still recognized. `ensureIsolation` writes an ownership
|
||||
* marker naming the live omp process; a
|
||||
* sandbox whose owner is still running is reported `live` and never
|
||||
* removed without `--all`, so `clear` reclaims only crashed leftovers.
|
||||
*
|
||||
* Legacy entries from before the encoding change keep working because git still
|
||||
* tracks them by branch name. This command exists to GC them on demand.
|
||||
@@ -18,6 +20,7 @@ import * as fs from "node:fs/promises";
|
||||
import * as path from "node:path";
|
||||
import { getWorktreesDir, isEnoent } from "@oh-my-pi/pi-utils";
|
||||
import chalk from "chalk";
|
||||
import { hasLiveIsolationOwner } from "../task/isolation-ownership";
|
||||
import * as git from "../utils/git";
|
||||
|
||||
type WorktreeKind = "pr-checkout" | "task-isolation" | "empty" | "stray";
|
||||
@@ -214,10 +217,13 @@ async function classifyDir(dir: string): Promise<WorktreeEntry | null> {
|
||||
for (const mountDir of TASK_ISOLATION_MOUNT_DIRS) {
|
||||
const mountStat = await fs.stat(path.join(dir, mountDir)).catch(() => null);
|
||||
if (!mountStat?.isDirectory()) continue;
|
||||
const live = await hasLiveIsolationOwner(dir);
|
||||
return {
|
||||
path: dir,
|
||||
kind: "task-isolation",
|
||||
orphanReason: "task-isolation leftover (no live task owns it)",
|
||||
// Only after confirming no live owner is the "no live task" claim true.
|
||||
// A running subagent's sandbox stays live so `clear` won't delete it.
|
||||
orphanReason: live ? undefined : "task-isolation leftover (no live task owns it)",
|
||||
};
|
||||
}
|
||||
return null;
|
||||
|
||||
@@ -0,0 +1,58 @@
|
||||
/**
|
||||
* Ownership marker for task-isolation sandboxes under `~/.omp/wt/`.
|
||||
*
|
||||
* Each isolation base dir (`ensureIsolation` in {@link ./worktree}) holds a
|
||||
* compact `m` mount plus this marker file naming the omp process that created
|
||||
* it. `omp worktree clear` consults the marker so it can distinguish a live
|
||||
* subagent's sandbox from a crashed run's leftover instead of deleting both.
|
||||
*/
|
||||
import * as path from "node:path";
|
||||
|
||||
/** Marker file written into a task-isolation base dir identifying its owner. */
|
||||
export const ISOLATION_OWNER_FILE = ".omp-isolation-owner.json";
|
||||
|
||||
/** Recorded owner of a task-isolation sandbox. */
|
||||
export interface IsolationOwner {
|
||||
/** PID of the omp process that created and owns the sandbox. */
|
||||
pid: number;
|
||||
/** Task id the sandbox was materialised for. */
|
||||
id: string;
|
||||
}
|
||||
|
||||
/**
|
||||
* Record the current process as owner of the sandbox rooted at `baseDir`.
|
||||
*
|
||||
* Written before the isolation backend materialises `m` so a concurrent
|
||||
* `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 };
|
||||
await Bun.write(path.join(baseDir, ISOLATION_OWNER_FILE), JSON.stringify(owner));
|
||||
}
|
||||
|
||||
/**
|
||||
* Whether a live omp process still owns the sandbox at `baseDir`.
|
||||
*
|
||||
* A missing or malformed marker means no verifiable owner — a crashed run or a
|
||||
* 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.
|
||||
*/
|
||||
export async function hasLiveIsolationOwner(baseDir: string): Promise<boolean> {
|
||||
let decoded: unknown;
|
||||
try {
|
||||
decoded = await Bun.file(path.join(baseDir, ISOLATION_OWNER_FILE)).json();
|
||||
} catch {
|
||||
return false;
|
||||
}
|
||||
if (typeof decoded !== "object" || decoded === null || !("pid" in decoded)) return false;
|
||||
const pid = decoded.pid;
|
||||
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";
|
||||
}
|
||||
}
|
||||
@@ -6,6 +6,7 @@ import * as natives from "@oh-my-pi/pi-natives";
|
||||
import { getWorktreeDir, logger, Snowflake } from "@oh-my-pi/pi-utils";
|
||||
import * as git from "../utils/git";
|
||||
import * as jj from "../utils/jj";
|
||||
import { writeIsolationOwner } from "./isolation-ownership";
|
||||
import { mapWithConcurrencyLimit } from "./parallel";
|
||||
|
||||
const { IsoBackendKind } = natives;
|
||||
@@ -434,6 +435,13 @@ export async function ensureIsolation(
|
||||
|
||||
for (const candidate of candidates) {
|
||||
await fs.rm(baseDir, { recursive: true, force: true });
|
||||
// Claim ownership before the backend materialises `m`. Backends only
|
||||
// create/replace `mergedDir` (and overlay upper/work), never the base
|
||||
// dir, so the marker survives `isoStart` — and a concurrent
|
||||
// `omp worktree clear` never sees this sandbox without a live owner,
|
||||
// even while a large clone is still in progress.
|
||||
await fs.mkdir(baseDir, { recursive: true });
|
||||
await writeIsolationOwner(baseDir, id);
|
||||
try {
|
||||
await natives.isoStart(candidate, repoRoot, mergedDir);
|
||||
// Sever the isolation's git metadata from the source checkout. Copy
|
||||
|
||||
@@ -0,0 +1,72 @@
|
||||
import { afterEach, beforeEach, describe, expect, it, vi } from "bun:test";
|
||||
import * as fs from "node:fs/promises";
|
||||
import * as os from "node:os";
|
||||
import * as path from "node:path";
|
||||
import { clearWorktrees } from "@oh-my-pi/pi-coding-agent/cli/worktree-cli";
|
||||
import { ISOLATION_OWNER_FILE, writeIsolationOwner } from "@oh-my-pi/pi-coding-agent/task/isolation-ownership";
|
||||
import { setWorktreesDir } from "@oh-my-pi/pi-utils";
|
||||
|
||||
/**
|
||||
* Regression for #6761: `omp worktree clear` (no `--all`) must delete only
|
||||
* task-isolation sandboxes whose owner process is gone. A sandbox owned by a
|
||||
* live omp process holds a running subagent's uncaptured work and must survive.
|
||||
*/
|
||||
describe("worktree clear task-isolation ownership", () => {
|
||||
let base: string;
|
||||
let savedEnv: string | undefined;
|
||||
|
||||
beforeEach(async () => {
|
||||
base = await fs.mkdtemp(path.join(os.tmpdir(), "omp-wt-clear-"));
|
||||
savedEnv = process.env.OMP_WORKTREE_DIR;
|
||||
delete process.env.OMP_WORKTREE_DIR;
|
||||
setWorktreesDir(base);
|
||||
vi.spyOn(console, "log").mockImplementation(() => {});
|
||||
});
|
||||
|
||||
afterEach(async () => {
|
||||
setWorktreesDir(undefined);
|
||||
if (savedEnv === undefined) delete process.env.OMP_WORKTREE_DIR;
|
||||
else process.env.OMP_WORKTREE_DIR = savedEnv;
|
||||
vi.restoreAllMocks();
|
||||
await fs.rm(base, { recursive: true, force: true });
|
||||
});
|
||||
|
||||
async function makeSandbox(name: string): Promise<string> {
|
||||
const dir = path.join(base, name);
|
||||
await fs.mkdir(path.join(dir, "m"), { recursive: true });
|
||||
await Bun.write(path.join(dir, "m", "work.txt"), "uncaptured\n");
|
||||
return dir;
|
||||
}
|
||||
|
||||
/** A pid that has been spawned and reaped, so `kill(pid, 0)` reports ESRCH. */
|
||||
async function deadPid(): Promise<number> {
|
||||
const proc = Bun.spawn(["true"], { stdout: "ignore", stderr: "ignore" });
|
||||
await proc.exited;
|
||||
return proc.pid;
|
||||
}
|
||||
|
||||
it("keeps live-owned sandboxes and reclaims dead/markerless/corrupt ones", async () => {
|
||||
const live = await makeSandbox("tlive0001");
|
||||
await writeIsolationOwner(live, "live0001"); // marker names this test process
|
||||
|
||||
const dead = await makeSandbox("tdead0002");
|
||||
await Bun.write(path.join(dead, ISOLATION_OWNER_FILE), JSON.stringify({ pid: await deadPid(), id: "dead0002" }));
|
||||
|
||||
const orphan = await makeSandbox("tnone0003"); // no marker at all (crashed pre-marker run)
|
||||
|
||||
const corrupt = await makeSandbox("tbad00004");
|
||||
await Bun.write(path.join(corrupt, ISOLATION_OWNER_FILE), "{ not json");
|
||||
|
||||
await clearWorktrees({ all: false, dryRun: false, json: true });
|
||||
|
||||
const exists = async (p: string): Promise<boolean> =>
|
||||
await fs.stat(p).then(
|
||||
() => true,
|
||||
() => false,
|
||||
);
|
||||
expect(await Bun.file(path.join(live, "m", "work.txt")).exists()).toBe(true);
|
||||
expect(await exists(dead)).toBe(false);
|
||||
expect(await exists(orphan)).toBe(false);
|
||||
expect(await exists(corrupt)).toBe(false);
|
||||
});
|
||||
});
|
||||
Reference in New Issue
Block a user