Give the retry back-off its own setting instead of reusing HttpTimeout

[werwolfby]

FinanceNetConfiguration.HttpTimeout is documented and used as the HTTP client timeout, but the same value is also passed to Polly as the per-attempt back-off base. Raising the HTTP timeout silently multiplies total retry latency, which is the opposite of what the setting reads like it does.

Cause

src/Extensions/ServiceCollectionExtensions.cs:49:

PollyPolicyFactory.GetRetryPolicy(options.Value.HttpRetryCount, options.Value.HttpTimeout, logger)
//                                                              ^^^^^^^^^^^^^^^^^^^^^^^^

against src/Utilities/PollyPolicyFactory.cs:10, where the second parameter is the back-off base:

public static AsyncRetryPolicy GetRetryPolicy<T>(int retryCount, int waitTimeSecs, ILogger<T> logger)
    // ...
    retryAttempt => TimeSpan.FromSeconds(waitTimeSecs * retryAttempt)   // linear: 20s, 40s, 60s, ...

The same field is separately — and correctly — applied as client.Timeout at ServiceCollectionExtensions.cs:70, 89, 99, 109.

With the shipped defaults (HttpRetryCount = 10, HttpTimeout = 20) the linear back-off sums to:

20 x (1+2+3+...+10) = 20 x 55 = 1100 s  ~ 18 min 20 s

of sleeping, plus 10 HTTP round trips, for a single failing call. That figure is computed from the defaults rather than waited out end to end, but the first minute of it is easy to observe.

Changes

• Add FinanceNetConfiguration.HttpRetrySleepTime (default 1 second) and pass that to the policy factory. HttpTimeout keeps its documented meaning as the HTTP client timeout and nothing else.

• Replace the linear back-off with exponential growth from that base, capped per attempt, plus jitter:

  internal static TimeSpan GetRetryDelay(int retryAttempt, int baseWaitTimeSecs)
  {
      if (baseWaitTimeSecs <= 0)
      {
          return TimeSpan.Zero;
      }
      var exponent = Math.Min(retryAttempt - 1, 30);  // keep Pow away from infinity
      var backOffSecs = Math.Min(baseWaitTimeSecs * Math.Pow(2, exponent), MaxRetryDelaySecs);
      var jitterMs = RandomNumberGenerator.GetInt32(0, baseWaitTimeSecs * 1000);
      return TimeSpan.FromSeconds(backOffSecs) + TimeSpan.FromMilliseconds(jitterMs);
  }

  The cap (30 s) keeps a high retry count from running away; the jitter stops concurrent callers retrying in lockstep, which matters against a provider that just rate-limited all of them. A base of 0 retries without waiting, which is convenient in tests.

The same defaults now total 1+2+4+8+16+30+30+30+30+30 = 181 s before jitter, instead of 1100 s.

Compatibility

HttpRetrySleepTime is new and defaults to a sane value, so no caller has to do anything. Two behavioural notes:

• Anyone who raised HttpTimeout will find their retries much faster than before. That is the fix, but it is a real change for anyone who had — knowingly or not — been using it to pace retries. Setting HttpRetrySleepTime restores explicit control.
• Back-off is no longer deterministic, by design. GetRetryDelay is internal, so this is not a public API change.

README and CLAUDE.md are updated to document the new setting and to state that HttpTimeout must not be reused as the back-off base.

Tests

Adds RetryBackOffTests covering the default value, the growth curve, the cap, the presence of jitter, the zero-base shortcut, and — the regression that actually matters — that a generous HttpTimeout no longer drives the back-off:

services.AddFinanceNet(new FinanceNetConfiguration
{
    HttpRetryCount = 2,
    HttpTimeout = 30,          // generous transport timeout ...
    HttpRetrySleepTime = 0,    // ... must not become a 30s + 60s back-off
});

That test resolves the real policy from the registry, so it exercises the wiring rather than the factory in isolation. Against the current code it takes 90 seconds; with the fix, milliseconds.

TestCategory=Unit passes: 163 tests, 0 failures. No new analyzer warnings.

Note

