Startup snapshots (4/4): dirty-page tooling (opt-in build define)

[Jarred-Sumner]

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

One file, compiled only with -DBUN_STARTUP_SNAPSHOT_TOOLING=1, so it is not in shipped binaries: a census of which snapshot pages a restored process dirties, attributed by object type, and write traps that record the writer's stack — the instrument used to get the memory numbers in the previous PR and to find what was dirtying pages. StartupSnapshot.h in the runtime PR already carries the inline no-op stubs this replaces.

[robobun]

^{Updated 7:04 AM PT - Aug 9th, 2026}

@Jarred-Sumner, your commit d2b100e is building: #90937

[robobun]

^{Updated 2:10 AM PT - Aug 11th, 2026}

✅ @Jarred-Sumner, your commit 8d6b72e0d6175f191cdf83b50ac1f05a47c01f08 passed in Build #92107! 🎉

---

🧪   To try this PR locally:

bunx bun-pr 37262

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

bun-37262 --bun

[claude]

Re-checked the four items from the previous pass against 981a161: the footprint lambda is now Darwin-guarded, the cmd-file entry is "snapshot", 8, munmap(raw.recs, ...) sits after the fopen block, and the unified-source coupling is gone (immap is declared in StartupSnapshot.h, and TrapRec/s_trap*/s_prev* are now defined in this file). All four resolved; only the new nits below remain.

Extended reasoning...

The author pushed a new revision after my first review. I diffed the current file against each of the four earlier findings: (1) the Discriminator footprint lambda now returns -1.0 under #if !OS(DARWIN) before any Mach call, so Linux with the tooling define compiles; (2) the cmd-file dispatcher reads !strncmp(buf, "snapshot", 8) matching the chain convention; (3) munmap(raw.recs, raw.cap * sizeof(Rec)) is outside the if (FILE* f2 = fopen(...)) block so it pairs with the unconditional mmap; (4) immap is now an extern-linkage declaration in StartupSnapshot.h:49 (no longer file-static in the .cpp), and TrapRec, s_trapRecs, s_trapCount, s_trapCap, s_prevBus, s_prevSegv are defined locally in this TU rather than borrowed from the sibling — the file compiles standalone. I'm noting this so the next pass doesn't re-derive it; the three new inline comments are separate, lower-severity observations in opt-in diagnostic code and don't need restating here.

[claude]

Beyond the inline nit: verified the latest butterfly-window fix — property-only butterflies now compare exactly [allocBase, allocBase + oolBytes), and indexed shapes include the header + (ArrayStorage-adjusted) elements, so both the neighbor-cell false-positive and the ArrayStorage tail miss are closed. Also checked whether recordUsedBlock's std::vector/map inserts could re-enter mi_heap_main() mid-mi_heap_visit_blocks — they go through the process allocator (bmalloc/WTF fastMalloc → mimalloc default heap), which is freshHeap by the time this runs after a real restore, so no self-iteration.

Extended reasoning...

This run's inline finding is a hygiene nit (bare -Wformat pragma without push/pop) whose own reasoning notes it adds no incremental leak because StartupSnapshot.cpp:103 already has the identical bare pragma in every supported build. Recording two things examined beyond it: (1) the butterfly diff window in dumpMutatedSnapshotObjects went through three fix rounds; the current form (n = oolBytes + (idx ? sizeof(IndexingHeader) + post : 0) with base = bf - oolBytes - sizeof(IndexingHeader)) matches JSC's Butterfly::totalSize/fromBase geometry for all three shape families, so that thread is closed. (2) A finder flagged recordUsedBlock allocating while mi_heap_visit_blocks(mi_heap_main(), ...) iterates — refuted because the containers allocate through the default heap, not mi_heap_main(), once freshHeap is set (real restore path) or before the tooling filesnap swaps the default. The earlier follow-up about snapshotFd on the pwrite-failure path is still open on the thread; not restating it here.

[claude]

I reviewed this PR and the current pass found no new issues — every earlier inline finding has been addressed. Because it's ~1,800 lines of new C++ touching JSC heap iteration, signal handlers, and raw mmap/mprotect, a human look would still be worthwhile even though it's gated behind an opt-in build define.

What was reviewed:

• The butterfly window in dumpMutatedSnapshotObjects now matches the allocation exactly (property-only, contiguous, and ArrayStorage shapes).
• -Werror=unused on both platforms: no remaining unused labels/variables under the tooling define.
• The -Wformat pragma push/pop pair sits at file scope outside every #if, so it balances on musl/unsupported builds and doesn't leak into UnifiedSource siblings.
• The trap handler chains to the previous SIGBUS/SIGSEGV action when the fault is outside frozenRanges, and s_trapCap == 0 correctly disables recording without disabling fault resolution.

Extended reasoning...

Overview

