Skip to content

Revert PR #52 "fix(executor): use real Fetcher for owner-id outbound wrapper"#53

Merged
jonnyparris merged 1 commit intomainfrom
revert/global-outbound-wrapper
Apr 27, 2026
Merged

Revert PR #52 "fix(executor): use real Fetcher for owner-id outbound wrapper"#53
jonnyparris merged 1 commit intomainfrom
revert/global-outbound-wrapper

Conversation

@jonnyparris
Copy link
Copy Markdown
Owner

Summary

Reverts #52. The fix swapped one runtime error for another.

What happened

The LOADER.get() approach returned a real Fetcher (good — workerd accepted it as globalOutbound), but that stub cannot be transferred across the boundary into the codemode-loaded sandbox Worker. Runtime error from inside any codemode fetch():

Entrypoints to dynamically-loaded workers cannot be transferred to other Workers, because the system does not know how to reload this Worker from scratch. Instead, have the parent Worker expose an entrypoint which constructs the dynamic worker and forwards to it.

This was confirmed live after deploying b52345f to production by running await fetch(...) inside a fresh codemode session.

Net effect

Outbound fetch() from codemode is still broken — just at a later stage. The original duck-typed wrapper at least failed at executor instantiation; this version defers the error to actual fetch time, which is arguably worse for diagnosis.

Path forward

A proper fix needs to satisfy two constraints workerd enforces:

  1. globalOutbound must be a real Fetcher (rules out the duck-typed object)
  2. The Fetcher must be transferable across LOADER.get() boundaries (rules out another LOADER.get() stub)

Likely options:

  • Add a real WorkerEntrypoint class statically declared in wrangler.jsonc (e.g. OutboundForwarder) that wraps OUTBOUND and uses workerd per-call props for the per-request ownerId
  • Move the owner-id channel out of the request entirely (per-session DO lookup keyed by something the sandbox can't forge)

Both need real design work and tests; punting until properly thought through.

🤖 generated by ruskin's agent

…#52)"

This reverts commit b52345f.

The LOADER.get() approach swapped one error for another. The wrapper
worker stub returned by getEntrypoint() cannot be transferred across
the boundary into another LOADER-spawned Worker (the codemode sandbox).
Runtime error from inside codemode fetch():

  Entrypoints to dynamically-loaded workers cannot be transferred to
  other Workers, because the system does not know how to reload this
  Worker from scratch. Instead, have the parent Worker expose an
  entrypoint which constructs the dynamic worker and forwards to it.

Net effect: outbound fetch() from codemode is still broken, just at
a later stage. Reverting to the duck-typed wrapper while a proper
fix (likely a real WorkerEntrypoint subclass with per-call props,
or moving the owner-id channel out of the request entirely) is
designed.
@jonnyparris jonnyparris merged commit 40f0a37 into main Apr 27, 2026
2 checks passed
@jonnyparris jonnyparris deleted the revert/global-outbound-wrapper branch April 27, 2026 07:44
jonnyparris added a commit that referenced this pull request Apr 27, 2026
… props (#54)

Replaces the broken `wrapOutboundWithOwner` wrapper with workerd's
native per-call props mechanism on the OUTBOUND self-binding.

## The two failed attempts before this

1. Original duck-typed wrapper: returned `{ fetch: ... } as Fetcher`
   for `globalOutbound`. Workerd validates that globalOutbound must be
   a real ServiceStub and rejected it at executor instantiation:
   'Incorrect type for the globalOutbound field on WorkerCode...'

2. PR #52 / LOADER.get() wrapper: produced a real Fetcher via dynamic
   loader, but that stub was non-transferable across the LOADER.get()
   boundary into codemode's sandbox. Live error from inside codemode:
   'Entrypoints to dynamically-loaded workers cannot be transferred
   to other Workers...' Reverted in PR #53.

## The fix workerd actually wants

`OUTBOUND` is already declared as a self-binding to AllowlistOutbound
in wrangler.jsonc, which makes it a `LoopbackServiceStub`. Loopback
stubs are callable: `env.OUTBOUND({ props: { ownerId } })` returns a
real, transferable Fetcher with per-call props attached. The receiving
WorkerEntrypoint reads them from `this.ctx.props` — which the sandbox
cannot tamper with, since props ride workerd's RPC channel, not the
request body.

## Changes

- `src/types.ts`: type `OUTBOUND` as
  `LoopbackServiceStub<AllowlistOutbound>` so the callable signature is
  visible to TS and `env.OUTBOUND({ props })` typechecks.
- `src/outbound.ts`: AllowlistOutbound now extends
  `WorkerEntrypoint<Env, AllowlistOutboundProps>`. `fetch()` reads
  `this.ctx.props?.ownerId` instead of an injected request header.
  `OWNER_ID_HEADER` constant deleted.
- `src/executor.ts`: `wrapOutboundWithOwner` (LOADER.get attempt)
  replaced with `bindOutboundWithOwner` — a one-liner that calls
  `outbound({ props: { ownerId } })`. No wrapper, no extra worker.
- `src/agentic.ts`: same call-site updated.
- `test/outbound.test.ts`: 4 new unit tests for the producer side
  (passthrough cases, props injection, no cross-mixing per ownerId).
  643 → 647 tests passing.

## Migration safety

If only one side of the change deploys (e.g. mid-rollout), the worst
case is that AllowlistOutbound sees no ownerId and falls back to no
per-user auth — the same behaviour as the broken state we're leaving.
No regressions are introduced.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant