Skip to content

fix(scheduler): bound runner teardown and retry failed env creates - #190

Merged
luthermonson merged 2 commits into
mainfrom
fix/windows-lifecycle
Sep 1, 2026
Merged

luthermonson merged 2 commits into
mainfrom
fix/windows-lifecycle

Conversation

@luthermonson

Copy link
Copy Markdown
Contributor

Fixes the two remaining defects behind the 2026-09-01 Windows-node wedge, where one dead containerd shim produced two symptoms and blocked two consecutive upgrade drains.

Incident

A runner env's teardown logged destroying runner environment and then nothing, forever — Runtime.Destroy was blocked on <-exitCh with no select and a context.Background() from every call site; when the shim is dead the exit event never arrives. That hung destroy pinned a MaxConcurrent slot for 1h+ (the wait-goroutine's defer <-s.sem runs after Destroy) while active_jobs had already been decremented — a node that looks idle-ish but can't admit work. Five seconds into the hang, the next env create failed with failed to create shim task: ttrpc: closed, and the create-failure path — unlike the claim-failure branch 15 lines above it — never calls enqueueRetryIfEligible, so the job sat queued for 1h43m (the only recovery is the 30-min reconcile loop, and a drain suppresses every re-dispatch route).

Fixes

Bounded Destroy with an abandon-to-reaper contract (pkg/runtime). Destroy now derives WithTimeout (new [runtime] destroy_timeout, default 5m; zero-value config fails safe to the default). Teardown steps run on their own goroutine; on expiry Destroy logs and returns, abandoning whatever remains — which is exactly what the v0.2.8 dead-container reaper (ReapDeadContainers) and startup CleanOrphans exist to reclaim. The kill-wait itself is separately bounded (30s, waitTaskExit — mirroring the select Runtime.Wait already used 20 lines below) so a missing exit event can't eat the whole budget and starve later steps that would still succeed. A late-finishing teardown still logs runner environment destroyed for the operator trail.

Slot release decoupled from teardown (pkg/scheduler). Both wait-goroutines (local sem, linuxSem) release the concurrency slot immediately after the job is untracked, before Destroy — teardown can no longer delay admission at all. Safe against create/teardown races: every env is keyed by its unique claim.RunnerName (container ID, runner dir, netns/HCN endpoint, dind socket all derive from it). This also unblocks handleCompleted's stranded defer sweepOrphanRunners().

Create failures now retry (pkg/scheduler). In place: 2 extra attempts at 5s/15s for transiently-classified errors (ttrpc: closed → errUnknownRetryable, pinned by test) — the slot is held and the JIT claim minted, so immediate retry is far cheaper than release-and-requeue; short-circuits on drain (checked before and after each sleep) and ctx cancellation. On exhaustion: the same cleanup as before, then enqueueRetryIfEligible onto the existing backoff queue — mirroring the claim branch — in both the local and Linux-dispatch paths.

Windows busy-probe now honors its timeout (pkg/runnerbusy). The hcsshim calls discarded their context.Context, so busyProbeTimeout was not enforced on Windows — the exact wedged-shim-blocks-the-sweep case PR #151 introduced the timeout for. Calls moved to a goroutine with a select; a wedged probe yields Unknown (fail-safe veto).

Deliberately skipped: narrowing network_windows.go's w.mu (shared setup/teardown mutex) — the mutex keeps IPAM allocation and HCN endpoint creation atomic, and per-endpoint locking is a subsystem rewrite; with Destroy bounded, a wedged HNS teardown can delay (not deadlock) the next setup.

Composes with #151: no changes to busy.go veto/nomination; the reap path's destroy gets the same bound via Runtime.Destroy itself.

Testing

  • destroy_test.go: an exitCh that never delivers cannot block past the bound; ctx/timer paths covered
  • retry_create_test.go: in-place ladder attempts/backoff, drain short-circuit (before and mid-sleep), non-retryable short-circuit, ttrpc: closed classification pinned
  • runnerbusy: wedged probe returns within ctx deadline
  • go build/go vet clean; go test green across scheduler/runtime/config/runnerbusy (-race not runnable on this box — no cgo; a Linux race run would be welcome belt-and-braces)

Two production incidents on a Windows node (2026-09-01), one root cause:
a dead containerd shim.

Incident 1: after job completion, Destroy blocked forever on a bare
<-exitCh — the killed task's exit event never arrives from a dead shim,
and every caller passes context.Background(). The hung destroy sat
behind the wait-goroutine's deferred slot release, pinning a
MaxConcurrent slot for 1h+ and wedging two consecutive upgrade drains.

Incident 2: five seconds into that hang, the next env create failed with
"failed to create shim task: ttrpc: closed" and the job waited 1h43m,
because the create-failure branch — unlike the claim-failure branch 15
lines up — never enqueued a retry, and GitHub does not re-deliver
workflow_job webhooks.

Teardown is now bounded end to end:

- Runtime.Destroy derives a WithTimeout ctx (new runtime.destroy_timeout
  knob; default 5m, non-positive falls back to the default — the
  zero-value Config fails safe, there is no unbounded configuration) and
  runs the teardown steps on their own goroutine. On expiry Destroy
  returns an error and ABANDONS the remaining teardown: steps that take
  no ctx (Dind.Stop, HCS/HNS internals) may stay parked, and what they
  leave behind is reclaimed by the periodic dead-container reaper
  (ReapDeadContainers) and the startup CleanOrphans sweep.
