81170ce · crush · git.iko.soy

git.iko.soy/crush

on main help sign in

refactor(config): replace reload booleans with reloadMu mutex

81170ceKieran Klukas5 months ago

internal/agent/coordinator.go+1−1

compare with main

unifiedsplit

@@ -260,7 +260,7 @@ func (c *coordinator) Run(ctx context.Context, sessionID string, prompt string,

260 }

261 beforeLoaded := c.skillTracker.LoadedNames()

262 var result *fantasy.AgentResult

263− var originalErr error = c.runWithUnauthorizedRetry(ctx, providerCfg, func() error {

263+ originalErr := c.runWithUnauthorizedRetry(ctx, providerCfg, func() error {

264 var err error

265 result, err = run()

266 return err

internal/config/load.go+10−8

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

108 valueResolver := NewShellVariableResolver(env)

109 store.resolver = valueResolver

110

111− // Disable auto-reload during initial load to prevent nested calls from

111+ // Hold reloadMu during initial load to prevent configureProviders

112− // config-modifying operations inside configureProviders.

112+ // from triggering auto-reload via RemoveConfigField.

113− store.autoReloadDisabled = true

113+ store.reloadMu.Lock()

114− defer func() { store.autoReloadDisabled = false }()

114+ defer store.reloadMu.Unlock()

115

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

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

@@ -266,9 +266,11 @@ func (c *Config) configureProviders(store *ConfigStore, env env.Env, resolver Va

266 switch {

267 case p.ID == catwalk.InferenceProviderAnthropic && config.OAuthToken != nil:

268 // Claude Code subscription is not supported anymore. Remove to show onboarding.

269− if !store.reloadInProgress {

269+ // RemoveConfigField persists the deletion to disk. The in-memory

270+ // state is kept consistent by the Providers.Del call below; any

271+ // concurrent reload that races with this write will also see the

272+ // removal because it re-reads from disk.

270− store.RemoveConfigField(ScopeGlobal, "providers.anthropic")

273+ store.RemoveConfigField(ScopeGlobal, "providers.anthropic")

271− }

272 c.Providers.Del(string(p.ID))

274 c.Providers.Del(string(p.ID))

273 continue

275 continue

274 case p.ID == catwalk.InferenceProviderCopilot && config.OAuthToken != nil:

276 case p.ID == catwalk.InferenceProviderCopilot && config.OAuthToken != nil:

@@ -445,7 +447,7 @@ func (c *Config) setDefaults(workingDir, dataDir string) {

445 c.applyLSPDefaults()

447 c.applyLSPDefaults()

446

448

447 // Add the default context paths if they are not already present

449 // Add the default context paths if they are not already present

448− c.Options.ContextPaths = append(defaultContextPaths, c.Options.ContextPaths...)

450+ c.Options.ContextPaths = append(slices.Clone(defaultContextPaths), c.Options.ContextPaths...)

449 slices.Sort(c.Options.ContextPaths)

451 slices.Sort(c.Options.ContextPaths)

450 c.Options.ContextPaths = slices.Compact(c.Options.ContextPaths)

452 c.Options.ContextPaths = slices.Compact(c.Options.ContextPaths)

451

453

internal/config/store.go+28−15

@@ -50,6 +50,10 @@ type RuntimeOverrides struct {

50// RemoveConfigField, RefreshOAuthToken) to prevent both in-process

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

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

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

53type ConfigStore struct {

57type ConfigStore struct {

54 config *Config

58 config *Config

55 workingDir string

59 workingDir string

@@ -61,10 +65,9 @@ type ConfigStore struct {

61 overrides RuntimeOverrides

65 overrides RuntimeOverrides

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

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

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

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

64− autoReloadDisabled bool // set during load/reload to prevent re-entrancy

65− reloadInProgress bool // set during reload to avoid disk writes mid-reload

67− mu sync.Mutex

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

70+ reloadMu sync.Mutex // serialises ReloadFromDisk calls

68}

71}

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

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

@@ -720,19 +723,18 @@ func (s *ConfigStore) captureStalenessSnapshot(paths []string) {

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

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

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

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

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

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

726+// Concurrent calls are serialised via reloadMu.

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

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

724 if s.workingDir == "" {

728 if s.workingDir == "" {

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

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

726 }

730 }

731+ s.reloadMu.Lock()

732+ defer s.reloadMu.Unlock()

733+ return s.reloadFromDiskLocked(ctx)

734+}

727

735

728− // Disable auto-reload during reload to prevent nested/re-entrant calls.

736+// reloadFromDiskLocked performs the actual reload. Caller must hold reloadMu.

729− s.autoReloadDisabled = true

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

730− s.reloadInProgress = true

731− defer func() {

732− s.autoReloadDisabled = false

733− s.reloadInProgress = false

734− }()

735−

736 // Migrate deprecated disable_notifications before reloading config.

738 // Migrate deprecated disable_notifications before reloading config.

737 migrateDisableNotifications()

739 migrateDisableNotifications()

738

740

@@ -835,11 +837,22 @@ func (s *ConfigStore) ReloadFromDisk(ctx context.Context) error {

835// disabled during load/reload flows, or when working directory is not set

837// disabled during load/reload flows, or when working directory is not set

836// (e.g., during testing). Only actual reload failures return an error.

838// (e.g., during testing). Only actual reload failures return an error.

837func (s *ConfigStore) autoReload(ctx context.Context) error {

839func (s *ConfigStore) autoReload(ctx context.Context) error {

838− if s.autoReloadDisabled {

839− return nil // Expected skip: already in load/reload flow

840− }

841 if s.workingDir == "" {

840 if s.workingDir == "" {

842 return nil // Expected skip: working directory not set

841 return nil // Expected skip: working directory not set

843 }

842 }

843+ // Skip if a reload is already in progress. This handles both

844+ // concurrent auto-reloads after parallel writes and re-entrant

845+ // calls from configureProviders during a reload.

846+ //

847+ // Note: if a write completes after the in-progress reload has

848+ // already read the config file, that write won't be reflected in

849+ // memory until the next reload. This is acceptable because writes

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

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

844− return s.ReloadFromDisk(ctx)

852+ // write should call ReloadFromDisk explicitly.

853+ if !s.reloadMu.TryLock() {

854+ return nil

855+ }

856+ defer s.reloadMu.Unlock()

857+ return s.reloadFromDiskLocked(ctx)

845}

858}

internal/config/store_test.go+3−9

@@ -497,27 +497,21 @@ func TestAutoReloadDisabledDuringReload(t *testing.T) {

497 }`

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

499

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

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

501− // This should NOT cause infinite recursion thanks to autoReloadDisabled guard

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

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

503 require.NoError(t, err)

504

505− // Verify the store loaded successfully and autoReloadDisabled was unset

506− require.False(t, store.autoReloadDisabled)

507−

508 // Capture snapshot and verify reload also works without recursion

505 // Capture snapshot and verify reload also works without recursion

509 store.globalDataPath = configPath

506 store.globalDataPath = configPath

510 store.CaptureStalenessSnapshot([]string{configPath})

507 store.CaptureStalenessSnapshot([]string{configPath})

511

508

512− // Modify file and reload - this should work without re-entrancy issues

509+ // Modify file and reload — this should work without re-entrancy issues

513 time.Sleep(10 * time.Millisecond)

510 time.Sleep(10 * time.Millisecond)

514 require.NoError(t, os.WriteFile(configPath, []byte(`{"options": {"debug": true}}`), 0o600))

511 require.NoError(t, os.WriteFile(configPath, []byte(`{"options": {"debug": true}}`), 0o600))

515

512

516 err = store.ReloadFromDisk(context.Background())

513 err = store.ReloadFromDisk(context.Background())

517 require.NoError(t, err)

514 require.NoError(t, err)

518−

519− // Verify reload completed successfully

520− require.False(t, store.autoReloadDisabled, "autoReloadDisabled should be false after ReloadFromDisk")

521}

515}

522

516

523// TestSetConfigFields_AutoReloadsAtomically verifies that SetConfigFields writes

517// TestSetConfigFields_AutoReloadsAtomically verifies that SetConfigFields writes