Fix dev proxy websocket cleanup

[jfurneaux]

Summary

Fixes a Meteor dev proxy failure mode where abandoned websocket upgrades can block unrelated HTTP requests through meteor --port.

The dev proxy keeps a shared http.Agent({ maxSockets: 100 }) for proxied HTTP traffic. Websocket upgrades were using that same pooled agent, so enough client sockets closing during unresolved upstream upgrades could consume all agent slots. After that, normal HTTP requests through the proxy waited behind abandoned websocket proxy requests and the dev app appeared wedged.

This PR:

• keeps the shared agent for normal HTTP proxying
• opts websocket proxy requests out of the shared HTTP agent with agent: false
• tears down pending upstream websocket upgrade requests when the client socket closes first
• destroys paired websocket sockets when either side closes after upgrade
• destroys the shared proxy agent after the HTTP server has drained active connections
• adds a self-test that reproduces the blocked-HTTP behavior before the fix
• bumps meteor-tool from 3.4.1 to 3.4.2

Broken Before / Fixed After

Broken before, with the regression test applied to current devel behavior:

./meteor self-test "dev proxy releases abandoned websocket upgrades" --retries 0
✗ ... fail! (2232 ms)
=> Test threw exception: Error: Timed out waiting for HTTP response through proxy

Fixed after this PR:

./meteor self-test "dev proxy releases abandoned websocket upgrades" --retries 0
✓ ... ok! (224 ms)
All other tests passed.

Validation

./meteor self-test "dev proxy releases abandoned websocket upgrades" --retries 0
✓ ... ok! (224 ms)
All other tests passed.

./meteor self-test "server outputs port number on restarting" --retries 0
✓ ... ok! (30219 ms)
All other tests passed.

Summary by CodeRabbit

• Chores

  • Bumped command-line tool package version to 3.4.2.

• Bug Fixes

  • Improved proxy resource and websocket upgrade handling to prevent socket leaks and ensure clean shutdowns.

• Tests

  • Added a selftest validating proxy behavior for abandoned websocket-style upgrades and subsequent request recovery.

[netlify]

✅ Deploy Preview for v3-meteor-api-docs ready!

Name	Link
🔨 Latest commit	ddb0a65
🔍 Latest deploy log	https://app.netlify.com/projects/v3-meteor-api-docs/deploys/6a5a2d6de49c7a0008425cc0
😎 Deploy Preview	https://deploy-preview-14386.docs-online.meteor.com
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

---

To edit notification comments on pull requests, go to your Netlify project configuration.

[netlify]

✅ Deploy Preview for v3-migration-docs canceled.

Name	Link
🔨 Latest commit	6475a7c
🔍 Latest deploy log	https://app.netlify.com/projects/v3-migration-docs/deploys/6a0b60889231e3000800816c

[coderabbitai]

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

• @coderabbitai resume to resume automatic reviews.
• @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

• ▶️ Resume reviews
• 🔍 Trigger review

📝 Walkthrough

Walkthrough

Bumps the Meteor CLI package version; changes proxy runner to use a shared upstream HTTP agent with explicit destruction and improved websocket upgrade lifecycle handling; adds a selftest that exercises abandoned websocket-style upgrade connections.

Changes

Cohort / File(s)	Summary
Version Bump packages/meteor-tool/package.js	Package metadata version in Package.describe updated from '3.4.1' to '3.4.2'.
Proxy agent & websocket lifecycle tools/runners/run-proxy.js	Introduce a shared self.proxyAgent for upstream proxy connections and destroy it on stop (both startup-in-progress and normal shutdown); add proxyReqWs handler to track and destroy upstream websocket request/socket if client closes before upgrade; pass agent: false for per-connection websocket proxy calls.
Websocket upgrade selftest tools/tests/run-proxy.js	Add selftest that opens multiple raw websocket-style upgrade requests to the proxy, leaves upgrades unresolved, verifies target saw upgrade sockets, destroys client sockets to simulate abandoned upgrades, and asserts subsequent proxied HTTP requests succeed.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title 'Fix dev proxy websocket cleanup' directly and concisely summarizes the main focus of the pull request, which centers on fixing websocket cleanup issues in the dev proxy.
Docstring Coverage	✅ Passed	Docstring coverage is 90.00% which is sufficient. The required threshold is 80.00%.
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.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

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 and usage tips.}

