From 3c5a8f398f403a58b5e2a94500ad5bf34952ffab Mon Sep 17 00:00:00 2001 From: Neil <4138956+nwparker@users.noreply.github.com> Date: Sat, 30 May 2026 09:48:03 -0700 Subject: [PATCH] fix: clean up diagnostic upload listeners --- .../diagnostic-upload-http.test.ts | 66 ++++++++++ .../observability/diagnostic-upload-http.ts | 116 +++++++++++------- 2 files changed, 137 insertions(+), 45 deletions(-) create mode 100644 src/main/observability/diagnostic-upload-http.test.ts diff --git a/src/main/observability/diagnostic-upload-http.test.ts b/src/main/observability/diagnostic-upload-http.test.ts new file mode 100644 index 00000000000..6ad6f2dcf9d --- /dev/null +++ b/src/main/observability/diagnostic-upload-http.test.ts @@ -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) + }) +}) diff --git a/src/main/observability/diagnostic-upload-http.ts b/src/main/observability/diagnostic-upload-http.ts index 95528ab7cbd..17e6b186085 100644 --- a/src/main/observability/diagnostic-upload-http.ts +++ b/src/main/observability/diagnostic-upload-http.ts @@ -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 { 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() })