Skip to content

fix: drop response frames addressed to another request - #247

Open
bepsvpt wants to merge 1 commit into
pestphp:5.xfrom
bepsvpt:fix/drop-stale-response-frames
Open

fix: drop response frames addressed to another request#247
bepsvpt wants to merge 1 commit into
pestphp:5.xfrom
bepsvpt:fix/drop-stale-response-frames

Conversation

@bepsvpt

@bepsvpt bepsvpt commented Aug 10, 2026

Copy link
Copy Markdown

Summary

Client::execute() breaks its read loop when the waitUntil lifecycle event arrives:

if (
    (isset($response['id']) && $response['id'] === $requestId)
    || (isset($params['waitUntil']) && isset($response['params']['add']) && $params['waitUntil'] === $response['params']['add'])
) {
    break;
}

Page::goto() always sends waitUntil: 'load', and the load event usually arrives before goto's own response. So the goto response stays in the websocket buffer and is never read. The next execute() call reads it, and two problems happen:

  • Errors go to the wrong request. The error check throws on any error frame, without checking the frame's id. When afterEach closes the context while a goto is not settled yet, Playwright answers that goto late with a TargetClosedError. The next command that reads the socket gets this error. In practice, the next test's first browser operation fails with Target page, context or browser has been closed (and is marked risky, because it fails before any assertion). If nothing reads the socket until terminate(), Browser::close throws instead, and the run exits 1 even when every test passed. The failing test changes from run to run because it depends on timing.
  • Results go to the wrong request. A leftover success frame can be read as the next command's result ([browser plugin] Client::execute() waitUntil early-break strands responses and desynchronizes subsequent commands pest#1759).

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. The waitUntil break condition is not changed.

Fixes pestphp/pest#1759.

This change is compatible with #241, which fixes hang problems in the same loop and already mentions this issue: "that issue's suggested fix applies cleanly on top of this one".

Verification

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. Results on that suite:

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 (same approach as #241):

  • 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 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, xhigh).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[browser plugin] Client::execute() waitUntil early-break strands responses and desynchronizes subsequent commands

1 participant