feat(FN-4973): reconcile stale active-session worktree entries in executor cleanup
Fusion-Task-Id: FN-4973 Fusion-Task-Lineage: 2edb5479-a01a-4775-b121-f4be32792e7a
This commit is contained in:
committed by
gsxdsm
parent
3a53c6778c
commit
6aea112a22
@@ -109,8 +109,12 @@ vi.mock("../worktree-names.js", async () => {
|
||||
});
|
||||
vi.mock("../worktree-pool.js", async (importOriginal) => {
|
||||
const actual = await importOriginal<typeof import("../worktree-pool.js")>();
|
||||
const backend = await vi.importActual<typeof import("../worktree-backend.js")>("../worktree-backend.js");
|
||||
return {
|
||||
...actual,
|
||||
ActiveSessionWorktreeRemovalError: backend.ActiveSessionWorktreeRemovalError,
|
||||
RemovalReason: backend.RemovalReason,
|
||||
removeWorktree: vi.fn(actual.removeWorktree),
|
||||
classifyTaskWorktree: vi.fn().mockResolvedValue({ ok: true }),
|
||||
describeRegisteredWorktrees: vi.fn().mockResolvedValue({ rawOutput: "", canonicalized: [] }),
|
||||
isUsableTaskWorktree: vi.fn().mockResolvedValue(true),
|
||||
|
||||
104
packages/engine/src/__tests__/executor-worktree-conflict.test.ts
Normal file
104
packages/engine/src/__tests__/executor-worktree-conflict.test.ts
Normal file
@@ -0,0 +1,104 @@
|
||||
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 } from "../worktree-backend.js";
|
||||
import * as worktreePoolModule from "../worktree-pool.js";
|
||||
import { createMockStore, resetExecutorMocks } from "./executor-test-helpers.js";
|
||||
|
||||
const CONFLICT_PATH = "/tmp/test/.worktrees/stale-self-owned";
|
||||
|
||||
describe("FN-4973: executor worktree conflict cleanup", () => {
|
||||
beforeEach(() => {
|
||||
resetExecutorMocks();
|
||||
activeSessionRegistry.clear();
|
||||
});
|
||||
|
||||
it("reconciles stale self-owned registry entry before removal", async () => {
|
||||
const store = createMockStore();
|
||||
const executor = new TaskExecutor(store, "/tmp/test");
|
||||
store.listTasks.mockResolvedValue([]);
|
||||
activeSessionRegistry.registerPath(CONFLICT_PATH, { taskId: "FN-4973", kind: "executor", ownerKey: "FN-4973" });
|
||||
|
||||
const removeSpy = vi.spyOn(worktreePoolModule, "removeWorktree").mockResolvedValue(undefined);
|
||||
const result = await (executor as any).cleanupConflictingWorktree(CONFLICT_PATH, "fusion/fn-4973", "FN-4973");
|
||||
|
||||
expect(result).toBe(true);
|
||||
expect(removeSpy).toHaveBeenCalled();
|
||||
expect(activeSessionRegistry.lookupByPath(CONFLICT_PATH)).toBeNull();
|
||||
expect(store.logEntry).toHaveBeenCalledWith(
|
||||
"FN-4973",
|
||||
"Reconciled stale self-owned active-session registration",
|
||||
CONFLICT_PATH,
|
||||
);
|
||||
});
|
||||
|
||||
it("does not reconcile when same-task in-memory binding is live and refuses removal", async () => {
|
||||
const store = createMockStore();
|
||||
const executor = new TaskExecutor(store, "/tmp/test");
|
||||
store.listTasks.mockResolvedValue([]);
|
||||
(executor as any).activeWorktrees.set("FN-4973", CONFLICT_PATH);
|
||||
activeSessionRegistry.registerPath(CONFLICT_PATH, { taskId: "FN-4973", kind: "executor", ownerKey: "FN-4973" });
|
||||
|
||||
vi.spyOn(worktreePoolModule, "removeWorktree").mockRejectedValue(
|
||||
new ActiveSessionWorktreeRemovalError({
|
||||
worktreePath: CONFLICT_PATH,
|
||||
taskId: "FN-4973",
|
||||
kind: "executor",
|
||||
ownerKey: "FN-4973",
|
||||
reason: worktreePoolModule.RemovalReason.ExecutorDispose,
|
||||
}),
|
||||
);
|
||||
|
||||
const result = await (executor as any).cleanupConflictingWorktree(CONFLICT_PATH, "fusion/fn-4973", "FN-4973");
|
||||
expect(result).toBe(false);
|
||||
expect(activeSessionRegistry.lookupByPath(CONFLICT_PATH)?.taskId).toBe("FN-4973");
|
||||
});
|
||||
|
||||
it("does not reconcile foreign-task registry entries and keeps refusal behavior", async () => {
|
||||
const store = createMockStore();
|
||||
const executor = new TaskExecutor(store, "/tmp/test");
|
||||
store.listTasks.mockResolvedValue([]);
|
||||
activeSessionRegistry.registerPath(CONFLICT_PATH, { taskId: "FN-OTHER", kind: "executor", ownerKey: "FN-OTHER" });
|
||||
|
||||
vi.spyOn(worktreePoolModule, "removeWorktree").mockRejectedValue(
|
||||
new ActiveSessionWorktreeRemovalError({
|
||||
worktreePath: CONFLICT_PATH,
|
||||
taskId: "FN-OTHER",
|
||||
kind: "executor",
|
||||
ownerKey: "FN-OTHER",
|
||||
reason: worktreePoolModule.RemovalReason.ExecutorDispose,
|
||||
}),
|
||||
);
|
||||
|
||||
const result = await (executor as any).cleanupConflictingWorktree(CONFLICT_PATH, "fusion/fn-4973", "FN-4973");
|
||||
expect(result).toBe(false);
|
||||
expect(activeSessionRegistry.lookupByPath(CONFLICT_PATH)?.taskId).toBe("FN-OTHER");
|
||||
});
|
||||
|
||||
it("reconciles once on race-window ActiveSessionWorktreeRemovalError then retries removal", async () => {
|
||||
const store = createMockStore();
|
||||
const executor = new TaskExecutor(store, "/tmp/test");
|
||||
store.listTasks.mockResolvedValue([]);
|
||||
|
||||
const removeSpy = vi.spyOn(worktreePoolModule, "removeWorktree");
|
||||
removeSpy
|
||||
.mockImplementationOnce(async () => {
|
||||
activeSessionRegistry.registerPath(CONFLICT_PATH, { taskId: "FN-4973", kind: "executor", ownerKey: "FN-4973" });
|
||||
throw new ActiveSessionWorktreeRemovalError({
|
||||
worktreePath: CONFLICT_PATH,
|
||||
taskId: "FN-4973",
|
||||
kind: "executor",
|
||||
ownerKey: "FN-4973",
|
||||
reason: worktreePoolModule.RemovalReason.ExecutorDispose,
|
||||
});
|
||||
})
|
||||
.mockResolvedValueOnce(undefined);
|
||||
|
||||
const result = await (executor as any).cleanupConflictingWorktree(CONFLICT_PATH, "fusion/fn-4973", "FN-4973");
|
||||
|
||||
expect(result).toBe(true);
|
||||
expect(removeSpy).toHaveBeenCalledTimes(2);
|
||||
expect(activeSessionRegistry.lookupByPath(CONFLICT_PATH)).toBeNull();
|
||||
});
|
||||
});
|
||||
@@ -0,0 +1,136 @@
|
||||
import { EventEmitter } from "node:events";
|
||||
import { beforeEach, describe, expect, it, vi } from "vitest";
|
||||
import "../executor-test-helpers.js";
|
||||
import type { Settings, Task, TaskStore } from "@fusion/core";
|
||||
import { TaskExecutor } from "../../executor.js";
|
||||
import { SelfHealingManager } from "../../self-healing.js";
|
||||
import { activeSessionRegistry } from "../../active-session-registry.js";
|
||||
import { ActiveSessionWorktreeRemovalError, RemovalReason } from "../../worktree-backend.js";
|
||||
import * as worktreePoolModule from "../../worktree-pool.js";
|
||||
import { createMockStore, resetExecutorMocks } from "../executor-test-helpers.js";
|
||||
|
||||
const TASK_ID = "FN-4973";
|
||||
const CONFLICT_PATH = "/tmp/test/.worktrees/solar-flame";
|
||||
|
||||
function makeStore(task: Task): TaskStore & EventEmitter {
|
||||
const emitter = new EventEmitter();
|
||||
const settings = {
|
||||
autoMerge: true,
|
||||
globalPause: false,
|
||||
enginePaused: false,
|
||||
baseBranch: "main",
|
||||
mergeStrategy: "direct",
|
||||
autoRecovery: { mode: "deterministic-only", maxRetries: 3 },
|
||||
} as unknown as Settings;
|
||||
return Object.assign(emitter, {
|
||||
getSettings: vi.fn(async () => settings),
|
||||
getTask: vi.fn(async () => task),
|
||||
listTasks: vi.fn(async ({ column }: { column?: string } = {}) => (column === "in-progress" ? [task] : [task])),
|
||||
updateTask: vi.fn(async (_id: string, updates: Partial<Task>) => Object.assign(task, updates)),
|
||||
moveTask: vi.fn(async (_id: string, column: Task["column"]) => {
|
||||
task.column = column;
|
||||
return task;
|
||||
}),
|
||||
logEntry: vi.fn(async () => undefined),
|
||||
appendAgentLog: vi.fn(async () => undefined),
|
||||
updateSettings: vi.fn(async () => settings),
|
||||
clearStaleExecutionStartBranchReferences: vi.fn(() => []),
|
||||
recordRunAuditEvent: vi.fn(async () => undefined),
|
||||
walCheckpoint: vi.fn(() => ({ busy: 0, log: 0, checkpointed: 0 })),
|
||||
archiveTaskAndCleanup: vi.fn(async () => ({})),
|
||||
mergeTask: vi.fn(async () => undefined),
|
||||
getRootDir: vi.fn(() => "/tmp/test"),
|
||||
}) as unknown as TaskStore & EventEmitter;
|
||||
}
|
||||
|
||||
function makeTask(): Task {
|
||||
return {
|
||||
id: TASK_ID,
|
||||
title: "test",
|
||||
description: "test",
|
||||
column: "in-progress",
|
||||
branch: "fusion/fn-4973",
|
||||
worktree: CONFLICT_PATH,
|
||||
paused: false,
|
||||
userPaused: false,
|
||||
dependencies: [],
|
||||
steps: [],
|
||||
currentStep: 0,
|
||||
log: [],
|
||||
createdAt: new Date().toISOString(),
|
||||
updatedAt: new Date().toISOString(),
|
||||
} as unknown as Task;
|
||||
}
|
||||
|
||||
describe("FN-4973 reliability interactions: stale self-owned active-session recovery", () => {
|
||||
beforeEach(() => {
|
||||
resetExecutorMocks();
|
||||
activeSessionRegistry.clear();
|
||||
vi.restoreAllMocks();
|
||||
});
|
||||
|
||||
it("FN-4973 reconciles stale self-owned entry and remains clear across self-healing sweeps", async () => {
|
||||
const store = createMockStore();
|
||||
store.listTasks.mockResolvedValue([]);
|
||||
const executor = new TaskExecutor(store, "/tmp/test");
|
||||
activeSessionRegistry.registerPath(CONFLICT_PATH, { taskId: TASK_ID, kind: "executor", ownerKey: TASK_ID });
|
||||
|
||||
vi.spyOn(worktreePoolModule, "removeWorktree").mockResolvedValue(undefined);
|
||||
|
||||
const cleaned = await (executor as any).cleanupConflictingWorktree(CONFLICT_PATH, "fusion/fn-4973", TASK_ID);
|
||||
expect(cleaned).toBe(true);
|
||||
expect(activeSessionRegistry.lookupByPath(CONFLICT_PATH)).toBeNull();
|
||||
|
||||
const task = makeTask();
|
||||
const healingStore = makeStore(task);
|
||||
const manager = new SelfHealingManager(healingStore as any, { rootDir: "/tmp/test" } as any);
|
||||
await manager.reconcileTaskWorktreeMetadata();
|
||||
await manager.reclaimStaleActiveBranches();
|
||||
|
||||
expect(activeSessionRegistry.lookupByPath(CONFLICT_PATH)).toBeNull();
|
||||
manager.stop();
|
||||
});
|
||||
|
||||
it("FN-4973 preserves FN-4811 foreign-task ownership refusal", async () => {
|
||||
const store = createMockStore();
|
||||
store.listTasks.mockResolvedValue([]);
|
||||
const executor = new TaskExecutor(store, "/tmp/test");
|
||||
activeSessionRegistry.registerPath(CONFLICT_PATH, { taskId: "FN-FOREIGN", kind: "executor", ownerKey: "FN-FOREIGN" });
|
||||
|
||||
vi.spyOn(worktreePoolModule, "removeWorktree").mockRejectedValue(
|
||||
new ActiveSessionWorktreeRemovalError({
|
||||
worktreePath: CONFLICT_PATH,
|
||||
taskId: "FN-FOREIGN",
|
||||
kind: "executor",
|
||||
ownerKey: "FN-FOREIGN",
|
||||
reason: RemovalReason.ExecutorDispose,
|
||||
}),
|
||||
);
|
||||
|
||||
const cleaned = await (executor as any).cleanupConflictingWorktree(CONFLICT_PATH, "fusion/fn-4973", TASK_ID);
|
||||
expect(cleaned).toBe(false);
|
||||
expect(activeSessionRegistry.lookupByPath(CONFLICT_PATH)?.taskId).toBe("FN-FOREIGN");
|
||||
});
|
||||
|
||||
it("FN-4973 refuses same-task live in-memory binding", async () => {
|
||||
const store = createMockStore();
|
||||
store.listTasks.mockResolvedValue([]);
|
||||
const executor = new TaskExecutor(store, "/tmp/test");
|
||||
(executor as any).activeWorktrees.set(TASK_ID, CONFLICT_PATH);
|
||||
activeSessionRegistry.registerPath(CONFLICT_PATH, { taskId: TASK_ID, kind: "executor", ownerKey: TASK_ID });
|
||||
|
||||
vi.spyOn(worktreePoolModule, "removeWorktree").mockRejectedValue(
|
||||
new ActiveSessionWorktreeRemovalError({
|
||||
worktreePath: CONFLICT_PATH,
|
||||
taskId: TASK_ID,
|
||||
kind: "executor",
|
||||
ownerKey: TASK_ID,
|
||||
reason: RemovalReason.ExecutorDispose,
|
||||
}),
|
||||
);
|
||||
|
||||
const cleaned = await (executor as any).cleanupConflictingWorktree(CONFLICT_PATH, "fusion/fn-4973", TASK_ID);
|
||||
expect(cleaned).toBe(false);
|
||||
expect(activeSessionRegistry.lookupByPath(CONFLICT_PATH)?.taskId).toBe(TASK_ID);
|
||||
});
|
||||
});
|
||||
@@ -43,7 +43,7 @@ import { resolveSandboxBackend } from "./sandbox/index.js";
|
||||
import type { SandboxBackend } from "./sandbox/types.js";
|
||||
import { ModelRegistry, SessionManager, type ToolDefinition, type AgentSession } from "@mariozechner/pi-coding-agent";
|
||||
import { PRIORITY_EXECUTE, type AgentSemaphore } from "./concurrency.js";
|
||||
import { RemovalReason, classifyTaskWorktree, describeRegisteredWorktrees, getRegisteredWorktreePaths, isGitRepository, isInsideWorktreesDir, isRegisteredGitWorktree, removeWorktree, type WorktreePool } from "./worktree-pool.js";
|
||||
import { ActiveSessionWorktreeRemovalError, RemovalReason, classifyTaskWorktree, describeRegisteredWorktrees, getRegisteredWorktreePaths, isGitRepository, isInsideWorktreesDir, isRegisteredGitWorktree, removeWorktree, type WorktreePool } from "./worktree-pool.js";
|
||||
import { activeSessionRegistry, executingTaskLock } from "./active-session-registry.js";
|
||||
import {
|
||||
StaleWorktreeIndexLockError,
|
||||
@@ -8865,6 +8865,15 @@ Backward compat fallback: if JSON is unavailable, you may still begin output wit
|
||||
* Handles locked worktrees by unlocking first.
|
||||
* Returns true if cleanup succeeded.
|
||||
*/
|
||||
private hasActiveWorktreeBinding(taskId: string, worktreePath: string): boolean {
|
||||
for (const [activeTaskId, activePath] of this.activeWorktrees) {
|
||||
if (activeTaskId === taskId && activePath === worktreePath) {
|
||||
return true;
|
||||
}
|
||||
}
|
||||
return false;
|
||||
}
|
||||
|
||||
private async cleanupConflictingWorktree(
|
||||
worktreePath: string,
|
||||
branch: string,
|
||||
@@ -8895,15 +8904,47 @@ Backward compat fallback: if JSON is unavailable, you may still begin output wit
|
||||
executorLog.warn(`${taskId}: failed to unlock conflicting worktree ${worktreePath} before cleanup: ${msg}`);
|
||||
}
|
||||
|
||||
// Remove stale self-owned active-session entry if no live in-memory binding exists.
|
||||
const activeSessionRecord = activeSessionRegistry.lookupByPath(worktreePath);
|
||||
if (activeSessionRecord?.taskId === taskId && !this.hasActiveWorktreeBinding(taskId, worktreePath)) {
|
||||
const reconcileResult = activeSessionRegistry.reconcileStaleSelfOwned(worktreePath, taskId);
|
||||
if (reconcileResult.reconciled) {
|
||||
await this.store.logEntry(taskId, "Reconciled stale self-owned active-session registration", worktreePath);
|
||||
executorLog.log(
|
||||
`[executor] reconciled stale self-owned active-session entry taskId=${taskId} worktreePath=${worktreePath} reason=${reconcileResult.reason}`,
|
||||
);
|
||||
}
|
||||
}
|
||||
|
||||
// Remove the worktree
|
||||
const settings = await this.store.getSettings();
|
||||
await removeWorktree({
|
||||
const removeArgs = {
|
||||
worktreePath,
|
||||
rootDir: this.rootDir,
|
||||
settings,
|
||||
taskId,
|
||||
reason: RemovalReason.ExecutorDispose,
|
||||
});
|
||||
} as const;
|
||||
try {
|
||||
await removeWorktree(removeArgs);
|
||||
} catch (error: unknown) {
|
||||
if (
|
||||
error instanceof ActiveSessionWorktreeRemovalError
|
||||
&& error.details.taskId === taskId
|
||||
&& !this.hasActiveWorktreeBinding(taskId, worktreePath)
|
||||
) {
|
||||
const reconcileResult = activeSessionRegistry.reconcileStaleSelfOwned(worktreePath, taskId);
|
||||
if (reconcileResult.reconciled) {
|
||||
await this.store.logEntry(taskId, "Reconciled stale self-owned active-session registration", worktreePath);
|
||||
executorLog.log(
|
||||
`[executor] reconciled stale self-owned active-session entry taskId=${taskId} worktreePath=${worktreePath} reason=${reconcileResult.reason}`,
|
||||
);
|
||||
}
|
||||
await removeWorktree(removeArgs);
|
||||
} else {
|
||||
throw error;
|
||||
}
|
||||
}
|
||||
await this.store.logEntry(taskId, `Removed conflicting worktree`, worktreePath);
|
||||
|
||||
// Delete the branch if it exists
|
||||
|
||||
Reference in New Issue
Block a user