d4dc84e · crush · git.iko.soy

git.iko.soy/crush

fix(config): prevent startup deadlock when configured model ID is invalid

Separate model resolution from persistence in configureSelectedModels. The old design used a persist bool flag that silently toggled disk writes, but the write path had a hidden precondition (caller must hold writeMu) that was not enforced by the type system. When Load held writeMu and called configureSelectedModels(persist=true) with an invalid model ID, UpdatePreferredModel re-acquired the same mutex, causing a self-deadlock. resolveSelectedModels is now a pure function that returns corrected model selections and fallback flags without touching the store or disk. Callers assign results and persist fallbacks explicitly, making the locking contract visible at each call site. Co-authored-by: vismaytiwari <vismay.t@gmail.com>

commitd4dc84e91471a6c794a037ff5012e42fd070e960 authorKieran Klukas <kieran@dunkirk.sh> dateSat 27 Jun 2026, 15:02 (4 months ago) parent5f8810f

3 files changed+174−49

compare with main

unifiedsplit

internal/config/load.go+47−22 internal/config/load_test.go+95−9 internal/config/store.go+32−18

internal/config/load.go+47−22

@@ -125,9 +125,28 @@ func Load(workingDir, dataDir string, debug bool) (*ConfigStore, error) {

125125 return store, nil

126126 }

127127

