Skip to content

Startup snapshots (3/4): bun build --compile --snapshot, Bun.build({ snapshot }), docs - #37261

Open
Jarred-Sumner wants to merge 1 commit into
claude/startup-snapshot-runtimefrom
claude/startup-snapshot-build
Open

Startup snapshots (3/4): bun build --compile --snapshot, Bun.build({ snapshot }), docs#37261
Jarred-Sumner wants to merge 1 commit into
claude/startup-snapshot-runtimefrom
claude/startup-snapshot-build

Conversation

@Jarred-Sumner

@Jarred-Sumner Jarred-Sumner commented Aug 9, 2026

Copy link
Copy Markdown
Collaborator

Third of four: #37259 (deps) → #37260 (runtime + Bun.startupSnapshot) → #37261 (bun build --snapshot) → #37262 (tooling). Base is claude/startup-snapshot-runtime.

Surface
bun build --compile --snapshot[=auto|manual] --snapshot-io=strict|local|network Builds the executable, runs it once, embeds the snapshot it takes, as-is (the executable maps its own pages). auto: taken once startup drains; manual: the app calls take(). The default io policy refuses fs/spawn/dns/net during the build; local/network allow and report each use with call sites. No snapshot → the build fails and says why.
bun build --snapshot --outfile exe The snapshot step alone against an executable built earlier (cross-compile anywhere, snapshot on the target); in place and re-runnable. The build tells the executable what to do through a marking in its own payload — nothing goes through env or argv.
Bun.build({ compile, snapshot }) Same, programmatically; throws without compile.
Docs docs/bundler/startup-snapshots.mdx (new page), pointer from executables.mdx
Mechanics
StandaloneModuleGraph.rs Stamping the marking bits, appending the snapshot to the serialized payload, embedding into / rewriting an executable
exe_format/elf.rs Rewriting an ELF that already carries a payload: the block records where its slot is; a rewrite overwrites in place when the new payload fits and otherwise replaces the block, so a file carries one block however often it is rewritten (verified: three consecutive rewrites leave the file the same size). Mach-O rewrites were already in place.
build_command.rs, Arguments.rs, JSBundler.rs, js_bundle_completion_task.rs, context.rs Flag/option parsing, the run-once step, exit-code reporting
Tests startup-snapshot-build.test.ts (its own file; it shares the support probe, envs and one round-trip ceiling with the runtime file's harness): embedded restore (asserts the copied data segment stays small — the payload must never be part of it), auto mode, the split step, Bun.build incl. the bun-<platform> target shorthand and every rejected shape (bad values, no compile, standalone HTML — also rejected on the CLI), the mode-specific messages when the app exits without a snapshot, io policy, main(), stdio (three cases), signals, wasm, the wasm tier-up race, stale sidecars and failed runs, and a Linux-only forced stack-overlap launch

Numbers, each tool built with --bytecode versus the same plus a snapshot (Linux arm64 container, CI binary; private memory is RssAnon at exit):

tool wall clock private memory snapshot
babel + preset-env 50.9 → 26.8 ms 38.5 → 20.2 MB 50 MB
postcss + autoprefixer 45.5 → 17.3 ms 32.6 → 17.4 MB 35 MB
sass 36.0 → 14.0 ms 33.1 → 17.2 MB 36 MB
typescript 29.4 → 12.0 ms 32.0 → 16.2 MB 31 MB
prettier 21.9 → 13.8 ms 20.6 → 11.2 MB 25 MB
markdown-it 9.6 → 7.9 ms 7.7 → 6.6 MB 8 MB
yaml 8.6 → 8.2 ms 7.2 → 7.3 MB 7 MB
esbuild-wasm, warmed 117 → 24 ms 135 MB
noop 2.1 → 2.1 ms 6 MB

macOS: smaller wall-clock wins (a snapshot launch there is one extra exec) with the same 60–65% drop in user CPU; typescript's footprint 37 → 24 MB. A large interactive application, one turn then idle: 228–232 MB → 144–158 MB.

Builds on the runtime branch and passes the full file on its own binary (21 pass, 1 Linux-only skip on macOS).

@robobun

robobun commented Aug 9, 2026

Copy link
Copy Markdown
Collaborator
Updated 1:47 AM PT - Aug 11th, 2026

@Jarred-Sumner, your commit c999b85 is building: #92106

@Jarred-Sumner
Jarred-Sumner force-pushed the claude/startup-snapshot-runtime branch from 2f3b8df to 63904c9 Compare August 9, 2026 14:41
@Jarred-Sumner
Jarred-Sumner force-pushed the claude/startup-snapshot-build branch from 7eef131 to 8ba57e7 Compare August 9, 2026 14:41
@Jarred-Sumner
Jarred-Sumner force-pushed the claude/startup-snapshot-runtime branch from 63904c9 to fa373fb Compare August 9, 2026 15:53
@Jarred-Sumner
Jarred-Sumner force-pushed the claude/startup-snapshot-build branch from 8ba57e7 to 87c2262 Compare August 9, 2026 15:53
@Jarred-Sumner
Jarred-Sumner force-pushed the claude/startup-snapshot-runtime branch from fa373fb to 581ca93 Compare August 9, 2026 16:22
@Jarred-Sumner
Jarred-Sumner force-pushed the claude/startup-snapshot-build branch from 87c2262 to 052d69c Compare August 9, 2026 16:23
@Jarred-Sumner
Jarred-Sumner marked this pull request as ready for review August 9, 2026 17:39
@Jarred-Sumner
Jarred-Sumner requested a review from alii as a code owner August 9, 2026 17:39
Comment thread src/runtime/cli/Arguments.rs Outdated
Comment thread src/exe_format/elf.rs Outdated
Comment thread src/options_types/context.rs Outdated
@Jarred-Sumner
Jarred-Sumner force-pushed the claude/startup-snapshot-runtime branch from 581ca93 to 1bb17a7 Compare August 9, 2026 20:30
@Jarred-Sumner
Jarred-Sumner force-pushed the claude/startup-snapshot-build branch from 052d69c to 5da7ff9 Compare August 9, 2026 20:30
Comment thread src/exe_format/elf.rs
Comment thread test/js/bun/startup-snapshot/startup-snapshot.test.ts Outdated
@Jarred-Sumner
Jarred-Sumner force-pushed the claude/startup-snapshot-runtime branch from 1bb17a7 to 8370a83 Compare August 9, 2026 21:10
@Jarred-Sumner
Jarred-Sumner force-pushed the claude/startup-snapshot-build branch from 5da7ff9 to 4351993 Compare August 9, 2026 21:10
@Jarred-Sumner
Jarred-Sumner force-pushed the claude/startup-snapshot-runtime branch from 8370a83 to 45ca038 Compare August 9, 2026 21:22
@Jarred-Sumner
Jarred-Sumner force-pushed the claude/startup-snapshot-build branch from 93aeb96 to 0dd4b79 Compare August 9, 2026 21:22
@Jarred-Sumner
Jarred-Sumner force-pushed the claude/startup-snapshot-runtime branch from 45ca038 to 6638c2f Compare August 9, 2026 21:42
@Jarred-Sumner
Jarred-Sumner force-pushed the claude/startup-snapshot-build branch from 0dd4b79 to f1f73ea Compare August 9, 2026 21:42
Comment thread src/runtime/cli/build_command.rs Outdated
Comment thread src/runtime/cli/build_command.rs Outdated
Comment thread src/standalone_graph/StandaloneModuleGraph.rs Outdated
@Jarred-Sumner
Jarred-Sumner force-pushed the claude/startup-snapshot-runtime branch from 6638c2f to d71ca1c Compare August 9, 2026 22:08
@Jarred-Sumner
Jarred-Sumner force-pushed the claude/startup-snapshot-build branch from f1f73ea to d35d7c2 Compare August 9, 2026 22:08
@Jarred-Sumner
Jarred-Sumner force-pushed the claude/startup-snapshot-runtime branch from d71ca1c to 7649dcb Compare August 9, 2026 22:09
@Jarred-Sumner
Jarred-Sumner force-pushed the claude/startup-snapshot-build branch from d35d7c2 to 79f7605 Compare August 9, 2026 22:09
@Jarred-Sumner
Jarred-Sumner force-pushed the claude/startup-snapshot-build branch from f2bc63a to 3f08c31 Compare August 10, 2026 04:37

@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.

Additional findings (outside current diff — PR may have been updated during review):

  • 🔴 test/js/bun/startup-snapshot/startup-snapshot.test.ts:989 — The fix applied for the earlier review comment on build_command.rs:1597 reworded the manual-mode Ok(_) message to "with --snapshot=manual the app has to call Bun.startupSnapshot.take() before it exits", but this assertion still checks for the old "in manual mode the app has to call Bun.startupSnapshot.take()" — the substring "in manual mode" no longer appears anywhere in src/, so this test will fail deterministically on every platform where hasSnapshots is true. Update the assertion to the new wording, e.g. toContain("with --snapshot=manual the app has to call Bun.startupSnapshot.take()"). REVIEW.md: "When changing output/defaults/messages, grep the suite for assertions on the old behavior and update them in the same PR."

    Extended reasoning...

    What the bug is

    The still-open review comment on build_command.rs:1597 ("The Ok(_) arm's message ... is emitted regardless of mode") was addressed by branching on mode and rewording the manual-mode message. build_command.rs:1591 now emits:

    Ok(_) if mode == CompileStartupSnapshot::Manual => format!(
        "{} exited without taking a snapshot: with --snapshot=manual the app has to call Bun.startupSnapshot.take() before it exits",
        ...
    ),

    But startup-snapshot.test.ts:989 still asserts on the old wording:

    expect(m.out).toContain("in manual mode the app has to call Bun.startupSnapshot.take()");

    The substring "in manual mode" is not present in the new message, so toContain will fail.

    The specific code path that triggers it

    The test builds auto-fixture.js with --compile --snapshot=manual. auto-fixture.js never calls Bun.startupSnapshot.take() and, when isBuildingSnapshot() is true, prints nothing and has nothing keeping the event loop alive — so the child exits with status 0 without writing a sidecar. In run_startup_snapshot_step:

    • status = Ok(SpawnStatus { code: 0 }), so ran_ok = true.
    • stat(snapshot_z) fails (no sidecar), so written = false.
    • The failure block is entered; the match falls through the NOT_QUIET (== 70), == -1, and !st.is_ok() guards to Ok(_) if mode == CompileStartupSnapshot::Manual.
    • The message emitted is "<exe> exited without taking a snapshot: with --snapshot=manual the app has to call Bun.startupSnapshot.take() before it exits".

    m.out is stderr + stdout of the bun build process, which (via spawn_sync_inherit) also captures the child's inherited stdio — but neither the build nor the child emits "in manual mode" anywhere.

    Why nothing else prevents it

    rg 'in manual mode' across the whole repo returns exactly one hit: this test line. rg 'the app has to call Bun.startupSnapshot' src/ returns exactly one hit: build_command.rs:1591 with the new wording. There is no other emitter that could satisfy the old substring. The test is gated by snapshotTest = test.skipIf(!hasSnapshots), so it runs on macOS and glibc Linux — every CI lane where snapshots are supported.

    Step-by-step proof

    1. CI runs bun bd test test/js/bun/startup-snapshot/startup-snapshot.test.ts on a macOS or glibc-Linux runner; hasSnapshots is true, so the "local I/O during the build ..." test executes.
    2. It reaches const m = build(["--compile", "--snapshot=manual", join(import.meta.dir, "auto-fixture.js"), "--outfile", join(String(dir), "manual")]).
    3. bun build compiles the executable, marks it TAKE_STARTUP_SNAPSHOT | STARTUP_SNAPSHOT_MANUAL, unlinks any stale sidecar, and spawns it.
    4. auto-fixture.js runs: epoch() is 0, isBuildingSnapshot() is true, so the last-line console.log is skipped; nothing awaits and no timer is armed; the process exits 0 without ever calling take().
    5. run_startup_snapshot_step sees ran_ok && !written, matches the Ok(_) if mode == Manual arm, and returns Err("<exe> exited without taking a snapshot: with --snapshot=manual the app has to call Bun.startupSnapshot.take() before it exits").
    6. BuildCommand::exec prints that via Output::print_errorln and exits 1; m.out therefore contains "with --snapshot=manual the app has to call ..." and m.code == 1.
    7. expect(m.out).toContain("in manual mode the app has to call Bun.startupSnapshot.take()") fails — "in manual mode" ⊄ "with --snapshot=manual".

    Impact

    Deterministic CI failure of startup-snapshot.test.ts on every snapshot-capable platform. This is exactly the case REVIEW.md's "Never silently weaken, skip, or delete an existing test" section calls out: "When changing output/defaults/messages, grep the suite for assertions on the old behavior and update them in the same PR." The message was reworded in response to review, but the one test that pinned it was not updated.

    How to fix

    Change line 989 to match the new wording:

    expect(m.out).toContain("with --snapshot=manual the app has to call Bun.startupSnapshot.take()");

    (Or, if you'd rather keep the assertion loose enough to survive future rewordings while still proving the manual-mode arm was hit, toContain("exited without taking a snapshot") plus the existing expect(m.code).toBe(1) would also do — but matching the actual user-facing message is the stronger assertion REVIEW.md prefers.)

@Jarred-Sumner
Jarred-Sumner force-pushed the claude/startup-snapshot-runtime branch from b30d888 to dd39ff5 Compare August 10, 2026 05:02
@Jarred-Sumner
Jarred-Sumner force-pushed the claude/startup-snapshot-build branch from 3f08c31 to ef5c16e Compare August 10, 2026 05:02
@Jarred-Sumner
Jarred-Sumner force-pushed the claude/startup-snapshot-runtime branch from dd39ff5 to 28517f9 Compare August 10, 2026 05:07
@Jarred-Sumner
Jarred-Sumner force-pushed the claude/startup-snapshot-build branch from ef5c16e to ef03742 Compare August 10, 2026 05:07
Comment thread test/js/bun/startup-snapshot/startup-snapshot-build.test.ts Outdated

@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.

All prior review feedback has been addressed — the last item (stale assertion at startup-snapshot-build.test.ts:357) now matches the reworded manual-mode message. No new issues found this round. Given the scope (new --snapshot/--snapshot-io CLI flags and Bun.build({ snapshot }) API, ELF payload rewriting in elf.rs, executable payload stamping in StandaloneModuleGraph.rs), a human pass is still warranted.

What was reviewed:

  • run_startup_snapshot_step error-path cleanup (executable unmarked on every failure exit) and mode-branched failure messages
  • ELF re-injection: previous_block_slot validation, in-place rewrite bounds, trailer/slot round-trip; the split-step test asserts file size does not accumulate across rewrites
  • snapshot: false handling in JSBundler.rsis_boolean() sets Auto regardless, but snapshot_requested excludes false up front so parse_startup_snapshot_options is never reached with it
  • Test assertions cross-checked against the current message strings in build_command.rs
Extended reasoning...

Overview

Part 3 of a 4-PR stack adding startup snapshots. This PR wires the build-time surface: bun build --compile --snapshot[=auto|manual] --snapshot-io=..., the standalone bun build --snapshot --outfile <exe> step, Bun.build({ snapshot }), a new docs page, and 11 new tests. Core mechanics live in StandaloneModuleGraph.rs (payload trailer stamping, snapshot appending, executable rewriting) and elf.rs (re-injecting into an ELF that already carries a payload block).

Security risks

The build step spawns the user's own freshly-built executable with inherited stdio in the user's own environment — no elevation or new trust boundary. The default --snapshot-io=strict policy refuses fs/net/spawn during the snapshot run; local/network are opt-in and reported. ELF rewriting reads/writes only the file the user named. No injection, auth, or credential surface.

Level of scrutiny

High. This is a substantial new experimental feature (~1k net LOC across 22 files) that:

  • Adds user-facing API surface (CLI flags, Bun.build option, .d.ts) — API-design decisions a maintainer should sign off on.
  • Rewrites ELF program headers and section tables in place — a bug corrupts every compiled executable on Linux.
  • Manipulates the standalone-executable payload trailer format with unaligned raw-pointer reads of repr(C) structs.

None of that fits the "simple, mechanical, obvious" bar for auto-approval.

Other factors

The PR has been through ~10 review rounds and the author has addressed every finding (help-text API name, #[inline] displacement, doc-comment contradictions, EXDEV fallback, mkdir-on-typo, signal-death reporting, mode-branched error messages, and the stale test assertion). The current diff is clean against those findings and the bug-hunting system found nothing new. Test coverage is thorough (embedded restore, auto/manual modes, split step, JS API, IO policy, stdio, signals, wasm, ELF rewrite idempotence). This should be straightforward for a human to approve, but it is not a change I can approve without one.

Comment thread test/js/bun/startup-snapshot/startup-snapshot-build.test.ts Outdated
Comment thread src/runtime/api/JSBundler.rs
Comment thread test/js/bun/startup-snapshot/startup-snapshot-build.test.ts Outdated
Comment thread src/runtime/cli/build_command.rs
Comment thread src/runtime/api/JSBundler.rs

@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.

I reviewed this revision and found no new issues — every thread from earlier rounds is resolved and the fixes are in the diff. Because this adds new user-facing API surface (--snapshot[=auto|manual], --snapshot-io, Bun.build({ snapshot })), rewrites ELF/Mach-O payloads in place, and has the build spawn the just-compiled executable, a human look is still worthwhile.

What was reviewed:

  • elf.rs rewrite path: trailer-recorded BUN_COMPILED slot, in-place vs. regrow, zero-fill ranges — the earlier off-by-8 and stale doc comment are fixed; previous_block_slot bounds-checks the slot vaddr against the RW segment.
  • StandaloneModuleGraph.rs: payload read/append with u32 offset/length now uses try_from (no silent truncation); rewrite_executable picks the injector by file magic.
  • run_startup_snapshot_step: stale-sidecar unlink, marking cleared on every error exit, mode-specific failure messages, signal-death reported as such.
  • Option parsing: snapshot requires compile (including the target: "bun-*" shorthand), rejected under standalone-HTML on both CLI and JS API, { mode/io: undefined } defaults.
Extended reasoning...

Overview

Third of four stacked PRs adding startup snapshots. This one wires the build-time surface: bun build --compile --snapshot[=auto|manual] --snapshot-io=strict|local|network, the standalone bun build --snapshot --outfile exe step, and Bun.build({ snapshot }). Native changes: elf.rs gains re-injectable .bun blocks (trailer-recorded slot vaddr, in-place overwrite when the new payload fits, otherwise regrow at the same vaddr); StandaloneModuleGraph.rs gains append_startup_snapshot_to_serialized / read_executable_payload / set_startup_snapshot_build_flags / embed_startup_snapshot_into_executable / rewrite_executable, and to_executable grows a prebuilt_payload parameter. build_command.rs gains run_startup_snapshot_step (mark → spawn → embed → clear) and the standalone-step entry; Arguments.rs and JSBundler.rs parse the new options; js_bundle_completion_task.rs runs the step for Bun.build. Plus a new docs page, bun.d.ts types, and a 488-line test file with 8 fixtures.

Security risks

The build now runs the executable it just produced (with inherited stdio) to take the snapshot; that is the feature, and it is only reached when the user explicitly asks for it. The snapshot is a raw copy of the process's memory embedded into the shipped binary — the docs page warns about this explicitly. --snapshot-io gates what the app may touch during that run (default strict refuses fs/spawn/dns/net). read_executable_payload and previous_block_slot do bounds-check header-derived offsets before slicing, and the u32 payload offset/length now go through try_from rather than as. No new attack surface on ordinary launches; the risk is the usual "whatever the build environment exposed is in the snapshot", which is a documented property of the feature rather than a defect in this PR.

Level of scrutiny

High. This is new user-facing API (CLI flags + JS option + a docs page), it manipulates ELF/Mach-O binaries at the byte level with hand-computed offsets, and it introduces a build step that executes user code. API-shape decisions (top-level snapshot vs. under compile, the auto/manual split, the io policy names) are the kind a maintainer should sign off on. The ELF re-inject logic is subtle enough that it went through several review rounds here.

Other factors

The PR has been through ~28 inline findings across earlier automated rounds (help-text typos, an ELF zero-fill off-by-8, stale-sidecar acceptance, u32 truncation, error-path marking not cleared, make_open_path creating directories on typo, undefined-valued option handling, dead timeout arg, harness-preamble duplication, the target: "bun-*" shorthand + snapshot interaction, and the standalone-HTML silent-drop on both CLI and JS API). Every thread is marked resolved and the corresponding fixes are visible in the current diff and covered by the new tests. Test coverage is broad (11 build-time tests including the split step, JS API, io policy, standalone-HTML rejection, stdio, signals, wasm, and a Linux-only forced stack-overlap case). Given the scope — new API surface, binary-format rewriting, and part 3 of a 4-PR stack — this should be approved by a human, not auto-approved.

@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.

This automated pass found nothing new; every earlier thread on this PR has been addressed and resolved. Given the scope — new public API surface (--snapshot/--snapshot-io, Bun.build({ snapshot })), the ELF .bun-block rewrite-in-place logic, and the run-once/embed step that spawns the built executable — a human sign-off on part 3 of the snapshot series is still warranted.

What was reviewed: option parsing and validation on both the CLI and Bun.build paths (including the standalone-HTML and target: "bun-<platform>" shorthand interactions, now rejected/handled on both sides); run_startup_snapshot_step's failure branches (stale sidecar unlinked first, marking cleared on every error path, signal vs. status vs. auto/manual messages); elf.rs's previous-block detection and in-place rewrite (trailer bounds-checked against the RW segment, section sh_size now the aligned block so re-injection is idempotent); and append_startup_snapshot_to_serialized's 32-bit offset/length guard.

Extended reasoning...

Overview

Third of a four-PR stack landing startup snapshots. This PR wires the build side: bun build --compile --snapshot[=auto|manual] --snapshot-io=…, the standalone bun build --snapshot --outfile <exe> step, and Bun.build({ snapshot }). Native changes span build_command.rs (the run-once/embed orchestration in run_startup_snapshot_step), Arguments.rs (flag parsing), JSBundler.rs / js_bundle_completion_task.rs (JS-API option parsing and the same snapshot step), StandaloneModuleGraph.rs (reading a compiled executable's payload back, stamping build-flag bits, appending the snapshot page-aligned, and re-emitting through to_executable with a new prebuilt_payload parameter), and exe_format/elf.rs (making write_bun_section re-entrant so an already-injected ELF can be rewritten in place instead of stacking blocks). Also a new docs page, .d.ts typing for the option, ~490 lines of new tests, and eight fixture files.

Security risks

The build step spawns the just-produced executable with inherited stdio to take its snapshot; that is the feature's whole point and is documented as such (the docs page explicitly warns that whatever the build environment exposed is in the snapshot). The default --snapshot-io=strict refuses fs/net/spawn during that run and the loosened modes report every use with call sites. read_executable_payload locates the trailer by scanning for the last TRAILER occurrence and bounds-checks byte_count against the file offset before slicing; append_startup_snapshot_to_serialized refuses payloads/snapshots whose offsets don't fit u32; previous_block_slot validates the recorded slot vaddr sits inside the RW segment's file-backed range and points back at the current block before treating the file as a rewrite target. rewrite_executable sniffs the file's own magic to pick the injector so pointing the standalone step at another OS's binary doesn't corrupt it. No new attack surface beyond what --compile already implies.

Level of scrutiny

High. This is new user-facing API on both the CLI and Bun.build, and the ELF changes alter how every Linux --compile payload block is laid out (each block now carries an 8-byte trailer recording the BUN_COMPILED slot, and sh_size is set to the page-aligned block so a subsequent rewrite can find and replace it). The Mach-O side was already re-injectable per the description; ELF is the new path. This is well past the bar for auto-approval.

Other factors

There has been an extensive back-and-forth on this PR across many rounds — every finding is resolved: signal-death reporting vs. "status -1", the outfile parent directory not being auto-created, the mode-specific no-snapshot messages and the stale test assertion after they were reworded, the EXDEV hard-link fallback in the tests, the shared startup-snapshot-harness.ts extraction (and the leftover per-test timeout), the target: "bun-<platform>" shorthand + snapshot interaction, and the standalone-HTML rejection on both the CLI and JS-API sides. The test file exercises embedded restore, auto mode, the split step re-run (asserting the file doesn't accumulate blocks), Bun.build validation (six bad configs including the shorthand and the HTML case), the io policy, main(), stdio across pipe/file/tty, signals, wasm traps, wasm tier-up quiescence, a stale-sidecar guard, and a Linux-only forced stack-overlap launch. Deferring so a maintainer signs off on the API shape and the ELF-format change as part of the four-PR series.

@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.

This automated review pass found no new issues, and every item raised in earlier rounds now looks addressed in the diff (the JS-API standalone-HTML rejection and its test case are in). Given the scope — new user-facing --snapshot surface, ELF/Mach-O payload rewriting, and a build step that runs the produced executable — a human sign-off is still warranted before this lands.

What was reviewed

  • elf.rs rewrite-in-place: trailer/slot bookkeeping, the previous_block_slot validation, and that the file never accumulates superseded blocks.
  • run_startup_snapshot_step: every failure arm clears the TAKE_STARTUP_SNAPSHOT marking and unlinks the sidecar; mode-specific messages match the test assertions.
  • CLI ↔ Bun.build parity for every rejected shape (no compile, bad values, cross-compile, standalone HTML, --snapshot-io alone) — both halves now reject with tests.
  • append_startup_snapshot_to_serialized: 4 GiB overflow guard on the 32-bit StringPointer fields.
Extended reasoning...

Overview

Third of four stacked PRs adding startup snapshots. This one wires --snapshot[=auto|manual] / --snapshot-io into bun build --compile, the standalone snapshot step (bun build --snapshot --outfile exe), and Bun.build({ snapshot }). Supporting changes: StandaloneModuleGraph.rs gains functions to stamp build flags into and embed a snapshot blob into an existing compiled executable; elf.rs learns to rewrite its own earlier output in place (or replace the block when the new payload is larger) so a file never carries more than one payload block; build_command.rs gains run_startup_snapshot_step, which marks the executable, runs it once inheriting stdio, then embeds the sidecar and cleans up on every path. A new ~490-line test file plus fixtures cover embedded restore, auto mode, the split step, the JS API including every rejected config, io policy, main(), stdio, signals, wasm, and a Linux-only forced stack-overlap launch. A new docs page and bun.d.ts entry document the feature as experimental.

Security risks

The feature has real security surface, and the docs call it out honestly: a launch that resumes from a snapshot runs with ASLR disabled; the snapshot is a copy of the process's memory, so anything the build environment exposed is baked in; --snapshot-io deliberately governs what the build-time run may touch, defaulting to strict (nothing). The build step spawns the just-produced executable with inherited stdio and the parent's environment — that is by design (the app's env/argv belong to the app; the mode is communicated through a payload marking, not env). None of these are defects in this PR, but they are exactly the kind of design trade-offs a maintainer should sign off on rather than an automated reviewer.

Level of scrutiny

High. This is new user-facing API on both the CLI and Bun.build, it rewrites ELF/Mach-O binaries in place (a mistake there corrupts every compiled executable), and it introduces a build step that executes user code. Earlier automated rounds surfaced eight distinct items (stale assertion, EXDEV fallback, dead timeout arg, duplicated harness preamble, target-shorthand + snapshot, standalone-HTML on both CLI and JS API, mode-specific error messages, directory-creation on typo), all of which the author fixed and all threads are now resolved — which raises confidence, but does not make this a candidate for auto-approval.

Other factors

The test file is thorough (variant matrix, negative contracts, io policy, re-run idempotence with a size assertion, stale-sidecar rejection). The elf.rs change is the most delicate piece; the in-place path validates the recorded slot vaddr against the RW segment and checks it currently points at the section before trusting it, and the grow path guards new_file_offset > move_src_start before the zero-fill so a rewrite that reuses the previous block's address doesn't slice with a reversed range. append_startup_snapshot_to_serialized guards the 32-bit StringPointer fields with u32::try_from and returns None on overflow, which the caller reports. I did not find anything new to flag on this pass.

@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.

I reviewed this PR and didn't find any bugs this pass — every earlier thread is resolved and the JS-API standalone-HTML rejection now matches the CLI. Because this adds ~150 lines of ELF section-rewrite logic (in-place block replacement via a new trailer word) and a build step that spawns and re-embeds into the just-built executable, a human look would still be worthwhile.

What was reviewed:

  • elf.rs rewrite path: trailer placement at aligned_new_size - 8, the previous_block_slot bounds check against p_filesz, and that sh_size = aligned_new_size keeps the trailer inside the section on the next read.
  • run_startup_snapshot_step: every failure path clears the TAKE_STARTUP_SNAPSHOT marking and unlinks the sidecar; a stale sidecar from an earlier run is unlinked before the child spawns.
  • append_startup_snapshot_to_serialized: 4 GiB u32::try_from guard on offset/length, and min_len padding for the Mach-O grow-only injector.
  • Option parsing: snapshot with the target: "bun-<platform>" shorthand and compile: false both reach the right error, and standalone-HTML rejects snapshot on both CLI and Bun.build.
Extended reasoning...

Overview

Part 3 of the startup-snapshot series: adds bun build --compile --snapshot[=auto|manual] and --snapshot-io, the split bun build --snapshot --outfile <exe> step, and Bun.build({ snapshot }). Implementation spans CLI/option parsing (Arguments.rs, context.rs, JSBundler.rs), the run-once-then-embed step (build_command.rs::run_startup_snapshot_step, js_bundle_completion_task.rs), payload stamping/embedding (StandaloneModuleGraph.rs: append_startup_snapshot_to_serialized, set_startup_snapshot_build_flags, embed_startup_snapshot_into_executable, rewrite_executable, and a new prebuilt_payload argument to to_executable), and ELF re-injection support (exe_format/elf.rs). A new docs page, a .d.ts addition, seven fixtures and a ~490-line test file cover embedded restore, auto/manual modes, the split step, io policy, main(), stdio, signals, wasm, tier-up quiescence, stale sidecars, the standalone-HTML rejection, and a Linux-only forced stack-overlap launch.

Security risks

The build step spawns the freshly compiled executable with inherited stdio and the parent's environment; the docs explicitly warn that whatever the build environment exposes ends up in the snapshot. --snapshot-io=strict (the default) refuses fs/spawn/net/dns during that run — enforcement lives in the runtime PR (#37260), so this PR trusts the marking bits it stamps into the payload. rewrite_executable sniffs the file's own magic to pick the injector, so pointing the split step at another OS's executable does not corrupt it. read_executable_payload scans for the last TRAILER in the file and bounds-checks byte_count before slicing; previous_block_slot validates the recorded slot vaddr sits inside the RW segment's file-backed range and currently points at the block before treating the file as a rewrite target. No auth/crypto/permissions surfaces are touched.

Level of scrutiny

High. The ELF changes alter how a payload block is placed on repeated writes (new trailer word, in-place overwrite when it fits, segment regrow at the same vaddr otherwise, sh_size now set to the page-aligned block so the next read finds the trailer). Getting an offset wrong here produces a broken binary rather than a test failure. StandaloneModuleGraph.rs gains ~220 lines of unaligned-read/serialize logic over the trailer format. The feature is experimental and platform-gated (macOS + glibc Linux), which bounds blast radius, but the inject path is shared with every --compile build.

Other factors

This PR has been through many review rounds; every prior inline thread is marked resolved and the diff reflects each fix (mode-specific no-snapshot messages, signal-death reporting via SpawnStatus::code() == -1, no-create on the split step's directory open, the shared startup-snapshot-harness.ts, the bun-<platform> target-shorthand path through CompileOptions::from_js, and the standalone-HTML rejection on both CLI and JS API with tests for each). Test coverage is broad and asserts specific error text and exit codes. No bugs were found this pass. Deferring solely on scope: new user-facing API, binary-format rewriting, and cross-platform executable manipulation are the kind of change a maintainer should sign off on.

@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.

This automated pass ran clean — no new findings, and every earlier thread is resolved in the current diff. Given the scope (new user-facing --snapshot/Bun.build({ snapshot }) surface, the ELF in-place-rewrite path, and a new docs page), a human sign-off on the API shape and the binary-format changes is still worth having.

What was reviewed:

  • elf.rs rewrite path — trailer/slot bookkeeping, the in-place-fits branch, and that a fresh template still takes the append path (previous_block_slot returns None on the linker's 8-byte .bun).
  • run_startup_snapshot_step error exits — every failure arm now unmarks the executable and unlinks the sidecar; stale-sidecar and signal-death cases covered by tests.
  • Option parsing on both CLI and Bun.build sides, incl. the target: "bun-<platform>" shorthand and the standalone-HTML rejection — both now symmetric and tested.
  • append_startup_snapshot_to_serialized — the u32 offset/length are checked with try_from, so >4 GiB payloads fail with a message rather than truncating.
Extended reasoning...

Overview

Third of a four-PR stack adding startup snapshots. This PR wires the build-time surface: bun build --compile --snapshot[=auto|manual] --snapshot-io=..., the standalone bun build --snapshot --outfile <exe> step, Bun.build({ snapshot }), and a new docs page. The load-bearing native changes are in src/exe_format/elf.rs (making write_bun_section idempotent so an executable can be rewritten repeatedly without accumulating dead blocks), src/standalone_graph/StandaloneModuleGraph.rs (reading a compiled executable's payload, stamping build flags into it, appending a snapshot, and re-emitting through the existing inject/sign path), and src/runtime/cli/build_command.rs (run_startup_snapshot_step: mark → spawn once → embed → unmark, with every failure exit restoring the executable to an ordinary state). CLI parsing, JSBundler.rs option parsing, bun.d.ts, and a ~490-line test file round it out.

Security risks

The build step spawns the freshly-built executable with inherited stdio; the --snapshot-io policy (enforced by the runtime from PR #37260, not this PR) gates what that run may touch, defaulting to strict. read_executable_payload scans an on-disk file for the TRAILER magic and reads a repr(C) Offsets struct at an unaligned offset — bounds are checked (tpos < size_of::<Offsets>, byte_count > offsets_pos), and read_unaligned is used, so a malformed file produces an error rather than UB. The ELF rewrite path validates the recovered slot vaddr sits inside the RW segment and currently points at the block before treating the file as previously-written. No auth/crypto/permissions code is touched.

Level of scrutiny

High. This is new public API (CLI flags, a Bun.build option, a docs page) and it modifies the ELF injector — a code path where a mistake produces an executable that segfaults at exec time on a subset of platforms. That is exactly the class of change the approval guidelines say a human should weigh in on. The PR has already been through many rounds of automated review (~20 findings, all addressed and resolved), and this run found nothing new, but the API-design and docs-wording calls are not something an automated pass should sign off on alone.

Other factors

Test coverage is thorough for the surface added here: embedded restore, auto vs. manual, the split step re-run (asserting the file does not grow), the JS API's rejected shapes (including the target-shorthand and standalone-HTML cases added in response to review), io policy under strict and local, main(), stdio across three build/launch tty combinations, signals, wasm and the tier-up race, stale sidecars, and a Linux-only forced stack-overlap launch. Every prior inline thread is marked resolved, and I spot-checked that the fixes are actually in the current diff (the manual-mode message assertion, the compile_target.is_some() shorthand branch in CompileOptions::from_js, the standalone-HTML rejection in both build_command.rs and JSBundler.rs, the #[inline] on write_u64_le, the sidecar unlink before spawn). to_executable gained a prebuilt_payload parameter; both pre-existing call sites pass None.

@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.

I reviewed this revision and the bug-hunting pass found nothing new; every earlier thread is resolved in the current diff. Given the size of the surface added here — new --snapshot/--snapshot-io flags and Bun.build({ snapshot }), the ELF .bun-section rewrite path, and a build step that spawns the compiled executable — a human look is still worthwhile before this lands.

What was reviewed:

  • elf.rs rewrite-in-place: trailer/slot bookkeeping, the previous_block_slot bounds checks, and that repeated rewrites don't stack blocks (covered by the split-step test).
  • run_startup_snapshot_step: sidecar unlink ordering, every failure branch clears the TAKE_STARTUP_SNAPSHOT marking, and mode-specific error messages match the test assertions.
  • CLI ↔ Bun.build parity for every rejected shape (no compile, standalone HTML, cross-compile, bad mode/io, target: "bun-<platform>" shorthand) — each pair now lands in the same validation layer with a test.
Extended reasoning...

Overview

Part 3 of a 4-PR stack adding startup snapshots. This PR wires the build-time surface: bun build --compile --snapshot[=auto|manual] --snapshot-io=..., the standalone bun build --snapshot --outfile <exe> step, Bun.build({ snapshot }), and a new docs page. The load-bearing native changes are in src/exe_format/elf.rs (rewriting a previously-injected .bun section in place so repeated snapshot embeds don't accumulate), src/standalone_graph/StandaloneModuleGraph.rs (reading an existing executable's payload trailer, appending a page-aligned snapshot to it, re-emitting through to_executable with a prebuilt_payload), and src/runtime/cli/build_command.rs (run_startup_snapshot_step: mark the exe, spawn it once with inherited stdio, embed the sidecar, clear the marking on every exit path). Arguments.rs/JSBundler.rs/context.rs are option plumbing; js_bundle_completion_task.rs calls the same step from the JS API. ~490 lines of new tests plus eight fixtures.

Security risks

The build step runs the freshly-compiled executable with inherited stdio and the parent's environment (spawn_sync_inherit). That executable is the user's own code, so this is equivalent to bun run — not a new trust boundary — but it does mean a bun build --snapshot in CI executes the app once, which the docs call out ("take snapshots in an environment that holds nothing you would not ship"). The default --snapshot-io=strict refuses fs/net/spawn inside the snapshotted process, but that policy is enforced by the runtime PR (#37260), not here. The ELF rewrite path does bounded pointer arithmetic on a file the user controls; previous_block_slot validates the recorded slot vaddr against the RW segment's p_vaddr..p_vaddr+p_filesz and cross-checks that the slot currently points at the block before trusting it, and read_executable_payload bounds-checks offsets.byte_count against the trailer position. append_startup_snapshot_to_serialized rejects payloads whose offset/length don't fit in the trailer's u32 fields. I did not find an unchecked index on adversarial input.

Level of scrutiny

High. This is a substantial new user-facing feature with executable-format manipulation (page-aligned offsets, section-header rewrites, PT_LOAD growth) where an off-by-one corrupts every compiled binary that goes through the snapshot step. It also introduces API-design decisions (flag names, snapshot: true | { mode, io } shape, the split-step CLI form) that a maintainer should sign off on. The PR has already been through ~20 rounds of automated review with every finding fixed and tested, which raises confidence in correctness, but does not substitute for a human weighing the design and the ELF logic.

Other factors

Test coverage is thorough for what this PR owns: embedded restore with a data-segment-size ceiling, auto vs manual, the split step re-run (asserts the file doesn't grow), Bun.build with every rejected config counted, io-policy strict vs local, main(), stdio replacement across three fd-kind transitions, signals, wasm traps, wasm tier-up quiescing, stale sidecars, and a Linux-only forced stack-overlap launch. All prior review threads are marked resolved and each fix is visible in the current diff (harness extraction, mode-specific messages, standalone-HTML rejection on both CLI and JS API, target: "bun-<platform>" shorthand reaching snapshot validation, dead 60_000 arg removed). No outstanding reviewer comments. The base is another branch in the stack, so this cannot land independently anyway.

@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.

I reviewed this round and the bug-hunting pass came back clean; every earlier thread is resolved. Given the scope — new user-facing API surface (--snapshot, --snapshot-io, Bun.build({ snapshot })), the ELF in-place block-rewrite logic, and payload serialization changes in StandaloneModuleGraph — a human sign-off is still worthwhile.

What was reviewed:

  • ELF write_bun_section rewrite path: trailer/slot bookkeeping, the previous_block in-place vs. regrow branches, and that sh_size = aligned_new_size keeps the fits-in-place check consistent across rewrites.
  • run_startup_snapshot_step: sidecar unlink-before-run, marking cleared on every failure path, mode-specific messages match the test assertions.
  • Bun.build option parsing: the target: "bun-<platform>" shorthand + snapshot interaction and the standalone-HTML rejection on both CLI and JS-API sides.
  • append_startup_snapshot_to_serialized: 32-bit offset/length overflow guard and min_len padding for Mach-O grow-only rewrites.
Extended reasoning...

Overview

Part 3 of a 4-PR stack adding startup snapshots. This PR wires the build-time surface: bun build --compile --snapshot[=auto|manual] --snapshot-io=..., the standalone bun build --snapshot --outfile <exe> step, and Bun.build({ snapshot }). Production code spans build_command.rs (the run-once step, ~150 lines), Arguments.rs (flag parsing), JSBundler.rs / js_bundle_completion_task.rs (JS-API option parsing and dispatch), StandaloneModuleGraph.rs (~220 lines: reading/rewriting an executable's payload, appending a snapshot page-aligned, stamping build-marking flags), exe_format/elf.rs (~80 lines: teaching write_bun_section to detect and replace its own earlier block so repeated rewrites don't accumulate), plus two new option enums, a .d.ts addition, and a new docs page. Tests add ~490 lines across a new build test file and eight fixtures.

Security risks

The snapshot step spawns the just-built executable with inherited stdio and the build's environment; the docs correctly warn that the snapshot is a copy of process memory and will contain whatever the build environment exposed. The --snapshot-io policy (default strict) is enforced by the runtime PR (#37260), not here — this PR only stamps the marking bits into the payload so the executable knows which policy to apply. read_executable_payload scans the file for the last TRAILER occurrence and bounds-checks byte_count before slicing; append_startup_snapshot_to_serialized guards the 32-bit StringPointer fields with u32::try_from. previous_block_slot validates that the recorded slot vaddr lies within the RW segment's p_filesz and currently points at the block before treating the file as a rewrite. None of this handles untrusted network input, but the ELF rewrite path is the kind of byte-offset arithmetic where an off-by-one corrupts a shipped binary, so it warrants a maintainer's eye.

Level of scrutiny

High. This is net-new user-facing API (CLI flags, a Bun.build option, a docs page) with design choices a maintainer should ratify — flag naming, the strict|local|network policy vocabulary, whether snapshot is top-level on BuildConfig vs. under compile, and the exact wording of the seven distinct failure messages in run_startup_snapshot_step. The ELF rewrite change alters how every bun build --compile output on Linux is laid out (the trailing slot-vaddr word is now always written, and sh_size is now the page-aligned block size rather than the exact content size), which is behaviour-preserving for the runtime reader but changes the on-disk shape.

Other factors

The PR has been through ~15 review rounds with the author; every thread is resolved and each fix landed with a test (target-shorthand + snapshot, standalone-HTML rejection on both entry points, stale-sidecar unlink, mode-specific failure messages, the harness extraction). The test file exercises the embedded-restore round trip, auto vs. manual, the split step with re-run (asserting the file doesn't grow), the JS API's rejected-config matrix, io policy, main(), stdio replacement, signals, wasm, and a Linux-only forced stack-overlap case. Coverage looks thorough for what this PR adds; the runtime mechanics it depends on live in #37260.

@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.

I reviewed this PR again and found no new issues — every prior inline comment has been addressed. Because this adds a new user-facing surface (--snapshot, Bun.build({ snapshot })) and reworks ELF payload rewriting plus spawns the built executable during the build, a human look would still be worthwhile.

What was reviewed:

  • elf.rs rewrite-in-place: trailer/slot bookkeeping, bounds on the in-place overwrite path, and that repeated rewrites don't stack blocks — covered by the split-step size assertion.
  • run_startup_snapshot_step: every failure arm clears the TAKE_STARTUP_SNAPSHOT marking and unlinks the sidecar; stale-sidecar and signal-death cases tested.
  • CLI/JS-API parity for validation (no-compile, standalone-HTML, cross-compile, bad enum values, bun-<platform> shorthand) — each has a matching rejection and test.
  • append_startup_snapshot_to_serialized: 4 GiB u32 overflow guarded via try_from; padding/alignment math checked against Offsets layout.
Extended reasoning...

Overview

Third of four stacked PRs for startup snapshots. Adds the build-time surface: bun build --compile --snapshot[=auto|manual] --snapshot-io=..., the standalone bun build --snapshot --outfile exe step, and Bun.build({ snapshot }). Touches build_command.rs (~200 new lines including run_startup_snapshot_step), Arguments.rs (flag parsing), JSBundler.rs / js_bundle_completion_task.rs (JS API), StandaloneModuleGraph.rs (~220 new lines: reading/rewriting executable payloads, appending snapshots), exe_format/elf.rs (~80 lines: making write_bun_section re-entrant so a file can be rewritten in place without accumulating blocks), context.rs (two new option enums), bun.d.ts, a new docs page, and a ~490-line test file with eight fixtures.

Security risks

The build step spawns the freshly-built executable (spawn_sync_inherit), which runs arbitrary user code as part of bun build. That is the feature's design and is documented ("Take snapshots in an environment that holds nothing you would not ship"); the default --snapshot-io=strict is enforced by the runtime PR (#37260), not this one. read_executable_payload scans an on-disk file for the TRAILER magic and reads a repr(C) Offsets struct at an unaligned offset — bounds are checked (tpos < size_of::<Offsets>(), byte_count > offsets_pos), and the ELF rewriter's new previous_block_slot validates the recorded slot vaddr sits inside the RW segment and currently points at the block before trusting it. No injection/auth surface; the rewrite_executable magic-byte sniff falls back to Mac for anything not ELF/PE, which is fine because the Mach-O injector will reject a non-Mach-O file rather than corrupt it.

Level of scrutiny

High. This is new user-facing API (CLI flag + Bun.build option + a docs page), it modifies executable-format handling (ELF section/segment growth now supports in-place rewrite), and it changes to_executable's signature. It has been through ~10 rounds of inline review on this PR already — every thread is resolved and the fixes are visible in the current diff (mode-specific error messages, standalone-HTML rejection on both CLI and JS API, bun-<platform> shorthand honoring snapshot, harness dedup, stale-sidecar unlink, signal-death wording). But the shape of the API and the ELF-rewrite invariants are the kind of thing a maintainer should sign off on.

Other factors

Test coverage is thorough for a feature PR: embedded restore, auto mode, split step with re-run and file-size non-growth assertion, JS-API bad-config matrix (six rejected shapes counted by error message), IO policy (strict fails / local reports), main(), stdio across three fd-kind combinations, signals, wasm + wasm tier-up quiescence, stale sidecar, and a Linux-only forced stack-overlap launch. All snapshot tests are gated by the shared snapshotTest/withSnapshots harness so unsupported lanes skip. The PR description reports the file passes on its own binary (21 pass, 1 Linux-only skip on macOS).

@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.

This round of automated review ran clean — every thread from earlier rounds is resolved and no new issues were found. Given the scope (new user-facing --snapshot / Bun.build({ snapshot }) surface, ELF PT_LOAD/section-header rewriting for re-injectable payloads, and a build step that spawns the compiled executable), a human look before merge is still warranted.

What was reviewed:

  • elf.rs re-inject path: trailer/slot validation, in-place vs. regrow, and that repeated rewrites don't stack blocks or corrupt BSS zero-fill.
  • run_startup_snapshot_step: every failure path clears the TAKE_STARTUP_SNAPSHOT marking and the stale sidecar; mode-specific messages; signal-death vs. exit-code reporting.
  • CompileOptions::from_js / standalone-HTML: the target: "bun-<platform>" shorthand + snapshot combination now parses into the shorthand's options and reaches the cross-compile message; the JS-API standalone-HTML rejection matches the CLI's.
  • append_startup_snapshot_to_serialized: 4 GiB overflow guard on the u32 trailer fields, alignment padding, and min_len handling for Mach-O grow-only.
Extended reasoning...

Overview

Third of a four-PR stack adding startup snapshots. This PR wires the build-time surface: bun build --compile --snapshot[=auto|manual] --snapshot-io=…, the standalone bun build --snapshot --outfile exe step, and Bun.build({ snapshot }). It touches ELF payload injection (exe_format/elf.rs) to support rewriting an executable that already carries a payload, adds payload-format helpers in StandaloneModuleGraph.rs (append snapshot, set/clear build flags, re-embed), plumbs two new option enums through context.rs / Arguments.rs / JSBundler.rs / js_bundle_completion_task.rs, and adds run_startup_snapshot_step in build_command.rs which spawns the freshly-built executable, waits for it to write a sidecar, and embeds it. ~1 k lines of new code across 21 files, plus a 488-line test file and a new docs page.

Security risks

The build step executes the just-compiled user program with inherited stdio; that is the point of the feature, and the io-policy default (strict) refuses fs/net/spawn during the run — enforced in the runtime PR (#37260), not here. The ELF rewriter reads section-header offsets from the file and uses them to compute slice indices; previous_block_slot bounds-checks the trailer against p_filesz and validates the slot points back at the current block before trusting it, and the fits-in-place branch only writes within sh_size. read_executable_payload bounds-checks byte_count against offsets_pos. I did not find a way for a malformed input to write outside self.data, but binary-format code of this kind is where a second pair of eyes is most valuable.

Level of scrutiny

High. This is new user-facing API (CLI flags, a Bun.build option, a docs page), it rewrites executable files in place, and it changes the on-disk payload format (adds a trailer word to every ELF .bun block and a snapshot field to Offsets). API-shape decisions — the snapshot top-level option vs. nesting under compile, the --snapshot-io policy names, the re-runnable in-place step — are the kind REVIEW.md flags for maintainer sign-off.

Other factors

Over ~10 review rounds every issue I raised was addressed (stale-assertion, mode-specific error messages, EXDEV fallback, dead 60_000 arg, harness dedup, target: "bun-<platform>" + snapshot interaction, standalone-HTML silently dropping the flag on both CLI and JS API, signal-death reporting, non-creating dir open). Test coverage is thorough — auto/manual, split step, JS-API rejections, io-policy, stdio/signal/wasm restore, stale sidecars, and a Linux-only forced stack-overlap case. All threads are resolved and this run's bug hunt found nothing new; the remaining reason not to auto-approve is scope, not open concerns.

@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.

I reviewed this PR across several rounds and every issue raised has been addressed; this run found nothing new. Given the scope — new public --snapshot / Bun.build({ snapshot }) API, ELF payload-rewriting (write_bun_section now handles re-injection), and a build step that spawns the compiled executable — a human look is still warranted before merge.

What was reviewed:

  • elf.rs re-inject path: trailer/slot validation, in-place overwrite bounds, and that the segment-regrow path reuses the previous block's vaddr without accumulating.
  • run_startup_snapshot_step: every failure arm clears the TAKE_STARTUP_SNAPSHOT marking and the stale-sidecar unlink runs before the spawn.
  • Option parsing symmetry between CLI and Bun.build (target-shorthand + snapshot, standalone-HTML rejection, cross-compile message) — now covered by tests on both sides.
Extended reasoning...

Overview

Third PR in a four-part stack adding startup snapshots. This one wires the build-time surface: bun build --compile --snapshot[=auto|manual] --snapshot-io=..., the standalone bun build --snapshot --outfile exe step, and Bun.build({ snapshot }). Native changes span build_command.rs (the run-once/embed step, ~150 new lines), StandaloneModuleGraph.rs (~220 lines: reading a compiled executable's payload, appending a page-aligned snapshot, rewriting via the normal inject path), elf.rs (~80 lines: making write_bun_section re-entrant so repeated rewrites replace rather than stack), Arguments.rs/JSBundler.rs/context.rs (flag/option plumbing), plus a new docs page, .d.ts typings, and a ~490-line test file with eight fixtures.

Security risks

The build step spawns a freshly compiled executable with inherited stdio and the user's environment — expected for a --compile build tool, and the default --snapshot-io=strict policy has the runtime refuse fs/net/spawn during that run. rewrite_executable sniffs the file's own magic to pick the injector rather than trusting the host OS, so pointing the split step at a foreign binary fails cleanly. read_executable_payload bounds-checks offsets.byte_count against the file before slicing. No auth/crypto/permission surfaces are touched.

Level of scrutiny

High. This is new user-facing API (CLI flags, a Bun.build option, a docs page) and it modifies the ELF injector's core write path — a path every Linux --compile build goes through, whether or not --snapshot is used. The re-inject logic (trailer word → slot vaddr → in-place vs. regrow) is subtle enough that I would not want it to land on automated review alone, and the API shape (mode/io names, the split-step UX, the docs page's framing) is the kind of design surface a maintainer should sign off on.

Other factors

The PR has been through roughly fifteen rounds of inline findings on this thread, every one addressed with a fix and (where behavioral) a test: signal-death reporting, stale-sidecar handling, mode-specific failure messages, the target: "bun-<platform>" shorthand interaction, the standalone-HTML silent-drop on both CLI and JS API, the harness extraction, and the dead per-test timeout. Test coverage is broad (embedded restore with a data-segment size assertion, auto mode, split step re-run, JS-API bad-config matrix, io policy, main(), stdio in three shapes, signals, wasm and its tier-up race, stale sidecar, and a Linux-only forced stack-overlap case). The feature is gated behind an explicit flag and marked experimental in docs. All of that argues the change is in good shape — it just exceeds the complexity threshold for auto-approval.

@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.

I reviewed this round and found no new issues — every prior finding has been addressed and the JS-API standalone-HTML rejection now matches the CLI. Given the scope (new user-facing --snapshot flags and Bun.build({ snapshot }) surface, ELF payload-rewrite logic in elf.rs, and a build step that spawns the produced executable), a human sign-off on the feature is still worthwhile.

What was reviewed:

  • ELF write_bun_section rewrite path: in-place overwrite when the new payload fits, segment-regrow otherwise; trailer/slot bookkeeping and the previous_block_slot validation keep a single block across repeated rewrites.
  • run_startup_snapshot_step: marking → spawn → embed sequencing, with the executable reset to an ordinary state on every failure branch and stale sidecars unlinked before the run.
  • CLI/JS-API parity for the snapshot options, including the standalone-HTML and target: "bun-<platform>" shorthand paths, and the mode-specific failure messages.
Extended reasoning...

Overview

Third of a four-PR stack adding startup snapshots. This PR wires the build-time surface: bun build --compile --snapshot[=auto|manual] --snapshot-io=..., the standalone bun build --snapshot --outfile exe step, and Bun.build({ snapshot }). Rust changes span build_command.rs (~200 new lines: run_startup_snapshot_step, flag validation, three call sites), StandaloneModuleGraph.rs (~220 lines: payload trailer read/rewrite, set_startup_snapshot_build_flags, embed_startup_snapshot_into_executable, to_executable gaining a prebuilt_payload parameter), elf.rs (~80 lines: making write_bun_section re-runnable on its own output), Arguments.rs/JSBundler.rs/context.rs (option parsing), plus a new docs page, bun.d.ts typings, and a 488-line test file with seven fixtures.

Security risks

The snapshot step spawns the just-built executable with inherited stdio and the build's environment. The docs explicitly warn that whatever the build environment exposed ends up frozen into the snapshot; the default --snapshot-io=strict refuses fs/spawn/net during the run, and local/network are opt-in with per-use reporting. The ELF rewriter reads offsets from the file's own section headers and validates the trailer slot lands inside the RW segment before dereferencing it (previous_block_slot); read_executable_payload bounds-checks offsets.byte_count against the trailer position and append_startup_snapshot_to_serialized refuses payloads whose offset/length overflow the 32-bit trailer fields. Nothing here touches auth, crypto, or permissions; the risk profile is more about binary-format correctness than a security boundary.

Level of scrutiny

High. This is new user-facing API surface (a documented CLI flag pair and a Bun.build option), it modifies executable files in place, and write_bun_section is load-bearing for every --compile build on Linux — a regression there would break compiled executables generally, not just snapshots. That warrants a maintainer's eye on the design (flag names, io-policy defaults, the split-step workflow) and on the ELF rewrite invariants.

Other factors

The PR has been through many review rounds on this thread; every inline comment (stale assertions, the target: "bun-<platform>" shorthand path, the standalone-HTML CLI+JS-API pair, the harness extraction, the dead timeout arg) is resolved and reflected in the current diff. Test coverage is broad: embedded restore with the copied-data-segment size assertion, the split step re-run with a file-size check that superseded blocks don't accumulate, Bun.build bad-config rejections including the HTML case, io-policy strict vs local, mode-specific failure messages, main(), stdio, signals, wasm, tier-up quiescence, stale sidecars, and a Linux-only forced stack-overlap launch. The bug-hunting pass on this revision found nothing new. I have not previously posted an approve/defer body on this PR — only inline findings — so this is the first summary.

@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.

I reviewed this round and found no new issues — all seven earlier findings have been addressed. Given the scope (new user-facing --snapshot / Bun.build({ snapshot }) surface, the ELF in-place block-replacement logic in elf.rs, and the payload-rewrite path in StandaloneModuleGraph.rs), a human pass is still warranted.

What was reviewed:

  • elf.rs rewrite path: trailer/slot bookkeeping, the previous_block fit-in-place vs. regrow branch, and that sh_size = aligned_new_size keeps the trailer inside the section on re-read.
  • append_startup_snapshot_to_serialized / embed_startup_snapshot_into_executable: alignment padding, the u32 offset/length overflow guard, and min_len padding for the Mach-O grow-only injector.
  • run_startup_snapshot_step: every failure arm clears the TAKE_STARTUP_SNAPSHOT marking and unlinks the sidecar; stale-sidecar unlink happens before the run.
  • Option parsing parity between CLI and Bun.build (target-shorthand, compile: false, standalone-HTML) — now symmetric and covered by tests.
Extended reasoning...

Overview

Third of a four-PR stack adding startup snapshots. This PR wires the build-time surface: bun build --compile --snapshot[=auto|manual] --snapshot-io=..., the standalone bun build --snapshot --outfile exe step, and Bun.build({ snapshot }). It touches Arguments.rs/build_command.rs (flag parsing, the run-once step), JSBundler.rs/js_bundle_completion_task.rs (JS API), StandaloneModuleGraph.rs (stamping build-mode flags into the payload trailer, appending the snapshot, rewriting an existing executable), exe_format/elf.rs (re-injecting into an ELF that already carries a payload — new trailer-tracked slot so repeated rewrites replace rather than accumulate), plus a new docs page, bun.d.ts, and a 488-line test file with eight fixtures.

Security risks

The build step spawns the freshly compiled executable with inherited stdio and the parent's environment. The --snapshot-io policy (default strict) is enforced by the runtime (PR #37260), not here; this PR only passes the flag bits through the payload trailer. read_executable_payload scans for the TRAILER byte sequence and validates byte_count <= offsets_pos before slicing; append_startup_snapshot_to_serialized guards the u32 offset/length with try_from. previous_block_slot bounds-checks the trailer read and validates the slot vaddr sits inside the RW segment's file-backed range and currently points at the block's own vaddr before treating it as a rewrite. No new network or credential surface.

Level of scrutiny

High. elf.rs is the injector for every Linux --compile build; the new rewrite path changes how sh_size is recorded (now aligned_new_size, so the trailer word at cap - 8 is inside the section on the next read) and adds a branch that overwrites the block in place when the new payload fits. A mistake here corrupts compiled executables. StandaloneModuleGraph.rs gains a re-serialization path that reads the trailer out of an arbitrary file by scanning for a magic string. The CLI/JS-API option parsing has already gone through several review rounds on this PR (target-shorthand + snapshot, standalone-HTML rejection on both sides, mode-specific error messages) and is now covered by tests that count which validation error each config hits.

Other factors

Seven prior inline findings from earlier rounds are all resolved and tested. Test coverage is thorough for the option surface and the round-trip (embedded restore, split step, re-run, io policy, stdio/signal/wasm survival, stale sidecar, forced stack-overlap). The ELF rewrite-in-place path is exercised by the "can be re-run" test's size assertion (file does not grow on the second embed). This is new user-facing API on an experimental feature, part of a stacked series the maintainer is actively driving — deferring rather than approving.

@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.

I reviewed this revision and the bug-hunting pass found nothing new; every earlier thread has been addressed. Given the scope — new user-facing API surface (--snapshot, Bun.build({ snapshot })), ELF payload rewriting in elf.rs, and the run-once-then-embed step that spawns the built executable — a human sign-off is still worthwhile.

What was reviewed:

  • elf.rs in-place rewrite path: previous_block_slot validation, the new_content_size <= bun_section.size overwrite branch, and that repeated rewrites don't accumulate blocks.
  • run_startup_snapshot_step: every failure arm clears the TAKE_STARTUP_SNAPSHOT marking and unlinks the sidecar; signal death vs non-zero vs exit-0-no-snapshot messages branch on mode.
  • StandaloneModuleGraph payload append/strip: 32-bit offset/length overflow is rejected rather than truncated; magic-byte dispatch in rewrite_executable picks the injector by file format, not host.
  • CLI/JS-API parity: --snapshot rejected for standalone-HTML on both surfaces; target: "bun-<platform>" shorthand now carries snapshot options through instead of dropping them.
Extended reasoning...

Overview

Third of a four-PR stack adding startup snapshots. This PR wires the build-time surface: bun build --compile --snapshot[=auto|manual] --snapshot-io=…, the standalone bun build --snapshot --outfile exe step, Bun.build({ snapshot }), and a new docs page. Native changes: ~150 lines in build_command.rs (run_startup_snapshot_step — mark the executable, spawn it once, read the sidecar, embed it, clear the marking on every failure path), ~220 lines in StandaloneModuleGraph.rs (append_startup_snapshot_to_serialized, read_executable_payload, set_startup_snapshot_build_flags, embed_startup_snapshot_into_executable, rewrite_executable, plus a new prebuilt_payload parameter on to_executable), ~80 lines in elf.rs (in-place rewrite of a previously-injected .bun block so re-snapshotting doesn't stack payloads), option parsing in JSBundler.rs/Arguments.rs/context.rs, and a 488-line test file with eight fixtures.

Security risks

The build step spawns an executable it just produced (with the parent's environment inherited via spawn_sync_inherit) and then rewrites that executable's ELF/Mach-O payload in place. read_executable_payload does bounds-check offsets.byte_count against the trailer position, and append_startup_snapshot_to_serialized refuses when the offset or length won't fit in the trailer's u32 fields. previous_block_slot validates the recorded slot vaddr sits inside the RW segment's file-backed range and currently points at the block before trusting it. The default --snapshot-io=strict policy refuses fs/net/spawn during the snapshot run. None of this is auth/crypto, but it is binary-format manipulation of the shipped artifact, which is exactly the kind of thing a maintainer should eyeball.

Level of scrutiny

High. This is new user-facing API (CLI flags, a Bun.build option, a docs page), it touches the compiled-executable payload format, and elf.rs is load-bearing for every --compile build on Linux. It is not a mechanical change.

Other factors

The PR has been through roughly twenty rounds of inline review on this thread, all of which the author addressed and resolved (stale test assertion, ELF rewrite correctness, standalone-HTML silently dropping --snapshot on both CLI and JS API, the target: "bun-…" shorthand path, harness deduplication, dead per-test timeout, signal-death reporting, sidecar hygiene). The current revision passes the bug-hunting system with no findings. Test coverage is broad: embedded restore, auto mode, the split step, Bun.build with every rejected shape, io-policy strict vs local, main(), stdio replacement, signals, wasm, wasm tier-up quiescence, stale sidecars, and a Linux-only forced stack-overlap launch. That said, the ELF in-place rewrite path and the payload strip-and-re-append logic are subtle enough that maintainer review is the right call before this lands as part of the stack.

…bed the snapshot it takes; the snapshot step on its own (bun build --snapshot --outfile exe); Bun.build({ snapshot }); docs

@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.

I reviewed this PR again and found no new issues — every prior inline finding (28 threads across the ELF rewriter, payload trailer handling, CLI/JS-API validation parity, error-path cleanup, and test harness) has been addressed and resolved. Given the scope — new user-facing API surface, ELF/Mach-O in-place rewriting, and a build step that executes the compiled binary — a human sign-off is still worthwhile.

What was reviewed: the ELF re-injection path (in-place overwrite vs. regrow, trailer/slot round-trip, BSS zero-fill guard); append_startup_snapshot_to_serialized bounds and u32 truncation guard; run_startup_snapshot_step error paths always clearing the take-a-snapshot marking and unlinking stale sidecars; CLI ↔ Bun.build validation parity including the standalone-HTML and target: "bun-<platform>" shorthand cases.

Extended reasoning...

Overview

Third of four stacked PRs adding startup snapshots. This one wires bun build --compile --snapshot[=auto|manual] --snapshot-io=... and Bun.build({ snapshot }): after producing the executable, run it once with a marking in its payload, embed the resulting snapshot in place, and clear the marking. Touches elf.rs (re-injecting into an ELF that already carries a payload — overwrite in place when it fits, otherwise replace at the same vaddr so repeated rewrites never accumulate), StandaloneModuleGraph.rs (payload trailer stamping, appending a page-aligned snapshot, reading/rewriting an existing executable's payload), build_command.rs / Arguments.rs / JSBundler.rs / js_bundle_completion_task.rs (flag parsing, the run-once step, exit-code and mode-specific error reporting, cross-compile and standalone-HTML rejections), plus a new docs page, a bun.d.ts addition, and a 488-line test file with fixtures.

Security risks

The build step spawns and runs the just-compiled executable with inherited stdio; the --snapshot-io policy (enforced by the runtime PR, not this one) gates what that process may touch. The executable is derived from the user's own entrypoints, so this is not an untrusted-input surface, but it does mean bun build now executes user code by design. read_executable_payload scans an arbitrary file for the TRAILER magic and reads an unaligned Offsets struct from before it — bounds are checked (tpos < size_of::<Offsets>(), offsets.byte_count > offsets_pos), and rewrite_executable sniffs the file's own magic to pick the injector rather than trusting the host OS. The ELF previous_block_slot validates that the recorded slot vaddr sits inside the RW segment and currently points at the block before treating the file as a rewrite target. No auth/crypto/permissions code is touched.

Level of scrutiny

High. This is new user-facing API (CLI flags, a Bun.build option, a docs page) on top of binary-format manipulation where an off-by-one corrupts every compiled executable. The ELF path in particular now has two write shapes (in-place and regrow) that must both keep the BUN_COMPILED slot, the section header, the PT_LOAD extent, and the trailer consistent. The feature is gated experimental and platform-limited (macOS + glibc Linux), which bounds blast radius, but the to_executable signature change (prebuilt_payload: Option<&[u8]>) and the .bun section trailer touch every --compile build regardless of --snapshot.

Other factors

This PR has been through many review rounds — 28 inline threads from prior automated passes covering stale sidecars passing for a fresh snapshot, u32 truncation of snapshot offsets, error exits leaving the executable in take-a-snapshot mode, the ELF trailer/padding zero-fill ordering, CLI vs. JS-API validation parity (target shorthand, standalone HTML), signal-death reporting, and test-harness deduplication — all confirmed fixed by the author and marked resolved. Test coverage is thorough (embedded restore, split step re-runnable without file growth, every rejected config shape on both CLI and JS API, io policy, stdio/signal/wasm round-trips, forced stack-overlap on Linux). The current bug-hunting pass found nothing new. Deferring because the combination of new API surface, executable-format rewriting, and a build step that runs user code is exactly the kind of change a maintainer should sign off on.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants