[miniflare] Forward service binding and tail consumer props across the dev registry#13649
Conversation
🦋 Changeset detectedLatest commit: ad4d60c The changes in this PR will be included in the next version bump. 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 |
workers-devprod
requested review from
a team and
dario-piotrowicz
and removed request for
a team
|
Codeowners approval required for this PR:
Show detailed file reviewers |
Changeset ReviewFile Reviewed:
|
| Criterion | Status | Notes |
|---|---|---|
| Version Type | ✅ | patch is appropriate for this bug fix |
| Changelog Quality | ✅ | Clear description of the bug (props being dropped), explains the fix, includes helpful wrangler.json configuration example |
| Markdown Headers | ✅ | No h1/h2/h3 headers present |
| Analytics | ✅ | No analytics changes |
| Dependabot | ✅ | Not a dependabot changeset |
| Experimental Features | ✅ | Not an experimental feature |
Positive Notes
- Well-written description explaining both the problem ("props were silently dropped") and the solution
- Useful code example showing the
wrangler.jsonconfiguration withprops - Correctly identifies both affected packages (
miniflareandwrangler) - Clear explanation of the behavior change for the target worker's entrypoint
![ask-bonk[bot]](https://avatars.githubusercontent.com/in/2454580?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
|
Review posted successfully on PR #13649. The implementation is solid — the only finding is that the changeset unnecessarily includes |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
petebacondarwin
force-pushed
the
fix/dev-registry-props-forwarding
branch
from
d41cb7c to
a4778bd
Compare
create-cloudflare
@cloudflare/kv-asset-handler
miniflare
@cloudflare/pages-shared
@cloudflare/unenv-preset
@cloudflare/vite-plugin
@cloudflare/vitest-pool-workers
@cloudflare/workers-editor-shared
@cloudflare/workers-utils
wrangler
commit: |
petebacondarwin
force-pushed
the
fix/dev-registry-props-forwarding
branch
4 times, most recently
from
78f11c7 to
e7cd34e
Compare
…e dev registry When a service binding or tail consumer configured with `props` targeted a worker running in a separate miniflare instance via the dev registry, the `props` were silently dropped and the remote entrypoint saw an empty `ctx.props`. The rewrite that redirects such bindings to the local dev-registry proxy worker overwrote the user-supplied props with internal routing metadata, and the proxy never forwarded props to the remote when calling `getEntrypoint` over the workerd debug port. Preserve the original props as `userProps` inside the proxy's routing metadata, then pass them through as the third argument to `WorkerdDebugPortClient.getEntrypoint`, which workerd already accepts and surfaces as `ctx.props` on the callee.
petebacondarwin
force-pushed
the
fix/dev-registry-props-forwarding
branch
from
e7cd34e to
ad4d60c
Compare
|
This seems like a legitimate issue: https://github.com/cloudflare/workers-sdk/actions/runs/24884767213/job/72871751377?pr=13649#step:7:694 👀 |
I ran it again and it passed, so I think it was a flake?? |
oh... it failed 2/3 times in a row with the same error message that mentioned the dev registry, so it looked legitimate to me 😮 |
workers-devprod
left a comment
workers-devprod
left a comment
There was a problem hiding this comment.
Codeowners reviews satisfied
|
|
||
| // Read the captured ctx.props back via the remote's default fetch. | ||
| const res = await remote.dispatchFetch("http://placeholder"); | ||
| expect(res.status).toBe(200); |
There was a problem hiding this comment.
if res.status is not 200, we won't read the response body with res.json on the next line, which means we will fail when MINIFLARE_ASSERT_BODIES_CONSUMED=true
There was a problem hiding this comment.
https://github.com/cloudflare/workerd/actions/runs/24897548221/job/72909545765?pr=6663 <- failing here now (unrelated workerd changes)
| await vi.waitFor( | ||
| async () => { | ||
| const res = await local.dispatchFetch("http://placeholder"); | ||
| expect(res.status).toBe(200); |
There was a problem hiding this comment.
4 participants
Fixes a bug where service binding and tail consumer
propswere silently dropped when the target worker was running in a separate local dev instance (via the dev registry).When Miniflare encounters a service binding or tail consumer pointing at a worker that isn't in the current Miniflare config, it rewrites the binding to redirect through a local
dev-registry-proxyworker. Two issues on that path caused props to be lost:getExternalServiceEntrypointsinpackages/miniflare/src/index.tsoverwrote the user-suppliedpropswith internal routing metadata ({ service, entrypoint }).WorkerdDebugPortClient.getEntrypoint(service, entrypoint)without ever passing the user's props as the optional third argument.The fix preserves the original props as
userPropsinside the proxy's routing metadata, and passes them togetEntrypoint(service, entrypoint, userProps). Workerd's debug-port RPC already accepts this argument (seeworker-interface.capnp—getEntrypoint @0 (service :Text, entrypoint :Text, props :Frankenvalue)) and populatesctx.propson the callee, so no workerd-side changes are needed.The same rewrite/drop pattern applies to tail consumers (
workerOpts.core.tails), so both are fixed in one pass. Durable Object bindings are not affected because workerd has nopropsfield onDurableObjectNamespaceDesignator; streaming tails aren't rewritten by the dev registry at all today and are out of scope here.A picture of a cute animal (not mandatory, but encouraged)