Skip to content

libuv_sys: restore uv_translate_sys_error and uv_os_getppid (fixes the Windows build on main) - #38735

Closed
robobun wants to merge 1 commit into
mainfrom
farm/4e6aa17d/restore-libuv-sys-externs
Closed

robobun wants to merge 1 commit into
mainfrom
farm/4e6aa17d/restore-libuv-sys-externs

Conversation

@robobun

@robobun robobun commented Aug 14, 2026

Copy link
Copy Markdown
Collaborator

Problem

Fix

  • src/libuv_sys/libuv.rs: restore the two extern "C" declarations, byte for byte as they were before Remove dead code from libuv_sys, cares_sys, simdutf FFI, test_runner, and C++ bindings #37332 (uv_translate_sys_error(c_int) -> c_int, uv_os_getppid() -> uv_pid_t). Both match uv.h; the call sites still wrap them in unsafe, so the plain (non-safe) declarations are the ones that compile without warnings.
  • test/internal/source-lints/libuv-sys-exports.test.ts: a source lint that collects every crate-root item bun_libuv_sys exports (column-0 pub items, extern "C" block contents, pub use re-exports, lib.rs constants) and resolves every name the rest of src/ reaches through the crate against it: bun_libuv_sys::x, bun_sys::windows::libuv::x, items pulled in with use, and paths through a local use ... as uv alias. It runs on any host, so the next removal of a declaration that Windows-only code still uses is reported without a Windows target installed (on this tree it resolves 655 references to 161 distinct items across 63 files). bun run rust:check-all stays the complete check; this is the part of it that runs in under a second on a released bun.
  • Verified:
    • cargo check -p bun_runtime --target x86_64-pc-windows-msvc without the declarations reproduces exactly the three E0425 errors above; with them it passes. cargo check --workspace passes for both x86_64-pc-windows-msvc and aarch64-pc-windows-msvc with zero warnings.
    • The lint without the declarations fails naming exactly the three call sites rustc names (ipc_host.rs:374: uv_os_getppid, node_cluster_binding.rs:391: uv_translate_sys_error, node_util_binding.rs:75: uv_translate_sys_error) and passes with them (bun bd test test/internal/source-lints/libuv-sys-exports.test.ts).
    • test/internal/source-lints/dead-symbols-ffi-sys-test-runner-cpp.test.ts (the pin list added by Remove dead code from libuv_sys, cares_sys, simdutf FFI, test_runner, and C++ bindings #37332) still passes; it does not list either symbol.
    • cargo fmt --check -p bun_libuv_sys, prettier, and tsc on the new test file are clean.

The other red test in build 96732, test/js/node/test/parallel/test-http-chunk-problem.js on Linux aarch64 (also red on main build 96727), is a separate break and is not addressed here.

Background

  • bun_libuv_sys (src/libuv_sys) is Bun's hand-written Rust FFI surface for libuv, which Bun only uses on Windows. lib.rs is compiled everywhere, but the whole binding module libuv.rs is #![cfg(windows)], and lib.rs re-exports it with pub use libuv::*, so callers write bun_libuv_sys::uv_fs_open. bun_sys::windows re-exports the crate as libuv, which is why most callers spell it bun_sys::windows::libuv or use bun_sys::windows::libuv as uv;.
  • uv_translate_sys_error converts a Win32/WinSock error code into libuv's negative UV_E* code. node:cluster uses it on Windows in two places: the shared-handle bind binding (cluster_raw_bind) and the round-robin handle (RoundRobinHandle.ts, through the uvTranslateSysError binding), so a failed bind reaches the worker as the same UV_E* code Node's cluster protocol carries. uv_os_getppid is libuv's portable parent-pid lookup; process.send() on Windows falls back to it for the pid that WSADuplicateSocketW needs when a socket handle is sent over IPC and the channel has not reported the peer pid.
  • A cfg(...) attribute removes code from compilation entirely on targets where it does not apply, so rustc on a non-Windows host never resolves paths inside cfg(windows) code. That is what lets a declaration and its only callers disagree without any error on the host that verified the change.
  • test/internal/source-lints/ holds tests that only read the source tree (see its README); they run against a released bun in seconds and do not need the build under test.

… crate's exports against their callers

The dead-code sweep in #37332 removed these two extern declarations from
bun_libuv_sys, but #31829 (merged in between) had started calling them from
cfg(windows) code in node_util_binding.rs, node_cluster_binding.rs and
ipc_host.rs, so the Windows build of bun_runtime fails with E0425 on main.

Both sides of this crate are cfg(windows), so a non-Windows cargo check
cannot see the mismatch. The new source lint resolves every crate-root name
the rest of src/ reaches through bun_libuv_sys (directly, via
bun_sys::windows::libuv, or via a local alias) against the items the crate
exports, and reports the three call sites on any host.
@coderabbitai

coderabbitai Bot commented Aug 14, 2026

Copy link
Copy Markdown
Contributor

Warning

Review limit reached

@robobun, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 29 seconds

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

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 configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: fcc885af-3527-4128-a31f-b55b207ecace

📥 Commits

Reviewing files that changed from the base of the PR and between e7460e3 and 79439a8.

📒 Files selected for processing (2)
  • src/libuv_sys/libuv.rs
  • test/internal/source-lints/libuv-sys-exports.test.ts

Comment @coderabbitai help to get the list of available commands.

@robobun

robobun commented Aug 14, 2026 •

Copy link
Copy Markdown
Collaborator Author

@robobun

robobun commented Aug 14, 2026 •

Copy link
Copy Markdown
Collaborator Author

Status: closed as a duplicate of #38733, which was opened a minute earlier with the same fix (the identical 4-line restore of uv_translate_sys_error / uv_os_getppid in src/libuv_sys/libuv.rs and an equivalent source lint). Follow that PR for the Windows build fix on main.

For the record: main's Windows build-bun lanes have failed since #37332 landed on top of #31829 (error[E0425]: cannot find function `uv_translate_sys_error` in crate `bun_libuv_sys` at node_cluster_binding.rs:391 and node_util_binding.rs:75, uv_os_getppid at ipc_host.rs:374); test/regression/issue/29169.test.ts and the other Windows test lanes in build 96732 are collateral of that. Reproduced with cargo check -p bun_runtime --target x86_64-pc-windows-msvc; with the declarations restored, cargo check --workspace passes for both Windows triples and the :windows: build-bun lanes passed in CI here (build 96811) and on #38733.

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM — restores two extern "C" declarations that #37332 removed while cfg(windows) callers still use them, plus a source lint to catch the next instance.

What was reviewed:

  • Signatures match uv.h (int uv_translate_sys_error(int), uv_pid_t uv_os_getppid(void)); uv_pid_t is already defined at libuv.rs:91.
  • Confirmed the three call sites exist as described (node_cluster_binding.rs:391, node_util_binding.rs:75, ipc_host.rs:374) and wrap the calls in unsafe.
  • The lint follows the established test/internal/source-lints/ pattern, guards against vacuous passes (resolved > 0, anchor names), and flags any new pub use ... ::* glob it doesn't follow.
Extended reasoning...

Overview

Two changes: (1) restore uv_translate_sys_error and uv_os_getppid in src/libuv_sys/libuv.rs's extern "C" block — 4 added lines total; (2) add test/internal/source-lints/libuv-sys-exports.test.ts, a ~190-line regex-based lint that resolves every bun_libuv_sys::<item> reference in src/**/*.rs against the crate's actual exports so a future removal of a still-used declaration fails on any host without a Windows toolchain.

Security risks

None. The Rust change only re-adds FFI declarations for functions libuv already exports (and which are already listed in symbols.def/symbols.dyn/linker scripts). The test only reads source files and runs git ls-tree; it introduces no runtime behavior.

Level of scrutiny

Low for the core fix — it's a mechanical restore of declarations deleted by an over-eager dead-code sweep, and main's Windows build is currently broken without it. I verified the signatures against src/jsc/bindings/libuv/uv.h:421,1289 and confirmed uv_pid_t is still defined (libuv.rs:91). The three call sites the PR names all exist and call these via unsafe { bun_libuv_sys::... }, so plain (non-safe) declarations are correct.

Medium for the lint — it's heuristic regex over rustfmt-formatted source, but it lives beside ~24 sibling lints of the same shape in test/internal/source-lints/ and follows their conventions (module-scope scan, comment-stripping, tracked-file gating via git ls-tree). It defends against silent decay: an anchor test asserts each declaration shape (struct, extern fn, trait, module, constant, re-export) is actually parsed, unfollowedGlobs fails loudly if a new pub use ... ::* appears, and resolved > 0 prevents an empty scan from passing. The PR description confirms it fails on the pre-fix tree naming exactly the three sites rustc names, and passes after — satisfying the "prove the test fails for the right reason" requirement.

Other factors

No prior reviews or outstanding comments. The PR is a build-fix for main (Windows lanes are red on build 96727), so urgency favors landing. test/internal/source-lints/dead-symbols-ffi-sys-test-runner-cpp.test.ts (the pin list from #37332) does not list either symbol, so no conflict there.

@github-actions

Copy link
Copy Markdown
Contributor

This PR may be a duplicate of:

  1. libuv_sys: restore uv_translate_sys_error / uv_os_getppid (Windows build of main) and lint crate references #38733 - Restores the same two extern "C" declarations (uv_translate_sys_error, uv_os_getppid) in src/libuv_sys/libuv.rs with a byte-identical diff, and adds an equivalent test/internal/source-lints/ lint for bun_libuv_sys references.

🤖 Generated with Claude Code

@robobun

robobun commented Aug 14, 2026

Copy link
Copy Markdown
Collaborator Author

Duplicate of #38733, which was opened first with the same change. Closing this one in its favor.

@robobun robobun closed this Aug 14, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant