Remove dead code from uws_sys, webcore bindings, crash_handler, and scripts - #37181
Remove dead code from uws_sys, webcore bindings, crash_handler, and scripts#37181robobun wants to merge 12 commits into
Conversation
…ctions, and scripts uws_sys: 23 unused C API wrappers in libuwsockets.cpp, 14 in libuwsockets_h3.cpp, their orphaned Rust extern declarations and safe wrappers (override_write_offset, add_pre_handler, from_named_pipe), and two handler typedefs only the deleted wrappers used. webcore/jsc bindings: JSBufferList (native BufferList superseded by the JS streams implementation; its lazy class structure had zero reachable accessors), ~800 WebKit-inherited DOMConstructorID enum entries bun never wires (also shrinks the per-GlobalObject constructor array from 849 to 51 slots), NAPI_PERISH, OPENSSL_ARRAY_SIZE, hasCustomPtrTraits, and stray forward declarations. crash_handler: the error-return-trace apparatus (unreachable behind const false HAVE_ERROR_RETURN_TRACING since Rust has no error return traces), the no-op --verbose-error-trace debug flag that fed it, the unused StackLine Display impl, and HandleOom impls nothing instantiates. collections: unused Default impl for PriorityQueue. scripts: clippy-loop/ one-shot campaign tooling (campaign completed 2026-05, zero in-repo invokers), orphan LLVM bitcode blob src/base64/neonbase64, commented-out $create*Error helpers in bun/sql.ts. Verified by rg across src/, vendor/, scripts/, and regenerated codegen output; cargo check on all 10 CI target triples; full debug build; and a source lint pinning the removed symbols.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThe change deletes retired Clippy automation, simplifies crash and OOM handling, removes obsolete JSC/WebCore declarations, prunes unsupported uWS interfaces, and adds source-lint checks for deleted symbols and files. ChangesDead code and API cleanup
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Beyond the inline nit, I spot-checked two areas: (1) the trimmed DOMConstructorID enum — grepped all DOMConstructorID:: references across src/jsc/bindings/ and confirmed exactly 51 usages, each mapping to a retained entry, and numberOfDOMConstructors = 51 matches the enum count; (2) show_trace in handle_root_error — still read at the if !show_trace { Global::exit(1) } guard, so removing the later assignments doesn't orphan it.
Extended reasoning...
The reported nit (orphaned m_subspaceForBufferList / m_clientSubspaceForBufferList fields) already covers the one incomplete-cleanup issue found. I additionally verified the two highest-risk removals: the DOMConstructors array-shrink (all 51 call sites accounted for, count constant matches) and the crash_handler show_trace refactor (variable still has a live reader after the mutating assignments were dropped). Given the -2.9k line scope across uws_sys FFI, ZigGlobalObject, and DOMConstructors, deferring to human review; this note is just to record what was already checked.
Deleting the translation unit while its ZigGlobalObject accessors go away in the same change does not survive a partial source-tree restore, where the resurrected file compiles against the trimmed header. Defer the JSBufferList removal and drop its entries from the source lint.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
The HandleOom trait had a single remaining impl; its only caller (js_parser scan_imports) threads Result<T, AllocError> directly.
|
Updated 12:01 AM PT - Aug 8th, 2026
✅ @robobun, your commit 33d3ab446f9d6a63e3262f6149c7caee983aadb5 passed in 🧪 To try this PR locally: bunx bun-pr 37181That installs a local version of the PR into your bun-37181 --bun |
There was a problem hiding this comment.
Additional findings (outside current diff — PR may have been updated during review):
-
🟡
src/crash_handler/handle_oom.rs:26-28— Removing theResult<T, crate::Error>/ bareError/ bareAllocErrorHandleOomimpls orphans doc comments insrc/bun_core/lib.rs:1012("The full multi-arm version (which narrows mixed error sets) lives inbun_crash_handler::handle_oom") and:1029("For the narrowing version seebun_crash_handler::HandleOom") — there is no longer a multi-arm or narrowing version. Since this PR already rewrote the doc inhandle_oom.rsto reflect the reduced surface, the cross-referencing docs inbun_coreshould be updated in the same sweep.Extended reasoning...
What changed vs. what the docs still claim
This PR removes three of the four
HandleOomimpls fromsrc/crash_handler/handle_oom.rs:impl HandleOom for AllocError(bare error →Infallible)impl<T> HandleOom for Result<T, crate::Error>(mixed error set → same union with OOM subtracted — the narrowing arm)impl HandleOom for crate::Error(bare mixed error → same set with OOM subtracted)
Only
impl<T> HandleOom for Result<T, AllocError>survives. The PR correspondingly rewrote the doc comment inhandle_oom.rsitself from a multi-paragraph "OOM-only vs. other errors possible" matrix explanation down to a two-line "Unwraps aResult<T, AllocError>".However,
src/bun_core/lib.rsstill has two doc comments that describe the removed behavior as if it exists inbun_crash_handler:- Line 1012–1016 (on
bun_core::handle_oom): "The full multi-arm version (which narrows mixed error sets) lives inbun_crash_handler::handle_oom; … this tier-0 alias is the OOM-only arm — sufficient for theResult<T, AllocError>/Result<T, Error>callers". After this PR,bun_crash_handler::handle_oomis single-arm and does not narrow mixed error sets, and it no longer acceptsResult<T, Error>at all. - Line 1029 (on
UnwrapOrOom): "For the narrowing version seebun_crash_handler::HandleOom". The narrowing behavior (mixed error set → subset without OOM) was exactly the removedResult<T, crate::Error>impl.
Line 1033–1035 ("Callers that want a strict
error{OutOfMemory}-only whitelist should usebun_crash_handler::HandleOom") is arguably still accurate — the surviving impl is the strict whitelist — so only 1012 and 1029 are unambiguously stale.Step-by-step proof
- Before this PR,
bun_crash_handler::HandleOomhad four impls; the doc block above them described an "input-shape × OOM-only matrix" with per-arm section comments (── .error_union, isOomOnlyError == true → …, etc.). src/bun_core/lib.rs:1012cross-references that structure: "the full multi-arm version … lives inbun_crash_handler::handle_oom", and line 1029 cross-references the narrowing arm specifically.- This PR deletes those three impls and rewrites the local doc in
handle_oom.rsto describe the new single-arm surface — so the PR author explicitly acknowledged the surface changed and updated the local documentation. - The cross-references in
bun_core/lib.rswere not updated, so they now point readers at a "multi-arm" / "narrowing" version that no longer exists.
Why it matters here specifically
REVIEW.md → Code style: "Delete dead code in the same PR that makes it dead." Comments describing removed functionality are stale documentation this PR itself orphaned. This PR is a dead-code sweep and already updated the doc in
handle_oom.rsto reflect the reduced surface — the cross-referencing docs inbun_coreare the same class of leftover as them_subspaceForBufferListfields already flagged on this PR.Impact
None on correctness — purely misleading documentation that will send a future reader looking for a "narrowing" / "multi-arm"
bun_crash_handler::HandleOomthat no longer exists.Fix
In
src/bun_core/lib.rs:- Rewrite the
handle_oomdoc (~lines 1011–1016) to drop the "full multi-arm version … lives inbun_crash_handler" sentence and theResult<T, Error>mention — both crates now expose the same single-arm behavior. - Drop the "For the narrowing version see
bun_crash_handler::HandleOom" sentence from theUnwrapOrOomdoc (~line 1029).
…mnant add_post_handler (both loop types), the Handler struct it returned, the LoopCtxCb alias, and the uws_loop_addPostHandler extern plus C++ wrapper had zero callers; the removal-side C++ was already deleted. In handle_root_error the show_trace local chose between two identical silent exit(1) paths once the trace block was gone. Also drop the stale multi-arm handle_oom cross-references from bun_core docs.
The previous commit unintentionally restored the orphan bitcode blob.
|
Addressed the review follow-ups in dfc555a and 96e6789: the add_post_handler chain (both loop impls, Handler, LoopCtxCb, extern declaration, C++ wrapper) is deleted, the show_trace local in handle_root_error is collapsed, and the stale multi-arm handle_oom cross-references in bun_core docs are rewritten. The source lint now pins the addPostHandler symbols as well. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/jsc/bindings/webcore/DOMConstructors.h`:
- Around line 63-65: Replace the hardcoded numberOfDOMConstructors value with an
enum sentinel representing the end of the DOM constructor entries, then derive
the array size from that sentinel. Update ConstructorArray and getDOMConstructor
usage as needed so the sentinel is not treated as a valid constructor and
indexing remains bounded automatically when enum entries change.
- Around line 13-56: Add a terminal count enumerator to DOMConstructorID and
replace the independent literal 51 used for m_array’s extent with that
enumerator. Update numberOfDOMConstructors or its equivalent sizing declaration
so getDOMConstructor remains covered automatically when new constructor IDs are
added.
In `@test/internal/source-lints/dead-symbols-uws-webcore-crash-scripts.test.ts`:
- Around line 43-50: Update existsInHead to use git ls-tree instead of git
cat-file -e, treating empty stdout as a missing path and returning true when the
path is present. Check the command’s exitCode separately and throw on any
nonzero result so Git failures are never converted into a deleted-file result.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: a6ab037d-bd4d-4a7c-bbbc-434daa986354
📒 Files selected for processing (38)
scripts/clippy-loop/apply-patches.tsscripts/clippy-loop/collect-all-targets.shscripts/clippy-loop/collect-pr-comments.shscripts/clippy-loop/collect.shscripts/clippy-loop/de-unsafe.workflow.tsscripts/clippy-loop/edit-round.workflow.tsscripts/clippy-loop/fix-round.workflow.tsscripts/clippy-loop/group-by-file.tsscripts/clippy-loop/harvest.tsscripts/clippy-loop/review-round.workflow.tsscripts/clippy-loop/shard.tsscripts/clippy-loop/split-diags.tssrc/base64/neonbase64src/bun_core/lib.rssrc/collections/lib.rssrc/crash_handler/handle_oom.rssrc/crash_handler/lib.rssrc/js/bun/sql.tssrc/jsc/bindings/JSDOMWrapper.hsrc/jsc/bindings/dh-primes.hsrc/jsc/bindings/napi.hsrc/jsc/bindings/node/crypto/KeyObject.hsrc/jsc/bindings/webcore/DOMConstructors.hsrc/jsc/bindings/webcore/JSPerformance.hsrc/jsc/bindings/webcore/SerializedScriptValue.hsrc/runtime/api/crash_handler_jsc.rssrc/runtime/cli/Arguments.rssrc/runtime/cli/mod.rssrc/uws_sys/Loop.rssrc/uws_sys/Response.rssrc/uws_sys/WebSocket.rssrc/uws_sys/_libusockets.hsrc/uws_sys/h3.rssrc/uws_sys/lib.rssrc/uws_sys/libuwsockets.cppsrc/uws_sys/libuwsockets_h3.cppsrc/uws_sys/socket.rstest/internal/source-lints/dead-symbols-uws-webcore-crash-scripts.test.ts
💤 Files with no reviewable changes (30)
- src/jsc/bindings/node/crypto/KeyObject.h
- src/jsc/bindings/dh-primes.h
- scripts/clippy-loop/collect-pr-comments.sh
- scripts/clippy-loop/de-unsafe.workflow.ts
- scripts/clippy-loop/split-diags.ts
- src/uws_sys/_libusockets.h
- scripts/clippy-loop/review-round.workflow.ts
- scripts/clippy-loop/edit-round.workflow.ts
- src/jsc/bindings/webcore/JSPerformance.h
- scripts/clippy-loop/apply-patches.ts
- src/jsc/bindings/JSDOMWrapper.h
- scripts/clippy-loop/shard.ts
- scripts/clippy-loop/group-by-file.ts
- src/uws_sys/h3.rs
- scripts/clippy-loop/harvest.ts
- src/jsc/bindings/napi.h
- scripts/clippy-loop/collect-all-targets.sh
- scripts/clippy-loop/fix-round.workflow.ts
- src/js/bun/sql.ts
- src/runtime/cli/Arguments.rs
- scripts/clippy-loop/collect.sh
- src/uws_sys/lib.rs
- src/uws_sys/Response.rs
- src/collections/lib.rs
- src/uws_sys/Loop.rs
- src/jsc/bindings/webcore/SerializedScriptValue.h
- src/uws_sys/WebSocket.rs
- src/uws_sys/socket.rs
- src/uws_sys/libuwsockets_h3.cpp
- src/uws_sys/libuwsockets.cpp
…'s file check numberOfDOMConstructors now derives from DOMConstructorID::Count so a new enum entry cannot silently index past ConstructorArray. existsInHead uses git ls-tree and throws on git failure instead of reading any nonzero exit as a deleted file.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/internal/source-lints/dead-symbols-uws-webcore-crash-scripts.test.ts`:
- Around line 49-50: Update the error constructed in the git ls-tree failure
branch to include repoRoot, r.exitCode, and a non-empty fallback when r.stderr
is empty, while preserving the existing stderr details when available. Keep the
failure associated with the path p and provide a clear remediation-oriented
diagnostic.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 7b26d373-ca1c-4d59-ba3b-438bdd27a3dc
📒 Files selected for processing (2)
src/jsc/bindings/webcore/DOMConstructors.htest/internal/source-lints/dead-symbols-uws-webcore-crash-scripts.test.ts
Same class as the OPENSSL_ARRAY_SIZE removal: both macros supported prime tables that no longer exist in this header, and the sole includer (ncrypto.cpp) references neither.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/internal/source-lints/dead-symbols-uws-webcore-crash-scripts.test.ts`:
- Line 124: Update the regex in the dead-symbol test entry for
“src/jsc/bindings/dh-primes.h” to apply word boundaries to both deleted symbols,
using a grouped alternation for OPENSSL_ARRAY_SIZE and TOBN so longer
identifiers such as OPENSSL_ARRAY_SIZE_OLD do not match.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: aa5d7fa8-47e7-44d3-9ab4-5cec83cdf8b2
📒 Files selected for processing (2)
src/jsc/bindings/dh-primes.htest/internal/source-lints/dead-symbols-uws-webcore-crash-scripts.test.ts
💤 Files with no reviewable changes (1)
- src/jsc/bindings/dh-primes.h
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. Because it's a large sweep (−2.4k lines across 37 files) that touches the per-GlobalObject DOM constructor array sizing, the uws HTTP/WebSocket C-ABI surface, and the top-level handle_root_error control flow, a human look would still be worthwhile.
What was reviewed:
- DOMConstructors.h trim: retained IDs cover every
DOMConstructorID::reference;Countsentinel now sizes the array. handle_root_errorcollapse: theInvalidArgument|Invalid Bunfig|InstallFailedarm still reachesGlobal::exit(1)on both SHOW_CRASH_TRACE settings with no new output.- uws wrapper deletions: each removed
uws_*symbol has no remaining Rust extern or in-tree caller;*_with_optionslive paths kept. HandleOomtrait → plain fn: no remainingResult<T, crate::Error>/ bare-Errorcallers;rust:check-allreportedly green.
Extended reasoning...
Overview
Dead-code sweep removing ~2,400 lines across four subsystems: 24 unused uws_* C-ABI wrappers plus their Rust extern declarations and safe-fn wrappers (Loop/Response/WebSocket/h3/socket); the DOMConstructorID enum trimmed from 849 to 51 entries with the WriteBarrier array now sized by a Count sentinel; the crash-handler error-return-trace apparatus (const-false-gated in Rust) including the --verbose-error-trace debug flag, the show_trace local, the StackLine Display impl, and the HandleOom trait collapsed to a plain fn handle_oom(Result<T, AllocError>); assorted C++ header cruft (NAPI_PERISH, TOBN, OPENSSL_ARRAY_SIZE, hasCustomPtrTraits, dead forward decls); the retired scripts/clippy-loop/ tooling; the orphan src/base64/neonbase64 bitcode blob; commented-out $create*Error helpers in bun/sql.ts; and a new source-lint test pinning the removed symbols.
Security risks
None identified. Pure deletions of unreferenced code plus doc/comment trims. No auth, crypto, permission, or input-validation logic is weakened; the only crypto-adjacent edit (dh-primes.h) removes unused macros. The handle_root_error change preserves exit codes and message output on every arm.
Level of scrutiny
High. Despite being deletions, the change reshapes a per-GlobalObject GC-visited array (DOMConstructors), removes 38 C-ABI symbols from the HTTP/WebSocket server FFI surface, changes a public crash-handler signature, and drops a debug-build CLI flag. Each of these is production-critical native code where a missed reference (especially from regenerated codegen or vendor/bun-uws internals) would be a compile break at best and an OOB array read at worst. The PR description's verification (grep across src/vendor/test/codegen, rust:check-all on 10 triples, full bun bd, targeted smoke tests) is thorough, but the scope is well past what should merge on automated review alone.
Other factors
All earlier review threads (mine and CodeRabbit's) are resolved by follow-up commits: the add_post_handler chain, show_trace remnant, DOMConstructors sentinel, TOBN macro, existsInHead git-failure handling, and rustfmt whitespace were all fixed. The bug-hunting system found nothing new this run. The added source-lint test follows the existing test/internal/source-lints/ pattern and reads HEAD to sidestep the git-stash stray-file issue. Given the breadth and the number of follow-up fixes already applied during review, a maintainer sign-off on the final state is appropriate.
|
The single red in build 90524 is test/bake/deinitialization.test.ts on Windows 2019 x64 (the fixture dev server crashes with exit code 3 during teardown). Reproduced it on a Windows machine at about a 20 percent rate on this branch (3/15) and at the same rate on main f972c28 (5/25) with the identical crash, so it is a pre-existing teardown race unrelated to this diff; it has been reported separately. Everything else in the build passed or was retry-flake. |
88523d0 to
b9d7aa9
Compare
Same class as the handle_root_error parameter this branch already removed: both callers pass None, so the TraceSeed::ErrorReturn variant and its match arms were unreachable. The enum loses its now-unused lifetime parameter.
The label existed for the removed TraceSeed::ErrorReturn early exit; the block now ends in its tail expression.
There was a problem hiding this comment.
I reviewed this PR and found no bugs; all earlier inline findings have been addressed. Given the breadth (net -2.4k lines across uws_sys FFI, the crash handler, and the DOMConstructors array sizing), a human sign-off is still worthwhile.
What was reviewed:
- DOMConstructors.h trim: the 51 surviving
DOMConstructorIDentries match everygetDOMConstructor<…>site insrc/jsc/; the array is now sized from theCountsentinel and nothing casts the enum numerically. handle_root_errorcontrol-flow: the removedshow_tracebranches andhandle_error_return_trace_extracall sat behindHAVE_ERROR_RETURN_TRACING = false, so the earlyGlobal::exit(1)for InvalidArgument/Bunfig/InstallFailed collapsing to the tail exit is behavior-preserving.bun_crash_handler::handle_oomtrait → plain fn: only importer isjs_parser/scan/scan_imports.rswithResult<T, AllocError>; the removedError/bare-AllocErrorimpls had no instantiations (cargo check on all 10 triples arbitrates).- uws_sys: spot-checked that the deleted
Handlerstruct /add_*_handler/from_named_pipe/ C wrappers have no remaining references, and the_with_optionslive paths stay intact.
Extended reasoning...
Overview
Dead-code sweep removing ~2,380 lines across 38 files: 24 unused C wrappers in libuwsockets.cpp and 14 in libuwsockets_h3.cpp plus their orphaned Rust extern declarations and safe wrappers (override_write_offset, add_pre/post_handler, Handler struct, from_named_pipe, us_fault_clear, LIBUS_LISTEN_DISALLOW_REUSE_PORT_FAILURE); the DOMConstructorID enum trimmed from 849 WebKit-inherited entries to the 51 bun references (also shrinking the per-GlobalObject WriteBarrier array); assorted dead C++ macros/forward decls (NAPI_PERISH, TOBN, OPENSSL_ARRAY_SIZE, hasCustomPtrTraits, MemoryHandle/FragmentedSharedBuffer); the crash handler's error-return-trace apparatus (const-false-gated in Rust) plus the always-None error_return_trace params on handle_root_error/panic_impl and the TraceSeed::ErrorReturn variant; the HandleOom trait collapsed to a plain Result<T, AllocError> fn; PriorityQueue::Default; the --verbose-error-trace debug flag; the scripts/clippy-loop/ one-shot tooling; the orphan neonbase64 bitcode blob; commented-out $create*Error helpers in sql.ts. A new source-lint test pins each removed symbol against reintroduction.
Security risks
None identified. No auth/crypto/permission logic changes — the touched crypto files (dh-primes.h, KeyObject.h) only lose unused macro scaffolding and a commented-out declaration. No input-handling paths are relaxed.
Level of scrutiny
High. This is not a mechanical config tweak: it rewrites handle_root_error control flow, changes a public fn signature (panic_impl, handle_root_error), drops a CLI flag, shrinks a per-GlobalObject GC-visited array's static size, and deletes ~300 lines of C++ FFI glue that backs Bun.serve's HTTP/WS paths. Each removal is individually verifiable as dead, and the Rust side is arbitrated by cargo check on all 10 triples plus a full debug build — but the aggregate surface across production-critical subsystems (HTTP server FFI, crash reporting, JSC DOM constructor caching) is large enough that a maintainer should confirm no removed symbol is load-bearing via a path grep can't see (e.g., in-flight branches, external tooling that parses --verbose-error-trace).
Other factors
The PR has been through four review iterations; every prior inline finding (mine and CodeRabbit's) is resolved, including the accidental restore commit (88523d0) that was force-dropped, the same-class siblings TOBN/add_post_handler/panic_impl's param, and the vestigial 'blk label. CI build 90524 was green except for a Windows bake/deinitialization.test.ts teardown flake reproduced at the same rate on main. The handle_root_error behavioral equivalence and DOMConstructors coverage were spot-checked directly against the tree in this review pass. Deferring for a human sign-off on scope, not on any identified defect.
2 participants
Dead code sweep: net -2,382 lines across 38 files. Every removal was verified to have zero references across src/, vendor/, scripts/, test/, and regenerated build/debug/codegen output before deletion.
uws_sys (-470)
C++ bindings (-860)
crash_handler (-270)
Misc Rust (-20)
Scripts and assets (-1,170)
Verification
Noted but deliberately not removed (for a future pass)
[review] gate passed · iteration 4 · 38 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 4 passed · 2 rejected · iteration 4
evidence per changed file