[Bug]: concurrent sessions silently lose each other's persisted config, including API keys

[dennisonbertram]

Work type

Bug / regression

Observed behavior

Two harnesscli sessions running at once silently lose each other's persisted
settings. persistConfigField reads the whole config, mutates one field, and
writes the whole file back, with no lock anywhere between the read and the
write:

func persistConfigField(mutate func(*harnessconfig.Config)) error {
	cfg, err := harnessconfig.Load()   // reads the whole file
	if err != nil { return err }
	mutate(cfg)
	return harnessconfig.Save(cfg)     // writes the whole file
}

Session A loads, session B loads, A saves a starred model, B saves a theme —
B's write contains A's pre-change snapshot, so A's starred model is gone. No
error, no warning, exit 0 on both sides.

The write itself is atomic (temp file plus rename), which is what makes this
easy to miss: no reader ever sees a torn file. Atomicity of the write is not
serialization of the transaction.

Expected behavior

A change made in one session is not silently discarded by a concurrent save
from another. Either the read-modify-write is serialized with an inter-process
lock, or Save merges against the on-disk state rather than overwriting it.

Reproduction

Preconditions: two harnesscli --tui sessions, or any two processes calling
persistConfigField.

1. Session A and session B both start (both load the config).
2. In A, star a model (persistConfigField writes starred_models).
3. In B, change the theme (persistConfigField writes theme).
4. Inspect ~/.config/harnesscli/config.json.

Actual: the file carries B's theme and not A's starred model.

Deterministic version without a TUI: call Load in two goroutines or
processes, mutate different fields, Save both, and observe the first mutation
missing.

User and operational impact

Affected users: anyone running more than one session, which is the normal case
here — the repo's own workflow routinely has a TUI open while another session
works.

Severity: moderate-to-high, because of what is stored. Config holds
APIKeys, StarredModels, Gateway, Theme, HistoryEntries, and since
#1424 Model/Provider/ReasoningEffort. A lost update can silently discard a
freshly-entered API key, leaving the user to re-enter it with no indication of
why it vanished.

The silence is the sharp edge. Nothing surfaces the loss; the setting simply is
not there next time, which reads as "it didn't save" rather than "another
session overwrote it".

No security implications beyond the key loss itself; nothing is exposed, only
dropped.

Workaround: none reliable. Avoiding concurrent sessions is not realistic.

Suspected seam and search evidence

Owning code:

• cmd/harnesscli/tui/config_persist.go — persistConfigField, the
  load-mutate-save wrapper. All five call sites go through it.
• cmd/harnesscli/config/config.go — Load and Save.

