[ClientModel] RetryPolicy does not sanity check `Retry-After` value

[jsquire]

Summary

The implementation of the System.ClientModel retry policy will strictly honor a Retry-After header returned regardless of the length it specifies. (src)

Though the retry and overall network request is governed by a cancellation token, an extreme Retry-After value can cause an unexpected delay that strongly resembles a client hang to callers. For example, in OpenAI #404, the service returns a Retry-After of more than 114 hours (~4.75 DAYS). It seems inappropriate to unconditionally honor this.

Proposed change

I'd propose that when evaluating a Retry-After header, the policy applies the following logic:

• Calculate the delay for the current retry.

• If the Retry-After duration exceeds that duration, "consume" another retry.

• If the Retry-After duration exceeds the total duration from those retry attempts, repeat.

• If the Retry-After duration is less or equal to the total duration from consumed retry attempts, adjust the retry counter to match and apply the delay.

• If all retries were consumed and the Retry-After duration exceeds the total duration from all configured retries, do not retry. Just bubble the original exception.

Why am I proposing this?

• The current behavior for an extreme Retry-After is unexpected and confusing.

• User studies have shown that a small number of callers make use of cancellation tokens.

• If the client knows the retry would fail after a period as long as all of the configured retries, it is better to return control to the caller and allow them to evaluate whether or not to attempt the operation again. Applying an unreasonably long retry delay impacts the caller's responsiveness to its users.

[jsquire]

//cc: @christothes, @KrzysztofCwalina, @m-redding

[KrzysztofCwalina]

Maybe we should add a property to the retry policy called MaxDelay and set its default value to something reasonable.

[jsquire]

Maybe we should add a property to the retry policy called MaxDelay and set its default value to something reasonable.

Existing pattern for that in DelayStrategy suffers from the same issue. If the Retry-After value is greater than maxDelay - we just use it verbatim.

The bigger issue is that there is no value in retrying after maxDelay if it is less than Retry-After because the service told us upfront that it would fail. We need to have some methodology that allows us to cap Retry-After and just not bother to retry if that period is outside what we think is reasonable. We could use a maxDelay here if we deviate from the meaning in DelayStrategy and have this one mean "the maximum we'll wait across ALL retry attempts" rather than a single retry attempt.

[christothes]

Perhaps we should throw immediately (with a new exception type?) in the case that Rety-After is longer than the total duration of all calculated retries using maxDelay. This would allow callers to treat it more like an LRO.