[Jarred-Sumner]

Part of the clone-tracer sweep.

Callsite: src/runtime/server/server_body.rs:2532 — BunString::clone_utf8(url.href) inside on_fetch (the Server.prototype.fetch() host function used by tests/SSR helpers, not the per-request hot path).

Tracer: 0 hits / 0 bytes in the bundler workload (programmatic-only path), so this is a tidy-up rather than a perf win — but it also removes a dead URL::parse.

Change

• Drop the second URL::parse(&owned_url_buf). URL::parse (src/url/lib.rs:618) always sets href to its full input, and nothing between the re-parse and the clone reads any other url field, so the re-parse produced exactly owned_url_buf and was dead work.
• Instead of clone_utf8(url.href) (allocate a new WTFStringImpl, copy bytes, drop the Vec), hand the already-heap-owned Vec<u8> to BunString::create_external_globally_allocated_latin1 so WTF::ExternalStringImpl adopts the buffer zero-copy and frees it via the global allocator when GC collects the Request.
• Gated on strings::is_all_ascii: clone_utf8 decodes UTF-8, while create_external_..._latin1 reinterprets bytes as Latin-1. Non-ASCII URLs (rare but reachable via JSValue::to_slice) keep the clone_utf8 fallback so multibyte sequences aren't mangled.

Safety

• owned_url_buf is a global-allocator Vec<u8> in both branches (strings::append(..).into_vec() / temp_url_str.to_vec()), matching the helper's mi_free finalizer contract — same pattern as RuntimeTranspilerStore and webcore/encoding.rs.
• Runs on the JS thread; Request.url is a plain WTFStringImpl (not Atom), so no per-thread atom-table hazard.
• The early-return on invalid body still drops owned_url_buf as a normal Vec.
• No new unsafe.

Tests

bun bd test test/js/bun/http/bun-server.test.ts -t "server.fetch"             # 2 pass
bun bd test test/js/bun/http/bun-serve-fetch-invalid-args.test.ts             # 1 pass
bun bd test test/js/bun/http/bun-serve-routes.test.ts -t "server.fetch"       # 1 pass

The full bun-server.test.ts run has one pre-existing failure (should not use 100% CPU when websocket is idle) that also fails with an origin/main build on this machine and is unrelated to URL handling.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: 4bae9369-9265-4861-b3f1-3486db2ebdb1

📥 Commits

Reviewing files that changed from the base of the PR and between af0f6ed and bbeaa7e.

📒 Files selected for processing (1)

• src/runtime/server/server_body.rs

---

Walkthrough

This PR optimizes URL handling in NewServer::on_fetch by eliminating redundant URL parsing. The code now parses the input URL once, selects an owned or borrowed buffer based on hostname presence, and constructs the request URL string with appropriate encoding before passing it to Request::init2.

Changes

URL parsing and encoding optimization

Layer / File(s)	Summary
URL parse once and build request URL src/runtime/server/server_body.rs	Input URL is parsed once with URL::parse, then owned_url_buf is computed conditionally (owned joined base+pathname if hostname is empty; otherwise borrowed original slice). Request URL string is constructed from owned_url_buf using external Latin1 BunString for all-ASCII bytes or UTF-8 clone otherwise, avoiding reparse and passing directly into Request::init2.

🚥 Pre-merge checks | ✅ 4