- The kill-wait itself is separately bounded (30s) so a missing exit
  event cannot starve the teardown steps that still can succeed.
- The wait-goroutines (local sem and linuxSem twin) release their
  concurrency slot as soon as the job is untracked, BEFORE destroy, so
  even a bounded teardown can never delay the next job's admission.
  Safe because every env is keyed by its unique claim.RunnerName —
  container ID, runner dir, netns/HCN endpoint all derive from it — so
  a freshly admitted job cannot collide with an in-flight teardown.

Failed env creates now retry:

- Runtime.Create gets a small in-place ladder (5s, 15s) for transient
  errors while the slot and JIT claim are still held; skipped for
  non-retryable errors, during drain, and on ctx cancellation.
- When the ladder runs out, the create-failure branches (local and
  Linux dispatch) enqueue on the existing retry queue, mirroring the
  claim-failure branch — "ttrpc: closed" classifies retryable.

Hardening: the Windows busy probe (runnerbusy.ContainerBusy) now runs
its uncancellable hcsshim calls on a goroutine and honors ctx, so
busyProbeTimeout is actually enforced on Windows and a dead shim/GCS
cannot wedge the sweep behind it.

Composes with #151: only post-completion/exit destroys change behavior;
the busy-veto and sweep nomination logic are untouched, and the reap
path's destroy picks up the same bound via Runtime.Destroy.
Adversarial review of the parent commit found the bound leaked at both
ends; all fixed here:

- The OnTaskDestroy hook ran synchronously BEFORE the bounded goroutine.
  On Windows the hook closes an HCS handle — exactly the class of call
  that wedges on a dead shim — so a wedged hook blocked Destroy before
  any timeout machinery existed. The hook now runs inside the teardown
  goroutine.
- destroySteps shared Destroy's caller-bounded ctx, whose deferred
  cancel fired the moment Destroy returned on timeout — so the
  'continues in the background' contract was false: every remaining
  ctx-aware step failed instantly with context.Canceled once a wedge
  cleared. The teardown goroutine now gets its own detached ctx
  (Background + namespace, budget 2x DestroyTimeout) so an abandoned
  teardown can actually finish late.
- Task.Delete in destroySteps lacked WithProcessKill: after an abandoned
  kill-wait, a task whose process died but whose exit event was lost
  failed the delete with FailedPrecondition, the container delete failed
  too, and the periodic reaper could never converge (it fails safe on a
  Running-reading task) — the snapshot survived until restart.
- ResolvedDestroyTimeout now floors at 1s: TOML decodes a bare integer
  destroy_timeout = 300 as 300 NANOSECONDS without error, which would
  have made every teardown 'time out' instantly and abandon its cleanup.
- destroyWaitCompleted prefers the done channel when both it and the
  deadline are ready, so a teardown finishing at the buzzer is not
  mis-logged as abandoned; factored out and unit-tested (100-iteration
  anti-coin-flip test).
- The abandonment contract comment now states honestly that leaked HNS
  endpoints + pool IPs are reclaimed by NOTHING (restart re-adopts and
  re-reserves them; pool headroom absorbs the leak) — the known residual,
  accepted over holding the scheduler hostage.
@luthermonson

Copy link
Copy Markdown
Contributor Author

Adversarial review completed (agent-driven; concurrency, locking, and #151-composition traced end-to-end). The scheduler core came back clean — slot-release single-caller proof, lock balance, ID-keyed teardown safety, retry ctx/drain semantics, and the reap-path interaction with #151 all CONFIRMED correct. The destroy bound itself had holes; all fixed in ee70361:

  • Hook outside the bound (MAJOR): OnTaskDestroy ran synchronously before the bounded goroutine — and on Windows it's an HCS Close(), exactly the wedge class this PR defends against. Now runs inside the teardown goroutine.
  • Cancel-on-return killed the background teardown (MAJOR): destroySteps shared the caller-bounded ctx, so the moment Destroy returned on timeout, the deferred cancel made every remaining ctx-aware step fail instantly with context.Canceled — "continues in the background" was false. The goroutine now gets a detached ctx (2× DestroyTimeout) so an abandoned teardown can actually finish late.
  • Reaper couldn't converge on the abandoned case (MINOR): Task.Delete in destroySteps lacked WithProcessKill; a dead-process/lost-exit-event task failed the delete with FailedPrecondition and the pinned snapshot survived until restart. Now matches teardownContainer.
  • TOML footgun (MINOR, verified live): destroy_timeout = 300 decodes as 300 nanoseconds with no error — instant abandon on every teardown. ResolvedDestroyTimeout now floors sub-second values to the default.
  • Deadline race (NIT): a teardown finishing at the buzzer could be mis-logged as abandoned; the wait now prefers done when both are ready (destroyWaitCompleted, unit-tested with a 100-iteration anti-coin-flip case).
  • The abandonment contract comment now states honestly that leaked HNS endpoints/pool IPs are reclaimed by nothing (restart re-adopts + re-reserves; pool headroom absorbs it) — known residual, accepted over holding the scheduler hostage. An endpoint reaper is possible follow-up work.

Acknowledged, not addressed here: no integration-level test pins slot-release-before-destroy or the new enqueue call sites (the pure-function ladders are pinned; a scheduler-harness test with a blocking Destroy is the right follow-up), and teardownContainer's own <-exitCh retains the pre-existing unbounded receive (startup-only path; worth a follow-up sweep). -race still not runnable on this Windows box — CI on Linux should run it.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant