Merge #107577 #110376 #110379

107577: lint: add new deferunlockcheck pass r=Santamaura a=Santamaura

This commit adds a new pass which checks for
`...Unlock()` expressions without `defer` which
could result in a lock being held indefinitely.

Resolves #105366

Release note: None

110376: roachtest: fix logic for exact replication r=arulajmani,kvoli a=andrewbaptist

Previously the check for exact replication was incorrectly counting the number of replicas that were NOT at the desired replication count. This fortunately worked becase it was a no-op in the only place that used it since it was run immediately when all replicas had only a single replica.

This correctly fixes the check.

Epic: none
Fixes: #109905
Fixes: #109906

Release note: None

110379: roachpb: deflake and unskip test make priority r=arulajmani,nvanbenschoten a=kvoli

`TestMakePriority` was skipped in #110354 due to flakes. The test began
to flake after upgrading to `go1.20` in #109773, as the global rand was
seeded differently.

Bump the number of trials from 100k to 750k in order to collect more
samples of the underlying distribution, which deflakes the test. This
doubles the average test time, from 200ms to 400ms on a GCP C2 instance.

Passes 50k runs:

```
dev test pkg/roachpb -f TestMakePriority -v --stress --count=50000
...
//pkg/roachpb:roachpb_test                             PASSED in 0.9s
Stats over 50000 runs: max = 0.9s, min = 0.3s, avg = 0.4s, dev = 0.0s
```

Fixes: #110303
Release note: None

Co-authored-by: Alex Santamaura <[email protected]>
Co-authored-by: Andrew Baptist <[email protected]>
Co-authored-by: Austen McClernon <[email protected]>

Author: 4 people

BUILD.bazel

@@ -189,6 +189,7 @@ nogo(
             "@org_golang_x_tools//go/analysis/passes/unreachable:go_default_library",
             "@org_golang_x_tools//go/analysis/passes/unsafeptr:go_default_library",
             "@org_golang_x_tools//go/analysis/passes/unusedresult:go_default_library",
+            "//pkg/testutils/lint/passes/deferunlockcheck",
             "//pkg/testutils/lint/passes/descriptormarshal",
             "//pkg/testutils/lint/passes/errcheck",
             "//pkg/testutils/lint/passes/errcmp",

build/bazelutil/nogo_config.json

@@ -28,6 +28,170 @@
             "_test\\.go$": "tests"
         }
     },
