[miniflare] Close all open handles on dispose to prevent process hangs#13515
[miniflare] Close all open handles on dispose to prevent process hangs#13515petebacondarwin merged 14 commits intomainfrom
Conversation
🦋 Changeset detectedLatest commit: e82c46d The changes in this PR will be included in the next version bump. Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
Codeowners approval required for this PR:
Show detailed file reviewers |
Changeset ReviewFiles Reviewed
Summary✅ All changesets look good Review Details
|
| Criteria | Status |
|---|---|
| Version Type | ✅ patch appropriate for bug fix |
| Changelog Quality | ✅ Clear description of the cleanup issue with specific resources listed |
| Markdown Headers | ✅ No h1/h2/h3 headers |
| Analytics | ✅ N/A - no analytics changes |
| Dependabot | ✅ N/A |
| Experimental Features | ✅ N/A |
fix-esbuild-cleanup-race.md
| Criteria | Status |
|---|---|
| Version Type | ✅ patch appropriate for bug fix |
| Changelog Quality | ✅ Clear explanation of the race condition and fix |
| Markdown Headers | ✅ No h1/h2/h3 headers |
| Analytics | ✅ N/A - no analytics changes |
| Dependabot | ✅ N/A |
| Experimental Features | ✅ N/A |
|
The function is async, so Both changes are sound:
The changeset is correctly formatted as a LGTM |
create-cloudflare
@cloudflare/kv-asset-handler
miniflare
@cloudflare/pages-shared
@cloudflare/unenv-preset
@cloudflare/vite-plugin
@cloudflare/vitest-pool-workers
@cloudflare/workers-editor-shared
wrangler
commit: |
workers-devprod
left a comment
There was a problem hiding this comment.
Codeowners reviews satisfied
68b4b1e to
e67dd24
Compare
|
Thanks for review @jamesopstad but I think there is something bad going on here - switched to draft for now |
8d05cc9 to
ba7c019
Compare
89950d7 to
71237ce
Compare
Force a code-owners-plus review
- Fix esbuild cleanup race in BundlerController: await build promise before calling stopWatching so esbuild context is always disposed - Fix HyperdriveProxyController.dispose(): add missing return in .map() callback so net.Server close is actually awaited - Close WebSocketServer instances (#liveReloadServer, #webSocketServer) in Miniflare.dispose() so connected client sockets are cleaned up - Close InspectorProxy runtime WebSocket on dispose instead of relying on process death to break the connection - Clear ProxyClientBridge finalizeBatch setTimeout on dispose
…ispose Fold in fix from PR #13523: call server.closeAllConnections() before server.close() so active HTTP keep-alive or WebSocket connections don't prevent the close callback from firing.
Co-authored-by: devin-ai-integration[bot] <158243242+devin-ai-integration[bot]@users.noreply.github.com>
The previous approach awaited buildPromise in the cleanup function, which could block the entire DevEnv teardown if the initial build was still in flight. Instead, stop esbuild immediately if the build has completed, or fire-and-forget the cleanup chain if it hasn't.
Use persist: false to avoid SQLite SQLITE_BUSY_RECOVERY errors when back-to-back test runs contend on the same .wrangler/state directory. These tests don't need persistence between runs.
net.Server.close() waits for all existing connections to end before calling its callback. For TCP proxy servers with lingering sockets (e.g. from in-progress TLS negotiation), this blocks dispose indefinitely. Just call close() to stop accepting new connections without awaiting the callback.
… IPC channel
The third-party exit-hook package registers a permanent
process.on('message') listener that is never removed. In Node.js,
a 'message' listener refs the IPC channel handle, preventing child
processes (e.g. node --test runners) from exiting after tests complete.
Replace it with a lightweight inline module that removes all process
listeners when the last callback is unregistered, allowing clean exit.
Also close #devRegistryDispatcher (undici Pool) during dispose —
same class of leaked-socket bug already fixed for #runtimeDispatcher.
During config updates, the previous #devRegistryDispatcher Pool was silently overwritten without being closed, leaking TCP sockets. This mirrors the existing cleanup for #runtimeDispatcher at the same call site.
The 8s timeout was marginally too tight for Windows CI where workerd process teardown and TCP socket cleanup can take ~7s. Bumping to 15s gives sufficient headroom while still catching genuine hangs.
b31f554 to
1311255
Compare
jamesopstad
left a comment
There was a problem hiding this comment.
Approving but posted a couple of non-blocking comments from the Devin review that could be addressed here or as follow ups.
workers-devprod
left a comment
There was a problem hiding this comment.
Codeowners reviews satisfied
Supersedes #13520, #13521, #13522, #13523.
Problem
The
start-worker-node-testfixture (which usesnode --testto exercise theunstable_startWorkerAPI) was hanging indefinitely in CI. Unlike vitest-based tests,node --testruns each test file as a child process and waits for it to exit naturally. If any event-loop handles remain open after tests complete, the process never exits and CI hangs forever.The root cause was not a single leak but a cascade of unclosed resources across
Miniflare.dispose()and the wranglerDevEnvteardown path. Each fix in this PR addresses a distinct handle that could keep the Node.js event loop alive after disposal.Changes
miniflare (patch)
Undici Pool leaks:
#runtimeDispatcher(undiciPool) indispose()after the runtime is killed. Without this, lingering TCP sockets in the pool keep the event loop alive indefinitely.#runtimeDispatcherPool when the runtime entry URL changes during config updates, preventing socket leaks on everysetOptions()call.#devRegistryDispatcherPool indispose()— same class of leak as#runtimeDispatcher.#devRegistryDispatcherPool before replacement during config updates — without this, every call to#assembleAndUpdateConfig()leaked the previous Pool's TCP sockets.WebSocket and HTTP server leaks:
WebSocketServerinstances (#liveReloadServer,#webSocketServer) indispose()so connected client sockets don't keep the event loop alive. These usenoServer: trueso they don't own an HTTP server, but connected WebSocket clients still hold open sockets.InspectorProxyController.dispose()by callingserver.closeAllConnections()beforeserver.close(), so active keep-alive/WebSocket connections don't prevent the close callback from firing.InspectorProxyruntime WebSocket on dispose instead of relying on process death to break the connection.Hyperdrive proxy:
HyperdriveProxyController.dispose(): add missingreturnin.map()callback soPromise.allSettledactually waits fornet.Serverinstances to close.net.Server.close()callback — TCP proxy servers with lingering sockets (e.g. from in-progress TLS negotiation) would block indefinitely. Just callclose()to stop accepting new connections without awaiting the callback.IPC channel / exit-hook:
exit-hookpackage with a lightweight inline implementation that removes all process listeners when the last callback is unregistered. The original package registers a permanentprocess.on('message')listener that refs the IPC channel handle, preventingnode --testchild processes from exiting after tests complete.Miscellaneous:
ProxyClientBridgefinalization batchsetTimeouton dispose.wrangler (patch)
BundlerController: the cleanup function returned byrunBuild()now stops esbuild immediately if the build has completed, or fire-and-forget the cleanup chain if it hasn't. The previous approach awaitedbuildPromisein the cleanup function, which could block the entireDevEnvteardown if the initial build was still in flight.Fixture changes
persist: false) instart-worker-node-testto avoidSQLITE_BUSY_RECOVERYerrors when back-to-back test runs contend on the same.wrangler/statedirectory.--test-timeout=15000on the fixture to fail fast on genuine hangs while giving sufficient headroom for Windows CI, where workerd process teardown and TCP socket cleanup can take ~7s.Verification
start-worker-node-testpassed without hangingminiflare-node-testfixture: 6/6 passedstart-worker-node-testandminiflare-node-testfixtures, plus full miniflare test suite