fix(FN-1001): fix retry race condition in executor and scheduler
- Executor skips redundant moveTask call on manual retry (was causing race with scheduler) - Scheduler now triggers scheduling on todo column transitions to pick up retried tasks - Add tests for retry race condition fix covering both executor and scheduler paths
This commit is contained in:
@@ -4842,6 +4842,41 @@ describe("TaskExecutor bounded recovery retries", () => {
|
||||
);
|
||||
});
|
||||
|
||||
it("skips moveTask when task is already in todo at execute start", async () => {
|
||||
const store = createMockStore();
|
||||
const executor = new TaskExecutor(store, "/tmp/test", {});
|
||||
|
||||
mockedCreateHaiAgent.mockImplementation(async () => ({
|
||||
session: {
|
||||
prompt: vi.fn(async () => {
|
||||
// Simulate stuck kill
|
||||
executor.markStuckAborted("FN-001", true);
|
||||
throw new Error("Stuck task");
|
||||
}),
|
||||
dispose: vi.fn(),
|
||||
state: {},
|
||||
},
|
||||
}) as any);
|
||||
|
||||
await executor.execute({
|
||||
id: "FN-001",
|
||||
title: "Test",
|
||||
description: "Test",
|
||||
column: "todo", // Task was already in todo when execute started
|
||||
dependencies: [],
|
||||
steps: [],
|
||||
currentStep: 0,
|
||||
log: [],
|
||||
createdAt: new Date().toISOString(),
|
||||
updatedAt: new Date().toISOString(),
|
||||
});
|
||||
|
||||
// Should NOT call moveTask because task is already in todo
|
||||
expect(store.moveTask).not.toHaveBeenCalledWith("FN-001", "todo");
|
||||
// Should still clean up and mark as stuck-killed
|
||||
expect(store.updateTask).toHaveBeenCalledWith("FN-001", { status: "stuck-killed" });
|
||||
});
|
||||
|
||||
it("clears recovery metadata after successful run completes", async () => {
|
||||
const store = createMockStore();
|
||||
|
||||
|
||||
@@ -1198,8 +1198,15 @@ export class TaskExecutor {
|
||||
}
|
||||
}
|
||||
await this.store.updateTask(task.id, { status: "stuck-killed", worktree: undefined, branch: undefined });
|
||||
await this.store.moveTask(task.id, "todo");
|
||||
executorLog.log(`${task.id} moved to todo for retry after stuck kill`);
|
||||
// Only move to todo if not already there. The task.column check uses the
|
||||
// captured task object from execute() start — if the task was already in "todo"
|
||||
// when execute() started (e.g., resumed orphan), we skip the redundant move.
|
||||
if (task.column !== "todo") {
|
||||
await this.store.moveTask(task.id, "todo");
|
||||
executorLog.log(`${task.id} moved to todo for retry after stuck kill`);
|
||||
} else {
|
||||
executorLog.log(`${task.id} already in todo — skipping redundant move`);
|
||||
}
|
||||
} catch (err: any) {
|
||||
executorLog.error(`Failed to requeue stuck task ${task.id}: ${err.message}`);
|
||||
}
|
||||
|
||||
@@ -285,6 +285,47 @@ describe("Scheduler", () => {
|
||||
// So moveTask won't be called for a task already in in-progress
|
||||
expect(store.moveTask).not.toHaveBeenCalled();
|
||||
});
|
||||
|
||||
it("triggers scheduling when task moves to todo (retry)", async () => {
|
||||
// Mock filesystem validation so schedule() can proceed
|
||||
vi.mocked(existsSync).mockReturnValue(true);
|
||||
vi.mocked(readFile).mockResolvedValue("# Task\nDo something");
|
||||
|
||||
// Return FN-001 in todo with satisfied deps
|
||||
const listTasksMock = vi.fn()
|
||||
.mockResolvedValueOnce([]) // Initial schedule from start()
|
||||
.mockResolvedValueOnce([
|
||||
createMockTask({ id: "FN-001", column: "todo", dependencies: [] }),
|
||||
]);
|
||||
|
||||
const store = createMockStore({
|
||||
listTasks: listTasksMock,
|
||||
getSettings: vi.fn().mockResolvedValue({ maxConcurrent: 2, maxWorktrees: 4 }),
|
||||
updateTask: vi.fn().mockResolvedValue(undefined),
|
||||
moveTask: vi.fn().mockResolvedValue(undefined),
|
||||
});
|
||||
|
||||
const scheduler = new Scheduler(store);
|
||||
scheduler.start();
|
||||
|
||||
// Wait for initial schedule pass to complete
|
||||
await new Promise((r) => setTimeout(r, 10));
|
||||
|
||||
// Find and call the task:moved handler
|
||||
const onCalls = (store.on as any).mock.calls;
|
||||
const movedHandler = onCalls.find((call: any) => call[0] === "task:moved")?.[1];
|
||||
expect(movedHandler).toBeDefined();
|
||||
|
||||
// Simulate task:moved to todo (retry scenario)
|
||||
const todoTask = createMockTask({ id: "FN-001", column: "in-progress" });
|
||||
await movedHandler({ task: todoTask, from: "in-progress", to: "todo" });
|
||||
|
||||
// Wait for async schedule to complete
|
||||
await new Promise((r) => setTimeout(r, 10));
|
||||
|
||||
// Verify schedule() was called — task in todo should be scheduled
|
||||
expect(store.moveTask).toHaveBeenCalledWith("FN-001", "in-progress");
|
||||
});
|
||||
});
|
||||
|
||||
describe("task unpause scheduling", () => {
|
||||
|
||||
@@ -191,11 +191,11 @@ export class Scheduler {
|
||||
void this.handleMissionTaskCompletion(task.id, task.sliceId);
|
||||
}
|
||||
|
||||
// Event-driven scheduling: when a dependency completes (task moves to "done"),
|
||||
// Event-driven scheduling: when a task moves to "done" (completion) or "todo" (retry/manual move),
|
||||
// trigger scheduling immediately so waiting tasks can start without waiting
|
||||
// for the next poll interval (up to 15 seconds).
|
||||
if (to === "done") {
|
||||
schedulerLog.log("Task completed — triggering scheduling");
|
||||
if (to === "done" || to === "todo") {
|
||||
schedulerLog.log(`Task moved to ${to} — triggering scheduling`);
|
||||
this.schedule();
|
||||
}
|
||||
});
|
||||
|
||||
Reference in New Issue
Block a user