Repository navigation
server: shared ServerHost so http and ws transports can share one socket - #303
Merged
Merged
Conversation
|
Warning Review limit reached
Next review available in: 38 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (41)
📝 WalkthroughWalkthroughChangesShared server hosting
Possibly related PRs
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
🧪 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 |
Extract socket ownership from the transports into a new @nmtjs/server package: per-runtime hosts (uWS/Bun/Deno) own listen/TLS, /healthy, upgrade-vs-HTTP dispatch and the uWS fetch translation, with refcounted start/stop. Transports become tenants that either own a private host (listen mode, unchanged behavior) or mount onto a shared one via the new `server` option, so HTTP and WS serve from a single listen address and the gateway reports one URL under both proxyable types. - transports' runtime adapters shrink to thin host registrations - WS behavior options hoisted to top-level `ws` (runtime.ws before); dead WS `cors` option removed; unix URLs unified to proto+unix:// - neem e2e: shared-server fixture drives the native proxy end-to-end (same URL under http+ws types, HTTP RPC, /healthy and WS upgrade through the proxy port); proxy unit test pins same-URL dual-type upstream keying - uWS must stay external when bundling workers under neem: its .node binary is loaded via a runtime-computed require
denny-il
force-pushed
the
dev/shared-server-host
branch
from
July 30, 2026 07:00
2b0172a to
4a8d084
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 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 `@packages/neem/tests/e2e/shared-server.spec.ts`:
- Around line 76-84: Bound each WebSocket attempt in the promise created by
waitFor: add a per-attempt timer that closes the WebSocket and resolves false if
neither onopen nor onerror fires, ensuring waitFor can continue to its existing
timeout. Update the connection logic around the WebSocket constructor and settle
the promise only once.
In `@packages/server/src/host.ts`:
- Around line 59-85: Update stop() so it does not clear this.#bound before the
in-flight bind promise settles and teardown completes; retain the promise
reference throughout the await and close sequence to prevent concurrent start()
calls from initiating a second bind. Adjust the final state cleanup only after
the existing bound.catch(...) and close() operations finish, while preserving
start()’s rollback behavior for failed binds.
In `@packages/server/src/runtimes/bun.ts`:
- Around line 59-61: Update the Bun runtime’s `/healthy` handling in the routes
configuration around Object.assign so it intercepts the path independently of
the HTTP method, matching the method-agnostic behavior of the Deno host. Ensure
methods such as HEAD reach the host-owned health check instead of falling
through to the general fetch handler.
- Around line 42-85: Update the return logic after Bun.serve in the runtime
startup method to detect Unix-socket listeners and convert Bun’s unix:///path
format into the established http+unix:// or https+unix:// address format,
selecting the scheme from TLS configuration. Preserve the existing
server.url.href return behavior for TCP listeners.
In `@packages/server/src/runtimes/node.ts`:
- Around line 190-217: The catch block around the request body stream and
fetchHandler must map PayloadTooLargeError to a 413 response instead of
InternalServerErrorHttpResponse(). Detect that error before generic
logging/fallback handling, assign the corresponding 413 helper response, and
preserve the existing 500 behavior for all other errors.
In `@packages/ws-transport/tests/shared-server.spec.ts`:
- Around line 95-101: Move the openSocket liveness check from the finally block
into the try block before teardown, and leave the finally block responsible only
for stopping httpWorker and wsWorker. Ensure wsWorker.stop(wsParams) executes
even when the socket probe rejects, preserving the original failure and
preventing the host from remaining bound.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: fb9da62d-9e52-49c7-972c-ebc429c7de9c
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (41)
packages/http-transport/package.jsonpackages/http-transport/src/adapter.tspackages/http-transport/src/constants.tspackages/http-transport/src/runtimes/bun.tspackages/http-transport/src/runtimes/deno.tspackages/http-transport/src/runtimes/node.tspackages/http-transport/src/types.tspackages/http-transport/src/utils.tspackages/http-transport/tests/_helpers/test-utils.tspackages/http-transport/tests/node-runtime.spec.tspackages/neem/package.jsonpackages/neem/tests/e2e/fixtures/cases/shared-server/api.planner.tspackages/neem/tests/e2e/fixtures/cases/shared-server/api.runtime.tspackages/neem/tests/e2e/fixtures/cases/shared-server/neem.config.tspackages/neem/tests/e2e/fixtures/cases/shared-server/shared-server.worker.tspackages/neem/tests/e2e/shared-server.spec.tspackages/neem/tests/unit/proxy.spec.tspackages/server/package.jsonpackages/server/src/host.tspackages/server/src/index.tspackages/server/src/runtimes/bun.tspackages/server/src/runtimes/deno.tspackages/server/src/runtimes/node.tspackages/server/src/types.tspackages/server/src/utils.tspackages/server/tests/chunked-stream.spec.tspackages/server/tests/shared-host.spec.tspackages/server/tsconfig.build.jsonpackages/server/tsconfig.jsonpackages/server/vitest.config.tspackages/ws-transport/package.jsonpackages/ws-transport/src/adapter.tspackages/ws-transport/src/runtimes/bun.tspackages/ws-transport/src/runtimes/deno.tspackages/ws-transport/src/runtimes/node.tspackages/ws-transport/src/types.tspackages/ws-transport/src/utils.tspackages/ws-transport/tests/shared-server.spec.tspackages/ws-transport/tests/stream-e2e.spec.tstsconfig.build.jsontsconfig.json
- host: a start() racing the final stop() now reclaims the live socket instead of binding a second server the stop orphans; close() is serialized against rebinds - bun host: unix sockets report proto+unix:// like the other runtimes; /healthy responds to any method (deno already did, node switched to .any as well) - node host: PayloadTooLargeError from the body cap maps to 413 when the tenant rethrows it instead of a generic 500 - tests: race regression for the host, bounded per-attempt WS connect in the neem e2e, teardown ordering fix in the shared-server transport spec
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
1 participant
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
Extracts socket ownership out of the transports into a new
@nmtjs/serverpackage so the HTTP and WS transports can listen on a single socket instead of two.createServerHost({ listen, tls, maxRequestBodySize, runtime })(@nmtjs/server/node|bun|deno) owns everything previously duplicated across the six transport adapters: listen/TLS,/healthy, upgrade-vs-HTTP dispatch, the uWS→fetch body translation, crossws adapter creation, send-status interpretation and the uWS ws-behavior defaults.start()and closes on the laststop(). The runtime server is materialized at bind time from collected registrations (required by Bun's all-options-at-serve()constraint; also makes stop→start rebind cleanly).OneOf<[{ listen, tls, runtime }, { server }]>— standalone mode is unchanged, shared mode is additive:Gateway, protocol and both transport cores required no changes. Both workers report the same bound URL, so the gateway registers one address under both
httpandwsproxyable types, which the Neem proxy'sruntimeName:type:urlupstream keying already handles.Breaking changes
runtime.ws→ top-levelws(runtimenow consistently means server-level runtime options, mirroring the HTTP transport; on Bun the formerruntime.serveris nowruntime).corsoption — declared but never read anywhere (browsers don't apply CORS to WS upgrades).http+unix:///https+unix://(node HTTP previously reported bareunix://, WS reportedproto+unix://).Notes
PayloadTooLargeErrorand the response helpers moved to@nmtjs/serverand are re-exported from the transports — the 413 path matches byinstanceof, so class identity must be shared..nodebinary via a runtime-computed require the bundler cannot see. The e2e fixture demonstrates the pattern (build: { rolldown: { external: ['uWebSockets.js'] } }+ uWS in node_modules).Tests
packages/server: shared-host suite (HTTP + WS +/healthyon one socket, refcounted stop, rebind, no-tenant 404s, registration guards) plus the relocated uWS chunked-stream dispatcher tests.packages/ws-transport/tests/shared-server.spec.ts: both real transports mounted on one host — same reported URL, HTTP RPC and WS messages both served, one worker stopping doesn't kill the other's socket.packages/neem: unit test pinning same-URL dual-type upstream keying; e2e fixture boots a real Gateway (both transports, one host) behind the real native proxy and verifies both upstream types on one URL, HTTP RPC,/healthyand WS upgrade through the proxy port.Full workspace suite, typecheck, oxlint and oxfmt are green.
Summary by CodeRabbit
New Features
Bug Fixes
Tests