Fix flaky TestRaceBetweenConfigPollAndDbConfigUpdate

[torcolvin]

Fix flaky TestRaceBetweenConfigPollAndDbConfigUpdate

2026-07-27T01:44:45.788Z [INF] HTTP: c:#562 db:db1 #562:     --> 500 Internal error: UpdateConfig failed to persist updated registry after 5 attempts:  (409.1 ms)
    api_collections_test.go:987: 
        	Error Trace:	/home/ubuntu/workspace/Pipeline_PR-8480/rest/utilities_testing.go:1289
        	            				/home/ubuntu/workspace/Pipeline_PR-8480/rest/api_collections_test.go:987
        	Error:      	Not equal: 
        	            	expected: 201
        	            	actual  : 500
        	Test:       	TestRaceBetweenConfigPollAndDbConfigUpdate
        	Messages:   	Response status 500 "Internal Server Error" (expected 201 "Created")
        	            	for POST <http://127.0.0.1/db1/_config> : {"error":"Internal Server Error","reason":"Internal error: UpdateConfig failed to persist updated registry after 5 attempts:"}
2026-07-27T01:44:45.789Z [INF] db:db1 Closing db /db1 (bucket "rosmar1")

UpdateConfig's registry-persist retry loop and the periodic cluster-compat node heartbeat (clusterCompatManager.Refresh) both CAS-write the same_sync:registry document. The test drives ConfigUpdateFrequency down to 50ms to stress a different race (db config update vs. config poll), but this also cranks up the heartbeat's write cadence to roughly match the retry loop's own cadence, so all 5 of UpdateConfig's CAS-retry attempts could collide with the heartbeat and exhaust the retry budget, surfacing as a spurious 500 to the client and failing the test.

The original fixes were to increase the retries by configUpdateMaxRetryAttempts * 5 but I wanted to showcase a fix of a jitter instead of an increased retries, which may never unstick. This seems good for production code as well, but writing to CBS is likely to add a jitter anyway.

[factory-droid]

Droid finished @torcolvin's task —— View job

---

[Copilot]

Pull request overview

This PR addresses flakiness in TestRaceBetweenConfigPollAndDbConfigUpdate by reducing repeated CAS collisions on the _sync:registry document during high-frequency config polling/heartbeats. It introduces a shared retry helper with randomized jitter and increases the retry budget for UpdateConfig’s registry persist step to better tolerate contention from the periodic cluster-compat heartbeat.

Changes:

• Add bootstrapContext.retry to consolidate CAS retry loops and introduce small randomized backoff between attempts.
• Increase UpdateConfig’s registry persist retry budget to better survive CAS contention with cluster-compat heartbeats.
• Refactor InsertConfig, UpdateConfig, DeleteConfig, and registry node/freeze operations to use the shared retry helper.

[factory-droid]

[P1] UpdateConfig allows nil callback result but will panic

The ConfigManager interface comment says updateCallback can return nil to remove the config, but UpdateConfig immediately uses updatedConfig (e.g., passes it into registry.upsertDatabaseConfig(...) and later dereferences it to set timestamps/write the document). If any caller returns (nil, nil), this will panic. Either enforce updatedConfig != nil with an explicit error, or implement the documented "remove config" behavior (and adjust the comment if nil is not actually supported).

[factory-droid]

The jittered CAS retry refactor should reduce lockstep collisions and matches the flaky test failure mode described. The main correctness issue is that the documented (nil, nil) callback result for UpdateConfig is not handled and can lead to a nil dereference/panic if used.

[Copilot]

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.

Suppressed comments (4)

rest/config_manager.go:322

• Same issue as UpdateConfig: on CAS retry exhaustion, the returned error message ends with a trailing ':' and discards the last CAS-mismatch error, producing an unhelpful error string.

	if err != nil {
		if base.IsCasMismatch(err) {
			return fmt.Errorf("DeleteConfig failed to persist updated registry after %d attempts:", configUpdateMaxRetryAttempts)
		}
		return err

rest/config_manager.go:167

• UpdateConfig now retries up to updateConfigRegistryPersistMaxRetryAttempts (via the jittered sleeper), but the log message still prints the max as configUpdateMaxRetryAttempts. Once attempts exceed 5, logs will show misleading values like attempt 6/5.

	err, _ = base.RetryLoopWithOptions(ctx, "UpdateConfig", func(retryState base.RetryState) (bool, error, any) {
		base.InfofCtx(ctx, base.KeyConfig, "UpdateConfig starting (attempt %d/%d)", retryState.Attempt, configUpdateMaxRetryAttempts)
		// Step 1. Fetch registry and databases - enforces registry/config synchronization

base/util.go:672

• CreateJitterSleeperFunc claims to sleep a random duration up to maxJitter, but mathrand.Intn(maxJitterMs) is exclusive of the upper bound, so the largest sleep is (maxJitterMs-1)ms. This also means the function can never actually sleep for the full maxJitter duration.

// CreateJitterSleeperFunc creates a RetrySleeper that waits a random duration up to maxJitter
// before each retried attempt, up to maxNumAttempts total attempts. Jitter decorrelates independent,
// periodically-firing retriers that would otherwise collide in lockstep.
func CreateJitterSleeperFunc(maxNumAttempts int, maxJitter time.Duration) RetrySleeper {
	maxJitterMs := int(maxJitter.Milliseconds())
	return func(numAttempts int) (bool, int) {
		// RetryLoopWithOptions/RetryLoopCas call the sleeper after an attempt that already ran,
		// so stopping at >= (rather than >) caps the worker at maxNumAttempts total calls.
		if numAttempts >= maxNumAttempts {
			return false, -1
		}
		if maxJitterMs <= 0 {
			return true, 0
		}
		// Floor at 1ms so a known retry never sleeps 0ms, without pushing the ceiling past maxJitter.
		return true, max(mathrand.Intn(maxJitterMs), 1)
	}

rest/config_manager.go:227

• On CAS retry exhaustion, the returned error message ends with a trailing ':' and drops the underlying CAS-mismatch error, which is exactly what surfaced in the flaky test logs (empty reason after the colon). Wrapping the last error makes the 500 easier to diagnose if it ever happens again.

This issue also appears on line 318 of the same file.

	if err != nil {
		if base.IsCasMismatch(err) {
			return 0, fmt.Errorf("UpdateConfig failed to persist updated registry after %d attempts:", updateConfigRegistryPersistMaxRetryAttempts)
		}
		return 0, err