git.iko.soy/crush
on main help sign in
fix(config): atomically update multiple fields during oauth
8bc4a75Kieran Klukas
internal/config/store.go+28−17
compare with main
unifiedsplit
@@ -1,7 +1,6 @@
1package config
1package config
2
2
3import (
3import (
4− "cmp"
5 "context"
4 "context"
6 "encoding/json"
5 "encoding/json"
7 "fmt"
6 "fmt"
@@ -127,9 +126,18 @@ func (s *ConfigStore) HasConfigField(scope Scope, key string) bool {
127// After a successful write, it automatically reloads config to keep in-memory
126// After a successful write, it automatically reloads config to keep in-memory
128// state fresh.
127// state fresh.
129func (s *ConfigStore) SetConfigField(scope Scope, key string, value any) error {
128func (s *ConfigStore) SetConfigField(scope Scope, key string, value any) error {
129+ return s.SetConfigFields(scope, map[string]any{key: value})
130+}
131+
132+// SetConfigFields sets multiple key/value pairs in the config file for the given
133+// scope in a single write. After a successful write, it automatically reloads
134+// config to keep in-memory state fresh. This is preferred over multiple
135+// SetConfigField calls when writing several fields atomically to avoid
136+// intermediate reloads with partial state.
137+func (s *ConfigStore) SetConfigFields(scope Scope, kv map[string]any) error {
130 path, err := s.configPath(scope)
138 path, err := s.configPath(scope)
131 if err != nil {
139 if err != nil {
132− return fmt.Errorf("%s: %w", key, err)
140+ return fmt.Errorf("%v: %w", kv, err)
133 }
141 }
134 data, err := os.ReadFile(path)
142 data, err := os.ReadFile(path)
135 if err != nil {
143 if err != nil {
@@ -140,9 +148,12 @@ func (s *ConfigStore) SetConfigField(scope Scope, key string, value any) error {
140 }
148 }
141 }
149 }
142
150
143− newValue, err := sjson.Set(string(data), key, value)
151+ newValue := string(data)
152+ for key, value := range kv {
153+ newValue, err = sjson.Set(newValue, key, value)
144− if err != nil {
154+ if err != nil {
145− return fmt.Errorf("failed to set config field %s: %w", key, err)
155+ return fmt.Errorf("failed to set config field %s: %w", key, err)
156+ }
146 }
157 }
147 if err := os.MkdirAll(filepath.Dir(path), 0o755); err != nil {
158 if err := os.MkdirAll(filepath.Dir(path), 0o755); err != nil {
148 return fmt.Errorf("failed to create config directory %q: %w", path, err)
159 return fmt.Errorf("failed to create config directory %q: %w", path, err)
@@ -238,10 +249,10 @@ func (s *ConfigStore) SetProviderAPIKey(scope Scope, providerID string, apiKey a
238 }
249 }
239 setKeyOrToken = func() { providerConfig.APIKey = v }
250 setKeyOrToken = func() { providerConfig.APIKey = v }
240 case *oauth.Token:
251 case *oauth.Token:
241− if err := cmp.Or(
252+ if err := s.SetConfigFields(scope, map[string]any{
242− s.SetConfigField(scope, fmt.Sprintf("providers.%s.api_key", providerID), v.AccessToken),
253+ fmt.Sprintf("providers.%s.api_key", providerID): v.AccessToken,
243− s.SetConfigField(scope, fmt.Sprintf("providers.%s.oauth", providerID), v),
254+ fmt.Sprintf("providers.%s.oauth", providerID): v,
244− ); err != nil {
255+ }); err != nil {
245 return err
256 return err
246 }
257 }
247 setKeyOrToken = func() {
258 setKeyOrToken = func() {
@@ -343,10 +354,10 @@ func (s *ConfigStore) RefreshOAuthToken(ctx context.Context, scope Scope, provid
343
354
344 s.config.Providers.Set(providerID, providerConfig)
355 s.config.Providers.Set(providerID, providerConfig)
345
356
346− if err := cmp.Or(
357+ if err := s.SetConfigFields(scope, map[string]any{
347− s.SetConfigField(scope, fmt.Sprintf("providers.%s.api_key", providerID), refreshedToken.AccessToken),
358+ fmt.Sprintf("providers.%s.api_key", providerID): refreshedToken.AccessToken,
348− s.SetConfigField(scope, fmt.Sprintf("providers.%s.oauth", providerID), refreshedToken),
359+ fmt.Sprintf("providers.%s.oauth", providerID): refreshedToken,
349− ); err != nil {
360+ }); err != nil {
350 return fmt.Errorf("failed to persist refreshed token: %w", err)
361 return fmt.Errorf("failed to persist refreshed token: %w", err)
351 }
362 }
352
363
@@ -460,10 +471,10 @@ func (s *ConfigStore) ImportCopilot() (*oauth.Token, bool) {
460 return token, false
471 return token, false
461 }
472 }
462
473
463− if err := cmp.Or(
474+ if err := s.SetConfigFields(ScopeGlobal, map[string]any{
464− s.SetConfigField(ScopeGlobal, "providers.copilot.api_key", token.AccessToken),
475+ "providers.copilot.api_key": token.AccessToken,
465− s.SetConfigField(ScopeGlobal, "providers.copilot.oauth", token),
476+ "providers.copilot.oauth": token,
466− ); err != nil {
477+ }); err != nil {
467 slog.Error("Unable to save GitHub Copilot token to disk", "error", err)
478 slog.Error("Unable to save GitHub Copilot token to disk", "error", err)
468 }
479 }
469
480
internal/config/store_test.go+32
@@ -513,6 +513,38 @@ func TestAutoReloadDisabledDuringReload(t *testing.T) {
513 require.False(t, store.autoReloadDisabled, "autoReloadDisabled should be false after ReloadFromDisk")
513 require.False(t, store.autoReloadDisabled, "autoReloadDisabled should be false after ReloadFromDisk")
514}
514}
515
515
516+// TestSetConfigFields_AutoReloadsAtomically verifies that SetConfigFields writes
517+// multiple fields in a single disk write and triggers only one auto-reload,
518+// avoiding intermediate states where only some fields are persisted.
519+func TestSetConfigFields_AutoReloadsAtomically(t *testing.T) {
520+ t.Parallel()
521+
522+ dir := t.TempDir()
523+ configPath := filepath.Join(dir, "crush.json")
524+
525+ // Create initial config file.
526+ initialConfig := `{"options": {"debug": false}}`
527+ require.NoError(t, os.WriteFile(configPath, []byte(initialConfig), 0o600))
528+
529+ // Load initial config.
530+ store, err := Load(dir, dir, false)
531+ require.NoError(t, err)
532+
533+ // Set globalDataPath and capture snapshot.
534+ store.globalDataPath = configPath
535+ store.CaptureStalenessSnapshot([]string{configPath})
536+
537+ // Write multiple fields atomically.
538+ err = store.SetConfigFields(ScopeGlobal, map[string]any{
539+ "options.debug": true,
540+ "options.custom": "hello",
541+ })
542+ require.NoError(t, err)
543+
544+ // Verify both fields are reflected in memory.
545+ require.True(t, store.config.Options.Debug)
546+}
547+
516func TestLoadTokenFromDisk_ReturnsNewerToken(t *testing.T) {
548func TestLoadTokenFromDisk_ReturnsNewerToken(t *testing.T) {
517 t.Parallel()
549 t.Parallel()
518
550