db: fix unpropagated error in Iterator.findPrevEntry

Previously, when iterating backwards within findPrevEntry, it was possible for
an error to stop backwards iteration without the error propagating up: If the
iterator state was already IterValid (eg, because we'd already observed a valid
KV), the error was ignored, and the most recent entry was returned. This could
cause a reader to observe an older version of the key than what was committed.

Author: jbowens

data_test.go

@@ -337,9 +337,7 @@ func printIterState(
 			validityStateStr = " at-limit"
 		}
 	}
-	if err := iter.Error(); err != nil {
-		fmt.Fprintf(b, "err=%v\n", err)
-	} else if validity == IterValid {
+	if validity == IterValid {
 		switch {
 		case iter.opts.pointKeys():
 			hasPoint, hasRange := iter.HasPointAndRange()
@@ -378,7 +376,11 @@ func printIterState(
 		}
 		fmt.Fprintln(b)
 	} else {
-		fmt.Fprintf(b, ".%s\n", validityStateStr)
+		if err := iter.Error(); err != nil {
+			fmt.Fprintf(b, "err=%v\n", err)
+		} else {
+			fmt.Fprintf(b, ".%s\n", validityStateStr)
+		}
 	}
 }
 

iterator.go

@@ -1088,8 +1088,14 @@ func (i *Iterator) findPrevEntry(limit []byte) {
 			return
 		}
 	}
-
 	// i.iterKey == nil, so broke out of the preceding loop.
+
+	// Is iterKey nil due to an error?
+	if i.err = i.iter.Error(); i.err != nil {
+		i.iterValidityState = IterExhausted
+		return
+	}
+
 	if i.iterValidityState == IterValid {
 		i.pos = iterPosPrev
 		if valueMerger != nil {

iterator_histories_test.go

@@ -68,7 +68,20 @@ func TestIterHistories(t *testing.T) {
 				err = firstError(err, iter.Close())
 				delete(iters, key)
 			}
+
 			if d != nil {
+				// Close all open snapshots.
+				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 {
+					err = firstError(err, ss[i].Close())
+				}
+
 				err = firstError(err, d.Close())
 				d = nil
 			}

testdata/iter_histories/errors

@@ -97,3 +97,50 @@ first
 ----
 err=injected error
 a: (a, .)
+
+# Test a scenario where an error occurs while the top-level Iterator is
+# Prev()-ing backwards through many versions of the same user key. In the below
+# test, reading the first block of the sstable (containing c.SET.13) fails. The
+# iterator must surface the error. Previously a bug existed that would allow the
+# iterator to mistakenly surface c.SET.12.
+
+define auto-compactions=off block-size=1 snapshots=(10,11,12,13)
+L1
+  c.SET.13:c13
+  c.SET.12:c12
+  c.SET.11:c11
+  c.SET.10:c10
+  c.SET.9:c9
+  c.SET.8:c8
+  d.SET.9:d9
+  e.SET.9:e9
+----
+1:
+  000004:[c#13,SET-e#9,SET]
+
+layout filename=000004.sst
+----
+         0  data (23)
+        28  data (23)
+        56  data (23)
+        84  data (23)
+       112  data (22)
+       139  data (22)
+       166  data (22)
+       193  index (113)
+       311  properties (643)
+       959  meta-index (33)
+       997  footer (53)
+      1050  EOF
+
+reopen auto-compactions=off enable-table-stats=false inject-errors=((ErrInjected (And (PathMatch "000004.sst") (OpFileReadAt 0))))
+----
+
+combined-iter
+last
+prev
+prev
+----
+e: (e9, .)
+d: (d9, .)
+err=injected error