Skip to content

bake: fix a plugin leak, ignoreDirs parsing, and a router crash - #45605

Merged
alii merged 4 commits into
mainfrom
ali/bake-config-review-fixes
Oct 16, 2026
Merged

alii merged 4 commits into
mainfrom
ali/bake-config-review-fixes

Conversation

@alii

@alii alii commented Oct 16, 2026 •

Copy link
Copy Markdown
Member

This PR fixes three bugs in the Bake config and router. Review bots found them on #45489. They were in the code before that PR.

The bugs

  1. A rejected app config leaked the bundler plugin. Framework::from_js creates the plugin when the framework has plugins. If a later step failed (for example root: 123), UserOptions::from_js returned the error, and nothing unprotected the plugin's JS object. Only the Drop of UserOptions freed it, and that value was never built.
  2. fileSystemRouterTypes[].ignoreDirs was ignored. The parser walked the outer fileSystemRouterTypes array instead of the ignoreDirs array. When a framework set ignoreDirs, the router got one "[object Object]" entry per router type in its place, so it scanned every directory, including .git and node_modules.
  3. A route with more than 64 params crashed the dev server. The router wrote the error message but set no cursor, so TinyLog::print sliced the path from index u32::MAX.

The fixes

  1. SplitBundlerOptions::destroy_plugin frees the plugin. The Drop of UserOptions calls it, and UserOptions::from_js also calls it when parsing fails after the plugin exists.
  2. The parser reads ignoreDirs from its own array. The error message no longer says that "*" is allowed, because it is not.
  3. The error highlights the full route path.

Tests

test/bake/app-options.test.ts has one test for each bug. All three fail on Bun 1.4.3:

  • the plugin test counts 10 leaked BundlerPlugin objects instead of 0;
  • the ignoreDirs test reports the invalid route in the ignored directory;
  • the 64-params test crashes with range start index 4294967295 out of range for slice of length 389.

On a local debug build, app-options, framework-router, dev/bundle and dev/html pass.

@alii

alii commented Oct 16, 2026

Copy link
Copy Markdown
Member Author

@robobun adopt

@robobun

robobun commented Oct 16, 2026 •

Copy link
Copy Markdown
Collaborator
Updated 5:05 PM PT - Oct 16th, 2026

@alii, your commit 8c1ba4c is building: #126923

Base automatically changed from ali/bake-restructure to main October 16, 2026 22:46

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

Nothing blocking. The comments below are optional suggestions. There is no need to push a fix for them before merging.

Beyond the inline findings, I also checked two sibling concerns and ruled them out: parse_plugin_array stores the plugin in self.plugin before its first fallible call, so the single destroy_plugin() in the Err arm of UserOptions::from_js covers failures inside Framework::from_js and the top-level plugins parse too; and the other on_router_syntax_error site (src/runtime/bake/router/mod.rs:676) is not affected by the cursor bug, since every Err out of style.parse goes through TinyLog::fail, which sets cursor_at/cursor_len.

Extended reasoning...

The change touches the Bake config parser (src/runtime/bake/config/mod.rs, bundler.rs, framework.rs) and the file-system router (src/runtime/bake/router/mod.rs), plus three new subprocess tests; it touches no auth, crypto, or injection surface. Three inline findings were reported (an unvalidated ignoreDirs entry type, a vacuous-on-success leak test fixture, and a pre-existing plugin leak on the accepted-config path), so this note only records the sibling plugin-lifetime and router-cursor paths that were examined and found unaffected.

Comment thread src/runtime/bake/config/framework.rs Resolved
Comment thread test/bake/app-options.test.ts Resolved
Comment thread src/runtime/bake/config/bundler.rs Resolved
@alii
alii force-pushed the ali/bake-config-review-fixes branch from ee4f15c to ed30f72 Compare October 16, 2026 22:54

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

Code review completed

Nothing new to post: everything this review found is already covered by existing comments on this pull request or didn't merit a separate one.

Still open from earlier reviews (3):

  • Unresolved: 3 minor or pre-existing.

@coderabbitai

coderabbitai Bot commented Oct 16, 2026 •

