Skip to content
Open
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions .changeset/spicy-donkeys-listen.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
---
"@cloudflare/vite-plugin": patch
---

Fix `vite build` hanging when `remoteBindings` is enabled

With `remoteBindings` enabled, `vite build` produced all of its output but then never exited, so builds had to be killed manually and could not complete in CI. Builds using remote bindings now finish and exit as expected.
Original file line number Diff line number Diff line change
@@ -1,11 +1,18 @@
import { fileURLToPath } from "node:url";
import { maybeStartOrUpdateRemoteProxySession } from "@cloudflare/remote-bindings";
import { Miniflare } from "miniflare";
import { createBuilder, preview } from "vite";
import { afterEach, describe, test, vi } from "vitest";
import { cloudflare } from "../index";
import type { RemoteProxySession } from "@cloudflare/remote-bindings";

vi.mock("@cloudflare/workers-utils");

vi.mock("@cloudflare/remote-bindings", async (importOriginal) => ({
...(await importOriginal<typeof import("@cloudflare/remote-bindings")>()),
maybeStartOrUpdateRemoteProxySession: vi.fn(),
}));

const fixturesPath = fileURLToPath(new URL("./fixtures", import.meta.url));

describe("preview server", () => {
Expand Down Expand Up @@ -38,4 +45,56 @@ describe("preview server", () => {
await previewServer.close();
expect(disposeSpy).toHaveBeenCalled();
});

test("disposes the remote proxy session when preview server is closed", async ({
expect,
}) => {
const dispose = vi.fn(async () => {});

vi.mocked(maybeStartOrUpdateRemoteProxySession).mockResolvedValue({
session: {
ready: Promise.resolve(),
dispose,
updateBindings: async () => {},
remoteProxyConnectionString:
"http://127.0.0.1:1234" as unknown as RemoteProxySession["remoteProxyConnectionString"],
},
remoteBindings: {},
});

const builder = await createBuilder({
root: fixturesPath,
logLevel: "silent",
plugins: [
cloudflare({
inspectorPort: false,
persistState: false,
remoteBindings: true,
}),
],
});

// Build the worker
await builder.buildApp();

// Start a preview server
const previewServer = await preview({
root: fixturesPath,
logLevel: "silent",
preview: { port: 0 },
plugins: [
cloudflare({
inspectorPort: false,
persistState: false,
remoteBindings: true,
}),
],
});

expect(dispose).not.toHaveBeenCalled();
// The session holds a listening server handle, so leaving it open keeps
// the event loop alive and `vite build` never exits
await previewServer.close();
expect(dispose).toHaveBeenCalled();
});
});
22 changes: 22 additions & 0 deletions packages/vite-plugin-cloudflare/src/miniflare-options.ts
Original file line number Diff line number Diff line change
Expand Up @@ -115,6 +115,28 @@ const remoteProxySessionsDataMap = new Map<
RemoteProxySessionData | null
>();

/**
* Disposes every remote proxy session that has been started.
*
* Each session runs a listening server, which keeps the event loop alive, so
* the sessions must be disposed for a `vite build` or a programmatic server
* close to be able to exit.
*/
export async function disposeRemoteProxySessions(): Promise<void> {
const remoteProxySessionsData = [...remoteProxySessionsDataMap.values()];
remoteProxySessionsDataMap.clear();

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wouldn't it be better to only clear this map after we're certain that the session disposition call passed for all sessions?
That way we could even add retries if it does fail for whatever reason.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. The previous version copied the values and cleared the map first, so a failed dispose() left a listening handle with nothing to retry against.

Pushed a531542: the map entry is deleted only after session.dispose() resolves. A later close, or a subsequent start that still reads the map, can retry. Added a test that rejects the first dispose and checks the session is still passed through as pre-existing.


await Promise.all(
remoteProxySessionsData.map(async (remoteProxySessionData) => {
try {
await remoteProxySessionData?.session.dispose();
} catch (error) {
debuglog("Failed to dispose remote proxy session:", error);
}
})
);
}
Comment thread
NuroDev marked this conversation as resolved.

function createRemoteBindingsLogger(logger: vite.Logger): RemoteBindingsLogger {
const write = (
level: "info" | "warn" | "error",
Expand Down
6 changes: 5 additions & 1 deletion packages/vite-plugin-cloudflare/src/plugins/dev.ts
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,10 @@ import {
compareWorkerNameToExportTypesMaps,
getCurrentWorkerNameToExportTypesMap,
} from "../export-types";
import { getDevMiniflareOptions } from "../miniflare-options";
import {
disposeRemoteProxySessions,
getDevMiniflareOptions,
} from "../miniflare-options";
import { UNKNOWN_HOST } from "../shared";
import {
createPlugin,
Expand Down Expand Up @@ -84,6 +87,7 @@ export const devPlugin = createPlugin("dev", (ctx) => {
} catch (error) {
debuglog("Failed to dispose Miniflare instance:", error);
}
await disposeRemoteProxySessions();

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What happens if this throws?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would have failed viteDevServer.close(). Unlike disposeMiniflare() just above it, this call was not wrapped, so a throw from disposeRemoteProxySessions() skipped the rest of teardown.

session.dispose() is still caught per session. The call site now matches the Miniflare try/catch, so a teardown error is logged and cannot fail the close.

}
}
};
Expand Down
14 changes: 11 additions & 3 deletions packages/vite-plugin-cloudflare/src/plugins/preview.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,10 @@ import { buildPublicUrl, Request as MiniflareRequest } from "miniflare";
import colors from "picocolors";
import { configureContainerPull, getDockerPath } from "../containers";
import { assertIsPreview } from "../context";
import { getPreviewMiniflareOptions } from "../miniflare-options";
import {
disposeRemoteProxySessions,
getPreviewMiniflareOptions,
} from "../miniflare-options";
import { createPlugin, createRequestHandler } from "../utils";
import { handleWebSocket } from "../websockets";
import { rewriteLegacyMiniflarePath } from "./trigger-handlers";
Expand All @@ -27,11 +30,16 @@ export const previewPlugin = createPlugin("preview", (ctx) => {
async configurePreviewServer(vitePreviewServer) {
assertIsPreview(ctx);

// Ensure Miniflare is disposed when the preview server is closed during prerendering
// Ensure Miniflare and any remote proxy sessions are disposed when the
// preview server is closed during prerendering
const closePreviewServer =
vitePreviewServer.close.bind(vitePreviewServer);
vitePreviewServer.close = async () => {
await Promise.all([ctx.disposeMiniflare(), closePreviewServer()]);
await Promise.all([
ctx.disposeMiniflare(),
disposeRemoteProxySessions(),
closePreviewServer(),
]);
};

const { miniflareOptions, containerTagToOptionsMap } =
Expand Down
Loading