diff --git a/.changeset/plan-review-shared-root-session-collision.md b/.changeset/plan-review-shared-root-session-collision.md new file mode 100644 index 0000000000..46524ad59a --- /dev/null +++ b/.changeset/plan-review-shared-root-session-collision.md @@ -0,0 +1,7 @@ +--- +"@runfusion/fusion": patch +--- + +summary: Two tasks can now run Plan Review at the same time instead of one failing and parking. +category: fix +dev: `TaskExecutor.sessionRegistryPath` now task-scopes the activeSessionRegistry key for any session rooted at `rootDir`, not just in workspace mode. Read-only graph nodes (Plan Review) run at the repo root, so the bare-root key made the second concurrent task throw `ActiveSessionPathHeldByForeignTaskError`, which surfaced as a Plan Review provider failure and burned the in-place retry budget. diff --git a/packages/engine/src/__tests__/executor-archive-releases-active-session.test.ts b/packages/engine/src/__tests__/executor-archive-releases-active-session.test.ts index 0f815b5b16..88186b191c 100644 --- a/packages/engine/src/__tests__/executor-archive-releases-active-session.test.ts +++ b/packages/engine/src/__tests__/executor-archive-releases-active-session.test.ts @@ -45,10 +45,16 @@ describe("archiving a task releases its active-session registry entries (FN-7717 it("releases a workflow-step session held by a task archived from triage, letting a successor acquire the same path", async () => { const { executor, store } = makeExecutor(); - // Task A registers a workflow-step (Plan Review) session on the shared root — the - // reported NEXT-508 case. + /* + FNXC:PlanReviewWorktree 2026-07-25-20:40: + A root-rooted session is now registered under a TASK-SCOPED synthetic key (sessionRegistryPath), + so assert release through `pathsForTask` — the bare-root key never exists. What archive must still + guarantee is unchanged: the task holds exactly one live entry before, and none after. + */ (executor as any).setActiveWorkflowStepSession("TASK-A", {}, SHARED_ROOT); - expect(activeSessionRegistry.isPathActive(SHARED_ROOT)).toBe(true); + const [heldPath] = activeSessionRegistry.pathsForTask("TASK-A"); + expect(heldPath).toBeDefined(); + expect(activeSessionRegistry.isPathActive(heldPath)).toBe(true); // Drive the archive transition: to === "archived", from a NON-in-progress column // (Plan Review runs in triage), exactly like archiveTask emits. @@ -57,12 +63,12 @@ describe("archiving a task releases its active-session registry entries (FN-7717 // Await the disposal chain the handler kicked off via trackTaskDisposal. await (executor as any).pendingTaskDisposals.get("TASK-A"); - expect(activeSessionRegistry.isPathActive(SHARED_ROOT)).toBe(false); + expect(activeSessionRegistry.isPathActive(heldPath)).toBe(false); expect(activeSessionRegistry.pathsForTask("TASK-A")).toHaveLength(0); // Successor task B can now register the same path without throwing. expect(() => - activeSessionRegistry.registerPath(SHARED_ROOT, { taskId: "TASK-B", kind: "workflow-step", ownerKey: "TASK-B#workflow-step" }), + activeSessionRegistry.registerPath(heldPath, { taskId: "TASK-B", kind: "workflow-step", ownerKey: "TASK-B#workflow-step" }), ).not.toThrow(); }); @@ -149,11 +155,18 @@ describe("archiving a task releases its active-session registry entries (FN-7717 it("reproduces the original ActiveSessionPathHeldByForeignTaskError before archive, and confirms it is gone after", async () => { const { executor, store } = makeExecutor(); - (executor as any).setActiveWorkflowStepSession("NEXT-508", {}, SHARED_ROOT); + /* + FNXC:PlanReviewWorktree 2026-07-25-20:40: + The original NEXT-508 symptom was reproduced on the shared ROOT. That path can no longer collide at + all (root keys are task-scoped), so the leak symptom is now reproduced on a per-task WORKTREE path, + where the foreign-task guard still applies and only archive can release the holder. + */ + const heldWorktree = `${SHARED_ROOT}-next508-worktree`; + (executor as any).setActiveWorkflowStepSession("NEXT-508", {}, heldWorktree); // Before archive: a second task trying to register the same path is rejected. expect(() => - activeSessionRegistry.registerPath(SHARED_ROOT, { taskId: "NEXT-433", kind: "workflow-step", ownerKey: "NEXT-433#workflow-step" }), + activeSessionRegistry.registerPath(heldWorktree, { taskId: "NEXT-433", kind: "workflow-step", ownerKey: "NEXT-433#workflow-step" }), ).toThrow(ActiveSessionPathHeldByForeignTaskError); store.emit("task:moved", { task: makeTask("NEXT-508"), from: "triage", to: "archived", source: "user" }); @@ -161,7 +174,7 @@ describe("archiving a task releases its active-session registry entries (FN-7717 // After archive: the successor can now acquire the path. expect(() => - activeSessionRegistry.registerPath(SHARED_ROOT, { taskId: "NEXT-433", kind: "workflow-step", ownerKey: "NEXT-433#workflow-step" }), + activeSessionRegistry.registerPath(heldWorktree, { taskId: "NEXT-433", kind: "workflow-step", ownerKey: "NEXT-433#workflow-step" }), ).not.toThrow(); }); }); diff --git a/packages/engine/src/__tests__/executor-root-rooted-concurrent-session.test.ts b/packages/engine/src/__tests__/executor-root-rooted-concurrent-session.test.ts new file mode 100644 index 0000000000..5f9eaf731b --- /dev/null +++ b/packages/engine/src/__tests__/executor-root-rooted-concurrent-session.test.ts @@ -0,0 +1,106 @@ +/* +FNXC:PlanReviewWorktree 2026-07-25-20:40 (concurrent root-rooted step sessions — single-repo regression): +Read-only graph nodes that need no worktree run rooted at the executor's `rootDir`, and a todo task has +no worktree of its own. Plan Review is the canonical one. With the bare root as the activeSessionRegistry +key, the SECOND task to reach such a node failed with "active-session path is held by task ; +task may not overwrite it" — narrated as a Plan Review provider failure, retried in place against a +hold retrying can never clear, then parked once the budget was spent (reported: FN-1398 holding +/home/ubuntu/dev/freemap-svelte while FN-1403 planned). + +Invariant under test: on a plain single-repo project (NO workspaceConfig — the workspace fix did not cover +this), two different tasks register root-rooted sessions concurrently across ALL THREE registration +surfaces (executor / step-session / workflow-step) without collision, each stays discoverable by liveness, +and each delete surface cleans up its synthetic key. Negative control: a genuine shared per-task worktree +path (not the root) still rejects the foreign-task overwrite. +*/ +import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; +import { EventEmitter } from "node:events"; +import type { TaskStore } from "@fusion/core"; +import { TaskExecutor } from "../executor.js"; +import { activeSessionRegistry, ActiveSessionPathHeldByForeignTaskError } from "../active-session-registry.js"; + +const ROOT = "/tmp/fusion-test-single-repo-project-root"; + +function createStore(): TaskStore & EventEmitter { + const emitter = new EventEmitter(); + return Object.assign(emitter, { + logEntry: vi.fn().mockResolvedValue(undefined), + getRunContextFor: vi.fn(), + getSettings: vi.fn().mockResolvedValue({}), + }) as unknown as TaskStore & EventEmitter; +} + +/** Plain single-repo executor: no workspaceConfig, so the pre-fix bare-root key applied. */ +function makeSingleRepoExecutor(): TaskExecutor { + return new TaskExecutor(createStore(), ROOT); +} + +describe("root-rooted concurrent session registration (single-repo project)", () => { + beforeEach(() => activeSessionRegistry.clear()); + afterEach(() => activeSessionRegistry.clear()); + + const surfaces: Array<[string, (executor: TaskExecutor, taskId: string) => void]> = [ + ["setActiveSession", (executor, taskId) => (executor as any).setActiveSession(taskId, {}, ROOT)], + ["setActiveStepExecutor", (executor, taskId) => (executor as any).setActiveStepExecutor(taskId, {}, ROOT)], + ["setActiveWorkflowStepSession", (executor, taskId) => (executor as any).setActiveWorkflowStepSession(taskId, {}, ROOT)], + ]; + + for (const [name, register] of surfaces) { + it(`lets two tasks register concurrently on the repo root via ${name}`, () => { + const executor = makeSingleRepoExecutor(); + + expect(() => register(executor, "FN-1398")).not.toThrow(); + expect(() => register(executor, "FN-1403")).not.toThrow(); + + const holder = activeSessionRegistry.pathsForTask("FN-1398"); + const planner = activeSessionRegistry.pathsForTask("FN-1403"); + expect(holder).toHaveLength(1); + expect(planner).toHaveLength(1); + expect(holder[0]).not.toEqual(planner[0]); + expect(activeSessionRegistry.isPathActive(holder[0])).toBe(true); + expect(activeSessionRegistry.isPathActive(planner[0])).toBe(true); + }); + } + + const cleanups: Array<[string, (executor: TaskExecutor, taskId: string) => void, (executor: TaskExecutor, taskId: string) => void]> = [ + [ + "deleteActiveSession", + (executor, taskId) => (executor as any).setActiveSession(taskId, {}, ROOT), + (executor, taskId) => (executor as any).deleteActiveSession(taskId), + ], + [ + "deleteActiveStepExecutor", + (executor, taskId) => (executor as any).setActiveStepExecutor(taskId, {}, ROOT), + (executor, taskId) => (executor as any).deleteActiveStepExecutor(taskId), + ], + [ + "deleteActiveWorkflowStepSession", + (executor, taskId) => (executor as any).setActiveWorkflowStepSession(taskId, {}, ROOT), + (executor, taskId) => (executor as any).deleteActiveWorkflowStepSession(taskId), + ], + ]; + + for (const [name, register, remove] of cleanups) { + it(`cleans up the task-scoped root key on ${name} (no leak)`, () => { + const executor = makeSingleRepoExecutor(); + // The in-memory activeWorktrees Set holds the REAL root; the delete surface must map it back to + // the synthetic key that was registered. + (executor as any).addActiveWorktree("FN-1403", ROOT); + register(executor, "FN-1403"); + expect(activeSessionRegistry.pathsForTask("FN-1403")).toHaveLength(1); + + remove(executor, "FN-1403"); + expect(activeSessionRegistry.pathsForTask("FN-1403")).toHaveLength(0); + }); + } + + it("still rejects a foreign-task overwrite on a shared per-task worktree path", () => { + const executor = makeSingleRepoExecutor(); + const sharedWorktree = `${ROOT}-worktrees/shared`; + + (executor as any).setActiveWorkflowStepSession("FN-1398", {}, sharedWorktree); + expect(() => (executor as any).setActiveWorkflowStepSession("FN-1403", {}, sharedWorktree)).toThrow( + ActiveSessionPathHeldByForeignTaskError, + ); + }); +}); diff --git a/packages/engine/src/__tests__/executor-workspace-concurrent-session.test.ts b/packages/engine/src/__tests__/executor-workspace-concurrent-session.test.ts index 978f0c23de..fdce142271 100644 --- a/packages/engine/src/__tests__/executor-workspace-concurrent-session.test.ts +++ b/packages/engine/src/__tests__/executor-workspace-concurrent-session.test.ts @@ -79,8 +79,14 @@ describe("workspace concurrent session registration", () => { }); it("still rejects a foreign-task overwrite for NON-workspace tasks (clobber guard preserved)", () => { + /* + FNXC:PlanReviewWorktree 2026-07-25-20:40: + The shared path under test must be a real per-task WORKTREE path, distinct from the executor's + rootDir — the root itself is now task-scoped in every mode (see sessionRegistryPath). Two tasks + landing on the identical worktree path is the cross-phase clobber this guard exists for. + */ const sharedWorktree = "/tmp/fusion-test-single-repo-worktree"; - const executor = new TaskExecutor(createStore(), sharedWorktree); // no workspaceConfig → singular path + const executor = new TaskExecutor(createStore(), "/tmp/fusion-test-single-repo-root"); // no workspaceConfig → singular path (executor as any).setActiveSession("FN-A", {}, sharedWorktree); // A second, different task on the identical real worktree path must still be rejected — this is the diff --git a/packages/engine/src/executor.ts b/packages/engine/src/executor.ts index 85241f4af6..c9a90c8c45 100644 --- a/packages/engine/src/executor.ts +++ b/packages/engine/src/executor.ts @@ -1973,8 +1973,23 @@ export class TaskExecutor { getActiveWorktreePaths() consumers that cd into a path are unaffected; only the registry key changes. Non-workspace tasks (unique worktree path != rootDir) are returned unchanged. */ + /* + FNXC:PlanReviewWorktree 2026-07-25-20:40 (concurrent root-rooted step sessions — single-repo collision): + The task-scoped key must apply to the shared repo root in EVERY project mode, not only workspace mode. + Read-only graph nodes that need no worktree (Plan Review is the canonical one — it reviews the + store-injected PROMPT.md, see FNXC:PlanReviewSpecInjection) run rooted at `this.rootDir`, and a todo + task has no worktree of its own. With the bare root as the registry key, two tasks reaching Plan Review + at the same time collided: the second failed with "active-session path is held by task ; + task may not overwrite it", which surfaced as a Plan Review provider failure, burned the + in-place retry budget against a hold that retrying can never clear, and left the task parked + (reported: FN-1398 holding /home/ubuntu/dev/freemap-svelte while FN-1403 planned). + Path-exclusivity on the shared root is not what keeps these sessions correct: write-capable nodes are + refused at the root outright (no-worktree-for-write-node above), real per-sub-repo exclusivity is the + workspace-repo-acquire lease, and every isPathActive consumer guards removable WORKTREE paths — the + root is never one. Liveness still works because the synthetic key stays in the registry under the task. + */ private sessionRegistryPath(taskId: string, worktreePath: string): string { - if (this.workspaceConfig && worktreePath === this.rootDir) { + if (worktreePath === this.rootDir) { return `${worktreePath}#session:${taskId}`; } return worktreePath;