fix: don't crash import on HttpClient.Timeout (default 100s)

[alexeyzimarev]

PR Summary by Qodo

Fix HttpClient retry timeouts to avoid import crash and add regression tests
🐞 Bug fix 🧪 Tests 🕐 20-40 Minutes

Walkthroughs

User Description

Summary

• kcap import crashed mid-run with an unhandled TaskCanceledException whenever a single HTTP call exceeded the .NET default HttpClient.Timeout of 100s. SendWithRetryAsync only caught HttpRequestException, so the timeout-induced cancellation escaped every retry / catch block. Most visibly this killed the classification probe (GET /api/sessions/{id}/last-line) — one slow probe out of 145 would terminate the whole import with no progress shown.
• Each retry attempt now runs under a linked CancellationTokenSource with an explicit per-attempt cap (60s default), and an exhausted-budget timeout is wrapped as HttpRequestException so the existing catch (HttpRequestException) blocks at the import / hook call sites degrade the session to ProbeError / "server unreachable" instead of crashing.
• New HttpClientExtensionsRetryTests cover: per-attempt timeout → HttpRequestException after budget exhausted, retry-and-recover on a single slow attempt, caller cancellation still propagates as OperationCanceledException, transient HttpRequestException retries within budget.

Test plan

• dotnet run --project test/Capacitor.Cli.Tests.Unit/Capacitor.Cli.Tests.Unit.csproj → 1283/1283 pass
• dotnet publish src/Capacitor.Cli/Capacitor.Cli.csproj -c Release → no IL3050/IL2026 AOT warnings
• Repro on a slow / hung server: probe times out → "Skipping {sid} [server unreachable]" instead of crash; import continues
• Smoke test a normal kcap import against a healthy server (no behaviour change)

🤖 Generated with Claude Code

AI Description

• Enforce per-attempt HTTP timeouts in retry loop to prevent import crashing on slow calls.
• Convert exhausted timeout cancellations into HttpRequestException for existing error handling.
• Add unit coverage for retry, timeout, and cancellation propagation behaviors.

Diagram

graph TD
  callers["Import/Hook callers"] --> retry["HttpClientExtensions.SendWithRetryAsync"] --> client["HttpClient"] --> api{{"Capacitor API"}}
  callerCt(("Caller CT")) --> retry
  attemptCt(("Per-attempt CTS")) --> retry
  retry --> outcome["HttpResponse or HttpRequestException"]

  subgraph Legend
    direction LR
    _cmp["Component"] ~~~ _tok(("Token/CTS")) ~~~ _ext{{"External"}}
  end

Loading

High-Level Assessment

The following are alternative approaches to this PR:

1. Set HttpClient.Timeout explicitly (or to Infinite) at client construction

• ➕ Centralizes timeout behavior without changing retry helper signature
• ➕ Avoids per-call CancellationTokenSource allocations
• ➖ Does not provide per-attempt vs total-budget separation
• ➖ Still risks TaskCanceledException surfacing at call sites depending on how requests are issued/handled
• ➖ Harder to ensure consistent behavior across all HTTP usages

2. Adopt a resiliency library (e.g., Polly) for retries/timeouts

• ➕ Standardized policies for retries, timeouts, jitter, and circuit breaking
• ➕ Clearer separation of concerns and richer telemetry hooks
• ➖ Adds dependency and policy configuration complexity
• ➖ Potentially more invasive refactor than needed for this specific crash

Recommendation: Keep the PR’s approach: enforcing a linked per-attempt timeout inside SendWithRetryAsync and converting exhausted-budget timeouts to HttpRequestException directly matches existing error-handling expectations at call sites while preserving caller-initiated cancellation semantics. Alternatives were considered, but they either don’t address the call-site exception shape consistently or add unnecessary dependency/complexity for the scope.

File Changes

Bug fix (1)

HttpClientExtensions.cs Add per-attempt timeout + cancellation-aware retry semantics +54/-9

Add per-attempt timeout + cancellation-aware retry semantics