128− if err := configureSelectedModels(store, store.knownProviders, true); err != nil {

128+ resolved, err := resolveSelectedModels(cfg, store.knownProviders)

129+ if err != nil {

129130 return nil, fmt.Errorf("failed to configure selected models: %w", err)

130131 }

132+ cfg.Models[SelectedModelTypeLarge] = resolved.Large

133+ cfg.Models[SelectedModelTypeSmall] = resolved.Small

134+

135+ // Persist any fallback corrections while we still hold writeMu.

136+ if resolved.LargeFallback {

137+ if err := store.updateLocked(ScopeGlobal, func(c *Config) map[string]any {

138+ return store.updatePreferredModelFields(c, SelectedModelTypeLarge, resolved.Large)

139+ }); err != nil {

140+ return nil, fmt.Errorf("failed to update preferred large model: %w", err)

141+ }

142+ }

143+ if resolved.SmallFallback {

144+ if err := store.updateLocked(ScopeGlobal, func(c *Config) map[string]any {

145+ return store.updatePreferredModelFields(c, SelectedModelTypeSmall, resolved.Small)

146+ }); err != nil {

147+ return nil, fmt.Errorf("failed to update preferred small model: %w", err)

148+ }

149+ }

131150 store.SetupAgents()

132151

133152 // Capture initial staleness snapshot

@@ -709,15 +728,29 @@ func (c *Config) defaultModelSelection(knownProviders []catwalk.Provider) (large

709728 return largeModel, smallModel, err

710729}

711730

712−func configureSelectedModels(store *ConfigStore, knownProviders []catwalk.Provider, persist bool) error {

713− c := store.config

714− defaultLarge, defaultSmall, err := c.defaultModelSelection(knownProviders)

731+// resolvedModels holds the result of resolving user-configured model

732+// selections against the provider catalog.

733+type resolvedModels struct {

734+ Large SelectedModel

735+ Small SelectedModel

736+ LargeFallback bool // true if Large was corrected to a default

737+ SmallFallback bool // true if Small was corrected to a default

738+}

739+

740+// resolveSelectedModels validates the user's configured model selections

741+// against the provider catalog, falling back to defaults when a model ID is

742+// invalid. It is pure resolution logic: it does not mutate the store or

743+// touch disk. The caller assigns the results to c.Models and persists any

744+// fallback corrections as appropriate.

745+func resolveSelectedModels(cfg *Config, knownProviders []catwalk.Provider) (resolvedModels, error) {

746+ var result resolvedModels

747+ defaultLarge, defaultSmall, err := cfg.defaultModelSelection(knownProviders)

715748 if err != nil {

716− return fmt.Errorf("failed to select default models: %w", err)

749+ return result, fmt.Errorf("failed to select default models: %w", err)

717750 }

718751 large, small := defaultLarge, defaultSmall

719752

720− largeModelSelected, largeModelConfigured := c.Models[SelectedModelTypeLarge]

753+ largeModelSelected, largeModelConfigured := cfg.Models[SelectedModelTypeLarge]

721754 if largeModelConfigured {

722755 if largeModelSelected.Model != "" {

723756 large.Model = largeModelSelected.Model

@@ -725,14 +758,10 @@ func configureSelectedModels(store *ConfigStore, knownProviders []catwalk.Provid

725758 if largeModelSelected.Provider != "" {

726759 large.Provider = largeModelSelected.Provider

727760 }

728− model := c.GetModel(large.Provider, large.Model)

761+ model := cfg.GetModel(large.Provider, large.Model)

729762 if model == nil {

730763 large = defaultLarge

731− if persist {

732− if err := store.UpdatePreferredModel(ScopeGlobal, SelectedModelTypeLarge, large); err != nil {

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

734− }

735− }

764+ result.LargeFallback = true

736765 } else {

737766 if largeModelSelected.MaxTokens > 0 {

738767 large.MaxTokens = largeModelSelected.MaxTokens

@@ -762,7 +791,7 @@ func configureSelectedModels(store *ConfigStore, knownProviders []catwalk.Provid

762791 }

763792 }

764793 }

765− smallModelSelected, smallModelConfigured := c.Models[SelectedModelTypeSmall]

794+ smallModelSelected, smallModelConfigured := cfg.Models[SelectedModelTypeSmall]

766795 if smallModelConfigured {

767796 if smallModelSelected.Model != "" {

768797 small.Model = smallModelSelected.Model

@@ -771,14 +800,10 @@ func configureSelectedModels(store *ConfigStore, knownProviders []catwalk.Provid

771800 small.Provider = smallModelSelected.Provider

772801 }

773802

774− model := c.GetModel(small.Provider, small.Model)

803+ model := cfg.GetModel(small.Provider, small.Model)

775804 if model == nil {

776805 small = defaultSmall

777− if persist {

778− if err := store.UpdatePreferredModel(ScopeGlobal, SelectedModelTypeSmall, small); err != nil {

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

780− }

781− }

806+ result.SmallFallback = true

782807 } else {

783808 if smallModelSelected.MaxTokens > 0 {

784809 small.MaxTokens = smallModelSelected.MaxTokens

@@ -827,9 +852,9 @@ func configureSelectedModels(store *ConfigStore, knownProviders []catwalk.Provid

827852 }

828853 }

829854

830− c.Models[SelectedModelTypeLarge] = large

831− c.Models[SelectedModelTypeSmall] = small

832− return nil

855+ result.Large = large

856+ result.Small = small

857+ return result, nil

833858}

834859

835860// lookupConfigs searches config files starting at cwd and walking up

internal/config/load_test.go+95−9

@@ -10,6 +10,7 @@ import (

1010 "os/exec"

1111 "path/filepath"

1212 "testing"

13+ "time"

1314

1415 "charm.land/catwalk/pkg/catwalk"

1516 "github.com/charmbracelet/crush/internal/csync"

@@ -1687,14 +1688,17 @@ func TestConfig_configureSelectedModels(t *testing.T) {

16871688 err := cfg.configureProviders(context.Background(), store, env, resolver, knownProviders)

16881689 require.NoError(t, err)

16891690

1690− err = configureSelectedModels(store, knownProviders, false)

1691− require.NoError(t, err)

1691+ resolved, resolveErr := resolveSelectedModels(cfg, knownProviders)

1692+ require.NoError(t, resolveErr)

1693+ cfg.Models[SelectedModelTypeLarge] = resolved.Large

1694+ cfg.Models[SelectedModelTypeSmall] = resolved.Small

16921695

16931696 // In-memory falls back to default.

1697+ require.True(t, resolved.LargeFallback)

16941698 require.Equal(t, "openai", cfg.Models[SelectedModelTypeLarge].Provider)

16951699 require.Equal(t, "large-model", cfg.Models[SelectedModelTypeLarge].Model)

16961700

1697− // Disk remains unchanged in reload mode.

1701+ // Disk remains unchanged (resolveSelectedModels never persists).

16981702 data, readErr := os.ReadFile(globalPath)

16991703 require.NoError(t, readErr)

17001704 require.Contains(t, string(data), `"provider":"ghost"`)

@@ -1737,8 +1741,10 @@ func TestConfig_configureSelectedModels(t *testing.T) {

17371741 err := cfg.configureProviders(context.Background(), testStore(cfg), env, resolver, knownProviders)

17381742 require.NoError(t, err)

17391743

1740− err = configureSelectedModels(testStore(cfg), knownProviders, true)

1741− require.NoError(t, err)

1744+ resolved, resolveErr := resolveSelectedModels(cfg, knownProviders)

1745+ require.NoError(t, resolveErr)

1746+ cfg.Models[SelectedModelTypeLarge] = resolved.Large

1747+ cfg.Models[SelectedModelTypeSmall] = resolved.Small

17421748 large := cfg.Models[SelectedModelTypeLarge]

17431749 small := cfg.Models[SelectedModelTypeSmall]

17441750 require.Equal(t, "larger-model", large.Model)

@@ -1799,8 +1805,10 @@ func TestConfig_configureSelectedModels(t *testing.T) {

17991805 err := cfg.configureProviders(context.Background(), testStore(cfg), env, resolver, knownProviders)

18001806 require.NoError(t, err)

18011807

1802− err = configureSelectedModels(testStore(cfg), knownProviders, true)

1803− require.NoError(t, err)

1808+ resolved, resolveErr := resolveSelectedModels(cfg, knownProviders)

1809+ require.NoError(t, resolveErr)

1810+ cfg.Models[SelectedModelTypeLarge] = resolved.Large

1811+ cfg.Models[SelectedModelTypeSmall] = resolved.Small

18041812 large := cfg.Models[SelectedModelTypeLarge]

18051813 small := cfg.Models[SelectedModelTypeSmall]

18061814 require.Equal(t, "large-model", large.Model)

@@ -1844,13 +1852,91 @@ func TestConfig_configureSelectedModels(t *testing.T) {

18441852 err := cfg.configureProviders(context.Background(), testStore(cfg), env, resolver, knownProviders)

18451853 require.NoError(t, err)

18461854

1847− err = configureSelectedModels(testStore(cfg), knownProviders, true)

1848− require.NoError(t, err)

1855+ resolved, resolveErr := resolveSelectedModels(cfg, knownProviders)

1856+ require.NoError(t, resolveErr)

1857+ cfg.Models[SelectedModelTypeLarge] = resolved.Large

1858+ cfg.Models[SelectedModelTypeSmall] = resolved.Small

18491859 large := cfg.Models[SelectedModelTypeLarge]

18501860 require.Equal(t, "large-model", large.Model)

18511861 require.Equal(t, "openai", large.Provider)

18521862 require.Equal(t, int64(100), large.MaxTokens)

18531863 })

1864+ t.Run("resolve and persist fallback under writeMu does not deadlock", func(t *testing.T) {

1865+ dir := t.TempDir()

1866+ globalPath := filepath.Join(dir, "crush.json")

1867+ require.NoError(t, os.WriteFile(globalPath, []byte(`{}`), 0o600))

1868+

1869+ knownProviders := []catwalk.Provider{

1870+ {

1871+ ID: "openai",

1872+ APIKey: "abc",

1873+ DefaultLargeModelID: "large-model",

1874+ DefaultSmallModelID: "small-model",

1875+ Models: []catwalk.Model{

1876+ {ID: "large-model", DefaultMaxTokens: 1000},

1877+ {ID: "small-model", DefaultMaxTokens: 500},

1878+ },

1879+ },

1880+ }

1881+

1882+ cfg := &Config{

1883+ Models: map[SelectedModelType]SelectedModel{

1884+ SelectedModelTypeLarge: {Provider: "openai", Model: "this-model-does-not-exist"},

1885+ SelectedModelTypeSmall: {Provider: "openai", Model: "also-does-not-exist"},

1886+ },

1887+ }

1888+ cfg.setDefaults(dir, "")

1889+ store := &ConfigStore{config: cfg, globalDataPath: globalPath}

1890+ env := env.NewFromMap(map[string]string{})

1891+ resolver := NewShellVariableResolver(env)

1892+ err := cfg.configureProviders(context.Background(), store, env, resolver, knownProviders)

1893+ require.NoError(t, err)

1894+

1895+ // Simulate the Load path: resolve (pure), then persist fallbacks

1896+ // under writeMu using updateLocked. Before the refactor, the

1897+ // combined configureSelectedModels(persist=true) self-deadlocked

1898+ // because UpdatePreferredModel re-acquired writeMu.

1899+ done := make(chan error, 1)

1900+ go func() {

1901+ resolved, resolveErr := resolveSelectedModels(cfg, knownProviders)

1902+ if resolveErr != nil {

1903+ done <- resolveErr

1904+ return

1905+ }

1906+ cfg.Models[SelectedModelTypeLarge] = resolved.Large

1907+ cfg.Models[SelectedModelTypeSmall] = resolved.Small

1908+

1909+ store.writeMu.Lock()

1910+ defer store.writeMu.Unlock()

1911+ if resolved.LargeFallback {

1912+ if err := store.updateLocked(ScopeGlobal, func(c *Config) map[string]any {

1913+ return store.updatePreferredModelFields(c, SelectedModelTypeLarge, resolved.Large)

1914+ }); err != nil {

1915+ done <- err

1916+ return

1917+ }

1918+ }

1919+ if resolved.SmallFallback {

1920+ if err := store.updateLocked(ScopeGlobal, func(c *Config) map[string]any {

1921+ return store.updatePreferredModelFields(c, SelectedModelTypeSmall, resolved.Small)

1922+ }); err != nil {

1923+ done <- err

1924+ return

1925+ }

1926+ }

1927+ done <- nil

1928+ }()

1929+

1930+ select {

1931+ case err := <-done:

1932+ require.NoError(t, err)

1933+ // Should have fallen back to defaults.

1934+ require.Equal(t, "large-model", cfg.Models[SelectedModelTypeLarge].Model)

1935+ require.Equal(t, "small-model", cfg.Models[SelectedModelTypeSmall].Model)

1936+ case <-time.After(5 * time.Second):

1937+ t.Fatal("resolve + persist deadlocked under writeMu")

1938+ }

1939+ })

18541940}

18551941

18561942func TestConfig_configureProviders_HyperAPIKeyFromEnv(t *testing.T) {

internal/config/store.go+32−18

@@ -335,7 +335,11 @@ func (s *ConfigStore) mutateInMemory(mutate func(*Config)) {

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

336336 s.writeMu.Lock()

337337 defer s.writeMu.Unlock()

338+ return s.updateLocked(scope, mutate)

339+}

338340

341+// updateLocked is the lock-free core of update. Caller must hold writeMu.

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

339343 nc := s.Config().cloneForWrite()

340344 fields := mutate(nc)

341345 s.setConfig(nc)

@@ -401,23 +405,30 @@ func (s *ConfigStore) RemoveConfigField(scope Scope, key string) error {

401405// UpdateAgentModel).

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

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

404− if c.Models == nil {

405− c.Models = make(map[SelectedModelType]SelectedModel)

406− }

407− c.Models[modelType] = model

408+ return s.updatePreferredModelFields(c, modelType, model)

409+ })

410+}

408411

409− fields := map[string]any{

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

411− }

412− if updated, changed := nextRecentModels(c, modelType, model); changed {

413− if c.RecentModels == nil {

414− c.RecentModels = make(map[SelectedModelType][]SelectedModel)

415− }

416− c.RecentModels[modelType] = updated

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

412+// updatePreferredModelFields builds the fields map for persisting a preferred

413+// model change. Shared between UpdatePreferredModel and direct updateLocked

414+// callers (e.g. Load).

415+func (s *ConfigStore) updatePreferredModelFields(c *Config, modelType SelectedModelType, model SelectedModel) map[string]any {

416+ if c.Models == nil {

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

418+ }

419+ c.Models[modelType] = model

420+

421+ fields := map[string]any{

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

423+ }

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

425+ if c.RecentModels == nil {

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

418427 }

419− return fields

420− })

428+ c.RecentModels[modelType] = updated

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

430+ }

431+ return fields

421432}

422433

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

@@ -986,14 +997,17 @@ func (s *ConfigStore) reloadFromDiskLocked(ctx context.Context) error {

986997 s.overrides = overrides

987998 s.workspacePath = workspacePath

988999

989− // Mirror startup flow: setup models and agents against NEW config

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

9901001 var setupErr error

9911002 if !cfg.IsConfigured() {

9921003 slog.Warn("No providers configured after reload")

9931004 } else {

994− if err := configureSelectedModels(s, providers, false); err != nil {

995− setupErr = fmt.Errorf("failed to configure selected models during reload: %w", err)

1005+ resolved, resolveErr := resolveSelectedModels(cfg, providers)

1006+ if resolveErr != nil {

1007+ setupErr = fmt.Errorf("failed to configure selected models during reload: %w", resolveErr)

9961008 } else {

1009+ cfg.Models[SelectedModelTypeLarge] = resolved.Large

1010+ cfg.Models[SelectedModelTypeSmall] = resolved.Small

9971011 s.SetupAgents()

9981012 }

9991013 }