+    "deferunlockcheck": {
+        "exclude_files": {
+            "pkg/.*\\.eg\\.go$": "generated code",
+            ".*\\.pb\\.go$": "generated code",
+            ".*\\.pb\\.gw\\.go$": "generated code",
+            "pkg/.*_generated\\.go$": "generated code",
+            "_test\\.go$": "tests",
+            "pkg/ccl/backupccl/backupinfo/manifest_handling.go": "flagged by linter, should be evaluated",
+            "pkg/ccl/backupccl/restore_job.go": "flagged by linter, should be evaluated",
+            "pkg/ccl/backupccl/restore_progress.go": "flagged by linter, should be evaluated",
+            "pkg/ccl/changefeedccl/cdctest/mock_webhook_sink.go": "flagged by linter, should be evaluated",
+            "pkg/ccl/changefeedccl/changefeed_processors.go": "flagged by linter, should be evaluated",
+            "pkg/ccl/changefeedccl/event_processing.go": "flagged by linter, should be evaluated",
+            "pkg/ccl/changefeedccl/kvevent/blocking_buffer.go": "flagged by linter, should be evaluated",
+            "pkg/ccl/changefeedccl/metrics.go": "flagged by linter, should be evaluated",
+            "pkg/ccl/changefeedccl/schemafeed/schema_feed.go": "flagged by linter, should be evaluated",
+            "pkg/ccl/changefeedccl/schema_registry.go": "flagged by linter, should be evaluated",
+            "pkg/ccl/changefeedccl/sink_kafka.go": "flagged by linter, should be evaluated",
+            "pkg/ccl/changefeedccl/sink_pubsub_v2.go": "flagged by linter, should be evaluated",
+            "pkg/ccl/multitenantccl/tenantcostclient/tenant_side.go": "flagged by linter, should be evaluated",
+            "pkg/ccl/sqlproxyccl/acl/watcher.go": "flagged by linter, should be evaluated",
+            "pkg/ccl/sqlproxyccl/tenantdirsvr/test_directory_svr.go": "flagged by linter, should be evaluated",
+            "pkg/ccl/streamingccl/streamclient/random_stream_client.go": "flagged by linter, should be evaluated",
+            "pkg/cli/clisqlexec/format_csv_tsv.go": "flagged by linter, should be evaluated",
+            "pkg/cli/clisqlshell/sql.go": "flagged by linter, should be evaluated",
+            "pkg/cli/syncbench/syncbench.go": "flagged by linter, should be evaluated",
+            "pkg/cmd/roachtest/cluster.go": "flagged by linter, should be evaluated",
+            "pkg/cmd/roachtest/test_runner.go": "flagged by linter, should be evaluated",
+            "pkg/gossip/client.go": "flagged by linter, should be evaluated",
+            "pkg/gossip/gossip.go": "flagged by linter, should be evaluated",
+            "pkg/config/system.go": "flagged by linter, should be evaluated",
+            "pkg/gossip/infostore.go": "flagged by linter, should be evaluated",
+            "pkg/gossip/server.go": "flagged by linter, should be evaluated",
+            "pkg/internal/rsg/rsg.go": "flagged by linter, should be evaluated",
+            "pkg/internal/sqlsmith/bulkio.go": "flagged by linter, should be evaluated",
+            "pkg/internal/sqlsmith/relational.go": "flagged by linter, should be evaluated",
+            "pkg/internal/sqlsmith/sampler.go": "flagged by linter, should be evaluated",
+            "pkg/internal/sqlsmith/scalar.go": "flagged by linter, should be evaluated",
+            "pkg/jobs/adopt.go": "flagged by linter, should be evaluated",
+            "pkg/jobs/job_scheduler.go": "flagged by linter, should be evaluated",
+            "pkg/jobs/progress.go": "flagged by linter, should be evaluated",
+            "pkg/kv/bulk/buffering_adder.go": "flagged by linter, should be evaluated",
+            "pkg/kv/bulk/sst_batcher.go": "flagged by linter, should be evaluated",
+            "pkg/kv/kvclient/kvcoord/dist_sender_mux_rangefeed.go": "flagged by linter, should be evaluated",
+            "pkg/kv/kvclient/kvcoord/txn_coord_sender.go": "flagged by linter, should be evaluated",
+            "pkg/kv/kvclient/kvstreamer/streamer.go": "flagged by linter, should be evaluated",
+            "pkg/kv/kvclient/kvtenant/connector.go": "flagged by linter, should be evaluated",
+            "pkg/kv/kvclient/rangecache/range_cache.go": "flagged by linter, should be evaluated",
+            "pkg/kv/kvclient/rangefeed/rangefeedcache/watcher.go": "flagged by linter, should be evaluated",
+            "pkg/kv/kvserver/allocator/allocatorimpl/allocator_scorer.go": "flagged by linter, should be evaluated",
+            "pkg/kv/kvserver/allocator/storepool/store_pool.go": "flagged by linter, should be evaluated",
+            "pkg/kv/kvserver/closedts/sidetransport/debug.go": "flagged by linter, should be evaluated",
+            "pkg/kv/kvserver/closedts/sidetransport/receiver.go": "flagged by linter, should be evaluated",
+            "pkg/kv/kvserver/closedts/sidetransport/sender.go": "flagged by linter, should be evaluated",
+            "pkg/kv/kvserver/concurrency/lock_table.go": "flagged by linter, should be evaluated",
+            "pkg/kv/kvserver/concurrency/lock_table_waiter.go": "flagged by linter, should be evaluated",
+            "pkg/kv/kvserver/intentresolver/intent_resolver.go": "flagged by linter, should be evaluated",
+            "pkg/kv/kvserver/liveness/liveness.go": "flagged by linter, should be evaluated",
+            "pkg/kv/kvserver/kvflowcontrol/kvflowcontroller/kvflowcontroller.go": "flagged by linter, should be evaluated",
+            "pkg/kv/kvserver/kvflowcontrol/kvflowhandle/kvflowhandle.go": "flagged by linter, should be evaluated",
+            "pkg/kv/kvserver/liveness/cache.go": "flagged by linter, should be evaluated",
+            "pkg/kv/kvserver/metrics.go": "flagged by linter, should be evaluated",
+            "pkg/kv/kvserver/queue.go": "flagged by linter, should be evaluated",
+            "pkg/kv/kvserver/raftentry/cache.go": "flagged by linter, should be evaluated",
+            "pkg/kv/kvserver/raft_log_queue.go": "flagged by linter, should be evaluated",
+            "pkg/kv/kvserver/raft_log_truncator.go": "flagged by linter, should be evaluated",
+            "pkg/kv/kvserver/raft_transport.go": "flagged by linter, should be evaluated",
+            "pkg/kv/kvserver/rangefeed/budget.go": "flagged by linter, should be evaluated",
+            "pkg/kv/kvserver/rangefeed/scheduler.go": "flagged by linter, should be evaluated",
+            "pkg/kv/kvserver/replica.go": "flagged by linter, should be evaluated",
+            "pkg/kv/kvserver/replica_app_batch.go": "flagged by linter, should be evaluated",
+            "pkg/kv/kvserver/replica_raft.go": "flagged by linter, should be evaluated",
+            "pkg/kv/kvserver/replica_raft_quiesce.go": "flagged by linter, should be evaluated",
+            "pkg/kv/kvserver/replica_raftstorage.go": "flagged by linter, should be evaluated",
+            "pkg/kv/kvserver/replica_rangefeed.go": "flagged by linter, should be evaluated",
+            "pkg/kv/kvserver/replica_write.go": "flagged by linter, should be evaluated",
+            "pkg/kv/kvserver/scheduler.go": "flagged by linter, should be evaluated",
+            "pkg/kv/kvserver/spanlatch/manager.go": "flagged by linter, should be evaluated",
+            "pkg/kv/kvserver/store.go": "flagged by linter, should be evaluated",
+            "pkg/kv/kvserver/store_create_replica.go": "flagged by linter, should be evaluated",
+            "pkg/kv/kvserver/store_gossip.go": "flagged by linter, should be evaluated",
+            "pkg/kv/kvserver/store_raft.go": "flagged by linter, should be evaluated",
+            "pkg/kv/kvserver/store_remove_replica.go": "flagged by linter, should be evaluated",
+            "pkg/kv/kvserver/txnwait/queue.go": "flagged by linter, should be evaluated",
+            "pkg/kv/kvnemesis/applier.go": "flagged by linter, should be evaluated",
+            "pkg/kv/kvnemesis/watcher.go": "flagged by linter, should be evaluated",
+            "pkg/kv/txn.go": "flagged by linter, should be evaluated",
+            "pkg/obs/event_exporter.go": "flagged by linter, should be evaluated",
+            "pkg/roachprod/install/cluster_synced.go": "flagged by linter, should be evaluated",
+            "pkg/roachprod/install/services.go": "flagged by linter, should be evaluated",
+            "pkg/roachprod/roachprod.go": "flagged by linter, should be evaluated",
+            "pkg/roachprod/vm/aws/aws.go": "flagged by linter, should be evaluated",
+            "pkg/roachprod/vm/azure/azure.go": "flagged by linter, should be evaluated",
+            "pkg/rpc/clock_offset.go": "flagged by linter, should be evaluated",
+            "pkg/rpc/peer.go": "flagged by linter, should be evaluated",
+            "pkg/settings/values.go": "flagged by linter, should be evaluated",
+            "pkg/server/node.go": "flagged by linter, should be evaluated",
+            "pkg/server/settings_cache.go": "flagged by linter, should be evaluated",
+            "pkg/server/telemetry/features": "flagged by linter, should be evaluated",
+            "pkg/spanconfig/spanconfigjob/job.go": "flagged by linter, should be evaluated",
+            "pkg/spanconfig/spanconfigkvsubscriber/kvsubscriber.go": "flagged by linter, should be evaluated",
+            "pkg/sql/catalog/lease/name_cache.go": "flagged by linter, should be evaluated",
+            "pkg/sql/colfetcher/colbatch_direct_scan.go": "flagged by linter, should be evaluated",
+            "pkg/sql/colfetcher/colbatch_scan.go": "flagged by linter, should be evaluated",
+            "pkg/sql/colfetcher/index_join.go": "flagged by linter, should be evaluated",
+            "pkg/sql/colflow/routers.go": "flagged by linter, should be evaluated",
+            "pkg/sql/colflow/stats.go": "flagged by linter, should be evaluated",
+            "pkg/sql/contention/contentionutils/concurrent_buffer_guard.go": "flagged by linter, should be evaluated",
+            "pkg/sql/contention/event_store.go": "flagged by linter, should be evaluated",
+            "pkg/sql/distsql_running.go": "flagged by linter, should be evaluated",
+            "pkg/sql/flowinfra/flow_registry.go": "flagged by linter, should be evaluated",
+            "pkg/sql/idxusage/local_idx_usage_stats.go": "flagged by linter, should be evaluated",
+            "pkg/sql/importer/import_processor.go": "flagged by linter, should be evaluated",
+            "pkg/sql/importer/import_processor_planning.go": "flagged by linter, should be evaluated",
+            "pkg/sql/pgwire/server.go": "flagged by linter, should be evaluated",
+            "pkg/sql/rowexec/indexbackfiller.go": "flagged by linter, should be evaluated",
+            "pkg/sql/rowflow/routers.go": "flagged by linter, should be evaluated",
+            "pkg/sql/schemachanger/corpus/corpus.go": "flagged by linter, should be evaluated",
+            "pkg/sql/sem/builtins/builtins.go": "flagged by linter, should be evaluated",
+            "pkg/sql/sqlliveness/slinstance/slinstance.go": "flagged by linter, should be evaluated",
+            "pkg/sql/sqlstats/ssmemstorage/ss_mem_iterator.go": "flagged by linter, should be evaluated",
+            "pkg/sql/stats/stats_cache.go": "flagged by linter, should be evaluated",
+            "pkg/sql/stmtdiagnostics/statement_diagnostics.go": "flagged by linter, should be evaluated",
+            "pkg/storage/metamorphic/deck.go": "flagged by linter, should be evaluated",
+            "pkg/storage/pebble.go": "flagged by linter, should be evaluated",
+            "pkg/util/admission/work_queue.go": "flagged by linter, should be evaluated",
+            "pkg/util/circuit/circuitbreaker.go": "flagged by linter, should be evaluated",
+            "pkg/util/every_n.go": "flagged by linter, should be evaluated",
+            "pkg/util/hlc/hlc.go": "flagged by linter, should be evaluated",
+            "pkg/util/interval/btree_based_interval.go": "flagged by linter, should be evaluated",
+            "pkg/util/log/buffered_sink.go": "flagged by linter, should be evaluated",
+            "pkg/util/log/channels.go": "flagged by linter, should be evaluated",
+            "pkg/util/log/clog.go": "flagged by linter, should be evaluated",
+            "pkg/util/log/file.go": "flagged by linter, should be evaluated",
+            "pkg/util/log/flags.go": "flagged by linter, should be evaluated",
+            "pkg/util/log/registry.go": "flagged by linter, should be evaluated",
+            "pkg/util/log/trace.go": "flagged by linter, should be evaluated",
+            "pkg/util/log/vmodule.go": "flagged by linter, should be evaluated",
+            "pkg/util/metric/hdrhistogram.go": "flagged by linter, should be evaluated",
+            "pkg/util/metric/metric.go": "flagged by linter, should be evaluated",
+            "pkg/util/mon/bytes_usage.go": "flagged by linter, should be evaluated",
+            "pkg/util/randutil/rand.go": "flagged by linter, should be evaluated",
+            "pkg/util/schedulerlatency/sampler.go": "flagged by linter, should be evaluated",
+            "pkg/util/stop/stopper.go": "flagged by linter, should be evaluated",
+            "pkg/util/startup/retry.go": "flagged by linter, should be evaluated",
+            "pkg/util/syncutil/int_map.go": "flagged by linter, should be evaluated",
+            "pkg/util/syncutil/mutex_sync_race.go": "flagged by linter, should be evaluated",
+            "pkg/util/syncutil/singleflight/singleflight.go": "flagged by linter, should be evaluated",
+            "pkg/util/tracing/crdbspan.go": "flagged by linter, should be evaluated",
+            "pkg/util/tracing/span.go": "flagged by linter, should be evaluated",
+            "pkg/util/tracing/tracer.go": "flagged by linter, should be evaluated",
+            "pkg/testutils/net.go": "flagged by linter, should be evaluated",
+            "pkg/testutils/storageutils/mocking.go": "flagged by linter, should be evaluated",
+            "pkg/testutils/testfixtures/test_fixtures.go": "flagged by linter, should be evaluated",
+            "pkg/workload/histogram/histogram.go": "flagged by linter, should be evaluated",
+            "pkg/workload/schemachange/deck.go": "flagged by linter, should be evaluated",
+            "pkg/workload/schemachange/schemachange.go": "flagged by linter, should be evaluated",
+            "pkg/workload/tpcc/generate.go": "flagged by linter, should be evaluated"
+        },
+        "only_files": {
+            "cockroach/pkg/.*$": "first-party code",
+            "cockroach/bazel-out/.*/bin/pkg/.*$": "first-party code"
+        }
+    },
     "errcheck": {
         "exclude_files": {
             "pkg/.*\\.eg\\.go$": "generated code",

pkg/BUILD.bazel

@@ -595,6 +595,7 @@ ALL_TESTS = [
     "//pkg/testutils/fingerprintutils:fingerprintutils_test",
     "//pkg/testutils/floatcmp:floatcmp_test",
     "//pkg/testutils/keysutils:keysutils_test",
+    "//pkg/testutils/lint/passes/deferunlockcheck:deferunlockcheck_test",
     "//pkg/testutils/lint/passes/errcmp:errcmp_test",
     "//pkg/testutils/lint/passes/errwrap:errwrap_test",
     "//pkg/testutils/lint/passes/fmtsafe:fmtsafe_test",
@@ -2174,6 +2175,8 @@ GO_TARGETS = [
     "//pkg/testutils/keysutils:keysutils",
     "//pkg/testutils/keysutils:keysutils_test",
     "//pkg/testutils/kvclientutils:kvclientutils",
+    "//pkg/testutils/lint/passes/deferunlockcheck:deferunlockcheck",
+    "//pkg/testutils/lint/passes/deferunlockcheck:deferunlockcheck_test",
     "//pkg/testutils/lint/passes/descriptormarshal:descriptormarshal",
     "//pkg/testutils/lint/passes/errcheck:errcheck",
     "//pkg/testutils/lint/passes/errcmp:errcmp",

pkg/cmd/roachtest/tests/util.go

@@ -106,7 +106,7 @@ func WaitForReplication(
 	var compStr string
 	switch waitForReplicationType {
 	case exactlyReplicationFactor:
-		compStr = "="
+		compStr = "!="
 	case atLeastReplicationFactor:
 		compStr = "<"
 	default:

pkg/cmd/roachvet/BUILD.bazel

@@ -6,6 +6,7 @@ go_library(
     importpath = "github.com/cockroachdb/cockroach/pkg/cmd/roachvet",
     visibility = ["//visibility:private"],
     deps = [
+        "//pkg/testutils/lint/passes/deferunlockcheck",
         "//pkg/testutils/lint/passes/errcmp",
         "//pkg/testutils/lint/passes/errwrap",
         "//pkg/testutils/lint/passes/fmtsafe",

pkg/cmd/roachvet/main.go

@@ -14,6 +14,7 @@
 package main
 
 import (
+	"github.com/cockroachdb/cockroach/pkg/testutils/lint/passes/deferunlockcheck"
 	"github.com/cockroachdb/cockroach/pkg/testutils/lint/passes/errcmp"
 	"github.com/cockroachdb/cockroach/pkg/testutils/lint/passes/errwrap"
 	"github.com/cockroachdb/cockroach/pkg/testutils/lint/passes/fmtsafe"
@@ -70,6 +71,7 @@ func main() {
 		nilness.Analyzer,
 		errwrap.Analyzer,
 		loopvarcapture.Analyzer,
+		deferunlockcheck.Analyzer,
 	)
 
 	// Standard go vet analyzers:

pkg/roachpb/BUILD.bazel

@@ -77,7 +77,6 @@ go_test(
         "//pkg/kv/kvserver/concurrency/isolation",
         "//pkg/kv/kvserver/concurrency/lock",
         "//pkg/storage/enginepb",
-        "//pkg/testutils/skip",
         "//pkg/testutils/zerofields",
         "//pkg/util",
         "//pkg/util/bitarray",

pkg/roachpb/data_test.go

@@ -26,7 +26,6 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/kv/kvserver/concurrency/isolation"
 	"github.com/cockroachdb/cockroach/pkg/kv/kvserver/concurrency/lock"
 	"github.com/cockroachdb/cockroach/pkg/storage/enginepb"
-	"github.com/cockroachdb/cockroach/pkg/testutils/skip"
 	"github.com/cockroachdb/cockroach/pkg/testutils/zerofields"
 	"github.com/cockroachdb/cockroach/pkg/util"
 	"github.com/cockroachdb/cockroach/pkg/util/bitarray"
@@ -909,7 +908,6 @@ func checkVal(val, expected, errFraction float64) bool {
 // in MakePriority returning priorities that are P times more likely
 // to be higher than a priority with user priority = 1.
 func TestMakePriority(t *testing.T) {
-	skip.WithIssue(t, 110303, "flaky test")
 	// Verify min & max.
 	if a, e := MakePriority(MinUserPriority), enginepb.MinTxnPriority; a != e {
 		t.Errorf("expected min txn priority %d; got %d", e, a)
@@ -932,7 +930,7 @@ func TestMakePriority(t *testing.T) {
 	}
 
 	// Generate values for all priorities.
-	const trials = 100000
+	const trials = 750000
 	values := make([][trials]enginepb.TxnPriority, len(userPs))
 	for i, userPri := range userPs {
 		for tr := 0; tr < trials; tr++ {

pkg/testutils/lint/lint_test.go

@@ -564,6 +564,7 @@ func TestLint(t *testing.T) {
 			":!*/doc.go",
 			":!util/syncutil/mutex_sync.go",
 			":!util/syncutil/mutex_sync_race.go",
+			":!testutils/lint/passes/deferunlockcheck/testdata/src/github.com/cockroachdb/cockroach/pkg/util/syncutil/mutex_sync.go",
 		)
 		if err != nil {
 			t.Fatal(err)

pkg/testutils/lint/passes/deferunlockcheck/BUILD.bazel

@@ -0,0 +1,32 @@
+load("@io_bazel_rules_go//go:def.bzl", "go_library", "go_test")
+
+go_library(
+    name = "deferunlockcheck",
+    srcs = ["deferunlockcheck.go"],
+    importpath = "github.com/cockroachdb/cockroach/pkg/testutils/lint/passes/deferunlockcheck",
+    visibility = ["//visibility:public"],
+    deps = [
+        "//pkg/testutils/lint/passes/loopvarcapture",
+        "//pkg/testutils/lint/passes/passesutil",
+        "@org_golang_x_tools//go/analysis",
+        "@org_golang_x_tools//go/analysis/passes/inspect",
+        "@org_golang_x_tools//go/ast/inspector",
+        "@org_golang_x_tools//go/types/typeutil",
+    ],
+)
+
+go_test(
+    name = "deferunlockcheck_test",
+    srcs = ["deferunlockcheck_test.go"],
+    args = ["-test.timeout=295s"],
+    data = glob(["testdata/**"]) + [
+        "@go_sdk//:files",
+    ],
+    deps = [
+        ":deferunlockcheck",
+        "//pkg/build/bazel",
+        "//pkg/testutils/datapathutils",
+        "//pkg/testutils/skip",
+        "@org_golang_x_tools//go/analysis/analysistest",
+    ],
+)