fix: image optimizer fallback stream reuse

[JaredStowell]

Fix /_vinext/image fallback behavior when image transformation fails after consuming the source stream.

This also fixes a Pages Router dev-mode parity bug where local image redirects did not percent-encode unicode paths in the Location header.

Root cause

The image optimization path on Workers streams the source image into the transform handler. If the transform implementation starts consuming the source stream and then throws, the original response body can no longer be reused safely for fallback.

Fix

1. tries the transform normally
2. tries to serve the original source response directly if transform fails
3. re-fetches the source image only if the fallback path cannot reuse the original body because it was already disturbed/locked

This keeps the normal success path streaming and avoids tee-based buffering, while still recovering correctly from consumed-stream failures.

Changes

• replace clone-based fallback in the shared image optimizer with refetch-on-failure fallback
• preserve cache/security headers on passthrough responses via a shared helper
• fix Pages Router dev image redirects to emit percent-encoded Location headers for unicode paths
• add regression tests for:
  • transform failure before body consumption
  • transform failure after body consumption
  • refetch fallback header behavior
  • refetch fallback 404 behavior
  • refetch fallback unsafe-content behavior
• add integration tests for local unicode and space-containing image URLs in both App Router and Pages Router

Testing

• pnpm test tests/shims.test.ts -t "handleImageOptimization"
• pnpm test tests/image-optimization-parity.test.ts
• pnpm run fmt
• pnpm run typecheck

[pkg-pr-new]

Open in StackBlitz

npm i https://pkg.pr.new/vinext@513

commit: 6b03dff

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 855211d229

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[james-elicx]

/bigbonk review

[ask-bonk]

Solid fix. The approach — try the passthrough first, catch the TypeError from new Response(lockedStream), then refetch — is the right tradeoff. It keeps the happy path streaming with zero overhead, and only pays the cost of a second fetch when the transform actually consumed the body before failing.

The createPassthroughImageResponse dedup is clean, the unicode redirect fix is correct (URL.pathname returns percent-encoded paths per WHATWG), and the test coverage for the new fallback paths is thorough.

One minor nit and one question below, but neither are blocking.

[ask-bonk]

Nit: if handlers.fetchAsset throws during the refetch (network error, DNS failure, etc.), the error propagates unhandled out of handleImageOptimization. This is arguably fine — there's not much else to do — but it would be a 500 instead of a graceful error response. Consider wrapping the entire catch body for consistency:

Suggested change

	return createPassthroughImageResponse(refetchedSource, imageConfig);
	return createPassthroughImageResponse(refetchedSource, imageConfig);
	} catch {
	return new Response("Image not found", { status: 404 });
	}

Or alternatively wrap just the fetchAsset call. Up to you — the current behavior is acceptable.

[ask-bonk]

Worth noting for future readers: the try here catches the synchronous TypeError thrown by new Response(disturbed_body) inside createPassthroughImageResponse. Per the Fetch spec, the Response constructor's extract algorithm throws synchronously if the body stream is disturbed or locked. This is well-tested by the "refetches the source when transform consumes the stream" test case. A brief comment here would help future maintainers understand the control flow at a glance:

Suggested change

	return createPassthroughImageResponse(source, imageConfig);
	// createPassthroughImageResponse throws a TypeError if source.body was
	// consumed by a failed transformImage call (locked/disturbed ReadableStream).
	try {
	return createPassthroughImageResponse(source, imageConfig);

[ask-bonk]

Minor: the & replacement here handles HTML entity encoding from the rendered markup, but & in the regex means literal & in the source. If the HTML is raw (not entity-encoded), this replaceAll is a no-op, which is fine. Just confirming this is intentional for robustness rather than a specific observed behavior?

[ask-bonk]

Review submitted. Here's a summary of my feedback:

Approved with minor suggestions:

1. image-optimization.ts:276 — Suggested adding a comment explaining why the try/catch exists around createPassthroughImageResponse. The catch handles the synchronous TypeError thrown by new Response(disturbed_body) per the Fetch spec's extract algorithm. This control flow isn't obvious without understanding the spec.

2. image-optimization.ts:289 — Noted that if handlers.fetchAsset throws during the refetch (network error, DNS failure), the error propagates unhandled as a 500. Suggested wrapping the catch body for a graceful 404 fallback, though the current behavior is acceptable.

3. image-optimization-parity.test.ts:74 — Minor question about the & → & replacement in getImageSrcFromHtml — confirming it's a robustness measure for HTML entity encoding.

The Codex review comment about source.clone() buffering is incorrect — this PR specifically avoids clone/tee. The approach is well-designed: zero overhead on the success path, refetch only when the transform actually consumed the stream before failing.

github run