fix(har): do not report zero sizes for bodies that were never read - #42371
fix(har): do not report zero sizes for bodies that were never read#42371Ayaan Gazali (ayaangazali) wants to merge 1 commit into
Conversation
A redirect or basic-auth retry hop made through APIRequestContext is destroyed before its response body is read, so the body never reaches the tracer. It still recorded bodySize 0 and content.size 0, which claims the server sent nothing. The result contradicts itself: an entry can carry Content-Length 40 in its own headers and bodySize 0 next to it. HAR 1.2 uses -1 for a size that is not available, which is what these entries start as, and what the browser path already leaves them as when the body cannot be read. Only record the sizes when a body was actually captured. That makes the undefined-buffer branch of _storeResponseContent unreachable, so it goes away and the parameter stops being optional.
|
Ayaan Gazali (@ayaangazali) please read the following Contributor License Agreement(CLA). If you agree with the CLA, please reply with the following information.
Contributor License AgreementContribution License AgreementThis Contribution License Agreement (“Agreement”) is agreed to by the party signing below (“You”),
|
Test results for "tests 1"4 flaky51370 passed, 1238 skipped Merge workflow run. |
Test results for "MCP"1 failed 8141 passed, 1361 skipped Merge workflow run. |
|
Ayaan Gazali (@ayaangazali) you need to respond to #42371 (comment) before we can proceed |
|
Understood, and sorry for the wait. The CLA is not accepted yet. It needs the account owner to post the acceptance, and I have flagged it again on my side. I will not say it is done until I can point at a passing license/cla check, since I got that wrong once already on #42342 and would rather not waste your time twice. |
Rationale
A redirect or basic-auth retry hop made through
APIRequestContextis destroyed before its responsebody is read (
server/fetch.ts:496and:507), soevent.bodyisundefinedwhen the tracer seesit. The tracer turned that into a concrete zero:
The resulting entry contradicts itself. Recording a 302 that carries a 40 byte body produces:
Content-Length: 40sits in the entry's own headers next tobodySize: 0, which asserts the serversent nothing. HAR 1.2 uses
-1for a size that is not available, and both fields already start at-1.The browser path in this same file already does the right thing: it only calls
_storeResponseContentinsideresponse.internalBody().then(buffer => ...), so when the body cannotbe read the
-1survives. The two paths disagreed about the identical "body never captured" case.Recording the sizes only when a body was actually captured makes the undefined-buffer branch of
_storeResponseContentunreachable, so that branch is deleted and the parameter stops beingoptional. Net effect is fewer lines than before. An empty body is still recorded as
0, since anempty
Bufferis truthy and genuinely means zero bytes.Test
should include redirects from API requestalready existed but only asserted the URLs and thattimingsis defined, so nothing pinned the sizes. Two assertions go there rather than into a newtest. On current main they fail with
Expected: -1, Received: 0.Green:
har.spec.ts64 passed,global-fetch.spec.ts+browsercontext-fetch.spec.ts192 passed,flintclean.Fixes #42370
the judgement call I am least sure of is whether anyone depends on the current
0, given how longit has been that way. I checked it against the HAR 1.2 wording and against your own browser-side
handling rather than going on instinct, but you will know the compatibility risk better than I do.
freshman in college, happy to be told this is not worth changing :)