ab74a3b · crush · git.iko.soy

git.iko.soy/crush

fix(config): guard reload-mutated store fields against lock-free readers

reloadFromDiskLocked republished resolver/loadedPaths/knownProviders/ workspacePath under writeMu only, but their public accessors took no lock — so a reload racing Resolver()/KnownProviders()/LoadedPaths()/configPath() was a data race (detected under -race: a leaked mcp.Initialize goroutine reading store.resolver during a config mutation). Extend configMu (which already guards the config pointer) to cover these siblings: write them under configMu.Lock in reload + rollback, read them under configMu.RLock. Also drop the redundant overrides save/restore from reload — it was a self-assignment (Preserve runtime overrides), so removing it is behavior- preserving and keeps Overrides()' write-through pointer out of the reload write set.

commitab74a3baaf4177181b2de61b5488cdb93dd92e46 authoriko <mail@iko.soy> dateWed 8 Jul 2026, 19:19 (3 months ago) parent699924d

1 file changed+37−16

compare with main

unifiedsplit

internal/config/store.go+37−16

internal/config/store.go+37−16

@@ -133,19 +133,28 @@ func (s *ConfigStore) WorkingDir() string {

133133

134134// Resolver returns the variable resolver.

135135func (s *ConfigStore) Resolver() VariableResolver {

136+ s.configMu.RLock()

137+ defer s.configMu.RUnlock()

136138 return s.resolver

137139}

138140

139141// Resolve resolves a variable reference using the configured resolver.

140142func (s *ConfigStore) Resolve(key string) (string, error) {

141− if s.resolver == nil {

143+ // Snapshot the resolver under the lock, then resolve without it:

144+ // ResolveValue may be slow and can call back into the store.

145+ s.configMu.RLock()

146+ resolver := s.resolver

147+ s.configMu.RUnlock()

148+ if resolver == nil {

142149 return "", fmt.Errorf("no variable resolver configured")

143150 }

144− return s.resolver.ResolveValue(key)

151+ return resolver.ResolveValue(key)

145152}

146153

147154// KnownProviders returns the list of known providers.

148155func (s *ConfigStore) KnownProviders() []catwalk.Provider {

156+ s.configMu.RLock()

157+ defer s.configMu.RUnlock()

149158 return s.knownProviders

150159}

151160

@@ -161,6 +170,8 @@ func (s *ConfigStore) Overrides() *RuntimeOverrides {

161170

162171// LoadedPaths returns the config file paths that were successfully loaded.

163172func (s *ConfigStore) LoadedPaths() []string {

173+ s.configMu.RLock()

174+ defer s.configMu.RUnlock()

164175 return slices.Clone(s.loadedPaths)

165176}

166177

@@ -233,10 +244,13 @@ func (s *ConfigStore) atomicWrite(scope Scope, fn func(current []byte) ([]byte,

233244func (s *ConfigStore) configPath(scope Scope) (string, error) {

234245 switch scope {

235246 case ScopeWorkspace:

236− if s.workspacePath == "" {

247+ s.configMu.RLock()

248+ workspacePath := s.workspacePath

249+ s.configMu.RUnlock()

250+ if workspacePath == "" {

237251 return "", ErrNoWorkspaceConfig

238252 }

239− return s.workspacePath, nil

253+ return workspacePath, nil

240254 default:

241255 return s.globalDataPath, nil

242256 }

@@ -484,8 +498,11 @@ func (s *ConfigStore) SetProviderAPIKey(scope Scope, providerID string, apiKey a

484498 return nil

485499 }

486500

501+ s.configMu.RLock()

502+ knownProviders := s.knownProviders

503+ s.configMu.RUnlock()

487504 var foundProvider *catwalk.Provider

488− for _, p := range s.knownProviders {

505+ for _, p := range knownProviders {

489506 if string(p.ID) == providerID {

490507 foundProvider = &p

491508 break

@@ -885,8 +902,11 @@ func (s *ConfigStore) CaptureStalenessSnapshot(paths []string) {

885902 }

886903

887904 // Also track workspace and global config paths if set

888− if s.workspacePath != "" {

889− abs, err := filepath.Abs(s.workspacePath)

905+ s.configMu.RLock()

906+ workspacePath := s.workspacePath

907+ s.configMu.RUnlock()

908+ if workspacePath != "" {

909+ abs, err := filepath.Abs(workspacePath)

890910 if err == nil {

891911 seen[abs] = struct{}{}

892912 }

@@ -968,9 +988,6 @@ func (s *ConfigStore) reloadFromDiskLocked(ctx context.Context) error {

968988 return fmt.Errorf("invalid hook configuration on reload: %w", err)

969989 }

970990

971− // Preserve runtime overrides

972− overrides := s.overrides

973−

974991 // Reconfigure providers

975992 env := env.New()

976993 resolver := NewShellVariableResolver(env)

@@ -988,16 +1005,19 @@ func (s *ConfigStore) reloadFromDiskLocked(ctx context.Context) error {

9881005 oldLoadedPaths := s.loadedPaths

9891006 oldResolver := s.resolver

9901007 oldKnownProviders := s.knownProviders

991− oldOverrides := s.overrides

9921008 oldWorkspacePath := s.workspacePath

9931009

994− // Update store state BEFORE running model/agent setup (so they see new config)

995− s.setConfig(cfg)

1010+ // Update store state BEFORE running model/agent setup (so they see new

1011+ // config). All reload-mutated fields are published atomically under

1012+ // configMu so the lock-free public readers (Resolver, KnownProviders,

1013+ // LoadedPaths, configPath) never observe a torn update.

1014+ s.configMu.Lock()

1015+ s.config = cfg

9961016 s.loadedPaths = loadedPaths

9971017 s.resolver = resolver

9981018 s.knownProviders = providers

999− s.overrides = overrides

10001019 s.workspacePath = workspacePath

1020+ s.configMu.Unlock()

10011021

10021022 // Mirror startup flow: setup models and agents against NEW config.

10031023 var setupErr error

@@ -1016,12 +1036,13 @@ func (s *ConfigStore) reloadFromDiskLocked(ctx context.Context) error {

10161036

10171037 // Rollback on setup failure

10181038 if setupErr != nil {

1019− s.setConfig(oldConfig)

1039+ s.configMu.Lock()

1040+ s.config = oldConfig

10201041 s.loadedPaths = oldLoadedPaths

10211042 s.resolver = oldResolver

10221043 s.knownProviders = oldKnownProviders

1023− s.overrides = oldOverrides

10241044 s.workspacePath = oldWorkspacePath

1045+ s.configMu.Unlock()

10251046 return setupErr

10261047 }

10271048