bake: fix dev server crashes in SCB dependency tracing and route syntax error reporting#31697
bake: fix dev server crashes in SCB dependency tracing and route syntax error reporting#31697robobun wants to merge 6 commits into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (3)
WalkthroughUnconditionally clone watched file paths for watcher ownership, make hot-update dependency tracing robust to vector growth by using an index-based loop, and fix route-parse error reporting by clamping/rebasing cursor positions and setting explicit spans for over-limit parameter patterns. ChangesDev Server Robustness Fixes
Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
|
Found 4 issues this PR may fix:
🤖 Generated with Claude Code |
|
Checked each suggested issue against the fixed mechanisms — none is a confident match, so not adding
This PR is scoped to the Sentry reports it was built from (BUN-3DYH, BUN-3DP9, BUN-3D8F) plus the #31647 watcher regression. |
|
This PR may be a duplicate of:
🤖 Generated with Claude Code |
|
Re the duplicate-PR flag: the overlap with #31695 is exactly one hunk — the |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — thanks for addressing the off-by-one and pinning the caret column in the test.
Extended reasoning...
Overview
Three targeted crash fixes in the bake dev server, each with a regression test:
DevServer.rs(finalize_bundle): replacesfor index in &vecwith index-basedwhile i < vec.len()iteration overclient_components_affected. I verified inincremental_graph.rs:1168-1171thattrace_dependenciesdoes push to this Vec when it visits an SCB, and the gts-bit check at the top oftrace_dependencies(line 1135) guarantees revisited entries early-return — so the index-based loop is semantically equivalent but iterator-safe.FrameworkRouter.rs: clampsTinyLog::print'scursor_attorel_path.len(), switches the 64-param error fromlog.write()tolog.fail()so the cursor is set, and makes the cursor rebase saturating with theu32::MAXsentinel preserved. Includes the off-by-one correction from my earlier inline comment, now test-pinned at the exact column.bundle_v2.rs:add_file::<{ cfg!(windows) }>→add_file::<true>so the watcher always owns its path copy instead of borrowing arena-backed storage.
Security risks
None. These are dev-server crash fixes; no auth, crypto, network-input parsing, or privilege boundaries are touched. Each change is strictly defensive (clamp, own-instead-of-borrow, index-instead-of-iterator).
Level of scrutiny
Moderate — the changes touch bundler/dev-server internals with memory-lifetime reasoning, but each hunk is small (< 20 effective LOC), well-commented, and covered by a regression test that the PR description confirms fails on unmodified main and passes with the fix. The author also ran a 5-minute ASAN stress test. None of the touched files are under CODEOWNERS.
Other factors
- I previously reviewed and left one inline nit (cursor-rebase off-by-one); it was fixed in 3c48d77 and the thread is resolved.
- The current bug-hunting pass found no issues.
- The one-hunk overlap with #31695 is acknowledged and benign (whichever lands second resolves a comment-only conflict).
- Worst-case downside of any individual change is benign: an extra string clone per watched file, extra no-op loop iterations, or a clamped cursor position.
There was a problem hiding this comment.
Thanks for addressing the cursor-rebase off-by-one — the fixes all look correct and strictly defensive to me, but since this touches finalize_bundle / incremental-graph tracing and watcher path lifetimes, I'd like a bake maintainer to confirm the index-loop is bounded (i.e. the gts-bit-already-set claim) before merge.
Extended reasoning...
Overview
Three targeted crash fixes in the bake dev server, each with a regression test:
DevServer.rs: replacesfor index in &vecwith awhile i < vec.len()index loop overclient_components_affectedto avoid iterator invalidation whentrace_dependenciesappends to that same Vec mid-iteration.FrameworkRouter.rs: clampsTinyLog::print'scursor_attorel_path.len(); switches the 64-param error fromlog.writetolog.failso a cursor is set; rebases the cursor with saturating arithmetic, guarding theu32::MAXsentinel and applying the-1correction from my earlier inline comment (now resolved and pinned by a stderr-alignment assertion in the new test).bundle_v2.rs: changesadd_file::<{ cfg!(windows) }>toadd_file::<true>so the watcher always owns its path copy instead of borrowing arena-backed slices on POSIX.
Security risks
None. All inputs are local filesystem paths and developer route files; the changes are defensive (clamping, saturating arithmetic, owned copies, index-based iteration) and only affect dev-server-local error reporting and HMR bookkeeping. No auth, network, crypto, or untrusted-input parsing is touched.
Level of scrutiny
Medium-high. Each individual change is small and strictly safer than what it replaces — there is no realistic way for a clamp, a saturating add, an owned clone, or an index-vs-iterator loop to regress correctness relative to the prior code. However, the surrounding subsystem is the bundler's incremental-rebuild core (finalize_bundle, graph trace bits, watcher lifetimes), where invariants are subtle and a Rust-port-of-Zig is in flight. The index-loop fix in particular relies on the claim that mid-loop-appended entries already have their gts bit set (so the loop can't grow unboundedly); that's stated in the comment and PR description but is the kind of invariant a maintainer who owns incremental_graph.rs should confirm.
Other factors
- My one prior inline nit (cursor rebase off-by-one) was addressed in 3c48d77 and the regression test now pins the exact caret column, so that thread is closed.
- The bug-hunting pass on the latest revision found nothing.
- One hunk overlaps with PR #31695 (the
add_file::<true>change); the author has acknowledged this and the overlap is comment-only on conflict. - robobun reported a ❌ on build #59822 for 3c48d77; the latest commit (c1cdd4f) is a CI retrigger and I don't have the updated status — worth confirming green before merge.
- The new
framework-router.test.tsassertion matches exact stderr whitespace/formatting fromTinyLog::print, which is somewhat brittle but intentional (it pins the caret alignment fix).
Deferring rather than approving because of the critical-path location and the unverified CI status, not because of any specific concern with the diff.
|
Re the boundedness of the index loop, the invariant is structural in if gts.bits(SIDE).is_set(file_index.get() as usize) {
return Ok(()); // dedup guard
}
gts.bits(SIDE).set(file_index.get() as usize); // bit set FIRST …
...
if is_scb {
… client_components_affected.push(…); // … push only AFTER the bit is set
}
The pre-existing |
|
CI status for merge: the diff is green on every lane that executed — 70/70 including linux x64/aarch64 (glibc+musl), ASAN, baseline, and all Windows test lanes, plus Format and clippy. The only red checks across both runs (59822, 59859) are |
|
CI status update (final): on the retried build, |
…route syntax error printing, watcher path lifetime) 1. finalize_bundle iterated incremental_result.client_components_affected while trace_dependencies appends every visited client component boundary back onto that list (its trace bit was just cleared), so with 3+ boundaries invalidated in one hot update the Vec reallocates mid-iteration and the stale iterator walks freed memory. Release builds read reused bytes (framework_routes_affected stores (route << 1) | flag u32s in the recycled block) as file indexes, crashing with "index out of bounds: the len is N but the index is N". Iterate by index against the live Vec; re-visiting appended entries is a no-op because their trace bit is already set. 2. The FrameworkRouter "more than 64 params" error path recorded its message with TinyLog::write, which does not set a cursor, leaving cursor_at at the u32::MAX sentinel from TinyLog::empty(). Printing the error then sliced rel_path[u32::MAX..], crashing the dev server while reporting a user's route typo. Set a whole-path cursor at that call site, clamp cursor_at in TinyLog::print so no producer state can index out of bounds, and make the cursor rebase saturating so fail()-less error paths keep the sentinel instead of wrapping. 3. The bundler registered watcher entries with CLONE_FILE_PATH only on Windows, storing the path slice directly on POSIX. Since dupe_alloc allocates the disjoint text/pretty case (every "ssr:"-prefixed SSR-graph path) from the per-bundle arena, the watchlist could point into freed memory once the bundle completed, crashing the watcher thread on the next file event. Always clone; the watcher dedups by hash so this is one copy per watched file.
rel_path starts one byte before the full_rel_path suffix it mirrors (the leading '/'), so rebasing fail()'s cursor position onto full_rel_path must subtract one; the underline/caret in printed route errors was shifted one column right of the offending character. Pin the corrected alignment in the regression test.
robobun
force-pushed
the
farm/0501dc54/fix-bake-dev-server-crashes
branch
from
c1cdd4f to
a047ebe
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/bake/framework-router.test.ts`:
- Around line 169-197: The test currently only reads proc.stdout incrementally
and waits to consume proc.stderr after killing the process, which can cause pipe
backpressure deadlocks; modify the test around the Bun.spawn usage so stderr is
drained concurrently with stdout (e.g., kick off an async reader/collector for
proc.stderr into a string while the existing for-await loop reads proc.stdout
and matches the URL), ensure both streams are being consumed before proc.kill()
and before awaiting proc.exited, and keep variables like stdout, stderr,
decoder, and proc references (proc.stdout / proc.stderr) intact so the final
assertions still use the collected stderr text.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 55e48fbd-0cc0-431e-b97b-d85ed09fcf3a
📒 Files selected for processing (5)
src/bundler/bundle_v2.rssrc/runtime/bake/DevServer.rssrc/runtime/bake/FrameworkRouter.rstest/bake/dev/bundle.test.tstest/bake/framework-router.test.ts
There was a problem hiding this comment.
All prior review feedback has been addressed and the bug hunter found nothing new, but these are memory-safety fixes (iterator invalidation, use-after-free, OOB slice) in core bundler/dev-server hot paths — worth a human pass on the invariant reasoning before merge.
Extended reasoning...
Overview
This PR fixes three independent dev-server crashes across src/bundler/bundle_v2.rs, src/runtime/bake/DevServer.rs, and src/runtime/bake/FrameworkRouter.rs, with regression tests in test/bake/dev/bundle.test.ts and test/bake/framework-router.test.ts. The fixes are: (1) switch a for &vec loop to index-based iteration because trace_dependencies appends to the same Vec mid-loop; (2) clamp TinyLog::cursor_at and give the 64-param error a real cursor so print can't slice OOB, plus correct the cursor rebase offset; (3) always clone watcher paths since dupe_alloc can hand out per-bundle-arena storage.
Security risks
None in the conventional sense — no auth, crypto, network-input parsing, or permission surface. The changes are crash-hardening of internal data structures. The watcher-path clone and the index-based loop strictly reduce UB exposure relative to the previous code.
Level of scrutiny
High. These are memory-safety fixes in production-critical paths (finalize_bundle, the file watcher registration in on_parse_task_complete, and the route scanner). The correctness of fix 1 hinges on the invariant that trace_dependencies sets the gts bit before pushing (so the index loop is bounded), and fix 3 changes lifetime ownership semantics for every watched file on POSIX. The reasoning in the PR description and the author's follow-up comment on boundedness is detailed and looks sound to me, but this is exactly the kind of subtle invariant a maintainer familiar with incremental_graph.rs should sign off on.
Other factors
- Both of my earlier inline comments (off-by-one in the cursor rebase; comment-style per CLAUDE.md) were addressed in 3c48d77 and 3e24bc8 respectively, and the CodeRabbit stderr-drain concern was addressed in 25a3391. All inline threads are resolved.
- The bug-hunting system found nothing on the current revision.
- New regression tests cover all three crashes and pin the corrected caret alignment; CI is green on every lane that ran (the darwin/musl failures are documented as fleet-wide and unrelated).
- There is a one-hunk overlap with #31695 (the
add_file::<true>fix), already acknowledged in-thread.
I'm deferring rather than approving because the changes, while small in line count, alter iteration-under-mutation and storage-lifetime behavior in hot bundler code — not the kind of mechanical change appropriate for bot auto-approval.
|
CI triage for build 61332 (head 3e24bc8): three red test lanes, none related to this diff.
Everything this PR touches remains green: the bake suites pass on the ASAN lane, Format/clippy pass, and all Linux build+test lanes are green. One retrigger was already spent earlier; leaving the rest to a maintainer. |
|
Build 61674 (after the branch was updated with main) carries no test signal: the darwin and linux build-cpp jobs expired in the agent queue without starting, cascading waiting_failed to every downstream test job — nothing was executed. The branch now includes #32042 (skips the flaky bunx test that failed the previous run), so the next triggered build should be clean. Not pushing a retrigger; a Buildkite rebuild by anyone with access will do. |
1 participant
Fixes three bake dev server crashes. The first two correspond to crash reports BUN-3DYH (
Panic: index out of bounds: the len is 23 but the index is 23infinalize_bundle) and BUN-3DP9 / BUN-3D8F (SIGSEGV inon_router_syntax_error→print); the third is a use-after-free introduced by #31647 that is not in any shipped canary yet.1.
finalize_bundleOOB: iterator invalidation onclient_components_affectedRepro: a
separateSSRGraphserver-components project with 3+"use client"boundaries sharing a client-side dependency; edit the shared file so one hot update invalidates every boundary.Cause:
finalize_bundleclears the graph-trace bits and then loopsfor index in &dev.incremental_result.client_components_affected. Eachtrace_dependencies(index, NoStop)call re-appends the boundary it visits to that same Vec (its trace bit was just cleared), so the list grows by one per iteration. Once a push crosses the capacity, the Vec reallocates and the loop'sslice::Iterkeeps walking the freed buffer. Under ASAN this is an immediate use-after-free at the exact Sentry stack (finalize_bundle ← finish_from_bake_dev_server ← on_parse_task_complete ← ServerComponentParseTask on_complete). In release, the freed block is promptly recycled by theframework_routes_affectedpushes in the same loop — those store(route_index << 1) | recurse_flagu32s, so the stale iterator yields small odd numbers (e.g. 23) as file indexes, andtrace_dependenciesdies withindex out of bounds: the len is 23 but the index is 23(len = server-graph file count). The Zig original (for (…items) |index|) had the identical latent bug.Fix: iterate by index against the live Vec. Entries appended mid-loop already had their trace bit set when pushed, so visiting them is a no-op — semantics are unchanged.
2. Segfault printing a router syntax error
Repro: start the dev server with a route path containing more than 64
[params](bun build/debug/bun-debug main.tswithroutes/[p0]/…/[p64]/index.tsx). Before the fix:panic: range start index 4294967295 out of range for slice of length 396(the Zig-era ReleaseFast equivalent of this unchecked slice was the reported SIGSEGV).Cause:
TinyLog::empty()initializescursor_at = u32::MAXas an "unset" sentinel; onlyTinyLog::fail()sets a real cursor. Theparam_count > 64error path calledlog.write(...)without ever setting the cursor, andTinyLog::printslicedrel_path[cursor_at..]with the sentinel. A user's route-file typo would kill the dev server while it tried to report the error.Fix: give the 64-param error a whole-path cursor, clamp
cursor_atinprintso no producer state can index out of bounds, and make the cursor rebase in the scan loop saturating sofail()-less error paths (e.g. OOM) keep the sentinel instead of wrapping.3. Watcher use-after-poison on arena-backed paths (regression from #31647)
Found while stress-testing the above. #31647 made
dupe_allocallocate the disjointtext\0pretty\0case from the per-bundle arena — which is everyssr:-prefixed SSR-graph path. The bundler's watcher registration usedadd_file::<{ cfg!(windows) }>, i.e. stored the path slice on POSIX, so the watchlist could point into the freed bundle arena once the bundle completed; the next file event crashed the watcher thread (ASAN: use-after-poison inDevServer::on_file_update). Always clone — every otheradd_filecaller already passestrue, and the watcher dedups by hash so it's one copy per watched file.Verification
test/bake/dev/bundle.test.ts"hot update touching multiple client component boundaries does not crash": fails on unmodified main (ASAN abort, "DevServer crashed"), passes with the fix. Exercises crashes 1 and 3.test/bake/framework-router.test.ts"dev server reports route syntax errors without crashing": on unmodified main the server dies before startup; with the fix it prints both router errors and serves requests. Exercises crash 2.test/bake/framework-router.test.ts(36 pass),test/bake/dev/bundle.test.ts(21),test/bake/dev/hot.test.ts(9),test/bake/dev/esm.test.ts(16) all green on the debug/ASAN build. The twocss.test.tsfailures (assertion failed: !chunk.content.is_css()) reproduce on unmodified main and are unrelated.separateSSRGraph) survives with zero sanitizer reports; before the fixes it crashed within seconds.