Repository navigation
Conversation
|
Updated 11:04 PM PT - Oct 21st, 2026
✅ @robobun, your commit be1b52a551ca328d3a79ad03adfc6aa75479ccca passed in 🧪 To try this PR locally: bunx bun-pr 46009That installs a local version of the PR into your bun-46009 --bun |
|
Status Reproduced on 1.4.3-canary (367d939) and on a release build of Reviewed: the self-review preferred a rename of such output files in the bundler, or a maintainer's answer on the level before a PR. This PR is open so that a maintainer can choose. The Notes compare the two fixes. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
WalkthroughThe change adds literal-path registration to HTTP routers and uses it for bundled files. Route keys distinguish literal bundled-file paths from pattern routes. Tests cover literal asset paths and route behavior across server configurations and protocols. ChangesLiteral bundled-file routes
Suggested reviewers: Priority: ➖ Normal Merge Risk: ⚪ Minimal · up to
|
There was a problem hiding this comment.
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 three things in packages/bun-uws/src/HttpRouter.h and ruled them out: the container handler's urlSegmentVector[0].data() - 1 reconstruction is safe because executeHandlers parses segment 0 before it walks the method node's children and an empty URL stops before the container runs; route reloads go through clearRoutes() (src/runtime/server/server_body.rs:1902, 2005), so a literal entry is never looked up by remove() while literalKeys is off; and pre-creating the container for every method before the handler-id loop does keep the id identical across methods, so cullNode's id shifting stays consistent.
Extended reasoning...
The change adds a literal-key mode to the vendored uWS HttpRouter and routes bundled HTML-import files through it on the h1, h2 and h3 apps, with StaticRouteEntry.path becoming a RouteKey enum; it touches request routing (which URL serves which bytes) but no auth or crypto surface. Four inline findings already signal that a human must look; this note only records the adjacent router lifetime and id-consistency concerns that were examined and did not reproduce.
Additional findings (outside the current diff — GitHub can't attach inline comments there):
-
🟡
src/runtime/server/mod.rs— A user HEAD route whose pattern bytes equal a bundled literal key makes HEAD requests for that file answer from the file's LOW any-method handler instead of the user's handler, and the user's explicit HEAD handler is silently bypassed. src/runtime/server/mod.rs:2491 compares route.route.path.as_bytes() with the Literal bytes without checking the key kind, so path_has_user_head_route is true and the HEAD registration is skipped, but the literal container (HttpRouter.h:81, priority -1 but HIGH) still runs ahead of the user's MEDIUM ":" route and serves on_request for HEAD. Fix: compare kinds as well as bytes (only a RouteKey::Pattern can equal a user pattern), and when a user HEAD route exists for the same URL, do not let the literal container answer HEAD for that key.Why this was flagged
Trigger: a
routesentry with a HEAD handler whose pattern is byte-equal to a bundled output path, e.g. a--compileserver with--asset-naming '[dir]/[name].[ext]'and a:v/b.pngfile. mod.rs:2485-2494 sees RouteMethod::Specific(HEAD) with equal bytes and sets path_has_user_head_route, so ServerConfig.rs:430-431 skips the file's HEAD MEDIUM handler. But ServerConfig.rs:435 still registers any_this for the Literal, which on h1 lands under the HEAD method node's literal container (HttpContext.h:1190 expands "*"); HttpRouter.h:97-102 sorts that container (isHighPriority) before the user's MEDIUM node, so HEAD requests run the file's on_request and the user handler never runs. On the base branch the same user HEAD route and file both lived in the pattern tree and the user's MEDIUM ":" handler ran before the file's LOW handler. Remedy: match kinds in path_has_user_head_route (RouteKey::is_pattern) and, when a user HEAD route covers the key, exclude HEAD from the literal any registration.Verification: Trigger: a
routeskey with an explicitHEADhandler whose bytes equal a bundled output path that has a:/*segment.src/runtime/server/mod.rs:2485-2494computespath_has_user_head_route, so a user HEAD pattern byte-equal to aRouteKey::Literalsets it true. The user's HEAD handler is never reached. On the base branch the user's HEAD handler won.
0882a79 to
8f3621e
Compare
|
On the review note about Built server.
On Runtime import. The file route is any-method. The key has the hash of the file (
The note is right for this case on HTTP/1.1: the file now answers HEAD at its own path. That agrees with the rest of the router. The file answers GET at that path, and a file with an ordinary name also answers HEAD before a user route The check has to compare the bytes and not the kind. A file with an ordinary name stays in the pattern tree, in the same node as a user key with the same bytes. With a check of the kind, the HEAD route of the file would be registered there, and uWS would replace the user HEAD route with it. The Notes of the description have these results. |
A production HTML import, and a server built with --target=bun or --compile, serves each bundled file under the output path of the file. HttpRouter read that path as a route pattern, so a name that starts a segment with ':' or '*' was a parameter or a wildcard. The file answered paths that are not its own, one file answered for another, and a user route of the same shape was replaced. HttpRouter gets a literal mode for add() and remove(). A literal key that a pattern cannot hold byte for byte is one child of a container node per method, and the handler of the container compares the whole URL. route() and executeHandlers() do not change. StaticRouteEntry now says if its path is a pattern or a literal, and every bundled file goes in through StaticRouteEntry::push_bundled_file.
itBundled.skip lists the case in the run. A plain if left it out.
apply_static_route stays where the HTTP/1.1 function was, with its comment unchanged. The HEAD check moves out of the closure.
328a5f2 to
be1b52a
Compare
Problem
Bun.serveregisters each bundled file of a production HTML import as a route pattern. A file named:smile:.pnganswers GET and POST/aboutwith the PNG and animmutablecache header. A dependency can supply the name."/:id"is replaced.HttpRouter::add(packages/bun-uws/src/HttpRouter.h:280) reads every key as a pattern.Fix
StaticRouteEntry.pathis aRouteKey:Patternfor a key ofroutes,Literalfor a bundled file (push_bundled_file).apply_static_routeregisters aLiteralinapp.literal_routes(..). There,HttpRouter::addputs a key that a pattern cannot hold under a container node, whose handler compares the whole URL.route()andexecuteHandlers()are not edited.bun-serve-html.test.ts(7 new),bun-serve-html-manifest.test.ts(3),bundler_html_server.test.ts(1). Self-reviewed: 32 concerns raised, 25 addressed (Notes).Background
uWS::HttpRouteris the route tree behindroutesand thefetchfallback, on HTTP/1.1, HTTP/2 and HTTP/3.--compile,--target=bun), adds one static route for each output file.Downsides
add()costs 1.1% more instructions.StaticRouteEntrygrows from 48 to 56 bytes. The binary grows by 5,888 bytes.:a b.pnganswered/:a%20b-<hash>.pngonly through the pattern. It returns 404 until bundler(html): percent-encode output paths written into src and href, and serve them under that name #41793 lands.Notes
Repro (1.4.3-canary 367d939 and a release build of
main017b243, Linux x64).index.htmlholds<img src="./:smile:.png">, and:smile:.pngis a file next to it.With this PR the last two lines are
200 text/plain;charset=utf-8 null.Other effects on
main. Each one has a test here.*.pnganswers every path at any depth.:a.pngand:b.png, both URLs return the bytes of:a.png.node_moduleswithurl("./*.png"), and an image over 128 KB, makes GET and POST/aboutreturn the image.NODE_ENV=production bun ./index.htmlwith an image*.pngreturns the image at/from the second request. The test mounts the page at/*, as that command does.bun build --target=bun --asset-naming '[dir]/[name].[ext]'::v/b.pnganswers/other/b.png, and*dir/a.pngis not served at all.*takes the place of thefetchhandler for GET.Not affected:
development: true. The dev server serves assets under/_bun/asset/.There is no user report and no issue. A review of #41793 found the defect. It has been there since #16395. The case for a fix is the effect: wrong bytes with a one-year cache header on URLs of the app, in production only, and a dependency can cause it.
The open question: router or bundler.
HttpRouter.h+93/-6path_template_print(src/bundler/options.rs), next to the..rewrite. Not built:smile:-<hash>.pnggets a new name--compilesrc/resolver/standalone_module_graph.rs). The..rewrite is off under--compilefor that reason, so this rewrite needs the same careroute()andexecuteHandlers()untouched, new code inadd()andremove()The self-review of this PR preferred the bundler rewrite, or a maintainer's answer before any PR. The design work before the code chose the router, because the registration is the step that reads a file name as a pattern. If the answer is the bundler, the tests here still apply. They read the URLs from the served page, and only the expected names change.
Self-review: 32 concerns, 25 addressed. Not taken:
/index.htmlfrom servers that have it today.--compiletest that runs on Windows. Nothing here ran on Windows. One test inbun-serve-html.test.tsgets its names from a plugin and runs there.Not fixed here. Each has a different cause.
bun ./:page.html ./other.htmlbuilds the route key/:pageinsrc/js/internal/html.ts./aboutreturns that page.docs/bundler/executables.mdxbuildsrouteskeys fromBun.embeddedFilesnames../, and the router drops its first byte:./pages/index.htmlanswers at/ages/index.html. This PR does not change that. The alias of a page in a directory likea:banswered every/<x>/index.html. It now answers no path."/:id": { GET }, answers the URL of every file route. A file with an ordinary name is included. An any-method route is under the*method node there (Http2Context.h:751,Http3Context.h:86), androute()tries the node of the request method first. HTTP/1.1 puts an any-method route under each method (HttpContext.h:1194). On 1.4.3-canary,routes: { "/logo.png": new Response("file"), "/:id": { GET } }returns the file forGET /logo.pngon HTTP/1.1, and the user route on HTTP/2 and HTTP/3.A user route with the same bytes as the path of such a file. The file answers first at that one path. The user route answers the other paths of its pattern.
maina user HEAD route answered HEAD at the path of the file. With this PR the file answers GET and HEAD there on HTTP/1.1. On HTTP/2 and HTTP/3 the user HEAD route still answers HEAD, because of the method node order above.mainthe HEAD route of the file replaces a user HEAD route with the same bytes, and the user route never runs. With this PR the file answers HEAD at its own path, and the user route answers the other paths.path_has_user_head_route,src/runtime/server/mod.rs:2490) compares the bytes and not the kind. A file with an ordinary name is in the same tree node as a user key with the same bytes, and there a HEAD route for the file would replace the user route.#41793. Both PRs add
StaticRouteEntry::push_bundled_fileat the same place, so git reports a conflict for the one that lands second. The merged function registers both keys as literals:If #41793 lands first, there is no time in which
:a b.pngreturns 404.Other open PRs on
HttpRouter.h(#45400, #35248, #44224, #35249, #37615). The container is a child of a method node withisHighPriorityand the name*/. It has to stay after the WebSocket upgrade routes and before every other child.How the router part works. In literal mode,
add()asksneedsLiteralNode(key): does a segment start with:or*, or does the key have 100 segments. If not, the key goes into the tree as before. If so, the key becomes one child, named by the whole key, of a container node under each method.executeHandlers()already runs a child whose name starts with*as a wildcard, so it runs the one handler of the container for each request. That handler compares the whole URL with the keys and runs the handlers of the key that is equal. On a miss it returns false, and the router goes on.needsLiteralNodesplits the key by itself. A version that calledgetUrlSegmentstopped the inlining ofgetUrlSegmentinto the HTTP/2 and HTTP/3executeHandlersin the release build.Measurements.
Release builds of
main017b243 and of this branch (ThinLTO, Linux x64):executeHandlers(HTTP/1: 124 instructions, HTTP/2 and HTTP/3: 198),getUrlSegment(113) and the 6 functions that hold the inlinedroute()are identical after address normalization (llvm-objdump -d). 0 differing instructions.size: text 106,739,870 to 106,745,758 (+5,888 bytes). data and bss do not change. 3 new C functions.ab -k -c 1, user-mode instructions of the server for each request (300,000 requests minus 100,000):main18,061 to 18,179 in 6 runs, this branch 18,075 to 18,185 in 3 runs. The difference is inside the spread ofmain(118).Harness over
HttpRouter.hwith the release flags oflibuwsockets.cpp(clang 23,-O3 -march=nehalem, libstdc++), and the same with ThinLTO. User-mode instructions fromperf_event_open, 200,000route()calls for each row:route()on a server with no such file,-O3. 0 of 14 rows differ.route()on the same servers with one file that is registered as HEAD + any,-O3. The three rows wheremainruns the wrong handler are the bug.sizeof(Node)88 to 88,sizeof(HttpRouter)3360 to 3360 (probe with the release flags).size_of::<StaticRouteEntry>()48 to 56 (const assert).any) costs 5,288,638 instructions onmainand 5,345,666 here (+1.1%). With the 60 files in literal mode: 5,351,142. Allocations for one file (HEAD + any): 186 and 186 for an ordinary name. 257 for the first:file (149 onmain, as a pattern), 145 for the next (109).mainheader against this header with the literal mode off: 160,000 randomadd/remove/routecalls, 0 mismatches. Property check with literal keys, yielding handlers, upgrade routes, re-add and remove: seeds 1 and 7, 1,200,000route()calls each, 0 violations. Both under ASAN and UBSAN.needsLiteralNodeagainst the split ofgetUrlSegment: 2,000,000 random keys, 0 mismatches.Suites run on the debug build of the final code. All pass:
bun-serve-html.test.ts,bun-serve-html-manifest.test.ts,bundler_html_server.test.ts,bun-serve-routes.test.ts,bun-serve-static.test.ts,serve-directory-routes.test.ts,serve-http2.test.ts,serve-http3.test.ts,websocket-server.test.ts,test/bake/dev/html.test.ts, and the source lints intest/internal/source-lints. In the last run, 3 tests inbun-serve-file.test.ts(pollableBun.file), 3 inserve-protocols.test.ts(HTTP/3 bodies) and 1 inbun-server.test.ts(TLS before the handshake) hit the 5 s timeout on a slow machine. None of them has a bundled file route, and the three files passed in full on the first version of this change.cargo check -p bun_runtimepasses forx86_64-pc-windows-msvcandaarch64-apple-darwin. After the rebase ontomain5738e23, the three test files of this PR,bun-serve-static.test.tsandbun-serve-routes.test.tspass on a new debug build.mainremoved the router of a server name (currentRouter), so the HTTP/1.1 setter now sets the one router of the context.no test proof · iteration 1 · platform-specific test(s) that do not run on this machine, deferring to CI, which covers all platforms: test/js/bun/http/bun-serve-html.test.ts, test/js/bun/http/bun-serve-html-manifest.test.ts, test/bundler/bundler_html_server.test.ts