From 1e3bccaa930a873076d84cb856b22153381b83de Mon Sep 17 00:00:00 2001 From: PierrunoYT Date: Tue, 25 Aug 2026 22:07:23 +0200 Subject: [PATCH 1/7] 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/7] 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/7] 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/7] 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/7] 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/7] 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}) From a20043491ca2be0f6e9be7e70a4e8dc7d94d3ebf Mon Sep 17 00:00:00 2001 From: PierrunoYT Date: Thu, 3 Sep 2026 21:49:04 +0200 Subject: [PATCH 7/7] fix(tui): surface unsaved model selections on every switch path MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit choosePicker replaces handleModelCommand's status text with its own applied-notice, so the "· not saved (…)" qualifier that text carried was dropped and the selection reported an unqualified success. The user only found out at the next launch, when the old persisted model came back. dispatchCommand drops the same text on a committed switch, so typed /model had the identical hole — the cross-provider path already returned the outcome as a value precisely because display copy is not a contract. handleModelCommand now returns that outcome too, and one modelAppliedNoticeFor decides how a committed switch is announced for both entry points, so neither can regain the gap independently. Recent-model history is written under the same lock, so a selection that already lost the lock would pay a SECOND full timeout for one keystroke and add a redundant save error. It now falls back to the in-session updateRecentModels for exactly that case, keyed on a new config.ErrLockUnavailable rather than on any persistence failure: a missing config file also fails the lookup, and that write still has its own reason to be attempted. Regressions cover both entry points. The picker test holds the config lock — the case the review asked for — and bounds elapsed time, which is what proves one keystroke costs one wait; it fails at 20s against the previous code. The typed test uses an unwritable config path so it proves the same plumbing without a second ten-second timeout. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01REzorhNj3F1DGPXyn5Uq7j --- internal/config/lock.go | 9 ++- internal/tui/command_center.go | 52 +++++++++---- internal/tui/model.go | 17 ++-- internal/tui/picker_test.go | 130 +++++++++++++++++++++++++++++++ internal/tui/transient_notice.go | 12 +++ 5 files changed, 195 insertions(+), 25 deletions(-) diff --git a/internal/config/lock.go b/internal/config/lock.go index 858d75500..0b1d61256 100644 --- a/internal/config/lock.go +++ b/internal/config/lock.go @@ -10,6 +10,13 @@ import ( "github.com/Gitlawb/zero/internal/lockutil" ) +// ErrLockUnavailable reports that the config lock was still held by another +// process (or goroutine) when configLockTimeout expired. A caller that would +// otherwise make a SECOND config write after a failed one branches on this: the +// lock has not been released, so the second attempt can only spend another full +// timeout to fail the same way, turning one keystroke into two stalls. +var ErrLockUnavailable = errors.New("config lock unavailable") + const ( configLockTimeout = 10 * time.Second configLockRetryDelay = 20 * time.Millisecond @@ -80,7 +87,7 @@ func lockConfigFile(path string) (func() error, error) { 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) + return nil, fmt.Errorf("config: timed out acquiring config lock for %s: %w", path, ErrLockUnavailable) } time.Sleep(configLockRetryDelay) } diff --git a/internal/tui/command_center.go b/internal/tui/command_center.go index a4491496c..571626b8a 100644 --- a/internal/tui/command_center.go +++ b/internal/tui/command_center.go @@ -1,6 +1,7 @@ package tui import ( + "errors" "fmt" "os" "path/filepath" @@ -394,31 +395,39 @@ func relativeAge(timestamp string, now time.Time) string { } } -func (m model) handleModelCommand(args string) (model, string) { +// handleModelCommand applies a model switch against the ACTIVE provider (the +// picker routes cross-provider selections to switchProviderModel instead). +// +// The returned error reports the same thing switchProviderModel's does: the +// in-session switch committed, but its config write did not. It is not +// redundant with the status text — the picker replaces that text with its own +// applied-notice, so a caller that only read the text would show an unqualified +// success for a selection that will be gone after a restart. +func (m model) handleModelCommand(args string) (model, string, error) { args = strings.TrimSpace(args) switch strings.ToLower(args) { case "": - return m, m.modelText(args) + return m, m.modelText(args), nil case "list", "ls": - return m, m.modelListText() + return m, m.modelListText(), nil } if m.pending { - return m, "Model\nCannot switch models while a run is active." + return m, "Model\nCannot switch models while a run is active.", nil } registry, err := modelregistry.DefaultRegistry() if err != nil { - return m, "Model\nFailed to load model catalog: " + err.Error() + return m, "Model\nFailed to load model catalog: " + err.Error(), nil } target, ok := m.resolveModelSwitchTarget(registry, args) if !ok { - return m, "Model\nunknown Zero model " + strconv.Quote(args) + return m, "Model\nunknown Zero model " + strconv.Quote(args), nil } if !config.HasProviderProfile(m.providerProfile) { - return m, "Model\nNo provider profile is available for TUI model switching." + return m, "Model\nNo provider profile is available for TUI model switching.", nil } if m.newProvider == nil { - return m, "Model\nProvider rebuild is not available for this TUI session." + return m, "Model\nProvider rebuild is not available for this TUI session.", nil } previousProviderName := m.providerName @@ -434,7 +443,7 @@ func (m model) handleModelCommand(args string) (model, string) { nextProfile = m.profileWithCredential(nextProfile) metadata, err := providers.ResolveRuntimeMetadata(nextProfile, providers.Options{}) if err != nil { - return m, "Model\n" + err.Error() + return m, "Model\n" + err.Error(), nil } if guarded, text, requested := m.requestCompactionBeforeModelSwitch(modelSwitchCompactionRequest{ @@ -442,12 +451,12 @@ func (m model) handleModelCommand(args string) (model, string) { TargetProvider: string(metadata.ProviderKind), TargetContextWindow: modelregistry.AgentContextWindow(m.modelContextWindow(target.modelID)), }, "Model"); requested { - return guarded, text + return guarded, text, nil } nextProvider, err := m.newProvider(nextProfile) if err != nil { - return m, "Model\n" + err.Error() + return m, "Model\n" + err.Error(), nil } persisted, persistErr := m.persistSelectedModel(nextProfile) @@ -464,14 +473,25 @@ func (m model) handleModelCommand(args string) (model, string) { // it once. Recording old-then-new leaves new at the front, with old right // behind 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}, + recent := []config.RecentModelEntry{ + {Provider: previousProviderName, Model: previousModel}, // Record under m.providerName (the same resolved value recentModelPairsForPicker // pins the active row with), not the raw nextProfile.Name: for a provider profile // with no Name set, those two diverge and the pinned active row would fail to // dedupe against the entry just persisted here, showing the same switch twice. - config.RecentModelEntry{Provider: m.providerName, Model: target.modelID}, - ) + {Provider: m.providerName, Model: target.modelID}, + } + if errors.Is(persistErr, config.ErrLockUnavailable) { + // The selection write already lost its race for the config lock, which is + // still held. A second blocking attempt would make one unavailable lock + // cost two full waits and add a redundant save error to the transcript, so + // keep the history in session only — the trade switchProviderModel makes. + // Any OTHER persistence failure (no config file yet, for one) says nothing + // about this write, which still has its own reason to be attempted. + m, _ = m.updateRecentModels(recent...) + } else { + m = m.recordRecentModels(recent...) + } // Drop a known-unsupported preference, void any profile bookkeeping the // drop erased, and re-derive an active profile's per-model effort fill. // The ring is authoritative only for catalog-resolved targets @@ -515,7 +535,7 @@ func (m model) handleModelCommand(args string) (model, string) { if warn := m.visionDropWarning(); warn != "" { lines = append(lines, warn) } - return m, strings.Join(lines, "\n") + return m, strings.Join(lines, "\n"), persistErr } // switchProviderModel switches the active provider to providerName (one of the diff --git a/internal/tui/model.go b/internal/tui/model.go index ea2c5ae4f..cae9da8d8 100644 --- a/internal/tui/model.go +++ b/internal/tui/model.go @@ -4474,14 +4474,11 @@ func (m model) choosePicker() (tea.Model, tea.Cmd) { // OwnerProvider is blank, matches the active provider, or (registry-fallback // / stale-history rows) doesn't resolve to any saved provider: apply against // the active provider instead of attempting an unresolvable provider switch. - m, text = m.handleModelCommand(item.Value) + m, text, switchPersistErr = 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 + notice, tone := m.modelAppliedNoticeFor(switchPersistErr) + return m.showTransientNoticeInline(notice, tone), cmd } m.transcript = reduceTranscript(m.transcript, transcriptAction{kind: actionAppendSystem, text: text}) case pickerEffort: @@ -4755,9 +4752,13 @@ func (m model) dispatchCommand(command parsedCommand) (tea.Model, tea.Cmd) { } previousProvider, previousModel := m.providerName, m.modelName text := "" - m, text = m.handleModelCommand(command.text) + var persistErr error + m, text, persistErr = m.handleModelCommand(command.text) if m.providerName != previousProvider || m.modelName != previousModel { - return m.showTransientNoticeInline(m.modelAppliedNotice(), transientNoticeSuccess), nil + // The status text this branch drops is the only other place the failure + // appears, so the typed command needs the qualifier as much as the picker. + notice, tone := m.modelAppliedNoticeFor(persistErr) + return m.showTransientNoticeInline(notice, tone), nil } m.transcript = reduceTranscript(m.transcript, transcriptAction{kind: actionAppendSystem, text: text}) return m, nil diff --git a/internal/tui/picker_test.go b/internal/tui/picker_test.go index cf60b186d..888e21de8 100644 --- a/internal/tui/picker_test.go +++ b/internal/tui/picker_test.go @@ -955,6 +955,136 @@ func TestCrossProviderPickerShowsPersistenceFailure(t *testing.T) { } } +// TestActiveProviderPickerShowsPersistenceFailure is the same-provider half of +// TestCrossProviderPickerShowsPersistenceFailure. A picker row owned by the +// ACTIVE provider goes through handleModelCommand, which reports a failed +// persistence attempt inside its status text — and choosePicker discards that +// text in favour of its own applied-notice. Without the outcome plumbed through +// as a value, the user saw an unqualified success for a selection the config +// never received, and only found out when the old model came back on restart. +// +// The elapsed-time bound is the second assertion, not decoration: the recent +// model history is written under the same lock, so a version that still tried +// to persist it after the selection write failed would pay TWO full lock +// timeouts for one keystroke. +func TestActiveProviderPickerShowsPersistenceFailure(t *testing.T) { + configPath := filepath.Join(t.TempDir(), "zero", "config.json") + if _, err := config.UpsertProvider(configPath, config.ProviderProfile{ + Name: "openrouter", CatalogID: "openrouter", Model: "google/gemini-2.5-pro", + }, true); err != nil { + t.Fatalf("seed openrouter 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) + } + }) + + m := newModel(context.Background(), Options{ + UserConfigPath: configPath, + ProviderName: "openrouter", + ModelName: "google/gemini-2.5-pro", + Provider: &fakeProvider{}, + ProviderProfile: config.ProviderProfile{ + Name: "openrouter", + CatalogID: "openrouter", + Model: "google/gemini-2.5-pro", + APIKeyEnv: "OPENROUTER_API_KEY", + Provider: string(config.ProviderKindOpenAICompatible), + BaseURL: "https://openrouter.ai/api/v1", + APIFormat: "chat-completions", + }, + NewProvider: func(config.ProviderProfile) (zeroruntime.Provider, error) { + return &fakeProvider{}, nil + }, + }) + // OwnerProvider left blank: the row resolves against the active provider, + // which is the branch that reaches handleModelCommand. + m.picker = &commandPicker{ + kind: pickerModel, + items: []pickerItem{{Value: "anthropic/claude-sonnet-4.5"}}, + } + + started := time.Now() + updated, _ := m.choosePicker() + elapsed := time.Since(started) + next := updated.(model) + + if next.modelName != "anthropic/claude-sonnet-4.5" { + t.Fatalf("in-session picker selection was lost: model=%q", next.modelName) + } + if !strings.Contains(next.transientNotice.text, "not saved (") || + !strings.Contains(next.transientNotice.text, "timed out acquiring config lock") { + t.Fatalf("picker notice = %q, want a concrete not-saved lock failure", next.transientNotice.text) + } + if next.transientNotice.tone != transientNoticeWarning { + t.Fatalf("picker notice tone = %v, want warning", next.transientNotice.tone) + } + if elapsed > 15*time.Second { + t.Fatalf("selection blocked for %s; want a single bounded persistence attempt", elapsed) + } + after, err := os.ReadFile(configPath) + if err != nil { + t.Fatalf("read config after failed selection: %v", err) + } + if !bytes.Equal(after, before) { + t.Fatalf("failed selection changed config\nbefore: %s\nafter: %s", before, after) + } +} + +// TestTypedModelCommandShowsPersistenceFailure is the typed half of the same +// gap: on a committed switch, dispatchCommand also replaces handleModelCommand's +// status text with the applied-notice, so "· not saved (…)" was dropped there +// too. An unwritable config path fails the persistence attempt immediately, +// which keeps this case off the ten-second lock timeout the picker test needs. +func TestTypedModelCommandShowsPersistenceFailure(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: "openrouter", + ModelName: "google/gemini-2.5-pro", + Provider: &fakeProvider{}, + ProviderProfile: config.ProviderProfile{ + Name: "openrouter", + CatalogID: "openrouter", + Model: "google/gemini-2.5-pro", + APIKeyEnv: "OPENROUTER_API_KEY", + Provider: string(config.ProviderKindOpenAICompatible), + BaseURL: "https://openrouter.ai/api/v1", + APIFormat: "chat-completions", + }, + NewProvider: func(config.ProviderProfile) (zeroruntime.Provider, error) { + return &fakeProvider{}, nil + }, + }) + m.input.SetValue("/model anthropic/claude-sonnet-4.5") + + updated, _ := m.Update(testKey(tea.KeyEnter)) + next := updated.(model) + + if next.modelName != "anthropic/claude-sonnet-4.5" { + t.Fatalf("in-session switch was lost: model=%q", next.modelName) + } + if !strings.Contains(next.transientNotice.text, "not saved (") { + t.Fatalf("notice = %q, want persistence failure", next.transientNotice.text) + } + if next.transientNotice.tone != transientNoticeWarning { + t.Fatalf("notice tone = %v, want warning", next.transientNotice.tone) + } +} + func TestModelCommandAcceptsManualModelForCustomProvider(t *testing.T) { var captured config.ProviderProfile m := newModel(context.Background(), Options{ diff --git a/internal/tui/transient_notice.go b/internal/tui/transient_notice.go index d8787adce..80b7c639e 100644 --- a/internal/tui/transient_notice.go +++ b/internal/tui/transient_notice.go @@ -12,6 +12,18 @@ func (m model) modelAppliedNotice() string { return "Model: " + displayValue(m.modelName, "none") + " · " + displayValue(m.providerName, "default provider") + " · effort " + m.effortDisplay() } +// modelAppliedNoticeFor renders the applied-notice for a committed model +// switch, qualifying it when the switch was not durably saved. Both entry +// points — typed /model and the picker — show this notice INSTEAD of the +// handler status text that carries the same qualifier, so a persistence +// failure is invisible on either path unless it is rendered here. +func (m model) modelAppliedNoticeFor(persistErr error) (string, transientNoticeTone) { + if persistErr != nil { + return m.modelAppliedNotice() + " · not saved (" + persistErr.Error() + ")", transientNoticeWarning + } + return m.modelAppliedNotice(), transientNoticeSuccess +} + func (m model) effortAppliedNotice() string { return "Reasoning effort: " + m.effortDisplay() }