Skip to content

CBG-5366: set sync info to binary document when cluster compat v4.1+#8291

Merged
bbrks merged 5 commits into
mainfrom
CBG-5366
May 25, 2026
Merged

CBG-5366: set sync info to binary document when cluster compat v4.1+#8291
bbrks merged 5 commits into
mainfrom
CBG-5366

Conversation

@gregns1
Copy link
Copy Markdown
Contributor

@gregns1 gregns1 commented May 22, 2026

CBG-5366

  • Introduces a versioned syncInfo encoding: at cluster compat ≥ 4.1, marshalSyncInfo prepends a SyncInfoTypeV1 discriminator byte; older clusters keep writing bare JSON so a not-yet-upgraded peer can still read the doc. DecodeSyncInfo handles both forms.
  • Plumbs *ClusterCompatVersion through InitSyncInfo / SetSyncInfoMetadataID / SetSyncInfoMetaVersion and their callers (bootstrap, server context, attachment migration, resync DCP) via closure on dbcontext
  • Replaces DataStore.Update/Add in the syncInfo write path with a GetRaw + WriteCas CAS-retry loop. Update/Add route through gocb's RawJSONTranscoder / SGJSONTranscoder, which reject or mis-tag the non-JSON V1 prefix byte; the new path uses syncInfoWriteOpts to pick the datatype tag from the payload (binary for V1, JSON for bare JSON), preserving the older-peer read path. Retry semantics match the existing Update (unbounded loop, retry on IsCasMismatch).
  • Adds a WriteCasCallback hook to LeakyDataStore and migrates TestInitSyncInfoErrors to it, since AddCallback no longer fires on the new write path.

Pre-review checklist

  • Removed debug logging (fmt.Print, log.Print, ...)
  • 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
  • Update Go module dependencies when merged

Integration Tests

Copilot AI review requested due to automatic review settings May 22, 2026 15:06
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR updates Sync Gateway’s _sync:syncInfo document encoding and write path to support a rolling-upgrade safe transition to a binary-prefixed (V1) format once the cluster-wide minimum Sync Gateway version reaches 4.1+. It also threads cluster compatibility version resolution through DatabaseContextOptions so long-lived components observe changes during upgrades.

Changes:

  • Introduces a versioned syncInfo encoding (legacy bare JSON vs. V1 discriminator byte + JSON) with DecodeSyncInfo handling both.
  • Switches syncInfo writes from Update/Add to a GetRaw + WriteCas CAS-retry loop to correctly set KV datatype (JSON vs binary) based on payload.
  • Plumbs cluster compat version resolution into DB context options and key call sites (bootstrap, resync DCP, attachment migration), with tests covering upgrade gating and both encodings.

Reviewed changes

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

Show a summary per file
File Description
rest/server_context.go Passes cluster compat version into syncInfo init and stores a closure on DB context options for dynamic re-resolution during rolling upgrades.
rest/config_manager.go Switches syncInfo read during metadataID computation to raw fetch + DecodeSyncInfo to support V1.
rest/config_manager_test.go Adds coverage for binary/V1 syncInfo decoding in computeMetadataID and corrupt-doc fallback behavior.
rest/cluster_compat_test.go Adds a rolling-upgrade gate test ensuring syncInfo stays legacy with a 4.0 peer and flips to V1 at 4.1+.
db/util_testing.go Updates syncInfo meta version assertion helper to use raw fetch + DecodeSyncInfo.
db/database.go Adds ClusterCompatVersion func() *base.ClusterCompatVersion to DatabaseContextOptions.
db/database_test.go Updates tests for new syncInfo function signatures (passing cluster compat version).
db/background_mgr_resync_dcp.go Passes resolved cluster compat version into syncInfo update after resync completion.
db/background_mgr_resync_dcp_test.go Adds end-to-end test verifying resync writes V1 syncInfo when ccv>=4.1.
db/background_mgr_attachment_migration.go Passes resolved cluster compat version into syncInfo meta version update after migration.
db/background_mgr_attachment_migration_test.go Adds end-to-end test verifying attachment migration writes V1 syncInfo when ccv>=4.1.
base/rosmar_cluster.go Fixes GetDocument exists return semantics and adds GetRawDocument implementation for bootstrap reads.
base/leaky_datastore.go Adds WriteCasCallback injection seam for CAS write path testing.
base/leaky_bucket.go Adds WriteCasCallback config option to support new syncInfo write path testing.
base/constants_syncdocs.go Implements versioned syncInfo encoding/decoding and CAS-based write/update logic using raw bytes + datatype control.
base/constants_syncdocs_test.go Updates error-injection tests for new write path and adds comprehensive tests for V1/legacy encodings and decode behavior.
base/bootstrap.go Extends BootstrapConnection with GetRawDocument and adds Couchbase Server implementation using raw transcoder.
base/bootstrap_test.go Adds tests for GetRawDocument and for correct GetDocument exists behavior across Rosmar/Couchbase implementations.

