Data race on fifoCleanupStarted between MountSecrets' writer goroutine and cleanup closure

[krishna3554]

Summary

MountSecrets shares the fifoCleanupStarted flag between the secrets-writer goroutine and the cleanup closure without any synchronization. This is a data race under the Go memory model: when the consumer's cleanup function runs on the main goroutine while the writer goroutine is inside its loop, both can access the plain bool concurrently.

Location

• File: pkg/controllers/secrets.go
• Function: MountSecrets
• Flag declaration: line 151 (fifoCleanupStarted := false)
• Write: line 155 (cleanupFIFO sets fifoCleanupStarted = true)
• Reads: lines 188, 202, 214 (if errors.Is(err, fs.ErrNotExist) && fifoCleanupStarted), performed from the writer goroutine

fifoCleanupStarted := false            // no mutex/atomic

cleanupFIFO := func() {
    fifoCleanupStarted = true          // written from whichever goroutine calls it
    ...
}

go func() {                            // writer goroutine
    for {
        f, err := os.OpenFile(mountPath, ...)
        if err != nil {
            // race: cleanup has already begun; no need to error
            if errors.Is(err, fs.ErrNotExist) && fifoCleanupStarted {   // unsynchronized read
                break
            }
        ...

Problem

cleanupFIFO is returned to the caller and invoked on the caller's goroutine (e.g. when the child process exits or the CLI shuts down), while the writer goroutine reads the same variable in three places to detect that cleanup has begun. There is no mutex, atomic, or channel establishing a happens-before edge between the write in cleanupFIFO and the reads in the goroutine.

Consequences:

1. It is a genuine data race (go test -race will report it whenever both paths overlap).
2. The guard is unreliable: even setting correctness-of-synchronization aside, the writer goroutine is typically blocked inside os.OpenFile on the FIFO (it only unblocks when a reader opens the pipe). After cleanup removes the pipe, a blocked-then-released open may observe a stale false and proceed to call cleanupFIFO() again and utils.HandleError, depending on timing — the exact scenario the comment // race: cleanup has already begun; no need to error tries to handle is not actually handled deterministically.

Note that the surrounding code clearly intends these checks as race mitigation (the repeated comments), but implements them with an unsynchronized shared variable.

Trigger / Reproduction

Static analysis finding — not confirmed by execution. Any code path where the returned cleanup function executes while the writer goroutine is active: run doppler run --mount <path> ... with a consumer of the mounted file, terminate the wrapped process, and observe the concurrent access (readily reproducible under -race in a unit test that calls MountSecrets and its cleanup concurrently).

Expected Behavior

The flag should be an atomic.Bool (or protected by a mutex) so the writer goroutine reliably observes that cleanup has started and exits its loop instead of re-entering error handling.

Actual Behavior

Unsynchronized read/write of a shared bool; behavior depends on timing and CPU visibility, and race-detector builds flag it.

Impact

Non-deterministic shutdown behavior of the secrets mount: spurious "Unable to mount secrets file" errors during teardown, missed early-exit from the writer loop, and CI failures under -race. The fix is small and local.

Suggested Direction

Replace fifoCleanupStarted := false with var fifoCleanupStarted atomic.Bool, use Store(true) in cleanupFIFO and Load() at the three read sites. Optionally also close/signal the blocked OpenFile (e.g. by opening the read end locally before removing the pipe) so the goroutine cannot stay blocked past cleanup.