• Introduces a per-attempt timeout (default 60s) enforced via a linked CancellationTokenSource for each retry attempt. Refactors SendWithRetryAsync to accept a token-aware send delegate, distinguishes caller cancellation from attempt timeout, and wraps exhausted-budget timeout cancellations into HttpRequestException so existing handlers degrade gracefully instead of crashing.

src/Capacitor.Cli.Core/HttpClientExtensions.cs

---

Tests (1)

HttpClientExtensionsRetryTests.cs Add regression tests for retry timeout and cancellation behavior +103/-0

Add regression tests for retry timeout and cancellation behavior

• Adds unit tests validating that per-attempt timeouts retry within the total budget and eventually surface as HttpRequestException when exhausted. Also covers retry recovery after a single slow attempt, propagation of caller cancellation as OperationCanceledException, and retrying transient HttpRequestException failures.

test/Capacitor.Cli.Tests.Unit/HttpClientExtensionsRetryTests.cs

---

[qodo-code-review]

Code Review by Qodo

🐞 Bugs (0) 📘 Rule violations (1) 📎 Requirement gaps (0) 🎨 UX issues (0)

1. Total timeout not enforced ✓ Resolved 🐞 Bug ☼ Reliability

Description

SendWithRetryAsync can exceed totalTimeout because each attempt always uses perAttemptTimeout
(default 60s) even when the total budget is smaller (default 30s), and it can also start another
attempt after backoff even when the budget has already elapsed. This can make CLI commands that rely
on the default timeout (and don’t pass a CancellationToken) block far longer than requested before
failing.

Code

