From 485017f93f720625cd94bb288b44be8bcbea5fc3 Mon Sep 17 00:00:00 2001 From: Claude Bot Date: Sun, 26 Oct 2025 02:56:39 +0000 Subject: [PATCH] fix: correct route registration order for WebSocket handlers MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The key issue was understanding how uWebSockets handles WebSocket routes: When app.ws() is called, it internally registers a GET handler that: 1. Checks for WebSocket upgrade headers (sec-websocket-key) 2. If present → handles WebSocket upgrade via the upgrade callback 3. If absent → calls req->setYield(true) to pass to next handler This means app.ws() must be registered BEFORE app.method(.GET) for the same path, so that: - WebSocket requests → handled by app.ws() - Regular GET requests → yield and fall through to GET handler Changes: - Register upgrade handler FIRST in user_routes_to_build (before method handlers) - This ensures correct registration order in server.zig - Removed websocket_only enum variant (not needed with correct ordering) - Updated test to expect error on server creation (not connection time) - Validate that route websocket requires upgrade handler All 8 tests now passing: ✓ route-specific websocket handlers work independently ✓ route-specific websocket with data in upgrade ✓ route-specific websocket with close handler ✓ global websocket handler still works ✓ mix of route-specific and global websocket handlers ✓ route-specific websocket with multiple HTTP methods ✓ route-specific websocket without upgrade handler errors ✓ server.reload() preserves route-specific websocket handlers 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude --- src/bun.js/api/server/ServerConfig.zig | 61 +++++++++---------- .../js/bun/http/serve-route-websocket.test.ts | 41 ++++++------- 2 files changed, 45 insertions(+), 57 deletions(-) diff --git a/src/bun.js/api/server/ServerConfig.zig b/src/bun.js/api/server/ServerConfig.zig index 0a74d43c2e..d5d1a75635 100644 --- a/src/bun.js/api/server/ServerConfig.zig +++ b/src/bun.js/api/server/ServerConfig.zig @@ -583,7 +583,6 @@ pub fn fromJS( var found = false; var websocket_ctx: ?WebSocketServerContext = null; var upgrade_callback: jsc.Strong.Optional = .empty; - var has_get_method = false; // Check for websocket and upgrade fields if (try value.getOwn(global, "websocket")) |ws_value| { @@ -598,25 +597,48 @@ pub fn fromJS( } } + // Validate: if route has websocket, it must have upgrade + if (websocket_ctx != null and upgrade_callback.impl == null) { + return global.throwInvalidArguments("Route has 'websocket' but missing 'upgrade' handler. Both must be specified together.", .{}); + } + + // If we have an upgrade handler, add it FIRST (before other method handlers) + // This ensures app.ws() is registered before app.method(.GET), so WebSocket + // upgrade requests are handled by app.ws(), and regular GET requests yield + // and fall through to the GET handler + if (upgrade_callback.impl != null) { + if (!found) { + try validateRouteName(global, path); + } + args.user_routes_to_build.append(.{ + .route = .{ + .path = bun.handleOom(bun.default_allocator.dupeZ(u8, path)), + .method = .{ .specific = .GET }, + }, + .callback = upgrade_callback, + .websocket = websocket_ctx, // May be null (uses global) + }) catch |err| bun.handleOom(err); + found = true; + } + + // Process HTTP method handlers (registered after upgrade handler) inline for (methods) |method| { if (try value.getOwn(global, @tagName(method))) |function| { if (!found) { try validateRouteName(global, path); } found = true; - if (method == .GET) has_get_method = true; if (function.isCallable()) { - // Only attach websocket to GET method (WebSocket upgrades use GET) - const ws_for_this_method = if (method == .GET) websocket_ctx else null; - + // Never attach websocket to method handlers + // WebSocket is handled separately via the upgrade callback args.user_routes_to_build.append(.{ .route = .{ .path = bun.handleOom(bun.default_allocator.dupeZ(u8, path)), .method = .{ .specific = method }, }, .callback = .create(function.withAsyncContextIfNeeded(global), global), - .websocket = ws_for_this_method, + .websocket = null, }) catch |err| bun.handleOom(err); } else if (try AnyRoute.fromJS(global, path, function, init_ctx)) |html_route| { var method_set = bun.http.Method.Set.initEmpty(); @@ -631,33 +653,6 @@ pub fn fromJS( } } - // If we have websocket/upgrade but no GET handler, create one - // WebSocket upgrades require GET method - if (found and !has_get_method and (websocket_ctx != null or upgrade_callback.impl != null)) { - args.user_routes_to_build.append(.{ - .route = .{ - .path = bun.handleOom(bun.default_allocator.dupeZ(u8, path)), - .method = .{ .specific = .GET }, - }, - .callback = upgrade_callback, - .websocket = websocket_ctx, - }) catch |err| bun.handleOom(err); - } - - // If we have websocket/upgrade but no method handlers at all, create a route for them - if (!found and (websocket_ctx != null or upgrade_callback.impl != null)) { - try validateRouteName(global, path); - args.user_routes_to_build.append(.{ - .route = .{ - .path = bun.handleOom(bun.default_allocator.dupeZ(u8, path)), - .method = .any, - }, - .callback = upgrade_callback, - .websocket = websocket_ctx, - }) catch |err| bun.handleOom(err); - found = true; - } - if (found) { bun.default_allocator.free(path); continue; diff --git a/test/js/bun/http/serve-route-websocket.test.ts b/test/js/bun/http/serve-route-websocket.test.ts index fb9241aeb9..ecffa23b18 100644 --- a/test/js/bun/http/serve-route-websocket.test.ts +++ b/test/js/bun/http/serve-route-websocket.test.ts @@ -268,33 +268,26 @@ describe("Bun.serve() route-specific WebSocket handlers", () => { ws.close(); }); - test("route-specific websocket without upgrade handler errors appropriately", async () => { - using server = Bun.serve({ - port: 0, - routes: { - "/ws": { - websocket: { - open(ws) { - ws.send("should not reach here"); + test("route-specific websocket without upgrade handler errors appropriately", () => { + // Should throw an error because websocket requires upgrade handler + expect(() => { + Bun.serve({ + port: 0, + routes: { + "/ws": { + websocket: { + open(ws) { + ws.send("should not reach here"); + }, + }, + // Note: no upgrade handler + GET() { + return new Response("This is not a WebSocket endpoint"); }, }, - // Note: no upgrade handler - GET() { - return new Response("This is not a WebSocket endpoint"); - }, }, - }, - }); - - // This should fail to upgrade since there's no upgrade() handler - const ws = new WebSocket(`ws://localhost:${server.port}/ws`); - let errorOccurred = false; - ws.onerror = () => { - errorOccurred = true; - }; - - await new Promise(resolve => setTimeout(resolve, 500)); - expect(errorOccurred).toBe(true); + }); + }).toThrow("Route has 'websocket' but missing 'upgrade' handler"); }); test("server.reload() preserves route-specific websocket handlers", async () => {