From fa8993f5920632dd6e4f87864b132a8e7138df3c Mon Sep 17 00:00:00 2001 From: Lev Brouk Date: Tue, 19 May 2026 10:24:32 -0700 Subject: [PATCH 1/2] fix(orchestrator): stop Checks health-loop leaking on a start/stop race MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Checks.Start is launched as `go sbx.Checks.Start(execCtx)`, and execCtx is derived through context.WithoutCancel so it never auto-cancels — the logHealth ticker loop is only ever ended by Checks.Stop cancelling the context Start installs. Start runs in a freshly-spawned goroutine, so it can be scheduled only after the sandbox's teardown (Pause -> Close -> doStop) has already run its Checks.Stop calls — common for short-lived build sandboxes and more likely under CPU contention. Every Stop that runs before Start's goroutine sees cancelCtx still nil and does nothing; Start then installs a fresh, never-cancelled context and enters the 20s health-check loop with nothing left to stop it. The loop and the per-tick health-check HTTP dials it spawns then leak for the process lifetime. Record that Stop ran via a `stopped` flag and check it in Start under the same mutex: if Stop already happened, Start returns without starting the loop. Correct for all orderings (Start first, Stop first, or mutex contention) regardless of how many times Stop is called. --- packages/orchestrator/pkg/sandbox/checks.go | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/packages/orchestrator/pkg/sandbox/checks.go b/packages/orchestrator/pkg/sandbox/checks.go index a9480185be..78ab3aeedc 100644 --- a/packages/orchestrator/pkg/sandbox/checks.go +++ b/packages/orchestrator/pkg/sandbox/checks.go @@ -28,6 +28,11 @@ type Checks struct { mu sync.Mutex cancelCtx context.CancelCauseFunc + // stopped records that Stop ran. Start is launched in its own goroutine, so + // a short-lived sandbox can Stop before Start is scheduled — at which point + // cancelCtx is still nil and Stop has nothing to cancel. Without this flag + // Start would then enter an uncancellable health-check loop that leaks. + stopped bool healthy atomic.Bool @@ -52,6 +57,13 @@ func NewChecks(sandbox *Sandbox, useClickhouseMetrics bool) *Checks { func (c *Checks) Start(ctx context.Context) { c.mu.Lock() + if c.stopped { + // Stop already ran before this goroutine was scheduled; don't start the + // health-check loop, it would never be cancelled. + c.mu.Unlock() + + return + } ctx, c.cancelCtx = context.WithCancelCause(ctx) c.mu.Unlock() @@ -62,6 +74,8 @@ func (c *Checks) Stop() { c.mu.Lock() defer c.mu.Unlock() + c.stopped = true + if c.cancelCtx != nil { c.cancelCtx(ErrChecksStopped) } From 41c038b58d96232cd5c5f23c2a6c4fa1d0d35d37 Mon Sep 17 00:00:00 2001 From: Lev Brouk Date: Tue, 19 May 2026 11:45:31 -0700 Subject: [PATCH 2/2] test(orchestrator): cover the Checks start/stop race Regression test for the previous commit: when Checks.Stop runs before the go-launched Checks.Start goroutine is scheduled, Start must observe the stop and return without entering logHealth. Verified to fail (Start enters the uncancellable health loop) when the stopped-flag guard is removed. --- .../orchestrator/pkg/sandbox/checks_test.go | 50 +++++++++++++++++++ 1 file changed, 50 insertions(+) create mode 100644 packages/orchestrator/pkg/sandbox/checks_test.go diff --git a/packages/orchestrator/pkg/sandbox/checks_test.go b/packages/orchestrator/pkg/sandbox/checks_test.go new file mode 100644 index 0000000000..e743687528 --- /dev/null +++ b/packages/orchestrator/pkg/sandbox/checks_test.go @@ -0,0 +1,50 @@ +//go:build linux + +package sandbox + +import ( + "context" + "testing" + "time" +) + +// TestChecks_StopBeforeStart_DoesNotStartHealthLoop is a regression test for +// the Checks start/stop race. +// +// Checks.Start is launched in its own goroutine (go sbx.Checks.Start(execCtx)), +// so the sandbox teardown path can run Checks.Stop before that goroutine is +// scheduled. execCtx is derived via context.WithoutCancel and never +// auto-cancels, so the logHealth ticker loop is only ever ended by Stop +// cancelling the context Start installs. If Start enters logHealth after Stop +// already ran, the loop — and the per-tick health-check HTTP dials it spawns — +// leak for the process lifetime. +// +// Start must observe the prior Stop and return without entering logHealth. +func TestChecks_StopBeforeStart_DoesNotStartHealthLoop(t *testing.T) { + t.Parallel() + + c := &Checks{} + + // Teardown wins the race: Stop runs before the Start goroutine. + c.Stop() + + returned := make(chan struct{}) + go func() { + c.Start(context.Background()) + close(returned) + }() + + select { + case <-returned: + // Start observed the prior Stop and skipped logHealth. + case <-time.After(2 * time.Second): + t.Fatal("Checks.Start entered the health loop after a prior Stop — goroutine leak") + } + + c.mu.Lock() + cancelCtx := c.cancelCtx + c.mu.Unlock() + if cancelCtx != nil { + t.Fatal("Checks.Start installed a health-loop context despite a prior Stop") + } +}