diff --git a/README.md b/README.md index 46d47cf..35da074 100644 --- a/README.md +++ b/README.md @@ -362,7 +362,7 @@ interface GroveLease { | `destroying` | Worktree removal in progress; idempotent `destroy()` resumes | | `quarantined` | Blocked; requires `repair()` or `destroy()` | -Re-acquiring the same `leaseId` with a compatible branch/ref is idempotent. Conflicting targets throw `LEASE_CONFLICT`. +Re-acquiring the same `leaseId` with a compatible branch/ref reuses its lease allocation and checkout. Lifecycle hooks may still run for the new call. Conflicting targets throw `LEASE_CONFLICT`. Branch creation defaults should be fail-first for new work. Use `ifExists: "reuse"` only when intentionally resuming or attaching to an existing local branch. `repair({ action: "resume-acquire" })` may reuse a branch created by the interrupted acquire so recovery can complete. @@ -373,13 +373,17 @@ Configure shell commands in `createGrove({ hooks })`. Hook cwd is the worktree p | Hook | When | |------|------| | `postCreate` | After a new physical slot is created | -| `postAcquire` | After branch/ref checkout | +| `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 | 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. + +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`. Set `onHookFailure: "fail"` to throw `HOOK_FAILED` on hook errors. diff --git a/packages/grove/src/lease-acquire.ts b/packages/grove/src/lease-acquire.ts index 8d1defc..1ec61dd 100644 --- a/packages/grove/src/lease-acquire.ts +++ b/packages/grove/src/lease-acquire.ts @@ -1,8 +1,13 @@ import { existsSync } from "node:fs"; -import type { GroveConfig, GroveFailedPhase, GroveLeaseTarget } from "./schemas.js"; +import type { + GroveConfig, + GroveFailedPhase, + GroveLeaseRecord, + GroveLeaseTarget, +} from "./schemas.js"; import type { AcquireLeaseOptions, GroveLease } from "./types.js"; import { checkoutBranch, checkoutDetached, getDefaultBranch, getHeadSha } from "./git/index.js"; -import { withStateLock } from "./lock.js"; +import { withLeaseHookLock, withStateLock } from "./lock.js"; import { assertCompatibleReacquire, branchOwnedByOtherLease, @@ -134,6 +139,16 @@ export async function quarantineFailedAcquire( }); } +async function runPostAcquireHook( + lease: GroveLease, + postAcquire?: (path: string, lease: GroveLease) => Promise, +): Promise { + if (!postAcquire) return; + + // postAcquire runs after the lease is usable; hook failures surface without quarantine. + await withLeaseHookLock(lease.path, () => postAcquire(lease.path, lease)); +} + /** Internal acquire mutator; `fetchOrigin` is handled by `Grove.acquire()` at the public boundary. */ export async function acquireLease( poolDir: string, @@ -151,7 +166,7 @@ export async function acquireLease( let targetWtPath = ""; let isNewSlot = false; - let returningExisting = false; + let existingLeaseRecord: GroveLeaseRecord | undefined; let leaseIdForCheckout = options.leaseId; await withStateLock(poolDir, async () => { @@ -191,7 +206,7 @@ export async function acquireLease( } await savePoolState(poolDir, state); targetWtPath = existing.path; - returningExisting = true; + existingLeaseRecord = { ...existing }; return; } } @@ -230,10 +245,10 @@ export async function acquireLease( leaseIdForCheckout = options.leaseId; }); - if (returningExisting) { - const state = await loadPoolState(poolDir, repoRoot); - const lease = findLease(state, options.leaseId)!; - return enrichLeaseReadOnly(lease); + if (existingLeaseRecord) { + const lease = await enrichLeaseReadOnly(existingLeaseRecord); + await runPostAcquireHook(lease, hooks.postAcquire); + return lease; } let lease: GroveLease; @@ -277,10 +292,7 @@ export async function acquireLease( throw err; } - if (hooks.postAcquire) { - // postAcquire runs after the lease is usable; hook failures are surfaced without quarantine. - await hooks.postAcquire(targetWtPath, lease); - } + await runPostAcquireHook(lease, hooks.postAcquire); return lease; } @@ -375,10 +387,7 @@ export async function resumeAcquireLease( throw err; } - if (hooks.postAcquire) { - // postAcquire runs after the lease is usable; hook failures are surfaced without quarantine. - await hooks.postAcquire(wtPath, lease); - } + await runPostAcquireHook(lease, hooks.postAcquire); return lease; } diff --git a/packages/grove/src/lock.ts b/packages/grove/src/lock.ts index c7b1348..1c59ec6 100644 --- a/packages/grove/src/lock.ts +++ b/packages/grove/src/lock.ts @@ -3,15 +3,11 @@ import { mkdir, writeFile } from "node:fs/promises"; import { join } from "node:path"; import { LockFailedError } from "./errors.js"; -export async function withStateLock( - groveDir: string, +async function withLock( + lockTarget: string, fn: () => Promise, opts?: LockOptions, ): Promise { - await mkdir(groveDir, { recursive: true }); - const lockTarget = join(groveDir, "grove-state.lock"); - await writeFile(lockTarget, "", { flag: "a" }); - const lockOpts: LockOptions = { retries: opts?.retries ?? { retries: 300, minTimeout: 500, maxTimeout: 2000 }, ...opts, @@ -30,3 +26,20 @@ export async function withStateLock( await release(); } } + +export async function withStateLock( + groveDir: string, + fn: () => Promise, + opts?: LockOptions, +): Promise { + await mkdir(groveDir, { recursive: true }); + const lockTarget = join(groveDir, "grove-state.lock"); + await writeFile(lockTarget, "", { flag: "a" }); + + return withLock(lockTarget, fn, opts); +} + +/** Serialize lease hooks that operate on the same active worktree. */ +export async function withLeaseHookLock(worktreePath: string, fn: () => Promise): Promise { + return withLock(worktreePath, fn); +} diff --git a/packages/grove/test/lease-hooks.integration.test.ts b/packages/grove/test/lease-hooks.integration.test.ts index 40c7341..97bcaf0 100644 --- a/packages/grove/test/lease-hooks.integration.test.ts +++ b/packages/grove/test/lease-hooks.integration.test.ts @@ -1,12 +1,19 @@ import { describe, it, expect, vi } from "vitest"; -import { readFile } from "node:fs/promises"; +import { readFile, writeFile } from "node:fs/promises"; import { join } from "node:path"; +import { execa } from "execa"; import { createTestGrove } from "./helpers/test-grove.js"; import { setupRepo } from "./helpers/git-repo.js"; import { failOnceHook, registerLeaseIntegrationCleanup } from "./helpers/lease-integration.js"; import { releaseLease } from "../src/lease-release.js"; import * as hooksModule from "../src/hooks.js"; +function countHook(counterPath: string): string { + const script = + "const fs = require('node:fs'); const p = process.argv[1]; const n = fs.existsSync(p) ? Number(fs.readFileSync(p, 'utf8')) : 0; fs.writeFileSync(p, String(n + 1));"; + return ["node", "-e", JSON.stringify(script), JSON.stringify(counterPath)].join(" "); +} + describe("lease hooks integration", () => { const cleanup = registerLeaseIntegrationCleanup(); @@ -38,6 +45,112 @@ describe("lease hooks integration", () => { expect(leases[0]?.state).toBe("leased"); }); + it("reruns postAcquire for each compatible reacquire without recreating the checkout", async () => { + const { repoDir, tmpDir, groveDir } = await setupRepo(); + cleanup.tmpDirs.push(tmpDir); + + const postCreateAttemptsPath = join(tmpDir, "post-create-attempts.txt"); + const postAcquireAttemptsPath = join(tmpDir, "post-acquire-attempts.txt"); + const grove = await createTestGrove({ + repoRoot: repoDir, + groveRoot: groveDir, + onHookFailure: "fail", + hooks: { + postCreate: [countHook(postCreateAttemptsPath)], + postAcquire: [failOnceHook(postAcquireAttemptsPath)], + }, + }); + const initialOptions = { + leaseId: "post-acquire-retry", + mode: "branch" as const, + branch: "post-acquire-retry-branch", + createBranch: { from: "main", ifExists: "fail" as const }, + }; + + await expect(grove.acquire(initialOptions)).rejects.toMatchObject({ code: "HOOK_FAILED" }); + + const committed = await grove.inspect(initialOptions.leaseId); + expect(committed).toMatchObject({ leaseId: initialOptions.leaseId, state: "leased" }); + expect(await readFile(postCreateAttemptsPath, "utf8")).toBe("1"); + expect(await readFile(postAcquireAttemptsPath, "utf8")).toBe("1"); + + await execa("git", ["commit", "--allow-empty", "-m", "preserve reacquire head"], { + cwd: committed!.path, + }); + const dirtyPath = join(committed!.path, "dirty.txt"); + await writeFile(dirtyPath, "keep me\n"); + const { stdout: headBefore } = await execa("git", ["rev-parse", "HEAD"], { + cwd: committed!.path, + }); + + const explicit = await grove.acquire({ + leaseId: initialOptions.leaseId, + mode: "branch", + branch: initialOptions.branch, + ifLeased: "return-existing", + }); + const implicit = await grove.acquire({ + leaseId: initialOptions.leaseId, + mode: "branch", + branch: initialOptions.branch, + }); + + expect(explicit).toMatchObject({ + leaseId: committed!.leaseId, + path: committed!.path, + state: "leased", + }); + expect(implicit).toMatchObject({ + leaseId: committed!.leaseId, + path: committed!.path, + state: "leased", + }); + expect(await readFile(postAcquireAttemptsPath, "utf8")).toBe("3"); + expect(await readFile(postCreateAttemptsPath, "utf8")).toBe("1"); + expect(await readFile(dirtyPath, "utf8")).toBe("keep me\n"); + await expect( + execa("git", ["rev-parse", "HEAD"], { cwd: committed!.path }), + ).resolves.toMatchObject({ stdout: headBefore }); + expect((await grove.stats()).pool.used).toBe(1); + }); + + it("does not run postAcquire for rejected existing-lease acquisitions", async () => { + const { repoDir, tmpDir, groveDir } = await setupRepo(); + cleanup.tmpDirs.push(tmpDir); + + const attemptsPath = join(tmpDir, "rejected-post-acquire-attempts.txt"); + const grove = await createTestGrove({ + repoRoot: repoDir, + groveRoot: groveDir, + hooks: { postAcquire: [countHook(attemptsPath)] }, + }); + const lease = await grove.acquire({ + leaseId: "rejected-post-acquire", + mode: "branch", + branch: "rejected-post-acquire-branch", + createBranch: { from: "main", ifExists: "fail" }, + }); + + await expect( + grove.acquire({ + leaseId: lease.leaseId, + mode: "branch", + branch: lease.branch!, + ifLeased: "fail", + }), + ).rejects.toMatchObject({ code: "LEASE_ALREADY_EXISTS" }); + await expect( + grove.acquire({ + leaseId: lease.leaseId, + mode: "branch", + branch: "incompatible-post-acquire-branch", + ifLeased: "return-existing", + }), + ).rejects.toMatchObject({ code: "LEASE_CONFLICT" }); + + expect(await readFile(attemptsPath, "utf8")).toBe("1"); + }); + it("postCreate hook failure quarantines a pending acquire", async () => { const { repoDir, tmpDir, groveDir } = await setupRepo(); cleanup.tmpDirs.push(tmpDir); diff --git a/packages/grove/test/state.test.ts b/packages/grove/test/state.test.ts index c127ae1..514049c 100644 --- a/packages/grove/test/state.test.ts +++ b/packages/grove/test/state.test.ts @@ -1,7 +1,7 @@ import { describe, it, expect, beforeEach, afterEach } from "vitest"; import { setupPathFixture } from "./helpers/git-repo.js"; import { readState, writeState, healState, stateFilePath } from "../src/state.js"; -import { withStateLock } from "../src/lock.js"; +import { withLeaseHookLock, withStateLock } from "../src/lock.js"; import type { GroveState } from "../src/schemas.js"; import { InvalidGroveStateError, LockFailedError } from "../src/errors.js"; import { rm, writeFile, mkdir, stat } from "node:fs/promises"; @@ -98,6 +98,51 @@ describe("State & Locking", () => { }); }); + describe("withLeaseHookLock", () => { + it("serializes callbacks for the same worktree", async () => { + const worktreePath = join(tmpDir, "hook-lock-worktree"); + await mkdir(worktreePath, { recursive: true }); + + const events: string[] = []; + let signalFirstStarted!: () => void; + const firstStarted = new Promise((resolve) => { + signalFirstStarted = resolve; + }); + let releaseFirst!: () => void; + const holdFirst = new Promise((resolve) => { + releaseFirst = resolve; + }); + const first = withLeaseHookLock(worktreePath, async () => { + events.push("first:start"); + signalFirstStarted(); + await holdFirst; + events.push("first:end"); + }); + await firstStarted; + + let signalSecondCalled!: () => void; + const secondCalled = new Promise((resolve) => { + signalSecondCalled = resolve; + }); + const second = (async () => { + signalSecondCalled(); + await withLeaseHookLock(worktreePath, async () => { + events.push("second:start"); + }); + })(); + await secondCalled; + + try { + expect(events).toEqual(["first:start"]); + } finally { + releaseFirst(); + await Promise.all([first, second]); + } + + expect(events).toEqual(["first:start", "first:end", "second:start"]); + }); + }); + describe("healState", () => { it("drops entries with missing paths", async () => { const state: GroveState = {