bug(orchestrator): Cleanup.Add has TOCTOU race — cleanup functions silently dropped after Run()

Summary

Cleanup.Add and Cleanup.AddPriority have a TOCTOU (time-of-check/time-of-use) race against Cleanup.run that silently discards cleanup functions without executing them, causing permanent resource leaks.

Root Cause

In packages/orchestrator/pkg/sandbox/cleanup.go:

run() sets hasRun before acquiring the lock (line 80 vs 82):

func (c *Cleanup) run(ctx context.Context) {
    c.hasRun.Store(true)   // ← written outside the lock
    c.mu.Lock()
    defer c.mu.Unlock()
    // ... drains c.cleanup and c.priorityCleanup
}

Add() reads hasRun before acquiring the lock (line 40 vs 49):

func (c *Cleanup) Add(ctx context.Context, f func(ctx context.Context) error) {
    if c.hasRun.Load() == true {   // ← read outside the lock
        // run f immediately
        return
    }
    c.mu.Lock()
    defer c.mu.Unlock()
    c.cleanup = append(c.cleanup, f)   // ← appended after run() may have drained
}

Race Window

Goroutine A (Add):    hasRun.Load() == false  →  [not yet locked]
Goroutine B (run):    hasRun.Store(true)  →  Lock()  →  drain all cleanup  →  Unlock()
Goroutine A (Add):    Lock()  →  append(f)   ← f is now in a drained slice, never executed

sync.Once in Run() prevents run() from being called a second time, so f is permanently lost — no log, no error, no signal.

Impact

The Cleanup type is the central resource-release mechanism for Firecracker sandbox lifecycle. The package has 28 call sites across sandbox startup (sandbox.go, resume.go, reboot.go, etc.) registering operations such as:

• Closing overlay filesystems
• Releasing network slots
• Removing Firecracker and UFFD socket files
• Stopping the Firecracker process

Any one of these silently dropped during a concurrent error-path teardown leaves a leaked resource on the host until the orchestrator restarts.

Fix

Move hasRun.Store(true) inside the lock in run(), and add a double-check in Add() / AddPriority() after acquiring mu:

func (c *Cleanup) Add(ctx context.Context, f func(ctx context.Context) error) {
    // Optimistic fast path.
    if c.hasRun.Load() {
        err := f(context.WithoutCancel(ctx))
        if err != nil {
            logger.L().Error(ctx, "failed to run function after cleanup has run", zap.Error(err))
        }
        return
    }

    c.mu.Lock()
    // Double-check: run() may have completed between the Load above and here.
    if c.hasRun.Load() {
        c.mu.Unlock()
        err := f(context.WithoutCancel(ctx))
        if err != nil {
            logger.L().Error(ctx, "failed to run function after cleanup has run", zap.Error(err))
        }
        return
    }
    c.cleanup = append(c.cleanup, f)
    c.mu.Unlock()
}

func (c *Cleanup) run(ctx context.Context) {
    c.mu.Lock()
    defer c.mu.Unlock()

    c.hasRun.Store(true)  // now set under the lock — closes the race window

    // ... rest unchanged
}

The same double-check applies to AddPriority.

Review Cleanup.Add, Cleanup.AddPriority, and Cleanup.run in packages/orchestrator/pkg/sandbox/cleanup.go, focusing on the lock and hasRun ordering. Confirm the change closes the TOCTOU window and that cleanup functions registered concurrently with Run() are no longer silently dropped, then exercise the package's existing tests with the race detector.