Retry with GET when HEAD fails, not just on 405 - #29
Merged
Merged
Conversation
Some hosts answer HEAD with an error for a page they serve fine on GET. support.google.com returns 404 to HEAD but 200 to GET, so every Google Analytics help link was reported as a broken external link on PRs. The retry now triggers on any 4xx or 5xx from HEAD instead of only 405, and the result of the GET decides the verdict. Genuinely missing pages still fail both methods and are still reported. Only failing links pay the extra request, and followRedirects discards the body once headers arrive, so the GET costs roughly what the HEAD did. Also covers the case where the GET redirects: the redirect suggestion now works for these hosts instead of being lost behind the HEAD error.
dvdstelt
changed the title
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.
Problem
Some hosts answer
HEADwith an error for a page they serve fine onGET.support.google.comis the clearest case: it returns 404 to HEAD and 200 to GET for the same URL.HyperHawk did HEAD first and only retried with GET on a
405, so a HEAD-only 404 went straight to the broken-link path and got posted as an inline review comment:Every Google Analytics help link in a PR was flagged this way, even though the pages are live.
Fix
checkExternalLinknow retries with GET on any4xx/5xxfrom HEAD instead of only405, and the GET result decides the verdict.Two side effects, both wanted:
Cost is bounded: only links that already look broken pay a second request, and
followRedirectscancels the response body as soon as headers arrive, so the GET costs about what the HEAD did.Verification
Against the real URLs:
support.google.com/analytics/answer/11109416support.google.com/analytics/answer/9143382support.google.com/analytics/answer/00000000(does not exist)Two new routes on the snapshot test server:
/head-404-get-200- 404 on HEAD, 200 on GET, expectedok/head-404-get-redirect- 404 on HEAD, 301 on GET, expected redirect suggestionThe pre-existing
/not-foundroute 404s on both methods and is still reported broken, so genuine breakage is not masked.npm run typecheckandnpm run test:snapshotpass.dist/is rebuilt and committed.Docs
docs/how-it-works.mdanddocs/troubleshooting.mdboth documented the old 405-only rule and are updated.Follow-up, not in this PR
Redirect suggestions can capture session parameters.
answer/9143382suggests a URL carrying?visit_id=...&rd=2, which is stale the moment it is applied. That weakness predates this change, but this PR makes it reachable for HEAD-hostile hosts, so it will surface more often. Stripping known session/tracking params inbuildRedirectUrlwould address it.