Skip to content

dotenv: read an explicit --env-file that is a pipe, FIFO or device - #40837

Merged
Jarred-Sumner merged 4 commits into
mainfrom
farm/49cf56cb/env-file-pipe
Aug 28, 2026
Merged

Jarred-Sumner merged 4 commits into
mainfrom
farm/49cf56cb/env-file-pipe

Conversation

@robobun

@robobun robobun commented Aug 28, 2026 •

Copy link
Copy Markdown
Collaborator

Fixes #30520.

Problem

Fix

  • An explicit entry is read whatever its kind, as in Node: the open has no O_NONBLOCK, so a FIFO waits for its writer like cat, and read(2) runs until EOF instead of pread. Default .env* discovery keeps the skip.
  • A pipe can be read once. A Worker starts from Loader::clone_for_worker, a copy of the parent's loader with the explicit entries marked loaded, so it does not open them again. A bun test --parallel worker loads no env file: its environment is the coordinator's map, and that map can carry --env-file in BUN_OPTIONS too.
  • Correct because the user named the path. Node reads any readable path and blocks on a FIFO with no writer. A re-exec with the same argv (--watch, fork()) reads the path again, as in Node. The docs say so.
  • Verified: 6 tests in test/cli/run/env.test.ts (--env-file that is not a regular file) and 3 in test/cli/test/parallel.test.ts. Seven fail with main's src/.

Background

  • Loader::load loads the --env-file list (load_env_file_dynamic) or probes the cwd for the default .env* names (load_env_file). Both call read_env_file_contents.
  • File::read_to_end reads with pread, which fails on a pipe. File::read_to_end_into is the read(2) loop bun run - uses for stdin.
  • A Worker clones the parent's env map and then runs configure_defines with the parent's options, which opened every --env-file entry again. The --parallel coordinator gives each child its env map and forwards its flags.
Notes

Repro on main (69c6138):

bun --env-file=<(echo A=1) -e 'console.log(process.env.A)'              # undefined, exit 0
echo A=1 | bun --env-file=/dev/stdin -e 'console.log(process.env.A)'    # undefined
mkfifo ff; (echo A=1 > ff &); bun --env-file=ff -e 'console.log(process.env.A)'  # undefined

With this change all three print 1. A writer that connects after bun started also works: bun waits in open. Bun 1.4.0 and 1.4.1 exit 1 with no output for the same commands (pread on a pipe fails with ESPIPE).

With the loader change alone, new Worker() and bun test --parallel hang on the FIFO case: they re-open the path and no writer is left. That is why the worker and test runner hunks are here. The worker test rewrites the env file after startup and checks the worker does not see the new key. The parallel FIFO tests fail fast with main's src/ (the coordinator skips the FIFO) and hang with the loader change alone.

The first revision forwarded --no-env-file to the --parallel workers instead of the list. Review found that BUN_OPTIONS=--env-file=fifo defeats that: the coordinator's env map, which becomes each worker's environment, still carries BUN_OPTIONS, and a worker splices it into its own argv. So the worker now clears its env file list itself (test_command.rs, under --test-worker) and the coordinator forwards neither --env-file nor --no-env-file. The FIFO test runs both routes.

Why the worker copies the loader's memo instead of clearing env_files in its transform options: a compiled executable runs apply_standalone_runtime_flags after the worker copied the options, and that resets disable_default_env_files. With cleared env_files the worker then loaded .env* while the main thread did not. The memo does not depend on those flags. The test a worker in a compiled executable pins it with BUN_OPTIONS=--env-file=.env.a.

Loader::load prints the [0.05ms] ".env" line only when the call loaded something. A worker's loader starts with the parent's entries marked loaded, and in debug builds its log level is Info, so it printed the parent's list for files it did not open.

What a re-exec does: --watch reloads and child_process.fork() / cluster.fork() children run the same argv, so they open the --env-file path again. A pipe is then at EOF and loads nothing (main: loads nothing every time). A FIFO waits for a new writer, which is what Node's --watch child and fork() child do. 1Password's FIFO serves every open. A reload that reuses the first read is a separate change.

