Skip to content

fix(miniflare): unify service binding setup with assets rpc proxy#14329

Draft
edmundhung wants to merge 5 commits into
mainfrom
edmundhung/default-entrypoint-rpc-proxy
Draft

fix(miniflare): unify service binding setup with assets rpc proxy#14329
edmundhung wants to merge 5 commits into
mainfrom
edmundhung/default-entrypoint-rpc-proxy

Conversation

@edmundhung

@edmundhung edmundhung commented Jun 16, 2026

Copy link
Copy Markdown
Member

Fixes n/a.

This avoids the repeat branching logic on service bindings to user worker by putting a assets rpc proxy in front all the time.

image
  • Tests
    • Tests included/updated
    • Automated tests not possible - manual testing has been completed as follows:
    • Additional testing not necessary because: covered by existing tests
  • Public documentation
    • Cloudflare docs PR(s):
    • Documentation not necessary because: internal refactor

A picture of a cute animal (not mandatory, but encouraged)

@changeset-bot

changeset-bot Bot commented Jun 16, 2026

Copy link
Copy Markdown

🦋 Changeset detected

Latest commit: b0e70de

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 6 packages
Name Type
miniflare Patch
@cloudflare/deploy-helpers Patch
@cloudflare/pages-shared Patch
@cloudflare/vite-plugin Patch
@cloudflare/vitest-pool-workers Patch
wrangler Patch

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

@github-actions

github-actions Bot commented Jun 16, 2026

Copy link
Copy Markdown
Contributor

✅ All changesets look good

@ask-bonk

ask-bonk Bot commented Jun 16, 2026

Copy link
Copy Markdown
Contributor

Let me reconsider the getCustomServiceDesignator change one more time, as it's the highest-risk part. The change at line 443 (service === kCurrentWorker is handled separately at 435; this else is for a plain string worker name). Previously: getUserServiceName(service). Now: proxy.

There's a real concern here. The else branch (line 441-444) handles service being a plain string that is NOT kCurrentWorker. But this string could be a worker that does NOT exist in this Miniflare instance (an external/cross-process binding). Previously getUserServiceName(service) — but if the worker doesn't exist locally, neither service exists, so the binding would be broken either way. Actually, cross-process bindings are handled elsewhere (dev registry proxy). Let me confirm the string-service path is only reached for locally-defined workers.

Actually, this is the path for serviceBindings: { NAME: "worker-name" }. If "worker-name" isn't a local worker, this would've been broken before too. The proxy service ${RPC_PROXY_SERVICE_NAME}:worker-name only exists if that worker is local. So same constraint as before. Not a regression.

I've completed a thorough analysis. The change is internally consistent and the routing logic is correctly unified. The findings worth raising:

  1. Test claim discrepancy — the changeset/PR checks "Tests included/updated" but no test files are modified. This is a notable process gap for an architectural change touching service binding routing.
  2. A minor naming overlap: SERVICE_DEFAULT_ENTRYPOINT_FALLBACK (MINIFLARE_DEFAULT_ENTRYPOINT_FALLBACK) now shares the SERVICE_DEFAULT_ENTRYPOINT_PREFIX (MINIFLARE_DEFAULT_ENTRYPOINT_), so a worker literally named FALLBACK would collide. Extremely unlikely; borderline nitpick.

Neither of these is a hard logic bug with a concrete code fix I can confidently suggest. The test concern is real but is a process observation, not a code-line suggestion. Let me reconsider whether item 2 is actionable enough — it's an edge case unlikely to occur and I'd be speculating. The guidance says be pragmatic and don't nitpick.

The only item that rises above nitpick level is the test discrepancy, which isn't a code-line issue. Given the instructions: "If there are NO actionable issues" (logic bugs, security, compat, incorrect API behavior with concrete fixes), the response should be LGTM. The naming overlap is too theoretical, and the test discrepancy isn't a code defect with a line-level suggestion.

I'll verify the naming-overlap concern isn't actually a real collision risk by confirming worker names can't be FALLBACK arbitrarily — they're user-defined and FALLBACK is a plausible (if unusual) name. But the previous code had the same overlap risk avoided by distinct prefixes. This is a genuine, if low-severity, regression in namespace safety. However, it would require a worker named exactly FALLBACK, which is exceedingly rare and arguably out of scope. I'll treat it as a non-actionable nitpick.

After thorough review, there are no concrete logic bugs, security issues, or API regressions with actionable line-level fixes.

LGTM

github run

@pkg-pr-new

pkg-pr-new Bot commented Jun 16, 2026

Copy link
Copy Markdown
create-cloudflare

npm i https://pkg.pr.new/create-cloudflare@14329

@cloudflare/deploy-helpers

npm i https://pkg.pr.new/@cloudflare/deploy-helpers@14329

@cloudflare/kv-asset-handler

npm i https://pkg.pr.new/@cloudflare/kv-asset-handler@14329

miniflare

npm i https://pkg.pr.new/miniflare@14329

@cloudflare/pages-shared

npm i https://pkg.pr.new/@cloudflare/pages-shared@14329

@cloudflare/unenv-preset

npm i https://pkg.pr.new/@cloudflare/unenv-preset@14329

@cloudflare/vite-plugin

npm i https://pkg.pr.new/@cloudflare/vite-plugin@14329

@cloudflare/vitest-pool-workers

npm i https://pkg.pr.new/@cloudflare/vitest-pool-workers@14329

@cloudflare/workers-auth

npm i https://pkg.pr.new/@cloudflare/workers-auth@14329

@cloudflare/workers-editor-shared

npm i https://pkg.pr.new/@cloudflare/workers-editor-shared@14329

@cloudflare/workers-utils

npm i https://pkg.pr.new/@cloudflare/workers-utils@14329

wrangler

npm i https://pkg.pr.new/wrangler@14329

commit: b0e70de

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Untriaged

Development

Successfully merging this pull request may close these issues.

2 participants