Skip to content

fix(media): use fetch for direct transfers #278

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
mmhospedagem wants to merge 1 commit into
base: master
Choose a base branch
from
Open

fix(media): use fetch for direct transfers #278

Show file tree
Hide file tree
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
53 changes: 53 additions & 0 deletions src/media/__tests__/media.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -973,6 +973,59 @@ test('media transfer client applies separate upload/download agents', async () =
}
})

test('media transfer client uses fetch when no proxy agent is configured', async () => {
const server = http.createServer((request, response) => {
request.resume()
request.on('end', () => {
response.writeHead(request.method === 'POST' ? 201 : 200, {
'content-type': 'text/plain'
})
response.end(request.method === 'POST' ? 'upload-fetch-ok' : 'download-fetch-ok')
})
})
await new Promise<void>((resolve, reject) => {
server.once('error', reject)
server.listen(0, '127.0.0.1', () => {
server.off('error', reject)
resolve()
})
})
const address = server.address()
if (!address || typeof address === 'string') {
throw new Error('failed to resolve media fetch test server address')
}

const originalFetch = globalThis.fetch
let fetchCalls = 0
globalThis.fetch = async (...args) => {
fetchCalls++
return originalFetch(...args)
}

try {
const mediaTransfer = new WaMediaTransferClient()
const base = `http://127.0.0.1:${address.port}`
const download = await mediaTransfer.downloadBytes({ url: `${base}/download` })
const uploadResponse = await mediaTransfer.uploadStream({

@cubic-dev-ai cubic-dev-ai Bot Sep 10, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P3: The new fetch path's riskiest behavior—uploading a Node Readable body through fetch, which relies on the newly added duplex: 'half' option in WaMediaTransferClient.httpRequest—has no test coverage. The new fetch test only uploads a Uint8Array, and the only Readable-upload test executes through the agent (http) path, so a regression in duplex streaming would pass CI. Add a fetch-path upload test that passes a Readable body and asserts the round-tripped bytes.

Prompt for AI agents 
Check if this issue is valid — if so, understand the root cause and fix it. At src/media/__tests__/media.test.ts, line 1009:

<comment>The new fetch path's riskiest behavior—uploading a Node `Readable` body through fetch, which relies on the newly added `duplex: 'half'` option in `WaMediaTransferClient.httpRequest`—has no test coverage. The new fetch test only uploads a `Uint8Array`, and the only Readable-upload test executes through the agent (http) path, so a regression in duplex streaming would pass CI. Add a fetch-path upload test that passes a `Readable` body and asserts the round-tripped bytes.</comment>

<file context>
@@ -973,6 +973,59 @@ test('media transfer client applies separate upload/download agents', async () =
+        const mediaTransfer = new WaMediaTransferClient()
+        const base = `http://127.0.0.1:${address.port}`
+        const download = await mediaTransfer.downloadBytes({ url: `${base}/download` })
+        const uploadResponse = await mediaTransfer.uploadStream({
+            url: `${base}/upload`,
+            method: 'POST',
</file context>

url: `${base}/upload`,
method: 'POST',
contentType: 'application/octet-stream',
body: new Uint8Array([1, 2, 3])
})
const upload = await mediaTransfer.readResponseBytes(uploadResponse)

assert.equal(fetchCalls, 2)
assert.equal(new TextDecoder().decode(download), 'download-fetch-ok')
assert.equal(uploadResponse.status, 201)
assert.equal(new TextDecoder().decode(upload), 'upload-fetch-ok')
} finally {
globalThis.fetch = originalFetch
await new Promise<void>((resolve) => {
server.close(() => resolve())
})
}
})

test('media transfer client routes through optional got when proxy agent is set', async () => {
const server = http.createServer((_request, response) => {
response.writeHead(200, { 'content-type': 'text/plain' })
Expand Down
31 changes: 30 additions & 1 deletion src/media/transfer/WaMediaTransferClient.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import http from 'node:http'
import https from 'node:https'
import type { Readable } from 'node:stream'
import { Readable } from 'node:stream'

import type { Logger } from '@infra/log/types'
import { DEFAULT_MEDIA_HOSTS } from '@media/constants'
Expand Down Expand Up @@ -311,6 +311,35 @@ export class WaMediaTransferClient {
init: TransferRequestInit,
agent: WaProxyAgent | undefined
): Promise<InternalTransferResponse> {
if (!agent && typeof fetch === 'function') {
const fetchInit = {
method: init.method ?? 'GET',
headers: init.headers,
body: init.body,
signal: init.signal ?? undefined
} as Parameters<typeof fetch>[1] & { duplex?: 'half' }
if (init.body && !(init.body instanceof Uint8Array)) {
fetchInit.duplex = 'half'
}

const response = await fetch(url, fetchInit)

@cubic-dev-ai cubic-dev-ai Bot Sep 10, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2: The fetch path follows HTTP redirects by default (redirect: 'follow'), while the retained http/https agent path does not follow redirects. This changes the observable semantics of StreamTransferResponse for 3xx responses: fetch transparently follows the redirect and reports the final status/ok, whereas the old path surfaced the 3xx. It also means the reported url on the returned response stays the originally requested URL even when the transfer actually completes against a different host, and cross-origin redirects can strip sensitive headers. If the media endpoints must not be silently redirected (or callers rely on seeing the 3xx), pass an explicit redirect: 'manual' init to match the prior behavior.

Prompt for AI agents 
Check if this issue is valid — if so, understand the root cause and fix it. At src/media/transfer/WaMediaTransferClient.ts, line 325:

<comment>The fetch path follows HTTP redirects by default (redirect: 'follow'), while the retained http/https agent path does not follow redirects. This changes the observable semantics of StreamTransferResponse for 3xx responses: fetch transparently follows the redirect and reports the final status/ok, whereas the old path surfaced the 3xx. It also means the reported `url` on the returned response stays the originally requested URL even when the transfer actually completes against a different host, and cross-origin redirects can strip sensitive headers. If the media endpoints must not be silently redirected (or callers rely on seeing the 3xx), pass an explicit `redirect: 'manual'` init to match the prior behavior.</comment>

<file context>
@@ -311,6 +311,35 @@ export class WaMediaTransferClient {
+                fetchInit.duplex = 'half'
+            }
+
+            const response = await fetch(url, fetchInit)
+            const headers: Record<string, string> = {}
+            response.headers.forEach((value, key) => {
</file context>

const headers: Record<string, string> = {}
response.headers.forEach((value, key) => {
headers[key] = value
})
const body = response.body ? Readable.fromWeb(response.body) : null
return {
status: response.status,
ok: response.ok,
headers,
body,
// eslint-disable-next-line @typescript-eslint/require-await
cancel: async () => {
body?.destroy()
}
}
}

const parsed = new URL(url)
const transport = parsed.protocol === 'https:' ? https : http
return new Promise<InternalTransferResponse>((resolve, reject) => {
Expand Down
Loading