platform/catalog: generic ProjectionStore[T] consumer + race-free holder; finish C1

- Generic SnapshotWriter[T]/SnapshotReader[T] over a value-agnostic raw layer; SnapshotHolder[T] now RWMutex-guards munmap vs in-flight Get (race-free swap, TestSnapshotHolder_SwapRace) — the drain grace is now just close-batching. - ProjectionStore[T]: snapshot begin|chunk|end assembly → atomic swap, epoch-gated delta overlay, tombstones, atomic epoch sidecar (ordered before the swap), cold-start reopen, mandatory ProjectionTransform[T]. Future-epoch deltas are dropped by contract (recover via next snapshot), not buffered. - Shared bus-framing constants so producer + consumer agree. - Bump slask-finder / go-cart-actor / backoffice pointers; refresh status.md + catalog-projection.md to the finished state. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
2026-06-28 21:07:39 +02:00
1 parent 5a3ffcb6bc
commit 8b454bccac
@@ -1,12 +1,18 @@
-// SnapshotHolder provides atomic hot-reload for SnapshotReader with drain
-// grace — solves the classic "Get concurrent with Close/munmap" race that
-// would otherwise produce uncatchable SIGBUS / SIGSEGV under reload.
+// SnapshotHolder provides atomic hot-reload for SnapshotReader and solves the
+// classic "Get concurrent with Close/munmap" race that would otherwise produce
+// uncatchable SIGBUS / SIGSEGV under reload.
 //
-// Pattern source: slask-finder's `RawItemsStore` and similar mmap+reload
-// stores, which delay unmap behind a configurable drain-grace timer so any
-// in-flight Get on the previously-current reader completes before the
-// underlying pages become unmapped. See docs/inventory-shape.md & the
-// finder WARP doc on mmap reload for the prior art.
+// Safety is an RWMutex (readMu): Get holds RLock across the load-and-read of a
+// reader, and a reader is only Close()d (munmap) under Lock, so a reader can
+// never be unmapped while a Get is reading it — regardless of how long that
+// Get's goroutine was descheduled. (A drain-grace timer alone, the original
+// approach, is only best-effort: a goroutine paused longer than the grace can
+// still read freed pages. TestSnapshotHolder_SwapRace reproduces that crash.)
+//
+// The drain grace is retained as a close-BATCHING optimization: it defers and
+// coalesces reader closes onto the reaper tick instead of taking the exclusive
+// Lock on every swap. Pattern source: slask-finder's mmap+reload stores.
+// See docs/inventory-shape.md for the prior art.
 //
 // Use:
 //
@@ -36,30 +42,39 @@ const DefaultDrainGrace = 30 * time.Second
 // out from under in-flight Get()s. Set retains the previous reader behind a
 // drain timer; after the timer elapses the reaper goroutine closes the old
 // reader (and runs its munmap).