[coderabbitai]

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

tools/runners/run-proxy.js (1)

195-201: ⚠️ Potential issue | 🟠 Major

Defer proxyAgent.destroy() until the server has drained active connections.

The comment at line 197 states that shutdown should allow existing connections to complete gracefully. However, calling self.proxyAgent.destroy() immediately (lines 200-201) contradicts this: http.Agent.destroy() terminates all sockets managed by the agent, including active ones. This will abruptly close in-flight proxied HTTP responses during a proxy stop.

Move the proxyAgent destruction to the server.close() callback (or defer it until after proxy work has actually drained) to let active requests complete.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@tools/runners/run-proxy.js` around lines 195 - 201, The shutdown currently
calls self.server.close() then immediately calls self.proxyAgent?.destroy(),
which force-closes all agent sockets and aborts in-flight proxied responses;
move the proxyAgent teardown into the server.close() callback (or equivalent
drain-complete handler) so that you call self.proxyAgent?.destroy() only after
the server has finished closing connections and active proxy work has drained;
update the code around self.server.close() to accept a callback/promise and in
that completion handler call self.proxyAgent?.destroy() and set self.proxyAgent
= null.

🧹 Nitpick comments (1)

tools/tests/run-proxy.js (1)

34-41: Avoid hardcoding the agent cap in this regression.

This reproduces the old starvation bug only while 100 stays in sync with tools/runners/run-proxy.js, Line 43. If maxSockets changes, the pre-fix behavior can stop failing here and the regression silently goes stale. Derive the count from proxy.proxyAgent.maxSockets or share a constant instead.

Possible change

     await proxy.start();
     proxy.setMode("proxy");

     var proxyPort = proxy.server.address().port;
-    for (var i = 0; i < 100; i++) {
+    var upgradeCount = proxy.proxyAgent.maxSockets;
+    for (var i = 0; i < upgradeCount; i++) {
       clientSockets.push(await openWebsocketUpgrade(proxyPort));
     }

     await waitUntil(function () {
-      return targetSockets.length === 100;
+      return targetSockets.length === upgradeCount;
     });

As per coding guidelines, tools/**: "Be thorough about correctness, edge cases, and performance in the CLI/build pipeline."

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@tools/tests/run-proxy.js` around lines 34 - 41, The test hardcodes 100 for
the number of client sockets which can get out of sync with the agent cap;
replace the literal 100 used in the loop that calls
openWebsocketUpgrade(proxy.server.address().port) and in the waitUntil check
against targetSockets.length with a derived value (e.g. let expected =
proxy.proxyAgent.maxSockets or a shared constant) and use expected for both
pushing to clientSockets and the waitUntil comparison so the test always matches
the actual proxy.proxyAgent.maxSockets cap (ensure a sensible fallback if
maxSockets can be undefined).

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Outside diff comments:
In `@tools/runners/run-proxy.js`:
- Around line 195-201: The shutdown currently calls self.server.close() then
immediately calls self.proxyAgent?.destroy(), which force-closes all agent
sockets and aborts in-flight proxied responses; move the proxyAgent teardown
into the server.close() callback (or equivalent drain-complete handler) so that
you call self.proxyAgent?.destroy() only after the server has finished closing
connections and active proxy work has drained; update the code around
self.server.close() to accept a callback/promise and in that completion handler
call self.proxyAgent?.destroy() and set self.proxyAgent = null.

---

