Fix (some) data races found by -race

[brianaydemir]

Fixes the races described in #3489.

Design notes by change

sync.Once + reset races (config, server_utils)

Three caches follow the same broken pattern: a sync.Once guards lazy initialization, but test-reset paths overwrite the Once variable and the cached values with bare assignments, racing with concurrent readers.

sync.Once serializes concurrent calls through it, but provides no protection against a concurrent assignment to the Once variable itself. The fix in all three cases is the same: introduce a sync.Mutex that is held by both the init path and the reset path, making them mutually exclusive. The Once continues to do its job (run setup at most once per cycle); the mutex closes the gap.

Helper functions (initTransport/resetTransport, resetFederationDiscoveryState/clearGlobalFederationState) encapsulate the lock scope so call sites stay linear. The lock is not held across the entire Do callback — only across the Do call and the variable assignments — which keeps the critical sections short.

For the federation discovery case, globalFedInfo was already an atomic.Pointer and remains so; only fedDiscoveryOnce and globalFedErr needed the new mutex.

Director ad cache initialization (director/director_advertise.go)

updateInternalDirectorCache was publishing a zero-value *directorInfo into the TTL cache via GetOrSet, then populating its fields afterward. Any reader that obtained the entry between GetOrSet and field population would see nil forwardAdChan, nil cancel, and an empty ad.

The fix constructs a fully initialized directorInfo — including context.WithCancel, forwardAdChan, and the ad itself — before calling GetOrSet. If GetOrSet reports that an existing entry won the race, the newly created cancel function is called immediately to release the unused context.

Advertisement.IOLoad read outside lock (director/monitor.go)

Advertisement already has an RWMutex used by SetIOLoad/GetIOLoad. Copying the embedded ServerAd struct directly bypassed that lock because Go struct copies are not atomic. The fix adds GetServerAd() which holds the read lock for the duration of the copy, mirroring the existing GetIOLoad pattern.

*gin.Context escaping to storage and I/O callers (oauth2/issuer, web_ui)

*gin.Context implements context.Context, so it compiles where a context.Context is expected. It is, however, mutable request state that gin reads and writes throughout the request lifecycle; passing it to storage or outbound-HTTP calls gives those callees a reference to that state, which races with gin's own concurrent modifications.

The fix passes ctx.Request.Context() instead — the underlying net/http request context, whose lifetime is tied to the HTTP request and which is safe to share with any goroutine. A small unexported helper requestBoundContext is introduced in the oauth2/issuer package to make the right choice explicit and to document the reason at the definition site. mirrorDowntimeToRegistry in web_ui follows the same pattern inline (reqCtx := ctx.Request.Context()) rather than calling the helper, because the helper is unexported and in a different package.

JWK key cloning (client/acquire_token.go)

The cached jwk.Key was treated as effectively immutable for the purposes of signing, but key.Set("kid", ...) mutates an internal map. The fix calls key.Clone() before setting the key ID, leaving the cached key untouched.

Director TTL cache snapshot (director/sort.go)

The repository already contains a comment in director_api.go noting that Range races with the eviction goroutine and that Items() should be used instead. This change applies the same fix to getAdsForPath in sort.go.

Narrow-scope fixes

• XRootD metric accumulators (metrics/xrootd_metrics.go): Two separate mutexes, one per accumulator family, rather than one coarse lock. This avoids serializing unrelated metric families while keeping each read-modify-write sequence atomic.

• Origin handler registration (origin_serve/handlers.go): A single mutex around the check-and-set of handlersRegistered. The registration path can return errors and tests need to reset the flag, so a mutex is less surprising than a sync.Once or atomic.Bool.

• Launcher error capture (launchers/launcher.go): Goroutine-local error variables instead of capturing the outer err. The existing error channel is unchanged; only the intermediate local assignments are made goroutine-local.

• PelicanFS stress test RNG (client/pelican_fs_test.go): Per-goroutine rand.Rand instances. No production code is affected.

• Test log hook (test_utils/utils.go): The cleanup hook checks entry.Caller directly rather than entry.Logger.ReportCaller. The global logger flag is only needed to decide whether to populate entry.Caller in the first place; by the time the hook fires, entry.Caller is already set or nil.

---

(Brian A: With a tip of the hat to Copilot.)

[Copilot]

Pull request overview

