You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
A push notification was recorded but never moved health state, so on Redis Enterprise and Redis Cloud a planned handoff still read as a fault. And even once it does move, nothing downstream could act on it: the pipeline's own timeout bounds nothing, so a caller waits out the connection's timeout whether or not a window is open.
What changed
The health window. An announced disruption opens a window and reports through InProgress, which does nothing beyond stopping health reporting from calling the handoff a fault — recovery stays the client's, since probing force-reconnects on a failed write and would fight the handoff it is meant to survive. Windows are tracked per operation family, so a MIGRATED closes the migration it started rather than a failover still in flight; MOVING has no completion and can only lapse. The Azure probe loop and an announced window overlap, so both report through one aggregate transition rather than each emitting its own start and end.
The bounds became settings. They used to be whatever the client defaulted to.
Setting
Default
Notes
MaintenanceRelaxedTimeout
twice the effective async timeout
A connection told to give up quickly stays proportionally patient.
MaintenanceRelaxedWindowMax
left to the client (3x the above)
Set, it caps the window however long the server announced.
FailFastBacklogPolicy
now true
Queuing while disconnected makes a command wait out the connection's timeout with nothing to wait for.
Both maintenance values travel as whole seconds and render with (int)TotalSeconds, so configured or derived they are rounded up and clamped to the 1-600s range that parses back. Without the rounding, a sub-second value would render as 0, and a zero window maximum clamps every window to nothing.
The pipeline timeout now binds. Polly's timeout is cooperative: it arms a token, awaits the callback, and raises TimeoutRejectedException only if the callback itself throws. A client that accepts no cancellation token cannot observe it, so the configured timeout bounded nothing and the retry, breaker and fallback downstream never saw a failure to act on. AbandonOnCancellation opts a pipeline into racing its callback against the token so control returns to the caller. That token carries the request timeout and the caller's own token alike, which is what the name says and why it is not AbandonOnTimeout. Seeded on for reads and left off everywhere else — a write can carry memory the caller reclaims on return, and abandoning one would recycle that buffer while it is still being written. The abandoned call keeps a faulted-continuation so its later failure is observed rather than escalating.
And it widens inside a window. A cap tight enough to fall through to another tier quickly is the wrong cap where the client is deliberately relaxing its own timeouts. DisruptionRequestTimeout replaces it while a disruption is in progress, resolved per operation through Polly's timeout generator rather than read at build time — the pipeline is held for the process, so a window opening later would never reach a fixed value. Left unconfigured it takes what the tier reports it is already relaxing to, so the two numbers cannot drift apart. Neither can shorten the normal timeout.
Review notes
IDisruptionState is the only new abstraction: two members, no vocabulary from any one tier. Without a registration, or without the second timeout, behaviour is unchanged.
IRedisPlannedMaintenance is deliberately untouched — its InProgress is shipped API, so the seam is satisfied by the implementation rather than by widening that interface.
The concurrency lives in the window bookkeeping; that is the part worth the most attention.
Testing
Full suite on both target frameworks, green: 1877 (net8.0) / 1898 (net10.0). Every new behaviour was red-checked by mutating the implementation to the weakest version the test would tolerate and confirming the intended test — and only that test — fails, each against a build verified to compile first.
🔎 Maintainer heads-up: automated triage flagged this PR as potentially material, so it may need a signed CLA in addition to the DCO sign-off.
Strong signals
adds public API surface (PublicAPI.Unshipped.txt in src/UiPath.Caching)
Other signals
large production change (+261 lines under src/)
This is advisory only — the bot does not decide. Please judge against the CLA criteria (material, product-critical, patent-sensitive, corporate contributor, broad commercial use). Note that thresholds can be gamed by splitting PRs, so use your judgement.
If a CLA is needed → add the cla-required label (a contributor comment with signing steps is posted automatically).
If it is not needed → replace needs-cla-review with cla-not-required so later pushes don't re-flag it.
The reason will be displayed to describe this comment to others. Learn more.
Copilot review overview
🟡 Changes recommended
Initialize effective configured maintenance bounds before subscribing to events, and clarify the handshake coverage limitation in the resilience documentation.
Get a fresh assessment by requesting another Copilot review.
…e the timeout bind
The push route was recorded but never moved health state, so on Redis Enterprise and
Redis Cloud the health check still read a planned handoff as a fault. It opens a
maintenance window now, which does nothing beyond stopping health reporting from calling
it a fault; recovery stays the client's, since probing force-reconnects on a failed write
and would fight the handoff it is meant to survive.
The client's own settings bound the window rather than durations chosen here, so the two
agree by construction about how long to stay patient, and it is measured off a monotonic
timestamp the way the client measures its own -- on a wall clock an NTP correction would
end health suppression early, or hold it open past the relaxation it shadows. An
announced duration is clamped to MaintenanceRelaxedTimeout..MaintenanceRelaxedWindowMax,
since windows of a couple of seconds have been observed and a client that trusted one
would stop being patient exactly when it mattered. A completion hands over to
MaintenancePostEventRelaxedDuration rather than closing at once, because the client keeps
treating failures as maintenance for that tail. The bounds are read before the
subscription goes live and again after the connection configurators run, since one of
them can change what the client is handed.
Windows are tracked per operation family, so a MIGRATED closes the migration it started
rather than a failover still in flight, and MOVING -- which has no completion at all --
can only lapse. A delay armed inline watches the deadline: handing that to the pool
leaves a gap in which the deadline passes unobserved, and a lost completion would then
never record its end.
The Azure probe loop and an announced window can overlap, so both report through one
aggregate transition instead of each emitting its own Redis.MaintenanceStarted/Ended.
Otherwise one route's end arrives while the other is still in maintenance, and a single
outage produces two starts.
Those bounds are settings now instead of whatever the client defaults to.
MaintenanceRelaxedTimeout derives twice the effective async timeout when unset, so a
connection told to give up quickly stays proportionally patient, and
MaintenanceRelaxedWindowMax can be capped alongside it. Both travel as whole seconds and
render with (int)TotalSeconds, so configured or derived they are rounded up and clamped
to the range that parses back -- a sub-second value renders as zero, and a zero window
maximum clamps every window to nothing while the settings still read as if relaxation
were on. FailFastBacklogPolicy defaults to true: queuing while disconnected makes a
command wait out the connection's own timeout with nothing to wait for.
None of that reaches a caller while the pipeline's own timeout bounds nothing. Polly's
timeout is cooperative -- it arms a token and awaits the callback, raising
TimeoutRejectedException only if the callback throws -- and a client that accepts no
cancellation token cannot observe it, so the caller waited out the connection's timeout
and the retry, breaker and fallback downstream never saw a failure to act on.
AbandonOnTimeout opts a pipeline into racing its callback against the token, seeded on
for reads and left off elsewhere: a write can carry memory the caller reclaims on return,
and abandoning one would recycle that buffer while it is still being written. The
abandoned call keeps a faulted-continuation so its later failure is observed rather than
escalating.
A cap tight enough to fall through to another tier quickly is the wrong cap inside a
window, where the client is deliberately relaxing its own timeouts.
DisruptionRequestTimeout replaces it for as long as a disruption is in progress, resolved
per operation through Polly's timeout generator rather than read when the pipeline is
built, since the pipeline is held for the process and a window opening later would never
reach a value fixed at build time. The pipeline learns of it through IDisruptionState, a
two-member seam carrying no vocabulary from any one tier; left unconfigured it takes what
that tier reports it is already relaxing to, so the numbers cannot drift apart. Neither
value can shorten the normal timeout.
Signed-off-by: Cosmin Staicu <cosmin.staicu@uipath.com>
GetConfiguration derives MaintenanceRelaxedTimeout from the async timeout,
but both connection paths apply IRedisConnectionConfigurators afterwards, so
a configurator that moves AsyncTimeout left the bound sized against the value
it replaced.
RedisConnectionConfigurators.ApplyAsync now snapshots the relaxed timeout,
runs the configurators, and re-derives through IRedisConfigurationOptionsProvider
.ReapplyDerivedBounds only when the value it handed them came back untouched --
a configurator that set the relaxed timeout itself outranks the derivation, as
does one supplied in the options or the connection string.
Also closes a race in WaitForRefusalAsync: the refusal count rises inside the
throw, before the catch up the stack has reported it, so waiting on the count
alone handed back a refusal whose report had not landed.
Signed-off-by: Cosmin Staicu <cosmin.staicu@uipath.com>
These bounds come from the auxiliary maintenance connection, not from the command multiplexer whose push event opens the window. RedisConnector independently rebuilds its configuration on every ForceReconnect (RedisConnector.cs:425,901-905), so a dynamic configuration provider or configurator can give the new command connection different timeout/window values while this singleton retains its initial auxiliary values; health suppression and SuggestedTimeout then no longer match the client's actual relaxation. Publish/adopt the effective command configuration whenever that multiplexer is built rather than deriving state from the separate subscription connection.
…ved the async timeout
DI supplies an empty configurator enumerable rather than null, so the previous
re-derivation ran on every connection -- including the no-connection-string
path, which deliberately leaves MaintenanceRelaxedTimeout unset rather than
pinning the client's own default against a later change.
ApplyAsync now snapshots AsyncTimeout as well and re-derives only when a
configurator actually changed it. Three tests cover the combinations: unset
when nothing moved, derived when a configurator supplies the async timeout
with no connection string, and the up-front derivation kept when a configurator
runs but leaves the timeout alone.
Also clears the Sonar findings on this PR: extracts ResolveTimeout from
GetBuilder (S3776, cognitive complexity 19), pragmas the NSubstitute ValueTask
arrangement in StartedAsync the way the two other arrangements in the file
already are (CA2012), and returns the recorded snapshots as arrays (CA1859).
Signed-off-by: Cosmin Staicu <cosmin.staicu@uipath.com>
…observable
Three fixes from review.
MaintenanceRelaxedTimeout is a non-nullable TimeSpan with no unset value of
its own, so comparing before and after cannot tell a configurator's own
assignment from the value it replaced: one that sets the timeout to what it
already held reads as untouched. Deriving up front is what destroyed that
signal, so GetConfiguration no longer derives -- it applies only what was
supplied -- and ReapplyDerivedBounds owns the derivation, running after the
configurators through RedisConnectionConfigurators.ApplyAsync. It reads
assignment off the rendered options, which list only what was actually set.
The derivation is new in this branch, so no shipped behaviour moves with it.
SeenRetention read _relaxedWindowMax and _postEventRelaxed without a lock while
AdoptMaintenanceBounds published them under _windowLock, so a push racing
adoption could pair a new max with an old tail and expire an identity that
still guarded an open window. The retention is now computed where the bounds
are published and read as one word.
IResiliencePipeline documented that ExecuteAsync always awaits the callback,
which the read pipeline no longer does. The remarks now state the rule that
holds: abandoning is per pipeline, and a caller handing over borrowed memory
must use one that does not abandon.
Signed-off-by: Cosmin Staicu <cosmin.staicu@uipath.com>
This value does not always replace RequestTimeout: ResolveTimeout deliberately ignores it when it is shorter. Describe it as a widening timeout (or mention the greater-than condition) so the public API contract matches the implementation.
Moving the derivation into ReapplyDerivedBounds left LoadMaintenanceBounds
adopting the client's raw default, so a window arriving between the
subscription going live and TryConnectAsync finishing the configurator pass
was sized by that default -- suggesting 10s on a connection configured to
relax to 1s. It now applies the base derivation before adopting; the
configurator pass still resizes it afterwards.
The test drives the gap through the gated configurator, which parks
TryConnectAsync inside the configurator pass, and is red at 10s without it.
IResiliencePipeline no longer promises defaultValue on timeout: the wrapper
propagates the cancellation, Polly converts it to TimeoutRejectedException,
and defaultValue belongs to the broken-circuit fallback. The remarks now say
only that control may return before the callback completes.
Signed-off-by: Cosmin Staicu <cosmin.staicu@uipath.com>
Method summaries to one line where the name did not already say it, and
multi-line comments cut to the non-obvious why. One comment restating what
the code plainly does is gone.
Signed-off-by: Cosmin Staicu <cosmin.staicu@uipath.com>
AbandonOnTimeout also abandons on ordinary caller cancellation: RaceCancellation waits on the callback token, and Polly links the caller token into that token (even with no timeout strategy, it is the caller token). The public contract currently promises timeout-only behavior, so consumers may enable it without realizing cancellation can return while their callback still uses resources. Either implement a timeout-specific race or document that any cancellation can abandon the callback across the API reference, settings, and changelog.
AbandonOnTimeout races the callback against Polly's token, and that token
carries the caller's own cancellation as well as the timeout -- so cancelling
abandons the callback just as the timeout does. Every statement of the
contract named only the timeout: the option summary, IResiliencePipeline,
settings.md, the CHANGELOG entry and the sample.
Behaviour is unchanged. It matters for the borrowed-memory rule that decides
which pipelines may abandon: a caller who cancels a write would get the buffer
back while Redis is still reading it, which is the same hazard that keeps the
write pipeline from abandoning at its timeout.
Two tests pin it: with the flag on, cancelling after the callback has provably
started returns well inside the callback's own duration; with it off, the same
cancellation waits for the callback to finish.
Signed-off-by: Cosmin Staicu <cosmin.staicu@uipath.com>
The reason will be displayed to describe this comment to others. Learn more.
Copilot review overview
🔵 Needs a closer look
Three moderate issues must be resolved before approval.
Review effort: Balanced Findings: None
Previously missed (1)
In code that hasn't changed since last review
Document sub-second values rounding up to one second
docs/reference/settings.md:59
The setting is implemented by WholeSeconds, which rounds any positive sub-second value up to 1 second (and the added test at tests/UiPath.Caching.Tests/Redis/RedisConnectorTests.cs:131-140 asserts that). This sentence therefore contradicts the shipped behavior and tells users that a value the library normalizes to 1s disables relaxation; document the rounded-up result instead.
The option races the callback against Polly's token, and that token carries
the request timeout and the caller's own token alike -- so the old name
promised less than the setting does. It is also the guard for the
borrowed-memory rule: a write pipeline with it on would hand the caller's
buffer back mid-write on cancellation, not only at the timeout, and the name
is what a consumer reads before deciding.
Never released -- PublicAPI.Unshipped.txt, under [Unreleased], newest tag
v2.0.1 -- so no consumer has seen the old name and there is no compatibility
step to carry. The window for this closes at the next release.
Behaviour is unchanged. The "despite the name" clauses added a commit ago are
gone with it, since the name now says it.
Signed-off-by: Cosmin Staicu <cosmin.staicu@uipath.com>
This says a sub-second value renders as 0, but the implementation applies WholeSeconds, which rounds values up to at least one second (src/UiPath.Caching/Redis/RedisConfigurationOptionsProvider.cs:94-95), and the new test expects 500ms to render as maintRelaxedTimeout=1 (tests/UiPath.Caching.Tests/Redis/RedisConnectorTests.cs:131-141). Update the description so consumers are not told that a sub-second setting disables relaxation.
The command connection can already be live when the service starts, so a
push landing while the bounds load had no handler and was lost. Adoption
resizes an open window, so subscribing first costs nothing.
Also correct settings.md: a sub-second relaxed timeout rounds up to 1s,
it does not render as 0.
Signed-off-by: Cosmin Staicu <cosmin.staicu@uipath.com>
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
cla-not-requiredMaintainer reviewed: no CLA required for this contribution
3 participants
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.
github-actions
Bot
added
the
needs-cla-review
The problem
A push notification was recorded but never moved health state, so on Redis Enterprise and Redis Cloud a planned handoff still read as a fault. And even once it does move, nothing downstream could act on it: the pipeline's own timeout bounds nothing, so a caller waits out the connection's timeout whether or not a window is open.
What changed
The health window. An announced disruption opens a window and reports through
InProgress, which does nothing beyond stopping health reporting from calling the handoff a fault — recovery stays the client's, since probing force-reconnects on a failed write and would fight the handoff it is meant to survive. Windows are tracked per operation family, so aMIGRATEDcloses the migration it started rather than a failover still in flight;MOVINGhas no completion and can only lapse. The Azure probe loop and an announced window overlap, so both report through one aggregate transition rather than each emitting its own start and end.The bounds became settings. They used to be whatever the client defaulted to.
MaintenanceRelaxedTimeoutMaintenanceRelaxedWindowMaxFailFastBacklogPolicytrueBoth maintenance values travel as whole seconds and render with
(int)TotalSeconds, so configured or derived they are rounded up and clamped to the 1-600s range that parses back. Without the rounding, a sub-second value would render as0, and a zero window maximum clamps every window to nothing.The pipeline timeout now binds. Polly's timeout is cooperative: it arms a token, awaits the callback, and raises
TimeoutRejectedExceptiononly if the callback itself throws. A client that accepts no cancellation token cannot observe it, so the configured timeout bounded nothing and the retry, breaker and fallback downstream never saw a failure to act on.AbandonOnCancellationopts a pipeline into racing its callback against the token so control returns to the caller. That token carries the request timeout and the caller's own token alike, which is what the name says and why it is notAbandonOnTimeout. Seeded on for reads and left off everywhere else — a write can carry memory the caller reclaims on return, and abandoning one would recycle that buffer while it is still being written. The abandoned call keeps a faulted-continuation so its later failure is observed rather than escalating.And it widens inside a window. A cap tight enough to fall through to another tier quickly is the wrong cap where the client is deliberately relaxing its own timeouts.
DisruptionRequestTimeoutreplaces it while a disruption is in progress, resolved per operation through Polly's timeout generator rather than read at build time — the pipeline is held for the process, so a window opening later would never reach a fixed value. Left unconfigured it takes what the tier reports it is already relaxing to, so the two numbers cannot drift apart. Neither can shorten the normal timeout.Review notes
IDisruptionStateis the only new abstraction: two members, no vocabulary from any one tier. Without a registration, or without the second timeout, behaviour is unchanged.IRedisPlannedMaintenanceis deliberately untouched — itsInProgressis shipped API, so the seam is satisfied by the implementation rather than by widening that interface.Testing
Full suite on both target frameworks, green: 1877 (net8.0) / 1898 (net10.0). Every new behaviour was red-checked by mutating the implementation to the weakest version the test would tolerate and confirming the intended test — and only that test — fails, each against a build verified to compile first.