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

pkg/cache/cache.go

@@ -19,9 +19,11 @@
 // back atomically via a temp file + rename. Two processes simultaneously
 // caching different keys both see their writes preserved; the lock
 // serializes the read-modify-write window so neither can clobber the
-// other. [Cache.Lookup] reloads the in-memory map when the file's mtime
-// has advanced since its last load, so cross-process writes become
-// visible without a restart.
+// other. Storing also adopts the state it read under the lock, so a
+// sibling's entries become visible to this process without waiting for a
+// reload. [Cache.Lookup] reloads the in-memory map when the file's mtime
+// has advanced since its last load, so cross-process writes made while
+// this process was idle become visible without a restart.
 //
 // Two normalization options are exposed:
 //
@@ -37,6 +39,7 @@ import (
 	"errors"
 	"fmt"
 	"log/slog"
+	"maps"
 	"os"
 	"path/filepath"
 	"strings"
@@ -168,11 +171,11 @@ func (c *Cache) Store(question, response string) {
 
 // persistToDisk takes the cross-process lock on c.path's sibling .lock
 // file, reloads the on-disk entries, merges (key, response), writes
-// atomically, and refreshes c.mtime. The caller must hold c.mu.
+// atomically, and adopts the merged state. The caller must hold c.mu.
 //
-// Skips the write — but still refreshes c.mtime — when the on-disk
-// state already has key → response, which keeps cross-process replays
-// free of redundant disk traffic.
+// Skips the write — but still adopts the on-disk state — when it already
+// has key → response, which keeps cross-process replays free of redundant
+// disk traffic.
 func (c *Cache) persistToDisk(key, response string) error {
 	unlock, err := lockFile(c.path)
 	if err != nil {
@@ -186,18 +189,43 @@ func (c *Cache) persistToDisk(key, response string) error {
 	}
 
 	if existing, ok := entries[key]; ok && existing == response {
-		c.mtime = mtimeOf(c.path)
+		c.adopt(entries)
 		return nil
 	}
 
 	entries[key] = response
 	if err := writeJSON(c.path, entries); err != nil {
 		return err
 	}
-	c.mtime = mtimeOf(c.path)
+	c.adopt(entries)
 	return nil
 }
 
+// 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 outright would discard every
+// in-memory entry the on-disk state does not mention, including one an earlier
+// failed [Cache.Store] kept on purpose.
+//
+// That preservation is narrow, and deliberately so — do not read it as making
+// unpersisted entries durable:
+//   - it lasts only until the next reload. [Cache.maybeReload] still replaces
+//     c.entries wholesale, so the first sibling write after a failed Store drops
+//     an unpersisted entry anyway.
+//   - it covers only keys absent from disk. When the on-disk state does mention
+//     the key, the value read under the lock wins and overwrites the in-memory
+//     one. Converging on what the lock protected is the right call for a cache,
+//     and [Cache.Store] overwrites by key by design.
+//
+// The caller must hold c.mu.
+func (c *Cache) adopt(entries map[string]string) {
+	maps.Copy(c.entries, entries)
+	c.mtime = mtimeOf(c.path)
+}
+
 // maybeReload reloads c.entries from disk when the file mtime has
 // advanced since our last load. Called from Lookup; a no-op when the
 // cache is in-memory only or when the file can't be stat'd (in-memory

pkg/cache/cache_test.go

@@ -385,3 +385,101 @@ func TestFileCache_lockFileNeverDeleted(t *testing.T) {
 	assert.Equal(t, info1.Sys(), info2.Sys(),
 		"lock file inode must be stable across Stores so flock semantics hold")
 }
+
+// TestFileCache_storeMakesSiblingEntriesVisible covers the in-memory half of
+// the cross-process contract. persistToDisk reads the current on-disk map,
+// merges its own key and writes it back — so a sibling's entries are preserved
+// on disk (asserted by TestFileCache_crossProcessConcurrentStoresPreserveAllEntries)
+// — but it also refreshes c.mtime. If the entries it just read are not adopted
+// into c.entries, the instance is marked up-to-date against a file it never
+// fully loaded, and maybeReload will never reload it: the sibling's entry stays
+// invisible to this process indefinitely.
+func TestFileCache_storeMakesSiblingEntriesVisible(t *testing.T) {
+	t.Parallel()
+
+	t.Run("after merging into an existing file", func(t *testing.T) {
+		t.Parallel()
+		path := filepath.Join(t.TempDir(), "cache.json")
+
+		cA, err := New(Config{Enabled: true, Path: path})
+		require.NoError(t, err)
+		cB, err := New(Config{Enabled: true, Path: path})
+		require.NoError(t, err)
+
+		cB.Store("question-1", "answer-1")
+		cA.Store("question-2", "answer-2")
+
+		// Both keys are on disk; this is the part that already worked.
+		data, err := os.ReadFile(path)
+		require.NoError(t, err)
+		var onDisk map[string]string
+		require.NoError(t, json.Unmarshal(data, &onDisk))
+		require.Equal(t, map[string]string{"question-1": "answer-1", "question-2": "answer-2"}, onDisk)
+
+		got, ok := cA.Lookup("question-2")
+		assert.True(t, ok)
+		assert.Equal(t, "answer-2", got)
+
+		got, ok = cA.Lookup("question-1")
+		assert.True(t, ok, "the sibling's entry is in the cache file and must be visible")
+		assert.Equal(t, "answer-1", got)
+	})
+
+	// persistToDisk short-circuits when the on-disk state already maps
+	// key -> response, and refreshes c.mtime on that path too. It must adopt
+	// the entries it read there as well.
+	t.Run("after the redundant-write shortcut", func(t *testing.T) {
+		t.Parallel()
+		path := filepath.Join(t.TempDir(), "cache.json")
+
+		cA, err := New(Config{Enabled: true, Path: path})
+		require.NoError(t, err)
+		cB, err := New(Config{Enabled: true, Path: path})
+		require.NoError(t, err)
+
+		cB.Store("shared", "same-answer")
+		cB.Store("only-b", "b-answer")
+
+		// Storing a pair the file already contains takes the shortcut.
+		cA.Store("shared", "same-answer")
+
+		got, ok := cA.Lookup("only-b")
+		assert.True(t, ok, "entries read during the shortcut must also become visible")
+		assert.Equal(t, "b-answer", got)
+	})
+}
+
+// TestFileCache_successfulStoreKeepsUnpersistedInMemoryEntry pins the contract
+// asserted by TestFileCache_persistenceFailureKeepsInMemory across a *later*
+// successful Store. Adopting the on-disk view by replacing c.entries would
+// discard entries that an earlier failed Store deliberately kept in memory,
+// so the adoption must merge.
+func TestFileCache_successfulStoreKeepsUnpersistedInMemoryEntry(t *testing.T) {
+	t.Parallel()
+
+	root := t.TempDir()
+	dir := filepath.Join(root, "cache")
+	path := filepath.Join(dir, "cache.json")
+
+	c, err := New(Config{Enabled: true, Path: path})
+	require.NoError(t, err)
+
+	// A plain file where the cache directory belongs makes MkdirAll fail, so
+	// the first Store cannot persist and keeps its entry in memory only.
+	require.NoError(t, os.WriteFile(dir, []byte("blocker"), 0o600))
+	c.Store("kept-in-memory", "value-1")
+	require.NoFileExists(t, path)
+
+	// Unblock the directory so the next Store persists successfully.
+	require.NoError(t, os.Remove(dir))
+	c.Store("persisted", "value-2")
+	require.FileExists(t, path)
+
+	got, ok := c.Lookup("kept-in-memory")
+	assert.True(t, ok, "a later successful Store must not drop an in-memory-only entry")
+	assert.Equal(t, "value-1", got)
+
+	got, ok = c.Lookup("persisted")
+	assert.True(t, ok)
+	assert.Equal(t, "value-2", got)
+}