storage: use AssertionFailedf for panics

Update a few panics that threw a simple string to throw an error constructed
through AssertionFailedf. Where applicable, this error is propagated through a
return value instead.

This is motivated by #150058, an instance where the panic string gets redacted,
requiring a bit more work to identify the origin of the panic (through mapping
the line number on the appropriate commit SHA).

Epic: none
Release note: none

Author: jbowens

pkg/storage/backup_compaction_iterator.go

@@ -116,7 +116,7 @@ func (f *BackupCompactionIterator) ValueLen() int {
 func (f *BackupCompactionIterator) HasPointAndRange() (bool, bool) {
 	hasPoint, hasRange := f.iter.HasPointAndRange()
 	if hasRange {
-		panic("unexpected range tombstone")
+		panic(errors.AssertionFailedf("unexpected range tombstone"))
 	}
 	return hasPoint, hasRange
 }

pkg/storage/batch.go

@@ -128,7 +128,7 @@ func (r *BatchReader) EngineKey() (EngineKey, error) {
 func (r *BatchReader) Value() []byte {
 	switch r.kind {
 	case pebble.InternalKeyKindDelete, pebble.InternalKeyKindSingleDelete:
-		panic("cannot call Value on a deletion entry")
+		panic(errors.AssertionFailedf("cannot call Value on a deletion entry"))
 	default:
 		return r.value
 	}

pkg/storage/batch_test.go

@@ -147,23 +147,55 @@ func TestBatchBasics(t *testing.T) {
 }
 
 func shouldPanic(t *testing.T, f func(), funcName string, expectedPanicStr string) {
+	t.Helper()
 	defer func() {
+		t.Helper()
 		if r := recover(); r == nil {
 			t.Fatalf("%v: test did not panic", funcName)
-		} else if r != expectedPanicStr {
-			t.Fatalf("%v: unexpected panic: %v", funcName, r)
+		} else if err, ok := r.(error); ok {
+			if errMsg := err.Error(); errMsg != expectedPanicStr {
+				t.Fatalf("%v: unexpected error panic: %q ≠ %q", funcName, errMsg, expectedPanicStr)
+			}
+		} else if errMsg, ok := r.(string); ok && errMsg != expectedPanicStr {
+			t.Fatalf("%v: unexpected panic: %q ≠ %q", funcName, errMsg, expectedPanicStr)
 		}
 	}()
 	f()
 }
 
-func shouldNotPanic(t *testing.T, f func(), funcName string) {
+func shouldPanicOrErr(t *testing.T, f func() error, funcName string, expectedPanicStr string) {
+	t.Helper()
+	var err error
+	defer func() {
+		t.Helper()
+		if r := recover(); r != nil {
+			if e, ok := r.(error); ok {
+				err = e
+			} else {
+				panic(r)
+			}
+		}
+	}()
+	err = f()
+	if err == nil {
+		t.Fatalf("%v: test did not panic", funcName)
+	} else if err.Error() != expectedPanicStr {
+		t.Fatalf("%v: unexpected panic: %q ≠ %q", funcName, err, expectedPanicStr)
+	}
+}
+
+func shouldNotPanicOrErr(t *testing.T, f func() error, funcName string) {
+	t.Helper()
 	defer func() {
+		t.Helper()
 		if r := recover(); r != nil {
 			t.Fatalf("%v: unexpected panic: %v", funcName, r)
 		}
 	}()
-	f()
+	err := f()
+	if err != nil {
+		t.Fatalf("%v: unexpected error: %v", funcName, err)
+	}
 }
 
 // TestReadOnlyBasics verifies that for a read-only ReadWriter (obtained via
@@ -181,23 +213,28 @@ func TestReadOnlyBasics(t *testing.T) {
 		t.Fatal("read-only is expectedly found to be closed")
 	}
 	a := mvccKey("a")
-	successTestCases := []func(){
-		func() {
-			_ = ro.MVCCIterate(context.Background(), a.Key, a.Key, MVCCKeyIterKind, IterKeyTypePointsOnly,
+	successTestCases := []func() error{
+		func() error {
+			return ro.MVCCIterate(context.Background(), a.Key, a.Key, MVCCKeyIterKind, IterKeyTypePointsOnly,
 				fs.UnknownReadCategory,
 				func(MVCCKeyValue, MVCCRangeKeyStack) error { return iterutil.StopIteration() })
 		},
-		func() {
-			iter, _ := ro.NewMVCCIterator(context.Background(), MVCCKeyIterKind, IterOptions{UpperBound: roachpb.KeyMax})
+		func() error {
+			iter, err := ro.NewMVCCIterator(context.Background(), MVCCKeyIterKind, IterOptions{UpperBound: roachpb.KeyMax})
 			iter.Close()
+			return err
 		},
-		func() {
-			iter, _ := ro.NewMVCCIterator(context.Background(), MVCCKeyIterKind, IterOptions{
+		func() error {
+			iter, err := ro.NewMVCCIterator(context.Background(), MVCCKeyIterKind, IterOptions{
 				MinTimestamp: hlc.MinTimestamp,
 				MaxTimestamp: hlc.MaxTimestamp,
 				UpperBound:   roachpb.KeyMax,
 			})
+			if err != nil {
+				return err
+			}
 			iter.Close()
+			return nil
 		},
 	}
 	defer func() {
@@ -207,25 +244,25 @@ func TestReadOnlyBasics(t *testing.T) {
 		}
 		shouldPanic(t, func() { ro.Close() }, "Close", "closing an already-closed pebbleReadOnly")
 		for i, f := range successTestCases {
-			shouldPanic(t, f, strconv.Itoa(i), "using a closed pebbleReadOnly")
+			shouldPanicOrErr(t, f, strconv.Itoa(i), "using a closed pebbleReadOnly")
 		}
 	}()
 
 	for i, f := range successTestCases {
-		shouldNotPanic(t, f, strconv.Itoa(i))
+		shouldNotPanicOrErr(t, f, strconv.Itoa(i))
 	}
 
 	// For a read-only ReadWriter, all Writer methods should panic.
-	failureTestCases := []func(){
-		func() { _ = ro.ApplyBatchRepr(nil, false) },
-		func() { _ = ro.ClearUnversioned(a.Key, ClearOptions{}) },
-		func() { _ = ro.SingleClearEngineKey(EngineKey{Key: a.Key}) },
-		func() { _ = ro.ClearRawRange(a.Key, a.Key, true, true) },
-		func() { _ = ro.Merge(a, nil) },
-		func() { _ = ro.PutUnversioned(a.Key, nil) },
+	failureTestCases := []func() error{
+		func() error { return ro.ApplyBatchRepr(nil, false) },
+		func() error { return ro.ClearUnversioned(a.Key, ClearOptions{}) },
+		func() error { return ro.SingleClearEngineKey(EngineKey{Key: a.Key}) },
+		func() error { return ro.ClearRawRange(a.Key, a.Key, true, true) },
+		func() error { return ro.Merge(a, nil) },
+		func() error { return ro.PutUnversioned(a.Key, nil) },
 	}
 	for i, f := range failureTestCases {
-		shouldPanic(t, f, strconv.Itoa(i), "not implemented")
+		shouldPanicOrErr(t, f, strconv.Itoa(i), "not implemented")
 	}
 
 	if err := e.PutUnversioned(mvccKey("a").Key, []byte("value")); err != nil {

pkg/storage/fs/file_registry.go

@@ -316,7 +316,7 @@ func (r *FileRegistry) maybeElideEntries(ctx context.Context) error {
 	for _, filename := range filenames {
 		entry, ok := r.writeMu.mu.entries[filename]
 		if !ok {
-			panic("entry disappeared from map")
+			panic(errors.AssertionFailedf("entry disappeared from map"))
 		}
 
 		// Some entries may be elided. This is used within

pkg/storage/intent_interleaving_iter.go

@@ -67,7 +67,7 @@ func (imr *intentInterleavingReader) NewMVCCIterator(
 ) (MVCCIterator, error) {
 	if (!opts.MinTimestamp.IsEmpty() || !opts.MaxTimestamp.IsEmpty()) &&
 		iterKind == MVCCKeyAndIntentsIterKind {
-		panic("cannot ask for interleaved intents when specifying timestamp hints")
+		return nil, errors.AssertionFailedf("cannot ask for interleaved intents when specifying timestamp hints")
 	}
 	if iterKind == MVCCKeyIterKind || opts.KeyTypes == IterKeyTypeRangesOnly {
 		return imr.wrappableReader.NewMVCCIterator(ctx, MVCCKeyIterKind, opts)
@@ -232,7 +232,7 @@ func newIntentInterleavingIterator(
 	ctx context.Context, reader Reader, opts IterOptions,
 ) (MVCCIterator, error) {
 	if !opts.MinTimestamp.IsEmpty() || !opts.MaxTimestamp.IsEmpty() {
-		panic("intentInterleavingIter must not be used with timestamp hints")
+		return nil, errors.AssertionFailedf("intentInterleavingIter must not be used with timestamp hints")
 	}
 	var lowerIsLocal, upperIsLocal bool
 	var constraint intentInterleavingIterConstraint
@@ -260,7 +260,7 @@ func newIntentInterleavingIterator(
 	if !opts.Prefix {
 		if opts.LowerBound == nil && opts.UpperBound == nil {
 			// This is the same requirement as pebbleIterator.
-			panic("iterator must set prefix or upper bound or lower bound")
+			return nil, errors.AssertionFailedf("iterator must set prefix or upper bound or lower bound")
 		}
 		// At least one bound is specified, so constraint != notConstrained. But
 		// may need to manufacture a bound for the currently unbounded side.

pkg/storage/lock_table_iterator.go

@@ -301,7 +301,7 @@ func (i *LockTableIterator) advanceToMatchingLock(
 					// zero UUID if we are in this branch, with the iterator positioned
 					// after the matchTxnID. Assert for good measure.
 					if i.matchTxnID == uuid.Nil {
-						panic("matchTxnID is unexpectedly the zero UUID")
+						panic(errors.AssertionFailedf("matchTxnID is unexpectedly the zero UUID"))
 					}
 					ltKey.TxnUUID = uuid.FromUint128(i.matchTxnID.ToUint128().Sub(1))
 					seekKey, *seekKeyBuf = ltKey.ToEngineKey(*seekKeyBuf)

pkg/storage/mvcc.go

@@ -1005,7 +1005,7 @@ func updateStatsOnClear(
 	// restoredNanos to orig.Timestamp (rule 1).
 	if restored != nil {
 		if restored.Txn != nil {
-			panic("restored version should never be an intent")
+			panic(errors.AssertionFailedf("restored version should never be an intent"))
 		}
 
 		ms.AgeTo(restoredNanos)

pkg/storage/open.go

@@ -354,7 +354,7 @@ func WALFailover(
 				return nil
 			}
 		default:
-			panic("unreachable")
+			panic(errors.AssertionFailedf("unreachable"))
 		}
 	}
 
@@ -382,7 +382,7 @@ func WALFailover(
 	case storageconfig.WALFailoverAmongStores:
 		// Fallthrough
 	default:
-		panic("unreachable")
+		panic(errors.AssertionFailedf("unreachable"))
 	}
 
 	// Either