From 5b40e82c41cbf5a677e8272fad8c8d49e42bb014 Mon Sep 17 00:00:00 2001 From: Philipp Kunz Date: Sun, 1 Jun 2025 14:41:19 +0000 Subject: [PATCH] Add tests for connect-disconnect and error handling in SmartProxy --- readme.hints.md | 61 +++- test/test.connect-disconnect-cleanup.node.ts | 242 +++++++++++++++ ...t.connection-cleanup-comprehensive.node.ts | 279 ++++++++++++++++++ .../smart-proxy/route-connection-handler.ts | 179 +++++------ 4 files changed, 654 insertions(+), 107 deletions(-) create mode 100644 test/test.connect-disconnect-cleanup.node.ts create mode 100644 test/test.connection-cleanup-comprehensive.node.ts diff --git a/readme.hints.md b/readme.hints.md index 8d3cb43..4b80e31 100644 --- a/readme.hints.md +++ b/readme.hints.md @@ -523,4 +523,63 @@ if (!record) { - **Consideration**: More frequent cleanup operations, but prevents queue backlog ### Migration Notes -No configuration changes needed. The improvements are automatic and backward compatible. \ No newline at end of file +No configuration changes needed. The improvements are automatic and backward compatible. + +## Early Client Disconnect Handling (v19.5.13+) + +### Issue +Connections were accumulating when clients connected but disconnected before sending data or during routing. This occurred in two scenarios: +1. **TLS Path**: Clients connecting and disconnecting before sending initial TLS handshake data +2. **Non-TLS Immediate Routing**: Clients disconnecting while backend connection was being established + +### Root Cause +1. **Missing Cleanup Handlers**: During initial data wait and immediate routing, no close/end handlers were attached to catch early disconnections +2. **Race Condition**: Backend connection attempts continued even after client disconnected, causing unhandled errors +3. **Timing Window**: Between accepting connection and establishing full bidirectional flow, disconnections weren't properly handled + +### Solution +1. **TLS Path Fix**: Added close/end handlers during initial data wait (lines 224-253 in route-connection-handler.ts) +2. **Immediate Routing Fix**: Used `setupSocketHandlers` for proper handler attachment (lines 180-205) +3. **Backend Error Handling**: Check if connection already closed before handling backend errors (line 1144) + +### Changes Made +```typescript +// 1. TLS path - handle disconnect before initial data +socket.once('close', () => { + if (!initialDataReceived) { + this.connectionManager.cleanupConnection(record, 'closed_before_data'); + } +}); + +// 2. Immediate routing path - proper handler setup +setupSocketHandlers(socket, (reason) => { + if (!record.outgoing || record.outgoing.readyState !== 'open') { + if (record.outgoing && !record.outgoing.destroyed) { + record.outgoing.destroy(); // Abort pending backend connection + } + this.connectionManager.cleanupConnection(record, reason); + } +}, undefined, 'immediate-route-client'); + +// 3. Backend connection error handling +onError: (error) => { + if (record.connectionClosed) { + logger.log('debug', 'Backend connection failed but client already disconnected'); + return; // Client already gone, nothing to clean up + } + // ... normal error handling +} +``` + +### Test Coverage +- `test/test.connect-disconnect-cleanup.node.ts` - Comprehensive test for early disconnect scenarios +- Tests verify connection count stays at 0 even with rapid connect/disconnect patterns +- Covers immediate disconnect, delayed disconnect, and mixed patterns + +### Performance Impact +- **Positive**: No more connection accumulation from early disconnects +- **Positive**: Immediate cleanup reduces memory usage +- **Positive**: Prevents resource exhaustion from rapid reconnection attempts + +### Migration Notes +No configuration changes needed. The fix is automatic and backward compatible. \ No newline at end of file diff --git a/test/test.connect-disconnect-cleanup.node.ts b/test/test.connect-disconnect-cleanup.node.ts new file mode 100644 index 0000000..a5f6fe9 --- /dev/null +++ b/test/test.connect-disconnect-cleanup.node.ts @@ -0,0 +1,242 @@ +import { tap, expect } from '@git.zone/tstest/tapbundle'; +import * as net from 'net'; +import * as plugins from '../ts/plugins.js'; + +// Import SmartProxy and configurations +import { SmartProxy } from '../ts/index.js'; + +tap.test('should handle clients that connect and immediately disconnect without sending data', async () => { + console.log('\n=== Testing Connect-Disconnect Cleanup ==='); + + // Create a SmartProxy instance + const proxy = new SmartProxy({ + ports: [8560], + enableDetailedLogging: false, + initialDataTimeout: 5000, // 5 second timeout for initial data + routes: [{ + name: 'test-route', + match: { ports: 8560 }, + action: { + type: 'forward', + target: { + host: 'localhost', + port: 9999 // Non-existent port + } + } + }] + }); + + // Start the proxy + await proxy.start(); + console.log('✓ Proxy started on port 8560'); + + // Helper to get active connection count + const getActiveConnections = () => { + const connectionManager = (proxy as any).connectionManager; + return connectionManager ? connectionManager.getConnectionCount() : 0; + }; + + const initialCount = getActiveConnections(); + console.log(`Initial connection count: ${initialCount}`); + + // Test 1: Connect and immediately disconnect without sending data + console.log('\n--- Test 1: Immediate disconnect ---'); + const connectionCounts: number[] = []; + + for (let i = 0; i < 10; i++) { + const client = new net.Socket(); + + // Connect and immediately destroy + client.connect(8560, 'localhost', () => { + // Connected - immediately destroy without sending data + client.destroy(); + }); + + // Wait a tiny bit + await new Promise(resolve => setTimeout(resolve, 10)); + + const count = getActiveConnections(); + connectionCounts.push(count); + if ((i + 1) % 5 === 0) { + console.log(`After ${i + 1} connect/disconnect cycles: ${count} active connections`); + } + } + + // Wait a bit for cleanup + await new Promise(resolve => setTimeout(resolve, 500)); + + const afterImmediateDisconnect = getActiveConnections(); + console.log(`After immediate disconnect test: ${afterImmediateDisconnect} active connections`); + + // Test 2: Connect, wait a bit, then disconnect without sending data + console.log('\n--- Test 2: Delayed disconnect ---'); + + for (let i = 0; i < 5; i++) { + const client = new net.Socket(); + + client.on('error', () => { + // Ignore errors + }); + + client.connect(8560, 'localhost', () => { + // Wait 100ms then disconnect without sending data + setTimeout(() => { + if (!client.destroyed) { + client.destroy(); + } + }, 100); + }); + } + + // Check count immediately + const duringDelayed = getActiveConnections(); + console.log(`During delayed disconnect test: ${duringDelayed} active connections`); + + // Wait for cleanup + await new Promise(resolve => setTimeout(resolve, 1000)); + + const afterDelayedDisconnect = getActiveConnections(); + console.log(`After delayed disconnect test: ${afterDelayedDisconnect} active connections`); + + // Test 3: Mix of immediate and delayed disconnects + console.log('\n--- Test 3: Mixed disconnect patterns ---'); + + const promises = []; + for (let i = 0; i < 20; i++) { + promises.push(new Promise((resolve) => { + const client = new net.Socket(); + + client.on('error', () => { + resolve(); + }); + + client.on('close', () => { + resolve(); + }); + + client.connect(8560, 'localhost', () => { + if (i % 2 === 0) { + // Half disconnect immediately + client.destroy(); + } else { + // Half wait 50ms + setTimeout(() => { + if (!client.destroyed) { + client.destroy(); + } + }, 50); + } + }); + + // Failsafe timeout + setTimeout(() => resolve(), 200); + })); + } + + // Wait for all to complete + await Promise.all(promises); + + const duringMixed = getActiveConnections(); + console.log(`During mixed test: ${duringMixed} active connections`); + + // Final cleanup wait + await new Promise(resolve => setTimeout(resolve, 1000)); + + const finalCount = getActiveConnections(); + console.log(`\nFinal connection count: ${finalCount}`); + + // Stop the proxy + await proxy.stop(); + console.log('✓ Proxy stopped'); + + // Verify all connections were cleaned up + expect(finalCount).toEqual(initialCount); + expect(afterImmediateDisconnect).toEqual(initialCount); + expect(afterDelayedDisconnect).toEqual(initialCount); + + // Check that connections didn't accumulate during the test + const maxCount = Math.max(...connectionCounts); + console.log(`\nMax connection count during immediate disconnect test: ${maxCount}`); + expect(maxCount).toBeLessThan(3); // Should stay very low + + console.log('\n✅ PASS: Connect-disconnect cleanup working correctly!'); +}); + +tap.test('should handle clients that error during connection', async () => { + console.log('\n=== Testing Connection Error Cleanup ==='); + + const proxy = new SmartProxy({ + ports: [8561], + enableDetailedLogging: false, + routes: [{ + name: 'test-route', + match: { ports: 8561 }, + action: { + type: 'forward', + target: { + host: 'localhost', + port: 9999 + } + } + }] + }); + + await proxy.start(); + console.log('✓ Proxy started on port 8561'); + + const getActiveConnections = () => { + const connectionManager = (proxy as any).connectionManager; + return connectionManager ? connectionManager.getConnectionCount() : 0; + }; + + const initialCount = getActiveConnections(); + console.log(`Initial connection count: ${initialCount}`); + + // Create connections that will error + const promises = []; + for (let i = 0; i < 10; i++) { + promises.push(new Promise((resolve) => { + const client = new net.Socket(); + + client.on('error', () => { + resolve(); + }); + + client.on('close', () => { + resolve(); + }); + + // Connect to proxy + client.connect(8561, 'localhost', () => { + // Force an error by writing invalid data then destroying + try { + client.write(Buffer.alloc(1024 * 1024)); // Large write + client.destroy(); + } catch (e) { + // Ignore + } + }); + + // Timeout + setTimeout(() => resolve(), 500); + })); + } + + await Promise.all(promises); + console.log('✓ All error connections completed'); + + // Wait for cleanup + await new Promise(resolve => setTimeout(resolve, 500)); + + const finalCount = getActiveConnections(); + console.log(`Final connection count: ${finalCount}`); + + await proxy.stop(); + console.log('✓ Proxy stopped'); + + expect(finalCount).toEqual(initialCount); + + console.log('\n✅ PASS: Connection error cleanup working correctly!'); +}); + +tap.start(); \ No newline at end of file diff --git a/test/test.connection-cleanup-comprehensive.node.ts b/test/test.connection-cleanup-comprehensive.node.ts new file mode 100644 index 0000000..bc9593e --- /dev/null +++ b/test/test.connection-cleanup-comprehensive.node.ts @@ -0,0 +1,279 @@ +import { tap, expect } from '@git.zone/tstest/tapbundle'; +import * as net from 'net'; +import * as plugins from '../ts/plugins.js'; + +// Import SmartProxy and configurations +import { SmartProxy } from '../ts/index.js'; + +tap.test('comprehensive connection cleanup test - all scenarios', async () => { + console.log('\n=== Comprehensive Connection Cleanup Test ==='); + + // Create a SmartProxy instance + const proxy = new SmartProxy({ + ports: [8570, 8571], // One for immediate routing, one for TLS + enableDetailedLogging: false, + initialDataTimeout: 2000, + socketTimeout: 5000, + routes: [ + { + name: 'non-tls-route', + match: { ports: 8570 }, + action: { + type: 'forward', + target: { + host: 'localhost', + port: 9999 // Non-existent port + } + } + }, + { + name: 'tls-route', + match: { ports: 8571 }, + action: { + type: 'forward', + target: { + host: 'localhost', + port: 9999 // Non-existent port + }, + tls: { + mode: 'passthrough' + } + } + } + ] + }); + + // Start the proxy + await proxy.start(); + console.log('✓ Proxy started on ports 8570 (non-TLS) and 8571 (TLS)'); + + // Helper to get active connection count + const getActiveConnections = () => { + const connectionManager = (proxy as any).connectionManager; + return connectionManager ? connectionManager.getConnectionCount() : 0; + }; + + const initialCount = getActiveConnections(); + console.log(`Initial connection count: ${initialCount}`); + + // Test 1: Rapid ECONNREFUSED retries (from original issue) + console.log('\n--- Test 1: Rapid ECONNREFUSED retries ---'); + for (let i = 0; i < 10; i++) { + await new Promise((resolve) => { + const client = new net.Socket(); + + client.on('error', () => { + client.destroy(); + resolve(); + }); + + client.on('close', () => { + resolve(); + }); + + client.connect(8570, 'localhost', () => { + // Send data to trigger routing + client.write('GET / HTTP/1.1\r\nHost: test.com\r\n\r\n'); + }); + + setTimeout(() => { + if (!client.destroyed) { + client.destroy(); + } + resolve(); + }, 100); + }); + + if ((i + 1) % 5 === 0) { + const count = getActiveConnections(); + console.log(`After ${i + 1} ECONNREFUSED retries: ${count} active connections`); + } + } + + // Test 2: Connect without sending data (immediate disconnect) + console.log('\n--- Test 2: Connect without sending data ---'); + for (let i = 0; i < 10; i++) { + const client = new net.Socket(); + + client.on('error', () => { + // Ignore + }); + + // Connect to non-TLS port and immediately disconnect + client.connect(8570, 'localhost', () => { + client.destroy(); + }); + + await new Promise(resolve => setTimeout(resolve, 10)); + } + + const afterNoData = getActiveConnections(); + console.log(`After connect-without-data test: ${afterNoData} active connections`); + + // Test 3: TLS connections that disconnect before handshake + console.log('\n--- Test 3: TLS early disconnect ---'); + for (let i = 0; i < 10; i++) { + const client = new net.Socket(); + + client.on('error', () => { + // Ignore + }); + + // Connect to TLS port but disconnect before sending handshake + client.connect(8571, 'localhost', () => { + // Wait 50ms then disconnect (before initial data timeout) + setTimeout(() => { + client.destroy(); + }, 50); + }); + + await new Promise(resolve => setTimeout(resolve, 100)); + } + + const afterTlsEarly = getActiveConnections(); + console.log(`After TLS early disconnect test: ${afterTlsEarly} active connections`); + + // Test 4: Mixed pattern - simulating real-world chaos + console.log('\n--- Test 4: Mixed chaos pattern ---'); + const promises = []; + + for (let i = 0; i < 30; i++) { + promises.push(new Promise((resolve) => { + const client = new net.Socket(); + const port = i % 2 === 0 ? 8570 : 8571; + + client.on('error', () => { + resolve(); + }); + + client.on('close', () => { + resolve(); + }); + + client.connect(port, 'localhost', () => { + const scenario = i % 5; + + switch (scenario) { + case 0: + // Immediate disconnect + client.destroy(); + break; + case 1: + // Send data then disconnect + client.write('GET / HTTP/1.1\r\nHost: test.com\r\n\r\n'); + setTimeout(() => client.destroy(), 20); + break; + case 2: + // Disconnect after delay + setTimeout(() => client.destroy(), 100); + break; + case 3: + // Send partial TLS handshake + if (port === 8571) { + client.write(Buffer.from([0x16, 0x03, 0x01])); // Partial TLS + } + setTimeout(() => client.destroy(), 50); + break; + case 4: + // Just let it timeout + break; + } + }); + + // Failsafe + setTimeout(() => { + if (!client.destroyed) { + client.destroy(); + } + resolve(); + }, 500); + })); + + // Small delay between connections + if (i % 5 === 0) { + await new Promise(resolve => setTimeout(resolve, 10)); + } + } + + await Promise.all(promises); + console.log('✓ Chaos test completed'); + + // Wait for any cleanup + await new Promise(resolve => setTimeout(resolve, 1000)); + + const afterChaos = getActiveConnections(); + console.log(`After chaos test: ${afterChaos} active connections`); + + // Test 5: NFTables route (should cleanup properly) + console.log('\n--- Test 5: NFTables route cleanup ---'); + const nftProxy = new SmartProxy({ + ports: [8572], + enableDetailedLogging: false, + routes: [{ + name: 'nftables-route', + match: { ports: 8572 }, + action: { + type: 'forward', + forwardingEngine: 'nftables', + target: { + host: 'localhost', + port: 9999 + } + } + }] + }); + + await nftProxy.start(); + + const getNftConnections = () => { + const connectionManager = (nftProxy as any).connectionManager; + return connectionManager ? connectionManager.getConnectionCount() : 0; + }; + + // Create NFTables connections + for (let i = 0; i < 5; i++) { + const client = new net.Socket(); + + client.on('error', () => { + // Ignore + }); + + client.connect(8572, 'localhost', () => { + setTimeout(() => client.destroy(), 50); + }); + + await new Promise(resolve => setTimeout(resolve, 100)); + } + + await new Promise(resolve => setTimeout(resolve, 500)); + + const nftFinal = getNftConnections(); + console.log(`NFTables connections after test: ${nftFinal}`); + + await nftProxy.stop(); + + // Final check on main proxy + const finalCount = getActiveConnections(); + console.log(`\nFinal connection count: ${finalCount}`); + + // Stop the proxy + await proxy.stop(); + console.log('✓ Proxy stopped'); + + // Verify all connections were cleaned up + expect(finalCount).toEqual(initialCount); + expect(afterNoData).toEqual(initialCount); + expect(afterTlsEarly).toEqual(initialCount); + expect(afterChaos).toEqual(initialCount); + expect(nftFinal).toEqual(0); + + console.log('\n✅ PASS: Comprehensive connection cleanup test passed!'); + console.log('All connection scenarios properly cleaned up:'); + console.log('- ECONNREFUSED rapid retries'); + console.log('- Connect without sending data'); + console.log('- TLS early disconnect'); + console.log('- Mixed chaos patterns'); + console.log('- NFTables connections'); +}); + +tap.start(); \ No newline at end of file diff --git a/ts/proxies/smart-proxy/route-connection-handler.ts b/ts/proxies/smart-proxy/route-connection-handler.ts index 97c4686..63e427a 100644 --- a/ts/proxies/smart-proxy/route-connection-handler.ts +++ b/ts/proxies/smart-proxy/route-connection-handler.ts @@ -176,8 +176,33 @@ export class RouteConnectionHandler { // If no routes require TLS handling and it's not port 443, route immediately if (!needsTlsHandling && localPort !== 443) { - // Set up error handler - socket.on('error', this.connectionManager.handleError('incoming', record)); + // Set up proper socket handlers for immediate routing + setupSocketHandlers( + socket, + (reason) => { + // Only cleanup if connection hasn't been fully established + // Check if outgoing connection exists and is connected + if (!record.outgoing || record.outgoing.readyState !== 'open') { + logger.log('debug', `Connection ${connectionId} closed during immediate routing: ${reason}`, { + connectionId, + remoteIP: record.remoteIP, + reason, + hasOutgoing: !!record.outgoing, + outgoingState: record.outgoing?.readyState, + component: 'route-handler' + }); + + // If there's a pending outgoing connection, destroy it + if (record.outgoing && !record.outgoing.destroyed) { + record.outgoing.destroy(); + } + + this.connectionManager.cleanupConnection(record, reason); + } + }, + undefined, // Use default timeout handler + 'immediate-route-client' + ); // Route immediately for non-TLS connections this.routeConnection(socket, record, '', undefined); @@ -221,6 +246,37 @@ export class RouteConnectionHandler { // Set up error handler socket.on('error', this.connectionManager.handleError('incoming', record)); + // Add close/end handlers to catch immediate disconnections + socket.once('close', () => { + if (!initialDataReceived) { + logger.log('warn', `Connection ${connectionId} closed before sending initial data`, { + connectionId, + remoteIP: record.remoteIP, + component: 'route-handler' + }); + if (initialTimeout) { + clearTimeout(initialTimeout); + initialTimeout = null; + } + this.connectionManager.cleanupConnection(record, 'closed_before_data'); + } + }); + + socket.once('end', () => { + if (!initialDataReceived) { + logger.log('debug', `Connection ${connectionId} ended before sending initial data`, { + connectionId, + remoteIP: record.remoteIP, + component: 'route-handler' + }); + if (initialTimeout) { + clearTimeout(initialTimeout); + initialTimeout = null; + } + // Don't cleanup on 'end' - wait for 'close' + } + }); + // First data handler to capture initial TLS handshake socket.once('data', (chunk: Buffer) => { // Clear the initial timeout since we've received data @@ -927,107 +983,6 @@ export class RouteConnectionHandler { } } - /** - * Setup improved error handling for the outgoing connection - * @deprecated This method is no longer used - error handling is done in createSocketWithErrorHandler - */ - private setupOutgoingErrorHandler( - connectionId: string, - targetSocket: plugins.net.Socket, - record: IConnectionRecord, - socket: plugins.net.Socket, - finalTargetHost: string, - finalTargetPort: number - ): void { - targetSocket.once('error', (err) => { - // This handler runs only once during the initial connection phase - const code = (err as any).code; - logger.log('error', - `Connection setup error for ${connectionId} to ${finalTargetHost}:${finalTargetPort}: ${err.message} (${code})`, - { - connectionId, - targetHost: finalTargetHost, - targetPort: finalTargetPort, - errorMessage: err.message, - errorCode: code, - component: 'route-handler' - } - ); - - // Resume the incoming socket to prevent it from hanging - socket.resume(); - - // Log specific error types for easier debugging - if (code === 'ECONNREFUSED') { - logger.log('error', - `Connection ${connectionId}: Target ${finalTargetHost}:${finalTargetPort} refused connection. Check if the target service is running and listening on that port.`, - { - connectionId, - targetHost: finalTargetHost, - targetPort: finalTargetPort, - recommendation: 'Check if the target service is running and listening on that port.', - component: 'route-handler' - } - ); - } else if (code === 'ETIMEDOUT') { - logger.log('error', - `Connection ${connectionId} to ${finalTargetHost}:${finalTargetPort} timed out. Check network conditions, firewall rules, or if the target is too far away.`, - { - connectionId, - targetHost: finalTargetHost, - targetPort: finalTargetPort, - recommendation: 'Check network conditions, firewall rules, or if the target is too far away.', - component: 'route-handler' - } - ); - } else if (code === 'ECONNRESET') { - logger.log('error', - `Connection ${connectionId} to ${finalTargetHost}:${finalTargetPort} was reset. The target might have closed the connection abruptly.`, - { - connectionId, - targetHost: finalTargetHost, - targetPort: finalTargetPort, - recommendation: 'The target might have closed the connection abruptly.', - component: 'route-handler' - } - ); - } else if (code === 'EHOSTUNREACH') { - logger.log('error', - `Connection ${connectionId}: Host ${finalTargetHost} is unreachable. Check DNS settings, network routing, or firewall rules.`, - { - connectionId, - targetHost: finalTargetHost, - recommendation: 'Check DNS settings, network routing, or firewall rules.', - component: 'route-handler' - } - ); - } else if (code === 'ENOTFOUND') { - logger.log('error', - `Connection ${connectionId}: DNS lookup failed for ${finalTargetHost}. Check your DNS settings or if the hostname is correct.`, - { - connectionId, - targetHost: finalTargetHost, - recommendation: 'Check your DNS settings or if the hostname is correct.', - component: 'route-handler' - } - ); - } - - // Clear any existing error handler after connection phase - targetSocket.removeAllListeners('error'); - - // Re-add the normal error handler for established connections - targetSocket.on('error', this.connectionManager.handleError('outgoing', record)); - - if (record.outgoingTerminationReason === null) { - record.outgoingTerminationReason = 'connection_failed'; - this.connectionManager.incrementTerminationStat('outgoing', 'connection_failed'); - } - - // Clean up the connection - this.connectionManager.initiateCleanupOnce(record, `connection_failed_${code}`); - }); - } /** * Sets up a direct connection to the target @@ -1090,6 +1045,16 @@ export class RouteConnectionHandler { host: finalTargetHost, onError: (error) => { // Connection failed - clean up everything immediately + // Check if connection record is still valid (client might have disconnected) + if (record.connectionClosed) { + logger.log('debug', `Backend connection failed but client already disconnected for ${connectionId}`, { + connectionId, + errorCode: (error as any).code, + component: 'route-handler' + }); + return; + } + logger.log('error', `Connection setup error for ${connectionId} to ${finalTargetHost}:${finalTargetPort}: ${error.message} (${(error as any).code})`, { @@ -1117,10 +1082,12 @@ export class RouteConnectionHandler { } // Resume the incoming socket to prevent it from hanging - socket.resume(); + if (socket && !socket.destroyed) { + socket.resume(); + } // Clean up the incoming socket - if (!socket.destroyed) { + if (socket && !socket.destroyed) { socket.destroy(); } @@ -1294,7 +1261,7 @@ export class RouteConnectionHandler { } }); - // Only set up basic properties - everything else happens in onConnect + // Set outgoing socket immediately so it can be cleaned up if client disconnects record.outgoing = targetSocket; record.outgoingStartTime = Date.now();