CBG-5006: Split ServerContext mutexes #7889

bbrks · 2025-11-19T16:06:24Z

Have locks for each part of ServerContext instead of a shared mutex - avoids R/W deadlocks between HTTP Server list and Databases and reduces scope of each lock.

Integration Tests

GSI=true,xattrs=true https://jenkins.sgwdev.com/job/SyncGatewayIntegration/170/

Copilot

Pull Request Overview

This PR refactors the ServerContext struct to use separate mutexes for different functional areas instead of a single shared mutex. The changes split the previous lock into _databasesLock (for database-related maps) and _httpServersLock (for HTTP server management), reducing lock contention and preventing potential read/write deadlocks.

Key changes:

Replaced single lock with _databasesLock and _httpServersLock
Renamed all database-related maps to use underscore prefix (_databases, _dbConfigs, _dbRegistry, _collectionRegistry)
Updated all lock acquisitions throughout the codebase to use the appropriate specialized lock

Reviewed Changes

Copilot reviewed 8 out of 8 changed files in this pull request and generated 1 comment.

Show a summary per file

File	Description
rest/server_context.go	Core changes to ServerContext struct definition, splitting the single lock into two specialized locks and renaming internal maps with underscore prefixes
rest/utilities_testing.go	Updated test helper functions to use `_databasesLock` instead of generic `lock`
rest/serverless_test.go	Updated test assertions to reference renamed database maps with underscore prefixes
rest/handler_config_database.go	Updated database configuration mutation handler to use `_databasesLock`
rest/config_test.go	Updated test assertions to use renamed `_dbConfigs` map
rest/config.go	Updated all database configuration operations to use `_databasesLock` and renamed map references
rest/api_test.go	Updated direct test access to renamed `_dbConfigs` map
rest/admin_api.go	Updated admin API handlers to use `_databasesLock` and renamed database maps

rest/server_context.go

Copilot

Pull Request Overview

Copilot reviewed 8 out of 8 changed files in this pull request and generated 2 comments.

rest/server_context.go

torcolvin

I wrote a lot of comments about paranoia about grabbing the lock but possibly panicking or not letting go of the lock because I believe the issue that spawned this ticket had a ServerContext.lock.Lock() that was not unlocked but no longer existed in the stack trace that we had.

I think that fixing these issues can be deferred to a separate ticket and some cases were really extra paranoid and probably don't need to be addressed.

torcolvin · 2025-11-20T14:22:14Z

rest/api_test.go

@@ -2990,7 +2989,7 @@
 	resp = rt.CreateDatabase(dbName, dbConfig)
 	RequireStatus(t, resp, http.StatusCreated)



this line should be a lock but actually i'd just change the test so it makes a different REST api call

I don't want to accidentally have the race detector pick up this error because we are doing something naughty in the tests and config polling runs simultaneously.

This test case isn't quite a regular /db/_config operation - it's effectively doing a persistent config load of an old configuration - bypassing the upfront validation that /db/_config would've run.

Either way, I can move it to a function to wrap in a read mutex (the read lock is required to read the dbName entry, but nothing more to update the underlying configuration)

torcolvin · 2025-11-20T14:22:34Z

rest/api_test.go


 	// Attempt to set the `AllowConflicts` property to true in the database configuration.
-	rt.RestTesterServerContext.dbConfigs[dbName].DatabaseConfig.DbConfig.AllowConflicts = base.Ptr(true)
+	rt.RestTesterServerContext._dbConfigs[dbName].DatabaseConfig.DbConfig.AllowConflicts = base.Ptr(true)


See above about potential race and just use /db/_config API

Nah, as above - this test is ensuring coverage of loading an older pre 4.0 config, where AllowConflicts was allowed to be set. I cannot set this value through the REST API anymore (intentionally so)

I have created a named function to do this to make it more obvious why it's being done this way.

torcolvin · 2025-11-20T14:22:44Z

rest/api_test.go


 	// Attempting to disable `enable_star_channel`
-	rt.ServerContext().dbConfigs[dbName].DatabaseConfig.CacheConfig.ChannelCacheConfig.EnableStarChannel = base.Ptr(false)
+	rt.ServerContext()._dbConfigs[dbName].DatabaseConfig.CacheConfig.ChannelCacheConfig.EnableStarChannel = base.Ptr(false)


As above, use REST API

torcolvin · 2025-11-20T14:28:06Z

rest/config.go

 			}
 		}
-		sc.lock.RUnlock()
+		sc._databasesLock.RUnlock()


I want to file a separate ticket for this, if there was a panic between this line and and the readLock, then I want this to unlock.

If the panic happens in config polling, I believe that this will take down Sync Gateway. However, this can get called from any db scoped REST API handler:

GetInactiveDatabase (if serverless) -> FetchAndLoadDatabaseSince -> fetchAndLoadConfigs

Since this is only serverless, this is low priority.

