From f5009566ad4c4631794ce46bbbe21bffa856bce0 Mon Sep 17 00:00:00 2001 From: JF Date: Sat, 22 Aug 2026 00:23:04 -0400 Subject: [PATCH] hardening(proxy): bound the remaining unbounded worker collections Closes the post-#395 retention audit umbrella (issue #405): - DapProxyWorker.preConnectQueue and commandQueue capped at MAX_QUEUED_COMMANDS (256); overflow answers the live requestId with an error response instead of queueing forever (silent eviction would hang the client) - ChildSessionManager.storedBreakpoints cleared in shutdown() and a file clearing to zero breakpoints deletes its key (replaying [] to a fresh child was a no-op) - CdpFunctionBreakpointBridge.scriptUrls FIFO-capped at MAX_SCRIPT_URLS (10k) for long-lived JS debuggees with module churn Co-Authored-By: Claude Fable 5 --- src/proxy/cdp-function-breakpoint-bridge.ts | Bin 24484 -> 25185 bytes src/proxy/child-session-manager.ts | 10 ++- src/proxy/dap-proxy-worker.ts | 31 +++++++++- .../cdp-function-breakpoint-bridge.test.ts | 20 +++++- tests/proxy/child-session-manager.test.ts | 29 +++++++++ tests/proxy/dap-proxy-worker.test.ts | 57 +++++++++++++++++- 6 files changed, 142 insertions(+), 5 deletions(-) diff --git a/src/proxy/cdp-function-breakpoint-bridge.ts b/src/proxy/cdp-function-breakpoint-bridge.ts index 6ae4004e449a012d3a11a1d7e975820999428c91..e685d0c71e2ee0a14b78c79e15dcd76015b46770 100644 GIT binary patch delta 713 zcma)3O>0v@6s6!QQp8;sBBzMqz0iCJ224W)DHI|WrLo{b2$MHAd1EFskC}O|eM(7J z?lppcz#mY^4{+n!rF;8JocEH5r5jh5nS0N<=bZcP`_}ItTbIr4d9zllpoRmMV5|*X zOt6s=iR*J*ef*5nNX&dQ5Z0!I2aj9ra34~uNnMJJIGzu{X_QVTL>8yQ#RVPD6`P3& zGtE;;h~m^J1e`KhG8q}Whj(NhPUp-;afpN&yIE`@6sNIJ!ja4y1>Ir)@Fge{krP_s z>f#F+tYe;;lruTBk~t!6o%M||lXAqOX(?T5Jxvox=ExLO%?h0*+BiS0{QjezXK#js zgX8|I*Ta+J7X$S0s5NZ0THR%R`)zR7(2QmyGJ`Ub6C;;j+qZAs{9askszsEg(t5;_ z#3v(^H|u$*N*#e2fj9*(Z(60S+H&P_{RoUivU{zmglTnfT&U=iYrzk{kuAez<|0m(cUd;-bBCXXCzf9d?&LJHH string | undefined; }; +/** + * Cap for the pre-connect and policy command queues (issue #405). Both are + * normally drained within one adapter handshake, so a queue this deep means + * the adapter is wedged — commands past the cap are rejected with an error + * response on their live requestId (silent eviction would hang the client). + */ +export const MAX_QUEUED_COMMANDS = 256; + export class DapProxyWorker { private logger: ILogger | null = null; private dapClient: IDapClient | null = null; @@ -934,11 +942,21 @@ export class DapProxyWorker { // Check if we're connected if (!this.dapClient) { if (this.state === ProxyState.INITIALIZING) { + if (this.preConnectQueue.length >= MAX_QUEUED_COMMANDS) { + // A wedged adapter never drains this queue; reject instead of + // growing without bound (issue #405). Every queued command holds a + // live requestId, so the overflow must answer, not silently drop. + this.sendDapResponse( + payload.requestId, false, undefined, + `pre-connect queue overflow (${MAX_QUEUED_COMMANDS} commands queued; adapter never became ready)` + ); + return; + } this.preConnectQueue.push(payload); this.logger?.info(`[Worker] Queued pre-connect DAP command: ${payload.dapCommand}`); return; } - + this.sendDapResponse(payload.requestId, false, undefined, 'DAP client not connected'); return; } @@ -970,8 +988,17 @@ export class DapProxyWorker { ); if (handling.shouldQueue) { + if (this.commandQueue.length >= MAX_QUEUED_COMMANDS) { + // Same shape as the pre-connect overflow: reject with an error on + // the live requestId rather than queueing forever (issue #405). + this.sendDapResponse( + payload.requestId, false, undefined, + `command queue overflow (${MAX_QUEUED_COMMANDS} commands queued; adapter is not draining)` + ); + return; + } this.logger!.info(`[Worker] ${handling.reason || 'Queuing command'}`); - + // Check if we need to inject configurationDone const initBehavior = this.adapterPolicy.getInitializationBehavior(); if (handling.shouldDefer && initBehavior.deferConfigDone) { diff --git a/tests/proxy/cdp-function-breakpoint-bridge.test.ts b/tests/proxy/cdp-function-breakpoint-bridge.test.ts index 62d5615d..2a4a967c 100644 --- a/tests/proxy/cdp-function-breakpoint-bridge.test.ts +++ b/tests/proxy/cdp-function-breakpoint-bridge.test.ts @@ -12,7 +12,7 @@ import { describe, it, expect, beforeEach, vi } from 'vitest'; import { EventEmitter } from 'events'; import type { DebugProtocol } from '@vscode/debugprotocol'; -import { CdpFunctionBreakpointBridge } from '../../src/proxy/cdp-function-breakpoint-bridge.js'; +import { CdpFunctionBreakpointBridge, MAX_SCRIPT_URLS } from '../../src/proxy/cdp-function-breakpoint-bridge.js'; type CdpHandler = (params: Record) => unknown; @@ -640,4 +640,22 @@ describe('CdpFunctionBreakpointBridge', () => { expect((out.body as DebugProtocol.StoppedEvent['body']).reason).toBe('breakpoint'); }); }); + + describe('scriptUrls cap (issue #405)', () => { + it('evicts the oldest scriptParsed entries past the cap', async () => { + await attach(); + + const overshoot = 10; + for (let i = 0; i < MAX_SCRIPT_URLS + overshoot; i++) { + cdp.emit('cdp-event', 'Debugger.scriptParsed', { scriptId: `s${i}`, url: `file:///f${i}.js` }); + } + + const urls = (bridge as unknown as { scriptUrls: Map }).scriptUrls; + expect(urls.size).toBe(MAX_SCRIPT_URLS); + // FIFO: the earliest scripts fell out, the newest survive + expect(urls.has('s0')).toBe(false); + expect(urls.has(`s${overshoot - 1}`)).toBe(false); + expect(urls.get(`s${MAX_SCRIPT_URLS + overshoot - 1}`)).toBe(`file:///f${MAX_SCRIPT_URLS + overshoot - 1}.js`); + }); + }); }); diff --git a/tests/proxy/child-session-manager.test.ts b/tests/proxy/child-session-manager.test.ts index 220017d9..e5010b98 100644 --- a/tests/proxy/child-session-manager.test.ts +++ b/tests/proxy/child-session-manager.test.ts @@ -909,4 +909,33 @@ describe('ChildSessionManager', () => { expect(bridge.detachCalls).toBe(2); }); }); + + describe('stored breakpoint lifecycle (issue #405)', () => { + beforeEach(() => { + manager = new ChildSessionManager({ + policy: JsDebugAdapterPolicy, + host: 'localhost', + port: 9229 + }); + }); + + it('clears stored breakpoints on shutdown', async () => { + manager.storeBreakpoints('/abs/app.js', [{ line: 1 }]); + manager.storeBreakpoints('/abs/lib.js', [{ line: 2 }]); + expect((manager as any).storedBreakpoints.size).toBe(2); + + await manager.shutdown(); + + expect((manager as any).storedBreakpoints.size).toBe(0); + }); + + it('deletes the entry when a file clears to zero breakpoints', () => { + manager.storeBreakpoints('/abs/app.js', [{ line: 1 }]); + expect((manager as any).storedBreakpoints.size).toBe(1); + + manager.storeBreakpoints('/abs/app.js', []); + + expect((manager as any).storedBreakpoints.size).toBe(0); + }); + }); }); diff --git a/tests/proxy/dap-proxy-worker.test.ts b/tests/proxy/dap-proxy-worker.test.ts index dce15dad..cc438e3c 100644 --- a/tests/proxy/dap-proxy-worker.test.ts +++ b/tests/proxy/dap-proxy-worker.test.ts @@ -7,7 +7,7 @@ import { EventEmitter } from 'events'; import type { ChildProcess } from 'child_process'; import path from 'path'; import { describe, it, expect, beforeEach, vi, type Mock } from 'vitest'; -import { DapProxyWorker } from '../../src/proxy/dap-proxy-worker.js'; +import { DapProxyWorker, MAX_QUEUED_COMMANDS } from '../../src/proxy/dap-proxy-worker.js'; import { GenericAdapterManager } from '../../src/proxy/dap-proxy-adapter-manager.js'; import { DapConnectionManager } from '../../src/proxy/dap-proxy-connection-manager.js'; import type { @@ -3794,4 +3794,59 @@ describe('DapProxyWorker', () => { expect(response.error).toContain('not configured'); }); }); + + describe('bounded worker queues (issue #405)', () => { + const dapPayload = (id: string): DapCommandPayload => ({ + cmd: 'dap', + sessionId: 'test-session', + requestId: id, + dapCommand: 'setBreakpoints', + dapArgs: {} + }); + + const responseFor = (requestId: string) => + mockMessageSender.send.mock.calls + .map((c) => c[0] as { type?: string; requestId?: string; success?: boolean; error?: string }) + .find((m) => m.type === 'dapResponse' && m.requestId === requestId); + + it('rejects a command with an error response when the pre-connect queue is full', async () => { + (worker as any).state = ProxyState.INITIALIZING; + (worker as any).dapClient = null; + (worker as any).preConnectQueue = Array.from( + { length: MAX_QUEUED_COMMANDS }, + (_, i) => dapPayload(`preconnect-${i}`) + ); + + await worker.handleCommand(dapPayload('overflow-pre')); + + expect((worker as any).preConnectQueue).toHaveLength(MAX_QUEUED_COMMANDS); + const response = responseFor('overflow-pre'); + expect(response?.success).toBe(false); + expect(String(response?.error)).toMatch(/queue/i); + }); + + it('rejects a command with an error response when the policy command queue is full', async () => { + (worker as any).state = ProxyState.CONNECTED; + (worker as any).dapClient = mockDapClient; + (worker as any).adapterPolicy = { + name: 'queue-test', + shouldQueueCommand: () => ({ shouldQueue: true, shouldDefer: false }), + getInitializationBehavior: () => ({}), + getDapClientBehavior: () => ({}) + }; + (worker as any).commandQueue = Array.from( + { length: MAX_QUEUED_COMMANDS }, + (_, i) => dapPayload(`queued-${i}`) + ); + + await worker.handleCommand(dapPayload('overflow-cmd')); + + // Overflow must not drain or grow the queue — the queued commands hold + // live requestIds that a silent evict would leave hanging forever. + expect((worker as any).commandQueue).toHaveLength(MAX_QUEUED_COMMANDS); + const response = responseFor('overflow-cmd'); + expect(response?.success).toBe(false); + expect(String(response?.error)).toMatch(/queue/i); + }); + }); });