db: return error from Close if open snapshots remain

Return an error from DB.Close if the database has open snapshots. This
helps ensure clients don't accidentally leak snapshots, which during
normal operation prevents dropping any keys older than the snapshot.

Fix cockroachdb#1986.

compaction_test.go

@@ -1222,6 +1222,7 @@ func TestManualCompaction(t *testing.T) {
 	var d *DB
 	defer func() {
 		if d != nil {
+			require.NoError(t, closeAllSnapshots(d))
 			require.NoError(t, d.Close())
 		}
 	}()
@@ -1236,6 +1237,7 @@ func TestManualCompaction(t *testing.T) {
 
 	reset := func(minVersion, maxVersion FormatMajorVersion) {
 		if d != nil {
+			require.NoError(t, closeAllSnapshots(d))
 			require.NoError(t, d.Close())
 		}
 		mem = vfs.NewMem()
@@ -1330,6 +1332,9 @@ func TestManualCompaction(t *testing.T) {
 
 			case "define":
 				if d != nil {
+					if err := closeAllSnapshots(d); err != nil {
+						return err.Error()
+					}
 					if err := d.Close(); err != nil {
 						return err.Error()
 					}
@@ -1840,6 +1845,7 @@ func TestCompactionDeleteOnlyHints(t *testing.T) {
 	var d *DB
 	defer func() {
 		if d != nil {
+			require.NoError(t, closeAllSnapshots(d))
 			require.NoError(t, d.Close())
 		}
 	}()
@@ -1848,6 +1854,9 @@ func TestCompactionDeleteOnlyHints(t *testing.T) {
 	reset := func() (*Options, error) {
 		if d != nil {
 			compactInfo = nil
+			if err := closeAllSnapshots(d); err != nil {
+				return nil, err
+			}
 			if err := d.Close(); err != nil {
 				return nil, err
 			}
@@ -2087,6 +2096,7 @@ func TestCompactionTombstones(t *testing.T) {
 	var d *DB
 	defer func() {
 		if d != nil {
+			require.NoError(t, closeAllSnapshots(d))
 			require.NoError(t, d.Close())
 		}
 	}()
@@ -2116,6 +2126,7 @@ func TestCompactionTombstones(t *testing.T) {
 			case "define":
 				if d != nil {
 					compactInfo = nil
+					require.NoError(t, closeAllSnapshots(d))
 					if err := d.Close(); err != nil {
 						return err.Error()
 					}
@@ -2189,6 +2200,22 @@ func TestCompactionTombstones(t *testing.T) {
 		})
 }
 
+func closeAllSnapshots(d *DB) error {
+	d.mu.Lock()
+	var ss []*Snapshot
+	l := &d.mu.snapshots
+	for i := l.root.next; i != &l.root; i = i.next {
+		ss = append(ss, i)
+	}
+	d.mu.Unlock()
+	for i := range ss {
+		if err := ss[i].Close(); err != nil {
+			return err
+		}
+	}
+	return nil
+}
+
 func TestCompactionReadTriggeredQueue(t *testing.T) {
 
 	// Convert a read compaction to a string which this test
@@ -2597,6 +2624,7 @@ func TestCompactionAllowZeroSeqNum(t *testing.T) {
 	var d *DB
 	defer func() {
 		if d != nil {
+			require.NoError(t, closeAllSnapshots(d))
 			require.NoError(t, d.Close())
 		}
 	}()
@@ -2628,6 +2656,7 @@ func TestCompactionAllowZeroSeqNum(t *testing.T) {
 			switch td.Cmd {
 			case "define":
 				if d != nil {
+					require.NoError(t, closeAllSnapshots(d))
 					if err := d.Close(); err != nil {
 						return err.Error()
 					}

db.go

@@ -1321,6 +1321,12 @@ func (d *DB) Close() error {
 	if d.opts.private.fsCloser != nil {
 		d.opts.private.fsCloser.Close()
 	}
+
+	// Return an error if the user failed to close all open snapshots.
+	if v := d.mu.snapshots.count(); v > 0 {
+		err = firstError(err, errors.Errorf("leaked snapshots: %d open snapshots on DB %p", v, d))
+	}
+
 	return err
 }
 

db_test.go

@@ -661,6 +661,7 @@ func TestUnremovableSingleDelete(t *testing.T) {
 
 	require.NoError(t, d.Set(key, valFirst, nil))
 	ss := d.NewSnapshot()
+	defer ss.Close()
 	require.NoError(t, d.SingleDelete(key, nil))
 	require.NoError(t, d.Set(key, valSecond, nil))
 	require.NoError(t, d.Flush())

range_del_test.go

@@ -27,6 +27,7 @@ func TestRangeDel(t *testing.T) {
 	var d *DB
 	defer func() {
 		if d != nil {
+			require.NoError(t, closeAllSnapshots(d))
 			require.NoError(t, d.Close())
 		}
 	}()
@@ -37,6 +38,9 @@ func TestRangeDel(t *testing.T) {
 		switch td.Cmd {
 		case "define":
 			if d != nil {
+				if err := closeAllSnapshots(d); err != nil {
+					return err.Error()
+				}
 				if err := d.Close(); err != nil {
 					return err.Error()
 				}

table_stats_test.go

@@ -40,6 +40,7 @@ func TestTableStats(t *testing.T) {
 	require.NoError(t, err)
 	defer func() {
 		if d != nil {
+			require.NoError(t, closeAllSnapshots(d))
 			require.NoError(t, d.Close())
 		}
 	}()
@@ -60,6 +61,7 @@ func TestTableStats(t *testing.T) {
 			return ""
 
 		case "define":
+			require.NoError(t, closeAllSnapshots(d))
 			require.NoError(t, d.Close())
 			loadedInfo = nil