You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
Split out of the #197 review. Pre-existing, not a regression — but it means #197's headline property ("a wedged macOS VM can no longer block the event loop") is not yet fully true.
The hazard
sweepOrphanRunners runs synchronously from the cleanupTicker arm of Run's select (scheduler.go:913). Before it reaches the now-bounded stopVMBounded, it calls vetoBusyNominations → probeRunnerBusy → macOSVMBusy, which ends in:
probeRunnerBusy does wrap ctx in context.WithTimeout(ctx, busyProbeTimeout) (busy.go:277), but macOSVMBusy spends that deadline only on ssh.ClientConfig.Timeout, which bounds dial + handshake. After the handshake there is no read deadline and session.Run does not observe ctx.
This is the same primitive #197 itself indicts at scheduler.go:1667-1670: "those golang.org/x/crypto/ssh session calls carry no deadline and do not observe ctx — a guest command that never returns wedges the wait forever." It's also the mechanism behind #178's original RCA.
Failure scenario
A macOS guest that accepts SSH but whose pgrep never returns — i.e. the #196 guest, which is documented to do exactly this — gets nominated as an orphan. The sweep parks in session.Run, and the scheduler's only event loop stops handling queued/in_progress/completed for every platform, with no drain on SIGTERM. Strictly worse than the slot leak #197 fixed: the whole node goes dark, not just macOS.
Why CI won't catch it
TestSweepOrphanRunners_WedgedMacOSVMCannotBlockTheEventLoop sets s.busyProbe = idleProbe (slots_test.go:656), so the test stubs out precisely this hazard while asserting the event loop is safe.
Fix
Run session.Run on a goroutine and select against ctx, closing the client on timeout so the session unblocks — the same shape #197 used for stopVMAsync/awaitUnwind. A test with an SSH server that accepts and then never responds would pin it (and the sweep test should stop stubbing the probe, or gain a sibling that doesn't).
Related follow-ups from the same review
The abandoned-VM cordon sits afterhandleQueued's attempt counter (scheduler.go:1063), so a cordoned job burns its zombie budget — ~50 min to permanently undispatchable, and it stays that way if a slow Vz stop later lifts the cordon. The drain/cordon does not stop webhook-driven provisioning — node never quiesces #154 cordon was deliberately placed above that counter for exactly this reason (scheduler.go:1041-1046).
Test gaps: reverting artifacts.ArtifactsDir(dataDir, vmID) back to the bare job id passes the whole suite (Config.Artifacts is nil in newSlotTestScheduler), and neither cordon check is individually pinned — deleting either one still passes.
No Prometheus metric for held_slots / abandoned_macos_vms; the fleet alerts off pkg/metrics, and those are exactly the two numbers worth alerting on.
Split out of the #197 review. Pre-existing, not a regression — but it means #197's headline property ("a wedged macOS VM can no longer block the event loop") is not yet fully true.
The hazard
sweepOrphanRunnersruns synchronously from thecleanupTickerarm ofRun's select (scheduler.go:913). Before it reaches the now-boundedstopVMBounded, it callsvetoBusyNominations→probeRunnerBusy→macOSVMBusy, which ends in:probeRunnerBusydoes wrap ctx incontext.WithTimeout(ctx, busyProbeTimeout)(busy.go:277), butmacOSVMBusyspends that deadline only onssh.ClientConfig.Timeout, which bounds dial + handshake. After the handshake there is no read deadline andsession.Rundoes not observe ctx.This is the same primitive #197 itself indicts at
scheduler.go:1667-1670: "those golang.org/x/crypto/ssh session calls carry no deadline and do not observe ctx — a guest command that never returns wedges the wait forever." It's also the mechanism behind #178's original RCA.Failure scenario
A macOS guest that accepts SSH but whose
pgrepnever returns — i.e. the #196 guest, which is documented to do exactly this — gets nominated as an orphan. The sweep parks insession.Run, and the scheduler's only event loop stops handlingqueued/in_progress/completedfor every platform, with no drain on SIGTERM. Strictly worse than the slot leak #197 fixed: the whole node goes dark, not just macOS.Why CI won't catch it
TestSweepOrphanRunners_WedgedMacOSVMCannotBlockTheEventLoopsetss.busyProbe = idleProbe(slots_test.go:656), so the test stubs out precisely this hazard while asserting the event loop is safe.Fix
Run
session.Runon a goroutine and select against ctx, closing the client on timeout so the session unblocks — the same shape #197 used forstopVMAsync/awaitUnwind. A test with an SSH server that accepts and then never responds would pin it (and the sweep test should stop stubbing the probe, or gain a sibling that doesn't).Related follow-ups from the same review
handleQueued's attempt counter (scheduler.go:1063), so a cordoned job burns its zombie budget — ~50 min to permanently undispatchable, and it stays that way if a slow Vz stop later lifts the cordon. The drain/cordon does not stop webhook-driven provisioning — node never quiesces #154 cordon was deliberately placed above that counter for exactly this reason (scheduler.go:1041-1046).artifacts.ArtifactsDir(dataDir, vmID)back to the bare job id passes the whole suite (Config.Artifactsis nil innewSlotTestScheduler), and neither cordon check is individually pinned — deleting either one still passes.held_slots/abandoned_macos_vms; the fleet alerts offpkg/metrics, and those are exactly the two numbers worth alerting on.