Repository navigation
Conversation
|
Warning Review limit reached
This review includes 6 billable files and costs up to $1.50.
Reviews can continue after your included limit without a manual trigger. An admin must approve usage-based billing. Or wait 13 minutes for your next included review. View limit detailsLimit details: You’ve used all 10 included reviews currently available. Review configuration: ⚙️ Run configuration
📒 Files selected for processing (6)
Comment |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. Because it rewrites six separate linker graph walks (chunk hashing, both export * walks, both composes walks, StaticRouteVisitor) and each rewrite must preserve the exact visitation order for hash stability and diagnostic correctness, a human look would still be worthwhile.
What was reviewed:
- Traced each explicit-stack conversion against its recursive original for order preservation — the push-then-reverse and per-frame-cursor patterns reproduce the same postorder in every case I stepped through.
- Checked that
add_exports's new stack is the same set of files the oldsource_index_stack[0..stack_end_pos]covered for both the shadowing and cycle checks. - Verified the
visit_namemark-on-entry change only affects inputs that previously never terminated (composes cycles avoiding the root); DAG and diamond cases produce the same name order. - Confirmed
mark_dynamic_exports_due_to_export_stardrains and marks exactly the current path on finding a dynamic file, matching the recursivereturn truepropagation.
Extended reasoning...
Overview
This PR converts six recursive graph traversals in the bundler's linker into explicit-stack DFS to prevent native stack overflows on deep import/export/composes/chunk chains, and adds a StackCheck guard to the one walk (find_imported_files_in_css_order) that stays recursive. It touches LinkerContext.rs, five files under linker_context/, and adds eight subprocess-based regression tests to bun-build-api.test.ts. It also fixes an unbounded-recursion bug in the composes-exports walk by marking classes as visited on entry rather than exit.
Security risks
None identified. This is internal bundler control-flow refactoring; no new user-controlled input parsing, no auth/crypto/permissions surface. The new user-facing error message is a fixed-format diagnostic.
Level of scrutiny
High. The linker is production-critical: chunk hashes feed output filenames (a silent order change would invalidate every existing hash-based test and break CDN caching), and the export * shadowing/ambiguity semantics determine whether bundles are correct. Each of the six rewrites is an independent algorithm transformation with its own order-preservation invariant, and the composes fix is a deliberate behavior change. This is exactly the kind of PR where a maintainer who owns the bundler should confirm the approach — particularly the choice to leave the CSS @import walk recursive behind StackCheck rather than converting it.
Other factors
The PR description is unusually thorough and the test coverage is strong (each converted walk has a deep-chain test that crashed the debug build before and passes after; two order-guard tests pin the observable ordering; the composes-cycle test pins the newly-terminating case). I stepped through each conversion by hand and found the transformations sound, and the automated bug-hunting pass found nothing. Deferring solely on size and criticality, not on any specific concern.
|
Status: ready for review. Reproduced on the current debug build with The ambiguous |
|
Updated 6:41 PM PT - Oct 6th, 2026
❌ @robobun, your commit 1fc4fc9 has 2 failures in
🧪 To try this PR locally: bunx bun-pr 38966That installs a local version of the PR into your bun-38966 --bun |
|
#40169 touches the same two walks ( |
|
Update: #40169 no longer touches the linker walks. It keeps the One thing worth folding in here. The cycle check in the new if !self
.stack
.iter()
.any(|frame| frame.source_index == other_source_index)Every file in an // in ExportStarContext
on_stack: Vec<bool>, // vec![false; named_exports.len()] at construction
// on push
self.on_stack[idx as usize] = true;
// on pop (both pop sites)
self.on_stack[frame.source_index as usize] = false;
// cycle check
if !self.on_stack[other_source_index as usize] { push }The shadowing loop over |
…ing the linker's stack The bundle thread has a 2 MiB stack and several linker walks still recursed once per graph edge, so a long enough chain of files crashed Bun.build() with a native stack overflow. Convert the export star walks, both CSS modules composes walks and the static route visitor to explicit-stack DFS that visits in the same order as before. The CSS @import order walk keeps recursing but checks the remaining stack before following an import and fails the build with an error instead of overflowing. Marking a composed class as visited before walking it also stops the exports object generation from recursing forever on a composes cycle that does not pass through the class being exported. The chunk hash walk is not part of this change any more: #40518 replaced it with final_chunk_hashes, which does not recurse. The test for a deep chunk chain stays.
53bccfd to
1fc4fc9
Compare
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| /// A file whose imports are being checked, and how many of its import | ||
| /// records have been looked at so far. |
There was a problem hiding this comment.
If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code
| /// | ||
| /// Explicit-stack DFS (was recursive, one call per import). `result` is | ||
| /// the answer the most recently finished file (or cache hit) gave to the | ||
| /// file below it on the stack: `true` finishes that file as well, so it | ||
| /// propagates down through every file on the stack, caching `true` for | ||
| /// each, the way the recursive form's early returns did; `false` lets the | ||
| /// file go on to its next import. |
There was a problem hiding this comment.
If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code
| /// Starts checking `source_index`'s imports unless the answer is already | ||
| /// known: cached by an earlier walk, or `false` for a file that is already | ||
| /// being checked further down the stack (a cycle). |
There was a problem hiding this comment.
If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code
| /// | ||
| /// The traversal recurses once per `@import`, so an import chain deeper than | ||
| /// the thread's stack allows is reported as an error on the bundler log and | ||
| /// yields an empty order. |
There was a problem hiding this comment.
If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code
| /// The `@import` that could not be followed because `visit` recurses | ||
| /// once per `@import` and the thread's stack was nearly exhausted. | ||
| /// Once set, the walk unwinds without visiting anything else. |
There was a problem hiding this comment.
If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code
| /// Visits the file `record` (an `@import` or `composes` in `importer`) | ||
| /// resolved to. Returns false if the walk has been abandoned, in which | ||
| /// case the caller returns as well. |
There was a problem hiding this comment.
If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code
| // return above intentionally skips it, and the `too_deep` returns | ||
| // below don't need it because the whole walk is discarded. |
There was a problem hiding this comment.
If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code
| // Split-borrow (see `LinkerContext::log_disjoint`). `link()` fails the | ||
| // build once `compute_chunks` returns. |
There was a problem hiding this comment.
If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code
| /// A class whose `composes` declarations are being walked, and | ||
| /// how far along them the walk is. |
There was a problem hiding this comment.
If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code
| /// Append the class's own name once its `composes` are done. | ||
| /// False for the root class, whose name the caller appends. |
There was a problem hiding this comment.
If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code
| /// Starts walking a composed class's own `composes` unless it has | ||
| /// been reached already. Its name is appended when that walk | ||
| /// completes, after the names it composes. | ||
| /// | ||
| /// The class is marked as reached on the way in. The recursive | ||
| /// form marked it on the way out, which appended the same names | ||
| /// in the same order whenever it terminated, but recursed forever | ||
| /// on a `composes` cycle that did not pass through the root class. |
There was a problem hiding this comment.
If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code
| /// Appends the name of every class that `css_ref` (a class in | ||
| /// the file `idx`, already marked as reached) transitively | ||
| /// composes, each after the names it composes itself. | ||
| /// | ||
| /// Explicit-stack DFS (was recursive, one `visit_composes` and | ||
| /// `visit_name` call per composed class). Each iteration handles | ||
| /// one name of one `composes` declaration of the class on top of | ||
| /// the stack, so the names, the `from global` strings and the | ||
| /// diagnostics come out in the order the recursion produced them. |
There was a problem hiding this comment.
If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code
| // E.g.: `composes: foo from global` | ||
| // | ||
| // In this example `foo` is global and won't be rewritten to a locally scoped | ||
| // name, so we can just add it as a string. |
There was a problem hiding this comment.
If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code
| /// One file on the path of an explicit-stack walk over the `export *` graph: | ||
| /// the file and how many of its `export_star_import_records` have been | ||
| /// followed so far. |
There was a problem hiding this comment.
If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code
| /// What the recursive form of `has_dynamic_exports_due_to_export_star` | ||
| /// returned immediately upon entering a file, if anything. |
There was a problem hiding this comment.
If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code
| /// Marks `source_index` as `EsmWithDynamicFallback` if it transitively | ||
| /// `export *`s from a file whose exports are not statically analyzable | ||
| /// (CommonJS, or an unresolved external `export *`), along with every file | ||
| /// on the `export *` path to that file. Returns whether `source_index` has | ||
| /// dynamic exports. | ||
| /// | ||
| /// Explicit-stack DFS (was per-edge recursive, one call per `export *`). | ||
| /// In the recursive form, finding such a file returned `true` through | ||
| /// every frame on the way back up, and each of those frames marked its | ||
| /// file: the frames on the stack at that moment are exactly the files to | ||
| /// mark. A file whose export stars are exhausted without finding one | ||
| /// returned `false`, which is popping it. |
There was a problem hiding this comment.
If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code
| /// Merge the re-exports reachable through `source_index`'s `export *` | ||
| /// statements into `resolved_exports[target_id]`. | ||
| /// | ||
| /// Explicit-stack DFS (was per-edge recursive, one call per `export *`). | ||
| /// `stack` is the chain of files being followed from `source_index`, which | ||
| /// the recursive form kept as a separate list: a file already on it is a | ||
| /// cycle and is not entered again, and a real export in any file on it | ||
| /// shadows the re-exports found below. |
There was a problem hiding this comment.
If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code
| // Search further through this file's export stars, unless doing so | ||
| // would loop: avoid infinite loops due to cycles in the export star graph |
There was a problem hiding this comment.
If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code
| /// A class (`Ref` local to the CSS file `IndexInt`) on the explicit | ||
| /// stack of `Visitor::visit`. |
There was a problem hiding this comment.
If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code
| /// Every frame names a class of a file that has a CSS AST: the root | ||
| /// comes from `validate_css_import_composes`, and `Enter` only | ||
| /// pushes classes it looked up in the composed file's AST. |
There was a problem hiding this comment.
If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code
| /// Records the properties of `root` and of everything it | ||
| /// (transitively) composes, warning when two files contribute the | ||
| /// same property. | ||
| /// | ||
| /// Explicit-stack DFS (was recursive, one call per composed class). | ||
| /// `Enter` pushes the composed classes in source order followed by | ||
| /// its own `Leave`, then reverses that tail so they pop in source | ||
| /// order, each one's subtree completing before the next pops, and | ||
| /// `Leave` last: the recursion's postorder, which is what decides | ||
| /// which style rule a conflicting property is first seen in. |
There was a problem hiding this comment.
If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code
| // Read-only deref: may be the same | ||
| // allocation as `ast`, so bind shared. |
There was a problem hiding this comment.
If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code
| // Otherwise it is composed from the global scope. | ||
| // | ||
| // See comment above for why we are skipping checking this for now. |
There was a problem hiding this comment.
If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code
robobun
changed the title
There was a problem hiding this comment.
Beyond the inline findings, I traced the four explicit-stack rewrites against the recursive forms they replace and did not find a behavior change: has_dynamic_exports_due_to_export_star drains exactly the frames the old true return path marked, add_exports computes reexports_default and the shadowing check over the same path the old source_index_stack held, the __css_validation Enter/Leave reversal reproduces the old postorder, and the composes emitter in generateCodeForLazyExport.rs only diverges on inputs that previously never terminated (the visited-on-entry change). StaticRouteVisitor's result propagation also matches the old early returns, including caching true for every file on the stack.
Extended reasoning...
The change converts five recursive graph walks in src/bundler/linker_context/ to explicit-stack DFS and adds a StackCheck guard plus a new error message to the CSS @ import order walk, with eight new subprocess tests in test/bundler/bun-build-api.test.ts; it touches no auth, crypto or injection surface. The bug hunt was cut off at its findings cap rather than running dry, and several posted findings concern test coverage and the dev-server path, so approval is not appropriate; this note only records which equivalence checks on the core walks were done and came up clean.
Findings marked 🟡 are optional suggestions and need no follow-up push.
| /// propagates down through every file on the stack, caching `true` for | ||
| /// each, the way the recursive form's early returns did; `false` lets the | ||
| /// file go on to its next import. | ||
| fn has_transitive_use_client_impl( |
There was a problem hiding this comment.
🔴 Maintainers get a rewritten has_transitive_use_client_impl with no test covering a deep or multi-level route import graph, so a regression in the new stack/result propagation would go unnoticed. The function at StaticRouteVisitor.rs:76 replaces recursion with a result-driven stack loop, but the only coverage is the one-level cases in test/bake/dev/production.test.ts and the author's manual check. Fix: add a bake production fixture where the "use client" component sits several server imports deep (and an all-server sibling that stays static) so the true propagation through multiple frames and the false exhaustion path are both asserted.
Why this was flagged
StaticRouteVisitor.rs:76-130 replaces the per-import recursion with an explicit stack and a result flag that pops and caches frames; StaticRouteVisitor.rs:132 adds enter(). The PR text states no new test was added for this walk and that the deep case was checked by hand. The repository review rules say every behavioral change ships an automated test and that manual verification does not count. The existing coverage the PR cites (a page importing a "use client" component directly, and a page without one) never pushes more than one frame, so the loop body at StaticRouteVisitor.rs:88-93 (popping with result == true and caching true for every frame on the stack) and the enter() cycle/cache paths for nested files are unexercised. On the base branch the recursive form was equally untested at depth, but this PR is the one changing it; a wrong propagation here would mark a route static and drop its client script tag in bake production builds without any test failing.
Verification: The new multi-frame result propagation at src/bundler/linker_context/StaticRouteVisitor.rs:89-130 only runs when a bake production route reaches a "use client" file through intermediate server modules. The only test change is test/bundler/bun-build-api.test.ts, which never reaches it; test/bake/dev/production.test.ts all import the "use client" file directly, so that path is unexercised.
| if let Some(TooDeep { | ||
| importer, | ||
| range, | ||
| kind, | ||
| }) = visitor.too_deep | ||
| { | ||
| let importer = &this.parse_graph().input_files.items_source()[importer.get() as usize]; | ||
| // Split-borrow (see `LinkerContext::log_disjoint`). `link()` fails the | ||
| // build once `compute_chunks` returns. | ||
| this.log_disjoint().add_range_error_fmt( | ||
| Some(importer), | ||
| range, | ||
| format_args!( | ||
| "Maximum call stack size exceeded while following this \"{}\" chain", | ||
| if kind == ImportKind::Composes { | ||
| "composes" | ||
| } else { | ||
| "@import" | ||
| } | ||
| ), | ||
| ); | ||
| return Vec::new(); |
There was a problem hiding this comment.
🟡 (optional) Dev server users whose stylesheet trips the new depth check get a silently empty stylesheet served as a successful build, with no error overlay. The too-deep error at findImportedFilesInCSSOrder.rs:396 goes to the transpiler log and the function returns an empty order; the dev-server caller at bundle_v2.rs:6117 builds a CSS chunk from it and keeps the entry in css_entry_points, so the file is recorded as built. Fix: surface the abandoned walk to the caller (e.g. return a Result or a flag) so the dev-server path fails the file the way scan_css_imports errors do at bundle_v2.rs:5921-5934, via handle_parse_task_failure plus swap_remove from css_entry_points, while Bun.build()/bun build keep failing through the linker log.
Why this was flagged
A dev-server project (bake / bun ./index.html dev) has a CSS @ import chain deep enough that StackCheck::is_safe_to_recurse() returns false in visit_import. find_imported_files_in_css_order then logs Maximum call stack size exceeded while following this "@ import" chain on this.log_disjoint() (findImportedFilesInCSSOrder.rs:396) and returns Vec::new(). The dev-server caller finish_from_bake_dev_server at bundle_v2.rs:6117-6133 pushes a Content::Css chunk with an empty imports_in_chunk_in_order and the entry stays in start.css_entry_points, so the incremental graph records the file as successfully bundled. The only trace is DevServer.rs:3289-3295, which prints errors in dev.log to stderr once and attaches nothing to the file, so the browser shows no error overlay. The base branch crashes the process here instead, but the sibling path for CSS import errors at bundle_v2.rs:5906-5934 routes through handle_parse_task_failure and removes the entry from css_entry_points, and this new error does not use it.
Verification: src/bundler/linker_context/findImportedFilesInCSSOrder.rs:388-408 log via this.log_disjoint().add_range_error_fmt(...) and return Vec::new(). The dev-server caller src/bundler/bundle_v2.rs:6117-6134 takes the empty order, builds a Content::Css chunk from it, and leaves the entry in start.css_entry_points. No insert_failure is made, so no error overlay.
| "a chain of 1000 CSS @imports either bundles or fails with a build error", | ||
| async () => { | ||
| // `find_imported_files_in_css_order` still recurses per @import, but it | ||
| // checks the remaining stack first. How deep it gets before giving up | ||
| // depends on the build (a debug build crashed between 300 and 400 files, | ||
| // a release build gets past 1000), so both outcomes are valid here; what | ||
| // is not is the child dying. | ||
| const length = 1000; | ||
| using dir = tempDir("build-api-css-import-chain-deep", cssImportChain(length)); | ||
| const result = await buildDeepGraphInChild(String(dir), { entrypoints: ["c0.css"] }); | ||
| if (result.success) { | ||
| expect(result.logs).toEqual([]); | ||
| expect(result.outputs).toBe(1); | ||
| expect(cssRuleOrder(result.entryText!)).toEqual(Array.from({ length }, (_, i) => length - 1 - i)); | ||
| } else { | ||
| expect(result.outputs).toBe(0); | ||
| expect(result.logs).toEqual([ | ||
| { | ||
| message: 'Maximum call stack size exceeded while following this "@import" chain', | ||
| file: expect.stringMatching(/^c\d+\.css$/), | ||
| lineText: expect.stringContaining("@import"), | ||
| notes: [], | ||
| }, | ||
| ]); | ||
| } | ||
| }, |
There was a problem hiding this comment.
🟡 (optional) Maintainers get a regression test for the new @ import stack check that cannot fail on a release build, before or after this change. The test at test/bundler/bun-build-api.test.ts:1140 accepts either outcome, and with 1000 files a release build (which the PR says only overflowed at 4000-8000 files) bundles everything on both the base and the fixed binary, so the TooDeep error path, its message and the empty-order return are never exercised there. Fix: make the error branch deterministic, for example a chain long enough to trip the check on release too or an internal knob that lowers the threshold, and assert that branch unconditionally; the same debug-only sizing applies to the 1000-class composes chain and both 1500-file export star chains. [also at: test/bundler/bun-build-api.test.ts:1150 - nit: the one test for the new Maximum call stack size exceeded while following this "@ import" chain error only asserts that message when the build happens to fail, so on release lanes the error path is never checked.]
Why this was flagged
The test "a chain of 1000 CSS @ imports either bundles or fails with a build error" at test/bundler/bun-build-api.test.ts:1140-1166 branches on result.success. The PR text states a release build of the unfixed code dies only between 4000 and 8000 files, and the fixed release build bundles all 1000, so on release both the base and the fixed binary take the success branch and the test passes either way; USE_SYSTEM_BUN=1 (CLAUDE.md line 108) therefore passes against the base. The new code at src/bundler/linker_context/findImportedFilesInCSSOrder.rs:133-140 (too_deep set) and :388-408 (add_range_error_fmt, return Vec::new()) only runs when the debug lane trips at file 287, so the release lane never checks the message, the reported file, or that the empty order does not break compute_chunks. The same sizing-to-debug applies to the composes chain (1000 classes, line 1168) and the export star chains (1500 files, lines 1266 and 1281).
Verification: On a release build, a 1000-file @ import chain finishes without tripping StackCheck. test/bundler/bun-build-api.test.ts:1150-1164 branches on result.success, so whichever branch the binary takes, the test passes; on a release build both the base and the fixed binary take the success branch and never exercise the new TooDeep path in src/bundler/linker_context/findImportedFilesInCSSOrder.rs.
| "a chain of 1000 CSS @imports either bundles or fails with a build error", | ||
| async () => { | ||
| // `find_imported_files_in_css_order` still recurses per @import, but it | ||
| // checks the remaining stack first. How deep it gets before giving up | ||
| // depends on the build (a debug build crashed between 300 and 400 files, | ||
| // a release build gets past 1000), so both outcomes are valid here; what | ||
| // is not is the child dying. | ||
| const length = 1000; | ||
| using dir = tempDir("build-api-css-import-chain-deep", cssImportChain(length)); | ||
| const result = await buildDeepGraphInChild(String(dir), { entrypoints: ["c0.css"] }); | ||
| if (result.success) { | ||
| expect(result.logs).toEqual([]); | ||
| expect(result.outputs).toBe(1); | ||
| expect(cssRuleOrder(result.entryText!)).toEqual(Array.from({ length }, (_, i) => length - 1 - i)); | ||
| } else { | ||
| expect(result.outputs).toBe(0); | ||
| expect(result.logs).toEqual([ | ||
| { | ||
| message: 'Maximum call stack size exceeded while following this "@import" chain', | ||
| file: expect.stringMatching(/^c\d+\.css$/), | ||
| lineText: expect.stringContaining("@import"), | ||
| notes: [], | ||
| }, | ||
| ]); | ||
| } | ||
| }, | ||
| deepGraphTimeout, |
There was a problem hiding this comment.
🟡 nit (optional): maintainers get regression coverage for only one of the three edges the new stack guard protects, so the other two can regress unnoticed. findImportedFilesInCSSOrder.rs:134 guards plain @ import, conditional @ import (line 235, the nested_conditions fork) and cross-file composes (line 319), and the "composes" wording at line 401 is never produced by any test. Fix: add sibling fixtures alongside the 1000-file @ import chain, one chain of @ import "./next.css" screen; and one chain of .c { composes: c from "./next.module.css" }, each asserting the exact message for its kind, so every guarded edge and both message variants are exercised.
Why this was flagged
The guard in findImportedFilesInCSSOrder.rs:134-141 is reached from three call sites: the conditional @ import branch at findImportedFilesInCSSOrder.rs:235 (which also runs the nested_conditions ManuallyDrop path and the drop(nested_import_records) at line 244 before the early return at line 246), the plain @ import branch at line 251, and the composes branch at line 319. The message at line 400-406 picks "composes" or "@ import" from kind. The only new fixture that can reach the guard is cssImportChain at test/bundler/bun-build-api.test.ts:1109, which emits unconditional @ import "./cN.css"; lines, so only the line 251 call site and the "@ import" wording are ever executed by the suite. A regression in the composes branch (for example dropping the !self.visit_import(...) return at line 319-327, which would resume the walk after too_deep is set) or in the conditional branch would still pass every test here. REVIEW.md asks that every sibling entry point receiving the same fix be covered; the base branch has no coverage either, but the base also has no guard to regress.
Verification: nit. Triggering condition: whenever the conditional-@ import fork or the cross-file composes edge of the new stack guard regresses, no test in this PR can catch it. The only new fixture that can reach the guard is cssImportChain (test/bundler/bun-build-api.test.ts:1116-1122), which emits bare @ import "./c${i+1}.css"; — plain branch only.
| test.concurrent( | ||
| "bundles a chain of 2200 chunks that import each other", | ||
| async () => { | ||
| // With splitting, every import() target becomes its own chunk, and each | ||
| // chunk's hash covers the chunks it imports. That walk | ||
| // (`append_isolated_hashes_for_imported_chunks`) used to recurse per |
There was a problem hiding this comment.
🟡 (optional) Maintainers get a 2200-file splitting build in the suite that passes on the base branch and names a function that does not exist. The comment at test/bundler/bun-build-api.test.ts:1313 cites append_isolated_hashes_for_imported_chunks, but no such symbol is in the tree and this diff does not touch chunk hashing (LinkerContext.rs:2660 already computes hashes with an iterative closure). The PR text's claim that chunk hashing became an explicit-stack DFS is not in the diff. Fix: either drop this test and the chunk-hashing claim, or include the chunk-hashing change it is meant to guard and name the real function; a test that cannot fail on base only adds a slow debug/ASAN build. [also at: test/bundler/bun-build-api.test.ts:1334 - Maintainers get a 2200-file splitting build test that guards nothing this PR changes and may pass on the base branch, costing 5-20s per debug run.; test/bundler/bun-build-api.test.ts:1335 - nit: maintainers get a 2200-file splitting build in every test run that guards nothing this PR changes. The comment at test/bundler/bun-build-api.test.ts:1313 names append_isolated_hashes_for_imported_chunks, which does not exist on the base or after this diff; chunk hashing is already a bitset fixpoint in final_chunk_hashes (src/bundler/LinkerContext.rs:2654).]
Why this was flagged
The test "bundles a chain of 2200 chunks that import each other" at test/bundler/bun-build-api.test.ts:1308 writes 2200 modules with import() chains and builds with splitting: true. Its comment at line 1313 says the walk append_isolated_hashes_for_imported_chunks used to recurse per imported chunk; a repo-wide grep finds no such symbol, and the only cross-chunk hash code is the non-recursive reachability closure in src/bundler/LinkerContext.rs:2660-2750, which this diff leaves untouched. So the base branch passes this test unchanged, meaning it certifies nothing in this PR while adding a 2200-file splitting build with a 120_000 ms timeout to every debug/ASAN run. The PR description's bullets about append_isolated_hashes_for_imported_chunks, Enter/Asset/Leave frames and &[Chunk] describe code absent from the diff, so the description disagrees with the code.
Verification: Every suite run executes this 2200-file splitting build. append_isolated_hashes_for_imported_chunks appears only in the test comment at test/bundler/bun-build-api.test.ts:1313; no such function exists. Chunk hashing in LinkerContext::final_chunk_hashes (src/bundler/LinkerContext.rs:2654) is already a non-recursive bitset fixpoint; the base commit bbdc5a5 contains the identical function.
| // chains build in well under a second on a release build but take 5-20s on | ||
| // a debug build with ASAN (more when the tests run concurrently), hence the | ||
| // timeout. | ||
| const deepGraphTimeout = 120_000; |
There was a problem hiding this comment.
🟡 nit (optional): Eight new tests each carry a 120 s per-test timeout (deepGraphTimeout, test/bundler/bun-build-api.test.ts:1055), which test/CLAUDE.md forbids ("Do not set a timeout on tests") and REVIEW.md says not to raise to make slow tests pass. The file already has three such outliers, so this is precedent, not a blocker. Fix: keep the chains at the minimum length that still overflows the 2 MiB stack on the unfixed build and drop the explicit timeout where the default suffices, or state in the test why each outlier needs it.
Why this was flagged
Each test.concurrent added between test/bundler/bun-build-api.test.ts:1113 and :1334 passes deepGraphTimeout (120_000) as its third argument. test/CLAUDE.md states tests must not set timeouts and REVIEW.md says to shrink the workload rather than raise the timeout. On a debug+ASAN runner the author reports 5-20s per test, so the suite's wall time for this file grows by minutes; on the base branch the file has only three timeout-bearing tests (lines 2383, 2445, 2514). No functional defect; this is a convention slip the repository's own rules call out.
Verification: nit. Triggering condition: whenever this test file runs. test/bundler/bun-build-api.test.ts:1055 declares const deepGraphTimeout = 120_000; and it is passed as the third argument to all eight new test.concurrent calls. test/CLAUDE.md:118-120 reads "Do not set a timeout on tests. Bun already has timeouts." The base branch has five timeout-bearing tests, not three. No functional defect.
This branch has not been deployed
2 participants
Problem
Bun.build()on a chain of CSS files where each one@imports the next dies with a native stack overflow once the chain is a few hundred files long (debug+ASAN:AddressSanitizer: stack-overflowinfind_imported_files_in_css_order::Visitor::visit, src/bundler/linker_context/findImportedFilesInCSSOrder.rs). Release builds die between 4000 and 8000 files. Bun 1.3.14 built the same project.@importorder,Visitor::visitin findImportedFilesInCSSOrder.rs (between 300 and 400 files)export *resolution,ExportStarContext::add_exportsin scanImportsAndExports.rs (between 1050 and 1100 files);DependencyWrapper::has_dynamic_exports_due_to_export_starwalks the same graphLinkerContext::append_isolated_hashes_for_imported_chunks(about 1560 chunks, withsplittingand a chain ofimport()s). This one is gone on main: bundler: one bundle-wide name per cross-chunk binding (no moreexport {x as y}/import {y as z}between chunks) #40518 replaced that walk withfinal_chunk_hashes, which does not recurse.composes: the property conflict check in scanImportsAndExports.rs and the exports object generation in generateCodeForLazyExport.rs (between 600 and 900 classes)StaticRouteVisitor(bake production builds) recurses per import of the route's JS import graphcomposescycle that does not pass through the class being exported (.x { composes: b } .b { composes: c } .c { composes: b }) recursed forever. That one crashes release builds on a three line file.Fix
export *(both walks), bothcomposeswalks andStaticRouteVisitorbecome explicit-stack DFS, the pattern bundler: convert per-edge-recursive JS-import-graph walks to explicit-stack DFS #34554 used: a frame per node, successors pushed in discovery order and the pushed tail reversed (or a per-frame cursor advanced one edge at a time), so nodes are visited, hashed, appended and diagnosed in exactly the order the recursion produced. Stack usage is now constant per walk; the depth lives in aVec.has_dynamic_exports_due_to_export_star: the recursive form returnedtruethrough every frame on the way up and each frame marked its file, so on finding a dynamic file the whole stack is marked and drained. It still returns that result: since bundler: lift module.exports = require() after side effects in unwrapped packages to export * #41188 the caller reads it.add_exports: the frame stack is also the import path the recursive form kept insource_index_stack, used for the cycle check and the shadowing check.bbdc5a519e. The chunk hashing conversion is no longer in this PR (see Problem).add_exportskeeps thedefaultrule main added for lifted CommonJS files (bundler: keep the default export of a lifted module.exports = require() #41298): it readsreexports_defaultfrom the frame stack.generate_code_for_lazy_export: a class is now marked on the way in. For every input that terminated before, the appended names are the same in the same order (a class could only be re-entered while still in progress through a cycle avoiding the root, and those inputs never terminated); for those cycles the walk now terminates.StaticRouteVisitor:resultcarries a finished file's answer to the file below it;truefinishes and caches every file on the stack, as the recursive early returns did.@importorder walk stays recursive but callsStackCheck::is_safe_to_recurse()before following each@import(or cross-filecomposes) edge. When it fails, the walk is abandoned andMaximum call stack size exceeded while following this "@import" chainis logged on the importing file at the import;link()already fails the build when the log has errors aftercompute_chunks(src/bundler/LinkerContext.rs:799), soBun.build()reports aBuildMessageandbun buildexits 1 with a code frame. This walk threads arena-backed condition lists through the recursion (thebitwise_copy/ManuallyDropinvariants in that file), so the explicit-stack rewrite is not worth its risk for a limit that is thousands of files deep in release builds; the check is the fallback bundler: give the bundle thread a 16 MiB stack #38862's review asked for where a rewrite is impractical.BundleThread::thread_maincallsconfigure_named_thread, which initializes the per-thread stack bounds (bun_core::output,StackCheck::configure_thread); the CLI's main thread is configured inbun_bin. The check leaves 128 KiB (256 KiB on Windows) of headroom, which is far more than onevisitframe plus what it calls.finish_from_bake_dev_server); there the error lands indev.log, which the dev server prints, and the bundle finishes with an empty order for that stylesheet instead of crashing.Bun.build()in a child process so an overflow shows up as a signal). On a debug build of mainbbdc5a519ewithout the src changes, five of them fail (the 1000-file@importchain, the 1000-class composes chain, the composes cycle, both 1500-fileexport *chains). The 2200-chunk chain passes there since bundler: one bundle-wide name per cross-chunk binding (no moreexport {x as y}/import {y as z}between chunks) #40518, and so do the two order guards (50-file@importchain, composes conflict through a chain). With the changes all eight pass.bundler_cjs2esm.test.tsandbundler_cjs.test.ts(the lifted CommonJS cases) give the same results as on main. The@importtest accepts either outcome because where the check trips depends on the build: the debug build reports the error at file 287, release builds bundle all 1000.export *tests run the bundled output (esm tail/cjs tail), the composes chain test checks the generated class list order, the conflict test checks which file is reported as the first definition (postorder), and the cycle test checks the generated class lists.StaticRouteVisitorhas no new test: it only runs in bake production builds, and test/bake/dev/production.test.ts already covers both answers (a page importing a"use client"component gets a script tag, a page without one does not). I also built a page whose client component sits three server components deep with this branch and checked the script tag is emitted, and the all-server variant stays static; the bake production tests take about 10s each on a debug build, so I did not add that fixture to the file.add_exportsstill scans the current path once per edge for the cycle check, as it did before, so the deepexport *tests take several seconds on a debug build (the new tests have a 120s timeout; all eight take well under a second each on a release build). Also not changed:match_import_with_exportrecurses once per ambiguousexport *alternative and loops forever on an ambiguous cycle; that is a separate bug and has been reported separately.Background
Bun.build()does not bundle on the JS thread; the work is queued to a single long-lived thread started in src/bundler/BundleThread.rs, and the linker (everything under src/bundler/linker_context/) runs there.std::thread::Builderwithoutstack_sizegives it 2 MiB.bun buildfrom the CLI runs the same code on the main thread (8 MiB on Linux), which is why the CLI needs about four times the chain length to fail.StackCheck(src/bun_core/util.rs): captures the current thread's stack end (WTF'sStackBounds, set up per thread byconfigure_thread) andis_safe_to_recurse()compares it with the current stack pointer; it is how the parser, printer and the JSON/TOML/YAML parsers turn deep input into an error instead of a crash. On a thread that never configured it, it always reports safe.Vecof frames. To keep depth-first postorder (visit everything a node depends on, then the node), a node'sEnterpushes its successors followed by its ownLeave, then reverses that slice so the first successor pops first andLeavepops last; walks that must interleave side effects with the descent instead keep a cursor in the frame and handle one edge per loop iteration.@importgraph (the deepest import first), which is what the order walk computes before the later passes dedupe repeated files. This is the walk that now reports an error when the chain is deeper than the stack allows.export *resolution: for every file, the linker merges the named exports of eachexport * fromtarget (and transitively theirs) into the file's resolved exports, skipping names shadowed by a real export of any file on the path and recording names supplied by two different targets as ambiguous. Files thatexport *from something not statically analyzable (CommonJS, or an unresolved external) are markedEsmWithDynamicFallbackand re-export at runtime instead; that marking is the first of the twoexport *walks.composes(CSS modules): a class's exported value is its own generated name preceded by the names of every class it composes, transitively, across files. The linker walks that graph twice: once to warn when two files composed together set the same property, and once to generate the exports object. Both are walks over classes, so a long chain of classes is enough to make them deep.