Skip to content

feat(providers): HttpFetcher with retry + feed payload validation (XWING-2223) - #8

Merged
smith-xyz merged 3 commits into
developmentfrom
feat/http-provider
Oct 21, 2026
Merged

smith-xyz merged 3 commits into
developmentfrom
feat/http-provider

Conversation

@Kyl-Bempah

@Kyl-Bempah Kyl-Bempah commented Oct 17, 2026 •

Copy link
Copy Markdown
Contributor

Moves the outbound-HTTP part of XWING-2223 out of traust and into core, per the Layers rule from #7: outbound HTTP lives inside providers. This is what traust-security/traust#61 retry code becomes once it moves into core; traust-security/traust#61 shrinks to config + feed metadata and calls this.

Plan note: this PR predates the standardization plans and has no plan file of its own; its home plan is plans/traust-standardization/4-dependencies/02-kyl-feeds-and-advisory-lookup.md (blocked on Michele's advisory rules). It is the groundwork that plan builds on.

What this adds

  • providers/http.py: HttpFetcher(user_agent, policy, *, clock=, sleep=).get(url) -> bytes, the outbound-HTTP counterpart to ProcessRunner.
    • 429 waits out Retry-After (seconds or HTTP-date), capped by max_wait_seconds.
    • 5xx and timeouts use capped exponential backoff, plus optional jitter.
    • Any other 4xx fails immediately: retrying a 403/404 just burns the rate-limit window.
    • Giving up raises ServiceError; a fail-fast 4xx also sets .status.
    • RetryPolicy is a frozen dataclass: max_attempts, timeout_seconds, backoff_base_seconds, max_wait_seconds, jitter_seconds.
    • The current time comes from an injected Clock (for HTTP-date Retry-After), so the drift ratchet sees no datetime.now(). Sleep is injectable for tests.
    • stdlib urllib only, so no new dependency.
  • providers/feeds.py: validate_payload(raw, filename) raises ValidationError when a download doesn't decode as its file type (gzip / JSON / UTF-8 CSV). This stops a corrupt 200 from overwriting a good cached copy. EPSS, KEV and rh-cve publish no checksum, so "corrupt" means "doesn't decode"; the VEX archive's checksum is XWING-2224.
  • README: the Providers row lists http and validate_payload.

Consumer

traust#61 (feat/feeds-retry): should now just call HttpFetcher & validate_payload. The PR also adds traust-core as a dependency.

Fits with #6 (drift ratchet)

Cross-checked on the rebased head: the same 3 baseline hits, nothing new. The urllib.request import in providers/http.py is covered by #6's allowed=("*/providers/*",) for http, so the PRs merge in any order without touching .ratchet.json.

Quality checklist

  • Comments only for the non-trivial (the retry-policy rationale)
  • Docstrings don't bloat (compressed to the two-line convention)
  • Don't reinvent (first fetcher in core; ProcessRunner is the pattern)
  • Shared logic is a helper, in one place (validate_payload lives with feeds)
  • Tests cover the edges (429 seconds/date/capped, backoff doubling + cap, fail-fast 4xx with status, gives-up, empty/corrupt/non-UTF-8 payloads)
  • Integration tests use real data, sanitized (offline fakes; no DB path — pure provider)
  • Minimal, clean code (87 lines; no dead code)
  • Queries are low latency (N/A — no queries)
  • Tests change only on purpose (all tests are new; none edited or deleted)

Changed tests

None — only new tests (test_http.py, test_feeds.py).

  • A person reviewed each changed test and the behaviour change is intended

Tests

  • tests/v1/providers/test_http.py: 503→200, timeout→200, 429 with Retry-After as seconds, as an HTTP-date and over the cap, backoff doubling and capping, 404 fails fast with status, gives up after max_attempts. All offline (fake urlopen, recorded sleeps).
  • tests/v1/providers/test_feeds.py: well-formed vs. empty / corrupt gzip / broken JSON / non-UTF-8 CSV.
  • Rebased onto development c061eaa: lint clean, 887 unit passed, ratchet green (cross-checked against feat(checks): drift ratchet tool, baseline, CI gate + docs (XWING-2210) #6's tool + baseline: 3 hits / 2 files, unchanged).

Open questions

  • Naming: HttpFetcher, matching ProcessRunner, endorsed — the ticket's HttpClient doesn't exist in core and the ratchet's use-instead names the providers, not a client.

@chubirka

Copy link
Copy Markdown
Member

Accuracy review: retry and payload-validation failure paths need correction

Reviewed head: ff70a6adaa361733cdc70810237288b04e45698e. Tested both the PR head and a clean integration with core main 5be9803. In addition to the existing tests, smoke checks used a real localhost HTTP server and the actual feed-cache caller from Traust #61.

P2 — An empty compressed CSV replaces a good cache and is marked fresh

providers/feeds.py:53–63 checks emptiness before decompression, not afterward. validate_payload(gzip.compress(b""), "epss_scores-current.csv.gz") succeeds.

Through the actual dependent fetch() caller, a 200 response containing that payload:

  • Replaced the previously populated EPSS cache.
  • Returned success: refreshed (20 bytes).
  • Left a zero-byte decoded CSV and recorded stale: false with no error.

Please reject empty decoded payloads before cache replacement. The current "corrupt/empty payload cannot overwrite a good cached copy" claim is not true for this case.

P2 — Invalid DEFLATE data escapes as zlib.error, not ValidationError

providers/feeds.py:57–65 does not catch every error from gzip.decompress(). A gzip header followed by invalid DEFLATE data raised:

zlib.error: Error -3 while decompressing data: invalid block type

The dependent caller catches ServiceError / ValueError, so this escaped rather than returning the stale-cache fallback. The old cache survived, but stale/error attempt metadata was not recorded. Please normalize the decompression failure into the promised ValidationError and cover this failure class.

P2 — Truncated HTTP bodies bypass retries and leak IncompleteRead

providers/http.py:51–67 does not handle http.client.IncompleteRead from r.read().

A real local server advertised a larger Content-Length than it delivered. With max_attempts=3, the fetcher made only one request and raised raw http.client.IncompleteRead, not ServiceError. The actual feed caller consequently escaped its fallback/error-recording path, although its old cache remained intact.

Please include truncated-response failures in the fetcher's retry/error contract, so callers receive the documented typed failure when recovery is exhausted. The normal 503→200 smoke did work and made two requests.

Exercised verification

The normal retry cases and merge-order/ratchet compatibility claims are supported. The passing tests do not cover the cache-loss and exception-boundary cases above. PostgreSQL was not exercised. No source changes were made.

Recommendation: changes requested before approval.

@smith-xyz
smith-xyz changed the base branch from main to development October 21, 2026 17:29
Kyl-Bempah and others added 2 commits October 21, 2026 15:52
Signed-off-by: Kyl-Bempah <24903083+Kyl-Bempah@users.noreply.github.com>
@smith-xyz

Copy link
Copy Markdown
Contributor

Rebased onto development (c061eaa) — clean, no conflicts — plus one touch-up: the HttpFetcher class docstring compressed to the two-line convention (the fail-fast rationale moved to a one-line comment at the decision point). PR description now carries the standardization format (plan note, quality checklist, changed tests). Cross-checked the ratchet on the rebased head: 3 hits / 2 files, byte-identical — merges in any order with #6. Naming question resolved in the description: HttpFetcher endorsed.

- validate_payload checks emptiness after decompression: an empty gzip
  body is an empty payload, never a cache replacement
- gzip.decompress failures normalize to ValidationError (zlib.error
  joined the caught set; invalid DEFLATE no longer escapes raw)
- truncated HTTP bodies (http.client.IncompleteRead) join the retryable
  transport errors: retried with backoff, ServiceError when exhausted

Each reproduced before fixing; regression tests for all five paths.
@smith-xyz

Copy link
Copy Markdown
Contributor

@chubirka all three findings reproduced before fixing, closed in 8f2bf1b:

  1. Empty compressed CSV — confirmed: validate_payload(gzip.compress(b""), "epss_scores-current.csv.gz") passed. The emptiness check now runs after decompression, so an empty gzip body is an empty payload and raises ValidationError before any cache replacement. (One structural note: the raise sits outside the try block because ValidationError subclasses ValueError — inside, it would re-wrap itself.)
  2. Invalid DEFLATE escapes as zlib.error — confirmed. zlib.error joined the caught set; invalid-DEFLATE payloads now raise the promised ValidationError.
  3. Truncated bodies leak IncompleteRead — confirmed: one request, raw exception. http.client.IncompleteRead now joins the retryable transport errors — retried with backoff, ServiceError when attempts are exhausted.

Regression tests for all five paths (empty gz, invalid DEFLATE, truncated→retry→success, truncated×3→gives up, plus the existing corrupt cases). Gates on this head: lint clean, 891 unit, 200 Postgres integration, coverage 93.9%; ratchet unchanged (3 hits / 2 files).

@smith-xyz
smith-xyz merged commit fc6ae24 into development Oct 21, 2026
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants