NCBC-4280: Keep a usable node when one KV connection fails to connect - #162
Open
davidkelly wants to merge 1 commit into
Open
NCBC-4280: Keep a usable node when one KV connection fails to connect#162davidkelly wants to merge 1 commit into
davidkelly wants to merge 1 commit into
Conversation
davidkelly
force-pushed
the
dk/pe-ramp-bootstrap
branch
from
August 12, 2026 16:07
d7f31e7 to
0ecbf89
Compare
davidkelly
added a commit
that referenced
this pull request
Aug 13, 2026
Review of PR #162 raised five points plus nits; all are addressed here. Cover the pool that is actually in use. The partial-pool fix landed in both ChannelConnectionPool and DataFlowConnectionPool, but the tests only covered the latter, while ChannelConnectionPool is the pool in play whenever Experiments.ChannelConnectionPools is true -- which it is by default. The two tests are ported into ChannelConnectionPoolTests, and both were confirmed to fail against the previous behaviour. Stop warning on every retry. The bootstrapper reattempts every BootstrapPollInterval, 2.5s by default, for as long as the subject is not bootstrapped, so raising Cluster.BootStrapAsync's catch-all to a warning emitted one warning and one stack trace every 2.5 seconds for the duration -- around 30 of them across the degraded window this change exists to survive, and forever for a 6.5-or-earlier cluster, which never bootstraps at the cluster level by design. The two fixes compounded: before this branch the unconditional Clear() stopped the retry loop after a single attempt, so the old Debug line was emitted once. Entering the failed state is now the warning and subsequent failures go to Debug until it bootstraps. The connect-time warning also no longer sends pre-6.6 users to WaitUntilReadyAsync, which will not help them; it names opening a bucket. Remove stale failures by identity rather than position. Taking a fixed count off the front of DeferredExceptions assumes the subject only appends during an attempt. It does not: Cluster.BootStrapAsync and CouchbaseBucket both clear the list on success. Should a clear land between the count being taken and the removal, the removal takes this attempt's failures instead of the previous attempt's -- reinstating exactly the defect being fixed. Current call ordering makes that unreachable, so this is not a live bug, but the invariant was implicit and unenforced. Removing the snapshotted exceptions by reference is order-independent and costs one small array per attempt. Make the new bootstrapper test deterministic. It asserted on DeferredExceptions while the loop was still running, and the attempt counter it waited on is incremented before the failure is recorded, so the wait could be released with the list mid-update and Assert.Single could fail. The subject now blocks on entry to the second attempt, which both proves the first attempt was processed to completion and freezes the state under assertion. Reject an empty pool in ChannelConnectionPool. AddConnectionsAsync drops a connection that comes back dead without throwing, so a pool whose connections all come back dead reaches the end of initialization empty with no exception for the partial-pool filter to consider, and the node is kept. Operations then queue against a channel with no processor reading it until the scale controller's next poll up to 30 seconds later. It now throws, which is what the caller already does with a pool whose connections threw. DataFlowConnectionPool is deliberately left alone here: it tolerates an empty pool and recovers in SendAsync via CleanupDeadConnectionsAsync, which SendAsync_DeadConnection_ReplacesConnectionAndStillSends covers. The asymmetry is noted in both files. Two smaller items. ConnectionFactory now null-checks the redactor like its other dependencies -- previously the only dereferences were in the TLS branch, and the new address log put one on the universal path. And the comment explaining why a sibling connection fails no longer attributes it to unequal reachability across a host's addresses: the private-endpoint hostname resolved to a single address, so the failure is per-flow loss to one address, not a choice between several. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
This PR addresses cold-start bootstrap failures in lossy-network windows (notably AWS PrivateLink) by ensuring nodes aren’t discarded when at least one KV connection successfully initializes, and by improving bootstrap failure retention and diagnostics so failures are visible and retries continue appropriately.
Changes:
- Allow connection pools to complete initialization with a partial pool when at least one connection succeeds; correct initialization-size logging.
- Fix bootstrap retry behavior by preserving deferred bootstrap failures across attempts and adjusting cluster bootstrap logging to be actionable without becoming noisy.
- Add unit tests covering partial-pool behavior (both pool implementations) and bootstrap deferred-failure retention.
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 |
|---|---|
| tests/Couchbase.UnitTests/Core/IO/Connections/DataFlow/DataFlowConnectionPoolTests.cs | Adds tests ensuring DataFlow pool tolerates partial success but throws on total failure. |
| tests/Couchbase.UnitTests/Core/IO/Connections/Channels/ChannelConnectionPoolTests.cs | Adds tests ensuring Channel pool tolerates partial success and rejects empty/dead-only pools. |
| tests/Couchbase.UnitTests/Core/BootstrapperTests.cs | Adds coverage to ensure deferred bootstrap failures are not cleared and retries continue. |
| src/Couchbase/Core/IO/Connections/DataFlow/DataFlowConnectionPool.cs | Swallows failures during initial connection creation when Size > 0; logs partial/actual pool size. |
| src/Couchbase/Core/IO/Connections/ConnectionFactory.cs | Adds redactor null-check; logs resolved address prior to connecting for better diagnostics. |
| src/Couchbase/Core/IO/Connections/Channels/ChannelConnectionPool.cs | Swallows failures during initial connection creation when Size > 0; rejects empty pool; logs partial/actual pool size. |
| src/Couchbase/Core/Bootstrapping/Bootstrapper.cs | Preserves deferred failures recorded during a bootstrap attempt; only removes pre-existing failures after the attempt. |
| src/Couchbase/Cluster.cs | Adds bootstrap failure state tracking for warning noise control; warns on ConnectAsync returning unbootstrapped cluster; improves deferred bootstrap logging. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Motivation ========== The Capella private-endpoint situational suite failed its scored ramp window with tens of thousands of errors per scenario, every one of them "Cluster has not yet bootstrapped". For a window after an AWS PrivateLink endpoint reports available, its data path is lossy rather than down. Two pool connections open to the same address; one completes TCP, TLS and Helo while its sibling never gets a SYN through and dies at KvConnectTimeout. AddConnectionsAsync keeps each connection that succeeds, but the Task.WhenAll it ends with rethrows if any failed, so InitializeAsync threw and the caller discarded the whole node along with its authenticated, working connection. With no nodes IsBootstrapped stayed false and every operation in the 28-second ramp failed. Two further defects hid the cause. Bootstrapper.Execute cleared DeferredExceptions unconditionally after BootStrapAsync returned, but Cluster.BootStrapAsync records most failures there rather than throwing, so the clear erased the failure that had just happened and, because IsBootstrapped derives from that list, both stopped the retry loop and left the reported exception carrying no inner exceptions. That same method also described any failure as ignorable on 6.5 or earlier, which excuses a global-config failure and nothing else. Modification ============ Tolerate a partial pool when at least one connection came up, and let the scale controller backfill the rest as it already does when a connection drops. An empty pool is still a failure. Both pools are fixed, though ChannelConnectionPool is the one in use because Experiments.ChannelConnectionPools defaults to true. That pool also now rejects a pool left empty by connections returning IsDead, which AddConnectionsAsync drops without throwing; DataFlowConnectionPool tolerates the same case deliberately and recovers in SendAsync. This puts .NET on the behaviour Java and Go already had. Both treat one connected connection as a usable node, and both default to one KV connection per node where .NET uses two, which is why .NET was the only one of the three exposed to a sibling connection failing at all. Bootstrapper now discards only the failures present before an attempt, removing them by identity because subjects clear the list themselves on success. Cluster.BootStrapAsync warns on entering the failed state and drops to Debug while it persists, rather than warning on every 2.5s poll, and ConnectAsync warns when it returns an unbootstrapped cluster. The address a KV connection is made to is now logged at Debug; on .NET Core and later it was never recorded anywhere, and that gap is what made the original analysis inconclusive. Results ======= op-capella-pe-sit-lite against server 8.0 passed 5 of 5 scenarios with zero failures and zero errors, against a baseline of 3 of 5. An earlier run of the same suite hit the partial-pool path in every scenario, 12 times in total, each pool backfilling to full within about 30 seconds once the degraded window passed. op-onprem-func-release on 8.0 and 7.6 exercised the new paths about 1270 times per leg without either warning ever firing, so healthy clusters are unaffected. Each fix has a unit test confirmed to fail against the previous behaviour.
davidkelly
force-pushed
the
dk/pe-ramp-bootstrap
branch
from
August 14, 2026 17:04
cdeee43 to
3036a66
Compare
davidkelly
marked this pull request as ready for review
August 14, 2026 17:08
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Motivation
The Capella private-endpoint situational suite failed its scored ramp
window with tens of thousands of errors per scenario, every one of them
"Cluster has not yet bootstrapped".
For a window after an AWS PrivateLink endpoint reports available, its
data path is lossy rather than down. Two pool connections open to the
same address; one completes TCP, TLS and Helo while its sibling never
gets a SYN through and dies at KvConnectTimeout. AddConnectionsAsync
keeps each connection that succeeds, but the Task.WhenAll it ends with
rethrows if any failed, so InitializeAsync threw and the caller
discarded the whole node along with its authenticated, working
connection. With no nodes IsBootstrapped stayed false and every
operation in the 28-second ramp failed.
Two further defects hid the cause. Bootstrapper.Execute cleared
DeferredExceptions unconditionally after BootStrapAsync returned, but
Cluster.BootStrapAsync records most failures there rather than throwing,
so the clear erased the failure that had just happened and, because
IsBootstrapped derives from that list, both stopped the retry loop and
left the reported exception carrying no inner exceptions. That same
method also described any failure as ignorable on 6.5 or earlier, which
excuses a global-config failure and nothing else.
Modification
Tolerate a partial pool when at least one connection came up, and let
the scale controller backfill the rest as it already does when a
connection drops. An empty pool is still a failure. Both pools are
fixed, though ChannelConnectionPool is the one in use because
Experiments.ChannelConnectionPools defaults to true. That pool also now
rejects a pool left empty by connections returning IsDead, which
AddConnectionsAsync drops without throwing; DataFlowConnectionPool
tolerates the same case deliberately and recovers in SendAsync.
This puts .NET on the behaviour Java and Go already had. Both treat one
connected connection as a usable node, and both default to one KV
connection per node where .NET uses two, which is why .NET was the only
one of the three exposed to a sibling connection failing at all.
Bootstrapper now discards only the failures present before an attempt,
removing them by identity because subjects clear the list themselves on
success. Cluster.BootStrapAsync warns on entering the failed state and
drops to Debug while it persists, rather than warning on every 2.5s
poll, and ConnectAsync warns when it returns an unbootstrapped cluster.
The address a KV connection is made to is now logged at Debug; on .NET
Core and later it was never recorded anywhere, and that gap is what made
the original analysis inconclusive.
Results
op-capella-pe-sit-lite against server 8.0 passed 5 of 5 scenarios with
zero failures and zero errors, against a baseline of 3 of 5. An earlier
run of the same suite hit the partial-pool path in every scenario, 12
times in total, each pool backfilling to full within about 30 seconds
once the degraded window passed.
op-onprem-func-release on 8.0 and 7.6 exercised the new paths about 1270
times per leg without either warning ever firing, so healthy clusters
are unaffected.
Each fix has a unit test confirmed to fail against the previous
behaviour.