For the issue that spawned this ticket (see https://jira.issues.couchbase.com/browse/CBG-5010 for the full logs), something locked the serverContext.lock mutex but didn't unlock it. I don't think it's this line but this is one of the only cases where this is called outside a defer.

I think it would be fine to do this as part of a separate defensive ticket.

There is already a ticket to review usage of the lock. This ticket is simply splitting out the http server lock.

I tested this by running /_ping and /db/_config in a loop to ensure that the db config update does not interfere with the ping response (which it now does not)

Will review with the other larger sc._databasesLock changes in CBG-5012

torcolvin · 2025-11-20T14:29:23Z

rest/config.go

-	sc.lock.RUnlock()
+	sc._databasesLock.RLock()
+	dbc, ok := sc._databases[dbName]
+	sc._databasesLock.RUnlock()


It is so unlikely that this would panic, but due to excessive paranoia I am almost tempted to wrap this in a function in case sc._databases was nil when this function was called in a case where panics are handled.

Feel free to disregard.

Will review with the other larger sc._databasesLock changes in CBG-5012

torcolvin · 2025-11-20T14:30:30Z

rest/config.go

 	}

-	return sc.dbConfigs, nil
+	return sc._dbConfigs, nil


This function is weird because it returns a value that would require you to hold a lock. This behavior should be noted in the docstring and considered.

I didn't look through the implications of this.

Yeah I agree this is weird and seems wrong - will review with the other larger sc._databasesLock changes in CBG-5012

torcolvin · 2025-11-20T14:31:02Z

rest/config.go

 	return sc.Serve(ctx, config, publicServer, config.API.PublicInterface, CreatePublicHandler(sc))
 }

 func sharedBucketDatabaseCheck(ctx context.Context, sc *ServerContext) (errors error) {


Should we lock this function or rename to _sharedBucketDatabaseCheck and note which lock you are required to hold?

Will review with the other larger sc._databasesLock changes in CBG-5012

torcolvin · 2025-11-20T14:31:23Z

rest/config.go

 }

 // Returns a list of buckets that are being shared by multiple databases.
 func sharedBuckets(dbContextMap map[string][]*db.DatabaseContext) (sharedBuckets []sharedBucket) {


Should we lock this function or rename to _sharedBuckets and note which lock you are required to hold?

Will review with the other larger sc._databasesLock changes in CBG-5012

torcolvin · 2025-11-20T14:34:07Z

rest/server_context.go

-	dbc := sc.databases_[name]
-	sc.lock.RUnlock()
+	sc._databasesLock.RLock()
+	dbc := sc._databases[name]


This could panic if sc._databases is nil, which I don't think would happen but might be worth protecting against?

Will review with the other larger sc._databasesLock changes in CBG-5012

torcolvin · 2025-11-20T14:36:24Z

rest/server_context.go

-			dbc := sc.databases_[name]
+			sc._databasesLock.RLock()
+			defer sc._databasesLock.RUnlock()
+			dbc := sc._databases[name]


Out of scope, do we want to unlock inside this loop?

Will review with the other larger sc._databasesLock changes in CBG-5012

…ig and loading it to simulate upgrade/old valid but now invalid configs

bbrks added 2 commits November 19, 2025 14:17

Rename ServerContext databases lock/fields for clarity about mutex scope

e8ffc70

Move ServerContext serverInfo map to newly scoped httpServers mutex

ce2cd12

Copilot AI review requested due to automatic review settings November 19, 2025 16:06

Copilot AI reviewed Nov 19, 2025

View reviewed changes

rest/server_context.go Show resolved Hide resolved

Rearrange code in ServerContext Close

ca69b40

bbrks requested a review from Copilot November 19, 2025 18:43

Copilot AI reviewed Nov 19, 2025

View reviewed changes

rest/server_context.go Show resolved Hide resolved

rest/server_context.go Show resolved Hide resolved

torcolvin reviewed Nov 20, 2025

View reviewed changes

torcolvin assigned bbrks Nov 20, 2025

Make it obvious that the tests are modifying the stored database conf…

4d6cdd4

…ig and loading it to simulate upgrade/old valid but now invalid configs

torcolvin approved these changes Nov 26, 2025

View reviewed changes

bbrks merged commit 5bb2ae6 into main Nov 26, 2025
42 checks passed

bbrks deleted the CBG-5006 branch November 26, 2025 13:38

bbrks added a commit that referenced this pull request Nov 26, 2025

[4.0.2 Backport] CBG-5007: CBG-5006: Split ServerContext mutexes (#7889)

11e081e

bbrks mentioned this pull request Nov 26, 2025

[4.0.2] CBG-5007: Split ServerContext mutexes #7898

Merged

1 task

		@@ -2990,7 +2989,7 @@
		resp = rt.CreateDatabase(dbName, dbConfig)
		RequireStatus(t, resp, http.StatusCreated)

CBG-5006: Split ServerContext mutexes #7889

CBG-5006: Split ServerContext mutexes #7889

Conversation

bbrks commented Nov 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Uh oh!

Uh oh!

Uh oh!

torcolvin left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bbrks Nov 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bbrks Nov 26, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bbrks Nov 26, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

bbrks commented Nov 19, 2025 •

edited

Loading

bbrks Nov 20, 2025 •

edited

Loading

bbrks Nov 26, 2025 •

edited

Loading

bbrks Nov 26, 2025 •

edited

Loading