fix: reserve and clear task worktrees correctly
This commit is contained in:
5
.changeset/reserve-worktree-before-in-progress.md
Normal file
5
.changeset/reserve-worktree-before-in-progress.md
Normal file
@@ -0,0 +1,5 @@
|
|||||||
|
---
|
||||||
|
"@gsxdsm/fusion": patch
|
||||||
|
---
|
||||||
|
|
||||||
|
Reserve a task worktree path before moving it into in-progress so active tasks do not appear without an assigned worktree.
|
||||||
@@ -2971,6 +2971,34 @@ Task with acceptance criteria
|
|||||||
expect(duplicated.status).toBeUndefined();
|
expect(duplicated.status).toBeUndefined();
|
||||||
});
|
});
|
||||||
|
|
||||||
|
it("clears nullable execution fields via updateTask(null)", async () => {
|
||||||
|
const task = await store.createTask({ description: "Test clear nullable execution fields", column: "todo" });
|
||||||
|
await store.updateTask(task.id, {
|
||||||
|
worktree: "/some/path",
|
||||||
|
branch: "fusion/fn-001",
|
||||||
|
baseBranch: "main",
|
||||||
|
baseCommitSha: "abc123",
|
||||||
|
status: "executing",
|
||||||
|
error: "boom",
|
||||||
|
});
|
||||||
|
|
||||||
|
const updated = await store.updateTask(task.id, {
|
||||||
|
worktree: null,
|
||||||
|
branch: null,
|
||||||
|
baseBranch: null,
|
||||||
|
baseCommitSha: null,
|
||||||
|
status: null,
|
||||||
|
error: null,
|
||||||
|
});
|
||||||
|
|
||||||
|
expect(updated.worktree).toBeUndefined();
|
||||||
|
expect(updated.branch).toBeUndefined();
|
||||||
|
expect(updated.baseBranch).toBeUndefined();
|
||||||
|
expect(updated.baseCommitSha).toBeUndefined();
|
||||||
|
expect(updated.status).toBeUndefined();
|
||||||
|
expect(updated.error).toBeUndefined();
|
||||||
|
});
|
||||||
|
|
||||||
it("does NOT copy dependencies", async () => {
|
it("does NOT copy dependencies", async () => {
|
||||||
const dep = await store.createTask({ description: "Dependency" });
|
const dep = await store.createTask({ description: "Dependency" });
|
||||||
const task = await store.createTask({ description: "Test task", dependencies: [dep.id] });
|
const task = await store.createTask({ description: "Test task", dependencies: [dep.id] });
|
||||||
|
|||||||
@@ -1016,7 +1016,7 @@ export class TaskStore extends EventEmitter<TaskStoreEvents> {
|
|||||||
|
|
||||||
async updateTask(
|
async updateTask(
|
||||||
id: string,
|
id: string,
|
||||||
updates: { title?: string; description?: string; prompt?: string; worktree?: string; status?: string | null; dependencies?: string[]; blockedBy?: string | null; paused?: boolean; baseBranch?: string; branch?: string; baseCommitSha?: string; size?: "S" | "M" | "L"; reviewLevel?: number; mergeRetries?: number; stuckKillCount?: number | null; recoveryRetryCount?: number | null; nextRecoveryAt?: string | null; modelProvider?: string | null; modelId?: string | null; validatorModelProvider?: string | null; validatorModelId?: string | null; error?: string | null; summary?: string | null; sessionFile?: string | null; workflowStepResults?: import("./types.js").WorkflowStepResult[] | null; modifiedFiles?: string[] | null; missionId?: string | null; sliceId?: string | null },
|
updates: { title?: string; description?: string; prompt?: string; worktree?: string | null; status?: string | null; dependencies?: string[]; blockedBy?: string | null; paused?: boolean; baseBranch?: string | null; branch?: string | null; baseCommitSha?: string | null; size?: "S" | "M" | "L"; reviewLevel?: number; mergeRetries?: number; stuckKillCount?: number | null; recoveryRetryCount?: number | null; nextRecoveryAt?: string | null; modelProvider?: string | null; modelId?: string | null; validatorModelProvider?: string | null; validatorModelId?: string | null; error?: string | null; summary?: string | null; sessionFile?: string | null; workflowStepResults?: import("./types.js").WorkflowStepResult[] | null; modifiedFiles?: string[] | null; missionId?: string | null; sliceId?: string | null },
|
||||||
): Promise<Task> {
|
): Promise<Task> {
|
||||||
return this.withTaskLock(id, async () => {
|
return this.withTaskLock(id, async () => {
|
||||||
// Validate that task doesn't depend on itself
|
// Validate that task doesn't depend on itself
|
||||||
@@ -1034,7 +1034,11 @@ export class TaskStore extends EventEmitter<TaskStoreEvents> {
|
|||||||
|
|
||||||
if (updates.title !== undefined) task.title = updates.title;
|
if (updates.title !== undefined) task.title = updates.title;
|
||||||
if (updates.description !== undefined) task.description = updates.description;
|
if (updates.description !== undefined) task.description = updates.description;
|
||||||
if (updates.worktree !== undefined) task.worktree = updates.worktree;
|
if (updates.worktree === null) {
|
||||||
|
task.worktree = undefined;
|
||||||
|
} else if (updates.worktree !== undefined) {
|
||||||
|
task.worktree = updates.worktree;
|
||||||
|
}
|
||||||
// Detect new dependencies being added to a todo task → auto-move to triage
|
// Detect new dependencies being added to a todo task → auto-move to triage
|
||||||
let movedToTriage = false;
|
let movedToTriage = false;
|
||||||
if (updates.dependencies !== undefined) {
|
if (updates.dependencies !== undefined) {
|
||||||
@@ -1064,9 +1068,21 @@ export class TaskStore extends EventEmitter<TaskStoreEvents> {
|
|||||||
task.blockedBy = updates.blockedBy;
|
task.blockedBy = updates.blockedBy;
|
||||||
}
|
}
|
||||||
if (updates.paused !== undefined) task.paused = updates.paused || undefined;
|
if (updates.paused !== undefined) task.paused = updates.paused || undefined;
|
||||||
if (updates.baseBranch !== undefined) task.baseBranch = updates.baseBranch;
|
if (updates.baseBranch === null) {
|
||||||
if (updates.branch !== undefined) task.branch = updates.branch;
|
task.baseBranch = undefined;
|
||||||
if (updates.baseCommitSha !== undefined) task.baseCommitSha = updates.baseCommitSha;
|
} else if (updates.baseBranch !== undefined) {
|
||||||
|
task.baseBranch = updates.baseBranch;
|
||||||
|
}
|
||||||
|
if (updates.branch === null) {
|
||||||
|
task.branch = undefined;
|
||||||
|
} else if (updates.branch !== undefined) {
|
||||||
|
task.branch = updates.branch;
|
||||||
|
}
|
||||||
|
if (updates.baseCommitSha === null) {
|
||||||
|
task.baseCommitSha = undefined;
|
||||||
|
} else if (updates.baseCommitSha !== undefined) {
|
||||||
|
task.baseCommitSha = updates.baseCommitSha;
|
||||||
|
}
|
||||||
if (updates.size !== undefined) task.size = updates.size;
|
if (updates.size !== undefined) task.size = updates.size;
|
||||||
if (updates.reviewLevel !== undefined) task.reviewLevel = updates.reviewLevel;
|
if (updates.reviewLevel !== undefined) task.reviewLevel = updates.reviewLevel;
|
||||||
if (updates.mergeRetries !== undefined) task.mergeRetries = updates.mergeRetries;
|
if (updates.mergeRetries !== undefined) task.mergeRetries = updates.mergeRetries;
|
||||||
|
|||||||
@@ -701,10 +701,10 @@ describe("POST /tasks/:id/retry", () => {
|
|||||||
|
|
||||||
expect(res.status).toBe(200);
|
expect(res.status).toBe(200);
|
||||||
expect(store.updateTask).toHaveBeenCalledWith("KB-001", {
|
expect(store.updateTask).toHaveBeenCalledWith("KB-001", {
|
||||||
status: undefined,
|
status: null,
|
||||||
error: undefined,
|
error: null,
|
||||||
worktree: undefined,
|
worktree: null,
|
||||||
branch: undefined,
|
branch: null,
|
||||||
});
|
});
|
||||||
expect(store.moveTask).toHaveBeenCalledWith("KB-001", "todo");
|
expect(store.moveTask).toHaveBeenCalledWith("KB-001", "todo");
|
||||||
});
|
});
|
||||||
@@ -734,10 +734,10 @@ describe("POST /tasks/:id/retry", () => {
|
|||||||
|
|
||||||
expect(res.status).toBe(200);
|
expect(res.status).toBe(200);
|
||||||
expect(store.updateTask).toHaveBeenCalledWith("KB-001", {
|
expect(store.updateTask).toHaveBeenCalledWith("KB-001", {
|
||||||
status: undefined,
|
status: null,
|
||||||
error: undefined,
|
error: null,
|
||||||
worktree: undefined,
|
worktree: null,
|
||||||
branch: undefined,
|
branch: null,
|
||||||
});
|
});
|
||||||
expect(store.moveTask).toHaveBeenCalledWith("KB-001", "todo");
|
expect(store.moveTask).toHaveBeenCalledWith("KB-001", "todo");
|
||||||
});
|
});
|
||||||
@@ -755,10 +755,10 @@ describe("POST /tasks/:id/retry", () => {
|
|||||||
|
|
||||||
expect(res.status).toBe(200);
|
expect(res.status).toBe(200);
|
||||||
expect(store.updateTask).toHaveBeenCalledWith("KB-001", {
|
expect(store.updateTask).toHaveBeenCalledWith("KB-001", {
|
||||||
status: undefined,
|
status: null,
|
||||||
error: undefined,
|
error: null,
|
||||||
worktree: undefined,
|
worktree: null,
|
||||||
branch: undefined,
|
branch: null,
|
||||||
});
|
});
|
||||||
expect(store.moveTask).toHaveBeenCalledWith("KB-001", "todo");
|
expect(store.moveTask).toHaveBeenCalledWith("KB-001", "todo");
|
||||||
expect(store.logEntry).toHaveBeenCalledWith("KB-001", "Retry requested from dashboard");
|
expect(store.logEntry).toHaveBeenCalledWith("KB-001", "Retry requested from dashboard");
|
||||||
|
|||||||
@@ -1794,10 +1794,10 @@ export function createApiRoutes(store: TaskStore, options?: ServerOptions): Rout
|
|||||||
return;
|
return;
|
||||||
}
|
}
|
||||||
await scopedStore.updateTask(req.params.id, {
|
await scopedStore.updateTask(req.params.id, {
|
||||||
status: undefined,
|
status: null,
|
||||||
error: undefined,
|
error: null,
|
||||||
worktree: undefined,
|
worktree: null,
|
||||||
branch: undefined,
|
branch: null,
|
||||||
});
|
});
|
||||||
await scopedStore.logEntry(req.params.id, "Retry requested from dashboard");
|
await scopedStore.logEntry(req.params.id, "Retry requested from dashboard");
|
||||||
const updated = await scopedStore.moveTask(req.params.id, "todo");
|
const updated = await scopedStore.moveTask(req.params.id, "todo");
|
||||||
|
|||||||
@@ -538,6 +538,78 @@ describe("Scheduler", () => {
|
|||||||
});
|
});
|
||||||
});
|
});
|
||||||
|
|
||||||
|
describe("worktree reservation", () => {
|
||||||
|
it("assigns a planned worktree path before moving a task to in-progress", async () => {
|
||||||
|
vi.mocked(existsSync).mockReturnValue(true);
|
||||||
|
vi.mocked(readFile).mockResolvedValue("# Task\nDo something");
|
||||||
|
|
||||||
|
const task = createMockTask({ id: "FN-010", column: "todo" });
|
||||||
|
const updateTask = vi.fn().mockResolvedValue(undefined);
|
||||||
|
const moveTask = vi.fn().mockResolvedValue(undefined);
|
||||||
|
const store = createMockStore({
|
||||||
|
listTasks: vi.fn().mockResolvedValue([task]),
|
||||||
|
getSettings: vi.fn().mockResolvedValue({ maxConcurrent: 2, maxWorktrees: 4, worktreeNaming: "task-id" }),
|
||||||
|
updateTask,
|
||||||
|
moveTask,
|
||||||
|
});
|
||||||
|
|
||||||
|
const scheduler = new Scheduler(store);
|
||||||
|
(scheduler as any).running = true;
|
||||||
|
await scheduler.schedule();
|
||||||
|
|
||||||
|
expect(updateTask).toHaveBeenCalledWith("FN-010", {
|
||||||
|
status: null,
|
||||||
|
blockedBy: null,
|
||||||
|
baseBranch: undefined,
|
||||||
|
worktree: "/test/project/.worktrees/fn-010",
|
||||||
|
});
|
||||||
|
expect(moveTask).toHaveBeenCalledWith("FN-010", "in-progress");
|
||||||
|
expect(updateTask.mock.invocationCallOrder[0]).toBeLessThan(moveTask.mock.invocationCallOrder[0]);
|
||||||
|
});
|
||||||
|
|
||||||
|
it("reserves unique random worktree names within the same scheduling pass", async () => {
|
||||||
|
vi.mocked(existsSync).mockReturnValue(true);
|
||||||
|
vi.mocked(readFile).mockResolvedValue("# Task\nDo something");
|
||||||
|
|
||||||
|
const randomSpy = vi.spyOn(Math, "random")
|
||||||
|
.mockReturnValueOnce(0)
|
||||||
|
.mockReturnValueOnce(0)
|
||||||
|
.mockReturnValueOnce(0)
|
||||||
|
.mockReturnValueOnce(0);
|
||||||
|
|
||||||
|
const updateTask = vi.fn().mockResolvedValue(undefined);
|
||||||
|
const moveTask = vi.fn().mockResolvedValue(undefined);
|
||||||
|
const store = createMockStore({
|
||||||
|
listTasks: vi.fn().mockResolvedValue([
|
||||||
|
createMockTask({ id: "FN-011", column: "todo" }),
|
||||||
|
createMockTask({ id: "FN-012", column: "todo" }),
|
||||||
|
]),
|
||||||
|
getSettings: vi.fn().mockResolvedValue({ maxConcurrent: 4, maxWorktrees: 4, worktreeNaming: "random" }),
|
||||||
|
updateTask,
|
||||||
|
moveTask,
|
||||||
|
});
|
||||||
|
|
||||||
|
const scheduler = new Scheduler(store);
|
||||||
|
(scheduler as any).running = true;
|
||||||
|
await scheduler.schedule();
|
||||||
|
|
||||||
|
expect(updateTask).toHaveBeenNthCalledWith(1, "FN-011", {
|
||||||
|
status: null,
|
||||||
|
blockedBy: null,
|
||||||
|
baseBranch: undefined,
|
||||||
|
worktree: "/test/project/.worktrees/amber-aspen",
|
||||||
|
});
|
||||||
|
expect(updateTask).toHaveBeenNthCalledWith(2, "FN-012", {
|
||||||
|
status: null,
|
||||||
|
blockedBy: null,
|
||||||
|
baseBranch: undefined,
|
||||||
|
worktree: "/test/project/.worktrees/amber-aspen-2",
|
||||||
|
});
|
||||||
|
|
||||||
|
randomSpy.mockRestore();
|
||||||
|
});
|
||||||
|
});
|
||||||
|
|
||||||
describe("semaphore integration", () => {
|
describe("semaphore integration", () => {
|
||||||
it("respects semaphore available count", async () => {
|
it("respects semaphore available count", async () => {
|
||||||
const semaphore = {
|
const semaphore = {
|
||||||
|
|||||||
@@ -3,6 +3,7 @@ import { existsSync } from "node:fs";
|
|||||||
import { readFile } from "node:fs/promises";
|
import { readFile } from "node:fs/promises";
|
||||||
import { join } from "node:path";
|
import { join } from "node:path";
|
||||||
import type { AgentSemaphore } from "./concurrency.js";
|
import type { AgentSemaphore } from "./concurrency.js";
|
||||||
|
import { generateReservedWorktreeName, slugify } from "./worktree-names.js";
|
||||||
import { schedulerLog } from "./logger.js";
|
import { schedulerLog } from "./logger.js";
|
||||||
import { type PrMonitor, type PrComment } from "./pr-monitor.js";
|
import { type PrMonitor, type PrComment } from "./pr-monitor.js";
|
||||||
import { getCurrentGitHubRepo } from "./github.js";
|
import { getCurrentGitHubRepo } from "./github.js";
|
||||||
@@ -348,6 +349,39 @@ export class Scheduler {
|
|||||||
return pathsOverlap(a, b);
|
return pathsOverlap(a, b);
|
||||||
}
|
}
|
||||||
|
|
||||||
|
/**
|
||||||
|
* Reserve the worktree path a task will use before it enters in-progress.
|
||||||
|
* This prevents tasks from appearing active without an assigned worktree.
|
||||||
|
*/
|
||||||
|
private planWorktreePath(
|
||||||
|
task: Task,
|
||||||
|
naming: string | undefined,
|
||||||
|
reservedNames: Set<string>,
|
||||||
|
): string {
|
||||||
|
if (task.worktree) {
|
||||||
|
const existingName = task.worktree.split("/").pop();
|
||||||
|
if (existingName) reservedNames.add(existingName);
|
||||||
|
return task.worktree;
|
||||||
|
}
|
||||||
|
|
||||||
|
let worktreeName: string;
|
||||||
|
switch (naming || "random") {
|
||||||
|
case "task-id":
|
||||||
|
worktreeName = task.id.toLowerCase();
|
||||||
|
break;
|
||||||
|
case "task-title":
|
||||||
|
worktreeName = slugify(task.title || task.description.slice(0, 60));
|
||||||
|
break;
|
||||||
|
case "random":
|
||||||
|
default:
|
||||||
|
worktreeName = generateReservedWorktreeName(this.store.getRootDir(), reservedNames);
|
||||||
|
break;
|
||||||
|
}
|
||||||
|
|
||||||
|
reservedNames.add(worktreeName);
|
||||||
|
return join(this.store.getRootDir(), ".worktrees", worktreeName);
|
||||||
|
}
|
||||||
|
|
||||||
/**
|
/**
|
||||||
* Run one scheduling pass.
|
* Run one scheduling pass.
|
||||||
*
|
*
|
||||||
@@ -521,6 +555,11 @@ export class Scheduler {
|
|||||||
// Resolve dependency order among todo tasks
|
// Resolve dependency order among todo tasks
|
||||||
const ordered = resolveDependencyOrder(todo);
|
const ordered = resolveDependencyOrder(todo);
|
||||||
let started = 0;
|
let started = 0;
|
||||||
|
const reservedWorktreeNames = new Set(
|
||||||
|
tasks
|
||||||
|
.map((task) => task.worktree?.split("/").pop())
|
||||||
|
.filter((name): name is string => Boolean(name)),
|
||||||
|
);
|
||||||
|
|
||||||
for (const taskId of ordered) {
|
for (const taskId of ordered) {
|
||||||
const task = tasks.find((t) => t.id === taskId)!;
|
const task = tasks.find((t) => t.id === taskId)!;
|
||||||
@@ -571,10 +610,20 @@ export class Scheduler {
|
|||||||
|
|
||||||
// Dependencies met — resolve base branch from in-review deps
|
// Dependencies met — resolve base branch from in-review deps
|
||||||
const baseBranch = this.resolveBaseBranch(task, tasks);
|
const baseBranch = this.resolveBaseBranch(task, tasks);
|
||||||
|
const plannedWorktree = this.planWorktreePath(
|
||||||
|
task,
|
||||||
|
settings.worktreeNaming,
|
||||||
|
reservedWorktreeNames,
|
||||||
|
);
|
||||||
|
|
||||||
// Clear status and move to in-progress
|
// Clear status, reserve worktree path, and then move to in-progress
|
||||||
schedulerLog.log(`Starting ${task.id}: ${task.title || task.id} (deps satisfied)`);
|
schedulerLog.log(`Starting ${task.id}: ${task.title || task.id} (deps satisfied)`);
|
||||||
await this.store.updateTask(task.id, { status: null, blockedBy: null, baseBranch: baseBranch ?? undefined });
|
await this.store.updateTask(task.id, {
|
||||||
|
status: null,
|
||||||
|
blockedBy: null,
|
||||||
|
baseBranch: baseBranch ?? undefined,
|
||||||
|
worktree: plannedWorktree,
|
||||||
|
});
|
||||||
await this.store.moveTask(task.id, "in-progress");
|
await this.store.moveTask(task.id, "in-progress");
|
||||||
this.options.onSchedule?.(task);
|
this.options.onSchedule?.(task);
|
||||||
started++;
|
started++;
|
||||||
|
|||||||
@@ -61,12 +61,26 @@ export function slugify(str: string): string {
|
|||||||
* @returns A unique worktree directory name (not a full path)
|
* @returns A unique worktree directory name (not a full path)
|
||||||
*/
|
*/
|
||||||
export function generateWorktreeName(rootDir: string): string {
|
export function generateWorktreeName(rootDir: string): string {
|
||||||
|
return generateReservedWorktreeName(rootDir);
|
||||||
|
}
|
||||||
|
|
||||||
|
/**
|
||||||
|
* Generate a unique worktree directory name while also avoiding names that
|
||||||
|
* have been reserved in-memory but may not exist on disk yet.
|
||||||
|
*/
|
||||||
|
export function generateReservedWorktreeName(
|
||||||
|
rootDir: string,
|
||||||
|
reservedNames: Set<string> = new Set(),
|
||||||
|
): string {
|
||||||
const adjective = ADJECTIVES[Math.floor(Math.random() * ADJECTIVES.length)];
|
const adjective = ADJECTIVES[Math.floor(Math.random() * ADJECTIVES.length)];
|
||||||
const noun = NOUNS[Math.floor(Math.random() * NOUNS.length)];
|
const noun = NOUNS[Math.floor(Math.random() * NOUNS.length)];
|
||||||
const baseName = `${adjective}-${noun}`;
|
const baseName = `${adjective}-${noun}`;
|
||||||
|
|
||||||
const worktreesDir = join(rootDir, ".worktrees");
|
const worktreesDir = join(rootDir, ".worktrees");
|
||||||
const existing = getExistingWorktreeNames(worktreesDir);
|
const existing = getExistingWorktreeNames(worktreesDir);
|
||||||
|
for (const reserved of reservedNames) {
|
||||||
|
existing.add(reserved);
|
||||||
|
}
|
||||||
|
|
||||||
if (!existing.has(baseName)) {
|
if (!existing.has(baseName)) {
|
||||||
return baseName;
|
return baseName;
|
||||||
|
|||||||
Reference in New Issue
Block a user