Replace hand-rolled HTTP retry with Microsoft.Extensions.Http.Resilience#2
Merged
Conversation
Captures the decision (and rationale) to adopt Microsoft.Extensions.Http .Resilience with a custom AddResilienceHandler pipeline rather than the stock AddStandardResilienceHandler or hand-rolled retry. The standard handler's defaults (10s per-attempt timeout, 1000 concurrent permits, 30s total timeout) would break large-PDF downloads and hammer sternpinball.com. The hand-rolled approach (added in Round 2 polish) already exists but only protects FileDownloader, leaving ManualsScraper without retry. This doc precedes the implementation commit so the rationale is preserved alongside the change for future readers.
…peline Wire Microsoft.Extensions.Http.Resilience 10.4.0 onto both HttpClients in Program.cs via AddResilienceHandler with a custom 2-stage pipeline: concurrency limiter (politeness, permit=MaxConcurrentDownloads) and Polly v8 retry (3 attempts, exponential backoff, jitter, MaxDelay 30s, ShouldRetryAfterHeader=true). The standard handler defaults are wrong for a polite single-host scraper — see docs/http-resilience-research.md. Effects: - ManualsScraper now gets retry for free (the gap that motivated this). - FileDownloader sheds 110 LOC: the retry loop, IsRetryableStatus, IsRetryableException, ComputeDelay, TryGetRetryAfterMs, MaxBackoffMs, and MaxRetryAfterSeconds — Polly owns all of it. - Pipeline is per-client by design — each AddResilienceHandler call gets its own name and config, so future Phase 2 clients (Azure AI, embeddings) can have different policies without touching this code. Tests: drop 3 retry-specific tests (RetriesAndSucceeds, ReturnsFailedAfter MaxRetries, ClientErrorDoesNotRetry) plus the SequencedHandler / CountingStatusHandler helpers. The behavior they verified is now Polly's responsibility, tested in the upstream Polly suite. All other FileDownloader tests (304 fast-path, 200 streaming, conditional headers, size cap, network exception) still pass: 68/68 green. Verified end-to-end: dry-run manuals scrape returns 166 links, 0 errors.
jkeeley2073
added a commit
that referenced
this pull request
May 4, 2026
Per /local-review on PR #68: grep -E exits 2 on a malformed extended regex, but run_rule wraps the grep call with `|| true` which masks exit 2 as "no match" — silently disabling the rule. A typo in the WORK_EMAIL_PATTERN secret would pass the workflow without ever checking commits against the work-email pattern. The narrow fix: pre-validate the pattern by running it against an empty stdin via printf '' | grep -E "$WORK_EMAIL_PATTERN" and checking grep's exit code directly. Exit 2 = malformed pattern, fail the workflow with an error annotation that names the issue and points the operator at how to fix the secret. Exit 0 (matches empty) or 1 (no match against empty) → pattern is well-formed, proceed to run_rule normally. The broader cleanup of run_rule itself (distinguishing grep exit codes 0/1/2 for every rule) is out of scope for this PR — the narrow fix here addresses the new rule's specific risk without touching pre-existing behavior of the other rules. Local review summary (retroactive on PR #68): 0 🔴, 3⚠️ findings. -⚠️ #1 (post-merge smoke test): already covered in the PR description's "Validation hand-off after merge" section. -⚠️ #2 (grep exit-2 silent swallow): fixed by this commit. -⚠️ #3 (doc-anchor verification): the comment cites "docs/build-spec.md Phase 2 § Scope item 9" which exists at build-spec.md:225 — verified, no change needed.
This was referenced May 8, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Round 2 added hand-rolled retry to
FileDownloaderonly. This PR replaces it with the Microsoft-blessedMicrosoft.Extensions.Http.Resiliencepackage, applied to both HttpClients via a shared resilience pipeline. Decision and rationale are committed alongside asdocs/http-resilience-research.md.Why not the stock
AddStandardResilienceHandler?Defaults are wrong for a polite single-host scraper:
We use
AddResilienceHandler(custom pipeline) with two stages: concurrency limiter (politeness) + Polly v8 retry. Skip per-attempt timeout (HttpClient.Timeout already suffices), total timeout, circuit breaker, hedging.Test plan
dotnet buildclean (0 warnings, 0 errors)dotnet test→ 68/68 pass (was 71; -3 retry-specific tests now Polly's responsibility)--source manuals --scrape-only --dry-runreturns 166 links, 0 errorsManualsScraperretry behavior next time Stern hiccups (no longer relies on a single fetch succeeding first try)Notable LOC delta
FileDownloader.csshrinks from 326 to 187 lines. Retry policy lives in Polly's tested code, not ours.Out of scope (deferred)
Range:header follow-up inFileDownloaderitself, independent of the resilience handler. For typical Stern PDF sizes (<20MB) a from-scratch retry is acceptable.ConfigurePrimaryHttpMessageHandler: skipped this PR; Polly's behavior is tested upstream and we don't need to re-verify it. Add if/when we customizeShouldHandleourselves.