diff --git a/server/appRuntime.test.ts b/server/appRuntime.test.ts index c0b91a4..68b8af4 100644 --- a/server/appRuntime.test.ts +++ b/server/appRuntime.test.ts @@ -61,6 +61,7 @@ async function waitForCondition( test("runtime lists active and archived PRs separately", async () => { const storage = new MemStorage(); const runtime = createAppRuntime({ + reclaimOrphanedWorktreesFn: async () => [], storage, startBackgroundServices: false, startWatcher: false, @@ -86,6 +87,7 @@ test("runtime lists active and archived PRs separately", async () => { test("runtime queueBabysit enqueues a babysit job using the configured agent", async () => { const storage = new MemStorage(); const runtime = createAppRuntime({ + reclaimOrphanedWorktreesFn: async () => [], storage, startBackgroundServices: false, startWatcher: false, @@ -116,6 +118,7 @@ test("runtime queueBabysit enqueues a babysit job using the configured agent", a test("runtime queueBabysit records durable PR work intent", async () => { const storage = new MemStorage(); const runtime = createAppRuntime({ + reclaimOrphanedWorktreesFn: async () => [], storage, startBackgroundServices: false, startWatcher: false, @@ -154,6 +157,7 @@ test("runtime queueBabysit records durable PR work intent", async () => { test("runtime activity preserves monitor follow-up labels", async () => { const storage = new MemStorage(); const runtime = createAppRuntime({ + reclaimOrphanedWorktreesFn: async () => [], storage, startBackgroundServices: false, startWatcher: false, @@ -180,6 +184,7 @@ test("runtime activity preserves monitor follow-up labels", async () => { test("runtime queueBabysit uses repo agent override when configured", async () => { const storage = new MemStorage(); const runtime = createAppRuntime({ + reclaimOrphanedWorktreesFn: async () => [], storage, startBackgroundServices: false, startWatcher: false, @@ -201,6 +206,7 @@ test("runtime queueBabysit uses repo agent override when configured", async () = test("runtime exposes the latest PR agent run status", async () => { const storage = new MemStorage(); const runtime = createAppRuntime({ + reclaimOrphanedWorktreesFn: async () => [], storage, startBackgroundServices: false, startWatcher: false, @@ -237,6 +243,7 @@ test("runtime exposes the latest PR agent run status", async () => { test("runtime setWatchEnabled updates the PR and emits a change event", async () => { const storage = new MemStorage(); const runtime = createAppRuntime({ + reclaimOrphanedWorktreesFn: async () => [], storage, startBackgroundServices: false, startWatcher: false, @@ -263,6 +270,7 @@ test("runtime setWatchEnabled updates the PR and emits a change event", async () test("runtime setDrainMode logs enable and disable transitions", async () => { const storage = new MemStorage(); const runtime = createAppRuntime({ + reclaimOrphanedWorktreesFn: async () => [], storage, startBackgroundServices: false, startWatcher: false, @@ -305,6 +313,7 @@ test("runtime clears stale CLI-missing drain mode once the agent command is avai const storage = new MemStorage(); const runtime = createAppRuntime({ + reclaimOrphanedWorktreesFn: async () => [], storage, startBackgroundServices: false, startWatcher: false, @@ -329,6 +338,7 @@ test("runtime clears stale CLI-missing drain mode once the agent command is avai test("runtime askQuestion persists the question and enqueues a durable job", async () => { const storage = new MemStorage(); const runtime = createAppRuntime({ + reclaimOrphanedWorktreesFn: async () => [], storage, startBackgroundServices: false, startWatcher: false, @@ -357,6 +367,7 @@ test("runtime askQuestion persists the question and enqueues a durable job", asy test("runtime updateConfig persists updates and exposes them through getConfig", async () => { const storage = new MemStorage(); const runtime = createAppRuntime({ + reclaimOrphanedWorktreesFn: async () => [], storage, startBackgroundServices: false, startWatcher: false, @@ -387,6 +398,7 @@ test("runtime updateConfig persists updates and exposes them through getConfig", test("manual sync runs immediately even when global manual mode is on", async () => { const storage = new MemStorage(); const runtime = createAppRuntime({ + reclaimOrphanedWorktreesFn: async () => [], storage, startBackgroundServices: false, startWatcher: false, @@ -420,6 +432,7 @@ test("manual sync can target only PRs or only issues", async () => { }, }; const runtime = createAppRuntime({ + reclaimOrphanedWorktreesFn: async () => [], storage, startBackgroundServices: false, startWatcher: false, @@ -464,6 +477,7 @@ test("automatic watcher does not sync issues when issue automation is off", asyn }, }; const runtime = createAppRuntime({ + reclaimOrphanedWorktreesFn: async () => [], storage, startBackgroundServices: false, startWatcher: false, @@ -500,6 +514,7 @@ test("automatic watcher runs issue sync without PR sync when PR automation is of }, }; const runtime = createAppRuntime({ + reclaimOrphanedWorktreesFn: async () => [], storage, startBackgroundServices: false, startWatcher: false, @@ -537,6 +552,7 @@ test("automatic watcher does nothing when PR and issue automation are both off", }, }; const runtime = createAppRuntime({ + reclaimOrphanedWorktreesFn: async () => [], storage, startBackgroundServices: false, startWatcher: false, @@ -575,6 +591,7 @@ test("automatic watcher can run PR and issue automation together", async () => { }, }; const runtime = createAppRuntime({ + reclaimOrphanedWorktreesFn: async () => [], storage, startBackgroundServices: false, startWatcher: false, @@ -1012,6 +1029,7 @@ test("syncRepos skips the issue sweep for a repo whose issue list responds 304", }; const runtime = createAppRuntime({ + reclaimOrphanedWorktreesFn: async () => [], storage, startBackgroundServices: false, startWatcher: false, @@ -1050,6 +1068,7 @@ test("listIssueCoverage reads persisted counts and does not call GitHub", async }); const runtime = createAppRuntime({ + reclaimOrphanedWorktreesFn: async () => [], storage, startBackgroundServices: false, startWatcher: false, @@ -1089,6 +1108,7 @@ test("syncRepos 304 probe does not fetch a GitHub open-issue count", async () => }; const runtime = createAppRuntime({ + reclaimOrphanedWorktreesFn: async () => [], storage, startBackgroundServices: false, startWatcher: false, @@ -1138,6 +1158,7 @@ test("syncRepos syncs issues and persists the new etag when the issue list chang }; const runtime = createAppRuntime({ + reclaimOrphanedWorktreesFn: async () => [], storage, startBackgroundServices: false, startWatcher: false, @@ -1181,6 +1202,7 @@ test("listIssues stays cached-only when issue automation is off", async () => { let buildOctokitCalls = 0; const runtime = createAppRuntime({ + reclaimOrphanedWorktreesFn: async () => [], storage, startBackgroundServices: false, startWatcher: false, @@ -1234,6 +1256,7 @@ test("listIssues excludes closed worked issues from the default open issue count ], "2026-05-03T19:00:00.000Z"); const runtime = createAppRuntime({ + reclaimOrphanedWorktreesFn: async () => [], storage, startBackgroundServices: false, startWatcher: false, @@ -1266,6 +1289,7 @@ test("getIssue stays cached-only when issue automation is off", async () => { let buildOctokitCalls = 0; const runtime = createAppRuntime({ + reclaimOrphanedWorktreesFn: async () => [], storage, startBackgroundServices: false, startWatcher: false, @@ -1338,6 +1362,7 @@ test("syncIssue refreshes worked issue metadata from GitHub", async () => { }; const runtime = createAppRuntime({ + reclaimOrphanedWorktreesFn: async () => [], storage, startBackgroundServices: false, startWatcher: false, @@ -1393,6 +1418,7 @@ test("runtime exposes queued issue work as current run status", async () => { }; const runtime = createAppRuntime({ + reclaimOrphanedWorktreesFn: async () => [], storage, startBackgroundServices: false, startWatcher: false, @@ -1419,6 +1445,7 @@ test("pickWatcherColdStartDelayMs stays within the 15-45s cold-start window", () test("start() defers the first watcher tick instead of firing it during start", async () => { let watcherRuns = 0; const runtime = createAppRuntime({ + reclaimOrphanedWorktreesFn: async () => [], storage: new MemStorage(), startBackgroundServices: false, startWatcher: true, @@ -1447,6 +1474,7 @@ test("syncRepos persists an issue-sweep backoff when the probe fails", async () }; const runtime = createAppRuntime({ + reclaimOrphanedWorktreesFn: async () => [], storage, startBackgroundServices: false, startWatcher: false, @@ -1482,6 +1510,7 @@ test("syncRepos skips an issue sweep for a repo whose persisted backoff is activ }; const runtime = createAppRuntime({ + reclaimOrphanedWorktreesFn: async () => [], storage, startBackgroundServices: false, startWatcher: false, @@ -1510,6 +1539,7 @@ test("syncRepos defers the next sweep for a repo whose issue list is unchanged", }; const runtime = createAppRuntime({ + reclaimOrphanedWorktreesFn: async () => [], storage, startBackgroundServices: false, startWatcher: false, diff --git a/server/appRuntime.ts b/server/appRuntime.ts index 43350f5..0f8baf4 100644 --- a/server/appRuntime.ts +++ b/server/appRuntime.ts @@ -32,8 +32,9 @@ import type { IStorage } from "./storage"; import { getDefaultStorage } from "./storage"; import { PRBabysitter } from "./babysitter"; import { resolveRepoAgentRuntimeSettings, resolveRepoCodingAgent } from "./agentSettings"; -import { commandExists, detectAgentUnavailability, type AgentUnavailabilityKind, type CodingAgent } from "./agentRunner"; +import { commandExists, detectAgentUnavailability, runCommand, type AgentUnavailabilityKind, type CodingAgent } from "./agentRunner"; import { planFailedJobRecovery } from "./failureRecovery"; +import { reclaimOrphanedWorktrees } from "./repoWorkspace"; import { applyEvaluationDecision, applyFlagDecision } from "./feedbackLifecycle"; import { applyManualFeedbackDecision } from "./manualFeedback"; import { childLogger } from "./logger"; @@ -112,6 +113,7 @@ export type AppRuntimeDependencies = { babysitter?: PRBabysitter; watcherScheduler?: WatcherScheduler; buildOctokitFn?: typeof buildOctokit; + reclaimOrphanedWorktreesFn?: typeof reclaimOrphanedWorktrees; startBackgroundServices?: boolean; startWatcher?: boolean; }; @@ -940,6 +942,7 @@ export function mapMergedPullsToReleaseSummaries(pulls: MergedPRSummary[]): Rele export function createAppRuntime(dependencies: AppRuntimeDependencies = {}): AppRuntime { const storage = dependencies.storage ?? getDefaultStorage(); const buildOctokitImpl = dependencies.buildOctokitFn ?? buildOctokit; + const reclaimOrphanedWorktreesImpl = dependencies.reclaimOrphanedWorktreesFn ?? reclaimOrphanedWorktrees; const events = new EventEmitter(); const socialPostJobs = new Map(); const backgroundJobQueue = dependencies.backgroundJobQueue ?? new BackgroundJobQueue(storage); @@ -1053,6 +1056,34 @@ export function createAppRuntime(dependencies: AppRuntimeDependencies = {}): App }, }); + /** + * Sweep worktrees stranded by runs that died without unwinding their cleanup. + * + * `includeActive` is only safe at startup, where the single-instance lock means + * nothing can legitimately hold a worktree. On the watcher tick we reclaim only + * what this process is not actively using. + */ + const sweepOrphanedWorktrees = async (options: { includeActive?: boolean } = {}) => { + try { + const reclaimed = await reclaimOrphanedWorktreesImpl({ + runCommand, + includeActive: options.includeActive, + }); + + for (const entry of reclaimed) { + log.info( + { repoCacheDir: entry.repoCacheDir, reclaimed: entry.reclaimed }, + "Reclaimed orphaned git worktrees left by an interrupted run", + ); + } + } catch (error) { + log.warn( + { err: error instanceof Error ? error.message : String(error) }, + "Orphaned worktree sweep failed", + ); + } + }; + /** * Revive parked background jobs whose park interval has elapsed, and un-park * PRs whose work is actually still in flight. Without this, a job that ran out @@ -1132,6 +1163,7 @@ export function createAppRuntime(dependencies: AppRuntimeDependencies = {}): App // automation running unattended: whatever blocked a job may well have been // fixed since, and nothing else tells us that it was. await recoverParkedWork(); + await sweepOrphanedWorktrees(); const rateLimit = getRateLimitState("core"); if (rateLimit.limited && rateLimit.resetAt) { @@ -2313,6 +2345,9 @@ export function createAppRuntime(dependencies: AppRuntimeDependencies = {}): App } if (startWatcher) { + // Runs interrupted by the last shutdown cannot have cleaned up after + // themselves, so clear their worktrees before anything claims a new one. + await sweepOrphanedWorktrees({ includeActive: true }); await refreshWatcherSchedule(); void babysitter.resumeInterruptedRuns(); watcherColdStartTimer = setTimeout(() => { diff --git a/server/backgroundJobHandlers.test.ts b/server/backgroundJobHandlers.test.ts index 0a3a201..12bba9f 100644 --- a/server/backgroundJobHandlers.test.ts +++ b/server/backgroundJobHandlers.test.ts @@ -995,3 +995,103 @@ test("process_release_run handler retries before parking the run as errored", as dispatcher.stop(); } }); + +test("work_issue handler keeps one status comment per issue instead of posting every attempt", async () => { + // Goal: a permanently failing issue used to collect a fresh "started" and "failed" + // comment on every attempt — dozens of identical notices on a single issue. The + // status comment is now edited in place. + const storage = new MemStorage(); + await storage.updateConfig({ + watchedRepos: ["acme/widgets"], + codingAgent: "claude", + postGitHubProgressReplies: true, + maxAgentRetryAttempts: 1, + }); + const queue = new BackgroundJobQueue(storage); + + const comments: Array<{ id: number; body: string }> = []; + let nextCommentId = 1; + let createCalls = 0; + let updateCalls = 0; + + const octokit = { + issues: { + get: async () => ({ + data: { + number: 21, + title: "Broken widget", + body: "It broke", + html_url: "https://github.com/acme/widgets/issues/21", + user: { login: "alice" }, + labels: [{ name: "bug" }], + assignees: [], + comments: 0, + created_at: "2026-08-03T17:00:00.000Z", + updated_at: "2026-08-03T18:00:00.000Z", + }, + }), + listComments: async () => ({ data: comments.map((c) => ({ id: c.id, body: c.body })) }), + createComment: async (params: { body: string }) => { + createCalls += 1; + const entry = { id: nextCommentId++, body: params.body }; + comments.push(entry); + return { data: { id: entry.id } }; + }, + updateComment: async (params: { comment_id: number; body: string }) => { + updateCalls += 1; + const target = comments.find((c) => c.id === params.comment_id); + if (target) { + target.body = params.body; + } + return { data: { id: params.comment_id } }; + }, + }, + }; + + const runJob = async () => { + const job = await queue.enqueue("work_issue", "acme/widgets#21", `work_issue:acme/widgets#21:${nextCommentId}`, { + repo: "acme/widgets", + issueNumber: 21, + issueTitle: "Broken widget", + issueUrl: "https://github.com/acme/widgets/issues/21", + baseBranch: "main", + }); + + const dispatcher = new BackgroundJobDispatcher({ + storage, + queue, + workerId: "dispatcher-1", + pollIntervalMs: 5, + leaseMs: 30_000, + heartbeatIntervalMs: 10, + retryBackoffMs: 0, + handlers: createBackgroundJobHandlers({ + storage, + deps: { + buildOctokitFn: async () => octokit as never, + resolveGitHubAuthTokenFn: async () => "gho_token", + runIssueWorkRepairFn: async () => { + throw new Error("Refusing to reclone repo cache while 22 registered worktree(s) still exist"); + }, + }, + }), + }); + + try { + await dispatcher.start(); + await waitForCondition(async () => (await storage.getBackgroundJob(job.id))?.status === "failed", 1_000); + } finally { + dispatcher.stop(); + } + }; + + await runJob(); + await runJob(); + await runJob(); + + assert.equal(createCalls, 1, "three failing runs must not produce three new comments"); + assert.ok(updateCalls > 0, "later status changes edit the existing comment"); + assert.equal(comments.length, 1); + assert.match(comments[0]?.body ?? "", /Issue work failed/); + assert.match(comments[0]?.body ?? "", /registered worktree/); +}); diff --git a/server/backgroundJobHandlers.ts b/server/backgroundJobHandlers.ts index 2ae80e0..e9721f8 100644 --- a/server/backgroundJobHandlers.ts +++ b/server/backgroundJobHandlers.ts @@ -22,7 +22,7 @@ import { resolveGitHubAuthToken, } from "./github"; import { buildIssueEvaluationComment, evaluateIssueForAutomation } from "./issueEvaluator"; -import { buildIssueReplyBody, buildIssueVerifyComment, buildIssueWorkStatusComment, buildPullRequestBody } from "./issueFormatter"; +import { buildIssueReplyBody, buildIssueVerifyComment, buildIssueWorkStatusComment, buildIssueWorkStatusMarker, buildPullRequestBody } from "./issueFormatter"; import { decomposeIssueBody, hashIssueBody } from "./issueDecompose"; import { verifySubtasksAgainstPr } from "./issueVerify"; import { runIssueWorkRepair } from "./issueWorkAgent"; @@ -172,6 +172,13 @@ export function createBackgroundJobHandlers(params: { } } + /** + * Maintain a single status comment per issue instead of appending a new one for + * every attempt. A permanently failing issue used to collect one "started" and + * one "failed" notice per recovery cycle, which turned into dozens of identical + * comments; editing the existing comment keeps the thread readable and still + * shows the current state. + */ async function postIssueWorkStatusComment( octokit: { issues: { @@ -181,6 +188,18 @@ export function createBackgroundJobHandlers(params: { issue_number: number; body: string; }) => Promise; + updateComment?: (params: { + owner: string; + repo: string; + comment_id: number; + body: string; + }) => Promise; + listComments?: (params: { + owner: string; + repo: string; + issue_number: number; + per_page?: number; + }) => Promise<{ data: Array<{ id: number; body?: string | null }> }>; }; }, parsedRepo: { owner: string; repo: string }, @@ -190,6 +209,23 @@ export function createBackgroundJobHandlers(params: { stage: string, ): Promise { try { + const existingId = await findIssueWorkStatusCommentId( + octokit, + parsedRepo, + issueNumber, + `${parsedRepo.owner}/${parsedRepo.repo}`, + ); + + if (existingId !== null && octokit.issues.updateComment) { + await octokit.issues.updateComment({ + owner: parsedRepo.owner, + repo: parsedRepo.repo, + comment_id: existingId, + body, + }); + return; + } + await octokit.issues.createComment({ owner: parsedRepo.owner, repo: parsedRepo.repo, @@ -203,6 +239,50 @@ export function createBackgroundJobHandlers(params: { } } + /** + * The status comment is edited in place, so without this line a reader cannot + * tell whether automation tried once or twenty times. + */ + function describeIssueWorkAttempts(attemptCount: number): string | null { + if (attemptCount <= 1) { + return null; + } + + return `Attempts: ${attemptCount}`; + } + + /** Locate the status comment PatchDeck already owns on this issue, if any. */ + async function findIssueWorkStatusCommentId( + octokit: { + issues: { + listComments?: (params: { + owner: string; + repo: string; + issue_number: number; + per_page?: number; + }) => Promise<{ data: Array<{ id: number; body?: string | null }> }>; + }; + }, + parsedRepo: { owner: string; repo: string }, + issueNumber: number, + repoFullName: string, + ): Promise { + if (!octokit.issues.listComments) { + return null; + } + + const marker = buildIssueWorkStatusMarker(repoFullName, issueNumber); + const response = await octokit.issues.listComments({ + owner: parsedRepo.owner, + repo: parsedRepo.repo, + issue_number: issueNumber, + per_page: 100, + }); + + const match = response.data.filter((comment) => (comment.body ?? "").includes(marker)).pop(); + return match ? match.id : null; + } + async function addIssueWorkStageLog( targetId: string, stage: string, @@ -622,6 +702,7 @@ export function createBackgroundJobHandlers(params: { issueUrl: issue.url, stage: "failed", detail: message, + attemptSummary: describeIssueWorkAttempts(job.attemptCount), }), targetId, "failed", @@ -664,6 +745,7 @@ export function createBackgroundJobHandlers(params: { issueUrl: issue.url, stage: "failed", detail: repairResult.rejectionReason ?? "Issue work not accepted", + attemptSummary: describeIssueWorkAttempts(job.attemptCount), }), targetId, "failed", diff --git a/server/issueFormatter.ts b/server/issueFormatter.ts index 23c2fe5..4801ff9 100644 --- a/server/issueFormatter.ts +++ b/server/issueFormatter.ts @@ -72,6 +72,8 @@ type IssueWorkStatusCommentInput = { issueUrl: string; stage: IssueWorkStatusStage; detail?: string | null; + /** Optional "attempt 3, last tried ..." line so a repeatedly edited comment shows progress. */ + attemptSummary?: string | null; }; export function buildIssueReplyBody(input: IssueReplyBodyInput): string { @@ -164,32 +166,49 @@ export function buildIssueVerifyComment(input: IssueVerifyCommentInput): string ].join("\n"); } +/** + * Hidden anchor identifying the single status comment PatchDeck maintains for an + * issue. Status is edited in place rather than appended, so a repeatedly failing + * issue keeps one current comment instead of a wall of identical notices. + */ +export function buildIssueWorkStatusMarker(repoFullName: string, issueNumber: number): string { + return ``; +} + export function buildIssueWorkStatusComment(input: IssueWorkStatusCommentInput): string { const issueLine = `[#${input.issueNumber} ${input.issueTitle}](${input.issueUrl})`; const detailLine = input.detail?.trim(); const safeDetailLine = detailLine ? redactLocalPaths(detailLine) : null; + const marker = buildIssueWorkStatusMarker(input.repoFullName, input.issueNumber); + const attemptLine = input.attemptSummary ? [`- ${input.attemptSummary}`] : []; switch (input.stage) { case "started": return [ + marker, `⏳ **Issue work started** — beginning work on ${issueLine}.`, "", `- Repo: \`${input.repoFullName}\``, `- Issue: ${issueLine}`, + ...attemptLine, ].join("\n"); case "verifying": return [ + marker, `✅ **Issue work verified** — code changes are ready for PR creation on ${issueLine}.`, "", `- Repo: \`${input.repoFullName}\``, safeDetailLine ? `- ${safeDetailLine}` : "- Verification finished in the worktree.", + ...attemptLine, ].join("\n"); case "failed": return [ + marker, `❌ **Issue work failed** — ${issueLine}.`, "", `- Repo: \`${input.repoFullName}\``, `- Reason: ${safeDetailLine || "No failure details provided."}`, + ...attemptLine, ].join("\n"); } } diff --git a/server/repoWorkspace.test.ts b/server/repoWorkspace.test.ts index 5c6a699..cdce89f 100644 --- a/server/repoWorkspace.test.ts +++ b/server/repoWorkspace.test.ts @@ -1,9 +1,9 @@ import assert from "node:assert/strict"; -import { mkdir, mkdtemp } from "fs/promises"; +import { mkdir, mkdtemp, rm, writeFile } from "fs/promises"; import os from "os"; import path from "path"; import test from "node:test"; -import { ensureRepoCache, preparePrWorktree, removePrWorktree } from "./repoWorkspace"; +import { ensureRepoCache, preparePrWorktree, reclaimOrphanedWorktrees, removePrWorktree } from "./repoWorkspace"; test("preparePrWorktree reuses the watched-repo cache and fetches fork heads on demand", async () => { const rootDir = await mkdtemp(path.join(os.tmpdir(), "codefactory-workspace-")); @@ -293,3 +293,138 @@ test("ensureRepoCache refuses to reclone when registered worktrees still exist o assert.equal(cloneCount, 0); }); + +/** + * Build a repo cache whose `.git/worktrees` holds a registration left behind by a + * run that died before its cleanup could run, plus the worktree directory itself. + * `git worktree prune` cannot clear this: the directory still exists. + */ +async function seedOrphanedWorktree(rootDir: string, name: string): Promise<{ + repoCacheDir: string; + registrationDir: string; + worktreePath: string; +}> { + const repoCacheDir = path.join(rootDir, "repos", "acme__widgets"); + const registrationDir = path.join(repoCacheDir, ".git", "worktrees", name); + const worktreePath = path.join(rootDir, "worktrees", "acme__widgets", name); + + await mkdir(registrationDir, { recursive: true }); + await mkdir(worktreePath, { recursive: true }); + await writeFile(path.join(registrationDir, "gitdir"), `${path.join(worktreePath, ".git")}\n`, "utf8"); + + return { repoCacheDir, registrationDir, worktreePath }; +} + +/** A git stand-in that mirrors the one side effect the reclaim path depends on. */ +function makeReclaimingGit(counters: { clones: number; removals: string[] }) { + return async (command: string, args: string[]) => { + if (command !== "git") { + return { code: 1, stdout: "", stderr: `unexpected command: ${command}` }; + } + + if (args[0] === "clone") { + counters.clones += 1; + return { code: 0, stdout: "cloned\n", stderr: "" }; + } + + if (args[2] === "worktree" && args[3] === "remove") { + const worktreePath = args[args.length - 1]; + counters.removals.push(worktreePath); + // Real git drops the registration as part of `worktree remove`. + const name = path.basename(worktreePath); + await rm(path.join(args[1], ".git", "worktrees", name), { recursive: true, force: true }); + return { code: 0, stdout: "", stderr: "" }; + } + + return { code: 0, stdout: "", stderr: "" }; + }; +} + +test("ensureRepoCache reclaims worktrees stranded by a dead run instead of refusing forever", async () => { + // Goal: cleanup only runs in a `finally` inside each run, so a crash strands the + // worktree. Those registrations used to block every future reclone permanently — + // `git worktree prune` cannot clear them while the directory still exists — which + // killed the repo's automation until a human intervened. + const rootDir = await mkdtemp(path.join(os.tmpdir(), "codefactory-workspace-orphan-")); + const { worktreePath } = await seedOrphanedWorktree(rootDir, "pr-18-run-dead"); + const counters = { clones: 0, removals: [] as string[] }; + + const result = await ensureRepoCache({ + rootDir, + repoFullName: "acme/widgets", + repoCloneUrl: "https://github.com/acme/widgets.git", + forceReclone: true, + runCommand: makeReclaimingGit(counters) as never, + }); + + assert.equal(result.healed, true); + assert.equal(counters.clones, 1, "the reclone must proceed once the orphan is cleared"); + assert.deepEqual(counters.removals, [worktreePath]); +}); + +test("reclaimOrphanedWorktrees sweeps caches and reports what it cleared", async () => { + const rootDir = await mkdtemp(path.join(os.tmpdir(), "codefactory-workspace-sweep-")); + const { repoCacheDir, worktreePath } = await seedOrphanedWorktree(rootDir, "pr-7-run-dead"); + const counters = { clones: 0, removals: [] as string[] }; + + const swept = await reclaimOrphanedWorktrees({ + rootDir, + runCommand: makeReclaimingGit(counters) as never, + }); + + assert.deepEqual(swept, [{ repoCacheDir, reclaimed: 1 }]); + assert.deepEqual(counters.removals, [worktreePath]); + + // A second sweep has nothing left to do. + const again = await reclaimOrphanedWorktrees({ + rootDir, + runCommand: makeReclaimingGit(counters) as never, + }); + assert.deepEqual(again, []); +}); + +test("reclaimOrphanedWorktrees leaves a worktree the current process is still using", async () => { + // The startup sweep may clear everything because the instance lock proves nothing + // is live, but the periodic sweep must never pull a worktree out from under a + // running agent. + const rootDir = await mkdtemp(path.join(os.tmpdir(), "codefactory-workspace-active-")); + const counters = { clones: 0, removals: [] as string[] }; + + const prepared = await preparePrWorktree({ + rootDir, + repoFullName: "acme/widgets", + repoCloneUrl: "https://github.com/acme/widgets.git", + headRepoFullName: "acme/widgets", + headRepoCloneUrl: "https://github.com/acme/widgets.git", + headRef: "feature", + prNumber: 99, + runId: "live-run", + runCommand: (async (command: string, args: string[]) => { + if (command !== "git") { + return { code: 1, stdout: "", stderr: `unexpected command: ${command}` }; + } + if (args[0] === "clone") { + counters.clones += 1; + return { code: 0, stdout: "cloned\n", stderr: "" }; + } + return { code: 0, stdout: "", stderr: "" }; + }) as never, + }); + + // Register it the way git would, so the sweep can see it at all. + const registrationDir = path.join(prepared.repoCacheDir, ".git", "worktrees", "pr-99-live-run"); + await mkdir(registrationDir, { recursive: true }); + await writeFile( + path.join(registrationDir, "gitdir"), + `${path.join(prepared.worktreePath, ".git")}\n`, + "utf8", + ); + + const swept = await reclaimOrphanedWorktrees({ + rootDir, + runCommand: makeReclaimingGit(counters) as never, + }); + + assert.deepEqual(swept, [], "an in-flight worktree is not an orphan"); + assert.deepEqual(counters.removals, []); +}); diff --git a/server/repoWorkspace.ts b/server/repoWorkspace.ts index 9a07581..edd5a60 100644 --- a/server/repoWorkspace.ts +++ b/server/repoWorkspace.ts @@ -1,4 +1,4 @@ -import { mkdir, readdir, rm } from "fs/promises"; +import { mkdir, readdir, readFile, rm } from "fs/promises"; import path from "path"; import { type CommandResult, runCommand } from "./agentRunner"; import { getCodeFactoryPaths } from "./paths"; @@ -33,6 +33,13 @@ type RemovePrWorktreeParams = { const repoMutationLocks = new Map>(); const activeRepoWorkspaceCounts = new Map(); +/** + * Worktrees this process is actively using. Anything registered in a repo cache + * but absent from here is an orphan: the run that owned it died without + * unwinding its cleanup. A single instance lock (see `instanceLock.ts`) means no + * other PatchDeck can own one, so the set is authoritative. + */ +const activeWorktreePaths = new Set(); function summarizeCommandFailure(result: CommandResult): string { return result.stderr.trim() || result.stdout.trim() || "no output"; @@ -127,6 +134,76 @@ async function pruneRegisteredWorktrees(repoCacheDir: string, run: GitRunner): P } } +/** + * Paths of every worktree registered in the cache. Each `.git/worktrees/` + * holds a `gitdir` file pointing at the worktree's own `.git`, so the worktree + * itself is that file's parent directory. + */ +async function listRegisteredWorktreePaths(repoCacheDir: string): Promise { + const worktreesDir = path.join(repoCacheDir, ".git", "worktrees"); + let entries; + try { + entries = await readdir(worktreesDir, { withFileTypes: true }); + } catch (error) { + if (error instanceof Error && "code" in error && error.code === "ENOENT") { + return []; + } + throw error; + } + + const paths: string[] = []; + for (const entry of entries) { + if (!entry.isDirectory()) { + continue; + } + + try { + const gitdir = await readFile(path.join(worktreesDir, entry.name, "gitdir"), "utf8"); + const trimmed = gitdir.trim(); + if (trimmed) { + paths.push(path.dirname(trimmed)); + } + } catch { + // A registration without a readable gitdir pointer is already broken; + // `git worktree prune` is the right tool for it. + } + } + + return paths; +} + +/** + * Remove worktrees registered in this cache that no live run owns, so a crashed + * run cannot strand the cache forever. Callers must already hold the repo + * mutation lock. + */ +async function reclaimOrphanedWorktreesUnlocked( + repoCacheDir: string, + run: GitRunner, + options: { includeActive?: boolean } = {}, +): Promise { + await pruneRegisteredWorktrees(repoCacheDir, run); + + const registered = await listRegisteredWorktreePaths(repoCacheDir); + const orphans = options.includeActive === true + ? registered + : registered.filter((worktreePath) => !activeWorktreePaths.has(worktreePath)); + + let reclaimed = 0; + for (const worktreePath of orphans) { + await runGit(run, ["-C", repoCacheDir, "worktree", "remove", "--force", worktreePath], 30000); + await rm(worktreePath, { recursive: true, force: true }); + activeWorktreePaths.delete(worktreePath); + reclaimed += 1; + } + + if (reclaimed > 0) { + await pruneRegisteredWorktrees(repoCacheDir, run); + } + + return reclaimed; +} + async function countRegisteredWorktrees(repoCacheDir: string): Promise { try { const worktreeEntries = await readdir(path.join(repoCacheDir, ".git", "worktrees"), { withFileTypes: true }); @@ -150,6 +227,14 @@ async function assertRepoCacheCanBeRecloned(repoCacheDir: string, run: GitRunner await pruneRegisteredWorktrees(repoCacheDir, run); + if (await countRegisteredWorktrees(repoCacheDir) > 0) { + // Registrations left by dead runs used to block the reclone permanently: + // `git worktree prune` only clears entries whose directory is already gone, + // so worktrees still on disk kept the count above zero forever and the repo + // could never recover without a human. + await reclaimOrphanedWorktreesUnlocked(repoCacheDir, run); + } + const registeredWorktreeCount = await countRegisteredWorktrees(repoCacheDir); if (registeredWorktreeCount > 0) { throw new Error( @@ -347,6 +432,7 @@ export async function preparePrWorktree(params: PreparePrWorktreeParams): Promis await addWorktree(cache.repoCacheDir, worktreePath, run); adjustActiveRepoWorkspaceCount(cache.repoCacheDir, 1); + activeWorktreePaths.add(worktreePath); return { repoCacheDir: cache.repoCacheDir, worktreePath, @@ -370,6 +456,56 @@ export async function removePrWorktree(params: RemovePrWorktreeParams): Promise< await rm(worktreePath, { recursive: true, force: true }); } finally { adjustActiveRepoWorkspaceCount(repoCacheDir, -1); + activeWorktreePaths.delete(worktreePath); } }); } + +/** + * Reclaim worktrees left behind by runs that never unwound their cleanup. + * + * Cleanup normally happens in a `finally` inside each run, which covers both the + * success and the throw path but not the process dying — a crash, a kill, or a + * machine restart mid-run strands the worktree. Nothing else sweeps them, so they + * accumulated until `assertRepoCacheCanBeRecloned` refused to heal the cache and + * the repo's automation stopped for good. + * + * `includeActive` is for startup, where the single-instance lock guarantees no + * worktree can legitimately be in use, so every registration is an orphan. + */ +export async function reclaimOrphanedWorktrees(params: { + rootDir?: string; + runCommand: GitRunner; + includeActive?: boolean; +}): Promise<{ repoCacheDir: string; reclaimed: number }[]> { + const paths = getCodeFactoryPaths(params.rootDir); + + let repoDirs; + try { + repoDirs = await readdir(paths.repoRootDir, { withFileTypes: true }); + } catch (error) { + if (error instanceof Error && "code" in error && error.code === "ENOENT") { + return []; + } + throw error; + } + + const results: { repoCacheDir: string; reclaimed: number }[] = []; + for (const entry of repoDirs) { + if (!entry.isDirectory()) { + continue; + } + + const repoCacheDir = path.join(paths.repoRootDir, entry.name); + const reclaimed = await withRepoMutationLock(repoCacheDir, async () => + reclaimOrphanedWorktreesUnlocked(repoCacheDir, params.runCommand, { + includeActive: params.includeActive, + })); + + if (reclaimed > 0) { + results.push({ repoCacheDir, reclaimed }); + } + } + + return results; +}