fix(persist): fail catalogue reads only when state is truly absent

LoadCatalogueReadOnly returned ErrNotExist for a fresh install (breaking `vev ls`) and skipped compaction recovery for .next/.prev crash states. A Persister with no store now reports errPersistenceUnavailable instead of silently reporting success, and Records() logs load failures instead of swallowing them.

commit: 9f082df214d10566c7bc453363e7415aa5abdde5
author: brice · 2mo ago · 2026-07-26 04:54 UTC
parent: fd389f8
signature: GPG VerifiedCD88F9E564E0102646FB6077A1A07C7C7C3E349D · arch-gpg Verified for the committer email

3 files · +236 -22

M internal/persist/catalogue_test.go +84 -0


@@ -18,6 +18,8 @@ func TestCatalogue(t *testing.T) {
18	18		t.Run("apply-rename-replace", testCatalogueApplyRenameReplace)
19	19		t.Run("metadata-update-preserves-authority", testCatalogueMetadataUpdatePreservesAuthority)
20	20		t.Run("read-only-malformed", testCatalogueLoadReadOnlyRejectsMalformedValue)
	21	+	t.Run("read-only-fresh-install", testCatalogueLoadReadOnlyFreshInstallHasNoSessions)
	22	+	t.Run("read-only-recovers-crash-states", testCatalogueLoadReadOnlyRecoversCrashStates)
21	23		}
22	24
23	25		func testCatalogueRecordRoundTrip(t *testing.T) {

@@ -145,3 +147,85 @@ func testCatalogueLoadReadOnlyRejectsMalformedValue(t *testing.T) {
145	147		_, err = LoadCatalogueReadOnly(dir)
146	148		require.Error(t, err)
147	149		}
	150	+
	151	+	// testCatalogueLoadReadOnlyFreshInstallHasNoSessions covers `vev ls` on a
	152	+	// machine with no daemon and no catalogue: none of sessions.kv, .next, or
	153	+	// .prev exist. This must yield an empty catalogue, not an error (regression:
	154	+	// a bare os.Stat propagated ErrNotExist and made `vev ls` exit 1).
	155	+	func testCatalogueLoadReadOnlyFreshInstallHasNoSessions(t *testing.T) {
	156	+	dir := privateDir(t)
	157	+	records, err := LoadCatalogueReadOnly(dir)
	158	+	require.NoError(t, err)
	159	+	require.Empty(t, records)
	160	+	}
	161	+
	162	+	// testCatalogueLoadReadOnlyRecoversCrashStates covers spec acceptance
	163	+	// criterion 3 ("every valid catalogue record appears in vev list") for a
	164	+	// listing taken while no daemon is running: whenever any compaction
	165	+	// candidate exists, LoadCatalogueReadOnly must apply the same fixed-path
	166	+	// recovery the daemon uses on startup rather than failing the read.
	167	+	func testCatalogueLoadReadOnlyRecoversCrashStates(t *testing.T) {
	168	+	// Built through an unrelated scratch path and copied into place, rather
	169	+	// than kv.Open'd directly at the target path: kv.Open recovers its own
	170	+	// base path on every call, and target paths here deliberately coexist
	171	+	// with sibling .next/.prev candidates that would otherwise be recovered
	172	+	// away before the test gets to arrange its crash state.
	173	+	writeCandidate := func(t *testing.T, path string, records ...domain.CatalogueRecord) {
	174	+	t.Helper()
	175	+	scratch := filepath.Join(t.TempDir(), "scratch", "store.kv")
	176	+	store, err := kv.Open(scratch)
	177	+	require.NoError(t, err)
	178	+	for _, record := range records {
	179	+	value, err := encodeRecordValue(record)
	180	+	require.NoError(t, err)
	181	+	require.NoError(t, store.Set([]byte(record.Name), value))
	182	+	}
	183	+	require.NoError(t, store.Close())
	184	+	data, err := os.ReadFile(scratch)
	185	+	require.NoError(t, err)
	186	+	require.NoError(t, os.WriteFile(path, data, 0o600))
	187	+	}
	188	+
	189	+	tests := []struct {
	190	+	name string
	191	+	write func(t *testing.T, dir string, want domain.CatalogueRecord)
	192	+	}{
	193	+	{
	194	+	// Crash between "rename current->.prev" and "rename .next->current":
	195	+	// no current, both .prev (stale) and .next (authoritative) exist.
	196	+	name: "prev-and-next-recovers-from-next",
	197	+	write: func(t *testing.T, dir string, want domain.CatalogueRecord) {
	198	+	stale := want
	199	+	stale.IncarnationID = domain.IncarnationID{9}
	200	+	writeCandidate(t, StorePath(dir)+".prev", stale)
	201	+	writeCandidate(t, StorePath(dir)+".next", want)
	202	+	},
	203	+	},
	204	+	{
	205	+	// Crash leaves only a predecessor behind; no current, no successor.
	206	+	name: "prev-only-recovers-from-prev",
	207	+	write: func(t *testing.T, dir string, want domain.CatalogueRecord) {
	208	+	writeCandidate(t, StorePath(dir)+".prev", want)
	209	+	},
	210	+	},
	211	+	}
	212	+	for _, tt := range tests {
	213	+	t.Run(tt.name, func(t *testing.T) {
	214	+	dir := privateDir(t)
	215	+	require.NoError(t, os.MkdirAll(dir, 0o700))
	216	+	want := validRecord("recovered", 1)
	217	+	tt.write(t, dir, want)
	218	+
	219	+	records, err := LoadCatalogueReadOnly(dir)
	220	+	require.NoError(t, err)
	221	+	require.ElementsMatch(t, []domain.CatalogueRecord{want}, records)
	222	+
	223	+	_, statErr := os.Stat(StorePath(dir))
	224	+	require.NoError(t, statErr, "recovery must leave a valid current catalogue in place")
	225	+	_, nextErr := os.Stat(StorePath(dir) + ".next")
	226	+	require.True(t, os.IsNotExist(nextErr), ".next must not survive recovery")
	227	+	_, prevErr := os.Stat(StorePath(dir) + ".prev")
	228	+	require.True(t, os.IsNotExist(prevErr), ".prev must not survive recovery")
	229	+	})
	230	+	}
	231	+	}


@@ -18,6 +18,8 @@ func TestCatalogue(t *testing.T) {
18	t.Run("apply-rename-replace", testCatalogueApplyRenameReplace)	18		t.Run("apply-rename-replace", testCatalogueApplyRenameReplace)
19	t.Run("metadata-update-preserves-authority", testCatalogueMetadataUpdatePreservesAuthority)	19		t.Run("metadata-update-preserves-authority", testCatalogueMetadataUpdatePreservesAuthority)
20	t.Run("read-only-malformed", testCatalogueLoadReadOnlyRejectsMalformedValue)	20		t.Run("read-only-malformed", testCatalogueLoadReadOnlyRejectsMalformedValue)
		21	+	t.Run("read-only-fresh-install", testCatalogueLoadReadOnlyFreshInstallHasNoSessions)
		22	+	t.Run("read-only-recovers-crash-states", testCatalogueLoadReadOnlyRecoversCrashStates)
21	}	23		}
22		24
23	func testCatalogueRecordRoundTrip(t *testing.T) {	25		func testCatalogueRecordRoundTrip(t *testing.T) {

@@ -145,3 +147,85 @@ func testCatalogueLoadReadOnlyRejectsMalformedValue(t *testing.T) {
145	_, err = LoadCatalogueReadOnly(dir)	147		_, err = LoadCatalogueReadOnly(dir)
146	require.Error(t, err)	148		require.Error(t, err)
147	}	149		}
		150	+
		151	+	// testCatalogueLoadReadOnlyFreshInstallHasNoSessions covers `vev ls` on a
		152	+	// machine with no daemon and no catalogue: none of sessions.kv, .next, or
		153	+	// .prev exist. This must yield an empty catalogue, not an error (regression:
		154	+	// a bare os.Stat propagated ErrNotExist and made `vev ls` exit 1).
		155	+	func testCatalogueLoadReadOnlyFreshInstallHasNoSessions(t *testing.T) {
		156	+	dir := privateDir(t)
		157	+	records, err := LoadCatalogueReadOnly(dir)
		158	+	require.NoError(t, err)
		159	+	require.Empty(t, records)
		160	+	}
		161	+
		162	+	// testCatalogueLoadReadOnlyRecoversCrashStates covers spec acceptance
		163	+	// criterion 3 ("every valid catalogue record appears in vev list") for a
		164	+	// listing taken while no daemon is running: whenever any compaction
		165	+	// candidate exists, LoadCatalogueReadOnly must apply the same fixed-path
		166	+	// recovery the daemon uses on startup rather than failing the read.
		167	+	func testCatalogueLoadReadOnlyRecoversCrashStates(t *testing.T) {
		168	+	// Built through an unrelated scratch path and copied into place, rather
		169	+	// than kv.Open'd directly at the target path: kv.Open recovers its own
		170	+	// base path on every call, and target paths here deliberately coexist
		171	+	// with sibling .next/.prev candidates that would otherwise be recovered
		172	+	// away before the test gets to arrange its crash state.
		173	+	writeCandidate := func(t *testing.T, path string, records ...domain.CatalogueRecord) {
		174	+	t.Helper()
		175	+	scratch := filepath.Join(t.TempDir(), "scratch", "store.kv")
		176	+	store, err := kv.Open(scratch)
		177	+	require.NoError(t, err)
		178	+	for _, record := range records {
		179	+	value, err := encodeRecordValue(record)
		180	+	require.NoError(t, err)
		181	+	require.NoError(t, store.Set([]byte(record.Name), value))
		182	+	}
		183	+	require.NoError(t, store.Close())
		184	+	data, err := os.ReadFile(scratch)
		185	+	require.NoError(t, err)
		186	+	require.NoError(t, os.WriteFile(path, data, 0o600))
		187	+	}
		188	+
		189	+	tests := []struct {
		190	+	name string
		191	+	write func(t *testing.T, dir string, want domain.CatalogueRecord)
		192	+	}{
		193	+	{
		194	+	// Crash between "rename current->.prev" and "rename .next->current":
		195	+	// no current, both .prev (stale) and .next (authoritative) exist.
		196	+	name: "prev-and-next-recovers-from-next",
		197	+	write: func(t *testing.T, dir string, want domain.CatalogueRecord) {
		198	+	stale := want
		199	+	stale.IncarnationID = domain.IncarnationID{9}
		200	+	writeCandidate(t, StorePath(dir)+".prev", stale)
		201	+	writeCandidate(t, StorePath(dir)+".next", want)
		202	+	},
		203	+	},
		204	+	{
		205	+	// Crash leaves only a predecessor behind; no current, no successor.
		206	+	name: "prev-only-recovers-from-prev",
		207	+	write: func(t *testing.T, dir string, want domain.CatalogueRecord) {
		208	+	writeCandidate(t, StorePath(dir)+".prev", want)
		209	+	},
		210	+	},
		211	+	}
		212	+	for _, tt := range tests {
		213	+	t.Run(tt.name, func(t *testing.T) {
		214	+	dir := privateDir(t)
		215	+	require.NoError(t, os.MkdirAll(dir, 0o700))
		216	+	want := validRecord("recovered", 1)
		217	+	tt.write(t, dir, want)
		218	+
		219	+	records, err := LoadCatalogueReadOnly(dir)
		220	+	require.NoError(t, err)
		221	+	require.ElementsMatch(t, []domain.CatalogueRecord{want}, records)
		222	+
		223	+	_, statErr := os.Stat(StorePath(dir))
		224	+	require.NoError(t, statErr, "recovery must leave a valid current catalogue in place")
		225	+	_, nextErr := os.Stat(StorePath(dir) + ".next")
		226	+	require.True(t, os.IsNotExist(nextErr), ".next must not survive recovery")
		227	+	_, prevErr := os.Stat(StorePath(dir) + ".prev")
		228	+	require.True(t, os.IsNotExist(prevErr), ".prev must not survive recovery")
		229	+	})
		230	+	}
		231	+	}

M internal/persist/persist.go +68 -11


@@ -3,6 +3,7 @@ package persist
3	3		import (
4	4		"errors"
5	5		"fmt"
	6	+	"log/slog"
6	7		"os"
7	8		"path/filepath"
8	9		"sort"

@@ -22,6 +23,25 @@ func StorePath(dir string) string { return filepath.Join(dir, filename) }
22	23		type Persister struct {
23	24		store ports.Store
24	25		mu sync.Mutex
	26	+	log *slog.Logger
	27	+	}
	28	+
	29	+	// Option configures a Persister at construction time.
	30	+	type Option func(*Persister)
	31	+
	32	+	// WithLogger directs diagnostics for failures that ports.Catalogue has no
	33	+	// error return for (see Records) to log instead of the default logger.
	34	+	func WithLogger(log *slog.Logger) Option {
	35	+	return func(p *Persister) { p.log = log }
	36	+	}
	37	+
	38	+	// logger returns a nil-safe logger, defaulting to slog.Default() when no
	39	+	// logger was configured.
	40	+	func (p Persister) logger() slog.Logger {
	41	+	if p != nil && p.log != nil {
	42	+	return p.log
	43	+	}
	44	+	return slog.Default()
25	45		}
26	46
27	47		// KVStore adapts the reusable WAL implementation to the persistence port.

@@ -56,7 +76,13 @@ func Open(dir string) (*Persister, error) {
56	76		p, _, err := openCurrentCatalogue(dir, false)
57	77		return p, err
58	78		}
59		-	func New(store ports.Store) *Persister { return &Persister{store: store} }
	79	+	func New(store ports.Store, opts ...Option) *Persister {
	80	+	p := &Persister{store: store}
	81	+	for _, opt := range opts {
	82	+	opt(p)
	83	+	}
	84	+	return p
	85	+	}
60	86
61	87		func openCurrentCatalogue(dir string, createProvenEmpty bool) (*Persister, []domain.CatalogueRecord, error) {
62	88		path := StorePath(dir)

@@ -87,16 +113,26 @@ func openCurrentCatalogue(dir string, createProvenEmpty bool) (*Persister, []dom
87	113		}
88	114
89	115		func LoadReadOnly(dir string) ([]domain.CatalogueRecord, error) { return LoadCatalogueReadOnly(dir) }
	116	+
	117	+	// LoadCatalogueReadOnly loads the catalogue without retaining a store handle.
	118	+	// A fresh install with no catalogue candidate at all is not an error: it
	119	+	// returns an empty slice. Whenever any candidate (sessions.kv, .next, or
	120	+	// .prev) exists, it goes through the same fixed-path compaction recovery the
	121	+	// daemon applies on startup (via openCurrentCatalogue / kv.Open), so a
	122	+	// .next-only or .prev-only crash state still yields every valid record.
	123	+	// Malformed or corrupt data is still a hard error.
90	124		func LoadCatalogueReadOnly(dir string) ([]domain.CatalogueRecord, error) {
91		-	path := StorePath(dir)
92		-	if _, err := os.Stat(path); err != nil {
	125	+	p, records, err := openCurrentCatalogue(dir, false)
	126	+	if err != nil {
	127	+	if errors.Is(err, errPersistenceUnavailable) {
	128	+	return []domain.CatalogueRecord{}, nil
	129	+	}
93	130		return nil, err
94	131		}
95		-	data, err := kv.Replay(path)
96		-	if err != nil {
	132	+	if err := p.Close(); err != nil {
97	133		return nil, err
98	134		}
99		-	return decodeAll(data)
	135	+	return records, nil
100	136		}
101	137
102	138		func (p *Persister) Save(record domain.CatalogueRecord) error { return p.Replace(record.Name, record) }

@@ -111,8 +147,17 @@ func (p *Persister) Create(record domain.CatalogueRecord) error {
111	147		}
112	148		return p.applyLocked(map[string]*domain.CatalogueRecord{record.Name: &record})
113	149		}
	150	+
	151	+	// Records satisfies ports.Catalogue, whose signature has no error return. A
	152	+	// load failure (unavailable store, or a malformed record surfaced by
	153	+	// decodeAll) must not silently look like an empty catalogue, so it is logged
	154	+	// here instead of being swallowed.
114	155		func (p *Persister) Records() []domain.CatalogueRecord {
115		-	records, _ := p.LoadCatalogue()
	156	+	records, err := p.LoadCatalogue()
	157	+	if err != nil {
	158	+	p.logger().Error("persist: loading catalogue records failed", "err", err)
	159	+	return []domain.CatalogueRecord{}
	160	+	}
116	161		return records
117	162		}
118	163		func (p *Persister) Record(name string) (domain.CatalogueRecord, bool) {

@@ -129,9 +174,12 @@ func (p *Persister) Record(name string) (domain.CatalogueRecord, bool) {
129	174		return record, err == nil
130	175		}
131	176		func (p Persister) Apply(records map[string]domain.CatalogueRecord) error {
132		-	if p == nil \|\| p.store == nil {
	177	+	if p == nil {
133	178		return nil
134	179		}
	180	+	if p.store == nil {
	181	+	return errPersistenceUnavailable
	182	+	}
135	183		p.mu.Lock()
136	184		defer p.mu.Unlock()
137	185		return p.applyLocked(records)

@@ -193,9 +241,12 @@ func (p Persister) applyLocked(records map[string]domain.CatalogueRecord) erro
193	241		return p.store.Sync()
194	242		}
195	243		func (p *Persister) Rename(oldName string, next domain.CatalogueRecord) error {
196		-	if p == nil \|\| p.store == nil {
	244	+	if p == nil {
197	245		return nil
198	246		}
	247	+	if p.store == nil {
	248	+	return errPersistenceUnavailable
	249	+	}
199	250		if oldName == next.Name {
200	251		return errors.New("persist: rename requires distinct names")
201	252		}

@@ -209,9 +260,12 @@ func (p *Persister) Replace(name string, next domain.CatalogueRecord) error {
209	260		// authoritative incarnation. Recovery and transaction fields are retained from
210	261		// the stored record rather than accepted from a potentially stale runtime copy.
211	262		func (p *Persister) UpdateMetadata(update domain.CatalogueMetadataUpdate) error {
212		-	if p == nil \|\| p.store == nil {
	263	+	if p == nil {
213	264		return nil
214	265		}
	266	+	if p.store == nil {
	267	+	return errPersistenceUnavailable
	268	+	}
215	269		p.mu.Lock()
216	270		defer p.mu.Unlock()
217	271		value, ok := p.store.Get([]byte(update.Name))

@@ -245,9 +299,12 @@ func (p *Persister) Delete(name string) error {
245	299		}
246	300		func (p *Persister) LoadAll() ([]domain.CatalogueRecord, error) { return p.LoadCatalogue() }
247	301		func (p *Persister) LoadCatalogue() ([]domain.CatalogueRecord, error) {
248		-	if p == nil \|\| p.store == nil {
	302	+	if p == nil {
249	303		return []domain.CatalogueRecord{}, nil
250	304		}
	305	+	if p.store == nil {
	306	+	return nil, errPersistenceUnavailable
	307	+	}
251	308		p.mu.Lock()
252	309		defer p.mu.Unlock()
253	310		data := make(map[string][]byte)

 import (
 	"errors"
 	"fmt"
+	"log/slog"
 	"os"
 	"path/filepath"
 	"sort"
 type Persister struct {
 	store ports.Store
 	mu    sync.Mutex
+	log   *slog.Logger
+}
+// Option configures a Persister at construction time.
+type Option func(*Persister)
+// WithLogger directs diagnostics for failures that ports.Catalogue has no
+// error return for (see Records) to log instead of the default logger.
+func WithLogger(log *slog.Logger) Option {
+	return func(p *Persister) { p.log = log }
+}
+// logger returns a nil-safe logger, defaulting to slog.Default() when no
+// logger was configured.
+func (p *Persister) logger() *slog.Logger {
+	if p != nil && p.log != nil {
+		return p.log
+	}
+	return slog.Default()
 }
 // KVStore adapts the reusable WAL implementation to the persistence port.
 	p, _, err := openCurrentCatalogue(dir, false)
 	return p, err
 }
-func New(store ports.Store) *Persister { return &Persister{store: store} }
+func New(store ports.Store, opts ...Option) *Persister {
+	p := &Persister{store: store}
+	for _, opt := range opts {
+		opt(p)
+	}
+	return p
+}
 func openCurrentCatalogue(dir string, createProvenEmpty bool) (*Persister, []domain.CatalogueRecord, error) {
 	path := StorePath(dir)
 }
 func LoadReadOnly(dir string) ([]domain.CatalogueRecord, error) { return LoadCatalogueReadOnly(dir) }
+// LoadCatalogueReadOnly loads the catalogue without retaining a store handle.
+// A fresh install with no catalogue candidate at all is not an error: it
+// returns an empty slice. Whenever any candidate (sessions.kv, .next, or
+// .prev) exists, it goes through the same fixed-path compaction recovery the
+// daemon applies on startup (via openCurrentCatalogue / kv.Open), so a
+// .next-only or .prev-only crash state still yields every valid record.
+// Malformed or corrupt data is still a hard error.
 func LoadCatalogueReadOnly(dir string) ([]domain.CatalogueRecord, error) {
-	path := StorePath(dir)
+	p, records, err := openCurrentCatalogue(dir, false)
-	if _, err := os.Stat(path); err != nil {
+	if err != nil {
+		if errors.Is(err, errPersistenceUnavailable) {
+			return []domain.CatalogueRecord{}, nil
+		}
 		return nil, err
 	}
-	data, err := kv.Replay(path)
+	if err := p.Close(); err != nil {
-	if err != nil {
 		return nil, err
 	}
-	return decodeAll(data)
+	return records, nil
 }
 func (p *Persister) Save(record domain.CatalogueRecord) error { return p.Replace(record.Name, record) }
 	}
 	return p.applyLocked(map[string]*domain.CatalogueRecord{record.Name: &record})
 }
+// Records satisfies ports.Catalogue, whose signature has no error return. A
+// load failure (unavailable store, or a malformed record surfaced by
+// decodeAll) must not silently look like an empty catalogue, so it is logged
+// here instead of being swallowed.
 func (p *Persister) Records() []domain.CatalogueRecord {
-	records, _ := p.LoadCatalogue()
+	records, err := p.LoadCatalogue()
+	if err != nil {
+		p.logger().Error("persist: loading catalogue records failed", "err", err)
+		return []domain.CatalogueRecord{}
+	}
 	return records
 }
 func (p *Persister) Record(name string) (domain.CatalogueRecord, bool) {
 	return record, err == nil
 }
 func (p *Persister) Apply(records map[string]*domain.CatalogueRecord) error {
-	if p == nil || p.store == nil {
+	if p == nil {
 		return nil
 	}
+	if p.store == nil {
+		return errPersistenceUnavailable
+	}
 	p.mu.Lock()
 	defer p.mu.Unlock()
 	return p.applyLocked(records)
 	return p.store.Sync()
 }
 func (p *Persister) Rename(oldName string, next domain.CatalogueRecord) error {
-	if p == nil || p.store == nil {
+	if p == nil {
 		return nil
 	}
+	if p.store == nil {
+		return errPersistenceUnavailable
+	}
 	if oldName == next.Name {
 		return errors.New("persist: rename requires distinct names")
 	}
 // authoritative incarnation. Recovery and transaction fields are retained from
 // the stored record rather than accepted from a potentially stale runtime copy.
 func (p *Persister) UpdateMetadata(update domain.CatalogueMetadataUpdate) error {
-	if p == nil || p.store == nil {
+	if p == nil {
 		return nil
 	}
+	if p.store == nil {
+		return errPersistenceUnavailable
+	}
 	p.mu.Lock()
 	defer p.mu.Unlock()
 	value, ok := p.store.Get([]byte(update.Name))
 }
 func (p *Persister) LoadAll() ([]domain.CatalogueRecord, error) { return p.LoadCatalogue() }
 func (p *Persister) LoadCatalogue() ([]domain.CatalogueRecord, error) {
-	if p == nil || p.store == nil {
+	if p == nil {
 		return []domain.CatalogueRecord{}, nil
 	}
+	if p.store == nil {
+		return nil, errPersistenceUnavailable
+	}
 	p.mu.Lock()
 	defer p.mu.Unlock()
 	data := make(map[string][]byte)

M internal/persist/persist_test.go +84 -11

@@ -1,7 +1,9 @@
1	1		package persist
2	2
3	3		import (
	4	+	"bytes"
4	5		"errors"
	6	+	"log/slog"
5	7		"path/filepath"
6	8		"testing"
7	9

@@ -64,16 +66,87 @@ func TestCatalogueBatchFailureDoesNotSync(t *testing.T) {
64	66		require.ErrorIs(t, err, batchErr)
65	67		}
66	68
67		-	func TestCatalogueNilStoreIsNoOp(t *testing.T) {
68		-	persisters := []*Persister{nil, New(nil)}
69		-	for _, p := range persisters {
70		-	require.NoError(t, p.Save(validRecord("one", 1)))
71		-	require.NoError(t, p.Apply(map[string]*domain.CatalogueRecord{"one": nil}))
72		-	require.NoError(t, p.UpdateMetadata(validRecord("one", 1).MetadataUpdate()))
73		-	require.NoError(t, p.Delete("one"))
74		-	records, err := p.LoadAll()
75		-	require.NoError(t, err)
76		-	require.Empty(t, records)
77		-	require.NoError(t, p.Close())
	69	+	// TestCatalogueNilPersisterIsNoOp covers the ephemeral/optional-persistence
	70	+	// sentinel: a nil *Persister. Every mutating and reading method must be a
	71	+	// silent no-op so callers that never enable persistence don't need nil
	72	+	// checks of their own.
	73	+	func TestCatalogueNilPersisterIsNoOp(t *testing.T) {
	74	+	var p *Persister
	75	+	require.NoError(t, p.Save(validRecord("one", 1)))
	76	+	require.NoError(t, p.Apply(map[string]*domain.CatalogueRecord{"one": nil}))
	77	+	require.NoError(t, p.UpdateMetadata(validRecord("one", 1).MetadataUpdate()))
	78	+	require.NoError(t, p.Delete("one"))
	79	+	records, err := p.LoadAll()
	80	+	require.NoError(t, err)
	81	+	require.Empty(t, records)
	82	+	require.NoError(t, p.Close())
	83	+	require.Empty(t, p.Records())
	84	+	}
	85	+
	86	+	// TestCatalogueUnavailableStoreErrors covers a non-nil Persister with no
	87	+	// backing store (e.g. New(nil)). Unlike the nil-persister sentinel above,
	88	+	// this is a Persister that was supposed to have a store: silently no-oping
	89	+	// here previously made recovery.Coordinator's checkpoint publish/retry/
	90	+	// discard paths believe a durable commit happened when nothing was written.
	91	+	// Every mutation and read-with-error must surface errPersistenceUnavailable,
	92	+	// matching Create; only Records() (which ports.Catalogue gives no error
	93	+	// return for) degrades to an empty slice, with the failure logged instead.
	94	+	func TestCatalogueUnavailableStoreErrors(t *testing.T) {
	95	+	p := New(nil)
	96	+	require.ErrorIs(t, p.Save(validRecord("one", 1)), errPersistenceUnavailable)
	97	+	require.ErrorIs(t, p.Apply(map[string]*domain.CatalogueRecord{"one": nil}), errPersistenceUnavailable)
	98	+	require.ErrorIs(t, p.Rename("one", validRecord("two", 1)), errPersistenceUnavailable)
	99	+	require.ErrorIs(t, p.UpdateMetadata(validRecord("one", 1).MetadataUpdate()), errPersistenceUnavailable)
	100	+	require.ErrorIs(t, p.Delete("one"), errPersistenceUnavailable)
	101	+	require.ErrorIs(t, p.Create(validRecord("one", 1)), errPersistenceUnavailable)
	102	+	records, err := p.LoadAll()
	103	+	require.ErrorIs(t, err, errPersistenceUnavailable)
	104	+	require.Empty(t, records)
	105	+	require.Empty(t, p.Records())
	106	+	require.NoError(t, p.Close())
	107	+	}
	108	+
	109	+	// TestCatalogueRecordsMakesLoadFailureObservable covers Records(), which
	110	+	// ports.Catalogue.Records() gives no error return for: a load failure must
	111	+	// not silently look like an empty catalogue to every caller. Records()
	112	+	// degrades to an empty slice but must emit the underlying error through its
	113	+	// (nil-safe, optional) logger rather than swallowing it outright.
	114	+	func TestCatalogueRecordsMakesLoadFailureObservable(t *testing.T) {
	115	+	tests := []struct {
	116	+	name string
	117	+	build func(t testing.T, log slog.Logger) *Persister
	118	+	}{
	119	+	{
	120	+	name: "unavailable-store",
	121	+	build: func(t testing.T, log slog.Logger) *Persister {
	122	+	return New(nil, WithLogger(log))
	123	+	},
	124	+	},
	125	+	{
	126	+	name: "malformed-record",
	127	+	build: func(t testing.T, log slog.Logger) *Persister {
	128	+	store := portsmocks.NewMockStore(t)
	129	+	store.EXPECT().Range(mock.Anything).Run(func(fn func([]byte, []byte) bool) {
	130	+	fn([]byte("bad"), []byte{0, 1})
	131	+	})
	132	+	return New(store, WithLogger(log))
	133	+	},
	134	+	},
	135	+	}
	136	+	for _, tt := range tests {
	137	+	t.Run(tt.name, func(t *testing.T) {
	138	+	var buf bytes.Buffer
	139	+	p := tt.build(t, slog.New(slog.NewTextHandler(&buf, nil)))
	140	+	require.Empty(t, p.Records())
	141	+	require.Contains(t, buf.String(), "loading catalogue records failed")
	142	+	})
78	143		}
79	144		}
	145	+
	146	+	// TestCatalogueRecordsDefaultsToSlogDefaultLogger covers the nil-safe logger
	147	+	// fallback: a Persister built without WithLogger must not panic when a load
	148	+	// failure needs to be reported.
	149	+	func TestCatalogueRecordsDefaultsToSlogDefaultLogger(t *testing.T) {
	150	+	p := New(nil)
	151	+	require.NotPanics(t, func() { require.Empty(t, p.Records()) })
	152	+	}

@@ -1,7 +1,9 @@
1		package persist	1		package persist
2			2
3		import (	3		import (
			4	+	"bytes"
4		"errors"	5		"errors"
			6	+	"log/slog"
5		"path/filepath"	7		"path/filepath"
6		"testing"	8		"testing"
7			9

@@ -64,16 +66,87 @@ func TestCatalogueBatchFailureDoesNotSync(t *testing.T) {
64		require.ErrorIs(t, err, batchErr)	66		require.ErrorIs(t, err, batchErr)
65		}	67		}
66			68
67	-	func TestCatalogueNilStoreIsNoOp(t *testing.T) {	69	+	// TestCatalogueNilPersisterIsNoOp covers the ephemeral/optional-persistence
68	-	persisters := []*Persister{nil, New(nil)}	70	+	// sentinel: a nil *Persister. Every mutating and reading method must be a
69	-	for _, p := range persisters {	71	+	// silent no-op so callers that never enable persistence don't need nil
70	-	require.NoError(t, p.Save(validRecord("one", 1)))	72	+	// checks of their own.
71	-	require.NoError(t, p.Apply(map[string]*domain.CatalogueRecord{"one": nil}))	73	+	func TestCatalogueNilPersisterIsNoOp(t *testing.T) {
72	-	require.NoError(t, p.UpdateMetadata(validRecord("one", 1).MetadataUpdate()))	74	+	var p *Persister
73	-	require.NoError(t, p.Delete("one"))	75	+	require.NoError(t, p.Save(validRecord("one", 1)))
74	-	records, err := p.LoadAll()	76	+	require.NoError(t, p.Apply(map[string]*domain.CatalogueRecord{"one": nil}))
75	-	require.NoError(t, err)	77	+	require.NoError(t, p.UpdateMetadata(validRecord("one", 1).MetadataUpdate()))
76	-	require.Empty(t, records)	78	+	require.NoError(t, p.Delete("one"))
77	-	require.NoError(t, p.Close())	79	+	records, err := p.LoadAll()
			80	+	require.NoError(t, err)
			81	+	require.Empty(t, records)
			82	+	require.NoError(t, p.Close())
			83	+	require.Empty(t, p.Records())
			84	+	}
			85	+
			86	+	// TestCatalogueUnavailableStoreErrors covers a non-nil Persister with no
			87	+	// backing store (e.g. New(nil)). Unlike the nil-persister sentinel above,
			88	+	// this is a Persister that was supposed to have a store: silently no-oping
			89	+	// here previously made recovery.Coordinator's checkpoint publish/retry/
			90	+	// discard paths believe a durable commit happened when nothing was written.
			91	+	// Every mutation and read-with-error must surface errPersistenceUnavailable,
			92	+	// matching Create; only Records() (which ports.Catalogue gives no error
			93	+	// return for) degrades to an empty slice, with the failure logged instead.
			94	+	func TestCatalogueUnavailableStoreErrors(t *testing.T) {
			95	+	p := New(nil)
			96	+	require.ErrorIs(t, p.Save(validRecord("one", 1)), errPersistenceUnavailable)
			97	+	require.ErrorIs(t, p.Apply(map[string]*domain.CatalogueRecord{"one": nil}), errPersistenceUnavailable)
			98	+	require.ErrorIs(t, p.Rename("one", validRecord("two", 1)), errPersistenceUnavailable)
			99	+	require.ErrorIs(t, p.UpdateMetadata(validRecord("one", 1).MetadataUpdate()), errPersistenceUnavailable)
			100	+	require.ErrorIs(t, p.Delete("one"), errPersistenceUnavailable)
			101	+	require.ErrorIs(t, p.Create(validRecord("one", 1)), errPersistenceUnavailable)
			102	+	records, err := p.LoadAll()
			103	+	require.ErrorIs(t, err, errPersistenceUnavailable)
			104	+	require.Empty(t, records)
			105	+	require.Empty(t, p.Records())
			106	+	require.NoError(t, p.Close())
			107	+	}
			108	+
			109	+	// TestCatalogueRecordsMakesLoadFailureObservable covers Records(), which
			110	+	// ports.Catalogue.Records() gives no error return for: a load failure must
			111	+	// not silently look like an empty catalogue to every caller. Records()
			112	+	// degrades to an empty slice but must emit the underlying error through its
			113	+	// (nil-safe, optional) logger rather than swallowing it outright.
			114	+	func TestCatalogueRecordsMakesLoadFailureObservable(t *testing.T) {
			115	+	tests := []struct {
			116	+	name string
			117	+	build func(t testing.T, log slog.Logger) *Persister
			118	+	}{
			119	+	{
			120	+	name: "unavailable-store",
			121	+	build: func(t testing.T, log slog.Logger) *Persister {
			122	+	return New(nil, WithLogger(log))
			123	+	},
			124	+	},
			125	+	{
			126	+	name: "malformed-record",
			127	+	build: func(t testing.T, log slog.Logger) *Persister {
			128	+	store := portsmocks.NewMockStore(t)
			129	+	store.EXPECT().Range(mock.Anything).Run(func(fn func([]byte, []byte) bool) {
			130	+	fn([]byte("bad"), []byte{0, 1})
			131	+	})
			132	+	return New(store, WithLogger(log))
			133	+	},
			134	+	},
			135	+	}
			136	+	for _, tt := range tests {
			137	+	t.Run(tt.name, func(t *testing.T) {
			138	+	var buf bytes.Buffer
			139	+	p := tt.build(t, slog.New(slog.NewTextHandler(&buf, nil)))
			140	+	require.Empty(t, p.Records())
			141	+	require.Contains(t, buf.String(), "loading catalogue records failed")
			142	+	})
78		}	143		}
79		}	144		}
			145	+
			146	+	// TestCatalogueRecordsDefaultsToSlogDefaultLogger covers the nil-safe logger
			147	+	// fallback: a Persister built without WithLogger must not panic when a load
			148	+	// failure needs to be reported.
			149	+	func TestCatalogueRecordsDefaultsToSlogDefaultLogger(t *testing.T) {
			150	+	p := New(nil)
			151	+	require.NotPanics(t, func() { require.Empty(t, p.Records()) })
			152	+	}