b10f890 · crush · git.iko.soy

git.iko.soy/crush

on main help sign in

fix(config): make concurrent config access race-free via copy-on-write

b10f890Kieran Klukas4 months ago

internal/agent/agenttest/coordinator.go+2−2

compare with main

unifiedsplit

@@ -55,8 +55,8 @@ func NewCoordinator(

55 Models: []catwalk.Model{{ID: modelID, DefaultMaxTokens: 4096}},

56 })

57 selected := config.SelectedModel{Provider: providerID, Model: modelID}

58− cfg.Config().Models[config.SelectedModelTypeLarge] = selected

58+ cfg.OverridePreferredModel(config.SelectedModelTypeLarge, selected)

59− cfg.Config().Models[config.SelectedModelTypeSmall] = selected

59+ cfg.OverridePreferredModel(config.SelectedModelTypeSmall, selected)

60 cfg.SetupAgents()

62 // Keep buildTools light: no sub-agent or agentic-fetch construction.

internal/app/app.go+5−5

@@ -433,10 +433,10 @@ func (app *App) overrideModelsForNonInteractive(ctx context.Context, largeModel,

433 }

434 largeProviderID = found.provider

435 slog.Info("Overriding large model for non-interactive run", "provider", found.provider, "model", found.modelID)

436− app.config.Config().Models[config.SelectedModelTypeLarge] = config.SelectedModel{

436+ app.config.OverridePreferredModel(config.SelectedModelTypeLarge, config.SelectedModel{

437 Provider: found.provider,

438 Model: found.modelID,

439− }

439+ })

440 }

441

442 // Override small model.

@@ -447,15 +447,15 @@ func (app *App) overrideModelsForNonInteractive(ctx context.Context, largeModel,

447 return err

448 }

449 slog.Info("Overriding small model for non-interactive run", "provider", found.provider, "model", found.modelID)

450− app.config.Config().Models[config.SelectedModelTypeSmall] = config.SelectedModel{

450+ app.config.OverridePreferredModel(config.SelectedModelTypeSmall, config.SelectedModel{

451 Provider: found.provider,

452 Model: found.modelID,

453− }

453+ })

454

455 case largeModel != "":

456 // No small model specified, but large model was - use provider's default.

457 smallCfg := app.GetDefaultSmallModel(largeProviderID)

458− app.config.Config().Models[config.SelectedModelTypeSmall] = smallCfg

458+ app.config.OverridePreferredModel(config.SelectedModelTypeSmall, smallCfg)

459 }

460

461 return app.AgentCoordinator.UpdateModels(ctx)

internal/backend/config.go+2−2

@@ -222,13 +222,13 @@ func (b *Backend) EnableDockerMCP(ctx context.Context, workspaceID string) error

222

223 if err := mcptools.InitializeSingle(ctx, config.DockerMCPName, ws.Cfg); err != nil {

224 disableErr := mcptools.DisableSingle(ws.Cfg, config.DockerMCPName)

225− delete(ws.Cfg.Config().MCP, config.DockerMCPName)

225+ ws.Cfg.RemoveDockerMCPInMemory()

226 return fmt.Errorf("failed to start docker MCP: %w", errors.Join(err, disableErr))

227 }

228

229 if err := ws.Cfg.PersistDockerMCPConfig(mcpConfig); err != nil {

230 disableErr := mcptools.DisableSingle(ws.Cfg, config.DockerMCPName)

231− delete(ws.Cfg.Config().MCP, config.DockerMCPName)

231+ ws.Cfg.RemoveDockerMCPInMemory()

232 return fmt.Errorf("docker MCP started but failed to persist configuration: %w", errors.Join(err, disableErr))

233 }

234

internal/config/clone_test.gonew file+46

@@ -0,0 +1,46 @@

1+package config

3+import (

4+ "testing"

6+ "github.com/charmbracelet/crush/internal/csync"

7+ "github.com/stretchr/testify/require"

8+)

10+// TestCloneForWrite_Isolation verifies that mutating a clone never reaches

11+// back into the original Config. This is the contract the store's

12+// copy-on-write mutators depend on for race-free publishing.

13+func TestCloneForWrite_Isolation(t *testing.T) {

14+ t.Parallel()

15+

16+ orig := &Config{

17+ Models: map[SelectedModelType]SelectedModel{

18+ SelectedModelTypeLarge: {Provider: "openai", Model: "gpt-4"},

19+ },

20+ RecentModels: map[SelectedModelType][]SelectedModel{

21+ SelectedModelTypeLarge: {{Provider: "openai", Model: "gpt-4"}},

22+ },

23+ MCP: MCPs{"a": {}},

24+ Providers: csync.NewMap[string, ProviderConfig](),

25+ Options: &Options{

26+ TUI: &TUIOptions{CompactMode: false},

27+ },

28+ }

29+

30+ clone := orig.cloneForWrite()

31+

32+ // Mutate every field the typed mutators touch.

33+ clone.Models[SelectedModelTypeLarge] = SelectedModel{Provider: "anthropic", Model: "claude"}

34+ clone.RecentModels[SelectedModelTypeLarge] = []SelectedModel{{Provider: "anthropic", Model: "claude"}}

35+ clone.MCP["b"] = MCPConfig{}

36+ clone.Options.TUI.CompactMode = true

37+ enabled := true

38+ clone.Options.TUI.Transparent = &enabled

39+

40+ // The original must be untouched.

41+ require.Equal(t, "openai", orig.Models[SelectedModelTypeLarge].Provider, "Models leaked")

42+ require.Equal(t, "openai", orig.RecentModels[SelectedModelTypeLarge][0].Provider, "RecentModels leaked")

43+ require.NotContains(t, orig.MCP, "b", "MCP leaked")

44+ require.False(t, orig.Options.TUI.CompactMode, "Options.TUI.CompactMode leaked")

45+ require.Nil(t, orig.Options.TUI.Transparent, "Options.TUI.Transparent leaked")

46+}

internal/config/config.go+39

@@ -619,6 +619,45 @@ type Config struct {

619 Agents map[string]Agent `json:"-"`

620}

621

622+// cloneForWrite returns a copy of c that the store's typed field mutators

623+// may modify without racing readers of the currently published Config.

624+//

625+// Reads of a published Config take no lock beyond the pointer load, so a

626+// mutator must never write through the live pointer. Instead it clones,

627+// mutates the clone, and atomically swaps it in. The clone gives fresh

628+// copies of every field a typed mutator touches in place — Models,

629+// RecentModels, MCP, and Options (with its nested TUI pointer). Providers

630+// is a *csync.Map (internally synchronized) and is shared by reference;

631+// the remaining fields are immutable after load from the mutators'

632+// standpoint and are likewise shared.

633+func (c *Config) cloneForWrite() *Config {

634+ nc := *c

635+ nc.Models = maps.Clone(c.Models)

636+ nc.RecentModels = maps.Clone(c.RecentModels)

637+ nc.MCP = maps.Clone(c.MCP)

638+ if c.Options != nil {

639+ opts := *c.Options

640+ if c.Options.TUI != nil {

641+ tui := *c.Options.TUI

642+ opts.TUI = &tui

643+ }

644+ nc.Options = &opts

645+ }

646+ return &nc

647+}

648+

649+// ensureTUI returns c.Options.TUI, allocating Options and TUI as needed so

650+// callers can assign TUI fields without nil checks.

651+func (c *Config) ensureTUI() *TUIOptions {

652+ if c.Options == nil {

653+ c.Options = &Options{}

654+ }

655+ if c.Options.TUI == nil {

656+ c.Options.TUI = &TUIOptions{}

657+ }

658+ return c.Options.TUI

659+}

660+

