Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 14 additions & 0 deletions packages/orchestrator/pkg/sandbox/checks.go
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand All @@ -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
}
Comment on lines +60 to +66

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

While the current change correctly handles the race between Start and Stop, the Start function is not idempotent. If Start is called more than once, it will overwrite c.cancelCtx, leaking the previously started health-check goroutine as its context can no longer be cancelled. To prevent this potential resource leak, Start should also check if it has already been started by verifying that c.cancelCtx is nil.

	if c.stopped || c.cancelCtx != nil {
		// Stop already ran, or Start is already running; do nothing.
		c.mu.Unlock()

		return
	}

ctx, c.cancelCtx = context.WithCancelCause(ctx)
c.mu.Unlock()

Expand All @@ -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)
}
Expand Down
50 changes: 50 additions & 0 deletions packages/orchestrator/pkg/sandbox/checks_test.go
Original file line number Diff line number Diff line change
@@ -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")
}
}
Loading