Merge #130346

130346: server: fix race in `TestCachedSettingsServerRestart`. r=stevendanna a=shubhamdhama

Before this change, we picked the expected value without waiting for all settings to be updated in the cache. This could lead to a race condition in rare cases where the settings watcher had not yet received some of the settings through rangefeed. We would assign those incomplete settings to `expectedSettingsCount`, and just before stopping the server, the settings cache would receive the remaining ones.

Informs: #124419. We haven't seen this issue on the current master yet, likely due to some improvements in rangefeed performance. This fix will be backported.

Epic: CRDB-38882
Release note: None

Co-authored-by: Shubham Dhama <[email protected]>

pkg/server/settings_cache_test.go

@@ -81,11 +81,12 @@ func TestCachedSettingsServerRestart(t *testing.T) {
 			},
 		},
 	}
-	var settingsCache []roachpb.KeyValue
+	var expectedSettingsCache []roachpb.KeyValue
 	ts := serverutils.StartServerOnly(t, serverArgs)
 	closedts.TargetDuration.Override(ctx, &ts.ClusterSettings().SV, 10*time.Millisecond)
 	closedts.SideTransportCloseInterval.Override(ctx, &ts.ClusterSettings().SV, 10*time.Millisecond)
 	kvserver.RangeFeedRefreshInterval.Override(ctx, &ts.ClusterSettings().SV, 10*time.Millisecond)
+	const expectedSettingsCount = 3
 	testutils.SucceedsSoon(t, func() error {
 		store, err := ts.GetStores().(*kvserver.Stores).GetStore(1)
 		if err != nil {
@@ -95,10 +96,18 @@ func TestCachedSettingsServerRestart(t *testing.T) {
 		if err != nil {
 			return err
 		}
-		if len(settings) == 0 {
-			return errors.New("empty settings loaded from store")
+
+		// Previously, we checked if len(settings) > 0, which led to a race
+		// condition where, in rare cases (under --race), the settings watcher
+		// had not yet received some settings through rangefeed. If we exit this
+		// function and assign expectedSettingsCount with those incomplete
+		// settings, the settings watcher may receive the remaining settings
+		// before we stop the server. See issue #124419 for more details.
+		if len(settings) < expectedSettingsCount {
+			return errors.Newf("unexpected count of settings: expected %d, found %d",
+				expectedSettingsCount, len(settings))
 		}
-		settingsCache = settings
+		expectedSettingsCache = settings
 		return nil
 	})
 	ts.Stopper().Stop(context.Background())
@@ -141,11 +150,11 @@ func TestCachedSettingsServerRestart(t *testing.T) {
 		if initialBoot {
 			return errors.New("server should not require initialization")
 		}
-		if !assert.ObjectsAreEqual(state.initialSettingsKVs, settingsCache) {
+		if !assert.ObjectsAreEqual(expectedSettingsCache, state.initialSettingsKVs) {
 			return errors.Newf(`initial state settings KVs does not match expected settings
 Expected: %+v
 Actual:   %+v
-`, settingsCache, state.initialSettingsKVs)
+`, expectedSettingsCache, state.initialSettingsKVs)
 		}
 		return nil
 	})