process: back main-thread process.env by live libc environ on POSIX - #35270
process: back main-thread process.env by live libc environ on POSIX#35270robobun wants to merge 9 commits into
Conversation
The main-thread process.env is now a JSRealEnvMap exotic object whose
get/set/delete/enumerate are live getenv/setenv/unsetenv/environ calls
under a process-wide mutex, matching Node's RealEnvStore. Previously it
was a lazy-getter snapshot of the startup environment, so a native
library's getenv() never observed a JS write and a native setenv() never
reached process.env; JS and C ran on two silently divergent environments
for the life of the process.
JS writes also update Bun's internal DotEnv map so Bun.spawn without an
env option, Bun.which, and fetch's proxy resolution observe runtime
process.env changes. .env-file values are seeded into environ when the
map is created so getenv() is the single source of truth and native
libraries see them too. Workers keep their snapshot env (Node gives
workers a MapKVStore, not the RealEnvStore); a main-rooted SHARE_ENV
store now writes through to setenv/unsetenv on POSIX like it already did
to SetEnvironmentVariableW on Windows.
Also fixes a pre-existing bug this exposes: worker and debugger VMs
called configure_defines() without forcing LoadAllWithoutInlining, so
their transpiler inlined process.env.X as a string literal from the
cloned DotEnv map. A worker spawned with env:{X:'v'} would read the
parent's launch value for bare process.env.X while
globalThis.process.env.X returned 'v'.
|
Warning Review limit reached
Next review available in: 5 minutes 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)
WalkthroughChangesThe PR connects POSIX Process environment synchronization
Possibly related issues
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
Found 5 issues this PR may fix:
🤖 Generated with Claude Code |
|
This PR may be a duplicate of:
🤖 Generated with Claude Code |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
|
Updated 9:39 AM PT - Jul 23rd, 2026
✅ @robobun, your commit 96403f2680ed7548ef353b3864bbe353c6de489d passed in 🧪 To try this PR locally: bunx bun-pr 35270That installs a local version of the PR into your bun-35270 --bun |
…fixups - JSRealEnvMap::getOwnPropertySlot / defineOwnProperty: adopt the owned clone_utf8 via transferToWTFString() instead of ref-and-leak via toWTFString(ZeroCopy). Adds an RSS regression test. - Bun__Process__setOSEnv / unsetOSEnv: take vm.proxy_env_storage.lock() around the DotEnv-map write so it cannot race a spawning worker's clone_with_allocator (the documented env-map serialisation point). - setenv() return is now checked; the DotEnv map is only updated when environ was, so a rejected name (contains '=') cannot diverge the two. - Replace the O(n^2) seed-into-environ with a .env-only overlay set: DotEnv-map keys absent from environ are recorded once and reads fall back to the map for exactly those keys. A JS write promotes a key into environ and removes it from the overlay. Large .env files no longer blow up startup and per-read cost stays O(|environ|). - test/preload.ts: process.env writes now reach setenv, so stop overwriting an explicitly-passed CI value; otherwise a child spawned with CI=false to exercise test.only() would flip back to is_ci()=true. - process-env-environ-sync.test.ts: guard libcPathForDlopen() on Windows. - process.test.js: drop the now-live (previously dead) TZ branch that assumed a UTC host default.
…freeze)
JSRealEnvMap::defineOwnProperty entered the move-to-base + unsetenv path
for any descriptor without a value, which is what Object.freeze/seal
pass ({writable:false, configurable:false}) for every environ key. That
wiped libc environ and the DotEnv map. Gate the unsetenv on an actual
accessor descriptor; attribute-only descriptors are accepted as no-ops
on the live environ view. Same guard applied to JSSharedEnvMap so a
main-rooted SHARE_ENV freeze cannot reach unsetenv via syncOSEnv.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/jsc/bindings/JSEnvironmentVariableMap.cpp (1)
361-378: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winDo not publish rejected environment names to
SharedEnvStore.
Bun__Process__setOSEnvrejects empty, embedded-NUL, and=names, but thisvoidhelper lets the callers still executestore->setat Lines 584 and 654. A main-rootedSHARE_ENVthen exposes a value that neither libcenvironnor Bun’s env map accepted. Return success through this ABI/helper and update the store only after a successful native write.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/jsc/bindings/JSEnvironmentVariableMap.cpp` around lines 361 - 378, The syncOSEnv helper must report whether the native environment update succeeded instead of returning void. Propagate success from Bun__Process__setOSEnv and the Windows equivalent, then update SharedEnvStore only when syncOSEnv confirms the write succeeded at its callers around the store->set operations; rejected empty, embedded-NUL, or “=” names must not be published.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/runtime/api/BunObject.rs`:
- Around line 2339-2340: Replace std::collections::HashSet with the
repository-approved hash-set implementation in DOTENV_OVERLAY and the related
initialization set around the referenced code, preserving their existing key and
overlay behavior.
- Around line 2493-2495: Update the overlay enumeration in the surrounding
environment-key collection flow to add only keys not already present in the
native keys collected from environ(). Preserve existing overlay-only keys, and
add an FFI regression test covering native setenv promotion of a key originating
from DOTENV_OVERLAY.
In `@test/js/node/process/process-env-environ-sync.test.ts`:
- Around line 11-12: Update the suite gate around “process.env <-> libc environ
on the main thread” so it excludes FreeBSD before evaluating
libcPathForDlopen(). Either narrow the condition to Linux and macOS, or extend
libcPathForDlopen() with a valid FreeBSD path before use.
- Around line 137-142: Reorder the assertions in the subprocess test so stdout
parsing and deltaMB validation occur before the exitCode check. Keep the
existing stderr assertion and threshold logic unchanged, and make the final
assertion in this block validate exitCode is 0.
---
Outside diff comments:
In `@src/jsc/bindings/JSEnvironmentVariableMap.cpp`:
- Around line 361-378: The syncOSEnv helper must report whether the native
environment update succeeded instead of returning void. Propagate success from
Bun__Process__setOSEnv and the Windows equivalent, then update SharedEnvStore
only when syncOSEnv confirms the write succeeded at its callers around the
store->set operations; rejected empty, embedded-NUL, or “=” names must not be
published.
🪄 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: 5c4cda03-772a-49db-8e17-15520dcd2485
📒 Files selected for processing (5)
src/jsc/bindings/JSEnvironmentVariableMap.cppsrc/runtime/api/BunObject.rstest/js/node/process/process-env-environ-sync.test.tstest/js/node/process/process.test.jstest/preload.ts
💤 Files with no reviewable changes (1)
- test/js/node/process/process.test.js
… test on linux/macos - Replace disallowed std::collections::HashSet with bun_collections::StringSet for the .env overlay and its init-time environ snapshot. - enumerateOSEnv: a native setenv of a .env-only key (bypassing the JS write that would remove it from the overlay) must not yield the key twice. - Bun__Process__setOSEnv now returns whether environ was updated; syncOSEnv propagates it so JSSharedEnvMap skips store->set when setenv rejected the name, keeping a main-rooted SHARE_ENV store consistent with environ. - process-env-environ-sync.test.ts: gate on isLinux || isMacOS (libcPathForDlopen throws on FreeBSD); assert exitCode after stdout.
Release lanes retain ~20-25MB of mimalloc pages from the 20k transient 4KB StringImpl allocations; a real leak is 80MB+, so a single 40MB bound discriminates on every build.
These are Bun-launcher flags, not Node engine options. Next.js and other frameworks serialize process.execArgv into NODE_OPTIONS for spawned workers; now that process.env.NODE_OPTIONS writes reach setenv(), a Turbopack-spawned real node inherits NODE_OPTIONS=--bun from environ and rejects it. Update the process.execArgv test fixtures accordingly.
The function only iterates the map; use env_loader() (immutable) instead of env_mut() so it does not form &mut Map that could alias a spawning worker's clone_with_allocator read, matching getOSEnv's fallback.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/jsc/bindings/JSEnvironmentVariableMap.cpp (1)
780-799: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Object.freeze(process.env)still allows adding new keys on the main thread.
JSRealEnvMap::put()writes straight through toBun__Process__setOSEnv()without an extensibility check, anddefineOwnProperty()’s data-descriptor path just delegates there. That meansprocess.env.NEWVAR = "x"can still mutate the OS environment after freeze; the new freeze test only covers existing keys.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/jsc/bindings/JSEnvironmentVariableMap.cpp` around lines 780 - 799, Update JSRealEnvMap::put() and the data-descriptor path in defineOwnProperty() to honor the object’s extensibility state before calling Bun__Process__setOSEnv(). When the environment map is non-extensible, reject attempts to add keys that are not already present, while preserving updates to existing properties and the existing symbol/base handling.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/jsc/bindings/JSEnvironmentVariableMap.cpp`:
- Around line 780-799: Update JSRealEnvMap::put() and the data-descriptor path
in defineOwnProperty() to honor the object’s extensibility state before calling
Bun__Process__setOSEnv(). When the environment map is non-extensible, reject
attempts to add keys that are not already present, while preserving updates to
existing properties and the existing symbol/base handling.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 079f7ee8-c1dd-44cc-ab8f-52c27fd3b696
📒 Files selected for processing (5)
src/jsc/bindings/JSEnvironmentVariableMap.cppsrc/runtime/api/BunObject.rssrc/runtime/node/node_process.rstest/js/node/process/process-env-environ-sync.test.tstest/js/node/process/process.test.js
|
Build 78679 is green for this diff. The four remaining failures are all marked flaky (each passed on at least one retry) and touch nothing related to this change: |
2 participants
process.envon the main thread (POSIX) was a lazy-getter snapshot of the startup environment: a JSprocess.env.X = "v"never calledsetenv(), so abun:ffigetenv(), any native library reading its config from the environment, or anLD_PRELOADtool saw the pristine launch environment for the life of the process. And a nativesetenv()/unsetenv()never reachedprocess.env, not on the next read, not afterObject.keys(), not after a spawn. JS and C ran on two silently divergent environments.Fix
The main-thread
process.envis now aJSRealEnvMapexotic object whosegetOwnPropertySlot/put/deleteProperty/getOwnPropertyNamesare livegetenv/setenv/unsetenv/environcalls under a process-wide mutex, matching Node'sRealEnvStore.Bun also auto-loads
.envfiles, and glibc'ssetenvis O(n), so seeding every.envkey intoenvironwould turn a large.envinto O(n^2) startup. Instead.env-only keys are recorded in an overlay set;process.envreads fall back to the DotEnv map for exactly those keys, and a JS write promotes the key intoenviron(so nativegetenvthen sees it). Nativegetenv()of a.env-only key that has never been written from JS still returns NULL, which is the pre-PR behavior.JS writes also update Bun's internal DotEnv map so
Bun.spawn({})without anenv:option,Bun.which, and fetch's proxy resolution observe runtimeprocess.envchanges.Workers keep their snapshot env (Node gives workers a
MapKVStore, not theRealEnvStore). A main-rootedSHARE_ENVstore now writes through tosetenv/unsetenvon POSIX like it already did toSetEnvironmentVariableWon Windows.Also fixes: worker transpiler inlined
process.env.*from the parent envWorker and debugger VMs called
configure_defines()without forcingLoadAllWithoutInlining, inheriting theTarget::Bundefault ofLoadAll, so their transpiler rewroteprocess.env.Xas a string literal from the cloned DotEnv map. A worker spawned withenv: { X: "v" }would read the parent's launch value for bareprocess.env.XwhileglobalThis.process.env.Xreturned"v":This pre-dates the main change but was exposed by it (runtime JS writes now land in the internal map that the worker clones). Both secondary VMs now set
LoadAllWithoutInliningbeforeconfigure_defines(), matchingbun run/bun test/bun repl.Behavior changes
'TZ' in process.envon POSIX now matches Node:falsewhenTZis not set in the OS environment (previously alwaystruebecause of the always-installed TZ accessor).Object.keys(process.env)on POSIX now enumerates in libcenvironarray order followed by.env-only keys in.env-file insertion order (previously DotEnv-map insertion order for everything).Verification
test/js/node/process/process-env-environ-sync.test.tscovers JS→C (set / overwrite / delete), C→JS (setenv / unsetenv / enumeration),Bun.spawninheritance of runtime writes, theinoperator,Object.freezenot touchingenviron, an RSS leak guard, and the worker inlining bug. All fail on the previous build, all pass here.env.test.ts,worker_threads.test.ts, and the Node parallel env tests pass.Fixes #34210
Overlap with open PRs
LoadAllWithoutInliningfix toweb_worker.rs(this PR also coversDebugger.rs); it carries a transpiler-cache regression test worth keeping regardless of which lands first.Bun.spawn/Bun.whichread the JSprocess.envobject directly instead of the DotEnv snapshot. This PR achieves the same observable result for those callers by syncing the DotEnv map on every JS write, while also fixing the nativegetenv/setenvdirection those PRs do not touch.Not fixed here
os.homedir()caches independently ofenviron; settingHOMEviasetenv()is not enough..env()PATH changes don't affect command resolution #25885: Bun Shell$.env({PATH})command resolution is a separate code path fromprocess.env.execFileSynccase already resolves correctly on main.no test proof · iteration 3 · Platform-specific test(s) that do not run on this machine. Deferring to CI, which covers all platforms: test/js/node/process/process-env-environ-sync.test.ts test/js/node/process/process.test.js