root/fusion
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

fix(FN-4811): refuse to force-remove worktrees actively bound to live sessions

Browse Source 
The executor's conflict-recovery paths (cleanupConflictingWorktree,
handleBranchConflict, and tryCreateWorktree's live-foreign/stale-resolved
branches) could force-remove a worktree even when it was currently bound
to an active executor session. This caused the FN-4781/FN-4804 cascade:

  - 'Execution blocked: assigned worktree path disappeared mid-task' as
    git deleted the live agent's filesystem out from under it
  - Two parallel runs for the same task alive simultaneously, with the
    second run started in a fresh worktree while the first was still
    holding the old session
  - Cross-task log attribution (an FN-4804 runContext writing to FN-4781)
  - Post-merge 'branch tip misbound but content found on main via trailer'
    rescues firing on every successful merge as the bookkeeping was
    corrupted mid-merge

Adds a hard liveness gate centralized in findActiveWorktreeOwner(), which
checks both the in-memory activeWorktrees map and the DB for non-done,
non-paused, in-progress tasks bound to the worktree. The gate fires at
two points:

  1. cleanupConflictingWorktree returns false (refuses removal) when an
     active owner is found, logging an FN-4811 refusal entry.
  2. handleBranchConflict short-circuits to 'sticky' BEFORE invoking
     inspectBranchConflict, because some inspection branches force-remove
     unconditionally.

When cleanup is refused, the live-foreign and stale-resolved branches in
tryCreateWorktree now FALL THROUGH to the suffix-rename path (rather
than returning null) so the requesting task can still proceed without
disturbing the live owner.

Tests:

  - New reliability-interactions backstop at
    src/__tests__/reliability-interactions/active-worktree-removal-liveness.test.ts
    covers findActiveWorktreeOwner (5 cases: in-memory match, requesting
    task excluded, DB-level match, paused exclusion, terminal-column
    exclusion, self-exclusion), cleanupConflictingWorktree gate (3 cases:
    in-memory refuse, DB refuse, no-owner proceed), and handleBranchConflict
    gate (2 cases: short-circuit + inspection-skipped, no-owner proceeds).
  - Updates existing executor-worktree.test.ts assertion that was
    documenting the bug behavior (force-removing active worktree) to match
    the new contract (refuses + falls through to suffix-rename).

Full engine suite: 307 files, 5035 tests pass.

Fusion-Task-Id: FN-4811
This commit is contained in:
Fusion
2026-05-16 15:36:07 -07:00
parent 3f8a5e6839
commit 4c26aa6e91
4 changed files with 348 additions and 11 deletions
Download Patch File Download Diff File Expand all files Collapse all files

34
packages/engine/src/__tests__/executor-worktree.test.ts
View File

