From 6726f78238bfccbe011df09d4de21250b25755ac Mon Sep 17 00:00:00 2001 From: Dotta Date: Sat, 10 Oct 2026 12:08:58 -0500 Subject: [PATCH] fix(computers): serialize shared project checkout setup Hold a confined per-project flock across clone, repository validation, checkout, and worktree creation so another agent cannot observe partially created Git state. Co-Authored-By: Paperclip --- .../modules/computers/adapters/boat.test.ts | 63 +++++++++++++++- .../computers/adapters/remote-program.ts | 71 +++++++++++-------- 2 files changed, 103 insertions(+), 31 deletions(-) diff --git a/server/src/modules/computers/adapters/boat.test.ts b/server/src/modules/computers/adapters/boat.test.ts index 2c2535116b0..5485df95652 100644 --- a/server/src/modules/computers/adapters/boat.test.ts +++ b/server/src/modules/computers/adapters/boat.test.ts @@ -13,7 +13,7 @@ import { import { createHash, randomUUID } from "node:crypto"; import { tmpdir } from "node:os"; import { join } from "node:path"; -import { spawnSync } from "node:child_process"; +import { spawn, spawnSync } from "node:child_process"; import { remoteProgram } from "./remote-program.js"; import { afterEach, describe, it, expect, vi } from "vitest"; import { boatBackend, createBoatTransportAdmission, desktopReadinessProgram, processProgram } from "./boat.js"; @@ -350,6 +350,67 @@ process.stdout.write(result.stdout||"");process.stderr.write(result.stderr||""); expect(config).not.toContain("credential"); expect(spawnSync(realGit, ["rev-parse", "HEAD"], { cwd: result.remoteCwd }).status).toBe(0); }); + it.each(["shared", "worktree"])("serializes concurrent first checkout creation for %s workspaces", async mode => { + const f = fixture(); + const source = join(f.temp, "source"), bin = join(f.temp, "bin"); + mkdirSync(source); mkdirSync(bin); + const git = spawnSync("which", ["git"], { encoding: "utf8" }).stdout.trim(); + for (const args of [["init"], ["-c", "user.name=Test", "-c", "user.email=test@example.com", "commit", "--allow-empty", "-m", "base"]]) { + expect(spawnSync(git, args, { cwd: source, encoding: "utf8" }).status).toBe(0); + } + const started = join(f.temp, "clone-started"), release = join(f.temp, "clone-release"), clones = join(f.temp, "clones"); + // Pause real Git cloning after its destination exists. The second actual + // remote program must wait instead of inspecting this unfinished checkout. + writeFileSync(join(bin, "git"), `#!${process.execPath} +const fs=require("node:fs"),{spawnSync}=require("node:child_process"); +const args=process.argv.slice(2),clone=args.indexOf("clone"); +if(clone>=0){ + fs.appendFileSync(process.env.FIXTURE_CLONES,"clone\\n"); + fs.mkdirSync(args.at(-1),{recursive:true});fs.writeFileSync(process.env.FIXTURE_STARTED,"ready"); + const deadline=Date.now()+5000; + while(!fs.existsSync(process.env.FIXTURE_RELEASE)){if(Date.now()>deadline)process.exit(90);Atomics.wait(new Int32Array(new SharedArrayBuffer(4)),0,0,10);} + const url=args[clone+2];args[clone+2]=process.env.FIXTURE_SOURCE; + const result=spawnSync(process.env.FIXTURE_GIT,args,{encoding:"utf8"}); + if(result.status!==0){process.stderr.write(result.stderr);process.exit(result.status??1);} + process.exit(spawnSync(process.env.FIXTURE_GIT,["-C",args.at(-1),"remote","set-url","origin",url]).status??1); +} +const result=spawnSync(process.env.FIXTURE_GIT,args,{encoding:"utf8"}); +process.stdout.write(result.stdout||"");process.stderr.write(result.stderr||"");process.exit(result.status??1); +`, { mode: 0o700 }); + const env = { ...process.env, PATH: `${bin}:${process.env.PATH}`, FIXTURE_GIT: git, FIXTURE_SOURCE: source, + FIXTURE_STARTED: started, FIXTURE_RELEASE: release, FIXTURE_CLONES: clones }; + const call = (taskId: string) => new Promise<{ remoteCwd: string }>((resolve, reject) => { + const child = spawn("python3", ["-c", remoteProgram.replaceAll("/home/user/paperclip/", `${f.temp}/`)], { env }); + let stdout = "", stderr = ""; + child.stdout.on("data", value => { stdout += value; }); child.stderr.on("data", value => { stderr += value; }); + child.on("error", reject); child.on("close", code => { + if (code !== 0) reject(new Error(stderr)); + else { try { resolve(JSON.parse(stdout)); } catch (error) { reject(error); } } + }); + child.stdin.end(JSON.stringify({ root: f.root, action: "workspace", mode, taskId, repositoryUrl: "https://github.com/company/shared.git" })); + }); + const first = call("first"); + await vi.waitFor(() => expect(existsSync(started)).toBe(true)); + const second = call("second"); + let status: unknown; + try { + status = await Promise.race([second.then(() => "completed"), new Promise(resolve => setTimeout(() => resolve("waiting"), 100))]); + } finally { writeFileSync(release, "continue"); } + const results = await Promise.all([first, second]); + expect(status).toBe("waiting"); + expect(readFileSync(clones, "utf8").trim().split("\n")).toHaveLength(1); + for (const [index, result] of results.entries()) { + expect(result.remoteCwd).toBe(mode === "shared" ? join(f.root, "checkout") : join(f.root, "tasks", index === 0 ? "first" : "second")); + expect(spawnSync(git, ["rev-parse", "--show-toplevel"], { cwd: result.remoteCwd, encoding: "utf8" }).stdout.trim()).toBe(result.remoteCwd); + } + }); + it("rejects substituted project locks without touching their target", () => { + const f = fixture(); mkdirSync(f.root); + const outside = join(f.temp, "outside"); writeFileSync(outside, "keep"); + symlinkSync(outside, join(f.root, ".paperclip-workspace.lock")); + expect(f.call({ action: "workspace", mode: "shared" })).toEqual({ error: "invalid" }); + expect(readFileSync(outside, "utf8")).toBe("keep"); + }); it.each([false, true])("uses the requested worktree branch separately from its start ref (explicit base: %s)", explicitBase => { const f = fixture(); const checkout = join(f.root, "checkout"); diff --git a/server/src/modules/computers/adapters/remote-program.ts b/server/src/modules/computers/adapters/remote-program.ts index e6697a06ed4..f6d0e7afd0e 100644 --- a/server/src/modules/computers/adapters/remote-program.ts +++ b/server/src/modules/computers/adapters/remote-program.ts @@ -60,36 +60,47 @@ if act=='seed': sys.exit(0) if act=='workspace': os.makedirs(root,exist_ok=True) - repo=p.get('repositoryUrl');branch=p.get('branch');task=p.get('taskId');base_ref=p.get('baseRef') - if base_ref and (base_ref.startswith('-') or '\n' in base_ref):fail('invalid') - checkout=os.path.join(root,'checkout') - if repo: - if not (repo.startswith('https://') or repo.startswith('ssh://') or repo.startswith('git@')) or '\n' in repo:fail('invalid') - if not os.path.exists(checkout): - # Resolve credentials on the controller for this operation. Only the clone - # child receives the helper environment; no credential file or Git config persists. - auth=p.get('gitAuth') or {};git_env=dict(os.environ) - git_env.update({'GIT_TERMINAL_PROMPT':'0','GIT_CONFIG_GLOBAL':'/dev/null','GIT_CONFIG_NOSYSTEM':'1'}) - git_env.update(auth.get('env',{})) - run(['git']+auth.get('configArgs',[])+['clone','--',repo,checkout],env=git_env) - elif run(['git','remote','get-url','origin'],checkout)!=repo:fail('conflict') - else: - os.makedirs(checkout,exist_ok=True) - if not os.path.isdir(os.path.join(checkout,'.git')):run(['git','init',checkout]) - if branch: - if branch.startswith('-') or '\n' in branch:fail('invalid') - run(['git','check-ref-format','--branch',branch]) - if p.get('mode')=='worktree': - if not task:fail('invalid') - target=os.path.join(root,'tasks',task);os.makedirs(os.path.dirname(target),exist_ok=True) - task_branch=branch or 'paperclip/task-'+task - if not os.path.exists(target):run(['git','worktree','add','-b',task_branch,target,base_ref or 'HEAD'],checkout) - run(['git','rev-parse','--show-toplevel'],target) - if run(['git','symbolic-ref','--quiet','--short','HEAD'],target)!=task_branch:fail('conflict') - else: - target=checkout - if branch:run(['git','checkout',branch],checkout) - print(json.dumps({'remoteCwd':target}));sys.exit(0) + # The root is the company/project placement on this physical computer. Keep + # clone validation, shared checkout, and worktree creation under one flock. + rootfd=os.open(root,os.O_RDONLY|os.O_DIRECTORY|os.O_NOFOLLOW) + try: + lock=os.open('.paperclip-workspace.lock',os.O_CREAT|os.O_RDWR|os.O_NOFOLLOW,0o600,dir_fd=rootfd) + except OSError:fail('invalid') + finally:os.close(rootfd) + with os.fdopen(lock,'r+') as workspace_lock: + info=os.fstat(workspace_lock.fileno()) + if not stat.S_ISREG(info.st_mode) or info.st_nlink!=1:fail('invalid') + fcntl.flock(workspace_lock,fcntl.LOCK_EX) + repo=p.get('repositoryUrl');branch=p.get('branch');task=p.get('taskId');base_ref=p.get('baseRef') + if base_ref and (base_ref.startswith('-') or '\n' in base_ref):fail('invalid') + checkout=os.path.join(root,'checkout') + if repo: + if not (repo.startswith('https://') or repo.startswith('ssh://') or repo.startswith('git@')) or '\n' in repo:fail('invalid') + if not os.path.exists(checkout): + # Resolve credentials on the controller for this operation. Only the clone + # child receives the helper environment; no credential file or Git config persists. + auth=p.get('gitAuth') or {};git_env=dict(os.environ) + git_env.update({'GIT_TERMINAL_PROMPT':'0','GIT_CONFIG_GLOBAL':'/dev/null','GIT_CONFIG_NOSYSTEM':'1'}) + git_env.update(auth.get('env',{})) + run(['git']+auth.get('configArgs',[])+['clone','--',repo,checkout],env=git_env) + elif run(['git','remote','get-url','origin'],checkout)!=repo:fail('conflict') + else: + os.makedirs(checkout,exist_ok=True) + if not os.path.isdir(os.path.join(checkout,'.git')):run(['git','init',checkout]) + if branch: + if branch.startswith('-') or '\n' in branch:fail('invalid') + run(['git','check-ref-format','--branch',branch]) + if p.get('mode')=='worktree': + if not task:fail('invalid') + target=os.path.join(root,'tasks',task);os.makedirs(os.path.dirname(target),exist_ok=True) + task_branch=branch or 'paperclip/task-'+task + if not os.path.exists(target):run(['git','worktree','add','-b',task_branch,target,base_ref or 'HEAD'],checkout) + run(['git','rev-parse','--show-toplevel'],target) + if run(['git','symbolic-ref','--quiet','--short','HEAD'],target)!=task_branch:fail('conflict') + else: + target=checkout + if branch:run(['git','checkout',branch],checkout) + print(json.dumps({'remoteCwd':target}));sys.exit(0) # Hold directory descriptors throughout each operation. Ancestor symlink swaps cannot # redirect a checked pathname outside the selected placement. def directory(parts,create=False,start=None):