diff --git a/packages/engine/src/__tests__/executor-test-helpers.ts b/packages/engine/src/__tests__/executor-test-helpers.ts index 46ee963f0..410ab7b91 100644 --- a/packages/engine/src/__tests__/executor-test-helpers.ts +++ b/packages/engine/src/__tests__/executor-test-helpers.ts @@ -109,8 +109,12 @@ vi.mock("../worktree-names.js", async () => { }); vi.mock("../worktree-pool.js", async (importOriginal) => { const actual = await importOriginal(); + const backend = await vi.importActual("../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), diff --git a/packages/engine/src/__tests__/executor-worktree-conflict.test.ts b/packages/engine/src/__tests__/executor-worktree-conflict.test.ts new file mode 100644 index 000000000..cc62323fe --- /dev/null +++ b/packages/engine/src/__tests__/executor-worktree-conflict.test.ts @@ -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(); + }); +}); diff --git a/packages/engine/src/__tests__/reliability-interactions/stale-self-owned-active-session-recovery.test.ts b/packages/engine/src/__tests__/reliability-interactions/stale-self-owned-active-session-recovery.test.ts new file mode 100644 index 000000000..4d17abaae --- /dev/null +++ b/packages/engine/src/__tests__/reliability-interactions/stale-self-owned-active-session-recovery.test.ts @@ -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) => 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); + }); +}); diff --git a/packages/engine/src/executor.ts b/packages/engine/src/executor.ts index e569442a6..9b17c7894 100644 --- a/packages/engine/src/executor.ts +++ b/packages/engine/src/executor.ts @@ -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