[vite-plugin] Dispose remote proxy sessions when the server closes - #15269
[vite-plugin] Dispose remote proxy sessions when the server closes#15269official-burak wants to merge 3 commits into
Conversation
With `remoteBindings` enabled, `vite build` wrote every file correctly and then hung forever. Each remote proxy session runs a listening server, which is a referenced libuv handle, so the event loop cannot drain while one stays open. Sessions are cached in a module-level map keyed by config path. That map was only ever read and written, never cleared, and the single existing `session.dispose()` call fires solely to replace a session whose auth changed. Nothing tore sessions down when the prerender pass finished and the preview server closed, so the build could not exit. This made remote bindings unusable in CI builds. Dispose the sessions and clear the map when the dev or preview server closes. The dev path is guarded by the existing restart check, so sessions are still reused across dev server restarts.
🦋 Changeset detectedLatest commit: a531542 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
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}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
@cloudflare/autoconfig
@cloudflare/build-output-utils
@cloudflare/codemods
@cloudflare/config
create-cloudflare
@cloudflare/deploy-helpers
@cloudflare/kv-asset-handler
miniflare
@cloudflare/pages-functions
@cloudflare/pages-shared
@cloudflare/unenv-preset
@cloudflare/vite-plugin
@cloudflare/vitest-plugin
@cloudflare/workers-auth
@cloudflare/workers-editor-shared
@cloudflare/workers-utils
wrangler
commit: |
|
A note on the red checks, in case it saves triage time. As far as I can tell none of them relate to this change, and no test actually fails. Vite Plugin E2E (Linux / macOS / Windows) — 44 tests passed, 0 failed. The two failed suites are the
Tests (Windows, packages-and-tools) — 1063 miniflare tests passed; the single failed suite is Vite Plugin Playground (macOS, vite-8) — 287 tests passed; one
Happy to rebase or re-run whenever that is useful. |
Per REVIEW.md, changesets should target users rather than maintainers. Drop the event-loop and session-disposal internals and state the observable behaviour instead.
|
Follow-up on the latest run after the changeset rewrite: the only remaining red check is Vite Plugin E2E (Windows). Linux and macOS both completed with 9/9 test files passing. On Windows the suite was still progressing (completed files were passing; That timeout is showing up on other unrelated PRs today as well (for example the hyperdrive PlanetScale PR, where Windows also fails while Linux/macOS pass). Nothing in this change looks involved: teardown in the logs completes in ~100ms, and Happy to rebase or wait on a re-run. |
| } catch (error) { | ||
| debuglog("Failed to dispose Miniflare instance:", error); | ||
| } | ||
| await disposeRemoteProxySessions(); |
There was a problem hiding this comment.
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.
| */ | ||
| export async function disposeRemoteProxySessions(): Promise<void> { | ||
| const remoteProxySessionsData = [...remoteProxySessionsDataMap.values()]; | ||
| remoteProxySessionsDataMap.clear(); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
Clearing the map before dispose ran meant a failed session.dispose() dropped the only handle we had, so a later close could not retry and the listening server could keep the event loop alive. Swallow dispose errors on the close path the same way Miniflare already does.
|
Codeowners approval required for this PR:
Show detailed file reviewers
|
|
CI update: the Windows Vite Plugin E2E timeout from the earlier run is gone. Latest checks on this PR are green (59/59 on the previous SHA). Pushed a531542 for the dispose-map review comments; that SHA is still running. |
3 participants
Fixes #15173.
With
remoteBindingsenabled,vite buildwrites every file correctly and then hangs forever. Each remote proxy session runs a listening server, which is a referenced libuv handle, so the event loop cannot drain while one stays open.Sessions are cached in a module-level map in
miniflare-options.ts, keyed by config path. That map was only ever read and written, never cleared, and the one existingsession.dispose()call fires solely to replace a session whose auth has changed. Nothing tore sessions down when the prerender pass finished and the preview server closed, so the build could not exit. This makes remote bindings unusable in any CI build.This adds
disposeRemoteProxySessions()and calls it when the dev and preview servers close, alongside the existingctx.disposeMiniflare(). The preview path is the one that fixes the reported hang, since duringvite buildthe preview server exists only to serve the prerender pass. The dev path leaks the same handle when the server is closed programmatically, so it is covered too; that call sits inside the existing!ctx.isRestartingDevServerguard, so sessions are still reused across dev server restarts.Disposal failures are swallowed through
debuglog, matching how the neighbouring Miniflare disposal already behaves, so a failing teardown cannot stop the server from closing.Test evidence
Added
disposes the remote proxy session when preview server is closedtopreview-server.spec.ts, next to the existing Miniflare disposal test. It mocksmaybeStartOrUpdateRemoteProxySessionso no account or network access is needed, builds the fixture worker, starts a preview server withremoteBindings: true, and asserts the session is disposed on close.Before the change:
After the change, the full package suite passes:
oxlint --deny-warnings --type-awarereports 0 warnings and 0 errors,oxfmt --checkpasses on the changed files, andtsc --buildis clean.Note
This is a contribution from an AI agent: Claude Opus 5. The investigation, fix and tests were produced with AI assistance, and reviewed and owned by me, Burak Keskin. I will be responding on this PR.