Merge #152001

152001: pkg/ccl/oidcccl,pkg/cli,pkg/sql/vecindex/cspann,pkg/storage: bug fixes r=sumeerbhola,dhartunian,InManuBytes a=srosenberg

The PR fixes several latent bugs identified by using a hybrid
linter strategy, detailed in [1]. These comprise,

* nil-pointer dereference when err != nil
* calling require.* inside background goroutine

[1] https://gist.github.com/srosenberg/3e36c70b5e78c85b84e355ee709b105d

Epic: none
Release note: None

Co-authored-by: Stan Rosenberg <[email protected]>

Author: craig[bot]

pkg/ccl/oidcccl/authentication_oidc.go

@@ -804,9 +804,9 @@ var ConfigureOIDC = func(
 					}
 				} else {
 					log.Infof(ctx, "OIDC: no identity map found for issuer %s; using %s without mapping", token.Issuer, tokenPrincipal)
-					if username, err := secuser.MakeSQLUsernameFromUserInput(tokenPrincipal, secuser.PurposeValidation); err != nil {
-						acceptedUsernames = append(acceptedUsernames, username.Normalized())
-					}
+					// N.B. err is elided when using secuser.PurposeValidation.
+					username, _ := secuser.MakeSQLUsernameFromUserInput(tokenPrincipal, secuser.PurposeValidation)
+					acceptedUsernames = append(acceptedUsernames, username.Normalized())
 				}
 			}
 		}
