server: fix the data race between runnerRef.unload and LogValue - #289
Merged
Merged
Conversation
The race CI job failed on 2026-09-08 with three DATA RACE reports in the multiple-loaded-models scheduler test: unload clears model, Options and gpus while holding refMu, but LogValue reads them holding nothing, and slog resolves that value lazily inside every slog.Debug(..., "runner", runner) call. Eleven of those log sites already hold refMu, so LogValue cannot take it -- a plain Lock there self-deadlocks. Guard exactly the fields unload clears with a second, leaf-level RWMutex: unload takes it after refMu, LogValue takes it alone and copies what it needs into locals before building the attrs. Nothing takes refMu while holding logMu, so the order is always refMu -> logMu and no deadlock is possible. The log output is unchanged, including the empty-model-name case. The new test reproduces all three reported races deterministically on the unfixed code (sched.go:1410, 1411, 1412) and is clean with the fix: go test -race -count=20 on it and on the test that failed in CI, plus the full -race server suite, vet and golangci-lint. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
This was referenced Sep 10, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes the intermittent
race (ubuntu-latest)CI failure. It is pre-existing onmain: it surfaced on two branches that never touchserver/(#287 and #286), and passed on other commits of those same branches.The race.
unloadclearsmodel,Optionsandgpuswhile holdingrefMu.LogValuereads them holding nothing, and slog resolves it lazily inside eachslog.Debug(…, "runner", runner)call. The failing job reported exactly those three fields (sched.go:1410, 1411, 1412 against 1562, 1577, 1565).Why not just lock in LogValue. Eleven of the ~25 log sites already hold
refMuwhen they log the runner (343, 408, 415, 417, 428, 433, 439, 451, 483, 762, 766), sorefMu.Lock()insideLogValuewould self-deadlock.TryLockwould not fix the race, only narrow it.The fix. A second, leaf-level
sync.RWMutexguarding exactly the fieldsunloadclears.unloadtakes it afterrefMu;LogValuetakes it alone and copies what it needs into locals before building the attrs. Nothing takesrefMuwhile holdinglogMu, so the order is alwaysrefMu → logMuand no deadlock is possible. Log output is unchanged, including the empty-model-name case (hasModel, notname != "").Verification (golang:1.26.0 container):
-race-race -count=20TestSchedRequestsMultipleLoadedModels(the CI failure),-race -count=20go test -race ./server/go vet,golangci-lintThe negative control is the point of the new test: it spawns a reader that resolves the value exactly as slog does, signals that it is running, and keeps reading past the unload, so the two overlap with no synchronisation of their own. A first version of the test passed even without the fix because the reader goroutine had not been scheduled before the unload; that version would have been worthless as a regression test.
🤖 Generated with Claude Code