feat(FN-4954): harden pool leasing invariants and wire runtime auditing
Fusion-Task-Id: FN-4954 Fusion-Task-Lineage: b3dd2d2f-3aa3-48f1-8a56-56e66518d139
This commit is contained in:
committed by
gsxdsm
parent
0606e3aaea
commit
7e90bce8bb
@@ -1577,7 +1577,7 @@ describe("Edge case: worktree deleted between scan and acquire", () => {
|
||||
mockedExistsSync.mockReturnValue(false);
|
||||
|
||||
// acquire() checks existsSync and prunes the stale entry
|
||||
const result = pool.acquire();
|
||||
const result = pool.acquire("FN-test");
|
||||
expect(result).toBeNull();
|
||||
expect(pool.size).toBe(0);
|
||||
});
|
||||
|
||||
@@ -54,7 +54,7 @@ describe("acquireTaskWorktree", () => {
|
||||
store,
|
||||
settings: { recycleWorktrees: true } as any,
|
||||
pool: {
|
||||
acquire: () => "/tmp/pooled",
|
||||
acquire: (_taskId: string) => "/tmp/pooled",
|
||||
prepareForTask,
|
||||
release,
|
||||
} as any,
|
||||
@@ -79,7 +79,7 @@ describe("acquireTaskWorktree", () => {
|
||||
store,
|
||||
settings: { recycleWorktrees: true } as any,
|
||||
pool: {
|
||||
acquire: () => "/tmp/pooled",
|
||||
acquire: (_taskId: string) => "/tmp/pooled",
|
||||
prepareForTask: vi.fn().mockResolvedValue({
|
||||
branch: "fusion/fn-1",
|
||||
worktreePath: "/tmp/live-existing",
|
||||
@@ -92,7 +92,7 @@ describe("acquireTaskWorktree", () => {
|
||||
createWorktree: vi.fn(),
|
||||
});
|
||||
|
||||
expect(release).toHaveBeenCalledWith("/tmp/pooled");
|
||||
expect(release).toHaveBeenCalledWith("/tmp/pooled", "FN-1");
|
||||
});
|
||||
|
||||
it("falls through to fresh creation when pooled worktree is incomplete and emits detection audit", async () => {
|
||||
@@ -107,7 +107,7 @@ describe("acquireTaskWorktree", () => {
|
||||
store,
|
||||
settings: { recycleWorktrees: true } as any,
|
||||
pool: {
|
||||
acquire: () => "/tmp/pooled",
|
||||
acquire: (_taskId: string) => "/tmp/pooled",
|
||||
prepareForTask: vi.fn().mockResolvedValue({ branch: "fusion/fn-1", worktreePath: "/tmp/pooled", reclaimed: false }),
|
||||
release: vi.fn(),
|
||||
} as any,
|
||||
|
||||
@@ -10,25 +10,42 @@ vi.mock("node:fs", () => ({
|
||||
|
||||
import { WorktreePool } from "../worktree-pool.js";
|
||||
|
||||
// FN-4954: deterministic repro for the rehydrate collision race.
|
||||
describe("WorktreePool double-lease reproduction", () => {
|
||||
// FN-4954 deterministic race backstop.
|
||||
describe("WorktreePool double-lease guard", () => {
|
||||
let pool: WorktreePool;
|
||||
|
||||
beforeEach(() => {
|
||||
pool = new WorktreePool();
|
||||
});
|
||||
|
||||
it("reproduces rehydrate re-adding a path that is already leased", () => {
|
||||
pool.release("/tmp/wt-race");
|
||||
it("prevents rehydrate from re-adding a leased path", () => {
|
||||
const violations: Array<{ phase: string; existingHolder: string }> = [];
|
||||
pool.setInvariantViolationHandler((violation) => {
|
||||
violations.push({ phase: violation.phase, existingHolder: violation.existingHolder });
|
||||
});
|
||||
|
||||
const firstLease = pool.acquire();
|
||||
pool.release("/tmp/wt-race");
|
||||
const firstLease = pool.acquire("FN-A");
|
||||
expect(firstLease).toBe("/tmp/wt-race");
|
||||
|
||||
// Simulates scan/rehydrate colliding with an in-flight lease.
|
||||
pool.rehydrate(["/tmp/wt-race"]);
|
||||
|
||||
// Expected invariant: leased paths must never be re-added to idle.
|
||||
// Current behavior (pre-fix) returns the same path again here.
|
||||
expect(pool.acquire()).toBeNull();
|
||||
expect(pool.acquire("FN-B")).toBeNull();
|
||||
expect(pool.size).toBe(0);
|
||||
expect(pool.getLeasedPaths().get("/tmp/wt-race")).toBe("FN-A");
|
||||
expect(violations).toEqual([{ phase: "rehydrate", existingHolder: "FN-A" }]);
|
||||
});
|
||||
|
||||
it("keeps release best-effort when releasing task differs", () => {
|
||||
const violations: Array<{ phase: string; requestingTaskId: string }> = [];
|
||||
pool.setInvariantViolationHandler((violation) => violations.push({ phase: violation.phase, requestingTaskId: violation.requestingTaskId }));
|
||||
|
||||
pool.release("/tmp/wt-race");
|
||||
expect(pool.acquire("FN-A")).toBe("/tmp/wt-race");
|
||||
|
||||
pool.release("/tmp/wt-race", "FN-B");
|
||||
expect(pool.has("/tmp/wt-race")).toBe(true);
|
||||
expect(pool.getLeasedPaths().size).toBe(0);
|
||||
expect(violations).toEqual([{ phase: "release", requestingTaskId: "FN-B" }]);
|
||||
});
|
||||
});
|
||||
|
||||
@@ -66,6 +66,7 @@ const mockedExistsSync = vi.mocked(existsSync);
|
||||
const mockedLstatSync = vi.mocked(lstatSync);
|
||||
const mockedReaddirSync = vi.mocked(readdirSync);
|
||||
const mockedRmSync = vi.mocked(rmSync);
|
||||
const TEST_TASK_ID = "FN-test";
|
||||
|
||||
let errorSpy: ReturnType<typeof vi.spyOn>;
|
||||
let warnSpy: ReturnType<typeof vi.spyOn>;
|
||||
@@ -91,12 +92,12 @@ describe("WorktreePool", () => {
|
||||
|
||||
describe("acquire", () => {
|
||||
it("returns null when pool is empty", () => {
|
||||
expect(pool.acquire()).toBeNull();
|
||||
expect(pool.acquire(TEST_TASK_ID)).toBeNull();
|
||||
});
|
||||
|
||||
it("returns a released path on acquire", () => {
|
||||
pool.release("/tmp/worktree-1");
|
||||
const result = pool.acquire();
|
||||
const result = pool.acquire(TEST_TASK_ID);
|
||||
expect(result).toBe("/tmp/worktree-1");
|
||||
});
|
||||
|
||||
@@ -106,7 +107,7 @@ describe("WorktreePool", () => {
|
||||
// First path doesn't exist, second does
|
||||
mockedExistsSync.mockImplementation((p) => p === "/tmp/good-worktree");
|
||||
|
||||
const result = pool.acquire();
|
||||
const result = pool.acquire(TEST_TASK_ID);
|
||||
expect(result).toBe("/tmp/good-worktree");
|
||||
expect(pool.size).toBe(0);
|
||||
});
|
||||
@@ -116,11 +117,30 @@ describe("WorktreePool", () => {
|
||||
pool.release("/tmp/stale-2");
|
||||
mockedExistsSync.mockReturnValue(false);
|
||||
|
||||
expect(pool.acquire()).toBeNull();
|
||||
expect(pool.acquire(TEST_TASK_ID)).toBeNull();
|
||||
expect(pool.size).toBe(0);
|
||||
});
|
||||
});
|
||||
|
||||
describe("double-lease invariant", () => {
|
||||
it("skips rehydrate entries that are already leased", () => {
|
||||
const handler = vi.fn();
|
||||
pool.setInvariantViolationHandler(handler);
|
||||
pool.release("/tmp/wt-lease");
|
||||
expect(pool.acquire(TEST_TASK_ID)).toBe("/tmp/wt-lease");
|
||||
|
||||
pool.rehydrate(["/tmp/wt-lease"]);
|
||||
|
||||
expect(pool.size).toBe(0);
|
||||
expect(pool.getLeasedPaths().get("/tmp/wt-lease")).toBe(TEST_TASK_ID);
|
||||
expect(handler).toHaveBeenCalledWith(expect.objectContaining({
|
||||
path: "/tmp/wt-lease",
|
||||
existingHolder: TEST_TASK_ID,
|
||||
phase: "rehydrate",
|
||||
}));
|
||||
});
|
||||
});
|
||||
|
||||
describe("release", () => {
|
||||
it("adds a path to the pool", () => {
|
||||
pool.release("/tmp/wt-1");
|
||||
@@ -141,9 +161,9 @@ describe("WorktreePool", () => {
|
||||
pool.release("/tmp/a");
|
||||
pool.release("/tmp/b");
|
||||
expect(pool.size).toBe(2);
|
||||
pool.acquire();
|
||||
pool.acquire(TEST_TASK_ID);
|
||||
expect(pool.size).toBe(1);
|
||||
pool.acquire();
|
||||
pool.acquire(TEST_TASK_ID);
|
||||
expect(pool.size).toBe(0);
|
||||
});
|
||||
});
|
||||
@@ -160,7 +180,7 @@ describe("WorktreePool", () => {
|
||||
|
||||
it("returns false after path is acquired", () => {
|
||||
pool.release("/tmp/wt");
|
||||
pool.acquire();
|
||||
pool.acquire(TEST_TASK_ID);
|
||||
expect(pool.has("/tmp/wt")).toBe(false);
|
||||
});
|
||||
});
|
||||
|
||||
Reference in New Issue
Block a user