diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index ee7f9d69c335..222da78383d1 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -854,6 +854,11 @@ jobs: echo "CMUX_VDISPLAY_LOCK_DIR=$CMUX_VDISPLAY_LOCK_DIR" >> "$GITHUB_ENV" echo "CMUX_VDISPLAY_LOCK_TOKEN=$CMUX_VDISPLAY_LOCK_TOKEN" >> "$GITHUB_ENV" + # Now that we hold the lock, reap any leaked display helper so a + # CGVirtualDisplay orphaned by a crashed/cancelled job cannot block + # this create on persistent self-hosted runners. + scripts/ci/virtual-display-lock.sh reap-strays || true + "$HELPER_PATH" \ --ready-path "$VDISPLAY_READY" \ --display-id-path "$VDISPLAY_ID_PATH" \ @@ -1399,6 +1404,11 @@ jobs: acquire_display_lock + # Reap any leaked display helper now that we hold the lock, so a + # CGVirtualDisplay orphaned by a crashed/cancelled job cannot block + # this create on persistent self-hosted runners. + scripts/ci/virtual-display-lock.sh reap-strays || true + # Launch display helper from shell (non-sandboxed). # Use --start-delay-ms instead of --start-path because the XCTest # runner is sandboxed and can't write to /tmp/ for the start signal. @@ -1563,6 +1573,11 @@ jobs: VDISPLAY_LOG="$RUNNER_TEMP/cmux-vdisplay-persistent.log" rm -f "$VDISPLAY_READY" "$VDISPLAY_ID_PATH" "$VDISPLAY_LOG" + # Now that we hold the lock, reap any leaked display helper so a + # CGVirtualDisplay orphaned by a crashed/cancelled job cannot block + # this create on persistent self-hosted runners. + scripts/ci/virtual-display-lock.sh reap-strays || true + "$HELPER_PATH" \ --modes "1920x1080" \ --ready-path "$VDISPLAY_READY" \ diff --git a/scripts/ci/virtual-display-lock.sh b/scripts/ci/virtual-display-lock.sh index 1b8f88a1ea0c..5a1d10ee88d8 100755 --- a/scripts/ci/virtual-display-lock.sh +++ b/scripts/ci/virtual-display-lock.sh @@ -14,7 +14,7 @@ LOCK_POLL_SECONDS="${CMUX_VDISPLAY_LOCK_POLL_SECONDS:-2}" usage() { cat >&2 <<'EOF' -usage: virtual-display-lock.sh acquire|set-owner |release +usage: virtual-display-lock.sh acquire|set-owner |reap-strays|release Coordinates host-global CGVirtualDisplay use between concurrent self-hosted macOS jobs. acquire prints CMUX_VDISPLAY_LOCK_DIR and @@ -204,6 +204,54 @@ release() { rm -rf "$LOCK_DIR" } +# List PIDs of running compiled create-virtual-display helper binaries, excluding +# the clang compile of the .m source and this script itself. Used to reap leaked +# helpers from crashed/cancelled jobs. +stray_helper_pids() { + # Match running compiled create-virtual-display helpers by full command line. + # Use `ps -o command=` (not `pgrep -fl`, whose output is the full argv on BSD + # but only the process name on Linux, which breaks the clang/.m exclusion) so + # the filter is identical on macOS runners and the Linux guard host. Exclude + # the clang compile of the .m source and this script itself. Tolerate no-match + # at every stage so an empty result is exit 0, not a pipefail that would abort + # reap_strays under `set -e`. + { ps -axww -o pid=,command= 2>/dev/null || true; } \ + | { grep 'create-virtual-display' || true; } \ + | { grep -v -e 'clang' -e 'create-virtual-display[.]m' -e 'virtual-display-lock' || true; } \ + | awk -v self="$$" '$1 != self { print $1 }' +} + +# Kill orphaned create-virtual-display helpers. Must be called while holding the +# lock: lock ownership makes CGVirtualDisplay access exclusive, so any live +# helper is a leak from a job that died without releasing. On persistent +# self-hosted runners (the minis) these orphans keep their CGVirtualDisplay +# alive and block every subsequent create, because only one CI virtual display +# identity is allowed at a time. Warp VMs never hit this since each job gets a +# fresh VM. +reap_strays() { + require_token_match || exit 0 + local pids + pids="$(stray_helper_pids)" + if [ -z "$pids" ]; then + echo "No stray virtual-display helpers to reap" >&2 + return 0 + fi + # shellcheck disable=SC2086 + echo "Reaping stray virtual-display helpers: $(echo $pids | tr '\n' ' ')" >&2 + # shellcheck disable=SC2086 + kill $pids 2>/dev/null || true + local _ + for _ in $(seq 1 50); do + pids="$(stray_helper_pids)" + [ -n "$pids" ] || return 0 + sleep 0.1 + done + # shellcheck disable=SC2086 + echo "Force-killing remaining virtual-display helpers: $(echo $pids | tr '\n' ' ')" >&2 + # shellcheck disable=SC2086 + kill -9 $pids 2>/dev/null || true +} + case "$COMMAND" in acquire) acquire @@ -211,6 +259,9 @@ case "$COMMAND" in set-owner) set_owner "${1:-}" ;; + reap-strays) + reap_strays + ;; release) release ;; diff --git a/tests/test_ci_virtual_display_lock.sh b/tests/test_ci_virtual_display_lock.sh index 4f77e4ccdc4b..0217459fe75b 100755 --- a/tests/test_ci_virtual_display_lock.sh +++ b/tests/test_ci_virtual_display_lock.sh @@ -130,4 +130,52 @@ if [ -d "$CMUX_VDISPLAY_LOCK_DIR" ]; then exit 1 fi -echo "PASS: virtual display lock serializes acquisition, preserves live-owner locks, reclaims ownerless and dead-owner locks, and releases only matching tokens" +# reap-strays kills leaked display helpers while the lock is held, leaves the +# clang compile of the source alone, and refuses to act without the lock token. +REAP_LOCK_DIR="$TMP_DIR/cmux-test-reap.lock" +REAP_ENV="$( + RUNNER_TEMP="$TMP_DIR" \ + CMUX_VDISPLAY_LOCK_DIR="$REAP_LOCK_DIR" \ + "$SCRIPT" acquire +)" +eval "$REAP_ENV" + +( exec -a "$TMP_DIR/create-virtual-display --ready-path /tmp/x" sleep 30 ) & +STRAY_PID=$! +( exec -a "clang -framework CoreGraphics -o $TMP_DIR/create-virtual-display scripts/create-virtual-display.m" sleep 30 ) & +COMPILE_PID=$! +sleep 0.3 + +# Without the token, reap-strays must refuse (non-zero) and kill nothing. +if RUNNER_TEMP="$TMP_DIR" CMUX_VDISPLAY_LOCK_DIR="$CMUX_VDISPLAY_LOCK_DIR" \ + "$SCRIPT" reap-strays >/dev/null 2>&1; then + echo "FAIL: reap-strays succeeded without the lock token" >&2 + exit 1 +fi +if ! kill -0 "$STRAY_PID" 2>/dev/null; then + echo "FAIL: reap-strays killed a helper without the lock token" >&2 + exit 1 +fi + +RUNNER_TEMP="$TMP_DIR" \ +CMUX_VDISPLAY_LOCK_DIR="$CMUX_VDISPLAY_LOCK_DIR" \ +CMUX_VDISPLAY_LOCK_TOKEN="$CMUX_VDISPLAY_LOCK_TOKEN" \ + "$SCRIPT" reap-strays >/dev/null 2>&1 +sleep 0.3 +if kill -0 "$STRAY_PID" 2>/dev/null; then + echo "FAIL: reap-strays did not kill the leaked display helper" >&2 + kill "$STRAY_PID" "$COMPILE_PID" 2>/dev/null || true + exit 1 +fi +if ! kill -0 "$COMPILE_PID" 2>/dev/null; then + echo "FAIL: reap-strays killed the clang compile of the helper source" >&2 + kill "$COMPILE_PID" 2>/dev/null || true + exit 1 +fi +kill "$COMPILE_PID" 2>/dev/null || true +RUNNER_TEMP="$TMP_DIR" \ +CMUX_VDISPLAY_LOCK_DIR="$CMUX_VDISPLAY_LOCK_DIR" \ +CMUX_VDISPLAY_LOCK_TOKEN="$CMUX_VDISPLAY_LOCK_TOKEN" \ + "$SCRIPT" release + +echo "PASS: virtual display lock serializes acquisition, preserves live-owner locks, reclaims ownerless and dead-owner locks, releases only matching tokens, and reap-strays kills leaked helpers (token-gated, compile-safe)"