fix(#6018): one atomicfile helper, 16 converted sites, and a guard on the common spellings

[cajasmota]

The class

Code across the tree wrote files atomically as tmp := path + ".tmp" → write → os.Rename. The temp name is deterministic per destination, so two concurrent writers O_TRUNC and interleave into the same tmp inode, then each renames a torn file into place. The failure is not a lost write — it is a garbled one that a later read treats as truth.

This had been independently diagnosed and fixed three times without anyone generalising it: internal/statusfile/statusfile.go (citing review #5734), internal/daemon/watchreg/watchreg.go, and internal/links (#5978). A fourth site, internal/daemon/sched/subprocess_runner.go:775, names it a "known hazard" and argues its own path is safe via a scheduler invariant — correct local reasoning, but a containment argument that depends on an unrelated invariant holding forever.

What this adds

internal/atomicfile.WriteFile(path, b, perm) — os.CreateTemp in the destination directory (so rename stays on one filesystem), write, Chmod, rename, remove-temp on every error path including a failed rename.

Both traps from #5978 are handled once, in the package, rather than 48 times at call sites:

• The Chmod is load-bearing (os.CreateTemp creates 0600) and is pinned by a test asserting os.Stat().Mode().Perm() across 0600/0644/0755. In concurrent link passes share a deterministic .tmp filename → rename ENOENT drops a pass's links #5978 deleting the chmod left the whole package green until an assertion was added; that trap is now closed at the source.
• perm is deliberately not umask-masked, unlike os.WriteFile which open(2) masks. Under umask 077 files widen 0600 → 0644. Restoring masking would mean reading syscall.Umask, which is process-global, write-only and racy; a mode that does not depend on whether the daemon, the CLI, or a child ran the write is the better property for per-user state. Documented and pinned.

Two further divergences from os.WriteFile are documented and pinned by tests: a symlink destination is replaced rather than written through, and a 0444 destination is overwritten where os.WriteFile refuses. Neither is a regression — the code being replaced was already rename-based.

It deliberately does not MkdirAll: os.CreateTemp fails on a missing directory exactly where os.WriteFile would, so every call site keeps its existing setup unchanged. Pinned, so nobody assumes otherwise.

16 sites converted

registry/registry.go (x2), mcp/docstate.go, graph/genpath.go (x3), graph/graph.go, daemon/mode/mode.go, daemon/pins.go, daemon/dead_letters.go, daemon/root_manifest.go, daemon/sched/rss.go, daemon/worktree/worktree.go, daemon/algo/cache.go, daemon/watch/quarantine.go (x2), docgen/llm_cache.go.

The two sites #6018 named as reachable today are both handled: registry.go (written by CLI processes and the daemon with no cross-process lock — a torn registry is a user-visible broken install) and mcp/docstate.go (parallel agent sessions, the workload this project is being optimised for).

sched/rss.go keeps its discarded errors verbatim. That is a deliberate best-effort design and not this sweep's to change; only the tearing is fixed.

Risk ordering, corrected on review

The first cut excluded all of internal/graph/* as "streams into an open file handle rather than writing one []byte". That rationale was wrong for the most exposed writer in the tree. genpath.go's WriteCurrentPointer / WriteCurrentPointerRaw write a single small []byte — a one-line drop-in — and they publish the graph generation pointer, written by the indexer and read cross-process by the daemon and MCP to locate the current graph. Both built the same fixed temp path in the same shared directory; a torn write there sends readers to a wrong or nonexistent generation. That is a strictly worse blast radius than the two best-effort caches the first cut converted while admitting their concurrency was unproven.

Reviewing that file turned up a third site, WriteGenGraph at :527, which publishes the graph file the pointer points at. genpath.go now leaves the ledger entirely.

graph.go:580 writeJSONAtomic is also converted — it used json.NewEncoder(f) but encodes one in-memory value. The blanket rationale is replaced with per-file reasoning, and WriteAtomic's exclusion is restated as what it actually is: marshalling the whole Document to []byte would double peak memory on the largest object in the process. A memory argument, not a streaming one.

One defect caught by mutation, not by review

json.Encoder.Encode appends a trailing newline; json.Marshal does not. The newline was restored — and mutation-testing showed that deleting it kept the entire graph package green. No test covered those bytes. That is precisely the #5978 deleted-chmod shape, one package over, and it would have shipped. sidecar_bytes_6018_test.go now pins the newline, both pretty and minified forms byte-for-byte against a live json.Encoder, and the mode. Independently verified: removing the newline fails TestWriteSidecar_TrailingNewline and TestWriteSidecar_ByteIdenticalToEncoder.

The guard — what it does and does not catch

An AST-based test walks every non-test .go file in the repo (cmd/, tools/, site/ included) and fails on the deterministic-temp shape outside an explicit ledger. The ledger only shrinks: the test also fails when an entry stops offending, so a stale entry cannot silently blind it.

Caught: path + ".tmp", raw-string form, filepath.Join(dir, name+".tmp"), fmt.Sprintf("%s.tmp", path), and file-local const suffixes.

Not caught, and recorded as want: 0 rows in the test table rather than omitted: a different suffix (.partial), a fixed name in a shared directory (the identical hazard with no concatenation at all), a cross-file const, and a variable-borne suffix. A scan found zero live instances of any of these today.

The subject line no longer claims to "end the class" — it says a guard on the common spellings, because that is what it is. The file doc states that a green run means no new instance of the covered spellings, not a clean tree.

Verification

go build ./... && go vet ./... && gofmt -l . clean. -race clean on all touched packages. Full suite exit 0, 229 packages, zero failures — taken from rtk proxy go test and the raw exit code, not the rtk summary line, which truncates above 10 MiB and prints a green summary over a red run. An earlier revision of this branch reported the suite green on that basis when it was not; the wrapper's unreliability is noted in the commit message.

12 mutations, all confirmed failing — including deleting the Chmod, reverting os.CreateTemp to os.Create(path+".tmp") (fails the concurrency harness on iteration 0), dropping the remove-on-error defer, reverting a converted site, adding a non-offending ledger entry, and blinding the detector (which trips a "scan found nothing at all" assertion, covering the guard-that-does-not-bind case explicitly). TestDeterministicTempName_FailsConcurrently is a negative control running the same 8-writer harness against the old idiom, failing the build if it ever stops tearing.

Changes beyond temp-naming, all deliberate

• A test-only seam in production code. TestStoreSave_RetriesTransientTmpFailure forced a transient failure by pre-creating path+".tmp" as a directory — which worked only because the name was deterministic. A writeStoreFile var in daemon/worktree/worktree.go lets the test inject a one-shot failure instead. Reviewed and accepted: no non-racy alternative exists short of an error-injecting filesystem, and the rewritten test is stronger — it asserts calls == 2, the first direct proof the retry happens and is exactly one, plus a new test covering the both-attempts-fail path that had no coverage before. Carries a note that it must not be swapped concurrently.
• Merged error strings at 4 sites (grepped repo-wide including tests: no assertion matches them).
• quarantine.persistLocked now audits rename failures it previously dropped — the function has no return value, so no caller can newly fail.
• worktree.save's retry now covers the rename, the same transient class Monorepo + git worktrees: missing .git false positive on setup, then the daemon melts down re-indexing every worktree (and MAX_WORKTREES_PER_REPO=0 won't disable it) #5675 targeted.
• The genpath pointer flip retries the staging write as well as the rename, so a hard failure (missing or unwritable directory) takes ~200 ms to surface instead of failing instantly. Documented; judged irrelevant against a publish that cannot complete.

Left for follow-up

~32 sites remain, on the ledger with per-file reasoning: graph/fbwriter/* (genuinely streaming — they need a different helper shape, and their destinations are unique per generation, though that gen-allocation exclusivity is argued rather than verified), install/*, cli/register.go, agentpatterns/*, and the dashboard/enrichment group.

Already safe and untouched: statusfile, daemon/watchreg, links (all on os.CreateTemp already), and the pid-suffixed flows temp in subprocess_runner.go.

One new accepted cost: with deterministic names, a temp orphaned by a crash was overwritten by the next write — self-healing. Unique names mean each crash leaves a distinct .X.tmp-<rand> and no sweeper exists for *.tmp-*. Bounded by crash frequency, documented on the package. #5978 made the same trade silently.

Independently adversarially reviewed.

Closes #6018