grep -rn "Lock\|flock\|mutex\|Mutex" cmd/harnesscli/config/*.go cmd/harnesscli/tui/config_persist.go returns nothing: the sequence is entirely
unserialized, in-process and across processes alike.

Save is careful about the write: os.CreateTemp in the same directory,
Chmod(0o600), Write, Sync, Close, then os.Rename. That guarantees a
reader never sees a partial file, and is presumably why the gap was not
noticed — the code looks like it was written with concurrency in mind, but it
protects the wrong boundary.

Call sites that can race (cmd/harnesscli/tui/model.go): starred models,
history entries, gateway, API keys, theme, and the model selection added in
#1424.

Provenance: found by the untrusted external reviewer (gpt-6-astra via the
Surplus proxy) reading only this package, then confirmed by inspecting the code
and grepping for any lock.

Blast-radius impact map

Callers and data flow: persistConfigField and the config package. Five call
sites, all in the TUI. No server, protocol, or provider code.

Config/env/defaults: the on-disk format is unchanged. A merging fix must
preserve unknown fields so an older or newer binary's keys are not dropped —
worth checking, since Config is a plain struct and json.Unmarshal discards
fields it does not know about. That is a second, latent instance of the same
class of bug: a binary that does not know a field will erase it on the next
save.

API/CLI/wire formats/tools: none.

Persistence/schema/cache: this is the persistence layer. Any lock file must
live beside the config, be cleaned up, and not deadlock a crashed session.

Concurrency/lifecycle: the point of the change. A lock held across a TUI
prompt would be a hang; the lock must cover only load-mutate-save, which is
sub-millisecond.

Security/auth/permissions/privacy: MkdirAll(dir, 0o755) leaves
~/.config/harnesscli world-readable and world-searchable. The file itself is
0600, so keys are not directly exposed, but the directory should be 0700 —
and MkdirAll does not tighten an existing directory, so a fix must chmod
explicitly. Tracked here because it is the same file and the same review;
split it out if it complicates the change.

TUI/web/macOS/other clients: TUI only. macapp does not use this store.

Deployment/observability/runbooks: none.

Compatibility: additive. No format change.

Existing tests/fixtures: cmd/harnesscli/config/config_test.go and
atomic_test.go — the latter covers the atomic write, not the transaction,
which is precisely the distinction this issue is about.

Documentation: docs/logs/engineering-log.md.

Regression test first

cmd/harnesscli/config/, TestConcurrentUpdatesDoNotLoseFields.

Two goroutines (or subprocesses, which better model the real case) each perform
load-mutate-save against the same HOME, one setting StarredModels, the
other setting Theme. Barrier them so both load before either saves. Assert
both fields are present afterwards.

Red before the fix: whichever saves last wins and the other field is empty.

Why it proves the bug: it asserts the surviving content, not the absence of a
torn file, which is the property atomic_test.go already covers and which is
not violated here.

False-positive controls: keep atomic_test.go green so a fix does not trade
atomicity for serialization; and assert a single-writer save still round-trips
every field, so a merge that drops unknown keys fails.

Fix boundaries

In scope:

• Serialize load-mutate-save, or make Save merge against the on-disk state.
• The concurrency test above.
• Directory permissions (0700, with an explicit chmod for existing
  directories).
• Engineering-log entry.

Out of scope, deliberately:

• Durability of the rename. Correct as raised — the parent directory is not
  fsynced, so the rename can be lost on power failure — but the code comment
  overclaims rather than the code being wrong, and it is a separate concern
  from concurrent loss. Fix the comment or the fsync in its own change.
• Discarded Close/Remove errors on failure paths. Real, low value.
• Preserving unknown JSON fields across versions. Same class, wider blast
  radius; deserves its own ticket rather than riding along.

Diagnostic and observability evidence

Before: run the two-writer reproduction and print the resulting JSON — one
field is missing, with no error from either writer.

After: both fields present. The signal is the file content, not an exit code,
since neither writer ever failed.

Verification plan

• Red: run the new test before the fix; record the missing field.
• Green: same test after.
• Targeted: go test ./cmd/harnesscli/config/... -race -count=5 — repeated,
  because a concurrency fix that passes once proves little.
• Full regression: go test ./cmd/... ./internal/....
• Real path: two harnesscli --tui sessions against a scratch HOME; star a
  model in one, change the theme in the other, then read the file and confirm
  both survive. This is the case the unit test approximates, and the one that
  actually bit.
• Confirm ~/.config/harnesscli is 0700 afterwards, including when it
  already existed as 0755.

Rollout and rollback

Single PR to main; picked up on the next scripts/install.sh. No migration —
the file format is unchanged. Rollback is reverting the commit, which restores
silent lost updates.

Rollback trigger: a lock that fails to release, hanging a session on startup.
Any lock must be non-blocking or short-timeout with a clear message rather than
an indefinite wait.

Documentation and handoff

docs/logs/engineering-log.md: the defect, the distinction between an atomic
write and a serialized transaction — the code protected the former and the bug
was in the latter — and the provenance.

Definition of done

• Concurrency test written first and observed losing a field
• Load-mutate-save serialized or merged; no field lost under concurrent writers
• Lock cannot hang a session; failure is loud and bounded
• atomic_test.go still green — atomicity not traded for serialization
• Config directory 0700, including when it already existed
• -race -count=5 green; full regression green
• Two real TUI sessions proven not to lose each other's settings
• Engineering log records the atomic-write versus serialized-transaction distinction