From 5b5f7b687d324b8835e90d064e6e16970ebc92ea Mon Sep 17 00:00:00 2001 From: Kieran Klukas Date: Fri, 29 May 2026 14:36:55 -0400 Subject: [PATCH] fix(config): sort SetConfigFields keys, remove redundant MkdirAll, rename test - Sort keys in SetConfigFields before applying sjson.Set for deterministic output regardless of map iteration order. - Remove duplicate os.MkdirAll from atomicWrite (already done in lockConfig). - Rename concurrent test to _concurrentInProcess to accurately reflect that it only exercises the in-process mutex, not the cross-process flock. --- internal/config/store.go | 16 +++++++++++----- internal/config/store_test.go | 8 +++++--- 2 files changed, 16 insertions(+), 8 deletions(-) diff --git a/internal/config/store.go b/internal/config/store.go index 259c495e81437dca17ee991eb91ad0a988b99785..9d9cef7d1b368251b03a495d5f0a3c8e44b17b5a 100644 --- a/internal/config/store.go +++ b/internal/config/store.go @@ -172,9 +172,6 @@ func (s *ConfigStore) atomicWrite(scope Scope, fn func(current []byte) ([]byte, return err } - if err := os.MkdirAll(filepath.Dir(path), 0o755); err != nil { - return fmt.Errorf("create config directory: %w", err) - } return atomicWriteFile(path, newData, 0o600) } @@ -221,11 +218,20 @@ func (s *ConfigStore) SetConfigField(scope Scope, key string, value any) error { // The write is protected by an in-process mutex and a cross-process flock // to prevent races between concurrent writers in different processes. func (s *ConfigStore) SetConfigFields(scope Scope, kv map[string]any) error { + // Sort keys for deterministic output regardless of map iteration + // order. This also ensures consistent results when callers pass + // overlapping JSONPath keys (e.g. "a" and "a.b"). + keys := make([]string, 0, len(kv)) + for k := range kv { + keys = append(keys, k) + } + slices.Sort(keys) + err := s.atomicWrite(scope, func(data []byte) ([]byte, error) { v := string(data) - for key, value := range kv { + for _, key := range keys { var sErr error - v, sErr = sjson.Set(v, key, value) + v, sErr = sjson.Set(v, key, kv[key]) if sErr != nil { return nil, fmt.Errorf("failed to set config field %s: %w", key, sErr) } diff --git a/internal/config/store_test.go b/internal/config/store_test.go index 601d3a8487bc339345b347e7d5276d57bf67666d..5094f668896723d7eddf3907e400f1250b8f46ec 100644 --- a/internal/config/store_test.go +++ b/internal/config/store_test.go @@ -732,9 +732,11 @@ func TestRefreshOAuthToken_UsesDiskTokenWhenDifferent(t *testing.T) { require.Equal(t, "refresh-abc", updatedConfig.OAuthToken.RefreshToken) } -// TestConfigStore_SetConfigFields_concurrent verifies that concurrent writes do -// not lose data when protected by the in-process mutex and cross-process flock. -func TestConfigStore_SetConfigFields_concurrent(t *testing.T) { +// TestConfigStore_SetConfigFields_concurrentInProcess verifies that +// concurrent in-process writes do not lose data when serialized by the +// s.mu mutex. This does not exercise the cross-process flock; testing +// that would require spawning a separate OS process. +func TestConfigStore_SetConfigFields_concurrentInProcess(t *testing.T) { t.Parallel() dir := t.TempDir()