fix: guard m.state.CurrentMode read in ModeManager.Transition - #2649
fix: guard m.state.CurrentMode read in ModeManager.Transition#2649Vishal4742 wants to merge 4 commits into
Conversation
Transition() read m.state.CurrentMode without holding m.mu while every other reader in the file (CurrentMode, CheckTransitions, cooldownExpired, etc.) acquires m.mu.RLock() first. The cortex loop and the mode-transition handler touch this field from separate goroutines, so the race detector flags the unguarded read. Acquire m.mu.RLock() around the read, release after copying into a local variable. The later m.mu.Lock() that mutates the state is unchanged. Adds TestTransitionConcurrentWithCurrentMode as a regression test, verified with go test -race -count=20.
There was a problem hiding this comment.
Pull request overview
This PR fixes a data race in ModeManager.Transition() by guarding the initial read of m.state.CurrentMode with m.mu.RLock()/RUnlock(), aligning it with the locking strategy used elsewhere in internal/runtime/manager.go. It also adds a regression test intended to reproduce the race under the Go race detector.
Changes:
- Guard the initial
m.state.CurrentModeread inModeManager.Transition()with anRLock. - Add a concurrent regression test that exercises
CurrentMode()andTransition()from separate goroutines (forgo test -race).
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| internal/runtime/manager.go | Adds RLock/RUnlock around the first CurrentMode read in Transition to eliminate a race. |
| internal/runtime/manager_test.go | Adds a concurrency test intended to catch the race via -race. |
| const iterations = 500 | ||
| var wg sync.WaitGroup | ||
|
|
||
| wg.Add(2) | ||
|
|
||
| go func() { | ||
| defer wg.Done() | ||
| for i := 0; i < iterations; i++ { | ||
| _ = m.CurrentMode() | ||
| } | ||
| }() | ||
|
|
||
| go func() { | ||
| defer wg.Done() | ||
| for i := 0; i < iterations; i++ { | ||
| target := "active" | ||
| if i%2 == 0 { | ||
| target = "idle" | ||
| } | ||
| m.Transition(target, "test", nil, nil) | ||
| } | ||
| }() | ||
|
|
||
| wg.Wait() |
There was a problem hiding this comment.
Good catch — you're right that without a start barrier the Go scheduler can let one goroutine run through all of its iterations before the other is dispatched, which would let this test pass on buggy code.
I've pushed 17daa72 to add a start gate: each goroutine blocks on <-start and the main goroutine closes the channel after both are launched, so both enter their tight loops at the same instant. With this in place, go test -race reliably flags the unguarded read in Transition() when the lock is missing, and stays silent when the lock is present.
Updated comment in the test now explains the rationale.
Without a start barrier, the Go scheduler can run one goroutine through all of its iterations before the other is even dispatched, which would let this test pass on buggy code that doesn't actually race. Close the start channel after both goroutines are launched so they enter their tight loops at the same instant. With this in place, -race reliably flags the unguarded read in Transition() when the lock is missing. Addresses the start-gate feedback from Copilot's PR review.
Problem I found
While reviewing the OM1 source I found a data race in
ModeManager.Transition()ininternal/runtime/manager.go. The function readsm.state.CurrentModewithout holdingm.mu, while every other reader in the same file (CurrentMode,CheckTransitions,cooldownExpired, etc.) acquiresm.mu.RLock()first. The cortex loop and the mode-transition handler touch this field from separate goroutines, sogo test -raceflags the unguarded read.My fix
I wrapped the first read in
m.mu.RLock()/m.mu.RUnlock()and copied the result into a local variable. The laterm.mu.Lock()that mutates the state is unchanged.func (m *ModeManager) Transition(toMode, reason string, exitHooks, entryHooks *hooks.Runner) { + m.mu.RLock() fromMode := m.state.CurrentMode + m.mu.RUnlock() + transitionKey := fromMode + "->" + toModeMy regression test
I added
TestTransitionConcurrentWithCurrentModetointernal/runtime/manager_test.go. It runsCurrentMode()andTransition()from separate goroutines for 500 iterations each, joined with async.WaitGroup. Without the fix,go test -racereports a data race on the read ofm.state.CurrentModeinTransition. With the fix, the race detector stays silent.How I verified locally
gofmt -l— cleango vet ./internal/runtime/...— cleangolangci-lint v1.64.8 run internal/runtime/...— cleango test -race -run TestTransitionConcurrentWithCurrentMode -count=20 ./internal/runtime/...— 20/20 PASS, 0 race warningsgo test -race -count=1 ./internal/runtime/...— PASS, 0 race warningsgo test -race ./...— 38 packages PASS, 0 race warningsA note on the
Run Nightly Integration TestsCI checkThe workflow requires the
OM1_API_KEYrepository secret, which GitHub Actions does not pass to workflows triggered bypull_requestevents from forks. The integration tests will therefore fail on this fork PR until a maintainer approves the workflow run on the Actions tab — at that point the secret becomes available. The other four CI checks (Build,Run Lint,Run Unit Tests,Pull Request Labeler) all pass on the push.About my previous PR
I opened PR #2646 before I had a chance to verify the fix locally and retracted it. This is the same fix, properly verified locally this time, with the regression test included.