Skip to content

ci: reap leaked virtual-display helpers before create (unblock display jobs on fleet) #6407

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
lawrencecchen merged 2 commits into from
Jun 19, 2026
Merged

ci: reap leaked virtual-display helpers before create (unblock display jobs on fleet) #6407

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
1cf1911
ci: reap leaked virtual-display helpers before creating one
lawrencecchen Jun 18, 2026
97a94ba
ci: identify stray display helpers with ps, not pgrep -fl
lawrencecchen Jun 18, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 15 additions & 0 deletions .github/workflows/ci.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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" \
Expand Down Expand Up @@ -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.
Expand Down Expand Up @@ -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" \
Expand Down
53 changes: 52 additions & 1 deletion scripts/ci/virtual-display-lock.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 <pid>|release
usage: virtual-display-lock.sh acquire|set-owner <pid>|reap-strays|release

Coordinates host-global CGVirtualDisplay use between concurrent self-hosted
macOS jobs. acquire prints CMUX_VDISPLAY_LOCK_DIR and
Expand Down Expand Up @@ -204,13 +204,64 @@ 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
Comment on lines +244 to +248

@greptile-apps greptile-apps Bot Jun 18, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Fixed-sleep poll loop for process teardown synchronization

reap_strays uses a sleep 0.1 × 50 wall-clock poll to wait for SIGTERM to take effect on non-child processes, which falls under the cmux-runtime-no-hacky-sleeps rule for build/runtime scripts. In bash there is no POSIX-compatible alternative for waiting on an arbitrary non-child process (you cannot wait on a PID you didn't fork), so this pattern is the standard workaround — but consider wrapping it in a named helper (e.g., wait_for_pids_exit) with a clearly documented timeout contract, or using wait -n / lsof polling if a finer-grained cancellation hook is ever needed.

Rule Used: Flag fixed sleeps, delayed dispatch, timers, polli... (source)

Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

Comment on lines +244 to +248

@coderabbitai coderabbitai Bot Jun 18, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Replace wall-clock polling in reap_strays shutdown path.

Line 239-Line 243 uses sleep 0.1 polling as synchronization in production shell runtime. That violates the no-hacky-sleeps policy and can still be timing-fragile under load.

As per coding guidelines, “Do not use fixed delays (sleep, ... polling loops, or fixed backoff) as synchronization mechanisms in production ... shell code.”

Suggested patch 
@@
-  local _
-  for _ in $(seq 1 50); do
-    pids="$(stray_helper_pids)"
-    [ -n "$pids" ] || return 0
-    sleep 0.1
-  done
+  pids="$(stray_helper_pids)"
+  [ -n "$pids" ] || return 0
🤖 Prompt for AI Agents 
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@scripts/ci/virtual-display-lock.sh` around lines 239 - 243, The reap_strays
function uses a polling loop with sleep 0.1 as a synchronization mechanism,
which violates the no-hacky-sleeps policy. Replace the for loop (lines 239-243)
that calls stray_helper_pids and sleeps with a proper synchronization mechanism.
Instead of polling with fixed delays, use event-based or signal-based
synchronization such as waiting for the processes returned by stray_helper_pids
to actually terminate, or checking process existence directly without the sleep
loop. This ensures the code waits for actual completion rather than relying on
timing assumptions.

Source: Coding guidelines

# 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
}
Comment on lines +231 to +253

@greptile-apps greptile-apps Bot Jun 18, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 reap_strays does not confirm stray PIDs are dead after kill -9

After the SIGKILL path, the function returns immediately with no check on whether the processes actually exited. In the overwhelmingly common case SIGKILL works instantly, but if a process is in an uninterruptible D-state (e.g., stuck on an NFS mount or kernel I/O), kill -9 will be silently accepted yet the process lives on. The new display helper would then be launched while the stray is still alive, reproducing exactly the leak-blocks-create scenario this PR is fixing. A brief post-SIGKILL stray_helper_pids check (with a warning but without aborting) would make the diagnosis clearer without changing the exit path.


case "$COMMAND" in
acquire)
acquire
;;
set-owner)
set_owner "${1:-}"
;;
reap-strays)
reap_strays
;;
release)
release
;;
Expand Down
50 changes: 49 additions & 1 deletion tests/test_ci_virtual_display_lock.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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

@coderabbitai coderabbitai Bot Jun 18, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Remove fixed sleeps from reap-strays test assertions.

Line 147 and Line 164 rely on fixed wall-clock delays before assertions (sleep 0.3). This makes the test timing-dependent and flaky.

As per coding guidelines, “Tests must not introduce fixed sleep ... used to wait for async readiness before an assertion,” and “Deadline-bounded polls of a real predicate ... are allowed.”

Suggested patch 
@@
-sleep 0.3
+for _ in $(seq 1 30); do
+  if kill -0 "$STRAY_PID" 2>/dev/null && kill -0 "$COMPILE_PID" 2>/dev/null; then
+    break
+  fi
+  sleep 0.1
+done
@@
-sleep 0.3
+for _ in $(seq 1 30); do
+  if ! kill -0 "$STRAY_PID" 2>/dev/null; then
+    break
+  fi
+  sleep 0.1
+done

Also applies to: 164-164

🤖 Prompt for AI Agents 
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@tests/test_ci_virtual_display_lock.sh` at line 147, Remove the fixed sleep
delays (sleep 0.3) from the reap-strays test assertions at lines 147 and 164 in
test_ci_virtual_display_lock.sh. Replace each fixed sleep with a
deadline-bounded polling loop that repeatedly checks a real predicate condition
until it becomes true or a timeout is reached, rather than relying on wall-clock
delays. This will make the test timing-independent and less flaky while
maintaining proper assertion synchronization.

Source: Coding guidelines


# 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)"
Loading