Skip to content

CBG-5950 guard DCP client metadata with per-vbucket locks - #8853

Merged
gregns1 merged 3 commits into
mainfrom
CBG-5950
Oct 6, 2026
Merged

gregns1 merged 3 commits into
mainfrom
CBG-5950

Conversation

@torcolvin

Copy link
Copy Markdown
Collaborator

A vbucket's DCP metadata is written by the owning DCP worker (SetSnapshot, UpdateSeq, Persist), but also off the worker: the OpenStream callback calls SetFailoverEntries, openStream calls Rollback and GetMeta, and GetMetadata reads every vbucket. Nothing synchronized these, so the race detector flags SetFailoverEntries and UpdateSeq against Persist. CBG-1866 fixed the failover case by routing it through the worker, and CBG-3247 undid that to keep the callback from blocking on the worker channel.

Lock each vbucket in every accessor. Persist copies each vbucket under its lock and writes to the bucket outside it, and the constructors share newDCPMetadataBase.

Why mutexes are fine here:

  • The locks are per vbucket, so workers never contend with each other. A vbucket's lock is only contended when a stream opens, rolls back or GetMetadata runs, which is rare next to mutations.
  • Uncontended, the lock adds about 10ns to UpdateSeq (2.5ns to 12.5ns), which is small next to the mutation callback each event already runs. 32 goroutines across 1024 vbuckets average 5ns per call.
  • No lock is held across I/O or a channel send, so the OpenStream callback still never waits on a worker queue, which keeps the goal of CBG-3247.

Add tests that reproduce both races; they fail under -race without the locks.

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

Integration Tests

A vbucket's DCP metadata is written by the owning DCP worker
(SetSnapshot, UpdateSeq, Persist), but also off the worker: the
OpenStream callback calls SetFailoverEntries, openStream calls Rollback
and GetMeta, and GetMetadata reads every vbucket. Nothing synchronized
these, so the race detector flags SetFailoverEntries and UpdateSeq
against Persist. CBG-1866 fixed the failover case by routing it through
the worker, and CBG-3247 undid that to keep the callback from blocking
on the worker channel.

Lock each vbucket in every accessor. Persist copies each vbucket under
its lock and writes to the bucket outside it, and the constructors share
newDCPMetadataBase.

Why mutexes are fine here:
- The locks are per vbucket, so workers never contend with each other.
  A vbucket's lock is only contended when a stream opens, rolls back or
  GetMetadata runs, which is rare next to mutations.
- Uncontended, the lock adds about 10ns to UpdateSeq (2.5ns to 12.5ns),
  which is small next to the mutation callback each event already runs.
  32 goroutines across 1024 vbuckets average 5ns per call.
- No lock is held across I/O or a channel send, so the OpenStream
  callback still never waits on a worker queue, which keeps the goal of
  CBG-3247.

Add tests that reproduce both races; they fail under -race without the
locks.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
@torcolvin
torcolvin requested review from a team and a balanced review from Copilot October 5, 2026 19:06
@factory-droid

factory-droid Bot commented Oct 5, 2026 •

Copy link
Copy Markdown
Contributor

Droid finished @torcolvin's task —— View job


LGTM — I did not find any high-confidence, actionable issues in the locking changes or the added race-repro tests.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🟡 Changes recommended

Rollback holds its vbucket mutex during potentially blocking trace logging, which can stall worker operations.

Review effort: Balanced
Findings: 1 Medium severity

Open (1)
What changed in this PR

Protects Sync Gateway’s DCP metadata from concurrent access using per-vbucket locks.

Changes:

  • Synchronizes metadata access and shares constructor initialization.
  • Copies metadata under locks before persistence.
  • Adds concurrency regression tests.
File Description
base/​dcp_client_metadata.go Adds per-vbucket locking and synchronized persistence snapshots.
base/​dcp_client_metadata_test.go Tests concurrent persistence, failover updates, and rollback.

💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread base/dcp_client_metadata.go Outdated
torcolvin and others added 2 commits October 5, 2026 16:06
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
@gregns1
gregns1 merged commit 8b6488a into main Oct 6, 2026
48 checks passed
@gregns1
gregns1 deleted the CBG-5950 branch October 6, 2026 10:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants