Merge cockroachdb#125004

125004: util/mon: re-enable assertion about short-living monitors r=yuzefovich a=yuzefovich

This commit addresses a couple of issues that were identified when we enabled the assertion that all short-living monitors have been stopped when their parent is stopped:
- we need to stop the in-memory temp storage disk monitor only in single-tenant deployments (due to the same reason for why we don't stop root SQL memory monitor there - there is a race around ungraceful shutdown of the server)
- we need to mark `kvFeed` memory monitor as long-living similar to how other monitors used by CDC.

Fixes: cockroachdb#124848.

Release note: None

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

pkg/ccl/changefeedccl/changefeed_processors.go

@@ -399,7 +399,10 @@ func (ca *changeAggregator) startKVFeed(
 	opts changefeedbase.StatementOptions,
 ) (kvevent.Reader, chan struct{}, chan error, error) {
 	cfg := ca.flowCtx.Cfg
-	kvFeedMemMon := mon.NewMonitorInheritWithLimit("kvFeed", memLimit, parentMemMon, false /* longLiving */)
+	// CDC DistSQL flows are long-living, so we mark the memory monitors
+	// accordingly.
+	const longLiving = true
+	kvFeedMemMon := mon.NewMonitorInheritWithLimit("kvFeed", memLimit, parentMemMon, longLiving)
 	kvFeedMemMon.StartNoReserved(ctx, parentMemMon)
 	buf := kvevent.NewThrottlingBuffer(
 		kvevent.NewMemBuffer(kvFeedMemMon.MakeBoundAccount(), &cfg.Settings.SV, &ca.metrics.KVFeedMetrics),

pkg/server/server.go

@@ -718,7 +718,7 @@ func NewServer(cfg Config, stopper *stop.Stopper) (serverctl.ServerStartupInterf
 		//
 		// Note that we don't do this for SQL servers of tenants since there we
 		// can have ungraceful shutdown whenever the node is quiescing, so we
-		// have some short-living monitors that are't stopped.
+		// have some short-living monitors that aren't stopped.
 		sqlMonitorAndMetrics.rootSQLMemoryMonitor.EmergencyStop(ctx)
 	}))
 

pkg/server/server_sql.go

@@ -858,9 +858,17 @@ func newSQLServer(ctx context.Context, cfg sqlServerArgs) (*SQLServer, error) {
 		RootSQLMemoryPoolSize:    cfg.MemoryPoolSize,
 	}
 	cfg.TempStorageConfig.Mon.SetMetrics(distSQLMetrics.CurDiskBytesCount, distSQLMetrics.MaxDiskBytesHist)
-	cfg.stopper.AddCloser(stop.CloserFn(func() {
-		cfg.TempStorageConfig.Mon.EmergencyStop(ctx)
-	}))
+	if codec.ForSystemTenant() {
+		// Stop the temp storage disk monitor to enforce (in test builds) that
+		// all short-living descendants are stopped too.
+		//
+		// Note that we don't do this for SQL servers of tenants since there we
+		// can have ungraceful shutdown whenever the node is quiescing, so we
+		// have some short-living monitors that aren't stopped.
+		cfg.stopper.AddCloser(stop.CloserFn(func() {
+			cfg.TempStorageConfig.Mon.EmergencyStop(ctx)
+		}))
+	}
 	if distSQLTestingKnobs := cfg.TestingKnobs.DistSQL; distSQLTestingKnobs != nil {
 		distSQLCfg.TestingKnobs = *distSQLTestingKnobs.(*execinfra.TestingKnobs)
 	}

pkg/util/mon/bytes_usage.go

@@ -653,13 +653,12 @@ func (mm *BytesMonitor) doStop(ctx context.Context, check bool) {
 				// Ignore mm itself if it is short-living.
 				numShortLiving--
 			}
-			// TODO(#124848): uncomment this.
-			//if numShortLiving > 0 {
-			//	panic(errors.AssertionFailedf(
-			//		"found %d short-living non-stopped monitors in %s\n%s",
-			//		numShortLiving, mm.name, sb.String(),
-			//	))
-			//}
+			if numShortLiving > 0 {
+				panic(errors.AssertionFailedf(
+					"found %d short-living non-stopped monitors in %s\n%s",
+					numShortLiving, mm.name, sb.String(),
+				))
+			}
 		}
 	}