bake: restore the dev server's visualizer pages, incremental graph feed and .bake-debug dumps - #38664
bake: restore the dev server's visualizer pages, incremental graph feed and .bake-debug dumps#38664robobun wants to merge 1 commit into
Conversation
…ed and .bake-debug dumps The Rust port put these behind a cargo feature that the build never enabled, so they were compiled out and later deleted as dead code. Gate them on feature_flags::BAKE_DEBUGGING_FEATURES like the rest of the debugging features instead: - serve /_bun/incremental_visualizer and /_bun/memory_visualizer (plus the /_bun/iv and /_bun/mv shortcuts) - publish the incremental graph on the "v" topic when a socket subscribes and after every bundle - debug builds dump every bundled module, the latest chunks and their source maps below .bake-debug/ in the cwd again
|
Warning Review limit reached
Next review available in: 15 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (6)
Comment |
|
Status: ready for review. Reproduced on the current canary ( Overlap notes for whoever merges: the visualizer serializer is byte-identical to the one in #38617 (the crash dump), so the second of the two to land just drops its copy; #37936 fills in the memory visualizer function this PR calls after each bundle. |
|
This PR may be a duplicate of:
🤖 Generated with Claude Code |
|
Not duplicates, but they are adjacent pieces of the same deleted feature, so here is how they fit together:
This PR is the part none of them cover: the |
There was a problem hiding this comment.
I reviewed this PR and the automated bug hunt found no issues. Given the size (~300 lines of new Rust across DevServer routing, a new binary serialization path, several new unsafe sibling-field accesses, and debug-only file I/O) plus the stated overlap with #37936 and #38617, a human look is still worthwhile before landing.
What was reviewed:
dump_dirlifecycle — opened after the box is fully initialized;sys::DirhasDrop, and the intermediateDirindump_bundle'smake_open_path().and_then(|dir| …)chain is dropped and closed afterFile::create.- The
_.._escaping indump_bundle_for_chunkkeeps dumps inside.bake-debug; debug-only, andjoin_abs_string_bufnormalises the result beforemake_open_path. - Visualizer frame layout matches what
incremental_visualizer.html/ the test decoder read (u32 counts, 6 flag bytes, edges after both file lists);is_set_allow_out_of_bound(i, true)treats not-yet-sized stale bits as stale. - New routes go through
dev_route_tramp, so the existing Host / DNS-rebinding guard covers them.
Extended reasoning...
Overview
This PR restores three DevServer debugging features that were dropped when the Zig→Rust port gated them behind a never-enabled cargo feature: the /_bun/incremental_visualizer and /_bun/memory_visualizer pages (plus /_bun/iv and /_bun/mv redirects), the MessageId::Visualizer frame published on the IncrementalVisualizer HMR topic, and the .bake-debug/ per-module and per-chunk source dumps in debug builds. It touches DevServer.rs (new dump_dir field, four new route handlers, ~150 lines of graph-serialization helpers, dump_bundle/dump_bundle_for_chunk), incremental_graph.rs (dump hooks in receive_chunk and both take_js_bundle_to_list*), source_map_store.rs (dump hook in render_json), memory_cost.rs (exhaustiveness match), mod.rs (ChunkKind::dump_file_name), and adds a 466-line test file exercising the pages, the frame contents across subscribe/bundle/hot-update/delete/SCB-demote, and the dump layout for both client-only and framework apps.
Security risks
Low. The new HTTP routes are gated on feature_flags::BAKE_DEBUGGING_FEATURES (canary/debug only) and are dispatched through the existing dev_route_tramp, so the DNS-rebinding Host check that guards every /_bun/* route applies. The pages serve static embedded HTML and the redirects target fixed constants. The .bake-debug writes are debug-build-only, rooted in the cwd, and dump_bundle_for_chunk rewrites ../ segments to _.._/ before joining under /<graph>/; the join goes through join_abs_string_buf, which normalises. A dump write failure only warns and never fails the bundle. No user-controlled input reaches these paths beyond project-relative source file names.
Level of scrutiny
Medium-high. While the surface area is developer-tooling only (nothing here runs in stable release builds except the always-None dump_dir field and the if feature_flags::BAKE_DEBUGGING_FEATURES early return), the change is not mechanical: it adds a struct field with Drop implications, several new unsafe { (*dev).field } sibling projections in incremental_graph.rs, a hand-rolled little-endian binary format that must stay byte-compatible with incremental_visualizer.html, and file I/O with path construction. The PR description also notes it duplicates serialization code from open PR #38617 and calls into emit_memory_visualizer_message_if_needed whose body is being restored in #37936 — a maintainer should confirm the intended landing order.
Other factors
The test coverage is unusually thorough for a debugging feature: it decodes the wire format independently, asserts trailing-byte exactness, sorts to avoid slot-order dependence, covers freed edges, deleted files, stale flags, server-side flags, and the _.._ path case, and byte-compares dumped chunks against what the browser would fetch. The tests correctly describe.skipIf on the build variant. Nothing in the diff triggered concerns from the automated review, and the specific things I checked (fd lifetimes on the make_open_path chain, sys::Dir Drop, the is_set_allow_out_of_bound semantics for out-of-range stale bits, route registration ordering vs. the app catch-all) all look correct. Deferring purely on size/complexity and the cross-PR coordination note, not on any identified defect.
|
On the landing order question from the review: no particular order is needed.
|
Problem
/_bun/incremental_visualizerand/_bun/memory_visualizerare not routed:src/runtime/bake/incremental_visualizer.htmlandmemory_visualizer.htmlare served by nothing (an app with a catch-all HTML route answers these URLs with its own page).svon/_bun/hmr) never produces avframe:hmr_socket.rscounts the subscriber and callsDevServer::emit_visualizer_message_if_needed, which is an empty function (DevServer.rs:5444before this change). The call after every bundle instart_next_bundle_if_presenthits the same empty function..bake-debug/dumps of what was bundled. Leftovers of the feature are still there: the.gitignoreentry, thestartoffsettake_js_bundle_to_listcomputes and discards, thedevparameter ofrender_jsonthat only the dump used.#[cfg(feature = "bake_debugging_features")], a cargo feature nothing enabled (scripts/build/rust.tspasses--featuresonly for the Windows shim), so it was always compiled out, and Narrow crate-internal Rust visibility across all targets and delete the code it proves dead #36184 deleted it as provably dead. The rest of the debugging features (CLI flags, the socket hooks) are gated on the runtime constfeature_flags::BAKE_DEBUGGING_FEATURESand survived.Fix
set_routesregisters the two pages (read withruntime_embed_file!, so embedded at build time in release builds) and the/_bun/ivand/_bun/mvredirects the Zig version had, behindBAKE_DEBUGGING_FEATURES. They use the existingdev_route_tramp, so the Host check covers them like every other/_bun/route.emit_visualizer_message_if_neededserializes both graphs and publishes them on theIncrementalVisualizertopic while a socket is subscribed, then hands over to the memory visualizer as before.write_visualizer_messageand its helpers) is the same code Restore BUN_DUMP_STATE_ON_CRASH: dump the DevServer graph when bun crashes #38617 adds for the crash dump; whichever PR lands second drops its copy..bake-debug/in the cwd when a DevServer is created (DUMP_SOURCES_DIR); in release buildsdump_dirstaysNone. The directory is opened after the struct is fully initialized, so every error return closes it throughDrop.dump_bundlewrites each module as it is received under<graph>/<path relative to the root>(..segments become_.._, keeping the dump inside the directory), the latest client and server chunk, and the source maps served or loaded for them (ChunkKind::dump_file_name). A dump that cannot be written only warns..gitignoreentry). The gate is the const the other debugging features already use, so stable release builds still do none of this. The dump default stays debug-only and unconfigurable, as it was.test/bake/dev/debugging-features.test.ts: the pages and redirects; the frame sent on subscribe, after bundling, after a hot update (a freed edge), after deleting a file (stale flag), and for a framework app with a"use client"component (server flags, both edge lists, the deleted client file once the directive is removed); the dumps of a client app (module files,latest_chunk.jsidentical to the served script after the header,latest_chunk.js.mapidentical to the served map) and of a framework app (server/modules including the_.._path of the framework entry point,latest_hmr.jsand its map).test/bake/dev/andtest/bake/*.test.tswith the debug build (onlyproduction.test.tsfails, by timing out at 5s in a debug build; it never creates a DevServer),test/internal/source-lints/,cargo clippy -p bun_runtime,cargo check -p bun_runtime --target x86_64-pc-windows-msvc.emit_memory_visualizer_message_if_neededbody this calls into), Restore BUN_DUMP_STATE_ON_CRASH: dump the DevServer graph when bun crashes #38617 restores the crash dump. Neither covers the routes, the feed or the dumps.Background
IncrementalGraphs (client and server): a list of files plusEdges recording which file imports which. A deleted file keeps its slot with an empty key so indices stay valid; a removed edge goes toedges_free_listfor reuse./_bun/hmrmultiplexes topics; a client subscribes by sendingsfollowed by topic bytes.vis the incremental visualizer topic and also the first byte (MessageId::Visualizer) of the framesincremental_visualizer.htmldecodes: both file lists, then both edge lists.Mis the memory visualizer's topic and frame id; those frames are already produced on main.BAKE_DEBUGGING_FEATURES(src/bun_core/feature_flags.rs) isIS_CANARY || IS_DEBUG. The visualizer pages exist in both kinds of builds, the source dumps only in debug builds."use client"file imported from server code: the server graph keeps a stub for it (is_client_component_boundary) and the client graph gets the real file. Removing the directive demotes it, which is the one path that deletes a file from a graph; the test uses it to cover the empty-name encoding.