fix: bound Playwright requests so a lost or unanswered response cannot hang the suite - #241
Open
joshmanders wants to merge 2 commits into
Open
fix: bound Playwright requests so a lost or unanswered response cannot hang the suite#241joshmanders wants to merge 2 commits into
joshmanders wants to merge 2 commits into
Conversation
joshmanders
marked this pull request as ready for review
July 30, 2026 20:08
|
yeah i somehow also encounter this. |
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.
A browser suite currently has no failure mode for a request that is never answered — only a wedged process. No error, no timeout, no exit; CI runs until the job limit. This fixes three causes of that, all in
Playwright/Client.php.1. Send the timeout as protocol metadata. Playwright 1.62.0 (microsoft/playwright#41712) moved the timeout off each method's params onto the protocol-level
metadata, where an absent value means no timeout at all. The client still sent it only inparams, so every request to a 1.62.0 server was implicitly unbounded. That's why an action on a zero-match locator hangs forever there but fails cleanly on 1.61.1. Sending it in both places is backwards compatible and needs no version check.Fixes pestphp/pest#1781.
2. Treat a closed connection as an error.
fetch()coerced thenullthatreceive()returns on a closed connection into'', which decodes tonull— which has neither anerrorkey to throw on nor anidkey to break on. The loop then spins at ~100% CPU forever. This one isn't version specific; it reproduces identically on 1.61.1 and 1.62.0, and anything that drops the socket triggers it — a crashed browser, an OOM-killed server, a flaky runner.Fixes pestphp/pest#1801.
3. Bound the request as a whole. Insurance against future protocol drift. Both bounds are needed and each covers the other's blind spot: the cancellation stops a request the server never answers, where
receive()blocks forever; the deadline check stops one that keeps receiving unrelated messages, wherereceive()returns without ever suspending and the cancellation never gets scheduled. A per-message timeout bounds neither — messages keep arriving, they're just never the one being awaited.Verification
Stock Laravel 13 app, both Playwright versions. Reproduction repo: https://github.com/joshmanders/pest-plugin-browser-hang-repro
Timeout 5000ms exceeded10.8sTimeout 5000ms exceeded11.2sTimeout 5000ms exceeded10.6sThe zero-match rows take ~10s rather than the configured 5000ms because the action goes through the assertion retry loop first — that's pestphp/pest#1755, untouched here.
This repo's suite: 359 passed on 1.62.0 under
--parallel. Four unit tests added; I checked they fail without the fix rather than passing vacuously.Related, and deliberately not addressed
pestphp/pest#1759 reports a separate defect in this same loop — the
waitUntilearly-break stranding a command's response and desynchronizing later ones. I've left the break condition exactly as it was so the two changes don't collide; that issue's suggested fix applies cleanly on top of this one.Notes for review
requestGraceSecondsis 30s on top of the request timeout, deliberately generous so a slow-but-healthy operation on a loaded CI box doesn't start failing. It's a property rather than a constant only so the tests can shrink it — happy to make it a constant and test it another way.params.timeoutis kept alongside the newmetadata.timeoutso servers older than 1.62.0 keep working.Two things found while investigating, not fixed here
Happy to open issues or PRs for either.
waitForLoadState(),waitForFunction()andwaitForURL()are silent no-ops.execute()is a generator function, so its body doesn't run until iterated — these three call it and discard the result. Confirmed by logging every message written to the socket duringvisit('/')->assertNoJavascriptErrors(), which routes throughwaitForLoadState('load'): zerowaitFor*messages. Present on4.xtoo. This bears onwithinFramehangs on pages with external iframes (Stripe, etc) pest#1650 and #1625, which attribute thewithinFramehang towaitForLoadState('networkidle')never resolving — that call currently does nothing at all, so the hang must originate in thewaitFor(['state' => 'attached'])that follows it.ServerManager::playwright()hands parallel workersAlreadyStartedPlaywrightServer::fromPersisted(), which reads.temp/playwright-server.jsonwithout checking anything is still listening, andmarkAsStopped()only runs on a graceful stop — so a killed run leaves the file pointing at a dead port. Distinct from [Browser plugin] playwright run-server is never killed on Debian/Ubuntu (/bin/sh = dash): orphaned node process after every run, terminal hangs when output is piped pest#1754, which covers the orphaned process itself.