fix: reserve and clear task worktrees correctly
This commit is contained in:
@@ -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", () => {
|
||||
it("respects semaphore available count", async () => {
|
||||
const semaphore = {
|
||||
|
||||
@@ -3,6 +3,7 @@ import { existsSync } from "node:fs";
|
||||
import { readFile } from "node:fs/promises";
|
||||
import { join } from "node:path";
|
||||
import type { AgentSemaphore } from "./concurrency.js";
|
||||
import { generateReservedWorktreeName, slugify } from "./worktree-names.js";
|
||||
import { schedulerLog } from "./logger.js";
|
||||
import { type PrMonitor, type PrComment } from "./pr-monitor.js";
|
||||
import { getCurrentGitHubRepo } from "./github.js";
|
||||
@@ -348,6 +349,39 @@ export class Scheduler {
|
||||
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.
|
||||
*
|
||||
@@ -521,6 +555,11 @@ export class Scheduler {
|
||||
// Resolve dependency order among todo tasks
|
||||
const ordered = resolveDependencyOrder(todo);
|
||||
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) {
|
||||
const task = tasks.find((t) => t.id === taskId)!;
|
||||
@@ -571,10 +610,20 @@ export class Scheduler {
|
||||
|
||||
// Dependencies met — resolve base branch from in-review deps
|
||||
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)`);
|
||||
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");
|
||||
this.options.onSchedule?.(task);
|
||||
started++;
|
||||
|
||||
@@ -61,12 +61,26 @@ export function slugify(str: string): string {
|
||||
* @returns A unique worktree directory name (not a full path)
|
||||
*/
|
||||
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 noun = NOUNS[Math.floor(Math.random() * NOUNS.length)];
|
||||
const baseName = `${adjective}-${noun}`;
|
||||
|
||||
const worktreesDir = join(rootDir, ".worktrees");
|
||||
const existing = getExistingWorktreeNames(worktreesDir);
|
||||
for (const reserved of reservedNames) {
|
||||
existing.add(reserved);
|
||||
}
|
||||
|
||||
if (!existing.has(baseName)) {
|
||||
return baseName;
|
||||
|
||||
Reference in New Issue
Block a user