fix: recover tasks stuck in review after pause
This commit is contained in:
@@ -3164,6 +3164,44 @@ describe("TaskExecutor global pause behavior", () => {
|
||||
expect(store.updateTask).not.toHaveBeenCalledWith("FN-001", { status: "failed" });
|
||||
});
|
||||
|
||||
it("finalizes to in-review when global pause hits after task_done", async () => {
|
||||
const store = createMockStore();
|
||||
|
||||
mockedCreateHaiAgent.mockImplementation(async (opts: any) => {
|
||||
const customTools = opts.customTools || [];
|
||||
return {
|
||||
session: {
|
||||
prompt: vi.fn().mockImplementation(async () => {
|
||||
const taskDoneTool = customTools.find((t: any) => t.name === "task_done");
|
||||
if (taskDoneTool) {
|
||||
await taskDoneTool.execute("tool-1", {});
|
||||
}
|
||||
store._trigger("settings:updated", {
|
||||
settings: { globalPause: true },
|
||||
previous: { globalPause: false },
|
||||
});
|
||||
throw new Error("Session terminated");
|
||||
}),
|
||||
dispose: vi.fn(),
|
||||
subscribe: vi.fn(),
|
||||
on: vi.fn(),
|
||||
sessionManager: { getLeafId: vi.fn().mockReturnValue("leaf-1") },
|
||||
state: {},
|
||||
},
|
||||
} as any;
|
||||
});
|
||||
|
||||
const executor = new TaskExecutor(store, "/tmp/test");
|
||||
await executor.execute({
|
||||
id: "FN-001", title: "Test", description: "T", column: "in-progress",
|
||||
dependencies: [], steps: [{ name: "Step 1", status: "pending" }], currentStep: 0, log: [],
|
||||
createdAt: new Date().toISOString(), updatedAt: new Date().toISOString(),
|
||||
});
|
||||
|
||||
expect(store.moveTask).toHaveBeenCalledWith("FN-001", "in-review");
|
||||
expect(store.moveTask).not.toHaveBeenCalledWith("FN-001", "todo");
|
||||
});
|
||||
|
||||
it("takes no action when globalPause remains false", async () => {
|
||||
const store = createMockStore();
|
||||
const disposeFn = vi.fn();
|
||||
|
||||
@@ -544,6 +544,12 @@ export class TaskExecutor {
|
||||
}
|
||||
}
|
||||
|
||||
private async shouldFinalizeCompletedTask(taskId: string, taskDone: boolean): Promise<boolean> {
|
||||
if (taskDone) return true;
|
||||
const task = await this.store.getTask(taskId);
|
||||
return this.isTaskWorkComplete(task);
|
||||
}
|
||||
|
||||
/**
|
||||
* Execute a review handoff: move the task to in-review column with
|
||||
* awaiting-user-review status, assign the requesting user, and dispose
|
||||
@@ -797,6 +803,7 @@ export class TaskExecutor {
|
||||
// the finally block so this.executing is cleared first (prevents re-dispatch race).
|
||||
// true = requeue to todo, false = budget exhausted (already marked failed).
|
||||
let stuckRequeue: boolean | null = null;
|
||||
let taskDone = false;
|
||||
|
||||
try {
|
||||
// Check dependencies
|
||||
@@ -1153,7 +1160,6 @@ export class TaskExecutor {
|
||||
// (block task_update status="done" until the agent re-reviews and gets APPROVE).
|
||||
const codeReviewVerdicts = new Map<number, ReviewVerdict>();
|
||||
|
||||
let taskDone = false;
|
||||
let wasPaused = false;
|
||||
// Mutable ref — populated after createKbAgent, tools access lazily via closure
|
||||
const sessionRef: { current: AgentSession | null } = { current: null };
|
||||
@@ -1375,9 +1381,16 @@ export class TaskExecutor {
|
||||
if (this.pausedAborted.has(task.id)) {
|
||||
this.pausedAborted.delete(task.id);
|
||||
wasPaused = true;
|
||||
executorLog.log(`${task.id} paused (graceful session exit) — moving to todo`);
|
||||
await this.store.logEntry(task.id, "Execution paused — session preserved for resume, moved to todo");
|
||||
await this.store.moveTask(task.id, "todo");
|
||||
if (await this.shouldFinalizeCompletedTask(task.id, taskDone)) {
|
||||
executorLog.log(`${task.id} paused after completion (graceful session exit) — finalizing to in-review`);
|
||||
await this.store.logEntry(task.id, "Execution paused after completion — finalizing to in-review");
|
||||
await this.store.moveTask(task.id, "in-review");
|
||||
this.options.onComplete?.(task);
|
||||
} else {
|
||||
executorLog.log(`${task.id} paused (graceful session exit) — moving to todo`);
|
||||
await this.store.logEntry(task.id, "Execution paused — session preserved for resume, moved to todo");
|
||||
await this.store.moveTask(task.id, "todo");
|
||||
}
|
||||
return;
|
||||
}
|
||||
|
||||
@@ -1576,19 +1589,26 @@ export class TaskExecutor {
|
||||
this.options.onComplete?.(task);
|
||||
} else if (this.pausedAborted.has(task.id)) {
|
||||
// Task was paused mid-execution — clean up worktree and move to todo
|
||||
executorLog.log(`${task.id} paused — moving to todo`);
|
||||
this.pausedAborted.delete(task.id);
|
||||
if (worktreePath && existsSync(worktreePath)) {
|
||||
try {
|
||||
execSync(`git worktree remove "${worktreePath}" --force`, { cwd: this.rootDir, stdio: "pipe" });
|
||||
executorLog.log(`Removed old worktree for paused task: ${worktreePath}`);
|
||||
} catch (cleanupErr: any) {
|
||||
executorLog.warn(`Failed to remove old worktree ${worktreePath}: ${cleanupErr.message}`);
|
||||
if (await this.shouldFinalizeCompletedTask(task.id, taskDone)) {
|
||||
executorLog.log(`${task.id} paused after completion — finalizing to in-review`);
|
||||
await this.store.logEntry(task.id, "Execution paused after completion — finalizing to in-review", undefined, this.currentRunContext);
|
||||
await this.store.moveTask(task.id, "in-review");
|
||||
this.options.onComplete?.(task);
|
||||
} else {
|
||||
executorLog.log(`${task.id} paused — moving to todo`);
|
||||
if (worktreePath && existsSync(worktreePath)) {
|
||||
try {
|
||||
execSync(`git worktree remove "${worktreePath}" --force`, { cwd: this.rootDir, stdio: "pipe" });
|
||||
executorLog.log(`Removed old worktree for paused task: ${worktreePath}`);
|
||||
} catch (cleanupErr: any) {
|
||||
executorLog.warn(`Failed to remove old worktree ${worktreePath}: ${cleanupErr.message}`);
|
||||
}
|
||||
}
|
||||
await this.store.updateTask(task.id, { worktree: undefined, branch: undefined });
|
||||
await this.store.logEntry(task.id, "Execution paused — agent terminated, moved to todo", undefined, this.currentRunContext);
|
||||
await this.store.moveTask(task.id, "todo");
|
||||
}
|
||||
await this.store.updateTask(task.id, { worktree: undefined, branch: undefined });
|
||||
await this.store.logEntry(task.id, "Execution paused — agent terminated, moved to todo", undefined, this.currentRunContext);
|
||||
await this.store.moveTask(task.id, "todo");
|
||||
} else if (this.stuckAborted.has(task.id)) {
|
||||
// Task was killed by stuck task detector — defer requeue to finally block
|
||||
// (after this.executing is cleared) to prevent re-dispatch race.
|
||||
|
||||
@@ -687,6 +687,70 @@ describe("SelfHealingManager", () => {
|
||||
});
|
||||
});
|
||||
|
||||
describe("recoverMergedReviewTasks", () => {
|
||||
it("moves merged in-review tasks to done and clears transient merge state", async () => {
|
||||
const managerWithRecovery = new SelfHealingManager(store, {
|
||||
rootDir: "/tmp/test-project",
|
||||
});
|
||||
|
||||
(store.listTasks as ReturnType<typeof vi.fn>).mockResolvedValue([
|
||||
{
|
||||
id: "FN-350",
|
||||
column: "in-review",
|
||||
status: "failed",
|
||||
error: "Invalid transition: 'todo' → 'done'. Valid targets: in-progress, triage",
|
||||
mergeRetries: 3,
|
||||
mergeDetails: {
|
||||
mergeConfirmed: true,
|
||||
mergedAt: "2026-01-01T00:00:00.000Z",
|
||||
},
|
||||
log: [],
|
||||
},
|
||||
]);
|
||||
|
||||
const result = await managerWithRecovery.recoverMergedReviewTasks();
|
||||
|
||||
expect(result).toBe(1);
|
||||
expect(store.updateTask).toHaveBeenCalledWith("FN-350", {
|
||||
status: null,
|
||||
error: null,
|
||||
mergeRetries: 0,
|
||||
});
|
||||
expect(store.moveTask).toHaveBeenCalledWith("FN-350", "done");
|
||||
expect(store.logEntry).toHaveBeenCalledWith(
|
||||
"FN-350",
|
||||
expect.stringContaining("merge already confirmed"),
|
||||
);
|
||||
|
||||
managerWithRecovery.stop();
|
||||
});
|
||||
|
||||
it("ignores in-review tasks without confirmed merge metadata", async () => {
|
||||
const managerWithRecovery = new SelfHealingManager(store, {
|
||||
rootDir: "/tmp/test-project",
|
||||
});
|
||||
|
||||
(store.listTasks as ReturnType<typeof vi.fn>).mockResolvedValue([
|
||||
{
|
||||
id: "FN-351",
|
||||
column: "in-review",
|
||||
mergeDetails: {
|
||||
mergeConfirmed: false,
|
||||
},
|
||||
log: [],
|
||||
},
|
||||
]);
|
||||
|
||||
const result = await managerWithRecovery.recoverMergedReviewTasks();
|
||||
|
||||
expect(result).toBe(0);
|
||||
expect(store.updateTask).not.toHaveBeenCalled();
|
||||
expect(store.moveTask).not.toHaveBeenCalled();
|
||||
|
||||
managerWithRecovery.stop();
|
||||
});
|
||||
});
|
||||
|
||||
describe("recoverOrphanedExecutions", () => {
|
||||
it("requeues in-progress tasks whose reserved worktree is missing", async () => {
|
||||
const getExecuting = vi.fn().mockReturnValue(new Set<string>());
|
||||
|
||||
@@ -325,6 +325,7 @@ export class SelfHealingManager {
|
||||
this.checkpointWal();
|
||||
await this.enforceWorktreeCap();
|
||||
await this.recoverCompletedTasks();
|
||||
await this.recoverMergedReviewTasks();
|
||||
await this.recoverMisclassifiedFailures();
|
||||
await this.recoverOrphanedExecutions();
|
||||
await this.recoverApprovedTriageTasks();
|
||||
@@ -387,6 +388,58 @@ export class SelfHealingManager {
|
||||
|
||||
// ── Misclassified failure recovery ───────────────────────────────
|
||||
|
||||
/**
|
||||
* Recover tasks that already merged successfully but never reached `done`.
|
||||
*
|
||||
* This catches races where the merge completed and merge metadata was stored,
|
||||
* but a later transition failed or another process moved the task before the
|
||||
* final `in-review` → `done` update completed.
|
||||
*
|
||||
* @returns Number of tasks recovered
|
||||
*/
|
||||
async recoverMergedReviewTasks(): Promise<number> {
|
||||
try {
|
||||
const tasks = await this.store.listTasks();
|
||||
|
||||
const mergedButNotDone = tasks.filter((t) =>
|
||||
t.column === "in-review" &&
|
||||
t.mergeDetails?.mergeConfirmed === true,
|
||||
);
|
||||
|
||||
if (mergedButNotDone.length === 0) return 0;
|
||||
|
||||
log.warn(`Found ${mergedButNotDone.length} merged task(s) stuck in in-review`);
|
||||
|
||||
let recovered = 0;
|
||||
for (const task of mergedButNotDone) {
|
||||
try {
|
||||
await this.store.updateTask(task.id, {
|
||||
status: null,
|
||||
error: null,
|
||||
mergeRetries: 0,
|
||||
});
|
||||
await this.store.moveTask(task.id, "done");
|
||||
await this.store.logEntry(
|
||||
task.id,
|
||||
"Auto-recovered: merge already confirmed — moved from in-review to done",
|
||||
);
|
||||
log.log(`Recovered merged task ${task.id}: moved to done`);
|
||||
recovered++;
|
||||
} catch (err: any) {
|
||||
log.error(`Failed to recover merged task ${task.id}: ${err.message}`);
|
||||
}
|
||||
}
|
||||
|
||||
if (recovered > 0) {
|
||||
log.log(`Recovered ${recovered} merged task(s) → done`);
|
||||
}
|
||||
return recovered;
|
||||
} catch (err: any) {
|
||||
log.error(`Merged review recovery failed: ${err.message}`);
|
||||
return 0;
|
||||
}
|
||||
}
|
||||
|
||||
/**
|
||||
* Recover tasks in `in-review` marked as `failed` where all steps are
|
||||
* actually done. This catches the case where an agent completed all work
|
||||
|
||||
Reference in New Issue
Block a user