Minor HTTP improvements #25283
Minor HTTP improvements #25283
Conversation
WalkthroughHTTP connection handling refactored to normalize Connection and Transfer-Encoding header comparisons with case-insensitive matching. A helper function centralizes keep-alive logic, and a regression test validates HTTP connection reuse behavior with various header casing scenarios. Changes
Possibly related PRs
Suggested reviewers
Pre-merge checks❌ Failed checks (1 warning, 1 inconclusive)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/http.zig (1)
2254-2280: Fix the comptime parameter mismatch in Transfer-Encoding comparisons.The code at lines 2254-2280 calls
strings.eqlCaseInsensitiveASCII(header.value, "gzip", true), but the function signature requires the third parametercheck_lento becomptime bool, not a runtime value. This causes a build error.The correct approach is already used elsewhere in the file: lines 633-635 use
std.ascii.eqlIgnoreCasefor the Connection header. Apply the same pattern here:- if (strings.eqlCaseInsensitiveASCII(header.value, "gzip", true)) { + if (std.ascii.eqlIgnoreCase(header.value, "gzip")) { if (!this.flags.disable_decompression) { this.state.transfer_encoding = Encoding.gzip; }(Apply similar changes to all Transfer-Encoding value comparisons: "deflate", "br", "zstd", "identity", and "chunked")
📜 Review details
Configuration used: Path: .coderabbit.yaml
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/http.zig(4 hunks)test/regression/issue/12053.test.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (10)
test/**/*.{js,ts,jsx,tsx}
📄 CodeRabbit inference engine (.cursor/rules/writing-tests.mdc)
test/**/*.{js,ts,jsx,tsx}: Write tests as JavaScript and TypeScript files using Jest-style APIs (test,describe,expect) and import frombun:test
Usetest.eachand data-driven tests to reduce boilerplate when testing multiple similar cases
Files:
test/regression/issue/12053.test.ts
test/regression/issue/**/*.test.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
When a test is for a specific numbered GitHub Issue, place it in
test/regression/issue/${issueNumber}.test.tswith a REAL issue number, not a placeholder
Files:
test/regression/issue/12053.test.ts
**/*.test.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.test.{ts,tsx}: For single-file tests, prefer using-eflag overtempDir
For multi-file tests, prefer usingtempDirfromharnessandBun.spawnover other temporary directory creation methods
Always useport: 0for network tests and do not hardcode ports or use custom random port number functions
UsenormalizeBunSnapshotto normalize snapshot output of tests
Never write tests that check for no 'panic' or 'uncaught exception' or similar in the test output - that is NOT a valid test
UsetempDirfromharnessto create temporary directories, do not usetmpdirSyncorfs.mkdtempSync
When spawning processes in tests, check stdout/stderr expectations BEFORE checking exit code to get more useful error messages on test failure
Do not write flaky tests - do not usesetTimeoutin tests, insteadawaitthe condition to be met as you are testing the CONDITION not the TIME PASSING
Verify your test fails withUSE_SYSTEM_BUN=1 bun test <file>and passes withbun bd test <file>- your test is NOT VALID if it passes withUSE_SYSTEM_BUN=1
Files:
test/regression/issue/12053.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:testwith files that end in*.test.{ts,js,jsx,tsx,mjs,cjs}
Do not write flaky tests. Never wait for time to pass in tests; always wait for the condition to be met instead of using an arbitrary amount of time
Never use hardcoded port numbers in tests. Always useport: 0to get a random port
Prefer concurrent tests over sequential tests usingtest.concurrentordescribe.concurrentwhen multiple tests spawn processes or write files, unless it's very difficult to make them concurrent
When spawning Bun processes in tests, usebunExeandbunEnvfromharnessto ensure the same build of Bun is used and debug logging is silenced
Use-eflag for single-file tests when spawning Bun processes
UsetempDir()from harness to create temporary directories with files for multi-file tests instead of creating files manually
Prefer async/await over callbacks in tests
When callbacks must be used and it's just a single callback, usePromise.withResolversto create a promise that can be resolved or rejected from a callback
Do not set a timeout on tests. Bun already has timeouts
UseBuffer.alloc(count, fill).toString()instead of'A'.repeat(count)to create repetitive strings in tests, as ''.repeat is very slow in debug JavaScriptCore builds
Usedescribeblocks for grouping related tests
Always useawait usingorusingto ensure proper resource cleanup in tests for APIs like Bun.listen, Bun.connect, Bun.spawn, Bun.serve, etc
Always check exit codes and test error scenarios in error tests
Usedescribe.each()for parameterized tests
UsetoMatchSnapshot()for snapshot testing
UsebeforeAll(),afterEach(),beforeEach()for setup/teardown in tests
Track resources (servers, clients) in arrays for cleanup inafterEach()
Files:
test/regression/issue/12053.test.ts
test/regression/issue/**/*.test.ts
📄 CodeRabbit inference engine (test/CLAUDE.md)
Regression tests for specific issues go in
/test/regression/issue/${issueNumber}.test.ts. Do not put tests without issue numbers in the regression directory
Files:
test/regression/issue/12053.test.ts
src/**/*.{cpp,zig}
📄 CodeRabbit inference engine (.cursor/rules/building-bun.mdc)
src/**/*.{cpp,zig}: Usebun bdorbun run build:debugto build debug versions for C++ and Zig source files; creates debug build at./build/debug/bun-debug
Run tests usingbun bd test <test-file>with the debug build; never usebun testdirectly as it will not include your changes
Execute files usingbun bd <file> <...args>; never usebun <file>directly as it will not include your changes
Enable debug logs for specific scopes usingBUN_DEBUG_$(SCOPE)=1environment variable
Code generation happens automatically as part of the build process; no manual code generation commands are required
Files:
src/http.zig
src/**/*.zig
📄 CodeRabbit inference engine (.cursor/rules/building-bun.mdc)
Use
bun.Output.scoped(.${SCOPE}, .hidden)for creating debug logs in Zig codeImplement core functionality in Zig, typically in its own directory in
src/
src/**/*.zig: Zig source code should be organized insrc/*.zigfor core runtime, JavaScript bindings, and package manager
In Zig code, be careful with allocators and use defer for cleanup to manage memory properly
UseBUN_DEBUG_QUIET_LOGS=1to disable debug logging, orBUN_DEBUG_<scopeName>=1to enable specific Output.scoped calls in debug builds
Runbun run zig:check-allto compile the Zig code on all platforms when making platform-specific changes
src/**/*.zig: Private fields in Zig are fully supported using the#prefix:struct { #foo: u32 };
Use decl literals in Zig for declaration initialization:const decl: Decl = .{ .binding = 0, .value = 0 };
Prefer@importat the bottom of the file (auto formatter will move them automatically)
Files:
src/http.zig
**/*.zig
📄 CodeRabbit inference engine (.cursor/rules/zig-javascriptcore-classes.mdc)
**/*.zig: Expose generated bindings in Zig structs usingpub const js = JSC.Codegen.JS<ClassName>with trait conversion methods:toJS,fromJS, andfromJSDirect
Use consistent parameter nameglobalObjectinstead ofctxin Zig constructor and method implementations
Usebun.JSError!JSValuereturn type for Zig methods and constructors to enable proper error handling and exception propagation
Implement resource cleanup usingdeinit()method that releases resources, followed byfinalize()called by the GC that invokesdeinit()and frees the pointer
UseJSC.markBinding(@src())in finalize methods for debugging purposes before callingdeinit()
For methods returning cached properties in Zig, declare external C++ functions usingextern fnandcallconv(JSC.conv)calling convention
Implement getter functions with naming patternget<PropertyName>in Zig that acceptthisandglobalObjectparameters and returnJSC.JSValue
Access JavaScript CallFrame arguments usingcallFrame.argument(i), check argument count withcallFrame.argumentCount(), and getthiswithcallFrame.thisValue()
For reference-counted objects, use.deref()in finalize instead ofdestroy()to release references to other JS objects
Files:
src/http.zig
src/**/*.{ts,zig,cpp}
📄 CodeRabbit inference engine (CLAUDE.md)
Always use absolute paths in file operations
Files:
src/http.zig
src/**/*.{ts,zig}
📄 CodeRabbit inference engine (CLAUDE.md)
Avoid shell commands in code - don't use
findorgrep; use Bun's Glob and built-in tools instead
Files:
src/http.zig
🧠 Learnings (12)
📓 Common learnings
Learnt from: markovejnovic
Repo: oven-sh/bun PR: 24423
File: test/js/node/path/browserify.test.js:472-475
Timestamp: 2025-11-07T17:28:58.045Z
Learning: markovejnovic prefers functional programming patterns (e.g., using reduce) over imperative loops and does not want style-based readability suggestions unless there's a concrete technical reason beyond personal preference.
<!--
📚 Learning: 2025-11-24T18:37:30.259Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: test/CLAUDE.md:0-0
Timestamp: 2025-11-24T18:37:30.259Z
Learning: Applies to test/**/*.test.{ts,js,jsx,tsx,mjs,cjs} : Track resources (servers, clients) in arrays for cleanup in `afterEach()`
Applied to files:
test/regression/issue/12053.test.ts
📚 Learning: 2025-11-24T18:37:30.259Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: test/CLAUDE.md:0-0
Timestamp: 2025-11-24T18:37:30.259Z
Learning: Applies to test/regression/issue/**/*.test.ts : Regression tests for specific issues go in `/test/regression/issue/${issueNumber}.test.ts`. Do not put tests without issue numbers in the regression directory
Applied to files:
test/regression/issue/12053.test.ts
📚 Learning: 2025-10-19T04:55:33.099Z
Learnt from: theshadow27
Repo: oven-sh/bun PR: 23798
File: test/js/bun/http/node-telemetry.test.ts:27-203
Timestamp: 2025-10-19T04:55:33.099Z
Learning: In test/js/bun/http/node-telemetry.test.ts and the Bun.telemetry._node_binding API, after the architecture refactor, the _node_binding interface only contains two methods: handleIncomingRequest(req, res) and handleWriteHead(res, statusCode). The handleRequestFinish hook and other lifecycle hooks were removed during simplification. Both current methods are fully tested.
Applied to files:
test/regression/issue/12053.test.ts
📚 Learning: 2025-10-18T05:23:24.403Z
Learnt from: theshadow27
Repo: oven-sh/bun PR: 23798
File: test/js/bun/telemetry-server.test.ts:91-100
Timestamp: 2025-10-18T05:23:24.403Z
Learning: In the Bun codebase, telemetry tests (test/js/bun/telemetry-*.test.ts) should focus on telemetry API behavior: configure/disable/isEnabled, callback signatures and invocation, request ID correlation, and error handling. HTTP protocol behaviors like status code normalization (e.g., 200 with empty body → 204) should be tested in HTTP server tests (test/js/bun/http/), not in telemetry tests. Keep separation of concerns: telemetry tests verify the telemetry API contract; HTTP tests verify HTTP semantics.
Applied to files:
test/regression/issue/12053.test.ts
📚 Learning: 2025-11-24T18:37:30.259Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: test/CLAUDE.md:0-0
Timestamp: 2025-11-24T18:37:30.259Z
Learning: Applies to test/**/*.test.{ts,js,jsx,tsx,mjs,cjs} : Never use hardcoded port numbers in tests. Always use `port: 0` to get a random port
Applied to files:
test/regression/issue/12053.test.ts
📚 Learning: 2025-11-24T18:35:50.422Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: .cursor/rules/writing-tests.mdc:0-0
Timestamp: 2025-11-24T18:35:50.422Z
Learning: See `test/harness.ts` for common test utilities and helpers
Applied to files:
test/regression/issue/12053.test.ts
📚 Learning: 2025-11-24T18:36:33.069Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T18:36:33.069Z
Learning: Applies to **/*.test.{ts,tsx} : Always use `port: 0` for network tests and do not hardcode ports or use custom random port number functions
Applied to files:
test/regression/issue/12053.test.ts
📚 Learning: 2025-11-24T18:37:30.259Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: test/CLAUDE.md:0-0
Timestamp: 2025-11-24T18:37:30.259Z
Learning: Applies to test/**/*.test.{ts,js,jsx,tsx,mjs,cjs} : Always use `await using` or `using` to ensure proper resource cleanup in tests for APIs like Bun.listen, Bun.connect, Bun.spawn, Bun.serve, etc
Applied to files:
test/regression/issue/12053.test.ts
📚 Learning: 2025-09-20T00:58:38.042Z
Learnt from: markovejnovic
Repo: oven-sh/bun PR: 22568
File: test/js/valkey/valkey.test.ts:561-564
Timestamp: 2025-09-20T00:58:38.042Z
Learning: For test/js/valkey/valkey.test.ts, do not comment on synchronous throw assertions for async Redis methods (like ctx.redis.set(), ctx.redis.unsubscribe(), etc.) - Bun's Redis client implementation differs from Node.js and can throw synchronously even for async methods. The maintainer has explicitly requested to stop looking at this error pattern.
Applied to files:
test/regression/issue/12053.test.ts
📚 Learning: 2025-11-24T18:36:59.706Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: src/bun.js/bindings/v8/CLAUDE.md:0-0
Timestamp: 2025-11-24T18:36:59.706Z
Learning: Applies to src/bun.js/bindings/v8/test/v8/v8.test.ts : Add corresponding test cases to test/v8/v8.test.ts using checkSameOutput() function to compare Node.js and Bun output
Applied to files:
test/regression/issue/12053.test.ts
📚 Learning: 2025-11-24T18:35:50.422Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: .cursor/rules/writing-tests.mdc:0-0
Timestamp: 2025-11-24T18:35:50.422Z
Learning: When fixing a Node.js compatibility bug, first verify the expected behavior works in Node.js before testing against Bun's canary version
Applied to files:
test/regression/issue/12053.test.ts
🧬 Code graph analysis (1)
test/regression/issue/12053.test.ts (3)
test/js/node/test/parallel/test-net-server-close.js (1)
sockets(27-27)test/js/node/test/parallel/test-http-outgoing-proto.js (1)
ClientRequest(7-7)test/js/node/test/parallel/test-http-parser.js (2)
req1(511-519)req2(521-527)
🔇 Additional comments (3)
src/http.zig (2)
635-637: LGTM! Keep-alive connection header support added correctly.The addition of explicit "keep-alive" handling complements the existing "close" handling and follows the same case-insensitive pattern per RFC 7230.
2233-2233: LGTM! Case-insensitive Content-Type check is correct.Using
containsCaseInsensitiveASCIIis appropriate here since Content-Type headers may include additional parameters (e.g.,text/event-stream; charset=utf-8).test/regression/issue/12053.test.ts (1)
1-291: Well-designed regression test suite.This test file comprehensively covers the HTTP Agent connection reuse bug (issue #12053) with multiple scenarios:
- Socket reuse verification
- Case-insensitive header handling
- Connection: close behavior
- Agent-level keepAlive configuration
The tests follow best practices:
- Uses
port: 0for dynamic port allocation- Proper resource cleanup with
agent.destroy()andserver.close()- Clear test names describing what's being verified
setImmediateusage is appropriate for event loop synchronization (not a flaky timing pattern)
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Path: .coderabbit.yaml
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 (1)
test/regression/issue/12053.test.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (5)
test/**/*.{js,ts,jsx,tsx}
📄 CodeRabbit inference engine (.cursor/rules/writing-tests.mdc)
test/**/*.{js,ts,jsx,tsx}: Write tests as JavaScript and TypeScript files using Jest-style APIs (test,describe,expect) and import frombun:test
Usetest.eachand data-driven tests to reduce boilerplate when testing multiple similar cases
Files:
test/regression/issue/12053.test.ts
test/regression/issue/**/*.test.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
When a test is for a specific numbered GitHub Issue, place it in
test/regression/issue/${issueNumber}.test.tswith a REAL issue number, not a placeholder
Files:
test/regression/issue/12053.test.ts
**/*.test.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.test.{ts,tsx}: For single-file tests, prefer using-eflag overtempDir
For multi-file tests, prefer usingtempDirfromharnessandBun.spawnover other temporary directory creation methods
Always useport: 0for network tests and do not hardcode ports or use custom random port number functions
UsenormalizeBunSnapshotto normalize snapshot output of tests
Never write tests that check for no 'panic' or 'uncaught exception' or similar in the test output - that is NOT a valid test
UsetempDirfromharnessto create temporary directories, do not usetmpdirSyncorfs.mkdtempSync
When spawning processes in tests, check stdout/stderr expectations BEFORE checking exit code to get more useful error messages on test failure
Do not write flaky tests - do not usesetTimeoutin tests, insteadawaitthe condition to be met as you are testing the CONDITION not the TIME PASSING
Verify your test fails withUSE_SYSTEM_BUN=1 bun test <file>and passes withbun bd test <file>- your test is NOT VALID if it passes withUSE_SYSTEM_BUN=1
Files:
test/regression/issue/12053.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:testwith files that end in*.test.{ts,js,jsx,tsx,mjs,cjs}
Do not write flaky tests. Never wait for time to pass in tests; always wait for the condition to be met instead of using an arbitrary amount of time
Never use hardcoded port numbers in tests. Always useport: 0to get a random port
Prefer concurrent tests over sequential tests usingtest.concurrentordescribe.concurrentwhen multiple tests spawn processes or write files, unless it's very difficult to make them concurrent
When spawning Bun processes in tests, usebunExeandbunEnvfromharnessto ensure the same build of Bun is used and debug logging is silenced
Use-eflag for single-file tests when spawning Bun processes
UsetempDir()from harness to create temporary directories with files for multi-file tests instead of creating files manually
Prefer async/await over callbacks in tests
When callbacks must be used and it's just a single callback, usePromise.withResolversto create a promise that can be resolved or rejected from a callback
Do not set a timeout on tests. Bun already has timeouts
UseBuffer.alloc(count, fill).toString()instead of'A'.repeat(count)to create repetitive strings in tests, as ''.repeat is very slow in debug JavaScriptCore builds
Usedescribeblocks for grouping related tests
Always useawait usingorusingto ensure proper resource cleanup in tests for APIs like Bun.listen, Bun.connect, Bun.spawn, Bun.serve, etc
Always check exit codes and test error scenarios in error tests
Usedescribe.each()for parameterized tests
UsetoMatchSnapshot()for snapshot testing
UsebeforeAll(),afterEach(),beforeEach()for setup/teardown in tests
Track resources (servers, clients) in arrays for cleanup inafterEach()
Files:
test/regression/issue/12053.test.ts
test/regression/issue/**/*.test.ts
📄 CodeRabbit inference engine (test/CLAUDE.md)
Regression tests for specific issues go in
/test/regression/issue/${issueNumber}.test.ts. Do not put tests without issue numbers in the regression directory
Files:
test/regression/issue/12053.test.ts
🧠 Learnings (16)
📓 Common learnings
Learnt from: markovejnovic
Repo: oven-sh/bun PR: 24423
File: test/js/node/path/browserify.test.js:472-475
Timestamp: 2025-11-07T17:28:58.045Z
Learning: markovejnovic prefers functional programming patterns (e.g., using reduce) over imperative loops and does not want style-based readability suggestions unless there's a concrete technical reason beyond personal preference.
<!--
📚 Learning: 2025-11-24T18:37:30.259Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: test/CLAUDE.md:0-0
Timestamp: 2025-11-24T18:37:30.259Z
Learning: Applies to test/regression/issue/**/*.test.ts : Regression tests for specific issues go in `/test/regression/issue/${issueNumber}.test.ts`. Do not put tests without issue numbers in the regression directory
Applied to files:
test/regression/issue/12053.test.ts
📚 Learning: 2025-11-24T18:37:30.259Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: test/CLAUDE.md:0-0
Timestamp: 2025-11-24T18:37:30.259Z
Learning: Applies to test/**/*.test.{ts,js,jsx,tsx,mjs,cjs} : Track resources (servers, clients) in arrays for cleanup in `afterEach()`
Applied to files:
test/regression/issue/12053.test.ts
📚 Learning: 2025-10-18T05:23:24.403Z
Learnt from: theshadow27
Repo: oven-sh/bun PR: 23798
File: test/js/bun/telemetry-server.test.ts:91-100
Timestamp: 2025-10-18T05:23:24.403Z
Learning: In the Bun codebase, telemetry tests (test/js/bun/telemetry-*.test.ts) should focus on telemetry API behavior: configure/disable/isEnabled, callback signatures and invocation, request ID correlation, and error handling. HTTP protocol behaviors like status code normalization (e.g., 200 with empty body → 204) should be tested in HTTP server tests (test/js/bun/http/), not in telemetry tests. Keep separation of concerns: telemetry tests verify the telemetry API contract; HTTP tests verify HTTP semantics.
Applied to files:
test/regression/issue/12053.test.ts
📚 Learning: 2025-10-19T04:55:33.099Z
Learnt from: theshadow27
Repo: oven-sh/bun PR: 23798
File: test/js/bun/http/node-telemetry.test.ts:27-203
Timestamp: 2025-10-19T04:55:33.099Z
Learning: In test/js/bun/http/node-telemetry.test.ts and the Bun.telemetry._node_binding API, after the architecture refactor, the _node_binding interface only contains two methods: handleIncomingRequest(req, res) and handleWriteHead(res, statusCode). The handleRequestFinish hook and other lifecycle hooks were removed during simplification. Both current methods are fully tested.
Applied to files:
test/regression/issue/12053.test.ts
📚 Learning: 2025-11-24T18:37:30.259Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: test/CLAUDE.md:0-0
Timestamp: 2025-11-24T18:37:30.259Z
Learning: Applies to test/**/*.test.{ts,js,jsx,tsx,mjs,cjs} : Never use hardcoded port numbers in tests. Always use `port: 0` to get a random port
Applied to files:
test/regression/issue/12053.test.ts
📚 Learning: 2025-11-24T18:35:50.422Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: .cursor/rules/writing-tests.mdc:0-0
Timestamp: 2025-11-24T18:35:50.422Z
Learning: See `test/harness.ts` for common test utilities and helpers
Applied to files:
test/regression/issue/12053.test.ts
📚 Learning: 2025-11-24T18:36:33.069Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T18:36:33.069Z
Learning: Applies to **/*.test.{ts,tsx} : Always use `port: 0` for network tests and do not hardcode ports or use custom random port number functions
Applied to files:
test/regression/issue/12053.test.ts
📚 Learning: 2025-11-24T18:37:30.259Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: test/CLAUDE.md:0-0
Timestamp: 2025-11-24T18:37:30.259Z
Learning: Applies to test/**/*.test.{ts,js,jsx,tsx,mjs,cjs} : Always use `await using` or `using` to ensure proper resource cleanup in tests for APIs like Bun.listen, Bun.connect, Bun.spawn, Bun.serve, etc
Applied to files:
test/regression/issue/12053.test.ts
📚 Learning: 2025-11-24T18:36:59.706Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: src/bun.js/bindings/v8/CLAUDE.md:0-0
Timestamp: 2025-11-24T18:36:59.706Z
Learning: Applies to src/bun.js/bindings/v8/test/v8/v8.test.ts : Add corresponding test cases to test/v8/v8.test.ts using checkSameOutput() function to compare Node.js and Bun output
Applied to files:
test/regression/issue/12053.test.ts
📚 Learning: 2025-11-24T18:36:33.069Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T18:36:33.069Z
Learning: Applies to test/js/bun/**/*.test.{ts,tsx} : For Bun-specific API tests, use the `test/js/bun/` directory (for http, crypto, ffi, shell, etc.)
Applied to files:
test/regression/issue/12053.test.ts
📚 Learning: 2025-10-30T03:48:10.513Z
Learnt from: theshadow27
Repo: oven-sh/bun PR: 24063
File: packages/bun-otel/test/context-propagation.test.ts:1-7
Timestamp: 2025-10-30T03:48:10.513Z
Learning: In Bun test files, `using` declarations at the describe block level execute during module load/parsing, not during test execution. This means they acquire and dispose resources before any tests run. For test-scoped resource management, use beforeAll/afterAll hooks instead. The pattern `beforeAll(beforeUsingEchoServer); afterAll(afterUsingEchoServer);` is correct for managing ref-counted test resources like the EchoServer in packages/bun-otel/test/ - the using block pattern should not be used at describe-block level for test resources.
<!-- [/add_learning]
Applied to files:
test/regression/issue/12053.test.ts
📚 Learning: 2025-09-02T05:33:37.517Z
Learnt from: Jarred-Sumner
Repo: oven-sh/bun PR: 22323
File: test/js/web/websocket/websocket-subprotocol.test.ts:74-75
Timestamp: 2025-09-02T05:33:37.517Z
Learning: In Bun's runtime, `await using` with Node.js APIs like `net.createServer()` is properly supported and should not be replaced with explicit cleanup. Bun has extended Node.js APIs with proper async dispose support.
Applied to files:
test/regression/issue/12053.test.ts
📚 Learning: 2025-10-25T17:20:19.041Z
Learnt from: theshadow27
Repo: oven-sh/bun PR: 24063
File: test/js/bun/telemetry/server-header-injection.test.ts:5-20
Timestamp: 2025-10-25T17:20:19.041Z
Learning: In the Bun telemetry codebase, tests are organized into two distinct layers: (1) Internal API tests in test/js/bun/telemetry/ use numeric InstrumentKind enum values to test Zig↔JS injection points and low-level integration; (2) Public API tests in packages/bun-otel/test/ use string InstrumentKind values ("http", "fetch", etc.) to test the public-facing BunSDK and instrumentation APIs. This separation allows internal tests to use efficient numeric enums for refactoring flexibility while the public API maintains a developer-friendly string-based interface.
Applied to files:
test/regression/issue/12053.test.ts
📚 Learning: 2025-09-15T20:47:57.118Z
Learnt from: cirospaciari
Repo: oven-sh/bun PR: 22696
File: src/sql/mysql/js/JSMySQLConnection.zig:306-322
Timestamp: 2025-09-15T20:47:57.118Z
Learning: bun.Async.KeepAlive uses a simple state machine (active/inactive/done) rather than reference counting, with internal status field preventing issues from multiple ref/unref calls, making additional idempotence guards unnecessary.
Applied to files:
test/regression/issue/12053.test.ts
📚 Learning: 2025-11-24T18:35:50.422Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: .cursor/rules/writing-tests.mdc:0-0
Timestamp: 2025-11-24T18:35:50.422Z
Learning: When fixing a Node.js compatibility bug, first verify the expected behavior works in Node.js before testing against Bun's canary version
Applied to files:
test/regression/issue/12053.test.ts
🔇 Additional comments (4)
test/regression/issue/12053.test.ts (4)
1-15: Excellent test documentation.The header comment clearly explains the issue, root causes, and testing approach. This context is valuable for future maintainers.
55-95: Strong test coverage for case-insensitive header handling.This test suite directly verifies the fix for case-insensitive Connection header handling. Using raw
net.createServerto control exact header casing is the right approach.
97-132: Correct handling of Connection: close.The test properly verifies that
Connection: close(in any casing) prevents connection reuse. Thesocket.end()call on line 105 correctly enforces the close semantics.
134-170: Thorough verification of connection reuse across multiple requests.Testing 5 sequential requests ensures the connection reuse mechanism is stable beyond just two requests. The test logic is sound.
| test.each([ | ||
| { keepAlive: true, expectedSockets: 1, description: "reuses TCP connection" }, | ||
| { keepAlive: false, expectedSockets: 2, description: "creates new TCP connection per request" }, | ||
| ])("agent with keepAlive: $keepAlive $description", async ({ keepAlive, expectedSockets }) => { | ||
| const agent = new http.Agent({ keepAlive }); | ||
| const serverSockets: Set<net.Socket> = new Set(); | ||
|
|
||
| // Track server-side sockets to verify connection reuse | ||
| const server = net.createServer(socket => { | ||
| serverSockets.add(socket); | ||
| socket.on("data", () => { | ||
| socket.write("HTTP/1.1 200 OK\r\n" + "Connection: keep-alive\r\n" + "Content-Length: 2\r\n" + "\r\n" + "OK"); | ||
| }); | ||
| }); | ||
|
|
||
| await new Promise<void>(resolve => server.listen(0, resolve)); | ||
| const { port } = server.address() as { port: number }; | ||
|
|
||
| const makeRequest = () => | ||
| new Promise<void>((resolve, reject) => { | ||
| http | ||
| .get({ hostname: "localhost", port, agent, path: "/" }, res => { | ||
| res.on("data", () => {}); | ||
| res.on("end", resolve); | ||
| }) | ||
| .on("error", reject); | ||
| }); | ||
|
|
||
| try { | ||
| await makeRequest(); | ||
| await makeRequest(); | ||
|
|
||
| expect(serverSockets.size).toBe(expectedSockets); | ||
| } finally { | ||
| agent.destroy(); | ||
| server.close(); | ||
| } | ||
| }); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider using await using for automatic resource cleanup.
The current try/finally pattern works, but the guidelines recommend using await using for resource management. Bun has extended Node.js APIs with async dispose support.
Consider refactoring to use await using:
- test.each([
+ test.each([
{ keepAlive: true, expectedSockets: 1, description: "reuses TCP connection" },
{ keepAlive: false, expectedSockets: 2, description: "creates new TCP connection per request" },
])("agent with keepAlive: $keepAlive $description", async ({ keepAlive, expectedSockets }) => {
- const agent = new http.Agent({ keepAlive });
+ await using agent = new http.Agent({ keepAlive });
const serverSockets: Set<net.Socket> = new Set();
- // Track server-side sockets to verify connection reuse
- const server = net.createServer(socket => {
+ await using server = net.createServer(socket => {
serverSockets.add(socket);
socket.on("data", () => {
socket.write("HTTP/1.1 200 OK\r\n" + "Connection: keep-alive\r\n" + "Content-Length: 2\r\n" + "\r\n" + "OK");
});
});
await new Promise<void>(resolve => server.listen(0, resolve));
const { port } = server.address() as { port: number };
const makeRequest = () =>
new Promise<void>((resolve, reject) => {
http
.get({ hostname: "localhost", port, agent, path: "/" }, res => {
res.on("data", () => {});
res.on("end", resolve);
})
.on("error", reject);
});
- try {
- await makeRequest();
- await makeRequest();
+ await makeRequest();
+ await makeRequest();
- expect(serverSockets.size).toBe(expectedSockets);
- } finally {
- agent.destroy();
- server.close();
- }
+ expect(serverSockets.size).toBe(expectedSockets);
});This pattern would apply to all test cases in this file and ensures cleanup happens automatically even if the test throws unexpectedly.
Based on coding guidelines for test files.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In test/regression/issue/12053.test.ts around lines 16 to 53, replace the manual
try/finally cleanup with Bun's async-disposable pattern: acquire the http.Agent
and the net.Server with "await using" so they are automatically disposed/closed
at scope exit; ensure the server and agent are created in the using scope (or
wrapped in objects that implement async dispose if necessary) and remove
agent.destroy()/server.close() from the finally block; apply the same refactor
to all similar tests in this file so resource cleanup is handled uniformly.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
test/regression/issue/12053.test.ts (1)
16-248: Excellent test coverage; considerawait usingfor resource cleanup.The test suite effectively validates the HTTP connection reuse fix with comprehensive scenarios including case-insensitive headers and various keepAlive configurations.
Per coding guidelines, consider using
await usingfor automatic resource cleanup instead of try/finally blocks:test.each([...])("...", async ({ keepAlive, expectedSockets }) => { await using agent = new http.Agent({ keepAlive }); const serverSockets: Set<net.Socket> = new Set(); await using server = net.createServer(socket => { serverSockets.add(socket); socket.on("data", () => { socket.write("HTTP/1.1 200 OK\r\nConnection: keep-alive\r\nContent-Length: 2\r\n\r\nOK"); }); }); await new Promise<void>(resolve => server.listen(0, resolve)); const { port } = server.address() as { port: number }; // ... test logic ... expect(serverSockets.size).toBe(expectedSockets); // No finally block needed - resources auto-cleanup });This pattern applies to all tests in the file and ensures cleanup even if assertions throw unexpectedly.
Based on coding guidelines for test files and past review comments.
📜 Review details
Configuration used: Path: .coderabbit.yaml
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/http.zig(5 hunks)test/regression/issue/12053.test.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (10)
src/**/*.{cpp,zig}
📄 CodeRabbit inference engine (.cursor/rules/building-bun.mdc)
src/**/*.{cpp,zig}: Usebun bdorbun run build:debugto build debug versions for C++ and Zig source files; creates debug build at./build/debug/bun-debug
Run tests usingbun bd test <test-file>with the debug build; never usebun testdirectly as it will not include your changes
Execute files usingbun bd <file> <...args>; never usebun <file>directly as it will not include your changes
Enable debug logs for specific scopes usingBUN_DEBUG_$(SCOPE)=1environment variable
Code generation happens automatically as part of the build process; no manual code generation commands are required
Files:
src/http.zig
src/**/*.zig
📄 CodeRabbit inference engine (.cursor/rules/building-bun.mdc)
Use
bun.Output.scoped(.${SCOPE}, .hidden)for creating debug logs in Zig codeImplement core functionality in Zig, typically in its own directory in
src/
src/**/*.zig: Zig source code should be organized insrc/*.zigfor core runtime, JavaScript bindings, and package manager
In Zig code, be careful with allocators and use defer for cleanup to manage memory properly
UseBUN_DEBUG_QUIET_LOGS=1to disable debug logging, orBUN_DEBUG_<scopeName>=1to enable specific Output.scoped calls in debug builds
Runbun run zig:check-allto compile the Zig code on all platforms when making platform-specific changes
src/**/*.zig: Private fields in Zig are fully supported using the#prefix:struct { #foo: u32 };
Use decl literals in Zig for declaration initialization:const decl: Decl = .{ .binding = 0, .value = 0 };
Prefer@importat the bottom of the file (auto formatter will move them automatically)
Files:
src/http.zig
**/*.zig
📄 CodeRabbit inference engine (.cursor/rules/zig-javascriptcore-classes.mdc)
**/*.zig: Expose generated bindings in Zig structs usingpub const js = JSC.Codegen.JS<ClassName>with trait conversion methods:toJS,fromJS, andfromJSDirect
Use consistent parameter nameglobalObjectinstead ofctxin Zig constructor and method implementations
Usebun.JSError!JSValuereturn type for Zig methods and constructors to enable proper error handling and exception propagation
Implement resource cleanup usingdeinit()method that releases resources, followed byfinalize()called by the GC that invokesdeinit()and frees the pointer
UseJSC.markBinding(@src())in finalize methods for debugging purposes before callingdeinit()
For methods returning cached properties in Zig, declare external C++ functions usingextern fnandcallconv(JSC.conv)calling convention
Implement getter functions with naming patternget<PropertyName>in Zig that acceptthisandglobalObjectparameters and returnJSC.JSValue
Access JavaScript CallFrame arguments usingcallFrame.argument(i), check argument count withcallFrame.argumentCount(), and getthiswithcallFrame.thisValue()
For reference-counted objects, use.deref()in finalize instead ofdestroy()to release references to other JS objects
Files:
src/http.zig
src/**/*.{ts,zig,cpp}
📄 CodeRabbit inference engine (CLAUDE.md)
Always use absolute paths in file operations
Files:
src/http.zig
src/**/*.{ts,zig}
📄 CodeRabbit inference engine (CLAUDE.md)
Avoid shell commands in code - don't use
findorgrep; use Bun's Glob and built-in tools instead
Files:
src/http.zig
test/**/*.{js,ts,jsx,tsx}
📄 CodeRabbit inference engine (.cursor/rules/writing-tests.mdc)
test/**/*.{js,ts,jsx,tsx}: Write tests as JavaScript and TypeScript files using Jest-style APIs (test,describe,expect) and import frombun:test
Usetest.eachand data-driven tests to reduce boilerplate when testing multiple similar cases
Files:
test/regression/issue/12053.test.ts
test/regression/issue/**/*.test.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
When a test is for a specific numbered GitHub Issue, place it in
test/regression/issue/${issueNumber}.test.tswith a REAL issue number, not a placeholder
Files:
test/regression/issue/12053.test.ts
**/*.test.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.test.{ts,tsx}: For single-file tests, prefer using-eflag overtempDir
For multi-file tests, prefer usingtempDirfromharnessandBun.spawnover other temporary directory creation methods
Always useport: 0for network tests and do not hardcode ports or use custom random port number functions
UsenormalizeBunSnapshotto normalize snapshot output of tests
Never write tests that check for no 'panic' or 'uncaught exception' or similar in the test output - that is NOT a valid test
UsetempDirfromharnessto create temporary directories, do not usetmpdirSyncorfs.mkdtempSync
When spawning processes in tests, check stdout/stderr expectations BEFORE checking exit code to get more useful error messages on test failure
Do not write flaky tests - do not usesetTimeoutin tests, insteadawaitthe condition to be met as you are testing the CONDITION not the TIME PASSING
Verify your test fails withUSE_SYSTEM_BUN=1 bun test <file>and passes withbun bd test <file>- your test is NOT VALID if it passes withUSE_SYSTEM_BUN=1
Files:
test/regression/issue/12053.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:testwith files that end in*.test.{ts,js,jsx,tsx,mjs,cjs}
Do not write flaky tests. Never wait for time to pass in tests; always wait for the condition to be met instead of using an arbitrary amount of time
Never use hardcoded port numbers in tests. Always useport: 0to get a random port
Prefer concurrent tests over sequential tests usingtest.concurrentordescribe.concurrentwhen multiple tests spawn processes or write files, unless it's very difficult to make them concurrent
When spawning Bun processes in tests, usebunExeandbunEnvfromharnessto ensure the same build of Bun is used and debug logging is silenced
Use-eflag for single-file tests when spawning Bun processes
UsetempDir()from harness to create temporary directories with files for multi-file tests instead of creating files manually
Prefer async/await over callbacks in tests
When callbacks must be used and it's just a single callback, usePromise.withResolversto create a promise that can be resolved or rejected from a callback
Do not set a timeout on tests. Bun already has timeouts
UseBuffer.alloc(count, fill).toString()instead of'A'.repeat(count)to create repetitive strings in tests, as ''.repeat is very slow in debug JavaScriptCore builds
Usedescribeblocks for grouping related tests
Always useawait usingorusingto ensure proper resource cleanup in tests for APIs like Bun.listen, Bun.connect, Bun.spawn, Bun.serve, etc
Always check exit codes and test error scenarios in error tests
Usedescribe.each()for parameterized tests
UsetoMatchSnapshot()for snapshot testing
UsebeforeAll(),afterEach(),beforeEach()for setup/teardown in tests
Track resources (servers, clients) in arrays for cleanup inafterEach()
Files:
test/regression/issue/12053.test.ts
test/regression/issue/**/*.test.ts
📄 CodeRabbit inference engine (test/CLAUDE.md)
Regression tests for specific issues go in
/test/regression/issue/${issueNumber}.test.ts. Do not put tests without issue numbers in the regression directory
Files:
test/regression/issue/12053.test.ts
🧠 Learnings (23)
📓 Common learnings
Learnt from: theshadow27
Repo: oven-sh/bun PR: 23798
File: test/js/bun/telemetry-server.test.ts:91-100
Timestamp: 2025-10-18T05:23:24.403Z
Learning: In the Bun codebase, telemetry tests (test/js/bun/telemetry-*.test.ts) should focus on telemetry API behavior: configure/disable/isEnabled, callback signatures and invocation, request ID correlation, and error handling. HTTP protocol behaviors like status code normalization (e.g., 200 with empty body → 204) should be tested in HTTP server tests (test/js/bun/http/), not in telemetry tests. Keep separation of concerns: telemetry tests verify the telemetry API contract; HTTP tests verify HTTP semantics.
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: .cursor/rules/registering-bun-modules.mdc:0-0
Timestamp: 2025-11-24T18:35:39.205Z
Learning: Add tests for new Bun runtime functionality
Learnt from: theshadow27
Repo: oven-sh/bun PR: 23798
File: test/js/bun/http/node-telemetry.test.ts:27-203
Timestamp: 2025-10-19T04:55:33.099Z
Learning: In test/js/bun/http/node-telemetry.test.ts and the Bun.telemetry._node_binding API, after the architecture refactor, the _node_binding interface only contains two methods: handleIncomingRequest(req, res) and handleWriteHead(res, statusCode). The handleRequestFinish hook and other lifecycle hooks were removed during simplification. Both current methods are fully tested.
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T18:36:33.069Z
Learning: Applies to test/js/bun/**/*.test.{ts,tsx} : For Bun-specific API tests, use the `test/js/bun/` directory (for http, crypto, ffi, shell, etc.)
Learnt from: cirospaciari
Repo: oven-sh/bun PR: 22946
File: test/js/sql/sql.test.ts:195-202
Timestamp: 2025-09-25T22:07:13.851Z
Learning: PR oven-sh/bun#22946: JSON/JSONB result parsing updates (e.g., returning parsed arrays instead of legacy strings) are out of scope for this PR; tests keep current expectations with a TODO. Handle parsing fixes in a separate PR.
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: src/bun.js/bindings/v8/CLAUDE.md:0-0
Timestamp: 2025-11-24T18:36:59.706Z
Learning: Applies to src/bun.js/bindings/v8/test/v8/v8.test.ts : Add corresponding test cases to test/v8/v8.test.ts using checkSameOutput() function to compare Node.js and Bun output
Learnt from: markovejnovic
Repo: oven-sh/bun PR: 24423
File: test/js/node/path/browserify.test.js:472-475
Timestamp: 2025-11-07T17:28:58.045Z
Learning: markovejnovic prefers functional programming patterns (e.g., using reduce) over imperative loops and does not want style-based readability suggestions unless there's a concrete technical reason beyond personal preference.
<!--
📚 Learning: 2025-09-05T18:45:29.200Z
Learnt from: markovejnovic
Repo: oven-sh/bun PR: 21728
File: src/valkey/js_valkey.zig:0-0
Timestamp: 2025-09-05T18:45:29.200Z
Learning: In JSValkeyClient.cloneWithoutConnecting() in src/valkey/js_valkey.zig, the address/username/password fields must be repointed to the duplicated connection_strings buffer to avoid use-after-free when the original client is destroyed. The original client properly frees connection_strings in ValkeyClient.deinit().
Applied to files:
src/http.zig
📚 Learning: 2025-11-24T18:37:30.259Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: test/CLAUDE.md:0-0
Timestamp: 2025-11-24T18:37:30.259Z
Learning: Applies to test/regression/issue/**/*.test.ts : Regression tests for specific issues go in `/test/regression/issue/${issueNumber}.test.ts`. Do not put tests without issue numbers in the regression directory
Applied to files:
test/regression/issue/12053.test.ts
📚 Learning: 2025-10-18T05:23:24.403Z
Learnt from: theshadow27
Repo: oven-sh/bun PR: 23798
File: test/js/bun/telemetry-server.test.ts:91-100
Timestamp: 2025-10-18T05:23:24.403Z
Learning: In the Bun codebase, telemetry tests (test/js/bun/telemetry-*.test.ts) should focus on telemetry API behavior: configure/disable/isEnabled, callback signatures and invocation, request ID correlation, and error handling. HTTP protocol behaviors like status code normalization (e.g., 200 with empty body → 204) should be tested in HTTP server tests (test/js/bun/http/), not in telemetry tests. Keep separation of concerns: telemetry tests verify the telemetry API contract; HTTP tests verify HTTP semantics.
Applied to files:
test/regression/issue/12053.test.ts
📚 Learning: 2025-10-19T04:55:33.099Z
Learnt from: theshadow27
Repo: oven-sh/bun PR: 23798
File: test/js/bun/http/node-telemetry.test.ts:27-203
Timestamp: 2025-10-19T04:55:33.099Z
Learning: In test/js/bun/http/node-telemetry.test.ts and the Bun.telemetry._node_binding API, after the architecture refactor, the _node_binding interface only contains two methods: handleIncomingRequest(req, res) and handleWriteHead(res, statusCode). The handleRequestFinish hook and other lifecycle hooks were removed during simplification. Both current methods are fully tested.
Applied to files:
test/regression/issue/12053.test.ts
📚 Learning: 2025-11-24T18:37:30.259Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: test/CLAUDE.md:0-0
Timestamp: 2025-11-24T18:37:30.259Z
Learning: Applies to test/**/*.test.{ts,js,jsx,tsx,mjs,cjs} : Track resources (servers, clients) in arrays for cleanup in `afterEach()`
Applied to files:
test/regression/issue/12053.test.ts
📚 Learning: 2025-11-24T18:35:50.422Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: .cursor/rules/writing-tests.mdc:0-0
Timestamp: 2025-11-24T18:35:50.422Z
Learning: See `test/harness.ts` for common test utilities and helpers
Applied to files:
test/regression/issue/12053.test.ts
📚 Learning: 2025-11-24T18:36:33.069Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T18:36:33.069Z
Learning: Applies to **/*.test.{ts,tsx} : Always use `port: 0` for network tests and do not hardcode ports or use custom random port number functions
Applied to files:
test/regression/issue/12053.test.ts
📚 Learning: 2025-11-24T18:37:30.259Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: test/CLAUDE.md:0-0
Timestamp: 2025-11-24T18:37:30.259Z
Learning: Applies to test/**/*.test.{ts,js,jsx,tsx,mjs,cjs} : Never use hardcoded port numbers in tests. Always use `port: 0` to get a random port
Applied to files:
test/regression/issue/12053.test.ts
📚 Learning: 2025-11-24T18:36:33.069Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T18:36:33.069Z
Learning: Applies to test/js/node/**/*.test.{ts,tsx} : For Node.js compatibility tests, use the `test/js/node/` directory
Applied to files:
test/regression/issue/12053.test.ts
📚 Learning: 2025-11-24T18:36:59.706Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: src/bun.js/bindings/v8/CLAUDE.md:0-0
Timestamp: 2025-11-24T18:36:59.706Z
Learning: Applies to src/bun.js/bindings/v8/test/v8/v8.test.ts : Add corresponding test cases to test/v8/v8.test.ts using checkSameOutput() function to compare Node.js and Bun output
Applied to files:
test/regression/issue/12053.test.ts
📚 Learning: 2025-11-24T18:35:08.612Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: .cursor/rules/dev-server-tests.mdc:0-0
Timestamp: 2025-11-24T18:35:08.612Z
Learning: Applies to test/bake/dev/ecosystem.test.ts : Organize ecosystem tests in ecosystem.test.ts for tests involving ensuring certain libraries are correct; prefer testing concrete bugs over testing entire packages
Applied to files:
test/regression/issue/12053.test.ts
📚 Learning: 2025-11-24T18:37:30.259Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: test/CLAUDE.md:0-0
Timestamp: 2025-11-24T18:37:30.259Z
Learning: Applies to test/**/*.test.{ts,js,jsx,tsx,mjs,cjs} : Always use `await using` or `using` to ensure proper resource cleanup in tests for APIs like Bun.listen, Bun.connect, Bun.spawn, Bun.serve, etc
Applied to files:
test/regression/issue/12053.test.ts
📚 Learning: 2025-11-06T00:58:23.965Z
Learnt from: markovejnovic
Repo: oven-sh/bun PR: 24417
File: test/js/bun/spawn/spawn.test.ts:903-918
Timestamp: 2025-11-06T00:58:23.965Z
Learning: In Bun test files, `await using` with spawn() is appropriate for long-running processes that need guaranteed cleanup on scope exit or when explicitly testing disposal behavior. For short-lived processes that exit naturally (e.g., console.log scripts), the pattern `const proc = spawn(...); await proc.exited;` is standard and more common, as evidenced by 24 instances vs 4 `await using` instances in test/js/bun/spawn/spawn.test.ts.
Applied to files:
test/regression/issue/12053.test.ts
📚 Learning: 2025-09-02T05:33:37.517Z
Learnt from: Jarred-Sumner
Repo: oven-sh/bun PR: 22323
File: test/js/web/websocket/websocket-subprotocol.test.ts:74-75
Timestamp: 2025-09-02T05:33:37.517Z
Learning: In Bun's runtime, `await using` with Node.js APIs like `net.createServer()` is properly supported and should not be replaced with explicit cleanup. Bun has extended Node.js APIs with proper async dispose support.
Applied to files:
test/regression/issue/12053.test.ts
📚 Learning: 2025-10-30T03:48:10.513Z
Learnt from: theshadow27
Repo: oven-sh/bun PR: 24063
File: packages/bun-otel/test/context-propagation.test.ts:1-7
Timestamp: 2025-10-30T03:48:10.513Z
Learning: In Bun test files, `using` declarations at the describe block level execute during module load/parsing, not during test execution. This means they acquire and dispose resources before any tests run. For test-scoped resource management, use beforeAll/afterAll hooks instead. The pattern `beforeAll(beforeUsingEchoServer); afterAll(afterUsingEchoServer);` is correct for managing ref-counted test resources like the EchoServer in packages/bun-otel/test/ - the using block pattern should not be used at describe-block level for test resources.
<!-- [/add_learning]
Applied to files:
test/regression/issue/12053.test.ts
📚 Learning: 2025-11-08T04:06:33.198Z
Learnt from: Jarred-Sumner
Repo: oven-sh/bun PR: 24491
File: test/js/bun/transpiler/declare-global.test.ts:17-17
Timestamp: 2025-11-08T04:06:33.198Z
Learning: In Bun test files, `await using` with Bun.spawn() is the preferred pattern for spawned processes regardless of whether they are short-lived or long-running. Do not suggest replacing `await using proc = Bun.spawn(...)` with `const proc = Bun.spawn(...); await proc.exited;`.
Applied to files:
test/regression/issue/12053.test.ts
📚 Learning: 2025-11-24T18:37:30.259Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: test/CLAUDE.md:0-0
Timestamp: 2025-11-24T18:37:30.259Z
Learning: Applies to test/**/*.test.{ts,js,jsx,tsx,mjs,cjs} : Prefer async/await over callbacks in tests
Applied to files:
test/regression/issue/12053.test.ts
📚 Learning: 2025-09-20T00:58:38.042Z
Learnt from: markovejnovic
Repo: oven-sh/bun PR: 22568
File: test/js/valkey/valkey.test.ts:561-564
Timestamp: 2025-09-20T00:58:38.042Z
Learning: For test/js/valkey/valkey.test.ts, do not comment on synchronous throw assertions for async Redis methods (like ctx.redis.set(), ctx.redis.unsubscribe(), etc.) - Bun's Redis client implementation differs from Node.js and can throw synchronously even for async methods. The maintainer has explicitly requested to stop looking at this error pattern.
Applied to files:
test/regression/issue/12053.test.ts
📚 Learning: 2025-09-03T01:30:58.001Z
Learnt from: markovejnovic
Repo: oven-sh/bun PR: 21728
File: test/js/valkey/valkey.test.ts:264-271
Timestamp: 2025-09-03T01:30:58.001Z
Learning: For test/js/valkey/valkey.test.ts PUB/SUB tests, avoid arbitrary sleeps and async-forEach. Instead, resolve a Promise from the subscriber callback when the expected number of messages is observed and await it with a bounded timeout (e.g., withTimeout + Promise.withResolvers) to account for Redis server→subscriber propagation.
Applied to files:
test/regression/issue/12053.test.ts
📚 Learning: 2025-11-24T18:36:33.069Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T18:36:33.069Z
Learning: Applies to test/js/bun/**/*.test.{ts,tsx} : For Bun-specific API tests, use the `test/js/bun/` directory (for http, crypto, ffi, shell, etc.)
Applied to files:
test/regression/issue/12053.test.ts
📚 Learning: 2025-10-25T17:20:19.041Z
Learnt from: theshadow27
Repo: oven-sh/bun PR: 24063
File: test/js/bun/telemetry/server-header-injection.test.ts:5-20
Timestamp: 2025-10-25T17:20:19.041Z
Learning: In the Bun telemetry codebase, tests are organized into two distinct layers: (1) Internal API tests in test/js/bun/telemetry/ use numeric InstrumentKind enum values to test Zig↔JS injection points and low-level integration; (2) Public API tests in packages/bun-otel/test/ use string InstrumentKind values ("http", "fetch", etc.) to test the public-facing BunSDK and instrumentation APIs. This separation allows internal tests to use efficient numeric enums for refactoring flexibility while the public API maintains a developer-friendly string-based interface.
Applied to files:
test/regression/issue/12053.test.ts
📚 Learning: 2025-11-24T18:35:50.422Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: .cursor/rules/writing-tests.mdc:0-0
Timestamp: 2025-11-24T18:35:50.422Z
Learning: When fixing a Node.js compatibility bug, first verify the expected behavior works in Node.js before testing against Bun's canary version
Applied to files:
test/regression/issue/12053.test.ts
🧬 Code graph analysis (1)
test/regression/issue/12053.test.ts (2)
src/js/node/_http_client.ts (2)
agent(988-990)agent(992-994)test/js/node/test/parallel/test-net-server-close.js (1)
sockets(27-27)
🔇 Additional comments (3)
src/http.zig (3)
596-605: LGTM! Well-designed helper function.The
connectionHeaderIsKeepAlivehelper properly encapsulates the Connection header logic with:
- Correct RFC 7230 section 6.1 reference
- Appropriate use of
?boolfor tri-state logic (close/keep-alive/other)- Case-insensitive comparison via
std.ascii.eqlIgnoreCase
644-646: LGTM! Correct usage of the helper.The conditional assignment correctly handles the optional return value—
allow_keepaliveis only updated when the Connection header value is recognized.
2295-2297: LGTM! Proper use of the helper in response handling.The Connection header logic correctly:
- Uses the
connectionHeaderIsKeepAlivehelper- Only applies to successful responses (2xx)
- Updates
allow_keepalivebased on the helper's return value
| // > All transfer-coding names are case-insensitive | ||
| // https://datatracker.ietf.org/doc/html/rfc7230#section-4 | ||
| if (strings.eqlCaseInsensitiveASCII(header.value, "gzip", true)) { | ||
| if (!this.flags.disable_decompression) { | ||
| this.state.transfer_encoding = Encoding.gzip; | ||
| } | ||
| } else if (strings.eqlComptime(header.value, "deflate")) { | ||
| } else if (strings.eqlCaseInsensitiveASCII(header.value, "deflate", true)) { | ||
| if (!this.flags.disable_decompression) { | ||
| this.state.transfer_encoding = Encoding.deflate; | ||
| } | ||
| } else if (strings.eqlComptime(header.value, "br")) { | ||
| } else if (strings.eqlCaseInsensitiveASCII(header.value, "br", true)) { | ||
| if (!this.flags.disable_decompression) { | ||
| this.state.transfer_encoding = .brotli; | ||
| } | ||
| } else if (strings.eqlComptime(header.value, "zstd")) { | ||
| } else if (strings.eqlCaseInsensitiveASCII(header.value, "zstd", true)) { | ||
| if (!this.flags.disable_decompression) { | ||
| this.state.transfer_encoding = .zstd; | ||
| } | ||
| } else if (strings.eqlComptime(header.value, "identity")) { | ||
| } else if (strings.eqlCaseInsensitiveASCII(header.value, "identity", true)) { | ||
| this.state.transfer_encoding = Encoding.identity; | ||
| } else if (strings.eqlComptime(header.value, "chunked")) { | ||
| } else if (strings.eqlCaseInsensitiveASCII(header.value, "chunked", true)) { | ||
| this.state.transfer_encoding = Encoding.chunked; | ||
| } else { |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider using std.ascii.eqlIgnoreCase for consistency.
The Transfer-Encoding checks use strings.eqlCaseInsensitiveASCII, while the Connection header helper uses std.ascii.eqlIgnoreCase. Using the same function throughout would improve consistency.
Apply this diff to align with the helper function pattern:
- if (strings.eqlCaseInsensitiveASCII(header.value, "gzip", true)) {
+ if (std.ascii.eqlIgnoreCase(header.value, "gzip")) {
if (!this.flags.disable_decompression) {
this.state.transfer_encoding = Encoding.gzip;
}
- } else if (strings.eqlCaseInsensitiveASCII(header.value, "deflate", true)) {
+ } else if (std.ascii.eqlIgnoreCase(header.value, "deflate")) {
if (!this.flags.disable_decompression) {
this.state.transfer_encoding = Encoding.deflate;
}
- } else if (strings.eqlCaseInsensitiveASCII(header.value, "br", true)) {
+ } else if (std.ascii.eqlIgnoreCase(header.value, "br")) {
if (!this.flags.disable_decompression) {
this.state.transfer_encoding = .brotli;
}
- } else if (strings.eqlCaseInsensitiveASCII(header.value, "zstd", true)) {
+ } else if (std.ascii.eqlIgnoreCase(header.value, "zstd")) {
if (!this.flags.disable_decompression) {
this.state.transfer_encoding = .zstd;
}
- } else if (strings.eqlCaseInsensitiveASCII(header.value, "identity", true)) {
+ } else if (std.ascii.eqlIgnoreCase(header.value, "identity")) {
this.state.transfer_encoding = Encoding.identity;
- } else if (strings.eqlCaseInsensitiveASCII(header.value, "chunked", true)) {
+ } else if (std.ascii.eqlIgnoreCase(header.value, "chunked")) {
this.state.transfer_encoding = Encoding.chunked;
} else {📝 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.
| // > All transfer-coding names are case-insensitive | |
| // https://datatracker.ietf.org/doc/html/rfc7230#section-4 | |
| if (strings.eqlCaseInsensitiveASCII(header.value, "gzip", true)) { | |
| if (!this.flags.disable_decompression) { | |
| this.state.transfer_encoding = Encoding.gzip; | |
| } | |
| } else if (strings.eqlComptime(header.value, "deflate")) { | |
| } else if (strings.eqlCaseInsensitiveASCII(header.value, "deflate", true)) { | |
| if (!this.flags.disable_decompression) { | |
| this.state.transfer_encoding = Encoding.deflate; | |
| } | |
| } else if (strings.eqlComptime(header.value, "br")) { | |
| } else if (strings.eqlCaseInsensitiveASCII(header.value, "br", true)) { | |
| if (!this.flags.disable_decompression) { | |
| this.state.transfer_encoding = .brotli; | |
| } | |
| } else if (strings.eqlComptime(header.value, "zstd")) { | |
| } else if (strings.eqlCaseInsensitiveASCII(header.value, "zstd", true)) { | |
| if (!this.flags.disable_decompression) { | |
| this.state.transfer_encoding = .zstd; | |
| } | |
| } else if (strings.eqlComptime(header.value, "identity")) { | |
| } else if (strings.eqlCaseInsensitiveASCII(header.value, "identity", true)) { | |
| this.state.transfer_encoding = Encoding.identity; | |
| } else if (strings.eqlComptime(header.value, "chunked")) { | |
| } else if (strings.eqlCaseInsensitiveASCII(header.value, "chunked", true)) { | |
| this.state.transfer_encoding = Encoding.chunked; | |
| } else { | |
| // > All transfer-coding names are case-insensitive | |
| // https://datatracker.ietf.org/doc/html/rfc7230#section-4 | |
| if (std.ascii.eqlIgnoreCase(header.value, "gzip")) { | |
| if (!this.flags.disable_decompression) { | |
| this.state.transfer_encoding = Encoding.gzip; | |
| } | |
| } else if (std.ascii.eqlIgnoreCase(header.value, "deflate")) { | |
| if (!this.flags.disable_decompression) { | |
| this.state.transfer_encoding = Encoding.deflate; | |
| } | |
| } else if (std.ascii.eqlIgnoreCase(header.value, "br")) { | |
| if (!this.flags.disable_decompression) { | |
| this.state.transfer_encoding = .brotli; | |
| } | |
| } else if (std.ascii.eqlIgnoreCase(header.value, "zstd")) { | |
| if (!this.flags.disable_decompression) { | |
| this.state.transfer_encoding = .zstd; | |
| } | |
| } else if (std.ascii.eqlIgnoreCase(header.value, "identity")) { | |
| this.state.transfer_encoding = Encoding.identity; | |
| } else if (std.ascii.eqlIgnoreCase(header.value, "chunked")) { | |
| this.state.transfer_encoding = Encoding.chunked; | |
| } else { |
🤖 Prompt for AI Agents
In src/http.zig around lines 2264 to 2286, the Transfer-Encoding comparisons use
strings.eqlCaseInsensitiveASCII while the rest of the codebase uses
std.ascii.eqlIgnoreCase; replace each
strings.eqlCaseInsensitiveASCII(header.value, "<token>", true) call with
std.ascii.eqlIgnoreCase(header.value, "<token>") (remove the third argument) to
match the helper pattern, and ensure std is in scope (or qualify/import it
consistently) so the file compiles.
Uh oh!
There was an error while loading. Please reload this page.