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 }; } /**