test: stabilize allowed hosts proxy cleanup

[LingyuCoder]

Summary

• Add a proxy server test helper that tracks upgraded sockets and waits for proxy shutdown.
• Use the helper in allowed-hosts e2e tests so browser, proxy, and dev-server teardown happens in a deterministic order.
• Add a focused regression test proving upgraded sockets are destroyed before the proxy server is considered closed.

Root cause

The allowed-hosts e2e cases create a new proxy and dev-server on fixed ports for each case. Some cases intentionally disconnect WebSocket clients. The old cleanup called proxy.close() without waiting for completion and without accounting for upgraded WebSocket sockets, allowing teardown from one case to race the next case under CI load.

Validation

• pnpm exec prettier --check tests/e2e/allowed-hosts.test.js tests/helpers/proxy-server.js tests/proxy-server.test.ts
• pnpm run lint
• pnpm run test:type
• CI=1 pnpm exec rstest tests/proxy-server.test.ts tests/e2e/allowed-hosts.test.js --reporter=default

[Copilot]

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 0bd4c73fe2

ℹ️ 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".

[coderabbitai]

📝 Walkthrough

Walkthrough

A new helper module adds listenProxyServer, trackProxySocket, and closeProxyServer for proxy server lifecycle management and upgraded socket cleanup. A new test file verifies shutdown behavior with an active upgrade and a manually tracked socket. The allowed-hosts e2e tests now use the shared proxy middleware and server helpers, and their cleanup order closes the browser before shutting down the proxy.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Title check	✅ Passed	It clearly summarizes the main change: stabilizing allowed-hosts proxy cleanup.
Description check	✅ Passed	It describes the proxy helper, e2e updates, root cause, and validation, all matching the changeset.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch seal/test-proxy-lifecycle

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[coderabbitai]

🧹 Nitpick comments (2)

tests/helpers/proxy-server.js (1)

30-55: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win

Guard against an undefined proxy argument.

If listenProxyServer throws before its result is assigned (e.g., app.listen errors), a caller's finally block invoking closeProxyServer(proxy)/closeServer(proxy) with proxy still undefined will throw a TypeError (Cannot read properties of undefined), masking the original error and potentially crashing test teardown instead of failing gracefully.

🛡️ Proposed guard

 async function closeProxyServer(proxy) {
+  if (!proxy) {
+    return;
+  }
+
   const proxySockets = proxy[kProxySockets];

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@tests/helpers/proxy-server.js` around lines 30 - 55, Guard closeProxyServer
and closeServer against an undefined proxy value so teardown does not throw a
TypeError when listenProxyServer fails before assignment. Update
closeProxyServer to no-op or resolve immediately when proxy is falsy, and make
closeServer safely handle a missing proxy before calling proxy.close. Use the
existing closeProxyServer and closeServer helpers as the fix points.

tests/e2e/allowed-hosts.test.js (1)

55-70: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoff

Consider extracting the repeated startProxy boilerplate into a shared helper.

The startProxy function body (app + createTrackedProxyMiddleware + listenProxyServer) is duplicated near-identically across ~20 test cases, differing only in the proxy options passed. A shared factory (e.g., createStartProxy(getProxyOptions)) could reduce duplication, though each case's inline options currently make individual tests self-contained and easy to scan independently. Optional — not blocking.

Also applies to: 127-142, 200-215, 274-289

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@tests/e2e/allowed-hosts.test.js` around lines 55 - 70, The repeated
startProxy setup in the allowed-hosts tests is duplicated across multiple cases
and should be centralized. Extract the shared app creation,
createTrackedProxyMiddleware wiring, and listenProxyServer call into a reusable
helper such as createStartProxy(getProxyOptions), then have each startProxy
wrapper pass only its per-test proxy options. Keep the existing startProxy
behavior intact while reducing boilerplate across the duplicated blocks.

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Nitpick comments:
In `@tests/e2e/allowed-hosts.test.js`:
- Around line 55-70: The repeated startProxy setup in the allowed-hosts tests is
duplicated across multiple cases and should be centralized. Extract the shared
app creation, createTrackedProxyMiddleware wiring, and listenProxyServer call
into a reusable helper such as createStartProxy(getProxyOptions), then have each
startProxy wrapper pass only its per-test proxy options. Keep the existing
startProxy behavior intact while reducing boilerplate across the duplicated
blocks.

In `@tests/helpers/proxy-server.js`:
- Around line 30-55: Guard closeProxyServer and closeServer against an undefined
proxy value so teardown does not throw a TypeError when listenProxyServer fails
before assignment. Update closeProxyServer to no-op or resolve immediately when
proxy is falsy, and make closeServer safely handle a missing proxy before
calling proxy.close. Use the existing closeProxyServer and closeServer helpers
as the fix points.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 5806be51-661d-40e8-81d0-f8812e1ecea0

📥 Commits

Reviewing files that changed from the base of the PR and between 0bd4c73 and 99371dd.

📒 Files selected for processing (3)

• tests/e2e/allowed-hosts.test.js
• tests/helpers/proxy-server.js
• tests/proxy-server.test.ts