Skip to content

perf: reduce Response.clone() overhead in tiered cache #1090

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
hugo-ccabral wants to merge 1 commit into
base: main
Choose a base branch
from
Open

perf: reduce Response.clone() overhead in tiered cache #1090

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 13 additions & 3 deletions runtime/caches/tiered.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,9 +44,14 @@ export function createTieredCache(
request: RequestInfo | URL,
matched: Response,
) {
const body = await matched.arrayBuffer();
Copy link
Copy Markdown
Contributor

@cubic-dev-ai cubic-dev-ai Bot Feb 25, 2026 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1: Reading matched.arrayBuffer() consumes the response body, so the matched response returned from match() will be unusable. Consider returning a new Response built from the buffered body (and using that for cache updates) or otherwise avoid consuming the response that is returned to callers.

Prompt for AI agents 
Check if this issue is valid — if so, understand the root cause and fix it. At runtime/caches/tiered.ts, line 47:

<comment>Reading `matched.arrayBuffer()` consumes the response body, so the `matched` response returned from `match()` will be unusable. Consider returning a new Response built from the buffered body (and using that for cache updates) or otherwise avoid consuming the response that is returned to callers.</comment>

<file context>
@@ -44,9 +44,14 @@ export function createTieredCache(
         request: RequestInfo | URL,
         matched: Response,
       ) {
+        const body = await matched.arrayBuffer();
+        const headers = matched.headers;
         await Promise.all(
</file context>
Fix with Cubic

const headers = matched.headers;
await Promise.all(
indexOfCachesToUpdate.map((index) =>
openedCaches[index].put(request, matched.clone())
openedCaches[index].put(
request,
new Response(body.slice(0), { headers }),
Copy link
Copy Markdown
Contributor

@cubic-dev-ai cubic-dev-ai Bot Feb 25, 2026 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2: The new Response created for cache tiers omits status and statusText, which changes the cached response semantics (e.g., 404 becomes 200). Preserve the original status fields when constructing the Response.

Prompt for AI agents 
Check if this issue is valid — if so, understand the root cause and fix it. At runtime/caches/tiered.ts, line 53:

<comment>The new Response created for cache tiers omits `status` and `statusText`, which changes the cached response semantics (e.g., 404 becomes 200). Preserve the original status fields when constructing the Response.</comment>

<file context>
@@ -44,9 +44,14 @@ export function createTieredCache(
-            openedCaches[index].put(request, matched.clone())
+            openedCaches[index].put(
+              request,
+              new Response(body.slice(0), { headers }),
+            )
           ),
</file context>
Suggested change
new Response(body.slice(0), { headers }),
new Response(body.slice(0), {
headers,
status: matched.status,
statusText: matched.statusText,
}),
Fix with Cubic

)
),
);
Comment on lines +47 to 56
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot Feb 25, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

status and statusText are silently dropped from reconstructed Responses.

new Response(body.slice(0), { headers }) defaults status to 200 and statusText to "". The Response constructor's init object accepts status and statusText as explicit fields. Any non-200 response (e.g., a cached 304, 206, or error response stored by the caller) will be written back to tiered caches with the wrong status, corrupting cache state.

The same defect is present in the put method (line 136).

🐛 Proposed fix – preserve status and statusText 
-        const body = await matched.arrayBuffer();
-        const headers = matched.headers;
+        const body = await matched.arrayBuffer();
+        const { status, statusText, headers } = matched;
         await Promise.all(
           indexOfCachesToUpdate.map((index) =>
             openedCaches[index].put(
               request,
-              new Response(body.slice(0), { headers }),
+              new Response(body.slice(0), { status, statusText, headers }),
             )
           ),
         );
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
const body = await matched.arrayBuffer();
const headers = matched.headers;
await Promise.all(
indexOfCachesToUpdate.map((index) =>
openedCaches[index].put(request, matched.clone())
openedCaches[index].put(
request,
new Response(body.slice(0), { headers }),
)
),
);
const body = await matched.arrayBuffer();
const { status, statusText, headers } = matched;
await Promise.all(
indexOfCachesToUpdate.map((index) =>
openedCaches[index].put(
request,
new Response(body.slice(0), { status, statusText, headers }),
)
),
);
🤖 Prompt for AI Agents 
Verify each finding against the current code and only fix it if needed.

In `@runtime/caches/tiered.ts` around lines 47 - 56, When reconstructing Responses
from a cached Response (variable matched) before calling
openedCaches[index].put, preserve matched.status and matched.statusText in the
Response init; specifically, pass status: matched.status and statusText:
matched.statusText (alongside headers) when creating the new Response from
body.slice(0). Apply the same change in the other occurrence inside the put
method (the code that creates new Response(body.slice(0), { headers })),
ensuring both read the original matched.status and matched.statusText so non-200
and error responses are stored with their original status.

}
Expand Down Expand Up @@ -123,8 +128,13 @@ export function createTieredCache(
request: RequestInfo | URL,
response: Response,
): Promise<void> => {
const putPromises = openedCaches.map((caches) =>
caches.put(request, response.clone())
const body = await response.arrayBuffer();
const headers = response.headers;
const putPromises = openedCaches.map((cache) =>
cache.put(
request,
new Response(body.slice(0), { headers }),
)
);
Comment on lines +131 to 138
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot Feb 25, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

status and statusText are dropped — same defect as updateTieredCaches.

new Response(body.slice(0), { headers }) reconstructs the response without status/statusText, so every cache tier receives a 200 OK regardless of the original response status.

🐛 Proposed fix 
-          const body = await response.arrayBuffer();
-          const headers = response.headers;
-          const putPromises = openedCaches.map((cache) =>
-            cache.put(
-              request,
-              new Response(body.slice(0), { headers }),
-            )
-          );
+          const body = await response.arrayBuffer();
+          const { status, statusText, headers } = response;
+          const putPromises = openedCaches.map((cache) =>
+            cache.put(
+              request,
+              new Response(body.slice(0), { status, statusText, headers }),
+            )
+          );
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
const body = await response.arrayBuffer();
const headers = response.headers;
const putPromises = openedCaches.map((cache) =>
cache.put(
request,
new Response(body.slice(0), { headers }),
)
);
const body = await response.arrayBuffer();
const { status, statusText, headers } = response;
const putPromises = openedCaches.map((cache) =>
cache.put(
request,
new Response(body.slice(0), { status, statusText, headers }),
)
);
🤖 Prompt for AI Agents 
Verify each finding against the current code and only fix it if needed.

In `@runtime/caches/tiered.ts` around lines 131 - 138, The code in the
openedCaches.map where it does cache.put(request, new Response(body.slice(0), {
headers })) loses the original response status and statusText, causing all
cached entries to be 200 OK; update that call in the same block (inside the
openedCaches.map / putPromises creation) to construct the new Response with the
original response.status and response.statusText passed in the options (e.g.,
new Response(body.slice(0), { headers, status: response.status, statusText:
response.statusText })) so each tier preserves the original status and
statusText.

await Promise.all(putPromises);
},
Expand Down
Loading