NCBC-4251: Add cancellation/timeout to unbounded semaphore and HTTP waits

[jeffrymorris]

Motivation

Prevents unbounded hangs: these locks/HTTP calls had no way to time out or be cancelled, so one stuck operation (or a disposed cluster) could block callers forever. Proactive hardening, not a bug fix for an observed incident.

Modifications

Cluster.cs: bind _bootstrapLock.WaitAsync to the cluster's own CancellationToken so bootstrapping can't hang past cluster disposal, matching ClusterContext.GetOrCreateBucketLockedAsync.

AttemptContext.cs: bound the rollback concurrency semaphore and the ATR-init mutex with KeyValueTimeout, throwing TimeoutException on expiry. Moves each WaitAsync outside its try/finally so a failed acquire no longer triggers a mismatched Release.

ClusterVersionProvider.cs: plumb a CancellationToken through GetVersionAsync -> DownloadConfigAsync -> HttpClient.GetAsync, linked with the cluster's own lifetime token as a fallback. Adds an optional parameter to the public IClusterVersionProvider interface.

Change-Id: I1708b9ce03a2bf9f4dddc096e9001702465f4efd

[Copilot]

Pull request overview

This PR hardens several async waits and HTTP calls by adding cancellation and/or timeouts to prevent unbounded hangs, especially during cluster disposal or stuck transaction rollback paths.

Changes:

• Plumbs CancellationToken into cluster version HTTP fetches and links caller cancellation with cluster lifetime cancellation.
• Binds cluster bootstrap semaphore acquisition to the cluster lifetime token.
• Adds timeouts to transaction rollback concurrency semaphore and ATR initialization lock to avoid indefinite waits.

Reviewed changes

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

File	Description
src/Couchbase/Core/Version/IClusterVersionProvider.cs	Adds a cancellation token parameter to the public version-provider API.
src/Couchbase/Core/Version/ClusterVersionProvider.cs	Propagates cancellation into HTTP/config download and deserialization.
src/Couchbase/Cluster.cs	Binds bootstrap semaphore wait to cluster lifetime cancellation token.
src/Couchbase/Client/Transactions/AttemptContext.cs	Adds timeout-bounded waits for rollback concurrency and ATR init lock.

Comments suppressed due to low confidence (1)

src/Couchbase/Core/Version/ClusterVersionProvider.cs:83

• CancellationToken is plumbed into DownloadConfigAsync, but OperationCanceledException will be caught by the generic catch below and logged as an error. If cancellation happens on the last server attempt, GetVersionAsync will return null instead of honoring cancellation, which breaks expected cancellation semantics.

                    _logger.LogTrace("Getting cluster version from {server}", server);

                    var config = await DownloadConfigAsync(httpClient, server, cancellationToken).ConfigureAwait(false);
                    if (config != null && config.Nodes != null)
                    {

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated no new comments.

Comments suppressed due to low confidence (3)

tests/Couchbase.UnitTests/Core/Version/ClusterVersionProviderTests.cs:55

• This comment claims cancellation happens after the request is already “in flight”, but the test cancels immediately after starting the task and doesn’t synchronize on DownloadConfigAsync actually being entered. That makes the comment misleading (and it may not exercise the intended code path). Consider rewording the comment, or add a synchronization point (e.g., a TaskCompletionSource signaled from DownloadConfigAsync) before cancelling.

            // Cancel once the request is in flight (hung inside DownloadConfigAsync), not before it starts,
            // so this exercises the per-server catch block rather than the loop's upfront cancellation check.
            callerCts.Cancel();

src/Couchbase/Core/Version/IClusterVersionProvider.cs:27

• Adding a new member to this public interface is a breaking change for any external code which implements IClusterVersionProvider (they’ll fail to compile until they add the new overload). If backward compatibility for implementers is required, consider introducing a new interface (e.g., ICancellableClusterVersionProvider) and registering it alongside IClusterVersionProvider, rather than extending the existing interface.

        /// <param name="cancellationToken">Cancellation token for the underlying HTTP request.</param>
        /// <returns>The <see cref="ClusterVersion"/>, or null if unavailable.</returns>
        ValueTask<ClusterVersion?> GetVersionAsync(CancellationToken cancellationToken);

tests/Couchbase.UnitTests/Core/Version/ClusterVersionProviderTests.cs:37

• CreateProvider allocates a CancellationTokenSource and a ClusterContext (IDisposable) but neither is disposed in the tests. This can leak timers/owned objects across test runs. Consider returning the ClusterContext (or an IDisposable wrapper) so the tests can Dispose it in a using/finally block (and also dispose the CTS when done).

This issue also appears on line 53 of the same file.

            clusterLifetimeCts = new CancellationTokenSource();
            var clusterContext = new ClusterContext(clusterLifetimeCts,
                new ClusterOptions().WithPasswordAuthentication("username", "password"));

[Copilot]

Pull request overview

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

Comments suppressed due to low confidence (3)

tests/Couchbase.UnitTests/Core/Version/ClusterVersionProviderTests.cs:51

• Cancel is issued immediately after starting the task, so the request may not actually be "in flight" yet. Await a signal from HangingClusterVersionProvider before cancelling to make the test deterministic.

            using var callerCts = new CancellationTokenSource();
            var versionTask = provider.GetVersionAsync(callerCts.Token).AsTask();

            // Cancel once the request is in flight (hung inside DownloadConfigAsync), not before it starts,
            // so this exercises the per-server catch block rather than the loop's upfront cancellation check.
            callerCts.Cancel();

tests/Couchbase.UnitTests/Core/Version/ClusterVersionProviderTests.cs:30

• This test intends to cancel while the "download" is in flight, but there is no synchronization ensuring DownloadConfigAsync has started before the token is cancelled. As written, cancellation can happen before the loop begins (ThrowIfCancellationRequested) so the test may pass without exercising the per-server catch/rethrow path it claims to cover.

This issue also appears on line 45 of the same file.

        private class HangingClusterVersionProvider : ClusterVersionProvider
        {
            public HangingClusterVersionProvider(ClusterContext clusterContext, ILogger<ClusterVersionProvider> logger)
                : base(clusterContext, logger)
            {

src/Couchbase/Core/Version/ClusterVersionProvider.cs:50

• GetVersionAsync creates an HttpClient but never disposes it (other usages of ICouchbaseHttpClientFactory generally use using). Also, the PR description mentions linking the caller token with the cluster lifetime token as a fallback, but only the caller token is currently used.

            var httpClient = _clusterContext.ServiceProvider.GetRequiredService<ICouchbaseHttpClientFactory>().Create();

            version = await GetVersionAsync(_clusterContext.Nodes.Select(p => p.ManagementUri).Distinct(), httpClient, cancellationToken)
                .ConfigureAwait(false);