fix: clean up diagnostic upload listeners

This commit is contained in:
Neil
2026-05-30 09:48:03 -07:00
parent 8f6f59a654
commit 3c5a8f398f
2 changed files with 137 additions and 45 deletions
@@ -0,0 +1,66 @@
import { EventEmitter } from 'node:events'
import { describe, expect, it, vi } from 'vitest'
const { httpRequestMock, httpsRequestMock } = vi.hoisted(() => ({
httpRequestMock: vi.fn(),
httpsRequestMock: vi.fn()
}))
vi.mock('node:http', () => ({ request: httpRequestMock }))
vi.mock('node:https', () => ({ request: httpsRequestMock }))
import { MAX_RESPONSE_BYTES, postJsonForJson } from './diagnostic-upload-http'
class FakeRequest extends EventEmitter {
destroy = vi.fn()
end = vi.fn()
write = vi.fn()
}
class FakeResponse extends EventEmitter {
statusCode = 200
destroy = vi.fn()
}
describe('diagnostic upload HTTP', () => {
it('removes request and response listeners after a successful response', async () => {
const request = new FakeRequest()
const response = new FakeResponse()
httpRequestMock.mockImplementationOnce((_options, callback) => {
callback(response)
return request
})
const result = postJsonForJson('http://diagnostics.example/upload', { ok: true }, 1000)
response.emit('data', Buffer.from('{"accepted":true}'))
response.emit('end')
await expect(result).resolves.toEqual({ accepted: true })
expect(request.listenerCount('error')).toBe(0)
expect(request.listenerCount('timeout')).toBe(0)
expect(response.listenerCount('data')).toBe(0)
expect(response.listenerCount('end')).toBe(0)
expect(response.listenerCount('error')).toBe(0)
})
it('removes listeners after rejecting an oversized response', async () => {
const request = new FakeRequest()
const response = new FakeResponse()
httpRequestMock.mockImplementationOnce((_options, callback) => {
callback(response)
return request
})
const result = postJsonForJson('http://diagnostics.example/upload', { ok: true }, 1000)
response.emit('data', Buffer.alloc(MAX_RESPONSE_BYTES + 1))
await expect(result).rejects.toThrow('diagnostic response exceeded size limit')
expect(request.destroy).toHaveBeenCalledTimes(1)
expect(response.destroy).toHaveBeenCalledTimes(1)
expect(request.listenerCount('error')).toBe(0)
expect(request.listenerCount('timeout')).toBe(0)
expect(response.listenerCount('data')).toBe(0)
expect(response.listenerCount('end')).toBe(0)
expect(response.listenerCount('error')).toBe(0)
})
})
@@ -1,4 +1,4 @@
import { request as httpRequest } from 'node:http'
import { request as httpRequest, type ClientRequest, type IncomingMessage } from 'node:http'
import { request as httpsRequest } from 'node:https'
import { URL } from 'node:url'
@@ -38,20 +38,81 @@ function postRaw(
): Promise<unknown> {
return new Promise((resolve, reject) => {
let settled = false
let req: ClientRequest | null = null
let res: IncomingMessage | null = null
const chunks: Buffer[] = []
let responseBytes = 0
function cleanupListeners(): void {
req?.off('error', onRequestError)
req?.off('timeout', onRequestTimeout)
res?.off('data', onResponseData)
res?.off('end', onResponseEnd)
res?.off('error', onResponseError)
}
function resolveOnce(value: unknown): void {
if (settled) {
return
}
settled = true
cleanupListeners()
resolve(value)
}
function rejectOnce(error: Error): void {
function rejectOnce(
error: Error,
options: { destroyRequest?: boolean; destroyResponse?: boolean } = {}
): void {
if (settled) {
return
}
settled = true
if (options.destroyRequest) {
req?.destroy()
}
if (options.destroyResponse) {
res?.destroy()
}
cleanupListeners()
reject(error)
}
function onResponseData(chunk: Buffer): void {
responseBytes += chunk.length
if (responseBytes > MAX_RESPONSE_BYTES) {
// Why: diagnostics endpoints should return tiny JSON envelopes.
// Cap response buffering so a bad endpoint cannot grow main memory.
rejectOnce(new Error('diagnostic response exceeded size limit'), {
destroyRequest: true,
destroyResponse: true
})
return
}
chunks.push(chunk)
}
function onResponseEnd(): void {
const status = res?.statusCode ?? 0
const text = Buffer.concat(chunks).toString('utf8')
if (status >= 200 && status < 300) {
try {
resolveOnce(text.length > 0 ? JSON.parse(text) : {})
} catch {
rejectOnce(new Error(`malformed JSON response (HTTP ${status})`))
}
} else {
// Why: this error can cross IPC into renderer toasts. Never
// include backend response bodies; they may contain infra detail.
rejectOnce(new Error(`HTTP ${status}`))
}
}
function onResponseError(): void {
rejectOnce(new Error('diagnostic network request failed'))
}
function onRequestError(): void {
// Why: request errors can include endpoint hostnames. The diagnostics
// endpoint contract keeps infrastructure details out of renderer IPC.
rejectOnce(new Error('diagnostic network request failed'))
}
function onRequestTimeout(): void {
rejectOnce(new Error('diagnostic network request timed out'), { destroyRequest: true })
}
let parsed: URL
try {
parsed = new URL(url)
@@ -64,7 +125,7 @@ function postRaw(
return
}
const protocol = parsed.protocol === 'https:' ? httpsRequest : httpRequest
const req = protocol(
req = protocol(
{
protocol: parsed.protocol,
hostname: parsed.hostname,
@@ -77,50 +138,15 @@ function postRaw(
...headers
}
},
(res) => {
const chunks: Buffer[] = []
let responseBytes = 0
res.on('data', (chunk: Buffer) => {
responseBytes += chunk.length
if (responseBytes > MAX_RESPONSE_BYTES) {
// Why: diagnostics endpoints should return tiny JSON envelopes.
// Cap response buffering so a bad endpoint cannot grow main memory.
rejectOnce(new Error('diagnostic response exceeded size limit'))
req.destroy()
res.destroy()
return
}
chunks.push(chunk)
})
res.on('end', () => {
const status = res.statusCode ?? 0
const text = Buffer.concat(chunks).toString('utf8')
if (status >= 200 && status < 300) {
try {
resolveOnce(text.length > 0 ? JSON.parse(text) : {})
} catch {
rejectOnce(new Error(`malformed JSON response (HTTP ${status})`))
}
} else {
// Why: this error can cross IPC into renderer toasts. Never
// include backend response bodies; they may contain infra detail.
rejectOnce(new Error(`HTTP ${status}`))
}
})
res.on('error', () => {
rejectOnce(new Error('diagnostic network request failed'))
})
(response) => {
res = response
res.on('data', onResponseData)
res.on('end', onResponseEnd)
res.on('error', onResponseError)
}
)
req.on('error', () => {
// Why: request errors can include endpoint hostnames. The diagnostics
// endpoint contract keeps infrastructure details out of renderer IPC.
rejectOnce(new Error('diagnostic network request failed'))
})
req.on('timeout', () => {
rejectOnce(new Error('diagnostic network request timed out'))
req.destroy()
})
req.on('error', onRequestError)
req.on('timeout', onRequestTimeout)
req.write(body)
req.end()
})