-type SnapshotHolder struct {
-	current atomic.Pointer[SnapshotReader]
+type SnapshotHolder[T any] struct {
+	current atomic.Pointer[SnapshotReader[T]]

-	drainsMu  sync.Mutex
-	drains    []*pendingClose
+	// readMu makes reclamation race-free: Get holds RLock across the
+	// Load-and-read of a reader, and a reader is only Close()d (munmap) under
+	// Lock — so a reader can never be unmapped while a Get is reading it, even
+	// if that Get's goroutine was descheduled past the drain grace. The grace
+	// (below) is now only close-BATCHING — defer + coalesce closes off the swap
+	// path — NOT the safety mechanism it was before (which was best-effort and
+	// could SIGSEGV under a long enough scheduler pause).
+	readMu sync.RWMutex
+
+	drainsMu   sync.Mutex
+	drains     []*pendingClose[T]
 	drainGrace time.Duration

 	stopCh chan struct{}
 	once   sync.Once
 }

-type pendingClose struct {
-	r        *SnapshotReader
+type pendingClose[T any] struct {
+	r        *SnapshotReader[T]
 	expireAt time.Time
 }

-// NewSnapshotHolder returns a holder with the given drain grace (the time
+// NewSnapshotHolder returns a holder of T with the given drain grace (the time
 // the previous reader lives behind Set before being closed). Zero or
 // negative grace falls back to DefaultDrainGrace.
-func NewSnapshotHolder(grace time.Duration) *SnapshotHolder {
+func NewSnapshotHolder[T any](grace time.Duration) *SnapshotHolder[T] {
 	if grace <= 0 {
 		grace = DefaultDrainGrace
 	}
-	h := &SnapshotHolder{
+	h := &SnapshotHolder[T]{
 		drainGrace: grace,
 		stopCh:     make(chan struct{}),
 	}
@@ -69,8 +84,8 @@ func NewSnapshotHolder(grace time.Duration) *SnapshotHolder {

 // Open is shorthand for Set(ReadSnapshotFile(path)). Errors leave the
 // holder's current reader unchanged.
-func (h *SnapshotHolder) Open(path string) error {
-	r, err := ReadSnapshotFile(path)
+func (h *SnapshotHolder[T]) Open(path string) error {
+	r, err := ReadSnapshotFile[T](path)
 	if err != nil {
 		return err
 	}
@@ -81,8 +96,8 @@ func (h *SnapshotHolder) Open(path string) error {
 // Reload atomically swaps the current reader for one freshly opened from
 // path. The old reader (if any) is queued for close after the drain grace
 // elapses; in-flight Get(s) on the old reader continue to work.
-func (h *SnapshotHolder) Reload(path string) error {
-	r, err := ReadSnapshotFile(path)
+func (h *SnapshotHolder[T]) Reload(path string) error {
+	r, err := ReadSnapshotFile[T](path)
 	if err != nil {
 		return err
 	}
@@ -92,7 +107,7 @@ func (h *SnapshotHolder) Reload(path string) error {

 // Set replaces the current reader with r. The previous reader is queued
 // for close after the drain grace elapses. r may be nil to clear.
-func (h *SnapshotHolder) Set(r *SnapshotReader) {
+func (h *SnapshotHolder[T]) Set(r *SnapshotReader[T]) {
 	if r == nil {
 		h.current.Store(nil)
 		return
@@ -102,7 +117,7 @@ func (h *SnapshotHolder) Set(r *SnapshotReader) {
 		return
 	}
 	h.drainsMu.Lock()
-	h.drains = append(h.drains, &pendingClose{
+	h.drains = append(h.drains, &pendingClose[T]{
 		r:        old,
 		expireAt: time.Now().Add(h.drainGrace),
 	})
@@ -110,17 +125,23 @@ func (h *SnapshotHolder) Set(r *SnapshotReader) {
 }

 // Get is the concurrent-safe shim over the current reader's Get. Returns
-// (zero, false) when the holder is empty.
-func (h *SnapshotHolder) Get(sku string) (Projection, bool) {
+// (zero, false) when the holder is empty. The RLock is held across the Load AND
+// the read so a concurrent reaper can't munmap this reader mid-read.
+func (h *SnapshotHolder[T]) Get(sku string) (T, bool) {
+	h.readMu.RLock()
+	defer h.readMu.RUnlock()
 	r := h.current.Load()
 	if r == nil {
-		return Projection{}, false
+		var zero T
+		return zero, false
 	}
 	return r.Get(sku)
 }

 // Len returns the count of the current reader (0 if empty).
-func (h *SnapshotHolder) Len() int {
+func (h *SnapshotHolder[T]) Len() int {
+	h.readMu.RLock()
+	defer h.readMu.RUnlock()
 	r := h.current.Load()
 	if r == nil {
 		return 0
@@ -131,22 +152,29 @@ func (h *SnapshotHolder) Len() int {
 // Close stops the reaper goroutine and closes the current reader + any
 // pending drains without waiting for the grace period. Safe to call
 // multiple times.
-func (h *SnapshotHolder) Close() error {
+func (h *SnapshotHolder[T]) Close() error {
 	var firstErr error
 	h.once.Do(func() {
 		close(h.stopCh)
+		// Close under Lock so no in-flight Get is reading these readers' mmap
+		// when we munmap them.
+		h.readMu.Lock()
 		r := h.current.Swap(nil)
 		if r != nil {
 			if err := r.Close(); err != nil {
 				firstErr = err
 			}
 		}
+		h.readMu.Unlock()
 		h.drainsMu.Lock()
 		drained := h.drains
 		h.drains = nil
 		h.drainsMu.Unlock()
 		for _, d := range drained {
-			if err := d.r.Close(); err != nil && firstErr == nil {
+			h.readMu.Lock()
+			err := d.r.Close()
+			h.readMu.Unlock()
+			if err != nil && firstErr == nil {
 				firstErr = err
 			}
 		}
@@ -156,7 +184,7 @@ func (h *SnapshotHolder) Close() error {

 // DrainCount returns the number of readers currently in the drain queue.
 // Useful for tests and operational metrics.
-func (h *SnapshotHolder) DrainCount() int {
+func (h *SnapshotHolder[T]) DrainCount() int {
 	h.drainsMu.Lock()
 	defer h.drainsMu.Unlock()
 	return len(h.drains)
@@ -164,10 +192,10 @@ func (h *SnapshotHolder) DrainCount() int {

 // reapNow scans the drain queue and Closes any readers whose grace has
 // elapsed. Called by the sweeper goroutine on a tick.
-func (h *SnapshotHolder) reapNow() {
+func (h *SnapshotHolder[T]) reapNow() {
 	now := time.Now()
-	var alive []*pendingClose
-	var reaped []*pendingClose
+	var alive []*pendingClose[T]
+	var reaped []*pendingClose[T]
 	h.drainsMu.Lock()
 	for _, d := range h.drains {
 		if !d.expireAt.After(now) {
@@ -179,11 +207,16 @@ func (h *SnapshotHolder) reapNow() {
 	h.drains = alive
 	h.drainsMu.Unlock()
 	for _, d := range reaped {
+		// Lock briefly so the munmap can't race an in-flight Get on this reader.
+		// Gets that loaded this reader hold RLock and finish before the Lock is
+		// granted; Gets starting after see the swapped-in reader, never this one.
+		h.readMu.Lock()
 		_ = d.r.Close()
+		h.readMu.Unlock()
 	}
 }

-func (h *SnapshotHolder) sweepLoop() {
+func (h *SnapshotHolder[T]) sweepLoop() {
 	interval := h.drainGrace / 4
 	if interval < 10*time.Millisecond {
 		interval = 10 * time.Millisecond