622func (c *Config) EnabledProviders() []ProviderConfig {

661func (c *Config) EnabledProviders() []ProviderConfig {

623 var enabled []ProviderConfig

662 var enabled []ProviderConfig

624 for p := range c.Providers.Seq() {

663 for p := range c.Providers.Seq() {

internal/config/docker_mcp.go+23−16

@@ -88,10 +88,13 @@ func (s *ConfigStore) PrepareDockerMCPConfig() (MCPConfig, error) {

88 }

90 mcpConfig := DockerMCPConfig()

91+ // In-memory only; persistence happens in PersistDockerMCPConfig.

92+ s.mutateInMemory(func(c *Config) {

91− if s.config.MCP == nil {

93+ if c.MCP == nil {

92− s.config.MCP = make(map[string]MCPConfig)

94+ c.MCP = make(map[string]MCPConfig)

93− }

95+ }

94− s.config.MCP[DockerMCPName] = mcpConfig

96+ c.MCP[DockerMCPName] = mcpConfig

97+ })

95 return mcpConfig, nil

98 return mcpConfig, nil

96}

99}

100

@@ -118,17 +121,21 @@ func (s *ConfigStore) EnableDockerMCP() error {

118

121

119// DisableDockerMCP removes Docker MCP configuration and persists the change.

122// DisableDockerMCP removes Docker MCP configuration and persists the change.

120func (s *ConfigStore) DisableDockerMCP() error {

123func (s *ConfigStore) DisableDockerMCP() error {

124+ return s.update(ScopeGlobal, func(c *Config) map[string]any {

121− if s.config.MCP == nil {

125+ if c.MCP == nil {

122− return nil

126+ return nil

123− }

127+ }

124−

125− // Remove from in-memory config.

126− delete(s.config.MCP, DockerMCPName)

128+ delete(c.MCP, DockerMCPName)

127−

129+ return map[string]any{"mcp": c.MCP}

128− // Persist the updated MCP map to the config file.

130+ })

129− if err := s.SetConfigField(ScopeGlobal, "mcp", s.config.MCP); err != nil {

130− return fmt.Errorf("failed to persist docker mcp removal: %w", err)

131− }

131+}

132

133− return nil

133+// RemoveDockerMCPInMemory removes the Docker MCP entry from the in-memory

134+// config via copy-on-write, without persisting to disk. It rolls back a

135+// staged PrepareDockerMCPConfig when starting or persisting the server

136+// fails, so callers must not mutate Config().MCP directly.

137+func (s *ConfigStore) RemoveDockerMCPInMemory() {

138+ s.mutateInMemory(func(c *Config) {

139+ delete(c.MCP, DockerMCPName)

140+ })

134}

141}

internal/config/docker_mcp_test.go+9−7

@@ -75,9 +75,10 @@ func TestEnableDockerMCP(t *testing.T) {

75 err := store.EnableDockerMCP()

76 require.NoError(t, err)

78− // Check in-memory config.

78+ // Check in-memory config via the store (copy-on-write publishes

79+ // a new Config; the captured cfg pointer stays unchanged).

79− require.True(t, cfg.IsDockerMCPEnabled())

80+ require.True(t, store.Config().IsDockerMCPEnabled())

80− mcpConfig, exists := cfg.MCP[DockerMCPName]

81+ mcpConfig, exists := store.Config().MCP[DockerMCPName]

81 require.True(t, exists)

82 require.True(t, exists)

82 require.Equal(t, MCPStdio, mcpConfig.Type)

83 require.Equal(t, MCPStdio, mcpConfig.Type)

83 require.Equal(t, "docker", mcpConfig.Command)

84 require.Equal(t, "docker", mcpConfig.Command)

@@ -145,9 +146,10 @@ func TestDisableDockerMCP(t *testing.T) {

145 err := store.DisableDockerMCP()

146 err := store.DisableDockerMCP()

146 require.NoError(t, err)

147 require.NoError(t, err)

147

148

148− // Check in-memory config.

149+ // Check in-memory config via the store (copy-on-write publishes a

150+ // new Config; the original cfg pointer is intentionally unchanged).

149− require.False(t, cfg.IsDockerMCPEnabled())

151+ require.False(t, store.Config().IsDockerMCPEnabled())

150− _, exists := cfg.MCP[DockerMCPName]

152+ _, exists := store.Config().MCP[DockerMCPName]

151 require.False(t, exists)

153 require.False(t, exists)

152 })

154 })

153

155

@@ -189,5 +191,5 @@ func TestEnableDockerMCPWithRealDockerWhenAvailable(t *testing.T) {

189

191

190 err := store.EnableDockerMCP()

192 err := store.EnableDockerMCP()

191 require.NoError(t, err)

193 require.NoError(t, err)

192− require.True(t, cfg.IsDockerMCPEnabled())

194+ require.True(t, store.Config().IsDockerMCPEnabled())

193}

195}

internal/config/load.go+3−3