Addresses data races surfaced by the Go race detector (issue #3489) across several packages: federation discovery globals, transport cache, origin handler registration, XRootD metric accumulators, director cache initialization, JWK key mutation, *gin.Context escaping into storage/HTTP callees, advertisement struct copies, and a few test-only races. The fixes consistently introduce mutexes alongside existing sync.Once reset paths, snapshot mutable shared state before passing it to goroutines/long-lived callers, and route HTTP-derived context.Context (rather than *gin.Context) into storage and outbound calls.

Changes:

• Add mutexes to guard sync.Once + variable reset patterns in config/transport.go, config/config.go, and server_utils/sitename.go.
• Fully initialize directorInfo before publishing to the TTL cache; add Advertisement.GetServerAd() for locked struct copy; snapshot stat.ReqHandler and use Items() instead of Range() for cache iteration.
• Route ctx.Request.Context() (via new requestBoundContext helper) into storage and outbound HTTP calls; clone JWK keys before mutation; per-goroutine RNG and goroutine-local errors in launcher/test code.

Reviewed changes

Copilot reviewed 19 out of 19 changed files in this pull request and generated no comments.

Show a summary per file

File	Description
config/config.go	Adds fedDiscoveryMu and helpers (resetFederationDiscoveryState, clearGlobalFederationState) guarding the Once + globals.
config/transport.go	Adds transportMu plus initTransport/resetTransport helpers to make init and reset mutually exclusive.
config/net_config.go	Replaces inline Once reset with ResetFederationForTest.
server_utils/sitename.go	Adds baseAdMu mutex around baseAdOnce/baseAd/baseAdErr.
server_utils/server_utils.go	Holds baseAdMu during ResetTestState re-init.
origin_serve/handlers.go	Adds handlersMu around the register/reset of handlersRegistered.
metrics/xrootd_metrics.go	Per-accumulator mutexes (lastStatsMu, lastOssStatsMu).
director/director_advertise.go	Fully constructs directorInfo before GetOrSet; cancels unused context on race loss.
director/monitor.go	Uses locked GetServerAd() instead of direct struct copy.
director/sort.go	Switches Range to Items() snapshot.
director/stat.go	Snapshots stat.ReqHandler into a local before launching goroutines.
server_structs/director.go	Adds (*Advertisement).GetServerAd() accessor under RLock.
oauth2/issuer/handlers.go	Introduces requestBoundContext helper and uses it for all storage calls.
oauth2/issuer/admin_handlers.go	Same requestBoundContext adoption for admin endpoints.
web_ui/server_api.go	Uses ctx.Request.Context() for GetFederation/MakeRequest.
launchers/launcher.go	Goroutine-local localErr/path instead of shared captures.
client/acquire_token.go	Clones jwk.Key before setting kid.
client/pelican_fs_test.go	Per-goroutine rand.Rand instance in stress test.
test_utils/utils.go	Drops entry.HasCaller() check (which reads racy global) in favor of entry.Caller != nil.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[h2zh]

I'm going to incremently review all these 14 commits one by one, and you could address these comments either all at once or in the opposite way. First of all, this PR needs to rebase to main because of the param generation overhaul. Here is the comment for the first commit (214f55b)

[h2zh]

The new mutex makes the atomic redundant. Now that every access to globalFedInfo happens under fedDiscoveryMu, a plain field would be equally safe. The atomic is leftover from when readers were lock-free; it's harmless but no longer load-bearing.

[h2zh]

Besides, this mutex is held during the entire fedDiscoveryMu, which includes a HTTP call in discoverFederationImpl. If this networking call hangs, the lock will hold for a long time. A context with timeout could mitigate this problem, however, most invocations of GetFederation(ctx) doesn't have a timeout in the codebase.
A possible fix could be refactoring GetFederation(ctx) function: Split the slow part (discovery HTTP call) from the part that needs the lock (publishing the three variables). Snapshot the Once under the lock, release, run discovery lock-free, then re-acquire only to store results.

[bbockelm]

Maybe simpler - replace the sync pointer with an atomic sync pointer?

That said, the definition of Reset is that it should only be done when no goroutine is running that could read the once. If we are seeing races then it strongly implies there are goroutines leaking between test runs: that would be the underlying issue!

[bbockelm]

I take that back - I see where we write to this during startup.

An atomic pointer would do!

That said, if we are worried about long-held mutexes, since the mutex lifetime is guaranteed to be the same as the Once blocking, we've not made the problem any worse or better.

[h2zh]

The following two comments are also for the sync.Once reset race condition. (commit ef2c087)

The commit message of ef2c087 asserts the mutex prevents reset-vs-getter-read races, and the resetTransport comment (config/transport.go:128-132) names "concurrent reads in the getters" as the hazard being guarded against. However the getters (transport.go:67-115) read the pointer outside transportMu, so neither is actually achieved. I ran race condition detector at test-linux.yml:95-107 locally and the result confirms that.

[h2zh]

InitClient (config/config.go:2380) still calls setupTransport() directly, bypassing this new mutex.

[h2zh]

SSRF transport singleton is similar to transport, which has the same race condition problem. Would you like to handle it in a new commit here or in a future PR?

[h2zh]

When I run the nightly race condition detector at test-linux.yml:95-107 locally against this PR, four other data races surface. Could you take a look?
This is the last piece of the first-round review. When you address these comments, could you make new commits instead of overwrite the existing ones? In this way, I can view the diff clearer.

[h2zh]

director/director_api.go:106 also needs this fix. Same for director_api.go:322, :369, :376.

[h2zh]

Also director/cache_ads.go:282-297

[h2zh]

Could you add a docstring here so human and agent knows this is the required function to get an ad? And adItem.Value().ServerAd is prohibited.

[h2zh]

Why here we don't add a lock for lastS3CacheStats in handleS3CacheStats?
That handler is reached through the same dispatch path as the one (handleOSSStats) this commit shows: handlePacket → handleOSSPacket → {handleOSSStats | handleS3CacheStats}. lastS3CacheStats has identical exposure to lastOssStats but was left unguarded