From 1e3bccaa930a873076d84cb856b22153381b83de Mon Sep 17 00:00:00 2001 From: PierrunoYT Date: Tue, 25 Aug 2026 22:07:23 +0200 Subject: [PATCH 1/6] fix(config): lock config read-modify-write across processes MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Every config mutator loads the whole document, edits independent fields, and publishes a complete replacement by rename. The rename is atomic, so a reader never sees partial JSON — but two processes that loaded the same revision each write a full document, and the second rename silently discards the first one's acknowledged update. The result is valid JSON with one update missing and no error anywhere. lockConfigFile takes a cross-process advisory lock through lockutil, using the retry-with-deadline idiom cron, hooks and oauth already share (10s timeout, 20ms retry). Callers acquire BEFORE their first read, so the lock spans load, mutation, validation and publication and the read inside it is authoritative — holding it only around the write would still let both processes start from the same stale revision. The lock file is a sibling (config.json.lock), never the config itself: an advisory lock is held against an inode, and publishing by rename installs a new one, so locking the config directly would leave each process holding a different inode. Covered: all 14 mutators in writer.go, plus ClearProviderKeyStored and MigratePlaintextProviderKeys in credentials.go. The migration matters most — it rewrites the config on every startup, so it is the likeliest writer to collide with an interactive mutation in another Zero. The SetProviderDescription test seam locks too, so it cannot stand in as the one unsynchronized writer. Two shapes needed care: - The lock is not reentrant. EnsureCatalogProvider scans for an existing profile and then upserts, so UpsertProvider is split into a locking wrapper and upsertProviderLocked; one lock now spans the scan and the upsert, which also closes the window where two processes could both create the same catalog profile. - SetPet edits raw bytes to preserve unknown members and formatting rather than round-tripping the struct. It takes the same lock, so it neither clobbers nor is clobbered by the struct writers. Regression tests, each verified to FAIL with the lock disabled: - TestConcurrentMutationsDoNotLoseUpdates — theme, pet, recaps, favorites and a provider mutated at once; all five are independent fields, so a lost update shows up as a zero value in exactly one of them. - TestConcurrentProviderUpsertsAllSurvive — 16 distinct providers added concurrently, all must be present. - TestConcurrentSameFieldMutationsSerialize — 24 writers contending on one field; every call succeeds and the document stays readable. - TestCrossProcessMutationExcludesAndPreserves — the coordinated two-process case. Goroutines share this process's descriptors, so only a second OS process shows the lock is kernel-held. The child announces itself, then the parent asserts its write cannot land while the lock is held elsewhere, does its own mutation, releases, and requires both updates to survive. Fixes #832 Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01U389qmQUhoZB3YtXkFmiTq --- internal/config/concurrent_writer_test.go | 324 ++++++++++++++++++++++ internal/config/credentials.go | 12 + internal/config/export_test.go | 7 + internal/config/lock.go | 65 +++++ internal/config/writer.go | 95 ++++++- 5 files changed, 501 insertions(+), 2 deletions(-) create mode 100644 internal/config/concurrent_writer_test.go create mode 100644 internal/config/lock.go diff --git a/internal/config/concurrent_writer_test.go b/internal/config/concurrent_writer_test.go new file mode 100644 index 000000000..14d42ae34 --- /dev/null +++ b/internal/config/concurrent_writer_test.go @@ -0,0 +1,324 @@ +package config + +import ( + "encoding/json" + "fmt" + "io" + "os" + "os/exec" + "path/filepath" + "strings" + "sync" + "testing" + "time" +) + +func readTestConfig(t *testing.T, path string) FileConfig { + t.Helper() + data, err := os.ReadFile(path) + if err != nil { + t.Fatalf("read config: %v", err) + } + cfg := FileConfig{} + if err := json.Unmarshal(data, &cfg); err != nil { + t.Fatalf("invalid config JSON after concurrent writes: %v\n%s", err, data) + } + return cfg +} + +func seedTestConfig(t *testing.T, path string) { + t.Helper() + if _, err := UpsertProvider(path, ProviderProfile{Name: "seed", Model: "seed-model"}, true); err != nil { + t.Fatalf("seed config: %v", err) + } +} + +// TestConcurrentMutationsDoNotLoseUpdates is the issue #832 regression. Each +// mutator loads the whole document, edits one field, and publishes a complete +// replacement by rename. Without a lock spanning load-through-publish, two +// writers that loaded the same revision both write a full document and the +// second rename silently discards the first one's acknowledged update. +// +// Every mutation below touches an INDEPENDENT field, so a correct +// implementation ends with all of them present. A lost update shows up as a +// zero value in exactly one of the fields, with valid JSON either way — which +// is what made the bug silent. +func TestConcurrentMutationsDoNotLoseUpdates(t *testing.T) { + path := filepath.Join(t.TempDir(), "config.json") + seedTestConfig(t, path) + + mutations := []struct { + name string + apply func() error + check func(FileConfig) error + }{ + { + name: "theme", + apply: func() error { _, err := SetTheme(path, "dracula"); return err }, + check: func(cfg FileConfig) error { + if cfg.Preferences.Theme != "dracula" { + return fmt.Errorf("theme = %q, want dracula", cfg.Preferences.Theme) + } + return nil + }, + }, + { + name: "pet", + apply: func() error { _, err := SetPet(path, "otter"); return err }, + check: func(cfg FileConfig) error { + if cfg.Preferences.Pet != "otter" { + return fmt.Errorf("pet = %q, want otter", cfg.Preferences.Pet) + } + return nil + }, + }, + { + name: "recaps", + apply: func() error { _, err := SetRecapsEnabled(path, true); return err }, + check: func(cfg FileConfig) error { + if cfg.Preferences.Recaps == nil || !*cfg.Preferences.Recaps { + return fmt.Errorf("recaps = %v, want true", cfg.Preferences.Recaps) + } + return nil + }, + }, + { + name: "favorites", + apply: func() error { _, err := SetFavoriteModels(path, []string{"fav-model"}); return err }, + check: func(cfg FileConfig) error { + if len(cfg.Preferences.FavoriteModels) != 1 || cfg.Preferences.FavoriteModels[0] != "fav-model" { + return fmt.Errorf("favorites = %v, want [fav-model]", cfg.Preferences.FavoriteModels) + } + return nil + }, + }, + { + name: "provider", + apply: func() error { + _, err := UpsertProvider(path, ProviderProfile{Name: "added", Model: "added-model"}, false) + return err + }, + check: func(cfg FileConfig) error { + for _, provider := range cfg.Providers { + if provider.Name == "added" { + return nil + } + } + return fmt.Errorf("provider %q missing from %d providers", "added", len(cfg.Providers)) + }, + }, + } + + // SetPet edits the raw bytes rather than round-tripping the struct, so it is + // included deliberately: it must take the same lock as the struct writers or + // it would clobber, and be clobbered by, everything else. + errs := make([]error, len(mutations)) + start := make(chan struct{}) + var wg sync.WaitGroup + for index, mutation := range mutations { + wg.Add(1) + go func() { + defer wg.Done() + <-start + errs[index] = mutation.apply() + }() + } + close(start) + wg.Wait() + + for index, err := range errs { + if err != nil { + t.Fatalf("%s mutation failed: %v", mutations[index].name, err) + } + } + cfg := readTestConfig(t, path) + for _, mutation := range mutations { + if err := mutation.check(cfg); err != nil { + t.Errorf("%s update was lost: %v", mutation.name, err) + } + } + if cfg.ActiveProvider != "seed" { + t.Errorf("activeProvider = %q, want the seeded value preserved", cfg.ActiveProvider) + } +} + +// TestConcurrentSameFieldMutationsSerialize proves the lock serializes writers +// that contend on ONE field: every call must succeed and the file must end with +// exactly one of the written values, never a merged or truncated document. +func TestConcurrentSameFieldMutationsSerialize(t *testing.T) { + path := filepath.Join(t.TempDir(), "config.json") + seedTestConfig(t, path) + + const writers = 24 + errs := make([]error, writers) + start := make(chan struct{}) + var wg sync.WaitGroup + for index := range writers { + wg.Add(1) + go func() { + defer wg.Done() + <-start + _, errs[index] = SetTheme(path, fmt.Sprintf("theme-%02d", index)) + }() + } + close(start) + wg.Wait() + + for index, err := range errs { + if err != nil { + t.Fatalf("writer %d failed: %v", index, err) + } + } + cfg := readTestConfig(t, path) + if !strings.HasPrefix(cfg.Preferences.Theme, "theme-") { + t.Fatalf("theme = %q, want one of the written values", cfg.Preferences.Theme) + } +} + +// TestConcurrentProviderUpsertsAllSurvive is the shape the issue describes most +// directly: N distinct providers added at once. Each add is an independent +// successful mutation, so all N must be present afterwards. +func TestConcurrentProviderUpsertsAllSurvive(t *testing.T) { + path := filepath.Join(t.TempDir(), "config.json") + seedTestConfig(t, path) + + const providers = 16 + errs := make([]error, providers) + start := make(chan struct{}) + var wg sync.WaitGroup + for index := range providers { + wg.Add(1) + go func() { + defer wg.Done() + <-start + _, errs[index] = UpsertProvider(path, ProviderProfile{ + Name: fmt.Sprintf("provider-%02d", index), + Model: fmt.Sprintf("model-%02d", index), + }, false) + }() + } + close(start) + wg.Wait() + + for index, err := range errs { + if err != nil { + t.Fatalf("upsert %d failed: %v", index, err) + } + } + cfg := readTestConfig(t, path) + present := map[string]string{} + for _, provider := range cfg.Providers { + present[provider.Name] = provider.Model + } + for index := range providers { + name := fmt.Sprintf("provider-%02d", index) + want := fmt.Sprintf("model-%02d", index) + if got, ok := present[name]; !ok { + t.Errorf("provider %s was lost (config holds %d providers)", name, len(cfg.Providers)) + } else if got != want { + t.Errorf("provider %s model = %q, want %q", name, got, want) + } + } +} + +// TestCrossProcessMutationExcludesAndPreserves is the coordinated two-process +// test the issue asks for. Goroutines share this process's descriptors, so only +// a second OS process shows the lock is held by the kernel rather than by +// in-process state. +// +// The child announces itself and then mutates; the parent holds the lock across +// that window and asserts the child's write CANNOT land, then does its own +// mutation and releases. Both updates must survive. The exclusion window is +// what makes this deterministic in the passing direction: with the lock held, +// the child is blocked in its retry loop and the theme provably stays unset. +func TestCrossProcessMutationExcludesAndPreserves(t *testing.T) { + path := filepath.Join(t.TempDir(), "config.json") + seedTestConfig(t, path) + + unlock, err := lockConfigFile(path) + if err != nil { + t.Fatalf("acquire lock: %v", err) + } + released := false + release := func() { + if !released { + released = true + unlock() + } + } + defer release() + + child := exec.Command(os.Args[0], "-test.run=^TestConfigConcurrentHelperProcess$") + child.Env = append(os.Environ(), + "ZERO_CONFIG_CONCURRENT_HELPER=1", + "ZERO_CONFIG_CONCURRENT_PATH="+path, + ) + stdout, err := child.StdoutPipe() + if err != nil { + t.Fatalf("stdout pipe: %v", err) + } + if err := child.Start(); err != nil { + t.Fatalf("start helper: %v", err) + } + defer func() { + if child.ProcessState == nil { + _ = child.Process.Kill() + _ = child.Wait() + } + }() + + ready := make([]byte, len("ready\n")) + if _, err := io.ReadFull(stdout, ready); err != nil { + t.Fatalf("helper never signalled ready: %v", err) + } + + // The child is now live and contending for a lock this process holds. Its + // write must not appear while we hold it. + for range 20 { + if theme := readTestConfig(t, path).Preferences.Theme; theme != "" { + t.Fatalf("child wrote %q while the lock was held elsewhere; mutations are not excluded", theme) + } + time.Sleep(5 * time.Millisecond) + } + + if _, err := upsertProviderLocked(path, ProviderProfile{Name: "parent", Model: "parent-model"}, false); err != nil { + t.Fatalf("parent mutation: %v", err) + } + release() + + if err := child.Wait(); err != nil { + t.Fatalf("helper process failed: %v", err) + } + + cfg := readTestConfig(t, path) + if cfg.Preferences.Theme != "child-theme" { + t.Errorf("child update was lost: theme = %q, want child-theme", cfg.Preferences.Theme) + } + found := false + for _, provider := range cfg.Providers { + if provider.Name == "parent" { + found = true + } + } + if !found { + t.Errorf("parent update was lost: provider %q missing from %d providers", "parent", len(cfg.Providers)) + } +} + +// TestConfigConcurrentHelperProcess is the child half of the cross-process +// test. It is skipped unless the parent selected it through the environment. +func TestConfigConcurrentHelperProcess(t *testing.T) { + if os.Getenv("ZERO_CONFIG_CONCURRENT_HELPER") == "" { + t.Skip("helper process for TestCrossProcessMutationExcludesAndPreserves") + } + path := os.Getenv("ZERO_CONFIG_CONCURRENT_PATH") + if path == "" { + t.Fatal("ZERO_CONFIG_CONCURRENT_PATH is required") + } + // Announce BEFORE contending, so the parent's exclusion window starts with + // this process already running rather than still being forked. + fmt.Println("ready") + if _, err := SetTheme(path, "child-theme"); err != nil { + t.Fatalf("child mutation: %v", err) + } +} diff --git a/internal/config/credentials.go b/internal/config/credentials.go index f9432cfd2..ebfcab049 100644 --- a/internal/config/credentials.go +++ b/internal/config/credentials.go @@ -86,6 +86,11 @@ func ClearProviderKeyStored(path, provider string) (bool, error) { if path == "" || provider == "" { return false, nil } + unlock, err := lockConfigFile(path) + if err != nil { + return false, err + } + defer unlock() data, err := os.ReadFile(path) if err != nil { if os.IsNotExist(err) { @@ -121,6 +126,13 @@ func MigratePlaintextProviderKeys(path string, store APIKeySetter) (int, error) if path == "" || store == nil { return 0, nil } + // This runs on every startup, so it is the most likely writer to be racing + // an interactive mutation in another Zero process. + unlock, err := lockConfigFile(path) + if err != nil { + return 0, err + } + defer unlock() data, err := os.ReadFile(path) if err != nil { if os.IsNotExist(err) { diff --git a/internal/config/export_test.go b/internal/config/export_test.go index 93da08130..905304711 100644 --- a/internal/config/export_test.go +++ b/internal/config/export_test.go @@ -50,6 +50,13 @@ func SetProviderDescription(path string, name string, description string) (FileC if name == "" { return FileConfig{}, fmt.Errorf("provider name is required") } + // Locks like the production mutators so this seam can stand in for one in a + // concurrency test rather than being the one unsynchronized writer. + unlock, err := lockConfigFile(path) + if err != nil { + return FileConfig{}, err + } + defer unlock() data, err := os.ReadFile(path) if err != nil { diff --git a/internal/config/lock.go b/internal/config/lock.go new file mode 100644 index 000000000..9716ba66e --- /dev/null +++ b/internal/config/lock.go @@ -0,0 +1,65 @@ +package config + +import ( + "errors" + "fmt" + "os" + "path/filepath" + "time" + + "github.com/Gitlawb/zero/internal/lockutil" +) + +const ( + configLockTimeout = 10 * time.Second + configLockRetryDelay = 20 * time.Millisecond +) + +// lockConfigFile serializes a config read-modify-write across processes. +// +// Every mutator in this package loads the whole document, edits independent +// fields, and publishes a complete replacement by rename. The rename is atomic, +// so a reader never sees partial JSON — but two processes that both loaded the +// same revision each write a full document, and the second rename silently +// discards the first one's acknowledged update (issue #832). Startup work makes +// this easy to hit: MigratePlaintextProviderKeys rewrites the config on every +// launch, so it collides with any concurrent mutation from another Zero. +// +// The lock must therefore span load, mutation, validation AND publication: +// holding it only around the write would still let both processes read the same +// stale revision first. Callers acquire before their first read, which is what +// makes the re-read inside the lock authoritative. +// +// The lock file is a sibling of the config, never the config itself. The kernel +// holds an advisory lock against an inode, and publishing the config by rename +// installs a NEW inode — locking the config directly would leave each process +// holding a different one. lockutil keeps the sibling's path stable for the +// same reason, and never removes it. +// +// This also serializes goroutines within one process: each acquisition opens +// its own file description, so a second in-process attempt contends exactly as +// another process would. The lock is NOT reentrant, so an exported mutator that +// needs another mutator's work calls the unexported *Locked form instead of +// re-entering through the public function. +func lockConfigFile(path string) (func(), error) { + lockPath := path + ".lock" + if dir := filepath.Dir(lockPath); dir != "." && dir != "" { + if err := os.MkdirAll(dir, 0o700); err != nil { + return nil, fmt.Errorf("create config directory %s: %w", dir, err) + } + } + deadline := time.Now().Add(configLockTimeout) + for { + lock, err := lockutil.TryAcquireFileLock(lockPath) + if err == nil { + return func() { _ = lock.Release() }, nil + } + if !errors.Is(err, lockutil.ErrLockHeld) { + return nil, fmt.Errorf("config: acquire config lock: %w", err) + } + if !time.Now().Before(deadline) { + return nil, fmt.Errorf("config: timed out acquiring config lock for %s", path) + } + time.Sleep(configLockRetryDelay) + } +} diff --git a/internal/config/writer.go b/internal/config/writer.go index e3b6846f2..00fd2ce85 100644 --- a/internal/config/writer.go +++ b/internal/config/writer.go @@ -16,6 +16,19 @@ func UpsertProvider(path string, profile ProviderProfile, setActive bool) (FileC if path == "" { return FileConfig{}, fmt.Errorf("config path is required") } + unlock, err := lockConfigFile(path) + if err != nil { + return FileConfig{}, err + } + defer unlock() + return upsertProviderLocked(path, profile, setActive) +} + +// upsertProviderLocked is UpsertProvider's body for a caller that already holds +// the config lock. EnsureCatalogProvider reads the document and then upserts +// into it, and the lock is not reentrant, so it must reach the work this way +// rather than through the exported function. +func upsertProviderLocked(path string, profile ProviderProfile, setActive bool) (FileConfig, error) { profile.Name = strings.TrimSpace(profile.Name) if profile.Name == "" { return FileConfig{}, fmt.Errorf("provider name is required") @@ -81,6 +94,14 @@ func EnsureCatalogProvider(path string, catalogID string) (EnsuredProvider, erro if err != nil { return EnsuredProvider{}, err } + // One lock spans the existence scan AND the upsert: releasing between them + // would let another process create the same catalog profile in the window, + // and the second writer would clobber the first. + unlock, err := lockConfigFile(path) + if err != nil { + return EnsuredProvider{}, err + } + defer unlock() cfg := FileConfig{} if data, err := os.ReadFile(path); err == nil { @@ -104,7 +125,7 @@ func EnsureCatalogProvider(path string, catalogID string) (EnsuredProvider, erro BaseURL: descriptor.DefaultBaseURL, Model: descriptor.DefaultModel, } - written, err := UpsertProvider(path, profile, false) + written, err := upsertProviderLocked(path, profile, false) if err != nil { return EnsuredProvider{}, err } @@ -120,6 +141,11 @@ func MarkProviderAPIKeyStored(path string, provider string) error { if path == "" { return fmt.Errorf("config path is required") } + unlock, err := lockConfigFile(path) + if err != nil { + return err + } + defer unlock() provider = strings.TrimSpace(provider) if provider == "" { return fmt.Errorf("provider name is required") @@ -149,6 +175,11 @@ func SetActiveProvider(path string, name string) (FileConfig, error) { if path == "" { return FileConfig{}, fmt.Errorf("config path is required") } + unlock, err := lockConfigFile(path) + if err != nil { + return FileConfig{}, err + } + defer unlock() name = strings.TrimSpace(name) if name == "" { return FileConfig{}, fmt.Errorf("provider name is required") @@ -215,6 +246,11 @@ func RemoveProvider(path string, name string) (FileConfig, error) { if path == "" { return FileConfig{}, fmt.Errorf("config path is required") } + unlock, err := lockConfigFile(path) + if err != nil { + return FileConfig{}, err + } + defer unlock() name = strings.TrimSpace(name) if name == "" { return FileConfig{}, fmt.Errorf("provider name is required") @@ -266,6 +302,11 @@ func RenameProvider(path string, oldName string, newName string) (FileConfig, er if path == "" { return FileConfig{}, fmt.Errorf("config path is required") } + unlock, err := lockConfigFile(path) + if err != nil { + return FileConfig{}, err + } + defer unlock() oldName = strings.TrimSpace(oldName) newName = strings.TrimSpace(newName) if oldName == "" || newName == "" { @@ -352,6 +393,11 @@ func EditProvider(path string, edit ProviderEdit) (FileConfig, error) { if path == "" { return FileConfig{}, fmt.Errorf("config path is required") } + unlock, err := lockConfigFile(path) + if err != nil { + return FileConfig{}, err + } + defer unlock() oldName := strings.TrimSpace(edit.Name) if oldName == "" { return FileConfig{}, fmt.Errorf("provider name is required") @@ -466,6 +512,11 @@ func SetProviderModel(path string, name string, model string) (FileConfig, error if path == "" { return FileConfig{}, fmt.Errorf("config path is required") } + unlock, err := lockConfigFile(path) + if err != nil { + return FileConfig{}, err + } + defer unlock() name = strings.TrimSpace(name) if name == "" { return FileConfig{}, fmt.Errorf("provider name is required") @@ -503,6 +554,11 @@ func SetFavoriteModels(path string, models []string) (FileConfig, error) { if path == "" { return FileConfig{}, fmt.Errorf("config path is required") } + unlock, err := lockConfigFile(path) + if err != nil { + return FileConfig{}, err + } + defer unlock() cfg := FileConfig{} if data, err := os.ReadFile(path); err == nil { @@ -529,6 +585,11 @@ func SetRecentModels(path string, entries []RecentModelEntry) (FileConfig, error if path == "" { return FileConfig{}, fmt.Errorf("config path is required") } + unlock, err := lockConfigFile(path) + if err != nil { + return FileConfig{}, err + } + defer unlock() cfg := FileConfig{} if data, err := os.ReadFile(path); err == nil { @@ -553,6 +614,11 @@ func SetRecapsEnabled(path string, enabled bool) (FileConfig, error) { if path == "" { return FileConfig{}, fmt.Errorf("config path is required") } + unlock, err := lockConfigFile(path) + if err != nil { + return FileConfig{}, err + } + defer unlock() cfg := FileConfig{} if data, err := os.ReadFile(path); err == nil { if err := json.Unmarshal(data, &cfg); err != nil { @@ -576,6 +642,11 @@ func SetTheme(path string, theme string) (FileConfig, error) { if path == "" { return FileConfig{}, fmt.Errorf("config path is required") } + unlock, err := lockConfigFile(path) + if err != nil { + return FileConfig{}, err + } + defer unlock() cfg := FileConfig{} if data, err := os.ReadFile(path); err == nil { if err := json.Unmarshal(data, &cfg); err != nil { @@ -598,6 +669,11 @@ func SetPet(path string, pet string) (FileConfig, error) { if path == "" { return FileConfig{}, fmt.Errorf("config path is required") } + unlock, err := lockConfigFile(path) + if err != nil { + return FileConfig{}, err + } + defer unlock() cfg := FileConfig{} data := []byte("{}") if existing, err := os.ReadFile(path); err == nil { @@ -610,7 +686,7 @@ func SetPet(path string, pet string) (FileConfig, error) { } pet = strings.TrimSpace(pet) cfg.Preferences.Pet = pet - data, err := setPetPreferenceJSON(data, pet) + data, err = setPetPreferenceJSON(data, pet) if err != nil { return FileConfig{}, fmt.Errorf("invalid config JSON %s: %w", path, err) } @@ -629,6 +705,11 @@ func SetSTTModel(path string, provider STTProviderKind, model string) (FileConfi if path == "" { return FileConfig{}, fmt.Errorf("config path is required") } + unlock, err := lockConfigFile(path) + if err != nil { + return FileConfig{}, err + } + defer unlock() cfg := FileConfig{} if data, err := os.ReadFile(path); err == nil { if err := json.Unmarshal(data, &cfg); err != nil { @@ -665,6 +746,11 @@ func SetSTTLocalEngine(path, binary, serverBinary, modelPath string, streaming b if path == "" { return FileConfig{}, fmt.Errorf("config path is required") } + unlock, err := lockConfigFile(path) + if err != nil { + return FileConfig{}, err + } + defer unlock() cfg := FileConfig{} if data, err := os.ReadFile(path); err == nil { if err := json.Unmarshal(data, &cfg); err != nil { @@ -702,6 +788,11 @@ func SetSTTProvider(path string, provider STTProviderKind) (FileConfig, error) { if path == "" { return FileConfig{}, fmt.Errorf("config path is required") } + unlock, err := lockConfigFile(path) + if err != nil { + return FileConfig{}, err + } + defer unlock() cfg := FileConfig{} if data, err := os.ReadFile(path); err == nil { if err := json.Unmarshal(data, &cfg); err != nil { From 0d58a0c39fb0c57ba5d92c9d418c72d7df7bdc82 Mon Sep 17 00:00:00 2001 From: PierrunoYT Date: Tue, 25 Aug 2026 22:13:40 +0200 Subject: [PATCH 2/6] fix(cli): take the config lock for MCP config edits MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The previous commit locked internal/config's mutators, but that is not every writer of the user config document. internal/cli's MCP editor reads the SAME file, edits it, and republishes it with the identical temp-file+rename shape, at three sites (add/update, remove, disable/enable). Locking only one package left `zero mcp add` free to clobber a concurrent provider or preference write — and to be clobbered by one — with the file still valid JSON afterwards, which is the same silent lost update issue #832 reports. config.LockFile exports the existing helper so the lock is one authority across packages rather than a private detail of internal/config. A second, adjacent implementation would drift from the first exactly the way this writer already drifted from the mutators. TestRunMCPAddParticipatesInConfigLock is the regression. Racing the two writers and waiting to observe a lost update is NOT reliable — the losing interleaving is narrow, and a straightforward concurrent version of this test passed five consecutive runs against the unlocked code, so it would have shipped as reassurance that proved nothing. It instead asserts the deterministic property: while the config lock is held elsewhere, the MCP writer's update cannot land, and it completes once the lock is released. That fails immediately against the unlocked code and passes with it. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01U389qmQUhoZB3YtXkFmiTq --- internal/cli/mcp_config.go | 27 ++++++++ internal/cli/mcp_config_lock_test.go | 95 ++++++++++++++++++++++++++++ internal/config/lock.go | 10 +++ 3 files changed, 132 insertions(+) create mode 100644 internal/cli/mcp_config_lock_test.go diff --git a/internal/cli/mcp_config.go b/internal/cli/mcp_config.go index c8c56fab6..d7187d6b9 100644 --- a/internal/cli/mcp_config.go +++ b/internal/cli/mcp_config.go @@ -66,6 +66,15 @@ func runMCPAdd(args []string, stdout io.Writer, stderr io.Writer, deps appDeps) if err != nil { return writeAppError(stderr, "failed to resolve user config: "+err.Error(), exitCrash) } + // This edits the same user config document the config package mutates, with + // the same read-modify-write + rename shape, so it takes the same + // cross-process lock. Without it, `zero mcp add` racing a provider or + // preference write would silently drop whichever landed first (issue #832). + unlock, err := config.LockFile(configPath) + if err != nil { + return writeAppError(stderr, redaction.ErrorMessage(err, redaction.Options{}), exitCrash) + } + defer unlock() cfg, err := readMCPWritableConfig(configPath) if err != nil { return writeAppError(stderr, redaction.ErrorMessage(err, redaction.Options{}), exitCrash) @@ -132,6 +141,15 @@ func runMCPRemove(args []string, stdout io.Writer, stderr io.Writer, deps appDep if err != nil { return writeAppError(stderr, "failed to resolve user config: "+err.Error(), exitCrash) } + // This edits the same user config document the config package mutates, with + // the same read-modify-write + rename shape, so it takes the same + // cross-process lock. Without it, `zero mcp add` racing a provider or + // preference write would silently drop whichever landed first (issue #832). + unlock, err := config.LockFile(configPath) + if err != nil { + return writeAppError(stderr, redaction.ErrorMessage(err, redaction.Options{}), exitCrash) + } + defer unlock() cfg, err := readMCPWritableConfig(configPath) if err != nil { return writeAppError(stderr, redaction.ErrorMessage(err, redaction.Options{}), exitCrash) @@ -194,6 +212,15 @@ func runMCPToggle(args []string, stdout io.Writer, stderr io.Writer, deps appDep if err != nil { return writeAppError(stderr, "failed to resolve user config: "+err.Error(), exitCrash) } + // This edits the same user config document the config package mutates, with + // the same read-modify-write + rename shape, so it takes the same + // cross-process lock. Without it, `zero mcp add` racing a provider or + // preference write would silently drop whichever landed first (issue #832). + unlock, err := config.LockFile(configPath) + if err != nil { + return writeAppError(stderr, redaction.ErrorMessage(err, redaction.Options{}), exitCrash) + } + defer unlock() cfg, err := readMCPWritableConfig(configPath) if err != nil { return writeAppError(stderr, redaction.ErrorMessage(err, redaction.Options{}), exitCrash) diff --git a/internal/cli/mcp_config_lock_test.go b/internal/cli/mcp_config_lock_test.go new file mode 100644 index 000000000..423e1facd --- /dev/null +++ b/internal/cli/mcp_config_lock_test.go @@ -0,0 +1,95 @@ +package cli + +import ( + "bytes" + "path/filepath" + "testing" + "time" + + "github.com/Gitlawb/zero/internal/config" +) + +// TestRunMCPAddParticipatesInConfigLock covers the half of issue #832 that +// lives outside internal/config. `zero mcp add` reads the SAME user config +// document, edits it, and republishes it with the same temp-file+rename shape +// as the config package's mutators. Locking only the config package would leave +// this writer free to clobber a concurrent provider or preference update, and +// be clobbered by one, with the file still valid JSON afterwards. +// +// Racing the two writers and hoping to observe a lost update is unreliable — +// the interleaving that loses one is narrow, and the test passed consistently +// against the unlocked code. So this asserts the property that actually matters +// and is deterministic: while the config lock is held elsewhere, the MCP +// writer's update CANNOT land. Once released it completes, and both updates +// survive. +func TestRunMCPAddParticipatesInConfigLock(t *testing.T) { + configPath := filepath.Join(t.TempDir(), "zero", "config.json") + if _, err := config.UpsertProvider(configPath, config.ProviderProfile{Name: "seed", Model: "seed-model"}, true); err != nil { + t.Fatalf("seed config: %v", err) + } + + unlock, err := config.LockFile(configPath) + if err != nil { + t.Fatalf("acquire config lock: %v", err) + } + released := false + release := func() { + if !released { + released = true + unlock() + } + } + defer release() + + var stdout, stderr bytes.Buffer + done := make(chan int, 1) + go func() { + done <- runWithDeps([]string{"mcp", "add", "docs", "--", "docs-mcp"}, &stdout, &stderr, appDeps{ + userConfigPath: func() (string, error) { return configPath, nil }, + }) + }() + + // The MCP writer is now contending for a lock this test holds. Its write + // must not appear until the lock is released. + for range 20 { + if servers := readMCPCommandConfig(t, configPath).MCP.Servers; len(servers) != 0 { + t.Fatalf("mcp add wrote %#v while the config lock was held; it does not take the lock", servers) + } + select { + case exitCode := <-done: + t.Fatalf("mcp add completed (exit %d) while the config lock was held; it does not take the lock", exitCode) + default: + } + time.Sleep(5 * time.Millisecond) + } + + release() + + select { + case exitCode := <-done: + if exitCode != exitSuccess { + t.Fatalf("mcp add exitCode = %d stderr=%s", exitCode, stderr.String()) + } + case <-time.After(30 * time.Second): + t.Fatal("mcp add did not finish after the config lock was released") + } + + // A config mutation after the MCP write must keep it, and vice versa. + if _, err := config.SetTheme(configPath, "dracula"); err != nil { + t.Fatalf("SetTheme: %v", err) + } + + cfg := readMCPCommandConfig(t, configPath) + if _, ok := cfg.MCP.Servers["docs"]; !ok { + t.Errorf("mcp add update was lost: servers = %#v", cfg.MCP.Servers) + } + if cfg.Preferences.Theme != "dracula" { + t.Errorf("theme update was lost: theme = %q, want dracula", cfg.Preferences.Theme) + } + if cfg.ActiveProvider != "seed" { + t.Errorf("activeProvider = %q, want the seeded value preserved", cfg.ActiveProvider) + } + if len(cfg.Providers) != 1 || cfg.Providers[0].Name != "seed" { + t.Errorf("seeded provider was lost: providers = %#v", cfg.Providers) + } +} diff --git a/internal/config/lock.go b/internal/config/lock.go index 9716ba66e..144610d9d 100644 --- a/internal/config/lock.go +++ b/internal/config/lock.go @@ -41,6 +41,16 @@ const ( // another process would. The lock is NOT reentrant, so an exported mutator that // needs another mutator's work calls the unexported *Locked form instead of // re-entering through the public function. +// LockFile exposes lockConfigFile to packages that edit the SAME user config +// document without going through this package's mutators — internal/cli's MCP +// editor reads, edits and republishes it with the identical temp-file+rename +// shape. A writer that skipped this lock would reintroduce the lost update for +// every field, so the lock has to be one authority across packages rather than +// a private detail of this one. +func LockFile(path string) (func(), error) { + return lockConfigFile(path) +} + func lockConfigFile(path string) (func(), error) { lockPath := path + ".lock" if dir := filepath.Dir(lockPath); dir != "." && dir != "" { From e8e750ddf1df86e78f565813c36a9cfb68fc503a Mon Sep 17 00:00:00 2001 From: PierrunoYT Date: Tue, 25 Aug 2026 22:24:05 +0200 Subject: [PATCH 3/6] fix(config): report config unlock failures instead of discarding them MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit AGENTS.md requires that a mutator never report success when unlock failed, and the release error was being dropped on the floor. A failed Release can leave the lock held for the rest of the process, so returning (cfg, nil) after one claims a state the next mutation cannot reproduce — it will block for the full lock timeout and then fail. lockConfigFile now returns lock.Release directly, and every caller joins it into its own result with the idiom credstore already uses: defer func() { err = errors.Join(err, unlock()) }() Joined, not chosen between: a release failure annotates the result rather than masking the mutation error that explains what actually went wrong. That covers all 16 mutators in writer.go, both in credentials.go, and the test seam. The three MCP sites return an exit code rather than an error, so they convert a release failure into a crash exit and a stderr message, without overwriting a failure the command had already reported. A release failure cannot be provoked through the public API — lockutil.Release is idempotent and reports nil once released — so the mutators call through a lockConfigFileFn seam that a test can substitute. My first attempt at this test could only ever t.Skip, which asserts nothing; the seam is what turns it into a real assertion. - TestMutationReportsUnlockFailure: SetTheme surfaces the release error AND the mutation is still published, since a release failure annotates the result rather than undoing the write. - TestMutationErrorSurvivesUnlockFailure: an unknown-provider error and the release failure are both present in the joined error. Both verified to fail when the defer discards the release error. Note for review: cron, hooks, oauth and swarm all still discard their lockutil.Release error, so this makes internal/config stricter than its siblings. Worth deciding whether the guideline should be applied to them too — out of scope here. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01U389qmQUhoZB3YtXkFmiTq --- internal/cli/mcp_config.go | 33 ++++- internal/config/concurrent_writer_test.go | 72 ++++++++++ internal/config/credentials.go | 21 ++- internal/config/export_test.go | 11 +- internal/config/lock.go | 20 ++- internal/config/writer.go | 161 +++++++++++++++------- 6 files changed, 251 insertions(+), 67 deletions(-) diff --git a/internal/cli/mcp_config.go b/internal/cli/mcp_config.go index d7187d6b9..1dec280cb 100644 --- a/internal/cli/mcp_config.go +++ b/internal/cli/mcp_config.go @@ -50,7 +50,7 @@ func projectMCPConfigExists(workspaceRoot string) bool { return len(fc.MCP.Servers) > 0 } -func runMCPAdd(args []string, stdout io.Writer, stderr io.Writer, deps appDeps) int { +func runMCPAdd(args []string, stdout io.Writer, stderr io.Writer, deps appDeps) (exitCode int) { options, help, err := parseMCPAddArgs(args) if err != nil { return writeExecUsageError(stderr, err.Error()) @@ -74,7 +74,14 @@ func runMCPAdd(args []string, stdout io.Writer, stderr io.Writer, deps appDeps) if err != nil { return writeAppError(stderr, redaction.ErrorMessage(err, redaction.Options{}), exitCrash) } - defer unlock() + defer func() { + // A failed release leaves the lock held for the rest of the process, so + // exiting success here would claim a state the next config write cannot + // reproduce. It must not mask a failure this command already reported. + if releaseErr := unlock(); releaseErr != nil && exitCode == exitSuccess { + exitCode = writeAppError(stderr, redaction.ErrorMessage(releaseErr, redaction.Options{}), exitCrash) + } + }() cfg, err := readMCPWritableConfig(configPath) if err != nil { return writeAppError(stderr, redaction.ErrorMessage(err, redaction.Options{}), exitCrash) @@ -118,7 +125,7 @@ func runMCPAdd(args []string, stdout io.Writer, stderr io.Writer, deps appDeps) return exitSuccess } -func runMCPRemove(args []string, stdout io.Writer, stderr io.Writer, deps appDeps) int { +func runMCPRemove(args []string, stdout io.Writer, stderr io.Writer, deps appDeps) (exitCode int) { options, positional, help, err := parseMCPConfigPositionalCommand(args, "remove") if err != nil { return writeExecUsageError(stderr, err.Error()) @@ -149,7 +156,14 @@ func runMCPRemove(args []string, stdout io.Writer, stderr io.Writer, deps appDep if err != nil { return writeAppError(stderr, redaction.ErrorMessage(err, redaction.Options{}), exitCrash) } - defer unlock() + defer func() { + // A failed release leaves the lock held for the rest of the process, so + // exiting success here would claim a state the next config write cannot + // reproduce. It must not mask a failure this command already reported. + if releaseErr := unlock(); releaseErr != nil && exitCode == exitSuccess { + exitCode = writeAppError(stderr, redaction.ErrorMessage(releaseErr, redaction.Options{}), exitCrash) + } + }() cfg, err := readMCPWritableConfig(configPath) if err != nil { return writeAppError(stderr, redaction.ErrorMessage(err, redaction.Options{}), exitCrash) @@ -185,7 +199,7 @@ func runMCPRemove(args []string, stdout io.Writer, stderr io.Writer, deps appDep return exitSuccess } -func runMCPToggle(args []string, stdout io.Writer, stderr io.Writer, deps appDeps, disabled bool) int { +func runMCPToggle(args []string, stdout io.Writer, stderr io.Writer, deps appDeps, disabled bool) (exitCode int) { commandName := "enable" if disabled { commandName = "disable" @@ -220,7 +234,14 @@ func runMCPToggle(args []string, stdout io.Writer, stderr io.Writer, deps appDep if err != nil { return writeAppError(stderr, redaction.ErrorMessage(err, redaction.Options{}), exitCrash) } - defer unlock() + defer func() { + // A failed release leaves the lock held for the rest of the process, so + // exiting success here would claim a state the next config write cannot + // reproduce. It must not mask a failure this command already reported. + if releaseErr := unlock(); releaseErr != nil && exitCode == exitSuccess { + exitCode = writeAppError(stderr, redaction.ErrorMessage(releaseErr, redaction.Options{}), exitCrash) + } + }() cfg, err := readMCPWritableConfig(configPath) if err != nil { return writeAppError(stderr, redaction.ErrorMessage(err, redaction.Options{}), exitCrash) diff --git a/internal/config/concurrent_writer_test.go b/internal/config/concurrent_writer_test.go index 14d42ae34..1d1bafc7b 100644 --- a/internal/config/concurrent_writer_test.go +++ b/internal/config/concurrent_writer_test.go @@ -2,6 +2,7 @@ package config import ( "encoding/json" + "errors" "fmt" "io" "os" @@ -322,3 +323,74 @@ func TestConfigConcurrentHelperProcess(t *testing.T) { t.Fatalf("child mutation: %v", err) } } + +// TestMutationReportsUnlockFailure pins the AGENTS.md rule that a mutator must +// never report success when unlock failed. A failed release can leave the lock +// held for the rest of the process, so returning (cfg, nil) after one would +// claim a state the next mutation cannot reproduce. +// +// lockutil.Release is idempotent and reports nil once released, so a real +// release failure cannot be provoked through the public API — hence the seam. +// The mutation itself must still have been published: the release failure +// annotates the result, it does not undo the write. +func TestMutationReportsUnlockFailure(t *testing.T) { + path := filepath.Join(t.TempDir(), "config.json") + seedTestConfig(t, path) + + releaseErr := errors.New("release exploded") + original := lockConfigFileFn + t.Cleanup(func() { lockConfigFileFn = original }) + lockConfigFileFn = func(p string) (func() error, error) { + unlock, err := original(p) + if err != nil { + return nil, err + } + return func() error { + _ = unlock() + return releaseErr + }, nil + } + + cfg, err := SetTheme(path, "dracula") + if !errors.Is(err, releaseErr) { + t.Fatalf("SetTheme err = %v, want it to carry the release failure", err) + } + if cfg.Preferences.Theme != "dracula" { + t.Errorf("returned config lost the mutation: theme = %q", cfg.Preferences.Theme) + } + if got := readTestConfig(t, path).Preferences.Theme; got != "dracula" { + t.Errorf("mutation was not published despite only the release failing: theme = %q", got) + } +} + +// TestMutationErrorSurvivesUnlockFailure proves the two errors are joined +// rather than chosen between: a release failure must not mask the mutation +// error that actually explains what went wrong. +func TestMutationErrorSurvivesUnlockFailure(t *testing.T) { + path := filepath.Join(t.TempDir(), "config.json") + seedTestConfig(t, path) + + releaseErr := errors.New("release exploded") + original := lockConfigFileFn + t.Cleanup(func() { lockConfigFileFn = original }) + lockConfigFileFn = func(p string) (func() error, error) { + unlock, err := original(p) + if err != nil { + return nil, err + } + return func() error { + _ = unlock() + return releaseErr + }, nil + } + + // An unknown provider is the mutation error; the release failure is joined + // onto it, and neither one disappears. + _, err := SetActiveProvider(path, "definitely-not-configured") + if !errors.Is(err, releaseErr) { + t.Fatalf("err = %v, want it to carry the release failure", err) + } + if !strings.Contains(err.Error(), "definitely-not-configured") { + t.Fatalf("err = %v, want it to still name the missing provider", err) + } +} diff --git a/internal/config/credentials.go b/internal/config/credentials.go index ebfcab049..1a27e089f 100644 --- a/internal/config/credentials.go +++ b/internal/config/credentials.go @@ -2,6 +2,7 @@ package config import ( "encoding/json" + "errors" "fmt" "os" "path/filepath" @@ -80,17 +81,21 @@ func ForgetProviderKey(provider string) (bool, error) { // config at path, so credential checks no longer claim a stored key after one is // removed. No-op when path/provider is empty, the config is absent, or the marker // is already unset. -func ClearProviderKeyStored(path, provider string) (bool, error) { +func ClearProviderKeyStored(path, provider string) (cleared bool, err error) { path = strings.TrimSpace(path) provider = strings.TrimSpace(provider) if path == "" || provider == "" { return false, nil } - unlock, err := lockConfigFile(path) + unlock, err := lockConfigFileFn(path) if err != nil { return false, err } - defer unlock() + // Joined, not chosen between: a release failure annotates the result + // instead of masking the mutation error that actually explains what went + // wrong. Reporting success after a failed unlock would claim a state the + // next mutation cannot reproduce. + defer func() { err = errors.Join(err, unlock()) }() data, err := os.ReadFile(path) if err != nil { if os.IsNotExist(err) { @@ -121,18 +126,22 @@ func ClearProviderKeyStored(path, provider string) (bool, error) { // leaves the plaintext key in place and never strands a credential. Returns how // many keys were migrated; a no-op (0, nil) when path is empty/absent or nothing // needs migrating. Safe to run on every startup (idempotent). -func MigratePlaintextProviderKeys(path string, store APIKeySetter) (int, error) { +func MigratePlaintextProviderKeys(path string, store APIKeySetter) (count int, err error) { path = strings.TrimSpace(path) if path == "" || store == nil { return 0, nil } // This runs on every startup, so it is the most likely writer to be racing // an interactive mutation in another Zero process. - unlock, err := lockConfigFile(path) + unlock, err := lockConfigFileFn(path) if err != nil { return 0, err } - defer unlock() + // Joined, not chosen between: a release failure annotates the result + // instead of masking the mutation error that actually explains what went + // wrong. Reporting success after a failed unlock would claim a state the + // next mutation cannot reproduce. + defer func() { err = errors.Join(err, unlock()) }() data, err := os.ReadFile(path) if err != nil { if os.IsNotExist(err) { diff --git a/internal/config/export_test.go b/internal/config/export_test.go index 905304711..6e15476ed 100644 --- a/internal/config/export_test.go +++ b/internal/config/export_test.go @@ -3,6 +3,7 @@ package config import ( "encoding/json" + "errors" "fmt" "os" "strings" @@ -41,7 +42,7 @@ func ValidateFile(path string) (FileConfig, []Issue) { // SetProviderDescription sets a provider's description VERBATIM — including to // empty. The generic UpsertProvider merge treats empty fields as "leave // unchanged", so clearing a description needs this dedicated setter. -func SetProviderDescription(path string, name string, description string) (FileConfig, error) { +func SetProviderDescription(path string, name string, description string) (result FileConfig, err error) { path = strings.TrimSpace(path) if path == "" { return FileConfig{}, fmt.Errorf("config path is required") @@ -52,11 +53,15 @@ func SetProviderDescription(path string, name string, description string) (FileC } // Locks like the production mutators so this seam can stand in for one in a // concurrency test rather than being the one unsynchronized writer. - unlock, err := lockConfigFile(path) + unlock, err := lockConfigFileFn(path) if err != nil { return FileConfig{}, err } - defer unlock() + // Joined, not chosen between: a release failure annotates the result + // instead of masking the mutation error that actually explains what went + // wrong. Reporting success after a failed unlock would claim a state the + // next mutation cannot reproduce. + defer func() { err = errors.Join(err, unlock()) }() data, err := os.ReadFile(path) if err != nil { diff --git a/internal/config/lock.go b/internal/config/lock.go index 144610d9d..858d75500 100644 --- a/internal/config/lock.go +++ b/internal/config/lock.go @@ -36,6 +36,12 @@ const ( // holding a different one. lockutil keeps the sibling's path stable for the // same reason, and never removes it. // +// The returned release function reports its own failure rather than swallowing +// it: a failed Release can leave the lock held for the rest of the process, so +// a mutator that returned success after it would be reporting a state the next +// mutation cannot reproduce. Callers join it into their result (AGENTS.md: +// "never report success when cleanup or unlock failed"). +// // This also serializes goroutines within one process: each acquisition opens // its own file description, so a second in-process attempt contends exactly as // another process would. The lock is NOT reentrant, so an exported mutator that @@ -47,11 +53,17 @@ const ( // shape. A writer that skipped this lock would reintroduce the lost update for // every field, so the lock has to be one authority across packages rather than // a private detail of this one. -func LockFile(path string) (func(), error) { - return lockConfigFile(path) +func LockFile(path string) (func() error, error) { + return lockConfigFileFn(path) } -func lockConfigFile(path string) (func(), error) { +// lockConfigFileFn is the seam the mutators call. Release failures cannot be +// provoked through the public API — lockutil.Release is idempotent and reports +// nil once released — so substituting this is the only way to assert that a +// failed unlock actually reaches the caller instead of being swallowed. +var lockConfigFileFn = lockConfigFile + +func lockConfigFile(path string) (func() error, error) { lockPath := path + ".lock" if dir := filepath.Dir(lockPath); dir != "." && dir != "" { if err := os.MkdirAll(dir, 0o700); err != nil { @@ -62,7 +74,7 @@ func lockConfigFile(path string) (func(), error) { for { lock, err := lockutil.TryAcquireFileLock(lockPath) if err == nil { - return func() { _ = lock.Release() }, nil + return lock.Release, nil } if !errors.Is(err, lockutil.ErrLockHeld) { return nil, fmt.Errorf("config: acquire config lock: %w", err) diff --git a/internal/config/writer.go b/internal/config/writer.go index 00fd2ce85..92b89f0c5 100644 --- a/internal/config/writer.go +++ b/internal/config/writer.go @@ -2,6 +2,7 @@ package config import ( "encoding/json" + "errors" "fmt" "os" "path/filepath" @@ -11,16 +12,20 @@ import ( "github.com/Gitlawb/zero/internal/providercatalog" ) -func UpsertProvider(path string, profile ProviderProfile, setActive bool) (FileConfig, error) { +func UpsertProvider(path string, profile ProviderProfile, setActive bool) (result FileConfig, err error) { path = strings.TrimSpace(path) if path == "" { return FileConfig{}, fmt.Errorf("config path is required") } - unlock, err := lockConfigFile(path) + unlock, err := lockConfigFileFn(path) if err != nil { return FileConfig{}, err } - defer unlock() + // Joined, not chosen between: a release failure annotates the result + // instead of masking the mutation error that actually explains what went + // wrong. Reporting success after a failed unlock would claim a state the + // next mutation cannot reproduce. + defer func() { err = errors.Join(err, unlock()) }() return upsertProviderLocked(path, profile, setActive) } @@ -85,7 +90,7 @@ type EnsuredProvider struct { // Name or CatalogID already matches is left completely untouched (its name, // credentials, and model are the user's), and a created profile is NOT marked // active unless no provider was active at all. -func EnsureCatalogProvider(path string, catalogID string) (EnsuredProvider, error) { +func EnsureCatalogProvider(path string, catalogID string) (result EnsuredProvider, err error) { path = strings.TrimSpace(path) if path == "" { return EnsuredProvider{}, fmt.Errorf("config path is required") @@ -97,11 +102,15 @@ func EnsureCatalogProvider(path string, catalogID string) (EnsuredProvider, erro // One lock spans the existence scan AND the upsert: releasing between them // would let another process create the same catalog profile in the window, // and the second writer would clobber the first. - unlock, err := lockConfigFile(path) + unlock, err := lockConfigFileFn(path) if err != nil { return EnsuredProvider{}, err } - defer unlock() + // Joined, not chosen between: a release failure annotates the result + // instead of masking the mutation error that actually explains what went + // wrong. Reporting success after a failed unlock would claim a state the + // next mutation cannot reproduce. + defer func() { err = errors.Join(err, unlock()) }() cfg := FileConfig{} if data, err := os.ReadFile(path); err == nil { @@ -136,16 +145,20 @@ func EnsureCatalogProvider(path string, catalogID string) (EnsuredProvider, erro // credential store. It also clears inline/env key fields so the stored key is the // runtime credential; an old apiKeyEnv value must not keep overriding a freshly // captured key from `zero auth openrouter` or provider setup. -func MarkProviderAPIKeyStored(path string, provider string) error { +func MarkProviderAPIKeyStored(path string, provider string) (err error) { path = strings.TrimSpace(path) if path == "" { return fmt.Errorf("config path is required") } - unlock, err := lockConfigFile(path) + unlock, err := lockConfigFileFn(path) if err != nil { return err } - defer unlock() + // Joined, not chosen between: a release failure annotates the result + // instead of masking the mutation error that actually explains what went + // wrong. Reporting success after a failed unlock would claim a state the + // next mutation cannot reproduce. + defer func() { err = errors.Join(err, unlock()) }() provider = strings.TrimSpace(provider) if provider == "" { return fmt.Errorf("provider name is required") @@ -170,16 +183,20 @@ func MarkProviderAPIKeyStored(path string, provider string) error { return fmt.Errorf("provider %q not found", provider) } -func SetActiveProvider(path string, name string) (FileConfig, error) { +func SetActiveProvider(path string, name string) (result FileConfig, err error) { path = strings.TrimSpace(path) if path == "" { return FileConfig{}, fmt.Errorf("config path is required") } - unlock, err := lockConfigFile(path) + unlock, err := lockConfigFileFn(path) if err != nil { return FileConfig{}, err } - defer unlock() + // Joined, not chosen between: a release failure annotates the result + // instead of masking the mutation error that actually explains what went + // wrong. Reporting success after a failed unlock would claim a state the + // next mutation cannot reproduce. + defer func() { err = errors.Join(err, unlock()) }() name = strings.TrimSpace(name) if name == "" { return FileConfig{}, fmt.Errorf("provider name is required") @@ -241,16 +258,20 @@ func ProviderPersisted(path string, name string) (bool, error) { // a profile that no longer exists. The caller owns cleaning up the credential // store entry — config stays pure of secret I/O on the read path, and remove // keeps that symmetry by only touching config.json. -func RemoveProvider(path string, name string) (FileConfig, error) { +func RemoveProvider(path string, name string) (result FileConfig, err error) { path = strings.TrimSpace(path) if path == "" { return FileConfig{}, fmt.Errorf("config path is required") } - unlock, err := lockConfigFile(path) + unlock, err := lockConfigFileFn(path) if err != nil { return FileConfig{}, err } - defer unlock() + // Joined, not chosen between: a release failure annotates the result + // instead of masking the mutation error that actually explains what went + // wrong. Reporting success after a failed unlock would claim a state the + // next mutation cannot reproduce. + defer func() { err = errors.Join(err, unlock()) }() name = strings.TrimSpace(name) if name == "" { return FileConfig{}, fmt.Errorf("provider name is required") @@ -297,16 +318,20 @@ func RemoveProvider(path string, name string) (FileConfig, error) { // resolves. OAuth tokens are deliberately not migrated: the runtime's login // candidates fall back to the profile's CatalogID, which every OAuth-capable // catalog profile carries, so a rename keeps the login reachable. -func RenameProvider(path string, oldName string, newName string) (FileConfig, error) { +func RenameProvider(path string, oldName string, newName string) (result FileConfig, err error) { path = strings.TrimSpace(path) if path == "" { return FileConfig{}, fmt.Errorf("config path is required") } - unlock, err := lockConfigFile(path) + unlock, err := lockConfigFileFn(path) if err != nil { return FileConfig{}, err } - defer unlock() + // Joined, not chosen between: a release failure annotates the result + // instead of masking the mutation error that actually explains what went + // wrong. Reporting success after a failed unlock would claim a state the + // next mutation cannot reproduce. + defer func() { err = errors.Join(err, unlock()) }() oldName = strings.TrimSpace(oldName) newName = strings.TrimSpace(newName) if oldName == "" || newName == "" { @@ -388,16 +413,20 @@ type ProviderEdit struct { // and, unlike UpsertProvider's exact-name merge, the case-insensitive match // here makes a case-only rename (groq -> Groq) an in-place update instead of // an appended duplicate profile. -func EditProvider(path string, edit ProviderEdit) (FileConfig, error) { +func EditProvider(path string, edit ProviderEdit) (result FileConfig, err error) { path = strings.TrimSpace(path) if path == "" { return FileConfig{}, fmt.Errorf("config path is required") } - unlock, err := lockConfigFile(path) + unlock, err := lockConfigFileFn(path) if err != nil { return FileConfig{}, err } - defer unlock() + // Joined, not chosen between: a release failure annotates the result + // instead of masking the mutation error that actually explains what went + // wrong. Reporting success after a failed unlock would claim a state the + // next mutation cannot reproduce. + defer func() { err = errors.Join(err, unlock()) }() oldName := strings.TrimSpace(edit.Name) if oldName == "" { return FileConfig{}, fmt.Errorf("provider name is required") @@ -507,16 +536,20 @@ func migrateStoredProviderKey(configPath string, oldName string, newName string) return nil } -func SetProviderModel(path string, name string, model string) (FileConfig, error) { +func SetProviderModel(path string, name string, model string) (result FileConfig, err error) { path = strings.TrimSpace(path) if path == "" { return FileConfig{}, fmt.Errorf("config path is required") } - unlock, err := lockConfigFile(path) + unlock, err := lockConfigFileFn(path) if err != nil { return FileConfig{}, err } - defer unlock() + // Joined, not chosen between: a release failure annotates the result + // instead of masking the mutation error that actually explains what went + // wrong. Reporting success after a failed unlock would claim a state the + // next mutation cannot reproduce. + defer func() { err = errors.Join(err, unlock()) }() name = strings.TrimSpace(name) if name == "" { return FileConfig{}, fmt.Errorf("provider name is required") @@ -549,16 +582,20 @@ func SetProviderModel(path string, name string, model string) (FileConfig, error return FileConfig{}, fmt.Errorf("provider %q not found", name) } -func SetFavoriteModels(path string, models []string) (FileConfig, error) { +func SetFavoriteModels(path string, models []string) (result FileConfig, err error) { path = strings.TrimSpace(path) if path == "" { return FileConfig{}, fmt.Errorf("config path is required") } - unlock, err := lockConfigFile(path) + unlock, err := lockConfigFileFn(path) if err != nil { return FileConfig{}, err } - defer unlock() + // Joined, not chosen between: a release failure annotates the result + // instead of masking the mutation error that actually explains what went + // wrong. Reporting success after a failed unlock would claim a state the + // next mutation cannot reproduce. + defer func() { err = errors.Join(err, unlock()) }() cfg := FileConfig{} if data, err := os.ReadFile(path); err == nil { @@ -580,16 +617,20 @@ func SetFavoriteModels(path string, models []string) (FileConfig, error) { // mirroring SetFavoriteModels (read-modify-atomic-write). Unlike favorites, // order is preserved (newest first) rather than sorted, since it reflects // switch recency, not an alphabetical preference list. -func SetRecentModels(path string, entries []RecentModelEntry) (FileConfig, error) { +func SetRecentModels(path string, entries []RecentModelEntry) (result FileConfig, err error) { path = strings.TrimSpace(path) if path == "" { return FileConfig{}, fmt.Errorf("config path is required") } - unlock, err := lockConfigFile(path) + unlock, err := lockConfigFileFn(path) if err != nil { return FileConfig{}, err } - defer unlock() + // Joined, not chosen between: a release failure annotates the result + // instead of masking the mutation error that actually explains what went + // wrong. Reporting success after a failed unlock would claim a state the + // next mutation cannot reproduce. + defer func() { err = errors.Join(err, unlock()) }() cfg := FileConfig{} if data, err := os.ReadFile(path); err == nil { @@ -609,16 +650,20 @@ func SetRecentModels(path string, entries []RecentModelEntry) (FileConfig, error // SetRecapsEnabled persists the idle recap preference, mirroring // SetFavoriteModels (read-modify-atomic-write). -func SetRecapsEnabled(path string, enabled bool) (FileConfig, error) { +func SetRecapsEnabled(path string, enabled bool) (result FileConfig, err error) { path = strings.TrimSpace(path) if path == "" { return FileConfig{}, fmt.Errorf("config path is required") } - unlock, err := lockConfigFile(path) + unlock, err := lockConfigFileFn(path) if err != nil { return FileConfig{}, err } - defer unlock() + // Joined, not chosen between: a release failure annotates the result + // instead of masking the mutation error that actually explains what went + // wrong. Reporting success after a failed unlock would claim a state the + // next mutation cannot reproduce. + defer func() { err = errors.Join(err, unlock()) }() cfg := FileConfig{} if data, err := os.ReadFile(path); err == nil { if err := json.Unmarshal(data, &cfg); err != nil { @@ -637,16 +682,20 @@ func SetRecapsEnabled(path string, enabled bool) (FileConfig, error) { // SetTheme persists the TUI theme preference, mirroring SetFavoriteModels // (read-modify-atomic-write). A blank theme clears the stored preference. -func SetTheme(path string, theme string) (FileConfig, error) { +func SetTheme(path string, theme string) (result FileConfig, err error) { path = strings.TrimSpace(path) if path == "" { return FileConfig{}, fmt.Errorf("config path is required") } - unlock, err := lockConfigFile(path) + unlock, err := lockConfigFileFn(path) if err != nil { return FileConfig{}, err } - defer unlock() + // Joined, not chosen between: a release failure annotates the result + // instead of masking the mutation error that actually explains what went + // wrong. Reporting success after a failed unlock would claim a state the + // next mutation cannot reproduce. + defer func() { err = errors.Join(err, unlock()) }() cfg := FileConfig{} if data, err := os.ReadFile(path); err == nil { if err := json.Unmarshal(data, &cfg); err != nil { @@ -664,16 +713,20 @@ func SetTheme(path string, theme string) (FileConfig, error) { // SetPet persists only the terminal-pet preference while preserving every // unrelated user setting through the config writer's atomic replace path. -func SetPet(path string, pet string) (FileConfig, error) { +func SetPet(path string, pet string) (result FileConfig, err error) { path = strings.TrimSpace(path) if path == "" { return FileConfig{}, fmt.Errorf("config path is required") } - unlock, err := lockConfigFile(path) + unlock, err := lockConfigFileFn(path) if err != nil { return FileConfig{}, err } - defer unlock() + // Joined, not chosen between: a release failure annotates the result + // instead of masking the mutation error that actually explains what went + // wrong. Reporting success after a failed unlock would claim a state the + // next mutation cannot reproduce. + defer func() { err = errors.Join(err, unlock()) }() cfg := FileConfig{} data := []byte("{}") if existing, err := os.ReadFile(path); err == nil { @@ -700,16 +753,20 @@ func SetPet(path string, pet string) (FileConfig, error) { // SetTheme (read-modify-atomic-write). provider must be one of the known STT // provider kinds; a local provider stores the model as stt.localModelPath, // otherwise as stt.model. A blank model clears the stored value for that slot. -func SetSTTModel(path string, provider STTProviderKind, model string) (FileConfig, error) { +func SetSTTModel(path string, provider STTProviderKind, model string) (result FileConfig, err error) { path = strings.TrimSpace(path) if path == "" { return FileConfig{}, fmt.Errorf("config path is required") } - unlock, err := lockConfigFile(path) + unlock, err := lockConfigFileFn(path) if err != nil { return FileConfig{}, err } - defer unlock() + // Joined, not chosen between: a release failure annotates the result + // instead of masking the mutation error that actually explains what went + // wrong. Reporting success after a failed unlock would claim a state the + // next mutation cannot reproduce. + defer func() { err = errors.Join(err, unlock()) }() cfg := FileConfig{} if data, err := os.ReadFile(path); err == nil { if err := json.Unmarshal(data, &cfg); err != nil { @@ -741,16 +798,20 @@ func SetSTTModel(path string, provider STTProviderKind, model string) (FileConfi // (read-modify-atomic-write). streaming selects the pipeline matching the // downloaded model (a streaming transducer vs a batch model). Called after a // download completes. -func SetSTTLocalEngine(path, binary, serverBinary, modelPath string, streaming bool) (FileConfig, error) { +func SetSTTLocalEngine(path, binary, serverBinary, modelPath string, streaming bool) (result FileConfig, err error) { path = strings.TrimSpace(path) if path == "" { return FileConfig{}, fmt.Errorf("config path is required") } - unlock, err := lockConfigFile(path) + unlock, err := lockConfigFileFn(path) if err != nil { return FileConfig{}, err } - defer unlock() + // Joined, not chosen between: a release failure annotates the result + // instead of masking the mutation error that actually explains what went + // wrong. Reporting success after a failed unlock would claim a state the + // next mutation cannot reproduce. + defer func() { err = errors.Join(err, unlock()) }() cfg := FileConfig{} if data, err := os.ReadFile(path); err == nil { if err := json.Unmarshal(data, &cfg); err != nil { @@ -783,16 +844,20 @@ func SetSTTLocalEngine(path, binary, serverBinary, modelPath string, streaming b } // SetSTTProvider persists just the dictation batch provider, mirroring SetTheme. -func SetSTTProvider(path string, provider STTProviderKind) (FileConfig, error) { +func SetSTTProvider(path string, provider STTProviderKind) (result FileConfig, err error) { path = strings.TrimSpace(path) if path == "" { return FileConfig{}, fmt.Errorf("config path is required") } - unlock, err := lockConfigFile(path) + unlock, err := lockConfigFileFn(path) if err != nil { return FileConfig{}, err } - defer unlock() + // Joined, not chosen between: a release failure annotates the result + // instead of masking the mutation error that actually explains what went + // wrong. Reporting success after a failed unlock would claim a state the + // next mutation cannot reproduce. + defer func() { err = errors.Join(err, unlock()) }() cfg := FileConfig{} if data, err := os.ReadFile(path); err == nil { if err := json.Unmarshal(data, &cfg); err != nil { From 7407c33b720de7fc806e597bb63601b34df4a114 Mon Sep 17 00:00:00 2001 From: Amp Date: Thu, 27 Aug 2026 16:23:51 +0000 Subject: [PATCH 4/6] fix(cli): complete MCP config lock handling Amp-Thread-ID: https://ampcode.com/threads/T-01a043da-1703-70c5-9d8d-904cd8fd964b Co-authored-by: Pierre Bruno --- internal/cli/mcp_config.go | 38 ++++- internal/cli/mcp_config_lock_test.go | 242 ++++++++++++++++++++------- 2 files changed, 209 insertions(+), 71 deletions(-) diff --git a/internal/cli/mcp_config.go b/internal/cli/mcp_config.go index 1dec280cb..ac35d135d 100644 --- a/internal/cli/mcp_config.go +++ b/internal/cli/mcp_config.go @@ -29,6 +29,8 @@ type mcpWritableConfig struct { serverRaw map[string]json.RawMessage } +var lockMCPConfigFile = config.LockFile + // projectMCPConfigExists reports whether the workspace's project ./.zero/config.json // declares any MCP servers, so the trust notice fires only when project MCP config was // actually skipped (mirroring projectHooksFileExists / projectPluginsDirExists). A @@ -68,13 +70,17 @@ func runMCPAdd(args []string, stdout io.Writer, stderr io.Writer, deps appDeps) } // This edits the same user config document the config package mutates, with // the same read-modify-write + rename shape, so it takes the same - // cross-process lock. Without it, `zero mcp add` racing a provider or + // cross-process lock. Without it, an MCP config edit racing a provider or // preference write would silently drop whichever landed first (issue #832). - unlock, err := config.LockFile(configPath) + unlock, err := lockMCPConfigFile(configPath) if err != nil { return writeAppError(stderr, redaction.ErrorMessage(err, redaction.Options{}), exitCrash) } + released := false defer func() { + if released { + return + } // A failed release leaves the lock held for the rest of the process, so // exiting success here would claim a state the next config write cannot // reproduce. It must not mask a failure this command already reported. @@ -96,6 +102,10 @@ func runMCPAdd(args []string, stdout io.Writer, stderr io.Writer, deps appDeps) if err := writeMCPWritableConfig(configPath, cfg); err != nil { return writeAppError(stderr, redaction.ErrorMessage(err, redaction.Options{}), exitCrash) } + if err := unlock(); err != nil { + return writeAppError(stderr, redaction.ErrorMessage(err, redaction.Options{}), exitCrash) + } + released = true if options.json { payload := struct { @@ -150,13 +160,17 @@ func runMCPRemove(args []string, stdout io.Writer, stderr io.Writer, deps appDep } // This edits the same user config document the config package mutates, with // the same read-modify-write + rename shape, so it takes the same - // cross-process lock. Without it, `zero mcp add` racing a provider or + // cross-process lock. Without it, an MCP config edit racing a provider or // preference write would silently drop whichever landed first (issue #832). - unlock, err := config.LockFile(configPath) + unlock, err := lockMCPConfigFile(configPath) if err != nil { return writeAppError(stderr, redaction.ErrorMessage(err, redaction.Options{}), exitCrash) } + released := false defer func() { + if released { + return + } // A failed release leaves the lock held for the rest of the process, so // exiting success here would claim a state the next config write cannot // reproduce. It must not mask a failure this command already reported. @@ -177,6 +191,10 @@ func runMCPRemove(args []string, stdout io.Writer, stderr io.Writer, deps appDep return writeAppError(stderr, redaction.ErrorMessage(err, redaction.Options{}), exitCrash) } } + if err := unlock(); err != nil { + return writeAppError(stderr, redaction.ErrorMessage(err, redaction.Options{}), exitCrash) + } + released = true if options.json { payload := struct { @@ -228,13 +246,17 @@ func runMCPToggle(args []string, stdout io.Writer, stderr io.Writer, deps appDep } // This edits the same user config document the config package mutates, with // the same read-modify-write + rename shape, so it takes the same - // cross-process lock. Without it, `zero mcp add` racing a provider or + // cross-process lock. Without it, an MCP config edit racing a provider or // preference write would silently drop whichever landed first (issue #832). - unlock, err := config.LockFile(configPath) + unlock, err := lockMCPConfigFile(configPath) if err != nil { return writeAppError(stderr, redaction.ErrorMessage(err, redaction.Options{}), exitCrash) } + released := false defer func() { + if released { + return + } // A failed release leaves the lock held for the rest of the process, so // exiting success here would claim a state the next config write cannot // reproduce. It must not mask a failure this command already reported. @@ -258,6 +280,10 @@ func runMCPToggle(args []string, stdout io.Writer, stderr io.Writer, deps appDep return writeAppError(stderr, redaction.ErrorMessage(err, redaction.Options{}), exitCrash) } } + if err := unlock(); err != nil { + return writeAppError(stderr, redaction.ErrorMessage(err, redaction.Options{}), exitCrash) + } + released = true if options.json { payload := struct { diff --git a/internal/cli/mcp_config_lock_test.go b/internal/cli/mcp_config_lock_test.go index 423e1facd..917ff1d82 100644 --- a/internal/cli/mcp_config_lock_test.go +++ b/internal/cli/mcp_config_lock_test.go @@ -2,19 +2,81 @@ package cli import ( "bytes" + "errors" + "os" "path/filepath" + "strings" "testing" "time" "github.com/Gitlawb/zero/internal/config" ) -// TestRunMCPAddParticipatesInConfigLock covers the half of issue #832 that -// lives outside internal/config. `zero mcp add` reads the SAME user config -// document, edits it, and republishes it with the same temp-file+rename shape -// as the config package's mutators. Locking only the config package would leave -// this writer free to clobber a concurrent provider or preference update, and -// be clobbered by one, with the file still valid JSON afterwards. +type mcpConfigLockCase struct { + name string + args []string + disabled bool + server bool +} + +func mcpConfigLockCases() []mcpConfigLockCase { + return []mcpConfigLockCase{ + {name: "add", args: []string{"mcp", "add", "docs", "--", "docs-mcp"}}, + {name: "remove", args: []string{"mcp", "remove", "docs"}, server: true}, + {name: "enable", args: []string{"mcp", "enable", "docs"}, server: true, disabled: true}, + {name: "disable", args: []string{"mcp", "disable", "docs"}, server: true}, + } +} + +func seedMCPConfigLockCase(t *testing.T, path string, testCase mcpConfigLockCase) { + t.Helper() + servers := map[string]config.MCPServerConfig{} + if testCase.server { + servers["docs"] = config.MCPServerConfig{Type: "stdio", Command: "docs-mcp", Disabled: testCase.disabled} + } + writeMCPCommandConfig(t, path, config.FileConfig{ + ActiveProvider: "seed", + Providers: []config.ProviderProfile{{Name: "seed", Model: "seed-model"}}, + MCP: config.MCPConfig{Servers: servers}, + }) +} + +func assertMCPConfigLockMutation(t *testing.T, path string, testCase mcpConfigLockCase) { + t.Helper() + cfg := readMCPCommandConfig(t, path) + server, exists := cfg.MCP.Servers["docs"] + switch testCase.name { + case "add": + if !exists { + t.Fatal("mcp add did not persist the server") + } + case "remove": + if exists { + t.Fatal("mcp remove left the server configured") + } + case "enable": + if !exists || server.Disabled { + t.Fatalf("mcp enable result = %+v, exists=%v", server, exists) + } + case "disable": + if !exists || !server.Disabled { + t.Fatalf("mcp disable result = %+v, exists=%v", server, exists) + } + } + if cfg.Preferences.Theme != "dracula" { + t.Errorf("theme update was lost: theme = %q, want dracula", cfg.Preferences.Theme) + } + if cfg.ActiveProvider != "seed" || len(cfg.Providers) != 1 || cfg.Providers[0].Name != "seed" { + t.Errorf("seeded provider was lost: active=%q providers=%#v", cfg.ActiveProvider, cfg.Providers) + } +} + +// TestRunMCPConfigCommandsParticipateInConfigLock covers the half of issue #832 +// that lives outside internal/config. MCP config commands read the SAME user +// config document, edit it, and republish it with the same temp-file+rename +// shape as the config package's mutators. Locking only the config package would +// leave these writers free to clobber a concurrent provider or preference +// update, and be clobbered by one, with the file still valid JSON afterwards. // // Racing the two writers and hoping to observe a lost update is unreliable — // the interleaving that loses one is narrow, and the test passed consistently @@ -22,74 +84,124 @@ import ( // and is deterministic: while the config lock is held elsewhere, the MCP // writer's update CANNOT land. Once released it completes, and both updates // survive. -func TestRunMCPAddParticipatesInConfigLock(t *testing.T) { - configPath := filepath.Join(t.TempDir(), "zero", "config.json") - if _, err := config.UpsertProvider(configPath, config.ProviderProfile{Name: "seed", Model: "seed-model"}, true); err != nil { - t.Fatalf("seed config: %v", err) - } +func TestRunMCPConfigCommandsParticipateInConfigLock(t *testing.T) { + for _, testCase := range mcpConfigLockCases() { + t.Run(testCase.name, func(t *testing.T) { + configPath := filepath.Join(t.TempDir(), "zero", "config.json") + seedMCPConfigLockCase(t, configPath, testCase) + before, err := os.ReadFile(configPath) + if err != nil { + t.Fatal(err) + } - unlock, err := config.LockFile(configPath) - if err != nil { - t.Fatalf("acquire config lock: %v", err) - } - released := false - release := func() { - if !released { - released = true - unlock() - } - } - defer release() + unlock, err := config.LockFile(configPath) + if err != nil { + t.Fatalf("acquire config lock: %v", err) + } + released := false + release := func() { + if !released { + released = true + if err := unlock(); err != nil { + t.Fatalf("release config lock: %v", err) + } + } + } + defer release() - var stdout, stderr bytes.Buffer - done := make(chan int, 1) - go func() { - done <- runWithDeps([]string{"mcp", "add", "docs", "--", "docs-mcp"}, &stdout, &stderr, appDeps{ - userConfigPath: func() (string, error) { return configPath, nil }, - }) - }() + var stdout, stderr bytes.Buffer + done := make(chan int, 1) + go func() { + done <- runWithDeps(testCase.args, &stdout, &stderr, appDeps{ + userConfigPath: func() (string, error) { return configPath, nil }, + }) + }() - // The MCP writer is now contending for a lock this test holds. Its write - // must not appear until the lock is released. - for range 20 { - if servers := readMCPCommandConfig(t, configPath).MCP.Servers; len(servers) != 0 { - t.Fatalf("mcp add wrote %#v while the config lock was held; it does not take the lock", servers) - } - select { - case exitCode := <-done: - t.Fatalf("mcp add completed (exit %d) while the config lock was held; it does not take the lock", exitCode) - default: - } - time.Sleep(5 * time.Millisecond) - } + for range 20 { + after, err := os.ReadFile(configPath) + if err != nil { + t.Fatal(err) + } + if !bytes.Equal(after, before) { + t.Fatalf("mcp %s changed config while its lock was held", testCase.name) + } + select { + case exitCode := <-done: + t.Fatalf("mcp %s completed (exit %d) while its lock was held", testCase.name, exitCode) + default: + } + time.Sleep(5 * time.Millisecond) + } - release() + release() + select { + case exitCode := <-done: + if exitCode != exitSuccess { + t.Fatalf("mcp %s exitCode = %d stderr=%s", testCase.name, exitCode, stderr.String()) + } + case <-time.After(30 * time.Second): + t.Fatalf("mcp %s did not finish after the config lock was released", testCase.name) + } - select { - case exitCode := <-done: - if exitCode != exitSuccess { - t.Fatalf("mcp add exitCode = %d stderr=%s", exitCode, stderr.String()) - } - case <-time.After(30 * time.Second): - t.Fatal("mcp add did not finish after the config lock was released") + if _, err := config.SetTheme(configPath, "dracula"); err != nil { + t.Fatalf("SetTheme: %v", err) + } + assertMCPConfigLockMutation(t, configPath, testCase) + }) } +} - // A config mutation after the MCP write must keep it, and vice versa. - if _, err := config.SetTheme(configPath, "dracula"); err != nil { - t.Fatalf("SetTheme: %v", err) - } +func TestRunMCPConfigCommandsReportLockAcquisitionFailure(t *testing.T) { + sentinel := errors.New("injected lock acquisition failure") + original := lockMCPConfigFile + lockMCPConfigFile = func(string) (func() error, error) { return nil, sentinel } + t.Cleanup(func() { lockMCPConfigFile = original }) - cfg := readMCPCommandConfig(t, configPath) - if _, ok := cfg.MCP.Servers["docs"]; !ok { - t.Errorf("mcp add update was lost: servers = %#v", cfg.MCP.Servers) - } - if cfg.Preferences.Theme != "dracula" { - t.Errorf("theme update was lost: theme = %q, want dracula", cfg.Preferences.Theme) + for _, testCase := range mcpConfigLockCases() { + t.Run(testCase.name, func(t *testing.T) { + configPath := filepath.Join(t.TempDir(), "zero", "config.json") + seedMCPConfigLockCase(t, configPath, testCase) + before, err := os.ReadFile(configPath) + if err != nil { + t.Fatal(err) + } + var stdout, stderr bytes.Buffer + exitCode := runWithDeps(testCase.args, &stdout, &stderr, appDeps{ + userConfigPath: func() (string, error) { return configPath, nil }, + }) + if exitCode != exitCrash || stdout.Len() != 0 || !strings.Contains(stderr.String(), sentinel.Error()) { + t.Fatalf("exit=%d stdout=%q stderr=%q, want lock error without success output", exitCode, stdout.String(), stderr.String()) + } + after, err := os.ReadFile(configPath) + if err != nil { + t.Fatal(err) + } + if !bytes.Equal(after, before) { + t.Fatal("command changed config after lock acquisition failed") + } + }) } - if cfg.ActiveProvider != "seed" { - t.Errorf("activeProvider = %q, want the seeded value preserved", cfg.ActiveProvider) +} + +func TestRunMCPConfigCommandsReleaseBeforeSuccessOutput(t *testing.T) { + sentinel := errors.New("injected lock release failure") + original := lockMCPConfigFile + lockMCPConfigFile = func(string) (func() error, error) { + return func() error { return sentinel }, nil } - if len(cfg.Providers) != 1 || cfg.Providers[0].Name != "seed" { - t.Errorf("seeded provider was lost: providers = %#v", cfg.Providers) + t.Cleanup(func() { lockMCPConfigFile = original }) + + for _, testCase := range mcpConfigLockCases() { + t.Run(testCase.name, func(t *testing.T) { + configPath := filepath.Join(t.TempDir(), "zero", "config.json") + seedMCPConfigLockCase(t, configPath, testCase) + var stdout, stderr bytes.Buffer + exitCode := runWithDeps(testCase.args, &stdout, &stderr, appDeps{ + userConfigPath: func() (string, error) { return configPath, nil }, + }) + if exitCode != exitCrash || stdout.Len() != 0 || !strings.Contains(stderr.String(), sentinel.Error()) { + t.Fatalf("exit=%d stdout=%q stderr=%q, want unlock error before success output", exitCode, stdout.String(), stderr.String()) + } + }) } } From 6eec32e76f5db94251d30f45d443094cde8261ce Mon Sep 17 00:00:00 2001 From: PierrunoYT Date: Thu, 27 Aug 2026 21:14:59 +0200 Subject: [PATCH 5/6] test(cli): synchronize MCP config lock assertion Amp-Thread-ID: https://ampcode.com/threads/T-01a0448f-5860-721c-8a47-5119fc57f685 Co-authored-by: Amp --- internal/cli/mcp_config_lock_test.go | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/internal/cli/mcp_config_lock_test.go b/internal/cli/mcp_config_lock_test.go index 917ff1d82..6b813caec 100644 --- a/internal/cli/mcp_config_lock_test.go +++ b/internal/cli/mcp_config_lock_test.go @@ -109,6 +109,14 @@ func TestRunMCPConfigCommandsParticipateInConfigLock(t *testing.T) { } defer release() + original := lockMCPConfigFile + lockAttempted := make(chan struct{}) + lockMCPConfigFile = func(path string) (func() error, error) { + close(lockAttempted) + return original(path) + } + t.Cleanup(func() { lockMCPConfigFile = original }) + var stdout, stderr bytes.Buffer done := make(chan int, 1) go func() { @@ -117,6 +125,12 @@ func TestRunMCPConfigCommandsParticipateInConfigLock(t *testing.T) { }) }() + select { + case <-lockAttempted: + case <-time.After(30 * time.Second): + t.Fatalf("mcp %s did not attempt config lock acquisition", testCase.name) + } + for range 20 { after, err := os.ReadFile(configPath) if err != nil { From 60d701d2bace356a8ff4c1bf9ea88e0aa143324b Mon Sep 17 00:00:00 2001 From: PierrunoYT Date: Sat, 29 Aug 2026 11:19:16 +0200 Subject: [PATCH 6/6] fix(tui): persist provider model selection atomically Amp-Thread-ID: https://ampcode.com/threads/T-01a04c92-2d1d-7508-91bc-416341b7e8b0 Co-authored-by: Amp --- internal/config/writer.go | 45 ++++++++++ internal/config/writer_test.go | 26 ++++++ internal/tui/command_center.go | 54 +++++++----- internal/tui/model.go | 7 +- internal/tui/picker.go | 26 ++++-- internal/tui/picker_test.go | 119 +++++++++++++++++++++++++-- internal/tui/profile_command_test.go | 6 +- internal/tui/provider_manager.go | 2 +- 8 files changed, 246 insertions(+), 39 deletions(-) diff --git a/internal/config/writer.go b/internal/config/writer.go index 92b89f0c5..4bc95bf6d 100644 --- a/internal/config/writer.go +++ b/internal/config/writer.go @@ -225,6 +225,51 @@ func SetActiveProvider(path string, name string) (result FileConfig, err error) return FileConfig{}, fmt.Errorf("provider %q not found", name) } +// SetActiveProviderModel persists a provider selection as one lock-held +// read-modify-write. The active provider and that provider's model are one UI +// choice; publishing them separately can leave a partially applied selection +// if another writer wins the lock between calls. +func SetActiveProviderModel(path string, name string, model string) (result FileConfig, err error) { + path = strings.TrimSpace(path) + if path == "" { + return FileConfig{}, fmt.Errorf("config path is required") + } + name = strings.TrimSpace(name) + if name == "" { + return FileConfig{}, fmt.Errorf("provider name is required") + } + model = strings.TrimSpace(model) + if model == "" { + return FileConfig{}, fmt.Errorf("model is required") + } + + unlock, err := lockConfigFileFn(path) + if err != nil { + return FileConfig{}, err + } + defer func() { err = errors.Join(err, unlock()) }() + + data, err := os.ReadFile(path) + if err != nil { + return FileConfig{}, fmt.Errorf("read config %s: %w", path, err) + } + cfg := FileConfig{} + if err := json.Unmarshal(data, &cfg); err != nil { + return FileConfig{}, fmt.Errorf("invalid config JSON %s: %w", path, err) + } + for index := range cfg.Providers { + if strings.EqualFold(strings.TrimSpace(cfg.Providers[index].Name), name) { + cfg.ActiveProvider = cfg.Providers[index].Name + cfg.Providers[index].Model = model + if err := writeConfigFile(path, cfg); err != nil { + return FileConfig{}, err + } + return cfg, nil + } + } + return FileConfig{}, fmt.Errorf("provider %q not found", name) +} + // ProviderPersisted reports whether a provider profile named name actually has // a row in the config file at path. A provider can appear in the resolved/ // in-memory provider list without ever being written to config.json — e.g. diff --git a/internal/config/writer_test.go b/internal/config/writer_test.go index c66fc26ba..0deff4827 100644 --- a/internal/config/writer_test.go +++ b/internal/config/writer_test.go @@ -151,6 +151,32 @@ func TestSetActiveProviderTightensExistingConfigFilePermissions(t *testing.T) { } } +func TestSetActiveProviderModelPersistsSelectionTogether(t *testing.T) { + path := filepath.Join(t.TempDir(), "zero.json") + writeConfigFixture(t, path, FileConfig{ + ActiveProvider: "openai", + Providers: []ProviderProfile{ + {Name: "openai", ProviderKind: ProviderKindOpenAI, Model: "gpt-4.1"}, + {Name: "Anthropic", ProviderKind: ProviderKindAnthropic, Model: "old-model"}, + }, + }, 0o600) + + cfg, err := SetActiveProviderModel(path, " anthropic ", " claude-sonnet-4.5 ") + if err != nil { + t.Fatalf("SetActiveProviderModel() error = %v", err) + } + if cfg.ActiveProvider != "Anthropic" || cfg.Providers[1].Model != "claude-sonnet-4.5" { + t.Fatalf("returned selection = active %q model %q", cfg.ActiveProvider, cfg.Providers[1].Model) + } + persisted := readConfigFixture(t, path) + if persisted.ActiveProvider != "Anthropic" || persisted.Providers[1].Model != "claude-sonnet-4.5" { + t.Fatalf("persisted selection = active %q model %q", persisted.ActiveProvider, persisted.Providers[1].Model) + } + if persisted.Providers[0].Model != "gpt-4.1" { + t.Fatalf("unrelated provider changed: %#v", persisted.Providers[0]) + } +} + func TestSetProviderModelUpdatesConfiguredProvider(t *testing.T) { path := filepath.Join(t.TempDir(), "zero.json") writeConfigFixture(t, path, FileConfig{ diff --git a/internal/tui/command_center.go b/internal/tui/command_center.go index f922ee5bd..a4491496c 100644 --- a/internal/tui/command_center.go +++ b/internal/tui/command_center.go @@ -523,21 +523,22 @@ func (m model) handleModelCommand(args string) (model, string) { // picker calls this when a model from a non-active provider is chosen, so the // picker can list every saved provider and switch across them (like a unified // provider+model selector). The key is loaded from the encrypted store / env. -// The returned bool reports whether the switch actually committed — callers -// that branch on the outcome (the provider manager) must use it, never the +// The returned bool reports whether the in-session switch committed — callers +// that branch on that outcome (the provider manager) must use it, never the // display text: UI copy is not a control-flow contract (a refusal quoting a // provider name could contain any substring, and rewording the success line -// must not change behavior). -func (m model) switchProviderModel(providerName, modelID string) (model, string, bool, tea.Cmd) { +// must not change behavior). The returned error reports that the in-session +// switch succeeded but its provider/model transaction was not durably saved. +func (m model) switchProviderModel(providerName, modelID string) (model, string, bool, tea.Cmd, error) { if m.pending { - return m, "Model\nCannot switch providers while a run is active.", false, nil + return m, "Model\nCannot switch providers while a run is active.", false, nil, nil } if m.newProvider == nil { - return m, "Model\nProvider rebuild is not available for this TUI session.", false, nil + return m, "Model\nProvider rebuild is not available for this TUI session.", false, nil, nil } target, ok := m.savedProviderByName(providerName) if !ok { - return m, "Model\nunknown provider " + strconv.Quote(providerName), false, nil + return m, "Model\nunknown provider " + strconv.Quote(providerName), false, nil, nil } previousProviderName := m.providerName previousModel := m.modelName @@ -551,11 +552,11 @@ func (m model) switchProviderModel(providerName, modelID string) (model, string, // keyless on purpose so newProvider attaches the bearer resolver + login key. if strings.TrimSpace(target.APIKey) == "" && strings.TrimSpace(target.AuthHeaderValue) == "" && (!hasDescriptor || !descriptor.Local) && !oauthLoginAvailable(target) { - return m, "Model\nprovider " + strconv.Quote(providerName) + " has no usable credential — run setup or `zero auth login " + providerName + "`.", false, nil + return m, "Model\nprovider " + strconv.Quote(providerName) + " has no usable credential — run setup or `zero auth login " + providerName + "`.", false, nil, nil } next, err := m.newProvider(target) if err != nil { - return m, "Model\n" + redaction.RedactString(err.Error(), redaction.Options{ExtraSecretValues: []string{target.APIKey}}), false, nil + return m, "Model\n" + redaction.RedactString(err.Error(), redaction.Options{ExtraSecretValues: []string{target.APIKey}}), false, nil, nil } m.provider = next m.providerProfile = target @@ -569,20 +570,24 @@ func (m model) switchProviderModel(providerName, modelID string) (model, string, // carried (pre-existing behavior) while the profile's own fill stays // conservative — it only ever applies where support is known. m = m.reconcileProfileAfterModelSwitch(m.availableReasoningEfforts()) - // Record the outgoing pair too — see the matching comment in - // handleModelCommand for why (keeps the session's starting model from - // silently dropping out of "Recent" on the first switch away from it). - // recordRecentModels batches both into a single normalize+persist instead - // of two separate disk writes for this one switch. - m = m.recordRecentModels( - config.RecentModelEntry{Provider: previousProviderName, Model: previousModel}, - config.RecentModelEntry{Provider: target.Name, Model: target.Model}, - ) // Keep sub-agent child processes on the same provider we just switched to. config.SetActiveProviderEnv(target.Name) - if strings.TrimSpace(m.userConfigPath) != "" { - _, _ = config.SetActiveProvider(m.userConfigPath, target.Name) - _, _ = config.SetProviderModel(m.userConfigPath, target.Name, target.Model) + path := strings.TrimSpace(m.userConfigPath) + var persistErr error + if path != "" { + _, persistErr = config.SetActiveProviderModel(path, target.Name, target.Model) + } + recent := []config.RecentModelEntry{ + {Provider: previousProviderName, Model: previousModel}, + {Provider: target.Name, Model: target.Model}, + } + if persistErr == nil { + // Record the outgoing pair too — see the matching comment in + // handleModelCommand for why. Persist history only after the selection + // transaction succeeds, so one unavailable lock causes one wait. + m = m.recordRecentModels(recent...) + } else { + m, _ = m.updateRecentModels(recent...) } // Warm discovery for the provider we just switched to, same as Init() does // for the provider active at launch — otherwise the context-usage gauge has @@ -597,10 +602,15 @@ func (m model) switchProviderModel(providerName, modelID string) (model, string, } } status := fmt.Sprintf("Model\nSwitched to %s · %s", target.Name, target.Model) + if path != "" && persistErr == nil { + status += " · saved" + } else if persistErr != nil { + status += " · not saved (" + persistErr.Error() + ")" + } if warn := m.visionDropWarning(); warn != "" { status += "\n" + warn } - return m, status, true, tea.Batch(cmds...) + return m, status, true, tea.Batch(cmds...), persistErr } // profileWithCredential fills a profile's APIKey for provider construction the same diff --git a/internal/tui/model.go b/internal/tui/model.go index 9473de06a..ea2c5ae4f 100644 --- a/internal/tui/model.go +++ b/internal/tui/model.go @@ -4464,11 +4464,12 @@ func (m model) choosePicker() (tea.Model, tea.Cmd) { case pickerModel: previousProvider, previousModel := m.providerName, m.modelName text := "" + var switchPersistErr error owner := strings.TrimSpace(item.OwnerProvider) _, ownerIsSavedProvider := m.savedProviderByName(owner) if owner != "" && !strings.EqualFold(owner, strings.TrimSpace(m.providerName)) && ownerIsSavedProvider { // A model from another saved provider: switch provider + model together. - m, text, _, cmd = m.switchProviderModel(owner, item.Value) + m, text, _, cmd, switchPersistErr = m.switchProviderModel(owner, item.Value) } else { // OwnerProvider is blank, matches the active provider, or (registry-fallback // / stale-history rows) doesn't resolve to any saved provider: apply against @@ -4476,6 +4477,10 @@ func (m model) choosePicker() (tea.Model, tea.Cmd) { m, text = m.handleModelCommand(item.Value) } if m.providerName != previousProvider || m.modelName != previousModel { + if switchPersistErr != nil { + notice := m.modelAppliedNotice() + " · not saved (" + switchPersistErr.Error() + ")" + return m.showTransientNoticeInline(notice, transientNoticeWarning), cmd + } return m.showTransientNoticeInline(m.modelAppliedNotice(), transientNoticeSuccess), cmd } m.transcript = reduceTranscript(m.transcript, transcriptAction{kind: actionAppendSystem, text: text}) diff --git a/internal/tui/picker.go b/internal/tui/picker.go index 563dffe62..2a0da566c 100644 --- a/internal/tui/picker.go +++ b/internal/tui/picker.go @@ -945,6 +945,23 @@ func (m model) persistFavoriteModels() error { // of the session. Returns the receiver unchanged (no re-normalization, no // write) when every pair has a blank model id. func (m model) recordRecentModels(pairs ...config.RecentModelEntry) model { + m, changed := m.updateRecentModels(pairs...) + if !changed { + return m + } + if path := strings.TrimSpace(m.userConfigPath); path != "" { + if _, err := config.SetRecentModels(path, m.recentModels); err != nil { + m.transcript = reduceTranscript(m.transcript, transcriptAction{kind: actionAppendError, text: "recent model save error: " + err.Error()}) + } + } + return m +} + +// updateRecentModels performs recordRecentModels' in-memory half. A model +// switch whose selection transaction failed still belongs in this session's +// picker history, but must not make another blocking persistence attempt while +// the same config lock is unavailable. +func (m model) updateRecentModels(pairs ...config.RecentModelEntry) (model, bool) { entries := append([]config.RecentModelEntry{}, m.recentModels...) changed := false for _, pair := range pairs { @@ -956,15 +973,10 @@ func (m model) recordRecentModels(pairs ...config.RecentModelEntry) model { entries = append([]config.RecentModelEntry{{Provider: strings.TrimSpace(pair.Provider), Model: modelID}}, entries...) } if !changed { - return m + return m, false } m.recentModels = normalizeRecentModelEntries(entries) - if path := strings.TrimSpace(m.userConfigPath); path != "" { - if _, err := config.SetRecentModels(path, m.recentModels); err != nil { - m.transcript = reduceTranscript(m.transcript, transcriptAction{kind: actionAppendError, text: "recent model save error: " + err.Error()}) - } - } - return m + return m, true } // normalizeRecentModelEntries trims, drops entries with no model id, diff --git a/internal/tui/picker_test.go b/internal/tui/picker_test.go index 817329402..cf60b186d 100644 --- a/internal/tui/picker_test.go +++ b/internal/tui/picker_test.go @@ -1,6 +1,7 @@ package tui import ( + "bytes" "context" "encoding/json" "errors" @@ -812,6 +813,12 @@ func TestModelCommandRecordsAndPersistsRecentHistory(t *testing.T) { // history, tagged with the provider actually switched to. func TestSwitchProviderModelRecordsRecentHistory(t *testing.T) { configPath := filepath.Join(t.TempDir(), "zero", "config.json") + if _, err := config.UpsertProvider(configPath, config.ProviderProfile{Name: "openai", CatalogID: "openai", Model: "gpt-5.1"}, true); err != nil { + t.Fatalf("seed openai provider: %v", err) + } + if _, err := config.UpsertProvider(configPath, config.ProviderProfile{Name: "ollama", CatalogID: "ollama", ProviderKind: config.ProviderKindOpenAICompatible, BaseURL: "http://localhost:11434/v1", Model: "kimi-k2.7-code:cloud"}, false); err != nil { + t.Fatalf("seed ollama provider: %v", err) + } m := newModel(context.Background(), Options{ UserConfigPath: configPath, ProviderName: "openai", @@ -827,8 +834,8 @@ func TestSwitchProviderModelRecordsRecentHistory(t *testing.T) { }, }) - next, status, _, _ := m.switchProviderModel("ollama", "kimi-k2.7-code:cloud") - wantStatus := "Model\nSwitched to ollama · kimi-k2.7-code:cloud" + next, status, _, _, _ := m.switchProviderModel("ollama", "kimi-k2.7-code:cloud") + wantStatus := "Model\nSwitched to ollama · kimi-k2.7-code:cloud · saved" if status != wantStatus { t.Fatalf("switchProviderModel() status = %q, want %q (a mismatch here means the switch itself failed, not the recentModels assertion below)", status, wantStatus) } @@ -846,6 +853,108 @@ func TestSwitchProviderModelRecordsRecentHistory(t *testing.T) { } } +func TestSwitchProviderModelReportsLockedConfigAsNotSavedWithoutPartialSelection(t *testing.T) { + configPath := filepath.Join(t.TempDir(), "zero", "config.json") + if _, err := config.UpsertProvider(configPath, config.ProviderProfile{Name: "openai", CatalogID: "openai", Model: "gpt-5.1"}, true); err != nil { + t.Fatalf("seed openai provider: %v", err) + } + if _, err := config.UpsertProvider(configPath, config.ProviderProfile{ + Name: "ollama", CatalogID: "ollama", ProviderKind: config.ProviderKindOpenAICompatible, + BaseURL: "http://localhost:11434/v1", Model: "old-model", + }, false); err != nil { + t.Fatalf("seed ollama provider: %v", err) + } + before, err := os.ReadFile(configPath) + if err != nil { + t.Fatalf("read seeded config: %v", err) + } + unlock, err := config.LockFile(configPath) + if err != nil { + t.Fatalf("hold config lock: %v", err) + } + t.Cleanup(func() { + if err := unlock(); err != nil { + t.Errorf("release config lock: %v", err) + } + }) + + t.Setenv("ZERO_PROVIDER", "openai") + m := newModel(context.Background(), Options{ + UserConfigPath: configPath, + ProviderName: "openai", + ModelName: "gpt-5.1", + Provider: &fakeProvider{}, + ProviderProfile: config.ProviderProfile{Name: "openai", CatalogID: "openai", Model: "gpt-5.1"}, + SavedProviders: []config.ProviderProfile{ + {Name: "openai", CatalogID: "openai", Model: "gpt-5.1"}, + {Name: "ollama", CatalogID: "ollama", ProviderKind: config.ProviderKindOpenAICompatible, BaseURL: "http://localhost:11434/v1", Model: "old-model"}, + }, + NewProvider: func(config.ProviderProfile) (zeroruntime.Provider, error) { + return &fakeProvider{}, nil + }, + }) + + started := time.Now() + next, status, switched, _, persistErr := m.switchProviderModel("ollama", "new-model") + if elapsed := time.Since(started); elapsed > 12*time.Second { + t.Fatalf("cross-provider switch blocked for %s; want one bounded persistence attempt", elapsed) + } + if !switched || next.providerName != "ollama" || next.modelName != "new-model" { + t.Fatalf("in-session switch was lost: switched=%v provider=%q model=%q", switched, next.providerName, next.modelName) + } + if persistErr == nil || !strings.Contains(persistErr.Error(), "timed out acquiring config lock") { + t.Fatalf("persistence error = %v, want config lock timeout", persistErr) + } + if !strings.Contains(status, "not saved (") || !strings.Contains(status, "timed out acquiring config lock") { + t.Fatalf("switch status = %q, want concrete not-saved lock failure", status) + } + after, err := os.ReadFile(configPath) + if err != nil { + t.Fatalf("read config after failed switch: %v", err) + } + if !bytes.Equal(after, before) { + t.Fatalf("failed switch partially changed config\nbefore: %s\nafter: %s", before, after) + } +} + +func TestCrossProviderPickerShowsPersistenceFailure(t *testing.T) { + root := t.TempDir() + blockedParent := filepath.Join(root, "not-a-directory") + if err := os.WriteFile(blockedParent, []byte("block config directory creation"), 0o600); err != nil { + t.Fatalf("write blocking parent: %v", err) + } + m := newModel(context.Background(), Options{ + UserConfigPath: filepath.Join(blockedParent, "config.json"), + ProviderName: "openai", + ModelName: "gpt-5.1", + Provider: &fakeProvider{}, + ProviderProfile: config.ProviderProfile{Name: "openai", CatalogID: "openai", Model: "gpt-5.1"}, + SavedProviders: []config.ProviderProfile{ + {Name: "openai", CatalogID: "openai", Model: "gpt-5.1"}, + {Name: "ollama", CatalogID: "ollama", ProviderKind: config.ProviderKindOpenAICompatible, BaseURL: "http://localhost:11434/v1", Model: "old-model"}, + }, + NewProvider: func(config.ProviderProfile) (zeroruntime.Provider, error) { + return &fakeProvider{}, nil + }, + }) + m.picker = &commandPicker{ + kind: pickerModel, + items: []pickerItem{{Value: "new-model", OwnerProvider: "ollama"}}, + } + + updated, _ := m.choosePicker() + next := updated.(model) + if next.providerName != "ollama" || next.modelName != "new-model" { + t.Fatalf("in-session picker switch was lost: provider=%q model=%q", next.providerName, next.modelName) + } + if !strings.Contains(next.transientNotice.text, "not saved (") { + t.Fatalf("picker notice = %q, want persistence failure", next.transientNotice.text) + } + if next.transientNotice.tone != transientNoticeWarning { + t.Fatalf("picker notice tone = %v, want warning", next.transientNotice.tone) + } +} + func TestModelCommandAcceptsManualModelForCustomProvider(t *testing.T) { var captured config.ProviderProfile m := newModel(context.Background(), Options{ @@ -1347,7 +1456,7 @@ func TestSwitchProviderModelWarmsDiscoveryForTheNewProvider(t *testing.T) { }, }) - next, text, ok, cmd := m.switchProviderModel("ollama", "kimi-k2.7-code:cloud") + next, text, ok, cmd, _ := m.switchProviderModel("ollama", "kimi-k2.7-code:cloud") if !ok || !strings.Contains(text, "Switched to ollama") { t.Fatalf("switch notice = %q (ok=%v), want a committed switch", text, ok) } @@ -1439,7 +1548,7 @@ func TestSwitchProviderModelUsesOAuthLoginWithoutInliningBearer(t *testing.T) { }, }) - next, text, ok, _ := m.switchProviderModel("chatgpt", "gpt-5.5") + next, text, ok, _, _ := m.switchProviderModel("chatgpt", "gpt-5.5") if !ok || !strings.Contains(text, "Switched to chatgpt") { t.Fatalf("switch should succeed on the stored OAuth login, got %q (ok=%v)", text, ok) } @@ -1472,7 +1581,7 @@ func TestSwitchProviderModelStillRejectsProviderWithNoCredential(t *testing.T) { }, }) - _, text, ok, _ := m.switchProviderModel("chatgpt", "gpt-5.5") + _, text, ok, _, _ := m.switchProviderModel("chatgpt", "gpt-5.5") if ok || !strings.Contains(text, "no usable credential") { t.Fatalf("expected the credential gate to refuse, got %q (ok=%v)", text, ok) } diff --git a/internal/tui/profile_command_test.go b/internal/tui/profile_command_test.go index f9884c7f9..51a9f7540 100644 --- a/internal/tui/profile_command_test.go +++ b/internal/tui/profile_command_test.go @@ -46,7 +46,7 @@ func TestProfileEffortReconciledOnModelSwitch(t *testing.T) { // Supported -> unsupported: the profile-applied level must not survive // onto a model with no effort ring. - m, text, ok, _ := m.switchProviderModel("ollama", "kimi-k2.7-code:cloud") + m, text, ok, _, _ := m.switchProviderModel("ollama", "kimi-k2.7-code:cloud") if !ok { t.Fatalf("switch to ollama failed: %q", text) } @@ -62,7 +62,7 @@ func TestProfileEffortReconciledOnModelSwitch(t *testing.T) { // Unsupported -> supported: switching (back) to a supporting model must // behave like selecting the profile there. - m, text, ok, _ = m.switchProviderModel("anthropic", "claude-sonnet-4.5") + m, text, ok, _, _ = m.switchProviderModel("anthropic", "claude-sonnet-4.5") if !ok { t.Fatalf("switch back to anthropic failed: %q", text) } @@ -183,7 +183,7 @@ func TestProfileEffortTouchedSurvivesModelSwitch(t *testing.T) { m, _ = m.handleProfileCommand("fast") m, _ = m.handleEffortCommand("high") - m, text, ok, _ := m.switchProviderModel("ollama", "kimi-k2.7-code:cloud") + m, text, ok, _, _ := m.switchProviderModel("ollama", "kimi-k2.7-code:cloud") if !ok { t.Fatalf("switch to ollama failed: %q", text) } diff --git a/internal/tui/provider_manager.go b/internal/tui/provider_manager.go index 6816074d7..e9041cdde 100644 --- a/internal/tui/provider_manager.go +++ b/internal/tui/provider_manager.go @@ -316,7 +316,7 @@ func (m model) activateManagerSelection() (model, tea.Cmd) { wizard.manageStatus = "Cannot switch providers while a run is active." return m, nil } - next, text, switched, cmd := m.switchProviderModel(row.profile.Name, row.profile.Model) + next, text, switched, cmd, _ := m.switchProviderModel(row.profile.Name, row.profile.Model) if switched { next.providerWizard = nil next.transcript = reduceTranscript(next.transcript, transcriptAction{kind: actionAppendSystem, text: text})