CBG-2234 rationalize close behavior

[torcolvin]

CBG-2234 rationalize close behavior in preparation for abstracted DCP client for DCP

Having GoCBDCPClient.Close return an error if it was already closed, but no error if it wasn't already closed was very confusing. This meant the initial error from Close were logged, but then you'd have to wait again for another error. This pattern will not work well with another implementation, so remove this separately.

• DCPClient.Close is an asynchronous function, since it can be called from the callback function, and would block. e.g. if ProcessFeedEvent calls Close then it could deadlock, so it was
• Change Start to not return a doneChan if there is an error. This also means that DCPClient.Close is not required to be called when Start fails.
• Always wait for doneChan in tests

This is prep for CBG-5013 to make communication with a dcp client easier to understand.

1. You can call Close any number of times.
2. Finishing is indicated by doneChannel from Start. It will return an err (or nil) a single time, and then the channel is closed. This means one caller can find the error.

For this code, consider if this cleanup will negatively affect unrelated backports. Not all of this code is well tested since it is primarily around error handling, but it also could have been buggy before!.

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

Integration Tests

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

[Copilot]

Pull request overview

This PR refactors the DCP client's close behavior to make it more intuitive and consistent across implementations. Previously, GoCBDCPClient.Close returned an error if already closed but no error otherwise, which was confusing. The changes standardize the close pattern: Close() is now a void, asynchronous function that can be called multiple times, while completion is signaled through the doneChan returned by Start().

Key changes:

• DCPClient.Close() no longer returns an error and can be safely called multiple times
• Start() returns nil for doneChan when it fails, eliminating the need to call Close() on startup failures
• All test code now properly waits for doneChan to ensure clean shutdown

Reviewed changes

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

Show a summary per file

File	Description
base/gocb_dcp_client.go	Changed Close() signature to be void and updated Start() to handle cleanup internally on failure
base/gocb_dcp_feed.go	Simplified DCP feed shutdown logic to use new Close() behavior and removed redundant error handling
base/dcp_client_test.go	Updated all tests to wait for doneChan and removed Close() error checks
db/attachment_compaction.go	Refactored mark, sweep, and cleanup phases to use simplified close pattern
db/attachment_compaction_test.go	Replaced custom wait logic with RequireBackgroundManagerState helper
db/background_mgr_attachment_migration.go	Updated to use new close pattern with proper doneChan handling
db/background_mgr_resync_dcp.go	Simplified termination logic to ignore close errors when background manager terminates
db/util_testing.go	Updated purge DCP feed to properly close and wait for doneChan on timeout

Comments suppressed due to low confidence (1)

db/attachment_compaction.go:1

• Extra space before the period in the log message. Should be "unexpectedly." instead of "unexpectedly .".

// Copyright 2022-Present Couchbase, Inc.

[Copilot]

The log format uses %s but dcpCloseError is an error type. This should use %v instead to properly format the error, or include the missing feedName and MD(bucketName) parameters to match the original logging pattern.

Suggested change

	InfofCtx(ctx, KeyDCP, "DCP Feed %q closed unexpectedly with %s", dcpCloseError)
	InfofCtx(ctx, KeyDCP, "DCP Feed %q for bucket %q closed unexpectedly with %v", feedName, MD(bucketName), dcpCloseError)

[torcolvin]

I dropped bucket from this because it is included with ctx. I don't think it matters whether error is logged with %s or %v?

[Copilot]

The error returned from doneChan is being logged but the function still returns markProcessFailureErr at line 176. If both err and markProcessFailureErr are non-nil, the logged error differs from the returned error, which could be confusing during debugging. Consider whether err should take precedence or be combined with markProcessFailureErr.

[torcolvin]

This is intentional here.