Skip to content

feat!: temporary retry-delay bounds and SSE retry: field handling - #126

Open
tanderson-ld wants to merge 4 commits into
mainfrom
ta/SDK-2791/retry-conformance
Open

feat!: temporary retry-delay bounds and SSE retry: field handling#126
tanderson-ld wants to merge 4 commits into
mainfrom
ta/SDK-2791/retry-conformance

Conversation

@tanderson-ld

@tanderson-ldtanderson-ld commented Aug 31, 2026

Copy link
Copy Markdown
Contributor

Summary

Gives consumers runtime control over reconnection timing, applies the SSE retry: field to the backoff instead of discarding it, and fixes several defects in the backoff path.

New API — breaking, since IEventSource gains members. Implementers must add them; callers of EventSource are unaffected.

voidSetTemporaryRetryDelayBounds(TimeSpaninitialDelay,TimeSpanmaxDelay)voidClearTemporaryRetryDelayBounds()

Temporary bounds revert automatically once a connection stays open for BackoffResetThreshold, or explicitly via Clear. Reapplying identical bounds is idempotent, so a caller may set them on every failure without pinning the delay at the minimum.

retry: field. A server-directed value replaces the minimum delay, is clamped to 1 hour, resets the backoff formula input so the next attempt uses the directed value directly, and is sticky until the server sends another. Ceilings and the active regime are never affected.

Note the 1-hour cap is unobservable at default configuration: the configured MaxRetryDelay of 30s bounds the delay first, so a directed value above 30s is reduced to it. This matches Java, whose DefaultRetryDelayStrategy applies the same clamp with the same 30s default. Go honors larger values only because it ships no default ceiling at all.

Fixes

  • The delay computation raised 2 to an unbounded exponent in floating point and clamped the result, throwing OverflowException past exponent 21 when the maximum exceeded int.MaxValue ms. That exception escaped the reconnect loop, stopping the stream permanently. Now integer-only, comparing against the ceiling before shifting.
  • A pending backoff wait could not be interrupted by Close(). Previously bounded by MaxRetryDelay (30s default); this change makes an hour reachable, so it matters more. RETRY §1.10.1.
  • InitialRetryDelay(TimeSpan.Zero) disabled backoff outright, because the parsed retry: value doubled as an on/off gate. That gate is gone.
  • A computed delay above Task.Delay's platform ceiling threw out of StartAsync, stopping the stream permanently. Clamped at int.MaxValue ms, the lower of the two target frameworks' ceilings.
  • A large retry: value threw OverflowException from TimeSpan.FromMilliseconds before the 1-hour clamp could apply, tearing the connection down on every read of the line and applying nothing. Now clamped in the integer domain at the wire boundary, which is where Java and Go clamp. The overflow is symmetric; both directions are covered.
  • The healthy-operation reset window was measured with DateTime.Now, a local wall clock, so a daylight-saving transition was a deterministic twice-yearly trigger and an NTP step could fire the reset early or suppress it entirely. Now measured with a monotonic Stopwatch.

Observable behavior changes

  • ReadyState after Close() on a never-started source goes RawShutdown rather than staying Raw, and Closed(Shutdown) now fires on that path where it was previously silent.
  • A server-directed retry: value now determines the delay duration. Previously the backoff was constructed once from InitialRetryDelay and the retry: value only gated whether any wait happened, so every positive value behaved identically and zero disabled the wait entirely.
  • Reconnect delays above int.MaxValue ms (~24.9 days) are clamped rather than escaping StartAsync.
  • The healthy-operation window is measured monotonically and is unaffected by clock adjustments.
  • A sub-millisecond InitialRetryDelay no longer grows into a normal backoff progression. The floating-point path kept such a value alive until doubling lifted it past 1ms; the integer path truncates it to zero up front.

