Skip to content

fix(http3): update cached Alt-Svc support - #536

Merged
barjin merged 1 commit into
apify:masterfrom
atirna:fix/alt-svc-h3-discovery
Sep 2, 2026
Merged

fix(http3): update cached Alt-Svc support#536
barjin merged 1 commit into
apify:masterfrom
atirna:fix/alt-svc-h3-discovery

Conversation

@atirna

@atirna atirna commented Sep 2, 2026

Copy link
Copy Markdown
Contributor

Why

Alt-Svc h3 discovery first records a host as not h3-capable, then needs to upgrade that cached value when the response advertises HTTP/3.

Verification

  • Before: cargo test -p impit http3::tests::alt_svc_discovery_updates_cached_h3_support -- --exact failed because the cached false value was retained.
  • After: cargo test -p impit passes, including the cache-upgrade regression test.
  • cargo fmt --check
  • cargo clippy -p impit --all-targets -- -D warnings

Fixes #471

@barjin barjin closed this Sep 2, 2026
@barjin barjin reopened this Sep 2, 2026

@barjin barjin left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Scale vs. impact: Proportional. +19/-4 in a single file, isolated to H3Engine::set_h3_support.

Root cause: Confirmed. The removed guard (if cache.contains_key(host) { return; }) made set_h3_support a write-once operation. In impit.rs::execute_request (lines 535-551), every non-h3 response first calls set_h3_support(&host, false), then conditionally calls set_h3_support(&host, true) if the response carries an h3-advertising Alt-Svc header. With the old guard, the first call (always false) permanently locked the cache entry, so the subsequent upgrade to true was silently dropped — a host that advertised h3 support would never actually be attempted over h3 on later requests. Removing the guard makes the cache reflect the latest write, matching the caller's evident intent (last-observed-signal wins, not first-write-wins).

Breaking changes: None. H3Engine and set_h3_support are internal (not part of the public API surface re-exported from the crate), so this is a pure behavior fix with no API shape change.

Test coverage note: The added test exercises H3Engine directly (set_h3_support/host_supports_h3) but doesn't cover the actual call site in impit.rs that triggered the bug (the false-then-conditional-true sequence driven by the Alt-Svc response header). A unit test alone is enough to pin the fixed method's contract, but doesn't guard against a future regression in the caller's sequencing.

Merge fitness: The change is minimal, correct, and directly addresses the described defect with a regression test for the core fix.


Generated by Claude Code

@barjin
barjin merged commit ea036f5 into apify:master Sep 2, 2026
50 of 52 checks passed
@atirna
atirna deleted the fix/alt-svc-h3-discovery branch September 2, 2026 12:24
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.

Alt-Svc HTTP/3 discovery is dead: set_h3_support early-returns before the Alt-Svc upgrade

4 participants