feat(proxy): support async target resolver in createWebSocketProxy - #196
Merged
Conversation
The `target` resolver was synchronous only, so consumers whose upstream address isn't known at connect time (a worker still booting, a backend being hot-reloaded) had to wrap the proxy hooks to await readiness before dialing. Allow `target` to return a promise. The `open` hook now registers the per-peer state up front (so client frames sent while the target resolves are buffered, bounded by `maxBufferSize`) and dials the upstream once the URL settles. `connectTimeout` covers the resolution too, so a resolver that never settles closes the peer with 1011 instead of hanging; a rejecting resolver closes with 1011 as well. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
📝 WalkthroughWalkthrough
ChangesAsync Target Resolution for WebSocket Proxy
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
docs/1.guide/7.proxy.md (1)
79-90: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider showing the
peerparameter in the async example for consistency.The synchronous dynamic-target example at line 69 accepts
peer, but the async example at line 84 omits it. Since the resolver type is(peer: Peer) => ..., including the parameter (even if unused) aligns the snippet with the API signature readers just saw and avoids the impression that async resolvers have a different signature.target: async () => { + target: async (peer) => {This is optional — the code is valid as-is.
🤖 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 `@docs/1.guide/7.proxy.md` around lines 79 - 90, The async `target` example in `createWebSocketProxy` should show the same resolver signature as the synchronous example by including the `peer` parameter, even if it is unused. Update the snippet in the proxy guide so the async resolver is clearly `(peer) => ...` to match the `Peer`-based API shape readers already saw and keep both examples consistent.test/proxy.test.ts (2)
199-215: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMake the buffering test deterministic.
With the fixed 50ms delay, a slow CI run could resolve the target before Line 214 sends
"early", so the test may pass without proving buffering. Use a manually resolved promise to guarantee the frame is sent during the async gap.🧪 Proposed deterministic test flow
+ let resolveTarget!: (url: string) => void; + const targetReady = new Promise<string>((resolve) => { + resolveTarget = resolve; + }); const asyncProxy = nodeAdapter({ hooks: createWebSocketProxy({ - target: async () => { - await new Promise((r) => setTimeout(r, 50)); - return upstreamURL; - }, + target: () => targetReady, }), }); @@ const ws = await wsConnect(`ws://localhost:${port}/`, { skip: 1 }); await ws.send("early"); + resolveTarget(upstreamURL); expect(await ws.next()).toBe("echo:early");🤖 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 `@test/proxy.test.ts` around lines 199 - 215, The buffering test is timing-dependent because the fixed delay in the async target can resolve before the ws.send("early") call runs. Update the proxy test around asyncProxy.handleUpgrade and wsConnect to use a manually controlled promise for the target resolution so the frame is definitely sent while the upstream is still unresolved, then resolve it explicitly before asserting the echoed message.
222-245: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for a never-settling resolver timeout.
This verifies rejection, but not the new
connectTimeoutbehavior fortarget: () => new Promise(() => {}). Add a small test with a shortconnectTimeoutand assert the peer closes with1011.🤖 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 `@test/proxy.test.ts` around lines 222 - 245, Add a focused test in proxy.test.ts alongside the existing createWebSocketProxy/handleUpgrade coverage that uses a target resolver returning a never-settling Promise and a short connectTimeout, then assert the ws peer closes with code 1011. Reuse the existing test setup patterns (nodeAdapter, createServer, wsConnect, CloseEvent) so the new case specifically verifies the connectTimeout path rather than the rejection path.src/proxy.ts (1)
376-381: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winUse a thenable check for async targets
instanceof Promisemisses cross-realm promises/thenables, so_resolveTarget()can fall through tonew URL(raw)and reject valid async resolvers.🤖 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/proxy.ts` around lines 376 - 381, In _resolveTarget, the async-target detection is too narrow because using instanceof Promise can miss thenables or cross-realm promises. Update the target resolution logic to use a thenable-style check on the raw value returned from the target resolver, and keep the existing URL coercion path for both sync and async results so valid async resolvers don’t fall through to new URL(raw).
🤖 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/proxy.ts`:
- Around line 25-26: The `connectTimeout` documentation in `proxy.ts` is
inconsistent between the top-level paragraph and the option description. Update
the `connectTimeout` option docs to match the behavior described near the
`Proxy`/`connectTimeout` references by stating it also covers target resolution,
not just the upstream handshake, so both descriptions use the same wording and
meaning.
In `@test/proxy.test.ts`:
- Around line 236-239: The close event listener in the proxy test is attached
too late, so the connection may close before the handler is registered and the
test can hang. Update the test around wsConnect and the
ws.ws.addEventListener("close", ...) setup so the close listener is registered
before awaiting the connection or otherwise ensure the listener is attached
first, keeping the existing Promise<CloseEvent> flow intact.
---
Nitpick comments:
In `@docs/1.guide/7.proxy.md`:
- Around line 79-90: The async `target` example in `createWebSocketProxy` should
show the same resolver signature as the synchronous example by including the
`peer` parameter, even if it is unused. Update the snippet in the proxy guide so
the async resolver is clearly `(peer) => ...` to match the `Peer`-based API
shape readers already saw and keep both examples consistent.
In `@src/proxy.ts`:
- Around line 376-381: In _resolveTarget, the async-target detection is too
narrow because using instanceof Promise can miss thenables or cross-realm
promises. Update the target resolution logic to use a thenable-style check on
the raw value returned from the target resolver, and keep the existing URL
coercion path for both sync and async results so valid async resolvers don’t
fall through to new URL(raw).
In `@test/proxy.test.ts`:
- Around line 199-215: The buffering test is timing-dependent because the fixed
delay in the async target can resolve before the ws.send("early") call runs.
Update the proxy test around asyncProxy.handleUpgrade and wsConnect to use a
manually controlled promise for the target resolution so the frame is definitely
sent while the upstream is still unresolved, then resolve it explicitly before
asserting the echoed message.
- Around line 222-245: Add a focused test in proxy.test.ts alongside the
existing createWebSocketProxy/handleUpgrade coverage that uses a target resolver
returning a never-settling Promise and a short connectTimeout, then assert the
ws peer closes with code 1011. Reuse the existing test setup patterns
(nodeAdapter, createServer, wsConnect, CloseEvent) so the new case specifically
verifies the connectTimeout path rather than the rejection path.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 2933688f-a82d-44a5-bfe7-2da0e15a0e46
📒 Files selected for processing (3)
docs/1.guide/7.proxy.mdsrc/proxy.tstest/proxy.test.ts
- await non-native thenables (structural check, not `instanceof Promise`) so Bluebird/cross-realm/custom thenable resolvers don't get stringified into `new URL()` and close every connection with 1011 - guard the upstream `message` listener with the stale-state check used by its sibling open/close/error listeners, so an in-flight upstream frame can't leak to a peer the proxy already tore down - wrap the buffer-flush loop in try/catch and clear the buffer in finally, matching the message hook, so an upstream racing into CLOSING after `open` doesn't let the exception escape or strand the buffer - document that the never-settles -> 1011 guarantee requires a non-zero connectTimeout; with `0` the peer is bounded only by maxBufferSize (1009) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
🧹 Nitpick comments (1)
test/proxy.test.ts (1)
222-250: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider extracting shared test server setup/teardown.
This test, along with the delayed-promise test (194-220) and the rejecting-resolver test (252-276), repeats the same
createServer/listen/waitForPort/try-finallycleanup boilerplate. A small helper (e.g.withTestServer(handler, fn)) would reduce duplication across the three new cases.🤖 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 `@test/proxy.test.ts` around lines 222 - 250, The new WebSocket proxy tests repeat the same server lifecycle boilerplate in the non-native thenable, delayed-promise, and rejecting-resolver cases. Extract the shared `createServer`/`listen`/`waitForPort`/`try-finally` cleanup into a small helper such as `withTestServer`, and use it in these tests so the setup/teardown stays consistent and the test bodies focus on resolver behavior.
🤖 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.
Nitpick comments:
In `@test/proxy.test.ts`:
- Around line 222-250: The new WebSocket proxy tests repeat the same server
lifecycle boilerplate in the non-native thenable, delayed-promise, and
rejecting-resolver cases. Extract the shared
`createServer`/`listen`/`waitForPort`/`try-finally` cleanup into a small helper
such as `withTestServer`, and use it in these tests so the setup/teardown stays
consistent and the test bodies focus on resolver behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ca34495a-d6f1-4324-8caa-a758f3b4e576
📒 Files selected for processing (3)
docs/1.guide/7.proxy.mdsrc/proxy.tstest/proxy.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/proxy.ts
1 task
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.
Summary
Allow
createWebSocketProxy'stargetresolver to be async (return a promise). Until now it was synchronous only, so consumers whose upstream address isn't known at connect time — a worker still booting, a backend being hot-reloaded, a registry lookup — had to wrap the proxy hooks to await readiness before dialing.Motivated by
unjs/env-runner#33, which hand-rolled exactly this: a customupgradehook awaiting worker readiness around the proxy. With an asynctargetthat collapses to a one-liner:What changed
targetnow accepts(peer) => string | URL | Promise<string | URL>.openhook restructured: registers the per-peer state (and startsconnectTimeout) up front, then dials the upstream synchronously or once the async target settles.maxBufferSizepath) and flushed on upstream open.connectTimeoutnow also bounds a resolver that never settles → peer closed1011instead of hanging.1011.UpstreamState.wsis now optional with guards inclose/error/message/_cleanupStatefor the "peer closed mid-resolution" window._dialUpstream.No behavior change for synchronous string/URL/function targets.
Tests
3 new cases: async resolver proxies successfully, early frames buffer-and-flush across the async gap, and a rejecting resolver closes with
1011. Full suite green (156 passed).Docs
Documented the async resolver under "Dynamic target" and updated the API signature +
connectTimeoutnote indocs/1.guide/7.proxy.md.🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Tests / Documentation