@@ -835,6 +835,7 @@ var ConfigureOIDC = func(
 		if err != nil {
 			log.Error(ctx, "OIDC: failed to marshal connection parameters (can this happen?)")
 			http.Error(w, genericCallbackHTTPError, http.StatusInternalServerError)
+			return
 		}
 
 		w.Header().Add("Content-Security-Policy", "sandbox")

pkg/cli/statement_bundle.go

@@ -432,7 +432,7 @@ func getExplainCombinations(
 				bucketMap[key] = []string{upperBound}
 				datum, err := rowenc.ParseDatumStringAs(ctx, colType, upperBound, &evalCtx, nil /* semaCtx */)
 				if err != nil {
-					panic("failed parsing datum string as " + datum.String() + " " + err.Error())
+					panic("failed parsing datum string as " + colType.String() + " " + err.Error())
 				}
 				if maxUpperBound == nil {
 					maxUpperBound = datum

pkg/cmd/roachtest/cluster.go

@@ -2557,6 +2557,9 @@ func (c *clusterImpl) RunWithDetailsSingleNode(
 		return install.RunResultDetails{}, errors.Newf("RunWithDetailsSingleNode received %d nodes. Use RunWithDetails if you need to run on multiple nodes.", len(nodes))
 	}
 	results, err := c.RunWithDetails(ctx, testLogger, options, args...)
+	if err != nil {
+		return install.RunResultDetails{}, err
+	}
 	return results[0], errors.CombineErrors(err, results[0].Err)
 }
 

pkg/cmd/roachtest/tests/mixed_version_cdc.go

@@ -702,7 +702,7 @@ func runCDCMixedVersionCheckpointing(ctx context.Context, t test.Test, c cluster
 			return false
 		}
 
-		if isAffectedBy148620(plan) && isExpectedErrorDueTo148620(err) {
+		if plan != nil && isAffectedBy148620(plan) && isExpectedErrorDueTo148620(err) {
 			t.Skipf("expected error due to #148620: %s", err)
 		}
 

pkg/cmd/roachtest/tests/tpc_utils.go

@@ -38,7 +38,7 @@ func loadTPCHDataset(
 	disableMergeQueue bool,
 ) (retErr error) {
 	_, err := db.Exec("SET CLUSTER SETTING sql.stats.automatic_collection.enabled = false;")
-	if retErr != nil {
+	if err != nil {
 		return err
 	}
 	defer func() {

pkg/sql/vecindex/cspann/BUILD.bazel

@@ -88,6 +88,7 @@ go_test(
         "@com_github_cockroachdb_datadriven//:datadriven",
         "@com_github_cockroachdb_errors//:errors",
         "@com_github_guptarohit_asciigraph//:asciigraph",
+        "@com_github_stretchr_testify//assert",
         "@com_github_stretchr_testify//require",
         "@org_golang_x_exp//slices",
         "@org_gonum_v1_gonum//floats/scalar",

pkg/sql/vecindex/cspann/fixup_processor_test.go

@@ -14,6 +14,7 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/util/leaktest"
 	"github.com/cockroachdb/cockroach/pkg/util/log"
 	"github.com/cockroachdb/cockroach/pkg/util/stop"
+	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 )
 
@@ -130,16 +131,16 @@ func TestProcess(t *testing.T) {
 		var counter atomic.Int32
 		fp.AddSplit(ctx, nil /* treeKey */, 1, 2, false /* singleStep */)
 		go func() {
-			require.NoError(t, fp.Process(ctx))
+			assert.NoError(t, fp.Process(ctx))
 			// counter should already have been incremented by the foreground
 			// goroutine.
-			require.Greater(t, counter.Add(1), int32(1))
+			assert.Greater(t, counter.Add(1), int32(1))
 		}()
 		go func() {
-			require.NoError(t, fp.Process(ctx))
+			assert.NoError(t, fp.Process(ctx))
 			// counter should already have been incremented by the foreground
 			// goroutine.
-			require.Greater(t, counter.Add(1), int32(1))
+			assert.Greater(t, counter.Add(1), int32(1))
 		}()
 
 		// Small delay to allow background goroutines to run.

pkg/sql/vecindex/cspann/index_test.go

@@ -34,6 +34,7 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/util/vector"
 	"github.com/cockroachdb/datadriven"
 	"github.com/cockroachdb/errors"
+	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 	"golang.org/x/exp/slices"
 )
@@ -998,7 +999,7 @@ func TestIndexConcurrency(t *testing.T) {
 				}
 
 				// Process any remaining fixups and close the index.
-				require.NoError(t, index.ProcessFixups(ctx))
+				assert.NoError(t, index.ProcessFixups(ctx))
 				index.Close()
 			}(i, min(i+vecsPerInstance, vectors.Count))
 		}

pkg/storage/sst.go

@@ -537,20 +537,20 @@ func CheckSSTConflicts(
 		UpperBound: keys.MaxKey,
 	})
 	if err != nil {
-		rkIter.Close()
 		return enginepb.MVCCStats{}, err
 	}
 	rkIter.SeekGE(NilKey)
 
-	if ok, err := rkIter.Valid(); err != nil {
-		rkIter.Close()
+	ok, err := rkIter.Valid()
+	rkIter.Close()
+	if err != nil {
 		return enginepb.MVCCStats{}, err
-	} else if ok {
+	}
+	if ok {
 		// If the incoming SST contains range tombstones, we cannot use prefix
 		// iteration.
 		usePrefixSeek = false
 	}
-	rkIter.Close()
 
 	rkIter, err = reader.NewMVCCIterator(ctx, MVCCKeyIterKind, IterOptions{
 		UpperBound:   rightPeekBound,
@@ -563,16 +563,17 @@ func CheckSSTConflicts(
 	rkIter.SeekGE(start)
 
 	var engineHasRangeKeys bool
-	if ok, err := rkIter.Valid(); err != nil {
-		rkIter.Close()
+	ok, err = rkIter.Valid()
+	rkIter.Close()
+	if err != nil {
 		return enginepb.MVCCStats{}, err
-	} else if ok {
+	}
+	if ok {
 		// If the engine contains range tombstones in this span, we cannot use prefix
 		// iteration.
 		usePrefixSeek = false
 		engineHasRangeKeys = true
 	}
-	rkIter.Close()
 
 	if usePrefixSeek {
 		// Prefix iteration and range key masking don't work together. See the