From 9f45f108e59f3903d50a98c7aa678929890258ac Mon Sep 17 00:00:00 2001 From: gsxdsm Date: Sun, 28 Jun 2026 00:08:53 -0700 Subject: [PATCH] FN-7174: reset lost steps before stuck requeue cleanup Preserve durable stuck-requeue progress while preventing retries from skipping deleted uncommitted work. - Reconcile completed and in-progress steps before stuck cleanup removes executor worktrees. - Reset step progress when a branch has no unique commits or git proof fails before checkout deletion. - Cover normal, loop-timeout, and force stuck-requeue paths with regression tests and documentation. Files changed: .changeset/fn-7174-stuck-requeue-progress.md | 7 + docs/architecture.md | 2 +- packages/core/src/types.ts | 9 +- ...xecutor-stuck-requeue-preserve-progress.test.ts | 282 +++++++++++++++++++++ packages/engine/src/executor.ts | 99 ++++---- 5 files changed, 349 insertions(+), 50 deletions(-) Fusion-Task-Id: FN-7174 Fusion-Task-Lineage: 3b8b09e7-fceb-4b3d-af80-993a8842a51a Co-authored-by: Fusion (runfusion.ai) --- .changeset/fn-7174-stuck-requeue-progress.md | 7 + docs/architecture.md | 2 +- packages/core/src/types.ts | 9 +- ...or-stuck-requeue-preserve-progress.test.ts | 282 ++++++++++++++++++ packages/engine/src/executor.ts | 107 ++++--- 5 files changed, 353 insertions(+), 54 deletions(-) create mode 100644 .changeset/fn-7174-stuck-requeue-progress.md create mode 100644 packages/engine/src/__tests__/executor-stuck-requeue-preserve-progress.test.ts diff --git a/.changeset/fn-7174-stuck-requeue-progress.md b/.changeset/fn-7174-stuck-requeue-progress.md new file mode 100644 index 0000000000..a57c54af62 --- /dev/null +++ b/.changeset/fn-7174-stuck-requeue-progress.md @@ -0,0 +1,7 @@ +--- +"@runfusion/fusion": patch +--- + +summary: Stuck re-queue no longer loses uncommitted work while keeping steps marked complete. +category: fix +dev: Reconciles lost-work steps before worktree removal across all three executor stuck-requeue paths; corrects the preserveProgressOnStuckRequeue docstring. diff --git a/docs/architecture.md b/docs/architecture.md index 4b1bc22ce4..cd17c8621e 100644 --- a/docs/architecture.md +++ b/docs/architecture.md @@ -694,7 +694,7 @@ Planning-phase stuck kills use the same `stuckKillCount` / `settings.maxStuckKil Active `fn_run_verification` subprocesses are a bounded progress signal (FN-6598). `createRunVerificationTool()` brackets each command with `StuckTaskDetector.beginVerification()` / `endVerification()`; while the command is active and still inside its own timeout plus cleanup grace, the detector suppresses `loop` and `no-progress-churn` classification so healthy marathon verification output cannot consume stuck-kill budget. `inactivity` is not suppressed: the verification runner must continue emitting line output or synthetic heartbeats, and if the process overruns its recorded deadline or never sends an end signal, normal detection resumes. -If loop recovery times out during compact-and-resume and the executor does not unwind within the bounded force-requeue grace window, `TaskExecutor.markStuckAborted()` now hard-cancels the hung task before clearing execution guards: spawned child agents are terminated, `awaitAbortInFlightTaskWork()` reaps API/step/workflow/configured-command/subagent/CLI surfaces, the task worktree is removed with `RemovalReason.ExecutorStuckKilled`, stale in-memory worktree/loop/paused/stuck state is cleared, and then the task is moved back to `todo` with the configured `preserveProgressOnStuckRequeue` semantics. The path preserves the concurrent-recovery guard: if the latest task column is no longer `in-progress`, it only clears the execution guard and does not reap/remove resources that a self-healing recovery now owns. Task logs distinguish loop detection, compaction timeout, force-kill cleanup start, force-requeue, and cleanup completion/failure. +If loop recovery times out during compact-and-resume and the executor does not unwind within the bounded force-requeue grace window, `TaskExecutor.markStuckAborted()` now hard-cancels the hung task before clearing execution guards: spawned child agents are terminated, `awaitAbortInFlightTaskWork()` reaps API/step/workflow/configured-command/subagent/CLI surfaces, completed/in-progress steps are reconciled against committed branch state before any checkout deletion, the task worktree is removed with `RemovalReason.ExecutorStuckKilled`, stale in-memory worktree/loop/paused/stuck state is cleared, and then the task is moved back to `todo` with the configured `preserveProgressOnStuckRequeue` semantics. With preserve-progress enabled, committed step progress is retained; when the branch has no unique commits, affected steps are reset to `pending` before the worktree/branch are cleared so a retry cannot skip deleted uncommitted-only work. The path preserves the concurrent-recovery guard: if the latest task column is no longer `in-progress`, it only clears the execution guard and does not reap/remove resources that a self-healing recovery now owns. Task logs distinguish loop detection, compaction timeout, force-kill cleanup start, force-requeue, and cleanup completion/failure. - `recoverMissingWorktreeReviewFailures()` is a narrow failed-review recovery: only `status: "failed"` `in-review` tasks with the explicit session-start signature `Refusing to start coding agent in missing worktree:` (from `assertValidWorktreeSession()`) are requeued. Recovery clears stale session metadata (`worktree`, `branch`, `sessionFile`, transient failure state), preserves valid step progress/retry counters, logs the auto-recovery reason, and moves the task back to `todo` for a clean retry. - `recoverMergeableReviewTasks()` only re-enqueues truly eligible tasks; retry-exhausted review tasks are skipped to avoid re-enqueue/no-op loops that keep refreshing `updatedAt`. - `recoverAlreadyMergedReviewTasks()` auto-finalizes retry-exhausted `in-review` tasks when self-healing can prove their work already landed on the merge target. On this landed-content path it clears soft blockers (`paused`, stale `status: "failed"`, and residual `error`) before moving to `done`; true hard blockers (for example incomplete steps, awaiting-user-review, or failed pre-merge workflow steps) still park the task in stable `in-review/failed` state with a blocker error instead of entering an auto-finalize loop. diff --git a/packages/core/src/types.ts b/packages/core/src/types.ts index efd9801693..3da5312f9e 100644 --- a/packages/core/src/types.ts +++ b/packages/core/src/types.ts @@ -4118,10 +4118,11 @@ export interface ProjectSettings { * Default: 25. */ maxTotalRetriesBeforeFail?: number; /** When the stuck-task detector kills and re-queues a task, preserve the - * task's step progress (step statuses + currentStep) instead of resetting - * every step to `pending`. The worktree and branch are still cleared so - * the retry gets a fresh checkout, but completed steps stay completed so - * the agent can resume from where it left off. Default: true. */ + * task's recoverable step progress (step statuses + currentStep) instead + * of resetting every step to `pending`. Before clearing the worktree/branch + * for a fresh checkout, stuck-requeue cleanup resets completed/in-progress + * steps to `pending` if the branch has no unique commits, preventing deleted + * uncommitted-only work from being skipped on retry. Default: true. */ preserveProgressOnStuckRequeue?: boolean; /** Maximum number of times the self-healing manager may auto-revive a task parked * in `in-review` with a failed pre-merge workflow step. Also bounds the inline diff --git a/packages/engine/src/__tests__/executor-stuck-requeue-preserve-progress.test.ts b/packages/engine/src/__tests__/executor-stuck-requeue-preserve-progress.test.ts new file mode 100644 index 0000000000..5d99431de2 --- /dev/null +++ b/packages/engine/src/__tests__/executor-stuck-requeue-preserve-progress.test.ts @@ -0,0 +1,282 @@ +import { beforeEach, describe, expect, it, vi } from "vitest"; +import "./executor-test-helpers.js"; +import type { Task } from "@fusion/core"; +import { TaskExecutor } from "../executor.js"; +import { removeWorktree } from "../worktree-pool.js"; +import { + createMockStore, + mockCleanup, + mockExecuteAll, + mockedCreateFnAgent, + mockedDescribeRegisteredWorktrees, + mockedExecSync, + resetExecutorMocks, +} from "./executor-test-helpers.js"; + +const mockedRemoveWorktree = vi.mocked(removeWorktree); + +function createTask(overrides: Partial = {}): Task { + return { + id: "FN-7174", + title: "Preserve stuck progress", + description: "# test\n## Steps\n### Step 0: Preflight\n- [ ] check\n### Step 1: Implement\n- [ ] code", + prompt: "# test\n## Steps\n### Step 0: Preflight\n- [ ] check\n### Step 1: Implement\n- [ ] code", + column: "in-progress", + dependencies: [], + steps: [ + { name: "Step 0", status: "done" }, + { name: "Step 1", status: "in-progress" }, + { name: "Step 2", status: "pending" }, + ], + currentStep: 2, + log: [], + createdAt: new Date().toISOString(), + updatedAt: new Date().toISOString(), + worktree: "/tmp/test/.worktrees/fn-7174-worktree", + branch: "fusion/fn-7174", + baseCommitSha: "base-sha", + enabledWorkflowSteps: [], + ...overrides, + }; +} + +function installGitResult(kind: "uncommitted-only" | "committed") { + mockedExecSync.mockImplementation((cmd: string) => { + if (cmd.includes("git rev-parse --is-inside-work-tree")) return "true\n"; + if (cmd.includes("git merge-base")) return "base-sha\n"; + if (cmd.includes("git rev-parse")) { + return kind === "uncommitted-only" ? "base-sha\n" : "branch-sha\n"; + } + return ""; + }); +} + +function installGitProofFailure() { + mockedExecSync.mockImplementation((cmd: string) => { + if (cmd.includes("git rev-parse --is-inside-work-tree")) return "true\n"; + if (cmd.includes("git merge-base")) throw new Error("fatal: not a valid object name fusion/fn-7174"); + return ""; + }); +} + +function createMutableStore(task: Task, settings: Record = {}) { + const store = createMockStore(); + store.getSettings.mockResolvedValue({ + maxConcurrent: 2, + maxWorktrees: 4, + pollIntervalMs: 15000, + groupOverlappingFiles: false, + autoMerge: false, + ...settings, + }); + store.getTask.mockImplementation(async () => task); + store.updateStep.mockImplementation(async (_taskId: string, stepIndex: number, status: Task["steps"][number]["status"]) => { + task.steps[stepIndex].status = status; + return task; + }); + store.updateTask.mockImplementation(async (_taskId: string, updates: Partial) => { + Object.assign(task, updates); + return task; + }); + store.moveTask.mockImplementation(async (_taskId: string, column: Task["column"]) => { + task.column = column; + return task; + }); + return store; +} + +function installSingleSession(resolvePrompt: () => Promise | void = async () => {}) { + let started!: () => void; + const startedPromise = new Promise((resolve) => { + started = resolve; + }); + const session = { + prompt: vi.fn().mockImplementation(async () => { + started(); + await resolvePrompt(); + }), + dispose: vi.fn(), + subscribe: vi.fn(), + on: vi.fn(), + setThinkingLevel: vi.fn(), + sessionManager: { getLeafId: vi.fn().mockReturnValue("leaf-1") }, + getSessionStats: vi.fn().mockReturnValue({ tokens: {} }), + }; + mockedCreateFnAgent.mockResolvedValue({ session, sessionFile: "/tmp/session.json" } as any); + return { session, startedPromise }; +} + +async function runSingleSessionStuckRequeue(task: Task, settings: Record = {}) { + const store = createMutableStore(task, settings); + let releasePrompt!: () => void; + const promptRelease = new Promise((resolve) => { + releasePrompt = resolve; + }); + const { startedPromise } = installSingleSession(() => promptRelease); + const executor = new TaskExecutor(store as any, "/tmp/test", {}); + + const executePromise = executor.execute(task); + await startedPromise; + executor.markStuckAborted(task.id, true); + releasePrompt(); + await executePromise; + return { store, executor }; +} + +async function runStepSessionStuckRequeue(task: Task, settings: Record = {}) { + const store = createMutableStore(task, { + runStepsInNewSessions: true, + maxParallelSteps: 2, + ...settings, + }); + let release!: () => void; + mockExecuteAll.mockReturnValue(new Promise((resolve) => { + release = resolve; + })); + const executor = new TaskExecutor(store as any, "/tmp/test", {}); + + const executePromise = executor.execute(task); + await vi.waitFor(() => expect((executor as any).activeStepExecutors.has(task.id)).toBe(true)); + executor.markStuckAborted(task.id, true); + release(); + await executePromise; + return { store, executor }; +} + +describe("TaskExecutor stuck requeue preserve-progress reconciliation", () => { + beforeEach(() => { + resetExecutorMocks(); + mockedRemoveWorktree.mockResolvedValue(undefined as any); + mockedDescribeRegisteredWorktrees.mockResolvedValue({ + rawOutput: "worktree /tmp/test/.worktrees/fn-7174-worktree\nbranch refs/heads/fusion/fn-7174\n", + canonicalized: ["/tmp/test/.worktrees/fn-7174-worktree"], + }); + mockCleanup.mockResolvedValue(undefined); + installGitResult("uncommitted-only"); + }); + + it("reproduces the default preserve-progress corruption case and resets uncommitted-only steps before removing the worktree", async () => { + const task = createTask(); + const { store } = await runSingleSessionStuckRequeue(task); + + expect(task.steps.map((step) => step.status)).toEqual(["pending", "pending", "pending"]); + expect(task.currentStep).toBe(0); + expect(store.updateStep).toHaveBeenCalledTimes(2); + expect(mockedRemoveWorktree).toHaveBeenCalledWith(expect.objectContaining({ + worktreePath: "/tmp/test/.worktrees/fn-7174-worktree", + taskId: task.id, + expectedOwnerTaskId: task.id, + })); + expect(store.updateTask).toHaveBeenCalledWith(task.id, expect.objectContaining({ + worktree: null, + branch: null, + })); + expect(store.moveTask).toHaveBeenCalledWith(task.id, "todo", { preserveProgress: true }); + }); + + it("resets steps when git cannot prove a stale branch has durable commits before cleanup", async () => { + installGitProofFailure(); + const task = createTask({ branch: "fusion/missing-fn-7174" }); + const { store } = await runSingleSessionStuckRequeue(task); + + expect(task.steps.map((step) => step.status)).toEqual(["pending", "pending", "pending"]); + expect(task.currentStep).toBe(0); + expect(store.updateStep).toHaveBeenCalledTimes(2); + expect(mockedRemoveWorktree).toHaveBeenCalled(); + expect(store.moveTask).toHaveBeenCalledWith(task.id, "todo", { preserveProgress: true }); + }); + + it("keeps committed step progress unchanged on preserve-progress stuck requeue", async () => { + installGitResult("committed"); + const task = createTask(); + const { store } = await runSingleSessionStuckRequeue(task); + + expect(task.steps.map((step) => step.status)).toEqual(["done", "in-progress", "pending"]); + expect(task.currentStep).toBe(2); + expect(store.updateStep).not.toHaveBeenCalled(); + expect(mockedRemoveWorktree).toHaveBeenCalled(); + expect(store.moveTask).toHaveBeenCalledWith(task.id, "todo", { preserveProgress: true }); + }); + + it("keeps preserveProgress=false reset behavior while moving without preserve options", async () => { + const task = createTask(); + const { store } = await runSingleSessionStuckRequeue(task, { preserveProgressOnStuckRequeue: false }); + + expect(task.steps.map((step) => step.status)).toEqual(["pending", "pending", "pending"]); + expect(task.currentStep).toBe(0); + expect(store.moveTask).toHaveBeenCalledWith(task.id, "todo", undefined); + }); + + it("does nothing for no-work tasks with no completed or in-progress steps", async () => { + const task = createTask({ + steps: [ + { name: "Step 0", status: "pending" }, + { name: "Step 1", status: "pending" }, + ], + currentStep: 0, + }); + const { store } = await runSingleSessionStuckRequeue(task); + + expect(store.updateStep).not.toHaveBeenCalled(); + expect(task.steps.map((step) => step.status)).toEqual(["pending", "pending"]); + expect(store.moveTask).toHaveBeenCalledWith(task.id, "todo", { preserveProgress: true }); + }); + + it("preserves the concurrent-recovery guard without removing worktree or moving to todo", async () => { + const task = createTask({ column: "in-review" }); + const store = createMutableStore(task); + let releasePrompt!: () => void; + const promptRelease = new Promise((resolve) => { + releasePrompt = resolve; + }); + const { startedPromise } = installSingleSession(() => promptRelease); + const executor = new TaskExecutor(store as any, "/tmp/test", {}); + + const executePromise = executor.execute({ ...task, column: "in-progress" }); + await startedPromise; + executor.markStuckAborted(task.id, true); + releasePrompt(); + await executePromise; + + expect(store.updateStep).not.toHaveBeenCalled(); + expect(mockedRemoveWorktree).not.toHaveBeenCalled(); + expect(store.moveTask).not.toHaveBeenCalledWith(task.id, "todo", expect.anything()); + expect(store.moveTask).not.toHaveBeenCalledWith(task.id, "todo"); + }); + + it("applies the same lost-work reconciliation to the step-session requeue path", async () => { + const task = createTask(); + const { store } = await runStepSessionStuckRequeue(task); + + expect(task.steps.map((step) => step.status)).toEqual(["pending", "pending", "pending"]); + expect(task.currentStep).toBe(0); + expect(store.updateStep).toHaveBeenCalledTimes(2); + expect(mockedRemoveWorktree).toHaveBeenCalled(); + expect(store.moveTask).toHaveBeenCalledWith(task.id, "todo", { preserveProgress: true }); + }); + + it("applies the same lost-work reconciliation to the force-requeue grace-timeout path", async () => { + vi.useFakeTimers(); + const task = createTask(); + const store = createMutableStore(task); + let releasePrompt!: () => void; + const promptRelease = new Promise((resolve) => { + releasePrompt = resolve; + }); + const { startedPromise } = installSingleSession(() => promptRelease); + const executor = new TaskExecutor(store as any, "/tmp/test", {}); + + const executePromise = executor.execute(task); + await startedPromise; + executor.markStuckAborted(task.id, true); + await vi.advanceTimersByTimeAsync(60_000); + + expect(task.steps.map((step) => step.status)).toEqual(["pending", "pending", "pending"]); + expect(task.currentStep).toBe(0); + expect(mockedRemoveWorktree).toHaveBeenCalled(); + expect(store.moveTask).toHaveBeenCalledWith(task.id, "todo", { preserveProgress: true }); + + releasePrompt(); + await executePromise; + }); +}); diff --git a/packages/engine/src/executor.ts b/packages/engine/src/executor.ts index f872de1fc1..64dda6cd7a 100644 --- a/packages/engine/src/executor.ts +++ b/packages/engine/src/executor.ts @@ -8460,9 +8460,11 @@ export class TaskExecutor { const settings = await this.store.getSettings(); const preserveProgress = settings.preserveProgressOnStuckRequeue !== false; - if (!preserveProgress) { - await this.resetStepsIfWorkLost(latestTask); - } + /* + FNXC:StuckRequeue 2026-06-27-23:15: + Stuck requeue may destroy a checkout that contains only uncommitted step output. Always reconcile lost-work step state before worktree removal, even when preserve-progress is enabled, so a retry cannot skip code that no longer exists. + */ + await this.resetStepsIfWorkLost(latestTask); if (worktreePath && existsSync(worktreePath)) { try { @@ -10195,13 +10197,11 @@ export class TaskExecutor { const settings = await this.store.getSettings(); const preserveProgress = settings.preserveProgressOnStuckRequeue !== false; - // Reset steps whose work was never committed before destroying - // the worktree. Skipped when preserveProgress is on — the - // setting's whole point is to keep step status across the - // requeue so the agent can resume from where it left off. - if (!preserveProgress) { - await this.resetStepsIfWorkLost(latestTask); - } + /* + FNXC:StuckRequeue 2026-06-27-23:15: + Preserve-progress stuck requeues still remove the old checkout. Reconcile steps first so uncommitted-only output is reset to pending while committed progress can remain complete. + */ + await this.resetStepsIfWorkLost(latestTask); // Clean up the old worktree so the retry gets a fresh one if (worktreePath && existsSync(worktreePath)) { @@ -15259,48 +15259,55 @@ You have access to the file system to review changes.${verdictBlock}`; const branchHead = branchHeadStdout.trim(); if (mergeBase === branchHead) { - // Branch has no unique commits — all step work was lost - executorLog.warn( - `${task.id} branch has no unique commits — resetting ${completedSteps.length} step(s) to pending`, - ); - - for (let i = 0; i < task.steps.length; i++) { - if (task.steps[i].status === "done" || task.steps[i].status === "in-progress") { - await this.store.updateStep(task.id, i, "pending"); - } - } - - const refreshedTask = await this.store.getTask(task.id); - const prevCurrentStep = refreshedTask.currentStep; - if (refreshedTask.steps.length > 0) { - const firstPendingStep = refreshedTask.steps.findIndex((s) => s.status === "pending"); - const newCurrentStep = firstPendingStep >= 0 ? firstPendingStep : 0; - if (newCurrentStep !== prevCurrentStep) { - await this.store.updateTask(task.id, { currentStep: newCurrentStep }); - executorLog.log( - `${task.id}: reset currentStep to ${newCurrentStep} after lost-work reset (was ${prevCurrentStep})`, - ); - await this.store.logEntry( - task.id, - `Reset currentStep to ${newCurrentStep} after lost-work step reset (was ${prevCurrentStep})`, - ); - } - } - - await this.store.logEntry( - task.id, - `Reset ${completedSteps.length} step(s) to pending — branch had no commits (uncommitted work lost with worktree)`, - ); + await this.resetLostWorkStepProgress(task, completedSteps.length, "branch had no commits"); } } catch (err: unknown) { const msg = err instanceof Error ? err.message : String(err); - executorLog.warn(`${task.id}: step-reset-on-work-lost failed (non-fatal, steps keep current status): ${msg}`); - // Branch may not exist or git commands may fail — non-fatal. - // Steps keep their current status (safe default: agent can - // inspect the worktree and decide). + executorLog.warn( + `${task.id}: unable to prove surviving branch commits before worktree removal — resetting ${completedSteps.length} step(s) to pending: ${msg}`, + ); + /* + FNXC:StuckRequeue 2026-06-27-23:55: + Stuck-requeue cleanup is about to delete the checkout. If git cannot prove the branch has durable commits, treat completed/in-progress steps as lost work rather than preserving progress that may point at deleted uncommitted output. + */ + await this.resetLostWorkStepProgress(task, completedSteps.length, `git proof failed: ${msg}`); } } + private async resetLostWorkStepProgress(task: Task, completedStepCount: number, reason: string): Promise { + executorLog.warn( + `${task.id} ${reason} — resetting ${completedStepCount} step(s) to pending`, + ); + + for (let i = 0; i < task.steps.length; i++) { + if (task.steps[i].status === "done" || task.steps[i].status === "in-progress") { + await this.store.updateStep(task.id, i, "pending"); + } + } + + const refreshedTask = await this.store.getTask(task.id); + const prevCurrentStep = refreshedTask.currentStep; + if (refreshedTask.steps.length > 0) { + const firstPendingStep = refreshedTask.steps.findIndex((s) => s.status === "pending"); + const newCurrentStep = firstPendingStep >= 0 ? firstPendingStep : 0; + if (newCurrentStep !== prevCurrentStep) { + await this.store.updateTask(task.id, { currentStep: newCurrentStep }); + executorLog.log( + `${task.id}: reset currentStep to ${newCurrentStep} after lost-work reset (was ${prevCurrentStep})`, + ); + await this.store.logEntry( + task.id, + `Reset currentStep to ${newCurrentStep} after lost-work step reset (was ${prevCurrentStep})`, + ); + } + } + + await this.store.logEntry( + task.id, + `Reset ${completedStepCount} step(s) to pending — ${reason} (uncommitted work lost with worktree)`, + ); + } + /** * Mark a task as stuck-aborted so the executor's error handling * knows not to treat the disposed session as a genuine failure. @@ -15389,9 +15396,11 @@ You have access to the file system to review changes.${verdictBlock}`; // clear it to prevent a later subprocess unwind from logging/moving as a pause. this.clearPausedAborted(taskId); - if (!preserveProgress) { - await this.resetStepsIfWorkLost(latestTask); - } + /* + FNXC:StuckRequeue 2026-06-27-23:15: + The force path mirrors normal stuck-requeue cleanup: before reaping a hung executor's worktree, reconcile step progress against committed branch state so preserved progress never points at deleted uncommitted work. + */ + await this.resetStepsIfWorkLost(latestTask); let cleanupFailed = false; if (worktreePath && existsSync(worktreePath)) {