git.iko.soy/crush
on main help sign in
fix(config): prevent startup deadlock when configured model ID is invalid
d4dc84eKieran Klukas
internal/config/load.go+47−22
compare with main
unifiedsplit
@@ -125,9 +125,28 @@ func Load(workingDir, dataDir string, debug bool) (*ConfigStore, error) {
125 return store, nil
125 return store, nil
126 }
126 }
127
127
128− if err := configureSelectedModels(store, store.knownProviders, true); err != nil {
128+ resolved, err := resolveSelectedModels(cfg, store.knownProviders)
129+ if err != nil {
129 return nil, fmt.Errorf("failed to configure selected models: %w", err)
130 return nil, fmt.Errorf("failed to configure selected models: %w", err)
130 }
131 }
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+ }
131 store.SetupAgents()
150 store.SetupAgents()
132
151
133 // Capture initial staleness snapshot
152 // Capture initial staleness snapshot
@@ -709,15 +728,29 @@ func (c *Config) defaultModelSelection(knownProviders []catwalk.Provider) (large
709 return largeModel, smallModel, err
728 return largeModel, smallModel, err
710}
729}
711
730
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.
712−func configureSelectedModels(store *ConfigStore, knownProviders []catwalk.Provider, persist bool) error {
745+func resolveSelectedModels(cfg *Config, knownProviders []catwalk.Provider) (resolvedModels, error) {
713− c := store.config
746+ var result resolvedModels
714− defaultLarge, defaultSmall, err := c.defaultModelSelection(knownProviders)
747+ defaultLarge, defaultSmall, err := cfg.defaultModelSelection(knownProviders)
715 if err != nil {
748 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)
717 }
750 }
718 large, small := defaultLarge, defaultSmall
751 large, small := defaultLarge, defaultSmall
719
752
720− largeModelSelected, largeModelConfigured := c.Models[SelectedModelTypeLarge]
753+ largeModelSelected, largeModelConfigured := cfg.Models[SelectedModelTypeLarge]
721 if largeModelConfigured {
754 if largeModelConfigured {
722 if largeModelSelected.Model != "" {
755 if largeModelSelected.Model != "" {
723 large.Model = largeModelSelected.Model
756 large.Model = largeModelSelected.Model
@@ -725,14 +758,10 @@ func configureSelectedModels(store *ConfigStore, knownProviders []catwalk.Provid
725 if largeModelSelected.Provider != "" {
758 if largeModelSelected.Provider != "" {
726 large.Provider = largeModelSelected.Provider
759 large.Provider = largeModelSelected.Provider
727 }
760 }
728− model := c.GetModel(large.Provider, large.Model)
761+ model := cfg.GetModel(large.Provider, large.Model)
729 if model == nil {
762 if model == nil {
730 large = defaultLarge
763 large = defaultLarge
731− if persist {
764+ result.LargeFallback = true
732− if err := store.UpdatePreferredModel(ScopeGlobal, SelectedModelTypeLarge, large); err != nil {
733− return fmt.Errorf("failed to update preferred large model: %w", err)
734− }
735− }
736 } else {
765 } else {
737 if largeModelSelected.MaxTokens > 0 {
766 if largeModelSelected.MaxTokens > 0 {
738 large.MaxTokens = largeModelSelected.MaxTokens
767 large.MaxTokens = largeModelSelected.MaxTokens
@@ -762,7 +791,7 @@ func configureSelectedModels(store *ConfigStore, knownProviders []catwalk.Provid
762 }
791 }
763 }
792 }
764 }
793 }
765− smallModelSelected, smallModelConfigured := c.Models[SelectedModelTypeSmall]
794+ smallModelSelected, smallModelConfigured := cfg.Models[SelectedModelTypeSmall]
766 if smallModelConfigured {
795 if smallModelConfigured {
767 if smallModelSelected.Model != "" {
796 if smallModelSelected.Model != "" {
768 small.Model = smallModelSelected.Model
797 small.Model = smallModelSelected.Model
@@ -771,14 +800,10 @@ func configureSelectedModels(store *ConfigStore, knownProviders []catwalk.Provid
771 small.Provider = smallModelSelected.Provider
800 small.Provider = smallModelSelected.Provider
772 }
801 }
773
802
774− model := c.GetModel(small.Provider, small.Model)
803+ model := cfg.GetModel(small.Provider, small.Model)
775 if model == nil {
804 if model == nil {
776 small = defaultSmall
805 small = defaultSmall
777− if persist {
806+ result.SmallFallback = true
778− if err := store.UpdatePreferredModel(ScopeGlobal, SelectedModelTypeSmall, small); err != nil {
779− return fmt.Errorf("failed to update preferred small model: %w", err)
780− }
781− }
782 } else {
807 } else {
783 if smallModelSelected.MaxTokens > 0 {
808 if smallModelSelected.MaxTokens > 0 {
784 small.MaxTokens = smallModelSelected.MaxTokens
809 small.MaxTokens = smallModelSelected.MaxTokens
@@ -827,9 +852,9 @@ func configureSelectedModels(store *ConfigStore, knownProviders []catwalk.Provid
827 }
852 }
828 }
853 }
829
854
830− c.Models[SelectedModelTypeLarge] = large
855+ result.Large = large
831− c.Models[SelectedModelTypeSmall] = small
856+ result.Small = small
832− return nil
857+ return result, nil
833}
858}
834
859
835// lookupConfigs searches config files starting at cwd and walking up
860// lookupConfigs searches config files starting at cwd and walking up
internal/config/load_test.go+95−9
@@ -10,6 +10,7 @@ import (
10 "os/exec"
10 "os/exec"
11 "path/filepath"
11 "path/filepath"
12 "testing"
12 "testing"
13+ "time"
13
14
14 "charm.land/catwalk/pkg/catwalk"
15 "charm.land/catwalk/pkg/catwalk"
15 "github.com/charmbracelet/crush/internal/csync"
16 "github.com/charmbracelet/crush/internal/csync"
@@ -1687,14 +1688,17 @@ func TestConfig_configureSelectedModels(t *testing.T) {
1687 err := cfg.configureProviders(context.Background(), store, env, resolver, knownProviders)
1688 err := cfg.configureProviders(context.Background(), store, env, resolver, knownProviders)
1688 require.NoError(t, err)
1689 require.NoError(t, err)
1689
1690
1690− err = configureSelectedModels(store, knownProviders, false)
1691+ resolved, resolveErr := resolveSelectedModels(cfg, knownProviders)
1691− require.NoError(t, err)
1692+ require.NoError(t, resolveErr)
1693+ cfg.Models[SelectedModelTypeLarge] = resolved.Large
1694+ cfg.Models[SelectedModelTypeSmall] = resolved.Small
1692
1695
1693 // In-memory falls back to default.
1696 // In-memory falls back to default.
1697+ require.True(t, resolved.LargeFallback)
1694 require.Equal(t, "openai", cfg.Models[SelectedModelTypeLarge].Provider)
1698 require.Equal(t, "openai", cfg.Models[SelectedModelTypeLarge].Provider)
1695 require.Equal(t, "large-model", cfg.Models[SelectedModelTypeLarge].Model)
1699 require.Equal(t, "large-model", cfg.Models[SelectedModelTypeLarge].Model)
1696
1700
1697− // Disk remains unchanged in reload mode.
1701+ // Disk remains unchanged (resolveSelectedModels never persists).
1698 data, readErr := os.ReadFile(globalPath)
1702 data, readErr := os.ReadFile(globalPath)
1699 require.NoError(t, readErr)
1703 require.NoError(t, readErr)
1700 require.Contains(t, string(data), `"provider":"ghost"`)
1704 require.Contains(t, string(data), `"provider":"ghost"`)
@@ -1737,8 +1741,10 @@ func TestConfig_configureSelectedModels(t *testing.T) {
1737 err := cfg.configureProviders(context.Background(), testStore(cfg), env, resolver, knownProviders)
1741 err := cfg.configureProviders(context.Background(), testStore(cfg), env, resolver, knownProviders)
1738 require.NoError(t, err)
1742 require.NoError(t, err)
1739
1743
1740− err = configureSelectedModels(testStore(cfg), knownProviders, true)
1744+ resolved, resolveErr := resolveSelectedModels(cfg, knownProviders)
1741− require.NoError(t, err)
1745+ require.NoError(t, resolveErr)
1746+ cfg.Models[SelectedModelTypeLarge] = resolved.Large
1747+ cfg.Models[SelectedModelTypeSmall] = resolved.Small
1742 large := cfg.Models[SelectedModelTypeLarge]
1748 large := cfg.Models[SelectedModelTypeLarge]
1743 small := cfg.Models[SelectedModelTypeSmall]
1749 small := cfg.Models[SelectedModelTypeSmall]
1744 require.Equal(t, "larger-model", large.Model)
1750 require.Equal(t, "larger-model", large.Model)
@@ -1799,8 +1805,10 @@ func TestConfig_configureSelectedModels(t *testing.T) {
1799 err := cfg.configureProviders(context.Background(), testStore(cfg), env, resolver, knownProviders)
1805 err := cfg.configureProviders(context.Background(), testStore(cfg), env, resolver, knownProviders)
1800 require.NoError(t, err)
1806 require.NoError(t, err)
1801
1807
1802− err = configureSelectedModels(testStore(cfg), knownProviders, true)
1808+ resolved, resolveErr := resolveSelectedModels(cfg, knownProviders)
1803− require.NoError(t, err)
1809+ require.NoError(t, resolveErr)
1810+ cfg.Models[SelectedModelTypeLarge] = resolved.Large
1811+ cfg.Models[SelectedModelTypeSmall] = resolved.Small
1804 large := cfg.Models[SelectedModelTypeLarge]
1812 large := cfg.Models[SelectedModelTypeLarge]
1805 small := cfg.Models[SelectedModelTypeSmall]
1813 small := cfg.Models[SelectedModelTypeSmall]
1806 require.Equal(t, "large-model", large.Model)
1814 require.Equal(t, "large-model", large.Model)
@@ -1844,13 +1852,91 @@ func TestConfig_configureSelectedModels(t *testing.T) {
1844 err := cfg.configureProviders(context.Background(), testStore(cfg), env, resolver, knownProviders)
1852 err := cfg.configureProviders(context.Background(), testStore(cfg), env, resolver, knownProviders)
1845 require.NoError(t, err)
1853 require.NoError(t, err)
1846
1854
1847− err = configureSelectedModels(testStore(cfg), knownProviders, true)
1855+ resolved, resolveErr := resolveSelectedModels(cfg, knownProviders)
1848− require.NoError(t, err)
1856+ require.NoError(t, resolveErr)
1857+ cfg.Models[SelectedModelTypeLarge] = resolved.Large
1858+ cfg.Models[SelectedModelTypeSmall] = resolved.Small
1849 large := cfg.Models[SelectedModelTypeLarge]
1859 large := cfg.Models[SelectedModelTypeLarge]
1850 require.Equal(t, "large-model", large.Model)
1860 require.Equal(t, "large-model", large.Model)
1851 require.Equal(t, "openai", large.Provider)
1861 require.Equal(t, "openai", large.Provider)
1852 require.Equal(t, int64(100), large.MaxTokens)
1862 require.Equal(t, int64(100), large.MaxTokens)
1853 })
1863 })
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+ })
1854}
1940}
1855
1941
1856func TestConfig_configureProviders_HyperAPIKeyFromEnv(t *testing.T) {
1942func TestConfig_configureProviders_HyperAPIKeyFromEnv(t *testing.T) {
internal/config/store.go+32−18
@@ -335,7 +335,11 @@ func (s *ConfigStore) mutateInMemory(mutate func(*Config)) {
335func (s *ConfigStore) update(scope Scope, mutate func(*Config) map[string]any) error {
335func (s *ConfigStore) update(scope Scope, mutate func(*Config) map[string]any) error {
336 s.writeMu.Lock()
336 s.writeMu.Lock()
337 defer s.writeMu.Unlock()
337 defer s.writeMu.Unlock()
338+ return s.updateLocked(scope, mutate)
339+}
338
340
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 {
339 nc := s.Config().cloneForWrite()
343 nc := s.Config().cloneForWrite()
340 fields := mutate(nc)
344 fields := mutate(nc)
341 s.setConfig(nc)
345 s.setConfig(nc)
@@ -401,23 +405,30 @@ func (s *ConfigStore) RemoveConfigField(scope Scope, key string) error {
401// UpdateAgentModel).
405// UpdateAgentModel).
402func (s *ConfigStore) UpdatePreferredModel(scope Scope, modelType SelectedModelType, model SelectedModel) error {
406func (s *ConfigStore) UpdatePreferredModel(scope Scope, modelType SelectedModelType, model SelectedModel) error {
403 return s.update(scope, func(c *Config) map[string]any {
407 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+}
408
411
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+
409− fields := map[string]any{
421+ fields := map[string]any{
410− fmt.Sprintf("models.%s", modelType): model,
422+ fmt.Sprintf("models.%s", modelType): model,
411− }
423+ }
412− if updated, changed := nextRecentModels(c, modelType, model); changed {
424+ if updated, changed := nextRecentModels(c, modelType, model); changed {
413− if c.RecentModels == nil {
425+ if c.RecentModels == nil {
414− c.RecentModels = make(map[SelectedModelType][]SelectedModel)
426+ c.RecentModels = make(map[SelectedModelType][]SelectedModel)
415− }
416− c.RecentModels[modelType] = updated
417− fields[fmt.Sprintf("recent_models.%s", modelType)] = updated
418 }
427 }
419− return fields
428+ c.RecentModels[modelType] = updated
429+ fields[fmt.Sprintf("recent_models.%s", modelType)] = updated
420− })
430+ }
431+ return fields
421}
432}
422
433
423// SetCompactMode sets the compact mode setting and persists it.
434// SetCompactMode sets the compact mode setting and persists it.
@@ -986,14 +997,17 @@ func (s *ConfigStore) reloadFromDiskLocked(ctx context.Context) error {
986 s.overrides = overrides
997 s.overrides = overrides
987 s.workspacePath = workspacePath
998 s.workspacePath = workspacePath
988
999
989− // Mirror startup flow: setup models and agents against NEW config
1000+ // Mirror startup flow: setup models and agents against NEW config.
990 var setupErr error
1001 var setupErr error
991 if !cfg.IsConfigured() {
1002 if !cfg.IsConfigured() {
992 slog.Warn("No providers configured after reload")
1003 slog.Warn("No providers configured after reload")
993 } else {
1004 } else {
994− if err := configureSelectedModels(s, providers, false); err != nil {
1005+ resolved, resolveErr := resolveSelectedModels(cfg, providers)
1006+ if resolveErr != nil {
995− setupErr = fmt.Errorf("failed to configure selected models during reload: %w", err)
1007+ setupErr = fmt.Errorf("failed to configure selected models during reload: %w", resolveErr)
996 } else {
1008 } else {
1009+ cfg.Models[SelectedModelTypeLarge] = resolved.Large
1010+ cfg.Models[SelectedModelTypeSmall] = resolved.Small
997 s.SetupAgents()
1011 s.SetupAgents()
998 }
1012 }
999 }
1013 }