One new file, src/jsc/bindings/StartupSnapshotTooling.cpp (~1,800 lines), compiled only when -DBUN_STARTUP_SNAPSHOT_TOOLING=1 is set. It implements a dirty-page census attributed by JSC object type, byte-level diffing of snapshot pages against the backing file, mimalloc live-block/owner joins, and SIGBUS/SIGSEGV write traps that record the faulting stack. StartupSnapshot.h (from the base PR) already carries the no-op inline stubs this replaces, so default builds are unaffected.

Security risks

None for shipped binaries: the entire file is behind #if BUN_STARTUP_SNAPSHOT_TOOLING && BUN_STARTUP_SNAPSHOT_SUPPORTED and the define is not set in any shipped or CI configuration. Within a tooling build it installs signal handlers, mprotects heap ranges read-only, and reads env-controlled paths — all developer-local, not exposed to JS or the network.

Level of scrutiny

Medium-low for correctness impact (opt-in debugging instrument, worst case is a wrong number in a census or a local build break), but the sheer volume and the number of platform-gated raw-memory paths (Mach page queries, /proc/self/maps parsing, arm64 ucontext frame walking, MAP_FIXED remaps) mean it isn't the kind of change I'd approve without a human glance.

Other factors

The PR has been through ~15 rounds of automated review over two days; the author fixed every finding (Linux footprint guard, page-straddling filter, 4K→16K compare buffers, butterfly geometry off-by-8 on both ends, ArrayStorage vectorOffset, -Wunused-label/-Wunused-but-set-variable, pragma push/pop scoping including the follow-up musl correction). All inline threads are resolved. There is no automated test for this file — expected, since it only compiles under a manual define and its output is human-read reports.

[claude]

Beyond the inline nit: I re-checked the _GNU_SOURCE removal against gettid() in onMainThread()'s Linux branch — StartupSnapshot.cpp defines it at file top and sorts immediately before this file in the same unified TU, so <unistd.h> has already declared gettid by the time this file is reached. Dropping the define here is fine.

Extended reasoning...

My previous comment advised keeping #define _GNU_SOURCE for gettid(); the author removed it entirely. This run's verifier examined whether that breaks the Linux tooling build and concluded it does not: StartupSnapshot.cpp:1-2 defines _GNU_SOURCE before any include, the two files are adjacent in the unified-source sort, and BUN_STARTUP_SNAPSHOT_SUPPORTED is glibc-only on Linux — so gettid is already declared when this file's <unistd.h> include hits the header guard. Recording this so the open thread on that comment can be closed without re-investigation.

[claude]

Beyond the inline nit, I re-checked the _GNU_SOURCE removal against gettid() in onMainThread()'s Linux branch (my earlier comment had flagged the define as needed for it) — verifiers confirmed it still resolves in both unified and non-unified Linux builds, so the removal is fine.

Extended reasoning...

The only finding this run is a nit (dead/duplicate includes in the Darwin and unconditional blocks — the sibling of the already-fixed Linux block). Separately, finder agents raised that removing #define _GNU_SOURCE could leave gettid() undeclared on Linux at line ~1528; verifier agents refuted it for both unified and standalone TU configurations. Recording that here since my earlier inline comment on this PR had said the define was still load-bearing for gettid(), and the author removed it anyway — no follow-up needed on that point.

[claude]

I reviewed the latest revision and found no new issues — all prior inline findings have been addressed. Given this is ~1,800 lines of new C++ touching signal handlers, mprotect/mmap, and JSC heap iteration internals, a human pass is still worthwhile even though it's opt-in tooling gated behind BUN_STARTUP_SNAPSHOT_TOOLING.

What was checked this run:

• gettid() on Linux after the _GNU_SOURCE removal — still declared in this TU, no build break.
• The HeapIterationScope hoist in dumpNewCells and the butterfly-window geometry in dumpMutatedSnapshotObjects — both fixes landed as described.
• The include cleanup (22 headers removed) — no remaining unreferenced or duplicate includes.

Extended reasoning...

Overview

