fix(cache): adopt the on-disk state read during Store, not just its mtime

[dwin-gharibi]

persistToDisk read the on-disk map, merged its own key, wrote it back — and then advanced
c.mtime without keeping the entries it had just read. Since maybeReload only reloads when the
mtime differs, the instance was marked up to date against a file it never fully loaded, and a
sibling process's entries stayed invisible to Lookup indefinitely.

Closes #3927 .

What was happening

The file was always correct — TestFileCache_crossProcessConcurrentStoresPreserveAllEntries
already covered that. But it asserts only the on-disk result, which is precisely the half
that worked. The in-memory half was never asserted:

file on disk = {"question-1": "answer-1", "question-2": "answer-2"}

a.Lookup("question-2") = "answer-2", found=true    own entry fine
a.Lookup("question-1") = "",         found=false   on disk, invisible to A

Remove A's intervening Store and the documented mtime reload works exactly as advertised. That
control is what pinned the cause to persistToDisk rather than to the reload path.

Two branches had the bug, not one: the write path (:193-198) and the redundant-write shortcut
(:188-191), which also refreshed c.mtime after reading the on-disk map without adopting it.

The fix

The merged map is already in hand under both the mutex and the cross-process file lock, so adopt
it. Extracted as one helper used by both branches, so the asymmetry cannot reappear:

// adopt merges the on-disk state read under the lock into the in-memory map and
// refreshes c.mtime. Both must happen together: advancing the mtime alone would
// mark this instance up to date against a file whose sibling-written entries it
// never loaded, and [Cache.maybeReload] would then never reload them.
//
// The merge is deliberate — replacing c.entries would discard entries an earlier
// failed [Cache.Store] kept in memory on purpose. The caller must hold c.mu.
func (c *Cache) adopt(entries map[string]string) {
	for k, v := range entries {
		c.entries[k] = v
	}
	c.mtime = mtimeOf(c.path)
}

Binding the mtime refresh and the adoption into a single call is the actual point: the bug was
that those two facts could drift apart.

Why it merges instead of replacing

This is the one design decision worth reviewing, and the repo's own tests forced it.

TestFileCache_persistenceFailureKeepsInMemory (cache_test.go:186) pins that a Store whose
write fails deliberately keeps its entry in memory. The obvious one-liner —
c.entries = entries — would satisfy the cross-process bug but silently undo that contract on
the next successful Store:

Store("a", "1")   -> write fails, kept in memory only
Store("b", "2")   -> write succeeds; replacing c.entries drops "a"

The existing test does not catch this (it only performs one, failing, Store), so I added
TestFileCache_successfulStoreKeepsUnpersistedInMemoryEntry to cover it. Merging is also safe
against resurrecting deleted keys: Cache exposes no Delete, so entries are never removed from
the file.

Adoption happens only on the success paths. A failed persistToDisk leaves c.entries and
c.mtime untouched, so Store's documented in-memory fallback at :166 still applies.

Tests

pkg/cache/cache_test.go:

• TestFileCache_storeMakesSiblingEntriesVisible — the regression, one subtest per branch:

  • after merging into an existing file (the write path)
  • after the redundant-write shortcut (the existing == response path)

  Both first assert the file on disk contains both keys, so a failure reads unambiguously as "the
  in-memory view is stale", not "the write was lost".

• TestFileCache_successfulStoreKeepsUnpersistedInMemoryEntry — the merge-vs-replace guard
  described above. It reuses the existing suite's trick for forcing a write failure (a plain file
  where the cache directory belongs, making MkdirAll fail), then removes it so the next Store
  succeeds.

Written test-first. Both storeMakesSiblingEntriesVisible subtests failed on unpatched code with
the sibling's entry is in the cache file and must be visible / entries read during the shortcut must also become visible, while the embedded on-disk assertions passed — which is what
localises the defect. successfulStoreKeepsUnpersistedInMemoryEntry passed before the change as
well as after; it exists to fail if the fix is ever simplified into a replacement.

Neither new test depends on filesystem mtime granularity: they turn on an exact equality with
the mtime Store itself recorded, so there is no timing flake of the kind
TestFileCache_lookupReloadsAfterExternalWrite has to work around with os.Chtimes.

Verification

Toolchain go1.26.5, darwin/arm64.

Check	Result
go test ./pkg/cache/	ok — 20/20, all pre-existing tests included
go test -race -count=1 ./pkg/cache/	ok
go test -race -count=1 ./pkg/runtime/ ./pkg/agent/	ok (the pkg/cache consumers)
go test ./pkg/teamloader/ -run Cache	ok (6 buildAgentCache tests)
go build ./...	clean
go vet ./pkg/cache/	clean
gofmt -l pkg/cache/	no output
go test ./... (full suite, .env.test loaded)	only pkg/teamloader fails — pre-existing

The pre-existing tests that matter most here all still pass:
persistenceFailureKeepsInMemory, crossProcessConcurrentStoresPreserveAllEntries,
dedupSkipsRedundantWrite, lookupReloadsAfterExternalWrite, atomicWriteLeavesNoTempFiles,
concurrentStoreNeverYieldsTornFile, lockFileNeverDeleted.

[dwin-gharibi]

@Sayt-0

[aheritier]

Review — fix(cache): adopt the on-disk state read during Store

Reviewed at head 0eb7102. The diagnosis in #3927 is right, the fix is the minimal correct one, and the tests genuinely discriminate. I'd approve on the code — I'm leaving this as a comment only because CI has not run for this SHA: the ci workflow run is conclusion=action_required (fork PR awaiting maintainer approval), there are zero check-runs and the commit status is {"state":"pending","statuses":[]}. The windows-tests job matters here specifically — it's the one that exercises LockFileEx and the MkdirAll-failure trick the new tests reuse.

