mirror of
https://github.com/openclaw/openclaw.git
synced 2026-08-12 21:53:00 -06:00
fix(scripts): survive pr worktree teardown in lock release and self-heal stale registrations (#108870)
This commit is contained in:
Peter Steinberger
committed by
GitHub
GitHub
parent
05c2701e3d
commit
27065a65b7
@@ -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
|
||||
|
||||
|
||||
@@ -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,
|
||||
|
||||
@@ -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
|
||||
|
||||
|
||||
@@ -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");
|
||||
|
||||
Reference in New Issue
Block a user