fix(server): serialize managed-checkout materialization and stop misattributing clone failures (#10723)
## 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 `<target>.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
This commit is contained in:
parent
75f6256b76
commit
2c90cf0f2c
|
|
@ -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", () => {
|
||||
|
|
|
|||
|
|
@ -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 () => {
|
||||
|
|
|
|||
|
|
@ -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<typeof secretService>;
|
||||
|
|
|
|||
|
|
@ -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<string, Promise<{ cwd: string; warning: string | null }>>();
|
||||
|
||||
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 };
|
||||
}
|
||||
|
||||
/**
|
||||
|
|
|
|||
Loading…
Reference in New Issue