NCBC-4298: Make the config streaming backoff actually back off

[davidkelly]

Motivation

HttpStreamingConfigListener tries every management node in turn, and when all of them have failed it waits before starting the round again — with a delay meant to grow to 10 s, explicitly so that a total failure does not produce a failstorm (the comment at HttpStreamingConfigListener.cs:158-160 says exactly that).

That backoff has never worked:

private const int InitialDelayMs = 0;
...
await Task.Delay(delayMs).ConfigureAwait(false);
delayMs = Math.Min(delayMs * 10, MaxDelayMs);   // Math.Min(0 * 10, 10000) == 0, forever

delayMs starts at 0 and zero times ten is zero, so every round waited nothing: while all management endpoints were unreachable the client reattempted as fast as the failures came back, building an HttpClient and throwing and logging an exception per lap, for as long as the outage lasted. The 0 and the * 10 ramp arrived in the same commit (NCBC-2518, 2020), so this has never worked at any point.

Modification

Four changes.

1. InitialDelayMs 0 → 100, which is the ramp the arithmetic was clearly written for: 100 ms, 1 s, 10 s, 10 s. Nothing else about the ramp changes.

This is not what the ticket asked for, and the divergence is deliberate. NCBC-4298 asked for a 0 floor — 0 → 100 ms → 1 s → 10 s — on the grounds that keeping the first retry immediate leaves recovery from a transient blip exactly as fast as it is today, and that raising InitialDelayMs is "a behaviour change nobody asked for". That reasoning doesn't survive contact with the reset: delayMs is reset to InitialDelayMs on every config line received, not just at the start. So under a 0 floor, a node that accepts the connection, delivers one config and drops would be reconnected with no pause at all — the hot loop simply survives for the flapping case instead of the dead-endpoint case. Only a non-zero floor closes it. The ticket has been updated to match.

2. The wait is now given the listener's cancellation token. It had none — harmless at 0 ms, but a real wait without one would leave the background loop alive for up to ten seconds after the bucket was closed.

3. The round now checks that token per node, not only per lap. A shutdown landing mid-round previously walked every node still in the list, building an HttpClient for each and getting an immediate TaskCanceledException from GetAsync, which the inner catch logs at Error as "HTTP Streaming error." — one entry per management node on an ordinary bucket close. That's the same failstorm the backoff exists to prevent, on the shutdown path.

4. Task.Run is no longer handed the token. Copilot's second-pass finding, and correct: that overload only declines to start the delegate, so a Dispose landing between scheduling and dispatch ends the task Canceled for DisposeAsync to rethrow as TaskCanceledException on an ordinary shutdown. Worth noting it is pre-existing, not introduced here — master passed _cancellationTokenSource.Token to the same Task.Run since NCBC-2518 in 2020; the diff only changed which expression supplies it. It is also unreachable today, since nothing in production awaits the task and every test waits for the loop to have started before disposing. Taken anyway: the loop's first act is to check the token, so the overload buys nothing, and this is the last way DisposeAsync throws on a clean shutdown.

Not tested, for the same reason as the ObjectDisposedException below: it needs a Dispose to interleave between scheduling and dispatch, which is not deterministically reproducible.

The token is captured once before the loop and used throughout it: both IsCancellationRequested checks, the streaming request, and the wait, so the loop never touches the source. Dispose cancels the source and then disposes it, and reading Token afterwards throws ObjectDisposedException — which the inner catch logged as "HTTP Streaming error." on an ordinary shutdown. The capture itself can still throw, if a Dispose lands between StartListening's _disposed check and the capture; that is the window master already had, which read Token for the Task.Run overload at the same point on the same thread, so it is neither new nor widened here. It surfaces to the caller, which is what StartListening already documents for a disposed listener.

Tests

Four, none of which waits on a clock: the backoff grows and caps; disposal during a backoff does not wait it out; disposal mid-round does not try the remaining nodes; and the existing "keeps going after failures" test, which reached its third attempt instantly only because of this defect and now skips the wait rather than serving it.

They drive an internal Delay seam (as RetryOrchestrator and StellarRetryHandler already do) rather than a TimeProvider, and that is forced by the defect itself: a zero-length wait completes without ever creating a timer, so a fake clock observes nothing at all — a test built on one would hang rather than fail. Recording the requested duration is the only way to see a wait that never happened.

Three of the four were mutation tested, so no test passes vacuously (the fourth is not deterministically reproducible):

Mutation	Result
InitialDelayMs back to 0	backoff test fails in 112 ms — Expected [100ms, 1s, 10s, 10s], Actual [0, 0, 0, 0]
token not passed to the wait	disposal test fails in 96 ms — "The backoff was given a token which can never be cancelled"
per-node token check removed	mid-round test fails — Expected: 1, Actual: 3