Behavior that did not change:

  • An explicit entry that fails to open (missing, socket) is still skipped silently. cli: fail when an explicit --env-file cannot be loaded #36523 makes that fatal and touches load_env_file_dynamic too. The conflict is small: its open and read should keep the blocking open and the read(2) path from here.
  • An explicit entry that is a directory is read, read(2) fails with EISDIR, and the message prints in non-quiet mode (bun install). That is the behavior before dotenv: skip .env entries that are not regular files #40711.
  • No size bound on the streamed read. --env-file=/dev/zero grows until try_reserve fails with ENOMEM. Node aborts with bad_alloc on the same input, and bun run - reads stdin the same way.
  • macOS opens /dev/fd/N as a dup of the descriptor. No fcntl is done on it.
  • Windows never used O_NONBLOCK. The only change there is that a non-regular explicit file (a named pipe, CON) is read.

The removed test --env-file pointing at a FIFO pinned the skip with no writer. Its replacement, a FIFO, has a writer. Without one the open waits, as in Node.

Suites run with the debug build: env.test.ts (107 pass), no-envfile.test.ts, run-process-env.test.ts, parallel.test.ts (41 pass), web/workers/worker.test.ts, web/workers/worker-entry-point.test.ts, node/worker_threads/worker_threads.test.ts. Three worker.test.ts cases under terminate() races and lifecycle edges fail or time out at 5 s the same way with main's src/ in this debug build and are unrelated. cargo check -p bun_dotenv for x86_64-pc-windows-msvc and aarch64-apple-darwin, cargo clippy -p bun_dotenv clean.


no test proof · iteration 0 · platform-specific test(s) that do not run on this machine, deferring to CI, which covers all platforms: test/cli/test/parallel.test.ts, test/cli/run/env.test.ts

An explicit --env-file entry is now read whatever kind of file it is,
as in Node. The loader opens it without O_NONBLOCK, so a FIFO waits for
its writer, and reads a non-regular file with read(2) until EOF instead
of pread. Default .env discovery keeps the regular-file check: a
directory, FIFO, socket or device named .env is still skipped.

A pipe can be read once. A Worker's loader starts as a copy of the
parent's, with the explicit entries already marked loaded, so it does
not open them again. bun test --parallel children get --no-env-file
instead of the --env-file list; the values are in the environment they
inherit. Loader::load only prints the loaded-files line when this call
loaded something.
@robobun

robobun commented Aug 28, 2026 •

Copy link
Copy Markdown
Collaborator Author
Updated 12:32 PM PT - Aug 28th, 2026

❌ @robobun, your commit 5bb683d has 1 failures in Build #107926 (All Failures):


🧪   To try this PR locally:

bunx bun-pr 40837

That installs a local version of the PR into your bun-40837 executable, so you can run:

bun-40837 --bun

@coderabbitai

coderabbitai Bot commented Aug 28, 2026 •

Copy link
Copy Markdown
Contributor

Warning

Review limit reached

  • Run on-demand review

On-demand reviews are free for the next 23 days. After that, they cost $0.25 per reviewed file.

Or wait 4 minutes for your next included review.

View limit details

Limit details: You’ve used all 5 included reviews currently available.

Learn how review limits work.

Review configuration:

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: d1d41495-d964-4414-8b37-d29b50a07698

📥 Commits

Reviewing files that changed from the base of the PR and between d578a8c and 5bb683d.

📒 Files selected for processing (7)
  • docs/runtime/environment-variables.mdx
  • src/dotenv/env_loader.rs
  • src/jsc/web_worker.rs
  • src/runtime/cli/test/parallel/runner.rs
  • src/runtime/cli/test_command.rs
  • test/cli/run/env.test.ts
  • test/cli/test/parallel.test.ts

Comment @coderabbitai help to get the list of available commands.

Comment thread src/runtime/cli/test/parallel/runner.rs Outdated
…e took

