fix(cache): refresh serve daemon's cache from disk each render cycle (#7774)

* fix(cache): refresh serve daemon's cache from disk each render cycle The `serve` daemon (used by streaming mode) loads the on-disk Session and Device caches once at startup and only flushes them back on exit, so a write from a separate one-shot process was invisible to the running daemon until it restarted, and could even be clobbered by the daemon's own stale copy on shutdown. This affected `oh-my-posh toggle` (Session store) as well as `enable`/`disable` - e.g. `enable reload`, which config.Get in prompt.New checks to bypass its own config cache after an edit (Device store). Fixes #7758. Add cache.Refresh(), which re-syncs the in-memory store from disk when the file's mtime has advanced, merging entries by Timestamp so a value the daemon has itself set more recently than the file always wins. Call it for both stores at the start of every render cycle in serve.go, and once more before close() persists a dirty store, closing the shutdown clobber window too. Drop the Session-only guard on the mtime bump in store.close() so a Device write reliably updates the file's mtime on Windows as well (the mmap-backed write path doesn't do this on its own). Also documents the residual limitation (there's still a narrow window where a write can land mid-cycle) in the streaming docs. * fix(cache): reorder store fields to satisfy fieldalignment Adding mtime widened the struct's pointer-scannable prefix from 40 to 56 bytes (the time.Time's trailing *Location pointer landed after the three trailing bools). Reorder so pointer-bearing fields lead and the bools trail, matching what CI's fieldalignment check expects. --------- Co-authored-by: Claude <noreply@anthropic.com>
2026-08-24 02:34:19 -05:00 · 2026-08-05 07:08:04 +00:00
parent e446fdcf31
commit eda0185975
4 changed files with 277 additions and 9 deletions
@@ -14,6 +14,10 @@ import (
 )

 type store struct {
+	// mtime is the on-disk file's modification time as of the last load or
+	// refresh. A long-lived process (serve) uses it to detect writes made by
+	// other processes (e.g. toggle) between render cycles - see Refresh.
+	mtime    time.Time
 	cache    *maps.Concurrent[*Entry[any]]
 	filePath string
 	dirty    bool
@@ -72,6 +76,7 @@ func (s Store) init(filePath string, persist bool) {
 	store.persist = persist
 	store.dirty = false
 	store.locked = false
+	store.mtime = time.Time{}

 	reader, err := openFile(store.filePath)
 	if err != nil {
@@ -92,6 +97,10 @@ func (s Store) init(filePath string, persist bool) {

 	defer reader.Close()

+	if info, err := os.Stat(store.filePath); err == nil {
+		store.mtime = info.ModTime()
+	}
+
 	var list maps.Simple[*Entry[any]]

 	dec := gob.NewDecoder(reader)
@@ -132,10 +141,70 @@ func touchSessionFile(filePath string) {
 	}
 }

+// Refresh re-syncs the in-memory store with the on-disk file if it has
+// changed since the last load or refresh, merging entries by Timestamp (the
+// newer value wins). A short-lived, one-shot invocation never needs this -
+// init() already loads the current file once. It exists for a long-lived
+// process (serve) that keeps its cache in memory for the session: without
+// it, a write from another process (e.g. `toggle`, `enable`/`disable`)
+// would stay invisible until the daemon exits.
+func Refresh(s Store) {
+	defer log.Trace(time.Now(), string(s))
+
+	store := s.get()
+	if store == nil || store.locked || store.filePath == "" {
+		return
+	}
+
+	info, err := os.Stat(store.filePath)
+	if err != nil || !info.ModTime().After(store.mtime) {
+		return
+	}
+
+	reader, err := openFile(store.filePath)
+	if err != nil {
+		return
+	}
+
+	defer reader.Close()
+
+	var list maps.Simple[*Entry[any]]
+
+	dec := gob.NewDecoder(reader)
+	if err := dec.Decode(&list); err != nil {
+		log.Error(err)
+		return
+	}
+
+	for key, diskEntry := range list {
+		if diskEntry.Expired() {
+			continue
+		}
+
+		if current, found := store.cache.Get(key); found && current.Timestamp >= diskEntry.Timestamp {
+			continue
+		}
+
+		log.Debugf("(%s) refreshing %s from disk", string(s), key)
+		store.cache.Set(key, diskEntry)
+	}
+
+	store.mtime = info.ModTime()
+}
+
 func (s Store) close() {
 	defer log.Trace(time.Now(), string(s))

 	store := s.get()
+
+	// Pick up any write from another process one last time before a dirty
+	// store overwrites the file, so a change made after the last Refresh
+	// (e.g. right before the shell exits) isn't clobbered by this store's
+	// own, possibly stale, in-memory copy.
+	if store != nil && !store.locked && store.persist && store.dirty {
+		Refresh(s)
+	}
+
 	if store == nil || store.locked || !store.persist || !store.dirty {
 		if s == Session && store != nil && !store.locked && store.filePath != "" {
 			touchSessionFile(store.filePath)
@@ -169,15 +238,13 @@ func (s Store) close() {
 		log.Error(err)
 	}

-	if s != Session {
-		return
-	}
-
 	// On Windows, the mmap-backed write path doesn't reliably update the
-	// file's on-disk last-write-time (per Microsoft's docs), which can lead
-	// to an actively-used session cache being mistaken for stale and swept
-	// up by cache.Clear(). Explicitly bump the mtime now that the file is
-	// closed (and the mmap unmap/flush on Windows has happened).
+	// file's on-disk last-write-time (per Microsoft's docs). For the session
+	// store that can lead to an actively-used cache being mistaken for stale
+	// and swept up by cache.Clear(); for either store, a long-lived Refresh
+	// reader (serve) needs a trustworthy mtime to notice this write at all.
+	// Explicitly bump it now that the file is closed (and the mmap
+	// unmap/flush on Windows has happened).
 	if err := os.Chtimes(store.filePath, time.Now(), time.Now()); err != nil {
 		log.Error(err)
 	}
@@ -8,6 +8,8 @@ import (
 	"testing"
 	"time"

+	"github.com/jandedobbeleer/oh-my-posh/src/maps"
+
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 )
@@ -164,3 +166,181 @@ func TestGetSurvivesGobPointerRegistration(t *testing.T) {
 	require.True(t, ok, "expected a cache hit after a gob round-trip of a pointer-registered type")
 	assert.Equal(t, "value", got.Name)
 }
+
+// Guards against the #7758 class of bug: a long-lived process (serve) that
+// loaded the session cache once at startup must still see a write from a
+// separate one-shot process (e.g. `oh-my-posh toggle`) that landed on disk
+// afterwards, without losing values it has itself set more recently than
+// what's on disk (e.g. prompt_count_cache).
+func TestRefreshMergesExternalWriteByTimestamp(t *testing.T) {
+	origSession := session
+	t.Cleanup(func() { session = origSession })
+
+	filePath := filepath.Join(t.TempDir(), "session.cache")
+
+	// The daemon's in-memory state: it has its own, newer value for a key
+	// the external writer also touches, and predates the external write.
+	daemon := Session.new()
+	daemon.filePath = filePath
+	daemon.persist = true
+	daemon.mtime = time.Now().Add(-time.Hour)
+	daemon.cache.Set("shared_key", &Entry[any]{
+		Value:     "daemon-value",
+		Timestamp: time.Now().Unix(),
+		TTL:       -1,
+	})
+
+	// Simulate `oh-my-posh toggle` running as a separate one-shot process:
+	// it starts from a stale copy of shared_key and writes a brand new key.
+	writer := Session.new()
+	writer.filePath = filePath
+	writer.persist = true
+	writer.dirty = true
+	writer.cache.Set("shared_key", &Entry[any]{
+		Value:     "stale-external-copy",
+		Timestamp: time.Now().Unix() - 100,
+		TTL:       -1,
+	})
+	writer.cache.Set(TOGGLECACHE, &Entry[any]{
+		Value:     map[string]bool{"shell": true},
+		Timestamp: time.Now().Unix(),
+		TTL:       -1,
+	})
+	session = writer
+	Session.close()
+
+	// Back to the daemon's perspective: refresh should pick up the new
+	// toggle_cache key from disk...
+	session = daemon
+	Refresh(Session)
+
+	toggled, found := Get[map[string]bool](Session, TOGGLECACHE)
+	require.True(t, found, "toggle_cache written externally should be visible after Refresh")
+	assert.True(t, toggled["shell"])
+
+	// ...without clobbering the daemon's own newer value for a key both
+	// sides touched.
+	shared, found := Get[string](Session, "shared_key")
+	require.True(t, found)
+	assert.Equal(t, "daemon-value", shared, "a newer in-memory value must win over an older on-disk one")
+}
+
+// Guards against the daemon's own shutdown flush clobbering a write that
+// landed after its last Refresh but before it exits.
+func TestCloseRefreshesBeforePersistingToAvoidClobber(t *testing.T) {
+	origSession := session
+	t.Cleanup(func() { session = origSession })
+
+	filePath := filepath.Join(t.TempDir(), "session.cache")
+
+	daemon := Session.new()
+	daemon.filePath = filePath
+	daemon.persist = true
+	daemon.dirty = true
+	daemon.mtime = time.Now().Add(-time.Hour)
+	daemon.cache.Set("prompt_count_cache", &Entry[any]{
+		Value:     3,
+		Timestamp: time.Now().Unix(),
+		TTL:       -1,
+	})
+
+	// A toggle write lands on disk after the daemon's last refresh, right
+	// before the shell (and daemon) exit.
+	writer := Session.new()
+	writer.filePath = filePath
+	writer.persist = true
+	writer.dirty = true
+	writer.cache.Set(TOGGLECACHE, &Entry[any]{
+		Value:     map[string]bool{"shell": true},
+		Timestamp: time.Now().Unix(),
+		TTL:       -1,
+	})
+	session = writer
+	Session.close()
+
+	// The daemon now shuts down. Its close() must merge the external write
+	// in before overwriting the file, not blindly persist its stale copy.
+	session = daemon
+	Session.close()
+
+	reader, err := openFile(filePath)
+	require.NoError(t, err)
+	defer reader.Close()
+
+	var onDisk maps.Simple[*Entry[any]]
+	require.NoError(t, gob.NewDecoder(reader).Decode(&onDisk))
+
+	session = &store{cache: onDisk.ToConcurrent()}
+
+	toggled, found := Get[map[string]bool](Session, TOGGLECACHE)
+	require.True(t, found, "the external write must survive the daemon's own shutdown flush")
+	assert.True(t, toggled["shell"])
+
+	count, found := Get[int](Session, "prompt_count_cache")
+	require.True(t, found)
+	assert.Equal(t, 3, count)
+}
+
+// Guards against a variant of #7758: the serve daemon's Device store (which
+// holds the RELOAD flag config.Get checks in prompt.New to bypass its own
+// config cache) must also see a write from a separate `oh-my-posh enable
+// reload` process, not just the Session store.
+func TestRefreshPicksUpDeviceStoreWrite(t *testing.T) {
+	origDevice := device
+	t.Cleanup(func() { device = origDevice })
+
+	filePath := filepath.Join(t.TempDir(), "omp.cache")
+
+	daemon := Device.new()
+	daemon.filePath = filePath
+	daemon.persist = true
+	daemon.mtime = time.Now().Add(-time.Hour)
+	device = daemon
+
+	writer := Device.new()
+	writer.filePath = filePath
+	writer.persist = true
+	writer.dirty = true
+	writer.cache.Set("reload", &Entry[any]{
+		Value:     true,
+		Timestamp: time.Now().Unix(),
+		TTL:       -1,
+	})
+	device = writer
+	Device.close()
+
+	device = daemon
+	Refresh(Device)
+
+	reload, found := Get[bool](Device, "reload")
+	require.True(t, found, "`enable reload` written externally should be visible after Refresh")
+	assert.True(t, reload)
+}
+
+// Device's close() must bump the file's mtime as reliably as Session's does
+// (the mmap-backed Windows write path doesn't do this on its own) - without
+// it, Refresh would never notice an `enable`/`disable` write on Windows.
+func TestStoreCloseTouchesDeviceFileMTime(t *testing.T) {
+	origDevice := device
+	t.Cleanup(func() { device = origDevice })
+
+	filePath := filepath.Join(t.TempDir(), "omp.cache")
+
+	testStore := Device.new()
+	testStore.filePath = filePath
+	testStore.persist = true
+	testStore.dirty = true
+	testStore.cache.Set("reload", &Entry[any]{
+		Value:     true,
+		Timestamp: time.Now().Unix(),
+		TTL:       -1,
+	})
+	device = testStore
+
+	before := time.Now()
+	Device.close()
+
+	info, err := os.Stat(filePath)
+	require.NoError(t, err)
+	assert.False(t, info.ModTime().Before(before), "mtime should be bumped to the close time, not left stale")
+}
@@ -266,6 +266,18 @@ func startRenderCycle(req *serveRequest, out *os.File, envKeys map[string]struct
 		}
 	}()

+	// The daemon keeps its caches in memory for its whole lifetime (see the
+	// comment on copyRecords below) and only reads the on-disk files once,
+	// at startup. Refresh picks up writes from other processes that landed
+	// since the last cycle: Session for `oh-my-posh toggle` (a segment
+	// toggled mid-session shouldn't stay stuck until the daemon exits), and
+	// Device for `enable`/`disable` (e.g. `enable reload`, which config.Get
+	// in prompt.New checks to bypass its own config cache after an edit -
+	// without this the daemon would keep serving the old config until
+	// restarted).
+	cache.Refresh(cache.Session)
+	cache.Refresh(cache.Device)
+
 	// Apply the env overlay BEFORE constructing the engine so segment
 	// execution and config templates observe the calling shell's
 	// environment. v1 accepts the theoretical race with a still-running
@@ -397,7 +409,9 @@ func copyRecords(id int64, records <-chan string, out *os.File) chan struct{} {
 		// template caches in memory for the daemon's lifetime - that's the
 		// whole point of a long-lived process. Caches are only flushed to
 		// disk once, on clean shutdown (quit/EOF), via the cache.Close()/
-		// template.SaveCache() defer in createServeCmd.
+		// template.SaveCache() defer in createServeCmd. Reads are a
+		// different story: cache.Refresh() in startRenderCycle re-syncs from
+		// disk each cycle, so writes from other processes are still seen.
 	}()

 	return done
@@ -117,6 +117,13 @@ Requires [Clink][clink] v1.1.42 or later.
 </TabItem>
 </Tabs>

+## Known limitations
+
+The background process re-syncs its in-memory cache from disk before every render, so writes
+from another process (`toggle`, `enable`/`disable`) are picked up on the next prompt. There can
+still be a narrow window - a write that lands between the background process reading the cache
+and finishing that same render - where it takes one extra prompt to show up.
+
 ## Feedback

 If you encounter issues or have suggestions for the streaming feature, please open an issue on the