The ObjectDisposedException behaviour is not directly tested: provoking it needs disposal to interleave with setting up a request, which costs more test machinery than a one-line fix is worth.

Behaviour change

Deliberate, and it reaches beyond the outage case, so worth reviewing explicitly: delayMs is also reset to InitialDelayMs after each config line received, so reconnecting after a healthy stream ends now waits 100 ms rather than nothing.

That is what java does, and on purpose — ClusterManagerBucketRefresher converts a normally completed stream into an error so that it goes through the same retry path:

.doOnComplete(() -> {
  // If the stream completes normally we turn it into an exception so it also gets
  // handled in the retryWhen below.
  throw new ConfigException();
})
.retryWhen(Retry.any().exponentialBackoff(Duration.ofMillis(32), Duration.ofMillis(4096)).toReactorRetry())

What this PR deliberately does not do

Pick the shape of the ramp. ×10 reaches the 10 s ceiling after only two failed rounds — attempts land at t=0, 0.1 s, 1.1 s, 11.1 s, so three inside the first four seconds. Java doubles from 32 ms to ~4 s and gets roughly eight in the same window. For a memcached bucket that gap matters more than it looks, because this listener is the only ongoing config source (ConfigHandler.Subscribe starts it eagerly only for MemcachedBucket; couchbase buckets get it only when KV polling fails in a mixed cluster), so after a brief management blip a topology change can go unnoticed for up to 10 s.

The values are left alone because they are the ones already in the file — but since they have never executed in any shipped build, "already in the file" is not evidence they were ever right. Choosing them is a real decision, and a separate one from making the backoff run at all. Happy to fold a change in here if a reviewer would rather settle it now.

Results

Unit suite green on net8.0 and net10.0 (3000 passed).

https://couchbasecloud.atlassian.net/browse/NCBC-4298

🤖 Generated with Claude Code

[github-actions]

FIT performer image

Published:

ghcr.io/couchbase/dotnet-fit-performer:NCBC-4298

Run FIT locally against this PR:

fit run preset <preset-name> --performer dotnet-fit-performer:NCBC-4298

Or run it from the workflow here.

Note

Each push to this PR replaces the image and edits this comment. The image is deleted 7 days after the last push.

[Copilot]

🟡 Changes recommended

Cancellation-token capture still races disposal before the background delegate starts.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

Adds effective exponential backoff to prevent rapid retries when all configuration-streaming endpoints fail.

Changes:

• Starts retry delays at 100 ms and caps them at 10 seconds.
• Makes backoff cancellation-aware and adds deterministic tests.
• Reuses a captured cancellation token throughout the listener loop.

File summaries

File	Description
HttpStreamingConfigListener.cs	Implements cancellable exponential backoff.
HttpStreamingConfigListenerTests.cs	Tests retry progression, continuation, and disposal.

Review details

• Files reviewed: 2/2 changed files
• Comments generated: 1
• Review effort level: Balanced

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[davidkelly]

Checked go as well, since the PR notes the java divergence as an open question. There is no cross-SDK convention to match — the three disagree completely:

	wait after a failed round	growth	cap	after a round that got a config	periodic redial
java	32 ms	×2	~4 s	still backs off — doOnComplete turns a completed stream into a ConfigException on purpose	reader-idle, configIdleRedialTimeout, default 5 m
go	10 s	none — flat	10 s	no wait at all — iterSawConfig skips it	hard 10 s connection lifetime
this PR	100 ms	×10	10 s	100 ms	none (NCBC-4299)

So java is aggressive and exponential, go is a flat ten seconds with no ramp, and these values sit between them with a cap equal to go's entire delay. Any value we picked would disagree with someone, so I have left them as they are — they are the ones already in the file, and the defect was the 0, not the shape.

The one genuine fork is behavioural rather than numeric, and it is the behaviour change called out above: does a round that successfully got a config still pay a delay? Java says yes, go says no, and this PR follows java. Matching go instead would mean waiting only when the round produced nothing, which would restore the previous .NET behaviour exactly while still fixing the failure case. I would leave it — 100 ms on a stream reconnect is noise — but it is a real choice rather than something to inherit by accident.

(gocbcore/basehttpcfgcontroller.go doLoop, defaults at agent.go:148-160.)

[Copilot]

🟡 Changes recommended

Immediate disposal can cancel the scheduled background task and cause DisposeAsync to throw.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

• Files reviewed: 2/2 changed files
• Comments generated: 1
• Review effort level: Balanced

[Copilot]

🟢 Approval recommended

The implementation and tests consistently address the retry failstorm and clean-shutdown cases without unresolved issues.

Review details

• Files reviewed: 2/2 changed files
• Comments generated: 0 new
• Review effort level: Balanced