diff --git a/packages/bun-uws/src/WebSocketContextData.h b/packages/bun-uws/src/WebSocketContextData.h index b675f65dc9c8..47946d7c1653 100644 --- a/packages/bun-uws/src/WebSocketContextData.h +++ b/packages/bun-uws/src/WebSocketContextData.h @@ -88,7 +88,25 @@ struct WebSocketContextData { margin = (unsigned short) (margin << 1); } idleTimeoutComponents = { - idleTimeout - (sendPingsAutomatically ? margin : 0), /* reduce normal idleTimeout if it is extended by ping-timeout */ + /* idleTimeout == 0 is an intentional, distinct "off" value, not + * an ordinary small timeout: App.h's ws() validation terminates + * with "Error: idleTimeout must be either 0 or greater than 8!" + * if a caller passes anything in (0, 8) (see App.h:414-416), + * and Bun's own WebSocketServerContext.rs config-translation + * layer explicitly exempts 0 from its "round up to 8" clamp for + * the same reason. Special-case it rather than let it fall into + * the subtraction below: idleTimeout - margin + * underflows the unsigned short (0 - 4 == 65532), which + * us_socket_timeout would then treat as a very real ~252-second + * timeout (65532 seconds, tick-wheel-rounded) instead of no + * timeout at all. us_socket_timeout(s, 0) already means + * "disabled" (see socket.c), so pass 0 straight through. + * Only .first (the idle-detection arm) is affected: .second + * keeps its normal margin value below unchanged, since it also + * doubles as the post-end() force-close grace period (see + * WebSocket.h's end()), which is unrelated to idle-timeout and + * must keep firing regardless of idleTimeout. */ + idleTimeout == 0 ? 0 : idleTimeout - (sendPingsAutomatically ? margin : 0), /* reduce normal idleTimeout if it is extended by ping-timeout */ margin /* ping-timeout - also used for end() timeout */ }; } diff --git a/src/js/internal-for-testing.ts b/src/js/internal-for-testing.ts index 39663d1026fb..24663f1de370 100644 --- a/src/js/internal-for-testing.ts +++ b/src/js/internal-for-testing.ts @@ -23,6 +23,20 @@ export const xxHash3ForTesting: (view: ArrayBufferView, seed?: number | bigint) 2, ); +// uWS's WebSocketContextData::calculateIdleTimeoutComponents +// (packages/bun-uws/src/WebSocketContextData.h), driven directly so a test +// can assert the idleTimeout: 0 special-case deterministically -- the +// pre-fix regression's real-world symptom is a ~252s timeout, far too long +// for any test to wait out. +export const websocketIdleTimeoutComponentsForTesting: ( + idleTimeout: number, + sendPingsAutomatically: boolean, +) => [number, number] = $newCppFunction( + "websocket_idle_timeout_testing.cpp", + "Bun__websocketIdleTimeoutComponentsForTesting", + 2, +); + export const SQL = $cpp("JSSQLStatement.cpp", "createJSSQLStatementConstructor"); export const patchInternals = { diff --git a/src/jsc/bindings/websocket_idle_timeout_testing.cpp b/src/jsc/bindings/websocket_idle_timeout_testing.cpp new file mode 100644 index 000000000000..e5e9ad2a5eaa --- /dev/null +++ b/src/jsc/bindings/websocket_idle_timeout_testing.cpp @@ -0,0 +1,72 @@ +// Testing-only JS binding for uWS's +// WebSocketContextData::calculateIdleTimeoutComponents +// (packages/bun-uws/src/WebSocketContextData.h). +// +// idleTimeout: 0's pre-fix regression symptom is a real-world ~252-second +// timeout (65532, the unsigned-short underflow of 0 - 4, tick-wheel-rounded +// -- see the fix commit and the "websocket idleTimeout: 0" describe block in +// test/js/bun/websocket/websocket-server.test.ts) -- far too long for any +// test to wait out. This binding drives the fixed arithmetic directly and +// deterministically instead, so a test can assert idleTimeout: 0 produces +// idle-detection component 0 without any socket, timer, or wall-clock wait. +// +// calculateIdleTimeoutComponents is a non-static member, but it only reads +// sendPingsAutomatically and writes idleTimeoutComponents; the constructor +// stores nothing but the TopicTree* it's given, and that pointer is +// otherwise untouched by either. So a default-constructed instance with a +// nullptr topicTree drives it standalone, with no App/socket/loop +// scaffolding. SSL/USERDATA are picked to match Bun's own concrete +// instantiation of the sibling WebSocketContext template (see +// src/uws_sys/libuwsockets.cpp's `uWS::WebSocketContext` +// / `uWS::WebSocket`); neither template parameter is +// referenced by calculateIdleTimeoutComponents itself, so the SSL value +// chosen here (false) is arbitrary. +// +// Kept in its own TU (not folded into an existing bindings.cpp) so this +// testing-only entry point -- and its direct #include of a vendored uWS +// template header -- stays isolated, mirroring xxhash3_testing.cpp's +// separation of its testing entry point from xxhash3.cpp. + +#include "root.h" + +#include "websocket_idle_timeout_testing.h" + +#include + +#include "ZigGlobalObject.h" +#include "JavaScriptCore/JSObject.h" +#include "JavaScriptCore/ObjectConstructor.h" +#include "JavaScriptCore/ArrayConstructor.h" +#include + +namespace Bun { + +// (idleTimeout: number, sendPingsAutomatically: boolean) -> [number, number] +BUN_DEFINE_HOST_FUNCTION(Bun__websocketIdleTimeoutComponentsForTesting, (JSC::JSGlobalObject * globalObject, JSC::CallFrame* callFrame)) +{ + auto& vm = JSC::getVM(globalObject); + auto scope = DECLARE_THROW_SCOPE(vm); + + // toUInt32 is a defined conversion (no float-cast UB for NaN/Inf/negatives, + // matching xxhash3_testing.cpp's identical reasoning for its seed + // argument). The truncating cast to unsigned short below is then + // well-defined (modulo 65536), matching the real field type in + // WebSocketContextData::idleTimeoutComponents / its calculateIdleTimeoutComponents parameter. + uint32_t idleTimeout32 = callFrame->argument(0).toUInt32(globalObject); + RETURN_IF_EXCEPTION(scope, {}); + unsigned short idleTimeout = static_cast(idleTimeout32); + + bool sendPingsAutomatically = callFrame->argument(1).toBoolean(globalObject); + + uWS::WebSocketContextData data(nullptr); + data.sendPingsAutomatically = sendPingsAutomatically; + data.calculateIdleTimeoutComponents(idleTimeout); + + auto* result = JSC::JSArray::create(vm, globalObject->arrayStructureForIndexingTypeDuringAllocation(JSC::ArrayWithContiguous), 2); + result->putDirectIndex(globalObject, 0, JSC::jsNumber(data.idleTimeoutComponents.first)); + result->putDirectIndex(globalObject, 1, JSC::jsNumber(data.idleTimeoutComponents.second)); + + RELEASE_AND_RETURN(scope, JSC::JSValue::encode(result)); +} + +} // namespace Bun diff --git a/src/jsc/bindings/websocket_idle_timeout_testing.h b/src/jsc/bindings/websocket_idle_timeout_testing.h new file mode 100644 index 000000000000..05dd744bbc5f --- /dev/null +++ b/src/jsc/bindings/websocket_idle_timeout_testing.h @@ -0,0 +1,18 @@ +#pragma once + +#include "root.h" + +namespace Bun { + +// Testing-only entry point for uWS's +// WebSocketContextData::calculateIdleTimeoutComponents +// (packages/bun-uws/src/WebSocketContextData.h), exposed via +// `bun:internal-for-testing` so a test can assert its output directly +// instead of waiting out the ~252-second real-world symptom of the pre-fix +// unsigned-short underflow (see websocket_idle_timeout_testing.cpp). +// Signature: (idleTimeout: number, sendPingsAutomatically: boolean) -> +// [number, number] (the idle-detection component and the +// ping/end()-grace-period component, in that order). +BUN_DECLARE_HOST_FUNCTION(Bun__websocketIdleTimeoutComponentsForTesting); + +} // namespace Bun diff --git a/test/js/bun/websocket/websocket-server.test.ts b/test/js/bun/websocket/websocket-server.test.ts index 2043fd4a1ed3..a1c4ab605545 100644 --- a/test/js/bun/websocket/websocket-server.test.ts +++ b/test/js/bun/websocket/websocket-server.test.ts @@ -1,5 +1,6 @@ import type { Server, ServerWebSocket, Subprocess, WebSocketHandler } from "bun"; import { serve, spawn } from "bun"; +import { websocketIdleTimeoutComponentsForTesting } from "bun:internal-for-testing"; import { afterEach, describe, expect, it } from "bun:test"; import { bunEnv, bunExe, forceGuardMalloc, isWindows, tempDir } from "harness"; import net, { isIP } from "node:net"; @@ -1759,6 +1760,199 @@ describe.concurrent("publish() return value reflects subscriber backpressure", ( }); }); +// Regression coverage for `websocket: { idleTimeout: 0 }`. +// +// The bug: uWS's WebSocketContextData::calculateIdleTimeoutComponents(0) +// computed `idleTimeout - margin` (0 - 4) on an `unsigned short`, which +// underflows to 65532. uSockets' tick wheel (4-second granularity, `% 240` +// slots -- see packages/bun-usockets/src/socket.c's us_socket_timeout) turned +// that into a *real* ~252-second timeout instead of "disabled": every +// `idleTimeout: 0` websocket got a ping at ~248s and, if unanswered, a +// force-close with ERR_WEBSOCKET_TIMEOUT four seconds later -- instead of +// never timing out. `idleTimeout: 0` is an intentional, distinct "off" +// value, not an ordinary small timeout: App.h's ws() validation terminates +// with "Error: idleTimeout must be either 0 or greater than 8!" for anything +// in (0, 8) (see App.h:414-416), and the "uws does not allow idleTimeout to +// be between (0, 8)" comment in WebSocketServerContext.rs exempts 0 from its +// "round up to 8" clamp for the same reason. +// +// The regression guard for the actual underflow is the "unit: +// calculateIdleTimeoutComponents" test just below: it drives +// WebSocketContextData::calculateIdleTimeoutComponents directly (via +// `websocketIdleTimeoutComponentsForTesting`, backed by +// websocket_idle_timeout_testing.cpp, exposed through +// bun:internal-for-testing) and asserts idleTimeout: 0 produces +// idle-detection component 0, not 65532 (the unsigned-short underflow of +// 0 - 4) -- deterministically, with no socket, timer, or wall-clock wait. +// That test fails on an unfixed build and passes on this one: the true +// red-before/green-after contrast this bug needed, which a bounded-wait +// socket test cannot provide (see below). +// +// The two `it.concurrent` tests below the unit test are NOT redundant with +// it and are deliberately kept: they are end-to-end wiring coverage, proving +// the fixed arithmetic actually reaches a real socket through Bun.serve -> +// uWS -> uSockets, against a deliberately unresponsive raw-socket client: +// 1) a small nonzero idleTimeout (8s -- uWS's minimum granularity, same as +// the "should allow use of custom timeout" test in +// test/js/bun/http/serve.test.ts:2598) still pings and +// then force-closes an idle connection, within a bounded window -- i.e. +// the ping/close mechanism itself functions; +// 2) idleTimeout: 0, under the exact same conditions, produces neither a +// ping nor a close within that same bounded window. +// By construction this pair cannot distinguish "genuinely disabled" from +// "still broken but with some timeout longer than our wait window" -- the +// historical bug's real symptom is ~252s, far past any window a test suite +// should wait -- which is exactly why the unit test above exists to give the +// definitive answer. This pair's job is instead to catch a *class* of future +// regression in the wiring itself -- e.g. the ping/close mechanism breaking, +// or idleTimeout: 0 no longer reaching calculateIdleTimeoutComponents at all +// -- not to re-derive the original forensic timing measurement. +describe.concurrent("websocket idleTimeout: 0", () => { + it("unit: calculateIdleTimeoutComponents(0, ...) yields idle-detection component 0, not 65532", () => { + // Drives WebSocketContextData::calculateIdleTimeoutComponents directly, + // with no socket, timer, or wall-clock wait -- see + // src/jsc/bindings/websocket_idle_timeout_testing.cpp. On an unfixed + // build, idleTimeout: 0 underflows the unsigned short (0 - margin) to + // 65532 here; on the fixed build it's 0. The second (ping/end()-grace) + // component is unaffected by the fix and stays at the margin (4, uWS's + // minimum) either way. + expect(websocketIdleTimeoutComponentsForTesting(0, true)).toEqual([0, 4]); + expect(websocketIdleTimeoutComponentsForTesting(0, false)).toEqual([0, 4]); + }); + + async function connectRaw(port: number): Promise { + return await new Promise((resolve, reject) => { + const socket = net.connect({ port, host: "127.0.0.1" }, () => resolve(socket)); + socket.on("error", reject); + }); + } + + async function handshake(socket: net.Socket): Promise { + socket.write( + "GET / HTTP/1.1\r\n" + + "Host: x\r\n" + + "Upgrade: websocket\r\n" + + "Connection: Upgrade\r\n" + + "Sec-WebSocket-Key: dGhlIHNhbXBsZSBub25jZQ==\r\n" + + "Sec-WebSocket-Version: 13\r\n\r\n", + ); + await new Promise((resolve, reject) => { + let response = ""; + function onData(d: Buffer) { + response += d.toString("latin1"); + if (!response.includes("\r\n\r\n")) return; + socket.off("data", onData); + if (response.includes(" 101 ")) resolve(); + else reject(new Error("upgrade failed: " + response)); + } + socket.on("data", onData); + socket.on("error", reject); + }); + } + + // Silently collects any bytes the server sends and reports whether the + // socket was closed by the server, up to `ms` milliseconds. Never rejects: + // a server-initiated abrupt close (RST) surfaces as `closed: true` rather + // than an uncaught "error" event. + function waitForCloseOrTimeout(socket: net.Socket, ms: number): Promise<{ closed: boolean; bytes: Buffer }> { + return new Promise(resolve => { + let bytes = Buffer.alloc(0); + let finished = false; + + const onData = (d: Buffer) => { + bytes = Buffer.concat([bytes, d]); + }; + const onCloseLike = () => finish(true); + const finish = (closed: boolean) => { + if (finished) return; + finished = true; + socket.off("data", onData); + socket.off("close", onCloseLike); + socket.off("error", onCloseLike); + clearTimeout(timer); + resolve({ closed, bytes }); + }; + const timer = setTimeout(() => finish(false), ms); + + socket.on("data", onData); + socket.on("close", onCloseLike); + socket.on("error", onCloseLike); + }); + } + + it.concurrent( + "sanity: a small nonzero idleTimeout still pings then force-closes an unresponsive socket", + async () => { + await using server = serve({ + port: 0, + websocket: { + idleTimeout: 8, // uws's minimum reliable granularity -- see "should allow use of custom timeout" in test/js/bun/http/serve.test.ts:2598 + open() {}, + message() {}, + }, + fetch(req, server) { + if (server.upgrade(req)) return; + return new Response("no upgrade", { status: 400 }); + }, + }); + + const socket = await connectRaw(server.port); + try { + await handshake(socket); + + // Deliberately never read-and-respond meaningfully after the + // handshake: don't send a pong, don't send anything. The server's + // idle timer should fire an unmasked ping frame (0x89 0x00), get no + // reply, and force-close. + const { closed, bytes } = await waitForCloseOrTimeout(socket, 20_000); + + expect(closed).toBeTrue(); + // We should have observed the ping frame before the close. + expect(bytes.length).toBeGreaterThanOrEqual(2); + expect(bytes[0]).toBe(0x89); // FIN(1) + opcode PING(0x9) + expect(bytes[1]).toBe(0x00); // zero-length, unmasked (server->client) + } finally { + socket.destroy(); + } + }, + 30_000, + ); + + it.concurrent( + "idleTimeout: 0 pings or force-closes neither, under the same conditions", + async () => { + await using server = serve({ + port: 0, + websocket: { + idleTimeout: 0, + open() {}, + message() {}, + }, + fetch(req, server) { + if (server.upgrade(req)) return; + return new Response("no upgrade", { status: 400 }); + }, + }); + + const socket = await connectRaw(server.port); + try { + await handshake(socket); + + // Same bounded wait as the sanity case above -- long enough that the + // 8s case reliably closes within it, so an idleTimeout: 0 websocket + // sharing that same (buggy) code path would also have fired by now. + const { closed, bytes } = await waitForCloseOrTimeout(socket, 20_000); + + expect(closed).toBeFalse(); + expect(bytes.length).toBe(0); + } finally { + socket.destroy(); + } + }, + 30_000, + ); +}); + // https://github.com/oven-sh/bun/issues/34158 it.each(["server", "client"] as const)( "server.stop() promise resolves after the last websocket closes (%s-initiated close)",