Skip to content

macOSVMBusy's session.Run has no deadline — the busy probe can still wedge the scheduler event loop #199

Description

@luthermonson

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:

err = session.Run("pgrep -x Runner.Worker >/dev/null")   // pkg/scheduler/busy_macosvm.go:90

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 after 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).
  • 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.

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions