Skip to content

Remove loopback for Asset worker#14417

Open
matthewdavidrodgers wants to merge 3 commits into
mainfrom
asset-worker-outer-entrypoint-class
Open

Remove loopback for Asset worker#14417
matthewdavidrodgers wants to merge 3 commits into
mainfrom
asset-worker-outer-entrypoint-class

Conversation

@matthewdavidrodgers

@matthewdavidrodgers matthewdavidrodgers commented Jun 24, 2026

Copy link
Copy Markdown
Contributor

Fixes WC-5345.

Bypass the Asset Worker loopback on normal request and RPC paths

The inner asset entrypoint is now the default, avoiding the latency added by forwarding every call through ctx.exports. The outer loopback entrypoint and its cohort-routing infrastructure remain available as named exports in the Asset Worker and Miniflare bundles so the boundary can be re-enabled later.


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

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


Open in Devin Review

@changeset-bot

changeset-bot Bot commented Jun 24, 2026

Copy link
Copy Markdown

🦋 Changeset detected

Latest commit: 7dc3f67

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

This PR includes changesets to release 7 packages
Name Type
@cloudflare/workers-shared Patch
miniflare Patch
@cloudflare/deploy-helpers Patch
@cloudflare/pages-shared Patch
@cloudflare/vite-plugin Patch
wrangler Patch
@cloudflare/vitest-pool-workers 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-project-automation github-project-automation Bot moved this to Untriaged in workers-sdk Jun 24, 2026
@workers-devprod workers-devprod requested review from a team and jamesopstad and removed request for a team June 24, 2026 20:37
@workers-devprod

workers-devprod commented Jun 24, 2026

Copy link
Copy Markdown
Contributor

Codeowners approval required for this PR:

  • @cloudflare/deploy-config
  • ✅ @cloudflare/wrangler
Show detailed file reviewers
  • packages/workers-shared/asset-worker/src/worker.ts: [@cloudflare/deploy-config]

@github-actions

Copy link
Copy Markdown
Contributor

✅ All changesets look good

@ask-bonk

ask-bonk Bot commented Jun 24, 2026

Copy link
Copy Markdown
Contributor

The miniflare asset service uses the default export of asset-worker.mjs (no explicit entrypoint specified means default), which is now AssetWorkerInner. Since enable_ctx_exports is set and AssetWorkerInner is still exported, the loopback infra remains valid too. This is consistent with the intent.

I've completed a thorough review. The change is functionally sound:

  • Inner runFetchRequest already handles Sentry, analytics (EntrypointType.Inner), error handling, and metrics submission, so bypassing the outer doesn't drop observability.
  • Both entrypoints implement AssetWorkerMethods, so the Service<AssetWorker> type contract in router-worker (type-only import) is preserved.
  • No DO migrations or named entrypoints reference the class identity, so swapping the default export is safe.
  • Test assertions match mockJaegerBinding().getSpanContext() output and the commented-out version routing.
  • Changeset is appropriate.

One minor note worth raising: the new test uses Object.defineProperty(ctx, "exports", { value: ... }) without configurable: true. If createExecutionContext() ever begins providing an exports property, this would throw. But this is non-blocking test infrastructure and the property is currently absent. Not actionable enough to block.

No logic bugs, security issues, backward-compatibility violations, or incorrect API behavior found.

LGTM

github run

@pkg-pr-new

pkg-pr-new Bot commented Jun 24, 2026

Copy link
Copy Markdown
@cloudflare/autoconfig

npm i https://pkg.pr.new/@cloudflare/autoconfig@14417

create-cloudflare

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

@cloudflare/deploy-helpers

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

@cloudflare/kv-asset-handler

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

miniflare

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

@cloudflare/pages-shared

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

@cloudflare/unenv-preset

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

@cloudflare/vite-plugin

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

@cloudflare/vitest-pool-workers

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

@cloudflare/workers-auth

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

@cloudflare/workers-editor-shared

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

@cloudflare/workers-utils

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

wrangler

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

commit: 7dc3f67

@devin-ai-integration devin-ai-integration Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Devin Review found 2 potential issues.

Open in Devin Review

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🚩 Trace context behavior change when AssetWorkerInner is called directly

When AssetWorkerInner is called directly as the default export (not via loopback), loopbackCtx.props?.traceContext at packages/workers-shared/asset-worker/src/worker.ts:523 evaluates to null. This null value is then passed to this.env.JAEGER.runWithSpanContext(null, ...) at line 526. Previously, AssetWorkerOuter would capture the active span context via this.env.JAEGER.getSpanContext() and forward it to the inner entrypoint through ctx.props. Now the inner entrypoint passes null when called directly, relying on the JAEGER binding to either preserve the existing runtime-established context or handle null as a no-op. This is likely fine because the runtime should already have the correct trace context active when calling the default entrypoint directly, but it's worth confirming that runWithSpanContext(null, ...) doesn't inadvertently clear an active trace context in production.

(Refers to lines 522-537)

Open in Devin Review

Was this helpful? React with 👍 or 👎 to provide feedback.

Comment on lines +6 to +8
Bypass the Asset Worker loopback on normal request and RPC paths

The inner asset entrypoint is now the default, avoiding the latency added by forwarding every call through `ctx.exports`. The outer loopback entrypoint and its cohort-routing infrastructure remain available as named exports in the Asset Worker and Miniflare bundles so the boundary can be re-enabled later.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changeset description contains implementation details instead of user-facing impact

The REVIEW.md rule states: "Changesets should target users of the tools (e.g. Wrangler users) rather than maintainers. Avoid including implementation details like 'moves X from hybridModules to nativeModules'." The changeset body mentions internal implementation concepts: "inner asset entrypoint", "ctx.exports", "outer loopback entrypoint", "cohort-routing infrastructure", and "named exports in the Asset Worker and Miniflare bundles." A user-facing description should focus on the performance benefit (e.g., "Improve asset serving latency by removing an unnecessary internal request hop").

Suggested change
Bypass the Asset Worker loopback on normal request and RPC paths
The inner asset entrypoint is now the default, avoiding the latency added by forwarding every call through `ctx.exports`. The outer loopback entrypoint and its cohort-routing infrastructure remain available as named exports in the Asset Worker and Miniflare bundles so the boundary can be re-enabled later.
Improve asset serving performance by removing an unnecessary internal dispatch hop
Asset requests and RPC calls now avoid an extra round-trip through an internal forwarding layer, reducing latency. The forwarding infrastructure is preserved for future use by cohort-based deployments.
Open in Devin Review

Was this helpful? React with 👍 or 👎 to provide feedback.

@jamesopstad jamesopstad left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. The Devin comments will need resolving before merging.

@matthewdavidrodgers matthewdavidrodgers force-pushed the asset-worker-outer-entrypoint-class branch from af4b7ea to 7dc3f67 Compare June 25, 2026 20:32
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.

3 participants