standalone: env var to disable the post-load madvise of embedded source - #37357
Conversation
…mbedded source resident Since #29320, a compiled executable calls madvise(MADV_DONTNEED) on its embedded source section once the entrypoint has loaded, so the pages are re-read from the executable on disk the next time something touches them (a lazy require(), resolving a stack trace). Give deployments where that re-read is undesirable a way out: setting the flag in the environment of the running executable skips the hint. It is read at runtime by the compiled binary, not at `bun build --compile` time, and follows the existing BUN_FEATURE_FLAG_DISABLE_* escape hatches (checked inside the implementing crate, falsy values keep the default).
|
@robobn adopt |
WalkthroughAdds a runtime feature flag for standalone ChangesStandalone madvise control
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@docs/bundler/executables.mdx`:
- Around line 401-412: Update the “Keeping the embedded source resident” section
to qualify the behavior as applying only on supported targets where Bun issues
the source-page hint, specifically macOS, Linux, and Android. Revise the final
platform-scope sentence so unsupported targets such as FreeBSD are not implied
to release source pages, while retaining the existing Windows and direct-script
exceptions.
In `@test/js/bun/compile/standalone-madvise-tla.test.ts`:
- Around line 48-50: Update the baseline child environment used by the
standalone madvise test to explicitly set
BUN_FEATURE_FLAG_DISABLE_STANDALONE_MADVISE to undefined when constructing
bunEnv, ensuring ambient process environment values cannot disable the feature
while preserving the existing assertions.
🪄 Autofix
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: 444c78f9-d63b-4652-bdd6-806010d8c898
📒 Files selected for processing (4)
docs/bundler/executables.mdxsrc/bun_core/env_var.rssrc/standalone_graph/StandaloneModuleGraph.rstest/js/bun/compile/standalone-madvise-tla.test.ts
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — straightforward escape-hatch env var following the established BUN_FEATURE_FLAG_DISABLE_* pattern.
What was reviewed:
- The flag check is an early return before the madvise call, inside the non-Windows cfg block;
.get().unwrap_or(false)matches sibling call sites (e.g.BUN_FEATURE_FLAG_DISABLE_EPOLL_PWAIT2,BUN_FEATURE_FLAG_DISABLE_STREAMING_INSTALL). new_feature_flag!with{}defaults tofalse, so unset/0keep the madvise firing as documented.- Test covers flag=
1and flag=0against the same compiled binary and adds anot.toContain("skipped")guard to the default case;bunEnvdoesn't set this flag so the baseline path is genuinely exercised.
Extended reasoning...
Overview
Adds BUN_FEATURE_FLAG_DISABLE_STANDALONE_MADVISE, a runtime opt-out for the post-entrypoint madvise(MADV_DONTNEED) on the embedded source section of bun build --compile binaries. Four files: one new_feature_flag! line in env_var.rs, an early-return guard in StandaloneModuleGraph::hint_source_pages_dont_need(), an extension of the existing debug-only test, and a docs section in executables.mdx.
Security risks
None. The flag only skips a best-effort memory-residency hint; no user input reaches a syscall argument, no allocation, no pointer arithmetic on the new path (the return happens before the page-rounding math). The env var goes through the same cached boolean machinery every other feature flag uses.
Level of scrutiny
Low. This is the canonical escape-hatch shape used dozens of times in this file — a boolean feature flag registered via macro and checked with .get().unwrap_or(false) at the single call site. The guarded operation was already best-effort (failures are logged and ignored), so skipping it entirely is strictly a subset of existing behavior. Windows and the plain interpreter are unaffected because the check sits inside the #[cfg(not(windows))] / mac-linux-android block, after the get_data() and len == 0 guards.
Other factors
The test extension follows harness conventions: spreads bunEnv, drains stdout/stderr/exited concurrently, asserts stderr and exit code last, and reuses the compiled binary from the existing case (proving the flag is read at runtime, not build time). It asserts both the presence of the skipped log when set and its absence when 0/unset, plus the absence of the MADV_DONTNEED log when skipped. Confirmed harness.ts doesn't pre-set this flag. The docs accurately describe scope (runtime-only, no effect on Windows or bun direct).
|
This PR may be a duplicate of:
🤖 Generated with Claude Code |
The flag check now returns silently alongside the len == 0 check, so the test asserts that no hintSourcePagesDontNeed line is logged when the flag is set. The unset baseline clears the variable explicitly so an ambient value on the host cannot leak into it. Docs name the platforms where the hint is issued.
There was a problem hiding this comment.
LGTM — a one-line new_feature_flag! registration plus an early-return guard, following the existing BUN_FEATURE_FLAG_DISABLE_* pattern exactly.
Checked that .get().unwrap_or(false) matches sibling call sites (e.g. DISABLE_MEMFD, DISABLE_SQL_AUTO_PIPELINING, DISABLE_EPOLL_PWAIT2).
Checked that the guard sits inside the existing cfg(macos/linux/android) block, so Windows/other targets are unaffected and len == 0 still short-circuits first.
Checked the test loop: undefined in the spread bunEnv drops any ambient value, "0" stays falsy per string_is_truthy, and the negative assertion for "1" is the only new branch.
All of cirospaciari's review threads (comment, log line, docs) are resolved in the current diff.
Extended reasoning...
Overview
Three files: a single new_feature_flag!(BUN_FEATURE_FLAG_DISABLE_STANDALONE_MADVISE, ...) line in src/bun_core/env_var.rs (inserted alphabetically among the other DISABLE_* flags), a guard added to the existing len == 0 early return in StandaloneModuleGraph::hint_source_pages_dont_need(), and an extension of the existing debug-only standalone-madvise-tla.test.ts to loop over three env values. A doc-comment sentence on the function was also updated.
Security risks
None. The flag only skips an advisory madvise(MADV_DONTNEED) hint that is already best-effort. No new syscalls, no untrusted input parsing, no allocation, no pointer arithmetic reached by the new branch. Reading an env var via the cached feature_flag accessor is the same mechanism used by dozens of other escape hatches.
Level of scrutiny
Low. This is a mechanical escape-hatch addition that mirrors an established pattern byte-for-byte — I grepped and confirmed .get().unwrap_or(false) is exactly how BUN_FEATURE_FLAG_DISABLE_MEMFD, _SQL_AUTO_PIPELINING, and _EPOLL_PWAIT2 are checked. The guard is placed inside the existing #[cfg(any(macos, linux, android))] block after len is computed, so it cannot affect the Windows path or the get_data() bailouts, and len == 0 still short-circuits before the env lookup. new_feature_flag! with {} defaults to false, so unwrap_or(false) is redundant-but-consistent with neighbors.
Other factors
The maintainer (cirospaciari) actively drove this PR via inline feedback (drop the flag comment, drop the skipped log, drop the docs section) and every thread is resolved in the current diff — the env_var.rs line has no comment, there is no separate "skipped" log, and docs/bundler/executables.mdx is no longer in the changed-files list. The test follows harness conventions: {...bunEnv, KEY: undefined} to strip an ambient value, pipes drained concurrently via Promise.all, exit code asserted last, test.skipIf(isWindows || !isDebug) preserved. The "0" case correctly asserts the hint still fires because string_is_truthy treats "0" as falsy. The PR description documents that the "1" case fails against a build without the change, satisfying the fails-for-the-right-reason bar. #37356 is noted as the mutually-exclusive alternative, which is a maintainer decision outside this review's scope.
There was a problem hiding this comment.
LGTM — a one-line new_feature_flag! registration plus an early-return guard, following the existing BUN_FEATURE_FLAG_DISABLE_* pattern verbatim.
What was reviewed:
.get().unwrap_or(false)matches the call shape used by every otherBUN_FEATURE_FLAG_DISABLE_*consumer (dns.rs, PostgresSQLConnection.rs, analytics/lib.rs, etc.).- The guard sits before both
scoped_log!sites inhint_source_pages_dont_need(), so the test'snot.toContain("hintSourcePagesDontNeed:")assertion for the"1"case cannot pass vacuously. - Test loop covers unset/
"0"/"1", spreadsbunEnvwith the flag key set toundefinedfor the baseline (hermetic), drains stdout/stderr/exited concurrently, and asserts exitCode last.
Extended reasoning...
Overview
Three files: (1) src/bun_core/env_var.rs gains one new_feature_flag! line inserted alphabetically among ~30 siblings; (2) src/standalone_graph/StandaloneModuleGraph.rs OR's the flag check into the existing len == 0 early return inside the macOS/Linux/Android #[cfg] block of hint_source_pages_dont_need(), and extends the doc comment by one clause; (3) the existing debug-only test standalone-madvise-tla.test.ts is extended to run the compiled binary under three flag values instead of one.
Security risks
None. The flag only controls whether an advisory madvise(MADV_DONTNEED) is issued on file-backed pages of the executable's own embedded source. Skipping it keeps pages resident (slightly higher RSS); issuing it lets the kernel reclaim them. No user input reaches a syscall argument, no allocation, no privilege boundary.
Level of scrutiny
Low. This is a mechanical escape-hatch addition matching a heavily-precedented pattern in the same file. The consumption site (.get().unwrap_or(false)) is byte-identical to how BUN_FEATURE_FLAG_DISABLE_SQL_AUTO_PIPELINING, BUN_FEATURE_FLAG_DISABLE_ADDRCONFIG, BUN_FEATURE_FLAG_DISABLE_EPOLL_PWAIT2, etc. are read elsewhere. The only behavioral change when the flag is unset is none — the added disjunct is || false.
Other factors
A maintainer (cirospaciari) has been actively driving this PR: adopted it, requested the flag comment be dropped, the skipped-log line be dropped, and the docs section be removed — all done and all threads resolved. The test follows harness conventions (await using, Promise.all pipe drain, ...bunEnv spread with undefined to strip ambient values, exitCode asserted last) and the PR description records that the "1" case fails against a build without the change. The competing #37356 (delete the madvise outright) is a direction call for maintainers, not a correctness concern for this PR.
3 participants
What does this PR do?
Adds
BUN_FEATURE_FLAG_DISABLE_STANDALONE_MADVISE, a runtime opt-out for the behavior introduced in #29320: abun build --compileexecutable callsmadvise(MADV_DONTNEED)on its embedded source section once the entrypoint has loaded, so anything that touches the source afterwards (a lazyrequire(), resolving a stack trace) pages it back in from the executable on disk. Deployments where that re-read is undesirable can set the flag in the environment of the running executable to keep the pages resident:The compiled binary reads it at runtime, nothing is baked in at compile time. It follows the existing
BUN_FEATURE_FLAG_DISABLE_*escape hatches: registered inenv_var.rs, checked inhint_source_pages_dont_need()(which returns early, sharing the existinglen == 0return), falsy values such as0keep the default. No effect on Windows or on the plain interpreter, which never issue the hint. Like the otherBUN_FEATURE_FLAG_DISABLE_*escape hatches it is not documented.#37356 is the alternative of removing the madvise call altogether; the two are mutually exclusive.
How did you verify your code works?
test/js/bun/compile/standalone-madvise-tla.test.tsnow runs the same compiled binary three times withBUN_DEBUG_StandaloneModuleGraph=1: with the variable unset (explicitly removed from the child env so an ambient value on the host cannot leak in), set to0, and set to1. The first two must log ahintSourcePagesDontNeed:line, the last must not log one at all, since the flag path returns before logging anything. The test stays debug-only because it relies on the scoped logger.Against a debug build without this change the
1case fails as expected:With the change,
bun bd test test/js/bun/compile/standalone-madvise-tla.test.tspasses (22 assertions).no test proof · iteration 1 · Platform-specific test(s) that do not run on this machine. Deferring to CI, which covers all platforms: test/js/bun/compile/standalone-madvise-tla.test.ts