Skip to content

Simpler code to cache value #2808

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
dgageot wants to merge 1 commit into from
+7 −25
Closed

Simpler code to cache value #2808

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
32 changes: 7 additions & 25 deletions pkg/userid/userid.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,26 +23,20 @@ import (
// stored under [paths.GetConfigDir].
const fileName = "user-uuid"

var (
mu sync.Mutex
cached string
)

// Get returns the persistent UUID identifying this cagent installation.
//
// On the first call it tries to read the value from
// `$configDir/user-uuid`; if the file does not exist, is empty, or
// cannot be read, a fresh UUID is generated and persisted (best
// effort). The result is cached in memory for the lifetime of the
// process so subsequent calls do not touch the filesystem.
func Get() string {
mu.Lock()
defer mu.Unlock()
var Get = sync.OnceValue(get)

if cached != "" {
return cached
}
func ResetForTests() {
Copy link
Copy Markdown

@docker-agent docker-agent May 17, 2026

Choose a reason for hiding this comment

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

[MEDIUM] Data race: unsynchronised write to Get in ResetForTests() vs concurrent reads

ResetForTests() assigns a fresh sync.OnceValue to the package-level variable Get without any synchronisation. Meanwhile, parallel tests (e.g. TestHeaders uses t.Parallel()) or the HTTP client may be reading Get to call it. Writing to a shared variable from one goroutine while another goroutine reads it is a data race under the Go memory model — go test -race will flag it.

The old implementation avoided this by holding mu inside ResetForTests() before clearing cached. The new implementation has no equivalent protection on the variable itself.

Suggested fix: protect the variable with a sync.RWMutex, use sync/atomic (atomic.Value or a pointer), or — simplest for a test-only reset — guard the write and read with a mutex:

var getMu sync.RWMutex
var getOnce = sync.OnceValue(get)

func Get() string {
    getMu.RLock()
    f := getOnce
    getMu.RUnlock()
    return f()
}

func ResetForTests() {
    getMu.Lock()
    getOnce = sync.OnceValue(get)
    getMu.Unlock()
}

Get = sync.OnceValue(get)
}

func get() string {
file := filePath()

if data, err := os.ReadFile(file); err == nil {
Expand All @@ -52,8 +46,7 @@ func Get() string {
// rather than propagating invalid data to telemetry and
// the gateway.
if _, err := uuid.Parse(existing); err == nil {
cached = existing
return cached
return existing
}
// File contains invalid UUID — fall through and regenerate.
}
Expand All @@ -66,18 +59,7 @@ func Get() string {
// disk we still cache it in memory so the same identifier is used
// for the rest of this process.
_ = save(file, id)
cached = id
return cached
}

// ResetForTests clears the in-memory cache. Tests in any package
// that rely on a deterministic config dir override should call this
// after [paths.SetConfigDir] to force the next [Get] call to re-read
// from disk.
func ResetForTests() {
mu.Lock()
defer mu.Unlock()
cached = ""
return id
}

func filePath() string {
Expand Down
Loading