✅ Passed checks (4 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title accurately describes the main change: replacing clone_utf8 with ExternalString adoption for URL buffers in Server.fetch().
Description check	✅ Passed	The description covers what the PR does, verification via targeted tests, and detailed technical justification, but lacks a formal 'How did you verify your code works?' section.
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.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

---

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[robobun]

^{Updated 5:16 PM PT - May 19th, 2026}

❌ @Jarred-Sumner, your commit bbeaa7e has 1 failures in Build #56250 (All Failures):

• test/regression/issue/03830.test.ts - code 1 on 🐧 13 x64-asan

---

🧪   To try this PR locally:

bunx bun-pr 31058

That installs a local version of the PR into your bun-31058 executable, so you can run:

bun-31058 --bun

[github-actions]

This PR may be a duplicate of:

1. server: borrow URL via Cow instead of to_vec() in server.fetch(string) #31062 - Both optimize the same URL string copy in server.fetch(string) path; server: borrow URL via Cow instead of to_vec() in server.fetch(string) #31062 uses Cow to avoid to_vec() while server: adopt URL Vec into ExternalString in Server.fetch() instead of clone_utf8 #31058 adopts the Vec into an ExternalString

🤖 Generated with Claude Code

[claude]

I didn't find any issues — the reasoning checks out (URL::parse does set href = base, and the adoption matches the webcore/encoding.rs pattern) — but since this changes buffer ownership across the WTF::ExternalStringImpl FFI boundary in server code, it's worth a quick human sanity check rather than a bot auto-approve.

Extended reasoning...

Overview

This PR touches a single function, on_fetch in src/runtime/server/server_body.rs (the Server.prototype.fetch() host function). It removes a redundant second URL::parse(&owned_url_buf) call and replaces BunString::clone_utf8(url.href) with BunString::create_external_globally_allocated_latin1(owned_url_buf) for ASCII URLs, falling back to clone_utf8 for non-ASCII. Net diff is ~20 lines.

I verified the supporting claims:

• URL::parse (src/url/lib.rs:618) sets href = base unconditionally, and nothing between the removed re-parse and the use site reads any other url field, so the second parse was indeed dead.
• create_external_globally_allocated_latin1 (src/bun_core/string/mod.rs:414) takes Vec<u8> by value and transfers ownership to a WTF::ExternalStringImpl whose finalizer calls mi_free on the base pointer.
• Both strings::append(..).into_vec() (via Box<[u8]> from a global-allocator Vec) and temp_url_str.to_vec() produce global-allocator Vec<u8>s, satisfying the allocator contract.
• The same adoption pattern is already used at src/runtime/webcore/encoding.rs:256,267,283.
• The ASCII gate is correct: clone_utf8 decodes UTF-8 while the external-latin1 path reinterprets bytes 1:1, so non-ASCII must keep the UTF-8 path.
• The early-return on invalid body (line ~2524) happens before adoption, so owned_url_buf is dropped normally as a Vec there.

Security risks

None apparent. No new unsafe, no parsing changes, no auth/permissions surface. The risk profile is memory-safety (use-after-free / allocator mismatch) rather than security; that risk is mitigated by reusing an established, already-shipping helper.

Level of scrutiny

Moderate. The change is small, well-reasoned, and follows an existing pattern, but it shifts a heap buffer's lifetime from "freed at end of Rust scope" to "freed by WebKit's GC via an ExternalStringImpl finalizer." Ownership transfers across an FFI boundary into GC-managed memory in server code are the kind of thing that benefits from a second pair of human eyes even when they look right — a subtle allocator or threading mismatch here would manifest as a hard-to-reproduce crash, not a test failure.

Other factors

• The PR description is unusually thorough on safety invariants and cites the relevant precedent callsites.
• Targeted tests pass (bun-server.test.ts -t "server.fetch", bun-serve-fetch-invalid-args.test.ts, bun-serve-routes.test.ts -t "server.fetch").
• Bug-hunting system found nothing.
• This is a non-hot-path tidy-up (0 tracer hits), so there's no urgency pressure that would justify skipping human review.

[Jarred-Sumner]

CI failures here are all tagged [pre-existing]/[flaky] by ci:errors — none are [new]. Retried.

[claude]

LGTM — verified URL::parse sets href = input so the dropped re-parse was dead, and the owned Vec comes from the global allocator matching the safe create_external_globally_allocated_latin1 contract.

Extended reasoning...

Overview

Single-file change in src/runtime/server/server_body.rs inside on_fetch (the Server.prototype.fetch() host function, programmatic path only — not the per-request hot path). Two pieces: (1) drop a redundant second URL::parse(&owned_url_buf) whose only consumed field was href, and (2) when the joined-URL arm produces an owned all-ASCII Vec<u8>, hand it to BunString::create_external_globally_allocated_latin1 instead of copying via clone_utf8. Borrowed and non-ASCII inputs keep the clone_utf8 fallback.

Verification

• src/url/lib.rs:618 confirms URL::parse unconditionally sets url.href = base (the full input), so the removed re-parse only reproduced owned_url_buf and was dead work — nothing between the re-parse and the old clone_utf8(url.href) read any other url field.
• strings::append (src/bun_core/string/immutable.rs:1683) builds via Vec::with_capacity → into_boxed_slice(), so .into_vec() yields a global-allocator (mimalloc) Vec<u8>, satisfying the helper's documented contract (src/bun_core/string/mod.rs:410-431). The helper is a safe pub fn taking Vec<u8> by value with no unsafe at the callsite, and is already used identically in src/runtime/webcore/encoding.rs:16.
• The is_all_ascii gate is correct: for bytes < 0x80, Latin-1 and UTF-8 are bit-identical, so adopting as Latin-1 preserves the string the old clone_utf8 would have produced; non-ASCII falls back unchanged.
• The early-return on invalid body still drops owned_url_buf as a normal Cow/Vec (the adoption happens after that block), so no leak on the error path.

Security risks

None. No user-controlled parsing changes, no new unsafe, no auth/crypto. The ownership transfer is encapsulated by an existing safe wrapper whose finalizer contract matches the allocator of the input.

Level of scrutiny

Low-to-moderate. Memory ownership across FFI is normally worth a careful look, but this uses an established safe helper rather than open-coding the transfer, the reasoning in the PR description is precise and checks out against the source, and the path is a test/SSR helper rather than the request hot path.

Other factors

No CODEOWNERS for this path. Bug-hunting system found nothing. Targeted server.fetch tests pass per the description, and the author confirmed CI failures are pre-existing/flaky. A sibling PR (#31062) touches the same callsite with a different approach — that's a merge-coordination question, not a correctness concern for this diff.

[claude]

Thanks for the rebase — I was wrong about the rust.ts hunk (it was already on main; stale base on my end). The remaining server_body.rs change LGTM.

Extended reasoning...

Overview

Single-file change to src/runtime/server/server_body.rs in on_fetch (the programmatic Server.prototype.fetch() helper, not the per-request hot path). It (1) drops a redundant second URL::parse(&owned_url_buf) whose only consumer was url.href, which by construction equals the input buffer, and (2) when the joined-URL arm produced an owned all-ASCII Vec<u8>, hands it to BunString::create_external_globally_allocated_latin1 (zero-copy adoption into a WTF::ExternalStringImpl) instead of clone_utf8. Borrowed and non-ASCII inputs keep the clone_utf8 fallback.

Correction on prior review

My earlier comment claimed commit af0f6ede introduced an out-of-scope scripts/build/rust.ts hunk. The author pointed out the -Zthreads=8 block was already on origin/main (via #31096) and only appeared as an addition because the PR base predated it. After rebase the diff is server_body.rs-only, confirming the author was right and I was reading a stale base. That thread is resolved.

Security risks

None identified. No new unsafe; the FFI ownership transfer is fully encapsulated in the existing safe wrapper BunString::create_external_globally_allocated_latin1 (src/bun_core/string/mod.rs:414), which already handles the ManuallyDrop / mi_free-finalizer contract and is used identically in webcore/encoding.rs. The owned Vec<u8> originates from strings::append(..).into_vec() — a global-allocator Vec::with_capacity round-tripped through Box<[u8]> — so it satisfies the wrapper's allocator contract. The is_all_ascii gate ensures Latin-1 reinterpretation is byte-identical to the previous UTF-8 decode; non-ASCII URLs fall back to clone_utf8 so multibyte sequences are not mangled. The early-return on invalid body still drops owned_url_buf via normal Cow/Vec Drop.

Level of scrutiny

Low–moderate. The change touches string lifetime across the Rust/C++ boundary, which would normally warrant caution, but it does so strictly through an established safe API that already enforces the invariant centrally. The dead-URL::parse removal is a straightforward observation that nothing between the re-parse and the consumer read any field other than href, and href == input. Net diff is ~20 lines, no control-flow changes, no new abstractions.

Other factors

Targeted tests pass (bun-server.test.ts -t "server.fetch", bun-serve-fetch-invalid-args.test.ts, bun-serve-routes.test.ts -t "server.fetch"); CI failures are tagged pre-existing/flaky per the author. The bug-hunting system found no issues. The PR description includes a thorough safety analysis that matches what I verified in the source.