fix: uploads, bodiless responses, late responses and attempt timeouts under load

[itscark]

Five defects, found while moving a Laravel Nova suite (≈370 browser tests) to 5.x. The first three come with a test that is red on 5.x and green with the fix, the last two are behaviour the existing suite covers; all 381 tests, pint, rector and phpstan stay green.

1. Uploaded files arrive corrupted

parseMultipartBody() runs every part through mb_ltrim($part, "\r\n"). mb_ltrim() replaces each byte that is not valid UTF-8 with ?, and a file part is binary:

bin2hex(mb_ltrim("\r\n\x89PNG\r\n\x1a\n\x00\xff\xfe\x80", "\r\n"));
// 3f504e470d0a1a0a003f3f3f  — the PNG signature is gone, so is every byte >= 0x80

Any image upload therefore fails Laravel's image rule on the way in ("The logo field must be an image"), while a text file passes, which is why the existing UploadTest did not notice. The leading CRLF is now stripped with a byte regex (pint's mb_str_functions rule would rewrite a plain ltrim() straight back). New test: it keeps the bytes of an uploaded file that are not valid UTF-8.

2. A 204 or 304 is framed chunked, and Chromium reads the terminator as the next response

handleRequest() builds new Response($status, $headers, $content). Amp's constructor calls setBody() first, which sets Content-Length from the string, and then setHeaders(), which replaces every header with the given ones — so the length is dropped again and amphp/http-server falls back to Transfer-Encoding: chunked. For a response without a body that means it writes the headers and then 0\r\n\r\n. Chromium treats those five bytes as the start of the next response on the keep-alive socket; when they arrive after it has already reused the socket, that request fails with net::ERR_INVALID_HTTP_RESPONSE and whatever it asked for never loads. Under CPU load (CI) this is a random blank page: in our suite the resource that lost was Nova's core bundle, with createNovaApp is not defined as the only trace, and a later click on the dead page hung the worker. The organic trigger is the 304 the browser gets when it revalidates a cached script or stylesheet, plus any 204 the application answers.

The response is now built in the order the constructor should use: status, headers, then body, so every string body carries its Content-Length. New test Visit/BodilessResponseTest reads the raw bytes off the socket for a 204, a 304 and a normal body.

3. A late response of an earlier request is taken for the current one

With the action timeout reaching Playwright again (5.0), calls can now time out — and some of them time out after their consumer has stopped reading: Client::execute() returns from a goto as soon as the waitUntil state arrives, and querySelector returns on the handle's __create__ event, both before the request's own response. Those responses were always left in the socket; while they were successes that nobody read, nothing happened. On a busy runner the late response is an error — "Timeout 1000ms exceeded" for a navigation that had long reached load — and the client handed it to whatever request was in flight, because it threw on any error message and processResultResponse() returned the first result.value it saw.

Wire capture from a 367-test Laravel suite run next to ten CPU hogs (every request logged with its id):

SEND 6ac13231f329c goto      timeout=1000  /nova/dashboards/statistics
SEND 6ac13232435a3 click     timeout=15000 [dusk="year-over-year-chart-years-selected"]
RECV for=6ac13232435a3 id=6ac13231f329c  error="Timeout 1000ms exceeded."   ← the goto's, thrown for the click

And the second shape: the request that was wrongly failed never has its own answer read either, so the next call gets it — Page::javaScriptErrors(): Return value must be of type array, true returned was assertNoJavaScriptErrors() receiving an isVisible result. 363 stale value responses and 22 stale errors in that one run.

Client::execute() now passes over any message whose id is another request's; events (no id) and the request's own response are handed on as before. Unit-tested with a scripted connection (tests/Unit/Playwright/ClientTest): a late error and a late result of an earlier request are ignored, the request's own error is thrown, events still come through. Both "late" tests are red on 5.x.

4. The attempt timeout no longer stays at one second whatever the configured timeout is

Execution::waitForExpectation() capped every attempt at usingTimeout(1_000, …). That cap dates from when the whole timeout was one second; the overall timeout became configurable later and the attempt did not follow, so on a runner where a single round trip takes longer than a second every attempt fails the same way until the budget is gone. An attempt is now a fifth of the configured timeout, at least one second — unchanged for the 5 s default and for this suite's 2 s, and a project that sets ->timeout(15_000) for its CI gets 3 s attempts.

5. Navigations are not retried

navigate, refresh, back and forward join click & co. as non-awaitable: a repeated goto restarts the page load the previous attempt was waiting for, so under load none of the attempts ever finishes (the capture above shows five one-second navigations in a row, each aborting the last). Playwright already waits for the load state itself, with the full timeout.

Same 5.x suite before/after: 381 passed, pint, rector and phpstan clean.

🤖 Generated with Claude Code