From 2c90cf0f2c60d3851880eca3c643c01313af9ffd Mon Sep 17 00:00:00 2001 From: Devin Foley Date: Sun, 2 Aug 2026 22:14:58 -0700 Subject: [PATCH] fix(server): serialize managed-checkout materialization and stop misattributing clone failures (#10723) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Thinking Path > - Paperclip is the open source app people use to manage AI agents for work > - Repo-only project workspaces are materialized by a server-side managed `git clone` into a per-project directory (#10720 added credentials for private repos) > - Two issues on the same project routinely wake seconds apart, and both runs race the same clone target > - The loser fails with "destination path already exists", and its failure cleanup removes the directory out from under the winner's in-progress clone — both runs then fail every round > - This pull request serializes materialization per target directory and makes the clone land atomically via a temp sibling + rename, so the shared target is never partial and never removed > - The benefit is that concurrent runs on the same project converge: one clone happens, everyone adopts it, and unrelated failures no longer blame the GitHub credential ## Linked Issues or Issue Description **What happened?** With isolated workspaces enabled on a project whose only workspace is repo-only, unblocking two issues at once produced lockstep mutual destruction (observed live, two consecutive rounds): both runs called the managed-checkout materialization concurrently; one clone created the target directory, the other's `git clone` failed with `fatal: destination path '…' already exists and is not an empty directory`, and that run's failure cleanup deleted the directory while the first clone was still writing into it (`fatal: could not set 'core…'`). Both runs failed `workspace_validation_failed`; their staggered retries could race again. The failure message also wrongly claimed the GitHub credential "was rejected or lacks access" — the collision had nothing to do with auth. **Expected behavior** Concurrent materializations of the same project checkout share one clone; a completed checkout is never removed by a failing sibling; the credential is only blamed for auth-shaped failures. **Steps to reproduce** 1. Project with a repo-only workspace (private repo, isolated workspaces on). 2. Move two issues on that project to `todo` at the same time so both runs start within seconds. 3. Both runs fail workspace validation with "destination path already exists" / "could not set 'core…'" instead of one clone succeeding. **Paperclip version or commit** `master` (75f6256b76). ## What Changed - `ensureManagedProjectWorkspace` serializes in-flight materializations per target cwd (a module-level promise map): concurrent callers share one attempt. - The clone lands in a `.clone-XXXXXX` temp sibling created with `mkdtemp`, then moves into place with an atomic `rename`. Clone failure removes only the temp directory; the shared target is never created partially and never deleted. If the target appears between the emptiness check and the rename (another process won), the completed checkout is adopted instead of failing the run. - `describeGitAuthFailure` attributes the GitHub credential only when the error matches the auth-failure pattern; unrelated failures (path collisions, network errors) no longer claim the token was rejected. ## Verification - `cd server && npx vitest run src/__tests__/heartbeat-managed-clone-credentials.test.ts src/__tests__/git-credentials.test.ts src/__tests__/heartbeat-workspace-session.test.ts` — 169 tests pass, including new cases: concurrent materializations of the same checkout succeed with one shared result and no temp litter; failed clones leave neither target nor temp directories; an authenticated clone failing for non-auth reasons does not blame the credential. - `pnpm --filter @paperclipai/server typecheck` — clean. ## Risks - Low risk. The serialization is in-process and keyed by exact target path; the temp+rename pattern stays on the same filesystem (sibling path) so the rename is atomic. Single-run behavior is byte-identical apart from the temp-dir intermediate. - The rename-conflict adoption path accepts a checkout another materialization completed; the pre-existing `gitDirExists` adoption semantics are unchanged. ## Model Used Claude Fable 5 (`claude-fable-5`, extended thinking, agentic tool use via Claude Code CLI). ## Checklist - [x] I have included a thinking path that traces from project context to this change - [x] I have specified the model used (with version and capability details) - [x] I have checked ROADMAP.md and confirmed this PR does not duplicate planned core work - [x] I have searched GitHub for duplicate or related PRs and linked them above - [x] I have either (a) linked existing issues with `Fixes: #` / `Closes #` / `Refs #` OR (b) described the issue in-PR following the relevant issue template - [x] I have not referenced internal/instance-local Paperclip issues or links (only public GitHub `#NNN` / `github.com/paperclipai/paperclip` URLs) - [x] My branch name describes the change (e.g. `docs/...`, `fix/...`) and contains no internal Paperclip ticket id or instance-derived details - [x] I have run tests locally and they pass - [x] I have added or updated tests where applicable - [x] I have updated relevant documentation to reflect my changes - [x] I have considered and documented any risks above - [ ] All Paperclip CI gates are green - [ ] Greptile is 5/5 with no open P2s, recommendations, or follow-ups - [x] I will address all Greptile and reviewer comments before requesting merge --- server/src/__tests__/git-credentials.test.ts | 9 +++ ...eartbeat-managed-clone-credentials.test.ts | 48 ++++++++++++-- server/src/services/git-credentials.ts | 15 ++--- server/src/services/heartbeat.ts | 62 +++++++++++++++---- 4 files changed, 110 insertions(+), 24 deletions(-) diff --git a/server/src/__tests__/git-credentials.test.ts b/server/src/__tests__/git-credentials.test.ts index 17fed11736..deba88d578 100644 --- a/server/src/__tests__/git-credentials.test.ts +++ b/server/src/__tests__/git-credentials.test.ts @@ -272,6 +272,15 @@ describe("describeGitAuthFailure", () => { used: null, })).toBeNull(); }); + + it("stays silent for non-auth failures even when a credential was used", () => { + // A credential present during an unrelated failure (network outage, target-path + // collision) must not be blamed for it. + expect(describeGitAuthFailure({ + error: "fatal: destination path '/x/y' already exists and is not an empty directory.", + used: { source: "company_secret", secretName: "GH_TOKEN" }, + })).toBeNull(); + }); }); describe("DEFAULT_GITHUB_TOKEN_SECRET_NAMES", () => { diff --git a/server/src/__tests__/heartbeat-managed-clone-credentials.test.ts b/server/src/__tests__/heartbeat-managed-clone-credentials.test.ts index 146a11df50..2a0f177885 100644 --- a/server/src/__tests__/heartbeat-managed-clone-credentials.test.ts +++ b/server/src/__tests__/heartbeat-managed-clone-credentials.test.ts @@ -72,22 +72,56 @@ describe("ensureManagedProjectWorkspace clone credentials", () => { } }); - it("names the company-secret credential when an authenticated clone fails", async () => { + it("does not blame the credential when an authenticated clone fails for non-auth reasons", async () => { + // The failure here is a missing local path, not an auth rejection — the error must not + // claim the credential "was rejected". Attribution for genuinely auth-shaped failures is + // covered by the describeGitAuthFailure unit tests in git-credentials.test.ts. const missingRepo = path.join(os.tmpdir(), "paperclip-definitely-missing", "repo.git"); const resolveGitAuth = vi.fn(async () => ({ - // Empty configArgs keep this offline: the failure comes from the missing local path, - // the message must still attribute the credential that was in play. configArgs: [], env: { [GIT_CREDENTIAL_TOKEN_ENV_KEY]: "token", GIT_TERMINAL_PROMPT: "0" }, source: "company_secret" as const, secretName: "GH_TOKEN", })); - await expect(ensureManagedProjectWorkspace({ + const error = await ensureManagedProjectWorkspace({ companyId: "company-authfail", projectId: "project-1", repoUrl: missingRepo, resolveGitAuth, - })).rejects.toThrow(/the GH_TOKEN company-secret GitHub credential/); + }).then( + () => { throw new Error("expected the clone to fail"); }, + (err: unknown) => err as Error, + ); + expect(error.message).toContain("Failed to prepare managed checkout"); + expect(error.message).not.toContain("GH_TOKEN company-secret GitHub credential"); + }); + + it("serializes concurrent materializations of the same managed checkout", async () => { + const sourceRepo = await createLocalSourceRepo(); + try { + const [first, second] = await Promise.all([ + ensureManagedProjectWorkspace({ + companyId: "company-concurrent", + projectId: "project-1", + repoUrl: sourceRepo, + }), + ensureManagedProjectWorkspace({ + companyId: "company-concurrent", + projectId: "project-1", + repoUrl: sourceRepo, + }), + ]); + expect(first.cwd).toBe(second.cwd); + expect(first.warning).toBeNull(); + expect(second.warning).toBeNull(); + const gitDir = await fs.stat(path.join(first.cwd, ".git")); + expect(gitDir.isDirectory()).toBe(true); + // No temp clone directories left behind next to the target. + const siblings = await fs.readdir(path.dirname(first.cwd)); + expect(siblings.filter((name) => name.includes(".clone-"))).toEqual([]); + } finally { + await fs.rm(sourceRepo, { recursive: true, force: true }); + } }); it("does not mention credentials when an unauthenticated clone fails for non-auth reasons", async () => { @@ -106,7 +140,7 @@ describe("ensureManagedProjectWorkspace clone credentials", () => { expect(error.message).not.toContain("company secret"); }); - it("removes the partially created directory when the clone fails", async () => { + it("leaves neither the target nor temp directories behind when the clone fails", async () => { const missingRepo = path.join(os.tmpdir(), "paperclip-definitely-missing", "repo.git"); const companyId = "company-cleanup"; const projectId = "project-1"; @@ -118,6 +152,8 @@ describe("ensureManagedProjectWorkspace clone credentials", () => { // Filesystem-path repo "URLs" derive no repo name, so the managed dir is the _default slot. const cwd = resolveManagedProjectWorkspaceDir({ companyId, projectId }); await expect(fs.stat(cwd)).rejects.toMatchObject({ code: "ENOENT" }); + const siblings = await fs.readdir(path.dirname(cwd)); + expect(siblings.filter((name) => name.includes(".clone-"))).toEqual([]); }); it("keeps using a pre-existing non-git directory as-is without attempting a clone", async () => { diff --git a/server/src/services/git-credentials.ts b/server/src/services/git-credentials.ts index 4675103fd0..520ee6e81a 100644 --- a/server/src/services/git-credentials.ts +++ b/server/src/services/git-credentials.ts @@ -110,24 +110,25 @@ const GIT_AUTH_FAILURE_PATTERN = /authentication failed|could not read username|could not read password|invalid username or password|terminal prompts disabled|repository not found|not accessible|permission denied|HTTP 40[13]|The requested URL returned error: 40[13]/i; /** - * Turn a failed authenticated (or unauthenticated) git network operation into an actionable - * suffix for the error message. Returns null when the failure does not look auth-related and - * no credential was in play. + * Turn a failed git network operation into an actionable suffix for the error message. + * Returns null when the failure does not look auth-related — a credential that was merely + * present during an unrelated failure (network outage, target-path collision) must not be + * blamed for it. */ export function describeGitAuthFailure(input: { error: string; used: { source: GitCredential["source"]; secretName: string | null } | null; }): string | null { + if (!GIT_AUTH_FAILURE_PATTERN.test(input.error)) { + return null; + } if (input.used) { const label = input.used.secretName ? `the ${input.used.secretName} company-secret GitHub credential` : "the server-environment GitHub credential"; return `The operation authenticated with ${label}, which was rejected or lacks access to this repository.`; } - if (GIT_AUTH_FAILURE_PATTERN.test(input.error)) { - return "No GitHub credential is configured — add a GITHUB_TOKEN or GH_TOKEN company secret in Settings → Secrets, or configure a local checkout cwd for this project workspace."; - } - return null; + return "No GitHub credential is configured — add a GITHUB_TOKEN or GH_TOKEN company secret in Settings → Secrets, or configure a local checkout cwd for this project workspace."; } type SecretServiceLike = ReturnType; diff --git a/server/src/services/heartbeat.ts b/server/src/services/heartbeat.ts index 2413c46af0..f027ab3d9c 100644 --- a/server/src/services/heartbeat.ts +++ b/server/src/services/heartbeat.ts @@ -1498,6 +1498,14 @@ function deriveRepoNameFromRepoUrl(repoUrl: string | null): string | null { } } +/** + * In-flight managed-checkout materializations keyed by target cwd. Two issues on the same + * project can wake within seconds of each other; without this, both runs raced the same + * clone target — the loser saw "destination path already exists" and its failure cleanup + * deleted the winner's in-progress clone, so both runs failed every round. + */ +const managedCheckoutMaterializations = new Map>(); + export async function ensureManagedProjectWorkspace(input: { companyId: string; projectId: string; @@ -1510,6 +1518,22 @@ export async function ensureManagedProjectWorkspace(input: { projectId: input.projectId, repoName: deriveRepoNameFromRepoUrl(input.repoUrl), }); + const inFlight = managedCheckoutMaterializations.get(cwd); + if (inFlight) return inFlight; + const attempt = materializeManagedProjectWorkspace(cwd, input).finally(() => { + managedCheckoutMaterializations.delete(cwd); + }); + managedCheckoutMaterializations.set(cwd, attempt); + return attempt; +} + +async function materializeManagedProjectWorkspace( + cwd: string, + input: { + repoUrl: string | null; + resolveGitAuth?: GitRemoteAuthProvider | null; + }, +): Promise<{ cwd: string; warning: string | null }> { await fs.mkdir(path.dirname(cwd), { recursive: true }); const stats = await fs.stat(cwd).catch(() => null); @@ -1520,11 +1544,12 @@ export async function ensureManagedProjectWorkspace(input: { return { cwd, warning: null }; } - const gitDirExists = await fs - .stat(path.resolve(cwd, ".git")) - .then((entry) => entry.isDirectory()) - .catch(() => false); - if (gitDirExists) { + const hasAdoptableGitDir = () => + fs + .stat(path.resolve(cwd, ".git")) + .then((entry) => entry.isDirectory()) + .catch(() => false); + if (await hasAdoptableGitDir()) { return { cwd, warning: null }; } @@ -1539,9 +1564,14 @@ export async function ensureManagedProjectWorkspace(input: { await fs.rm(cwd, { recursive: true, force: true }); } + // Clone into a temp sibling, then move into place atomically. The shared target directory + // is never created in a partial state and never removed on failure, so a concurrent + // materialization (another process, or a run racing this one) can neither adopt a broken + // checkout nor lose its own completed one. const auth = input.resolveGitAuth ? await input.resolveGitAuth(input.repoUrl) : null; + const cloneTmpDir = await fs.mkdtemp(`${cwd}.clone-`); try { - await execFile("git", [...(auth?.configArgs ?? []), "clone", input.repoUrl, cwd], { + await execFile("git", [...(auth?.configArgs ?? []), "clone", input.repoUrl, cloneTmpDir], { env: { // Spread order matters: the sanitizer strips PAPERCLIP_*, which would remove the // credential-helper token env if it came first. GIT_TERMINAL_PROMPT=0 fails a @@ -1553,12 +1583,8 @@ export async function ensureManagedProjectWorkspace(input: { }, timeout: MANAGED_WORKSPACE_GIT_CLONE_TIMEOUT_MS, }); - return { cwd, warning: null }; } catch (error) { - // The clone only ever starts from a missing or just-emptied directory, so removing it on - // failure cannot destroy operator data — but leaving it would poison the next run: a - // timeout-killed clone leaves a partial .git that the gitDirExists probe above adopts. - await fs.rm(cwd, { recursive: true, force: true }).catch(() => undefined); + await fs.rm(cloneTmpDir, { recursive: true, force: true }).catch(() => undefined); const reason = error instanceof Error ? error.message : String(error); const authNote = describeGitAuthFailure({ error: reason, @@ -1568,6 +1594,20 @@ export async function ensureManagedProjectWorkspace(input: { `Failed to prepare managed checkout for "${input.repoUrl}" at "${cwd}": ${reason}${authNote ? ` ${authNote}` : ""}`, )); } + + try { + await fs.rename(cloneTmpDir, cwd); + } catch (renameError) { + await fs.rm(cloneTmpDir, { recursive: true, force: true }).catch(() => undefined); + // The target appearing between the emptiness check and the rename means another + // materialization won the race; adopt its checkout instead of failing the run. + if (await hasAdoptableGitDir()) { + return { cwd, warning: null }; + } + const reason = renameError instanceof Error ? renameError.message : String(renameError); + throw new Error(`Failed to move managed checkout into place at "${cwd}": ${reason}`); + } + return { cwd, warning: null }; } /**