[miniflare] Route storage/remote bindings through shared services via props#14743
[miniflare] Route storage/remote bindings through shared services via props#14743penalosa wants to merge 1 commit into
Conversation
… props Move the per-binding configuration for storage and remote (mixed-mode) bindings out of per-resource workerd services and into runtime `ctx.props`, so a single shared service can serve any number of bindings. - Local KV namespaces now share one entry service; the namespace id is passed via props and resolved in object-entry.worker.ts (idFromName). - remoteProxyClientWorker() is now script-only; the connection string, binding name and trace id travel via props (buildRemoteProxyProps), read in remote-proxy-client.worker.ts and the dispatch-namespace proxy. - All remote-binding plugins emit one shared remote-proxy service instead of one per resource. - explorer.ts reads the KV namespace id from binding props rather than parsing it out of the service name.
penalosa
added
the
ci:no-changeset-required
|
|
Codeowners approval required for this PR:
Show detailed file reviewers
|
@cloudflare/autoconfig
@cloudflare/config
create-cloudflare
@cloudflare/deploy-helpers
@cloudflare/kv-asset-handler
miniflare
@cloudflare/pages-shared
@cloudflare/unenv-preset
@cloudflare/vite-plugin
@cloudflare/vitest-pool-workers
@cloudflare/workers-auth
@cloudflare/workers-editor-shared
@cloudflare/workers-utils
wrangler
commit: |
![ask-bonk[bot]](https://avatars.githubusercontent.com/in/2454580?s=60&v=4){kind=small}
There was a problem hiding this comment.
I'm Bonk, and I've done a quick review of your PR.
Summary: Moves per-binding storage/remote config out of per-resource workerd services and into ctx.props, letting one shared service back many bindings. The local storage path is exercised by the existing KV/R2/D1 suites and the remote service-binding path by remote-bindings-access-warning.spec.ts, so the behaviour-preserving claim is reasonably supported.
Explorer registers a colliding "remote" id for remote bindings (low severity). Remote KV/R2/D1 bindings now all point at a shared service name (kv:ns:remote, r2:bucket:remote, d1:db:remote), but constructExplorerBindingMap still derives the resource id by string-stripping that name. Every remote binding of a given type therefore registers the literal id "remote", so multiple remote bindings overwrite each other in IDToBindingName, and a bogus "remote" id leaks into the explorer map (selecting it would try to open storage for a namespace literally named remote). KV reads the id from props but still falls through to the name parse for remote (yielding "remote"); the R2 block (explorer.ts:218, const bucketName = binding.r2Bucket.name.replace(/^r2:bucket:/, "")) and the D1 block (explorer.ts:177, const databaseId = innerBinding.service?.name?.replace(/^d1:db:/, "")) weren't updated at all and have the same problem. Since remote resources aren't locally browsable, the cleanest fix is to skip the shared remote service name in all three blocks. Inline suggestion below covers the KV block; please apply the equivalent !== "remote" guard to the R2 and D1 blocks.
| } | ||
| } | ||
| if (namespaceId === undefined) { | ||
| namespaceId = binding.kvNamespace.name.replace(/^kv:ns:/, ""); |
There was a problem hiding this comment.
Remote KV namespaces now point at the shared kv:ns:remote service and carry buildRemoteProxyProps (no MINIFLARE_NAMESPACE), so the props lookup misses and this fallback strips the name to the literal "remote". Multiple remote namespaces all collapse to id "remote" and overwrite each other in IDToBindingName.kv. Consider skipping the shared remote service here:
| namespaceId = binding.kvNamespace.name.replace(/^kv:ns:/, ""); | |
| if (namespaceId === undefined) { | |
| const rawId = binding.kvNamespace.name.replace(/^kv:ns:/, ""); | |
| // Skip the shared remote-proxy service — remote namespaces aren't | |
| // locally explorable and would otherwise all register id "remote". | |
| if (rawId !== "remote") { | |
| namespaceId = rawId; | |
| } | |
| } |
|
Review posted successfully to PR #14743. I reviewed the PR and posted a single COMMENT review with one inline suggestion. The one actionable finding: Explorer registers a colliding Everything else checks out: the local storage path is covered by the existing KV/R2/D1 plugin suites and the remote service-binding path by |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| if (namespaceId === undefined) { | ||
| namespaceId = binding.kvNamespace.name.replace(/^kv:ns:/, ""); | ||
| } |
There was a problem hiding this comment.
🟡 Explorer collapses all remote KV namespaces to a single wrong entry
Remote KV namespaces are recorded in the explorer under the literal id "remote" (binding.kvNamespace.name.replace(/^kv:ns:/, "") at packages/miniflare/src/plugins/core/explorer.ts:204) instead of their real namespace id, so multiple remote namespaces overwrite each other and none can be matched correctly.
Impact: When more than one remote (mixed-mode) KV namespace is configured, the local explorer lists/keys them incorrectly, dropping all but one.
Why the fallback breaks for remote namespaces
Before this PR, remote KV bindings used a service name of the form kv:ns:<id>[:remote-<sha>], so stripping the kv:ns: prefix yielded a per-namespace id. This PR changes remote namespaces in packages/miniflare/src/plugins/kv/index.ts:95 to all share the single service name KV_REMOTE_SERVICE_NAME = kv:ns:remote, and their props (packages/miniflare/src/plugins/kv/index.ts:96-99) carry remoteProxyConnectionString/binding — not SharedBindings.TEXT_NAMESPACE.
In constructExplorerBindingMap, JSON.parse(propsJson)[SharedBindings.TEXT_NAMESPACE] is therefore undefined for remote namespaces, so the code falls through to binding.kvNamespace.name.replace(/^kv:ns:/, ""), which is always "remote". Every remote namespace then collides on IDToBindingName.kv["remote"]. The inline comment at packages/miniflare/src/plugins/core/explorer.ts:185-186 claiming remote namespaces "still encode the id in the service name (kv:ns:ID[:remoteSuffix])" is no longer accurate.
Was this helpful? React with 👍 or 👎 to provide feedback.
2 participants
Move the per-binding configuration for storage and remote (mixed-mode) bindings out of per-resource workerd services and into runtime
ctx.props, so a single shared service can serve any number of bindings.object-entry.worker.ts(idFromName)remoteProxyClientWorker()is now script-only — the connection string, binding name and trace id travel via props (buildRemoteProxyProps), read inremote-proxy-client.worker.tsand the dispatch-namespace proxyexplorer.tsreads the KV namespace id from binding props rather than parsing it out of the service nameNo user-facing behaviour change — this is internal plumbing that lets a single service back any number of bindings, replacing the previous one-service-per-resource model.