cacheRobotClient is not concurrency-safe
Description
cacheRobotClient keeps shared mutable state without synchronization, but the same client instance can be accessed from multiple goroutines.
The affected state lives in client.go:
lmlastUpdateforcedRefreshServerNames
These fields are read and written from multiple methods without any mutex, for example:
There is also a realistic concurrent path in production:
- request handling reads and mutates the cache via Robot lookup code in util.go
- credential hot-reload can update the same client in the background via hotreload.go
This means cache invalidation, forced refresh bookkeeping, and normal reads can overlap.
Why this matters
This can lead to data races and inconsistent cache behavior, for example:
- concurrent read/write of
m
- concurrent read/write of
forcedRefreshServerNames
- stale or partially refreshed cache state across
l, m, and lastUpdate
- cache invalidation in
SetCredentials racing with ongoing lookups
Even if this does not always crash, it makes behavior timing-dependent and hard to reason about.
Expected behavior
cacheRobotClient should be safe for concurrent use by multiple goroutines.
Actual behavior
The client relies on unsynchronized shared mutable state.
Suggested fix
Protect the cache state with a sync.RWMutex or sync.Mutex and make cache updates atomic across:
lmlastUpdateforcedRefreshServerNames
In particular, SetCredentials, ServerGetListForceRefresh, ServerGet, ServerGetList, and the forced-refresh bookkeeping methods should all participate in the same locking scheme.
Acceptance criteria
- no unsynchronized access to mutable fields in
cacheRobotClient
- concurrent reads and invalidations are safe
- add a focused concurrency test for cache access plus
SetCredentials
go test -race ./... stays clean
cacheRobotClientis not concurrency-safeDescription
cacheRobotClientkeeps shared mutable state without synchronization, but the same client instance can be accessed from multiple goroutines.The affected state lives in client.go:
lmlastUpdateforcedRefreshServerNamesThese fields are read and written from multiple methods without any mutex, for example:
There is also a realistic concurrent path in production:
This means cache invalidation, forced refresh bookkeeping, and normal reads can overlap.
Why this matters
This can lead to data races and inconsistent cache behavior, for example:
mforcedRefreshServerNamesl,m, andlastUpdateSetCredentialsracing with ongoing lookupsEven if this does not always crash, it makes behavior timing-dependent and hard to reason about.
Expected behavior
cacheRobotClientshould be safe for concurrent use by multiple goroutines.Actual behavior
The client relies on unsynchronized shared mutable state.
Suggested fix
Protect the cache state with a
sync.RWMutexorsync.Mutexand make cache updates atomic across:lmlastUpdateforcedRefreshServerNamesIn particular,
SetCredentials,ServerGetListForceRefresh,ServerGet,ServerGetList, and the forced-refresh bookkeeping methods should all participate in the same locking scheme.Acceptance criteria
cacheRobotClientSetCredentialsgo test -race ./...stays clean