Honour CancellationToken through the retry policy

[werwolfby]

Every public API accepts a CancellationToken, but cancellation currently has no effect: the token is never handed to Polly, so the back-off sleeps between retries are uninterruptible.

The token does reach HttpClient.SendAsync, so an in-flight request is cancellable. But with the default configuration (HttpRetryCount = 10, HttpTimeout = 20, the latter also used as the linear back-off base) a failing call spends the overwhelming majority of its time asleep between attempts — exactly where the token cannot reach. A caller with a 20 s budget was still running well past 75 s.

Cause

All call sites use ExecuteAsync(Func<Task<T>>), the overload that takes no CancellationToken:

return await _retryPolicy.ExecuteAsync(async () =>
{
    var document = await Helper.FetchHtmlDocumentAsync(httpClient, _logger, url, token)...

Changes

• Switch all 15 _retryPolicy.ExecuteAsync sites to the token-aware overload, and use the token Polly hands the delegate rather than capturing the outer one. For the bare retry policy registered today these are the same token, but the call sites resolve an opaque AsyncPolicy from the registry — if a timeout or circuit breaker is ever wrapped around it, the captured-token version would silently keep waiting.

  File	Sites
  src/Services/YahooFinanceService.cs	6
  src/Services/AlphaVantageService.cs	4
  src/Services/XetraService.cs	2
  src/Services/DataHubService.cs	2
  src/Utilities/YahooSessionManager.cs	1

• Stop swallowing cancellation in the surrounding error handling. Each catch-all that wrapped everything in FinanceNetException now rethrows OperationCanceledException when the caller's token was cancelled:

  catch (OperationCanceledException) when (token.IsCancellationRequested)
  {
      throw;
  }

  Without this the fix above would still surface as FinanceNetException. The guard is deliberately narrow — cancellation is only rethrown when the caller cancelled, so an HttpClient timeout (TaskCanceledException) is still wrapped exactly as before.

• YahooFinanceService.GetInstrumentsAsync and FetchSymbolsAsync no longer degrade cancellation into a logged warning or a partial result.

Tests

Adds CancellationTokenPropagationTests: six entry points across all four services and the session manager, each cancelled 150 ms into a call whose policy is configured to sleep 2 s between attempts. Each test asserts an OperationCanceledException and that the call gave up well before the back-off would have elapsed.

Against the current code all six fail with FinanceNetException after sleeping out the full retry budget — the fixture takes a full minute. With the fix it takes about a second.

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

Note

Only the cancellation problem is addressed here, deliberately kept to one concern. The back-off sizing (HttpTimeout doubling as the retry back-off base) and the retrying of empty result sets are separate issues that compound with this one; I have fixes for those on separate branches and can open them as follow-up PRs if useful.

---

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

🤖 Generated with Claude Code

[thorstenalpers]

Thank you for this, and for the way you split the four PRs. Keeping the back-off sizing, the empty-result retries and the dropped-symbol logging out of this branch is what made it reviewable in one sitting, and the description told me what to check rather than what to admire — including the numbers, which is rare and made the whole review faster. I appreciate the work.

This fixes a real and surprisingly large problem, and the diff is tightly scoped to it. I reproduced the symptom before reviewing the fix: with a token cancelled after 150 ms, YahooFinanceService.GetInstrumentsAsync runs for 30089 ms on main and finally throws FinanceNetException; on this branch it gives up after 157 ms with TaskCanceledException. I also confirmed the new tests are genuine regression tests rather than restatements of the implementation. No blocking issues; the notes below are all minor.

Verification

Check	Command	Exit	Result
Head matches PR	git rev-parse HEAD	0	4cc078e9…
Release build (analyzers on)	dotnet build Finance.NET.slnx --configuration Release	0	0 Warning(s) / 0 Error(s)
Unit tests (CI gate)	dotnet test tests/Tests.csproj -c Release --filter "TestCategory=Unit"	0	Failed: 0, Passed: 162, Skipped: 0, Total: 162
Regression proof, single file	revert src/Services/DataHubService.cs to main, rerun	1	Failed: 1, Passed: 5
Regression proof, all files	revert src/ to main, rerun	1	Failed: 6, Passed: 0, Duration: 1 m
All call sites converted	grep ExecuteAsync( over src/	0	15 sites, all async ct =>
Release-notes requirement	changed-file list	0	no <Version> bump, so none required
SonarCloud quality gate	dotnet-sonarscanner	—	NOT CHECKED (no SONAR_TOKEN)
Integration tests	--filter "TestCategory=Integration"	—	NOT CHECKED (live endpoints; Alpha Vantage also needs FinanceNet:AlphaVantageApiKey)
Long-running tests	--filter "TestCategory=Long-Running"	—	NOT CHECKED (not part of the CI gate)

All claims in the description checked out, including the two numeric ones: reverting src/ produces exactly Failed: 6, Passed: 0 ... Duration: 1 m, and the unit run reports Passed! - Failed: 0, Passed: 162.

Minor

The path with the largest improvement is the one path without a test. src/Services/YahooFinanceService.cs:309

The six tests cover 6 of the 15 converted call sites. The uncovered ones are mostly copy-paste identical to a covered one, with one exception: FetchSymbolsAsync / GetInstrumentsAsync is the only site whose surrounding control flow genuinely differs — a typed-less catch that returns a partial result, plus a log-and-continue loop above it. If that guard is ever removed, the failure mode is not a wrong exception type but a silently truncated instrument list returned as success. That is also the path where I measured the biggest win (30089 ms → 157 ms), so it seems worth pinning down. A seventh case in the existing fixture covers it — entirely your call whether to take it:

[Test]
public void GetInstrumentsAsync_Cancelled_StopsWithoutWaitingOutTheBackOff()
{
    var service = new YahooFinanceService(
        Mock.Of<ILogger<YahooFinanceService>>(),
        _mockHttpClientFactory.Object,
        _mockPolicyRegistry.Object,
        Mock.Of<IYahooSessionManager>());

    // Unfiltered, so cancellation has to escape both the per-type log-and-continue loop
    // and the partial-result catch in FetchSymbolsAsync.
    AssertCancelsPromptly(token => service.GetInstrumentsAsync(null, token));
}

I verified this one is worth its keep: with src/Services/YahooFinanceService.cs reverted to main it fails after 30 s, and it is green in 1 s on your branch.

While checking, I also filled in the remaining call sites (GetProfileAsync, GetSummaryAsync, GetFinancialsAsync, Alpha Vantage GetOverviewAsync / GetIntradayRecordsAsync / GetForexRecordsAsync, DataHub GetSp500InstrumentsAsync), which brings the fixture to 14 cases covering 14 of the 15 converted sites — the 15th, XetraService.GetDownloadUrl, is private and covered transitively. Reverting all of src/ fails all 14; the green run costs 2 s. Happy to hand that over as a patch if you want it, but it is your branch and your fixture, so I would rather offer than push.

Handle<Exception>() also matches OperationCanceledException, leaving one bogus retry warning. src/Utilities/PollyPolicyFactory.cs:13

Worth stating what I checked here, since it is easy to assume the worse case: this does not cause a hang. Polly calls ThrowIfCancellationRequested() at the top of each iteration, so an already-cancelled token short-circuits at 0 ms without invoking the delegate at all, and the back-off sleep is token-aware. What remains is that a request cancelled in flight is still classified as retryable, so onRetry fires exactly once and logs Retry 1 after 00:00:20 due to A task was canceled. before the sleep aborts immediately. Log noise only, and arguably out of scope for this PR — but since the policy factory is the root of this behaviour, it may fit the follow-up branch you mentioned:

return Policy
    .Handle<Exception>(ex => ex is not OperationCanceledException)
    .WaitAndRetryAsync(

(Not offered as a suggestion block, since PollyPolicyFactory.cs is not part of this diff.)

A documented invariant becomes false. CLAUDE.md:44

That line states "All failures are wrapped in FinanceNetException." After this change, cancellation deliberately is not. This is the right behaviour, but it is a behavioural breaking change for anyone using a single catch (FinanceNetException) as their complete error handling. Updating that sentence here, and carrying a note into release-notes/ when the next version ships, would keep the docs honest. No release-notes file is required by this PR itself — correctly, since it does not bump <Version>.

The wall-clock threshold may flake in CI. tests/Services/CancellationTokenPropagationTests.cs:35

The lower bound is safe: cancelling before the first attempt still yields OperationCanceledException via Polly's loop-entry check. The upper bound is the risk — CI runs this suite under coverlet instrumentation and the Sonar scanner on a shared ubuntu-latest runner, and 1500 ms is not a lot of headroom. Since the exception-type assertion already separates fixed from broken on its own (I confirmed the reverted code throws FinanceNetException), the timing bound could be looser without losing power — 1900 ms is still comfortably below the 2 s sleep.

Nit

XML doc on a private field. tests/Services/CancellationTokenPropagationTests.cs:34

Repo convention is XML <summary> on public API surfaces only. The rationale is worth keeping, just not as a doc comment:

    // Comfortably below the first back-off sleep, comfortably above the cancel delay.

The Alpha Vantage fallback policy is never exercised. tests/Services/CancellationTokenPropagationTests.cs:59

TryGet is mocked to succeed, so the hardcoded fallback in AlphaVantageService.cs:46-59 never runs in these tests. I reproduced its shape separately (.Handle<HttpRequestException>().Or<TaskCanceledException>()) and it behaves identically under the token-aware overload, so this is a coverage gap rather than a defect.

Out of scope, noted only

Both pre-date this PR (git show origin/main confirms they come from cab295e), so nothing to do here — flagging them in case they are useful for the follow-up branches:

• German strings in library code, against the repo's English-only rule: src/Services/XetraService.cs:72 and src/Services/AlphaVantageService.cs:55-63.
• src/Services/XetraService.cs:49 runs a nested retry policy — GetDownloadUrl opens its own ExecuteAsync inside the outer policy's delegate, so worst case is retryCount² attempts. This compounds with the back-off sizing you already plan to address separately.