diff --git a/pkg/cache/cache.go b/pkg/cache/cache.go index 247da7af9..d78f524ab 100644 --- a/pkg/cache/cache.go +++ b/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,7 +189,7 @@ 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 } @@ -194,10 +197,35 @@ func (c *Cache) persistToDisk(key, response string) error { 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 diff --git a/pkg/cache/cache_test.go b/pkg/cache/cache_test.go index 59dcf9b80..0372ce3f1 100644 --- a/pkg/cache/cache_test.go +++ b/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) +}