[bepsvpt]

Summary

Client::execute() processes every frame it reads, including response frames whose id belongs to another request. A response stays in the websocket buffer when a caller stops reading the generator before the response arrives. The next execute() call then reads it.

#266 removed the main source of these frames: the waitUntil early break that left goto() responses unread (pestphp/pest#1759). Two callers still return early, on the __create__ event that arrives before the response:

• InteractsWithPlaywright::processElementCreationResponse()
• Page::evaluateHandle(), when the result is a JSHandle

The error check and the callers' result checks do not compare the frame's id with the current request. So a stranded frame can give its error or its result to an unrelated command.

This PR skips frames whose id belongs to another request, before the error check. Event frames have no id, so they are not affected. Error frames for the current request still throw.

Refs pestphp/pest#1759. #266 removed its root cause. This PR adds the id check that the issue also asks for.

Verification

These runs used v5.0.0, before #266. On the current base, goto() no longer leaves its response unread, so the table shows the original failure, not the current one.

I logged every sendText / receive with request ids on a real Laravel browser suite (12 tests). Every stale TargetClosedError frame came from an abandoned goto.

Client	Runs	Result
v5.0.0 as shipped	7	6 failed — one victim per run, across 6 different tests; one run passed 12/12 but exited 1
this PR	6	all 12/12 passed, exit 0

Tests

Four unit tests for Client::execute(), using a stubbed WebsocketConnection:

• drops a response addressed to another request
• does not fail the current request on an error frame addressed to another one
• still fails the current request on its own error frame
• still yields event frames, which carry no id

The first two fail on the current 5.x without the src/ change. The last two make sure existing behavior stays the same.

AI assistance

Root-cause tracing, fix, and tests: Claude Code (claude-fable-5 + claude-opus-5-5, xhigh).

[Michael-Brooks]

I'm also running into this same issue. I hope this PR gets reviewed soon

[babul]

I am also running into this issue.

[cyppe]

Same. Feels like this plugin is a bit forgotten. But hope someone will be allowed to help nuno with through PRs to keep the momentum of this awesome plugin going.

[likemusic]

Independent confirmation of the symptom, on a suite unrelated to this repository.

The Target page, context or browser has been closed failure described here reproduces on a third-party set of ~130 browser tests running against v5.0.1: it strikes before any assertion has run, and the name of the failing test moves from run to run. That matches your description exactly — a late error frame from one request being read by the next one, so the test that reports it is whichever happened to read the socket next.

One observation that may help whoever reviews this: the failing-test-name drift makes the bug easy to misfile. On that suite it had been sitting in a quarantine group, understood as "flaky application code", because nothing about the symptom points at the transport — the test fails before it asserts anything, so there is no assertion to blame, and re-running moves the failure elsewhere.

A second, unrelated thing that makes this class of bug slower to diagnose than it should be: the original exception does not survive a browser failure. BrowserExpectationFailedException::from() returns a new ExpectationFailedException and passes $e->getPrevious() as its third argument rather than $e, so the exception that actually came out of Playwright — and its stack — is dropped. Measured on 5.x: getPrevious() on what reaches the test is NULL. The reported line is fine (PHPUnit filters the plugin's frames and lands on the call that threw); what is missing is the original error underneath it. I will open that as its own PR — mentioning it here only because it is part of why a transport-level failure reads as an application one.

Nothing to add on the fix itself — skipping frames whose id belongs to another request, before the error check, is the right shape.

(Edited: an earlier version of this comment claimed the reported line need not be the throwing call. That was wrong — I had measured it inside a checkout of the plugin itself, where its frames are not filtered as vendor code. In a normal project the line is correct; the loss is the original exception, not the location.)

[likemusic]

Follow-up, on how this sits next to #266 — both change the same loop, so I applied them together on top of a78ede5.

They apply cleanly in either order and do not conflict. Combined, the loop skips frames whose id belongs to another request (this PR) and breaks only on its own response, with the waitUntil shortcut gone (#266) — the same rule from both ends rather than two competing ones.

Measured on PHP 8.4.24 / Playwright 1.62.1 / Chromium, Linux:

• all 8 tests from both PRs pass together (19 assertions);
• with the source changes reverted and only the test files kept, 5 of 7 fail — so both sets of unit tests genuinely pin the behaviour;
• full suite with both applied: 347 passed, 27 skipped, 1 failed, and that one failure is an external-URL test that cannot reach example.com through my proxy. Nothing regressed.

I also spent a while trying to reproduce the stranded-frame symptom at the browser level — a test that deliberately leaves a navigation unsettled when it ends, followed by a clean test — and could not get it to fail on demand. The unit stub here is the reliable formulation.

[theseekeroftrue]

Independent confirmation from a project running ~1,500 browser tests in parallel (6 processes):

• Symptom: assertNoJavaScriptErrors() fails with TypeError: Pest\Browser\Playwright\Page::javaScriptErrors(): Return value must be of type array, true returned. In every case the call just before it was an assertScript(...) whose result was true, so javaScriptErrors() read the previous request's response. Related: #1910 / #1759 in pestphp/pest.
• Frequency: about 10 such failures in two weeks of CI runs, more often when the machine is loaded.
• Still present in v5.1.2 (Client::execute() unchanged from v5.0.1).

Correction to an earlier version of this comment: we applied exactly this change locally (skip frames whose id is not the current $requestId). A unit test like the one here passes with it. But the full browser suite did not stay green: two patched runs had 2 and 5 failures out of 1,511 (new symptoms: net::ERR_ABORTED on goto, Timeout 5000ms exceeded, a missed selection). The unpatched run on the same machine passed all 1,511. So skipping the frames alone looks incomplete. One likely reason: goto() still returns on its waitUntil event, before its own response arrives. #266 (await the matching navigation response) may be the missing half. We reverted the local patch for now.

[likemusic]

@theseekeroftrue Your diagnosis matches what I measured earlier: skipping stale frames (this PR) and awaiting the matching navigation response (#266) are two halves of one rule, not competing fixes. The regressions you saw with this PR applied — net::ERR_ABORTED on goto, stray timeouts, a missed selection — are what an unawaited goto leaves behind: it returned on its waitUntil event before its own response arrived, so a frame was still stranded for the next request to misread. You applied the id check on a base that didn't yet have #266.

#266 merged into 5.x on 26 Oct, after your comment. On a base that now awaits the navigation response, this PR stops competing with it and complements it instead. Re-verified on current 5.x (2b5c7b5, with #266) + this PR: the four ClientTest units pass (14 assertions), and the full browser suite is 275 passed, 1 skipped, 0 failed (574 assertions). As before, I couldn't force your 1,511-test symptom on demand, so this is the plugin's own suite rather than a replica of yours — but the failure class you hit is tied to the goto early-return that #266 has now closed.

(The lost-original-exception aside from my earlier comment is resolved separately — #268, merged 24 Sep.)