[remote-bindings] Extract remote bindings + dev proxy into standalone packages#14618
[remote-bindings] Extract remote bindings + dev proxy into standalone packages#14618penalosa wants to merge 10 commits into
Conversation
Create a new @cloudflare/dev-proxy package as the single source of truth for the ProxyWorker and its type/util cluster (createDeferred, urlFromParts, ProxyData, proxy-worker protocol types, serialiseError) plus a Miniflare harness (createProxyWorkerOptions/sendProxyWorkerMessage). Repoint wrangler onto it: startDevWorker/utils.ts and events.ts become re-export shims, and ProxyController builds the ProxyWorker via the shared harness (inline contents) instead of the worker: embed. Behaviour is unchanged - all startDevWorker tests pass.
…dings auth Support @cloudflare/remote-bindings running outside wrangler: - workers-utils: add CLOUDFLARE_CONFIG_DIR / CLOUDFLARE_AUTH_CONFIG_FILE / CLOUDFLARE_OAUTH_CLIENT_ID / CLOUDFLARE_ALLOW_GLOBAL_API_KEY / CLOUDFLARE_LOGIN_COMMAND to the env-var name union; export getGlobalConfigDirFromEnv and honour CLOUDFLARE_CONFIG_DIR in getGlobalConfigPath; add the shared CfWorkerInitWithName type used by both wrangler's dev path and remote bindings when building preview uploads. - workers-auth: export getClientIdFromEnv (CLOUDFLARE_OAUTH_CLIENT_ID reader) so a delegated tool can refresh a stored OAuth token.
Introduce @cloudflare/remote-bindings, a standalone package (usable without wrangler) for proxying a Worker's remote bindings to the Cloudflare edge. This commit lands the proven, edge-facing pieces: - pick-remote-bindings: select bindings that must resolve remotely - create-worker-preview: edge-preview session + token upload via the shared workers-utils API client (single source of truth, no duplicated auth/URL handling) - auth: env-var/OAuth credential resolver for delegated (non-wrangler) use - ProxyServerWorker: the minimal-mode edge worker that dispatches proxied requests to the real bindings (bundled inline via a tsdown virtual module) - types/logger + barrel exports The Miniflare-backed local driver (start-session) follows in a subsequent commit.
Extract the remote-mode ProxyData shape (HTTPS to the preview host on 443, injecting cf-workers-preview-token + Access headers) from wrangler's RemoteRuntimeController into createRemoteModeProxyData in @cloudflare/dev-proxy, so wrangler dev --remote and @cloudflare/remote-bindings drive the shared ProxyWorker with an identical, single-sourced configuration. RemoteRuntimeController now calls the shared helper; behaviour is unchanged (RemoteRuntimeController tests pass).
Add the local driver (startRemoteProxySession / maybeStartOrUpdateRemoteProxySession): instead of a bespoke HTTP/WS proxy, run the SAME ProxyWorker as wrangler dev (@cloudflare/dev-proxy) inside Miniflare, configured for remote mode via the shared createRemoteModeProxyData. Token injection, expiry detection and HTTP/WS forwarding are therefore wrangler's proven logic, not a reimplementation. The 1-hour preview token is refreshed both proactively (shared PREVIEW_TOKEN_REFRESH_INTERVAL, now sourced from dev-proxy and re-exported by wrangler's utils) and reactively on the ProxyWorker's previewTokenExpired message, mirroring RemoteRuntimeController.
The ProxyWorker harness depends on miniflare, but wrangler's InspectorProxyWorker (a workerd worker) imports wrangler's utils/events re-export shims. Importing the dev-proxy barrel from those shims dragged miniflare into the worker bundle, breaking it at runtime (No such module "fs/promises"). Split the pure protocol layer (types, control-message shapes, createDeferred, urlFromParts, createRemoteModeProxyData, serialiseError, refresh interval) into a @cloudflare/dev-proxy/protocol entry point with no miniflare dependency. The main entry re-exports it plus the harness. wrangler's worker-facing shims now import from /protocol so worker bundles stay miniflare-free.
🦋 Changeset detectedLatest commit: 504d2d0 The changes in this PR will be included in the next version bump. This PR includes changesets to release 3 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
|
|
I'm Bonk, and I've done a quick review of your PR. Extracts Wrangler's remote-bindings + dev-proxy machinery into standalone One behaviour regression stands out, plus a minor teardown race. Issues (highest severity first)1. [medium] Empty-bindings regression in the public The new package impl now throws
if (Object.keys(bindings).length === 0) {
throw new Error("Cannot start remote proxy session with no bindings");
}On
if (!authSameAsBefore) {
...
remoteProxySession = await startRemoteProxySession(remoteBindings, { ... }); // remoteBindings may be {}
} else {
...
if (!remoteProxySession) {
if (Object.keys(remoteBindings).length > 0) { // <-- guarded here
remoteProxySession = await startRemoteProxySession(remoteBindings, { ... });
}
}
...
}The 2. [low] Teardown race in
|
|
Posted the review to PR #14618. Summary of what I found:
The extraction itself (dev-proxy |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| await play(token); | ||
| scheduleRefresh(); |
There was a problem hiding this comment.
🔴 Local proxy server leaks when initial configuration fails
The local proxy server is started and left running if the initial configuration step fails (play(token) at packages/remote-bindings/src/start-session.ts:304), because no cleanup disposes the server before the error propagates.
Impact: A Miniflare process remains running on a random port with no handle to shut it down, leaking system resources on every failed session start.
Miniflare instance created but never disposed on play() failure
The Miniflare instance mf is created at packages/remote-bindings/src/start-session.ts:219 and its ready promise is awaited at line 299. If play(token) at line 304 throws (e.g. a network error sending the proxy worker message via sendProxyWorkerMessage), the error propagates out of startRemoteProxySession without ever calling mf.dispose(). The dispose closure is only returned as part of the session object at line 332, which is never reached.
The caller (packages/remote-bindings/src/start-session.ts:176-179) wraps the error but has no reference to mf to clean up.
Prompt for agents
In packages/remote-bindings/src/start-session.ts, the code after `const url = await mf.ready` (around line 299) calls `play(token)` and `scheduleRefresh()` without a try/catch. If either throws, the Miniflare instance `mf` is leaked because `dispose()` is only available on the returned session object, which is never constructed.
Wrap lines 304-305 in a try/catch that calls `await mf.dispose()` before re-throwing. Something like:
try {
await play(token);
scheduleRefresh();
} catch (error) {
disposed = true;
clearTimeout(refreshTimer);
await mf.dispose();
throw error;
}
This ensures the Miniflare server is cleaned up if the initial play fails.
Was this helpful? React with 👍 or 👎 to provide feedback.
| currentBindings = newBindings; | ||
| token = await uploadToken(newBindings); | ||
| await play(token); | ||
| scheduleRefresh(); |
There was a problem hiding this comment.
🔴 Binding state corrupted on failed update, causing repeated refresh failures
The current binding state is overwritten with new values (currentBindings = newBindings at packages/remote-bindings/src/start-session.ts:311) before the upload succeeds, so a failed update poisons the periodic token refresh with the bad bindings.
Impact: After a single failed binding update, the automatic token refresh (every 50 minutes) silently retries with the unapplied bindings instead of the last working set, potentially causing repeated failures.
Premature mutation of currentBindings before async uploadToken completes
updateBindings at packages/remote-bindings/src/start-session.ts:307-320 sets currentBindings = newBindings on line 311, then calls await uploadToken(newBindings) on line 312. If uploadToken throws, the error is re-thrown at line 316, but currentBindings has already been mutated to the failed value.
The refreshToken function at line 260-278 calls uploadToken(currentBindings) on line 269, so it will now use the bindings that failed to upload rather than the last-known-good set.
The fix is to defer the assignment until after uploadToken and play succeed:
const newToken = await uploadToken(newBindings);
await play(newToken);
currentBindings = newBindings;
token = newToken;
scheduleRefresh();| currentBindings = newBindings; | |
| token = await uploadToken(newBindings); | |
| await play(token); | |
| scheduleRefresh(); | |
| const newToken = await uploadToken(newBindings); | |
| await play(newToken); | |
| currentBindings = newBindings; | |
| token = newToken; |
Was this helpful? React with 👍 or 👎 to provide feedback.
| try { | ||
| if (isJSRPCBinding(request)) { | ||
| return await newWorkersRpcResponse( | ||
| return newWorkersRpcResponse( |
There was a problem hiding this comment.
🔍 Removing await before return inside try/catch silently bypasses error handling
The ProxyServerWorker template changes return await newWorkersRpcResponse(...) to return newWorkersRpcResponse(...) and return await fetcher.fetch(...) to return fetcher.fetch(...) at packages/remote-bindings/templates/ProxyServerWorker.ts:111 and packages/remote-bindings/templates/ProxyServerWorker.ts:128. Both are inside an async function's try/catch block. This is a well-known JavaScript footgun: when you return await inside a try/catch, the promise settles within the try block so rejections are caught. When you just return a promise, it's returned directly and rejections bypass the catch block entirely. The catch block at line 136-141 handles BindingNotFoundError (returning 400) and generic errors (returning 500 with the error message). After this change, if newWorkersRpcResponse or fetcher.fetch returns a rejected promise, the formatted error response is lost and the rejection propagates unhandled to the workerd runtime instead.
Was this helpful? React with 👍 or 👎 to provide feedback.
| function refreshToken(): Promise<void> { | ||
| if (disposed) { | ||
| return Promise.resolve(); | ||
| } | ||
| if (refreshing) { | ||
| return refreshing; | ||
| } | ||
| refreshing = (async () => { | ||
| try { | ||
| token = await uploadToken(currentBindings); | ||
| await play(token); | ||
| scheduleRefresh(); | ||
| } catch (error) { | ||
| logger.error("Failed to refresh remote bindings preview token", error); | ||
| } finally { | ||
| refreshing = undefined; | ||
| } | ||
| })(); | ||
| return refreshing; |
There was a problem hiding this comment.
🔍 Preview session is never recreated during token refresh, unlike wrangler's RemoteRuntimeController
In packages/remote-bindings/src/start-session.ts:260-278, the refreshToken function only re-uploads the worker and re-plays the token, but it reuses the original session object (captured in the closure at line 165). In contrast, wrangler's RemoteRuntimeController.#refreshPreviewToken (packages/wrangler/src/api/startDevWorker/RemoteRuntimeController.ts:427-431) recreates the preview session before refreshing the token. If the preview session itself becomes invalid (not just the token), the remote-bindings refresh path would fail repeatedly without recovery. This may be acceptable for the remote-bindings use case (shorter-lived sessions), but it's a behavioral difference from wrangler's proven pattern worth noting.
Was this helpful? React with 👍 or 👎 to provide feedback.
workers-devprod
requested review from
a team and
emily-shen
and removed request for
a team
penalosa
force-pushed
the
penalosa/extract-dev-env
branch
from
f1f4e24 to
50a3919
Compare
Replace wrangler's startWorker-based remote proxy session (which spun up a full DevEnv with an embedded ProxyServerWorker template) with a thin wrapper over @cloudflare/remote-bindings' startRemoteProxySession. Wrangler now wires in its logger + auth system and preserves its error surfacing (UserErrors re-thrown directly), while the actual proxying uses the shared ProxyWorker via the package. Remove the now-unused worker:remoteBindings/ProxyServerWorker template and its wrangler.jsonc. Update remote-bindings-errors.test.ts to exercise the real package path (the previous test mocked wrangler's create-worker-preview and asserted DevEnv-internal ErrorEvent shapes that no longer apply). All wrangler remote-bindings unit tests pass (21).
The ProxyWorker (dev-proxy) and ProxyServerWorker (remote-bindings) templates are embedded worker scripts bundled at build time; like wrangler/templates they use workers-types globals and edge-only patterns incompatible with the Node-focused lint rules. Add both dirs to ignorePatterns, matching the existing packages/wrangler/templates exclusion.
Repoint remote proxy sessions from wrangler's re-exported helper to the standalone @cloudflare/remote-bindings package. Import it lazily (dynamic import at the call site) so the Node-only package stays out of the plugin's config-load module graph — Vite evaluates the config in a pure-ESM context. Import the RemoteProxySession type from @cloudflare/remote-bindings and switch the session config field from account_id to the package's accountId.
@cloudflare/autoconfig
create-cloudflare
@cloudflare/deploy-helpers
@cloudflare/dev-proxy
@cloudflare/kv-asset-handler
miniflare
@cloudflare/pages-shared
@cloudflare/remote-bindings
@cloudflare/unenv-preset
@cloudflare/vite-plugin
@cloudflare/vitest-pool-workers
@cloudflare/workers-auth
@cloudflare/workers-editor-shared
@cloudflare/workers-utils
wrangler
commit: |
penalosa
force-pushed
the
penalosa/extract-dev-env
branch
from
50a3919 to
c75b779
Compare
| function refreshToken(): Promise<void> { | ||
| if (disposed) { | ||
| return Promise.resolve(); | ||
| } | ||
| if (refreshing) { | ||
| return refreshing; | ||
| } | ||
| refreshing = (async () => { | ||
| try { | ||
| token = await uploadToken(currentBindings); | ||
| await play(token); | ||
| scheduleRefresh(); | ||
| } catch (error) { | ||
| logger.error("Failed to refresh remote bindings preview token", error); | ||
| } finally { | ||
| refreshing = undefined; | ||
| } | ||
| })(); | ||
| return refreshing; | ||
| } |
There was a problem hiding this comment.
🔍 Token refresh errors are silently logged, not propagated
In packages/remote-bindings/src/start-session.ts:267-278, when refreshToken() fails (e.g. network error, expired OAuth token that can't be refreshed), the error is logged via logger.error but not propagated. The refresh timer is also not rescheduled on failure (no scheduleRefresh() in the catch block), meaning after a single refresh failure the session will never attempt another proactive refresh — only reactive refreshes triggered by previewTokenExpired messages from the ProxyWorker. This mirrors wrangler's RemoteRuntimeController behavior where refresh failures are also logged-and-swallowed, but the difference is that wrangler's controller re-schedules via #scheduleRefresh in #updatePreviewToken which is always called. Here, a failed refresh means no more proactive refreshes until the next updateBindings call.
Was this helpful? React with 👍 or 👎 to provide feedback.
penalosa
force-pushed
the
penalosa/extract-dev-env
branch
from
c75b779 to
504d2d0
Compare
|
✅ All changesets look good |
| const updateBindings = async ( | ||
| newBindings: Record<string, Binding> | ||
| ): Promise<void> => { | ||
| try { | ||
| currentBindings = newBindings; | ||
| token = await uploadToken(newBindings); | ||
| await play(token); | ||
| scheduleRefresh(); | ||
| } catch (error) { | ||
| throw new Error("Failed to update remote proxy bindings", { | ||
| cause: error, | ||
| }); | ||
| } | ||
| }; |
There was a problem hiding this comment.
🔍 updateBindings and refreshToken are not serialized against each other
In packages/remote-bindings/src/start-session.ts:307-319, updateBindings performs uploadToken + play + scheduleRefresh without any serialization against the concurrent refreshToken function (lines 260-279). While refreshToken serializes against itself via the refreshing promise guard, nothing prevents updateBindings and refreshToken from running simultaneously. If the proactive refresh timer fires while updateBindings is mid-flight, both will call uploadToken concurrently — one with old bindings (from refreshToken reading currentBindings before line 311 executes) and one with new bindings. This could result in a stale token briefly winning the race. The old wrangler implementation used a Mutex (runtimeMessageMutex) and event-based coordination to avoid this. In practice, the window is small and the next scheduled refresh would correct it, but it's a behavioral regression from the old implementation's stricter serialization.
Was this helpful? React with 👍 or 👎 to provide feedback.
2 participants
Extract Wrangler's remote-bindings machinery into standalone packages so consumers (starting with the Vite plugin) can establish remote-binding proxy sessions without depending on
wrangler, while keeping a single source of truth for the proxy logic.What changed
@cloudflare/dev-proxy— single home for theProxyWorker. Wrangler'sProxyControllernow consumes it (via a miniflare-free/protocolentry point so the workerdInspectorProxyWorkerkeeps building). Also shares the remote-modeProxyDataconstruction used bywrangler dev --remote.@cloudflare/remote-bindings— edge-preview primitives (create-worker-preview,pick-remote-bindings, env-drivenauth) plusstart-session.ts, which drives the sharedProxyWorkerinside Miniflare exactly aswrangler dev --remotedoes (proactive + reactive preview-token refresh).start-remote-proxy-session.tsis a thin wrapper over the package (wiring wrangler's logger +requireAuth);create-worker-previewis de-duplicated intoremote-bindingswith a smalldev/preview.tsshim that injects wrangler's logger + interactivity predicate (behaviour preserved).import("@cloudflare/remote-bindings")(keeps the Node-only package out of Vite's pure-ESM config-load graph) instead ofwrangler.workers-utils/workers-auth/deploy-helpers(env-var unions, shared types).Single source of truth: wrangler loses this logic (net reduction) and the new packages gain it — no reimplementation of the proxy path.
A picture of a cute animal (not mandatory, but encouraged)