[torcolvin]

CBG-5909

Stacked on #8834 (CBG-5731). Merge that first; this PR's base is CBG-5731.

I wrote a config_concurrency_test.go to flush out the missing changes.

Makes database config updates safe against concurrent registry writers. Each section shows the operations that break, then the fix.

1. UpdateConfig reloads the database again on each retry

1. node1 POST /db/_config reads the registry.
2. node1 runs the update callback, and the callback reloads the database.
3. node2 writes a heartbeat to the registry.
4. node1 tries to write the registry, and the write fails because the CAS changed.
5. node1 retries, runs the callback again, and reloads the database again.
6. Steps 3 to 5 repeat until node1 runs out of retries and returns a 500 error.

Fix: On a retry, node1 reuses the result of the earlier callback if the config's cfgCas did not change, so only the registry write is retried.

2. Config read waits and rolls back after a concurrent delete

1. node1 POST /db/_config reads the registry and sees version v1.
2. node2 DELETE /db/ removes the database from the registry and deletes its config.
3. node1 reads the config for v1 and gets "not found".
4. node1 keeps retrying until configRetryTimeout ends, then rolls back the registry.

Fix: When the config is missing or has the wrong version, node1 reads the registry again, and if v1 is gone it returns ErrConfigRegistryReloadRequired right away.

3. Config read fails when the config is newer than the registry

1. node1 POST /db/_config reads the registry and sees version v1.
2. node2 POST /db/_config writes v2 to the registry and to the config.
3. node1 reads the config and gets v2, which is newer than v1.
4. node1 fails with ErrConfigVersionMismatch.

Fix: getDatabaseConfig changes ErrConfigVersionMismatch to ErrConfigRegistryReloadRequired, so node1 reads the registry again and continues with v2.

4. handlePutDbConfig sets cfgCas on a database that was removed or reloaded

1. node1 PUT /db/_config writes the config and reloads the database.
2. node2 DELETE /db/ deletes the database.
3. node1's config poller removes the database from _dbConfigs.
4. The node1 handler sets _dbConfigs["db"].cfgCas, gets a nil entry, and panics.

In a second version of this problem, node2 sends POST /db/_config at step 2 and the poller on node1 loads the newer version at step 3. At step 4, node1 then writes its old CAS onto the newer config.

Fix: The handler sets cfgCas only if the database is still in _dbConfigs with the same version that the handler wrote.

5. Test hook

RestTesterConfig.ConnectToBucketFn lets tests hook database reloads. RestTesterCluster passes it to every node. It cannot be used together with LeakyBucketConfig.

6. Test requests fail when a concurrent delete removes the database

1. Test goroutine A sends DELETE /db/.
2. Test goroutine B calls rt.SendAdminRequest with /db/_config.
3. templateResource looks up the database to fill in the template, and the lookup fails or races with the delete.
4. Goroutine B's test fails before it sends the request.

Fix: templateResource returns the path unchanged when it contains no {{, so it does not look up the database.

7. Tests

• TestUpdateDbConfigWithConcurrentPeerHeartbeats covers case 1: a heartbeat runs during every reload, and the update must succeed with one reload.
• TestGetDatabaseConfigDeletedAfterRegistryRead covers case 2, and TestGetDatabaseConfigNewerThanRegistry covers case 3.
• TestConcurrentDbConfigOperationsAcrossNodes sends random config operations to a 3-node cluster, then checks that every node and the registry end up with the same config.

TestConcurrentDbConfigOperationsAcrossNodes depends on #8834: without it, a concurrent DELETE /db/ returns 500 in about 1 of 5 runs. With #8834, it passed 45/45 runs with -race.

Pre-review checklist

• Logging sensitive data? Make sure it's tagged (e.g. base.UD(docID), base.MD(dbName))
• Updated relevant information in the API specifications (such as endpoint descriptions, schemas, ...) in docs/api

Dependencies (if applicable)

• Link upstream PRs: CBG-5731 Finish a database delete when another node already deleted it #8834

Integration Tests

• https://jenkins.sgwdev.com/job/SyncGatewayIntegration-Pipeline/0000/

🤖 Generated with Claude Code

[Copilot]

Copilot review overview

🟢 Approval recommended

The concurrency paths are consistently guarded and covered by focused and cluster-level tests.

Review effort: Balanced
Findings: None

What changed in this PR

Improves persistent database configuration safety during concurrent registry writes and multi-node operations.

Changes:

• Reuses update callbacks across registry-only CAS retries.
• Detects stale registry versions and safely reloads.
• Adds concurrency hooks, safeguards, and multi-node tests.

File	Description
rest/​config_manager.go	Handles concurrent registry changes safely.
rest/​admin_api.go	Safely updates loaded config CAS.
rest/​utilities_testing.go	Adds bucket connection hook and template shortcut.
rest/​utilities_testing_rest_tester_cluster.go	Propagates connection hooks across nodes.
rest/​config_manager_test.go	Tests stale registry handling.
rest/​config_concurrency_test.go	Exercises randomized multi-node operations.
rest/​cluster_compat_test.go	Tests heartbeat contention during updates.

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[factory-droid]

Droid finished @torcolvin's task —— View job

---

Phase 2 validation complete. No inline comments to post. LGTM: the concurrency fixes match the PR description and the new tests cover the reported edge cases.

[bbrks]

All of my comments are on the new random concurrency test which needs to be skipped anyway until a follow-up fix is made to avoid the use of closed bucket panic.

Can roll these comments forwards into the PR to re-enable the test?

[bbrks]

This endpoint uses a full keyspace not db (sync functions are collection specific)

[bbrks]

should this be using a placeholder instead of db?

[bbrks]

I don't think I'd expect this to be used for another test, so having standalone builder functions like this (Claude tends to gravitate towards this?) means we're just polluting the rest package APIs really

[bbrks]

inline struct if it's test specific

[bbrks]

It would be nice if this randomness was an injectable seed so it could be deterministically re-run. It's not too easy to grab shuffle value when -shuffle=on is used - but it is possible (via TestMain) which might be nice?

// testing keeps the seed it picks for -shuffle=on to itself. Choose our own value here instead so tests can read and a run can be repeated with -shuffle=N.
flag.Parse()
if f := flag.Lookup("test.shuffle"); f != nil && f.Value.String() == "on" {
		if err := flag.Set("test.shuffle", strconv.FormatInt(time.Now().UnixNano(), 10)); err != nil {
				FatalfCtx(ctx, "could not set test.shuffle: %v", err)
		}
}