This PR adds a single new file, src/jsc/bindings/StartupSnapshotTooling.cpp (~1,800 lines), compiled only under -DBUN_STARTUP_SNAPSHOT_TOOLING=1 (never in shipped binaries or default CI). It is diagnostic instrumentation for the startup-snapshot feature (PRs #37259-#37261): a dirty-page census attributed by object type, byte-level diffs of snapshot pages against the backing file, per-cell mutation reports over immortal MarkedBlocks, mimalloc live-block/profiler integration, and a SIGSEGV/SIGBUS write-trap that records the first writer's stack per page. Commands are driven via signals or a $BUN_MEMDEBUG/cmd.<pid> file and hooked from the event loop tick.

Security risks

None for shipped builds — the entire file is behind an opt-in compile-time define and the fallback stub is a no-op. The code itself does risky things (installs SIGSEGV/SIGBUS handlers, mprotect(PROT_READ) over live heap ranges, MAP_FIXED remaps of process memory, walks the JSC heap outside normal API surface), but all of it is developer-local instrumentation reading only the current process's own memory and env vars. No network, no privilege boundaries, no untrusted input beyond the operator's own env/command file.

Level of scrutiny

Medium-low correctness bar (it's throwaway diagnostic tooling; a miscount degrades a report, not user behavior) but high review cost: the file is large, dense, platform-branched (Darwin vs Linux, arm64-specific unwinding), and leans on JSC internals (butterfly geometry, MarkedBlock immortality, HeapIterationScope invariants) plus mimalloc private hooks. That combination is exactly where subtle invariant violations hide, and the file has no automated tests (reasonable for this kind of tooling, but it means the only verification is the author's manual runs).

Other factors

The PR has been through ~30 rounds of inline review over two days; every finding I raised (Darwin-only Mach APIs on Linux, unbalanced #pragma diagnostic, butterfly window off-by-8/off-by-16, missing HeapIterationScope before forEachLiveCell, dead/duplicate includes, unused-variable -Werror breaks, etc.) was addressed with a fix commit and confirmation. This run's finder agents raised only the gettid() declaration concern, which verifiers refuted. No outstanding unresolved threads remain. Still, ~1,800 lines of new native code with signal handlers and heap-walking is well past the "simple/mechanical" threshold for auto-approval — deferring so a maintainer can give it a final read, particularly the Linux code paths (most of the author's verification was on Darwin/arm64).

[claude]

Beyond the inline nit, I re-checked the _GNU_SOURCE removal against gettid() in onMainThread() (Linux branch) — it's fine: this file is unified-built after StartupSnapshot.cpp, whose top-of-file #define _GNU_SOURCE covers the whole TU, so gettid is declared.

Extended reasoning...

The author removed _GNU_SOURCE after my earlier comment had said to keep it for gettid(); a verifier this run confirmed the removal is safe because StartupSnapshotTooling.cpp participates in unified sources (not in noUnify) and sorts after StartupSnapshot.cpp, which defines _GNU_SOURCE at line 1 before any includes — so <unistd.h> sees it TU-wide. Recording this so it isn't re-raised. Not approving: ~1,800 lines of new signal-handler / mprotect / JSC-heap-iteration C++ is beyond auto-approval scope even for opt-in tooling.

[claude]

I reviewed this pass and found no issues — 572d2a8 correctly widened the last uint32_t sz consumer and its %u format string, and a re-scan of the file confirms the remaining %us are source offsets. Given the size (a ~1,800-line new TU with signal handlers, mprotect, and JSC heap iteration), a human sign-off on the final state is still worthwhile even though it's compiled only under BUN_STARTUP_SNAPSHOT_TOOLING.

What was reviewed: the two size-widening commits (containers, sentinels, all consumers) and the braced if (s_dir) guard; the fileSnapshotHeap → startupSnapshotToolingIndexAtFreeze dedup; the trap-arm MAP_FAILED early-return wording; and the butterfly-window arithmetic in dumpMutatedSnapshotObjects against the earlier thread — all match what was agreed.

Extended reasoning...

Overview

One new file, src/jsc/bindings/StartupSnapshotTooling.cpp (~1,780 lines), gated behind #if BUN_STARTUP_SNAPSHOT_TOOLING && BUN_STARTUP_SNAPSHOT_SUPPORTED so it is absent from shipped binaries. It provides developer-only instrumentation for the startup-snapshot feature: a page-dirtying census attributed by JSC subspace / mimalloc size class, byte-level diffs of dirty pages against the snapshot file, per-cell mutation classification, write-fault traps with stack capture, and a command-file/signal interface polled from the event loop. The header in the runtime PR already carries no-op stubs for the entry points.

Security risks

None material. The file is compiled only under an explicit build define, never in release artifacts, and is driven entirely by developer-set BUN_* environment variables and a local command file. The signal handler chains to the previous handler for non-snapshot faults. Output paths are constructed from developer-supplied BUN_MEMDEBUG/TMPDIR — acceptable for local tooling.

Level of scrutiny

Medium-low for correctness-of-product (nothing here reaches users), but the code itself is intricate: raw mmap/mprotect, a SIGBUS/SIGSEGV handler that walks the frame chain, iteration over the live JSC heap, and hand-computed butterfly geometry. That intricacy is why this PR accumulated ~200 timeline events across two days, with dozens of findings raised and fixed (Linux build guards, MAP_FAILED checks, allocation under HeapIterationScope, misleading-indentation regression, uint32_t byte-length truncation, butterfly window off-by-8s, etc.). This run's bug-hunt found nothing new, and I verified each of the most-recent fix commits landed as described in the diff.

Other factors

• No automated tests, which is consistent with the file's nature (opt-in diagnostic tooling exercised interactively; the shipped-binary path compiles it out).
• The last open thread (uint32_t widening) was addressed in 572d2a8; the reply says the file was grepped for stragglers, and the diff bears that out.
• The change is large enough that "simple, mechanical, or obvious" does not apply. It's the capstone of a four-PR stack and a maintainer glance over the final shape before merge is appropriate — hence deferring rather than shadow-approving.