Merge #149943

149943: testutils/lint: add a linter to prohibit ignoring cancel func r=yuzefovich a=yuzefovich

We recently fixed a couple of issues (one was fresh, another was dormant
for several years) where we had a memory leak because the cancel
function returned on `stop.Stopper.WithCancelOnQuiesce` was ignored. In
some cases (like when we have a singleton component that has lifetime
matching that of the server) this is benign, but in some other cases
this can be problematic. In order to prevent similar memory leaks from
sneaking in in the future, this commit adds a linter that prohibits
ignoring the cancel func. For the cases where it's benign, we add
`nolint:quiesce` option.

Informs: #149658.
Epic: None

Release note: None

Co-authored-by: Yahor Yuzefovich <[email protected]>

Author: craig[bot]

pkg/ccl/cliccl/mt_proxy.go

@@ -108,7 +108,9 @@ func initLogging(cmd *cobra.Command) (ctx context.Context, stopper *stop.Stopper
 	if err != nil {
 		return ctx, nil, err
 	}
-	ctx, _ = stopper.WithCancelOnQuiesce(ctx)
+	// The proxy will shutdown via the stopper, so we can ignore the
+	// cancellation function here.
+	ctx, _ = stopper.WithCancelOnQuiesce(ctx) // nolint:quiesce
 	return ctx, stopper, err
 }
 

pkg/ccl/sqlproxyccl/balancer/balancer.go

@@ -223,7 +223,10 @@ func NewBalancer(
 	}
 
 	// Ensure that ctx gets cancelled on stopper's quiescing.
-	ctx, _ = stopper.WithCancelOnQuiesce(ctx)
+	//
+	// The balancer shares the same lifetime as the proxy which will shutdown
+	// via the stopper, so we can ignore the cancellation function here.
+	ctx, _ = stopper.WithCancelOnQuiesce(ctx) // nolint:quiesce
 
 	q, err := newRebalancerQueue(ctx, metrics)
 	if err != nil {

pkg/ccl/sqlproxyccl/balancer/conn_tracker.go

@@ -48,7 +48,10 @@ func NewConnTracker(
 	ctx context.Context, stopper *stop.Stopper, timeSource timeutil.TimeSource,
 ) (*ConnTracker, error) {
 	// Ensure that ctx gets cancelled on stopper's quiescing.
-	ctx, _ = stopper.WithCancelOnQuiesce(ctx)
+	//
+	// The tracker shares the same lifetime as the proxy which will shutdown
+	// via the stopper, so we can ignore the cancellation function here.
+	ctx, _ = stopper.WithCancelOnQuiesce(ctx) // nolint:quiesce
 
 	if timeSource == nil {
 		timeSource = timeutil.DefaultTimeSource{}

pkg/ccl/sqlproxyccl/proxy_handler.go

@@ -194,7 +194,9 @@ func newProxyHandler(
 	proxyMetrics *metrics,
 	options ProxyOptions,
 ) (*proxyHandler, error) {
-	ctx, _ = stopper.WithCancelOnQuiesce(ctx)
+	// The proxy handler shares the same lifetime as the proxy which will shutdown
+	// via the stopper, so we can ignore the cancellation function here.
+	ctx, _ = stopper.WithCancelOnQuiesce(ctx) // nolint:quiesce
 
 	handler := proxyHandler{
 		stopper:       stopper,

pkg/rpc/context.go

@@ -546,7 +546,9 @@ func NewContext(ctx context.Context, opts ContextOptions) *Context {
 		}
 	}
 
-	masterCtx, _ := opts.Stopper.WithCancelOnQuiesce(ctx)
+	// The RPC context has the same lifetime as the stopper (which often matches
+	// the lifetime of the server), so we can ignore the cancellation function.
+	masterCtx, _ := opts.Stopper.WithCancelOnQuiesce(ctx) // nolint:quiesce
 
 	secCtx := NewSecurityContext(
 		SecurityContextOptions{

pkg/sql/stats/automatic_stats.go

@@ -533,7 +533,9 @@ func (r *Refresher) SetDraining() {
 func (r *Refresher) Start(
 	ctx context.Context, stopper *stop.Stopper, refreshInterval time.Duration,
 ) error {
-	stoppingCtx, _ := stopper.WithCancelOnQuiesce(context.Background())
+	// The refresher has the same lifetime as the server, so the cancellation
+	// function can be ignored and it'll be called by the stopper.
+	stoppingCtx, _ := stopper.WithCancelOnQuiesce(context.Background()) // nolint:quiesce
 	bgCtx := r.AnnotateCtx(stoppingCtx)
 	r.startedTasksWG.Add(1)
 	if err := stopper.RunAsyncTask(bgCtx, "refresher", func(ctx context.Context) {

pkg/sql/stmtdiagnostics/statement_diagnostics.go

@@ -150,7 +150,9 @@ func NewRegistry(db isql.DB, st *cluster.Settings) *Registry {
 
 // Start will start the polling loop for the Registry.
 func (r *Registry) Start(ctx context.Context, stopper *stop.Stopper) {
-	ctx, _ = stopper.WithCancelOnQuiesce(ctx)
+	// The registry has the same lifetime as the server, so the cancellation
+	// function can be ignored and it'll be called by the stopper.
+	ctx, _ = stopper.WithCancelOnQuiesce(ctx) // nolint:quiesce
 
 	// Since background statement diagnostics collection is not under user
 	// control, exclude it from cost accounting and control.

pkg/testutils/lint/lint_test.go

@@ -2593,6 +2593,49 @@ func TestLint(t *testing.T) {
 		}
 	})
 
+	// This linter prohibits ignoring the context.CancelFunc that is returned on
+	// stop.Stopper.WithCancelOnQuiesce call (which can result in a memory
+	// leak).
+	//
+	// If the context is derived for a server singleton and has the same
+	// lifetime as the server, this linter can be ignored with
+	// 'nolint:quiesce' comment.
+	t.Run("TestWithCancelOnQuiesce", func(t *testing.T) {
+		t.Parallel()
+		cmd, stderr, filter, err := dirCmd(
+			pkgDir,
+			"git",
+			"grep",
+			"-nE",
+			`_.*WithCancelOnQuiesce`,
+			"--",
+			"*",
+			":!*_test.go",
+		)
+		if err != nil {
+			t.Fatal(err)
+		}
+
+		if err := cmd.Start(); err != nil {
+			t.Fatal(err)
+		}
+
+		if err := stream.ForEach(stream.Sequence(
+			filter,
+			stream.GrepNot(`nolint:quiesce`),
+		), func(s string) {
+			t.Errorf("\n%s <- forbidden; ensure the cancellation function is called", s)
+		}); err != nil {
+			t.Error(err)
+		}
+
+		if err := cmd.Wait(); err != nil {
+			if out := stderr.String(); len(out) > 0 {
+				t.Fatalf("err=%s, stderr=%s", err, out)
+			}
+		}
+	})
+
 	// RoachVet is expensive memory-wise and thus should not run with t.Parallel().
 	// RoachVet includes all of the passes of `go vet` plus first-party additions.
 	// See pkg/cmd/roachvet.