The fix

adopt (pkg/cache/cache.go:209-212) binds the two facts that had drifted apart — the merged map read under the lock, and c.mtime — into one call, and both call sites (:190 shortcut, :198 write path) go through it. That's the right shape: the bug was an asymmetry between two branches, and a single helper is what makes it non-reintroducible. Locking is sound: adopt runs with c.mu held (via Store) and inside the flock window, so the mtime it stats cannot belong to content it didn't read. maps.Copy copies rather than aliases the local map, so no post-adopt mutation of entries can leak into c.entries. No exported API changes; go build ./... clean and the pkg/cache consumers (pkg/runtime, pkg/agent, pkg/teamloader) pass untouched.

Test adequacy — mutation-tested

I re-ran the suite against five mutants of the production code, keeping the PR's tests. All five were killed, and each by the specific assertion that should catch it:

Mutant	Result
pkg/cache/cache.go reverted to main wholesale	FAIL — both storeMakesSiblingEntriesVisible subtests (cache_test.go:424, :447); everything else passed
write path only reverted to c.mtime = mtimeOf(c.path) (:198)	FAIL — after merging into an existing file only
shortcut only reverted (:190)	FAIL — after the redundant-write shortcut only
adopt replaces instead of merging (c.entries = entries)	FAIL — successfulStoreKeepsUnpersistedInMemoryEntry: "a later successful Store must not drop an in-memory-only entry"
adopt merges but drops the c.mtime refresh	FAIL — successfulStoreKeepsUnpersistedInMemoryEntry

The per-branch isolation (mutants 2 and 3 each failing exactly one subtest) is what I'd want from a two-branch fix, and the embedded on-disk require.Equal in the first subtest does its job: a failure reads as "stale in-memory view", not "lost write". -race -count=3 ./pkg/cache/ is stable, and neither new test leans on mtime granularity — they turn on exact equality with the mtime Store recorded, so no os.Chtimes workaround is needed.

[optional] adopt's doc comment claims a bit more than the merge delivers

cache.go:207-208 says replacing c.entries "would discard entries an earlier failed Store kept in memory on purpose". True, but the merge only protects such entries in two narrow senses, and the comment reads as a general guarantee:

1. Only until the next reload. maybeReload still does c.entries = fresh (cache.go:245), so the first sibling write after the failed Store wipes the unpersisted entry anyway. Probe (extends the new test with sib.Store("sibling", "value-3") before the final Lookup): kept-in-memory is gone. Same effect as mutant 5, which is why dropping the mtime refresh also breaks successfulStoreKeepsUnpersistedInMemoryEntry.
2. Only for keys absent from disk. If the failed Store's key is on disk with an older value, adopt overwrites the newer in-memory value with the stale on-disk one. This is a behavior change vs. main — same probe (sibling writes k → old-from-sibling; local Store("k","new-local") fails via a 0o500 cache dir; then a successful Store("other","v"); Lookup("k")): main → "new-local", this PR → "old-from-sibling".

I don't think (2) needs a code change — converging on the state read under the lock is defensible for a cache, and Store overwrites entries by design — but the comment should be narrowed to what holds, e.g. "preserves in-memory entries that the on-disk state does not mention; a later reload may still drop them." Leaving the current wording risks the next reader treating unpersisted entries as durable.

[optional] Package doc still describes visibility as a Lookup-only property

The package doc (cache.go:19-24) — the text #3927 quoted as the contract — still attributes cross-process visibility solely to Lookup's mtime reload. One half-sentence noting that Store adopts the state it read under the lock would close the loop for the next reader.

[optional] Commit hygiene before merge

main's convention scopes by package (fix(cache):, exactly as your PR title does) rather than by file (fix(pkg/cache/cache.go):), and style(pkg/cache/cache.go): linting to fix the lint problems is a fixup that AGENTS.md's "logical and atomic commits" would have squashed into the first commit. Cosmetic; only worth doing if you're pushing again anyway.

Verification I ran (go1.26.5, darwin/arm64)

go build ./... clean · go test -race -count=3 ./pkg/cache/ ok · go test ./pkg/runtime ./pkg/agent ok · go test -run Cache ./pkg/teamloader ok · golangci-lint run ./pkg/cache/... (fresh cache) 0 issues · gofmt -l pkg/cache/ empty · go vet ./pkg/cache/ clean. Windows, task test-binary, check-plan-cross and the license check remain unverified — they need the gated CI run.

Happy to flip to approve once ci is green.

[docker-agent]

Assessment: 🟢 APPROVE

The fix is correct and well-designed. The adopt() helper properly solves the original bug by atomically binding the mtime refresh and the in-memory merge, and the merge strategy (rather than replace) correctly preserves in-memory entries from failed persists.

Lower-confidence findings (not posted inline)

• [low] pkg/cache/cache.go:211 — TOCTOU between writeJSON and mtimeOf could in theory cause c.mtime to reflect an interloper's write, suppressing a future maybeReload (confidence: 🟠 weak 52/100). Practically a non-issue: mtimeOf is called while the advisory file lock is still held (the defer unlock() fires after adopt returns), so all cooperating Cache processes are locked out. Only a process bypassing the advisory lock protocol (e.g., a manual file edit) could race this window. The scenario is outside the documented cross-process contract and the window is microseconds wide.

[dwin-gharibi]

Done. @aheritier