From 27065a65b766253c22cc5cea673cb71183d0fd1b Mon Sep 17 00:00:00 2001 From: Peter Steinberger Date: Thu, 16 Jul 2026 03:13:44 -0700 Subject: [PATCH] fix(scripts): survive pr worktree teardown in lock release and self-heal stale registrations (#108870) --- scripts/pr-lib/common.sh | 30 ++++- scripts/pr-lib/process-group-runner.mjs | 24 +++- scripts/pr-lib/worktree.sh | 13 ++- test/scripts/pr-operation-lock.test.ts | 144 +++++++++++++++++++++++- 4 files changed, 200 insertions(+), 11 deletions(-) diff --git a/scripts/pr-lib/common.sh b/scripts/pr-lib/common.sh index de8b7f918896..6c9fc93d8066 100644 --- a/scripts/pr-lib/common.sh +++ b/scripts/pr-lib/common.sh @@ -251,7 +251,22 @@ is_repo_pr_worktree_dir() { remove_worktree_if_present() { local path="$1" + local registered_path="" + local registered_parent="" + registered_parent=$(resolve_existing_dir_path "$(dirname "$path")" 2>/dev/null || true) + if [ -n "$registered_parent" ]; then + registered_path="$registered_parent/$(basename "$path")" + fi + if [ ! -e "$path" ]; then + # A torn-down PR worktree once left a stale registration that poisoned + # every later worktree add until the registration was pruned. + if [ -n "$registered_path" ] && worktree_is_registered "$registered_path"; then + git worktree prune || true + if worktree_is_registered "$registered_path"; then + echo "Warning: failed to remove registered worktree $path" + fi + fi return 0 fi @@ -260,13 +275,22 @@ remove_worktree_if_present() { return 0 fi - local registered_path - registered_path="$(resolve_existing_dir_path "$(dirname "$path")")/$(basename "$path")" if [ -n "$registered_path" ] && worktree_is_registered "$registered_path"; then - git worktree remove "$registered_path" --force >/dev/null 2>&1 || true + local remove_error + if ! remove_error=$(git worktree remove --force "$registered_path" 2>&1); then + echo "Warning: git worktree remove failed for $path: $remove_error" + fi fi if [ ! -e "$path" ]; then + # See the stale-registration recovery above: removal can delete the path + # while leaving Git's linked-worktree metadata behind. + if [ -n "$registered_path" ] && worktree_is_registered "$registered_path"; then + git worktree prune || true + if worktree_is_registered "$registered_path"; then + echo "Warning: failed to remove registered worktree $path" + fi + fi return 0 fi diff --git a/scripts/pr-lib/process-group-runner.mjs b/scripts/pr-lib/process-group-runner.mjs index 7ae1275deacc..05ee3223bc6b 100644 --- a/scripts/pr-lib/process-group-runner.mjs +++ b/scripts/pr-lib/process-group-runner.mjs @@ -1,6 +1,7 @@ import { spawn, spawnSync } from "node:child_process"; -import { constants } from "node:os"; -import { basename, resolve } from "node:path"; +import { mkdtempSync, readFileSync, rmSync, writeFileSync } from "node:fs"; +import { constants, tmpdir } from "node:os"; +import { basename, join, resolve } from "node:path"; import { fileURLToPath } from "node:url"; const SIGNAL_GRACE_MS = 5000; @@ -20,7 +21,22 @@ if (process.platform === "win32") { } const repoRoot = resolve(repoRootArg); +const invocationCwd = process.cwd(); +// The supervisor must not retain a cwd inside a worktree the operation may +// delete; only the child keeps the caller's original cwd. +process.chdir(repoRoot); const lockScript = fileURLToPath(new URL("./operation-lock.sh", import.meta.url)); +const lockSnapshotDir = mkdtempSync(join(tmpdir(), "openclaw-pr-lock-release-")); +const lockScriptSnapshot = join(lockSnapshotDir, "operation-lock.sh"); +// merge-run can delete this revision's script directory before lock release. +writeFileSync(lockScriptSnapshot, readFileSync(lockScript)); +process.once("exit", () => { + try { + rmSync(lockSnapshotDir, { force: true, recursive: true }); + } catch { + // Best-effort cleanup must not change the operation result. + } +}); const locks = new Map(); let notificationBuffer = ""; let discardingOversizedNotificationLine = false; @@ -138,7 +154,7 @@ for (const signal of FORWARDED_SIGNALS) { } const child = spawn(script, args, { - cwd: process.cwd(), + cwd: invocationCwd, detached: true, env: { ...process.env, @@ -320,7 +336,7 @@ function releaseLock({ lockRef, ownerOid }) { "release_pr_operation_lock", ].join("\n"), "operation-lock-release", - lockScript, + lockScriptSnapshot, repoRoot, lockRef, ownerOid, diff --git a/scripts/pr-lib/worktree.sh b/scripts/pr-lib/worktree.sh index 271858102df1..01711e7b17ab 100644 --- a/scripts/pr-lib/worktree.sh +++ b/scripts/pr-lib/worktree.sh @@ -53,7 +53,18 @@ enter_worktree() { git checkout -B "temp/pr-$pr" origin/main fi else - git worktree add "$dir" -b "temp/pr-$pr" origin/main + local resolved_parent="" + resolved_parent=$(resolve_existing_dir_path "$(dirname "$dir")" 2>/dev/null || true) + local resolved_dir="" + if [ -n "$resolved_parent" ]; then + resolved_dir="$resolved_parent/$(basename "$dir")" + fi + if [ ! -e "$dir" ] && [ -n "$resolved_dir" ] && worktree_is_registered "$resolved_dir"; then + echo "Pruning stale worktree registration for $dir" + git worktree prune + fi + # Per-PR locking makes resetting this script-owned branch namespace safe. + git worktree add "$dir" -B "temp/pr-$pr" origin/main cd "$dir" fi diff --git a/test/scripts/pr-operation-lock.test.ts b/test/scripts/pr-operation-lock.test.ts index 04f103c3f97c..8e9156630aa2 100644 --- a/test/scripts/pr-operation-lock.test.ts +++ b/test/scripts/pr-operation-lock.test.ts @@ -154,18 +154,23 @@ function installPrCliFixture(repoDir: string) { async function runSupervisedFixture( repoDir: string, fixture: string, - options: { accelerateTimeouts?: boolean; env?: NodeJS.ProcessEnv } = {}, + options: { + accelerateTimeouts?: boolean; + cwd?: string; + env?: NodeJS.ProcessEnv; + runner?: string; + } = {}, ) { const controller = spawn( process.execPath, [ ...(options.accelerateTimeouts ? ["--require", createProcessGroupTimingPreload()] : []), - processGroupRunner, + options.runner ?? processGroupRunner, repoDir, fixture, ], { - cwd: repoDir, + cwd: options.cwd ?? repoDir, env: { ...process.env, ...options.env }, stdio: ["ignore", "pipe", "pipe"], }, @@ -1024,6 +1029,43 @@ describePosix("scripts/pr per-PR operation lock", () => { expect(refExists(repoDir)).toBe(false); }); + it("releases the lock after the operation deletes its runner worktree", async () => { + const repoDir = createRepo(); + const doomedDir = tempDirs.make("openclaw-pr-self-deleting-runner-"); + const copiedLibDir = join(doomedDir, "pr-lib"); + mkdirSync(copiedLibDir, { recursive: true }); + for (const file of ["operation-lock.sh", "process-group-runner.mjs"]) { + cpSync(join(repoRoot, "scripts/pr-lib", file), join(copiedLibDir, file)); + } + const copiedRunner = join(copiedLibDir, "process-group-runner.mjs"); + const fixture = join(doomedDir, "delete-own-worktree.sh"); + writeFileSync( + fixture, + [ + "#!/usr/bin/env bash", + "set -euo pipefail", + `source '${join(copiedLibDir, "operation-lock.sh")}'`, + `repo_root() { printf '%s\\n' '${repoDir}'; }`, + "acquire_pr_operation_lock 42", + "echo 'fixture: lock acquired'", + `rm -rf '${doomedDir}'`, + "echo 'fixture: runner worktree deleted'", + ].join("\n"), + ); + chmodSync(fixture, 0o755); + + const result = await runSupervisedFixture(repoDir, fixture, { + cwd: doomedDir, + runner: copiedRunner, + }); + + expect(result.status, `${result.stdout}\n${result.stderr}`).toBe(0); + expect(result.stdout).toContain("fixture: lock acquired"); + expect(result.stdout).toContain("fixture: runner worktree deleted"); + expect(result.stderr).not.toContain("Retaining the operation lock"); + expect(refExists(repoDir)).toBe(false); + }); + it("retains the exact owner when supervisor release cannot take the ref lock", async () => { const repoDir = createRepo(); // Retry counts are covered above; this integration case only needs the real ref-lock failure. @@ -1955,6 +1997,102 @@ describePosix("scripts/pr per-PR operation lock", () => { ).toBe(1); }); + it("prunes a registered worktree whose directory is already gone", () => { + const repoDir = createRepo(); + const worktreeDir = join(repoDir, ".worktrees", "pr-42"); + mkdirSync(dirname(worktreeDir), { recursive: true }); + execFileSync("git", ["worktree", "add", "-q", "-b", "pr-42", worktreeDir], { + cwd: repoDir, + }); + const canonicalWorktreeDir = realpathSync(worktreeDir); + rmSync(worktreeDir, { recursive: true }); + + const result = runLockShell(repoDir, ['remove_worktree_if_present ".worktrees/pr-42"']); + + expect(result.status, `${result.stdout}\n${result.stderr}`).toBe(0); + expect( + execFileSync("git", ["worktree", "list", "--porcelain"], { + cwd: repoDir, + encoding: "utf8", + }), + ).not.toContain(canonicalWorktreeDir); + }); + + it("surfaces git worktree remove stderr without making cleanup fatal", () => { + const repoDir = createRepo(); + const worktreeDir = join(repoDir, ".worktrees", "pr-42"); + mkdirSync(dirname(worktreeDir), { recursive: true }); + execFileSync("git", ["worktree", "add", "-q", "-b", "pr-42", worktreeDir], { + cwd: repoDir, + }); + + const result = runLockShell(repoDir, [ + "git() {", + " if [ \"$1 $2\" = 'worktree remove' ]; then", + " echo 'fixture remove failure' >&2", + " return 1", + " fi", + ' command git "$@"', + "}", + 'remove_worktree_if_present ".worktrees/pr-42"', + ]); + + expect(result.status, `${result.stdout}\n${result.stderr}`).toBe(0); + expect(result.stdout).toContain( + "Warning: git worktree remove failed for .worktrees/pr-42: fixture remove failure", + ); + expect(existsSync(worktreeDir)).toBe(true); + }); + + it("prunes a missing registration and resets its script-owned branch on worktree add", () => { + const repoDir = createRepo(); + execFileSync("git", ["remote", "add", "origin", repoDir], { cwd: repoDir }); + const physicalWorktreesDir = join(repoDir, "linked-worktrees"); + mkdirSync(physicalWorktreesDir); + symlinkSync(physicalWorktreesDir, join(repoDir, ".worktrees"), "dir"); + const worktreeDir = join(repoDir, ".worktrees", "pr-42"); + execFileSync("git", ["worktree", "add", "-q", "-b", "temp/pr-42", worktreeDir], { + cwd: repoDir, + }); + rmSync(worktreeDir, { recursive: true }); + + const result = runLockShell(repoDir, [ + "ensure_gh_api_auth() { return 0; }", + "enter_worktree 42", + ]); + + expect(result.status, `${result.stdout}\n${result.stderr}`).toBe(0); + expect(result.stdout).toContain("Pruning stale worktree registration for .worktrees/pr-42"); + expect(existsSync(worktreeDir)).toBe(true); + expect( + execFileSync("git", ["branch", "--show-current"], { + cwd: worktreeDir, + encoding: "utf8", + }).trim(), + ).toBe("temp/pr-42"); + }); + + it("resets an existing script-owned branch when adding a fresh worktree", () => { + const repoDir = createRepo(); + execFileSync("git", ["remote", "add", "origin", repoDir], { cwd: repoDir }); + execFileSync("git", ["branch", "temp/pr-43"], { cwd: repoDir }); + const worktreeDir = join(repoDir, ".worktrees", "pr-43"); + + const result = runLockShell(repoDir, [ + "ensure_gh_api_auth() { return 0; }", + "enter_worktree 43", + ]); + + expect(result.status, `${result.stdout}\n${result.stderr}`).toBe(0); + expect(existsSync(worktreeDir)).toBe(true); + expect( + execFileSync("git", ["branch", "--show-current"], { + cwd: worktreeDir, + encoding: "utf8", + }).trim(), + ).toBe("temp/pr-43"); + }); + it("refuses a symlink alias to another registered worktree", () => { const repoDir = createRepo(); const worktreesDir = join(repoDir, ".worktrees");