diff --git a/README.md b/README.md index 35da074..aebc4cc 100644 --- a/README.md +++ b/README.md @@ -376,12 +376,14 @@ Configure shell commands in `createGrove({ hooks })`. Hook cwd is the worktree p | `postAcquire` | After a fresh branch/ref checkout and on every compatible re-acquire | | `preRelease` | Before lease cleanup | | `postRelease` | After lease cleanup | -| `preDestroy` | Before worktree removal | +| `preDestroy` | Before worktree removal, while the worktree path still exists | Crash recovery may run `postCreate` more than once. Keep that hook idempotent; reused physical slots do not run it. `postAcquire` runs once for every compatible `acquire()` call, including calls that return an existing lease. A retry starts the configured command list from the beginning, so keep every command idempotent. Grove serializes `postAcquire` executions for the same worktree, while different worktrees may run hooks concurrently. +Resuming `destroy()` after the worktree was already removed skips `preDestroy` and converges the remaining Git registration, slot directory, and state cleanup. Process safety checks cover the entire owned slot directory before hooks and physical removal. + Hook configuration is programmatic and is not persisted with lease state. Every process that acquires from a pool must create Grove with the intended, consistent hook configuration. Lease hooks receive: `GROVE_LEASE_ID`, `GROVE_SLOT_NAME`, `GROVE_BRANCH`, `GROVE_REPO_ROOT`, `GROVE_WORKTREE_PATH`. diff --git a/packages/grove/src/git/worktree.ts b/packages/grove/src/git/worktree.ts index 1a6371f..3a9efac 100644 --- a/packages/grove/src/git/worktree.ts +++ b/packages/grove/src/git/worktree.ts @@ -1,5 +1,6 @@ import { runGit } from "./run.js"; import { branchRef, hasRemote } from "./branch.js"; +import { resolvePathWithExistingAncestor } from "../path-boundary.js"; export async function addWorktree(repoRoot: string, path: string, branch: string): Promise { const ref = await branchRef(repoRoot, branch); @@ -10,6 +11,30 @@ export async function removeWorktree(repoRoot: string, path: string): Promise { + const targetPath = await resolvePathWithExistingAncestor(path); + const output = await runGit(repoRoot, ["worktree", "list", "--porcelain", "-z"]); + const registeredPaths = output + .split("\0") + .filter((field) => field.startsWith("worktree ")) + .map((field) => field.slice("worktree ".length)); + const registeredTargets = await Promise.all(registeredPaths.map(resolvePathWithExistingAncestor)); + return registeredTargets.includes(targetPath); +} + +export async function removeWorktreeIfRegistered(repoRoot: string, path: string): Promise { + if (!(await isWorktreeRegistered(repoRoot, path))) { + return; + } + try { + await removeWorktree(repoRoot, path); + } catch (error) { + if (await isWorktreeRegistered(repoRoot, path)) { + throw error; + } + } +} + export async function resetWorktree( path: string, branch: string, diff --git a/packages/grove/src/lease-destroy.ts b/packages/grove/src/lease-destroy.ts index 84665b3..2d251c9 100644 --- a/packages/grove/src/lease-destroy.ts +++ b/packages/grove/src/lease-destroy.ts @@ -1,8 +1,9 @@ import { basename, dirname, normalize } from "node:path"; +import { existsSync } from "node:fs"; import { rm } from "node:fs/promises"; import type { GroveConfig, GroveLeaseRecord, GroveSlot } from "./schemas.js"; import type { DestroyLeaseOptions } from "./types.js"; -import { removeWorktree } from "./git/index.js"; +import { removeWorktreeIfRegistered } from "./git/index.js"; import { withStateLock } from "./lock.js"; import { assertPathWithinPool } from "./path-boundary.js"; import { isWorktreeInUse } from "./process/detect.js"; @@ -151,6 +152,23 @@ async function loadDestroyRemovalTarget( return { lease, slot, wtPath: slot.path }; } +async function loadValidatedDestroyTarget( + poolDir: string, + repoRoot: string, + context: DestroyContext, +): Promise<{ lease: GroveLeaseRecord; slot: GroveSlot; wtPath: string; slotDir: string }> { + const target = await loadDestroyRemovalTarget( + poolDir, + repoRoot, + context.leaseId, + context.slotName, + context.wtPath, + ); + await assertPathWithinPool(poolDir, target.wtPath); + const slotDir = destroySlotDirectory(poolDir, target.wtPath, target.slot.slotName); + return { ...target, slotDir }; +} + async function assertDestroyStillPending( poolDir: string, repoRoot: string, @@ -244,7 +262,16 @@ async function completeDestroy( return; } - await hooks.preDestroy?.(context.wtPath, context.leaseEnvVars); + const beforeHook = await loadValidatedDestroyTarget(poolDir, config.repoRoot, context); + await assertWorktreeSafeForCleanup(beforeHook.slotDir, beforeHook.slot, beforeHook.lease, { + force: context.force, + ignoreOwnerReservation: true, + message: DESTROY_UNSAFE_MESSAGE(beforeHook.slotDir), + }); + + if (existsSync(beforeHook.wtPath)) { + await hooks.preDestroy?.(beforeHook.wtPath, context.leaseEnvVars); + } if ( !(await assertDestroyStillPending( @@ -257,23 +284,19 @@ async function completeDestroy( return; } - const { lease, slot, wtPath } = await loadDestroyRemovalTarget( + const { lease, slot, wtPath, slotDir } = await loadValidatedDestroyTarget( poolDir, config.repoRoot, - context.leaseId, - context.slotName, - context.wtPath, + context, ); - await assertWorktreeSafeForCleanup(wtPath, slot, lease, { + await assertWorktreeSafeForCleanup(slotDir, slot, lease, { force: context.force, ignoreOwnerReservation: true, - message: DESTROY_UNSAFE_MESSAGE(wtPath), + message: DESTROY_UNSAFE_MESSAGE(slotDir), }); - await assertPathWithinPool(poolDir, wtPath); - const slotDir = destroySlotDirectory(poolDir, wtPath, slot.slotName); - await removeWorktree(config.repoRoot, wtPath); + await removeWorktreeIfRegistered(config.repoRoot, wtPath); await rm(slotDir, { recursive: true, force: true }); } catch (err) { const reason = err instanceof Error ? err.message : "destroy failed"; diff --git a/packages/grove/src/path-boundary.ts b/packages/grove/src/path-boundary.ts index 2b21d5f..b207990 100644 --- a/packages/grove/src/path-boundary.ts +++ b/packages/grove/src/path-boundary.ts @@ -1,15 +1,38 @@ -import { isAbsolute, relative } from "node:path"; +import { basename, dirname, isAbsolute, join, relative, resolve } from "node:path"; import { realpath } from "node:fs/promises"; import { PathOutsidePoolError } from "./errors.js"; -export async function assertPathWithinPool(poolDir: string, targetPath: string): Promise { - const poolReal = await realpath(poolDir); - let targetReal = targetPath; - try { - targetReal = await realpath(targetPath); - } catch { - // Target may not exist yet; use the configured path for boundary checks. +function isMissingPathError(error: unknown): error is NodeJS.ErrnoException { + return ( + error instanceof Error && + "code" in error && + (error.code === "ENOENT" || error.code === "ENOTDIR") + ); +} + +export async function resolvePathWithExistingAncestor(targetPath: string): Promise { + let ancestor = resolve(targetPath); + const missingSegments: string[] = []; + + while (true) { + try { + const ancestorReal = await realpath(ancestor); + return join(ancestorReal, ...missingSegments.reverse()); + } catch (error) { + if (!isMissingPathError(error) || dirname(ancestor) === ancestor) { + throw error; + } + missingSegments.push(basename(ancestor)); + ancestor = dirname(ancestor); + } } +} + +export async function assertPathWithinPool(poolDir: string, targetPath: string): Promise { + const [poolReal, targetReal] = await Promise.all([ + realpath(poolDir), + resolvePathWithExistingAncestor(targetPath), + ]); const rel = relative(poolReal, targetReal); if (rel.startsWith("..") || isAbsolute(rel)) { diff --git a/packages/grove/test/lease-destroy.integration.test.ts b/packages/grove/test/lease-destroy.integration.test.ts index d190663..13f5e72 100644 --- a/packages/grove/test/lease-destroy.integration.test.ts +++ b/packages/grove/test/lease-destroy.integration.test.ts @@ -1,12 +1,28 @@ import { describe, it, expect } from "vitest"; import { execa } from "execa"; import { existsSync } from "node:fs"; -import { mkdir, readFile, writeFile } from "node:fs/promises"; -import { join } from "node:path"; +import { mkdir, readFile, realpath, rm, symlink, writeFile } from "node:fs/promises"; +import { dirname, join } from "node:path"; import { createTestGrove } from "./helpers/test-grove.js"; import { setupRepo } from "./helpers/git-repo.js"; import { registerLeaseIntegrationCleanup } from "./helpers/lease-integration.js"; import { readLeaseFirstState, writeLeaseFirstState } from "../src/state-v1.js"; +import { destroyLease } from "../src/lease-destroy.js"; + +function writeMarkerHook(markerPath: string): string { + const script = "require('node:fs').writeFileSync(process.argv[1], 'ran')"; + return ["node", "-e", JSON.stringify(script), JSON.stringify(markerPath)].join(" "); +} + +async function gitWorktreePaths(repoDir: string): Promise { + const { stdout } = await execa("git", ["worktree", "list", "--porcelain", "-z"], { + cwd: repoDir, + }); + return stdout + .split("\0") + .filter((field) => field.startsWith("worktree ")) + .map((field) => field.slice("worktree ".length)); +} describe("lease destroy integration", () => { const cleanup = registerLeaseIntegrationCleanup(); @@ -112,6 +128,177 @@ describe("lease destroy integration", () => { expect(existsSync(lease.path)).toBe(false); }); + it("blocks destroy before preDestroy when a process uses the owned slot directory", async () => { + const { repoDir, tmpDir, groveDir } = await setupRepo(); + cleanup.tmpDirs.push(tmpDir); + + const markerPath = join(tmpDir, "pre-destroy-ran.txt"); + const grove = await createTestGrove({ + repoRoot: repoDir, + groveRoot: groveDir, + onHookFailure: "fail", + hooks: { preDestroy: [writeMarkerHook(markerPath)] }, + }); + const lease = await grove.acquire({ + leaseId: "slot-root-safety", + mode: "branch", + branch: "slot-root-safety-branch", + createBranch: { from: "main", ifExists: "fail" }, + }); + + const state = await readLeaseFirstState(grove.poolDir); + state.slots[0]!.ownerPid = undefined; + state.slots[0]!.ownerStartedAt = undefined; + await writeLeaseFirstState(grove.poolDir, state); + + const siblingPath = join(dirname(lease.path), "worker-cache"); + await mkdir(siblingPath); + const child = execa(process.execPath, ["-e", "setInterval(() => {}, 1000)"], { + cwd: siblingPath, + reject: false, + }); + await new Promise((resolve, reject) => { + child.once("spawn", resolve); + child.once("error", reject); + }); + + try { + await expect(grove.destroy(lease.leaseId)).rejects.toMatchObject({ + code: "UNSAFE_CLEANUP", + }); + expect(existsSync(markerPath)).toBe(false); + expect(existsSync(lease.path)).toBe(true); + expect(await grove.inspect(lease.leaseId)).toMatchObject({ state: "quarantined" }); + } finally { + child.kill("SIGTERM"); + await child; + } + }); + + it("blocks destroy when preDestroy starts a process in the owned slot directory", async () => { + const { repoDir, tmpDir, groveDir } = await setupRepo(); + cleanup.tmpDirs.push(tmpDir); + + const grove = await createTestGrove({ repoRoot: repoDir, groveRoot: groveDir }); + const lease = await grove.acquire({ + leaseId: "post-hook-slot-root-safety", + mode: "branch", + branch: "post-hook-slot-root-safety-branch", + createBranch: { from: "main", ifExists: "fail" }, + }); + const state = await readLeaseFirstState(grove.poolDir); + state.slots[0]!.ownerPid = undefined; + state.slots[0]!.ownerStartedAt = undefined; + await writeLeaseFirstState(grove.poolDir, state); + + const siblingPath = join(dirname(lease.path), "hook-worker-cache"); + await mkdir(siblingPath); + let stopChild = async (): Promise => {}; + try { + await expect( + destroyLease(grove.poolDir, { repoRoot: repoDir }, lease.leaseId, undefined, { + preDestroy: async () => { + const child = execa(process.execPath, ["-e", "setInterval(() => {}, 1000)"], { + cwd: siblingPath, + reject: false, + }); + stopChild = async () => { + child.kill("SIGTERM"); + await child; + }; + await new Promise((resolve, reject) => { + child.once("spawn", resolve); + child.once("error", reject); + }); + }, + }), + ).rejects.toMatchObject({ code: "UNSAFE_CLEANUP" }); + + expect(existsSync(lease.path)).toBe(true); + expect(await grove.inspect(lease.leaseId)).toMatchObject({ state: "quarantined" }); + } finally { + await stopChild(); + } + }); + + it.each(["present", "absent"] as const)( + "resumes destroy when the checkout is missing and Git registration is %s", + async (registration) => { + const { repoDir, tmpDir, groveDir } = await setupRepo(); + cleanup.tmpDirs.push(tmpDir); + + const grove = await createTestGrove({ + repoRoot: repoDir, + groveRoot: groveDir, + onHookFailure: "fail", + hooks: { preDestroy: ["exit 1"] }, + }); + const lease = await grove.acquire({ + leaseId: `missing-checkout-${registration}`, + mode: "branch", + branch: `missing-checkout-${registration}-branch`, + createBranch: { from: "main", ifExists: "fail" }, + }); + + const state = await readLeaseFirstState(grove.poolDir); + state.leases[0]!.state = "destroying"; + state.slots[0]!.state = "destroying"; + state.slots[0]!.ownerPid = process.pid; + await writeLeaseFirstState(grove.poolDir, state); + + if (registration === "present") { + await rm(lease.path, { recursive: true, force: true }); + } else { + await execa("git", ["worktree", "remove", "--force", "--", lease.path], { + cwd: repoDir, + }); + } + + await grove.destroy(lease.leaseId, { force: true }); + + expect(await grove.inspect(lease.leaseId)).toBeNull(); + expect(existsSync(dirname(lease.path))).toBe(false); + expect(await gitWorktreePaths(repoDir)).not.toContain(lease.path); + }, + ); + + it("resumes missing-checkout destroy through a symlinked pool path", async () => { + const { repoDir, tmpDir } = await setupRepo(); + cleanup.tmpDirs.push(tmpDir); + + const physicalRoot = join(tmpDir, "physical-grove"); + const linkedRoot = join(tmpDir, "linked-grove"); + await mkdir(physicalRoot); + await symlink(physicalRoot, linkedRoot, process.platform === "win32" ? "junction" : "dir"); + + const grove = await createTestGrove({ + repoRoot: repoDir, + groveDir: join(linkedRoot, "pool"), + onHookFailure: "fail", + hooks: { preDestroy: ["exit 1"] }, + }); + const lease = await grove.acquire({ + leaseId: "symlinked-missing-checkout", + mode: "branch", + branch: "symlinked-missing-checkout-branch", + createBranch: { from: "main", ifExists: "fail" }, + }); + const physicalLeasePath = await realpath(lease.path); + + const state = await readLeaseFirstState(grove.poolDir); + state.leases[0]!.state = "destroying"; + state.slots[0]!.state = "destroying"; + state.slots[0]!.ownerPid = process.pid; + await writeLeaseFirstState(grove.poolDir, state); + await rm(lease.path, { recursive: true, force: true }); + + await grove.destroy(lease.leaseId, { force: true }); + + expect(await grove.inspect(lease.leaseId)).toBeNull(); + expect(existsSync(dirname(lease.path))).toBe(false); + expect(await gitWorktreePaths(repoDir)).not.toContain(physicalLeasePath); + }); + it("rejects deleteBranch in lease-first destroy MVP", async () => { const { repoDir, tmpDir, groveDir } = await setupRepo(); cleanup.tmpDirs.push(tmpDir);