fix(http): disable keep-alive on proxy authentication failure (407) #25884
Conversation
|
Updated 8:18 PM PT - Jan 7th, 2026
❌ @Jarred-Sumner, your commit 37be8db has 3 failures in
🧪 To try this PR locally: bunx bun-pr 25884That installs a local version of the PR into your bun-25884 --bun |
WalkthroughMinor comment fix and explicit disabling of keep-alive in HTTP proxy denial and HTTP 407 handling; test imports updated to add Changes
Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Disabled knowledge base sources:
📒 Files selected for processing (1)
🧰 Additional context used📓 Path-based instructions (2)**/*.zig📄 CodeRabbit inference engine (CLAUDE.md)
Files:
src/**/*.zig📄 CodeRabbit inference engine (src/CLAUDE.md)
Files:
🧠 Learnings (2)📚 Learning: 2025-09-15T20:47:57.118ZApplied to files:
📚 Learning: 2026-01-05T16:32:07.551ZApplied to files:
🔇 Additional comments (3)
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
🤖 Fix all issues with AI agents
In @src/http.zig:
- Around line 525-526: Update the inline comment that currently reads "is not
possible to reuse Proxy with TLS, so disable keepalive if url is tunneling
HTTPS" to restore the missing pronoun so it reads "it is not possible to reuse
Proxy with TLS, so disable keepalive if url is tunneling HTTPS" to improve
readability; locate that comment string in src/http.zig and replace accordingly.
In @test/js/bun/http/proxy.test.js:
- Around line 143-152: The anonymous tests in the loop use test(async () => {
... }) making failures hard to trace; update the test declarations to include
descriptive names (e.g., test(name, async () => { ... })) that reflect the
request/proxy configuration returned by callback() or switch to a parameterized
pattern like it.each with descriptive labels; use values from callback() (the
request or proxy details) to build the test title so each case (the
fetch(request, { verbose: true, proxy })) is uniquely identifiable during
failures.
📜 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.zigtest/js/bun/http/proxy.test.js
🧰 Additional context used
📓 Path-based instructions (3)
**/*.zig
📄 CodeRabbit inference engine (CLAUDE.md)
In Zig code, be careful with allocators and use defer for cleanup
Files:
src/http.zig
src/**/*.zig
📄 CodeRabbit inference engine (src/CLAUDE.md)
src/**/*.zig: Use the#prefix for private fields in Zig structs, e.g.,struct { #foo: u32 };
Use Decl literals in Zig, e.g.,const decl: Decl = .{ .binding = 0, .value = 0 };
Place@importstatements at the bottom of the file in Zig (auto formatter will handle positioning)
Never use@import()inline inside functions in Zig; always place imports at the bottom of the file or containing struct
Files:
src/http.zig
test/**/*.test.{ts,js,jsx,tsx,mjs,cjs}
📄 CodeRabbit inference engine (test/CLAUDE.md)
test/**/*.test.{ts,js,jsx,tsx,mjs,cjs}: Usebun bd test <...test file>to run tests with compiled code changes. Do not usebun testas it will not include your changes.
Usebun:testfor files ending in*.test.{ts,js,jsx,tsx,mjs,cjs}. For test files without .test extension in test/js/node/test/{parallel,sequential}/*.js, usebun bd <file>instead ofbun bd test <file>since they expect exit code 0.
Do not set a timeout on tests. Bun already has timeouts built-in.
Files:
test/js/bun/http/proxy.test.js
🧠 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: 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: test/CLAUDE.md:0-0
Timestamp: 2026-01-05T23:04:01.518Z
Learning: Applies to test/**/*.test.{ts,js,jsx,tsx,mjs,cjs} : Use `bun bd test <...test file>` to run tests with compiled code changes. Do not use `bun test` as it will not include your changes.
📚 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:
src/http.zig
📚 Learning: 2026-01-05T16:32:07.551Z
Learnt from: alii
Repo: oven-sh/bun PR: 25474
File: src/bun.js/event_loop/Sigusr1Handler.zig:0-0
Timestamp: 2026-01-05T16:32:07.551Z
Learning: In Zig codebases (e.g., Bun), treat std.posix.sigaction as returning void and do not perform runtime error handling for its failure. The Zig standard library views sigaction failures as programmer errors (unreachable) because they only occur with invalid signals like SIGKILL/SIGSTOP. Apply this pattern across Zig files that call sigaction (e.g., crash_handler.zig, main.zig, filter_run.zig, process.zig) and ensure failures are not handled as recoverable errors; prefer reaching an explicit unreachable/compile-time assumption when such failures are detected.
Applied to files:
src/http.zig
📚 Learning: 2026-01-05T23:04:01.518Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: test/CLAUDE.md:0-0
Timestamp: 2026-01-05T23:04:01.518Z
Learning: Applies to test/**/*.test.{ts,js,jsx,tsx,mjs,cjs} : Use `bun bd test <...test file>` to run tests with compiled code changes. Do not use `bun test` as it will not include your changes.
Applied to files:
test/js/bun/http/proxy.test.js
📚 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/js/bun/http/proxy.test.js
📚 Learning: 2026-01-05T23:04:01.518Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: test/CLAUDE.md:0-0
Timestamp: 2026-01-05T23:04:01.518Z
Learning: Applies to test/**/*.test.{ts,js,jsx,tsx,mjs,cjs} : Use `bun:test` for files ending in `*.test.{ts,js,jsx,tsx,mjs,cjs}`. For test files without .test extension in test/js/node/test/{parallel,sequential}/*.js, use `bun bd <file>` instead of `bun bd test <file>` since they expect exit code 0.
Applied to files:
test/js/bun/http/proxy.test.js
📚 Learning: 2025-09-20T03:39:41.770Z
Learnt from: pfgithub
Repo: oven-sh/bun PR: 22534
File: test/regression/issue/21830.fixture.ts:14-63
Timestamp: 2025-09-20T03:39:41.770Z
Learning: Bun's test runner supports async describe callbacks, unlike Jest/Vitest where describe callbacks must be synchronous. The syntax `describe("name", async () => { ... })` is valid in Bun.
Applied to files:
test/js/bun/http/proxy.test.js
📚 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/js/bun/http/proxy.test.js
📚 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/js/bun/http/proxy.test.js
📚 Learning: 2026-01-05T23:04:01.518Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: test/CLAUDE.md:0-0
Timestamp: 2026-01-05T23:04:01.518Z
Learning: Organize unit tests by module in directories like `/test/js/bun/` and `/test/js/node/`.
Applied to files:
test/js/bun/http/proxy.test.js
📚 Learning: 2025-12-16T00:21:32.179Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-16T00:21:32.179Z
Learning: Applies to **/*.test.ts?(x) : Never use `bun test` directly - always use `bun bd test` to run tests with debug build changes
Applied to files:
test/js/bun/http/proxy.test.js
📚 Learning: 2025-10-19T02:44:46.354Z
Learnt from: theshadow27
Repo: oven-sh/bun PR: 23798
File: packages/bun-otel/context-propagation.test.ts:1-1
Timestamp: 2025-10-19T02:44:46.354Z
Learning: In the Bun repository, standalone packages under packages/ (e.g., bun-vscode, bun-inspector-protocol, bun-plugin-yaml, bun-plugin-svelte, bun-debug-adapter-protocol, bun-otel) co-locate their tests with package source code using *.test.ts files. This follows standard npm/monorepo patterns. The test/ directory hierarchy (test/js/bun/, test/cli/, test/js/node/) is reserved for testing Bun's core runtime APIs and built-in functionality, not standalone packages.
Applied to files:
test/js/bun/http/proxy.test.js
📚 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/js/bun/http/proxy.test.js
📚 Learning: 2025-12-16T00:21:32.179Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-16T00:21:32.179Z
Learning: Applies to **/*.test.ts?(x) : For multi-file tests, prefer `tempDir` and `Bun.spawn` over single-file tests
Applied to files:
test/js/bun/http/proxy.test.js
📚 Learning: 2025-11-24T18:37:11.466Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: src/js/CLAUDE.md:0-0
Timestamp: 2025-11-24T18:37:11.466Z
Learning: Write JS builtins for Bun's Node.js compatibility and APIs, and run `bun bd` after changes
Applied to files:
test/js/bun/http/proxy.test.js
📚 Learning: 2026-01-05T23:04:01.518Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: test/CLAUDE.md:0-0
Timestamp: 2026-01-05T23:04:01.518Z
Learning: When spawning Bun processes in tests, use `bunExe` and `bunEnv` from `harness` to ensure the same build of Bun is used and debug logging is silenced.
Applied to files:
test/js/bun/http/proxy.test.js
📚 Learning: 2025-12-16T00:21:32.179Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-16T00:21:32.179Z
Learning: Applies to **/*.test.ts?(x) : Verify tests fail with `USE_SYSTEM_BUN=1 bun test <file>` and pass with `bun bd test <file>` - tests are invalid if they pass with USE_SYSTEM_BUN=1
Applied to files:
test/js/bun/http/proxy.test.js
📚 Learning: 2025-10-20T01:38:02.660Z
Learnt from: theshadow27
Repo: oven-sh/bun PR: 23798
File: packages/bun-otel/BunFetchInstrumentation.ts:126-131
Timestamp: 2025-10-20T01:38:02.660Z
Learning: In BunFetchInstrumentation.ts, the force-restore to ORIGINAL_FETCH in the disable() method is intentionally kept (despite appearing unsafe) because it's required for proper test cleanup when instrumentation is repeatedly enabled/disabled. Without it, 15 distributed tracing and context propagation tests fail. Shimmer's unwrap() doesn't reliably restore the original fetch in Bun's globalThis context. The isBunOtelPatched safety check ensures the restore only happens when the current fetch is still ours, preventing clobbering of other tools' wrappers.
Applied to files:
test/js/bun/http/proxy.test.js
📚 Learning: 2025-10-08T13:48:02.430Z
Learnt from: Jarred-Sumner
Repo: oven-sh/bun PR: 23373
File: test/js/bun/tarball/extract.test.ts:107-111
Timestamp: 2025-10-08T13:48:02.430Z
Learning: In Bun's test runner, use `expect(async () => { await ... }).toThrow()` to assert async rejections. Unlike Jest/Vitest, Bun does not require `await expect(...).rejects.toThrow()` - the async function wrapper with `.toThrow()` is the correct pattern for async error assertions in Bun tests.
Applied to files:
test/js/bun/http/proxy.test.js
📚 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/js/bun/http/proxy.test.js
📚 Learning: 2025-12-16T00:21:32.179Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-16T00:21:32.179Z
Learning: Applies to test/**/*.test.ts?(x) : Always use `port: 0` in tests - do not hardcode ports or use custom random port number functions
Applied to files:
test/js/bun/http/proxy.test.js
📚 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/js/bun/http/proxy.test.js
📚 Learning: 2026-01-05T23:04:01.518Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: test/CLAUDE.md:0-0
Timestamp: 2026-01-05T23:04:01.518Z
Learning: Applies to test/**/*.test.{ts,js,jsx,tsx,mjs,cjs} : Do not set a timeout on tests. Bun already has timeouts built-in.
Applied to files:
test/js/bun/http/proxy.test.js
🧬 Code graph analysis (1)
test/js/bun/http/proxy.test.js (1)
test/harness.ts (1)
bunExe(102-105)
🔇 Additional comments (5)
src/http.zig (2)
2342-2344: LGTM: Correctly disables keep-alive on proxy denial.When the proxy denies the CONNECT request (non-200 response), this ensures the connection won't be reused. This prevents subsequent requests from attempting to reuse a closed connection.
2349-2351: LGTM: Core fix for 407 keep-alive issue.This correctly disables keep-alive when receiving HTTP 407 (Proxy Authentication Required), preventing subsequent requests from hanging on closed connections. The fix directly addresses the PR objective.
test/js/bun/http/proxy.test.js (3)
1-1: LGTM: Test organization improved with concurrent execution.The addition of
describeand use ofdescribe.concurrentenables concurrent test execution. The shared proxy, auth_proxy, and server instances should safely handle concurrent requests.Also applies to: 79-79
183-209: LGTM: Excellent test for the keep-alive fix.This test directly validates the fix by sending multiple concurrent requests with invalid proxy credentials. Before the fix, the second batch would hang due to keep-alive on failed connections. The test correctly consumes response bodies to avoid resource leaks.
211-232: LGTM: Correct use of Bun.spawn for environment variable tests.Both test blocks correctly use
bunExe()andbunEnvfrom harness, ensuring the same build of Bun is used and debug logging is silenced. The spawn patterns properly await both exit code and stderr.Also applies to: 234-259
|
|
||
| // is not possible to reuse Proxy with TLS, so disable keepalive if url is tunneling HTTPS |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Minor wording improvement suggestion.
The comment dropped "it" ("is not possible" vs "it is not possible"). While grammatically acceptable, restoring "it" would improve readability.
♻️ Suggested refinement
- // is not possible to reuse Proxy with TLS, so disable keepalive if url is tunneling HTTPS
+ // it is not possible to reuse Proxy with TLS, so disable keepalive if url is tunneling HTTPS📝 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.
| // is not possible to reuse Proxy with TLS, so disable keepalive if url is tunneling HTTPS | |
| // it is not possible to reuse Proxy with TLS, so disable keepalive if url is tunneling HTTPS |
🤖 Prompt for AI Agents
In @src/http.zig around lines 525 - 526, Update the inline comment that
currently reads "is not possible to reuse Proxy with TLS, so disable keepalive
if url is tunneling HTTPS" to restore the missing pronoun so it reads "it is not
possible to reuse Proxy with TLS, so disable keepalive if url is tunneling
HTTPS" to improve readability; locate that comment string in src/http.zig and
replace accordingly.
| test(async () => { | ||
| const [request, proxy] = callback(); | ||
| gc(); | ||
| const response = await fetch(request, { verbose: true, proxy }); | ||
| gc(); | ||
| const text = await response.text(); | ||
| gc(); | ||
| expect(text).toBe("Hello, World"); | ||
| }); | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Add descriptive test names for better debugging.
The tests in this loop lack descriptive names, making it difficult to identify which request configuration fails when debugging.
♻️ Suggested improvement
Consider adding descriptive test names based on the request configuration:
- for (let callback of requests) {
- test(async () => {
+ for (let i = 0; i < requests.length; i++) {
+ test(`proxy request configuration ${i + 1}/${requests.length}`, async () => {
- const [request, proxy] = callback();
+ const [request, proxy] = requests[i]();
gc();
const response = await fetch(request, { verbose: true, proxy });
gc();Or better yet, use it.each with descriptive labels for each configuration.
📝 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.
| test(async () => { | |
| const [request, proxy] = callback(); | |
| gc(); | |
| const response = await fetch(request, { verbose: true, proxy }); | |
| gc(); | |
| const text = await response.text(); | |
| gc(); | |
| expect(text).toBe("Hello, World"); | |
| }); | |
| } | |
| for (let i = 0; i < requests.length; i++) { | |
| test(`proxy request configuration ${i + 1}/${requests.length}`, async () => { | |
| const [request, proxy] = requests[i](); | |
| gc(); | |
| const response = await fetch(request, { verbose: true, proxy }); | |
| gc(); | |
| const text = await response.text(); | |
| gc(); | |
| expect(text).toBe("Hello, World"); | |
| }); | |
| } |
🤖 Prompt for AI Agents
In @test/js/bun/http/proxy.test.js around lines 143 - 152, The anonymous tests
in the loop use test(async () => { ... }) making failures hard to trace; update
the test declarations to include descriptive names (e.g., test(name, async () =>
{ ... })) that reflect the request/proxy configuration returned by callback() or
switch to a parameterized pattern like it.each with descriptive labels; use
values from callback() (the request or proxy details) to build the test title so
each case (the fetch(request, { verbose: true, proxy })) is uniquely
identifiable during failures.
Summary
describe.concurrentand asyncBun.spawnpatternsTest plan
simultaneous proxy auth failures should not hangthat verifies multiple concurrent requests with invalid proxy credentials complete without hanging🤖 Generated with Claude Code