wasi: number WASI_SIG* constants in wasi_snapshot_preview1 order - #39392
wasi: number WASI_SIG* constants in wasi_snapshot_preview1 order#39392Jarred-Sumner wants to merge 2 commits into
Conversation
proc_raise(sig) looked the guest signal up in a table numbered alphabetically, so e.g. SIGTERM (15) was delivered to the host as SIGTRAP. Use the preview1 enum order (SIGHUP=1 .. SIGSYS=30) and add the five signals that were missing.
|
Updated 9:32 PM PT - Aug 16th, 2026
✅ @Jarred-Sumner, your commit 7b351964b94469fc905d9eb0d3c7ecde8d00a0b1 passed in 🧪 To try this PR locally: bunx bun-pr 39392That installs a local version of the PR into your bun-39392 --bun |
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 25 minutes Limit details: You’ve used all 1 included review currently available under your plan. You completed 71 included PR reviews in the past 7 days; at that activity level, included reviews refill at 1 review per hour. 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 |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — the preview1 signal renumbering is correct against the spec and SIGNAL_MAP/proc_raise are the only consumers. The two inline notes are nits (a vacuous backstop assertion in the wasm fixture and an untested/broad catch on the ENOTSUP path); neither affects the fix itself.
Extended reasoning...
Overview
This PR renumbers the WASI_SIG* constants in src/js/node/wasi.ts from the old alphabetical (wasi_unstable) order to the wasi_snapshot_preview1 signal enum order (SIGHUP=1 … SIGSYS=30), adds the five previously-missing entries to SIGNAL_MAP (SIGPROF/SIGWINCH/SIGPOLL/SIGPWR/SIGSYS), and wraps bindings.kill in a try/catch that returns WASI_ENOTSUP when the host lacks a signal. Two tests are added: a mocked-kill table check for all 30 signals plus out-of-range → EINVAL, and an end-to-end bun raise.wasm subprocess that asserts proc_raise(15) delivers SIGTERM.
Security risks
None. This is a constant-table correction in the WASI polyfill; proc_raise already targeted process.pid and the change only fixes which signal name is passed. No new capability is exposed.
Level of scrutiny
Low-to-medium. The renumbering is mechanical and spec-verifiable — I checked it against the preview1 signal variant order and it matches exactly. Grep confirms WASI_SIG* constants are referenced nowhere outside wasi.ts, and SIGNAL_MAP is consumed only by proc_raise, so there are no other call sites to break. The end-to-end test's primary assertion (signalCode === "SIGTERM") is load-bearing and would fail on the pre-fix build where 15 mapped to SIGTRAP.
Other factors
Two nit-level findings will be posted inline: (1) the hand-assembled wasm fixture encodes i32.const 77 as a single byte, which signed-LEB128 decodes as −51, so the .not.toBe(77) backstop is vacuous — but the SIGTERM assertion above it still catches the regression; (2) the bare catch {} maps every error to ENOTSUP rather than only ERR_UNKNOWN_SIGNAL, and neither test exercises that branch. Both are worth tidying but don't affect correctness of the fix, so approving.
…128 sentinel in fixture
There was a problem hiding this comment.
LGTM — both earlier nits are addressed: the catch is now narrowed to ERR_UNKNOWN_SIGNAL with rethrow (and covered by the new "reports ENOTSUP … rethrows anything else" test), and the wasm fixture's sentinel is now 42 so the .not.toBe(42) backstop is reachable.
Also checked: the WASI_SIG* constants are referenced nowhere outside SIGNAL_MAP, so the renumbering has no other consumers to break; the new preview1 order matches the cited spec and wasi-libc's __WASI_SIGNAL_*; proc_raise(0)/proc_raise(31) → EINVAL bounds are asserted.
Extended reasoning...
Overview
Renumbers the WASI_SIG* constants in src/js/node/wasi.ts from the (buggy) alphabetical 0-24 order to the wasi_snapshot_preview1 signal enum order (1-30), adds the five previously-missing entries to SIGNAL_MAP, and makes proc_raise return WASI_ENOTSUP when the host's process.kill throws ERR_UNKNOWN_SIGNAL (rethrowing anything else). Three new tests in test/js/bun/wasm/wasi.test.js: a mocked-kill enumeration of all 30 signals + out-of-range → EINVAL, an ENOTSUP/rethrow test, and an end-to-end bun raise.wasm subprocess check that guest proc_raise(15) delivers host SIGTERM.
Prior review follow-up
My previous run left two 🟡 nits. The follow-up commit (7b351964) addresses both:
- Over-broad
catch {}→ nowcatch (err) { if (err?.code === "ERR_UNKNOWN_SIGNAL") return WASI_ENOTSUP; throw err; }, and a dedicated test asserts both the ENOTSUP mapping (mock throws withcode: "ERR_UNKNOWN_SIGNAL"→proc_raise(28) === 58) and the rethrow path (mock throws a plainError→.toThrow("kill binding bug")). Deleting either clause of the handler now breaks a test. - LEB128 sentinel →
77replaced with42(=0x2A, bit 6 clear → decodes as +42) in the wasm body, the WAT comment, and the.not.toBe(42)assertion, so the fall-through backstop is no longer vacuous.
Security risks
None. proc_raise already called process.kill(process.pid, …) on a string from a fixed table; this PR only corrects which string maps to which guest integer and narrows an error handler. No new capability is exposed.
Level of scrutiny
Low-to-medium. This is a constant-table correction in a JS-side polyfill (node:wasi in Bun is a temporary vendored wasi-js shim, per the file header). The new numbering is spec-derived and the test hard-codes the same table independently with a spec citation. I grep-verified WASI_SIG* has no consumers outside SIGNAL_MAP in this file, so the renumbering can't silently break another call site.
Other factors
- The mocked-kill test asserts the exact strong invariant (
raiseddeep-equals the full 30-entry preview1 list in order), covers both boundaries (0 and 31 → EINVAL), and assertsraised.lengthdidn't grow after the EINVAL calls. - The end-to-end test is
skipIf(isWindows)(POSIX signals), usestempDir/bunEnv/bunExe(), and its load-bearing assertionsignalCode === "SIGTERM"would fail on the pre-fix build (which delivered SIGTRAP). - No outstanding human reviewer comments; CodeRabbit was rate-limited and left no findings.
2 participants
What
bun prog.wasm: WASIproc_raise(sig)looked the guest signal up in a table whoseWASI_SIG*constants were numbered alphabetically (the pre-preview1wasi_unstableorder) instead of thewasi_snapshot_preview1signalenum, so e.g. guestSIGTERM(15) was delivered to the host asSIGTRAP— which also printed the "Bun has crashed" banner. Renumbered to preview1 order (SIGHUP=1 … SIGSYS=30, matching wasi-libc__WASI_SIGNAL_*), added the five signals missing fromSIGNAL_MAP, andproc_raisereturnsENOTSUPwhen the host has no such signal (e.g.SIGPOLL/SIGPWRon macOS) instead of throwing out of the import.Repro (before)
A hand-assembled wasm module that calls
wasi_snapshot_preview1.proc_raise(15):bun raise.wasm→signal=SIGTRAP+ crash banner (expectedSIGTERM).proc_raise(6)→SIGHUP,proc_raise(1)→SIGALRM.Tests
test/js/bun/wasm/wasi.test.js— "proc_raise uses wasi_snapshot_preview1 signal numbering" (all 30 + out-of-range → EINVAL, mocked kill) and "bun prog.wasm: proc_raise(SIGTERM) delivers SIGTERM to the host process". Fail on the current canary; pass here.