From 94e0b0c70b712da0c14e2964278b7695985bd8b5 Mon Sep 17 00:00:00 2001 From: Jeremy McSpadden <211150+jeremymcs@users.noreply.github.com> Date: Thu, 20 Aug 2026 08:20:54 -0500 Subject: [PATCH] fix: reclaim stranded worktrees and stop repeating issue status comments MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two faults combined to strand a repo permanently and then announce it dozens of times on the issue. Worktree cleanup only ran in a `finally` inside each run, which covers the success and throw paths but not the process dying. A crash, a kill, or a machine restart mid-run left the worktree registered forever, and nothing swept it — `worktreeRootDir` was referenced only where paths are built. Once enough leaked, `assertRepoCacheCanBeRecloned` refused to heal the cache and that repo's automation stopped for good. `git worktree prune` could not help, because it only clears registrations whose directory is already gone. Orphans are now reclaimed: on startup, where the single-instance lock proves none can legitimately be in use; on the watcher tick, for any worktree this process is not actively holding; and before refusing a reclone, so an already stuck cache heals itself. Worktrees belonging to live runs are tracked and never touched, and an unresolvable registration still refuses rather than guessing. Issue status comments were posted with `createComment` every time, with no marker and no dedupe. Each recovery cycle added a fresh "started" and "failed" notice, so a permanently failing issue accumulated identical comments — the PR side already solved this with an anchored reply it edits in place. Issue comments now carry the same kind of hidden marker and update the existing comment, with an attempt count so a reader can still tell it retried. The runtime's worktree sweep is injectable and stubbed in the runtime tests, so the suite never touches the real PatchDeck home. Verified: npm run check, npm run test:all (809 tests), npx eslint ., npm run build. --- server/appRuntime.test.ts | 30 ++++++ server/appRuntime.ts | 37 ++++++- server/backgroundJobHandlers.test.ts | 100 +++++++++++++++++++ server/backgroundJobHandlers.ts | 84 +++++++++++++++- server/issueFormatter.ts | 19 ++++ server/repoWorkspace.test.ts | 139 ++++++++++++++++++++++++++- server/repoWorkspace.ts | 138 +++++++++++++++++++++++++- 7 files changed, 542 insertions(+), 5 deletions(-) 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; +}