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) {