Files
zero/internal/config/export_test.go
b04fe96e9e fix(config): lock config read-modify-write across processes (#960)
* fix(config): lock config read-modify-write across processes

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) <[email protected]>
Claude-Session: https://claude.ai/code/session_01U389qmQUhoZB3YtXkFmiTq

* fix(cli): take the config lock for MCP config edits

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) <[email protected]>
Claude-Session: https://claude.ai/code/session_01U389qmQUhoZB3YtXkFmiTq

* fix(config): report config unlock failures instead of discarding them

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) <[email protected]>
Claude-Session: https://claude.ai/code/session_01U389qmQUhoZB3YtXkFmiTq

* 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 <[email protected]>

* test(cli): synchronize MCP config lock assertion

Amp-Thread-ID: https://ampcode.com/threads/T-01a0448f-5860-721c-8a47-5119fc57f685
Co-authored-by: Amp <[email protected]>

* fix(tui): persist provider model selection atomically

Amp-Thread-ID: https://ampcode.com/threads/T-01a04c92-2d1d-7508-91bc-416341b7e8b0
Co-authored-by: Amp <[email protected]>

* fix(tui): surface unsaved model selections on every switch path

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) <[email protected]>
Claude-Session: https://claude.ai/code/session_01REzorhNj3F1DGPXyn5Uq7j

---------

Co-authored-by: Claude Opus 5 (1M context) <[email protected]>
Co-authored-by: Amp <[email protected]>
2026-09-12 08:44:43 +08:00

85 lines
3.2 KiB
Go

// Test seams: helpers only test code uses, kept out of the production binary.
package config
import (
"encoding/json"
"errors"
"fmt"
"os"
"strings"
)
// ToolsOverride builds a ToolsConfig that explicitly overrides the deferred-tool
// threshold (including to 0, which disables deferral). Use this for programmatic
// Overrides — a bare ToolsConfig{DeferThreshold: 0} is indistinguishable from
// "unset" and will not override.
func ToolsOverride(deferThreshold int) ToolsConfig {
return ToolsConfig{DeferThreshold: deferThreshold, deferThresholdSet: true}
}
// ValidateFile reads and parses path as a Zero FileConfig and runs the same
// semantic provider/model rules used during resolution. It returns the parsed
// config (zero value on parse failure) plus any structured issues. A parse
// failure yields a single issue whose Message is the underlying JSON error's
// text (already flattened via Error(), not chained — callers cannot recover
// the original *json.SyntaxError / *json.UnmarshalTypeError via errors.As).
func ValidateFile(path string) (FileConfig, []Issue) {
data, err := os.ReadFile(path)
if err != nil {
return FileConfig{}, []Issue{{Message: fmt.Sprintf("read config %s: %v", path, err)}}
}
var cfg FileConfig
if err := json.Unmarshal(data, &cfg); err != nil {
return FileConfig{}, []Issue{{Message: fmt.Sprintf("invalid config JSON %s: %v", path, err)}}
}
issues := validateSemantics(cfg)
issues = append(issues, unknownFieldIssues(data)...)
return cfg, issues
}
// 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) (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")
}
// 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 := lockConfigFileFn(path)
if err != nil {
return FileConfig{}, err
}
// 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 {
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.Providers[index].Description = strings.TrimSpace(description)
if err := writeConfigFile(path, cfg); err != nil {
return FileConfig{}, err
}
return cfg, nil
}
}
return FileConfig{}, fmt.Errorf("provider %q not found", name)
}