Comment thread base/constants_syncdocs.go Outdated
Comment on lines +31 to +38
type syncInfoMetaVersion byte

const (
// SyncInfoTypeUnknown is an unused byte value but here for clarity between the zero value
SyncInfoTypeUnknown syncInfoMetaVersion = iota
// SyncInfoTypeV1 is used to denote a sync info document in version 4.1 and later
SyncInfoTypeV1
)
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Changed to syncInfoEncodingType

@gregns1 gregns1 requested a review from bbrks May 22, 2026 15:30
Copy link
Copy Markdown
Member

@bbrks bbrks left a comment

Choose a reason for hiding this comment

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

LGTM - I had some comments but they were all on the cluster compat version gate and we can just follow-up with some refactoring there to tidy up. Filed CBG-5406 to track given the minimal usage in 4.1

Comment thread rest/server_context.go
Comment on lines +837 to +840
var ccv *base.ClusterCompatVersion
if sc.ClusterCompat != nil {
ccv = sc.ClusterCompat.ClusterCompatVersion()
}
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This pattern feels like there should just be a function on ServerContext that returns cluster compat version or nil and is doing all of its own nil checking, etc. inside.

Comment thread db/database.go
ImportVersion uint64 // Version included in import DCP checkpoints, incremented when collections added to db
DisablePublicAllDocs bool // Disable public access to the _all_docs endpoint for this database
StoreLegacyRevTreeData *bool // Whether to store additional data for legacy rev tree support in delta sync and replication backup revs
ClusterCompatVersion func() *base.ClusterCompatVersion // ClusterCompatVersion returns the current cluster-wide minimum SG version, or nil if not yet known.
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

CCV (or the function to get it) is better suited to be stored on DatabaseContext than in the options.

Comment on lines +200 to +204
var ccv *base.ClusterCompatVersion
if a.databaseCtx.Options.ClusterCompatVersion != nil {
ccv = a.databaseCtx.Options.ClusterCompatVersion()
}
if err := base.SetSyncInfoMetaVersion(ctx, dbc.dataStore, MetaVersionValue, ccv); err != nil {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

likewise helper function on DatabaseContext that returns ccv or nil and does its own nil checking will help keep code dependent on CCV cleaner.

if err != nil {
return nil, fmt.Errorf("Error marshalling syncInfo: %v", err)
}
if clusterCompatVersion != nil && clusterCompatVersion.AtLeast(4, 1) {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Could maybe improve the ergonomics of this API by pushing the nil check down into the AtLeast method itself (returning false is fine for that case)

A typed nil won't panic if the method is defined on the pointer and lets us avoid nil checks at usage time.

@bbrks bbrks merged commit 090b174 into main May 25, 2026
48 checks passed
@bbrks bbrks deleted the CBG-5366 branch May 25, 2026 12:23
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