From 31ad2573807d9cf2d019a52d84e9befb6f934856 Mon Sep 17 00:00:00 2001 From: Reiers Date: Sat, 11 Jul 2026 13:16:34 +0200 Subject: [PATCH] fix(#114): wait for async touch/eviction goroutines before closing badger DB Store.Get bumped LRU asynchronously via 'go s.touch(c)', and Store.Put spawned 'go s.evictToTarget()' on over-cap. Neither goroutine was tracked, so Store.Close could return while they were mid-transaction against the underlying badger DB. Once db.Close() finished, the async goroutines panicked with a nil-deref inside badger's memTable traversal (getMemTables -> memTable.IncrRef). Fix: track background goroutines with sync.WaitGroup. spawnBG() gates Add() under a lifecycle mutex so a spawn racing Close is either counted (Close waits for it) or rejected (spawnBG returns nil). Close flips closed under the same mutex, then bg.Wait()s outside the lock, then closes the DB. Both async paths (touch and evictToTarget) use spawnBG uniformly. Adds TestCloseWaitsForAsyncTouchAndEviction, 32 iterations of the racy Get+Put+Close pattern the original test suite tripped on. Reproduces the pre-fix panic ~5/5 without -race; passes 10/10 with the fix. Closes #114. --- state/cache/cache.go | 55 ++++++++++++++++++++++++++++++++++++--- state/cache/cache_test.go | 54 ++++++++++++++++++++++++++++++++++++++ 2 files changed, 106 insertions(+), 3 deletions(-) diff --git a/state/cache/cache.go b/state/cache/cache.go index 4c267c0..bbc77cf 100644 --- a/state/cache/cache.go +++ b/state/cache/cache.go @@ -66,6 +66,16 @@ type Store struct { puts atomic.Uint64 evictions atomic.Uint64 evictionSem chan struct{} + + // Background goroutine lifecycle. spawnMu guards the atomic + // (closed.Load, bg.Add) ordering so a background job spawned right + // before Close is always counted in bg before Close waits on it. Close + // flips closed under spawnMu, then bg.Wait()s outside the lock, then + // calls db.Close(). This prevents async touch()/evictToTarget() + // goroutines from executing a Badger transaction against a torn-down + // DB (nil-deref in memTable.IncrRef). Fixes #114. + spawnMu sync.Mutex + bg sync.WaitGroup } // Options configures the persistent cache. @@ -103,14 +113,39 @@ func Open(path string, opts Options) (*Store, error) { return s, nil } -// Close flushes and closes the underlying Badger DB. +// Close flushes and closes the underlying Badger DB. Safe to call from +// any goroutine; concurrent Get/Put in flight when Close is entered are +// allowed to finish, and any background touch/eviction goroutines already +// spawned pre-Close are waited on before the DB is torn down. func (s *Store) Close() error { + s.spawnMu.Lock() if s.closed.Swap(true) { + s.spawnMu.Unlock() return nil } + s.spawnMu.Unlock() + // From here, spawnBG() sees closed=true under spawnMu and refuses to + // register new background jobs, so bg is monotonically decreasing. + s.bg.Wait() return s.db.Close() } +// spawnBG registers a background goroutine (touch / eviction) with the +// Store lifecycle. Returns nil if the Store is closing/closed; the +// caller then does nothing. Otherwise returns a done func the caller MUST +// call exactly once (defer) when the goroutine exits. spawnBG is O(1) +// and takes a short mutex, matching the pre-existing atomic-check cost +// of the goroutines it gates. +func (s *Store) spawnBG() func() { + s.spawnMu.Lock() + defer s.spawnMu.Unlock() + if s.closed.Load() { + return nil + } + s.bg.Add(1) + return s.bg.Done +} + // --- combined.Cache surface (hamt.BlockGetter + Put) --- // Get returns the raw bytes for c, or an error if absent. Bumps the @@ -131,7 +166,14 @@ func (s *Store) Get(_ context.Context, c cid.Cid) ([]byte, error) { } s.hits.Add(1) // Touch lastAccess asynchronously; never block a read on the LRU bump. - go s.touch(c) + // spawnBG gates against a concurrent Close so touch never runs against + // a torn-down DB (see #114). + if done := s.spawnBG(); done != nil { + go func() { + defer done() + s.touch(c) + }() + } return out, nil } @@ -252,8 +294,15 @@ func (s *Store) put(c cid.Cid, raw []byte, forcePin bool) error { } s.puts.Add(1) // Trigger eviction opportunistically when over cap (non-blocking). + // spawnBG gates against a concurrent Close so the goroutine never runs + // against a torn-down DB (see #114). if s.sizeBytes.Load() > s.softCap { - go s.evictToTarget() + if done := s.spawnBG(); done != nil { + go func() { + defer done() + s.evictToTarget() + }() + } } return nil } diff --git a/state/cache/cache_test.go b/state/cache/cache_test.go index 0326539..988878e 100644 --- a/state/cache/cache_test.go +++ b/state/cache/cache_test.go @@ -3,6 +3,7 @@ package cache import ( "context" "fmt" + "sync" "testing" "github.com/ipfs/go-cid" @@ -107,6 +108,59 @@ func TestPersistsAcrossReopen(t *testing.T) { } } +// TestCloseWaitsForAsyncTouchAndEviction is the regression test for #114. +// Before the fix, Store.Get spawned an async LRU-bump goroutine (touch) +// and Store.Put spawned an async over-cap eviction goroutine; both +// captured s.db and raced against Close, panicking with a nil-deref +// inside badger's memTable traversal when the DB tore down under them. +// After the fix, Close waits for any pre-Close background job to finish +// before closing the DB. This test hammers the racy pattern many times +// and asserts no goroutine escapes Close. +func TestCloseWaitsForAsyncTouchAndEviction(t *testing.T) { + for iter := 0; iter < 32; iter++ { + // Tiny cap so every Put trips the over-cap eviction spawn path. + s, err := Open(t.TempDir(), Options{SoftCapBytes: 128}) + if err != nil { + t.Fatalf("iter %d Open: %v", iter, err) + } + + // Seed one CID that Get can find and touch async. + data := []byte("seed-" + fmt.Sprint(iter)) + c := mkCID(t, data) + s.Put(c, data) + + // Fan out concurrent Gets (each fires go s.touch) and Puts (each + // fires go s.evictToTarget once over cap). Close is called with + // touches/evictions likely still in flight. + var wg sync.WaitGroup + for i := 0; i < 24; i++ { + wg.Add(1) + go func() { + defer wg.Done() + _, _ = s.Get(context.Background(), c) + }() + } + for i := 0; i < 24; i++ { + wg.Add(1) + go func(i int) { + defer wg.Done() + d := []byte(fmt.Sprintf("p-%d-%d", iter, i)) + s.Put(mkCID(t, d), d) + }(i) + } + wg.Wait() + + // Close must not panic and must return promptly even if async + // touch/evict goroutines are still executing at the moment of + // entry. If the fix regresses, badger's DB.Close will race and + // this call panics (or the async goroutines panic and crash the + // test binary). + if err := s.Close(); err != nil { + t.Fatalf("iter %d Close: %v", iter, err) + } + } +} + // TestEvictionRespectsSoftCapAndPins: over-cap inserts trigger LRU // eviction of unpinned blocks; pinned blocks survive. func TestEvictionRespectsSoftCapAndPins(t *testing.T) {