From eeca809193c18a48de8ac38789819f2addafdede Mon Sep 17 00:00:00 2001 From: roboomp Date: Mon, 27 Jul 2026 03:42:05 +0000 Subject: [PATCH] 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 --- packages/coding-agent/CHANGELOG.md | 4 ++ packages/coding-agent/src/cli/worktree-cli.ts | 12 +++- .../src/task/isolation-ownership.ts | 58 +++++++++++++++ packages/coding-agent/src/task/worktree.ts | 8 +++ .../test/cli/worktree-clear-isolation.test.ts | 72 +++++++++++++++++++ 5 files changed, 151 insertions(+), 3 deletions(-) create mode 100644 packages/coding-agent/src/task/isolation-ownership.ts create mode 100644 packages/coding-agent/test/cli/worktree-clear-isolation.test.ts diff --git a/packages/coding-agent/CHANGELOG.md b/packages/coding-agent/CHANGELOG.md index cc84edf8d..d71c4c22c 100644 --- a/packages/coding-agent/CHANGELOG.md +++ b/packages/coding-agent/CHANGELOG.md @@ -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 diff --git a/packages/coding-agent/src/cli/worktree-cli.ts b/packages/coding-agent/src/cli/worktree-cli.ts index 92350ce12..c554373c1 100644 --- a/packages/coding-agent/src/cli/worktree-cli.ts +++ b/packages/coding-agent/src/cli/worktree-cli.ts @@ -8,8 +8,10 @@ * `/.git/worktrees//`. * - **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 { 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; diff --git a/packages/coding-agent/src/task/isolation-ownership.ts b/packages/coding-agent/src/task/isolation-ownership.ts new file mode 100644 index 000000000..c77485b86 --- /dev/null +++ b/packages/coding-agent/src/task/isolation-ownership.ts @@ -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 { + 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 { + 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"; + } +} diff --git a/packages/coding-agent/src/task/worktree.ts b/packages/coding-agent/src/task/worktree.ts index c7e60d8fd..ecb26de15 100644 --- a/packages/coding-agent/src/task/worktree.ts +++ b/packages/coding-agent/src/task/worktree.ts @@ -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 diff --git a/packages/coding-agent/test/cli/worktree-clear-isolation.test.ts b/packages/coding-agent/test/cli/worktree-clear-isolation.test.ts new file mode 100644 index 000000000..6e0e03efe --- /dev/null +++ b/packages/coding-agent/test/cli/worktree-clear-isolation.test.ts @@ -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 { + 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 { + 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 => + 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); + }); +});