test(config): force the stale-snapshot interleaving that Get-then-Save loses

commit: aa019b41bfadc7722ffc3150e99da6bb983f741d
author: brice · 8d ago · 2026-10-14 08:36 UTC
parent: ba63da0
signature: GPG VerifiedCD88F9E564E0102646FB6077A1A07C7C7C3E349D · arch-gpg Verified for the committer email

2 files · +51 -28

M internal/infrastructure/config/loader.go +13 -2


@@ -24,6 +24,11 @@ type Manager struct {
24	24		callbacks []func(*Config)
25	25		watching bool
26	26		skipNextReload bool // Set by Save() to prevent fsnotify reload from overwriting in-memory config
	27	+
	28	+	// afterSnapshot is a test seam called right after Get or Update copies the
	29	+	// current config (Get calls it without the lock held, Update with it held).
	30	+	// It is nil outside tests.
	31	+	afterSnapshot func()
27	32		}
28	33
29	34		type automaticEnvConfig struct {

@@ -517,10 +522,13 @@ func normalizeMedia(config *Config) {
517	522		// Get returns the current configuration (thread-safe).
518	523		func (m Manager) Get() Config {
519	524		m.mu.RLock()
520		-	defer m.mu.RUnlock()
521		-
522	525		// Return a copy to prevent external modification
523	526		configCopy := *m.config
	527	+	m.mu.RUnlock()
	528	+
	529	+	if m.afterSnapshot != nil {
	530	+	m.afterSnapshot()
	531	+	}
524	532		return &configCopy
525	533		}
526	534

@@ -548,6 +556,9 @@ func (m Manager) Update(mutate func(Config)) error {
548	556		defer m.mu.Unlock()
549	557
550	558		updated := *m.config
	559	+	if m.afterSnapshot != nil {
	560	+	m.afterSnapshot()
	561	+	}
551	562		mutate(&updated)
552	563
553	564		return m.saveLocked(&updated)

 	callbacks      []func(*Config)
 	watching       bool
 	skipNextReload bool // Set by Save() to prevent fsnotify reload from overwriting in-memory config
+	// afterSnapshot is a test seam called right after Get or Update copies the
+	// current config (Get calls it without the lock held, Update with it held).
+	// It is nil outside tests.
+	afterSnapshot func()
 }
 type automaticEnvConfig struct {
 // Get returns the current configuration (thread-safe).
 func (m *Manager) Get() *Config {
 	m.mu.RLock()
-	defer m.mu.RUnlock()
 	// Return a copy to prevent external modification
 	configCopy := *m.config
+	m.mu.RUnlock()
+	if m.afterSnapshot != nil {
+		m.afterSnapshot()
+	}
 	return &configCopy
 }
 	defer m.mu.Unlock()
 	updated := *m.config
+	if m.afterSnapshot != nil {
+		m.afterSnapshot()
+	}
 	mutate(&updated)
 	return m.saveLocked(&updated)

M internal/infrastructure/config/mode_legend_preferences_gateway_test.go +38 -26


@@ -2,6 +2,7 @@ package config
2	2
3	3		import (
4	4		"context"
	5	+	"sync/atomic"
5	6		"testing"
6	7
7	8		"github.com/bnema/dumber/internal/domain/entity"

@@ -38,40 +39,51 @@ func newWatchedTestManager(t testing.T, configHome string) Manager {
38	39		return mgr
39	40		}
40	41
41		-	func TestManagerUpdate_OverlappingUpdatesDoNotLoseWrites(t *testing.T) {
	42	+	// TestManagerUpdate_UpdateAfterStaleSnapshotDoesNotLoseWrites forces the
	43	+	// interleaving a Get()-then-Save() implementation loses: another update
	44	+	// publishes right after the snapshot the first update works from was taken.
	45	+	// Update must apply its change on top of that update instead of overwriting it.
	46	+	func TestManagerUpdate_UpdateAfterStaleSnapshotDoesNotLoseWrites(t *testing.T) {
42	47		configHome := t.TempDir()
43	48		mgr := newWatchedTestManager(t, configHome)
44	49		legend := NewModeLegendPreferencesGateway(mgr)
45	50		omnibox := NewOmniboxPreferencesGateway(mgr)
46	51
47		-	otherStarted := make(chan struct{})
48		-	otherDone := make(chan error, 1)
49		-
50		-	// The first update starts a second one while it still holds the lock. The
51		-	// second update must wait and then be applied on top of the first result.
52		-	err := mgr.Update(func(cfg *Config) {
53		-	go func() {
54		-	close(otherStarted)
55		-	otherDone <- omnibox.SaveOmniboxInitialBehavior(context.Background(), entity.OmniboxInitialBehaviorMostVisited)
	52	+	saveOther := func() error {
	53	+	return omnibox.SaveOmniboxInitialBehavior(context.Background(), entity.OmniboxInitialBehaviorMostVisited)
	54	+	}
	55	+	queued := make(chan error, 1)
	56	+	var fired atomic.Bool
	57	+	mgr.afterSnapshot = func() {
	58	+	// The nested update calls this hook too; only the first snapshot reacts.
	59	+	if fired.Swap(true) {
	60	+	return
	61	+	}
	62	+	func() {
	63	+	if mgr.mu.TryLock() {
	64	+	// The snapshot was taken without keeping the lock: another
	65	+	// update publishes before this one continues.
	66	+	mgr.mu.Unlock()
	67	+	require.NoError(t, saveOther())
	68	+	queued <- nil
	69	+	return
	70	+	}
	71	+	// The lock is held across the whole update: the other update has
	72	+	// to queue behind it.
	73	+	go func() { queued <- saveOther() }()
56	74		}()
57		-	<-otherStarted
58		-	cfg.Workspace.Styling.ModeLegendCollapsed = true
59		-	})
60		-	require.NoError(t, err)
61		-	require.NoError(t, <-otherDone)
62		-
63		-	got := mgr.Get()
64		-	require.True(t, got.Workspace.Styling.ModeLegendCollapsed)
65		-	require.Equal(t, entity.OmniboxInitialBehaviorMostVisited, got.Omnibox.InitialBehavior)
	75	+	}
66	76
67		-	reloaded := newTestOmniboxPreferencesGatewayManager(t, configHome).Get()
68		-	require.True(t, reloaded.Workspace.Styling.ModeLegendCollapsed)
69		-	require.Equal(t, entity.OmniboxInitialBehaviorMostVisited, reloaded.Omnibox.InitialBehavior)
	77	+	require.NoError(t, legend.SaveModeLegendCollapsed(context.Background(), true))
	78	+	require.NoError(t, <-queued)
70	79
71		-	require.NoError(t, legend.SaveModeLegendCollapsed(context.Background(), false))
72		-	got = mgr.Get()
73		-	require.False(t, got.Workspace.Styling.ModeLegendCollapsed)
74		-	require.Equal(t, entity.OmniboxInitialBehaviorMostVisited, got.Omnibox.InitialBehavior)
	80	+	for name, got := range map[string]*Config{
	81	+	"in-memory": mgr.Get(),
	82	+	"on-disk": newTestOmniboxPreferencesGatewayManager(t, configHome).Get(),
	83	+	} {
	84	+	require.True(t, got.Workspace.Styling.ModeLegendCollapsed, name)
	85	+	require.Equal(t, entity.OmniboxInitialBehaviorMostVisited, got.Omnibox.InitialBehavior, name)
	86	+	}
75	87		}
76	88
77	89		func TestManagerUpdate_AppliesToLatestConfigAfterSave(t *testing.T) {


@@ -2,6 +2,7 @@ package config
2			2
3		import (	3		import (
4		"context"	4		"context"
			5	+	"sync/atomic"
5		"testing"	6		"testing"
6			7
7		"github.com/bnema/dumber/internal/domain/entity"	8		"github.com/bnema/dumber/internal/domain/entity"

@@ -38,40 +39,51 @@ func newWatchedTestManager(t testing.T, configHome string) Manager {
38		return mgr	39		return mgr
39		}	40		}
40			41
41	-	func TestManagerUpdate_OverlappingUpdatesDoNotLoseWrites(t *testing.T) {	42	+	// TestManagerUpdate_UpdateAfterStaleSnapshotDoesNotLoseWrites forces the
			43	+	// interleaving a Get()-then-Save() implementation loses: another update
			44	+	// publishes right after the snapshot the first update works from was taken.
			45	+	// Update must apply its change on top of that update instead of overwriting it.
			46	+	func TestManagerUpdate_UpdateAfterStaleSnapshotDoesNotLoseWrites(t *testing.T) {
42		configHome := t.TempDir()	47		configHome := t.TempDir()
43		mgr := newWatchedTestManager(t, configHome)	48		mgr := newWatchedTestManager(t, configHome)
44		legend := NewModeLegendPreferencesGateway(mgr)	49		legend := NewModeLegendPreferencesGateway(mgr)
45		omnibox := NewOmniboxPreferencesGateway(mgr)	50		omnibox := NewOmniboxPreferencesGateway(mgr)
46			51
47	-	otherStarted := make(chan struct{})	52	+	saveOther := func() error {
48	-	otherDone := make(chan error, 1)	53	+	return omnibox.SaveOmniboxInitialBehavior(context.Background(), entity.OmniboxInitialBehaviorMostVisited)
49	-		54	+	}
50	-	// The first update starts a second one while it still holds the lock. The	55	+	queued := make(chan error, 1)
51	-	// second update must wait and then be applied on top of the first result.	56	+	var fired atomic.Bool
52	-	err := mgr.Update(func(cfg *Config) {	57	+	mgr.afterSnapshot = func() {
53	-	go func() {	58	+	// The nested update calls this hook too; only the first snapshot reacts.
54	-	close(otherStarted)	59	+	if fired.Swap(true) {
55	-	otherDone <- omnibox.SaveOmniboxInitialBehavior(context.Background(), entity.OmniboxInitialBehaviorMostVisited)	60	+	return
			61	+	}
			62	+	func() {
			63	+	if mgr.mu.TryLock() {
			64	+	// The snapshot was taken without keeping the lock: another
			65	+	// update publishes before this one continues.
			66	+	mgr.mu.Unlock()
			67	+	require.NoError(t, saveOther())
			68	+	queued <- nil
			69	+	return
			70	+	}
			71	+	// The lock is held across the whole update: the other update has
			72	+	// to queue behind it.
			73	+	go func() { queued <- saveOther() }()
56		}()	74		}()
57	-	<-otherStarted	75	+	}
58	-	cfg.Workspace.Styling.ModeLegendCollapsed = true
59	-	})
60	-	require.NoError(t, err)
61	-	require.NoError(t, <-otherDone)
62	-
63	-	got := mgr.Get()
64	-	require.True(t, got.Workspace.Styling.ModeLegendCollapsed)
65	-	require.Equal(t, entity.OmniboxInitialBehaviorMostVisited, got.Omnibox.InitialBehavior)
66			76
67	-	reloaded := newTestOmniboxPreferencesGatewayManager(t, configHome).Get()	77	+	require.NoError(t, legend.SaveModeLegendCollapsed(context.Background(), true))
68	-	require.True(t, reloaded.Workspace.Styling.ModeLegendCollapsed)	78	+	require.NoError(t, <-queued)
69	-	require.Equal(t, entity.OmniboxInitialBehaviorMostVisited, reloaded.Omnibox.InitialBehavior)
70			79
71	-	require.NoError(t, legend.SaveModeLegendCollapsed(context.Background(), false))	80	+	for name, got := range map[string]*Config{
72	-	got = mgr.Get()	81	+	"in-memory": mgr.Get(),
73	-	require.False(t, got.Workspace.Styling.ModeLegendCollapsed)	82	+	"on-disk": newTestOmniboxPreferencesGatewayManager(t, configHome).Get(),
74	-	require.Equal(t, entity.OmniboxInitialBehaviorMostVisited, got.Omnibox.InitialBehavior)	83	+	} {
			84	+	require.True(t, got.Workspace.Styling.ModeLegendCollapsed, name)
			85	+	require.Equal(t, entity.OmniboxInitialBehaviorMostVisited, got.Omnibox.InitialBehavior, name)
			86	+	}
75		}	87		}
76			88
77		func TestManagerUpdate_AppliesToLatestConfigAfterSave(t *testing.T) {	89		func TestManagerUpdate_AppliesToLatestConfigAfterSave(t *testing.T) {