Worth a reviewer's attention

  • n resets on a retry: hint, matching Java and Go, which diverges from RETRY §1.11.2 ("MUST NOT alter the attempts counter"). All three implementations do this; worth a spec question rather than three separate deviations.
  • max == 0 means zero delay, not "no ceiling, use the base" as in Java and Go. Pre-existing .NET behavior, preserved by the parity vectors, and the literal reading of an API that always passes both bounds explicitly. Java and Go treat 0 as an unset sentinel, which this API does not have.
  • A bounds change resets the backoff progression. Java's activateRetryDelayStrategy is a documented pointer swap that never resets, with per-strategy state. Here the reset is load-bearing: raising the minimum while n is large would compute min << n and pin at the ceiling immediately. Consequence for the consumer: the extended regime must raise the minimum, not just the ceiling.
  • EventSource gains an internal BackOff accessor used only by tests.
  • Implemented with the backoff owning the active bounds rather than layering a nullable override on EventSource; one less piece of state.

Testing

296 unit tests, including:

  • 85 parity vectors generated from the replaced algorithm, so the rewrite is pinned to the delays the library produced before it.
  • An exact BigInteger reference for the delay formula over curated bound pairs and a million randomized checks, pinning the shift-based arithmetic to a reference that cannot overflow. Removing the n >= 63 shift guard fails it with min=1 max=2 n=64 actual=1 expected=2 — C# masking << 64 into << 0.
  • An end-to-end test that a retry: value changes the actual wait, not just the backoff's internal state. Asserts a lower bound only, since jitter makes T/2 a deterministic floor.

SSE contract tests pass with no new skips. Fixes were mutation-verified — the implementation was temporarily broken in each case to confirm the tests fail. net462 unit tests run only on Windows, so CI covers that leg.

SDK-2791
SDK-2855


Note

Overview
Breaking:IEventSource adds SetTemporaryRetryDelayBounds and ClearTemporaryRetryDelayBounds so callers can change min/max reconnect delays at runtime; temporary bounds auto-revert after BackoffResetThreshold or via Clear.

Reconnect timing is reworked end-to-end: ExponentialBackoffWithDecorrelation is thread-safe, uses overflow-safe integer delay math, supports runtime bounds and a sticky server-directed minimum, and always drives backoff waits (removing the old _retryDelay on/off gate). SSE retry: values now set that minimum (clamped to 1 hour at parse time), reset the backoff step, and respect active ceilings—including temporary ones.

Shutdown and lifecycle:Close() transitions never-started sources to Shutdown and raises Closed; backoff waits use a shutdown token and cap sleep at int.MaxValue ms so Close() can interrupt long waits and huge delays cannot fault StartAsync. Healthy-connection reset uses Stopwatch instead of DateTime.Now.

Reviewed by Cursor Bugbot for commit 7a39b04. Bugbot is set up for automated code reviews on this repo. Configure here.