Nitpick comments:
In `@tools/tests/run-proxy.js`:
- Around line 34-41: The test hardcodes 100 for the number of client sockets
which can get out of sync with the agent cap; replace the literal 100 used in
the loop that calls openWebsocketUpgrade(proxy.server.address().port) and in the
waitUntil check against targetSockets.length with a derived value (e.g. let
expected = proxy.proxyAgent.maxSockets or a shared constant) and use expected
for both pushing to clientSockets and the waitUntil comparison so the test
always matches the actual proxy.proxyAgent.maxSockets cap (ensure a sensible
fallback if maxSockets can be undefined).

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: b0b8a3e8-bf90-4afc-8ab0-4e4ada8695f1

📥 Commits

Reviewing files that changed from the base of the PR and between 280df59 and 21f8bee.

📒 Files selected for processing (3)

• packages/meteor-tool/package.js
• tools/runners/run-proxy.js
• tools/tests/run-proxy.js

[coderabbitai]

Actionable comments posted: 1

🧹 Nitpick comments (1)

tools/tests/run-proxy.js (1)

51-60: Wait for the server shutdown to finish.

target.close() is asynchronous here, but the finally block does not wait for it. That can leave the listener alive long enough to leak handles into later selftests.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@tools/tests/run-proxy.js` around lines 51 - 60, The finally block destroys
sockets but calls target.close() (and proxy.stop() if it is async) without
awaiting their completion, which can leave the listener alive; change the
cleanup to wait for shutdown by converting target.close() (and proxy.stop() if
it returns/accepts a callback) into a Promise and await it (e.g., wrap
target.close in a Promise that resolves/rejects in its callback) before
finishing the finally block, while still destroying clientSockets and
targetSockets first to ensure they’re closed.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@tools/tests/run-proxy.js`:
- Around line 44-60: Replace the fixed sleep(100) with a deterministic wait for
socket teardown: after calling clientSockets.forEach(socket.destroy) await a
Promise.all that listens for each socket's 'close' (or 'end'/'error') event (use
a helper that resolves per socket) and only then call await
expectHttpResponse(proxyPort, 'ok'); likewise ensure the finally block waits for
Promise.all over clientSockets and targetSockets close events before invoking
proxy.stop() and target.close() so teardown is not timing-dependent (reference
clientSockets, targetSockets, sleep, expectHttpResponse, proxy.stop,
target.close).

---

Nitpick comments:
In `@tools/tests/run-proxy.js`:
- Around line 51-60: The finally block destroys sockets but calls target.close()
(and proxy.stop() if it is async) without awaiting their completion, which can
leave the listener alive; change the cleanup to wait for shutdown by converting
target.close() (and proxy.stop() if it returns/accepts a callback) into a
Promise and await it (e.g., wrap target.close in a Promise that resolves/rejects
in its callback) before finishing the finally block, while still destroying
clientSockets and targetSockets first to ensure they’re closed.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: b9b6a3bc-172d-401b-a9f1-a06a7e2f60e6

📥 Commits

Reviewing files that changed from the base of the PR and between 21f8bee and f577955.

📒 Files selected for processing (2)

• tools/runners/run-proxy.js
• tools/tests/run-proxy.js

🚧 Files skipped from review as they are similar to previous changes (1)

• tools/runners/run-proxy.js

[CLAassistant]

All committers have signed the CLA.

[coderabbitai]

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@tools/tests/run-proxy.js`:
- Around line 116-123: The waitForSocketToClose function can hang when called on
sockets already destroyed; update it to check socket.destroyed in addition to
socket.closed and immediately resolve if either is true. Locate
waitForSocketToClose and change the initial guard from checking socket.closed to
checking (socket.closed || socket.destroyed) so the function does not attach a
'close' listener to an already-destroyed socket.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 2fa69752-608b-42cf-8800-c5643bf54cec

📥 Commits

Reviewing files that changed from the base of the PR and between f577955 and 125ff2a.

📒 Files selected for processing (1)

• tools/tests/run-proxy.js

[italojs]

please, do not update this pkg version

[italojs]

use let/const please

[italojs]

@jfurneaux can you please fix the conflicts? this PR is prettry good, i'm just waitting the comments be addressed so we can merge it into 3.5.1