Repository navigation
ci: reap leaked virtual-display helpers before create (unblock display jobs on fleet) - #6407
Conversation
On persistent self-hosted runners a CGVirtualDisplay helper orphaned by a crashed or cancelled job keeps its display alive and blocks every later create, because only one CI virtual display identity can exist at a time. Warp VMs never hit this since each job gets a fresh VM, but the fleet Macs do, which is why the two display jobs couldn't move off Warp. Add a `reap-strays` subcommand to virtual-display-lock.sh that kills orphaned create-virtual-display helpers. It is token-gated (only acts while the caller holds the host-global display lock, so any live helper is necessarily a leak) and excludes the clang compile of the source so a concurrent job's build is never killed. Call it in all three display setup steps (tests-build-and-lag, ui-regressions, persistent) right after acquiring the lock, before launching the new helper. No behavior change on Warp (no strays there); unblocks running the display jobs on the fleet minis. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds a Changesreap-strays subcommand and CI integration
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error, 1 warning)
✅ Passed checks (20 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Greptile SummaryAdds a
Confidence Score: 5/5Safe to merge; the reaping is scoped tightly by lock ownership and process-name filtering, with no behavioral change on Warp VMs All three reap-strays call sites in ci.yml correctly export CMUX_VDISPLAY_LOCK_DIR and CMUX_VDISPLAY_LOCK_TOKEN before invoking the subprocess, so require_token_match will always have the token available. The stray_helper_pids filter is sound for macOS and Linux. Two existing review threads already track the sleep-poll pattern and post-SIGKILL verification; no new blocking concerns were found. No files require special attention Important Files Changed
Reviews (2): Last reviewed commit: "ci: identify stray display helpers with ..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| for _ in $(seq 1 50); do | ||
| pids="$(stray_helper_pids)" | ||
| [ -n "$pids" ] || return 0 | ||
| sleep 0.1 | ||
| done |
There was a problem hiding this comment.
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!
| 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 | ||
| } |
There was a problem hiding this comment.
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.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with 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.
Inline comments:
In `@scripts/ci/virtual-display-lock.sh`:
- Around line 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.
In `@tests/test_ci_virtual_display_lock.sh`:
- 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.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 4d55a778-dd87-4309-af8a-59f28f11a17b
📒 Files selected for processing (3)
.github/workflows/ci.ymlscripts/ci/virtual-display-lock.shtests/test_ci_virtual_display_lock.sh
| for _ in $(seq 1 50); do | ||
| pids="$(stray_helper_pids)" | ||
| [ -n "$pids" ] || return 0 | ||
| sleep 0.1 | ||
| done |
There was a problem hiding this comment.
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
| 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 |
There was a problem hiding this comment.
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
+doneAlso 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
pgrep -fl prints the full argv on BSD/macOS but only the process name on Linux, where the workflow-guard host runs the lock test. That made the clang/.m exclusion silently no-op on Linux, so reap-strays killed the compile fake and the guard test failed. Use `ps -axww -o pid=,command=`, which yields the full command line identically on both platforms. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This branch was successfully deployed
1 participant
Why
The two display-requiring jobs (
tests-build-and-lag,ui-regressions) route to Warp because creating a CGVirtualDisplay appeared impossible on the self-hosted fleet. Root-caused on the macОS 26 minis: cvd creates reliably from the runner's own gui session (verified 3/3, display ids 15/17/19, no sudo). The real blocker is leaked helpers: acreate-virtual-displayprocess orphaned by a crashed/cancelled job keeps its display alive, and because only one CI virtual display identity can exist at a time, every subsequent create fails. Persistent runners accumulate these leaks; Warp VMs don't (fresh VM per job).What
reap-straystoscripts/ci/virtual-display-lock.sh: kills orphanedcreate-virtual-displayhelpers. Token-gated (only runs while holding the host-global display lock, so any live helper is provably a leak) and compile-safe (excludes theclang … create-virtual-display.mbuild so a concurrent job's compile is never killed).No behavior change on Warp (no strays there). This unblocks pointing
MACOS_RUNNER_DISPLAYat the fleet minis to drop Warp.Tests
Extended
tests/test_ci_virtual_display_lock.sh: reap-strays kills a leaked helper, preserves the clang compile, and refuses without the lock token.test_ci_self_hosted_guard.shstill passes.🤖 Generated with Claude Code
Need help on this PR? Tag
/codesmithwith what you need. Autofix is disabled.Note
Low Risk
CI-only lock script and workflow hooks; no app runtime behavior, with tests covering token gating and compile exclusion.
Overview
Adds
reap-straystoscripts/ci/virtual-display-lock.shso jobs that hold the host-global display lock can kill orphanedcreate-virtual-displayprocesses left behind when a prior job crashes or is cancelled. On persistent self-hosted macOS runners, those leaks keep a CGVirtualDisplay alive and block the next create; Warp VMs are unaffected.Reaping is token-gated (requires the lock token) and compile-safe (finds helpers via
psfull command lines, excludingclang … create-virtual-display.mand the lock script). It SIGTERMs strays, waits briefly, then SIGKILLs any survivors..github/workflows/ci.ymlinvokesreap-straysimmediately afteracquirein all three virtual-display setup paths (tests-build-and-lag, display-churn UI regression, persistent display for browser-find).tests/test_ci_virtual_display_lock.shnow checks that reaping refuses without a token, kills a simulated stray helper, and does not kill a fake clang compile.Reviewed by Cursor Bugbot for commit 97a94ba. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by cubic
Reaps leaked virtual-display helpers before creating a new CGVirtualDisplay to prevent stuck jobs on persistent macOS runners, unblocking display jobs on the fleet. No-op on Warp VMs.
reap-straystoscripts/ci/virtual-display-lock.sh(token-gated, compile-safe) to kill orphanedcreate-virtual-displayhelpers, usingps -axww -o pid=,command=for consistent macOS/Linux detection.reap-straysright after acquiring the display lock in all three display setup steps in.github/workflows/ci.yml.tests/test_ci_virtual_display_lock.shto verify reaping behavior, token checks, compile exclusion, and theps-based detection on the Linux guard host.Written for commit 97a94ba. Summary will update on new commits.
Summary by CodeRabbit
Chores
Tests