CBG-5061: Sync Function Refactor by RIT3shSapata · Pull Request #8152 · couchbase/sync_gateway

RIT3shSapata · 2026-04-07T13:36:37Z

CBG-5061

Describe your PR here...

Refactored few code paths that were not pre processing the document as other functions.
Deleted the few keys that were added into the document that are not required for sync function

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

Pull request overview

Refactors sync-function invocation preparation in resync-related code paths to reuse prepareSyncFn instead of older revision-body helpers.

Changes:

Updated resync processing to call prepareSyncFn before getChannelsAndAccess.
Updated active-rev sync-function recalculation to use prepareSyncFn instead of getAvailable1xRev + unmarshal.

Reviewed changes

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

File	Description
db/database.go	Switches per-leaf resync preparation to `prepareSyncFn` prior to running sync/channel/access calculation.
db/crud.go	Switches active-rev recalculation body preparation to `prepareSyncFn`.

Copilot · 2026-04-07T13:41:14Z

db/database.go

 	changed := 0
 	doc.History.forEachLeaf(func(rev *RevInfo) {
-		bodyBytes, _, err := db.get1xRevFromDoc(ctx, doc, rev.ID, false)
+		body, metaMap, _, err := db.prepareSyncFn(doc, doc)


prepareSyncFn(doc, doc) inside forEachLeaf always prepares the current document body/rev (newRevID = doc.RevID), but the subsequent getChannelsAndAccess(..., rev.ID) is meant to evaluate each leaf revision. This mismatch means conflicting leaf revisions will all be re-synced using the same body/_rev, producing incorrect channel/access results. Consider loading the body for rev.ID (as the previous code did) or introducing a helper that prepares a sync-fn body for an arbitrary revID so BodyRev matches rev.ID.

Copilot · 2026-04-07T13:41:15Z

db/database.go

-		metaMap, err := doc.GetMetaMap(db.UserXattrKey())
-		if err != nil {
-			return
+			base.WarnfCtx(ctx, "Error preparing sync function for document '%s': %v", docid, err)


If prepareSyncFn returns an error, the callback currently logs but continues into getChannelsAndAccess with potentially nil/invalid body/metaMap, which can lead to incorrect results or runtime errors in the sync function invocation. Return early from the leaf callback when preparation fails.

Suggested change

base.WarnfCtx(ctx, "Error preparing sync function for document '%s': %v", docid, err)

// Skip this leaf when sync function preparation fails so the sync function

// is not invoked with invalid or incomplete inputs.

base.WarnfCtx(ctx, "Error preparing sync function for document %q: %v", base.UD(docid), err)

return

Copilot · 2026-04-07T13:41:15Z

db/crud.go

+	curBody, _, _, err := db.prepareSyncFn(doc, doc)
 	if err != nil {
 		return
 	}


prepareSyncFn unmarshals user xattrs via doc.GetMetaMap(...) and validates the body, but this call site discards the returned metaMap and only needs the current body. This adds extra work and can introduce a new failure path (e.g., invalid rawUserXattr) even though metaMap is already provided to this function. Consider using a lighter-weight helper to get a mutable body (or refactor prepareSyncFn/add a variant) that doesn't re-fetch meta when the caller already has it.

Copilot · 2026-04-07T13:41:15Z

db/database.go

-		metaMap, err := doc.GetMetaMap(db.UserXattrKey())
-		if err != nil {
-			return
+			base.WarnfCtx(ctx, "Error preparing sync function for document '%s': %v", docid, err)


This warning logs the document ID without redaction (docid). Since document IDs are user data, wrap it with base.UD(...) (and preferably use a %q-style format consistent with nearby logs) to avoid leaking sensitive data in logs.

Suggested change

base.WarnfCtx(ctx, "Error preparing sync function for document '%s': %v", docid, err)

base.WarnfCtx(ctx, "Error preparing sync function for document %q: %v", base.UD(docid), err)

RIT3shSapata · 2026-04-08T12:09:13Z

@bbrks I've reverted the use of prepareSyncFn in both the functions for the following reasons:

getResyncedDocument: copilot's comment, made a lot of sense of how by using prepare sync function we're just recomputing the access for the same document content.
recalculateSyncFnForActiveRev: By using prepareSyncFn in here, resulted in lot of failure in tests with tombstones.

So I just decided to remove the keys that are not required for Sync Function after the bodyBytes is fetched.

Copilot

Pull request overview

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

Copilot · 2026-04-08T12:09:43Z

db/database.go

+		// removing the following fields as these fields are not required for sync function
+		if _, ok := body[BodyAttachments]; ok {
+			delete(body, BodyAttachments)
+		}


In this resync path we first call get1xRevFromDoc, which injects _attachments into the JSON bytes when attachments exist, and then immediately unmarshal and delete _attachments. This adds avoidable CPU/memory overhead during resync. Consider fetching the revision body without stamping _attachments (e.g., via getRevision/getAvailableRev and then only injecting _id/_rev/_deleted as needed, or by introducing a helper that builds the sync-function input body without attachments).

Copilot · 2026-04-08T12:09:43Z

db/database.go

+		if _, ok := body[BodyRevisions]; ok {
+			delete(body, BodyRevisions)
+		}


This block removes _revisions before calling the sync function, but get1xRevFromDoc is called with listRevisions=false in this function, so _revisions should not be injected in the first place. If you’re guarding against legacy bodies containing _revisions, please add a brief comment explaining that; otherwise this can be removed to reduce noise and keep behavior consistent with the rest of the code.

Copilot · 2026-04-08T12:09:43Z

db/crud.go

+	// removing _attachments, as attachments are not required to be passed
+	// into sync function
+	if _, ok := curBody[BodyAttachments]; ok {
+		delete(curBody, BodyAttachments)
+	}


Similar to resync: getAvailable1xRev stamps _attachments into the JSON bytes, then this code unmarshals and deletes _attachments before running the sync function. If attachments are intentionally excluded from sync, consider avoiding injecting them earlier (e.g., a variant of getAvailable1xRev that doesn’t include attachments, or a helper to build the sync-function input body).

Copilot · 2026-04-08T12:09:43Z

db/database.go

+		// removing the following fields as these fields are not required for sync function
+		if _, ok := body[BodyAttachments]; ok {
+			delete(body, BodyAttachments)
+		}
+		if _, ok := body[BodyRevisions]; ok {
+			delete(body, BodyRevisions)
+		}


These changes alter the document body passed into the sync function during resync / active-rev recalculation (specifically excluding _attachments, and potentially _revisions). There are existing tests for getResyncedDocument in db/database_test.go; please add coverage asserting the sync function does not see these fields in these code paths to prevent regressions.

Copilot · 2026-04-08T12:09:44Z

db/crud.go

+	// removing _attachments, as attachments are not required to be passed
+	// into sync function
+	if _, ok := curBody[BodyAttachments]; ok {
+		delete(curBody, BodyAttachments)
+	}


Please add/extend tests in db/crud_test.go to cover recalculateSyncFnForActiveRev ensuring the sync function input body does not include _attachments (and documenting/covering whether _revisions should be present or not). This is a behavior change that’s easy to regress.

Suggested change

// removing _attachments, as attachments are not required to be passed

// into sync function

if _, ok := curBody[BodyAttachments]; ok {

delete(curBody, BodyAttachments)

}

// Recalculating channel/access for an older active revision must not expose

// attachment metadata to the sync function. `_attachments` is stripped here

// to keep this path aligned with the body shape expected by sync-function

// evaluation and to make the behavior explicit in this regression-prone area.

//

// Note that `_revisions` is intentionally not removed in this step; whether it

// is present is determined by the revision body returned from storage and any

// downstream body normalization, not by this attachment-specific safeguard.

delete(curBody, BodyAttachments)

Copilot · 2026-04-08T12:09:44Z

db/database.go

 		if err != nil {
 			return
 		}
+


The PR description says resync paths were refactored to use prepareSyncFn instead of getAvailable1xRev/get1xRevFromDoc, but in getResyncedDocument we still build the sync-function body via get1xRevFromDoc (then strip fields). Either update this resync path to use prepareSyncFn (or an equivalent helper) or adjust the PR description to match the actual change.

refactor using preparesync

2cc0cc9

RIT3shSapata requested a review from bbrks April 7, 2026 13:36

RIT3shSapata assigned bbrks Apr 7, 2026

Copilot AI review requested due to automatic review settings April 7, 2026 13:36

Copilot AI reviewed Apr 7, 2026

View reviewed changes

RIT3shSapata assigned RIT3shSapata and unassigned bbrks Apr 7, 2026

RIT3shSapata added 2 commits April 8, 2026 17:25

fix lint and failing tests

a464541

fixes based on pr comments

4cb07ad

RIT3shSapata requested a review from Copilot April 8, 2026 12:05

RIT3shSapata assigned bbrks and unassigned RIT3shSapata Apr 8, 2026

Copilot started reviewing on behalf of RIT3shSapata April 8, 2026 12:05 View session

Copilot AI reviewed Apr 8, 2026

View reviewed changes

lint fix

a6d9692

RIT3shSapata assigned RIT3shSapata and unassigned bbrks Apr 8, 2026

-			base.WarnfCtx(ctx, "Error preparing sync function for document '%s': %v", docid, err)
+			// Skip this leaf when sync function preparation fails so the sync function
+			// is not invoked with invalid or incomplete inputs.
+			base.WarnfCtx(ctx, "Error preparing sync function for document %q: %v", base.UD(docid), err)
+			return

	base.WarnfCtx(ctx, "Error preparing sync function for document '%s': %v", docid, err)
	base.WarnfCtx(ctx, "Error preparing sync function for document %q: %v", base.UD(docid), err)

-	// removing _attachments, as attachments are not required to be passed
-	// into sync function
-	if _, ok := curBody[BodyAttachments]; ok {
-		delete(curBody, BodyAttachments)
-	}
+	// Recalculating channel/access for an older active revision must not expose
+	// attachment metadata to the sync function. `_attachments` is stripped here
+	// to keep this path aligned with the body shape expected by sync-function
+	// evaluation and to make the behavior explicit in this regression-prone area.
+	//
+	// Note that `_revisions` is intentionally not removed in this step; whether it
+	// is present is determined by the revision body returned from storage and any
+	// downstream body normalization, not by this attachment-specific safeguard.
+	delete(curBody, BodyAttachments)

Conversation

RIT3shSapata commented Apr 7, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pre-review checklist

Dependencies (if applicable)

Integration Tests

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Apr 7, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 7, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 7, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 7, 2026

Choose a reason for hiding this comment

Uh oh!

RIT3shSapata commented Apr 8, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI Apr 8, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 8, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 8, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 8, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 8, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 8, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

RIT3shSapata commented Apr 7, 2026 •

edited

Loading