Repository navigation
[miniflare] Verify remote Hyperdrive bindings in local dev (#14900) - #16070
edmundhung wants to merge 4 commits into
Conversation
🦋 Changeset detectedLatest commit: 8fd130d The changes in this PR will be included in the next version bump. This PR includes changesets to release 12 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 |
edmundhung
added
the
ci:run-remote-tests
|
I'm Bonk, and I've done a quick review of your PR.
|
@cloudflare/autoconfig
@cloudflare/build-output-utils
@cloudflare/codemods
@cloudflare/config
@cloudflare/containers-shared
create-cloudflare
@cloudflare/deploy-helpers
@cloudflare/kv-asset-handler
miniflare
@cloudflare/pages-functions
@cloudflare/pages-shared
@cloudflare/runtime-types
@cloudflare/unenv-preset
@cloudflare/vite-plugin
@cloudflare/vitest-plugin
@cloudflare/workers-auth
@cloudflare/workers-editor-shared
@cloudflare/workers-utils
wrangler
commit: |
|
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.#teardownServer(`remote:${name}`); | ||
| this.#servers.set(`remote:${name}`, server); | ||
| this.#remoteBridgePorts.set(name, port); | ||
| return port; |
There was a problem hiding this comment.
🟡 Removed Hyperdrive bindings leave bridges running
When a reload removes a remote binding, createRemoteTcpBridge never revisits its key. Its listener and existing database tunnels remain open until Miniflare closes.
Learn more
getServices creates bridges only for bindings still present. Replacing the same key tears down its previous listener, but removing the binding entirely never calls this method for that key. #servers, #connections, and #remoteBridgePorts retain it until full disposal. This also leaves old Node-side binding objects holding a still-usable address after removal.
Example: Start local dev with DB remote, open a pooled database connection, then remove DB from the config and reload. The old bridge keeps accepting connections and the pooled tunnel remains live despite the removed binding.
Recommended fix: Reconcile the controller's active server keys against the full current set of bindings on each configuration update. Tear down absent keys and delete their port entries.
Was this helpful? React with 👍 or 👎 to provide feedback.
| const server = net.createServer((clientSocket) => { | ||
| this.#trackConnection(`remote:${name}`, clientSocket); | ||
| this.#handleRemoteBridgeConnection(clientSocket, wsHref, bindingName); | ||
| }); |
There was a problem hiding this comment.
🟥 Unauthenticated local bridge exposes remote database
Any local process can dial createRemoteTcpBridge without authentication. The bridge opens a tunnel to the deployed Hyperdrive database, granting that process database access.
Was this helpful? React with 👍 or 👎 to provide feedback.
`HyperdriveBindingOptions.dev` accepts `remote` (via `BindingDevOptions`), but `convertToWranglerConfig` only mapped `dev.connectionString` for the `hyperdrive` case, so the flag was silently dropped. A `cloudflare.config.ts` user asking for a remote Hyperdrive binding got a local one instead, and was then required to supply a local connection string — the exact setup `remote: true` exists to avoid. The neighbouring `flagship` and `images` cases already map `dev.remote`; this brings `hyperdrive` in line with them. Found by Bonk's review on cloudflare#16070. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LBVk4gda8JuffGPqrR2XKv
|
Thank you for setting this up, @edmundhung — the credentialed run is the one thing I could never get from a fork, since GitHub withholds repository secrets there and the Hyperdrive E2E silently skipped on every green build of #14900. Seeing I've looked at the Windows failure. I think the good news is bigger than the bad news, but there's a real question underneath it that I don't want to paper over. The functional assertion passed; the equality assertion failed. The failure is Why the equality assertion can't hold. The test fetches credentials twice: once through Two independent fetches can land on two edge instances and come back with two different credential sets, both valid for their own connection. That the direct probe succeeded with the second set is the evidence: these weren't bad credentials, just different ones. Dropping that assertion and keeping the two query assertions would, I think, leave the test proving exactly what it was written to prove. The part I'd rather raise than quietly fix. If two fetches can disagree, the ordinary flow has the same exposure: Bonk's finding was real, and it was mine. The I've carried your one-line fix back to #14900 along with your two test cases, and verified both fail without it. #14900 is also now rebased onto current main and back to Happy to push the E2E assertion change to this branch myself if that's useful, or to leave it to you — whichever fits better with carrying this back. |
…ding fails `seedRemoteHyperdriveBindings()` was awaited inline in the returned object, so a rejection — a WebSocket error, a missing header, or its ten-second timeout — threw *after* the session was already live. The caller never received the handle, leaving no way to dispose the started proxy: the session leaked for the rest of the process's life. Disposing it in a catch would be no better. The session is shared with every other remote binding in the worker, so tearing it down over a Hyperdrive-only failure would take unrelated service, KV, and R2 bindings with it. Warn and continue with no credentials instead: the Hyperdrive bindings are degraded for this round, and `buildMiniflareBindingOptions` already warns again per binding, but the session stays owned by the caller and usable by everything else. A reload re-enters this function and retries the seed. Found by Devin's review on cloudflare#16070. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LBVk4gda8JuffGPqrR2XKv
`handleSeedConnectionString` sat in the first branch of the proxy worker's `fetch`, returning a remote Hyperdrive binding's live connection string as JSON to any request carrying `MF-HD-Seed` and a binding name. Nothing authenticated that request beyond reaching the worker. It is also dead: credentials moved to the WebSocket upgrade response (`MF-HD-Connection-String`) precisely because they belong to the connection that mints them, and `seedRemoteHyperdriveBindings` has read them from there since. The endpoint was left behind, so it carried the disclosure risk without being on any path. Its three tests go with it. Found by Devin's review on cloudflare#14900; @edmundhung removed the same endpoint independently in cloudflare#16070. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LBVk4gda8JuffGPqrR2XKv
`HyperdriveBindingOptions.dev` accepts `remote` (via `BindingDevOptions`), but `convertToWranglerConfig` only mapped `dev.connectionString` for the `hyperdrive` case, so the flag was silently dropped. A `cloudflare.config.ts` user asking for a remote Hyperdrive binding got a local one instead, and was then required to supply a local connection string — the exact setup `remote: true` exists to avoid. The neighbouring `flagship` and `images` cases already map `dev.remote`; this brings `hyperdrive` in line with them. Found by Bonk's review on cloudflare#16070. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LBVk4gda8JuffGPqrR2XKv
…ding fails `seedRemoteHyperdriveBindings()` was awaited inline in the returned object, so a rejection — a WebSocket error, a missing header, or its ten-second timeout — threw *after* the session was already live. The caller never received the handle, leaving no way to dispose the started proxy: the session leaked for the rest of the process's life. Disposing it in a catch would be no better. The session is shared with every other remote binding in the worker, so tearing it down over a Hyperdrive-only failure would take unrelated service, KV, and R2 bindings with it. Warn and continue with no credentials instead: the Hyperdrive bindings are degraded for this round, and `buildMiniflareBindingOptions` already warns again per binding, but the session stays owned by the caller and usable by everything else. A reload re-enters this function and retries the seed. Found by Devin's review on cloudflare#16070. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LBVk4gda8JuffGPqrR2XKv
`handleSeedConnectionString` sat in the first branch of the proxy worker's `fetch`, returning a remote Hyperdrive binding's live connection string as JSON to any request carrying `MF-HD-Seed` and a binding name. Nothing authenticated that request beyond reaching the worker. It is also dead: credentials moved to the WebSocket upgrade response (`MF-HD-Connection-String`) precisely because they belong to the connection that mints them, and `seedRemoteHyperdriveBindings` has read them from there since. The endpoint was left behind, so it carried the disclosure risk without being on any path. Its three tests go with it. Found by Devin's review on cloudflare#14900; @edmundhung removed the same endpoint independently in cloudflare#16070. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LBVk4gda8JuffGPqrR2XKv
4aaedd7 to
48ac455
Compare
643c159 to
ceec2e1
Compare
With remote: true, application code still runs locally and uses an ordinary database driver, but its connections reach the deployed Hyperdrive binding. For example, pg.Client can use env.DB.connectionString and run SELECT 1 without knowing about remote bindings. Miniflare implements this shared path; Wrangler and Vite only pass through the configuration and resolve local fallbacks. The local Hyperdrive binding needs a TCP destination, whereas remote bindings provide remote calls (RPC) and streams over WebSocket. A bridge listening on 127.0.0.1 joins those interfaces without changing the driver or workerd. The existing local remote-session gateway forwards its WebSocket to a proxy Worker on Cloudflare, which holds the real Hyperdrive binding. Neither the bridge nor the proxy interprets SQL: they carry the driver's normal database traffic. During startup, the bridge opens a persistent RPC session and asks the proxy Worker for the binding's connection string. Those details configure the local binding before the local runtime starts. remoteProxyConnectionString names the gateway URL, not the database URL. When a driver connects, the bridge calls connect() on the same remote handle and copies bytes between the local socket and the returned streams. Three driver connections get three independent database sockets, not one shared database connection. The shared handle matters because credentials from remote binding instance A may not authenticate a socket opened by instance B. Fetching a connection string separately from opening sockets can cross that boundary. One session owns both operations instead of seeding credentials from a throwaway connection. - `ProxyServerWorker.ts`: The remote adapter exposes the native binding's connection string and a fresh connect() stream pair through the same handle. - `hyperdrive/index.ts`: Worker bindings route their synthetic database address to the TCP bridge. Node bindings instead expose its loopback address and port; both receive the session's user, password, and database together. - `hyperdrive-proxy.ts`: Each remote listener owns its session and accepted clients. An unchanged target reuses it. A successful reload retires unused listeners without interrupting existing connections; a failed reload rolls back new candidates. Shutdown cancels startup and closes active and retired resources. The existing local database/TLS proxy remains a separate connection path. - `remote-affinity.spec.ts`: Different credentials for each simulated session make accidental cross-session reuse fail. Worker and Node clients exercise reloads, database ID changes, reconnection, and another binding's failure. Co-authored-by: Henry Jang <jang@hyochan.site>
The existing local TLS proxy can still be waiting for a database greeting or TLS handshake when its client disconnects. Closing the listener alone does not close that outbound connection, especially after a reload retires the listener. Give each accepted client ownership of its outbound TCP, TLS, and fallback sockets before waiting for them. Client disconnects and shutdown close the whole connection. A closed peer rejects pending reads and handshake waits, and cannot trigger a new plaintext fallback after its client has gone away. PostgreSQL startup bytes may arrive while the proxy waits for the database's SSL response. Consume a bounded startup buffer so client EOF remains visible, then replay those bytes in order once TLS or plaintext fallback is ready. - `proxy.spec.ts`: Controlled pauses at the greeting, SSL response, and TLS handshake check cleanup before negotiation completes. Delayed SSL responses also check that buffering preserves startup bytes for TLS and fallback paths. Co-authored-by: Henry Jang <jang@hyochan.site>
Failed startup already tears down its development environment, but a teardown error can replace the failure the caller needs to diagnose. Preserve the original error both when startup throws and when it fails before becoming ready, even if cleanup also rejects. For a started session, repeated dispose() calls share one teardown promise, including its rejection. Cleanup is attempted once rather than starting overlapping teardown operations. Co-authored-by: Henry Jang <jang@hyochan.site>
A database greeting or a separately fetched connection string does not prove that the local binding can authenticate through its bridge. Exercise actual SQL queries against provisioned MySQL and PostgreSQL Hyperdrive resources using ordinary database drivers. The drivers and PostgreSQL types are test-only dependencies, not additions to the runtime bundle. - `hyperdrive-remote.test.ts`: Each protocol runs concurrent queries through wrangler dev and a Node-side query through getPlatformProxy(). An invalid local connection string confirms remote mode does not use the local fallback. - `miniflare-remote-resources.test.ts`: A direct Miniflare consumer configures the remote binding and runs a MySQL query before and after setOptions(). This exercises the shared implementation independently of Wrangler's dev adapter. Co-authored-by: Henry Jang <jang@hyochan.site>
ceec2e1 to
8fd130d
Compare
3 participants
Related to #14842 and #14900.
This is a temporary testing ground for remote Hyperdrive support. Please do not merge it; the agreed fixes will go back to #14900.
remote: truewithout a local database connection string.wrangler devandgetPlatformProxy(), plus a direct Miniflare query across reloads.The long-idle workaround is intentionally excluded.