From f06281961eed14df99d15ba30223ea87bef3afd8 Mon Sep 17 00:00:00 2001 From: gsxdsm Date: Wed, 24 Jun 2026 15:13:43 -0700 Subject: [PATCH] fix(workspace): persist workspaceWorktrees and isolate concurrent session leases MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Multiworkspace tasks could not complete due to two independent bugs: 1. task.workspaceWorktrees had no SQLite column / rowToTask mapping, so fn_acquire_repo_worktree's updateTask write was dropped on every persist (applyTaskPatch writes the DB-round-tripped task back to task.json). Every later getTask returned undefined, so fn_task_done's scope verifier read {} and blocked with "acquired no sub-repo worktrees", and isWorkspaceTask() consumers misfired. Persist it mirroring mergeDetails (schema column + v129 migration + db-migrate + defineTaskColumn + TaskRow + rowToTask). 2. In workspace mode every task ran rooted at the shared browse-only root, and setActiveSession registered that path keyed only by path — so a second concurrent workspace task was rejected by the foreign-task guard ("active-session path ... is held by ..."). Give each task a task-scoped synthetic session key (sessionRegistryPath), applied at all register and unregister sites; the in-memory worktree Set still holds the real root. Regression tests assert the persistence invariant across getTask/listTasks/ store-reopen and concurrent session registration across all three session surfaces; both verified to fail without the fix. Co-Authored-By: Claude Opus 4.8 (1M context) --- .../multiworkspace-worktree-persistence.md | 5 + .../src/__tests__/store-persistence.test.ts | 53 +++++++++++ packages/core/src/db-migrate.ts | 8 +- packages/core/src/db.ts | 18 +++- packages/core/src/store.ts | 18 ++++ ...cutor-workspace-concurrent-session.test.ts | 92 +++++++++++++++++++ packages/engine/src/executor.ts | 43 +++++++-- 7 files changed, 227 insertions(+), 10 deletions(-) create mode 100644 .changeset/multiworkspace-worktree-persistence.md create mode 100644 packages/engine/src/__tests__/executor-workspace-concurrent-session.test.ts diff --git a/.changeset/multiworkspace-worktree-persistence.md b/.changeset/multiworkspace-worktree-persistence.md new file mode 100644 index 0000000000..2179e7a766 --- /dev/null +++ b/.changeset/multiworkspace-worktree-persistence.md @@ -0,0 +1,5 @@ +--- +"@runfusion/fusion": patch +--- + +Fix multiworkspace tasks failing to complete. `task.workspaceWorktrees` is now durably persisted (it previously had no SQLite column, so `fn_acquire_repo_worktree`'s write was dropped on every persist and `fn_task_done` always reported "acquired no sub-repo worktrees"). Concurrent workspace tasks no longer collide on the shared browse-root active-session path — each task gets a task-scoped session key, so a second workspace task no longer fails with "active-session path … is held by …". diff --git a/packages/core/src/__tests__/store-persistence.test.ts b/packages/core/src/__tests__/store-persistence.test.ts index 54f4670338..a39f43ac25 100644 --- a/packages/core/src/__tests__/store-persistence.test.ts +++ b/packages/core/src/__tests__/store-persistence.test.ts @@ -59,6 +59,59 @@ describe("TaskStore", () => { }); }); + // FNXC:Workspace 2026-06-24-15:30 (multiworkspace fn_task_done regression): + // task.workspaceWorktrees previously had NO SQLite column / no rowToTask mapping, so + // fn_acquire_repo_worktree's updateTask({workspaceWorktrees}) set it only in memory and the very + // next getTask (SQLite round-trip) dropped it. fn_task_done's scope verifier then read `{}` and + // refused with "acquired no sub-repo worktrees", and every isWorkspaceTask() consumer misfired. + // The invariant: the per-sub-repo worktree map survives write→read across ALL surfaces — + // getTask, listTasks, AND a full store reopen (SQLite + task.json + reconcile). + describe("workspaceWorktrees persistence", () => { + const sampleMap = { + swarmclaw: { worktreePath: "/ws/swarmclaw/.worktrees/ivory-raven", branch: "fusion/mult-002", baseCommitSha: "a327402" }, + OpenVide: { worktreePath: "/ws/OpenVide/.worktrees/light-ember", branch: "fusion/mult-001" }, + }; + + it("round-trips the per-sub-repo worktree map through write and getTask", async () => { + const task = await harness.store().createTask({ description: "Workspace task" }); + + const updated = await harness.store().updateTask(task.id, { workspaceWorktrees: sampleMap }); + expect(updated.workspaceWorktrees).toEqual(sampleMap); + + // The smoking-gun assertion: getTask reads back from SQLite (rowToTask), not the in-memory + // mutation. Before the fix this returned undefined because no column persisted the map. + const detail = await harness.store().getTask(task.id); + expect(detail.workspaceWorktrees).toEqual(sampleMap); + }); + + it("returns the map from listTasks", async () => { + const task = await harness.store().createTask({ description: "Workspace task in list" }); + await harness.store().updateTask(task.id, { workspaceWorktrees: sampleMap }); + + const listed = (await harness.store().listTasks()).find((t) => t.id === task.id); + expect(listed?.workspaceWorktrees).toEqual(sampleMap); + }); + + it("survives a full store reopen (SQLite + task.json + reconcile)", async () => { + const task = await harness.store().createTask({ description: "Workspace task across reopen" }); + await harness.store().updateTask(task.id, { workspaceWorktrees: sampleMap }); + + await harness.reopenDiskBackedStore(); + + const detail = await harness.store().getTask(task.id); + expect(detail.workspaceWorktrees).toEqual(sampleMap); + }); + + it("normalizes an empty map to undefined so isWorkspaceTask stays false", async () => { + const task = await harness.store().createTask({ description: "Empty workspace map" }); + const updated = await harness.store().updateTask(task.id, { workspaceWorktrees: {} }); + expect(updated.workspaceWorktrees ?? {}).toEqual({}); + + const detail = await harness.store().getTask(task.id); + expect(detail.workspaceWorktrees).toBeUndefined(); + }); + }); + describe("tokenUsage persistence", () => { it("round-trips per-model token buckets through write and read", async () => { const task = await harness.store().createTask({ description: "Per-model token task" }); diff --git a/packages/core/src/db-migrate.ts b/packages/core/src/db-migrate.ts index 30d10cab8e..6322a87e22 100644 --- a/packages/core/src/db-migrate.ts +++ b/packages/core/src/db-migrate.ts @@ -226,10 +226,11 @@ async function migrateTasks(fusionDir: string, db: Database): Promise { columnMovedAt, dependencies, steps, log, attachments, steeringComments, comments, workflowStepResults, prInfo, issueInfo, sourceIssueProvider, sourceIssueRepository, sourceIssueExternalIssueId, sourceIssueNumber, sourceIssueUrl, sourceIssueClosedAt, - mergeDetails, breakIntoSubtasks, noCommitsExpected, enabledWorkflowSteps, modifiedFiles, sliceId + mergeDetails, breakIntoSubtasks, noCommitsExpected, enabledWorkflowSteps, modifiedFiles, sliceId, + workspaceWorktrees ) VALUES ( ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, - ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ? + ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ? ) `); @@ -300,6 +301,9 @@ async function migrateTasks(fusionDir: string, db: Database): Promise { toJson(task.enabledWorkflowSteps || []), toJson(task.modifiedFiles || []), task.sliceId ?? null, + // FNXC:Workspace 2026-06-24-15:30: carry the per-sub-repo worktree map through the legacy + // task.json→SQLite rebuild so a workspace task migrated from disk keeps its acquired worktrees. + toJsonNullable(task.workspaceWorktrees), ); migrated++; } catch (err) { diff --git a/packages/core/src/db.ts b/packages/core/src/db.ts index 2ef6ca39b4..f84d0c1e59 100644 --- a/packages/core/src/db.ts +++ b/packages/core/src/db.ts @@ -162,7 +162,7 @@ export function isFts5CorruptionError(error: unknown): boolean { // ── Schema Definition ──────────────────────────────────────────────── -const SCHEMA_VERSION = 128; +const SCHEMA_VERSION = 129; const TASKS_FTS_AUTOMERGE = 8; const TASKS_FTS_CRISISMERGE = 16; @@ -347,7 +347,11 @@ CREATE TABLE IF NOT EXISTS tasks ( deletedAt TEXT, allowResurrection INTEGER DEFAULT 0, transitionPending TEXT, - customFields TEXT DEFAULT '{}' + customFields TEXT DEFAULT '{}', + -- FNXC:Workspace 2026-06-24-15:30: per-sub-repo worktree map (JSON) for workspace-mode tasks. + -- Source of truth for getSchemaCompatibilityTableSchemas(), so existing DBs are backfilled by + -- ensureSchemaCompatibility() at boot and fresh DBs get it here. See store.ts TaskRow note. + workspaceWorktrees TEXT ); -- Config table (single row with project settings) @@ -5289,6 +5293,16 @@ export class Database { }); } + if (version < 129) { + // FNXC:Workspace 2026-06-24-15:30: add the workspaceWorktrees column so workspace-mode tasks + // can durably persist their per-sub-repo worktree map. Backfill is also covered by + // ensureSchemaCompatibility() (SCHEMA_SQL is its source of truth); this versioned migration keeps + // migrated and fresh-from-SCHEMA_SQL DBs converged. + this.applyMigration(129, () => { + this.addColumnIfMissing("tasks", "workspaceWorktrees", "TEXT"); + }); + } + } /** diff --git a/packages/core/src/store.ts b/packages/core/src/store.ts index b5e85e2411..6db34a6944 100644 --- a/packages/core/src/store.ts +++ b/packages/core/src/store.ts @@ -277,6 +277,13 @@ interface TaskRow { sourceIssueUrl: string | null; sourceIssueClosedAt: string | null; mergeDetails: string | null; + // FNXC:Workspace 2026-06-24-15:30 (FN-multiworkspace persistence): the per-sub-repo worktree + // map MUST have its own SQLite column. Before this it was a Task field with NO column/rowToTask + // mapping, so updateTask set it only in-memory and applyTaskPatch wrote the DB-round-tripped + // task (without it) back to task.json — the map was silently dropped on every persist. That made + // fn_task_done's scope verifier always read `{}` ("acquired no sub-repo worktrees") and broke + // every isWorkspaceTask() consumer. Stored as JSON text, same shape as mergeDetails. + workspaceWorktrees: string | null; breakIntoSubtasks: number | null; noCommitsExpected: number | null; enabledWorkflowSteps: string | null; @@ -429,6 +436,10 @@ const TASK_COLUMN_DESCRIPTORS: TaskColumnDescriptor[] = [ defineTaskColumn("sourceIssueUrl", (task) => task.sourceIssue?.url ?? null), defineTaskColumn("sourceIssueClosedAt", (task) => task.sourceIssue?.closedAt ?? null), defineTaskColumn("mergeDetails", (task) => toJsonNullable(task.mergeDetails)), + // FNXC:Workspace 2026-06-24-15:30: persist the per-sub-repo worktree map so fn_acquire_repo_worktree's + // write survives the SQLite round-trip getChangedTaskColumns/rowToTask use. Without this descriptor the + // column diff never sees a change and the field never reaches the DB. + defineTaskColumn("workspaceWorktrees", (task) => toJsonNullable(task.workspaceWorktrees)), defineTaskColumn("breakIntoSubtasks", (task) => task.breakIntoSubtasks ? 1 : 0), defineTaskColumn("noCommitsExpected", (task) => task.noCommitsExpected ? 1 : 0), defineTaskColumn("enabledWorkflowSteps", (task) => toJson(task.enabledWorkflowSteps || [])), @@ -2150,6 +2161,13 @@ export class TaskStore extends EventEmitter { }; })(), mergeDetails: fromJson(row.mergeDetails), + // FNXC:Workspace 2026-06-24-15:30: deserialize the per-sub-repo worktree map. An empty/null map + // normalizes to undefined so isWorkspaceTask() (keys-length>0) and the scope verifier behave the + // same as a task that never acquired a sub-repo. + workspaceWorktrees: (() => { + const w = fromJson(row.workspaceWorktrees); + return w && Object.keys(w).length > 0 ? w : undefined; + })(), breakIntoSubtasks: row.breakIntoSubtasks ? true : undefined, noCommitsExpected: row.noCommitsExpected ? true : undefined, enabledWorkflowSteps: (() => { const e = fromJson(row.enabledWorkflowSteps); return e && e.length > 0 ? e : undefined; })(), diff --git a/packages/engine/src/__tests__/executor-workspace-concurrent-session.test.ts b/packages/engine/src/__tests__/executor-workspace-concurrent-session.test.ts new file mode 100644 index 0000000000..978f0c23de --- /dev/null +++ b/packages/engine/src/__tests__/executor-workspace-concurrent-session.test.ts @@ -0,0 +1,92 @@ +/* +FNXC:Workspace 2026-06-24-15:45 (concurrent workspace tasks — shared browse-root collision regression): +In workspace mode every task runs its agent session rooted at the SHARED browse-only workspace root +(`this.rootDir`); per-sub-repo worktrees are acquired on demand. The session registrations +(executor / step-session / workflow-step) are keyed in the GLOBAL path-keyed activeSessionRegistry, +whose foreign-task guard rejects a second task registering a path already held by a different task. +With the bare root as the key, the SECOND concurrent workspace task failed with +"active-session path is held by task ; task may not overwrite it" — so only ONE +task per workspace could ever run (the reported MULT-001 vs MULT-002 failure). + +Invariant under test (across ALL session-registration surfaces): two different workspace tasks sharing +the browse-root register concurrently WITHOUT collision, each remains discoverable by liveness +(pathsForTask returns a task-scoped key), and cleanup leaves no leaked entry. Negative control: a +NON-workspace executor (unique worktree path) still rejects a foreign-task overwrite, so the +cross-phase-clobber guard is preserved. +*/ +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 WORKSPACE_ROOT = "/tmp/fusion-test-workspace-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; +} + +function makeWorkspaceExecutor(): TaskExecutor { + const executor = new TaskExecutor(createStore(), WORKSPACE_ROOT); + (executor as any).workspaceConfig = { repos: ["swarmclaw", "OpenVide"] }; + return executor; +} + +describe("workspace concurrent session registration", () => { + beforeEach(() => activeSessionRegistry.clear()); + afterEach(() => activeSessionRegistry.clear()); + + it("lets two workspace tasks register executor sessions on the shared browse-root without collision", () => { + const executor = makeWorkspaceExecutor(); + + // Both tasks pass the SAME shared workspace root as worktreePath — the pre-fix collision point. + expect(() => (executor as any).setActiveSession("MULT-001", {}, WORKSPACE_ROOT)).not.toThrow(); + expect(() => (executor as any).setActiveSession("MULT-002", {}, WORKSPACE_ROOT)).not.toThrow(); + + // Each task stays discoverable by liveness via a DISTINCT task-scoped registry key. + const a = activeSessionRegistry.pathsForTask("MULT-001"); + const b = activeSessionRegistry.pathsForTask("MULT-002"); + expect(a).toHaveLength(1); + expect(b).toHaveLength(1); + expect(a[0]).not.toEqual(b[0]); + expect(a[0]).toContain("MULT-001"); + expect(b[0]).toContain("MULT-002"); + }); + + it("cleans up the task-scoped session key on deleteActiveSession (no leak)", () => { + const executor = makeWorkspaceExecutor(); + // The in-memory activeWorktrees Set holds the REAL root; deleteActiveSession must still map it + // back to the synthetic key it registered. + (executor as any).addActiveWorktree("MULT-001", WORKSPACE_ROOT); + (executor as any).setActiveSession("MULT-001", {}, WORKSPACE_ROOT); + expect(activeSessionRegistry.pathsForTask("MULT-001")).toHaveLength(1); + + (executor as any).deleteActiveSession("MULT-001"); + expect(activeSessionRegistry.pathsForTask("MULT-001")).toHaveLength(0); + }); + + it("does not collide across the step-session and workflow-step surfaces either", () => { + const executor = makeWorkspaceExecutor(); + expect(() => (executor as any).setActiveStepExecutor("MULT-001", {}, WORKSPACE_ROOT)).not.toThrow(); + expect(() => (executor as any).setActiveStepExecutor("MULT-002", {}, WORKSPACE_ROOT)).not.toThrow(); + expect(() => (executor as any).setActiveWorkflowStepSession("MULT-001", {}, WORKSPACE_ROOT)).not.toThrow(); + expect(() => (executor as any).setActiveWorkflowStepSession("MULT-002", {}, WORKSPACE_ROOT)).not.toThrow(); + }); + + it("still rejects a foreign-task overwrite for NON-workspace tasks (clobber guard preserved)", () => { + const sharedWorktree = "/tmp/fusion-test-single-repo-worktree"; + const executor = new TaskExecutor(createStore(), sharedWorktree); // 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 + // cross-phase-clobber protection the workspace fix must not weaken. + expect(() => (executor as any).setActiveSession("FN-B", {}, sharedWorktree)).toThrow( + ActiveSessionPathHeldByForeignTaskError, + ); + }); +}); diff --git a/packages/engine/src/executor.ts b/packages/engine/src/executor.ts index 3a8b23ef14..df189829e6 100644 --- a/packages/engine/src/executor.ts +++ b/packages/engine/src/executor.ts @@ -1620,9 +1620,31 @@ export class TaskExecutor { this.completionFinalizedTaskIds.delete(taskId); } + /* + FNXC:Workspace 2026-06-24-15:45 (concurrent workspace tasks — shared browse-root collision): + In workspace mode `this.rootDir` is the SHARED browse-only (non-git) workspace root, and EVERY + workspace task runs its agent session rooted there (per-sub-repo worktrees are acquired on demand). + The session registrations below are keyed in the GLOBAL path-keyed activeSessionRegistry, whose + foreign-task guard rejects a second task registering a path already held by a different task. With + the bare root as the key, the second concurrent workspace task fails with "active-session path + is held by task ; task may not overwrite it" — so only ONE task per workspace + could ever run. Per-task session liveness does NOT require path-exclusivity on the shared root + (real per-sub-repo exclusivity is enforced separately by the workspace-repo-acquire lease in + worktree-acquisition.ts, keyed by sub-repo path). Give each task a task-scoped synthetic session + key so the registry stays per-task. The in-memory activeWorktrees Set still holds the REAL root, so + getActiveWorktreePaths() consumers that cd into a path are unaffected; only the registry key changes. + Non-workspace tasks (unique worktree path != rootDir) are returned unchanged. + */ + private sessionRegistryPath(taskId: string, worktreePath: string): string { + if (this.workspaceConfig && worktreePath === this.rootDir) { + return `${worktreePath}#session:${taskId}`; + } + return worktreePath; + } + private setActiveSession(taskId: string, sessionState: ActiveExecutorSessionState, worktreePath: string): void { this.activeSessions.set(taskId, sessionState); - activeSessionRegistry.registerPath(worktreePath, { taskId, kind: "executor", ownerKey: taskId }); + activeSessionRegistry.registerPath(this.sessionRegistryPath(taskId, worktreePath), { taskId, kind: "executor", ownerKey: taskId }); } private markGraphExecuteSelfRequeued(taskId: string): void { @@ -1638,14 +1660,17 @@ export class TaskExecutor { // FNXC:Workspace 2026-06-21-12:00: KTD2 — when no explicit path is given, unregister EVERY worktree path the task holds (a workspace task holds N sub-repo paths); single-repo tasks resolve a one-element set. const resolvedWorktreePaths = worktreePath ? [worktreePath] : this.getActiveWorktreePaths(taskId); for (const path of resolvedWorktreePaths) { - activeSessionRegistry.unregisterPath(path); + // FNXC:Workspace 2026-06-24-15:45: map through sessionRegistryPath so the task-scoped synthetic + // session key registered for the shared workspace browse-root is the one we unregister (the + // in-memory Set holds the REAL root). Non-workspace/sub-repo paths pass through unchanged. + activeSessionRegistry.unregisterPath(this.sessionRegistryPath(taskId, path)); } } private setActiveStepExecutor(taskId: string, stepExecutor: StepSessionExecutor, worktreePath: string, seenSteeringIds = new Set()): void { this.activeStepExecutors.set(taskId, stepExecutor); this.activeStepExecutorSeenSteeringIds.set(taskId, seenSteeringIds); - activeSessionRegistry.registerPath(worktreePath, { taskId, kind: "step-session", ownerKey: `${taskId}#step-session` }); + activeSessionRegistry.registerPath(this.sessionRegistryPath(taskId, worktreePath), { taskId, kind: "step-session", ownerKey: `${taskId}#step-session` }); } private deleteActiveStepExecutor(taskId: string, worktreePath?: string): void { @@ -1656,14 +1681,17 @@ export class TaskExecutor { // FNXC:Workspace 2026-06-21-12:00: KTD2 — unregister every held worktree path (Set), not one. const resolvedWorktreePaths = worktreePath ? [worktreePath] : this.getActiveWorktreePaths(taskId); for (const path of resolvedWorktreePaths) { - activeSessionRegistry.unregisterPath(path); + // FNXC:Workspace 2026-06-24-15:45: map through sessionRegistryPath so the task-scoped synthetic + // session key registered for the shared workspace browse-root is the one we unregister (the + // in-memory Set holds the REAL root). Non-workspace/sub-repo paths pass through unchanged. + activeSessionRegistry.unregisterPath(this.sessionRegistryPath(taskId, path)); } } private setActiveWorkflowStepSession(taskId: string, session: AgentSession, worktreePath: string, seenSteeringIds = new Set()): void { this.activeWorkflowStepSessions.set(taskId, session); this.activeWorkflowStepSessionSeenSteeringIds.set(taskId, seenSteeringIds); - activeSessionRegistry.registerPath(worktreePath, { taskId, kind: "workflow-step", ownerKey: `${taskId}#workflow-step` }); + activeSessionRegistry.registerPath(this.sessionRegistryPath(taskId, worktreePath), { taskId, kind: "workflow-step", ownerKey: `${taskId}#workflow-step` }); } private deleteActiveWorkflowStepSession(taskId: string, worktreePath?: string): void { @@ -1672,7 +1700,10 @@ export class TaskExecutor { // FNXC:Workspace 2026-06-21-12:00: KTD2 — unregister every held worktree path (Set), not one. const resolvedWorktreePaths = worktreePath ? [worktreePath] : this.getActiveWorktreePaths(taskId); for (const path of resolvedWorktreePaths) { - activeSessionRegistry.unregisterPath(path); + // FNXC:Workspace 2026-06-24-15:45: map through sessionRegistryPath so the task-scoped synthetic + // session key registered for the shared workspace browse-root is the one we unregister (the + // in-memory Set holds the REAL root). Non-workspace/sub-repo paths pass through unchanged. + activeSessionRegistry.unregisterPath(this.sessionRegistryPath(taskId, path)); } }