feat: runtime-native WebSocket upgrade proxying - #33
Merged
Merged
Conversation
Add `RunnerManager.wsProxyPlugin()` — a srvx plugin that proxies
WebSocket upgrades to the worker, picking the strategy by host runtime:
Node uses the raw upgrade socket (transparent passthrough), Bun/Deno
terminate with crossws and bridge over a `WebSocket` client (no Node
upgrade socket exists there). The plugin reads the active runner lazily
so it survives hot-reloads, and `cli.ts` now uses it instead of the
Node-only `server.node.server.on("upgrade")` wiring.
- Add `EnvRunner.address` (live worker address, used by the bridge)
- `BaseEnvRunner.upgrade()` awaits readiness instead of dropping early
- Fix `RunnerManager.waitForReady()` registering its listener directly
on `_messageListeners` (never forwarded to the runner)
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds TLS-aware runner proxying, a runtime-native WebSocket proxy plugin, manager/CLI wiring for it, and expanded tests and docs covering Node, Bun, and Deno paths. ChangesCross-runtime WebSocket proxying
Estimated code review effort: 4 (Complex) | ~60 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Expose only `RunnerManager.wsProxyPlugin()`; drop the public `createRunnerWSProxyPlugin` export (no concrete consumer, and an export is easy to add later but breaking to remove). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/common/base-runner.ts`:
- Around line 84-90: The upgrade path in base-runner leaves the raw socket open
when `waitForReady()` fails and `this.ready`/`this._address` never become valid,
which causes the Node `"upgrade"` handshake to hang. Update the `BaseRunner`
upgrade handling to explicitly settle `context.node.socket` before
returning—either send a failure response or destroy the socket in the
`waitForReady()`/early-return path so the connection does not remain pending.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ec2b70e9-c5eb-4924-a23e-5300c1d8ffbb
📒 Files selected for processing (11)
.agents/ARCHITECTURE.md.agents/TESTS.mdAGENTS.mdREADME.mdsrc/cli.tssrc/common/base-runner.tssrc/common/ws-proxy.tssrc/index.tssrc/manager.tssrc/types.tstest/websocket.test.ts
Signals the return value is a srvx plugin (the prior `wsProxyPlugin` named the mechanism but not where it plugs in). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/manager.ts (1)
151-154: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winHandle already-ready runners attached after
waitForReady()starts.The listener is now forwarded across reloads, but it only resolves on a future message. If
waitForReady()starts while no runner is attached and_attach()later receives a runner withready === true,_attach()flushes the queue without emitting a message, so this promise can time out even though the manager is ready.Proposed fix
// Register via `onMessage` so the listener is forwarded to the active // runner (and re-forwarded to a fresh one across reloads); a direct // `_messageListeners` add would never receive the worker's ready message. this.onMessage(listener); + if (this.ready) { + clearTimeout(timer); + this.offMessage(listener); + resolve(); + }And notify ready listeners when attaching an already-ready runner:
// If already ready, flush immediately if (runner.ready) { this._flushQueue(); + for (const fn of this._readyListeners) fn(this, runner.address); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/manager.ts` around lines 151 - 154, The readiness flow in manager.ts does not resolve waitForReady() when _attach() connects a runner that is already ready, because the queued listeners are flushed without any message being emitted. Update _attach() in Manager so that when an attached runner has ready === true it notifies the registered ready listeners immediately (the same listeners used by waitForReady()/onMessage forwarding), instead of relying only on a future worker message; keep the existing reload forwarding behavior intact.
🧹 Nitpick comments (1)
pnpm-workspace.yaml (1)
6-11: 🔒 Security & Privacy | 🔵 Trivial | 💤 Low valueTighten the release-age exclusions
crossws*,exsolve*,httpxy*, andsrvx*also match any future package that shares those prefixes. Since only these four dependencies are pinned here, exact names would be narrower and safer:crossws,exsolve,httpxy,srvx.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pnpm-workspace.yaml` around lines 6 - 11, The minimumReleaseAgeExclude entries are too broad because the wildcard patterns in pnpm-workspace.yaml can match future packages beyond the intended dependencies. Update the exclusion list to use the exact package names instead of the current prefix matches, keeping the scope limited to the four pinned packages referenced by this config.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/common/ws-proxy.ts`:
- Around line 60-65: The upstream WebSocket URL builder in ws-proxy should
bracket IPv6 hosts before interpolation because getRunner()?.address.host may be
a bare hostname from url.hostname. Update the URL construction logic in the same
block that uses addr.host, addr.port, and the new URL(peer.request.url) values
so colon-containing hosts are wrapped in [ ] before forming ws://...; keep the
existing fallback for non-IPv6 hosts.
---
Outside diff comments:
In `@src/manager.ts`:
- Around line 151-154: The readiness flow in manager.ts does not resolve
waitForReady() when _attach() connects a runner that is already ready, because
the queued listeners are flushed without any message being emitted. Update
_attach() in Manager so that when an attached runner has ready === true it
notifies the registered ready listeners immediately (the same listeners used by
waitForReady()/onMessage forwarding), instead of relying only on a future worker
message; keep the existing reload forwarding behavior intact.
---
Nitpick comments:
In `@pnpm-workspace.yaml`:
- Around line 6-11: The minimumReleaseAgeExclude entries are too broad because
the wildcard patterns in pnpm-workspace.yaml can match future packages beyond
the intended dependencies. Update the exclusion list to use the exact package
names instead of the current prefix matches, keeping the scope limited to the
four pinned packages referenced by this config.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 44b6be58-081c-4bc1-8868-5d0688d4b740
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (10)
.agents/ARCHITECTURE.md.agents/TESTS.mdAGENTS.mdREADME.mdpackage.jsonpnpm-workspace.yamlsrc/cli.tssrc/common/ws-proxy.tssrc/manager.tstest/websocket.test.ts
✅ Files skipped from review due to trivial changes (1)
- README.md
🚧 Files skipped from review as they are similar to previous changes (1)
- src/cli.ts
The pnpm `minimumReleaseAgeExclude` policy only covers the top-level install; the deno-process and bun-process runner subprocesses resolve npm deps independently and enforce their own minimum-release-age (Deno applies a 24h gate by default since 2.9), rejecting a same-day release such as crossws 0.4.7. Mirror the exclude list via `deno.json` (minimumDependencyAge.exclude) and `bunfig.toml` (minimumReleaseAgeExcludes) so those subprocesses trust the unjs-authored deps. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/common/ws-proxy.ts (1)
33-40: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winAdd error handling to
server.ready().
void server.ready().then(...)has no rejection handler; ifready()rejects, this becomes an unhandled promise rejection.🔧 Proposed fix
return (server) => { - void server.ready().then(() => { + void server.ready().then(() => { const httpServer = server.node?.server as NodeHttpServer | undefined; httpServer?.on("upgrade", (req: IncomingMessage, socket: Socket, head: Buffer) => { getRunner()?.upgrade?.({ node: { req, socket, head } }); }); - }); + }, (err) => console.error("[env-runner] ws proxy setup failed", err)); };🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/common/ws-proxy.ts` around lines 33 - 40, The `server.ready()` call in `wsProxy` is only chained with `.then(...)`, so a rejection becomes unhandled. Update the `return (server) => { ... }` logic to handle failures from `server.ready()` explicitly by adding a rejection path (for example, a `.catch(...)` or equivalent async try/catch) around the `httpServer` upgrade listener setup, while keeping the existing `getRunner()?.upgrade?.(...)` behavior unchanged when readiness succeeds.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/common/ws-proxy.ts`:
- Around line 33-40: The `server.ready()` call in `wsProxy` is only chained with
`.then(...)`, so a rejection becomes unhandled. Update the `return (server) => {
... }` logic to handle failures from `server.ready()` explicitly by adding a
rejection path (for example, a `.catch(...)` or equivalent async try/catch)
around the `httpServer` upgrade listener setup, while keeping the existing
`getRunner()?.upgrade?.(...)` behavior unchanged when readiness succeeds.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e317a9da-7944-497b-95ad-bc3d744096ff
📒 Files selected for processing (2)
src/common/ws-proxy.tstest/websocket.test.ts
The Bun/Deno bridge built the upstream `ws://` target by hand and never ran in CI (the host process is always Node, which uses the raw-socket passthrough), so several address assumptions went unchecked: - IPv6 hosts weren't bracketed (`ws://::1:port` was malformed) - a `WorkerAddress` with `socketPath` was silently treated as "not ready" Extract the construction into a pure, unit-tested `resolveWSProxyTarget()`: - use the worker's reported host, bracket IPv6 authorities - Unix sockets: emit `ws+unix://` on Bun (its `WebSocket` accepts the scheme and it survives crossws's `new URL()` wrap); throw a clear error on Deno, whose `WebSocket` only reaches a socket via the unstable `client` option crossws doesn't forward - add a `tls` flag to `WorkerAddress` and emit `wss://` / `wss+unix://` for it Covers the previously-unreachable branch with direct unit tests. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The plugin's Bun/Deno branch (crossws terminate-and-redial) never runs under the Node-hosted vitest process, so it had no execution coverage. Add a fixture that runs under bun/deno — a crossws echo worker behind a front srvx server carrying the proxy plugin — and connect a client through it from the Node test, asserting the round-trip. Skipped when the runtime is unavailable. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
httpxy only negotiates TLS for a URL-string target with an `https:`/`wss:`
scheme — the `{ host, port }` object form always connects in cleartext. So for a
TLS worker (`address.tls`), build an `https://host:port` target for `proxyFetch`
(with `ssl.rejectUnauthorized: false`) and a `wss://host:port` target for
`proxyUpgrade` (`secure: false`), skipping cert verification since a local
worker typically self-signs. IPv6 authorities are bracketed. Cleartext and
Unix-socket workers keep passing the address object unchanged.
This gives the Node passthrough the same wss/TLS support the Bun/Deno bridge
already has via the address `tls` flag.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/common/base-runner.ts (1)
91-95: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winSettle the claimed upgrade socket when readiness never arrives.
If
waitForReady()times out or the runner closes before becoming ready, Line 94 returns without writing a response or destroyingcontext.node.socket. In the Node"upgrade"path the socket has already been claimed, so the client handshake just hangs until its own timeout.Suggested fix
if (!this.ready || !this._address) { + if (!context.node.socket.destroyed) { + context.node.socket.write( + "HTTP/1.1 503 Service Unavailable\r\nConnection: close\r\n\r\n", + ); + context.node.socket.destroy(); + } return; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/common/base-runner.ts` around lines 91 - 95, The early return in base-runner’s readiness check leaves the claimed upgrade socket unresolved when waitForReady() fails or the runner closes before becoming ready. Update the readiness/upgrade flow in BaseRunner so that the branch around this check explicitly settles the upgrade connection by responding or destroying context.node.socket before returning, ensuring the Node "upgrade" path cannot hang when readiness never arrives.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In `@src/common/base-runner.ts`:
- Around line 91-95: The early return in base-runner’s readiness check leaves
the claimed upgrade socket unresolved when waitForReady() fails or the runner
closes before becoming ready. Update the readiness/upgrade flow in BaseRunner so
that the branch around this check explicitly settles the upgrade connection by
responding or destroying context.node.socket before returning, ensuring the Node
"upgrade" path cannot hang when readiness never arrives.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9f7c34c5-ff18-4175-9941-5cd0b82694cb
📒 Files selected for processing (6)
src/common/base-runner.tssrc/common/ws-proxy.tssrc/types.tstest/fixtures/ws-bridge-server.mjstest/tls-passthrough.test.tstest/websocket.test.ts
- TLS: verify the worker cert by default; add a universal `insecure` opt-in (threaded through every runner, EnvServer, and loadRunner) that re-enables the cert skip on the Node fetch/upgrade passthrough. The Bun/Deno crossws bridge always verifies (crossws exposes no cert-skip hook). - upgrade(): destroy the raw client socket on the not-ready give-up path so it isn't leaked (fd + hanging client) when the worker never becomes ready. - Stop double-bracketing IPv6 hosts in _tlsTarget()/resolveWSProxyTarget() (parseServerAddress reports an already-bracketed URL.hostname). - RunnerManager.waitForReady() now rejects promptly when the manager closes mid-wait instead of waiting out the full timeout. - ws-proxy Node branch: handle a rejected server.ready() to avoid an unhandled rejection from the fire-and-forget upgrade-attach hook. - Harden the flaky tls-passthrough test (static import + assert the httpxy mock is active); expand TLS/IPv6/socket-cleanup/close-reject coverage. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
- tls-passthrough: drop the `vi.mock("httpxy")` approach — mocking a module
base-runner imports transitively is unreliable under coverage instrumentation
(the real httpxy runs and hits the network). Assert the pure `_tlsTarget()`
target/scheme decision, the `insecure` flag, and the upgrade give-up socket
cleanup directly instead.
- Bump the internal 5s `waitForReady()` default to 15s at test call sites
(runners/websocket suites) so cold entry imports on loaded CI runners don't
trip "did not become ready in time".
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The 5s default was too tight for cold-start-heavy runners (miniflare/workerd, bun/deno subprocesses) on loaded CI runners, intermittently tripping "Runner did not become ready in time" across the vercel/netlify/virtual suites. Raise the default in both BaseEnvRunner and RunnerManager (timeout-rejection tests already pass explicit short timeouts, so they're unaffected) and drop the per-call test overrides now that the default covers them. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Scopes the flag name to what it actually affects — TLS certificate verification — so it doesn't read as loosening anything else. Renamed across every runner, `EnvServer`, `loadRunner`, the protected `_insecureTLS` field, docs, and tests. No behavior change. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Removes the TLS-proxy target selection (`_tlsTarget`, `WorkerAddress.tls`) and the `insecureTLS` opt-in from every runner, `EnvServer`, and `loadRunner`. No built-in worker ever reported `tls: true` (`parseServerAddress()` drops the scheme), so the whole path was unreachable through the default runners; drop it until it's wired end-to-end. Also fixes `RunnerManager.upgrade()` silently dropping the raw upgrade socket when no runner is active: it now awaits the runner (like `fetch()`) and destroys the socket when none is available, so a client no longer hangs and the fd isn't leaked during a crash/reload gap. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…times crossws's `createWebSocketProxy` defaults to its own `WebSocket` client, which dials `ws://` and `ws+unix://` uniformly on Node/Bun/Deno. Drop the Bun-only `unixScheme` gating and the Deno throw from `resolveWSProxyTarget()` — a Unix-socket worker now bridges on every host (Deno needs `--unstable-net`). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
pi0
approved these changes
Jul 2, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Adds
RunnerManager.wsProxyPlugin()— a srvx plugin that proxies WebSocket upgrades to the worker, picking the strategy by host runtime: Node uses the raw upgrade socket (transparent passthrough), Bun/Deno terminate with crossws and bridge over aWebSocketclient (no Node upgrade socket exists there). The plugin reads the active runner lazily so it survives hot-reloads;cli.tsuses it instead of the Node-onlyserver.node.server.on("upgrade")wiring.EnvRunner.address(live worker address, used by the bridge)BaseEnvRunner.upgrade()awaits readiness instead of dropping earlyRunnerManager.waitForReady()registering its listener directly on_messageListeners(never forwarded to the runner)No breaking API changes.
🤖 Generated with Claude Code
Summary by CodeRabbit