This is the last of the three interacting issues, and it is the one that sets the size of the other two. An unknown symbol triggers a retry storm (#56), sized by this misapplied timeout value, which cannot be cancelled (#55). Any one of the three substantially mitigates the others; together they turn an 18-minute unkillable stall into a fast, cancellable failure.

It touches PollyPolicyFactory and ServiceCollectionExtensions only, so it does not conflict with #55 or #57. #56 also touches PollyPolicyFactory, but a different line (the Handle predicate rather than the sleep-duration provider), so at most a trivial rebase.

---

These changes were generated with Claude Code, and I have reviewed them.

🤖 Generated with Claude Code

[thorstenalpers]

Thank you for this — the underlying diagnosis is correct and I verified it: on main, HttpTimeout really is used twice, as client.Timeout and as the Polly back-off base, and RegisteredPolicy_BacksOffByHttpRetrySleepTime_NotHttpTimeout genuinely fails when the wiring is reverted (I measured 96,176 ms against the old code). Introducing HttpRetrySleepTime is the right fix and the cap plus jitter are a real improvement over linear growth.

Two things I would like changed before merge: the new default of 1 second works against the rate limits the retry policy exists for, and the claim that this does not conflict with #56 does not hold.

Verification

Run locally against eafeb075 in a detached worktree.

Check	Command	Exit	Status
Build	dotnet build Finance.NET.slnx --configuration Release	0	VERIFIED — "Build succeeded. 0 Warning(s) 0 Error(s)"
CI gate	dotnet test tests/Tests.csproj -c Release --filter "TestCategory=Unit"	0	VERIFIED — "Failed: 0, Passed: 163, Skipped: 0, Total: 163"
Revert proof (wiring)	revert ServiceCollectionExtensions.cs to main, run RetryBackOffTests	1	VERIFIED — 2 failures, incl. "retries slept for 96176ms"
Revert proof (factory)	revert PollyPolicyFactory.cs to main, build	1	VERIFIED — CS0117; the four GetRetryDelay tests cannot compile against main, so they cover new behaviour rather than the regression
Merge vs #56	git merge-tree --write-tree --name-only HEAD refs/pr/56	1	VERIFIED — CONFLICT in src/Utilities/PollyPolicyFactory.cs
Merge vs #55 / #57	same, against each head	0 / 0	VERIFIED — no conflict
BOM survey	byte scan of src/**/*.cs	0	VERIFIED — 46 files with BOM, 7 without
Integration tests	—	—	NOT CHECKED — live provider endpoints, no FinanceNet:AlphaVantageApiKey available
Long-Running tests	—	—	NOT CHECKED — excluded from the CI gate by convention
SonarCloud	—	—	NOT CHECKED — no SONAR_TOKEN. The in-build SonarAnalyzer produced 0 warnings; server-side rules were not run
Back-off arithmetic	—	—	INFERRED from the code, not waited out in real time

Note: ci.yml has no pull_request trigger and pins its checkout to ref: main, so this PR has no CI status of its own — these local runs are the only evidence. That is on us and I am fixing it separately.

Effective back-off for a default-configured caller

	Before	After
Base	HttpTimeout = 20 s	HttpRetrySleepTime = 1 s
Delays	20, 40, 60, 80, 100, 120, 140, 160, 180, 200	1, 2, 4, 8, 16, 30, 30, 30, 30, 30 (+ 0–1 s jitter each)
Retry fires at	20, 60, 120, 200, 300, 420, 560, 720, 900, 1100 s	1, 3, 7, 15, 31, 61, 91, 121, 151, 181 s
Total	1100 s	181–191 s

Your 1100 s and 181 s figures both check out.

Major

The new default fights the rate limits the retry exists for — src/FinanceNetConfiguration.cs:21

HttpRetryCount is documented one property up as "Default retries for failed http requests (caused by rate limits)". With a 1 s base, retries 1 through 5 fire at t ≈ 1, 3, 7, 15 and 31 s — all five inside a typical 60 s rate-limit window. Before, only one attempt (t = 20 s) landed inside it.

Failure scenario: default configuration, Alpha Vantage answers 429. AlphaVantageService.cs:186-189 calls EnsureSuccessStatusCode() inside _retryPolicy.ExecuteAsync, so every attempt is a real request. Instead of burning 1 request in the first 60 s, it burns 5. On the free tier (25 requests/day) a single failing call now consumes 20% of the daily quota instead of 4%. Wall-clock recovery is essentially unchanged (t ≈ 61 s vs t ≈ 60 s), so the win here is the ceiling — 181 s instead of 1100 s — and you keep that ceiling with a larger base.

Suggest 2–5 s. The decoupling itself is not in question.

    /// <summary>
    /// Base wait between retries in seconds, default 5 seconds. Retries back off
    /// exponentially from this base (5s, 10s, 20s, ...) with jitter, capped per attempt.
    /// Set to 0 to retry without waiting.
    /// </summary>
    [Required] public int HttpRetrySleepTime { get; set; } = 5;

The PR conflicts with #56, contrary to the description — src/Utilities/PollyPolicyFactory.cs:1-2

The description says #56 touches "a different line […] so at most a trivial rebase". Measured:

$ git merge-tree --write-tree --name-only HEAD refs/pr/56
CONFLICT (content): Merge conflict in src/Utilities/PollyPolicyFactory.cs

The conflict is confined to the using block:

<<<<<<< HEAD
using System;
using System.Security.Cryptography;
=======
using System;          <- with BOM
using Finance.Net.Exceptions;
>>>>>>> refs/pr/56

The body of the file merges cleanly — #56's Handle predicate and this PR's GetRetryDelay end up side by side without markers, and they are complementary. The conflict exists only because this PR strips the file's BOM (see below). Restore the BOM and the merge is clean.

Minor

Unrelated BOM removal — src/Utilities/PollyPolicyFactory.cs:1

The diff shows -using System; / +using System; with only the BOM differing. A byte scan of src/**/*.cs says 46 of 53 files carry a UTF-8 BOM and this file was one of them. The change is unrelated to the PR's concern and is the sole cause of the #56 conflict. Please restore it — I will add a charset key to .editorconfig so this stops being a per-PR judgement call.

Jitter is scaled by the base, not by the computed delay — src/Utilities/PollyPolicyFactory.cs:40

RandomNumberGenerator.GetInt32(0, baseWaitTimeSecs * 1000) bounds the jitter by one base interval regardless of how large the back-off actually got. Failure scenario: 50 callers get rate-limited at the same moment on default settings. From retry 6 onwards every one of them waits 30 s + U[0, 1000 ms) — the whole herd fires inside a 1-second window, every 30 seconds. That is exactly the lockstep the doc comment says the jitter prevents; it only softens it by 3%.

The same line also has an unchecked int overflow: baseWaitTimeSecs * 1000 goes negative for HttpRetrySleepTime > 2_147_483, and GetInt32(0, negative) then throws ArgumentOutOfRangeException from inside the sleep-duration provider, replacing the original provider failure with an unrelated one. Bounding the jitter by the already-capped backOffSecs fixes both:

        var exponent = Math.Min(retryAttempt - 1, 30);
        var backOffSecs = Math.Min(baseWaitTimeSecs * Math.Pow(2, exponent), MaxRetryDelaySecs);
        var jitterMs = RandomNumberGenerator.GetInt32(0, (int)(backOffSecs * 1000));
        return TimeSpan.FromSeconds(backOffSecs) + TimeSpan.FromMilliseconds(jitterMs);

Note this widens the ranges asserted in GetRetryDelay_GrowsExponentially, which would need updating to [n, 2n].

New test file where two existing ones are the home — tests/Utilities/RetryBackOffTests.cs

tests/Utilities/PollyPolicyFactoryTests.cs already covers this class, and tests/Extensions/ServiceCollectionExtensionsTests.cs:39-40,83-84 already asserts that every config value round-trips through AddFinanceNet — but it was not extended with HttpRetrySleepTime. Instead the assertion lives in a new file.

Failure scenario: the next person adding a property to FinanceNetConfiguration reads ServiceCollectionExtensionsTests as the canonical "all config values are propagated" test, does not see HttpRetrySleepTime there, and concludes no such convention exists. Please move the round-trip assertion into ServiceCollectionExtensionsTests and the GetRetryDelay cases into PollyPolicyFactoryTests.

The integration harness gets slower, not faster — tests/TestHelper.cs:30-31

TestHelper sets HttpTimeout = 3, HttpRetryCount = 10 and was not updated. The effective back-off there was 3 × 55 = 165 s; it is now 181 s plus up to 10 s of jitter. Nothing breaks (_tests-template.yml has no timeout-minutes), but the one place in the repo that deliberately kept the base low now waits longer. Setting HttpRetrySleepTime explicitly there would keep the intent visible.

XML docs on internal members — src/Utilities/PollyPolicyFactory.cs:11,27-31, tests/Utilities/RetryBackOffTests.cs:15-19

MaxRetryDelaySecs is internal const, GetRetryDelay is internal, RetryBackOffTests is a test class. CLAUDE.md scopes XML <summary> to public API surfaces, and GenerateDocumentationFile emits nothing for internal members, so these blocks reach no output. The repo also avoids multi-line comment blocks; lines 27-31 and 15-19 are five lines each.

Descriptive comments in the tests — tests/Utilities/RetryBackOffTests.cs:37,77,78

// 1s, 2s, 4s, 8s, each plus up to one base interval of jitter. restates the InRange assertions directly below it. The test names already carry the intent.

Nits

• tests/Utilities/RetryBackOffTests.cs:71-91 — measured on reverted code: Failed RegisteredPolicy_BacksOffByHttpRetrySleepTime_NotHttpTimeout [1 m 36 s]. Milliseconds when green, the full old back-off when red. A [Timeout(5000)] would make the same statement without the wait.
• README.md:53, CLAUDE.md:39 — both mention only "base for exponential back-off". The 30 s per-attempt cap is what makes the default HttpRetryCount = 10 tolerable and it appears nowhere outside the code.
• The description says the PR "touches PollyPolicyFactory and ServiceCollectionExtensions only"; it also touches FinanceNetConfiguration.cs, README.md, CLAUDE.md and a new test file.

Question

Was the default of 1 deliberate given that HttpRetryCount's own doc names rate limits as the reason retries exist? If so, that reasoning belongs in the release notes.

Merge plan and release note

Measured across all six pairings: this PR is clean against #55 and #57 and conflicts only with #56, in the using block. Planned order: #57 → this one → #55 → #56.

On versioning: this PR correctly leaves <Version> alone — the repo bumps in a separate release PR. But it adds public API (FinanceNetConfiguration.HttpRetrySleepTime) and changes default runtime behaviour for every existing caller (1100 s → 181 s of back-off), so the release that carries it will be a minor bump to 1.2.0, and release-notes/v1.2.0.md needs both facts as user-facing bullets — not just the new setting. I will handle that in the release PR.