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) } 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") + } +}