diff --git a/readme.plan.md b/readme.plan.md deleted file mode 100644 index f148f78..0000000 --- a/readme.plan.md +++ /dev/null @@ -1,165 +0,0 @@ -# SmartProxy Socket Handling Fix Plan - -Reread CLAUDE.md file for guidelines - -## Implementation Summary (COMPLETED) - -The critical socket handling issues have been fixed: - -1. **Prevented Server Crashes**: Created `createSocketWithErrorHandler()` utility that attaches error handlers immediately upon socket creation, preventing unhandled ECONNREFUSED errors from crashing the server. - -2. **Fixed Memory Leaks**: Updated forwarding handlers to properly clean up client sockets when server connections fail, ensuring connection records are removed from tracking. - -3. **Key Changes Made**: - - Added `createSocketWithErrorHandler()` in `socket-utils.ts` - - Updated `https-passthrough-handler.ts` to use safe socket creation - - Updated `https-terminate-to-http-handler.ts` to use safe socket creation - - Ensured client sockets are destroyed when server connections fail - - Connection cleanup now triggered by socket close events - -4. **Test Results**: Server no longer crashes on ECONNREFUSED errors, and connections are properly cleaned up. - -## Problem Summary - -The SmartProxy server is experiencing critical issues: -1. **Server crashes** due to unhandled socket connection errors (ECONNREFUSED) -2. **Memory leak** with steadily rising active connection count -3. **Race conditions** between socket creation and error handler attachment -4. **Orphaned sockets** when server connections fail - -## Root Causes - -### 1. Delayed Error Handler Attachment -- Sockets created without immediate error handlers -- Error events can fire before handlers attached -- Causes uncaught exceptions and server crashes - -### 2. Incomplete Cleanup Logic -- Client sockets not cleaned up when server connection fails -- Connection counter only decrements after BOTH sockets close -- Failed server connections leave orphaned client sockets - -### 3. Missing Global Error Handlers -- No process-level uncaughtException handler -- No process-level unhandledRejection handler -- Any unhandled error crashes entire server - -## Implementation Plan - -### Phase 1: Prevent Server Crashes (Critical) - -#### 1.1 Add Global Error Handlers -- [x] ~~Add global error handlers in main entry point~~ (Removed per user request - no global handlers) -- [x] Log errors with context -- [x] ~~Implement graceful shutdown sequence~~ (Removed - handled locally) - -#### 1.2 Fix Socket Creation Race Condition -- [x] Modify socket creation to attach error handlers immediately -- [x] Update all forwarding handlers (https-passthrough, http, etc.) -- [x] Ensure error handlers attached in same tick as socket creation - -### Phase 2: Fix Memory Leaks (High Priority) - -#### 2.1 Fix Connection Cleanup Logic -- [x] Clean up client socket immediately if server connection fails -- [x] Decrement connection counter on any socket failure (handled by socket close events) -- [x] Implement proper cleanup for half-open connections - -#### 2.2 Improve Socket Utils -- [x] Create new utility function for safe socket creation with immediate error handling -- [x] Update createIndependentSocketHandlers to handle immediate failures -- [ ] Add connection tracking debug utilities - -### Phase 3: Comprehensive Testing (Important) - -#### 3.1 Create Test Cases -- [x] Test ECONNREFUSED scenario -- [ ] Test timeout handling -- [ ] Test half-open connections -- [ ] Test rapid connect/disconnect cycles - -#### 3.2 Add Monitoring -- [ ] Add connection leak detection -- [ ] Add metrics for connection lifecycle -- [ ] Add debug logging for socket state transitions - -## Detailed Implementation Steps - -### Step 1: Global Error Handlers (ts/proxies/smart-proxy/smart-proxy.ts) -```typescript -// Add in constructor or start method -process.on('uncaughtException', (error) => { - logger.log('error', 'Uncaught exception', { error }); - // Graceful shutdown -}); - -process.on('unhandledRejection', (reason, promise) => { - logger.log('error', 'Unhandled rejection', { reason, promise }); -}); -``` - -### Step 2: Safe Socket Creation Utility (ts/core/utils/socket-utils.ts) -```typescript -export function createSocketWithErrorHandler( - options: net.NetConnectOpts, - onError: (err: Error) => void -): net.Socket { - const socket = net.connect(options); - socket.on('error', onError); - return socket; -} -``` - -### Step 3: Fix HttpsPassthroughHandler (ts/forwarding/handlers/https-passthrough-handler.ts) -- Replace direct socket creation with safe creation -- Handle server connection failures immediately -- Clean up client socket on server connection failure - -### Step 4: Fix Connection Counting -- Decrement on ANY socket close, not just when both close -- Track failed connections separately -- Add connection state tracking - -### Step 5: Update All Handlers -- [ ] https-passthrough-handler.ts -- [ ] http-handler.ts -- [ ] https-terminate-to-http-handler.ts -- [ ] https-terminate-to-https-handler.ts -- [ ] route-connection-handler.ts - -## Success Criteria - -1. **No server crashes** on ECONNREFUSED or other socket errors -2. **Active connections** remain stable (no steady increase) -3. **All sockets** properly cleaned up on errors -4. **Memory usage** remains stable under load -5. **Graceful handling** of all error scenarios - -## Testing Plan - -1. Simulate ECONNREFUSED by targeting closed ports -2. Monitor active connection count over time -3. Stress test with rapid connections -4. Test with unreachable hosts -5. Test with slow/timing out connections - -## Rollback Plan - -If issues arise: -1. Revert socket creation changes -2. Keep global error handlers (they add safety) -3. Add more detailed logging for debugging -4. Implement fixes incrementally - -## Timeline - -- Phase 1: Immediate (prevents crashes) -- Phase 2: Within 24 hours (fixes leaks) -- Phase 3: Within 48 hours (ensures stability) - -## Notes - -- The race condition is the most critical issue -- Connection counting logic needs complete overhaul -- Consider using a connection state machine for clarity -- Add connection lifecycle events for debugging \ 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 282c79b..bd27736 100644 --- a/ts/proxies/smart-proxy/route-connection-handler.ts +++ b/ts/proxies/smart-proxy/route-connection-handler.ts @@ -1074,19 +1074,13 @@ export class RouteConnectionHandler { } // Create the target socket with immediate error handling - let targetSocket: plugins.net.Socket; + let connectionEstablished = false; - // Flag to track if initial connection failed - let connectionFailed = false; - - targetSocket = createSocketWithErrorHandler({ + const targetSocket = createSocketWithErrorHandler({ port: finalTargetPort, host: finalTargetHost, onError: (error) => { - // Mark connection as failed - connectionFailed = true; - - // Connection failed - clean up immediately + // Connection failed - clean up everything immediately logger.log('error', `Connection setup error for ${connectionId} to ${finalTargetHost}:${finalTargetPort}: ${error.message} (${(error as any).code})`, { @@ -1099,6 +1093,20 @@ export class RouteConnectionHandler { } ); + // Log specific error types for easier debugging + if ((error as any).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' + } + ); + } + // Resume the incoming socket to prevent it from hanging socket.resume(); @@ -1107,18 +1115,184 @@ export class RouteConnectionHandler { socket.destroy(); } - // Clean up the connection record - this.connectionManager.initiateCleanupOnce(record, `connection_failed_${(error as any).code || 'unknown'}`); + // Clean up the connection record - this is critical! + this.connectionManager.cleanupConnection(record, `connection_failed_${(error as any).code || 'unknown'}`); + }, + onConnect: () => { + connectionEstablished = true; + + if (this.settings.enableDetailedLogging) { + logger.log('info', `Connection ${connectionId} established to target ${finalTargetHost}:${finalTargetPort}`, { + connectionId, + targetHost: finalTargetHost, + targetPort: finalTargetPort, + component: 'route-handler' + }); + } + + // Clear any error listeners added by createSocketWithErrorHandler + targetSocket.removeAllListeners('error'); + + // Add the normal error handler for established connections + targetSocket.on('error', this.connectionManager.handleError('outgoing', record)); + + // Flush any pending data to target + if (record.pendingData.length > 0) { + const combinedData = Buffer.concat(record.pendingData); + + if (this.settings.enableDetailedLogging) { + console.log( + `[${connectionId}] Forwarding ${combinedData.length} bytes of initial data to target` + ); + } + + // Write pending data immediately + targetSocket.write(combinedData, (err) => { + if (err) { + logger.log('error', `Error writing pending data to target for connection ${connectionId}: ${err.message}`, { + connectionId, + error: err.message, + component: 'route-handler' + }); + return this.connectionManager.initiateCleanupOnce(record, 'write_error'); + } + }); + + // Clear the buffer now that we've processed it + record.pendingData = []; + record.pendingDataSize = 0; + } + + // Set up independent socket handlers for half-open connection support + const { cleanupClient, cleanupServer } = createIndependentSocketHandlers( + socket, + targetSocket, + (reason) => { + this.connectionManager.initiateCleanupOnce(record, reason); + } + ); + + // Setup socket handlers with custom timeout handling + setupSocketHandlers(socket, cleanupClient, (sock) => { + // Don't close on timeout for keep-alive connections + if (record.hasKeepAlive) { + sock.setTimeout(this.settings.socketTimeout || 3600000); + } + }, 'client'); + + setupSocketHandlers(targetSocket, cleanupServer, (sock) => { + // Don't close on timeout for keep-alive connections + if (record.hasKeepAlive) { + sock.setTimeout(this.settings.socketTimeout || 3600000); + } + }, 'server'); + + // Forward data from client to target with backpressure handling + socket.on('data', (chunk: Buffer) => { + record.bytesReceived += chunk.length; + this.timeoutManager.updateActivity(record); + + if (targetSocket.writable) { + const flushed = targetSocket.write(chunk); + + // Handle backpressure + if (!flushed) { + socket.pause(); + targetSocket.once('drain', () => { + socket.resume(); + }); + } + } + }); + + // Forward data from target to client with backpressure handling + targetSocket.on('data', (chunk: Buffer) => { + record.bytesSent += chunk.length; + this.timeoutManager.updateActivity(record); + + if (socket.writable) { + const flushed = socket.write(chunk); + + // Handle backpressure + if (!flushed) { + targetSocket.pause(); + socket.once('drain', () => { + targetSocket.resume(); + }); + } + } + }); + + // Log successful connection + logger.log('info', + `Connection established: ${record.remoteIP} -> ${finalTargetHost}:${finalTargetPort}` + + `${serverName ? ` (SNI: ${serverName})` : record.lockedDomain ? ` (Domain: ${record.lockedDomain})` : ''}`, + { + remoteIP: record.remoteIP, + targetHost: finalTargetHost, + targetPort: finalTargetPort, + sni: serverName || undefined, + domain: !serverName && record.lockedDomain ? record.lockedDomain : undefined, + component: 'route-handler' + } + ); + + // Add TLS renegotiation handler if needed + if (serverName) { + // Create connection info object for the existing connection + const connInfo = { + sourceIp: record.remoteIP, + sourcePort: record.incoming.remotePort || 0, + destIp: record.incoming.localAddress || '', + destPort: record.incoming.localPort || 0, + }; + + // Create a renegotiation handler function + const renegotiationHandler = this.tlsManager.createRenegotiationHandler( + connectionId, + serverName, + connInfo, + (_connectionId, reason) => this.connectionManager.initiateCleanupOnce(record, reason) + ); + + // Store the handler in the connection record so we can remove it during cleanup + record.renegotiationHandler = renegotiationHandler; + + // Add the handler to the socket + socket.on('data', renegotiationHandler); + + if (this.settings.enableDetailedLogging) { + logger.log('info', `TLS renegotiation handler installed for connection ${connectionId} with SNI ${serverName}`, { + connectionId, + serverName, + component: 'route-handler' + }); + } + } + + // Set connection timeout + record.cleanupTimer = this.timeoutManager.setupConnectionTimeout(record, (record, reason) => { + logger.log('warn', `Connection ${connectionId} from ${record.remoteIP} exceeded max lifetime, forcing cleanup`, { + connectionId, + remoteIP: record.remoteIP, + component: 'route-handler' + }); + this.connectionManager.initiateCleanupOnce(record, reason); + }); + + // Mark TLS handshake as complete for TLS connections + if (record.isTLS) { + record.tlsHandshakeComplete = true; + } } }); - // Only proceed with setup if connection didn't fail immediately - if (!connectionFailed) { - record.outgoing = targetSocket; - record.outgoingStartTime = Date.now(); + // Only set up basic properties - everything else happens in onConnect + record.outgoing = targetSocket; + record.outgoingStartTime = Date.now(); - // Apply socket optimizations - targetSocket.setNoDelay(this.settings.noDelay); + // Apply socket optimizations + targetSocket.setNoDelay(this.settings.noDelay); // Apply keep-alive settings if enabled if (this.settings.keepAlive) { @@ -1146,12 +1320,6 @@ export class RouteConnectionHandler { } } - // Setup improved error handling for outgoing connection - this.setupOutgoingErrorHandler(connectionId, targetSocket, record, socket, finalTargetHost, finalTargetPort); - - // Note: Close handlers are managed by independent socket handlers above - // We don't register handleClose here to avoid bilateral cleanup - // Setup error handlers for incoming socket socket.on('error', this.connectionManager.handleError('incoming', record)); @@ -1213,178 +1381,10 @@ export class RouteConnectionHandler { // Apply socket timeouts this.timeoutManager.applySocketTimeouts(record); - // Track outgoing data for bytes counting + // Track outgoing data for bytes counting (moved from the duplicate connect handler) targetSocket.on('data', (chunk: Buffer) => { record.bytesSent += chunk.length; this.timeoutManager.updateActivity(record); }); - - // Wait for the outgoing connection to be ready before setting up piping - targetSocket.once('connect', () => { - if (this.settings.enableDetailedLogging) { - logger.log('info', `Connection ${connectionId} established to target ${finalTargetHost}:${finalTargetPort}`, { - connectionId, - targetHost: finalTargetHost, - targetPort: finalTargetPort, - component: 'route-handler' - }); - } - - // Clear the initial connection error handler - targetSocket.removeAllListeners('error'); - - // Add the normal error handler for established connections - targetSocket.on('error', this.connectionManager.handleError('outgoing', record)); - - // Flush any pending data to target - if (record.pendingData.length > 0) { - const combinedData = Buffer.concat(record.pendingData); - - if (this.settings.enableDetailedLogging) { - console.log( - `[${connectionId}] Forwarding ${combinedData.length} bytes of initial data to target` - ); - } - - // Write pending data immediately - targetSocket.write(combinedData, (err) => { - if (err) { - logger.log('error', `Error writing pending data to target for connection ${connectionId}: ${err.message}`, { - connectionId, - error: err.message, - component: 'route-handler' - }); - return this.connectionManager.initiateCleanupOnce(record, 'write_error'); - } - }); - - // Clear the buffer now that we've processed it - record.pendingData = []; - record.pendingDataSize = 0; - } - - // Set up independent socket handlers for half-open connection support - const { cleanupClient, cleanupServer } = createIndependentSocketHandlers( - socket, - targetSocket, - (reason) => { - this.connectionManager.initiateCleanupOnce(record, reason); - } - ); - - // Setup socket handlers with custom timeout handling - setupSocketHandlers(socket, cleanupClient, (sock) => { - // Don't close on timeout for keep-alive connections - if (record.hasKeepAlive) { - sock.setTimeout(this.settings.socketTimeout || 3600000); - } - }, 'client'); - - setupSocketHandlers(targetSocket, cleanupServer, (sock) => { - // Don't close on timeout for keep-alive connections - if (record.hasKeepAlive) { - sock.setTimeout(this.settings.socketTimeout || 3600000); - } - }, 'server'); - - // Forward data from client to target with backpressure handling - socket.on('data', (chunk: Buffer) => { - record.bytesReceived += chunk.length; - this.timeoutManager.updateActivity(record); - - if (targetSocket.writable) { - const flushed = targetSocket.write(chunk); - - // Handle backpressure - if (!flushed) { - socket.pause(); - targetSocket.once('drain', () => { - socket.resume(); - }); - } - } - }); - - // Forward data from target to client with backpressure handling - targetSocket.on('data', (chunk: Buffer) => { - record.bytesSent += chunk.length; - this.timeoutManager.updateActivity(record); - - if (socket.writable) { - const flushed = socket.write(chunk); - - // Handle backpressure - if (!flushed) { - targetSocket.pause(); - socket.once('drain', () => { - targetSocket.resume(); - }); - } - } - }); - - // Log successful connection - logger.log('info', - `Connection established: ${record.remoteIP} -> ${finalTargetHost}:${finalTargetPort}` + - `${serverName ? ` (SNI: ${serverName})` : record.lockedDomain ? ` (Domain: ${record.lockedDomain})` : ''}`, - { - remoteIP: record.remoteIP, - targetHost: finalTargetHost, - targetPort: finalTargetPort, - sni: serverName || undefined, - domain: !serverName && record.lockedDomain ? record.lockedDomain : undefined, - component: 'route-handler' - } - ); - - // Add TLS renegotiation handler if needed - if (serverName) { - // Create connection info object for the existing connection - const connInfo = { - sourceIp: record.remoteIP, - sourcePort: record.incoming.remotePort || 0, - destIp: record.incoming.localAddress || '', - destPort: record.incoming.localPort || 0, - }; - - // Create a renegotiation handler function - const renegotiationHandler = this.tlsManager.createRenegotiationHandler( - connectionId, - serverName, - connInfo, - (_connectionId, reason) => this.connectionManager.initiateCleanupOnce(record, reason) - ); - - // Store the handler in the connection record so we can remove it during cleanup - record.renegotiationHandler = renegotiationHandler; - - // Add the handler to the socket - socket.on('data', renegotiationHandler); - - if (this.settings.enableDetailedLogging) { - logger.log('info', `TLS renegotiation handler installed for connection ${connectionId} with SNI ${serverName}`, { - connectionId, - serverName, - component: 'route-handler' - }); - } - } - - // Set connection timeout - record.cleanupTimer = this.timeoutManager.setupConnectionTimeout(record, (record, reason) => { - logger.log('warn', `Connection ${connectionId} from ${record.remoteIP} exceeded max lifetime, forcing cleanup`, { - connectionId, - remoteIP: record.remoteIP, - component: 'route-handler' - }); - this.connectionManager.initiateCleanupOnce(record, reason); - }); - - // Mark TLS handshake as complete for TLS connections - if (record.isTLS) { - record.tlsHandshakeComplete = true; - } - }); - } // End of if (!connectionFailed) } } \ No newline at end of file