The coordinator's env map is the worker's environment, and that map
carries BUN_OPTIONS. A worker spliced a BUN_OPTIONS --env-file back into
its argv and opened the path again, so the forwarded --no-env-file did
not cover it. The worker now clears its env file list itself, and the
coordinator stops forwarding --env-file and --no-env-file.
Comment thread src/dotenv/env_loader.rs Outdated
Comment thread src/dotenv/env_loader.rs Outdated
Comment thread src/dotenv/env_loader.rs Outdated
Comment thread src/dotenv/env_loader.rs Outdated
Comment thread src/dotenv/env_loader.rs Outdated
Comment thread src/dotenv/env_loader.rs Outdated
Comment thread src/dotenv/env_loader.rs Outdated
Comment thread src/runtime/cli/test/parallel/runner.rs Outdated
Comment thread src/runtime/cli/test_command.rs Outdated
Comment thread src/runtime/cli/test_command.rs
Comment thread src/dotenv/env_loader.rs Outdated
Comment thread src/runtime/cli/test_command.rs Outdated
@robobun

robobun commented Aug 28, 2026

Copy link
Copy Markdown
Collaborator Author

CI on 5bb683d: every lane passes except darwin x64, where test/js/web/url/url.test.ts fails with TypeError: Invalid URL in the Unicode 16 IDNA case. That test fails on main on the same runner (ICU table version) and this change does not touch URL parsing. The other red-then-green entries are retries that passed. Ready for review.

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re-reviewed after the follow-up commits — no bugs found. The BUN_OPTIONS re-read I flagged earlier is now handled by clearing env_files and forcing disable_default_env_files under test_worker in test_command.rs (before ctx.args.clone() and configure_defines), and the new parallel.test.ts FIFO test exercises both the argv and BUN_OPTIONS routes. A human sign-off is still worthwhile since this is a user-visible behavior change: an explicit --env-file FIFO with no writer now blocks startup where it was previously skipped.

What was reviewed

  • clone_for_worker() matches init_with_map() field-for-field except for the cloned custom_files_loaded, so worker loader semantics are unchanged apart from the intended memo.
  • read_env_file_contents: regular files keep the st_size == 0 early-out and pread path; only explicit non-regular files hit read_to_end_into; default discovery still returns Empty for non-regular.
  • build_worker_argv no longer forwards --env-file/--no-env-file; verified the worker-side clear covers the gap for both flag sources.
  • Removed --env-file pointing at a FIFO test pinned the now-changed behavior; its replacement (a FIFO) has a writer and asserts the new contract.
Extended reasoning...

Overview

This PR makes explicit --env-file=<path> read pipes/FIFOs/devices to EOF (matching Node), while default .env* discovery still skips non-regular files. To keep single-read pipes from being reopened, it threads the loader's custom_files_loaded memo into Worker VMs via a new clone_for_worker(), and makes bun test --parallel workers load no env files at all (they inherit the coordinator's environment). Touches src/dotenv/env_loader.rs, src/jsc/web_worker.rs, src/runtime/cli/test/parallel/runner.rs, src/runtime/cli/test_command.rs, docs, and adds nine tests across env.test.ts and parallel.test.ts.

Security risks

Env files carry secrets, but the change narrows rather than widens exposure: secrets can now be piped in without touching disk, and workers stop re-reading files. The unbounded read_to_end_into on /dev/zero is noted in the PR and matches both Node and bun run -. No new path-traversal or injection surface — the path is still opened with openat(cwd, ...) and open failures are still swallowed as before.

Level of scrutiny

Medium-high. The logic per hunk is small, but the behavior change is user-visible and startup-blocking (a FIFO with no writer now hangs open(2) where it used to be skipped), and it touches three subsystems (dotenv, worker VM init, test-parallel runner) whose interaction was the subject of my earlier finding. That interaction is now closed with a targeted fix and a test that covers both flag-delivery routes.

Other factors

The follow-up commits addressed my prior inline finding directly: the worker-side clear in test_command.rs runs after BUN_OPTIONS has been spliced into argv but before configure_defines, so it neutralizes both routes without needing to strip BUN_OPTIONS from the forwarded environment. The loaded_count() gate on print_loaded prevents workers from logging files they never opened. Test coverage is thorough (stdin pipe, process substitution, FIFO with writer, comma-list precedence, Worker no-reread, compiled-executable + BUN_OPTIONS, parallel FIFO ×2 routes). Given the intentional behavior change and cross-cutting reach, deferring for a human ack rather than approving.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

--env-file and Bun.file().text() silently drop data on FIFOs; breaks 1Password's local-env-file integration

2 participants