fix: avoid immutable header mutation for mapped streaming responses on node#1551
fix: avoid immutable header mutation for mapped streaming responses on node#1551Falven wants to merge 1 commit intoPortkey-AI:mainfrom
Conversation
…n node Problem\n- Streaming /v1/responses requests can fail in Node runtime with `TypeError: immutable` when the response object returned from the mapped path has immutable headers.\n\nRoot cause\n- `ResponseService.create` always calls `updateHeaders`, which appends and deletes headers in place.\n- For `isResponseAlreadyMapped=true` with streaming responses, the underlying `Response` may have an immutable header guard in undici.\n\nChange\n- In `ResponseService.create`, clone mapped streaming responses on Node into a new `Response` using copied status/statusText/body/headers before `updateHeaders` runs.\n- Keep all existing header enrichment/removal logic unchanged.\n\nWhy this is safe\n- Scope is minimal and only applies to the problematic path: mapped + streaming + Node runtime.\n- Non-streaming and non-Node paths are unaffected.\n- Response body streaming is preserved while ensuring headers are mutable for Portkey metadata injection.
There was a problem hiding this comment.
Pull request overview
This PR fixes a TypeError: immutable crash that occurs in the Node.js runtime when streaming responses go through the mapped response path in /v1/responses. The root cause is that Node's undici fetch implementation returns Response objects with immutable headers, and updateHeaders() tries to mutate them in-place.
Changes:
- Clones the streaming mapped response into a new
Responsewith explicitly mutable headers (vianew Headers()) beforeupdateHeaders()is called, but only when the specific conditions that trigger the bug are met (isResponseAlreadyMapped && isStreaming && node runtime).
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if ( | ||
| isResponseAlreadyMapped && | ||
| this.context.isStreaming && | ||
| getRuntimeKey() == 'node' | ||
| ) { | ||
| finalMappedResponse = new Response(finalMappedResponse.body, { | ||
| status: finalMappedResponse.status, | ||
| statusText: finalMappedResponse.statusText, | ||
| headers: new Headers(finalMappedResponse.headers), | ||
| }); | ||
| } |
There was a problem hiding this comment.
This new code path (isResponseAlreadyMapped && isStreaming && node) lacks a corresponding unit test. The existing test file at tests/unit/src/handlers/services/responseService.test.ts has comprehensive tests for create() but none where isStreaming is true. Consider adding a test that sets mockRequestContext.isStreaming = true, mocks getRuntimeKey to return 'node', and verifies that updateHeaders succeeds without throwing (i.e., the cloned response has mutable headers). You could also verify the cloned response preserves status, statusText, and headers from the original.
Description: (required)
TypeError: immutablefailure in Node runtime when/v1/responsesstreaming output goes through the mapped response path.isResponseAlreadyMapped && isStreaming && node) into a newResponsewith copied status, statusText, body, and headers beforeupdateHeaders()mutates headers.x-portkey-*response headers.Tests Run/Test cases added: (required)
pnpm run buildPOST /v1/responseswith"stream": truereturns SSE successfully and does not throwTypeError: immutable.Type of Change: