feat(FN-5346): add post-completion defensive backstop and shared reconcile
The merge adds a post-completion defensive backstop that probes and removes stale same-task `activeSessionRegistry` entries on `done`/`archived` transitions, completing FN-5346 with a shared reconcile helper, a defensive ownership probe wired into paused cleanup, audit event alignment, and regressio Fusion-Task-Id: FN-5346
This commit is contained in:
committed by
gsxdsm
parent
e96cb09982
commit
f798378693
@@ -1,5 +1,8 @@
|
||||
import { beforeEach, describe, expect, it, vi } from "vitest";
|
||||
import { activeSessionRegistry } from "../active-session-registry.js";
|
||||
import {
|
||||
activeSessionRegistry,
|
||||
reconcileSelfOwnedActiveSessionForRemoval,
|
||||
} from "../active-session-registry.js";
|
||||
|
||||
describe("activeSessionRegistry", () => {
|
||||
beforeEach(() => {
|
||||
@@ -62,4 +65,48 @@ describe("activeSessionRegistry", () => {
|
||||
});
|
||||
expect(activeSessionRegistry.lookupByPath("/tmp/w1")).toBeNull();
|
||||
});
|
||||
|
||||
it("reconcileSelfOwnedActiveSessionForRemoval returns no-entry when path is unregistered", () => {
|
||||
expect(
|
||||
reconcileSelfOwnedActiveSessionForRemoval(activeSessionRegistry, "/tmp/missing", "FN-1", () => false),
|
||||
).toEqual({ action: "no-entry" });
|
||||
});
|
||||
|
||||
it("reconcileSelfOwnedActiveSessionForRemoval returns foreign-task without clearing", () => {
|
||||
activeSessionRegistry.registerPath("/tmp/w1", { taskId: "FN-2", kind: "executor", ownerKey: "FN-2" });
|
||||
|
||||
expect(
|
||||
reconcileSelfOwnedActiveSessionForRemoval(activeSessionRegistry, "/tmp/w1", "FN-1", () => false),
|
||||
).toEqual({ action: "foreign-task", ownerTaskId: "FN-2" });
|
||||
expect(activeSessionRegistry.lookupByPath("/tmp/w1")?.taskId).toBe("FN-2");
|
||||
});
|
||||
|
||||
it("reconcileSelfOwnedActiveSessionForRemoval returns live-binding-refuses without clearing", () => {
|
||||
activeSessionRegistry.registerPath("/tmp/w1", { taskId: "FN-1", kind: "executor", ownerKey: "FN-1" });
|
||||
|
||||
expect(
|
||||
reconcileSelfOwnedActiveSessionForRemoval(activeSessionRegistry, "/tmp/w1", "FN-1", () => true),
|
||||
).toEqual({ action: "live-binding-refuses", ownerTaskId: "FN-1" });
|
||||
expect(activeSessionRegistry.lookupByPath("/tmp/w1")?.taskId).toBe("FN-1");
|
||||
});
|
||||
|
||||
it("reconcileSelfOwnedActiveSessionForRemoval clears stale same-task entry", () => {
|
||||
activeSessionRegistry.registerPath("/tmp/w1", { taskId: "FN-1", kind: "executor", ownerKey: "FN-1" });
|
||||
|
||||
expect(
|
||||
reconcileSelfOwnedActiveSessionForRemoval(activeSessionRegistry, "/tmp/w1", "FN-1", () => false),
|
||||
).toEqual({ action: "reconciled" });
|
||||
expect(activeSessionRegistry.lookupByPath("/tmp/w1")).toBeNull();
|
||||
});
|
||||
|
||||
it("reconcileSelfOwnedActiveSessionForRemoval is idempotent", () => {
|
||||
activeSessionRegistry.registerPath("/tmp/w1", { taskId: "FN-1", kind: "executor", ownerKey: "FN-1" });
|
||||
|
||||
expect(
|
||||
reconcileSelfOwnedActiveSessionForRemoval(activeSessionRegistry, "/tmp/w1", "FN-1", () => false),
|
||||
).toEqual({ action: "reconciled" });
|
||||
expect(
|
||||
reconcileSelfOwnedActiveSessionForRemoval(activeSessionRegistry, "/tmp/w1", "FN-1", () => false),
|
||||
).toEqual({ action: "no-entry" });
|
||||
});
|
||||
});
|
||||
|
||||
@@ -28,7 +28,7 @@ describe("FN-4973: executor worktree conflict cleanup", () => {
|
||||
expect(activeSessionRegistry.lookupByPath(CONFLICT_PATH)).toBeNull();
|
||||
expect(store.logEntry).toHaveBeenCalledWith(
|
||||
"FN-4973",
|
||||
"Cleared stale self-owned activeSessionRegistry entry",
|
||||
"Cleared stale self-owned active-session entry before remove",
|
||||
CONFLICT_PATH,
|
||||
);
|
||||
});
|
||||
|
||||
@@ -0,0 +1,128 @@
|
||||
import { beforeEach, describe, expect, it, vi } from "vitest";
|
||||
import "../executor-test-helpers.js";
|
||||
import { TaskExecutor } from "../../executor.js";
|
||||
import { activeSessionRegistry } from "../../active-session-registry.js";
|
||||
import { ActiveSessionWorktreeRemovalError, RemovalReason } from "../../worktree-backend.js";
|
||||
import { executorLog } from "../../logger.js";
|
||||
import { WorktreePool } from "../../worktree-pool.js";
|
||||
import * as worktreePoolModule from "../../worktree-pool.js";
|
||||
import { createMockStore, mockedExistsSync, resetExecutorMocks } from "../executor-test-helpers.js";
|
||||
|
||||
const ROOT = "/tmp/test";
|
||||
const PATH = "/tmp/test/.worktrees/fn-5346";
|
||||
const TASK_ID = "FN-5346";
|
||||
|
||||
describe("FN-5346 reliability interactions: post-completion stale self-owned binding", () => {
|
||||
beforeEach(() => {
|
||||
resetExecutorMocks();
|
||||
vi.restoreAllMocks();
|
||||
activeSessionRegistry.clear();
|
||||
mockedExistsSync.mockReturnValue(true);
|
||||
});
|
||||
|
||||
it("reconciles stale same-task registry entry during cleanup()", async () => {
|
||||
const store = createMockStore();
|
||||
const executor = new TaskExecutor(store, ROOT);
|
||||
(executor as any).activeWorktrees.set(TASK_ID, PATH);
|
||||
activeSessionRegistry.registerPath(PATH, { taskId: TASK_ID, kind: "executor", ownerKey: TASK_ID });
|
||||
const removeSpy = vi.spyOn(worktreePoolModule, "removeWorktree").mockResolvedValue(undefined);
|
||||
|
||||
await executor.cleanup(TASK_ID);
|
||||
|
||||
expect(removeSpy).toHaveBeenCalledTimes(1);
|
||||
expect(activeSessionRegistry.lookupByPath(PATH)).toBeNull();
|
||||
expect(store.logEntry).toHaveBeenCalledWith(
|
||||
TASK_ID,
|
||||
"Cleared stale self-owned active-session entry before remove",
|
||||
PATH,
|
||||
);
|
||||
expect((executorLog.warn as any).mock.calls.some((call: unknown[]) => String(call[0]).includes("[FN-5346]"))).toBe(true);
|
||||
});
|
||||
|
||||
it("recovers same stale registry entry on first attempt after restart-style fresh executor", async () => {
|
||||
const store = createMockStore();
|
||||
const executor = new TaskExecutor(store, ROOT);
|
||||
activeSessionRegistry.registerPath(PATH, { taskId: TASK_ID, kind: "executor", ownerKey: TASK_ID });
|
||||
const removeSpy = vi.spyOn(worktreePoolModule, "removeWorktree").mockResolvedValue(undefined);
|
||||
|
||||
await (executor as any).handleDepAbortCleanup(TASK_ID, PATH);
|
||||
|
||||
expect(removeSpy).toHaveBeenCalledTimes(1);
|
||||
expect(activeSessionRegistry.lookupByPath(PATH)).toBeNull();
|
||||
});
|
||||
|
||||
it("preserves refusal for truly-live same-task bindings", async () => {
|
||||
const store = createMockStore();
|
||||
const executor = new TaskExecutor(store, ROOT);
|
||||
(executor as any).activeWorktrees.set(TASK_ID, PATH);
|
||||
activeSessionRegistry.registerPath(PATH, { taskId: TASK_ID, kind: "executor", ownerKey: TASK_ID });
|
||||
vi.spyOn(worktreePoolModule, "removeWorktree").mockRejectedValue(
|
||||
new ActiveSessionWorktreeRemovalError({
|
||||
worktreePath: PATH,
|
||||
taskId: TASK_ID,
|
||||
kind: "executor",
|
||||
ownerKey: TASK_ID,
|
||||
reason: RemovalReason.ExecutorDispose,
|
||||
}),
|
||||
);
|
||||
|
||||
await (executor as any).cleanupConflictingWorktree(PATH, "fusion/fn-5346", TASK_ID);
|
||||
|
||||
expect(activeSessionRegistry.lookupByPath(PATH)?.taskId).toBe(TASK_ID);
|
||||
expect(store.logEntry).not.toHaveBeenCalledWith(
|
||||
TASK_ID,
|
||||
"Cleared stale self-owned active-session entry before remove",
|
||||
PATH,
|
||||
);
|
||||
});
|
||||
|
||||
it("preserves FN-4811 refusal for foreign active owner", async () => {
|
||||
const store = createMockStore();
|
||||
store.listTasks.mockResolvedValue([]);
|
||||
const executor = new TaskExecutor(store, ROOT);
|
||||
(executor as any).activeWorktrees.set("FN-FOREIGN", PATH);
|
||||
activeSessionRegistry.registerPath(PATH, { taskId: "FN-FOREIGN", kind: "executor", ownerKey: "FN-FOREIGN" });
|
||||
const removeSpy = vi.spyOn(worktreePoolModule, "removeWorktree").mockResolvedValue(undefined);
|
||||
|
||||
const cleaned = await (executor as any).cleanupConflictingWorktree(PATH, "fusion/fn-5346", TASK_ID);
|
||||
|
||||
expect(cleaned).toBe(false);
|
||||
expect(removeSpy).not.toHaveBeenCalled();
|
||||
expect(activeSessionRegistry.lookupByPath(PATH)?.taskId).toBe("FN-FOREIGN");
|
||||
});
|
||||
|
||||
it("is idempotent across repeated cleanup sweeps", async () => {
|
||||
const store = createMockStore();
|
||||
const executor = new TaskExecutor(store, ROOT);
|
||||
(executor as any).activeWorktrees.set(TASK_ID, PATH);
|
||||
activeSessionRegistry.registerPath(PATH, { taskId: TASK_ID, kind: "executor", ownerKey: TASK_ID });
|
||||
const removeSpy = vi.spyOn(worktreePoolModule, "removeWorktree").mockResolvedValue(undefined);
|
||||
|
||||
await executor.cleanup(TASK_ID);
|
||||
(executor as any).activeWorktrees.set(TASK_ID, PATH);
|
||||
await executor.cleanup(TASK_ID);
|
||||
|
||||
const clearedCalls = (store.logEntry as any).mock.calls.filter(
|
||||
(call: unknown[]) => call[1] === "Cleared stale self-owned active-session entry before remove",
|
||||
);
|
||||
expect(clearedCalls).toHaveLength(1);
|
||||
expect(removeSpy).toHaveBeenCalledTimes(2);
|
||||
});
|
||||
|
||||
it("preserves FN-4954 pool lease bookkeeping while reconciling stale registry", async () => {
|
||||
const pool = new WorktreePool();
|
||||
pool.rehydrate([PATH]);
|
||||
expect(pool.acquire(TASK_ID)).toBe(PATH);
|
||||
const beforeLeased = new Map(pool.getLeasedPaths());
|
||||
|
||||
const store = createMockStore();
|
||||
const executor = new TaskExecutor(store, ROOT);
|
||||
(executor as any).activeWorktrees.set(TASK_ID, PATH);
|
||||
activeSessionRegistry.registerPath(PATH, { taskId: TASK_ID, kind: "executor", ownerKey: TASK_ID });
|
||||
vi.spyOn(worktreePoolModule, "removeWorktree").mockResolvedValue(undefined);
|
||||
|
||||
await executor.cleanup(TASK_ID);
|
||||
|
||||
expect(new Map(pool.getLeasedPaths())).toEqual(beforeLeased);
|
||||
});
|
||||
});
|
||||
@@ -35,7 +35,7 @@ describe("FN-4976: stale self-owned activeSessionRegistry deadlock backstop", ()
|
||||
expect(activeSessionRegistry.lookupByPath(PATH)).toBeNull();
|
||||
expect(store.logEntry).toHaveBeenCalledWith(
|
||||
TASK_ID,
|
||||
"Cleared stale self-owned activeSessionRegistry entry",
|
||||
"Cleared stale self-owned active-session entry before remove",
|
||||
PATH,
|
||||
);
|
||||
});
|
||||
@@ -53,7 +53,7 @@ describe("FN-4976: stale self-owned activeSessionRegistry deadlock backstop", ()
|
||||
expect(activeSessionRegistry.lookupByPath(PATH)?.taskId).toBe("FN-OTHER");
|
||||
const messages = store.logEntry.mock.calls.map((call: any[]) => String(call[1] ?? ""));
|
||||
expect(messages.some((m: string) => m.includes("Refused to remove conflicting worktree"))).toBe(true);
|
||||
expect(messages.some((m: string) => m.includes("Cleared stale self-owned activeSessionRegistry entry"))).toBe(false);
|
||||
expect(messages.some((m: string) => m.includes("Cleared stale self-owned active-session entry before remove"))).toBe(false);
|
||||
});
|
||||
|
||||
it("FN-4976 leaves behavior unchanged when no stale entry exists", async () => {
|
||||
@@ -70,6 +70,6 @@ describe("FN-4976: stale self-owned activeSessionRegistry deadlock backstop", ()
|
||||
expect(result).toBe(true);
|
||||
expect(unregisterSpy).not.toHaveBeenCalledWith(PATH);
|
||||
const messages = store.logEntry.mock.calls.map((call: any[]) => String(call[1] ?? ""));
|
||||
expect(messages.some((m: string) => m.includes("Cleared stale self-owned activeSessionRegistry entry"))).toBe(false);
|
||||
expect(messages.some((m: string) => m.includes("Cleared stale self-owned active-session entry before remove"))).toBe(false);
|
||||
});
|
||||
});
|
||||
|
||||
@@ -1,5 +1,6 @@
|
||||
import { describe, it, expect, vi, beforeEach } from "vitest";
|
||||
import {
|
||||
ActiveSessionWorktreeRemovalError,
|
||||
NativeWorktreeBackend,
|
||||
WorktrunkOperationError,
|
||||
WorktrunkWorktreeBackend,
|
||||
@@ -7,6 +8,7 @@ import {
|
||||
resolveWorktreeBackend,
|
||||
RemovalReason,
|
||||
} from "../worktree-backend.js";
|
||||
import { activeSessionRegistry } from "../active-session-registry.js";
|
||||
|
||||
const { execMock, accessMock, existsSyncMock, parseIndexLockPathMock, classifyStaleLockMock, tryRemoveStaleLockMock, parseStaleRegistrationPathMock, recoverStaleRegistrationMock, installGuardMock } = vi.hoisted(() => {
|
||||
const mock = vi.fn();
|
||||
@@ -73,6 +75,7 @@ beforeEach(() => {
|
||||
recoverStaleRegistrationMock.mockResolvedValue({ recovered: true, actions: ["prune"] });
|
||||
classifyStaleLockMock.mockResolvedValue({ kind: "fresh", reason: "fresh" });
|
||||
tryRemoveStaleLockMock.mockResolvedValue({ removed: true });
|
||||
activeSessionRegistry.clear();
|
||||
});
|
||||
|
||||
describe("NativeWorktreeBackend", () => {
|
||||
@@ -852,6 +855,93 @@ describe("removeWorktree", () => {
|
||||
}),
|
||||
).rejects.toMatchObject({ code: "worktrunk_binary_missing", operation: "remove" });
|
||||
});
|
||||
|
||||
it("reconciles same-task stale active session when defensive owner probe says not live", async () => {
|
||||
execMock.mockResolvedValue({ stdout: "", stderr: "" });
|
||||
const audit = { git: vi.fn().mockResolvedValue(undefined) } as any;
|
||||
activeSessionRegistry.registerPath("/repo/.worktrees/fn-1", {
|
||||
taskId: "FN-1",
|
||||
kind: "executor",
|
||||
ownerKey: "FN-1/executor",
|
||||
});
|
||||
|
||||
await removeWorktree({
|
||||
rootDir: "/repo",
|
||||
worktreePath: "/repo/.worktrees/fn-1",
|
||||
settings: {},
|
||||
audit,
|
||||
reason: RemovalReason.ExecutorDispose,
|
||||
expectedOwnerTaskId: "FN-1",
|
||||
liveOwnerProbe: () => false,
|
||||
});
|
||||
|
||||
expect(activeSessionRegistry.lookupByPath("/repo/.worktrees/fn-1")).toBeNull();
|
||||
expect(audit.git).toHaveBeenCalledWith(
|
||||
expect.objectContaining({
|
||||
type: "worktree:active-session-reconciled",
|
||||
target: "/repo/.worktrees/fn-1",
|
||||
metadata: { taskId: "FN-1", source: "removeWorktree-defensive" },
|
||||
}),
|
||||
);
|
||||
});
|
||||
|
||||
it("preserves refusal when same-task owner is still live", async () => {
|
||||
activeSessionRegistry.registerPath("/repo/.worktrees/fn-1", {
|
||||
taskId: "FN-1",
|
||||
kind: "executor",
|
||||
ownerKey: "FN-1/executor",
|
||||
});
|
||||
|
||||
await expect(
|
||||
removeWorktree({
|
||||
rootDir: "/repo",
|
||||
worktreePath: "/repo/.worktrees/fn-1",
|
||||
settings: {},
|
||||
reason: RemovalReason.ExecutorDispose,
|
||||
expectedOwnerTaskId: "FN-1",
|
||||
liveOwnerProbe: () => true,
|
||||
}),
|
||||
).rejects.toBeInstanceOf(ActiveSessionWorktreeRemovalError);
|
||||
});
|
||||
|
||||
it("preserves foreign-owner refusal with defensive owner hints", async () => {
|
||||
activeSessionRegistry.registerPath("/repo/.worktrees/fn-1", {
|
||||
taskId: "FN-2",
|
||||
kind: "executor",
|
||||
ownerKey: "FN-2/executor",
|
||||
});
|
||||
|
||||
await expect(
|
||||
removeWorktree({
|
||||
rootDir: "/repo",
|
||||
worktreePath: "/repo/.worktrees/fn-1",
|
||||
settings: {},
|
||||
reason: RemovalReason.ExecutorDispose,
|
||||
expectedOwnerTaskId: "FN-1",
|
||||
liveOwnerProbe: () => false,
|
||||
}),
|
||||
).rejects.toMatchObject({
|
||||
name: "ActiveSessionWorktreeRemovalError",
|
||||
details: expect.objectContaining({ taskId: "FN-2" }),
|
||||
});
|
||||
});
|
||||
|
||||
it("keeps pre-FN-5346 behavior when defensive owner hints are omitted", async () => {
|
||||
activeSessionRegistry.registerPath("/repo/.worktrees/fn-1", {
|
||||
taskId: "FN-1",
|
||||
kind: "executor",
|
||||
ownerKey: "FN-1/executor",
|
||||
});
|
||||
|
||||
await expect(
|
||||
removeWorktree({
|
||||
rootDir: "/repo",
|
||||
worktreePath: "/repo/.worktrees/fn-1",
|
||||
settings: {},
|
||||
reason: RemovalReason.ExecutorDispose,
|
||||
}),
|
||||
).rejects.toBeInstanceOf(ActiveSessionWorktreeRemovalError);
|
||||
});
|
||||
});
|
||||
|
||||
describe("resolveWorktreeBackend", () => {
|
||||
|
||||
Reference in New Issue
Block a user