FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

[miniflare] Start runtime disposal before browser/proxy cleanup (#15143) · cloudflare/workers-sdk@2e0c962 · GitHub

/ workers-sdk Public

Commit 2e0c962

Browse files
teamleaderleo
authored
[miniflare] Start runtime disposal before browser/proxy cleanup (#15143)
1 parent 6529f0c commit 2e0c962

3 files changed

Lines changed: 248 additions & 51 deletions

File tree

‎.changeset/fuzzy-cats-dispose.md‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
1+
---
2+
"miniflare": patch
3+
---
4+
5+
Prevent `workerd` from remaining running during Miniflare shutdown when browser or proxy cleanup is slow or fails.

‎packages/miniflare/src/index.ts‎

Lines changed: 87 additions & 51 deletions
Original file line numberDiff line numberDiff line change
@@ -3123,70 +3123,106 @@ export class Miniflare {
31233123
// Note `dispose()`ing the `#proxyClient` implicitly poison's proxies, but
31243124
// we'd like them to be poisoned synchronously here.
31253125
this.#proxyClient?.poisonProxies();
3126+
// Preserve a readiness failure without allowing it to skip teardown.
3127+
let waitForReadyFailed = false;
3128+
let waitForReadyError: unknown;
31263129
try {
31273130
await this.#waitForReady(/* disposing */ true);
3128-
} finally {
3131+
} catch (error) {
3132+
waitForReadyFailed = true;
3133+
waitForReadyError = error;
3134+
}
3135+
3136+
// Runtime.dispose() requests workerd termination synchronously before
3137+
// returning its child-exit promise. Start it before awaiting independent
3138+
// cleanup so those hooks cannot delay or skip the termination request.
3139+
let runtimeDisposePromise: Promise<void>;
3140+
try {
3141+
runtimeDisposePromise = Promise.resolve(this.#runtime?.dispose());
3142+
} catch (error) {
3143+
runtimeDisposePromise = Promise.reject(error);
3144+
}
3145+
// Attach a rejection handler immediately so a fast runtime failure cannot
3146+
// become unhandled while independent cleanup is still pending.
3147+
const runtimeDisposeOutcome = runtimeDisposePromise.then(
3148+
() => ({ ok: true as const }),
3149+
(error: unknown) => ({ ok: false as const, error })
3150+
);
3151+
3152+
// Preserve the existing first cleanup error, while still waiting for the
3153+
// already-started runtime exit before the outer disposal settles.
3154+
let independentCleanupFailed = false;
3155+
let independentCleanupError: unknown;
3156+
try {
3157+
// Cleanup as much as possible even if `#init()` threw.
31293158
await this.#closeBrowserProcesses();
31303159

31313160
// Remove exit hook, we're cleaning up what they would've cleaned up now
31323161
this.#removeExitHook?.();
31333162

3134-
// Cleanup as much as possible even if `#init()` threw
31353163
await this.#proxyClient?.dispose();
3136-
await this.#runtime?.dispose();
3137-
// Close the undici Pool used for dispatching fetch requests to the
3138-
// runtime. This must happen after the runtime is disposed, so that
3139-
// in-flight connections are broken and close immediately. Without this,
3140-
// lingering sockets in the Pool can keep the Node.js event loop alive.
3141-
// The Pool may already be destroyed (e.g., if workerd was SIGKILL'd and
3142-
// all connections broke), so ignore ClientDestroyedError.
3143-
try {
3144-
await this.#runtimeDispatcher?.close();
3145-
} catch {}
3146-
// Also close the dev-registry dispatcher (same issue as above).
3147-
try {
3148-
await this.#devRegistryDispatcher?.close();
3149-
} catch {}
3164+
} catch (error) {
3165+
independentCleanupFailed = true;
3166+
independentCleanupError = error;
3167+
}
31503168

3151-
await this.#stopLoopbackServer();
3152-
// Close the WebSocket server so any connected clients are disconnected
3153-
// and their sockets don't keep the event loop alive. It uses
3154-
// `noServer: true` so it doesn't own an HTTP server, but connected
3155-
// WebSocket clients still hold open sockets.
3156-
this.#webSocketServer.close();
3157-
// Best-effort cleanup: on Windows, workerd may not release file handles
3158-
// immediately after disposal, causing EBUSY errors. The temp directory
3159-
// lives in os.tmpdir() so the OS will clean it up eventually.
3160-
removeDir(this.#tmpPath, { fireAndForget: true });
3161-
// Clean up email session directories in the project temp path. When no
3162-
// project temp path is supplied, these live inside `#tmpPath` and are
3163-
// already removed above.
3164-
const emailPaths = getEmailPathsToClean(
3165-
this.#sharedOpts.resourceTmpPath,
3166-
this.#tmpPath
3167-
);
3168-
if (emailPaths) {
3169-
try {
3170-
await removeDir(emailPaths.sessionDir);
3171-
} catch (e) {
3172-
this.#log.debug(
3173-
`Unable to remove email session directory: ${String(e)}`
3174-
);
3175-
}
3169+
const runtimeCleanupOutcome = await runtimeDisposeOutcome;
3170+
// Close the undici Pool used for dispatching fetch requests to the
3171+
// runtime. This must happen after the runtime is disposed, so that
3172+
// in-flight connections are broken and close immediately. Without this,
3173+
// lingering sockets in the Pool can keep the Node.js event loop alive.
3174+
// The Pool may already be destroyed (e.g., if workerd was SIGKILL'd and
3175+
// all connections broke), so ignore ClientDestroyedError.
3176+
try {
3177+
await this.#runtimeDispatcher?.close();
3178+
} catch {}
3179+
// Also close the dev-registry dispatcher (same issue as above).
3180+
try {
3181+
await this.#devRegistryDispatcher?.close();
3182+
} catch {}
3183+
3184+
await this.#stopLoopbackServer();
3185+
// Close the WebSocket server so any connected clients are disconnected
3186+
// and their sockets don't keep the Node.js event loop alive. It uses
3187+
// `noServer: true` so it doesn't own an HTTP server, but connected
3188+
// WebSocket clients still hold open sockets.
3189+
this.#webSocketServer.close();
3190+
// Best-effort cleanup: on Windows, workerd may not release file handles
3191+
// immediately after disposal, causing EBUSY errors. The temp directory
3192+
// lives in os.tmpdir() so the OS will clean it up eventually.
3193+
removeDir(this.#tmpPath, { fireAndForget: true });
3194+
// Clean up email session directories in the project temp path. When no
3195+
// project temp path is supplied, these live inside `#tmpPath` and are
3196+
// already removed above.
3197+
const emailPaths = getEmailPathsToClean(
3198+
this.#sharedOpts.resourceTmpPath,
3199+
this.#tmpPath
3200+
);
3201+
if (emailPaths) {
3202+
try {
3203+
await removeDir(emailPaths.sessionDir);
3204+
} catch (e) {
3205+
this.#log.debug(
3206+
`Unable to remove email session directory: ${String(e)}`
3207+
);
31763208
}
3209+
}
31773210

3178-
// Close the inspector proxy server if there is one
3179-
await this.#maybeInspectorProxyController?.dispose();
3180-
// Unregister workers from dev registry and stop the file watcher
3181-
await this.#devRegistry.dispose();
3211+
// Close the inspector proxy server if there is one
3212+
await this.#maybeInspectorProxyController?.dispose();
3213+
// Unregister workers from dev registry and stop the file watcher
3214+
await this.#devRegistry.dispose();
31823215

3183-
// shutdown hyperdrive proxies if any exist
3184-
await this.#hyperdriveProxyController.dispose();
3216+
// shutdown hyperdrive proxies if any exist
3217+
await this.#hyperdriveProxyController.dispose();
31853218

3186-
// Remove from instance registry as last step in `finally`, to make sure
3187-
// all dispose steps complete
3188-
maybeInstanceRegistry?.delete(this);
3189-
}
3219+
// Remove from instance registry as last step in disposal, preserving the
3220+
// existing behavior when an earlier cleanup operation fails.
3221+
maybeInstanceRegistry?.delete(this);
3222+
3223+
if (independentCleanupFailed) throw independentCleanupError;
3224+
if (!runtimeCleanupOutcome.ok) throw runtimeCleanupOutcome.error;
3225+
if (waitForReadyFailed) throw waitForReadyError;
31903226
}
31913227
}
31923228

‎packages/miniflare/test/teardown-lifecycle.spec.ts‎

Lines changed: 156 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,156 @@
1+
import childProcess from "node:child_process";
2+
import path from "node:path";
3+
import { Miniflare, ProxyClient } from "miniflare";
4+
import { afterEach, test, vi } from "vitest";
5+
import { WebSocketServer } from "ws";
6+
import { singleModuleManifest } from "./test-shared";
7+
8+
async function createReadyMiniflare(): Promise<Miniflare> {
9+
const mf = new Miniflare({
10+
workers: [
11+
{
12+
config: {
13+
type: "worker",
14+
name: "",
15+
compatibilityDate: "2025-05-01",
16+
manifest: singleModuleManifest(`export default {
17+
fetch() {
18+
return new Response("ok");
19+
}
20+
}`),
21+
},
22+
},
23+
],
24+
});
25+
await mf.ready;
26+
return mf;
27+
}
28+
29+
function findKilledWorkerd(
30+
kill: ReturnType<typeof vi.spyOn>
31+
): childProcess.ChildProcess | undefined {
32+
for (let index = 0; index < kill.mock.calls.length; index++) {
33+
const [signal] = kill.mock.calls[index];
34+
const child = kill.mock.contexts[index];
35+
if (
36+
signal === "SIGKILL" &&
37+
child instanceof childProcess.ChildProcess &&
38+
path.basename(child.spawnfile).toLowerCase().startsWith("workerd")
39+
) {
40+
return child;
41+
}
42+
}
43+
}
44+
45+
afterEach(() => {
46+
vi.restoreAllMocks();
47+
});
48+
49+
test("Miniflare: dispose requests workerd termination while proxy cleanup is pending", async ({
50+
expect,
51+
}) => {
52+
const mf = await createReadyMiniflare();
53+
let markProxyDisposeStarted!: () => void;
54+
let releaseProxyDispose!: () => void;
55+
const proxyDisposeStarted = new Promise<void>((resolve) => {
56+
markProxyDisposeStarted = resolve;
57+
});
58+
const proxyDisposeBlocked = new Promise<void>((resolve) => {
59+
releaseProxyDispose = resolve;
60+
});
61+
const proxyDispose = vi
62+
.spyOn(ProxyClient.prototype, "dispose")
63+
.mockImplementationOnce(() => {
64+
markProxyDisposeStarted();
65+
return proxyDisposeBlocked;
66+
});
67+
const kill = vi.spyOn(childProcess.ChildProcess.prototype, "kill");
68+
69+
const disposePromise = mf.dispose();
70+
try {
71+
await proxyDisposeStarted;
72+
expect(findKilledWorkerd(kill)).toBeDefined();
73+
} finally {
74+
releaseProxyDispose();
75+
proxyDispose.mockRestore();
76+
await disposePromise;
77+
}
78+
});
79+
80+
test("Miniflare: dispose waits for workerd exit and continues cleanup before returning proxy cleanup failure", async ({
81+
expect,
82+
}) => {
83+
let markRuntimeExitObserved!: () => void;
84+
let releaseRuntimeExit!: () => void;
85+
const runtimeExitObserved = new Promise<void>((resolve) => {
86+
markRuntimeExitObserved = resolve;
87+
});
88+
const runtimeExitBlocked = new Promise<void>((resolve) => {
89+
releaseRuntimeExit = resolve;
90+
});
91+
const originalEmit = childProcess.ChildProcess.prototype.emit;
92+
let interceptedRuntimeExit = false;
93+
const emit = vi
94+
.spyOn(childProcess.ChildProcess.prototype, "emit")
95+
.mockImplementation(function (
96+
this: childProcess.ChildProcess,
97+
event: string | symbol,
98+
...args: unknown[]
99+
) {
100+
if (
101+
!interceptedRuntimeExit &&
102+
event === "exit" &&
103+
path.basename(this.spawnfile).toLowerCase().startsWith("workerd")
104+
) {
105+
interceptedRuntimeExit = true;
106+
markRuntimeExitObserved();
107+
void runtimeExitBlocked.then(() => {
108+
Reflect.apply(originalEmit, this, [event, ...args]);
109+
});
110+
return true;
111+
}
112+
return Reflect.apply(originalEmit, this, [event, ...args]) as boolean;
113+
});
114+
const mf = await createReadyMiniflare();
115+
const proxyDispose = vi
116+
.spyOn(ProxyClient.prototype, "dispose")
117+
.mockRejectedValueOnce(new Error("injected proxy cleanup failure"));
118+
const webSocketClose = vi.spyOn(WebSocketServer.prototype, "close");
119+
const kill = vi.spyOn(childProcess.ChildProcess.prototype, "kill");
120+
121+
let runtimeExitReleased = false;
122+
let firstDisposeSettled = false;
123+
const firstDisposeResult = mf.dispose().then(
124+
() => {
125+
firstDisposeSettled = true;
126+
return undefined;
127+
},
128+
(error: unknown) => {
129+
firstDisposeSettled = true;
130+
return error;
131+
}
132+
);
133+
134+
try {
135+
await runtimeExitObserved;
136+
await new Promise<void>((resolve) => setImmediate(resolve));
137+
expect(firstDisposeSettled).toBe(false);
138+
expect(findKilledWorkerd(kill)).toBeDefined();
139+
140+
releaseRuntimeExit();
141+
runtimeExitReleased = true;
142+
const firstDisposeError = await firstDisposeResult;
143+
expect(webSocketClose).toHaveBeenCalled();
144+
expect(firstDisposeError).toBeInstanceOf(Error);
145+
expect((firstDisposeError as Error).message).toContain(
146+
"injected proxy cleanup failure"
147+
);
148+
} finally {
149+
if (!runtimeExitReleased) releaseRuntimeExit();
150+
emit.mockRestore();
151+
proxyDispose.mockRestore();
152+
webSocketClose.mockRestore();
153+
await firstDisposeResult;
154+
await mf.dispose().catch(() => {});
155+
}
156+
});

0 commit comments

Comments
 (0)
Back | FazBrowse Home | New Git URL