Fix missing Connection and Keep-Alive headers in node:http #23671
Conversation
WalkthroughAdds tests for Connection/Keep-Alive header behavior and threads explicit keep-alive control (shouldKeepAlive, keepAliveTimeout) through the HTTP header emission path by extending writeFetchHeadersToUWSResponse, updating writeHead signatures across C++/Zig/TypeScript, and passing server keepAliveTimeout into writeHead calls. Changes
Suggested reviewers
Pre-merge checks❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
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: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
src/js/node/_http_server.ts(5 hunks)test/js/node/http/node-http-connection-headers.test.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (12)
test/**
📄 CodeRabbit inference engine (.cursor/rules/writing-tests.mdc)
Place all tests under the test/ directory
Files:
test/js/node/http/node-http-connection-headers.test.ts
test/js/**/*.{js,ts}
📄 CodeRabbit inference engine (.cursor/rules/writing-tests.mdc)
Place JavaScript and TypeScript tests under test/js/
Files:
test/js/node/http/node-http-connection-headers.test.ts
test/js/node/**/*.{js,ts}
📄 CodeRabbit inference engine (.cursor/rules/writing-tests.mdc)
Place Node.js module compatibility tests under test/js/node/, separated by module (e.g., test/js/node/assert/)
Files:
test/js/node/http/node-http-connection-headers.test.ts
test/**/*.{js,ts}
📄 CodeRabbit inference engine (.cursor/rules/writing-tests.mdc)
test/**/*.{js,ts}: Write tests in JavaScript or TypeScript using Bun’s Jest-style APIs (test, describe, expect) and run with bun test
Prefer data-driven tests (e.g., test.each) to reduce boilerplate
Use shared utilities from test/harness.ts where applicable
Files:
test/js/node/http/node-http-connection-headers.test.ts
test/**/*.test.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
test/**/*.test.{ts,tsx}: Test files must live under test/ and end with .test.ts or .test.tsx
In tests, always use port: 0; do not hardcode ports or roll your own random port
Prefer normalizeBunSnapshot for snapshotting test output instead of asserting raw strings
Do not write tests that assert absence of crashes (e.g., 'no panic' or 'no uncaught exception')
Use Bun’s Jest-compatible runner (import { test, expect } from "bun:test") for tests
Avoid shell commands like find or grep in tests; use Bun’s Glob and built-in tools instead
Prefer running tests via bun bd test and use provided harness utilities (bunEnv, bunExe, tempDir)
Use Bun.spawn with proper stdio handling and await proc.exited in process-spawning tests
Files:
test/js/node/http/node-http-connection-headers.test.ts
test/js/node/**
📄 CodeRabbit inference engine (CLAUDE.md)
Place Node.js compatibility tests under test/js/node/
Files:
test/js/node/http/node-http-connection-headers.test.ts
test/**/*.test.{ts,js,jsx,tsx,mjs,cjs}
📄 CodeRabbit inference engine (test/CLAUDE.md)
test/**/*.test.{ts,js,jsx,tsx,mjs,cjs}: Usebun:testfor files ending with*.test.{ts,js,jsx,tsx,mjs,cjs}
Prefer concurrent tests (test.concurrent/describe.concurrent) over sequential when feasible
Organize tests withdescribeblocks to group related tests
Use utilities likedescribe.each,toMatchSnapshot, and lifecycle hooks (beforeAll,beforeEach,afterEach) and track resources for cleanup
Files:
test/js/node/http/node-http-connection-headers.test.ts
test/**/*.{ts,tsx,js,jsx,mjs,cjs}
📄 CodeRabbit inference engine (test/CLAUDE.md)
For large/repetitive strings, use
Buffer.alloc(count, fill).toString()instead of"A".repeat(count)
Files:
test/js/node/http/node-http-connection-headers.test.ts
test/js/{bun,node}/**
📄 CodeRabbit inference engine (test/CLAUDE.md)
Organize unit tests by module under
/test/js/bun/and/test/js/node/
Files:
test/js/node/http/node-http-connection-headers.test.ts
src/js/node/**
📄 CodeRabbit inference engine (CLAUDE.md)
Organize built-in Node.js compatibility modules under src/js/node/ (e.g., node:fs, node:path)
Files:
src/js/node/_http_server.ts
src/js/node/**/*.{ts,js}
📄 CodeRabbit inference engine (src/js/CLAUDE.md)
Place Node.js compatibility modules (e.g., node:fs, node:path) under node/
Files:
src/js/node/_http_server.ts
src/js/{builtins,node,bun,thirdparty,internal}/**/*.{ts,js}
📄 CodeRabbit inference engine (src/js/CLAUDE.md)
src/js/{builtins,node,bun,thirdparty,internal}/**/*.{ts,js}: Use require() with string literals only (no dynamic requires)
Do not use ESM import syntax; write modules as CommonJS with export default { ... }
Export via export default {} for modules
Use .$call and .$apply; never use .call or .apply
Prefer JSC intrinsics/private $ APIs for performance (e.g., $Array.from, map.$set, $newArrayWithSize, $debug, $assert)
Validate callbacks with $isCallable and throw $ERR_INVALID_ARG_TYPE with the correct parameter name and expected type
Use process.platform and process.arch for platform detection (rely on inlining/dead-code elimination)
Files:
src/js/node/_http_server.ts
🧬 Code graph analysis (1)
src/js/node/_http_server.ts (2)
src/js/node/_http_outgoing.ts (2)
headers(314-318)headers(319-321)src/js/internal/http.ts (2)
headersSymbol(389-389)Headers(365-365)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Format
🔇 Additional comments (2)
src/js/node/_http_server.ts (1)
1217-1238: LGTM - Helper function and call sites are correctly implemented.The
ensureConnectionHeadershelper correctly:
- Respects user-set Connection headers by checking
hasHeader("connection")first- Lazily creates the headers object when needed
- Uses optional chaining to safely access
response.socket?.server- Converts milliseconds to seconds with
Math.floor(HTTP spec requires integer seconds)- Is invoked at all critical write paths (end, write, _send, flushHeaders) before headers are sent
This implementation aligns with Node.js's
_http_outgoing.jsbehavior as described in the PR objectives.Also applies to: 1396-1396, 1507-1507, 1611-1611, 1679-1679
test/js/node/http/node-http-connection-headers.test.ts (1)
1-178: LGTM - Comprehensive test coverage for Connection header behavior.The test suite thoroughly validates the new header management functionality:
- Default keep-alive behavior with 5-second timeout
- Respect for user-set Connection headers
- Custom server keepAliveTimeout configuration
- POST requests with body handling
- Both explicit and implicit writeHead scenarios
All tests properly:
- Use
port: 0for dynamic port allocation (as per coding guidelines)- Clean up servers in finally blocks
- Use bun:test APIs correctly
- Follow a consistent and readable pattern
src/js/node/_http_server.ts
Outdated
| // Add Connection and Keep-Alive headers if not already set | ||
| // This matches Node.js behavior in _http_outgoing.js _storeHeader() | ||
| if (!response.hasHeader("connection")) { | ||
| const shouldKeepAlive = response.shouldKeepAlive; | ||
| // Lazily create headers if they don't exist | ||
| const headers = response[headersSymbol] ?? (response[headersSymbol] = new Headers()); | ||
|
|
||
| if (shouldKeepAlive) { | ||
| headers.set("Connection", "keep-alive"); | ||
|
|
||
| // Add Keep-Alive timeout if server has it configured | ||
| const server = response.socket?.server; | ||
| if (server?.keepAliveTimeout) { | ||
| const timeoutSeconds = Math.floor(server.keepAliveTimeout / 1000); | ||
| headers.set("Keep-Alive", `timeout=${timeoutSeconds}`); | ||
| } | ||
| } else { | ||
| headers.set("Connection", "close"); | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Eliminate code duplication by calling the helper function.
The logic here is identical to the ensureConnectionHeaders helper defined below (lines 1217-1238). This violates the DRY principle and makes the code harder to maintain.
Apply this diff to use the helper function instead:
- // Add Connection and Keep-Alive headers if not already set
- // This matches Node.js behavior in _http_outgoing.js _storeHeader()
- if (!response.hasHeader("connection")) {
- const shouldKeepAlive = response.shouldKeepAlive;
- // Lazily create headers if they don't exist
- const headers = response[headersSymbol] ?? (response[headersSymbol] = new Headers());
-
- if (shouldKeepAlive) {
- headers.set("Connection", "keep-alive");
-
- // Add Keep-Alive timeout if server has it configured
- const server = response.socket?.server;
- if (server?.keepAliveTimeout) {
- const timeoutSeconds = Math.floor(server.keepAliveTimeout / 1000);
- headers.set("Keep-Alive", `timeout=${timeoutSeconds}`);
- }
- } else {
- headers.set("Connection", "close");
- }
- }
+ ensureConnectionHeaders(response);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Add Connection and Keep-Alive headers if not already set | |
| // This matches Node.js behavior in _http_outgoing.js _storeHeader() | |
| if (!response.hasHeader("connection")) { | |
| const shouldKeepAlive = response.shouldKeepAlive; | |
| // Lazily create headers if they don't exist | |
| const headers = response[headersSymbol] ?? (response[headersSymbol] = new Headers()); | |
| if (shouldKeepAlive) { | |
| headers.set("Connection", "keep-alive"); | |
| // Add Keep-Alive timeout if server has it configured | |
| const server = response.socket?.server; | |
| if (server?.keepAliveTimeout) { | |
| const timeoutSeconds = Math.floor(server.keepAliveTimeout / 1000); | |
| headers.set("Keep-Alive", `timeout=${timeoutSeconds}`); | |
| } | |
| } else { | |
| headers.set("Connection", "close"); | |
| } | |
| } | |
| ensureConnectionHeaders(response); |
🤖 Prompt for AI Agents
In src/js/node/_http_server.ts around lines 1191 to 1210, the block that adds
Connection and Keep-Alive headers duplicates the logic from the
ensureConnectionHeaders helper (defined below). Replace the duplicated logic by
calling ensureConnectionHeaders(response) (or the exact helper name in scope) at
this location, removing the manual header manipulation; ensure the helper is in
scope or import/hoist it if necessary so behavior remains identical (including
handling response.shouldKeepAlive and server.keepAliveTimeout).
…plementation) Fixes #19540 This change adds automatic `Connection` and `Keep-Alive` headers to HTTP responses when using Bun's `node:http` module, matching Node.js behavior. **Implementation is in native C++ code**. **Problem**: Bun's node:http implementation was missing the automatic addition of Connection and Keep-Alive headers that Node.js includes by default. This caused compatibility issues with frameworks like NestJS and strict HTTP clients like Chrome. **Solution**: - Modified `NodeHTTPServer__writeHead()` in NodeHTTP.cpp to automatically add Connection headers after writing user headers - Sets `Connection: keep-alive` when no Connection header is present - Sets `Keep-Alive: timeout=5` (default timeout) - Respects user-set Connection headers (doesn't override them) - Handles both FetchHeaders objects and plain JavaScript objects **Implementation Details**: - Logic added to `writeFetchHeadersToUWSResponse()` template function - Checks for existing Connection header (case-insensitive) - Adds default headers if not found - Hardcoded 5-second timeout matches Node.js default **Testing**: - Updated test suite in node-http-connection-headers.test.ts - All 7 tests pass - Tests cover default behavior, user overrides, and implicit/explicit writeHead scenarios - No regressions in existing node:http tests 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
robobun
force-pushed
the
claude/fix-missing-connection-keepalive-headers
branch
from
c67dee4 to
4bca618
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
src/bun.js/bindings/NodeHTTP.cpp (1)
674-680: Same missing conditional logic as lines 585-591.This code segment has the same critical issues:
- No check for
shouldKeepAliveflag- Hardcoded timeout instead of using server's
keepAliveTimeout- No handling of
Connection: closecaseRefer to the comment on lines 585-591 for the suggested fix. The same conditional logic needs to be applied here.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
src/bun.js/bindings/NodeHTTP.cpp(6 hunks)test/js/node/http/node-http-connection-headers.test.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (12)
test/**
📄 CodeRabbit inference engine (.cursor/rules/writing-tests.mdc)
Place all tests under the test/ directory
Files:
test/js/node/http/node-http-connection-headers.test.ts
test/js/**/*.{js,ts}
📄 CodeRabbit inference engine (.cursor/rules/writing-tests.mdc)
Place JavaScript and TypeScript tests under test/js/
Files:
test/js/node/http/node-http-connection-headers.test.ts
test/js/node/**/*.{js,ts}
📄 CodeRabbit inference engine (.cursor/rules/writing-tests.mdc)
Place Node.js module compatibility tests under test/js/node/, separated by module (e.g., test/js/node/assert/)
Files:
test/js/node/http/node-http-connection-headers.test.ts
test/**/*.{js,ts}
📄 CodeRabbit inference engine (.cursor/rules/writing-tests.mdc)
test/**/*.{js,ts}: Write tests in JavaScript or TypeScript using Bun’s Jest-style APIs (test, describe, expect) and run with bun test
Prefer data-driven tests (e.g., test.each) to reduce boilerplate
Use shared utilities from test/harness.ts where applicable
Files:
test/js/node/http/node-http-connection-headers.test.ts
test/**/*.test.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
test/**/*.test.{ts,tsx}: Test files must live under test/ and end with .test.ts or .test.tsx
In tests, always use port: 0; do not hardcode ports or roll your own random port
Prefer normalizeBunSnapshot for snapshotting test output instead of asserting raw strings
Do not write tests that assert absence of crashes (e.g., 'no panic' or 'no uncaught exception')
Use Bun’s Jest-compatible runner (import { test, expect } from "bun:test") for tests
Avoid shell commands like find or grep in tests; use Bun’s Glob and built-in tools instead
Prefer running tests via bun bd test and use provided harness utilities (bunEnv, bunExe, tempDir)
Use Bun.spawn with proper stdio handling and await proc.exited in process-spawning tests
Files:
test/js/node/http/node-http-connection-headers.test.ts
test/js/node/**
📄 CodeRabbit inference engine (CLAUDE.md)
Place Node.js compatibility tests under test/js/node/
Files:
test/js/node/http/node-http-connection-headers.test.ts
test/**/*.test.{ts,js,jsx,tsx,mjs,cjs}
📄 CodeRabbit inference engine (test/CLAUDE.md)
test/**/*.test.{ts,js,jsx,tsx,mjs,cjs}: Usebun:testfor files ending with*.test.{ts,js,jsx,tsx,mjs,cjs}
Prefer concurrent tests (test.concurrent/describe.concurrent) over sequential when feasible
Organize tests withdescribeblocks to group related tests
Use utilities likedescribe.each,toMatchSnapshot, and lifecycle hooks (beforeAll,beforeEach,afterEach) and track resources for cleanup
Files:
test/js/node/http/node-http-connection-headers.test.ts
test/**/*.{ts,tsx,js,jsx,mjs,cjs}
📄 CodeRabbit inference engine (test/CLAUDE.md)
For large/repetitive strings, use
Buffer.alloc(count, fill).toString()instead of"A".repeat(count)
Files:
test/js/node/http/node-http-connection-headers.test.ts
test/js/{bun,node}/**
📄 CodeRabbit inference engine (test/CLAUDE.md)
Organize unit tests by module under
/test/js/bun/and/test/js/node/
Files:
test/js/node/http/node-http-connection-headers.test.ts
**/*.{cpp,h}
📄 CodeRabbit inference engine (.cursor/rules/javascriptcore-class.mdc)
**/*.{cpp,h}: When exposing a JS class with public Constructor and Prototype, define three C++ types: class Foo : public JSC::DestructibleObject (if it has C++ fields), class FooPrototype : public JSC::JSNonFinalObject, and class FooConstructor : public JSC::InternalFunction
If the class has C++ data members, inherit from JSC::DestructibleObject and provide proper destruction; if it has no C++ fields (only JS properties), avoid a class and use JSC::constructEmptyObject(vm, structure) with putDirectOffset
Prefer placing the subspaceFor implementation in the .cpp file rather than the header when possible
Files:
src/bun.js/bindings/NodeHTTP.cpp
**/*.cpp
📄 CodeRabbit inference engine (.cursor/rules/javascriptcore-class.mdc)
**/*.cpp: Include "root.h" at the top of C++ binding files to satisfy lints
Define prototype properties using a const HashTableValue array and declare accessors/functions with JSC_DECLARE_* macros
Prototype classes should subclass JSC::JSNonFinalObject, provide create/createStructure, DECLARE_INFO, finishCreation that reifies static properties, and set mayBePrototype on the Structure
Custom getters should use JSC_DEFINE_CUSTOM_GETTER, jsDynamicCast to validate this, and throwThisTypeError on mismatch
Custom setters should use JSC_DEFINE_CUSTOM_SETTER, validate this via jsDynamicCast, and store via WriteBarrier/set semantics
Prototype functions should use JSC_DEFINE_HOST_FUNCTION, validate this with jsDynamicCast, and return encoded JSValue
Constructors should subclass JSC::InternalFunction, return internalFunctionSpace in subspaceFor, set the prototype property as non-configurable/non-writable, and provide create/createStructure
Provide a setup function that builds the Prototype, Constructor, and Structure, and assigns them to the LazyClassStructure initializer
Use the cached Structure via globalObject->m_.get(globalObject) when constructing instances
Expose constructors to Zig via an extern "C" function that returns the constructor from the LazyClassStructure
Provide an extern "C" Bun____toJS function that creates an instance using the cached Structure and returns an EncodedJSValue
Files:
src/bun.js/bindings/NodeHTTP.cpp
src/bun.js/bindings/**/*.cpp
📄 CodeRabbit inference engine (CLAUDE.md)
src/bun.js/bindings/**/*.cpp: When a JS class has a public constructor, implement Foo, FooPrototype, and FooConstructor (JSDestructibleObject, JSNonFinalObject, InternalFunction)
Define class properties using HashTableValue arrays in C++ bindings
Add iso subspaces for classes with C++ fields
Cache structures in ZigGlobalObject for C++-backed classes
Files:
src/bun.js/bindings/NodeHTTP.cpp
🧬 Code graph analysis (1)
src/bun.js/bindings/NodeHTTP.cpp (3)
src/js/node/http2.ts (1)
headers(377-379)src/js/node/_http_outgoing.ts (2)
headers(314-318)headers(319-321)src/bun.js/bindings/webcore/HTTPHeaderNames.cpp (2)
httpHeaderNameString(689-696)httpHeaderNameString(689-689)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Format
🔇 Additional comments (5)
src/bun.js/bindings/NodeHTTP.cpp (5)
529-529: LGTM!Proper initialization of the tracking variable.
537-539: LGTM!Correct detection of existing Connection header in common headers using enum comparison.
578-580: LGTM!Proper case-insensitive detection of Connection header in uncommon headers.
611-611: Skip commenting.Same pattern as line 529 - already reviewed.
615-615: Correct call to updated function.Properly passes
trueforaddConnectionHeadersparameter. However, this call will inherit the issues flagged in thewriteFetchHeadersToUWSResponsefunction regarding missingshouldKeepAlivelogic and hardcoded timeout.Ensure that the issues in
writeFetchHeadersToUWSResponse(lines 585-591) are addressed to make this call fully functional.
| import { expect, test } from "bun:test"; | ||
| import { createServer } from "node:http"; | ||
|
|
||
| test("should include Connection: keep-alive and Keep-Alive headers by default", async () => { | ||
| const server = createServer((req, res) => { | ||
| res.writeHead(200, { "Content-Type": "text/plain" }); | ||
| res.end("Hello World"); | ||
| }); | ||
|
|
||
| await new Promise<void>(resolve => server.listen(0, () => resolve())); | ||
| const port = (server.address() as any).port; | ||
|
|
||
| try { | ||
| const response = await fetch(`http://localhost:${port}/`); | ||
| const text = await response.text(); | ||
|
|
||
| expect(text).toBe("Hello World"); | ||
| expect(response.headers.get("connection")).toBe("keep-alive"); | ||
| const keepAlive = response.headers.get("keep-alive"); | ||
| expect(keepAlive).toMatch(/timeout=\d+/); | ||
| // Default keepAliveTimeout is 5000ms (5 seconds) | ||
| expect(keepAlive).toMatch(/timeout=5/); | ||
| } finally { | ||
| server.close(); | ||
| } | ||
| }); | ||
|
|
||
| test("should respect user-set Connection: close header", async () => { | ||
| const server = createServer((req, res) => { | ||
| res.setHeader("Connection", "close"); | ||
| res.writeHead(200); | ||
| res.end("test"); | ||
| }); | ||
|
|
||
| await new Promise<void>(resolve => server.listen(0, () => resolve())); | ||
| const port = (server.address() as any).port; | ||
|
|
||
| try { | ||
| const response = await fetch(`http://localhost:${port}/`); | ||
| const text = await response.text(); | ||
|
|
||
| expect(text).toBe("test"); | ||
| expect(response.headers.get("connection")).toBe("close"); | ||
| expect(response.headers.get("keep-alive")).toBeNull(); | ||
| } finally { | ||
| server.close(); | ||
| } | ||
| }); | ||
|
|
||
| test("should respect user-set Connection: keep-alive header", async () => { | ||
| const server = createServer((req, res) => { | ||
| res.setHeader("Connection", "keep-alive"); | ||
| res.setHeader("Keep-Alive", "timeout=30"); | ||
| res.writeHead(200); | ||
| res.end("test"); | ||
| }); | ||
|
|
||
| await new Promise<void>(resolve => server.listen(0, () => resolve())); | ||
| const port = (server.address() as any).port; | ||
|
|
||
| try { | ||
| const response = await fetch(`http://localhost:${port}/`); | ||
| const text = await response.text(); | ||
|
|
||
| expect(text).toBe("test"); | ||
| expect(response.headers.get("connection")).toBe("keep-alive"); | ||
| expect(response.headers.get("keep-alive")).toBe("timeout=30"); | ||
| } finally { | ||
| server.close(); | ||
| } | ||
| }); | ||
|
|
||
| test("should use default keepAliveTimeout (5 seconds)", async () => { | ||
| const server = createServer((req, res) => { | ||
| res.writeHead(200); | ||
| res.end("test"); | ||
| }); | ||
|
|
||
| await new Promise<void>(resolve => server.listen(0, () => resolve())); | ||
| const port = (server.address() as any).port; | ||
|
|
||
| try { | ||
| const response = await fetch(`http://localhost:${port}/`); | ||
| const text = await response.text(); | ||
|
|
||
| expect(text).toBe("test"); | ||
| expect(response.headers.get("connection")).toBe("keep-alive"); | ||
| // Default timeout is 5 seconds (hardcoded in C++ layer) | ||
| expect(response.headers.get("keep-alive")).toBe("timeout=5"); | ||
| } finally { | ||
| server.close(); | ||
| } | ||
| }); | ||
|
|
||
| test("should include Connection headers with POST requests", async () => { | ||
| const server = createServer((req, res) => { | ||
| let body = ""; | ||
| req.on("data", chunk => { | ||
| body += chunk; | ||
| }); | ||
| req.on("end", () => { | ||
| res.writeHead(200, { "Content-Type": "application/json" }); | ||
| res.end( | ||
| JSON.stringify({ | ||
| success: true, | ||
| receivedData: JSON.parse(body), | ||
| }), | ||
| ); | ||
| }); | ||
| }); | ||
|
|
||
| await new Promise<void>(resolve => server.listen(0, () => resolve())); | ||
| const port = (server.address() as any).port; | ||
|
|
||
| try { | ||
| const response = await fetch(`http://localhost:${port}/`, { | ||
| method: "POST", | ||
| headers: { "Content-Type": "application/json" }, | ||
| body: JSON.stringify({ test: "data" }), | ||
| }); | ||
|
|
||
| const data = await response.json(); | ||
|
|
||
| expect(data.success).toBe(true); | ||
| expect(data.receivedData.test).toBe("data"); | ||
| expect(response.headers.get("connection")).toBe("keep-alive"); | ||
| expect(response.headers.get("keep-alive")).toMatch(/timeout=\d+/); | ||
| } finally { | ||
| server.close(); | ||
| } | ||
| }); | ||
|
|
||
| test("should include Connection headers when using setHeader before writeHead", async () => { | ||
| const server = createServer((req, res) => { | ||
| res.setHeader("Content-Type", "text/plain"); | ||
| res.setHeader("X-Custom-Header", "value"); | ||
| res.writeHead(200); | ||
| res.end("test"); | ||
| }); | ||
|
|
||
| await new Promise<void>(resolve => server.listen(0, () => resolve())); | ||
| const port = (server.address() as any).port; | ||
|
|
||
| try { | ||
| const response = await fetch(`http://localhost:${port}/`); | ||
| const text = await response.text(); | ||
|
|
||
| expect(text).toBe("test"); | ||
| expect(response.headers.get("connection")).toBe("keep-alive"); | ||
| expect(response.headers.get("keep-alive")).toMatch(/timeout=\d+/); | ||
| expect(response.headers.get("x-custom-header")).toBe("value"); | ||
| } finally { | ||
| server.close(); | ||
| } | ||
| }); | ||
|
|
||
| test("should include Connection headers when not calling writeHead explicitly", async () => { | ||
| const server = createServer((req, res) => { | ||
| // Not calling writeHead - it will be called implicitly | ||
| res.end("test"); | ||
| }); | ||
|
|
||
| await new Promise<void>(resolve => server.listen(0, () => resolve())); | ||
| const port = (server.address() as any).port; | ||
|
|
||
| try { | ||
| const response = await fetch(`http://localhost:${port}/`); | ||
| const text = await response.text(); | ||
|
|
||
| expect(text).toBe("test"); | ||
| expect(response.headers.get("connection")).toBe("keep-alive"); | ||
| expect(response.headers.get("keep-alive")).toMatch(/timeout=\d+/); | ||
| } finally { | ||
| server.close(); | ||
| } | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Test coverage is missing scenarios mentioned in PR objectives.
The PR objectives state that the implementation should:
- Set
Connection: closewhenshouldKeepAliveis false - Set
Keep-Alive: timeout=Xbased on the serverkeepAliveTimeout
However, the current test suite doesn't cover:
- Scenarios where the server has
shouldKeepAliveset to false (should result inConnection: close) - Custom
keepAliveTimeoutvalues (all tests only verify the hardcoded 5-second timeout)
Add tests for these scenarios:
test("should set Connection: close when shouldKeepAlive is false", async () => {
const server = createServer((req, res) => {
res.shouldKeepAlive = false;
res.writeHead(200);
res.end("test");
});
await new Promise<void>(resolve => server.listen(0, () => resolve()));
const port = (server.address() as any).port;
try {
const response = await fetch(`http://localhost:${port}/`);
expect(response.headers.get("connection")).toBe("close");
expect(response.headers.get("keep-alive")).toBeNull();
} finally {
server.close();
}
});
test("should use custom keepAliveTimeout", async () => {
const server = createServer((req, res) => {
res.writeHead(200);
res.end("test");
});
server.keepAliveTimeout = 30000; // 30 seconds
await new Promise<void>(resolve => server.listen(0, () => resolve()));
const port = (server.address() as any).port;
try {
const response = await fetch(`http://localhost:${port}/`);
expect(response.headers.get("connection")).toBe("keep-alive");
expect(response.headers.get("keep-alive")).toBe("timeout=30");
} finally {
server.close();
}
});🤖 Prompt for AI Agents
In test/js/node/http/node-http-connection-headers.test.ts lines 1-176: tests are
missing coverage for two PR objectives — when res.shouldKeepAlive is false the
response must send Connection: close and omit Keep-Alive, and when
server.keepAliveTimeout is changed the Keep-Alive timeout value must reflect
that; add two tests mirroring existing test structure: (1) createServer handler
sets res.shouldKeepAlive = false, writeHead/end, fetch and assert connection ===
"close" and keep-alive is null; (2) createServer normal handler, set
server.keepAliveTimeout = 30000 (30s) before listen, fetch and assert connection
=== "keep-alive" and keep-alive === "timeout=30"; ensure servers are properly
listened/backed off and always closed in finally blocks.
|
|
||
| expect(text).toBe("test"); | ||
| expect(response.headers.get("connection")).toBe("keep-alive"); | ||
| // Default timeout is 5 seconds (hardcoded in C++ layer) |
There was a problem hiding this comment.
Comment acknowledges incomplete implementation.
The comment "// Default timeout is 5 seconds (hardcoded in C++ layer)" confirms that the current implementation doesn't use the server's keepAliveTimeout setting as stated in the PR objectives. This hardcoded value should be replaced with dynamic retrieval from the server configuration.
Based on PR objectives, the timeout should be based on server.keepAliveTimeout, not hardcoded.
🤖 Prompt for AI Agents
In test/js/node/http/node-http-connection-headers.test.ts around line 88, the
comment and test assume a hardcoded default timeout of 5 seconds from the C++
layer; change the test and implementation to read the server.keepAliveTimeout
value instead of relying on a hardcoded constant: retrieve
server.keepAliveTimeout (falling back to a sensible default if undefined), pass
that value into the code that initializes connection timeouts (or expose it to
the C++ binding), and update the comment and assertions to reflect the dynamic
timeout so the test verifies the server-configured keepAliveTimeout rather than
a fixed 5s.
Fixes #19540 by adding automatic Connection and Keep-Alive headers to HTTP responses, matching Node.js behavior. Changes: - Modified JS layer (_http_server.ts) to pass shouldKeepAlive and keepAliveTimeout parameters to native writeHead function - Updated Zig layer (NodeHTTPResponse.zig) to extract these parameters from JSValue and pass them to C++ - Modified C++ layer (NodeHTTP.cpp) to accept parameters and use them to set appropriate Connection headers: - Connection: keep-alive + Keep-Alive: timeout=X when shouldKeepAlive is true - Connection: close when shouldKeepAlive is false - Headers only added if user hasn't explicitly set Connection header - Added comprehensive tests including custom timeout support 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
robobun
force-pushed
the
claude/fix-missing-connection-keepalive-headers
branch
from
bec0881 to
9d644e9
Compare
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/bun.js/bindings/NodeHTTP.cpp (1)
621-699: Critical: Missing Content-Length/Date flags in object header path leads to uWS auto-injected headersWhen headers are passed as a plain object (not FetchHeaders), you don’t set uWS state bits for Content-Length and Date. This causes uWS to auto-insert Transfer-Encoding: chunked and/or Date, leading to duplicated or conflicting headers. The FetchHeaders path handles this correctly.
Set HTTP_WROTE_CONTENT_LENGTH_HEADER with writeMark() and HTTP_WROTE_DATE_HEADER in both object-enumeration loops.
Apply this diff:
@@ - bool hasConnectionHeader = false; + bool hasConnectionHeader = false; + auto* data = response->getHttpResponseData(); @@ - structure->forEachProperty(vm, [&](const auto& entry) { + structure->forEachProperty(vm, [&](const auto& entry) { JSValue headerValue = headersObject->getDirect(entry.offset()); if (!headerValue.isString()) { return true; } String key = entry.key(); // Check for Connection header (case-insensitive) if (key.length() == 10 && WTF::equalIgnoringASCIICase(key, "connection")) { hasConnectionHeader = true; } + + // Prevent uWS auto-insertion for Content-Length and Date + if (key.length() == 14 && WTF::equalIgnoringASCIICase(key, "content-length")) { + if (!(data->state & uWS::HttpResponseData<isSSL>::HTTP_WROTE_CONTENT_LENGTH_HEADER)) { + data->state |= uWS::HttpResponseData<isSSL>::HTTP_WROTE_CONTENT_LENGTH_HEADER; + response->writeMark(); + } + } else if (key.length() == 4 && WTF::equalIgnoringASCIICase(key, "date")) { + data->state |= uWS::HttpResponseData<isSSL>::HTTP_WROTE_DATE_HEADER; + } String value = headerValue.toWTFString(globalObject); RETURN_IF_EXCEPTION(scope, false); writeResponseHeader<isSSL>(response, key, value); return true; }); @@ for (unsigned i = 0; i < propertyNames.size(); ++i) { JSValue headerValue = headersObject->getIfPropertyExists(globalObject, propertyNames[i]); RETURN_IF_EXCEPTION(scope, ); if (!headerValue.isString()) { continue; } String key = propertyNames[i].string(); // Check for Connection header (case-insensitive) if (key.length() == 10 && WTF::equalIgnoringASCIICase(key, "connection")) { hasConnectionHeader = true; } + + // Prevent uWS auto-insertion for Content-Length and Date + if (key.length() == 14 && WTF::equalIgnoringASCIICase(key, "content-length")) { + if (!(data->state & uWS::HttpResponseData<isSSL>::HTTP_WROTE_CONTENT_LENGTH_HEADER)) { + data->state |= uWS::HttpResponseData<isSSL>::HTTP_WROTE_CONTENT_LENGTH_HEADER; + response->writeMark(); + } + } else if (key.length() == 4 && WTF::equalIgnoringASCIICase(key, "date")) { + data->state |= uWS::HttpResponseData<isSSL>::HTTP_WROTE_DATE_HEADER; + } String value = headerValue.toWTFString(globalObject); RETURN_IF_EXCEPTION(scope, void()); writeResponseHeader<isSSL>(response, key, value); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (4)
src/bun.js/api/server/NodeHTTPResponse.zig(4 hunks)src/bun.js/bindings/NodeHTTP.cpp(8 hunks)src/js/node/_http_server.ts(4 hunks)test/js/node/http/node-http-connection-headers.test.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (19)
**/*.{cpp,h}
📄 CodeRabbit inference engine (.cursor/rules/javascriptcore-class.mdc)
**/*.{cpp,h}: When exposing a JS class with public Constructor and Prototype, define three C++ types: class Foo : public JSC::DestructibleObject (if it has C++ fields), class FooPrototype : public JSC::JSNonFinalObject, and class FooConstructor : public JSC::InternalFunction
If the class has C++ data members, inherit from JSC::DestructibleObject and provide proper destruction; if it has no C++ fields (only JS properties), avoid a class and use JSC::constructEmptyObject(vm, structure) with putDirectOffset
Prefer placing the subspaceFor implementation in the .cpp file rather than the header when possible
Files:
src/bun.js/bindings/NodeHTTP.cpp
**/*.cpp
📄 CodeRabbit inference engine (.cursor/rules/javascriptcore-class.mdc)
**/*.cpp: Include "root.h" at the top of C++ binding files to satisfy lints
Define prototype properties using a const HashTableValue array and declare accessors/functions with JSC_DECLARE_* macros
Prototype classes should subclass JSC::JSNonFinalObject, provide create/createStructure, DECLARE_INFO, finishCreation that reifies static properties, and set mayBePrototype on the Structure
Custom getters should use JSC_DEFINE_CUSTOM_GETTER, jsDynamicCast to validate this, and throwThisTypeError on mismatch
Custom setters should use JSC_DEFINE_CUSTOM_SETTER, validate this via jsDynamicCast, and store via WriteBarrier/set semantics
Prototype functions should use JSC_DEFINE_HOST_FUNCTION, validate this with jsDynamicCast, and return encoded JSValue
Constructors should subclass JSC::InternalFunction, return internalFunctionSpace in subspaceFor, set the prototype property as non-configurable/non-writable, and provide create/createStructure
Provide a setup function that builds the Prototype, Constructor, and Structure, and assigns them to the LazyClassStructure initializer
Use the cached Structure via globalObject->m_.get(globalObject) when constructing instances
Expose constructors to Zig via an extern "C" function that returns the constructor from the LazyClassStructure
Provide an extern "C" Bun____toJS function that creates an instance using the cached Structure and returns an EncodedJSValue
Files:
src/bun.js/bindings/NodeHTTP.cpp
src/bun.js/bindings/**/*.cpp
📄 CodeRabbit inference engine (CLAUDE.md)
src/bun.js/bindings/**/*.cpp: When a JS class has a public constructor, implement Foo, FooPrototype, and FooConstructor (JSDestructibleObject, JSNonFinalObject, InternalFunction)
Define class properties using HashTableValue arrays in C++ bindings
Add iso subspaces for classes with C++ fields
Cache structures in ZigGlobalObject for C++-backed classes
Files:
src/bun.js/bindings/NodeHTTP.cpp
test/**
📄 CodeRabbit inference engine (.cursor/rules/writing-tests.mdc)
Place all tests under the test/ directory
Files:
test/js/node/http/node-http-connection-headers.test.ts
test/js/**/*.{js,ts}
📄 CodeRabbit inference engine (.cursor/rules/writing-tests.mdc)
Place JavaScript and TypeScript tests under test/js/
Files:
test/js/node/http/node-http-connection-headers.test.ts
test/js/node/**/*.{js,ts}
📄 CodeRabbit inference engine (.cursor/rules/writing-tests.mdc)
Place Node.js module compatibility tests under test/js/node/, separated by module (e.g., test/js/node/assert/)
Files:
test/js/node/http/node-http-connection-headers.test.ts
test/**/*.{js,ts}
📄 CodeRabbit inference engine (.cursor/rules/writing-tests.mdc)
test/**/*.{js,ts}: Write tests in JavaScript or TypeScript using Bun’s Jest-style APIs (test, describe, expect) and run with bun test
Prefer data-driven tests (e.g., test.each) to reduce boilerplate
Use shared utilities from test/harness.ts where applicable
Files:
test/js/node/http/node-http-connection-headers.test.ts
test/**/*.test.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
test/**/*.test.{ts,tsx}: Test files must live under test/ and end with .test.ts or .test.tsx
In tests, always use port: 0; do not hardcode ports or roll your own random port
Prefer normalizeBunSnapshot for snapshotting test output instead of asserting raw strings
Do not write tests that assert absence of crashes (e.g., 'no panic' or 'no uncaught exception')
Use Bun’s Jest-compatible runner (import { test, expect } from "bun:test") for tests
Avoid shell commands like find or grep in tests; use Bun’s Glob and built-in tools instead
Prefer running tests via bun bd test and use provided harness utilities (bunEnv, bunExe, tempDir)
Use Bun.spawn with proper stdio handling and await proc.exited in process-spawning tests
Files:
test/js/node/http/node-http-connection-headers.test.ts
test/js/node/**
📄 CodeRabbit inference engine (CLAUDE.md)
Place Node.js compatibility tests under test/js/node/
Files:
test/js/node/http/node-http-connection-headers.test.ts
test/**/*.test.{ts,js,jsx,tsx,mjs,cjs}
📄 CodeRabbit inference engine (test/CLAUDE.md)
test/**/*.test.{ts,js,jsx,tsx,mjs,cjs}: Usebun:testfor files ending with*.test.{ts,js,jsx,tsx,mjs,cjs}
Prefer concurrent tests (test.concurrent/describe.concurrent) over sequential when feasible
Organize tests withdescribeblocks to group related tests
Use utilities likedescribe.each,toMatchSnapshot, and lifecycle hooks (beforeAll,beforeEach,afterEach) and track resources for cleanup
Files:
test/js/node/http/node-http-connection-headers.test.ts
test/**/*.{ts,tsx,js,jsx,mjs,cjs}
📄 CodeRabbit inference engine (test/CLAUDE.md)
For large/repetitive strings, use
Buffer.alloc(count, fill).toString()instead of"A".repeat(count)
Files:
test/js/node/http/node-http-connection-headers.test.ts
test/js/{bun,node}/**
📄 CodeRabbit inference engine (test/CLAUDE.md)
Organize unit tests by module under
/test/js/bun/and/test/js/node/
Files:
test/js/node/http/node-http-connection-headers.test.ts
**/*.zig
📄 CodeRabbit inference engine (.cursor/rules/javascriptcore-class.mdc)
**/*.zig: Declare the extern C symbol in Zig and export a Zig-friendly alias for use
Wrap the Bun____toJS extern in a Zig method that takes a JSGlobalObject and returns JSC.JSValue
Files:
src/bun.js/api/server/NodeHTTPResponse.zig
src/{**/js_*.zig,bun.js/api/**/*.zig}
📄 CodeRabbit inference engine (.cursor/rules/registering-bun-modules.mdc)
Use bun.JSError!JSValue for proper error propagation in JS-exposed Zig functions
Files:
src/bun.js/api/server/NodeHTTPResponse.zig
src/bun.js/**/*.zig
📄 CodeRabbit inference engine (.cursor/rules/zig-javascriptcore-classes.mdc)
src/bun.js/**/*.zig: In Zig binding structs, expose generated bindings via pub const js = JSC.Codegen.JS and re-export toJS/fromJS/fromJSDirect
Constructors and prototype methods should return bun.JSError!JSC.JSValue to integrate Zig error handling with JS exceptions
Use parameter name globalObject (not ctx) and accept (*JSC.JSGlobalObject, *JSC.CallFrame) in binding methods/constructors
Implement getters as get(this, globalObject) returning JSC.JSValue and matching the .classes.ts interface
Provide deinit() for resource cleanup and finalize() that calls deinit(); use bun.destroy(this) or appropriate destroy pattern
Access JS call data via CallFrame (argument(i), argumentCount(), thisValue()) and throw errors with globalObject.throw(...)
For properties marked cache: true, use the generated Zig accessors (NameSetCached/GetCached) to work with GC-owned values
In finalize() for objects holding JS references, release them using .deref() before destroy
Files:
src/bun.js/api/server/NodeHTTPResponse.zig
src/**/*.zig
📄 CodeRabbit inference engine (CLAUDE.md)
In Zig code, manage memory carefully and use defer for cleanup of allocations/resources
src/**/*.zig: Use the # prefix to declare private fields in Zig structs (e.g., struct { #foo: u32 })
Prefer decl literals when initializing values in Zig (e.g., const decl: Decl = .{ .binding = 0, .value = 0 })
Place @import directives at the bottom of Zig files
Use @import("bun") instead of @import("root").bunWhen adding debug logs in Zig, create a scoped logger and log via Bun APIs:
const log = bun.Output.scoped(.${SCOPE}, .hidden);thenlog("...", .{})
Files:
src/bun.js/api/server/NodeHTTPResponse.zig
src/js/node/**
📄 CodeRabbit inference engine (CLAUDE.md)
Organize built-in Node.js compatibility modules under src/js/node/ (e.g., node:fs, node:path)
Files:
src/js/node/_http_server.ts
src/js/node/**/*.{ts,js}
📄 CodeRabbit inference engine (src/js/CLAUDE.md)
Place Node.js compatibility modules (e.g., node:fs, node:path) under node/
Files:
src/js/node/_http_server.ts
src/js/{builtins,node,bun,thirdparty,internal}/**/*.{ts,js}
📄 CodeRabbit inference engine (src/js/CLAUDE.md)
src/js/{builtins,node,bun,thirdparty,internal}/**/*.{ts,js}: Use require() with string literals only (no dynamic requires)
Do not use ESM import syntax; write modules as CommonJS with export default { ... }
Export via export default {} for modules
Use .$call and .$apply; never use .call or .apply
Prefer JSC intrinsics/private $ APIs for performance (e.g., $Array.from, map.$set, $newArrayWithSize, $debug, $assert)
Validate callbacks with $isCallable and throw $ERR_INVALID_ARG_TYPE with the correct parameter name and expected type
Use process.platform and process.arch for platform detection (rely on inlining/dead-code elimination)
Files:
src/js/node/_http_server.ts
🧬 Code graph analysis (2)
src/bun.js/bindings/NodeHTTP.cpp (1)
src/bun.js/bindings/webcore/HTTPHeaderNames.cpp (2)
httpHeaderNameString(689-696)httpHeaderNameString(689-689)
src/js/node/_http_server.ts (1)
src/js/internal/http.ts (1)
headersSymbol(389-389)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Format
🔇 Additional comments (4)
src/bun.js/bindings/NodeHTTP.cpp (2)
585-599: Connection/Keep-Alive logic mirrors Node behaviorThe conditional write of Connection: keep-alive/close and Keep-Alive: timeout= (using shouldKeepAlive and keepAliveTimeout) looks correct and aligned with Node’s _storeHeader behavior.
1071-1078: No action needed WebCore__FetchHeaders__toUWSResponse is only invoked by the Zig binding in the Node HTTP server path, so its default-injecting Connection/Keep-Alive behavior cannot affect non-Node contexts.test/js/node/http/node-http-connection-headers.test.ts (1)
4-26: Tests cover defaults, user overrides, custom timeout, POST, and implicit headersThe cases validate Node-compat header semantics well; default timeout assertion matches server default (5s). Good use of port: 0 and proper cleanup.
Also applies to: 50-71, 94-116, 118-154, 156-178, 180-199
src/bun.js/api/server/NodeHTTPResponse.zig (1)
410-423: Wiring shouldKeepAlive/keepAliveTimeout through writeHead is correct
- Externs extended for http/https are consistent.
- JS arguments are parsed with sane defaults (true, 5000).
- Values are propagated to the C++ writeHead handlers.
Also applies to: 426-445, 470-479, 502-508
| const shouldKeepAlive = this.shouldKeepAlive; | ||
| const keepAliveTimeout = this.socket?.server?.keepAliveTimeout ?? 5000; | ||
| handle.writeHead(this.statusCode, this.statusMessage, this[headersSymbol], shouldKeepAlive, keepAliveTimeout); | ||
|
|
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Deduplicate keep-alive param computation
The same shouldKeepAlive/keepAliveTimeout computation repeats in 4 paths. Extract a helper to reduce drift.
Apply this refactor:
@@
- const shouldKeepAlive = this.shouldKeepAlive;
- const keepAliveTimeout = this.socket?.server?.keepAliveTimeout ?? 5000;
- handle.writeHead(this.statusCode, this.statusMessage, this[headersSymbol], shouldKeepAlive, keepAliveTimeout);
+ const [ska, kat] = getConnectionParams(this);
+ handle.writeHead(this.statusCode, this.statusMessage, this[headersSymbol], ska, kat);
@@
- const shouldKeepAlive = this.shouldKeepAlive;
- const keepAliveTimeout = this.socket?.server?.keepAliveTimeout ?? 5000;
- handle.writeHead(this.statusCode, this.statusMessage, this[headersSymbol], shouldKeepAlive, keepAliveTimeout);
+ const [ska, kat] = getConnectionParams(this);
+ handle.writeHead(this.statusCode, this.statusMessage, this[headersSymbol], ska, kat);
@@
- const shouldKeepAlive = this.shouldKeepAlive;
- const keepAliveTimeout = this.socket?.server?.keepAliveTimeout ?? 5000;
- handle.writeHead(this.statusCode, this.statusMessage, this[headersSymbol], shouldKeepAlive, keepAliveTimeout);
+ const [ska, kat] = getConnectionParams(this);
+ handle.writeHead(this.statusCode, this.statusMessage, this[headersSymbol], ska, kat);
@@
- const shouldKeepAlive = this.shouldKeepAlive;
- const keepAliveTimeout = this.socket?.server?.keepAliveTimeout ?? 5000;
- handle.writeHead(this.statusCode, this.statusMessage, this[headersSymbol], shouldKeepAlive, keepAliveTimeout);
+ const [ska, kat] = getConnectionParams(this);
+ handle.writeHead(this.statusCode, this.statusMessage, this[headersSymbol], ska, kat);Add helper (outside the selected ranges):
function getConnectionParams(res: any): [boolean, number] {
const shouldKeepAlive = res.shouldKeepAlive;
const keepAliveTimeout = res.socket?.server?.keepAliveTimeout ?? 5000;
return [shouldKeepAlive, keepAliveTimeout];
}Also applies to: 1464-1467, 1569-1572, 1638-1641
🤖 Prompt for AI Agents
In src/js/node/_http_server.ts around lines 1352-1355, the keep-alive
computation (shouldKeepAlive and keepAliveTimeout) is duplicated across four
code paths; add a new helper function getConnectionParams(res: any): [boolean,
number] outside the selected ranges that returns [res.shouldKeepAlive,
res.socket?.server?.keepAliveTimeout ?? 5000], then replace the local
computations at 1352-1355 and the other locations (1464-1467, 1569-1572,
1638-1641) to call the helper and destructure its result before calling
handle.writeHead (or equivalent) so all paths reuse the single helper.
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/bun.js/api/server/NodeHTTPResponse.zig (1)
405-413: Keep‑alive params threaded correctly; consider minimal validation on timeoutEnd‑to‑end plumbing looks solid: args parsed with sane defaults, forwarded to both HTTP/HTTPS paths, and applied in writeHeadInternal. Minor safety nit: clamp keepAliveTimeout to a reasonable upper bound (and document units) to avoid accidental huge values.
- const keep_alive_timeout: u32 = if (!keep_alive_timeout_value.isUndefined()) - @intCast(keep_alive_timeout_value.toU32()) - else - 5000; + const keep_alive_timeout_raw: u32 = if (!keep_alive_timeout_value.isUndefined()) + keep_alive_timeout_value.toU32() + else + 5000; + // Clamp to e.g. 24h to avoid extreme values + const keep_alive_timeout: u32 = @min(keep_alive_timeout_raw, 24 * 60 * 60 * 1000);As per coding guidelines
Also applies to: 415-423, 426-445, 470-479, 487-496, 502-508
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (4)
src/bun.js/api/server/NodeHTTPResponse.zig(4 hunks)src/bun.js/bindings/NodeHTTP.cpp(8 hunks)src/js/node/_http_server.ts(4 hunks)test/js/node/http/node-http-connection-headers.test.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (19)
test/**
📄 CodeRabbit inference engine (.cursor/rules/writing-tests.mdc)
Place all tests under the test/ directory
Files:
test/js/node/http/node-http-connection-headers.test.ts
test/js/**/*.{js,ts}
📄 CodeRabbit inference engine (.cursor/rules/writing-tests.mdc)
Place JavaScript and TypeScript tests under test/js/
Files:
test/js/node/http/node-http-connection-headers.test.ts
test/js/node/**/*.{js,ts}
📄 CodeRabbit inference engine (.cursor/rules/writing-tests.mdc)
Place Node.js module compatibility tests under test/js/node/, separated by module (e.g., test/js/node/assert/)
Files:
test/js/node/http/node-http-connection-headers.test.ts
test/**/*.{js,ts}
📄 CodeRabbit inference engine (.cursor/rules/writing-tests.mdc)
test/**/*.{js,ts}: Write tests in JavaScript or TypeScript using Bun’s Jest-style APIs (test, describe, expect) and run with bun test
Prefer data-driven tests (e.g., test.each) to reduce boilerplate
Use shared utilities from test/harness.ts where applicable
Files:
test/js/node/http/node-http-connection-headers.test.ts
test/**/*.test.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
test/**/*.test.{ts,tsx}: Test files must live under test/ and end with .test.ts or .test.tsx
In tests, always use port: 0; do not hardcode ports or roll your own random port
Prefer normalizeBunSnapshot for snapshotting test output instead of asserting raw strings
Do not write tests that assert absence of crashes (e.g., 'no panic' or 'no uncaught exception')
Use Bun’s Jest-compatible runner (import { test, expect } from "bun:test") for tests
Avoid shell commands like find or grep in tests; use Bun’s Glob and built-in tools instead
Prefer running tests via bun bd test and use provided harness utilities (bunEnv, bunExe, tempDir)
Use Bun.spawn with proper stdio handling and await proc.exited in process-spawning tests
Files:
test/js/node/http/node-http-connection-headers.test.ts
test/js/node/**
📄 CodeRabbit inference engine (CLAUDE.md)
Place Node.js compatibility tests under test/js/node/
Files:
test/js/node/http/node-http-connection-headers.test.ts
test/**/*.test.{ts,js,jsx,tsx,mjs,cjs}
📄 CodeRabbit inference engine (test/CLAUDE.md)
test/**/*.test.{ts,js,jsx,tsx,mjs,cjs}: Usebun:testfor files ending with*.test.{ts,js,jsx,tsx,mjs,cjs}
Prefer concurrent tests (test.concurrent/describe.concurrent) over sequential when feasible
Organize tests withdescribeblocks to group related tests
Use utilities likedescribe.each,toMatchSnapshot, and lifecycle hooks (beforeAll,beforeEach,afterEach) and track resources for cleanup
Files:
test/js/node/http/node-http-connection-headers.test.ts
test/**/*.{ts,tsx,js,jsx,mjs,cjs}
📄 CodeRabbit inference engine (test/CLAUDE.md)
For large/repetitive strings, use
Buffer.alloc(count, fill).toString()instead of"A".repeat(count)
Files:
test/js/node/http/node-http-connection-headers.test.ts
test/js/{bun,node}/**
📄 CodeRabbit inference engine (test/CLAUDE.md)
Organize unit tests by module under
/test/js/bun/and/test/js/node/
Files:
test/js/node/http/node-http-connection-headers.test.ts
**/*.zig
📄 CodeRabbit inference engine (.cursor/rules/javascriptcore-class.mdc)
**/*.zig: Declare the extern C symbol in Zig and export a Zig-friendly alias for use
Wrap the Bun____toJS extern in a Zig method that takes a JSGlobalObject and returns JSC.JSValue
Files:
src/bun.js/api/server/NodeHTTPResponse.zig
src/{**/js_*.zig,bun.js/api/**/*.zig}
📄 CodeRabbit inference engine (.cursor/rules/registering-bun-modules.mdc)
Use bun.JSError!JSValue for proper error propagation in JS-exposed Zig functions
Files:
src/bun.js/api/server/NodeHTTPResponse.zig
src/bun.js/**/*.zig
📄 CodeRabbit inference engine (.cursor/rules/zig-javascriptcore-classes.mdc)
src/bun.js/**/*.zig: In Zig binding structs, expose generated bindings via pub const js = JSC.Codegen.JS and re-export toJS/fromJS/fromJSDirect
Constructors and prototype methods should return bun.JSError!JSC.JSValue to integrate Zig error handling with JS exceptions
Use parameter name globalObject (not ctx) and accept (*JSC.JSGlobalObject, *JSC.CallFrame) in binding methods/constructors
Implement getters as get(this, globalObject) returning JSC.JSValue and matching the .classes.ts interface
Provide deinit() for resource cleanup and finalize() that calls deinit(); use bun.destroy(this) or appropriate destroy pattern
Access JS call data via CallFrame (argument(i), argumentCount(), thisValue()) and throw errors with globalObject.throw(...)
For properties marked cache: true, use the generated Zig accessors (NameSetCached/GetCached) to work with GC-owned values
In finalize() for objects holding JS references, release them using .deref() before destroy
Files:
src/bun.js/api/server/NodeHTTPResponse.zig
src/**/*.zig
📄 CodeRabbit inference engine (CLAUDE.md)
In Zig code, manage memory carefully and use defer for cleanup of allocations/resources
src/**/*.zig: Use the # prefix to declare private fields in Zig structs (e.g., struct { #foo: u32 })
Prefer decl literals when initializing values in Zig (e.g., const decl: Decl = .{ .binding = 0, .value = 0 })
Place @import directives at the bottom of Zig files
Use @import("bun") instead of @import("root").bunWhen adding debug logs in Zig, create a scoped logger and log via Bun APIs:
const log = bun.Output.scoped(.${SCOPE}, .hidden);thenlog("...", .{})
Files:
src/bun.js/api/server/NodeHTTPResponse.zig
**/*.{cpp,h}
📄 CodeRabbit inference engine (.cursor/rules/javascriptcore-class.mdc)
**/*.{cpp,h}: When exposing a JS class with public Constructor and Prototype, define three C++ types: class Foo : public JSC::DestructibleObject (if it has C++ fields), class FooPrototype : public JSC::JSNonFinalObject, and class FooConstructor : public JSC::InternalFunction
If the class has C++ data members, inherit from JSC::DestructibleObject and provide proper destruction; if it has no C++ fields (only JS properties), avoid a class and use JSC::constructEmptyObject(vm, structure) with putDirectOffset
Prefer placing the subspaceFor implementation in the .cpp file rather than the header when possible
Files:
src/bun.js/bindings/NodeHTTP.cpp
**/*.cpp
📄 CodeRabbit inference engine (.cursor/rules/javascriptcore-class.mdc)
**/*.cpp: Include "root.h" at the top of C++ binding files to satisfy lints
Define prototype properties using a const HashTableValue array and declare accessors/functions with JSC_DECLARE_* macros
Prototype classes should subclass JSC::JSNonFinalObject, provide create/createStructure, DECLARE_INFO, finishCreation that reifies static properties, and set mayBePrototype on the Structure
Custom getters should use JSC_DEFINE_CUSTOM_GETTER, jsDynamicCast to validate this, and throwThisTypeError on mismatch
Custom setters should use JSC_DEFINE_CUSTOM_SETTER, validate this via jsDynamicCast, and store via WriteBarrier/set semantics
Prototype functions should use JSC_DEFINE_HOST_FUNCTION, validate this with jsDynamicCast, and return encoded JSValue
Constructors should subclass JSC::InternalFunction, return internalFunctionSpace in subspaceFor, set the prototype property as non-configurable/non-writable, and provide create/createStructure
Provide a setup function that builds the Prototype, Constructor, and Structure, and assigns them to the LazyClassStructure initializer
Use the cached Structure via globalObject->m_.get(globalObject) when constructing instances
Expose constructors to Zig via an extern "C" function that returns the constructor from the LazyClassStructure
Provide an extern "C" Bun____toJS function that creates an instance using the cached Structure and returns an EncodedJSValue
Files:
src/bun.js/bindings/NodeHTTP.cpp
src/bun.js/bindings/**/*.cpp
📄 CodeRabbit inference engine (CLAUDE.md)
src/bun.js/bindings/**/*.cpp: When a JS class has a public constructor, implement Foo, FooPrototype, and FooConstructor (JSDestructibleObject, JSNonFinalObject, InternalFunction)
Define class properties using HashTableValue arrays in C++ bindings
Add iso subspaces for classes with C++ fields
Cache structures in ZigGlobalObject for C++-backed classes
Files:
src/bun.js/bindings/NodeHTTP.cpp
src/js/node/**
📄 CodeRabbit inference engine (CLAUDE.md)
Organize built-in Node.js compatibility modules under src/js/node/ (e.g., node:fs, node:path)
Files:
src/js/node/_http_server.ts
src/js/node/**/*.{ts,js}
📄 CodeRabbit inference engine (src/js/CLAUDE.md)
Place Node.js compatibility modules (e.g., node:fs, node:path) under node/
Files:
src/js/node/_http_server.ts
src/js/{builtins,node,bun,thirdparty,internal}/**/*.{ts,js}
📄 CodeRabbit inference engine (src/js/CLAUDE.md)
src/js/{builtins,node,bun,thirdparty,internal}/**/*.{ts,js}: Use require() with string literals only (no dynamic requires)
Do not use ESM import syntax; write modules as CommonJS with export default { ... }
Export via export default {} for modules
Use .$call and .$apply; never use .call or .apply
Prefer JSC intrinsics/private $ APIs for performance (e.g., $Array.from, map.$set, $newArrayWithSize, $debug, $assert)
Validate callbacks with $isCallable and throw $ERR_INVALID_ARG_TYPE with the correct parameter name and expected type
Use process.platform and process.arch for platform detection (rely on inlining/dead-code elimination)
Files:
src/js/node/_http_server.ts
🧬 Code graph analysis (2)
src/bun.js/bindings/NodeHTTP.cpp (1)
src/bun.js/bindings/webcore/HTTPHeaderNames.cpp (2)
httpHeaderNameString(689-696)httpHeaderNameString(689-689)
src/js/node/_http_server.ts (1)
src/js/internal/http.ts (1)
headersSymbol(389-389)
🔇 Additional comments (3)
test/js/node/http/node-http-connection-headers.test.ts (1)
1-199: Good coverage of Connection/Keep‑Alive scenariosTests exercise defaults, overrides, custom timeout, POST, and implicit headers. Looks correct and aligned with Node semantics.
src/bun.js/bindings/NodeHTTP.cpp (2)
513-599: Header injection logic matches Node behavior
- Detects Connection header across common/uncommon collections.
- Adds Connection: keep-alive + Keep-Alive: timeout or Connection: close based on shouldKeepAlive, only when absent.
- ms→s conversion mirrors Node (floor).
Looks good.
Also applies to: 621-699, 703-725
1070-1077: Limit default keep-alive injection to node contexts
The new WebCore__FetchHeaders__toUWSResponse wrapper (NodeHTTP.cpp:1070–1077) always uses default keep-alive settings, impacting all callers (including Zig-based fetch). If you intend this only for node:http, pass through shouldKeepAlive/keepAliveTimeout or guard injection for non-node contexts.
| const shouldKeepAlive = this.shouldKeepAlive; | ||
| const keepAliveTimeout = this.socket?.server?.keepAliveTimeout ?? 5000; | ||
| handle.writeHead(this.statusCode, this.statusMessage, this[headersSymbol], shouldKeepAlive, keepAliveTimeout); | ||
|
|
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Deduplicate keep‑alive param computation
The same two lines repeat across four sites. Extract a tiny helper to reduce drift.
@@
- const shouldKeepAlive = this.shouldKeepAlive;
- const keepAliveTimeout = this.socket?.server?.keepAliveTimeout ?? 5000;
- handle.writeHead(this.statusCode, this.statusMessage, this[headersSymbol], shouldKeepAlive, keepAliveTimeout);
+ const { shouldKeepAlive, keepAliveTimeout } = getKeepAliveParams(this);
+ handle.writeHead(this.statusCode, this.statusMessage, this[headersSymbol], shouldKeepAlive, keepAliveTimeout);
@@
- const shouldKeepAlive = this.shouldKeepAlive;
- const keepAliveTimeout = this.socket?.server?.keepAliveTimeout ?? 5000;
- handle.writeHead(this.statusCode, this.statusMessage, this[headersSymbol], shouldKeepAlive, keepAliveTimeout);
+ const { shouldKeepAlive, keepAliveTimeout } = getKeepAliveParams(this);
+ handle.writeHead(this.statusCode, this.statusMessage, this[headersSymbol], shouldKeepAlive, keepAliveTimeout);
@@
- const shouldKeepAlive = this.shouldKeepAlive;
- const keepAliveTimeout = this.socket?.server?.keepAliveTimeout ?? 5000;
- handle.writeHead(this.statusCode, this.statusMessage, this[headersSymbol], shouldKeepAlive, keepAliveTimeout);
+ const { shouldKeepAlive, keepAliveTimeout } = getKeepAliveParams(this);
+ handle.writeHead(this.statusCode, this.statusMessage, this[headersSymbol], shouldKeepAlive, keepAliveTimeout);
@@
- const shouldKeepAlive = this.shouldKeepAlive;
- const keepAliveTimeout = this.socket?.server?.keepAliveTimeout ?? 5000;
- handle.writeHead(this.statusCode, this.statusMessage, this[headersSymbol], shouldKeepAlive, keepAliveTimeout);
+ const { shouldKeepAlive, keepAliveTimeout } = getKeepAliveParams(this);
+ handle.writeHead(this.statusCode, this.statusMessage, this[headersSymbol], shouldKeepAlive, keepAliveTimeout);Add once near ServerResponse:
function getKeepAliveParams(self: any) {
return {
shouldKeepAlive: self.shouldKeepAlive,
keepAliveTimeout: self.socket?.server?.keepAliveTimeout ?? 5000,
};
}Also applies to: 1464-1467, 1569-1572, 1638-1641
🤖 Prompt for AI Agents
In src/js/node/_http_server.ts around lines 1352-1355 (and similarly 1464-1467,
1569-1572, 1638-1641), the computation of shouldKeepAlive and keepAliveTimeout
is duplicated; add a small helper function near ServerResponse named
getKeepAliveParams(self: any) that returns { shouldKeepAlive:
self.shouldKeepAlive, keepAliveTimeout: self.socket?.server?.keepAliveTimeout ??
5000 }, then replace the duplicated two-line blocks with a single call to that
helper and destructure the returned values before calling handle.writeHead
(i.e., const { shouldKeepAlive, keepAliveTimeout } = getKeepAliveParams(this);).
1 participant
Summary
Fixes #19540 by adding automatic
ConnectionandKeep-Aliveheaders to HTTP responses in Bun'snode:httpimplementation, matching Node.js behavior.Problem
Since Bun 1.2.6, HTTP responses from
node:httpservers were missing theConnection: keep-aliveandKeep-Alive: timeout=Xheaders that Node.js adds automatically. This caused issues with frameworks like NestJS where Chrome would truncate response payloads.Solution
Implemented a three-layer solution that passes connection state parameters from JavaScript through Zig to C++:
_http_server.ts): Modified to passshouldKeepAliveandkeepAliveTimeoutparameters to the nativewriteHeadfunctionNodeHTTPResponse.zig): Updated to extract these parameters from JSValue and forward them to C++NodeHTTP.cpp): Modified to accept parameters and automatically add:Connection: keep-alive+Keep-Alive: timeout=XwhenshouldKeepAliveis trueConnection: closewhenshouldKeepAliveis falseHeaders are only added if the user hasn't explicitly set a
Connectionheader, ensuring user preferences are respected.Changes
shouldKeepAliveandkeepAliveTimeoutfrom JS to native codeConnectionheadersTest plan
All tests passing:
Test coverage includes:
🤖 Generated with Claude Code