fix(miniflare): disable keep-alive timeout on the loopback server - #14850
fix(miniflare): disable keep-alive timeout on the loopback server#14850exKAZUu wants to merge 4 commits into
Conversation
workerd pools and reuses connections to the loopback server, and Node's default keepAliveTimeout (5s) races with that reuse: Node closes an idle pooled socket just as workerd sends the next request on it, which fails inside the Worker with "Network connection lost". Disable the idle timeouts, mirroring the undici dispatch pools in the opposite direction. Fixes cloudflare#14848
🦋 Changeset detectedLatest commit: 5a5103e The changes in this PR will be included in the next version bump. This PR includes changesets to release 8 packages
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 |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
NuroDev
left a comment
NuroDev
left a comment
There was a problem hiding this comment.
Could we add a deterministic regression test for this?
Also, is headersTimeout = 0 necessary? That controls how long clients may take to send request headers & is unrelated to idle keep-alive connections.
…ession test headersTimeout only limits how long a client may take to send request headers; it does not apply to idle keep-alive sockets, so it is not needed for this fix. Verified empirically: with keepAliveTimeout = 0 and a short headersTimeout, an idle keep-alive socket survives past the headersTimeout and still serves a second request. The new test talks to the loopback server over a raw keep-alive socket, idles past Node's default keepAliveTimeout of 5 seconds, and asserts a second request still succeeds. Without the fix the socket is closed deterministically at ~5s and the test fails.
|
@NuroDev Thanks for the review — both points addressed. Regression test: Added a deterministic one in headersTimeout: You're right — it limits how long a client may take to send request headers and doesn't govern idle keep-alive sockets, so it isn't needed for this fix. I confirmed empirically that with |
@cloudflare/autoconfig
@cloudflare/config
create-cloudflare
@cloudflare/deploy-helpers
@cloudflare/kv-asset-handler
miniflare
@cloudflare/pages-shared
@cloudflare/unenv-preset
@cloudflare/vite-plugin
@cloudflare/vitest-pool-workers
@cloudflare/workers-auth
@cloudflare/workers-editor-shared
@cloudflare/workers-utils
wrangler
commit: |
| script: `addEventListener("fetch", (event) => { | ||
| event.respondWith(new Response("<p>👋</p>", { | ||
| headers: { "Content-Type": "text/html;charset=utf-8" } | ||
| })); | ||
| })`, |
There was a problem hiding this comment.
Nit pick: We could probably make this a module script instead of a service worker. EG: export default { fetch: () => {...} };
workers-devprod
left a comment
workers-devprod
left a comment
There was a problem hiding this comment.
Codeowners reviews satisfied
3 participants
Fixes #14848.
The loopback server was a plain
node:httpserver with default timeouts, so Node closed idle keep-alive sockets after the defaultserver.keepAliveTimeoutof 5 seconds. workerd pools and reuses connections to the loopback server, so this raced with connection reuse: Node closes an idle pooled socket (clean FIN) just as workerd sends the next request on it, and that request fails inside the Worker withError: Network connection lost..This change disables
keepAliveTimeouton the loopback server, mirroring the undici pools used for dispatch in the opposite direction, which already disable their timeouts with a "Disable timeouts for local dev" comment.Full analysis and evidence in #14848. Summary: with
@cloudflare/vite-pluginand a large SSR module graph on a cold optimizer cache (~3,000fetchModuleloopback calls per page load with multi-second idle gaps), 2/3 cold starts failed withNetwork connection loston stock Miniflare, while 4/4 succeeded with this change applied; the Node side logs no socket error on the failing runs, consistent with a clean keep-alive close losing the reuse race.keepAliveTimeout(5s), and asserts a second request on the same socket still succeeds. Without the fix, Node deterministically closes the idle socket at ~5s and the test fails. The original end-to-end race was also manually verified via the A/B experiment in [miniflare] Loopback server uses Node's default 5s keepAliveTimeout, causing intermittent "Network connection lost" in workerd #14848 (patched Miniflare 4/4 cold-start successes vs 2/3 failures on stock; reproduced under both Node 24 and Bun).