diff --git a/lib/daemon/__tests__/worktree-reconciler.test.ts b/lib/daemon/__tests__/worktree-reconciler.test.ts index 82df31db..ba46c90f 100644 --- a/lib/daemon/__tests__/worktree-reconciler.test.ts +++ b/lib/daemon/__tests__/worktree-reconciler.test.ts @@ -1,6 +1,6 @@ import { describe, test, expect, beforeEach } from "bun:test"; import { execSync } from "child_process"; -import { existsSync, mkdtempSync, realpathSync, rmSync, writeFileSync } from "fs"; +import { chmodSync, existsSync, mkdirSync, mkdtempSync, realpathSync, rmSync, writeFileSync } from "fs"; import { tmpdir } from "os"; import { basename, join } from "path"; import type { Logger } from "pino"; @@ -326,6 +326,36 @@ describe("createWorktreeReconciler", () => { expect(loadRegistry(repoName).length).toBe(1); // just main, adopted once }); + test("runOnce reaps .trash-* leftovers in the default and configured roots", async () => { + // What a daemon crash mid-reap leaves behind: the dispose renamed the tree + // but its detached `rm -rf` never finished (or never started). + const configuredRoot = realpathSync(mkdtempSync(join(tmpdir(), "rtrecon-root-"))); + writeJson(join(repoDataDir(repoName), "config.json"), { + worktrees: { root: configuredRoot }, + }); + + const defaultRootTrash = join(repo, ".worktrees", ".trash-hotel-1700000000000"); + const configuredTrash = join(configuredRoot, ".trash-india-1700000000001"); + const live = join(configuredRoot, "juliet"); + for (const dir of [defaultRootTrash, configuredTrash, live]) { + mkdirSync(dir, { recursive: true }); + writeFileSync(join(dir, "file.txt"), "x\n"); + } + + const reconciler = createWorktreeReconciler({ + cache: { entries: {} }, + repoIndex: () => ({ [repoName]: repo }), + emit: () => {}, + log: fakeLog(), + }); + + await reconciler.runOnce(); + + expect(existsSync(defaultRootTrash)).toBe(false); + expect(existsSync(configuredTrash)).toBe(false); + expect(existsSync(live)).toBe(true); + }); + test("runOnce with the app disabled still syncs the registry, but skips reactor/freshen/replenish", async () => { // A dedicated repoName (not the shared "acme" the other tests in this // describe use): the prior test's `kick()` is deliberately unawaited by @@ -350,6 +380,9 @@ describe("createWorktreeReconciler", () => { sh(`git add -A && git ${GIT_ID} commit -m feat`, clone); sh(`git push -q origin main`, clone); + const seededTrash = join(repo, ".worktrees", ".trash-kilo-1700000000002"); + mkdirSync(seededTrash, { recursive: true }); + const beforeSha = await headSha(repo); const events: Array<{ type: string; data: any }> = []; const reconciler = createWorktreeReconciler({ @@ -372,6 +405,9 @@ describe("createWorktreeReconciler", () => { // Reactor skipped: it never even opened/wrote its state file. expect(existsSync(__test__.reactorStatePath())).toBe(false); expect(events.length).toBe(0); + // Reap skipped: it deletes directories, so it is gated like every other + // mutating duty. + expect(existsSync(seededTrash)).toBe(true); }); }); @@ -587,21 +623,26 @@ describe("merge reactor (detectTransitions)", () => { test("a failed worktree removal is retried, never advertised as disposable", async () => { const rec = ephemeralTree("hotel", "feat-hotel"); - // A locked worktree makes `git worktree remove --force` refuse (it wants - // --force twice) without touching the directory: a mechanical, transient - // removal failure, which must NOT be reported to the user as "disposable". - sh(`git -C ${repo} worktree lock ${rec.path}`); - - await detect(mrCache("feat-hotel", "opened")); - await detect(mrCache("feat-hotel", "merged")); - - expect(existsSync(rec.path)).toBe(true); - expect(tracked(rec.path)!.state).toBe("claimed"); - expect(tracked(rec.path)!.disposableReason).toBeUndefined(); - expect(events.some((e) => e.type === "worktree:disposable")).toBe(false); - expect(reactorState().mrState[`${repoName}:feat-hotel`]).toBe("opened"); + // Disposal renames the tree into a sibling trash dir, so the mechanical, + // transient failure to reproduce is a root nothing can be renamed within. + // The tree survives untouched, and that must NOT be reported to the user + // as "disposable". + const root = join(repo, ".worktrees"); + chmodSync(root, 0o555); + + try { + await detect(mrCache("feat-hotel", "opened")); + await detect(mrCache("feat-hotel", "merged")); + + expect(existsSync(rec.path)).toBe(true); + expect(tracked(rec.path)!.state).toBe("claimed"); + expect(tracked(rec.path)!.disposableReason).toBeUndefined(); + expect(events.some((e) => e.type === "worktree:disposable")).toBe(false); + expect(reactorState().mrState[`${repoName}:feat-hotel`]).toBe("opened"); + } finally { + chmodSync(root, 0o755); + } - sh(`git -C ${repo} worktree unlock ${rec.path}`); await detect(mrCache("feat-hotel", "merged")); expect(existsSync(rec.path)).toBe(false); diff --git a/lib/daemon/worktree-reconciler.ts b/lib/daemon/worktree-reconciler.ts index b70fa38e..d85fe741 100644 --- a/lib/daemon/worktree-reconciler.ts +++ b/lib/daemon/worktree-reconciler.ts @@ -45,6 +45,7 @@ import { type WorktreeAppConfig, } from "../worktree/config.ts"; import { killWorktreeProcesses } from "./worktree-process-kill.ts"; +import { reapTrashInRoots } from "../worktree/trash.ts"; export interface ReconcilerDeps { cache: { entries: Record }; @@ -162,7 +163,9 @@ async function reconcilePass(deps: ReconcileDeps, attempt: number): Promise { + const { repoName, repoPath, log } = deps; + const cfg = loadWorktreeRepoConfig(repoName, repoPath); + const reaped = await reapTrashInRoots([join(repoPath, ".worktrees"), cfg.root], log); + if (reaped > 0) log.info({ repo: repoName, count: reaped }, "worktree trash reaped"); +} + /** Whether a repo has any worktree state worth reconciling: registry entries or a declared "worktrees" config. */ function repoHasWorktreeActivity(repoName: string): boolean { if (loadRegistry(repoName).length > 0) return true; @@ -1030,6 +1053,11 @@ export function createWorktreeReconciler(deps: ReconcilerDeps): { } catch (err) { deps.log.warn({ err, repo: repoName }, "worktree reconciler: replenish/shrink pass failed"); } + try { + await reapRepoTrash({ repoName, repoPath, log: deps.log }); + } catch (err) { + deps.log.warn({ err, repo: repoName }, "worktree reconciler: trash reap pass failed"); + } } } @@ -1058,6 +1086,7 @@ export function createWorktreeReconciler(deps: ReconcilerDeps): { export const __test__ = { detectTransitions, + reapRepoTrash, reactorStatePath, freshenRepo, replenishAndShrink, diff --git a/lib/worktree/__tests__/config.test.ts b/lib/worktree/__tests__/config.test.ts index ee0fd019..3adcbeed 100644 --- a/lib/worktree/__tests__/config.test.ts +++ b/lib/worktree/__tests__/config.test.ts @@ -8,6 +8,7 @@ import { loadWorktreeRepoConfig, resolveImplicitInstall, resolveReadySteps, + stripEnvPrefix, loadWorktreeAppConfig, type WorktreeRepoConfig, } from "../config.ts"; @@ -78,6 +79,18 @@ describe("worktree config", () => { expect(cfg.root).toBe(join(process.env.HOME!, "wt-root")); }); + test("drops dot-leading namePool entries", () => { + // A pool entry named ".trash-x" would build a tree the reconciler's reap + // duty then deletes as a leftover. The reaper is the only rm -rf in the + // codebase; this is the door it can come through. + const repoPath = tmpRepoPath("rtcfg-repo-"); + writeJson(join(repoDataDir("myrepo"), "config.json"), { + worktrees: { namePool: [".trash-x", "luna"] }, + }); + const cfg = loadWorktreeRepoConfig("myrepo", repoPath); + expect(cfg.namePool).toEqual(["luna"]); + }); + test("leaves an absolute root unchanged", () => { const repoPath = tmpRepoPath("rtcfg-repo-"); writeJson(join(repoDataDir("myrepo"), "config.json"), { @@ -101,7 +114,7 @@ describe("worktree config", () => { JSON.stringify({ name: "x", packageManager: "pnpm@9.1.0" }) ); expect(resolveImplicitInstall(repoPath)).toEqual({ - run: "pnpm install --side-effects-cache", + run: "pnpm install", when: "changed:pnpm-lock.yaml", }); }); @@ -131,7 +144,7 @@ describe("worktree config", () => { writeFileSync(join(repoPath, "package.json"), JSON.stringify({ name: "x" })); writeFileSync(join(repoPath, "pnpm-lock.yaml"), ""); expect(resolveImplicitInstall(repoPath)).toEqual({ - run: "pnpm install --side-effects-cache", + run: "pnpm install", when: "changed:pnpm-lock.yaml", }); }); @@ -168,7 +181,7 @@ describe("worktree config", () => { ready: [{ run: "node scripts/gen-types.js", when: "changed:db/schema/**" }], }; expect(resolveReadySteps(cfg, repoPath)).toEqual([ - { run: "pnpm install --side-effects-cache", when: "changed:pnpm-lock.yaml" }, + { run: "pnpm install", when: "changed:pnpm-lock.yaml" }, { run: "node scripts/gen-types.js", when: "changed:db/schema/**" }, ]); }); @@ -200,7 +213,7 @@ describe("worktree config", () => { ready: [{ run: "pnpm lint" }], }; expect(resolveReadySteps(cfg, repoPath)).toEqual([ - { run: "pnpm install --side-effects-cache", when: "changed:pnpm-lock.yaml" }, + { run: "pnpm install", when: "changed:pnpm-lock.yaml" }, { run: "pnpm lint" }, ]); }); @@ -215,6 +228,115 @@ describe("worktree config", () => { }; expect(resolveReadySteps(cfg, repoPath)).toEqual(cfg.ready); }); + + test("an env-var prefix on the declared install still suppresses the implicit one", () => { + const repoPath = tmpRepoPath("rtcfg-resolve5-"); + writeFileSync(join(repoPath, "package.json"), JSON.stringify({ name: "x" })); + writeFileSync(join(repoPath, "pnpm-lock.yaml"), ""); + const cfg: WorktreeRepoConfig = { + onDeck: 0, + root: join(repoPath, ".worktrees"), + branchFormat: "-", + ready: [{ run: "SKIP_GEN_TYPES=1 pnpm install --side-effects-cache" }], + }; + expect(resolveReadySteps(cfg, repoPath)).toEqual(cfg.ready); + }); + + test("an `env VAR=value` prefix on the declared install still suppresses the implicit one", () => { + const repoPath = tmpRepoPath("rtcfg-resolve6-"); + writeFileSync(join(repoPath, "package.json"), JSON.stringify({ name: "x" })); + writeFileSync(join(repoPath, "pnpm-lock.yaml"), ""); + const cfg: WorktreeRepoConfig = { + onDeck: 0, + root: join(repoPath, ".worktrees"), + branchFormat: "-", + ready: [{ run: "env FOO=bar pnpm install" }], + }; + expect(resolveReadySteps(cfg, repoPath)).toEqual(cfg.ready); + }); + + test("an `env -i` prefix on the declared install still suppresses the implicit one", () => { + const repoPath = tmpRepoPath("rtcfg-resolve8-"); + writeFileSync(join(repoPath, "package.json"), JSON.stringify({ name: "x" })); + writeFileSync(join(repoPath, "pnpm-lock.yaml"), ""); + const cfg: WorktreeRepoConfig = { + onDeck: 0, + root: join(repoPath, ".worktrees"), + branchFormat: "-", + ready: [{ run: "env -i PATH=/usr/bin pnpm install" }], + }; + expect(resolveReadySteps(cfg, repoPath)).toEqual(cfg.ready); + }); + + test("a command that merely starts with the letters `install` does not suppress it", () => { + const repoPath = tmpRepoPath("rtcfg-resolve9-"); + writeFileSync(join(repoPath, "package.json"), JSON.stringify({ name: "x" })); + writeFileSync(join(repoPath, "pnpm-lock.yaml"), ""); + const cfg: WorktreeRepoConfig = { + onDeck: 0, + root: join(repoPath, ".worktrees"), + branchFormat: "-", + ready: [{ run: "pnpm installer" }], + }; + expect(resolveReadySteps(cfg, repoPath)).toEqual([ + { run: "pnpm install", when: "changed:pnpm-lock.yaml" }, + { run: "pnpm installer" }, + ]); + }); + + test("an env-var prefix on a NON-install step does not suppress the implicit install", () => { + const repoPath = tmpRepoPath("rtcfg-resolve7-"); + writeFileSync(join(repoPath, "package.json"), JSON.stringify({ name: "x" })); + writeFileSync(join(repoPath, "pnpm-lock.yaml"), ""); + const cfg: WorktreeRepoConfig = { + onDeck: 0, + root: join(repoPath, ".worktrees"), + branchFormat: "-", + ready: [{ run: "SKIP_X=1 pnpm lint" }], + }; + expect(resolveReadySteps(cfg, repoPath)).toEqual([ + { run: "pnpm install", when: "changed:pnpm-lock.yaml" }, + { run: "SKIP_X=1 pnpm lint" }, + ]); + }); + }); + + describe("stripEnvPrefix", () => { + test("leaves a bare command alone", () => { + expect(stripEnvPrefix("pnpm install")).toBe("pnpm install"); + }); + + test("strips one or many leading assignments", () => { + expect(stripEnvPrefix("A=1 pnpm install")).toBe("pnpm install"); + expect(stripEnvPrefix("A=1 B=2 pnpm install --frozen-lockfile")).toBe( + "pnpm install --frozen-lockfile", + ); + }); + + test("strips a leading `env`, with or without assignments after it", () => { + expect(stripEnvPrefix("env pnpm install")).toBe("pnpm install"); + expect(stripEnvPrefix("env FOO=bar pnpm install")).toBe("pnpm install"); + }); + + test("strips env's own options, including ones that take an argument", () => { + expect(stripEnvPrefix("env -i PATH=/usr/bin pnpm install")).toBe("pnpm install"); + expect(stripEnvPrefix("env --ignore-environment pnpm install")).toBe("pnpm install"); + expect(stripEnvPrefix("env -u FOO pnpm install")).toBe("pnpm install"); + }); + + test("never eats a command's own flags — options only strip after `env`", () => { + expect(stripEnvPrefix("pnpm -r install")).toBe("pnpm -r install"); + expect(stripEnvPrefix("A=1 pnpm --filter x install")).toBe("pnpm --filter x install"); + }); + + test("tolerates quoted assignment values containing spaces", () => { + expect(stripEnvPrefix('FOO="a b" pnpm install')).toBe("pnpm install"); + }); + + test("never eats the command itself", () => { + expect(stripEnvPrefix("A=1")).toBe("A=1"); + expect(stripEnvPrefix("env")).toBe("env"); + }); }); describe("loadWorktreeAppConfig", () => { diff --git a/lib/worktree/__tests__/create.test.ts b/lib/worktree/__tests__/create.test.ts index 0f880de2..70b7440d 100644 --- a/lib/worktree/__tests__/create.test.ts +++ b/lib/worktree/__tests__/create.test.ts @@ -1,13 +1,13 @@ import { describe, test, expect, beforeEach } from "bun:test"; import { execSync } from "child_process"; -import { existsSync, mkdtempSync, readFileSync, realpathSync } from "fs"; +import { existsSync, mkdirSync, mkdtempSync, readFileSync, realpathSync } from "fs"; import { tmpdir } from "os"; import { join } from "path"; import { writeJson } from "../../json-store.ts"; import { repoDataDir } from "../../rt-paths.ts"; -import { loadRegistry } from "../registry.ts"; +import { loadRegistry, saveRegistry, type TreeRecord } from "../registry.ts"; import { branchExistsLocalAsync, listWorktreesAsync } from "../git-async.ts"; -import { createTree, type CreateDeps } from "../create.ts"; +import { createTree, scrapTree, type CreateDeps } from "../create.ts"; function makeRepo(): string { // realpathSync: git canonicalizes /var -> /private/var on macOS (Global Constraints) @@ -115,3 +115,88 @@ describe("createTree", () => { expect(registry.length).toBe(0); }); }); + +describe("scrapTree", () => { + let repo: string; + let repoName: string; + let events: Array<{ type: string; data: unknown }>; + + beforeEach(() => { + process.env.HOME = realpathSync(mkdtempSync(join(tmpdir(), "rtcreate-home-"))); + repo = makeRepo(); + addBareOrigin(repo); + repoName = "acme"; + events = []; + }); + + test("tolerates a record whose worktree and branch never came to exist", async () => { + const rec: TreeRecord = { + name: "ghost", + path: join(repo, ".worktrees", "ghost"), + kind: "ephemeral", + state: "creating", + branch: "on-deck/ghost", + createdAt: new Date().toISOString(), + }; + saveRegistry(repoName, [rec]); + + await scrapTree(makeDeps(repoName, repo, events), rec); + + expect(loadRegistry(repoName).length).toBe(0); + }); + + test("a rename that fails with the tree still on disk keeps branch and record for retry", async () => { + const path = join(repo, ".worktrees", "stuck"); + execSync(`git -C ${repo} worktree add -b on-deck/stuck ${path}`, { shell: "/bin/zsh", stdio: "pipe" }); + const rec: TreeRecord = { + name: "stuck", + path, + kind: "ephemeral", + state: "creating", + branch: "on-deck/stuck", + createdAt: new Date().toISOString(), + }; + saveRegistry(repoName, [rec]); + + // A non-empty directory already at the trash destination makes the rename + // fail (ENOTEMPTY) while the tree remains — deterministic even as root, + // unlike a read-only parent. Freeze Date.now so the destination is known. + const stamp = 1234567890; + const realNow = Date.now; + Date.now = () => stamp; + mkdirSync(join(repo, ".worktrees", `.trash-stuck-${stamp}`, "occupied"), { recursive: true }); + try { + await scrapTree(makeDeps(repoName, repo, events), rec); + } finally { + Date.now = realNow; + } + + expect(existsSync(path)).toBe(true); + expect(loadRegistry(repoName)).toEqual([rec]); + const branches = execSync(`git -C ${repo} branch --list on-deck/stuck`, { shell: "/bin/zsh" }).toString(); + expect(branches).toContain("on-deck/stuck"); + }); + + test("scraps a tree git itself would refuse to remove", async () => { + // A locked worktree makes `git worktree remove --force` refuse (it wants + // --force twice). A rename does not care: a mid-create scrap must always + // get the half-built tree out of the way, whatever state git is in. + const path = join(repo, ".worktrees", "locked"); + execSync(`git -C ${repo} worktree add -b on-deck/locked ${path}`, { shell: "/bin/zsh", stdio: "pipe" }); + execSync(`git -C ${repo} worktree lock ${path}`, { shell: "/bin/zsh", stdio: "pipe" }); + const rec: TreeRecord = { + name: "locked", + path, + kind: "ephemeral", + state: "creating", + branch: "on-deck/locked", + createdAt: new Date().toISOString(), + }; + saveRegistry(repoName, [rec]); + + await scrapTree(makeDeps(repoName, repo, events), rec); + + expect(existsSync(path)).toBe(false); + expect(loadRegistry(repoName).length).toBe(0); + }); +}); diff --git a/lib/worktree/__tests__/dispose.test.ts b/lib/worktree/__tests__/dispose.test.ts index 761030af..ac8f30f2 100644 --- a/lib/worktree/__tests__/dispose.test.ts +++ b/lib/worktree/__tests__/dispose.test.ts @@ -1,8 +1,8 @@ import { describe, test, expect, beforeEach } from "bun:test"; import { execSync } from "child_process"; -import { existsSync, mkdirSync, mkdtempSync, writeFileSync, realpathSync } from "fs"; +import { chmodSync, existsSync, mkdirSync, mkdtempSync, readdirSync, writeFileSync, realpathSync } from "fs"; import { tmpdir } from "os"; -import { join } from "path"; +import { basename, dirname, join } from "path"; import { repoDataDir } from "../../rt-paths.ts"; import { saveSyncConfig } from "../../sync-config.ts"; import { loadRegistry, saveRegistry, type TreeRecord } from "../registry.ts"; @@ -50,6 +50,15 @@ function addTree(repo: string, name: string, branch: string, base = "origin/main return path; } +/** Poll until `cond` holds — for the detached reaper, which nobody awaits. */ +async function waitFor(cond: () => boolean, timeoutMs = 5000): Promise { + const deadline = Date.now() + timeoutMs; + while (!cond()) { + if (Date.now() > deadline) throw new Error("timed out waiting for condition"); + await new Promise((r) => setTimeout(r, 20)); + } +} + function commitIn(worktree: string, file: string, content: string): void { writeFileSync(join(worktree, file), content); execSync(`git add ${file} && git ${GIT_ID} commit -m change`, { @@ -566,17 +575,57 @@ describe("disposeTree", () => { expect(loadRegistry(repoName).length).toBe(1); }); - test("a worktree git refuses to remove refuses \"remove-failed\" and keeps the registry row", async () => { + test("a tree that cannot be renamed refuses \"remove-failed\" and keeps the registry row", async () => { + const path = addTree(repo, "tree-a", "feature-a"); + const rec = register(repoName, ephemeral("tree-a", path, "feature-a")); + // A rename needs write permission on the PARENT: a read-only .worktrees is + // the mechanical, transient failure that stands in for a busy/held tree. + const root = join(repo, ".worktrees"); + chmodSync(root, 0o555); + + try { + const result = await disposeTree(makeDeps(), rec, { force: true }); + expect(result).toEqual({ disposed: false, refusal: "remove-failed" }); + // Nothing downstream of the rename ran: the tree, its branch, and its + // registry row are all still there for the retry. + expect(existsSync(path)).toBe(true); + expect(loadRegistry(repoName).length).toBe(1); + expect(await branchExistsLocalAsync(repo, "feature-a")).toBe(true); + expect((await listWorktreesAsync(repo))!.some((w) => w.path === path)).toBe(true); + expect(events.length).toBe(0); + } finally { + chmodSync(root, 0o755); + } + }); + + test("disposal renames the tree to a .trash-* sibling and reaps it in the background", async () => { const path = addTree(repo, "tree-a", "feature-a"); const rec = register(repoName, ephemeral("tree-a", path, "feature-a")); - // A locked worktree needs --force twice; one --force fails and leaves the tree. - execSync(`git -C ${repo} worktree lock ${path}`, { shell: "/bin/zsh", stdio: "pipe" }); + const root = join(repo, ".worktrees"); - const result = await disposeTree(makeDeps(), rec, { force: true }); - expect(result).toEqual({ disposed: false, refusal: "remove-failed" }); - expect(existsSync(path)).toBe(true); - expect(loadRegistry(repoName).length).toBe(1); - expect(await branchExistsLocalAsync(repo, "feature-a")).toBe(true); + const infos: Array> = []; + const deps = makeDeps({ + log: { info: (fields: unknown) => infos.push(fields as Record), warn: () => {} }, + }); + const result = await disposeTree(deps, rec, { force: true }); + expect(result).toEqual({ disposed: true }); + + // The tree is gone from its own path the moment dispose returns, moved to + // a trash sibling the log names (the reaper may already have eaten it). + expect(existsSync(path)).toBe(false); + const trash = infos.find((f) => typeof f.trash === "string")!.trash as string; + expect(dirname(trash)).toBe(root); + expect(basename(trash).startsWith(".trash-tree-a-")).toBe(true); + expect(readdirSync(root).every((e) => e.startsWith(".trash-tree-a-"))).toBe(true); + + // The registration, branch, and registry row all went with it. + expect((await listWorktreesAsync(repo))!.some((w) => w.path === path)).toBe(false); + expect(await branchExistsLocalAsync(repo, "feature-a")).toBe(false); + expect(loadRegistry(repoName).length).toBe(0); + expect(events.some((e) => e.type === "worktree:disposed")).toBe(true); + + // …and the detached reaper finishes the delete without anyone awaiting it. + await waitFor(() => readdirSync(root).length === 0); }); test("guard order: dirty is reported before unpushed", async () => { diff --git a/lib/worktree/__tests__/trash.test.ts b/lib/worktree/__tests__/trash.test.ts new file mode 100644 index 00000000..0bfa6d91 --- /dev/null +++ b/lib/worktree/__tests__/trash.test.ts @@ -0,0 +1,151 @@ +import { describe, test, expect, beforeEach } from "bun:test"; +import { existsSync, mkdirSync, mkdtempSync, readFileSync, realpathSync, writeFileSync } from "fs"; +import { tmpdir } from "os"; +import { basename, dirname, join } from "path"; +import { + TRASH_PREFIX, + reapTrashDir, + reapTrashInRoots, + trashPathFor, + trashTree, +} from "../trash.ts"; + +function capturingLog(): { log: { warn: (...args: unknown[]) => void }; warns: unknown[][] } { + const warns: unknown[][] = []; + return { log: { warn: (...args: unknown[]) => warns.push(args) }, warns }; +} + +/** A directory standing in for a worktree, with one file inside it. */ +function makeTree(root: string, name: string): string { + const path = join(root, name); + mkdirSync(path, { recursive: true }); + writeFileSync(join(path, "file.txt"), "content\n"); + return path; +} + +describe("worktree trash", () => { + let root: string; + + beforeEach(() => { + // realpathSync: /var -> /private/var on macOS (Global Constraints) + root = realpathSync(mkdtempSync(join(tmpdir(), "rttrash-"))); + }); + + describe("trashPathFor", () => { + test("is a sibling of the tree, prefixed and stamped", () => { + const path = trashPathFor(join(root, "hotel"), "hotel", 1_700_000_000_000); + expect(dirname(path)).toBe(root); + expect(basename(path)).toBe(`${TRASH_PREFIX}hotel-1700000000000`); + }); + + test("rejects a name containing a path separator", () => { + expect(() => trashPathFor(join(root, "x"), "x/../../outside")).toThrow( + /single path component/, + ); + expect(() => trashPathFor(join(root, "x"), "x\\y")).toThrow(/single path component/); + expect(() => trashPathFor(join(root, "x"), "")).toThrow(/single path component/); + }); + + test("two disposals of the same name never collide", () => { + const a = trashPathFor(join(root, "hotel"), "hotel", 1); + const b = trashPathFor(join(root, "hotel"), "hotel", 2); + expect(a).not.toBe(b); + }); + }); + + describe("trashTree", () => { + test("moves the tree out of the way, contents intact", async () => { + const tree = makeTree(root, "hotel"); + + const result = await trashTree(tree, "hotel"); + expect(result.ok).toBe(true); + if (!result.ok) throw new Error("expected ok"); + + expect(existsSync(tree)).toBe(false); + expect(existsSync(result.trashPath)).toBe(true); + expect(readFileSync(join(result.trashPath, "file.txt"), "utf8")).toBe("content\n"); + expect(basename(result.trashPath).startsWith(TRASH_PREFIX)).toBe(true); + }); + + test("a separator-containing name reports the failure without renaming", async () => { + const tree = makeTree(root, "sierra"); + const result = await trashTree(tree, "sierra/../escape"); + expect(result.ok).toBe(false); + expect(existsSync(tree)).toBe(true); + }); + + test("a path that cannot be renamed reports the failure instead of throwing", async () => { + const result = await trashTree(join(root, "never-existed"), "never-existed"); + expect(result.ok).toBe(false); + if (result.ok) throw new Error("expected failure"); + expect((result.err as NodeJS.ErrnoException).code).toBe("ENOENT"); + }); + }); + + describe("reapTrashDir", () => { + test("deletes the trash directory and everything under it", async () => { + const tree = makeTree(root, "hotel"); + mkdirSync(join(tree, "node_modules", "dep"), { recursive: true }); + writeFileSync(join(tree, "node_modules", "dep", "index.js"), "//\n"); + const result = await trashTree(tree, "hotel"); + if (!result.ok) throw new Error("expected ok"); + + const { log, warns } = capturingLog(); + await reapTrashDir(result.trashPath, log); + + expect(existsSync(result.trashPath)).toBe(false); + expect(warns.length).toBe(0); + }); + + test("refuses a path that is not a trash directory", async () => { + const tree = makeTree(root, "hotel"); + const { log, warns } = capturingLog(); + + await reapTrashDir(tree, log); + + expect(existsSync(tree)).toBe(true); + expect(warns.length).toBe(1); + }); + }); + + describe("reapTrashInRoots", () => { + test("reaps every trash dir across roots and leaves real trees alone", async () => { + const other = realpathSync(mkdtempSync(join(tmpdir(), "rttrash-other-"))); + const live = makeTree(root, "hotel"); + const stale1 = makeTree(root, `${TRASH_PREFIX}india-1`); + const stale2 = makeTree(root, `${TRASH_PREFIX}juliet-2`); + const stale3 = makeTree(other, `${TRASH_PREFIX}kilo-3`); + + const { log, warns } = capturingLog(); + const reaped = await reapTrashInRoots([root, other], log); + + expect(reaped).toBe(3); + expect(existsSync(stale1)).toBe(false); + expect(existsSync(stale2)).toBe(false); + expect(existsSync(stale3)).toBe(false); + expect(existsSync(live)).toBe(true); + expect(warns.length).toBe(0); + }); + + test("a root that does not exist is not an error", async () => { + const { log, warns } = capturingLog(); + expect(await reapTrashInRoots([join(root, "nope")], log)).toBe(0); + expect(warns.length).toBe(0); + }); + + test("a root that cannot be read (not just missing) is warned about", async () => { + const file = join(root, "not-a-dir"); + writeFileSync(file, ""); + const { log, warns } = capturingLog(); + expect(await reapTrashInRoots([file], log)).toBe(0); + expect(warns.length).toBe(1); + expect(warns[0]?.[0]).toMatchObject({ root: file }); + }); + + test("the same root listed twice is swept once", async () => { + makeTree(root, `${TRASH_PREFIX}lima-1`); + const { log } = capturingLog(); + expect(await reapTrashInRoots([root, root], log)).toBe(1); + }); + }); +}); diff --git a/lib/worktree/config.ts b/lib/worktree/config.ts index 3bb9d7a0..b5ef5901 100644 --- a/lib/worktree/config.ts +++ b/lib/worktree/config.ts @@ -71,7 +71,9 @@ export function loadWorktreeRepoConfig(repoName: string, repoPath: string): Work branchFormat: declared.branchFormat ?? "-", ready: declared.ready ?? [], }; - if (declared.namePool) cfg.namePool = declared.namePool; + // A dot-leading name would build a tree the reconciler's reap duty then + // deletes as a `.trash-*` leftover, so the pool never gets to declare one. + if (declared.namePool) cfg.namePool = declared.namePool.filter((n) => !n.startsWith(".")); return cfg; } @@ -79,8 +81,18 @@ export function loadWorktreeRepoConfig(repoName: string, repoPath: string): Work type Manager = "pnpm" | "bun" | "yarn" | "npm"; +/** + * pnpm's default is deliberately a PLAIN install, not `--side-effects-cache`: + * that cache replays a dependency's recorded postinstall effects instead of + * re-running it, and it only ever captured files written inside node_modules. + * A dep whose postinstall writes outside the package dir (prisma generating + * into apps/backend/generated/, say) is therefore silently skipped on a fresh + * tree — the install "succeeds" and the tree is missing generated code. The + * flag stays available as a declared ready step for repos verified free of + * out-of-tree generators; it is not safe as a blind default. + */ const MANAGER_STEP: Record = { - pnpm: { run: "pnpm install --side-effects-cache", when: "changed:pnpm-lock.yaml" }, + pnpm: { run: "pnpm install", when: "changed:pnpm-lock.yaml" }, bun: { run: "bun install", when: "changed:bun.lock*" }, yarn: { run: "yarn install", when: "changed:yarn.lock" }, npm: { run: "npm install", when: "changed:package-lock.json" }, @@ -120,19 +132,67 @@ export function resolveImplicitInstall(repoPath: string): ReadyStep | null { return manager ? MANAGER_STEP[manager] : null; } +/** One leading `VAR=value` assignment (value optionally quoted). */ +const ASSIGNMENT_TOKEN = /^[A-Za-z_][A-Za-z0-9_]*=(?:"[^"]*"|'[^']*'|\S*)\s+/; +const ENV_WORD = /^env\s+/; +const OPTION_TOKEN = /^(-\S*)\s+/; +/** env(1) short options whose argument is the NEXT token (`-u NAME` etc.). */ +const ENV_ARG_OPTIONS = new Set(["-u", "-C", "-P", "-S"]); + +/** + * Drop a shell env prefix from a declared run, leaving the command word first: + * `SKIP_GEN_TYPES=1 pnpm install` → `pnpm install`, `env -i FOO=bar pnpm + * install` → `pnpm install`. Purely for recognising WHICH command a step runs + * (the install-dedup test below); the step itself is always executed verbatim, + * env prefix included. Option words are only consumed after an `env`, so a + * command's own flags are never eaten. Every token match requires trailing + * whitespace, so a run that is nothing but a prefix (`A=1`, `env`) is left + * alone rather than emptied. + */ +export function stripEnvPrefix(run: string): string { + let rest = run.trimStart(); + let inEnv = false; + for (;;) { + if (ASSIGNMENT_TOKEN.test(rest)) { + rest = rest.replace(ASSIGNMENT_TOKEN, ""); + continue; + } + if (ENV_WORD.test(rest)) { + rest = rest.replace(ENV_WORD, ""); + inEnv = true; + continue; + } + const option = inEnv ? rest.match(OPTION_TOKEN) : null; + if (option) { + rest = rest.slice(option[0].length); + if (ENV_ARG_OPTIONS.has(option[1] ?? "")) rest = rest.replace(/^\S+\s+/, ""); + continue; + } + return rest; + } +} + /** * Implicit install first UNLESS cfg.ready already declares its own install * step for the detected manager (run starts with " install", e.g. * "pnpm install --side-effects-cache") — only an install step replaces the * implicit one; any other declared command for that manager (e.g. "pnpm * lint") does not suppress it. Otherwise cfg.ready in order. + * + * The prefix test runs against the env-stripped run: declaring + * `SKIP_GEN_TYPES=1 pnpm install` is still declaring the install, and letting + * an env prefix hide it from the dedup made the tree install twice. */ export function resolveReadySteps(cfg: WorktreeRepoConfig, repoPath: string): ReadyStep[] { const manager = detectManager(repoPath); if (!manager) return cfg.ready; - const installPrefix = `${manager} install`; - const alreadyDeclared = cfg.ready.some((step) => step.run.startsWith(installPrefix)); + const installWords = `${manager} install`; + const alreadyDeclared = cfg.ready.some((step) => { + const command = stripEnvPrefix(step.run); + // Whole-token match: `pnpm install --flag` counts, `pnpm installer` does not. + return command === installWords || command.startsWith(`${installWords} `); + }); if (alreadyDeclared) return cfg.ready; return [MANAGER_STEP[manager], ...cfg.ready]; diff --git a/lib/worktree/create.ts b/lib/worktree/create.ts index 79e39e1a..99e7c773 100644 --- a/lib/worktree/create.ts +++ b/lib/worktree/create.ts @@ -9,6 +9,7 @@ * (worktree + on-deck branch + registry row) and reports typed detail. */ +import { existsSync } from "fs"; import { join } from "path"; import { loadRegistry, @@ -27,14 +28,11 @@ import { pickName } from "./names.ts"; import { loadWorktreeRepoConfig, resolveReadySteps, type WorktreeRepoConfig } from "./config.ts"; import { runReadySteps } from "./ready.ts"; import { withTreeLock } from "./locks.ts"; +import { reapTrashDir, trashTree } from "./trash.ts"; import { reconcileForRepo } from "../daemon/doppler-sync.ts"; const CREATE_TIMEOUT_MS = 5 * 60_000; -/** `git worktree remove` deletes node_modules by hand; a pnpm-scale tree on - * APFS routinely outruns runGit's 60s default. */ -const REMOVE_TIMEOUT_MS = 5 * 60_000; - /** Longest slice of a failed step's output carried into the log line. */ const MAX_LOGGED_OUTPUT = 2000; @@ -160,15 +158,33 @@ async function runCreate( } /** - * Force-remove the worktree, delete its on-deck/ branch, and prune the - * registry entry. Tolerant of partial existence: the worktree may not exist - * yet (git worktree add never ran or failed before creating it), and the - * branch may not exist either. + * Get rid of a half-built tree: rename it into trash (see trash.ts) with a + * detached reap behind it, prune the registration, delete its on-deck/ + * branch, and drop the registry entry. Tolerant of partial existence — the + * worktree may not exist yet (git worktree add never ran or failed before + * creating it), and the branch may not exist either — and, since it renames + * rather than asking git to unlink, it returns instantly however far the + * install got before the create failed. */ export async function scrapTree(deps: CreateDeps, rec: TreeRecord): Promise { - await runGit(deps.repoPath, ["worktree", "remove", "--force", rec.path], { - timeoutMs: REMOVE_TIMEOUT_MS, - }); + const trashed = await trashTree(rec.path, rec.name); + if (trashed.ok) { + void reapTrashDir(trashed.trashPath, deps.log); + } else if (existsSync(rec.path)) { + // Absent is the normal case here (`git worktree add` may never have run); + // present-and-unrenameable means the tree is still on disk, so keep its + // branch and registry record — dropping them would strand a live + // directory as unmanaged. The reconciler scraps orphaned `creating` + // entries every pass, so this retries until the rename goes through. + deps.log.warn( + { repo: deps.repoName, tree: rec.name, path: rec.path, err: trashed.err }, + "worktree scrap: trash rename failed", + ); + return; + } + // Collects the registration whose directory just went missing. Unconditional: + // scrap is the tolerant path, and every step below runs on best effort. + await runGit(deps.repoPath, ["worktree", "prune"]); if (rec.branch) { await runGit(deps.repoPath, ["branch", "-D", rec.branch]); } diff --git a/lib/worktree/dispose.ts b/lib/worktree/dispose.ts index e69391a0..1dd3857b 100644 --- a/lib/worktree/dispose.ts +++ b/lib/worktree/dispose.ts @@ -18,16 +18,13 @@ import { hasFreshAttendantLease } from "./lease.ts"; import { loadSyncConfig, matchRule } from "../sync-config.ts"; import { repoDataDir } from "../rt-paths.ts"; import { killWorktreeProcesses } from "../daemon/worktree-process-kill.ts"; +import { reapTrashDir, trashTree } from "./trash.ts"; /** Merge-reactor disposals ignore claims younger than this (stale-event protection). */ const GRACE_MS = 10 * 60_000; const FETCH_TIMEOUT_MS = 2 * 60_000; -/** `git worktree remove` deletes node_modules by hand; a pnpm-scale tree on - * APFS routinely outruns runGit's 60s default. */ -const REMOVE_TIMEOUT_MS = 5 * 60_000; - // ─── Dirty classification (harvested from parking-lot.ts, execSync → runGit) ── /** One entry from `git status --porcelain`. */ @@ -252,26 +249,26 @@ export async function disposeTree( } } - // --force here is plumbing, not a safety statement: `git worktree remove` - // refuses on any untracked file, and the guard above (or the caller's - // explicit force) already made the decision. - const removal = await runGit(repoPath, ["worktree", "remove", "--force", rec.path], { - timeoutMs: REMOVE_TIMEOUT_MS, - }); - if (removal.exitCode !== 0) { - const output = (removal.stdout + removal.stderr).trim(); + // One atomic rename, not a recursive unlink: see trash.ts. Everything below + // is fast, so the verb returns in seconds however large the tree was. + const trashed = await trashTree(rec.path, rec.name); + if (!trashed.ok) { log.warn( - { repo: repoName, tree: rec.name, path: rec.path, output }, - "git worktree remove failed during dispose", + { repo: repoName, tree: rec.name, path: rec.path, err: trashed.err }, + "worktree trash rename failed during dispose", ); // A directory that is already gone is the expected failure and disposal // continues (the registry row is the thing left to clean up). A tree still - // on disk means removal genuinely failed — locked file, permissions — and - // pruning the registry there would orphan a real worktree with its - // metadata lost. Refuse instead; the caller retries or forces. + // at rec.path means the rename genuinely failed — held directory, + // permissions — and pruning the registry there would orphan a real + // worktree with its metadata lost. Refuse instead; the caller retries. if (existsSync(rec.path)) return refuse("remove-failed"); } + // The registration now points at a path that no longer exists, which is + // exactly what prune collects. + await runGit(repoPath, ["worktree", "prune"]); + if (rec.branch) { // Already-gone branches are expected (the reactor disposes after a merge // that may have deleted it), so a failure here is not worth a warning. @@ -287,7 +284,19 @@ export async function disposeTree( branch: rec.branch, discarded, }); - log.info({ repo: repoName, tree: rec.name, path: rec.path }, "worktree disposed"); + log.info( + { + repo: repoName, + tree: rec.name, + path: rec.path, + ...(trashed.ok ? { trash: trashed.trashPath } : {}), + }, + "worktree disposed", + ); + + // Fire-and-forget: the bytes go away on their own time, and a trash dir this + // process never gets to finish is swept by the reconciler's reap duty. + if (trashed.ok) void reapTrashDir(trashed.trashPath, log); return { disposed: true }; } diff --git a/lib/worktree/trash.ts b/lib/worktree/trash.ts new file mode 100644 index 00000000..6a2ba373 --- /dev/null +++ b/lib/worktree/trash.ts @@ -0,0 +1,128 @@ +/** + * Trash-and-reap: how rt makes a worktree directory go away. + * + * `git worktree remove` unlinks the tree file by file. On a pnpm-scale + * node_modules that is minutes of syscalls, and it runs inline in whatever + * verb asked for the disposal — so `rt worktree dispose` blocked for as long + * as the tree was big, and the timeout that bounded it killed the unlink + * mid-flight, leaving a half-deleted directory that was neither a worktree nor + * gone. + * + * So disposal renames instead: one same-volume rename into a sibling + * `.trash--` directory. That is atomic and instant regardless of + * tree size, and it either happened or it didn't — never half. Everything + * after it (prune, branch -D, registry) is fast and keys off the same fact: + * the tree dir is no longer at rec.path. Verb latency becomes O(seconds). + * + * The real unlink is a detached `rm -rf` nobody waits on and nothing times + * out. If that process (or the whole daemon) dies mid-delete, the leftover is + * a `.trash-*` directory, which is exactly what the reconciler's reap duty + * sweeps on a later pass — a crash costs disk, never correctness. + */ + +import { readdir, rename } from "fs/promises"; +import { basename, dirname, join } from "path"; + +/** Marks a directory as rt's to delete. Nothing without this prefix is ever reaped. */ +export const TRASH_PREFIX = ".trash-"; + +/** Minimal log surface: pino's, and the plain object the CLI paths pass. */ +export interface TrashLog { + warn: (...args: unknown[]) => void; +} + +export type TrashResult = { ok: true; trashPath: string } | { ok: false; err: unknown }; + +/** + * The sibling directory `path` gets renamed to. A sibling (not a shared trash + * root) so the rename never crosses a filesystem, which is what makes it + * atomic and instant; the epoch stamp keeps repeat disposals of the same tree + * name from colliding. A name with a path separator could steer the rename + * outside the root (and past the basename guard in reapTrashDir), so it is + * rejected outright. + */ +export function trashPathFor(path: string, name: string, now: number = Date.now()): string { + if (!name || name.includes("/") || name.includes("\\")) { + throw new Error(`worktree trash name must be a single path component: ${JSON.stringify(name)}`); + } + return join(dirname(path), `${TRASH_PREFIX}${name}-${now}`); +} + +/** + * Rename the tree out of the way. Never throws: a busy or unwritable directory + * comes back as `{ ok: false }` and the caller decides — dispose refuses + * ("remove-failed"), scrap shrugs, because mid-create the tree may not exist + * at all. + */ +export async function trashTree(path: string, name: string): Promise { + try { + const trashPath = trashPathFor(path, name); + await rename(path, trashPath); + return { ok: true, trashPath }; + } catch (err) { + return { ok: false, err }; + } +} + +/** + * Delete a trash directory in a detached `rm -rf` with no timeout: nothing + * upstream waits for it, and a kill mid-unlink is the very failure mode this + * design exists to remove. The returned promise settles when the child does, + * for the reconciler's one-at-a-time sweep (and for tests); callers on the + * dispose path drop it. + * + * Refuses any path not named `.trash-*` — the one guard between this and an + * `rm -rf` of a live worktree. + */ +export async function reapTrashDir(trashPath: string, log: TrashLog): Promise { + if (!basename(trashPath).startsWith(TRASH_PREFIX)) { + log.warn({ path: trashPath }, "worktree reap refused: not a trash directory"); + return; + } + + try { + const proc = Bun.spawn(["rm", "-rf", "--", trashPath], { + stdin: "ignore", + stdout: "ignore", + stderr: "ignore", + // Survives the CLI process that started it: the point is that no caller + // has to stay alive for the delete to finish. + detached: true, + }); + proc.unref(); + const exitCode = await proc.exited; + if (exitCode !== 0) { + log.warn({ path: trashPath, exitCode }, "worktree trash reap failed"); + } + } catch (err) { + log.warn({ err, path: trashPath }, "worktree trash reap could not be spawned"); + } +} + +/** + * Sweep every `.trash-*` directory out of `roots`, one at a time (a reap is + * IO-bound and there is never a hurry — this is the crash-leftover path, not + * the dispose path). A root that does not exist is normal, not an error: a + * repo may simply never have had a worktree. Returns how many were reaped. + */ +export async function reapTrashInRoots(roots: string[], log: TrashLog): Promise { + let reaped = 0; + for (const root of new Set(roots)) { + let entries: string[]; + try { + entries = await readdir(root); + } catch (err) { + // A root that never existed is normal; anything else hides stale trash. + if ((err as NodeJS.ErrnoException)?.code !== "ENOENT") { + log.warn({ err, root }, "worktree trash sweep could not read root"); + } + continue; + } + for (const entry of entries) { + if (!entry.startsWith(TRASH_PREFIX)) continue; + await reapTrashDir(join(root, entry), log); + reaped += 1; + } + } + return reaped; +}