Conversation
The stats logger could stall, leaving gaps in sg_stats.log, whenever a database config update held a lock the stats path also needed. Two independent locks were involved: - _databasesLock: updateCalculatedStats took RLock, but a config update holds the write lock across an infinite index-readiness wait, blocking the reader for the whole (unbounded) window. - base.SgwStats.dbStatsMapMutex: String() held it for the entire marshal, and NewDBStats/ClearDBStats held it for all Prometheus (un)registration under _databasesLock. Decouple the reader from both: - ServerContext now keeps a lock-free atomic snapshot of the database contexts, refreshed under _databasesLock at each _databases mutation. updateCalculatedStats reads the snapshot, never the lock. - dbStatsMapMutex is now held only for the map operation: String() marshals a shallow copy, and NewDBStats/ClearDBStats do their Prometheus work outside the lock. The unregister* calls only touch Prometheus (not the DbStats maps), so this stays race-free. The stats ticker now keeps emitting (stale-but-present) while databases are added, removed or reloaded. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
This PR addresses CBG-5472 by ensuring periodic stats logging cannot stall behind long-held database/config locks, keeping sg_stats.log emission continuous (potentially stale, but present) during database add/remove/reload and index readiness waits.
Changes:
- Added a lock-free atomic snapshot of database contexts in
ServerContext, soupdateCalculatedStatsnever blocks on_databasesLock. - Narrowed
base.SgwStats.dbStatsMapMutexcritical sections:String()marshals a shallow copy, andNewDBStats/ClearDBStatsonly lock for map mutation (Prometheus work outside the lock). - Added unit tests covering lock-independence and concurrent stats serialization/DB registration/removal scenarios.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| rest/server_context.go | Maintains an atomic snapshot of DB contexts and updates calculated stats from the snapshot instead of _databasesLock. |
| rest/server_context_test.go | Adds regression tests ensuring stats calculation doesn’t block on _databasesLock, and remains safe during concurrent removal. |
| base/stats.go | Narrows dbStatsMapMutex usage and performs serialization/Prometheus operations outside the map lock. |
| base/stats_test.go | Adds tests for serialization behavior across Clear/New and concurrent String()/DB registration/removal. |
Comment on lines
+1458
to
1463
| dbStats.unregisterCacheStats() | ||
| dbStats.unregisterCBLReplicationPullStats() | ||
| dbStats.unregisterCBLReplicationPushStats() | ||
| for replName := range dbStats.DbReplicatorStats { | ||
| dbStats.unregisterReplicationStats(replName) | ||
| } |
Comment on lines
+224
to
+226
| func TestStatsSerializationConcurrentWithDBRegistration(t *testing.T) { | ||
| stats := &SgwStats{DbStats: map[string]*DbStats{}} | ||
|
|
Comment on lines
+213
to
+218
| _, err := stats.NewDBStats(dbName, false, false, false, false, nil, nil) | ||
| require.NoError(t, err) | ||
| require.Contains(t, stats.String(), dbName) | ||
|
|
||
| stats.ClearDBStats(dbName) | ||
| require.NotContains(t, stats.String(), dbName) |
Comment on lines
+410
to
+414
| select { | ||
| case <-done: | ||
| case <-time.After(10 * time.Second): | ||
| t.Fatal("updateCalculatedStats blocked on _databasesLock held by a simulated config update (CBG-5472)") | ||
| } |
Guard ClearDBStats's iteration of the lazily-written DbReplicatorStats map with dbReplicatorStatsMutex. Replications register their stats at runtime via DBReplicatorStats() (writing that map under the mutex), which can overlap a database teardown; iterating without the mutex could trigger a fatal "concurrent map read and map write". This race is pre-existing (not introduced by CBG-5472) but is in-theme and cheap to fix here. Adds a regression test that reproduces the fatal map race without the fix. Also address reviewer feedback: - Make TestStatsSerializationConcurrentWithDBRegistration exercise the real Prometheus (un)registration path (the base TestMain forces SkipPrometheusStatsRegistration=true), restoring the flag afterwards. - defer ClearDBStats cleanup in TestClearDBStatsRemovesFromSerialization so an early assertion failure can't leave stats registered. - Shorten the lock-independence regression timeout from 10s to 2s. - Document the SgwStats shallow-copy field-drift hazard in String(). - Document the _databases/databasesSnapshot sync invariant. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
CBG-5472
The stats logger could stall, leaving gaps in sg_stats.log, whenever a database config update held a lock the stats path also needed. Two independent locks were involved:
_databasesLock:updateCalculatedStatstookRLock, but a config update holds the write lock across an infinite index-readiness wait, blocking the reader for the whole (unbounded) window.base.SgwStats.dbStatsMapMutex:String()held it for the entire marshal, andNewDBStats/ClearDBStatsheld it for all Prometheus (un)registration under_databasesLock.Decouple the reader from both:
ServerContextnow keeps a lock-free atomic snapshot of the database contexts, refreshed under_databasesLockat each_databasesmutation.updateCalculatedStatsreads the snapshot, never the lock.dbStatsMapMutexis now held only for the map operation:String()marshals a shallow copy, andNewDBStats/ClearDBStatsdo their Prometheus work outside the lock. The unregister* calls only touch Prometheus (not theDbStatsmaps), so this stays race-free.The stats ticker now keeps emitting (stale-but-present) while databases are added, removed or reloaded.
Pre-review checklist
fmt.Print,log.Print, ...)base.UD(docID),base.MD(dbName))docs/apiDependencies (if applicable)
Integration Tests