Conversation
ffi-napi's NAPI addon calls uv_thread_self() during module init. Bun
panicked with "unsupported uv function: uv_thread_self" because the
symbol was a stubbed libuv function on POSIX, so require('ffi-napi')
crashed before any FFI call was made.
Implement it as a pthread_self() wrapper, matching upstream libuv
src/unix/thread.c. uv_thread_t is pthread_t on POSIX.
Fixes #29223
|
Updated 8:24 PM PT - Apr 12th, 2026
❌ @robobun, your commit 2b92865 has 2 failures in
🧪 To try this PR locally: bunx bun-pr 29228That installs a local version of the PR into your bun-29228 --bun |
|
Found 2 issues this PR may fix:
🤖 Generated with Claude Code |
|
This PR may be a duplicate of:
🤖 Generated with Claude Code |
|
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:
WalkthroughAdded a POSIX implementation of uv_thread_self, removed its stub and symbol export, updated generator comments to reference Changes
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/regression/issue/29223.test.ts`:
- Around line 100-107: Remove the redundant negative panic-string assertion:
delete the line that asserts expect(stderrStr).not.toContain("unsupported uv
function: uv_thread_self") in the test (leaving the const stdoutStr/stderrStr
declarations if still used) so the test only verifies stdoutStr.trim() === "OK"
and exitCode === 0 (i.e., keep the expect(stdoutStr.trim()).toBe("OK") and
expect(exitCode).toBe(0) assertions).
🪄 Autofix (Beta)
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: edd12ede-6729-4a01-accc-4c306d515b70
📒 Files selected for processing (7)
src/bun.js/bindings/libuv/generate_uv_posix_stubs_constants.tssrc/bun.js/bindings/uv-posix-polyfills.csrc/bun.js/bindings/uv-posix-stubs.ctest/napi/uv-stub-stuff/plugin.ctest/napi/uv-stub-stuff/uv_impl.ctest/napi/uv.test.tstest/regression/issue/29223.test.ts
💤 Files with no reviewable changes (2)
- test/napi/uv-stub-stuff/plugin.c
- src/bun.js/bindings/uv-posix-stubs.c
There was a problem hiding this comment.
LGTM — trivially correct one-liner matching upstream libuv, with good test coverage.
Extended reasoning...
Overview
The PR implements uv_thread_self on POSIX by adding a one-line return pthread_self() wrapper to uv-posix-polyfills.c, removes the panic stub from uv-posix-stubs.c, comments it out in the generator constants list (following the same pattern as uv_hrtime, uv_mutex_*, uv_once, uv_os_getpid, etc.), removes the dead test-plugin branch from plugin.c, and adds a regression test plus a unit test.
Security risks
None. This is a POSIX thread-identity function with no auth, crypto, or permission implications.
Level of scrutiny
Low. The implementation is a single-expression function that matches upstream libuv src/unix/thread.c exactly. uv_thread_t is typedefed to pthread_t on POSIX, so there is nothing to get wrong. The bug-hunter reports are all documentation nits (wrong file extension in a comment, a misleading comment referencing uv_thread_equal instead of pthread_equal, a redundant negative stderr assertion, and an explicit beforeAll timeout) — none affect correctness.
Other factors
The change follows an established pattern in the codebase. Test coverage is solid: two independent test paths (inline NAPI plugin and standalone regression binary) both exercise the new function. The change is self-contained and does not touch security-sensitive code.
- Drop negative panic-string assertion from the regression test; the stdout == 'OK' + exitCode == 0 checks already fully cover the regression and the negative check violated the 'no absence-of-panic' test rule in CLAUDE.md. - Drop the explicit 120_000 timeout on beforeAll to match the sibling test/napi/uv.test.ts pattern. - Fix the .cpp typo in the new constants-file comment (file is .c). - Fix a stale test comment that mentioned uv_thread_equal; the code uses pthread_equal.
There was a problem hiding this comment.
All prior feedback has been addressed. LGTM.
Extended reasoning...
Overview
The PR implements uv_thread_self on POSIX as a one-line pthread_self() wrapper in uv-posix-polyfills.c, removes the crashing stub from uv-posix-stubs.c, and updates the constants file and generated test plugin accordingly. It adds two test paths: a unit test via the existing uv_impl.c NAPI plugin, and a regression test that builds and runs a standalone addon calling uv_thread_self during Init.
Security risks
None. pthread_self() is a trivial thread identity function with no security implications. The only new code in production is a single-line return statement.
Level of scrutiny
Low. The implementation is a mechanical one-liner matching upstream libuv (src/unix/thread.c), and uv_thread_t is already typedef-d to pthread_t on POSIX. The stub removal and symbol list update follow the exact same pattern as the other already-implemented polyfills.
Other factors
My four prior inline comments (wrong .cpp extension in comments, unnecessary 120s beforeAll timeout, misleading uv_thread_equal comment, and the negative panic assertion) were all addressed in commit 7de5514. The one remaining nit — unnecessary async on a synchronous test callback — is already captured as an inline comment and does not affect correctness.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/regression/issue/29223.test.ts`:
- Around line 79-80: The test calls tempDirWithFiles("issue-29223", files) which
already creates and populates the tempdir, so the subsequent await
makeTree(tempdir, files) is redundant and should be removed; locate the test
code using tempDirWithFiles and makeTree (variables tempdir and files, functions
tempDirWithFiles and makeTree) and delete the extra makeTree(...) invocation so
the directory is only created/populated once.
🪄 Autofix (Beta)
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: 65db2615-a4ff-464c-be61-67331d7a1c21
📒 Files selected for processing (1)
test/regression/issue/29223.test.ts
There was a problem hiding this comment.
LGTM — all prior review feedback addressed; the verbose comment block nit is flagged inline.
Extended reasoning...
Overview
This PR adds a real POSIX implementation of uv_thread_self() as a one-line pthread_self() wrapper in src/bun.js/bindings/uv-posix-polyfills.c, removes its stub from uv-posix-stubs.c and the stub-generator constants, and adds tests in both test/napi/uv.test.ts and a new regression file test/regression/issue/29223.test.ts. Seven files are modified.
Security risks
None. The change is a trivial wrapper (return pthread_self()) that matches the upstream libuv implementation. No auth, crypto, or permissions code is touched.
Level of scrutiny
Low. The core implementation is a single-line function that delegates directly to a POSIX primitive. The pattern is established by the existing polyfills (uv_hrtime, uv_mutex_*, uv_once, etc.). Removing the stub from the generator constants follows the exact same pattern as those prior implementations.
Other factors
All issues raised in my previous review passes have been addressed by the author: the no-panic assertion was removed, the beforeAll timeout was dropped, all six .cpp→.c comment corrections were applied, the misleading uv_thread_equal reference in the comment was fixed, and the spurious async keyword was removed. The one remaining item — the 13-line prose comment block at the top of the regression test — is a style nit flagged in the inline comment. The core correctness of the fix is not in question.
|
✅ No merge conflicts detected when merging into Your branch is good to go! |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/regression/issue/29223.test.ts`:
- Around line 46-54: The test fails on macOS because binding.gyp contains the
GNU-specific linker flag "-Wl,--export-dynamic" (seen in the ldflags array)
which Apple ld does not accept; remove that flag from the ldflags entry in
binding.gyp or make its inclusion conditional only for non-Darwin platforms
(e.g., guard the addition when process.platform !== 'darwin') so the
describe.if(!isWindows) test can build successfully on macOS.
🪄 Autofix (Beta)
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: 2513f54f-6d12-44a8-8e55-7cc48000dd55
📒 Files selected for processing (1)
test/regression/issue/29223.test.ts
There was a problem hiding this comment.
LGTM — all feedback addressed.
Extended reasoning...
Overview
The PR adds a one-line POSIX implementation of uv_thread_self (return pthread_self();) in uv-posix-polyfills.c, removes the corresponding panic stub from uv-posix-stubs.c and the stub generator's symbol list, and adds regression + integration tests. Seven files changed, all tightly scoped to the single missing function.
Security risks
None. This is a read-only thread-identity query (pthread_self()) with no allocation, no synchronization, and no capability escalation.
Level of scrutiny
Low — the change is a single-line wrapper that directly matches upstream libuv (src/unix/thread.c). uv_thread_t is typedefed to pthread_t on POSIX (confirmed in uv/unix.h), so the return type is exact.
Other factors
All four rounds of review feedback were addressed in follow-up commits: the .cpp → .c comment typos (all six occurrences), the prohibited negative panic assertion, the extraneous async keyword, the explicit beforeAll timeout, the inaccurate comment citing uv_thread_equal, the redundant makeTree call, and the verbose prose header in the regression test file. No bugs were found by the automated system. The note about a potential duplicate PR (#28900) is a process concern rather than a code-correctness issue.
darwin-14-x64-test-bun failed on CI. Two plausible causes that this commit removes: 1. `ldflags: ['-Wl,--export-dynamic']` in binding.gyp — GNU-only flag. node-gyp's gyp generator ignores the `ldflags` key entirely when flavor == mac (it only reads `xcode_settings.OTHER_LDFLAGS`), so this is a no-op on darwin at best and an 'unknown option' from ld64 at worst depending on toolchain. Tiny NAPI modules don't need `--export-dynamic` anyway (that's for main executables exposing symbols to dlopen'd plugins, the opposite direction). 2. beforeAll default 5s timeout — cold `bun install` + node-gyp configure/build can easily exceed this on CI. Widen to 60s, matching the pattern used by other fixture-building regression tests.
- uv_impl.c test_thread_self: also compare against pthread_self() so a consistent-but-wrong constant would still fail. - 29223.test.ts: drop the 60s beforeAll timeout to match the sibling uv.test.ts; darwin CI failure on the prior commit turned out to be a canceled-build status rather than an actual timeout.
There was a problem hiding this comment.
LGTM — all prior review feedback addressed.
Extended reasoning...
Overview
The PR adds a one-line POSIX implementation of uv_thread_self (returning pthread_self()), removes the stub that previously panicked, updates the generator constants file, and adds both a unit test in test/napi/uv.test.ts and a regression test in test/regression/issue/29223.test.ts.
Security risks
None. This is a trivial wrapper around a standard POSIX syscall with no new attack surface.
Level of scrutiny
Low. The change is a single-line C function matching upstream libuv verbatim. uv_thread_t is typedefed to pthread_t on POSIX (confirmed in uv/unix.h), so return pthread_self() is the correct and complete implementation.
Other factors
Multiple rounds of review caught and addressed: the negative panic-string assertion, the missing pthread_self() comparison in the unit test, the spurious async keyword, the redundant makeTree call, the prose comment block, the .cpp vs .c typos in the generator file, and the beforeAll timeout. The preloaded PR timeline confirms all inline comments are resolved. No bugs were found by the automated bug hunting system.
There was a problem hiding this comment.
LGTM — all prior feedback addressed.
Extended reasoning...
Overview
Adds uv_thread_self to uv-posix-polyfills.c as a one-line pthread_self() wrapper, removes it from the stub generator constants and uv-posix-stubs.c, and adds both a unit test in the existing NAPI test module and a standalone regression test for #29223.
Security risks
None. pthread_self() is a straightforward read of the calling thread's ID with no security implications.
Level of scrutiny
Low. This is a single-line polyfill that exactly matches upstream libuv's src/unix/thread.c implementation. The change is mechanical and self-contained: add the real function, remove the panic stub, update the generator comment.
Other factors
All prior review feedback (comment text accuracy, test hygiene, timeout conventions, unnecessary async, weakened thread-identity assertion) was addressed across four follow-up commits. No bugs were found by the automated bug-hunting pass. The fix directly resolves a crash-on-require for ffi-napi users.
|
Handing off to maintainers. The core fix is in place and all review threads are resolved. CI has been red on
Same lane, same 1-of-9-shards pattern, different third-party test each run. Neither test has any plausible causal connection to a one-line |
|
Superseded by #29261 which implements the full |
What
Implement
uv_thread_selfon POSIX (Linux/macOS) as a one-linepthread_self()wrapper.Why
ffi-napi's prebuilt NAPI addon callsuv_thread_self()during module initialization. Bun used to panic withunsupported uv function: uv_thread_selfbecause the symbol was a stubbed libuv function on POSIX — sorequire('ffi-napi')crashed before any FFI call was made. The issue title says "empty structure" but the underlying cause has nothing to do with FFI structures.Reproducer from #29223
Pre-fix output
How
uv_thread_selftosrc/bun.js/bindings/uv-posix-polyfills.casreturn pthread_self();. Matches upstream libuvsrc/unix/thread.c.uv_thread_tis typedefed topthread_ton POSIX (seesrc/bun.js/bindings/libuv/uv/unix.h).uv_thread_selffromgenerate_uv_posix_stubs_constants.ts'ssymbols[]list so the generator no longer emits a stub for it, alongside the other real polyfills (uv_mutex_*,uv_once,uv_hrtime,uv_os_getpid, …).uv-posix-stubs.cand the matching test-plugin call fromtest/napi/uv-stub-stuff/plugin.c(both are generated from the same constants file).Tests
test/regression/issue/29223.test.ts: builds a minimal NAPI addon that callsuv_thread_self()from itsInitfunction (the exact shape of the ffi-napi crash) and asserts the addon loads cleanly,uv_thread_selfis consistent withpthread_self, and Bun exits 0.test/napi/uv.test.ts: newtestThreadSelfcase in the shareduv_impl.cNAPI plugin. Verifies two successiveuv_thread_self()calls refer to the same pthread viapthread_equal.Gate
Received: "… panic(main thread): unsupported uv function: uv_thread_self …".Fixes #29223