diff --git a/scripts/AGENTS.md b/scripts/AGENTS.md index f7b1bd269ef7..033c89b4ae17 100644 --- a/scripts/AGENTS.md +++ b/scripts/AGENTS.md @@ -20,6 +20,7 @@ This directory owns local tooling, script wrappers, and generated-artifact helpe ## PR Prepare Gates +- `scripts/pr` serializes review, prepare, and merge operations per PR across linked worktrees; `scripts/pr gc` skips active or indeterminate locks. A successful command return is the trusted synchronous-completion contract: every PR-state-mutating child must be joined before returning, and such work must never daemonize or explicitly escape both the operation group and lock-notification FD. Failed, interrupted, or controller-lost operations stay locked because detached children cannot be disproved; after verifying no child tools remain, use the reported exact-OID `scripts/pr lock-recover` command. Never bypass or delete these refs manually. - `scripts/pr prepare-gates` holds the heavy-check lock for its whole local gate block (`scripts/pr-gates-lock.mjs`), so concurrent gate runs across `.worktrees` queue as units instead of dying on child lock timeouts or vitest no-output watchdog kills. - `OPENCLAW_PR_GATES_REMOTE=testbox` runs the full-suite `pnpm test` gate on a Blacksmith Testbox through `scripts/crabbox-wrapper.mjs` (same delegation as `check:changed`); `pnpm build`/`pnpm check` stay local. The `tbx_` lease id and Actions run URL land in `.local/gates.env` (`REMOTE_GATES_*`) and `.local/prep.md`. Use it for reviewed trusted code when a loaded host makes the local 88-shard run stall-kill; contributor/fork code stays on secretless CI or sanitized AWS unless a maintainer explicitly approves credentialed execution. diff --git a/scripts/pr b/scripts/pr index 1f38df3b2e08..11f992b90876 100755 --- a/scripts/pr +++ b/scripts/pr @@ -21,6 +21,32 @@ if common_git_dir=$(git -C "$script_parent_dir" rev-parse --path-format=absolute fi fi +is_locked_pr_command() { + case "$1" in + review-init | review-checkout-main | review-checkout-pr | review-claim | review-guard | review-artifacts-init | review-validate-artifacts | review-tests | prepare-init | prepare-validate-commit | prepare-gates | prepare-push | prepare-sync-head | prepare-run | merge-verify | merge-run) return 0 ;; + *) return 1 ;; + esac +} + +is_supervised_pr_process() { + [ "${OPENCLAW_PR_DEDICATED_PROCESS_GROUP:-}" = "1" ] && + [ "${OPENCLAW_PR_LOCK_NOTIFY_FD:-}" = "3" ] && + [ "${OPENCLAW_PR_LOCK_SUPERVISOR_PID:-}" = "$PPID" ] +} + +if [ "${1-}" = "gc" ] || is_locked_pr_command "${1-}"; then + if is_supervised_pr_process; then + # Do not leak the one-shot marker to tools or nested wrapper calls. + unset OPENCLAW_PR_DEDICATED_PROCESS_GROUP + else + unset OPENCLAW_PR_DEDICATED_PROCESS_GROUP + unset OPENCLAW_PR_LOCK_NOTIFY_FD + unset OPENCLAW_PR_LOCK_SUPERVISOR_PID + command -v node >/dev/null 2>&1 || { echo "Missing required command: node" >&2; exit 1; } + exec node "$script_parent_dir/pr-lib/process-group-runner.mjs" "$script_parent_dir/.." "$script_self" "$@" + fi +fi + # shellcheck disable=SC1091 source "$script_parent_dir/lib/plain-gh.sh" @@ -29,6 +55,7 @@ usage() { Usage: scripts/pr ls scripts/pr gc [--dry-run] + scripts/pr lock-recover --confirmed-no-running-tools scripts/pr review-init scripts/pr review-checkout-main scripts/pr review-checkout-pr @@ -75,6 +102,8 @@ gh() { # shellcheck disable=SC1091 source "$script_parent_dir/pr-lib/worktree.sh" # shellcheck disable=SC1091 +source "$script_parent_dir/pr-lib/operation-lock.sh" +# shellcheck disable=SC1091 source "$script_parent_dir/pr-lib/common.sh" # shellcheck disable=SC1091 source "$script_parent_dir/pr-lib/changelog.sh" @@ -95,18 +124,56 @@ main() { exit 2 fi - require_cmds - local cmd="${1-}" shift || true + if [ "$cmd" = "lock-recover" ]; then + local pr="${1-}" + local owner_oid="${2-}" + local confirmation="${3-}" + [ -n "$pr" ] && [ -n "$owner_oid" ] && [ "$#" -eq 3 ] || { usage; exit 2; } + recover_pr_operation_lock "$pr" "$owner_oid" "$confirmation" + return + fi + + case "$cmd" in + ls) ;; + gc) + [ "$#" -eq 0 ] || { [ "$#" -eq 1 ] && [ "$1" = "--dry-run" ]; } || { + usage + exit 2 + } + ;; + review-tests) + [ "$#" -ge 2 ] || { usage; exit 2; } + ;; + review-init | review-checkout-main | review-checkout-pr | review-claim | review-guard | review-artifacts-init | review-validate-artifacts | prepare-init | prepare-validate-commit | prepare-gates | prepare-push | prepare-sync-head | prepare-run | merge-verify | merge-run) + [ "$#" -ge 1 ] || { usage; exit 2; } + ;; + *) + usage + exit 2 + ;; + esac + + require_cmds + + if is_locked_pr_command "$cmd"; then + local locked_pr="${1-}" + acquire_pr_operation_lock "$locked_pr" + trap 'exit 129' HUP + trap 'exit 130' INT + trap 'exit 131' QUIT + trap 'exit 143' TERM + fi + case "$cmd" in ls) list_pr_worktrees ;; gc) local dry_run=false - if [ "${1-}" = "--dry-run" ]; then + if [ "$#" -eq 1 ]; then dry_run=true fi gc_pr_worktrees "$dry_run" diff --git a/scripts/pr-lib/common.sh b/scripts/pr-lib/common.sh index 5788e91d5945..de8b7f918896 100644 --- a/scripts/pr-lib/common.sh +++ b/scripts/pr-lib/common.sh @@ -185,38 +185,30 @@ common_repo_root() { worktree_path_for_branch() { local branch="$1" local ref="refs/heads/$branch" - - git worktree list --porcelain | awk -v ref="$ref" ' - /^worktree / { - worktree=$2 - next - } - /^branch / { - if ($2 == ref) { - print worktree - found=1 - } - } - END { - if (!found) { - exit 1 - } - } - ' + local field worktree="" + while IFS= read -r -d '' field; do + case "$field" in + worktree\ *) worktree="${field#worktree }" ;; + "branch $ref") + [ -n "$worktree" ] || return 1 + printf '%s\n' "$worktree" + return 0 + ;; + "") worktree="" ;; + esac + done < <(git worktree list --porcelain -z) + return 1 } worktree_is_registered() { local path="$1" - git worktree list --porcelain | awk -v target="$path" ' - /^worktree / { - if ($2 == target) { - found=1 - } - } - END { - exit found ? 0 : 1 - } - ' + local field + while IFS= read -r -d '' field; do + case "$field" in + worktree\ *) [ "${field#worktree }" = "$path" ] && return 0 ;; + esac + done < <(git worktree list --porcelain -z) + return 1 } resolve_existing_dir_path() { @@ -263,20 +255,27 @@ remove_worktree_if_present() { return 0 fi - if worktree_is_registered "$path"; then - git worktree remove "$path" --force >/dev/null 2>&1 || true + if [ -L "$path" ] || ! is_repo_pr_worktree_dir "$path"; then + echo "Warning: refusing to remove non-canonical PR-worktree path $path" + 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 fi if [ ! -e "$path" ]; then return 0 fi - if worktree_is_registered "$path"; then + if [ -n "$registered_path" ] && worktree_is_registered "$registered_path"; then echo "Warning: failed to remove registered worktree $path" return 0 fi - if ! is_repo_pr_worktree_dir "$path"; then + if [ -L "$path" ] || ! is_repo_pr_worktree_dir "$path"; then echo "Warning: refusing to trash non-PR-worktree path $path" return 0 fi diff --git a/scripts/pr-lib/operation-lock.sh b/scripts/pr-lib/operation-lock.sh new file mode 100644 index 000000000000..e2a1de6c5df1 --- /dev/null +++ b/scripts/pr-lib/operation-lock.sh @@ -0,0 +1,388 @@ +# Per-PR process lock shared by review, prepare, merge, and worktree GC. +PR_OPERATION_LOCK_REF="" +PR_OPERATION_LOCK_OWNER_OID="" +PR_OPERATION_LOCK_CANDIDATE_PR="" +PR_OPERATION_LOCK_CANDIDATE_OID="" +PR_OPERATION_LOCK_BLOCKED_OID="" +PR_OPERATION_LOCK_BLOCKED_REASON="" + +is_canonical_pr_number() { + local pr="$1" + case "$pr" in ''|0|0*|*[!0-9]*) return 1 ;; esac +} + +pr_operation_lock_ref() { + local pr="$1" + is_canonical_pr_number "$pr" || return 1 + printf 'refs/openclaw/pr-operation-locks/%s\n' "$pr" +} + +pr_operation_lock_zero_oid() { + local object_format + object_format=$(git -C "$(repo_root)" rev-parse --show-object-format 2>/dev/null) || return 1 + case "$object_format" in + sha1) printf '%040d\n' 0 ;; + sha256) printf '%064d\n' 0 ;; + *) return 1 ;; + esac +} + +pr_operation_lock_process_identity() { + local pid="$1" + case "$pid" in ''|0|1|*[!0-9]*) return 1 ;; esac + TZ=UTC0 LC_ALL=C ps -o state= -o lstart= -p "$pid" 2>/dev/null | awk ' + NF { + state = $1 + $1 = "" + sub(/^[[:space:]]+/, "") + printf "%s\t%s\n", state, $0 + found = 1 + } + END { exit found ? 0 : 1 } + ' +} + +pr_operation_lock_process_birth() { + local identity state birth + identity=$(pr_operation_lock_process_identity "$1") || return 1 + IFS=$'\t' read -r state birth <<<"$identity" + case "$state" in Z*) return 1 ;; esac + [ -n "$birth" ] || return 1 + printf '%s\n' "$birth" +} + +pr_operation_lock_process_group_status() { + local pgid="$1" + case "$pgid" in ''|0|1|*[!0-9]*) return 1 ;; esac + node -e ' + const pgid = Number(process.argv[1]); + if (!Number.isSafeInteger(pgid) || pgid <= 1 || pgid > 0x7fffffff) { + process.stdout.write("indeterminate\n"); + process.exit(0); + } + try { + process.kill(-pgid, 0); + process.stdout.write("live\n"); + } catch (error) { + process.stdout.write(error?.code === "ESRCH" ? "dead\n" : "indeterminate\n"); + } + ' "$pgid" +} + +read_pr_operation_lock_owner() { + local owner_oid="$1" + local object_type payload parsed + object_type=$(git -C "$(repo_root)" cat-file -t "$owner_oid" 2>/dev/null) || return 1 + [ "$object_type" = "blob" ] || return 1 + payload=$(git -C "$(repo_root)" cat-file blob "$owner_oid" 2>/dev/null) || return 1 + parsed=$(printf '%s\n' "$payload" | awk -F= ' + NR == 1 && $0 == "version=3" { next } + NR == 2 && $0 == "state=active" { next } + NR == 3 && NF == 2 && $1 == "pgid" && $2 ~ /^[1-9][0-9]*$/ && $2 > 1 && $2 <= 2147483647 { + pgid = $2 + next + } + NR == 4 && NF == 2 && $1 == "supervisor_pid" && $2 ~ /^[1-9][0-9]*$/ && $2 > 1 && $2 <= 2147483647 { + supervisor_pid = $2 + next + } + NR == 5 && NF == 2 && $1 == "supervisor_birth" && length($2) > 0 && index($2, "\t") == 0 { + supervisor_birth = substr($0, length($1) + 2) + next + } + NR == 6 && NF == 2 && $1 == "token" && length($2) > 0 && index($2, "\t") == 0 { + token = $2 + next + } + { invalid = 1 } + END { + if (invalid || NR != 6 || pgid == "" || supervisor_pid == "" || supervisor_birth == "" || token == "") { + exit 1 + } + printf "%s\t%s\t%s\t%s\n", pgid, supervisor_pid, supervisor_birth, token + } + ') || return 1 + + local owner_pgid supervisor_pid supervisor_birth owner_token + IFS=$'\t' read -r owner_pgid supervisor_pid supervisor_birth owner_token <<<"$parsed" + case "$owner_pgid" in ''|0|1|*[!0-9]*) return 1 ;; esac + case "$supervisor_pid" in ''|0|1|*[!0-9]*) return 1 ;; esac + [ -n "$supervisor_birth" ] || return 1 + if [[ ! "$owner_token" =~ ^[0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12}$ ]]; then + return 1 + fi + printf '%s\t%s\t%s\t%s\n' "$owner_pgid" "$supervisor_pid" "$supervisor_birth" "$owner_token" +} + +clear_pr_operation_lock_state() { + PR_OPERATION_LOCK_REF="" + PR_OPERATION_LOCK_OWNER_OID="" + PR_OPERATION_LOCK_CANDIDATE_PR="" + PR_OPERATION_LOCK_CANDIDATE_OID="" + PR_OPERATION_LOCK_BLOCKED_OID="" + PR_OPERATION_LOCK_BLOCKED_REASON="" +} + +pr_operation_lock_owner_is_current() { + local root="$1" + local lock_ref="$2" + local expected_oid="$3" + local current_oid ref_status=0 + if git -C "$root" symbolic-ref -q "$lock_ref" >/dev/null 2>&1; then + return 2 + fi + if current_oid=$(git -C "$root" rev-parse --verify "$lock_ref" 2>/dev/null); then + [ "$current_oid" = "$expected_oid" ] && return 0 + return 1 + fi + git -C "$root" show-ref --verify --quiet "$lock_ref" 2>/dev/null || ref_status=$? + [ "$ref_status" -eq 1 ] && return 1 + return 2 +} + +release_pr_operation_lock() { + if [ -z "${PR_OPERATION_LOCK_REF:-}" ] || [ -z "${PR_OPERATION_LOCK_OWNER_OID:-}" ]; then + return 0 + fi + + if [ -n "${OPENCLAW_PR_LOCK_NOTIFY_FD:-}" ]; then + # The outer supervisor releases only after a clean group drain. A failed, + # interrupted, or controller-lost operation leaves this exact ref sticky. + clear_pr_operation_lock_state + return 0 + fi + + local root lock_ref owner_oid observed_oid ref_status + root=$(repo_root) || return 1 + lock_ref="$PR_OPERATION_LOCK_REF" + owner_oid="$PR_OPERATION_LOCK_OWNER_OID" + + local attempts=0 + while true; do + # The expected old object makes release a compare-and-swap: a delayed + # owner can never delete a successor's lock. + if git -C "$root" update-ref --no-deref -d "$lock_ref" "$owner_oid" 2>/dev/null; then + clear_pr_operation_lock_state + return 0 + fi + + if observed_oid=$(git -C "$root" rev-parse --verify "$lock_ref" 2>/dev/null); then + if [ "$observed_oid" != "$owner_oid" ]; then + clear_pr_operation_lock_state + return 0 + fi + else + ref_status=0 + git -C "$root" show-ref --verify --quiet "$lock_ref" 2>/dev/null || ref_status=$? + if [ "$ref_status" -eq 1 ]; then + clear_pr_operation_lock_state + return 0 + fi + if [ "$ref_status" -ne 0 ]; then + break + fi + fi + + attempts=$((attempts + 1)) + [ "$attempts" -lt 20 ] || break + sleep 0.05 + done + + echo "Unable to release the operation lock for ${lock_ref##*/}; the owner ref is unchanged." >&2 + return 1 +} + +notify_pr_operation_lock_supervisor() { + if [ -z "${OPENCLAW_PR_LOCK_NOTIFY_FD:-}" ]; then + return 0 + fi + case "$OPENCLAW_PR_LOCK_NOTIFY_FD" in ''|*[!0-9]*) return 1 ;; esac + printf '%s\t%s\n' "$PR_OPERATION_LOCK_REF" "$PR_OPERATION_LOCK_OWNER_OID" >&"$OPENCLAW_PR_LOCK_NOTIFY_FD" +} + +recover_pr_operation_lock() { + local pr="$1" + local expected_oid="$2" + local confirmation="${3-}" + is_canonical_pr_number "$pr" || { echo "Invalid PR number: $pr" >&2; return 2; } + [[ "$expected_oid" =~ ^([0-9a-f]{40}|[0-9a-f]{64})$ ]] || { + echo "Invalid operation-lock owner OID: $expected_oid" >&2 + return 2 + } + if [ "$confirmation" != "--confirmed-no-running-tools" ]; then + echo "Recovery requires --confirmed-no-running-tools after checking for detached PR tools." >&2 + return 2 + fi + + local root lock_ref observed_oid + root=$(repo_root) || return 1 + lock_ref=$(pr_operation_lock_ref "$pr") || return 1 + observed_oid=$(git -C "$root" rev-parse --verify "$lock_ref" 2>/dev/null) || { + echo "PR #$pr has no operation lock to recover." >&2 + return 1 + } + if [ "$observed_oid" != "$expected_oid" ]; then + echo "PR #$pr operation-lock owner changed; refusing to delete $observed_oid." >&2 + return 1 + fi + # PGID liveness cannot exclude a detached child or unrelated PGID reuse. + # Recovery authority is the explicit confirmation plus this exact-OID CAS. + if ! git -C "$root" update-ref --no-deref -d "$lock_ref" "$expected_oid" 2>/dev/null; then + echo "PR #$pr operation-lock owner changed during recovery; nothing was deleted." >&2 + return 1 + fi + echo "Recovered the stale operation lock for PR #$pr." +} + +prepare_pr_operation_lock_candidate() { + local pr="$1" + if [ "${PR_OPERATION_LOCK_CANDIDATE_PR:-}" = "$pr" ] && [ -n "${PR_OPERATION_LOCK_CANDIDATE_OID:-}" ]; then + return 0 + fi + + local root token group_status supervisor_pid supervisor_birth owner_oid + root=$(repo_root) || return 1 + token=$(node -e 'process.stdout.write(require("node:crypto").randomUUID())') || return 1 + group_status=$(pr_operation_lock_process_group_status "$$") || return 1 + [ "$group_status" = "live" ] || return 1 + supervisor_pid="${OPENCLAW_PR_LOCK_SUPERVISOR_PID:-$$}" + case "$supervisor_pid" in ''|0|1|*[!0-9]*) return 1 ;; esac + supervisor_birth=$(pr_operation_lock_process_birth "$supervisor_pid") || return 1 + owner_oid=$(printf 'version=3\nstate=active\npgid=%s\nsupervisor_pid=%s\nsupervisor_birth=%s\ntoken=%s\n' \ + "$$" "$supervisor_pid" "$supervisor_birth" "$token" | + git -C "$root" hash-object -w --stdin) || return 1 + PR_OPERATION_LOCK_CANDIDATE_PR="$pr" + PR_OPERATION_LOCK_CANDIDATE_OID="$owner_oid" +} + +try_acquire_pr_operation_lock() { + local pr="$1" + is_canonical_pr_number "$pr" || return 2 + PR_OPERATION_LOCK_BLOCKED_OID="" + PR_OPERATION_LOCK_BLOCKED_REASON="" + + local root lock_ref zero_oid owner_oid + root=$(repo_root) || return 2 + lock_ref=$(pr_operation_lock_ref "$pr") || return 2 + zero_oid=$(pr_operation_lock_zero_oid) || return 2 + prepare_pr_operation_lock_candidate "$pr" || return 2 + owner_oid="$PR_OPERATION_LOCK_CANDIDATE_OID" + + local unreadable_ref_attempts=0 + while true; do + if git -C "$root" update-ref --no-deref "$lock_ref" "$owner_oid" "$zero_oid" 2>/dev/null; then + PR_OPERATION_LOCK_REF="$lock_ref" + PR_OPERATION_LOCK_OWNER_OID="$owner_oid" + if ! notify_pr_operation_lock_supervisor; then + PR_OPERATION_LOCK_BLOCKED_OID="$owner_oid" + PR_OPERATION_LOCK_BLOCKED_REASON="not reported to its supervisor" + return 2 + fi + return 0 + fi + + local observed_oid owner_data owner_pgid supervisor_pid supervisor_birth owner_token group_status + if git -C "$root" symbolic-ref -q "$lock_ref" >/dev/null 2>&1; then + return 2 + fi + if ! observed_oid=$(git -C "$root" rev-parse --verify "$lock_ref" 2>/dev/null); then + # The supervisor may have released between our failed create-CAS and + # this read. A newly installed successor can also appear immediately, + # so one read miss is always a normal retry. + unreadable_ref_attempts=$((unreadable_ref_attempts + 1)) + if [ "$unreadable_ref_attempts" -le 20 ]; then + # A concurrent exact release can leave a short delete-to-create window, + # including Git's transient ref lock. Bound the wait so a persistently + # unreadable ref still fails closed. + sleep 0.05 + continue + fi + return 2 + fi + unreadable_ref_attempts=0 + if ! owner_data=$(read_pr_operation_lock_owner "$observed_oid"); then + local owner_status=0 + pr_operation_lock_owner_is_current "$root" "$lock_ref" "$observed_oid" || owner_status=$? + [ "$owner_status" -eq 1 ] && continue + [ "$owner_status" -eq 0 ] || return 2 + PR_OPERATION_LOCK_BLOCKED_OID="$observed_oid" + PR_OPERATION_LOCK_BLOCKED_REASON="unreadable" + return 2 + fi + IFS=$'\t' read -r owner_pgid supervisor_pid supervisor_birth owner_token <<<"$owner_data" + local supervisor_identity supervisor_state current_supervisor_birth + supervisor_identity=$(pr_operation_lock_process_identity "$supervisor_pid" 2>/dev/null || true) + supervisor_state="" + current_supervisor_birth="" + if [ -n "$supervisor_identity" ]; then + IFS=$'\t' read -r supervisor_state current_supervisor_birth <<<"$supervisor_identity" + fi + + # A group is active only while the exact supervisor incarnation still owns + # it. A reused PGID or orphaned descendant must surface explicit recovery. + group_status=$(pr_operation_lock_process_group_status "$owner_pgid") || return 2 + case "$group_status" in + live | dead) + if [[ "$supervisor_state" != Z* ]] && + [ -n "$current_supervisor_birth" ] && + [ "$current_supervisor_birth" = "$supervisor_birth" ] + then + # A dead group can precede its controller's final drain and exact + # release. Waiting also covers the ordinary live-operation case. + return 1 + fi + local owner_status=0 + pr_operation_lock_owner_is_current "$root" "$lock_ref" "$observed_oid" || owner_status=$? + [ "$owner_status" -eq 1 ] && continue + [ "$owner_status" -eq 0 ] || return 2 + # A missing controller cannot disprove a nested detached tool, even + # when its old group is dead. Only exact-OID recovery may clear it. + PR_OPERATION_LOCK_BLOCKED_OID="$observed_oid" + PR_OPERATION_LOCK_BLOCKED_REASON="orphaned" + return 2 + ;; + *) + local owner_status=0 + pr_operation_lock_owner_is_current "$root" "$lock_ref" "$observed_oid" || owner_status=$? + [ "$owner_status" -eq 1 ] && continue + [ "$owner_status" -eq 0 ] || return 2 + PR_OPERATION_LOCK_BLOCKED_OID="$observed_oid" + PR_OPERATION_LOCK_BLOCKED_REASON="indeterminate" + return 2 + ;; + esac + done +} + +acquire_pr_operation_lock() { + local pr="$1" + local announced=false + local lock_status=0 + while true; do + try_acquire_pr_operation_lock "$pr" || lock_status=$? + if [ "$lock_status" -eq 0 ]; then + return 0 + fi + if [ "$lock_status" -ne 1 ]; then + if [ -n "$PR_OPERATION_LOCK_BLOCKED_OID" ]; then + echo "The prior PR #$pr operation lock is $PR_OPERATION_LOCK_BLOCKED_REASON; detached child tools cannot be ruled out." >&2 + print_pr_operation_lock_recovery_guidance "$pr" + fi + echo "Unable to acquire the operation lock for PR #$pr." >&2 + return "$lock_status" + fi + if [ "$announced" = "false" ]; then + echo "Waiting for the active scripts/pr operation on PR #$pr to finish..." >&2 + announced=true + fi + lock_status=0 + sleep 0.2 + done +} + +print_pr_operation_lock_recovery_guidance() { + local pr="$1" + [ -n "${PR_OPERATION_LOCK_BLOCKED_OID:-}" ] || return 1 + echo "After verifying that no PR #$pr tools remain, recover the exact owner with:" >&2 + echo " scripts/pr lock-recover $pr $PR_OPERATION_LOCK_BLOCKED_OID --confirmed-no-running-tools" >&2 +} diff --git a/scripts/pr-lib/process-group-runner.mjs b/scripts/pr-lib/process-group-runner.mjs new file mode 100644 index 000000000000..a9085fb914ba --- /dev/null +++ b/scripts/pr-lib/process-group-runner.mjs @@ -0,0 +1,377 @@ +import { spawn, spawnSync } from "node:child_process"; +import { constants } from "node:os"; +import { resolve } from "node:path"; +import { fileURLToPath } from "node:url"; + +const SIGNAL_GRACE_MS = 5000; +const KILL_DRAIN_MS = 5000; +const POLL_MS = 25; +const MAX_NOTIFICATION_LINE_BYTES = 4096; +const FORWARDED_SIGNALS = ["SIGHUP", "SIGINT", "SIGQUIT", "SIGTERM"]; + +const [repoRootArg, script, ...args] = process.argv.slice(2); +if (!repoRootArg || !script) { + console.error("process-group-runner requires a repository root and script path"); + process.exit(2); +} +if (process.platform === "win32") { + console.error("scripts/pr operation locking requires a POSIX process group (use WSL on Windows)"); + process.exit(1); +} + +const repoRoot = resolve(repoRootArg); +const lockScript = fileURLToPath(new URL("./operation-lock.sh", import.meta.url)); +const locks = new Map(); +let notificationBuffer = ""; +let discardingOversizedNotificationLine = false; +let notificationEnded = false; +/** @type {Error | undefined} */ +let notificationFailure; +let receivedSignal; +let escalationTimer; +let killDeadline; +const operationGroup = { pid: undefined }; +let operationGroupGone = false; +let hadLingeringGroup = false; + +function delay(ms) { + return new Promise((resolveDelay) => { + setTimeout(resolveDelay, ms); + }); +} + +function toError(value, fallbackMessage) { + return value instanceof Error ? value : new Error(fallbackMessage); +} + +function exitCodeForSignal(signal) { + const signalNumber = constants.signals[signal]; + return typeof signalNumber === "number" ? 128 + signalNumber : 1; +} + +function processGroupStatus(pgid) { + if (operationGroupGone) { + return "dead"; + } + if (!Number.isSafeInteger(pgid) || pgid <= 1 || pgid > 0x7fffffff) { + return "indeterminate"; + } + try { + process.kill(-pgid, 0); + return "live"; + } catch (error) { + if (error?.code === "ESRCH") { + // Once absent, this operation group is gone forever. Never let later + // PGID reuse redirect a delayed signal or liveness probe. + operationGroupGone = true; + return "dead"; + } + return "indeterminate"; + } +} + +function signalProcessGroup(signal) { + const childPid = operationGroup.pid; + if (!childPid || operationGroupGone) { + return; + } + try { + process.kill(-childPid, signal); + } catch (error) { + if (error?.code === "ESRCH") { + operationGroupGone = true; + } else { + notificationFailure ??= new Error( + `Unable to signal scripts/pr process group with ${signal}: ${String(error)}`, + ); + } + } +} + +function escalateSignal() { + if (killDeadline) { + return; + } + killDeadline = Date.now() + KILL_DRAIN_MS; + signalProcessGroup("SIGKILL"); +} + +const signalHandlers = new Map(); +for (const signal of FORWARDED_SIGNALS) { + const handler = () => { + if (receivedSignal) { + escalateSignal(); + return; + } + receivedSignal = signal; + signalProcessGroup(signal); + escalationTimer = setTimeout(escalateSignal, SIGNAL_GRACE_MS); + }; + signalHandlers.set(signal, handler); + process.on(signal, handler); +} + +const child = spawn(script, args, { + cwd: process.cwd(), + detached: true, + env: { + ...process.env, + OPENCLAW_PR_DEDICATED_PROCESS_GROUP: "1", + OPENCLAW_PR_LOCK_NOTIFY_FD: "3", + OPENCLAW_PR_LOCK_SUPERVISOR_PID: String(process.pid), + }, + stdio: ["inherit", "inherit", "inherit", "pipe"], +}); +operationGroup.pid = child.pid; +if (killDeadline) { + signalProcessGroup("SIGKILL"); +} else if (receivedSignal) { + signalProcessGroup(receivedSignal); +} + +function consumeNotificationLine(line) { + const [lockRef, ownerOid, extra] = line.split("\t"); + if ( + extra !== undefined || + !/^refs\/openclaw\/pr-operation-locks\/[1-9][0-9]*$/u.test(lockRef ?? "") || + !/^(?:[0-9a-f]{40}|[0-9a-f]{64})$/u.test(ownerOid ?? "") + ) { + notificationFailure ??= new Error("scripts/pr emitted malformed operation-lock metadata"); + return; + } + + const owner = spawnSync("git", ["-C", repoRoot, "cat-file", "blob", ownerOid], { + encoding: "utf8", + stdio: ["ignore", "pipe", "ignore"], + }); + const ownerMatch = + owner.status === 0 + ? /^version=3\nstate=active\npgid=([1-9][0-9]*)\nsupervisor_pid=([1-9][0-9]*)\nsupervisor_birth=[^\t\n]+\ntoken=[0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12}\n?$/u.exec( + owner.stdout, + ) + : undefined; + const ownerPgid = ownerMatch ? Number(ownerMatch[1]) : undefined; + const supervisorPid = ownerMatch ? Number(ownerMatch[2]) : undefined; + if ( + ownerPgid === undefined || + supervisorPid === undefined || + !Number.isSafeInteger(ownerPgid) || + !Number.isSafeInteger(supervisorPid) || + ownerPgid <= 1 || + supervisorPid <= 1 || + ownerPgid > 0x7fffffff || + supervisorPid > 0x7fffffff || + ownerPgid !== child?.pid || + supervisorPid !== process.pid + ) { + notificationFailure ??= new Error( + "scripts/pr emitted an operation lock owned by another process group", + ); + return; + } + locks.set(`${lockRef}\0${ownerOid}`, { lockRef, ownerOid }); +} + +function finishNotifications() { + if (notificationEnded) { + return; + } + if (!discardingOversizedNotificationLine && notificationBuffer.length > 0) { + consumeNotificationLine(notificationBuffer); + } + notificationBuffer = ""; + notificationEnded = true; +} + +function consumeNotificationChunk(chunk) { + notificationBuffer += chunk; + while (true) { + const newline = notificationBuffer.indexOf("\n"); + if (discardingOversizedNotificationLine) { + if (newline === -1) { + notificationBuffer = ""; + return; + } + notificationBuffer = notificationBuffer.slice(newline + 1); + discardingOversizedNotificationLine = false; + continue; + } + if (newline === -1) { + if (Buffer.byteLength(notificationBuffer) > MAX_NOTIFICATION_LINE_BYTES) { + notificationFailure ??= new Error("scripts/pr operation-lock metadata line is too large"); + notificationBuffer = ""; + discardingOversizedNotificationLine = true; + } + return; + } + + const line = notificationBuffer.slice(0, newline); + notificationBuffer = notificationBuffer.slice(newline + 1); + if (Buffer.byteLength(line) > MAX_NOTIFICATION_LINE_BYTES) { + notificationFailure ??= new Error("scripts/pr operation-lock metadata line is too large"); + continue; + } + consumeNotificationLine(line); + } +} + +const notificationStream = child.stdio[3]; +notificationStream.setEncoding("utf8"); +notificationStream.on("data", consumeNotificationChunk); +notificationStream.once("error", (error) => { + notificationFailure ??= toError(error, "scripts/pr operation-lock notification stream failed"); +}); +notificationStream.once("end", finishNotifications); +notificationStream.once("close", finishNotifications); + +const childResult = await new Promise((resolveResult) => { + let settled = false; + const settle = (result) => { + if (settled) { + return; + } + settled = true; + resolveResult(result); + }; + child.once("error", (error) => { + notificationFailure ??= toError(error, "Unable to launch scripts/pr"); + settle({ code: 1, signal: null }); + }); + child.once("exit", (code, signal) => settle({ code, signal })); +}); + +const postExitGroupStatus = child.pid ? processGroupStatus(child.pid) : "dead"; +if (postExitGroupStatus === "indeterminate") { + notificationFailure ??= new Error("scripts/pr process-group state became indeterminate"); +} else if (postExitGroupStatus === "live") { + // A wrapper exit does not end same-group background work. Bound and drain + // forgotten jobs, but keep the lock because their terminal state is unknown. + hadLingeringGroup = true; + notificationFailure ??= new Error("scripts/pr process group remained active after wrapper exit"); + signalProcessGroup("SIGTERM"); + escalationTimer ??= setTimeout(escalateSignal, SIGNAL_GRACE_MS); +} else if (!notificationEnded) { + // A detached descendant may be the last writer. It cannot be signalled by + // this group supervisor, so bound the wait and retain the lock on timeout. + killDeadline ??= Date.now() + KILL_DRAIN_MS; +} + +async function waitForOperationDrain() { + while (true) { + const groupStatus = child.pid ? processGroupStatus(child.pid) : "dead"; + if (groupStatus === "indeterminate") { + throw new Error("scripts/pr process-group state became indeterminate"); + } + if (groupStatus === "dead" && notificationEnded) { + return; + } + if (killDeadline && Date.now() >= killDeadline) { + throw new Error( + `scripts/pr operation lifetime did not drain (group=${groupStatus}, pipe=${notificationEnded ? "closed" : "open"})`, + ); + } + await delay(POLL_MS); + } +} + +function releaseLock({ lockRef, ownerOid }) { + const env = { ...process.env }; + delete env.OPENCLAW_PR_LOCK_NOTIFY_FD; + const result = spawnSync( + "bash", + [ + "-c", + [ + "set -euo pipefail", + 'source "$1"', + 'SUPERVISOR_REPO_ROOT="$2"', + "repo_root() { printf '%s\\n' \"$SUPERVISOR_REPO_ROOT\"; }", + 'PR_OPERATION_LOCK_REF="$3"', + 'PR_OPERATION_LOCK_OWNER_OID="$4"', + "release_pr_operation_lock", + ].join("\n"), + "operation-lock-release", + lockScript, + repoRoot, + lockRef, + ownerOid, + ], + { encoding: "utf8", env, stdio: ["ignore", "pipe", "pipe"] }, + ); + if (result.status !== 0) { + throw new Error( + result.stderr.trim() || + `Unable to release the operation lock for ${lockRef.split("/").at(-1)}`, + ); + } +} + +function reportRetainedLock({ lockRef, ownerOid }) { + const pr = lockRef.split("/").at(-1); + console.error( + `Retaining the operation lock for PR #${pr}; detached child tools cannot be ruled out.`, + ); + console.error(`After verifying that no PR #${pr} tools remain, recover the exact owner with:`); + console.error(` scripts/pr lock-recover ${pr} ${ownerOid} --confirmed-no-running-tools`); +} + +let drained = false; +try { + await waitForOperationDrain(); + drained = true; +} catch (error) { + notificationFailure ??= toError(error, "scripts/pr operation drain failed"); + // An out-of-group descendant can inherit the write end indefinitely. Once + // the bounded drain fails, close our read end so that sentinel cannot keep + // the controller alive; the exact lock remains sticky for manual recovery. + finishNotifications(); + notificationStream.destroy(); +} + +if (escalationTimer) { + clearTimeout(escalationTimer); +} +for (const [signal, handler] of signalHandlers) { + process.off(signal, handler); +} + +// PR commands must join all state-mutating children before returning. A clean +// exit is that trusted completion signal; abnormal exits retain the lock because +// an escaped child can outlive both the recorded group and notification pipe. +const completedCleanly = + childResult.code === 0 && + !receivedSignal && + !childResult.signal && + !notificationFailure && + !hadLingeringGroup; +const retainedLocks = []; +if (drained && completedCleanly) { + for (const lock of locks.values()) { + try { + releaseLock(lock); + } catch (error) { + notificationFailure ??= toError(error, "Unable to release a scripts/pr operation lock"); + retainedLocks.push(lock); + } + } +} else { + retainedLocks.push(...locks.values()); +} +for (const lock of retainedLocks) { + reportRetainedLock(lock); +} + +if (notificationFailure) { + console.error(notificationFailure.message); +} + +if (receivedSignal) { + process.exitCode = exitCodeForSignal(receivedSignal); +} else if (childResult.code !== null) { + process.exitCode = childResult.code; +} else { + process.exitCode = childResult.signal ? exitCodeForSignal(childResult.signal) : 1; +} +if (notificationFailure && process.exitCode === 0) { + process.exitCode = 1; +} diff --git a/scripts/pr-lib/worktree.sh b/scripts/pr-lib/worktree.sh index b1ad27df9b58..4fe8cc8855ad 100644 --- a/scripts/pr-lib/worktree.sh +++ b/scripts/pr-lib/worktree.sh @@ -139,22 +139,45 @@ gc_pr_worktrees() { echo "skipping $dir (could not parse PR number)" continue fi + local lock_status=0 + try_acquire_pr_operation_lock "$pr" || lock_status=$? + if [ "$lock_status" -ne 0 ]; then + if [ "$lock_status" -eq 1 ]; then + echo "skipping $dir (PR #$pr has an active scripts/pr operation)" + elif [ -n "$PR_OPERATION_LOCK_BLOCKED_OID" ]; then + echo "skipping $dir (PR #$pr operation lock is $PR_OPERATION_LOCK_BLOCKED_REASON)" + print_pr_operation_lock_recovery_guidance "$pr" + else + echo "skipping $dir (PR #$pr operation lock state is indeterminate)" + fi + continue + fi local state state=$(gh pr view "$pr" --json state --jq .state 2>/dev/null || printf 'UNKNOWN') case "$state" in MERGED|CLOSED) if [ "$dry_run" = "true" ]; then echo "would remove $dir (PR #$pr state=$state)" + removed=$((removed + 1)) else remove_worktree_if_present "$dir" delete_local_branch_if_safe "temp/pr-$pr" delete_local_branch_if_safe "pr-$pr" delete_local_branch_if_safe "pr-$pr-prep" - echo "removed $dir (PR #$pr state=$state)" + if [ ! -e "$dir" ] && + ! git show-ref --verify --quiet "refs/heads/temp/pr-$pr" && + ! git show-ref --verify --quiet "refs/heads/pr-$pr" && + ! git show-ref --verify --quiet "refs/heads/pr-$pr-prep" + then + echo "removed $dir (PR #$pr state=$state)" + removed=$((removed + 1)) + else + echo "skipping $dir (cleanup incomplete)" + fi fi - removed=$((removed + 1)) ;; esac + release_pr_operation_lock done if [ "$removed" -eq 0 ]; then @@ -168,12 +191,9 @@ gc_pr_worktrees() { pr_number_from_worktree_dir() { local dir="$1" - local token - token="${dir##*/pr-}" - token="${token%%[^0-9]*}" - if [ -n "$token" ]; then - printf '%s\n' "$token" - return 0 - fi - return 1 + local basename=${dir##*/} + local token=${basename#pr-} + [ "$basename" != "$token" ] || return 1 + is_canonical_pr_number "$token" || return 1 + printf '%s\n' "$token" } diff --git a/scripts/test-projects.test-support.mjs b/scripts/test-projects.test-support.mjs index 30c9849e2b92..57288bbac36d 100644 --- a/scripts/test-projects.test-support.mjs +++ b/scripts/test-projects.test-support.mjs @@ -1402,7 +1402,9 @@ const TOOLING_SOURCE_TEST_TARGETS = new Map([ ["scripts/mobile-reauth.sh", ["test/scripts/auth-monitor.test.ts"]], ["scripts/committer", ["test/scripts/committer.test.ts"]], ["scripts/gh-read", ["test/scripts/gh-read.test.ts"]], - ["scripts/pr", ["test/scripts/pr-wrappers.test.ts"]], + ["scripts/pr", ["test/scripts/pr-operation-lock.test.ts", "test/scripts/pr-wrappers.test.ts"]], + ["scripts/pr-lib/operation-lock.sh", ["test/scripts/pr-operation-lock.test.ts"]], + ["scripts/pr-lib/process-group-runner.mjs", ["test/scripts/pr-operation-lock.test.ts"]], ["scripts/pr-merge", ["test/scripts/pr-wrappers.test.ts"]], ["scripts/pr-prepare", ["test/scripts/pr-wrappers.test.ts"]], ["scripts/pr-review", ["test/scripts/pr-wrappers.test.ts"]], @@ -2062,6 +2064,7 @@ const TOOLING_TEST_TARGETS = new Map([ "test/scripts/plugin-prerelease-test-plan.test.ts", ["test/scripts/plugin-prerelease-test-plan.test.ts"], ], + ["test/scripts/pr-operation-lock.test.ts", ["test/scripts/pr-operation-lock.test.ts"]], ["test/scripts/pr-wrappers.test.ts", ["test/scripts/pr-wrappers.test.ts"]], ["test/scripts/test-projects.test.ts", ["test/scripts/test-projects.test.ts"]], [ diff --git a/test/scripts/pr-operation-lock.test.ts b/test/scripts/pr-operation-lock.test.ts new file mode 100644 index 000000000000..0de6c1660b22 --- /dev/null +++ b/test/scripts/pr-operation-lock.test.ts @@ -0,0 +1,1819 @@ +import { + execFileSync, + spawn, + spawnSync, + type ChildProcess, + type SpawnOptions, +} from "node:child_process"; +import { + chmodSync, + cpSync, + existsSync, + mkdirSync, + readFileSync, + realpathSync, + symlinkSync, + unlinkSync, + writeFileSync, +} from "node:fs"; +import { dirname, join } from "node:path"; +import { pathToFileURL } from "node:url"; +import { afterEach, describe, expect, it } from "vitest"; +import { useAutoCleanupTempDirTracker } from "../helpers/temp-dir.js"; + +const tempDirs = useAutoCleanupTempDirTracker(afterEach); +const repoRoot = process.cwd(); +const commonScript = join(repoRoot, "scripts/pr-lib/common.sh"); +const lockScript = join(repoRoot, "scripts/pr-lib/operation-lock.sh"); +const processGroupRunner = join(repoRoot, "scripts/pr-lib/process-group-runner.mjs"); +const managedChildUrl = pathToFileURL(join(repoRoot, "scripts/lib/managed-child-process.mjs")).href; +const worktreeScript = join(repoRoot, "scripts/pr-lib/worktree.sh"); +const lockRef = "refs/openclaw/pr-operation-locks/42"; +const detachedChildren = new WeakSet(); +const goneProcessGroups = new Set(); + +function spawnDetached(command: string, args: readonly string[], options: SpawnOptions = {}) { + const child = spawn(command, args, { ...options, detached: true }); + detachedChildren.add(child); + if (child.pid) goneProcessGroups.delete(child.pid); + return child; +} + +function createRepo(nestedName?: string) { + const tempRoot = tempDirs.make("openclaw-pr-operation-lock-"); + const dir = nestedName ? join(tempRoot, nestedName) : tempRoot; + if (nestedName) mkdirSync(dir); + execFileSync("git", ["init", "-q", "-b", "main"], { cwd: dir }); + execFileSync("git", ["config", "user.name", "OpenClaw Test"], { cwd: dir }); + execFileSync("git", ["config", "user.email", "test@openclaw.invalid"], { cwd: dir }); + writeFileSync(join(dir, "base.txt"), "base\n"); + execFileSync("git", ["add", "base.txt"], { cwd: dir }); + execFileSync("git", ["commit", "-qm", "base"], { cwd: dir }); + return dir; +} + +function bashSource(repoDir: string, supervised = false) { + return [ + "set -euo pipefail", + ...(supervised + ? [] + : ["unset OPENCLAW_PR_LOCK_NOTIFY_FD", "unset OPENCLAW_PR_LOCK_SUPERVISOR_PID"]), + `source '${worktreeScript}'`, + `source '${lockScript}'`, + `source '${commonScript}'`, + `repo_root() { printf '%s\\n' '${repoDir}'; }`, + ]; +} + +function writeOperationFixture(repoDir: string, name: string, commands: string[]) { + const fixture = join(repoDir, name); + writeFileSync( + fixture, + ["#!/usr/bin/env bash", ...bashSource(repoDir, true), ...commands].join("\n"), + ); + chmodSync(fixture, 0o755); + return fixture; +} + +function installPrCliFixture(repoDir: string) { + const files = [ + "scripts/pr", + "scripts/lib/plain-gh.sh", + "scripts/pr-lib/worktree.sh", + "scripts/pr-lib/operation-lock.sh", + "scripts/pr-lib/process-group-runner.mjs", + "scripts/pr-lib/common.sh", + "scripts/pr-lib/changelog.sh", + "scripts/pr-lib/gates.sh", + "scripts/pr-lib/push.sh", + "scripts/pr-lib/review.sh", + "scripts/pr-lib/prepare-core.sh", + "scripts/pr-lib/merge.sh", + ]; + for (const file of files) { + const target = join(repoDir, file); + mkdirSync(dirname(target), { recursive: true }); + cpSync(join(repoRoot, file), target); + } + const cli = join(repoDir, "scripts/pr"); + chmodSync(cli, 0o755); + + const binDir = join(repoDir, "isolated-bin"); + mkdirSync(binDir); + for (const command of ["bash", "basename", "dirname", "git"]) { + const resolved = execFileSync("which", [command], { encoding: "utf8" }).trim(); + symlinkSync(resolved, join(binDir, command)); + } + return { binDir, cli }; +} + +async function runSupervisedFixture(repoDir: string, fixture: string) { + const controller = spawn(process.execPath, [processGroupRunner, repoDir, fixture], { + cwd: repoDir, + stdio: ["ignore", "pipe", "pipe"], + }); + let stdout = ""; + let stderr = ""; + controller.stdout!.setEncoding("utf8"); + controller.stderr!.setEncoding("utf8"); + controller.stdout!.on("data", (chunk) => (stdout += chunk)); + controller.stderr!.on("data", (chunk) => (stderr += chunk)); + try { + await new Promise((resolve, reject) => { + const timeout = setTimeout(() => { + controller.off("close", onClose); + reject(new Error(`controller did not close within 15000ms (${childStatus(controller)})`)); + }, 15_000); + const onClose = () => { + clearTimeout(timeout); + resolve(); + }; + controller.once("close", onClose); + }); + } catch (error) { + try { + if (refExists(repoDir)) { + const payload = execFileSync("git", ["cat-file", "blob", refOid(repoDir)], { + cwd: repoDir, + encoding: "utf8", + }); + const pgid = Number(/^version=3\nstate=active\npgid=([1-9][0-9]*)\n/u.exec(payload)?.[1]); + if (validProcessId(pgid)) { + await cleanupProcessGroup(pgid); + } + } + } catch { + // The controller still must die even if lock metadata is malformed. + } finally { + controller.kill("SIGKILL"); + try { + await waitForExit(controller, 2000); + } catch { + // Preserve the original bounded-exit failure below. + } + } + throw error; + } + return { status: controller.exitCode, signal: controller.signalCode, stdout, stderr }; +} + +function runLockShell(repoDir: string, commands: string[]) { + return spawnSync("bash", ["-c", [...bashSource(repoDir), ...commands].join("\n")], { + cwd: repoDir, + detached: true, + encoding: "utf8", + timeout: 10_000, + }); +} + +function spawnHolder(repoDir: string, statusFile: string, pr = 42, trapTerm = true) { + const traps = trapTerm + ? [ + "trap release_pr_operation_lock EXIT", + "trap 'exit 129' HUP", + "trap 'exit 130' INT", + "trap 'exit 143' TERM", + ] + : []; + return spawnDetached( + "bash", + [ + "-c", + [ + ...bashSource(repoDir), + ...traps, + `acquire_pr_operation_lock ${pr}`, + `printf 'held\\n' >'${statusFile}'`, + "while :; do sleep 1; done", + ].join("\n"), + ], + { cwd: repoDir, stdio: "ignore" }, + ); +} + +function spawnCandidate(repoDir: string, statusFile: string) { + return spawnDetached( + "bash", + [ + "-c", + [ + ...bashSource(repoDir), + "prepare_pr_operation_lock_candidate 42", + `printf 'prepared\\n' >'${statusFile}'`, + "while :; do sleep 1; done", + ].join("\n"), + ], + { cwd: repoDir, stdio: "ignore" }, + ); +} + +function spawnHolderWithChild(repoDir: string, statusFile: string, childPidFile: string) { + return spawnDetached( + "bash", + [ + "-c", + [ + ...bashSource(repoDir), + "acquire_pr_operation_lock 42", + `printf 'held\n' >'${statusFile}'`, + "sleep 30 &", + `printf '%s\n' \"$!\" >'${childPidFile}'`, + 'wait "$!"', + ].join("\n"), + ], + { cwd: repoDir, stdio: "ignore" }, + ); +} + +async function waitFor(predicate: () => boolean, timeoutMs = 5000) { + const deadline = Date.now() + timeoutMs; + while (Date.now() < deadline) { + if (predicate()) return true; + await new Promise((resolve) => setTimeout(resolve, 25)); + } + return false; +} + +function validProcessId(value: unknown): value is number { + return Number.isSafeInteger(value) && Number(value) > 1 && Number(value) <= 0x7fffffff; +} + +function readProcessIdFile(path: string) { + if (!existsSync(path)) return undefined; + const value = Number(readFileSync(path, "utf8").trim()); + return validProcessId(value) ? value : undefined; +} + +async function waitForProcessId(path: string) { + let pid: number | undefined; + const ready = await waitFor(() => { + pid = readProcessIdFile(path); + return pid !== undefined; + }); + if (!ready || pid === undefined) { + throw new Error(`process id was not written to ${path}`); + } + goneProcessGroups.delete(pid); + return pid; +} + +function childStatus(child: ChildProcess) { + return `pid=${child.pid ?? "unknown"} exit=${child.exitCode ?? "null"} signal=${child.signalCode ?? "null"}`; +} + +async function waitForExit(child: ChildProcess, timeoutMs = 5000) { + if (child.exitCode !== null || child.signalCode !== null) return; + await new Promise((resolve, reject) => { + let timeout: NodeJS.Timeout; + const onExit = () => { + clearTimeout(timeout); + resolve(); + }; + timeout = setTimeout(() => { + child.off("exit", onExit); + reject(new Error(`child did not exit within ${timeoutMs}ms (${childStatus(child)})`)); + }, timeoutMs); + child.once("exit", onExit); + if (child.exitCode !== null || child.signalCode !== null) { + child.off("exit", onExit); + clearTimeout(timeout); + resolve(); + } + }); +} + +async function stopChild(child: ChildProcess, signal: NodeJS.Signals) { + if (child.exitCode !== null || child.signalCode !== null) return; + signalTestChild(child, signal); + await waitForExit(child); +} + +async function stopChildLeader(child: ChildProcess, signal: NodeJS.Signals) { + if (child.exitCode !== null || child.signalCode !== null) return; + child.kill(signal); + await waitForExit(child); +} + +async function cleanupChildren(...children: Array) { + const failures: unknown[] = []; + for (const child of children) { + if (!child) continue; + try { + if (child.exitCode === null && child.signalCode === null) { + signalTestChild(child, "SIGKILL"); + await waitForExit(child, 2000); + } + } catch (error) { + failures.push(error); + } + } + if (failures.length > 0) { + throw new AggregateError(failures, "failed to clean up operation-lock test children"); + } +} + +function signalTestChild(child: ChildProcess, signal: NodeJS.Signals) { + if (detachedChildren.has(child) && child.pid) { + try { + killProcessGroup(child.pid, signal); + return; + } catch (error) { + const code = (error as NodeJS.ErrnoException).code; + if (code !== "ESRCH" && code !== "EPERM") throw error; + } + } + child.kill(signal); +} + +async function cleanupProcessGroup(pgid: number) { + if (!processGroupExists(pgid)) return; + try { + killProcessGroup(pgid, "SIGKILL"); + } catch (error) { + const code = (error as NodeJS.ErrnoException).code; + if (code === "ESRCH" || code === "EPERM") return; + throw error; + } + if (!(await waitFor(() => !processGroupExists(pgid), 2000))) { + throw new Error(`process group ${pgid} did not exit during cleanup`); + } +} + +function readOperationProcessGroup(repoDir: string) { + if (!refExists(repoDir)) return undefined; + try { + const payload = execFileSync("git", ["cat-file", "blob", refOid(repoDir)], { + cwd: repoDir, + encoding: "utf8", + }); + const pgid = Number(/^version=3\nstate=active\npgid=([1-9][0-9]*)\n/u.exec(payload)?.[1]); + return validProcessId(pgid) ? pgid : undefined; + } catch { + return undefined; + } +} + +async function cleanupController( + repoDir: string, + controller: ChildProcess, + operationPgidFile?: string, +) { + let pgid = operationPgidFile ? readProcessIdFile(operationPgidFile) : undefined; + pgid ??= readOperationProcessGroup(repoDir); + if (pgid) await cleanupProcessGroup(pgid); + await cleanupChildren(controller); + + pgid = operationPgidFile ? readProcessIdFile(operationPgidFile) : undefined; + pgid ??= readOperationProcessGroup(repoDir); + if (pgid) await cleanupProcessGroup(pgid); +} + +function refOid(repoDir: string, ref = lockRef) { + return execFileSync("git", ["rev-parse", ref], { cwd: repoDir, encoding: "utf8" }).trim(); +} + +function refExists(repoDir: string, ref = lockRef) { + return ( + spawnSync("git", ["show-ref", "--verify", "--quiet", ref], { + cwd: repoDir, + }).status === 0 + ); +} + +function processGroupExists(pgid: number) { + if (!validProcessId(pgid)) { + throw new Error(`refusing to probe invalid process group ${String(pgid)}`); + } + if (goneProcessGroups.has(pgid)) return false; + try { + process.kill(-pgid, 0); + return true; + } catch (error) { + const code = (error as NodeJS.ErrnoException).code; + // Fixtures never change identity. EPERM therefore means the original + // group exited and its numeric PGID now belongs to another user. + if (code === "ESRCH" || code === "EPERM") { + goneProcessGroups.add(pgid); + return false; + } + throw error; + } +} + +function killProcessGroup(pgid: number, signal: NodeJS.Signals) { + if (!validProcessId(pgid)) { + throw new Error(`refusing to signal invalid process group ${String(pgid)}`); + } + if (!goneProcessGroups.has(pgid)) process.kill(-pgid, signal); +} + +describe("scripts/pr process-group platform guard", () => { + it("keeps native Windows on the explicit WSL-only path", () => { + const source = readFileSync(processGroupRunner, "utf8"); + expect(source).toContain('process.platform === "win32"'); + expect(source).toContain("use WSL on Windows"); + if (process.platform !== "win32") return; + + const result = spawnSync(process.execPath, [processGroupRunner, repoRoot, "unused"], { + encoding: "utf8", + }); + expect(result.status).toBe(1); + expect(result.stderr).toContain("use WSL on Windows"); + }); +}); + +const describePosix = process.platform === "win32" ? describe.skip : describe; +describePosix("scripts/pr per-PR operation lock", () => { + it("serializes the same PR and releases the waiter after SIGTERM", async () => { + const repoDir = createRepo(); + const held = join(repoDir, "held"); + const acquired = join(repoDir, "acquired"); + const holder = spawnHolder(repoDir, held); + let waiter: ChildProcess | undefined; + try { + expect(await waitFor(() => existsSync(held))).toBe(true); + + waiter = spawnDetached( + "bash", + [ + "-c", + [ + ...bashSource(repoDir), + "acquire_pr_operation_lock 42", + `printf 'acquired\\n' >'${acquired}'`, + "release_pr_operation_lock", + ].join("\n"), + ], + { cwd: repoDir, stdio: "ignore" }, + ); + await new Promise((resolve) => setTimeout(resolve, 250)); + expect(existsSync(acquired)).toBe(false); + + await stopChild(holder, "SIGTERM"); + expect(await waitFor(() => existsSync(acquired))).toBe(true); + await waitForExit(waiter); + } finally { + await cleanupChildren(waiter, holder); + } + }); + + it("allows different PRs to proceed concurrently", async () => { + const repoDir = createRepo(); + const held = join(repoDir, "held"); + const holder = spawnHolder(repoDir, held); + try { + expect(await waitFor(() => existsSync(held))).toBe(true); + const other = runLockShell(repoDir, [ + "acquire_pr_operation_lock 43", + "release_pr_operation_lock", + ]); + expect(other.status, `${other.stdout}\\n${other.stderr}`).toBe(0); + } finally { + await cleanupChildren(holder); + } + }); + + it("does not publish a candidate paused before the create CAS", async () => { + const repoDir = createRepo(); + const prepared = join(repoDir, "prepared"); + const candidate = spawnCandidate(repoDir, prepared); + try { + expect(await waitFor(() => existsSync(prepared))).toBe(true); + const winner = runLockShell(repoDir, [ + "acquire_pr_operation_lock 42", + "release_pr_operation_lock", + ]); + expect(winner.status, `${winner.stdout}\\n${winner.stderr}`).toBe(0); + } finally { + await cleanupChildren(candidate); + } + }); + + it("requires exact recovery after a SIGKILL owner disappears", async () => { + const repoDir = createRepo(); + const held = join(repoDir, "held"); + const holder = spawnHolder(repoDir, held, 42, false); + try { + expect(await waitFor(() => existsSync(held))).toBe(true); + const ownerOid = refOid(repoDir); + await stopChild(holder, "SIGKILL"); + + const blocked = runLockShell(repoDir, [ + "set +e", + "acquire_pr_operation_lock 42", + "lock_status=$?", + "set -e", + 'printf "%s\\n" "$lock_status"', + ]); + expect(blocked.status).toBe(0); + expect(blocked.stdout.trim()).toBe("2"); + expect(blocked.stderr).toContain( + `scripts/pr lock-recover 42 ${ownerOid} --confirmed-no-running-tools`, + ); + expect(refOid(repoDir)).toBe(ownerOid); + + const recovered = runLockShell(repoDir, [ + `recover_pr_operation_lock 42 '${ownerOid}' --confirmed-no-running-tools`, + ]); + expect(recovered.status, `${recovered.stdout}\n${recovered.stderr}`).toBe(0); + expect(refExists(repoDir)).toBe(false); + } finally { + await cleanupChildren(holder); + } + }); + + it("makes an exact-OID late release harmless after a successor acquires", async () => { + const repoDir = createRepo(); + const firstHeld = join(repoDir, "first-held"); + const first = spawnHolder(repoDir, firstHeld, 42, false); + let second: ChildProcess | undefined; + try { + expect(await waitFor(() => existsSync(firstHeld))).toBe(true); + const oldOid = refOid(repoDir); + await stopChild(first, "SIGKILL"); + expect(await waitFor(() => !processGroupExists(first.pid!))).toBe(true); + const recovered = runLockShell(repoDir, [ + `recover_pr_operation_lock 42 '${oldOid}' --confirmed-no-running-tools`, + ]); + expect(recovered.status, `${recovered.stdout}\n${recovered.stderr}`).toBe(0); + + const secondHeld = join(repoDir, "second-held"); + second = spawnHolder(repoDir, secondHeld); + expect(await waitFor(() => existsSync(secondHeld))).toBe(true); + const successorOid = refOid(repoDir); + const lateRelease = runLockShell(repoDir, [ + `PR_OPERATION_LOCK_REF='${lockRef}'`, + `PR_OPERATION_LOCK_OWNER_OID='${oldOid}'`, + "release_pr_operation_lock", + ]); + expect(lateRelease.status, `${lateRelease.stdout}\\n${lateRelease.stderr}`).toBe(0); + expect(refOid(repoDir)).toBe(successorOid); + } finally { + await cleanupChildren(second, first); + } + }); + + it("requires confirmation and the current exact OID for recovery", async () => { + const repoDir = createRepo(); + const held = join(repoDir, "held"); + const holder = spawnHolder(repoDir, held, 42, false); + try { + expect(await waitFor(() => existsSync(held))).toBe(true); + const ownerOid = refOid(repoDir); + const wrongOid = execFileSync("git", ["rev-parse", "HEAD"], { + cwd: repoDir, + encoding: "utf8", + }).trim(); + + const unconfirmed = runLockShell(repoDir, [ + "set +e", + `recover_pr_operation_lock 42 '${ownerOid}'`, + "recovery_status=$?", + "set -e", + 'printf "%s\\n" "$recovery_status"', + ]); + expect(unconfirmed.status).toBe(0); + expect(unconfirmed.stdout.trim()).toBe("2"); + expect(unconfirmed.stderr).toContain("Recovery requires --confirmed-no-running-tools"); + expect(refOid(repoDir)).toBe(ownerOid); + + const wrongOwner = runLockShell(repoDir, [ + "set +e", + `recover_pr_operation_lock 42 '${wrongOid}' --confirmed-no-running-tools`, + "recovery_status=$?", + "set -e", + 'printf "%s\\n" "$recovery_status"', + ]); + expect(wrongOwner.status).toBe(0); + expect(wrongOwner.stdout.trim()).toBe("1"); + expect(refOid(repoDir)).toBe(ownerOid); + } finally { + await cleanupChildren(holder); + } + }); + + it("preserves a successor when recovery loses its exact-OID CAS", () => { + const repoDir = createRepo(); + const result = runLockShell(repoDir, [ + "owner_oid=$(printf 'owner-lock\\n' | git hash-object -w --stdin)", + "successor_oid=$(printf 'successor-lock\\n' | git hash-object -w --stdin)", + `git update-ref '${lockRef}' "$owner_oid"`, + "git() {", + ` if [ "$*" = "-C ${repoDir} update-ref --no-deref -d ${lockRef} $owner_oid" ]; then`, + ` command git -C '${repoDir}' update-ref '${lockRef}' "$successor_oid" "$owner_oid"`, + " return 1", + " fi", + ' command git "$@"', + "}", + "set +e", + 'recover_pr_operation_lock 42 "$owner_oid" --confirmed-no-running-tools', + "recovery_status=$?", + "set -e", + `printf '%s\\t%s\\n' "$recovery_status" "$(command git rev-parse '${lockRef}')"`, + ]); + const successorOid = execFileSync("git", ["hash-object", "--stdin"], { + cwd: repoDir, + input: "successor-lock\n", + encoding: "utf8", + }).trim(); + + expect(result.status, `${result.stdout}\n${result.stderr}`).toBe(0); + expect(result.stdout.trim()).toBe(`1\t${successorOid}`); + expect(result.stderr).toContain("owner changed during recovery"); + }); + + it("runs lock recovery without the normal PR toolchain", () => { + const repoDir = createRepo(); + const { binDir, cli } = installPrCliFixture(repoDir); + const ownerOid = execFileSync("git", ["rev-parse", "HEAD"], { + cwd: repoDir, + encoding: "utf8", + }).trim(); + execFileSync("git", ["update-ref", lockRef, ownerOid], { cwd: repoDir }); + + const result = spawnSync( + cli, + ["lock-recover", "42", ownerOid, "--confirmed-no-running-tools"], + { + cwd: repoDir, + encoding: "utf8", + env: { ...process.env, PATH: binDir }, + }, + ); + + expect(result.status, `${result.stdout}\n${result.stderr}`).toBe(0); + expect(result.stdout.trim()).toBe("Recovered the stale operation lock for PR #42."); + expect(refExists(repoDir)).toBe(false); + }); + + it("does not lock an unsupported command that shares a known prefix", () => { + const repoDir = createRepo(); + const { cli } = installPrCliFixture(repoDir); + const result = spawnSync(cli, ["review-not-a-command", "42"], { + cwd: repoDir, + encoding: "utf8", + }); + + expect(result.status, `${result.stdout}\n${result.stderr}`).toBe(2); + expect(result.stdout).toContain("Usage:"); + expect(refExists(repoDir)).toBe(false); + }); + + it("does not lock review-tests before validating its required target", () => { + const repoDir = createRepo(); + const { cli } = installPrCliFixture(repoDir); + const result = spawnSync(cli, ["review-tests", "42"], { + cwd: repoDir, + encoding: "utf8", + }); + + expect(result.status, `${result.stdout}\n${result.stderr}`).toBe(2); + expect(result.stdout).toContain("Usage:"); + expect(refExists(repoDir)).toBe(false); + }); + + it("does not trust an ambient dedicated-process-group marker", () => { + const repoDir = createRepo(); + const { binDir, cli } = installPrCliFixture(repoDir); + const reviewScript = join(repoDir, "scripts/pr-lib/review.sh"); + writeFileSync(reviewScript, `${readFileSync(reviewScript, "utf8")}\nreview_init() { :; }\n`); + for (const command of ["gh", "jq", "pnpm", "rg"]) { + const stub = join(binDir, command); + writeFileSync(stub, "#!/bin/sh\nexit 0\n"); + chmodSync(stub, 0o755); + } + const env = { + ...process.env, + OPENCLAW_PR_DEDICATED_PROCESS_GROUP: "1", + PATH: `${binDir}:${process.env.PATH ?? ""}`, + }; + delete env.OPENCLAW_PR_LOCK_NOTIFY_FD; + delete env.OPENCLAW_PR_LOCK_SUPERVISOR_PID; + + const result = spawnSync(cli, ["review-init", "42"], { + cwd: repoDir, + encoding: "utf8", + env, + }); + + expect(result.status, `${result.stdout}\n${result.stderr}`).toBe(0); + expect(refExists(repoDir)).toBe(false); + }); + + it.each([["--dryrun"], ["--dry-run", "extra"]])( + "rejects invalid gc arguments before cleanup: %s", + (...args: string[]) => { + const repoDir = createRepo(); + const { cli } = installPrCliFixture(repoDir); + const result = spawnSync(cli, ["gc", ...args], { + cwd: repoDir, + encoding: "utf8", + }); + + expect(result.status, `${result.stdout}\n${result.stderr}`).toBe(2); + expect(result.stdout).toContain("Usage:"); + expect(refExists(repoDir)).toBe(false); + }, + ); + + it("recovers an exact owner despite an unrelated reused live PGID", async () => { + const repoDir = createRepo(); + const unrelated = spawnDetached("sleep", ["30"], { stdio: "ignore" }); + try { + const unrelatedPgid = unrelated.pid!; + const result = runLockShell(repoDir, [ + `owner_oid=$(printf 'version=3\\nstate=active\\npgid=%s\\nsupervisor_pid=2147483647\\nsupervisor_birth=Mon Jan 1 00:00:00 1900\\ntoken=11111111-1111-1111-1111-111111111111\\n' '${unrelatedPgid}' | git hash-object -w --stdin)`, + `git update-ref '${lockRef}' "$owner_oid"`, + "set +e", + "acquire_pr_operation_lock 42", + "lock_status=$?", + "set -e", + 'printf "%s\\t%s\\n" "$lock_status" "$owner_oid"', + 'recover_pr_operation_lock 42 "$owner_oid" --confirmed-no-running-tools', + ]); + + expect(result.status, `${result.stdout}\n${result.stderr}`).toBe(0); + const [blockedLine] = result.stdout.trim().split("\n"); + const [, ownerOid] = blockedLine.split("\t"); + expect(blockedLine).toMatch(/^2\t[0-9a-f]{40}$/u); + expect(result.stderr).toContain("operation lock is orphaned"); + expect(result.stderr).toContain( + `scripts/pr lock-recover 42 ${ownerOid} --confirmed-no-running-tools`, + ); + expect(processGroupExists(unrelatedPgid)).toBe(true); + expect(refExists(repoDir)).toBe(false); + } finally { + await cleanupChildren(unrelated); + } + }); + + it("retries when the prior owner releases between failed create CAS and ref read", () => { + const repoDir = createRepo(); + const raceTriggered = join(repoDir, "race-triggered"); + const result = runLockShell(repoDir, [ + "prepare_pr_operation_lock_candidate 99", + "old_oid=$PR_OPERATION_LOCK_CANDIDATE_OID", + `git update-ref '${lockRef}' "$old_oid"`, + "git() {", + ` if [ ! -e '${raceTriggered}' ] && [[ "$*" == *"rev-parse --verify ${lockRef}"* ]]; then`, + ` : >'${raceTriggered}'`, + ` command git -C '${repoDir}' update-ref --no-deref -d '${lockRef}' "$old_oid"`, + " return 1", + " fi", + ' command git "$@"', + "}", + "acquire_pr_operation_lock 42", + "release_pr_operation_lock", + ]); + expect(result.status).toBe(0); + expect(existsSync(raceTriggered)).toBe(true); + }); + + it("retries when the finishing supervisor releases before an orphan verdict", () => { + const repoDir = createRepo(); + const result = runLockShell(repoDir, [ + "prepare_pr_operation_lock_candidate 42", + "stale_oid=$(printf 'version=3\\nstate=active\\npgid=2147483647\\nsupervisor_pid=2147483647\\nsupervisor_birth=Mon Jan 1 00:00:00 1900\\ntoken=11111111-1111-1111-1111-111111111111\\n' | git hash-object -w --stdin)", + `git update-ref '${lockRef}' "$stale_oid"`, + "pr_operation_lock_process_group_status() {", + ` command git -C '${repoDir}' update-ref --no-deref -d '${lockRef}' "$stale_oid"`, + " printf 'dead\\n'", + "}", + "pr_operation_lock_process_identity() { return 1; }", + "try_acquire_pr_operation_lock 42", + "release_pr_operation_lock", + ]); + + expect(result.status, `${result.stdout}\n${result.stderr}`).toBe(0); + expect(refExists(repoDir)).toBe(false); + }); + + it("keeps a live orphaned operation group sticky and surfaces recovery", async () => { + const repoDir = createRepo(); + const held = join(repoDir, "held"); + const childPid = join(repoDir, "child-pid"); + const holder = spawnHolderWithChild(repoDir, held, childPid); + try { + expect(await waitFor(() => existsSync(held) && existsSync(childPid))).toBe(true); + const ownerOid = refOid(repoDir); + // Leave the same-group child alive to model a controller-only failure. + await stopChildLeader(holder, "SIGKILL"); + + const blocked = runLockShell(repoDir, [ + "set +e", + "try_acquire_pr_operation_lock 42", + "lock_status=$?", + "set -e", + `printf '%s\t%s\t%s\n' "$lock_status" "$PR_OPERATION_LOCK_BLOCKED_REASON" "$(git rev-parse '${lockRef}')"`, + ]); + expect(blocked.status, `${blocked.stdout}\\n${blocked.stderr}`).toBe(0); + expect(blocked.stdout.trim()).toBe(`2\torphaned\t${ownerOid}`); + killProcessGroup(holder.pid!, "SIGTERM"); + expect(await waitFor(() => !processGroupExists(holder.pid!))).toBe(true); + + const stillBlocked = runLockShell(repoDir, [ + "set +e", + "acquire_pr_operation_lock 42", + "lock_status=$?", + "set -e", + 'printf "%s\\n" "$lock_status"', + ]); + expect(stillBlocked.status).toBe(0); + expect(stillBlocked.stdout.trim()).toBe("2"); + + const recovered = runLockShell(repoDir, [ + `recover_pr_operation_lock 42 '${ownerOid}' --confirmed-no-running-tools`, + "acquire_pr_operation_lock 42", + "release_pr_operation_lock", + ]); + expect(recovered.status, `${recovered.stdout}\\n${recovered.stderr}`).toBe(0); + } finally { + await cleanupChildren(holder); + } + }); + + it("rejects noncanonical aliases for the same PR number", () => { + const repoDir = createRepo(); + const result = runLockShell(repoDir, [ + "set +e", + "try_acquire_pr_operation_lock 00042", + "lock_status=$?", + "pr_number_from_worktree_dir .worktrees/pr-00042 >/dev/null", + "parse_status=$?", + "set -e", + 'printf "%s\t%s\n" "$lock_status" "$parse_status"', + ]); + + expect(result.status).toBe(0); + expect(result.stdout.trim()).toBe("2\t1"); + }); + + it("retries release while the owner ref is unchanged", () => { + const repoDir = createRepo(); + const result = runLockShell(repoDir, [ + "acquire_pr_operation_lock 42", + "owner_oid=$PR_OPERATION_LOCK_OWNER_OID", + "delete_attempts=0", + "git() {", + ` if [ "$*" = "-C ${repoDir} update-ref --no-deref -d ${lockRef} $owner_oid" ]; then`, + " delete_attempts=$((delete_attempts + 1))", + ' if [ "$delete_attempts" -lt 3 ]; then return 1; fi', + " fi", + ' command git "$@"', + "}", + "release_pr_operation_lock", + `if command git show-ref --verify --quiet '${lockRef}'; then ref_status=present; else ref_status=absent; fi`, + 'printf "%s\t%s\n" "$delete_attempts" "$ref_status"', + ]); + + expect(result.status, `${result.stdout}\n${result.stderr}`).toBe(0); + expect(result.stdout.trim()).toBe("3\tabsent"); + }); + + it("fails release when the owner ref stays unchanged", () => { + const repoDir = createRepo(); + const result = runLockShell(repoDir, [ + "acquire_pr_operation_lock 42", + "owner_oid=$PR_OPERATION_LOCK_OWNER_OID", + "git() {", + ` if [ "$*" = "-C ${repoDir} update-ref --no-deref -d ${lockRef} $owner_oid" ]; then return 1; fi`, + ' command git "$@"', + "}", + "set +e", + "release_pr_operation_lock", + "release_status=$?", + "set -e", + 'printf "%s\t%s\n" "$release_status" "$PR_OPERATION_LOCK_OWNER_OID"', + ]); + + expect(result.status).toBe(0); + expect(result.stdout.trim()).toBe(`1\t${refOid(repoDir)}`); + expect(result.stderr).toContain("Unable to release the operation lock for 42"); + }); + + it("has the process-group supervisor release the exact owner ref", async () => { + const repoDir = createRepo(); + const fixture = writeOperationFixture(repoDir, "acquire-once.sh", [ + "acquire_pr_operation_lock 42", + ]); + const result = await runSupervisedFixture(repoDir, fixture); + + expect(result.status, `${result.stdout}\n${result.stderr}`).toBe(0); + expect(refExists(repoDir)).toBe(false); + }); + + it("retains the exact owner when supervisor release cannot take the ref lock", async () => { + const repoDir = createRepo(); + const refLock = join(repoDir, ".git/refs/openclaw/pr-operation-locks/42.lock"); + const fixture = writeOperationFixture(repoDir, "blocked-release.sh", [ + "acquire_pr_operation_lock 42", + `: >'${refLock}'`, + ]); + const result = await runSupervisedFixture(repoDir, fixture); + const ownerOid = refOid(repoDir); + + expect(result.status, `${result.stdout}\n${result.stderr}`).toBe(1); + expect(result.stderr).toContain("Unable to release the operation lock for 42"); + expect(result.stderr).toContain( + `scripts/pr lock-recover 42 ${ownerOid} --confirmed-no-running-tools`, + ); + expect(refOid(repoDir)).toBe(ownerOid); + + unlinkSync(refLock); + const recovered = runLockShell(repoDir, [ + `recover_pr_operation_lock 42 '${ownerOid}' --confirmed-no-running-tools`, + ]); + expect(recovered.status, `${recovered.stdout}\n${recovered.stderr}`).toBe(0); + expect(refExists(repoDir)).toBe(false); + }); + + it("reports exact recovery when lock notification fails", () => { + const repoDir = createRepo(); + const result = runLockShell(repoDir, [ + "OPENCLAW_PR_LOCK_NOTIFY_FD=9", + "set +e", + "acquire_pr_operation_lock 42", + "lock_status=$?", + "set -e", + 'printf "%s\\n" "$lock_status"', + ]); + + expect(result.status, `${result.stdout}\n${result.stderr}`).toBe(0); + expect(result.stdout.trim()).toBe("2"); + const ownerOid = refOid(repoDir); + expect(result.stderr).toContain( + `scripts/pr lock-recover 42 ${ownerOid} --confirmed-no-running-tools`, + ); + + const recovered = runLockShell(repoDir, [ + `recover_pr_operation_lock 42 '${ownerOid}' --confirmed-no-running-tools`, + ]); + expect(recovered.status, `${recovered.stdout}\n${recovered.stderr}`).toBe(0); + expect(refExists(repoDir)).toBe(false); + }); + + it("rejects a notification for a lock owned by another process group", async () => { + const repoDir = createRepo(); + const foreignRef = "refs/openclaw/pr-operation-locks/43"; + const foreignHeld = join(repoDir, "foreign-held"); + const foreignHolder = spawnHolder(repoDir, foreignHeld, 43); + try { + expect(await waitFor(() => existsSync(foreignHeld))).toBe(true); + const foreignOid = refOid(repoDir, foreignRef); + const fixture = writeOperationFixture(repoDir, "forged-notification.sh", [ + "acquire_pr_operation_lock 42", + `printf '%s\\t%s\\n' '${foreignRef}' '${foreignOid}' >&"$OPENCLAW_PR_LOCK_NOTIFY_FD"`, + ]); + const result = await runSupervisedFixture(repoDir, fixture); + + expect(result.status, `${result.stdout}\n${result.stderr}`).toBe(1); + expect(refOid(repoDir, foreignRef)).toBe(foreignOid); + expect(refExists(repoDir)).toBe(true); + expect(result.stderr).toContain("operation lock owned by another process group"); + + const ownerOid = refOid(repoDir); + const recovered = runLockShell(repoDir, [ + `recover_pr_operation_lock 42 '${ownerOid}' --confirmed-no-running-tools`, + ]); + expect(recovered.status, `${recovered.stdout}\n${recovered.stderr}`).toBe(0); + } finally { + await cleanupChildren(foreignHolder); + } + }); + + it.each([ + ["newline-terminated", "printf 'not-lock-metadata\\n'"], + ["unterminated", "printf 'not-lock-metadata'"], + ])("retains the lock after %s malformed supervisor metadata", async (_name, command) => { + const repoDir = createRepo(); + const fixture = writeOperationFixture(repoDir, "malformed-notification.sh", [ + "acquire_pr_operation_lock 42", + `${command} >&"$OPENCLAW_PR_LOCK_NOTIFY_FD"`, + ]); + const result = await runSupervisedFixture(repoDir, fixture); + const ownerOid = refOid(repoDir); + + expect(result.status, `${result.stdout}\n${result.stderr}`).toBe(1); + expect(result.stderr).toContain("malformed operation-lock metadata"); + expect(result.stderr).toContain( + `scripts/pr lock-recover 42 ${ownerOid} --confirmed-no-running-tools`, + ); + expect(refOid(repoDir)).toBe(ownerOid); + + const recovered = runLockShell(repoDir, [ + `recover_pr_operation_lock 42 '${ownerOid}' --confirmed-no-running-tools`, + ]); + expect(recovered.status, `${recovered.stdout}\n${recovered.stderr}`).toBe(0); + expect(refExists(repoDir)).toBe(false); + }); + + it("bounds an oversized unterminated supervisor metadata line", async () => { + const repoDir = createRepo(); + const fixture = writeOperationFixture(repoDir, "oversized-notification.sh", [ + "acquire_pr_operation_lock 42", + `node -e 'process.stdout.write("x".repeat(8192))' >&"$OPENCLAW_PR_LOCK_NOTIFY_FD"`, + ]); + const result = await runSupervisedFixture(repoDir, fixture); + const ownerOid = refOid(repoDir); + + expect(result.status, `${result.stdout}\n${result.stderr}`).toBe(1); + expect(result.stderr).toContain("operation-lock metadata line is too large"); + expect(result.stderr).toContain( + `scripts/pr lock-recover 42 ${ownerOid} --confirmed-no-running-tools`, + ); + expect(refOid(repoDir)).toBe(ownerOid); + + const recovered = runLockShell(repoDir, [ + `recover_pr_operation_lock 42 '${ownerOid}' --confirmed-no-running-tools`, + ]); + expect(recovered.status, `${recovered.stdout}\n${recovered.stderr}`).toBe(0); + expect(refExists(repoDir)).toBe(false); + }); + + it("uses successful command return as the trusted completion contract", async () => { + const repoDir = createRepo(); + const daemonPidFile = join(repoDir, "unrelated-daemon-pgid"); + const daemonScript = join(repoDir, "unrelated-daemon.mjs"); + const launcherScript = join(repoDir, "unrelated-daemon-launcher.mjs"); + writeFileSync(daemonScript, "setInterval(() => {}, 1000);\n"); + writeFileSync( + launcherScript, + [ + 'import { spawn } from "node:child_process";', + 'import fs from "node:fs";', + `const child = spawn(process.execPath, [${JSON.stringify(daemonScript)}], {`, + " detached: true,", + ' stdio: "ignore",', + "});", + `fs.writeFileSync(${JSON.stringify(daemonPidFile)}, String(child.pid));`, + "child.unref();", + ].join("\n"), + ); + const fixture = writeOperationFixture(repoDir, "clean-detached-launcher.sh", [ + "acquire_pr_operation_lock 42", + `node '${launcherScript}'`, + ]); + let daemonPgid: number | undefined; + try { + const result = await runSupervisedFixture(repoDir, fixture); + daemonPgid = await waitForProcessId(daemonPidFile); + + expect(result.status, `${result.stdout}\n${result.stderr}`).toBe(0); + expect(processGroupExists(daemonPgid)).toBe(true); + expect(refExists(repoDir)).toBe(false); + } finally { + daemonPgid ??= readProcessIdFile(daemonPidFile); + if (daemonPgid) await cleanupProcessGroup(daemonPgid); + } + }); + + it("retains a failed operation lock when a detached child outlives its launcher", async () => { + const repoDir = createRepo(); + const nestedPidFile = join(repoDir, "failed-nested-pgid"); + const nestedScript = join(repoDir, "failed-nested.mjs"); + const launcherScript = join(repoDir, "failing-launcher.mjs"); + writeFileSync( + nestedScript, + ['process.on("SIGTERM", () => {});', "setInterval(() => {}, 1000);"].join("\n"), + ); + writeFileSync( + launcherScript, + [ + 'import { spawn } from "node:child_process";', + 'import fs from "node:fs";', + `const child = spawn(process.execPath, [${JSON.stringify(nestedScript)}], {`, + " detached: true,", + ' stdio: "ignore",', + "});", + `fs.writeFileSync(${JSON.stringify(nestedPidFile)}, String(child.pid));`, + "process.exit(1);", + ].join("\n"), + ); + const fixture = writeOperationFixture(repoDir, "failed-operation.sh", [ + "acquire_pr_operation_lock 42", + `node '${launcherScript}'`, + ]); + let nestedPgid: number | undefined; + try { + const result = await runSupervisedFixture(repoDir, fixture); + expect(result.status, `${result.stdout}\n${result.stderr}`).toBe(1); + nestedPgid = await waitForProcessId(nestedPidFile); + expect(processGroupExists(nestedPgid!)).toBe(true); + const ownerOid = refOid(repoDir); + expect(result.stderr).toContain( + `scripts/pr lock-recover 42 ${ownerOid} --confirmed-no-running-tools`, + ); + + const blocked = runLockShell(repoDir, [ + "set +e", + "try_acquire_pr_operation_lock 42", + "lock_status=$?", + "set -e", + 'printf "%s\\n" "$lock_status"', + ]); + expect(blocked.status).toBe(0); + expect(blocked.stdout.trim()).toBe("2"); + + killProcessGroup(nestedPgid!, "SIGKILL"); + expect(await waitFor(() => !processGroupExists(nestedPgid!))).toBe(true); + const recovered = runLockShell(repoDir, [ + `recover_pr_operation_lock 42 '${ownerOid}' --confirmed-no-running-tools`, + ]); + expect(recovered.status, `${recovered.stdout}\n${recovered.stderr}`).toBe(0); + expect(refExists(repoDir)).toBe(false); + } finally { + nestedPgid ??= readProcessIdFile(nestedPidFile); + if (nestedPgid) await cleanupProcessGroup(nestedPgid); + } + }); + + it("exits after a bounded wait when a detached child keeps the notification pipe open", async () => { + const repoDir = createRepo(); + const nestedPidFile = join(repoDir, "pipe-holder-pgid"); + const nestedScript = join(repoDir, "pipe-holder.mjs"); + const launcherScript = join(repoDir, "pipe-holder-launcher.mjs"); + writeFileSync(nestedScript, "setInterval(() => {}, 1000);\n"); + writeFileSync( + launcherScript, + [ + 'import { spawn } from "node:child_process";', + 'import fs from "node:fs";', + `const child = spawn(process.execPath, [${JSON.stringify(nestedScript)}], {`, + " detached: true,", + ' stdio: ["ignore", "ignore", "ignore", 3],', + "});", + `fs.writeFileSync(${JSON.stringify(nestedPidFile)}, String(child.pid));`, + "process.exit(1);", + ].join("\n"), + ); + const fixture = writeOperationFixture(repoDir, "pipe-holder-operation.sh", [ + "acquire_pr_operation_lock 42", + `node '${launcherScript}'`, + ]); + let nestedPgid: number | undefined; + try { + const startedAt = Date.now(); + const result = await runSupervisedFixture(repoDir, fixture); + const elapsed = Date.now() - startedAt; + nestedPgid = await waitForProcessId(nestedPidFile); + + expect(result.status, `${result.stdout}\n${result.stderr}`).toBe(1); + expect(elapsed).toBeLessThan(12_000); + expect(processGroupExists(nestedPgid)).toBe(true); + expect(result.stderr).toContain("operation lifetime did not drain"); + const ownerOid = refOid(repoDir); + + killProcessGroup(nestedPgid, "SIGKILL"); + expect(await waitFor(() => !processGroupExists(nestedPgid!))).toBe(true); + const recovered = runLockShell(repoDir, [ + `recover_pr_operation_lock 42 '${ownerOid}' --confirmed-no-running-tools`, + ]); + expect(recovered.status, `${recovered.stdout}\n${recovered.stderr}`).toBe(0); + expect(refExists(repoDir)).toBe(false); + } finally { + nestedPgid ??= readProcessIdFile(nestedPidFile); + if (nestedPgid) await cleanupProcessGroup(nestedPgid); + } + }, 15_000); + + it("waits while a live supervisor finishes draining a dead operation group", async () => { + const repoDir = createRepo(); + const operationPgidFile = join(repoDir, "finishing-operation-pgid"); + const holderPidFile = join(repoDir, "finishing-holder-pgid"); + const acquiredFile = join(repoDir, "finishing-waiter-acquired"); + const holderScript = join(repoDir, "finishing-holder.mjs"); + const launcherScript = join(repoDir, "finishing-launcher.mjs"); + writeFileSync(holderScript, "setTimeout(() => {}, 1500);\n"); + writeFileSync( + launcherScript, + [ + 'import { spawn } from "node:child_process";', + 'import fs from "node:fs";', + `const child = spawn(process.execPath, [${JSON.stringify(holderScript)}], {`, + " detached: true,", + ' stdio: ["ignore", "ignore", "ignore", 3],', + "});", + `fs.writeFileSync(${JSON.stringify(holderPidFile)}, String(child.pid));`, + "process.exit(0);", + ].join("\n"), + ); + const fixture = writeOperationFixture(repoDir, "finishing-operation.sh", [ + `printf '%s\\n' "$$" >'${operationPgidFile}'`, + "acquire_pr_operation_lock 42", + `node '${launcherScript}'`, + ]); + const controller = spawn(process.execPath, [processGroupRunner, repoDir, fixture], { + cwd: repoDir, + stdio: "ignore", + }); + let waiter: ChildProcess | undefined; + let holderPgid: number | undefined; + try { + const operationPgid = await waitForProcessId(operationPgidFile); + holderPgid = await waitForProcessId(holderPidFile); + expect(await waitFor(() => !processGroupExists(operationPgid))).toBe(true); + expect(refExists(repoDir)).toBe(true); + + const probe = runLockShell(repoDir, [ + "set +e", + "try_acquire_pr_operation_lock 42", + "lock_status=$?", + "set -e", + 'printf "%s\\n" "$lock_status"', + ]); + expect(probe.status, `${probe.stdout}\n${probe.stderr}`).toBe(0); + expect(probe.stdout.trim()).toBe("1"); + + waiter = spawnDetached( + "bash", + [ + "-c", + [ + ...bashSource(repoDir), + "acquire_pr_operation_lock 42", + `printf 'acquired\\n' >'${acquiredFile}'`, + "release_pr_operation_lock", + ].join("\n"), + ], + { cwd: repoDir, stdio: "ignore" }, + ); + await new Promise((resolve) => setTimeout(resolve, 250)); + expect(existsSync(acquiredFile)).toBe(false); + + await waitForExit(controller, 5000); + await waitForExit(waiter, 5000); + expect(controller.exitCode).toBe(0); + expect(waiter.exitCode).toBe(0); + expect(existsSync(acquiredFile)).toBe(true); + expect(refExists(repoDir)).toBe(false); + } finally { + holderPgid ??= readProcessIdFile(holderPidFile); + if (holderPgid) await cleanupProcessGroup(holderPgid); + await cleanupChildren(waiter); + await cleanupController(repoDir, controller, operationPgidFile); + } + }, 12_000); + + it("drains a same-group background job after its wrapper fails", async () => { + const repoDir = createRepo(); + const operationPgidFile = join(repoDir, "failed-operation-pgid"); + const backgroundPidFile = join(repoDir, "failed-background-pid"); + const fixture = writeOperationFixture(repoDir, "failed-background-operation.sh", [ + `printf '%s\\n' "$$" >'${operationPgidFile}'`, + "acquire_pr_operation_lock 42", + "sleep 30 &", + `printf '%s\\n' "$!" >'${backgroundPidFile}'`, + "exit 1", + ]); + + let operationPgid: number | undefined; + try { + const result = await runSupervisedFixture(repoDir, fixture); + operationPgid = await waitForProcessId(operationPgidFile); + const ownerOid = refOid(repoDir); + expect(result.status, `${result.stdout}\n${result.stderr}`).toBe(1); + expect(await waitForProcessId(backgroundPidFile)).toBeGreaterThan(1); + expect(processGroupExists(operationPgid)).toBe(false); + expect(refOid(repoDir)).toBe(ownerOid); + expect(result.stderr).toContain( + `scripts/pr lock-recover 42 ${ownerOid} --confirmed-no-running-tools`, + ); + + const recovered = runLockShell(repoDir, [ + `recover_pr_operation_lock 42 '${ownerOid}' --confirmed-no-running-tools`, + ]); + expect(recovered.status, `${recovered.stdout}\n${recovered.stderr}`).toBe(0); + expect(refExists(repoDir)).toBe(false); + } finally { + operationPgid ??= readProcessIdFile(operationPgidFile); + if (operationPgid) await cleanupProcessGroup(operationPgid); + } + }); + + it("fails and retains the lock when a clean wrapper leaves same-group work", async () => { + const repoDir = createRepo(); + const operationPgidFile = join(repoDir, "clean-background-operation-pgid"); + const fixture = writeOperationFixture(repoDir, "clean-background-operation.sh", [ + `printf '%s\\n' "$$" >'${operationPgidFile}'`, + "acquire_pr_operation_lock 42", + "sleep 30 &", + "exit 0", + ]); + + let operationPgid: number | undefined; + try { + const result = await runSupervisedFixture(repoDir, fixture); + operationPgid = await waitForProcessId(operationPgidFile); + const ownerOid = refOid(repoDir); + expect(result.status, `${result.stdout}\n${result.stderr}`).toBe(1); + expect(processGroupExists(operationPgid)).toBe(false); + expect(result.stderr).toContain("process group remained active after wrapper exit"); + expect(result.stderr).toContain( + `scripts/pr lock-recover 42 ${ownerOid} --confirmed-no-running-tools`, + ); + + const recovered = runLockShell(repoDir, [ + `recover_pr_operation_lock 42 '${ownerOid}' --confirmed-no-running-tools`, + ]); + expect(recovered.status, `${recovered.stdout}\n${recovered.stderr}`).toBe(0); + expect(refExists(repoDir)).toBe(false); + } finally { + operationPgid ??= readProcessIdFile(operationPgidFile); + if (operationPgid) await cleanupProcessGroup(operationPgid); + } + }); + + it("keeps gc lock ownership with the supervisor until gc exits", async () => { + const repoDir = createRepo(); + mkdirSync(join(repoDir, ".worktrees", "pr-42"), { recursive: true }); + const ghStarted = join(repoDir, "gc-gh-started"); + const ghContinue = join(repoDir, "gc-gh-continue"); + const outputFile = join(repoDir, "gc-output"); + const fixture = writeOperationFixture(repoDir, "gc.sh", [ + "gh() {", + ` : >'${ghStarted}'`, + ` while [ ! -e '${ghContinue}' ]; do sleep 0.05; done`, + " printf 'MERGED\\n'", + "}", + `gc_pr_worktrees true >'${outputFile}'`, + ]); + const controller = spawn(process.execPath, [processGroupRunner, repoDir, fixture], { + cwd: repoDir, + stdio: "ignore", + }); + try { + expect(await waitFor(() => existsSync(ghStarted) && refExists(repoDir))).toBe(true); + const probe = runLockShell(repoDir, [ + "set +e", + "try_acquire_pr_operation_lock 42", + "lock_status=$?", + "set -e", + 'printf "%s\\n" "$lock_status"', + ]); + expect(probe.status, `${probe.stdout}\n${probe.stderr}`).toBe(0); + expect(probe.stdout.trim()).toBe("1"); + + writeFileSync(ghContinue, "continue\n"); + await waitForExit(controller, 5000); + expect(controller.exitCode).toBe(0); + expect(readFileSync(outputFile, "utf8")).toContain("would remove .worktrees/pr-42"); + expect(refExists(repoDir)).toBe(false); + } finally { + writeFileSync(ghContinue, "continue\n"); + await cleanupController(repoDir, controller); + } + }); + + it("fails closed on malformed owner blobs", () => { + const repoDir = createRepo(); + const result = runLockShell(repoDir, [ + "bad_oid=$(printf 'not-a-lock\\n' | git hash-object -w --stdin)", + `git update-ref '${lockRef}' "$bad_oid"`, + "set +e", + "try_acquire_pr_operation_lock 42", + "lock_status=$?", + 'recover_pr_operation_lock 42 "$bad_oid" --confirmed-no-running-tools', + "recovery_status=$?", + "set -e", + 'printf "%s\\t%s\\n" "$lock_status" "$recovery_status"', + ]); + expect(result.status).toBe(0); + expect(result.stdout.trim().split("\n").at(-1)).toBe("2\t0"); + expect(refExists(repoDir)).toBe(false); + }); + + it("rejects special and out-of-range process-group ids", () => { + for (const pgid of ["1", "2147483648"]) { + const repoDir = createRepo(); + const result = runLockShell(repoDir, [ + 'supervisor_birth=$(pr_operation_lock_process_birth "$$")', + `bad_oid=$(printf 'version=3\\nstate=active\\npgid=${pgid}\\nsupervisor_pid=%s\\nsupervisor_birth=%s\\ntoken=11111111-1111-1111-1111-111111111111\\n' "$$" "$supervisor_birth" | git hash-object -w --stdin)`, + `git update-ref '${lockRef}' "$bad_oid"`, + "set +e", + "try_acquire_pr_operation_lock 42", + "lock_status=$?", + "set -e", + 'printf "%s\\n" "$lock_status"', + ]); + expect(result.status).toBe(0); + expect(result.stdout.trim()).toBe("2"); + } + }); + + it("fails closed when process-group liveness is not permitted", () => { + const repoDir = createRepo(); + const result = runLockShell(repoDir, [ + "prepare_pr_operation_lock_candidate 42", + 'supervisor_birth=$(pr_operation_lock_process_birth "$$")', + 'owner_oid=$(printf \'version=3\\nstate=active\\npgid=2\\nsupervisor_pid=%s\\nsupervisor_birth=%s\\ntoken=11111111-1111-1111-1111-111111111111\\n\' "$$" "$supervisor_birth" | git hash-object -w --stdin)', + `git update-ref '${lockRef}' "$owner_oid"`, + "node() { printf 'indeterminate\\n'; }", + "set +e", + "try_acquire_pr_operation_lock 42", + "lock_status=$?", + 'recover_pr_operation_lock 42 "$owner_oid" --confirmed-no-running-tools', + "recovery_status=$?", + "set -e", + 'printf "%s\\t%s\\n" "$lock_status" "$recovery_status"', + ]); + + expect(result.status).toBe(0); + expect(result.stdout.trim().split("\n").at(-1)).toBe("2\t0"); + expect(refExists(repoDir)).toBe(false); + }); + + it.runIf(process.platform === "linux")( + "conservatively keeps a lock whose process group contains a zombie", + async () => { + const repoDir = createRepo(); + const pidFile = join(repoDir, "zombie-pgid"); + const parent = spawnDetached( + "python3", + [ + "-c", + [ + "import os, time", + "pid = os.fork()", + "if pid == 0:", + " os.setpgid(0, 0)", + ` open(${JSON.stringify(pidFile)}, 'w').write(str(os.getpid()))`, + " os._exit(0)", + "time.sleep(30)", + ].join("\n"), + ], + { cwd: repoDir, stdio: "ignore" }, + ); + let zombiePgid: number | undefined; + try { + expect(await waitFor(() => existsSync(pidFile))).toBe(true); + zombiePgid = await waitForProcessId(pidFile); + expect( + await waitFor(() => { + const state = spawnSync("ps", ["-o", "state=", "-p", String(zombiePgid)], { + encoding: "utf8", + }).stdout.trim(); + return state.startsWith("Z"); + }), + ).toBe(true); + + const blocked = runLockShell(repoDir, [ + 'supervisor_birth=$(pr_operation_lock_process_birth "$$")', + `owner_oid=$(printf 'version=3\\nstate=active\\npgid=%s\\nsupervisor_pid=%s\\nsupervisor_birth=%s\\ntoken=11111111-1111-1111-1111-111111111111\\n' '${zombiePgid}' "$$" "$supervisor_birth" | git hash-object -w --stdin)`, + `git update-ref '${lockRef}' "$owner_oid"`, + "set +e", + "try_acquire_pr_operation_lock 42", + "lock_status=$?", + "set -e", + 'printf "%s\\n" "$lock_status"', + ]); + expect(blocked.status).toBe(0); + expect(blocked.stdout.trim()).toBe("1"); + + const ownerOid = refOid(repoDir); + await stopChild(parent, "SIGTERM"); + expect(await waitFor(() => !processGroupExists(zombiePgid!))).toBe(true); + const recovered = runLockShell(repoDir, [ + `recover_pr_operation_lock 42 '${ownerOid}' --confirmed-no-running-tools`, + "acquire_pr_operation_lock 42", + "release_pr_operation_lock", + ]); + expect(recovered.status, `${recovered.stdout}\n${recovered.stderr}`).toBe(0); + } finally { + await cleanupChildren(parent); + } + }, + 15_000, + ); + + it("keeps a dead owner sticky instead of guessing that detached work ended", () => { + const repoDir = createRepo(); + const result = runLockShell(repoDir, [ + "stale_oid=$(printf 'version=3\\nstate=active\\npgid=2147483647\\nsupervisor_pid=2147483647\\nsupervisor_birth=Mon Jan 1 00:00:00 1900\\ntoken=11111111-1111-1111-1111-111111111111\\n' | git hash-object -w --stdin)", + `git update-ref '${lockRef}' "$stale_oid"`, + "set +e", + "acquire_pr_operation_lock 42", + "lock_status=$?", + "set -e", + `printf '%s\t%s\n' "$lock_status" "$(command git rev-parse '${lockRef}')"`, + ]); + + expect(result.status).toBe(0); + expect(result.stdout.trim()).toBe(`2\t${refOid(repoDir)}`); + expect(result.stderr).toContain("detached child tools cannot be ruled out"); + expect(result.stderr).toContain( + `scripts/pr lock-recover 42 ${refOid(repoDir)} --confirmed-no-running-tools`, + ); + expect(result.stderr).toContain("Unable to acquire the operation lock for PR #42."); + }); + + it("preserves the exact lock if its controller is killed", async () => { + const repoDir = createRepo(); + const pidFile = join(repoDir, "operation-pgid"); + const held = join(repoDir, "held"); + const fixture = writeOperationFixture(repoDir, "operation.sh", [ + `printf '%s\\n' "$$" >'${pidFile}'`, + "acquire_pr_operation_lock 42", + `printf 'held\\n' >'${held}'`, + "while :; do sleep 1; done", + ]); + + const controller = spawn(process.execPath, [processGroupRunner, repoDir, fixture], { + cwd: repoDir, + stdio: "ignore", + }); + let pgid: number | undefined; + try { + expect(await waitFor(() => existsSync(pidFile) && existsSync(held))).toBe(true); + pgid = await waitForProcessId(pidFile); + const ownerOid = refOid(repoDir); + expect(processGroupExists(pgid!)).toBe(true); + + await stopChild(controller, "SIGKILL"); + expect(processGroupExists(pgid!)).toBe(true); + expect(refOid(repoDir)).toBe(ownerOid); + + const blockedWhileGroupLives = runLockShell(repoDir, [ + "set +e", + "try_acquire_pr_operation_lock 42", + "lock_status=$?", + "set -e", + 'printf "%s\\t%s\\t%s\\n" "$lock_status" "$PR_OPERATION_LOCK_BLOCKED_REASON" "$PR_OPERATION_LOCK_BLOCKED_OID"', + ]); + expect(blockedWhileGroupLives.status).toBe(0); + expect(blockedWhileGroupLives.stdout.trim()).toBe(`2\torphaned\t${ownerOid}`); + expect(processGroupExists(pgid!)).toBe(true); + expect(refOid(repoDir)).toBe(ownerOid); + + killProcessGroup(pgid!, "SIGTERM"); + expect(await waitFor(() => !processGroupExists(pgid!))).toBe(true); + const blocked = runLockShell(repoDir, [ + "set +e", + "acquire_pr_operation_lock 42", + "lock_status=$?", + "set -e", + 'printf "%s\\n" "$lock_status"', + ]); + expect(blocked.status).toBe(0); + expect(blocked.stdout.trim()).toBe("2"); + expect(refOid(repoDir)).toBe(ownerOid); + + const recovered = runLockShell(repoDir, [ + `recover_pr_operation_lock 42 '${ownerOid}' --confirmed-no-running-tools`, + "acquire_pr_operation_lock 42", + "release_pr_operation_lock", + ]); + expect(recovered.status, `${recovered.stdout}\n${recovered.stderr}`).toBe(0); + expect(refExists(repoDir)).toBe(false); + } finally { + await cleanupController(repoDir, controller, pidFile); + } + }); + + it("escalates a signal, drains its group, and retains the interrupted lock", async () => { + const repoDir = createRepo(); + const pidFile = join(repoDir, "operation-pgid"); + const childReady = join(repoDir, "child-ready"); + const fixture = writeOperationFixture(repoDir, "stubborn-operation.sh", [ + `printf '%s\\n' "$$" >'${pidFile}'`, + "trap 'exit 143' TERM", + "acquire_pr_operation_lock 42", + "(", + " trap '' HUP INT TERM", + ` printf 'ready\\n' >'${childReady}'`, + " while :; do sleep 1; done", + ") &", + 'wait "$!"', + ]); + const controller = spawn(process.execPath, [processGroupRunner, repoDir, fixture], { + cwd: repoDir, + stdio: "ignore", + }); + let pgid: number | undefined; + try { + expect(await waitFor(() => existsSync(pidFile) && existsSync(childReady))).toBe(true); + pgid = await waitForProcessId(pidFile); + expect(refExists(repoDir)).toBe(true); + + controller.kill("SIGTERM"); + await waitForExit(controller, 12_000); + + expect(controller.exitCode).toBe(143); + expect(processGroupExists(pgid!)).toBe(false); + expect(refExists(repoDir)).toBe(true); + const ownerOid = refOid(repoDir); + const recovered = runLockShell(repoDir, [ + `recover_pr_operation_lock 42 '${ownerOid}' --confirmed-no-running-tools`, + ]); + expect(recovered.status, `${recovered.stdout}\n${recovered.stderr}`).toBe(0); + expect(refExists(repoDir)).toBe(false); + } finally { + await cleanupController(repoDir, controller, pidFile); + } + }, 15_000); + + it("retains the lock when a nested managed process group escapes cancellation", async () => { + const repoDir = createRepo(); + const nestedPidFile = join(repoDir, "nested-pgid"); + const signalRelayedFile = join(repoDir, "nested-signal-relayed"); + const nestedScript = join(repoDir, "nested.mjs"); + const relayScript = join(repoDir, "relay.mjs"); + writeFileSync( + nestedScript, + [ + 'import fs from "node:fs";', + "fs.writeFileSync(process.argv[2], String(process.pid));", + 'process.on("SIGTERM", () => fs.writeFileSync(process.argv[3], "relayed\\n"));', + "setInterval(() => {}, 1000);", + ].join("\n"), + ); + writeFileSync( + relayScript, + [ + `import { runManagedCommand } from ${JSON.stringify(managedChildUrl)};`, + "process.exitCode = await runManagedCommand({", + " bin: process.execPath,", + ` args: [${JSON.stringify(nestedScript)}, ${JSON.stringify(nestedPidFile)}, ${JSON.stringify(signalRelayedFile)}],`, + ' stdio: "ignore",', + "});", + ].join("\n"), + ); + const fixture = writeOperationFixture(repoDir, "nested-operation.sh", [ + "acquire_pr_operation_lock 42", + `node '${relayScript}'`, + ]); + const controller = spawn(process.execPath, [processGroupRunner, repoDir, fixture], { + cwd: repoDir, + stdio: "ignore", + }); + let nestedPgid: number | undefined; + try { + expect(await waitFor(() => existsSync(nestedPidFile) && refExists(repoDir))).toBe(true); + nestedPgid = await waitForProcessId(nestedPidFile); + const ownerOid = refOid(repoDir); + expect(processGroupExists(nestedPgid!)).toBe(true); + + controller.kill("SIGTERM"); + expect(await waitFor(() => existsSync(signalRelayedFile))).toBe(true); + controller.kill("SIGTERM"); + await waitForExit(controller, 8000); + + expect(controller.exitCode).toBe(143); + expect(processGroupExists(nestedPgid!)).toBe(true); + expect(refOid(repoDir)).toBe(ownerOid); + const blocked = runLockShell(repoDir, [ + "set +e", + "try_acquire_pr_operation_lock 42", + "lock_status=$?", + "set -e", + 'printf "%s\\n" "$lock_status"', + ]); + expect(blocked.status).toBe(0); + expect(blocked.stdout.trim()).toBe("2"); + + killProcessGroup(nestedPgid!, "SIGKILL"); + expect(await waitFor(() => !processGroupExists(nestedPgid!))).toBe(true); + const recovered = runLockShell(repoDir, [ + `recover_pr_operation_lock 42 '${ownerOid}' --confirmed-no-running-tools`, + ]); + expect(recovered.status, `${recovered.stdout}\n${recovered.stderr}`).toBe(0); + expect(refExists(repoDir)).toBe(false); + } finally { + if (nestedPgid) await cleanupProcessGroup(nestedPgid); + await cleanupController(repoDir, controller); + } + }); + + it("has one dispatcher acquisition for composite prepare-run", () => { + const script = readFileSync(join(repoRoot, "scripts/pr"), "utf8"); + const runner = readFileSync(processGroupRunner, "utf8"); + expect(script.match(/acquire_pr_operation_lock/g)).toHaveLength(1); + expect(script).toContain('if [ "${1-}" = "gc" ] || is_locked_pr_command "${1-}"; then'); + expect(script).not.toMatch(/review-\*|prepare-\*|merge-\*/u); + expect(script).toContain( + "scripts/pr lock-recover --confirmed-no-running-tools", + ); + expect(script).toContain('recover_pr_operation_lock "$pr" "$owner_oid" "$confirmation"'); + expect(script).toContain('source "$script_parent_dir/pr-lib/operation-lock.sh"'); + expect(script).toContain( + 'pr-lib/process-group-runner.mjs" "$script_parent_dir/.." "$script_self" "$@"', + ); + expect(script).toContain('prepare_run "$pr"'); + expect(runner).toContain('process.platform === "win32"'); + expect(runner).toContain("requires a POSIX process group"); + expect(readFileSync(join(repoRoot, "scripts/pr-lib/prepare-core.sh"), "utf8")).not.toContain( + "acquire_pr_operation_lock", + ); + }); + + it("makes gc skip a PR while its operation lock is held", async () => { + const repoDir = createRepo(); + mkdirSync(join(repoDir, ".worktrees", "pr-42"), { recursive: true }); + const held = join(repoDir, "held"); + const holder = spawnHolder(repoDir, held); + try { + expect(await waitFor(() => existsSync(held))).toBe(true); + + const result = runLockShell(repoDir, [ + "gh() { if [ \"$1 $2\" = 'repo view' ]; then printf 'openclaw/openclaw\\n'; else printf 'MERGED\\n'; fi; }", + "gc_pr_worktrees false", + ]); + expect(result.status).toBe(0); + expect(result.stdout).toContain("has an active scripts/pr operation"); + expect(existsSync(join(repoDir, ".worktrees", "pr-42"))).toBe(true); + } finally { + await cleanupChildren(holder); + } + }); + + it("makes gc skip an unreadable lock and report exact recovery", () => { + const repoDir = createRepo(); + const worktreeDir = join(repoDir, ".worktrees", "pr-42"); + mkdirSync(worktreeDir, { recursive: true }); + const result = runLockShell(repoDir, [ + "bad_oid=$(printf 'not-a-lock\\n' | git hash-object -w --stdin)", + `git update-ref '${lockRef}' "$bad_oid"`, + "gh() { printf 'MERGED\\n'; }", + "gc_pr_worktrees false", + ]); + + expect(result.status, `${result.stdout}\n${result.stderr}`).toBe(0); + expect(result.stdout).toContain("operation lock is unreadable"); + expect(result.stderr).toContain( + `scripts/pr lock-recover 42 ${refOid(repoDir)} --confirmed-no-running-tools`, + ); + expect(existsSync(worktreeDir)).toBe(true); + }); + + it("does not report removal when gc cleanup leaves the worktree", () => { + const repoDir = createRepo(); + const worktreeDir = join(repoDir, ".worktrees", "pr-42"); + mkdirSync(worktreeDir, { recursive: true }); + const result = runLockShell(repoDir, [ + "gh() { printf 'MERGED\\n'; }", + "remove_worktree_if_present() { return 0; }", + "delete_local_branch_if_safe() { return 0; }", + "gc_pr_worktrees false", + ]); + + expect(result.status, `${result.stdout}\n${result.stderr}`).toBe(0); + expect(result.stdout).toContain("cleanup incomplete"); + expect(result.stdout).not.toContain("removed .worktrees/pr-42"); + expect(existsSync(worktreeDir)).toBe(true); + }); + + it("removes a registered relative worktree under a repo path with escapes", () => { + const repoDir = createRepo("repo with space \\ backslash"); + 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); + const located = runLockShell(repoDir, ["worktree_path_for_branch pr-42"]); + expect(located.status, `${located.stdout}\n${located.stderr}`).toBe(0); + expect(located.stdout.trim()).toBe(canonicalWorktreeDir); + + const result = runLockShell(repoDir, ["gh() { printf 'MERGED\\n'; }", "gc_pr_worktrees false"]); + + expect(result.status, `${result.stdout}\n${result.stderr}`).toBe(0); + expect(result.stdout).toContain("removed .worktrees/pr-42"); + expect(existsSync(worktreeDir)).toBe(false); + expect( + execFileSync("git", ["worktree", "list", "--porcelain"], { + cwd: repoDir, + encoding: "utf8", + }), + ).not.toContain(canonicalWorktreeDir); + expect( + spawnSync("git", ["show-ref", "--verify", "--quiet", "refs/heads/pr-42"], { + cwd: repoDir, + }).status, + ).toBe(1); + }); + + it("refuses a symlink alias to another registered worktree", () => { + const repoDir = createRepo(); + const worktreesDir = join(repoDir, ".worktrees"); + const targetDir = join(worktreesDir, "pr-99"); + const aliasDir = join(worktreesDir, "pr-42"); + mkdirSync(worktreesDir, { recursive: true }); + execFileSync("git", ["worktree", "add", "-q", "-b", "pr-99", targetDir], { + cwd: repoDir, + }); + const canonicalTargetDir = realpathSync(targetDir); + symlinkSync("pr-99", aliasDir, "dir"); + + const result = runLockShell(repoDir, ['remove_worktree_if_present ".worktrees/pr-42"']); + + expect(result.status, `${result.stdout}\n${result.stderr}`).toBe(0); + expect(result.stdout).toContain("refusing to remove non-canonical PR-worktree path"); + expect(existsSync(aliasDir)).toBe(true); + expect(existsSync(targetDir)).toBe(true); + expect( + execFileSync("git", ["worktree", "list", "--porcelain"], { + cwd: repoDir, + encoding: "utf8", + }), + ).toContain(canonicalTargetDir); + }); +}); diff --git a/test/scripts/test-projects.test.ts b/test/scripts/test-projects.test.ts index cea765986606..ede1ce7854f4 100644 --- a/test/scripts/test-projects.test.ts +++ b/test/scripts/test-projects.test.ts @@ -492,6 +492,13 @@ describe("scripts/test-projects changed-target routing", () => { } }); + it("keeps shared PR worktree helper edits on the full tooling owner suite", () => { + expect(resolveChangedTestTargetPlan(["scripts/pr-lib/worktree.sh"])).toEqual({ + mode: "targets", + targets: ["test/vitest/vitest.tooling.config.ts"], + }); + }); + it("routes nested e2e shell helpers through their sourced owner tests", () => { const expectedTargets = new Map([ [ @@ -1625,7 +1632,12 @@ describe("scripts/test-projects changed-target routing", () => { const expectedTargets = new Map([ ["scripts/committer", ["test/scripts/committer.test.ts"]], ["scripts/gh-read", ["test/scripts/gh-read.test.ts"]], - ["scripts/pr", ["test/scripts/pr-wrappers.test.ts"]], + [ + "scripts/pr", + ["test/scripts/pr-operation-lock.test.ts", "test/scripts/pr-wrappers.test.ts"], + ], + ["scripts/pr-lib/operation-lock.sh", ["test/scripts/pr-operation-lock.test.ts"]], + ["scripts/pr-lib/process-group-runner.mjs", ["test/scripts/pr-operation-lock.test.ts"]], ["scripts/pr-merge", ["test/scripts/pr-wrappers.test.ts"]], ["scripts/pr-prepare", ["test/scripts/pr-wrappers.test.ts"]], ["scripts/pr-review", ["test/scripts/pr-wrappers.test.ts"]],