Conversation
…te references The dead-code sweep in #37332 removed both extern declarations from bun_libuv_sys. #31829 merged in between and calls them from three cfg(windows) blocks (node_cluster_binding.rs, node_util_binding.rs, ipc_host.rs), so bun_runtime no longer compiles on Windows; the posix lanes never type-check those blocks. Restore the two declarations and add a source lint that checks every name referenced through bun_libuv_sys from src/ against the items the crate defines, so the next removal of a declaration with a Windows-only caller fails on every host in seconds.
|
Warning Review limit reached
Next review available in: 1 minute Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
Comment |
|
Updated 8:05 PM PT - Aug 14th, 2026
❌ @robobun, your commit c168b36 has 1 failures in
🧪 To try this PR locally: bunx bun-pr 38733That installs a local version of the PR into your bun-38733 --bun |
|
Status: closed, superseded by c04101b on main, which restores the same two declarations. See the closing comment below for why this branch is not rebased instead (a clean merge would declare |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — straightforward Windows build fix restoring two FFI declarations, plus a source-lint to catch the same class of break earlier.
What was reviewed
- Restored signatures match
uv.h(int uv_translate_sys_error(int),uv_pid_t uv_os_getppid(void)withuv_pid_t = c_inton Windows) and the three#[cfg(windows)]callers innode_cluster_binding.rs,node_util_binding.rs, andipc_host.rs. - Confirmed these are exactly the lines #37332 deleted; placement in the
extern "C"block matches neighboring declarations. - New lint follows the
test/internal/source-lints/conventions (README criteria: no spawn, no built binary; same shape asprimordials-exports.test.ts), sorts glob output for determinism, normalizes\\→/, and guards against vacuous passes withreferenced > 200. - Checked the alias-gating (
importsCrate) so files with an unrelateduv::(e.g.bun_core's ownwindows_sys::libuv) are not falsely flagged;crateNames()is intentionally a superset so the lint only reports definite misses.
Extended reasoning...
Overview
Two-part change: (1) restore uv_translate_sys_error and uv_os_getppid extern "C" declarations in src/libuv_sys/libuv.rs that #37332 removed as dead but which #31829 (merged in between) started calling from three #[cfg(windows)] sites, breaking the Windows build of main; (2) add test/internal/source-lints/libuv-sys-references.test.ts, a textual lint that any name referenced through bun_libuv_sys / bun_sys::windows::libuv / the uv:: and libuv:: aliases is defined in src/libuv_sys/, so this class of cfg-gated dangling reference surfaces on any host in seconds.
Security risks
None. The FFI restore re-adds two pre-existing libuv function declarations with signatures verified against src/jsc/bindings/libuv/uv.h and uv/win.h (uv_pid_t is int on Windows, matching pub type uv_pid_t = c_int). No new unsafe code paths, no user-input handling. The test only reads source files.
Level of scrutiny
Low-to-medium. The libuv.rs change is a mechanical revert of two lines to unbreak main on Windows — the compiler is the authority and the PR body shows both Windows targets pass cargo check. The new lint is test-only infrastructure in an established directory with ~20 sibling lints of the same shape; it is deliberately over-approximate on the definition side (impl methods and nested-module items count) so it can only fire on definite misses, and the alias regexes are gated per-file on a real use of the crate to avoid false positives from unrelated uv:: / libuv:: bindings.
Other factors
- Verified the three call sites exist at the cited lines and use the restored functions with matching types (
c_intin/out for the translator,uv_pid_tcast tou32for getppid). - The lint mirrors
primordials-exports.test.tsin structure (glob + sort, path-separator normalization, violations array, vacuous-pass guard) and satisfies the source-lints README criteria (nobunExe(), nobun:internal-for-testing, noBun.build). - The comment-handling choice (per-match
//check instead of stripping) is justified in-file with a concrete counterexample from the tree; theinLineCommenthelper correctly handles the start-of-file case vialastIndexOfreturning -1. - PR body documents that the test fails on unfixed main with exactly the three rustc-reported sites and passes with the restore, satisfying the fails-for-the-right-reason requirement.
|
Same fix landed in parallel in #38735 (branch |
|
Closing: main already has this fix. c04101b ("Fix merge issue") restored both declarations in This branch should not be merged on top of that: The lint in |
1 participant
Problem
cargo check --workspace --target x86_64-pc-windows-msvcon main (2d3b1ee):uv_translate_sys_erroranduv_os_getppiddeclarations fromsrc/libuv_sys/libuv.rs; at the time nothing called them.#[cfg(windows)]blocks.cfg(windows)block, so only the Windows build noticed.Fix
src/libuv_sys/libuv.rs: restore the twoextern "C"declarations. Signatures matchsrc/jsc/bindings/libuv/uv.h(int uv_translate_sys_error(int),uv_pid_t uv_os_getppid(void),uv_pid_tisinton Windows) and the three call sites; they are the lines Remove dead code from libuv_sys, cares_sys, simdutf FFI, test_runner, and C++ bindings #37332 removed.test/internal/source-lints/libuv-sys-references.test.ts: a source lint that collects every name referenced throughbun_libuv_sysfromsrc/(bun_libuv_sys::x,bun_sys::windows::libuv::x, theuv::/libuv::aliases in files that import the crate, anduse ...::{...}lists) and requires each to be an item the crate defines or re-exports. On main it fails with exactly the three file:line pairs rustc reports above; with the restore it passes. It runs on any host in the source-lints workflow (which also runs onmerge_group), so the same class of break is reported in seconds instead of after the Windows build. It does not replacerust:check-all: only crate-root names are checked, not signatures.cargo check --workspace --target x86_64-pc-windows-msvcand--target aarch64-pc-windows-msvcboth exit 0 with this change (the x64 one reproduces the three errors without it);bun bd test test/internal/source-lints/libuv-sys-references.test.tspasses (~1.7s debug, ~0.1s release) and fails on main;bun test test/internal/source-lints/all green;cargo fmt --checkclean.Background
bun_libuv_sys(src/libuv_sys/) is bun's hand-written Rust FFI surface for libuv, and it is compiled only on Windows (#![cfg(windows)]at the top oflibuv.rs). Other crates reach it either directly, throughbun_sys::windows::libuv(apub use bun_libuv_sys as libuv;re-export), or via the conventionaluse ... as uv;alias.#[cfg(windows)]as well. rustc drops cfg'd-out code before name resolution, so a Linux or macOScargo check/bun bdcannot tell whether a path into this crate still resolves; CLAUDE.md asks forbun run rust:check-all(acargo checkper CI target) when touching platform-gated code for this reason. That check was run for Remove dead code from libuv_sys, cares_sys, simdutf FFI, test_runner, and C++ bindings #37332, but against the main of the time, before cluster: port Node's cluster and child_process handle-passing suites (+43 upstream tests; cluster 54 → 85) and implement what they expose — round-robin fd handoff, SCHED_NONE shared handles, UDP clustering, IPC handle passing #31829 added the callers.test/internal/source-lints/holds tests that only read the source tree. They run in a GitHub Actions workflow against a released bun on everysrc/**/*.rschange and on merge groups, and are excluded from the Buildkite shards, which is what makes a textual check useful here even though rustc on Windows is the authority.cargo check on the Windows targets, before and after
Before (main, x86_64-pc-windows-msvc,
--keep-going):After (this branch):
Lint on main (
git stash push -- src/ && bun bd test test/internal/source-lints/libuv-sys-references.test.ts):The lint currently resolves 645 references against 559 crate names. Comments are skipped per match (a hit preceded by
//on its line) rather than by stripping them first:src/runtime/api/bun/subprocess.rs:205hasbuild/*/codegeninside a//comment, and a stripping pass treats that/*as opening a block comment that hides real code down to line 1529, including a real libuv reference at line 1266.