Repository navigation
Conversation
…d the c-ares config - bun_http_jsc: the bstr dependency. The WebSocket rewrite removed the last use. - http1_server_fallback.ts: the shouldKeepAlive member of the fallback response handle. ServerResponse keeps its own flag, so nothing reads the member. The closure parameter of the same name stays in use. - scripts/build/deps/cares.ts: HAVE_SYS_SOCKIO_H, HAVE_WS2IPDEF_H and HAVE_IO_H. No c-ares source file reads them.
|
Updated 12:57 PM PT - Oct 17th, 2026
✅ @robobun, your commit e47f71c3913586500a9c0f9302e87d995fea2246 passed in 🧪 To try this PR locally: bunx bun-pr 45753That installs a local version of the PR into your bun-45753 --bun |
|
Status: ready for review. CI is green. How this was verified:
PR: #45753 |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
WalkthroughThe changes remove selected platform header macros, the ChangesPlatform feature macros
HTTP JSC dependency
HTTP fallback response handle
Suggested reviewers: Priority: ⬇️ Low Merge Risk: ⚪ Minimal · up to
|
| Check name | Status | Explanation |
|---|---|---|
| Linked Issues check |  |
Check skipped because no linked issues were found for this pull request. |
| Out of Scope Changes check | |
Check skipped because no linked issues were found for this pull request. |
| Title check | |
The title clearly summarizes the three main dead-code removals: the bun_http_jsc dependency, the node:http fallback handle member, and the c-ares configuration macros. |
| Description check | |
The description explains the problem, fix, behavior impact, verification steps, and background. It does not use the exact template headings, but it provides the required information and is mostly comp… |
Full details: Description check
-
Explanation
The description explains the problem, fix, behavior impact, verification steps, and background. It does not use the exact template headings, but it provides the required information and is mostly complete.
✨ Finishing Touches: 💡 1
-
🛠️ Fix failing CI checks: 💡
-
- Commit to this branch
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.
Comment @coderabbitai help to get the list of available commands.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. Two of the three removals check out fully from this checkout; the c-ares macro removal affects the generated ares_config.h on Darwin, FreeBSD and Windows and can only be confirmed against the vendored c-ares tree (not present here), so a quick human check of those platforms' builds is still worthwhile.
What was reviewed:
bstrdrop insrc/http_jsc/Cargo.tomlandCargo.lock: no remainingbstrreference anywhere undersrc/http_jsc, including cfg-gated code.shouldKeepAliveremoval from the fallback handle: the parameter is still used for the socket keep-alive decision at line 137; every.shouldKeepAlivereader insrc/js/nodeandtest/is onreq/res, andServerResponse.prototype.shouldKeepAliveis backed by its ownkShouldKeepAliveslot, never the handle.HAVE_SYS_SOCKIO_H,HAVE_WS2IPDEF_H,HAVE_IO_Hinscripts/build/deps/cares.ts: no other consumer in the repo; whether c-ares sources at the pinned commit still test them cannot be checked from this clone.
Extended reasoning...
The change is a 3-insertion / 6-deletion dead-code sweep across four files: an unused Rust dependency in bun_http_jsc with its lockfile entry, an unread property on the JS node:http fallback response handle, and three HAVE_* feature macros in the hand-maintained c-ares config tables. It touches no security-sensitive surface. The Rust and JS removals were verified in-repo by grep (no bstr use; no handle.shouldKeepAlive reader). The c-ares macros change platform-gated generated config for Darwin, FreeBSD and Windows, and the vendored c-ares tree is not checked out here, so the author's claim that no source file reads them could not be independently confirmed; the failure mode would most likely be a loud compile error on those CI lanes rather than silent misbehavior, but that is what tipped the decision to defer rather than approve.
|
On the c-ares macros, the one point the review could not check:
|
Behaviour change: none
Problem
src/http_jsc/Cargo.tomllistsbstr, and no file in the crate uses it. WebSocket: real closing handshake, one failure path,wsrewritten on native transports (consolidates the open WebSocket PRs) #45223 removed the last use.src/js/internal/http1_server_fallback.ts:230) has ashouldKeepAlivemember that nothing reads.ServerResponsestores its own flag (src/js/node/_http_server.ts:4045).scripts/build/deps/cares.tsdefinesHAVE_SYS_SOCKIO_H,HAVE_WS2IPDEF_HandHAVE_IO_H. No c-ares source file reads them.Fix
Cargo.lockentry, the member, and the three macro names.rgfindsbstrnowhere else undersrc/http_jsc, no.shouldKeepAliveread on a handle, and the macro names only in upstream's build files.bun bd,cargo check -p bun_http_jsc --all-targetsfor Linux, Windows, macOS and FreeBSD, and the four node tests in Notes.Background
NodeHTTPResponsewhennode:httpserves a socket that Bun's HTTP server does not own.scripts/build/deps/cares.tswrites theares_config.hof c-ares from per-platform lists of macro names. A name that no source file tests has no effect.Downsides
Notes
Tests that use the fallback handle, run with the debug build:
test/js/node/test/parallel/test-http-generic-streams.js,test-http2-allow-http1.js,test-http2-https-fallback.js,test-http2-https-fallback-http-server-options.js.What this run scanned:
--gc-sectionsand joined the symbol tables with DWARF declaration lines. 1068 source functions lose every instance. Each candidate was deleted andcargo check --workspaceran for Windows, macOS, FreeBSD and Linux with--all-targets. A compile error put the candidate back. Most candidates are Windows or macOS code,const fnbuilders, or debugger helpers.pubgeneric or#[inline]functions that the debug binary never instantiates..rsfile is outside every crate's dep-info. The linker drops no non-inline function from a.cppfile undersrc/.src/js,scripts/,misctools/, and the small packages: read for unused bindings, members and files.Found, and left to the pull request that owns the lines:
Generator.writeZigandexample()inmisctools/unicode-generator.ts: Remove the Zig-emitting unicode table generator from misctools #41473 deletes the file.bun_sys::posix::s_isdirand the Windowsumaskstub: Remove dead code from bun_sys, bun_errno, and bun_libuv_sys #45698 deletes both. fs.watch: watch symlink entries of a recursive tree on Linux #43090 and bun patch: keep the copy of a global store package in the project #43556 calls_isdir.impl Defaultfor the h2SendWindowandRecvWindow: Remove dead code from the code generators, the build scripts, and ten crate roots #45709 deletes both. node:http2: withhold connection window credit after setLocalWindowSize() decreases it #41344 uses them.assertEncodingForWindowsinsrc/js/node/fs.ts: node:fs: reject a non-string, non-object options argument like node #41928 turns??into??=, which makes the variable a cache.hash()of thenew_hasher!types insrc/sha_hmac/sha.rs: AWS default credential chain, SigV4-signed fetch, Bun.aws / Bun.gcp #39210 and publish: add --provenance (npm provenance via Sigstore keyless signing) #30522 call it.impl StaticallyNamed for Ident,Pat,Nodein bun_lint andimpl From<Vec<Doc>>in bun_format: earlier sweeps left them for the owner of the new crates.Probably dead, not in this diff:
AbortSignal__Timeout__run(src/jsc/AbortSignal.rs): a#[no_mangle]export that no C++ file names.Debugimpls that nothing formats, 27 of them insrc/lint.#[host_fn]generates for a method of a.classes.tsclass: the linker drops it forBuildMessage,ResolveMessageandSubprocess.misctools/generate-cli-completions.tsandcompletions/bun-cli.json: nothing in the repository reads the JSON.