fix(api): Store is the sync boundary, not a pointer leak#27
Merged
Conversation
CI run 24617740013 caught a data race in TestHandleApplyRuntimePreflight:
team.Controller.evaluateHealth was mutating sess.HealthState through a
pointer returned by Store.ListSessions while daemon.handleGet was
json.Marshal'ing the same *Session. The mutex protected the map; it
did not protect the values.
Fix the contract, not the one pair:
- Store.{Get,List}* return value copies (with cloned slices for Team
and Runtime). Pointers to internal objects never escape.
- New Store.{UpdateSession,UpdateTeam}(key, func) take the write lock
and mutate the live object in place. UpdateSessionHeartbeat stays as
a convenience helper.
- session.Manager.Create commits PaneID+State=Running via UpdateSession
after pane spawn (was mutating the caller's pointer, which used to
alias the store's live object).
- session.Manager.ReapDead routes the Crashed+PaneID="" mutation
through UpdateSession.
- team.Controller.evaluateHealth opens an UpdateSession closure per
session so the entire health evaluation runs under the write lock.
- team.Controller.applyRestartPolicy and restartSession commit State
transitions through UpdateSession.
- team.Controller.InitiateShift / reconcileShift / shiftLaunch /
shiftDrain route Team.Generation and Team.Shift mutations through
UpdateTeam.
- api.Manifest.Apply routes the "update roles on existing team" path
through UpdateTeam (another silent race site).
- daemon.handleScale routes the Roles[i].Replicas = N mutation through
UpdateTeam.
- Tests that mutated LastHeartbeat on snapshot results now call
UpdateSession; tests that mutated Team.Roles[i].Replicas call
UpdateTeam; tests that read team.Shift state fetch fresh via
store.GetTeam.
- .claude/rules/go.md — new Rule 12: never return internal pointers
from a locked accessor, with correct/incorrect examples. References
orc finding-032.
All packages pass go test -race. golangci-lint clean.
Refs: aae-orc-ogto, orc finding-032
arcaven
added
type.bug
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.
Summary
go test -racecaught in CI run 24617740013: team controller'sevaluateHealthmutating*Sessionconcurrently with daemon'shandleGetmarshalling the same object.Store.{Get,List}*now return value copies; mutations go throughStore.{UpdateSession,UpdateTeam}(key, func(*T) error)which take the write lock. Pointers to internal objects never escape.session.Manager.Create/ReapDead,team.Controller.evaluateHealth/applyRestartPolicy/restartSession/InitiateShift/shift handlers,api.Manifest.Apply(silent cousin race),daemon.handleScale(another silent cousin)..claude/rules/go.mdRule 12 — never return internal pointers from a locked accessor, with examples.See orc
finding-032-store-sync-contract-leakfor the class-of-bug writeup + recommendations.Refs: aae-orc-ogto
Test plan
just test-race— all 17 packages passjust lint— 0 issuesjust fmt— cleanTestHandleApplyRuntimePreflight(the original failure) green under -race