Copy link
Copy Markdown
Contributor

Review in Change Stack →

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration
  • Configuration used: Repository: oven-sh/bun/.coderabbit.yaml
  • Review profile: ASSERTIVE
  • Plan: Advanced
  • Run ID: d75d04c8-3ffc-46ff-a3d2-34cabe72fefe
📥 Commits

Reviewing files that changed from the base of the PR and between b28212a and ed30f72.

📒 Files selected for processing: 5
  • src/runtime/bake/config/bundler.rs
  • src/runtime/bake/config/framework.rs
  • src/runtime/bake/config/mod.rs
  • src/runtime/bake/router/mod.rs
  • test/bake/app-options.test.ts

Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 8 remain after this review.

Walkthrough

Bake option parsing now destroys plugins when parsing fails. Router option parsing reads ignoreDirs from the correct property, and parameter-limit diagnostics span the full relative path.

Changes

Plugin Cleanup

Layer / File(s) Summary
Plugin cleanup during option parsing
src/runtime/bake/config/bundler.rs, src/runtime/bake/config/mod.rs, test/bake/app-options.test.ts
SplitBundlerOptions now provides plugin cleanup. Option parsing calls it when parsing fails, and UserOptions::drop uses it. A regression test checks that repeated rejected configurations do not change the bundler plugin count.

Router Options and Diagnostics

Layer / File(s) Summary
Read ignoreDirs for route scanning
src/runtime/bake/config/framework.rs, test/bake/app-options.test.ts
ignoreDirs now reads values from its own array property. A test checks that matching directories are excluded from route diagnostics.
Report full paths for parameter-limit errors
src/runtime/bake/router/mod.rs, test/bake/app-options.test.ts
When a route pattern exceeds 64 parameters, the syntax-error log spans the full relative path. A test checks the diagnostic for a 65-parameter route.

Priority: ➖ Normal

Merge Risk: ⚪ Minimal · up to ed30f

No actionable issue remains in the reviewed changes; merge after normal checks.

Pre-merge checks: Passed 4
Check name Status Explanation
Title check Passed The title clearly and concisely summarizes the three main fixes: a plugin leak, incorrect ignoreDirs parsing, and a router crash.
Description check Passed The description explains the three bugs, the corresponding fixes, and the verification results. It does not use the template headings, but it provides the required change summary and testing informati…
Linked Issues check Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check Passed Check skipped because no linked issues were found for this pull request.
Full details: Description check

Explanation

The description explains the three bugs, the corresponding fixes, and the verification results. It does not use the template headings, but it provides the required change summary and testing information.

  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

✨ Finishing Touches: 💡 1
🛠️ Fix failing CI checks: 💡
  • Commit to this branch
  • Create a new PR

@alii
alii merged commit 0b2d346 into main Oct 16, 2026
9 of 10 checks passed
@alii
alii deleted the ali/bake-config-review-fixes branch October 16, 2026 23:35

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

Nothing blocking. The comments below are optional suggestions. There is no need to push a fix for them before merging.

Comment thread test/bake/app-options.test.ts Resolved
springmin pushed a commit to springmin/bun that referenced this pull request Oct 17, 2026
37 upstream commits: SQL consolidation (oven-sh#45304), bun lint/check/fmt rewrite
(oven-sh#45595), b.bytes + bindgen Bun.* functions (oven-sh#45590), WebSocket closing
handshake rewrite (oven-sh#45223), install/yarn.lock migration (oven-sh#45616), YAML.parse
(oven-sh#45593), bake fixes (oven-sh#45605), file-behind-symlink installs (oven-sh#45477),
WebKit bump 87ee10da087b (oven-sh#45567).

Conflicts (14): src/install/{PackageInstall,PackageInstaller}.rs,
src/jsc/bindings/c-bindings.cpp, src/uws_sys/libuwsockets.cpp + 10 tests.
Resolution: upstream structure + re-applied OHOS patches (is_linked_folder
package.json identity, OHOS ELF signing, is_executable_file access(X_OK),
loop post handler) and the serialization policy (bun-audit/lifecycle/
catalogs: describe.concurrent/test.concurrent -> serial).
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