diff --git a/.changeset/fn-7717-archive-active-session-release.md b/.changeset/fn-7717-archive-active-session-release.md new file mode 100644 index 0000000000..49b1578ed8 --- /dev/null +++ b/.changeset/fn-7717-archive-active-session-release.md @@ -0,0 +1,7 @@ +--- +"@runfusion/fusion": patch +--- + +summary: Archiving a task now releases its active-session lock so the next task can run Plan Review. +category: fix +dev: task:moved handler in packages/engine/src/executor.ts now disposes active surfaces and sweeps activeSessionRegistry paths for any move to the terminal "archived" column (previously only from==="in-progress"); done/in-review merge leases are deliberately untouched. diff --git a/docs/architecture.md b/docs/architecture.md index 39d39d8c7a..c07c9288fa 100644 --- a/docs/architecture.md +++ b/docs/architecture.md @@ -2102,6 +2102,7 @@ This section preserves the detailed lifecycle/self-healing contracts that were f - **Landed-files attribution (FN-5103)**: Rebase-strategy `mergeDetails.landedFiles` / `filesChanged` / `insertions` / `deletions` are captured from task-attributable commits only via `filterFilesToOwnTaskCommits` (subject-prefix + trailer + bracket-prefix evidence), tagged `landedFilesAttributionRestricted: true`. Zero own commits → `landedFiles: []` and `noOpVerifiedShortCircuit: true`. FN-5304 guard: when `..HEAD` reports zero own commits, merger must also validate the source `fusion/` tip; if that source tip still has attributable own commits relative to `rebaseBaseSha`, throw `SilentNoOpAttributionMismatchError`, refuse writing `mergeConfirmed: true`, park the task in `in-review` with `status: "failed"`, and emit `merge:no-op-attribution-mismatch`. If source ref is unavailable, skip with diagnostic + `merge:no-op-attribution-mismatch-skipped` (`reason: "source-ref-unavailable"`). Attribution-helper failures fall back to the unrestricted `rebaseBaseSha..sha` walk and set `landedFilesCaptureFallback: 'attribution-failed'`. Self-healing `recoverDoneTaskMergeMetadata` skips reconcile when `landedFilesAttributionRestricted` or `noOpVerifiedShortCircuit` is set so the narrower set is not overwritten with the full range. Squash-strategy capture is unchanged. - **Soft-delete scheduler invalidation (FN-5137)**: `task:deleted` events must invalidate `AutoClaimSnapshotManager` and clear scheduler bookkeeping (`pausedTaskIds`, `failedTaskIds`, `wasNodeDispatchValidationBlocked`, `wasNodeBlocked`); `executor.execute()` / `resumeOrphaned()` / `resumeTaskForAgent()` refuse any task with `deletedAt` set. - **Soft-delete in-flight abort (FN-5142)**: `task:deleted` must immediately abort/dispose active executor work (`activeSessions`, `activeStepExecutors`, `activeWorkflowStepSessions`, reviewer subagents), interrupt active merge state (`mergeAbortController`, `activeMergeSession`, `activeMergeTaskId`, `mergeActive`, `mergeQueue`, `pausedReviewTaskIds`), and abort triage specify/subagent sessions for that id. Handlers are per-task and idempotent. +- **Archive releases active-session locks (FN-7717)**: `task:moved` with `to === "archived"` (from ANY column, including in-progress via a direct single-hop `fn_task_archive`, and triage/planning where Plan Review and other workflow-step sessions run) now disposes in-flight session surfaces via `awaitAbortInFlightTaskWork` and sweeps any remaining `activeSessionRegistry` paths for the task, so a leaked lock can never survive archive and block a successor task's `registerPath` on the same session path. The `to === "archived"` check is ordered BEFORE the `from === "in-progress"` branch so a direct in-progress→archived move gets the same full cleanup instead of falling into the narrower in-progress-only branch. `to === "done"`/`"in-review"` are deliberately excluded — those columns legitimately hold `ai-merge`/`workspace-repo-land` merge leases. - **Soft-delete audit + column reconcile (FN-5175)**: `TaskStore.deleteTask` records a `runAuditEvents` row (`mutationType: "task:deleted"`, `domain: "database"`) inside the same transaction that sets `deletedAt`, and sets `"column" = 'archived'` on the row. Callers without a heartbeat run context (`fn task delete`, pi extension, dashboard delete route) pass an `auditContext` with `agentId: "system"` and a synthetic `runId`. The watcher cross-instance emit path does NOT re-record the audit event. The row stays in `tasks` (not `archivedTasks`); `archiveTask` is unchanged. - **Soft-delete resurrection guard (FN-5208)**: `TaskStore.readTaskJson()` must never fall back to `.fusion/tasks//task.json` when the DB row exists with `deletedAt` set — it throws `TaskDeletedError`. `atomicCreateTaskJson` / `atomicWriteTaskJson` / `atomicWriteTaskJsonWithAudit` refuse to upsert a task whose row is currently soft-deleted (unless the in-memory task carries `deletedAt` itself, for soft-delete maintenance paths), emit a `[soft-delete-resurrection-blocked]` log line, and record a `task:resurrection-blocked` run-audit event. Stale in-flight planner/triage writes for a soft-deleted ID surface `TaskDeletedError` and abort cleanly without emitting `task:created`. - **Exhausted in-review visibility surfaces (FN-5513/FN-6569)**: retry-exhausted merge failures (`column='in-review'`, `status='failed'`, `mergeRetries >= maxAutoMergeRetries`, default `3`) can remain soft-deleted for lifecycle safety, but are now intentionally discoverable through opt-in read paths: `TaskStore.listExhaustedInReviewTasks({ includeDeleted })`, `GET /api/tasks/exhausted-in-review`, `GET /api/tasks/:id?includeDeleted=true`, CLI `fn_task_show` soft-delete fallback marker, CLI `fn_task_list({ includeDeleted: true })`, and the dashboard ReliabilityView "Exhausted in-review (hidden blockers)" panel. This complements FN-5488/FN-5496 downstream blocker healing by surfacing the upstream blocker without mutating lifecycle state. 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 new file mode 100644 index 0000000000..b67931032c --- /dev/null +++ b/packages/engine/src/__tests__/executor-archive-releases-active-session.test.ts @@ -0,0 +1,167 @@ +/* +FNXC:WorkflowLifecycle 2026-07-09-00:00 (FN-7717 regression): +Archiving a task must release every activeSessionRegistry entry it holds. Plan Review and +other workflow-step / step-session sessions run while a task is in triage/planning/todo (not +in-progress), so the executor's task:moved handler previously only disposed session surfaces +via the `from === "in-progress"` branch — a task archived from any OTHER column leaked its +registry entry and blocked a successor task from registering the same session path with +ActiveSessionPathHeldByForeignTaskError (NEXT-508 -> NEXT-433). This suite proves the fix +across all three registration surfaces (executor / step-session / workflow-step), the +leaked-entry sweep path (no in-memory session) including when archiving DIRECTLY from +in-progress in a single task:moved hop (a branch-ordering gap the fix also closes), the +done/in-review merge-lease exclusion, and the no-op case (task with no held paths). +*/ +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 SHARED_ROOT = "/tmp/fusion-test-archive-shared-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 makeExecutor(): { executor: TaskExecutor; store: TaskStore & EventEmitter } { + const store = createStore(); + const executor = new TaskExecutor(store, SHARED_ROOT); + return { executor, store }; +} + +function makeTask(id: string): any { + return { id, column: "archived" }; +} + +describe("archiving a task releases its active-session registry entries (FN-7717)", () => { + beforeEach(() => activeSessionRegistry.clear()); + afterEach(() => activeSessionRegistry.clear()); + + 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. + (executor as any).setActiveWorkflowStepSession("TASK-A", {}, SHARED_ROOT); + expect(activeSessionRegistry.isPathActive(SHARED_ROOT)).toBe(true); + + // Drive the archive transition: to === "archived", from a NON-in-progress column + // (Plan Review runs in triage), exactly like archiveTask emits. + store.emit("task:moved", { task: makeTask("TASK-A"), from: "triage", to: "archived", source: "user" }); + + // 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.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" }), + ).not.toThrow(); + }); + + it("releases executor and step-session surfaces archived from planning/todo columns", async () => { + const { executor, store } = makeExecutor(); + + (executor as any).setActiveSession("TASK-C", { session: { dispose: vi.fn() } }, `${SHARED_ROOT}-c`); + (executor as any).setActiveStepExecutor("TASK-D", { terminateAllSessions: vi.fn().mockResolvedValue(undefined) }, `${SHARED_ROOT}-d`); + + store.emit("task:moved", { task: makeTask("TASK-C"), from: "planning", to: "archived", source: "user" }); + store.emit("task:moved", { task: makeTask("TASK-D"), from: "todo", to: "archived", source: "user" }); + + await Promise.all([ + (executor as any).pendingTaskDisposals.get("TASK-C"), + (executor as any).pendingTaskDisposals.get("TASK-D"), + ]); + + expect(activeSessionRegistry.pathsForTask("TASK-C")).toHaveLength(0); + expect(activeSessionRegistry.pathsForTask("TASK-D")).toHaveLength(0); + }); + + it("sweeps a leaked registry entry with no in-memory session on archive", async () => { + const { executor, store } = makeExecutor(); + + // Simulate a LEAKED entry: registered directly in the registry with no corresponding + // in-memory activeSessions/activeStepExecutors/activeWorkflowStepSessions entry, so the + // abort call itself finds nothing to dispose — only the sweep clears it. + activeSessionRegistry.registerPath(`${SHARED_ROOT}-leak`, { taskId: "TASK-E", kind: "workflow-step", ownerKey: "TASK-E#workflow-step" }); + expect(activeSessionRegistry.isPathActive(`${SHARED_ROOT}-leak`)).toBe(true); + + store.emit("task:moved", { task: makeTask("TASK-E"), from: "in-review", to: "archived", source: "user" }); + await (executor as any).pendingTaskDisposals.get("TASK-E"); + + expect(activeSessionRegistry.isPathActive(`${SHARED_ROOT}-leak`)).toBe(false); + expect(activeSessionRegistry.pathsForTask("TASK-E")).toHaveLength(0); + }); + + it("sweeps a leaked registry entry when a task is archived DIRECTLY from in-progress (single task:moved hop, no todo stop)", async () => { + const { executor, store } = makeExecutor(); + + // fn_task_archive can move a live in-progress task straight to archived in one + // `task:moved` event (from: "in-progress", to: "archived") with no intermediate + // stop in "todo". Before the branch-ordering fix, this hit the narrower + // `from === "in-progress"` branch first and skipped the archive-only leaked-entry + // sweep, so a registry entry with no matching in-memory session would survive. + activeSessionRegistry.registerPath(`${SHARED_ROOT}-inprogress-leak`, { taskId: "TASK-I", kind: "workflow-step", ownerKey: "TASK-I#workflow-step" }); + expect(activeSessionRegistry.isPathActive(`${SHARED_ROOT}-inprogress-leak`)).toBe(true); + + store.emit("task:moved", { task: makeTask("TASK-I"), from: "in-progress", to: "archived", source: "user" }); + await (executor as any).pendingTaskDisposals.get("TASK-I"); + + expect(activeSessionRegistry.isPathActive(`${SHARED_ROOT}-inprogress-leak`)).toBe(false); + expect(activeSessionRegistry.pathsForTask("TASK-I")).toHaveLength(0); + }); + + it("does NOT clear a held merge lease when a task moves to done or in-review", async () => { + const { executor, store } = makeExecutor(); + + activeSessionRegistry.registerPath(`${SHARED_ROOT}-merge-done`, { taskId: "TASK-F", kind: "ai-merge", ownerKey: "TASK-F#ai-merge" }); + activeSessionRegistry.registerPath(`${SHARED_ROOT}-merge-review`, { taskId: "TASK-G", kind: "workspace-repo-land", ownerKey: "TASK-G#workspace-repo-land" }); + + store.emit("task:moved", { task: makeTask("TASK-F"), from: "in-progress", to: "done", source: "engine" }); + store.emit("task:moved", { task: makeTask("TASK-G"), from: "in-progress", to: "in-review", source: "engine" }); + + // These moves go through the existing `from === "in-progress"` branch, which is + // unrelated to and does not fire the new archive-only sweep — the merge lease survives. + await Promise.resolve(); + + expect(activeSessionRegistry.isPathActive(`${SHARED_ROOT}-merge-done`)).toBe(true); + expect(activeSessionRegistry.isPathActive(`${SHARED_ROOT}-merge-review`)).toBe(true); + }); + + it("is a no-op that does not throw when archiving a task with no held registry paths", async () => { + const { executor, store } = makeExecutor(); + + expect(() => + store.emit("task:moved", { task: makeTask("TASK-H"), from: "triage", to: "archived", source: "user" }), + ).not.toThrow(); + + await (executor as any).pendingTaskDisposals.get("TASK-H"); + expect(activeSessionRegistry.pathsForTask("TASK-H")).toHaveLength(0); + }); + + 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); + + // 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" }), + ).toThrow(ActiveSessionPathHeldByForeignTaskError); + + store.emit("task:moved", { task: makeTask("NEXT-508"), from: "triage", to: "archived", source: "user" }); + await (executor as any).pendingTaskDisposals.get("NEXT-508"); + + // 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" }), + ).not.toThrow(); + }); +}); diff --git a/packages/engine/src/executor.ts b/packages/engine/src/executor.ts index 167bfcdd8f..8d9e0c3e7f 100644 --- a/packages/engine/src/executor.ts +++ b/packages/engine/src/executor.ts @@ -2793,6 +2793,41 @@ export class TaskExecutor { })().catch((err) => executorLog.error(`Failed to start ${task.id}:`, err), ); + } else if (to === "archived") { + /* + FNXC:WorkflowLifecycle 2026-07-09-00:05: + Archived is terminal, so it must release every active-session registry entry the + task holds. Plan Review / other workflow-step and step-session sessions run while + the task is in triage/planning/todo (not in-progress), so the old + `from === "in-progress"`-only disposal branch below never fired for them — the + registry entry (activeSessions / activeStepExecutors / activeWorkflowStepSessions, + keyed on the shared project browse root) leaked past archive and blocked a + successor task from acquiring the same session path with + ActiveSessionPathHeldByForeignTaskError (FN-7717 / NEXT-508 -> NEXT-433). We + deliberately do NOT do this for to === "done" / "in-review": those columns + legitimately hold ai-merge / workspace-repo-land merge leases that must survive + the transition (FN-6736 / Phase C/D merge-lease guarantees). + + This branch is checked BEFORE `from === "in-progress"` (and handles it too — a + task can be archived directly from in-progress via fn_task_archive, a single + `task:moved` event with no intermediate todo hop). Ordering the plain + `from === "in-progress"`-only branch first would let that direct + in-progress → archived transition fall into the narrower branch and skip the + leaked-entry sweep below, re-opening the exact class of leak this fix closes for + that one origin column. `awaitAbortInFlightTaskWork` here is the same call the + in-progress branch makes (superset of its cleanup), so no case regresses. + */ + this.trackTaskDisposal( + task.id, + this.awaitAbortInFlightTaskWork(task.id, "task archived").then(() => { + // Belt-and-suspenders sweep: clear any registry entry that survived the + // abort above because its in-memory session map was already empty + // (a leaked entry with no live session to abort). + for (const path of activeSessionRegistry.pathsForTask(task.id)) { + activeSessionRegistry.unregisterPath(path); + } + }), + ); } else if (from === "in-progress") { this.trackTaskDisposal( task.id,