fix(retry): skip the content-length checkpoint for HEAD and for a 206 without content-range - #5610
Open
marko1olo wants to merge 2 commits into
Open
fix(retry): skip the content-length checkpoint for HEAD and for a 206 without content-range#5610marko1olo wants to merge 2 commits into
marko1olo wants to merge 2 commits into
Conversation
onResponseStart derives the resume checkpoint from content-length without
looking at the method, so a HEAD gets this.end = content-length - 1 while
its body is legitimately empty. onResponseEnd then requires
this.start === this.end + 1 and throws RequestRetryError('Content-Range
mismatch'); 200 is not a retryable status, so the request fails outright.
A HEAD has no body to checkpoint, so leave this.end null.
Signed-off-by: marko1olo <marko1olo@users.noreply.github.com>
parseRangeHeader returns { start: 0, end: null, size: null } for an absent
or empty value and only returns null when a value is present and fails the
regex, so the `range == null` guard in the first-206 branch never fires for
a response that has no content-range at all. Execution falls through to
`assert(end != null && Number.isFinite(end), 'invalid content-length')`,
which throws AssertionError [ERR_ASSERTION] out of onResponseStart before
any data reaches the handler.
RFC 9110 15.3.7.2 requires a server to omit the top-level content-range on
a multipart/byteranges response, since each part carries its own, so a
compliant server answering a multi-range request trips this.
A content-range that matches the regex always yields a numeric end, so
widening the guard to range.end == null only adds the absent and empty
cases. Neither can produce a resume checkpoint, so both take the existing
pass-through and the response is forwarded untouched.
Signed-off-by: marko1olo <marko1olo@users.noreply.github.com>
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #5610 +/- ##
==========================================
- Coverage 93.50% 93.49% -0.02%
==========================================
Files 110 110
Lines 38303 38303
==========================================
- Hits 35816 35812 -4
- Misses 2487 2491 +4 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
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
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.
This relates to...
Two defects in
RetryHandler's byte-range checkpoint logic. No issue filed for either.They are in one PR because they are the same three lines of logic and the same test file, and splitting them would mean two PRs touching the same block for the same reason. Each is a separate commit, so they can be taken apart if you would rather.
Rationale
1. A HEAD response with
Content-Lengthbreaks the retry interceptor.onResponseStartestablishes a resume checkpoint from the responseContent-Lengthwithout looking at the request method:A HEAD response legitimately carries
Content-Lengthdescribing the body a GET would return, while delivering no body at all. So the handler expectscontentLengthbytes, receives none, and the request fails withRequestRetryError: Content-Range mismatch. Any HEAD throughinterceptors.retry()orRetryAgentagainst a server that setsContent-Lengthon HEAD — which is normal — cannot succeed.2. A spec-compliant
multipart/byteranges206 trips an assertion.For a multi-range request, RFC 9110 §15.3.7.2 says the 206 carries
Content-Type: multipart/byterangesand no top-levelContent-Range; the ranges live in the part headers. The first-206 path handles that case deliberately — it forwards the response unchanged whenparseRangeHeaderreturns null — butparseRangeHeadernever returns null for an absent header, so the guard is unreachable and the request dies onAssertionError [ERR_ASSERTION]instead.An assertion failure is the wrong shape for "the server answered in a way this handler does not resume": it reads as an undici bug to the caller, and it is not recoverable.
Changes
lib/handler/retry-handler.js, two conditions:Content-Lengthcheckpoint when the request method is HEAD;test/interceptors/retry.jsgains a case for each. Both fail on currentmain.npm run test:interceptorsis 294 passing / 0 failing here, andnpx eslintis clean on both files.Features
Bug Fixes
Status