Skip to content

napi: polyfill uv_thread_* against pthread #29261

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
robobun wants to merge 6 commits into
base: main
Choose a base branch
from

review: no-op detach, drop negative panic assertion, reorder asserts

f7d0afc
Select commit
Loading
Failed to load commit list.
Open

napi: polyfill uv_thread_* against pthread #29261

review: no-op detach, drop negative panic assertion, reorder asserts
f7d0afc
Select commit
Loading
Failed to load commit list.
Claude / Claude Code Review completed Apr 13, 2026 in 17m 12s

Code review found 1 important issue

Found 4 candidates, confirmed 3. See review comments for details.

Details

Severity Count
🔴 Important 1
🟡 Nit 2
🟣 Pre-existing 0
Severity File:Line Issue
🔴 Important src/bun.js/bindings/uv-posix-polyfills.c:193-198 uv_thread_create_ex aborts on pthread_attr_setstacksize failure
🟡 Nit test/regression/issue/29260.test.ts:44 CLAUDE.md violation: explicit timeout on test body
🟡 Nit src/bun.js/bindings/libuv/generate_uv_posix_stubs_constants.ts:274-286 Stale uv_thread_detach entry in test_skipped after polyfill migration

Annotations

Check failure on line 198 in src/bun.js/bindings/uv-posix-polyfills.c

See this annotation in the file changed.

@claude claude / Claude Code Review

uv_thread_create_ex aborts on pthread_attr_setstacksize failure 

In uv_thread_create_ex, when UV_THREAD_HAS_STACK_SIZE is set and pthread_attr_setstacksize fails (e.g. EINVAL for a stack size below PTHREAD_STACK_MIN or not page-aligned), the polyfill calls abort() instead of returning UV__ERR(err) like the real libuv implementation. Any NAPI module that calls uv_thread_create_ex with a custom stack size that is too small or misaligned will crash the entire process rather than receiving a recoverable UV_EINVAL; fix by calling pthread_attr_destroy(attr) then re

Check warning on line 44 in test/regression/issue/29260.test.ts

See this annotation in the file changed.

@claude claude / Claude Code Review

CLAUDE.md violation: explicit timeout on test body 

The test body at line 44 of test/regression/issue/29260.test.ts passes { timeout: 30_000 } as an option to test(), violating test/CLAUDE.md line 120: "CRITICAL: Do not set a timeout on tests. Bun already has timeouts." The slow work (node-gyp cold build) lives in beforeAll and its 5-minute timeout is appropriate there; the test body itself only spawns a single Bun process that should finish in milliseconds, so the { timeout: 30_000 } argument should be removed.

Check warning on line 286 in src/bun.js/bindings/libuv/generate_uv_posix_stubs_constants.ts

See this annotation in the file changed.

@claude claude / Claude Code Review

Stale uv_thread_detach entry in test_skipped after polyfill migration 

Stale `uv_thread_detach` entry in `test_skipped` was not removed when the symbol was moved to polyfills. The entry is now dead code since `symbols_to_test = symbols.filter(s => \!test_skipped.includes(s))` can never match a symbol that isn't in `symbols`. Remove `"uv_thread_detach"` from `test_skipped` to keep the list consistent and eliminate a latent masking risk if the symbol is ever accidentally re-added to `symbols`.
View more details on Claude