@@ -111,10 +111,10 @@ func Load(workingDir, dataDir string, debug bool) (*ConfigStore, error) {

111 valueResolver := NewShellVariableResolver(env)

112 store.resolver = valueResolver

113

114− // Hold reloadMu during initial load to prevent configureProviders

114+ // Hold writeMu during initial load to prevent configureProviders

115 // from triggering auto-reload via RemoveConfigField.

116− store.reloadMu.Lock()

116+ store.writeMu.Lock()

117− defer store.reloadMu.Unlock()

117+ defer store.writeMu.Unlock()

118

119 if err := cfg.configureProviders(context.Background(), store, env, valueResolver, store.knownProviders); err != nil {

120 return nil, fmt.Errorf("failed to configure providers: %w", err)

internal/config/recent_models_test.go+67−170

@@ -39,158 +39,76 @@ func testStoreWithPath(cfg *Config, dir string) *ConfigStore {

39 }

40}

42−func TestRecordRecentModel_AddsAndPersists(t *testing.T) {

42+// configWithRecents builds a Config seeded with the given recent models for

43− t.Parallel()

43+// the large type, for exercising the pure nextRecentModels helper.

44−

44+func configWithRecents(recents ...SelectedModel) *Config {

45− dir := t.TempDir()

46− cfg := &Config{}

45+ return &Config{

47− cfg.setDefaults(dir, "")

46+ RecentModels: map[SelectedModelType][]SelectedModel{

48− store := testStoreWithPath(cfg, dir)

47+ SelectedModelTypeLarge: recents,

49−

48+ },

50− err := store.recordRecentModel(ScopeGlobal, SelectedModelTypeLarge, SelectedModel{Provider: "openai", Model: "gpt-4o"})

49+ }

51− require.NoError(t, err)

52−

53− // in-memory state

54− require.Len(t, cfg.RecentModels[SelectedModelTypeLarge], 1)

55− require.Equal(t, "openai", cfg.RecentModels[SelectedModelTypeLarge][0].Provider)

56− require.Equal(t, "gpt-4o", cfg.RecentModels[SelectedModelTypeLarge][0].Model)

57−

58− // persisted state

59− rm := readRecentModels(t, store.globalDataPath)

60− large, ok := rm[string(SelectedModelTypeLarge)].([]any)

61− require.True(t, ok)

62− require.Len(t, large, 1)

63− item, ok := large[0].(map[string]any)

64− require.True(t, ok)

65− require.Equal(t, "openai", item["provider"])

66− require.Equal(t, "gpt-4o", item["model"])

67}

50}

69−func TestRecordRecentModel_DedupeAndMoveToFront(t *testing.T) {

52+func TestNextRecentModels_AddsToFront(t *testing.T) {

70 t.Parallel()

53 t.Parallel()

72− dir := t.TempDir()

55+ cfg := configWithRecents()

73− cfg := &Config{}

56+ updated, changed := nextRecentModels(cfg, SelectedModelTypeLarge, SelectedModel{Provider: "openai", Model: "gpt-4o"})

74− cfg.setDefaults(dir, "")

57+ require.True(t, changed)

75− store := testStoreWithPath(cfg, dir)

58+ require.Equal(t, []SelectedModel{{Provider: "openai", Model: "gpt-4o"}}, updated)

59+}

77− // Add two entries

61+func TestNextRecentModels_DedupeAndMoveToFront(t *testing.T) {

78− require.NoError(t, store.recordRecentModel(ScopeGlobal, SelectedModelTypeLarge, SelectedModel{Provider: "openai", Model: "gpt-4o"}))

62+ t.Parallel()

79− require.NoError(t, store.recordRecentModel(ScopeGlobal, SelectedModelTypeLarge, SelectedModel{Provider: "anthropic", Model: "claude"}))

80− // Re-add first; should move to front and not duplicate

81− require.NoError(t, store.recordRecentModel(ScopeGlobal, SelectedModelTypeLarge, SelectedModel{Provider: "openai", Model: "gpt-4o"}))

83− got := cfg.RecentModels[SelectedModelTypeLarge]

64+ cfg := configWithRecents(

65+ SelectedModel{Provider: "anthropic", Model: "claude"},

66+ SelectedModel{Provider: "openai", Model: "gpt-4o"},

67+ )

68+ updated, changed := nextRecentModels(cfg, SelectedModelTypeLarge, SelectedModel{Provider: "openai", Model: "gpt-4o"})

84− require.Len(t, got, 2)

69+ require.True(t, changed)

85− require.Equal(t, SelectedModel{Provider: "openai", Model: "gpt-4o"}, got[0])

70+ require.Equal(t, []SelectedModel{

71+ {Provider: "openai", Model: "gpt-4o"},

86− require.Equal(t, SelectedModel{Provider: "anthropic", Model: "claude"}, got[1])

72+ {Provider: "anthropic", Model: "claude"},

73+ }, updated)

87}

74}

89−func TestRecordRecentModel_TrimsToMax(t *testing.T) {

76+func TestNextRecentModels_TrimsToMax(t *testing.T) {

90 t.Parallel()

77 t.Parallel()

92− dir := t.TempDir()

79+ var seed []SelectedModel

93− cfg := &Config{}

80+ for _, id := range []string{"m5", "m4", "m3", "m2", "m1"} {

94− cfg.setDefaults(dir, "")

95− store := testStoreWithPath(cfg, dir)

96−

97− // Insert 6 unique models; max is 5

98− entries := []SelectedModel{

99− {Provider: "p1", Model: "m1"},

100− {Provider: "p2", Model: "m2"},

101− {Provider: "p3", Model: "m3"},

102− {Provider: "p4", Model: "m4"},

103− {Provider: "p5", Model: "m5"},

104− {Provider: "p6", Model: "m6"},

81+ seed = append(seed, SelectedModel{Provider: "p", Model: id})

105 }

82 }

106− for _, e := range entries {

83+ cfg := configWithRecents(seed...)

107− require.NoError(t, store.recordRecentModel(ScopeGlobal, SelectedModelTypeLarge, e))

108− }

109−

110− // in-memory state

111− got := cfg.RecentModels[SelectedModelTypeLarge]

112− require.Len(t, got, 5)

113− // Newest first, capped at 5: p6..p2

114− require.Equal(t, SelectedModel{Provider: "p6", Model: "m6"}, got[0])

115− require.Equal(t, SelectedModel{Provider: "p5", Model: "m5"}, got[1])

116− require.Equal(t, SelectedModel{Provider: "p4", Model: "m4"}, got[2])

117− require.Equal(t, SelectedModel{Provider: "p3", Model: "m3"}, got[3])

118− require.Equal(t, SelectedModel{Provider: "p2", Model: "m2"}, got[4])

119

120− // persisted state: verify trimmed to 5 and newest-first order

85+ updated, changed := nextRecentModels(cfg, SelectedModelTypeLarge, SelectedModel{Provider: "p", Model: "m6"})

121− rm := readRecentModels(t, store.globalDataPath)

122− large, ok := rm[string(SelectedModelTypeLarge)].([]any)

123− require.True(t, ok)

86+ require.True(t, changed)

124− require.Len(t, large, 5)

87+ require.Len(t, updated, maxRecentModelsPerType)

125− // Build provider:model IDs and verify order

88+ require.Equal(t, SelectedModel{Provider: "p", Model: "m6"}, updated[0])

126− var ids []string

89+ require.Equal(t, SelectedModel{Provider: "p", Model: "m2"}, updated[maxRecentModelsPerType-1])

127− for _, v := range large {

128− m := v.(map[string]any)

129− ids = append(ids, m["provider"].(string)+":"+m["model"].(string))

130− }

131− require.Equal(t, []string{"p6:m6", "p5:m5", "p4:m4", "p3:m3", "p2:m2"}, ids)

132}

90}

133

134−func TestRecordRecentModel_SkipsEmptyValues(t *testing.T) {

92+func TestNextRecentModels_SkipsEmptyValues(t *testing.T) {

135 t.Parallel()

93 t.Parallel()

136

137− dir := t.TempDir()

95+ cfg := configWithRecents()

138− cfg := &Config{}

139− cfg.setDefaults(dir, "")

140− store := testStoreWithPath(cfg, dir)

141−

142− // Missing provider

143− require.NoError(t, store.recordRecentModel(ScopeGlobal, SelectedModelTypeLarge, SelectedModel{Provider: "", Model: "m"}))

96+ _, changed := nextRecentModels(cfg, SelectedModelTypeLarge, SelectedModel{Provider: "", Model: "m"})

144− // Missing model

97+ require.False(t, changed)

145− require.NoError(t, store.recordRecentModel(ScopeGlobal, SelectedModelTypeLarge, SelectedModel{Provider: "p", Model: ""}))

98+ _, changed = nextRecentModels(cfg, SelectedModelTypeLarge, SelectedModel{Provider: "p", Model: ""})

146−

99+ require.False(t, changed)

147− _, ok := cfg.RecentModels[SelectedModelTypeLarge]

148− // Map may be initialized, but should have no entries

149− if ok {

150− require.Len(t, cfg.RecentModels[SelectedModelTypeLarge], 0)

151− }

152− // No file should be written (stat via fs.FS)

153− baseDir := filepath.Dir(store.globalDataPath)

154− fileName := filepath.Base(store.globalDataPath)

155− _, err := fs.Stat(os.DirFS(baseDir), fileName)

156− require.True(t, os.IsNotExist(err))

157}

100}

158

101

159−func TestRecordRecentModel_NoPersistOnNoop(t *testing.T) {

102+func TestNextRecentModels_NoChangeWhenAlreadyFront(t *testing.T) {

160 t.Parallel()

103 t.Parallel()

161

104

162− dir := t.TempDir()

163− cfg := &Config{}

164− cfg.setDefaults(dir, "")

165− store := testStoreWithPath(cfg, dir)

166−

167 entry := SelectedModel{Provider: "openai", Model: "gpt-4o"}

105 entry := SelectedModel{Provider: "openai", Model: "gpt-4o"}

168− require.NoError(t, store.recordRecentModel(ScopeGlobal, SelectedModelTypeLarge, entry))

106+ cfg := configWithRecents(entry)

169−

170− baseDir := filepath.Dir(store.globalDataPath)

171− fileName := filepath.Base(store.globalDataPath)

172− before, err := fs.ReadFile(os.DirFS(baseDir), fileName)

173− require.NoError(t, err)

174−

175− // Get file ModTime to verify no write occurs

176− stBefore, err := fs.Stat(os.DirFS(baseDir), fileName)

177− require.NoError(t, err)

178− beforeMod := stBefore.ModTime()

179−

180− // Re-record same entry should be a no-op (no write)

181− require.NoError(t, store.recordRecentModel(ScopeGlobal, SelectedModelTypeLarge, entry))

107+ _, changed := nextRecentModels(cfg, SelectedModelTypeLarge, entry)

182−

108+ require.False(t, changed)

183− after, err := fs.ReadFile(os.DirFS(baseDir), fileName)

184− require.NoError(t, err)

185− require.Equal(t, string(before), string(after))

186−

187− // Verify ModTime unchanged to ensure truly no write occurred

188− stAfter, err := fs.Stat(os.DirFS(baseDir), fileName)

189− require.NoError(t, err)

190− require.True(t, stAfter.ModTime().Equal(beforeMod), "file ModTime should not change on noop")

191}

109}

192

110

193−func TestUpdatePreferredModel_UpdatesRecents(t *testing.T) {

111+func TestUpdatePreferredModel_PersistsModelAndRecents(t *testing.T) {

194 t.Parallel()

112 t.Parallel()

195

113

196 dir := t.TempDir()

114 dir := t.TempDir()

@@ -199,20 +117,24 @@ func TestUpdatePreferredModel_UpdatesRecents(t *testing.T) {

199 store := testStoreWithPath(cfg, dir)

117 store := testStoreWithPath(cfg, dir)

200

118

201 sel := SelectedModel{Provider: "openai", Model: "gpt-4o"}

119 sel := SelectedModel{Provider: "openai", Model: "gpt-4o"}

202− require.NoError(t, store.UpdatePreferredModel(ScopeGlobal, SelectedModelTypeSmall, sel))

120+ require.NoError(t, store.UpdatePreferredModel(ScopeGlobal, SelectedModelTypeLarge, sel))

203

121

204− // in-memory

122+ // in-memory state (read through the store; copy-on-write publishes a

123+ // new Config, so the seed cfg pointer is intentionally unchanged).

205− require.Equal(t, sel, cfg.Models[SelectedModelTypeSmall])

124+ require.Equal(t, sel, store.Config().Models[SelectedModelTypeLarge])

206− require.Len(t, cfg.RecentModels[SelectedModelTypeSmall], 1)

125+ require.Len(t, store.Config().RecentModels[SelectedModelTypeLarge], 1)

207

126

208− // persisted (read via fs.FS)

127+ // persisted state

209 rm := readRecentModels(t, store.globalDataPath)

128 rm := readRecentModels(t, store.globalDataPath)

210− small, ok := rm[string(SelectedModelTypeSmall)].([]any)

129+ large, ok := rm[string(SelectedModelTypeLarge)].([]any)

211 require.True(t, ok)

130 require.True(t, ok)

212− require.Len(t, small, 1)

131+ require.Len(t, large, 1)

132+ item := large[0].(map[string]any)

133+ require.Equal(t, "openai", item["provider"])

134+ require.Equal(t, "gpt-4o", item["model"])

213}

135}

214

136

215−func TestRecordRecentModel_TypeIsolation(t *testing.T) {

137+func TestUpdatePreferredModel_TypeIsolation(t *testing.T) {

216 t.Parallel()

138 t.Parallel()

217

139

218 dir := t.TempDir()

140 dir := t.TempDir()

@@ -220,42 +142,17 @@ func TestRecordRecentModel_TypeIsolation(t *testing.T) {

220 cfg.setDefaults(dir, "")

142 cfg.setDefaults(dir, "")

221 store := testStoreWithPath(cfg, dir)

143 store := testStoreWithPath(cfg, dir)

222

144

223− // Add models to both large and small types

224 largeModel := SelectedModel{Provider: "openai", Model: "gpt-4o"}

145 largeModel := SelectedModel{Provider: "openai", Model: "gpt-4o"}

225 smallModel := SelectedModel{Provider: "anthropic", Model: "claude"}

146 smallModel := SelectedModel{Provider: "anthropic", Model: "claude"}

147+ require.NoError(t, store.UpdatePreferredModel(ScopeGlobal, SelectedModelTypeLarge, largeModel))

148+ require.NoError(t, store.UpdatePreferredModel(ScopeGlobal, SelectedModelTypeSmall, smallModel))

226

149

227− require.NoError(t, store.recordRecentModel(ScopeGlobal, SelectedModelTypeLarge, largeModel))

150+ // Adding to large leaves small untouched.

228− require.NoError(t, store.recordRecentModel(ScopeGlobal, SelectedModelTypeSmall, smallModel))

229−

230− // in-memory: verify types maintain separate histories

231− require.Len(t, cfg.RecentModels[SelectedModelTypeLarge], 1)

232− require.Len(t, cfg.RecentModels[SelectedModelTypeSmall], 1)

233− require.Equal(t, largeModel, cfg.RecentModels[SelectedModelTypeLarge][0])

234− require.Equal(t, smallModel, cfg.RecentModels[SelectedModelTypeSmall][0])

235−

236− // Add another to large, verify small unchanged

237 anotherLarge := SelectedModel{Provider: "google", Model: "gemini"}

151 anotherLarge := SelectedModel{Provider: "google", Model: "gemini"}

238− require.NoError(t, store.recordRecentModel(ScopeGlobal, SelectedModelTypeLarge, anotherLarge))

152+ require.NoError(t, store.UpdatePreferredModel(ScopeGlobal, SelectedModelTypeLarge, anotherLarge))

239−

240− require.Len(t, cfg.RecentModels[SelectedModelTypeLarge], 2)

241− require.Len(t, cfg.RecentModels[SelectedModelTypeSmall], 1)

242− require.Equal(t, smallModel, cfg.RecentModels[SelectedModelTypeSmall][0])

243−

244− // persisted state: verify both types exist with correct lengths and contents

245− rm := readRecentModels(t, store.globalDataPath)

246

153

247− large, ok := rm[string(SelectedModelTypeLarge)].([]any)

154+ require.Len(t, store.Config().RecentModels[SelectedModelTypeLarge], 2)

248− require.True(t, ok)

155+ require.Equal(t, anotherLarge, store.Config().RecentModels[SelectedModelTypeLarge][0])

249− require.Len(t, large, 2)

250− // Verify newest first for large type

251− require.Equal(t, "google", large[0].(map[string]any)["provider"])

252− require.Equal(t, "gemini", large[0].(map[string]any)["model"])

253− require.Equal(t, "openai", large[1].(map[string]any)["provider"])

254− require.Equal(t, "gpt-4o", large[1].(map[string]any)["model"])

255−

256− small, ok := rm[string(SelectedModelTypeSmall)].([]any)

156+ require.Len(t, store.Config().RecentModels[SelectedModelTypeSmall], 1)

257− require.True(t, ok)

157+ require.Equal(t, smallModel, store.Config().RecentModels[SelectedModelTypeSmall][0])

258− require.Len(t, small, 1)

259− require.Equal(t, "anthropic", small[0].(map[string]any)["provider"])

260− require.Equal(t, "claude", small[0].(map[string]any)["model"])

261}

158}

internal/config/scopeb_race_test.gonew file+74

@@ -0,0 +1,74 @@

1+package config

3+import (

4+ "os"

5+ "path/filepath"

6+ "sync"

7+ "testing"

8+)

10+// TestScopeB_InPlaceMutationRace probes whether the store's in-place field

11+// mutators (e.g. SetCompactMode) race with concurrent Config() readers that

12+// walk the same field. This is the "Scope B" race left open after the

13+// pointer-swap fix. Run with -race.

14+// TestScopeB_InPlaceMutationRace verifies that the store's typed field

15+// mutators (e.g. SetCompactMode) no longer race concurrent Config()

16+// readers walking the same field. Copy-on-write publishing means the

17+// mutator swaps in a fresh Config rather than writing through the live

18+// pointer, so a reader always sees an immutable snapshot. Run with -race.

19+func TestScopeB_InPlaceMutationRace(t *testing.T) {

20+ dir := t.TempDir()

21+ configPath := filepath.Join(dir, "crush.json")

22+

23+ t.Setenv("CRUSH_GLOBAL_CONFIG", dir)

24+ t.Setenv("CRUSH_GLOBAL_DATA", dir)

25+ resetProviderState()

26+ t.Cleanup(resetProviderState)

27+

28+ cfg := `{

29+ "models": {"large": {"provider": "openai", "model": "gpt-4"}},

30+ "providers": {"openai": {"api_key": "k", "models": [{"id": "gpt-4", "name": "GPT-4"}]}},

31+ "options": {"tui": {"compact_mode": false}}

32+ }`

33+ if err := os.WriteFile(configPath, []byte(cfg), 0o600); err != nil {

34+ t.Fatal(err)

35+ }

36+

37+ store, err := Load(dir, dir, false)

38+ if err != nil {

39+ t.Fatal(err)

40+ }

41+ store.globalDataPath = configPath

42+

43+ var wg sync.WaitGroup

44+ stop := make(chan struct{})

45+

46+ // Reader: walks Options.TUI.CompactMode, the field SetCompactMode writes.

47+ wg.Add(1)

48+ go func() {

49+ defer wg.Done()

50+ for {

51+ select {

52+ case <-stop:

53+ return

54+ default:

55+ c := store.Config()

56+ if c != nil && c.Options != nil {

57+ _ = c.Options.TUI.CompactMode

58+ }

59+ }

60+ }

61+ }()

62+

63+ // Writer: flips compact mode, mutating Options in place.

64+ wg.Add(1)

65+ go func() {

66+ defer wg.Done()

67+ for i := 0; i < 50; i++ {

68+ _ = store.SetCompactMode(ScopeGlobal, i%2 == 0)

69+ }

70+ close(stop)

71+ }()

72+

73+ wg.Wait()

74+}

internal/config/store.go+163−118

@@ -51,9 +51,14 @@ type RuntimeOverrides struct {

51// goroutine races and, together with the shared lock.File, cross-process

52// races on the config file.

53//

54−// reloadMu serialises ReloadFromDisk calls to prevent concurrent reloads

54+// writeMu serialises every operation that produces a new in-memory Config:

55−// from racing on store fields. autoReload uses TryLock on reloadMu to

55+// the typed copy-on-write mutators (SetCompactMode, UpdatePreferredModel,

56−// skip redundant reloads when one is already in progress.

56+// ...) and ReloadFromDisk. Typed mutators take Lock; autoReload takes

57+// TryLock so a write triggered re-entrantly during a reload (e.g.

58+// configureProviders calling RemoveConfigField) skips the nested reload

59+// instead of deadlocking. This is what lets published Configs be treated

60+// as immutable: a mutator clones, mutates the clone, and swaps it in under

61+// writeMu rather than mutating the live Config in place.

57type ConfigStore struct {

62type ConfigStore struct {

58 config *Config

63 config *Config

59 workingDir string

64 workingDir string

@@ -66,15 +71,39 @@ type ConfigStore struct {

66 trackedConfigPaths []string // unique, normalized config file paths

71 trackedConfigPaths []string // unique, normalized config file paths

67 snapshots map[string]fileSnapshot // path -> snapshot at last capture

72 snapshots map[string]fileSnapshot // path -> snapshot at last capture

74+ // configMu guards the config pointer field against concurrent

75+ // readers (Config) and the writeMu-serialised swap (setConfig). It

76+ // protects the pointer word only; the pointed-to Config is treated

77+ // as immutable once published, since both reloads and typed mutators

78+ // build a fresh Config rather than mutating the live one.

79+ configMu sync.RWMutex

80+

69− mu sync.Mutex // serialises config file writes

81+ mu sync.Mutex // serialises config file writes

70− reloadMu sync.Mutex // serialises ReloadFromDisk calls

82+ writeMu sync.Mutex // serialises in-memory config production (mutators + reload)

71}

83}

73// Config returns the pure-data config struct (read-only after load).

85// Config returns the pure-data config struct (read-only after load).

86+//

87+// The pointer read is guarded by configMu so it can never tear against

88+// the reload swap in reloadFromDiskLocked. Reloads build a brand-new

89+// Config and swap it in rather than mutating the live one, so holding the

90+// returned pointer stays safe even across a concurrent reload — the reader

91+// keeps reading its (now immutable) snapshot.

74func (s *ConfigStore) Config() *Config {

92func (s *ConfigStore) Config() *Config {

93+ s.configMu.RLock()

94+ defer s.configMu.RUnlock()

75 return s.config

95 return s.config

76}

96}

98+// setConfig atomically swaps the active config pointer under configMu.

99+// Used by the reload path; in-place field mutators leave the pointer

100+// untouched and run under mu instead.

101+func (s *ConfigStore) setConfig(cfg *Config) {

102+ s.configMu.Lock()

103+ defer s.configMu.Unlock()

104+ s.config = cfg

105+}

106+

78// WorkingDir returns the current working directory.

107// WorkingDir returns the current working directory.

79func (s *ConfigStore) WorkingDir() string {

108func (s *ConfigStore) WorkingDir() string {

80 return s.workingDir

109 return s.workingDir

@@ -100,7 +129,7 @@ func (s *ConfigStore) KnownProviders() []catwalk.Provider {

100

129

101// SetupAgents configures the coder and task agents on the config.

130// SetupAgents configures the coder and task agents on the config.

102func (s *ConfigStore) SetupAgents() {

131func (s *ConfigStore) SetupAgents() {

103− s.config.SetupAgents()

132+ s.Config().SetupAgents()

104}

133}

105

134

106// Overrides returns the runtime overrides for this store.

135// Overrides returns the runtime overrides for this store.

@@ -212,25 +241,36 @@ func (s *ConfigStore) SetConfigField(scope Scope, key string, value any) error {

212 return s.SetConfigFields(scope, map[string]any{key: value})

241 return s.SetConfigFields(scope, map[string]any{key: value})

213}

242}

214

243

215−// SetConfigFields sets multiple key/value pairs in the config file for the given

244+// SetConfigFields sets multiple key/value pairs in the config file for the

216−// scope in a single write. After a successful write, it automatically reloads

245+// given scope in a single write, then reloads in-memory state from disk.

217−// config to keep in-memory state fresh. This is preferred over multiple

246+//

218−// SetConfigField calls when writing several fields atomically to avoid

247+// Use this for arbitrary external edits where the in-memory effect of the

219−// intermediate reloads with partial state.

248+// change is not known ahead of time. The typed mutators (which know exactly

249+// what changed) go through update instead and skip the reload.

220//

250//

221// The write is protected by an in-process mutex and a cross-process flock

251// The write is protected by an in-process mutex and a cross-process flock

222// to prevent races between concurrent writers in different processes.

252// to prevent races between concurrent writers in different processes.

223func (s *ConfigStore) SetConfigFields(scope Scope, kv map[string]any) error {

253func (s *ConfigStore) SetConfigFields(scope Scope, kv map[string]any) error {

224− return s.setConfigFields(scope, kv, true)

254+ if err := s.writeConfigFields(scope, kv); err != nil {

255+ return err

256+ }

257+ // Auto-reload to keep in-memory state fresh after config edits.

258+ // We use context.Background() since this is an internal operation that

259+ // shouldn't be cancelled by user context.

260+ if err := s.autoReload(context.Background()); err != nil {

261+ // Log warning but don't fail the write - disk is already updated.

262+ slog.Warn("Config file updated but failed to reload in-memory state", "error", err)

263+ }

264+ return nil

225}

265}

226

266

227−// setConfigFields performs the write and, when reload is true, refreshes

267+// writeConfigFields persists key/value pairs to the config file. It does not

228−// in-memory state from disk. Callers that have already updated the relevant

268+// touch in-memory config state or the staleness snapshot: callers either

229−// in-memory state themselves can pass reload=false to skip the expensive

269+// reload (SetConfigFields, whose reload recaptures the snapshot) or have

230−// full reparse (which rebuilds the provider catalog and agents). The disk

270+// already published an updated clone and capture the snapshot themselves

231−// is the source of truth for other processes either way; skipping reload

271+// (update). Both of those run under writeMu, which is what keeps the

232−// only avoids re-deriving state this process already holds.

272+// snapshot map free of concurrent writers.

233−func (s *ConfigStore) setConfigFields(scope Scope, kv map[string]any, reload bool) error {

273+func (s *ConfigStore) writeConfigFields(scope Scope, kv map[string]any) error {

234 // Sort keys for deterministic output regardless of map iteration

274 // Sort keys for deterministic output regardless of map iteration

235 // order. This also ensures consistent results when callers pass

275 // order. This also ensures consistent results when callers pass

236 // overlapping JSONPath keys (e.g. "a" and "a.b").

276 // overlapping JSONPath keys (e.g. "a" and "a.b").

@@ -240,42 +280,71 @@ func (s *ConfigStore) setConfigFields(scope Scope, kv map[string]any, reload boo

240 }

280 }

241 slices.Sort(keys)

281 slices.Sort(keys)

242

282

243− err := s.atomicWrite(scope, func(data []byte) ([]byte, error) {

283+ return s.atomicWrite(scope, func(data []byte) ([]byte, error) {

244 v := string(data)

284 v := string(data)

245 for _, key := range keys {

285 for _, key := range keys {

246 var sErr error

286 var sErr error

247− v, sErr = sjson.Set(v, key, kv[key])

287+ if v, sErr = sjson.Set(v, key, kv[key]); sErr != nil {

248− if sErr != nil {

249 return nil, fmt.Errorf("failed to set config field %s: %w", key, sErr)

288 return nil, fmt.Errorf("failed to set config field %s: %w", key, sErr)

250 }

289 }

251 }

290 }

252 return []byte(v), nil

291 return []byte(v), nil

253 })

292 })

254− if err != nil {

255− return err

256− }

293+}

257

294

258− if !reload {

295+// mutateInMemory applies a copy-on-write change to the config without

259− // The caller has already updated in-memory state; just refresh

296+// persisting. Under writeMu it clones the live config, lets mutate edit the

260− // the staleness snapshot so the file watcher does not treat our

297+// clone, and publishes it. This is the single primitive every in-memory

261− // own write as an external change.

298+// config change goes through, so a published Config is never mutated in

262− if path, perr := s.configPath(scope); perr == nil {

299+// place and readers always see a consistent snapshot.

263− s.captureStalenessSnapshot(append(slices.Clone(s.loadedPaths), path))

300+func (s *ConfigStore) mutateInMemory(mutate func(*Config)) {

264− }

301+ s.writeMu.Lock()

302+ defer s.writeMu.Unlock()

303+

304+ nc := s.Config().cloneForWrite()

305+ mutate(nc)

306+ s.setConfig(nc)

307+}

308+

309+// update applies a copy-on-write change and persists the reported fields.

310+// mutate edits the clone and returns the JSON-path fields to write to disk;

311+// because the clone already reflects the change, no reload is needed.

312+// Returning an empty map publishes the clone without a disk write.

313+func (s *ConfigStore) update(scope Scope, mutate func(*Config) map[string]any) error {

314+ s.writeMu.Lock()

315+ defer s.writeMu.Unlock()

316+

317+ nc := s.Config().cloneForWrite()

318+ fields := mutate(nc)

319+ s.setConfig(nc)

320+ if len(fields) == 0 {

265 return nil

321 return nil

266 }

322 }

267−

323+ if err := s.writeConfigFields(scope, fields); err != nil {

268− // Auto-reload to keep in-memory state fresh after config edits.

324+ return err

269− // We use context.Background() since this is an internal operation that

325+ }

270− // shouldn't be cancelled by user context.

326+ // Refresh the staleness snapshot so the file watcher does not treat

271− if err := s.autoReload(context.Background()); err != nil {

327+ // our own write as an external change. Safe to touch the snapshot map

272− // Log warning but don't fail the write - disk is already updated.

328+ // here because we hold writeMu.

273− slog.Warn("Config file updated but failed to reload in-memory state", "error", err)

329+ if path, err := s.configPath(scope); err == nil {

330+ s.captureStalenessSnapshot(append(slices.Clone(s.loadedPaths), path))

274 }

331 }

275−

276 return nil

332 return nil

277}

333}

278

334

335+// OverridePreferredModel sets the preferred model for the given type in

336+// memory only, without persisting. It is for per-run overrides (such as the

337+// non-interactive --model flags) that must not be written to the user's

338+// config file.

339+func (s *ConfigStore) OverridePreferredModel(modelType SelectedModelType, model SelectedModel) {

340+ s.mutateInMemory(func(c *Config) {

341+ if c.Models == nil {

342+ c.Models = make(map[SelectedModelType]SelectedModel)

343+ }

344+ c.Models[modelType] = model

345+ })

346+}

347+

279// RemoveConfigField removes a key from the config file for the given scope.

348// RemoveConfigField removes a key from the config file for the given scope.

280// After a successful write, it automatically reloads config to keep in-memory

349// After a successful write, it automatically reloads config to keep in-memory

281// state fresh.

350// state fresh.

@@ -304,47 +373,45 @@ func (s *ConfigStore) RemoveConfigField(scope Scope, key string) error {

304// persists it to the config file at the given scope. The selected model and

373// persists it to the config file at the given scope. The selected model and

305// the recent-models list are written together in a single config write.

374// the recent-models list are written together in a single config write.

306//

375//

307−// The in-memory config is updated directly here, so the write skips the

308−// full disk reparse/reload. The reload would otherwise rebuild the entire

376+// The write skips the full disk reparse/reload (which would rebuild the

309−// provider catalog and agents on every model switch, which dominates the

377+// provider catalog and agents on every model switch and dominate selection

310−// latency of selecting a model. Agents are refreshed separately by the

378+// latency); agents are refreshed separately by the caller (see

311−// caller (see UpdateAgentModel).

379+// UpdateAgentModel).

312func (s *ConfigStore) UpdatePreferredModel(scope Scope, modelType SelectedModelType, model SelectedModel) error {

380func (s *ConfigStore) UpdatePreferredModel(scope Scope, modelType SelectedModelType, model SelectedModel) error {

313− s.config.Models[modelType] = model

314−

315− fields := map[string]any{

381+ return s.update(scope, func(c *Config) map[string]any {

316− fmt.Sprintf("models.%s", modelType): model,

382+ if c.Models == nil {

317− }

383+ c.Models = make(map[SelectedModelType]SelectedModel)

318−

384+ }

319− // Compute the updated recent-models list in memory. When it changes,

385+ c.Models[modelType] = model

320− // fold the write into the same batch so we avoid a second write.

321− if updated, changed := s.nextRecentModels(modelType, model); changed {

322− s.config.RecentModels[modelType] = updated

323− fields[fmt.Sprintf("recent_models.%s", modelType)] = updated

324− }

325

386

326− if err := s.setConfigFields(scope, fields, false); err != nil {

387+ fields := map[string]any{

327− return fmt.Errorf("failed to update preferred model: %w", err)

388+ fmt.Sprintf("models.%s", modelType): model,

389+ }

390+ if updated, changed := nextRecentModels(c, modelType, model); changed {

391+ if c.RecentModels == nil {

392+ c.RecentModels = make(map[SelectedModelType][]SelectedModel)

393+ }

394+ c.RecentModels[modelType] = updated

395+ fields[fmt.Sprintf("recent_models.%s", modelType)] = updated

328− }

396+ }

329− return nil

397+ return fields

398+ })

330}

399}

331

400

332// SetCompactMode sets the compact mode setting and persists it.

401// SetCompactMode sets the compact mode setting and persists it.

333func (s *ConfigStore) SetCompactMode(scope Scope, enabled bool) error {

402func (s *ConfigStore) SetCompactMode(scope Scope, enabled bool) error {

334− if s.config.Options == nil {

403+ return s.update(scope, func(c *Config) map[string]any {

335− s.config.Options = &Options{}

404+ c.ensureTUI().CompactMode = enabled

336− }

337− s.config.Options.TUI.CompactMode = enabled

338− return s.SetConfigField(scope, "options.tui.compact_mode", enabled)

405+ return map[string]any{"options.tui.compact_mode": enabled}

406+ })

339}

407}

340

408

341// SetTransparentBackground sets the transparent background setting and persists it.

409// SetTransparentBackground sets the transparent background setting and persists it.

342func (s *ConfigStore) SetTransparentBackground(scope Scope, enabled bool) error {

410func (s *ConfigStore) SetTransparentBackground(scope Scope, enabled bool) error {

343− if s.config.Options == nil {

411+ return s.update(scope, func(c *Config) map[string]any {

344− s.config.Options = &Options{}

412+ c.ensureTUI().Transparent = &enabled

345− }

346− s.config.Options.TUI.Transparent = &enabled

347− return s.SetConfigField(scope, "options.tui.transparent", enabled)

413+ return map[string]any{"options.tui.transparent": enabled}

414+ })

348}

415}

349

416

350// SetProviderAPIKey sets the API key for a provider and persists it.

417// SetProviderAPIKey sets the API key for a provider and persists it.

@@ -376,10 +443,11 @@ func (s *ConfigStore) SetProviderAPIKey(scope Scope, providerID string, apiKey a

376 }

443 }

377 }

444 }

378

445

446+ cfg := s.Config()

379− providerConfig, exists = s.config.Providers.Get(providerID)

447+ providerConfig, exists = cfg.Providers.Get(providerID)

380 if exists {

448 if exists {

381 setKeyOrToken()

449 setKeyOrToken()

382− s.config.Providers.Set(providerID, providerConfig)

450+ cfg.Providers.Set(providerID, providerConfig)

383 return nil

451 return nil

384 }

452 }

385

453

@@ -406,7 +474,7 @@ func (s *ConfigStore) SetProviderAPIKey(scope Scope, providerID string, apiKey a

406 } else {

474 } else {

407 return fmt.Errorf("provider with ID %s not found in known providers", providerID)

475 return fmt.Errorf("provider with ID %s not found in known providers", providerID)

408 }

476 }

409− s.config.Providers.Set(providerID, providerConfig)

477+ cfg.Providers.Set(providerID, providerConfig)

410 return nil

478 return nil

411}

479}

412

480

@@ -420,7 +488,8 @@ func (s *ConfigStore) SetProviderAPIKey(scope Scope, providerID string, apiKey a

420// already rotated the refresh token — the disk is re-checked under lock

488// already rotated the refresh token — the disk is re-checked under lock

421// to recover the other process's token.

489// to recover the other process's token.

422func (s *ConfigStore) RefreshOAuthToken(ctx context.Context, scope Scope, providerID string) error {

490func (s *ConfigStore) RefreshOAuthToken(ctx context.Context, scope Scope, providerID string) error {

491+ cfg := s.Config()

423− providerConfig, exists := s.config.Providers.Get(providerID)

492+ providerConfig, exists := cfg.Providers.Get(providerID)

424 if !exists {

493 if !exists {

425 return fmt.Errorf("provider %s not found", providerID)

494 return fmt.Errorf("provider %s not found", providerID)

426 }

495 }

@@ -482,7 +551,7 @@ func (s *ConfigStore) RefreshOAuthToken(ctx context.Context, scope Scope, provid

482 providerConfig.SetupGitHubCopilot()

551 providerConfig.SetupGitHubCopilot()

483 }

552 }

484

553

485− s.config.Providers.Set(providerID, providerConfig)

554+ cfg.Providers.Set(providerID, providerConfig)

486

555

487 if err := s.SetConfigFields(scope, map[string]any{

556 if err := s.SetConfigFields(scope, map[string]any{

488 fmt.Sprintf("providers.%s.api_key", providerID): refreshedToken.AccessToken,

557 fmt.Sprintf("providers.%s.api_key", providerID): refreshedToken.AccessToken,

@@ -501,7 +570,7 @@ func (s *ConfigStore) applyToken(providerConfig ProviderConfig, token *oauth.Tok

501 if providerID == string(catwalk.InferenceProviderCopilot) {

570 if providerID == string(catwalk.InferenceProviderCopilot) {

502 providerConfig.SetupGitHubCopilot()

571 providerConfig.SetupGitHubCopilot()

503 }

572 }

504− s.config.Providers.Set(providerID, providerConfig)

573+ s.Config().Providers.Set(providerID, providerConfig)

505 return nil

574 return nil

506}

575}

507

576

@@ -541,20 +610,15 @@ func (s *ConfigStore) loadTokenFromDisk(scope Scope, providerID string) (*oauth.

541}

610}

542

611

543// nextRecentModels computes the recent-models list for the given type

612// nextRecentModels computes the recent-models list for the given type

544−// after recording the supplied model at the front, without persisting

613+// after recording the supplied model at the front, operating on the

545−// anything. It returns the new slice and whether it differs from the

614+// provided config without persisting anything. It returns the new slice

546−// current list. It also lazily initializes the RecentModels map. Callers

615+// and whether it differs from cfg's current list. Callers fold the result

547−// that want to persist the change can fold the result into a batched

616+// into a clone they are about to publish.

548−// config write; recordRecentModel wraps it for the standalone case.

549−func (s *ConfigStore) nextRecentModels(modelType SelectedModelType, model SelectedModel) ([]SelectedModel, bool) {

617+func nextRecentModels(cfg *Config, modelType SelectedModelType, model SelectedModel) ([]SelectedModel, bool) {

550 if model.Provider == "" || model.Model == "" {

618 if model.Provider == "" || model.Model == "" {

551 return nil, false

619 return nil, false

552 }

620 }

553

621

554− if s.config.RecentModels == nil {

555− s.config.RecentModels = make(map[SelectedModelType][]SelectedModel)

556− }

557−

558 eq := func(a, b SelectedModel) bool {

622 eq := func(a, b SelectedModel) bool {

559 return a.Provider == b.Provider && a.Model == b.Model

623 return a.Provider == b.Provider && a.Model == b.Model

560 }

624 }

@@ -564,7 +628,7 @@ func (s *ConfigStore) nextRecentModels(modelType SelectedModelType, model Select

564 Model: model.Model,

628 Model: model.Model,

565 }

629 }

566

630

567− current := s.config.RecentModels[modelType]

631+ current := cfg.RecentModels[modelType]

568 withoutCurrent := slices.DeleteFunc(slices.Clone(current), func(existing SelectedModel) bool {

632 withoutCurrent := slices.DeleteFunc(slices.Clone(current), func(existing SelectedModel) bool {

569 return eq(existing, entry)

633 return eq(existing, entry)

570 })

634 })

@@ -581,25 +645,6 @@ func (s *ConfigStore) nextRecentModels(modelType SelectedModelType, model Select

581 return updated, true

645 return updated, true

582}

646}

583

647

584−// recordRecentModel records a model in the recent models list and persists

585−// the change. It is retained for callers that update recent models in

586−// isolation; UpdatePreferredModel batches the same change into a single

587−// config write instead.

588−func (s *ConfigStore) recordRecentModel(scope Scope, modelType SelectedModelType, model SelectedModel) error {

589− updated, changed := s.nextRecentModels(modelType, model)

590− if !changed {

591− return nil

592− }

593−

594− s.config.RecentModels[modelType] = updated

595−

596− if err := s.SetConfigField(scope, fmt.Sprintf("recent_models.%s", modelType), updated); err != nil {

597− return fmt.Errorf("failed to persist recent models: %w", err)

598− }

599−

600− return nil

601−}

602−

603// NewTestStore creates a ConfigStore for testing purposes.

648// NewTestStore creates a ConfigStore for testing purposes.

604func NewTestStore(cfg *Config, loadedPaths ...string) *ConfigStore {

649func NewTestStore(cfg *Config, loadedPaths ...string) *ConfigStore {

605 return &ConfigStore{

650 return &ConfigStore{

@@ -777,17 +822,17 @@ func (s *ConfigStore) captureStalenessSnapshot(paths []string) {

777// ReloadFromDisk re-runs the config load/merge flow and updates the in-memory

822// ReloadFromDisk re-runs the config load/merge flow and updates the in-memory

778// config atomically. It rebuilds the staleness snapshot after successful reload.

823// config atomically. It rebuilds the staleness snapshot after successful reload.

779// On failure, the store state is rolled back to its previous state.

824// On failure, the store state is rolled back to its previous state.

780−// Concurrent calls are serialised via reloadMu.

825+// Concurrent calls are serialised via writeMu.

781func (s *ConfigStore) ReloadFromDisk(ctx context.Context) error {

826func (s *ConfigStore) ReloadFromDisk(ctx context.Context) error {

782 if s.workingDir == "" {

827 if s.workingDir == "" {

783 return fmt.Errorf("cannot reload: working directory not set")

828 return fmt.Errorf("cannot reload: working directory not set")

784 }

829 }

785− s.reloadMu.Lock()

830+ s.writeMu.Lock()

786− defer s.reloadMu.Unlock()

831+ defer s.writeMu.Unlock()

787 return s.reloadFromDiskLocked(ctx)

832 return s.reloadFromDiskLocked(ctx)

788}

833}

789

834

790−// reloadFromDiskLocked performs the actual reload. Caller must hold reloadMu.

835+// reloadFromDiskLocked performs the actual reload. Caller must hold writeMu.

791func (s *ConfigStore) reloadFromDiskLocked(ctx context.Context) error {

836func (s *ConfigStore) reloadFromDiskLocked(ctx context.Context) error {

792 // Migrate deprecated disable_notifications before reloading config.

837 // Migrate deprecated disable_notifications before reloading config.

793 migrateDisableNotifications()

838 migrateDisableNotifications()

@@ -800,8 +845,8 @@ func (s *ConfigStore) reloadFromDiskLocked(ctx context.Context) error {

800

845

801 // Apply defaults (using existing data directory if set)

846 // Apply defaults (using existing data directory if set)

802 var dataDir string

847 var dataDir string

803− if s.config != nil && s.config.Options != nil {

848+ if cur := s.Config(); cur != nil && cur.Options != nil {

804− dataDir = s.config.Options.DataDirectory

849+ dataDir = cur.Options.DataDirectory

805 }

850 }

806 cfg.setDefaults(s.workingDir, dataDir)

851 cfg.setDefaults(s.workingDir, dataDir)

807

852

@@ -842,7 +887,7 @@ func (s *ConfigStore) reloadFromDiskLocked(ctx context.Context) error {

842 }

887 }

843

888

844 // Save current state for potential rollback

889 // Save current state for potential rollback

845− oldConfig := s.config

890+ oldConfig := s.Config()

846 oldLoadedPaths := s.loadedPaths

891 oldLoadedPaths := s.loadedPaths

847 oldResolver := s.resolver

892 oldResolver := s.resolver

848 oldKnownProviders := s.knownProviders

893 oldKnownProviders := s.knownProviders

@@ -850,7 +895,7 @@ func (s *ConfigStore) reloadFromDiskLocked(ctx context.Context) error {

850 oldWorkspacePath := s.workspacePath

895 oldWorkspacePath := s.workspacePath

851

896

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

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

853− s.config = cfg

898+ s.setConfig(cfg)

854 s.loadedPaths = loadedPaths

899 s.loadedPaths = loadedPaths

855 s.resolver = resolver

900 s.resolver = resolver

856 s.knownProviders = providers

901 s.knownProviders = providers

@@ -871,7 +916,7 @@ func (s *ConfigStore) reloadFromDiskLocked(ctx context.Context) error {

871

916

872 // Rollback on setup failure

917 // Rollback on setup failure

873 if setupErr != nil {

918 if setupErr != nil {

874− s.config = oldConfig

919+ s.setConfig(oldConfig)

875 s.loadedPaths = oldLoadedPaths

920 s.loadedPaths = oldLoadedPaths

876 s.resolver = oldResolver

921 s.resolver = oldResolver

877 s.knownProviders = oldKnownProviders

922 s.knownProviders = oldKnownProviders

@@ -904,9 +949,9 @@ func (s *ConfigStore) autoReload(ctx context.Context) error {

904 // are rare and the next user action or file-watch tick will pick

949 // are rare and the next user action or file-watch tick will pick

905 // up the change. Callers that need guaranteed fresh state after a

950 // up the change. Callers that need guaranteed fresh state after a

906 // write should call ReloadFromDisk explicitly.

951 // write should call ReloadFromDisk explicitly.

907− if !s.reloadMu.TryLock() {

952+ if !s.writeMu.TryLock() {

908 return nil

953 return nil

909 }

954 }

910− defer s.reloadMu.Unlock()

955+ defer s.writeMu.Unlock()

911 return s.reloadFromDiskLocked(ctx)

956 return s.reloadFromDiskLocked(ctx)

912}

957}

internal/config/store_test.go+1−1

@@ -498,7 +498,7 @@ func TestAutoReloadDisabledDuringReload(t *testing.T) {

498 require.NoError(t, os.WriteFile(configPath, []byte(initialConfig), 0o600))

499

500 // Load will trigger configureProviders which removes anthropic OAuth config.

501− // This should NOT cause infinite recursion — reloadMu prevents re-entrant reloads.

501+ // This should NOT cause infinite recursion — writeMu prevents re-entrant reloads.

502 store, err := Load(dir, dir, false)

503 require.NoError(t, err)

504

internal/workspace/app_workspace.go+2−2

@@ -426,13 +426,13 @@ func (w *AppWorkspace) EnableDockerMCP(ctx context.Context) error {

426

427 if err := mcptools.InitializeSingle(ctx, config.DockerMCPName, w.store); err != nil {

428 disableErr := mcptools.DisableSingle(w.store, config.DockerMCPName)

429− delete(w.store.Config().MCP, config.DockerMCPName)

429+ w.store.RemoveDockerMCPInMemory()

430 return fmt.Errorf("failed to start docker MCP: %w", errors.Join(err, disableErr))

431 }

432

433 if err := w.store.PersistDockerMCPConfig(mcpConfig); err != nil {

434 disableErr := mcptools.DisableSingle(w.store, config.DockerMCPName)

435− delete(w.store.Config().MCP, config.DockerMCPName)

435+ w.store.RemoveDockerMCPInMemory()

436 return fmt.Errorf("docker MCP started but failed to persist configuration: %w", errors.Join(err, disableErr))

437 }

438