Repository navigation
process: NUL-terminate the PSI trigger that arms memoryPressure on Linux - #39654
Conversation
psi_write() in kernel/sched/psi.c replaces the last byte of the write with NUL before it parses the trigger. Bun wrote "some 150000 2000000" with no terminator, so the kernel parsed a 200 ms window and returned EINVAL. The watcher then installed with no OS backend, and the memoryPressure event never fired on Linux. Include the NUL in the write, as the kernel's own sample does. Add two bun:internal-for-testing hooks. memoryPressurePsiTrigger() returns the bytes Bun writes. memoryPressureWatcherHasOsBackend() reports whether the installed watcher holds a live OS source. The first new test checks the bytes on every Linux machine. The second arms a real trigger where the kernel allows it, and is skipped elsewhere. Co-authored-by: catpranks <pranks@catpranks.tv>
|
Warning Review limit reached
Next review available in: 11 minutes Limit details: You’ve used all 5 included reviews currently available. You can run this review on demand instead of waiting. On-demand reviews are free until September 18, 2026. After that, they cost $0.25 per reviewed file.
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 within each organization. For paid Pro and Pro+ reviews, CodeRabbit uses a developer's included PR review attempts over the past 7 days to set the current hourly allowance. At typical activity levels, the full plan allowance applies. Higher sustained activity can lower the allowance until earlier attempts leave the 7-day 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 (4)
Comment |
|
Status How this was confirmed: Related: #34018 has the same one-byte change from a fork. Its author is credited in the commit here. |
There was a problem hiding this comment.
LGTM — one-byte fix (NUL-terminate the PSI trigger string) with a clear kernel-source citation, plus test hooks so the regression is caught on every Linux CI run.
What was reviewed:
- The
PSI_TRIGGERconstant move topub(super)and the added\0—bun_sys::writealready took a&[u8], so the extra byte reachespsi_writeintact and the kernel'sbuf[buf_size-1] = '\0'now overwrites the NUL instead of the last window digit. - The two new
#[bun_jsc::host_fn]hooks and their codegen wiring:rustTargetmapsruntime/node/memory_pressure.rs+jsPsiTrigger/jsWatcherHasOsBackendtocrate::node::memory_pressure::{js_psi_trigger, js_watcher_has_os_backend}, which match the actualpub(crate)fn names;ArrayBuffer::create_buffercopies the&[u8]so no lifetime concern. has_os_backend'sunsafederef of the type-erased slot — same invariant astake_watcherright above it, and only reachable from the JS thread.- The two new tests: the byte-shape test runs on all Linux and would fail without the NUL; the arm test's
skipIfguard mirrorsopen_psi_fd's two-path fallback and closes its probe fd infinally.
Extended reasoning...
Overview
The PR fixes process.on("memoryPressure") never arming on Linux: open_psi_fd was writing b"some 150000 2000000" to /proc/pressure/memory, and psi_write() in the kernel overwrites the last received byte with NUL before parsing, so it saw a 200000 µs window and returned EINVAL. The fix is to include the NUL in the write. The constant is hoisted to module scope as pub(super) const PSI_TRIGGER so a new bun:internal-for-testing hook can return it, and a second hook exposes whether the installed watcher actually holds a live poll. Two tests exercise both hooks; the byte-shape test runs on every Linux machine, the live-arm test skips when the environment can't create a trigger.
Files touched: src/runtime/node/memory_pressure.rs (the fix + two host fns + has_os_backend helper), src/codegen/generate-js2native.ts (one map entry), src/js/internal-for-testing.ts (two exports), test/js/node/process/process-memory-pressure.test.ts (two tests + a capability probe).
Security risks
None. The only runtime-path change is one extra byte in a constant string written to a procfs file that Bun already opens. The new host functions are gated behind bun:internal-for-testing and are read-only (return a constant Buffer / a boolean derived from existing state). No user input is parsed.
Level of scrutiny
Low-to-medium. The production fix is a one-byte constant change with the kernel source line quoted in both the PR body and the code comment, and the PR author verified fail-before/pass-after. The rest is test scaffolding following the same `` / #[bun_jsc::host_fn] pattern used dozens of times in `internal-for-testing.ts`. I traced the codegen path (`rustIdentifierPaths` → `rustTarget` → snake_case) and confirmed it resolves to the actual `pub(crate)` function names in `crate::node::memory_pressure`. I also confirmed `ArrayBuffer::create_buffer(global, &[u8])` copies its input, so returning a reference to the static `PSI_TRIGGER` is fine.
Other factors
- The
has_os_backendunsafe block mirrors the existingtake_watcherSAFETY invariant one function above and is only called from a JS host fn on the JS thread, so no concurrent borrow. - cfg gating looks right:
ArrayBufferis imported only on Linux/Android;js_psi_triggerreturnsJSValue::NULLelsewhere;js_watcher_has_os_backendon Windows delegates toisInstalled(correct per the doc comment — Windows either starts the thread or leaves the slot empty). PR body sayscargo checkpassed on Windows/macOS/FreeBSD. - Tests follow harness conventions: subprocess spawn with
bunExe()/bunEnv, concurrent pipe drain, stdout asserted before exitCode,test.skipIfwith a real capability check that closes its probe fd infinally. - No prior reviews or outstanding comments on the PR.
Problem
process.on("memoryPressure")never fires on Linux.open_psi_fd(src/runtime/node/memory_pressure.rs:153) writessome 150000 2000000with no terminator.psi_write()inkernel/sched/psi.creplaces the last byte of a write with NUL before it parses. It sees a 200 ms window and returnsEINVAL, so the watcher installs with no OS source.Fix
strlen + 1bytes.bun:internal-for-testinghooks through$newRustFunction.memoryPressurePsiTrigger()returns the bytes Bun writes.memoryPressureWatcherHasOsBackend()tells whether the watcher holds a live poll.test/js/node/process/process-memory-pressure.test.ts. The first checks the bytes on every Linux machine. The second arms a real trigger. It is skipped where the test process cannot create one itself (most CI containers).cargo checkfor Windows, macOS and FreeBSD, clippy, andtest/internal/source-lints.Background
some <threshold_us> <window_us>to/proc/pressure/memoryarms a trigger on that fd.memory.pressure. kernfs terminates the cgroup write itself, so that fallback worked either way. Most hosts only offer the/procfile.Notes
Kernel source for the claim (v6.6,
kernel/sched/psi.c,psi_write):psi_trigger_createthen rejectswindow_us % 2000000 != 0for a writer withoutCAP_SYS_RESOURCE. A writer with that capability gets a 200 ms window on the RT polling aggregator instead, which needs a 75% stall to fire rather than 7.5%.The cgroup path (
kernel/cgroup/cgroup.c,pressure_write) receives a kernfs buffer that already has a NUL after the data, so it worked with and without this change.The first test also checks the two other rules
psi_trigger_createapplies to an unprivileged writer: the threshold fits in the window, and the window is a multiple of 2 s. The second test checkshasOsBackendbefore a listener is added (false), with one (true), and after it is removed (false). Its skip guard opens and writes the same two files, in the same order, thatopen_psi_fduses.This container denies
O_RDWRon/proc/pressure/memory(EACCES, AppArmor) and mounts the cgroup fs read-only, so the second test is skipped here. Fail-before output with only the NUL removed fromPSI_TRIGGER:#36525 was an earlier copy of the same fix. It was closed in favor of #34018, and its branch no longer merges. #31021 (opt-in shrink on pressure) still carries the unterminated string and needs this byte too.
no test proof · iteration 0 · Platform-specific test(s) that do not run on this machine. Deferring to CI, which covers all platforms: test/js/node/process/process-memory-pressure.test.ts