Repository navigation
fix(engine): look a host up again after a failed lookup instead of refusing it for the whole page - #7751
fix(engine): look a host up again after a failed lookup instead of refusing it for the whole page#7751miguel-heygen wants to merge 2 commits into
Conversation
![heygengenesis[bot]](https://avatars.githubusercontent.com/in/2913576?s=60&v=4){kind=small}
There was a problem hiding this comment.
Verdict: REQUEST CHANGES · 1 blocker · 0 important · 0 nits · head 4da32b17
The fix itself is right: only a failed lookup is forgotten, and requests already waiting share it. The one blocker is the red Comments run (inline).
Scope
Reviewed the delta on #7748 (a6037e14..4da32b17, 2 files). This PR is stacked, so the CI workflow (lint, typecheck, tests) has not run on it. Locally against chrome-headless-shell 157, both test files pass 10/10, three runs in a row. With the cache-drop line removed, the new test fails (1 of 1), which matches the fails-without-the-fix line.
| verdicts.set(target.hostname, verdict); | ||
| const asked = hostVerdict(host, resolveHost); | ||
| verdicts.set(host, asked); | ||
| // A failed lookup can be a passing blip, so only answers about resolved addresses are kept. |
There was a problem hiding this comment.
blocker: this line fails the Comments workflow's ratchet: RAISED comment share: packages/engine/src/services/pageNetworkPolicy.ts is 5.3% (4/76), at the base 4.5% (3/66). Drop it (that gives 3/75), or carry the reason in a name instead, e.g. a forgetFailedLookup helper. The test name already pins the behaviour.
| // A failed lookup can be a passing blip, so only answers about resolved addresses are kept. |
There was a problem hiding this comment.
|
Preview deployment for your docs. Learn more about Mintlify Previews.
💡 Tip: Enable Automations to automatically generate PRs for you. |
9d5edce to
b1d3b1d
Compare
What
With
PRODUCER_RESTRICT_PAGE_NETWORK=true, a host whose name lookup fails is refused for that request and lookedup again on the next one. Before, the failure was cached as a refusal for the rest of the page.
Why
Stacked on #7748, from its review. One transient DNS failure on a public host (a web font, a remote video, a CDN
script) would otherwise block every later request to that host for the rest of a long render, while Chrome on its
own would have retried. The asset would go missing from the output without the render failing.
Related work
Refs #7748 (base of this PR; GitHub retargets it to
mainwhen #7748 merges).How
pageNetworkPolicy.ts: the host check returns"public" | "non-public" | "unresolved"instead of a boolean.Answers about resolved addresses stay cached for the page. An
"unresolved"answer is dropped from the cacheonce it settles, so the next request to that host asks again. A request already waiting on the same lookup
shares its answer.
Test plan
New test in
pageNetworkPolicy.browser.test.ts(real headless Chrome): a name whose first lookup fails isrefused once, then allowed when the next lookup answers with a public address. The second request reaches the
target server; the first does not.
Fails without the fix: with the cache-drop line removed, the new test fails (1 of 1). With
restrictPageNetworkreplaced by a no-op, the file fails 6 of 8.
Both test files pass 10 of 10, three runs in a row on Linux;
tsc,oxlintandoxfmt --checkpass.Not exercised: Windows and macOS hosts; a real DNS failure (the test injects one through the resolver).