Adds two members to IEventSource so a consumer can slow reconnection at
runtime without reconfiguring or recreating the client:
void SetTemporaryRetryDelayBounds(TimeSpan initialDelay, TimeSpan maxDelay)
void ClearTemporaryRetryDelayBounds()
Temporary bounds revert to the configured values automatically once a
connection stays open for BackoffResetThreshold, or explicitly via Clear.
Reapplying identical bounds is idempotent, so a caller may set them on every
failure without pinning the delay at the minimum.
The SSE retry: field is now applied to the backoff rather than discarded. A
server-directed value replaces the minimum delay, is clamped to one hour,
resets the backoff formula input so the next attempt uses the directed value
directly, and is sticky: it survives bounds changes and healthy-operation
resets until the server sends another value. Ceilings are never affected, so
the bound on reconnection keeps applying.
Previously the parsed value only acted as an on/off switch, which also meant
configuring InitialRetryDelay(TimeSpan.Zero) disabled backoff outright. That
gate is gone; backoff is governed by the bounds alone.
The delay computation is reworked to integer arithmetic. It previously raised
2 to the power of an unbounded exponent in floating point and clamped the
result, which threw OverflowException once the exponent passed 21 with a
maximum above int.MaxValue milliseconds -- and that exception escaped the
reconnect loop, stopping the stream permanently. It now compares against the
ceiling before shifting, so no intermediate can exceed it. One behavior change
falls out: bounds below one millisecond now truncate to zero, where the former
floating-point path yielded one millisecond at the second attempt.
Test coverage includes 85 parity vectors generated from the replaced algorithm,
so the rewrite is pinned to the delays the library produced before it.
BREAKING CHANGE: IEventSource gains two members. Implementations of the
interface must add them; callers of EventSource are unaffected.
SDK-2791
Close() could not end a backoff wait already in progress: Task.Delay was given
no cancellation token, and the per-request token is created after the wait, so
CancelCurrentRequest had nothing to act on. The reconnect loop stayed alive
until the delay elapsed, holding the EventSource and its timer.
That was previously bounded by MaxRetryDelay, 30 seconds by default. Temporary
bounds and server-directed values now make an hour reachable, so the wait takes
a shutdown token that Close cancels.
RETRY specification Requirement 1.10.1.
SDK-2791
xUnit1031 flags Task.Wait inside a test method. Replacing it with WhenAny plus
an await keeps the deadlock guard and additionally surfaces an exception thrown
on the writer thread, which Wait followed by IsCompleted did not.
Four behavior fixes, plus documentation corrections and test coverage.
- Close() from the Raw state did not transition ReadyState, so a StartAsync
running afterwards saw Raw and began connecting: an unbounded reconnect loop.
Every state except Shutdown now transitions, so ReadyState reports Shutdown
and Closed(Shutdown) fires where that path was previously silent. This was a
regression introduced by b786b7d.
- A computed delay above Task.Delay's platform ceiling threw out of StartAsync,
stopping the stream permanently. Now clamped at int.MaxValue milliseconds,
the lower of the two target frameworks' ceilings.
- A large retry: value threw OverflowException from TimeSpan.FromMilliseconds
before the one-hour clamp could apply, tearing the connection down on every
read of the line and applying nothing at all. Now clamped in the integer
domain at the wire boundary, which is also where Java and Go clamp. The
overflow is symmetric, so both directions are covered.
- The healthy-operation reset window was measured with DateTime.Now, a local
wall clock, making a daylight-saving transition a deterministic twice-yearly
trigger and letting an NTP step fire the reset early or suppress it entirely.
Now measured with a monotonic Stopwatch.
Documentation: the public docs claimed ClearTemporaryRetryDelayBounds and the
healthy-operation restore bring back the configured InitialRetryDelay, which
does not hold once a retry: value has been received, since the effective
minimum resolves to the server-directed value first. Corrected in six places
across IEventSource and ConfigurationBuilder. Also restored the locking
precondition on the delay computation, which had been lost in a rename.
Tests: an exact BigInteger reference for the delay formula, over curated bound
pairs and a million randomized checks, pinning the shift-based arithmetic to a
reference that cannot overflow; and an end-to-end test that a retry: value
changes the actual wait rather than only the backoff's internal state.
@tanderson-ld
tanderson-ld marked this pull request as ready for review September 1, 2026 15:48
@tanderson-ld
tanderson-ld requested a review from a team as a code ownerSeptember 1, 2026 15:48
// Random.Next takes an int bound. 2^31 milliseconds is far longer than any
// reconnect delay we would use, so saturating here cannot affect a realistic
// delay.
int jitterBound = unjittered > int.MaxValue ? int.MaxValue : (int)unjittered;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: we seem to do the math.min / max for value clamping. Any reason we don't do it here?

Suggested change
intjitterBound=unjittered>int.MaxValue?int.MaxValue:(int)unjittered;
intjitterBound=Math.min(unjittered,int.MaxValue);

/// </para>
/// </remarks>
/// <param name="minDelay">the server-directed reconnection time</param>
public void SetServerDirectedMinDelay(TimeSpan minDelay)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we need to discuss this. If we don't trust the server to always send us safe values, having this unguarded could cause serious problems. I also don't know that setting this should reset the backoffN. We do guard against a negative number but that sets the value to 0. So if the server sent us a value 0 or negative, it could slam us with requests and never wait. If it sent us a 0 by accident and then later had an error, the event source would hit without a wait due to calculating a delay increase based on 0.

If it continually sent us a 0 or negative, the backoff would never advance due to n being reset.

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants

@tanderson-ld@jsonbailey