Fix Headers iteration order with set-cookie and null bodies for Response.error/redirect

Closed
RO
2mo ago
+139-45

Two fetch spec compliance fixes in the same area, both verified against Node (undici) and the spec.

Headers iteration hoists set-cookie out of the sorted order

const h = new Headers();
h.append("zz", "1");
h.append("aa", "2");
h.append("Set-Cookie", "s=1");
h.append("set-cookie", "s=2");
h.append("mm", "3");
[...h.keys()];
// bun:       ["aa", "mm", "zz", "set-cookie", "set-cookie"]
// node/spec: ["aa", "mm", "set-cookie", "set-cookie", "zz"]

The spec's sort and combine algorithm sorts header names lexicographically and keeps set-cookie entries uncombined at the name's position in that order. FetchHeaders::Iterator::next() (the single source of ordering for entries, keys, values, forEach, and spread) sorted the other names and then appended a sentinel for the whole set-cookie block (#4048 introduced this: "append set-cookie after sort"), so set-cookie always came last. Header iteration order is observable: request signing, cache keys, and [...headers] snapshots taken on another runtime all diverge.

Fix: after sorting, insert one marker per set-cookie value at the name's sorted position (std::lower_bound), and have the consuming loop emit setCookieHeaders[slot - setCookieKeyIndex] for marker slots. The iterator index is now a plain index into the sorted-and-combined list, which is what WebIDL prescribes when the list mutates between next() calls, so the old sentinel rebasing arithmetic is deleted. Server response serialization (toUWSResponse) walks the header map segments directly, not this iterator, so wire output is unchanged.

Existing expectations that encoded the set-cookie-last order are restored to the spec order: "Set-Cookies init" and "Headers append multiple" in test/js/web/fetch/fetch.test.ts, and headersInitMultiple / headersAppendMultiple in the Deno-ported test/js/deno/fetch/headers.test.ts (upstream Deno asserts the spec order for these today; #4048 had rewritten them to match the old behavior).

Response.error() and Response.redirect() carry a non-null body

Response.error().body;                          // bun: ReadableStream, node/spec: null
Response.redirect("https://example.com/").body; // same

Consequently Response.error().bodyUsed flips to true after .text(), which breaks "is there a body" branching on synthesized responses. Per Response.error() and Response.redirect(), both statics produce a response with a null body.

Both constructors in src/runtime/webcore/Response.rs used BodyValue::Empty (a present, zero-length body: .body materializes an empty ReadableStream and consuming marks it used) instead of BodyValue::Null. new Response() already used Null, which is why it was already correct. The two variants are handled identically everywhere else (server serialization, cloning, emptiness checks), so the change is limited to .body / .bodyUsed semantics.

Verification

New tests, cross-checked against Node v26:

  • test/js/web/fetch/headers.test.ts, "sort and combine": all five iteration surfaces, set-cookie sorted among close neighbors (set-b < set-cookie < set-cookie2 < set-d) while other duplicates combine, set-cookie as the first/last/only entry, and re-iteration after mutation.
  • test/js/web/fetch/fetch.test.ts: "has a null body" for both statics (body === null, bodyUsed stays false across text()).

On an unmodified build the new tests fail (set-cookie entries trail the list; .body is a ReadableStream) and the four updated expectations fail the same way; with the fix, headers.test.ts, deno/fetch/headers.test.ts, headers.undici.test.ts, response.test.ts, cookies.test.ts, and the Headers/Response blocks of fetch.test.ts pass.