src/Capacitor.Cli.Core/HttpClientExtensions.cs[R229-232]

        while (true) {
+            using var attemptCts = new CancellationTokenSource(perAttemptTimeout);
+            using var linkedCts  = CancellationTokenSource.CreateLinkedTokenSource(ct, attemptCts.Token);
+

Evidence

The implementation always uses perAttemptTimeout for the attempt CTS and only checks sw.Elapsed
in catch filters, so an attempt can run past the budget and the loop can continue after delays.
Defaults make this visible: total default is 30s while per-attempt default is 60s, and several
commands call *WithRetryAsync without a cancellation token so the timeout parameter is expected to
bound runtime.

src/Capacitor.Cli.Core/HttpClientExtensions.cs[79-90]
src/Capacitor.Cli.Core/HttpClientExtensions.cs[214-260]
src/Capacitor.Cli/Commands/ErrorsCommand.cs[6-19]
src/Capacitor.Cli/Commands/RecapCommand.cs[21-33]
src/Capacitor.Cli/WatcherManager.cs[251-272]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
`SendWithRetryAsync` treats `totalTimeout` as a *retry decision* threshold, not a strict wall-clock budget. Because it always constructs `attemptCts` with `perAttemptTimeout` (60s default) and doesn’t guard the loop before starting an attempt or sleeping, a call with `totalTimeout` < `perAttemptTimeout` (e.g., the default 30s) can still block for ~60s before failing, and backoff delays can also push execution beyond the budget.

### Issue Context
Many call sites invoke `GetWithRetryAsync/PostWithRetryAsync` without passing a `CancellationToken`, so `totalTimeout` is the only limiter. The code currently only checks `sw.Elapsed < totalTimeout` in exception filters.

### Fix Focus Areas
- src/Capacitor.Cli.Core/HttpClientExtensions.cs[79-90]
- src/Capacitor.Cli.Core/HttpClientExtensions.cs[214-260]

### Suggested fix approach
- Introduce a `budgetCts = new CancellationTokenSource(totalTimeout)` and link it with the caller token.
- For each attempt, set the attempt CTS to `min(perAttemptTimeout, remainingBudget)` (or just rely on the budget CTS + per-attempt CTS both linked).
- Before starting an attempt, if the budget is exhausted, throw `HttpRequestException` (to preserve current call-site behavior) when it’s *not* caller cancellation.
- Cap the backoff sleep to the remaining budget, and avoid starting a new attempt after the budget expires.
- Ensure caller cancellation still propagates as `OperationCanceledException` (don’t wrap) while budget exhaustion is wrapped into `HttpRequestException`.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

2. PerAttemptTimeout undocumented in README 📘 Rule violation ⚙ Maintainability

Description

The PR introduces a new default per-attempt HTTP timeout (PerAttemptTimeout = 60s) that changes
kcap import behavior on slow/hung servers, but README.md is not updated to reflect this default
behavior change. This can leave users without accurate guidance when imports start
failing/continuing due to the new timeout/retry behavior.

Code

src/Capacitor.Cli.Core/HttpClientExtensions.cs[R82-90]

+    /// <summary>
+    /// Per-attempt cap on a single HTTP call inside <see cref="SendWithRetryAsync(Func{CancellationToken, Task{HttpResponseMessage}}, TimeSpan, CancellationToken)"/>.
+    /// Enforced via a linked <see cref="CancellationTokenSource"/> so the wall-clock cap
+    /// is observable on the token we pass to <see cref="HttpClient"/> — not on the
+    /// client's own <see cref="HttpClient.Timeout"/> (default 100s), which would
+    /// otherwise raise an unhandled <see cref="TaskCanceledException"/> at every call
+    /// site whose <c>catch</c> only covers <see cref="HttpRequestException"/>.
+    /// </summary>
+    internal static readonly TimeSpan PerAttemptTimeout = TimeSpan.FromSeconds(60);

Evidence

Rule 3 requires README updates for user-facing CLI default behavior changes. The PR adds a new
default per-attempt timeout (PerAttemptTimeout) affecting how kcap import behaves under slow
HTTP calls, while the import documentation section in README.md does not describe this
timeout/retry behavior.

CLAUDE.md: Update README.md in the Same PR for Any User-Facing CLI Surface Changes
src/Capacitor.Cli.Core/HttpClientExtensions.cs[82-90]
README.md[80-93]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
This PR changes user-visible default behavior for long-running HTTP calls during `kcap import` by introducing a 60s per-attempt timeout and retry budgeting, but the README does not mention this behavior.

## Issue Context
Compliance requires updating `README.md` in the same PR for user-facing CLI default behavior changes.

## Fix Focus Areas
- README.md[80-93]
- src/Capacitor.Cli.Core/HttpClientExtensions.cs[82-90]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

3. Retry tests timing brittle ✓ Resolved 🐞 Bug ☼ Reliability

Description

HttpClientExtensionsRetryTests uses very small real-time budgets (e.g., 50ms per-attempt, 400ms
total) while the implementation uses real Task.Delay backoff, making the tests scheduler/CI-load
sensitive. This can produce flaky failures unrelated to correctness.

Code

test/Capacitor.Cli.Tests.Unit/HttpClientExtensionsRetryTests.cs[R21-25]

+        var ex = await Assert.That(async () => await HttpClientExtensions.SendWithRetryAsync(
+                    Send,
+                    totalTimeout: TimeSpan.FromMilliseconds(400),
+                    perAttemptTimeout: TimeSpan.FromMilliseconds(50),
+                    ct: CancellationToken.None

Evidence

The tests explicitly configure 50ms/400ms timing while the retry loop uses backoff delays starting
at 250ms, so outcomes depend on real scheduling and timing rather than only logic.

test/Capacitor.Cli.Tests.Unit/HttpClientExtensionsRetryTests.cs[15-32]
src/Capacitor.Cli.Core/HttpClientExtensions.cs[226-260]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
The retry tests depend on tight wall-clock timing (50ms/400ms). Under CI load or slow runners, the exponential backoff + scheduling jitter can cause nondeterministic attempt counts and longer-than-expected elapsed time.

### Issue Context
These are unit tests; flakiness increases pipeline noise and reduces confidence.

### Fix Focus Areas
- test/Capacitor.Cli.Tests.Unit/HttpClientExtensionsRetryTests.cs[8-32]

### Suggested fix approach
- Increase budgets (e.g., per-attempt 200-500ms, total 2-5s) while keeping assertions the same.
- Alternatively, refactor `SendWithRetryAsync` to accept an injectable delay/scheduler (test seam) so tests can run deterministically without real-time delays.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---