localstore: fix data race in test (#1452)

* localstore: fix data race in test

* fix goroutine leak in test

* undo

* remove superfluous done

pkg/localstore/gc.go

@@ -131,9 +131,9 @@ func (db *DB) collectGarbage() (collectedCount uint64, done bool, err error) {
 
 		collectedCount++
 		if collectedCount >= gcBatchSize {
-			// batch size limit reached,
-			// another gc run is needed
-			done = false
+			// batch size limit reached, however we don't
+			// know whether another gc run is needed until
+			// we weed out the dirty entries below
 			return true, nil
 		}
 		return false, nil
@@ -162,9 +162,6 @@ func (db *DB) collectGarbage() (collectedCount uint64, done bool, err error) {
 	for _, item := range candidates {
 		if swarm.NewAddress(item.Address).MemberOf(db.dirtyAddresses) {
 			collectedCount--
-			if gcSize-collectedCount > target {
-				done = false
-			}
 			continue
 		}
 
@@ -189,6 +186,10 @@ func (db *DB) collectGarbage() (collectedCount uint64, done bool, err error) {
 			return 0, false, err
 		}
 	}
+	if gcSize-collectedCount > target {
+		done = false
+	}
+
 	db.metrics.GCCommittedCounter.Add(float64(collectedCount))
 	db.gcSize.PutInBatch(batch, gcSize-collectedCount)
 

pkg/localstore/gc_test.go

@@ -725,21 +725,26 @@ func TestGC_NoEvictDirty(t *testing.T) {
 	// lower the maximal number of chunks in a single
 	// gc batch to ensure multiple batches.
 	defer func(s uint64) { gcBatchSize = s }(gcBatchSize)
-	gcBatchSize = 2
+	gcBatchSize = 1
 
-	chunkCount := 15
+	chunkCount := 10
 
 	db := newTestDB(t, &Options{
 		Capacity: 10,
 	})
 
-	closed := db.close
-
 	testHookCollectGarbageChan := make(chan uint64)
 	t.Cleanup(setTestHookCollectGarbage(func(collectedCount uint64) {
+		// don't trigger if we haven't collected anything - this may
+		// result in a race condition when we inspect the gcsize below,
+		// causing the database to shut down while the cleanup to happen
+		// before the correct signal has been communicated here.
+		if collectedCount == 0 {
+			return
+		}
 		select {
 		case testHookCollectGarbageChan <- collectedCount:
-		case <-closed:
+		case <-db.close:
 		}
 	}))
 
@@ -749,6 +754,7 @@ func TestGC_NoEvictDirty(t *testing.T) {
 		incomingChan <- struct{}{}
 		<-dirtyChan
 	}))
+	defer close(incomingChan)
 	addrs := make([]swarm.Address, 0)
 	mtx := new(sync.Mutex)
 	online := make(chan struct{})
@@ -773,7 +779,6 @@ func TestGC_NoEvictDirty(t *testing.T) {
 			}
 			dirtyChan <- struct{}{}
 		}
-
 	}()
 	<-online
 	// upload random chunks