@@ -1148,7 +1148,12 @@ describe("TaskExecutor worktree recovery", () => {
);
});
it("generates new worktree name when conflicting worktree belongs to active task in legacy rename mode", async () => {
it("generates new worktree name when conflicting worktree belongs to active task in legacy rename mode (FN-4811: refuses force-removal of active worktree)", async () => {
// FN-4811: When the conflicting worktree is bound to a live in-progress task, the
// executor MUST NOT force-remove it (doing so yanks the active session's filesystem
// and produces FN-4781/FN-4804-style cascade failures). Instead, with sibling-rename
// enabled, it falls through to the suffix-rename path so the requesting task gets a
// fresh worktree name without disturbing the live owner.
const store = createMockStore();
store.getSettings.mockResolvedValue({
maxConcurrent: 2,
@@ -1165,6 +1170,7 @@ describe("TaskExecutor worktree recovery", () => {
description: "Other task",
column: "in-progress",
worktree: "/tmp/test/.worktrees/green-sage",
paused: false,
dependencies: [],
steps: [],
currentStep: 0,
@@ -1180,7 +1186,7 @@ describe("TaskExecutor worktree recovery", () => {
let callCount = 0;
mockedExecSync.mockImplementation((cmd: string | string[]) => {
const command = typeof cmd === "string" ? cmd : cmd[0];
// First attempt fails with conflict
// First attempt fails with conflict, subsequent attempts (suffix-rename path) succeed.
if (command.includes("git worktree add") && callCount++ === 0) {
const error: any = new Error(
"fatal: 'fusion/fn-050' is already used by worktree at '/tmp/test/.worktrees/green-sage'",
@@ -1193,19 +1199,29 @@ describe("TaskExecutor worktree recovery", () => {
return Buffer.from("");
});
// Second generated name
// Second generated name for the suffix-rename path.
mockedGenerateWorktreeName.mockReturnValueOnce("jade-finch");
const executor = new TaskExecutor(store, "/tmp/test");
await executor.execute({ ...makeTask(), executionStartBranch: "fusion/fn-049" });
expect(store.logEntry).toHaveBeenCalledWith(
"FN-050",
expect.stringContaining("Removed foreign conflicting worktree and retrying"),
"/tmp/test/.worktrees/green-sage",
);
expect(mockedGenerateWorktreeName).toHaveBeenCalled();
// FN-4811 contract: the active worktree must NOT have been force-removed.
const removeCalls = mockedExecSync.mock.calls
.map((call) => String(call[0]))
.filter((command) => command.includes("git worktree remove"));
expect(
removeCalls.some((command) => command.includes("/tmp/test/.worktrees/green-sage")),
).toBe(false);
// The legacy "Removed foreign conflicting worktree and retrying" log must NOT fire
// for the actively-owned worktree (that path is the bug FN-4811 fixes).
const removalLogCalls = store.logEntry.mock.calls.map((c: any[]) => String(c[1] ?? ""));
expect(
removalLogCalls.some((m: string) => m.includes("Removed foreign conflicting worktree")),
).toBe(false);
// The suffix-rename path was taken instead.
expect(mockedGenerateWorktreeName).toHaveBeenCalled();
const worktreeAddCalls = mockedExecSync.mock.calls
.map((call) => String(call[0]))
.filter((command) => command.includes("git worktree add -b"));

229
packages/engine/src/__tests__/reliability-interactions/active-worktree-removal-liveness.test.ts Normal file
View File

@@ -0,0 +1,229 @@
/**
* FN-4811 reliability backstop: `cleanupConflictingWorktree` and `handleBranchConflict`
* must refuse to force-remove a worktree that is currently bound to an active executor
* session (in-memory map or DB-level in-progress task). Without this guard, FN-4546
* stale-active-branch reclaim, branch-conflict recovery, or startup cleanup paths can
* yank the filesystem out from under a live agent — producing the FN-4781/FN-4804
* cascade: "assigned worktree path disappeared mid-task", two parallel runs alive
* simultaneously, and cross-task contamination.
*
* The canonical guards live in `executor.ts`:
* - `findActiveWorktreeOwner(path, requestingTaskId)`
* - liveness short-circuit at top of `cleanupConflictingWorktree`
* - liveness short-circuit at top of `handleBranchConflict`
*/
import { describe, it, expect, vi, beforeEach } from "vitest";
import "../executor-test-helpers.js";
import { TaskExecutor } from "../../executor.js";
import { BranchConflictError } from "../../branch-conflicts.js";
import * as branchConflictModule from "../../branch-conflicts.js";
import { createMockStore, resetExecutorMocks } from "../executor-test-helpers.js";
const ACTIVE_PATH = "/tmp/test/.worktrees/lemon-reef";
const STALE_PATH = "/tmp/test/.worktrees/azure-peach";
function makeTask(overrides: Record<string, unknown> = {}) {
return {
id: "FN-4811",
title: "Test",
description: "Test task",
column: "in-progress",
dependencies: [],
steps: [],
currentStep: 0,
log: [],
createdAt: new Date().toISOString(),
updatedAt: new Date().toISOString(),
...overrides,
} as any;
}
function makeConflict(path: string): BranchConflictError {
return new BranchConflictError({
branchName: "fusion/fn-9999",
conflictingWorktreePath: path,
existingTipSha: "abc123def456",
strandedCommits: [],
startPoint: "HEAD",
recommendedAction: "test",
});
}
describe("FN-4811: active worktree removal liveness gate", () => {
beforeEach(() => {
resetExecutorMocks();
});
describe("findActiveWorktreeOwner", () => {
it("returns the owner taskId when activeWorktrees has another task using the path", async () => {
const store = createMockStore();
const executor = new TaskExecutor(store, "/tmp/test");
(executor as any).activeWorktrees.set("FN-OTHER", ACTIVE_PATH);
const owner = await (executor as any).findActiveWorktreeOwner(ACTIVE_PATH, "FN-4811");
expect(owner).toBe("FN-OTHER");
});
it("returns null when activeWorktrees only has the requesting task at the path", async () => {
const store = createMockStore();
const executor = new TaskExecutor(store, "/tmp/test");
(executor as any).activeWorktrees.set("FN-4811", ACTIVE_PATH);
store.listTasks.mockResolvedValue([]);
const owner = await (executor as any).findActiveWorktreeOwner(ACTIVE_PATH, "FN-4811");
expect(owner).toBeNull();
});
it("returns owner from DB when a non-done, non-paused, in-progress task uses the path", async () => {
const store = createMockStore();
const executor = new TaskExecutor(store, "/tmp/test");
store.listTasks.mockResolvedValue([
{ id: "FN-OWNER", worktree: ACTIVE_PATH, column: "in-progress", paused: false },
]);
const owner = await (executor as any).findActiveWorktreeOwner(ACTIVE_PATH, "FN-4811");
expect(owner).toBe("FN-OWNER");
});
it("ignores paused in-progress tasks (engine has released the session)", async () => {
const store = createMockStore();
const executor = new TaskExecutor(store, "/tmp/test");
store.listTasks.mockResolvedValue([
{ id: "FN-PAUSED", worktree: ACTIVE_PATH, column: "in-progress", paused: true },
]);
const owner = await (executor as any).findActiveWorktreeOwner(ACTIVE_PATH, "FN-4811");
expect(owner).toBeNull();
});
it("ignores done/in-review/todo tasks (no live session)", async () => {
const store = createMockStore();
const executor = new TaskExecutor(store, "/tmp/test");
store.listTasks.mockResolvedValue([
{ id: "FN-DONE", worktree: ACTIVE_PATH, column: "done", paused: false },
{ id: "FN-REVIEW", worktree: ACTIVE_PATH, column: "in-review", paused: false },
{ id: "FN-TODO", worktree: ACTIVE_PATH, column: "todo", paused: false },
]);
const owner = await (executor as any).findActiveWorktreeOwner(ACTIVE_PATH, "FN-4811");
expect(owner).toBeNull();
});
it("excludes the requesting task from DB liveness check", async () => {
const store = createMockStore();
const executor = new TaskExecutor(store, "/tmp/test");
store.listTasks.mockResolvedValue([
{ id: "FN-4811", worktree: ACTIVE_PATH, column: "in-progress", paused: false },
]);
const owner = await (executor as any).findActiveWorktreeOwner(ACTIVE_PATH, "FN-4811");
expect(owner).toBeNull();
});
});
describe("cleanupConflictingWorktree liveness gate", () => {
it("refuses removal when worktree is in activeWorktrees for another task", async () => {
const store = createMockStore();
const executor = new TaskExecutor(store, "/tmp/test");
(executor as any).activeWorktrees.set("FN-OTHER", ACTIVE_PATH);
store.listTasks.mockResolvedValue([]);
const result = await (executor as any).cleanupConflictingWorktree(
ACTIVE_PATH,
"fusion/fn-9999",
"FN-4811",
);
expect(result).toBe(false);
// No removal-success log should be emitted.
const logCalls = store.logEntry.mock.calls.map((c: any[]) => String(c[1] ?? ""));
expect(logCalls.some((m: string) => m === "Removed conflicting worktree")).toBe(false);
expect(logCalls.some((m: string) => m.includes("Refused to remove conflicting worktree"))).toBe(true);
});
it("refuses removal when DB shows a live in-progress task using the worktree", async () => {
const store = createMockStore();
const executor = new TaskExecutor(store, "/tmp/test");
store.listTasks.mockResolvedValue([
{ id: "FN-LIVE", worktree: ACTIVE_PATH, column: "in-progress", paused: false },
]);
const result = await (executor as any).cleanupConflictingWorktree(
ACTIVE_PATH,
"fusion/fn-9999",
"FN-4811",
);
expect(result).toBe(false);
const logCalls = store.logEntry.mock.calls.map((c: any[]) => String(c[1] ?? ""));
expect(logCalls.some((m: string) => m === "Removed conflicting worktree")).toBe(false);
});
it("proceeds with removal when no active owner is found (preserves existing behavior)", async () => {
const store = createMockStore();
const executor = new TaskExecutor(store, "/tmp/test");
store.listTasks.mockResolvedValue([
{ id: "FN-DONE", worktree: STALE_PATH, column: "done", paused: false },
]);
// Spy on removeWorktree to confirm it's invoked. The mocked exec in test helpers will
// handle the actual git command without touching disk.
const result = await (executor as any).cleanupConflictingWorktree(
STALE_PATH,
"fusion/fn-9999",
"FN-4811",
);
// result may be true or false depending on whether mocked exec succeeds, but the key
// assertion is that the refusal log was NOT emitted (i.e., we got past the gate).
const logCalls = store.logEntry.mock.calls.map((c: any[]) => String(c[1] ?? ""));
expect(logCalls.some((m: string) => m.includes("Refused to remove conflicting worktree"))).toBe(false);
// result is the actual outcome of the removal attempt; the gate didn't block it.
void result;
});
});
describe("handleBranchConflict liveness gate", () => {
it("returns 'sticky' without invoking inspection when conflict path is actively owned", async () => {
const store = createMockStore();
const executor = new TaskExecutor(store, "/tmp/test");
(executor as any).activeWorktrees.set("FN-OWNER", ACTIVE_PATH);
store.listTasks.mockResolvedValue([]);
const inspectSpy = vi.spyOn(branchConflictModule, "inspectBranchConflict");
const cleanupSpy = vi.spyOn(executor as any, "cleanupConflictingWorktree");
const result = await (executor as any).handleBranchConflict(
makeTask(),
makeConflict(ACTIVE_PATH),
);
expect(result).toBe("sticky");
// Critical: inspection must NOT run, because some inspection branches force-remove.
expect(inspectSpy).not.toHaveBeenCalled();
// And cleanup must NOT be invoked.
expect(cleanupSpy).not.toHaveBeenCalled();
// The refusal must be logged for observability.
const logCalls = store.logEntry.mock.calls.map((c: any[]) => String(c[1] ?? ""));
expect(logCalls.some((m: string) => m.includes("FN-4811") && m.includes("deferred"))).toBe(true);
});
it("proceeds with inspection when conflict path has no active owner", async () => {
const store = createMockStore();
const executor = new TaskExecutor(store, "/tmp/test");
store.listTasks.mockResolvedValue([]);
vi.spyOn(branchConflictModule, "inspectBranchConflict").mockResolvedValue({
kind: "stale-resolved",
} as any);
const result = await (executor as any).handleBranchConflict(
makeTask(),
makeConflict(STALE_PATH),
);
// stale-resolved path returns "retry" and clears worktree/branch.
expect(result).toBe("retry");
});
});
});

76
packages/engine/src/executor.ts
View File

@@ -7341,6 +7341,18 @@ Backward compat fallback: if JSON is unavailable, you may still begin output wit
}
private async handleBranchConflict(task: Task, error: BranchConflictError): Promise<"retry" | "reclaimed" | "sticky"> {
// FN-4811: Before invoking inspection-based recovery (which may force-remove the
// conflicting worktree), verify the conflict isn't currently bound to a live session.
// If it is, refuse the whole recovery dance — a force-remove here would yank an active
// task's filesystem out from under it, producing FN-4781/FN-4804-style cascade failures.
const activeOwner = await this.findActiveWorktreeOwner(error.conflictingWorktreePath, task.id);
if (activeOwner !== null) {
const refusalMessage = `[FN-4811] Branch conflict on ${error.branchName} deferred: conflicting worktree ${error.conflictingWorktreePath} is actively owned by ${activeOwner}`;
executorLog.warn(refusalMessage);
await this.store.logEntry(task.id, refusalMessage, undefined, this.currentRunContext);
return "sticky";
}
const inspection = await inspectBranchConflict({
repoDir: this.rootDir,
branchName: error.branchName,
@@ -8103,7 +8115,11 @@ Backward compat fallback: if JSON is unavailable, you may still begin output wit
await this.store.logEntry(taskId, `Cleaned up conflicting worktree, retrying`, path);
return this.tryCreateWorktree(branch, path, taskId, startPoint, attemptNumber, 0, allowSiblingBranchRename, settings);
}
return null;
// FN-4811: When git classifies a worktree as stale but the DB liveness gate refuses
// removal (an active task still has this worktree bound), fall through to the
// sibling-rename path rather than failing the whole conflict-recovery attempt. This
// preserves the live task while letting the requesting task proceed with a fresh
// worktree name.
}
if (inspection.kind === "reclaimable") {
@@ -8130,7 +8146,11 @@ Backward compat fallback: if JSON is unavailable, you may still begin output wit
await this.store.logEntry(taskId, `Removed foreign conflicting worktree and retrying`, inspection.livePath);
return this.tryCreateWorktree(branch, path, taskId, startPoint, attemptNumber, 0, allowSiblingBranchRename, settings);
}
return null;
// FN-4811: Cleanup was refused because the foreign worktree is actively bound to a
// live session. Force-removing would yank an active task's filesystem. Fall through
// to the sibling-rename path (suffix-2 through suffix-6) so the requesting task can
// proceed without disturbing the live owner. If sibling-rename is disabled, the
// generic conflict error below will trigger the caller's auto-recovery dispatcher.
}
if (!allowSiblingBranchRename) {
@@ -8241,6 +8261,45 @@ Backward compat fallback: if JSON is unavailable, you may still begin output wit
return otherUser !== null;
}
/**
* FN-4811: Determine whether `worktreePath` is currently bound to an active executor or
* merger session. If so, removing it would pull the rug out from under a live agent,
* producing the FN-4781/FN-4804 symptoms (worktree disappears mid-task, two parallel runs,
* cross-task contamination). Returns the task ID currently using the worktree, or null if
* the worktree is safe to remove.
*
* Liveness sources, in order:
* 1. In-memory `activeWorktrees` map (per-executor session tracking).
* 2. DB-level: any non-done, non-paused, in-progress task with `task.worktree === path`.
*
* The requesting task is excluded from the check because `cleanupConflictingWorktree` is
* only called for worktrees the requesting task is trying to displace.
*/
private async findActiveWorktreeOwner(
worktreePath: string,
requestingTaskId: string,
): Promise<string | null> {
for (const [taskId, path] of this.activeWorktrees) {
if (taskId !== requestingTaskId && path === worktreePath) {
return taskId;
}
}
try {
const tasks = await this.store.listTasks({ slim: true, includeArchived: false });
for (const t of tasks) {
if (t.id === requestingTaskId) continue;
if (t.worktree !== worktreePath) continue;
if (t.column !== "in-progress") continue;
if (t.paused === true) continue;
return t.id;
}
} catch (err: unknown) {
const msg = err instanceof Error ? err.message : String(err);
executorLog.warn(`findActiveWorktreeOwner: DB liveness check failed for ${worktreePath}: ${msg}`);
}
return null;
}
/**
* Clean up a conflicting worktree and its branch.
* Handles locked worktrees by unlocking first.
@@ -8251,6 +8310,19 @@ Backward compat fallback: if JSON is unavailable, you may still begin output wit
branch: string,
taskId: string,
): Promise<boolean> {
// FN-4811: Hard liveness gate — refuse to remove a worktree that is currently bound to
// an active executor/merger session, regardless of git-level conflict classification.
// This is the canonical guard against the FN-4781/FN-4804 race where a startup cleanup
// pass or branch-conflict recovery yanked the worktree of a still-running session, causing
// "assigned worktree path disappeared mid-task" + parallel-runs + cross-task contamination.
const activeOwner = await this.findActiveWorktreeOwner(worktreePath, taskId);
if (activeOwner !== null) {
const refusalMessage = `[FN-4811] Refused to remove worktree ${worktreePath}: actively owned by ${activeOwner} (requested by ${taskId})`;
executorLog.warn(refusalMessage);
await this.store.logEntry(taskId, `Refused to remove conflicting worktree — actively owned by another task`, `${worktreePath} (owner: ${activeOwner})`);
return false;
}
try {
// Check if worktree is locked and unlock if needed
try {