From 878e5492114d7a3e332c5fdbf5e5c211233fbdfd Mon Sep 17 00:00:00 2001 From: Thomas Becker Date: Thu, 18 Dec 2025 12:41:40 +0100 Subject: [PATCH] fix: use true connection timeout instead of idle timeout socket.setTimeout() is an idle timeout in Node.js (node docs)[https://nodejs.org/api/net.html#socketsettimeouttimeout-callback] - it fires after N ms of inactivity, not N ms after the connection attempt. This caused false timeout errors after successful data transfers when connections went idle for longer than the timeout period. Replace with JS setTimeout() that: - Fires N ms after connection attempt starts - Gets cleared on successful connect - Return 504 Gateway Timeout (more accurate than 502) Also adds proper close event handlers for socket cleanup. Fixes #228 --- .../registryProxy.connect-tunnel.spec.js | 14 ++-- .../src/registryProxy/tunnelRequestHandler.js | 66 +++++++++++-------- 2 files changed, 47 insertions(+), 33 deletions(-) diff --git a/packages/safe-chain/src/registryProxy/registryProxy.connect-tunnel.spec.js b/packages/safe-chain/src/registryProxy/registryProxy.connect-tunnel.spec.js index b6b0ed0..ace84ee 100644 --- a/packages/safe-chain/src/registryProxy/registryProxy.connect-tunnel.spec.js +++ b/packages/safe-chain/src/registryProxy/registryProxy.connect-tunnel.spec.js @@ -182,13 +182,13 @@ describe("registryProxy.connectTunnel", () => { const duration = Date.now() - startTime; - // Should return 502 Bad Gateway + // Should return 504 Gateway Timeout (not 502 - 504 is for actual timeouts) assert.ok( - responseData.includes("HTTP/1.1 502 Bad Gateway"), - "Should return 502 for timeout" + responseData.includes("HTTP/1.1 504 Gateway Timeout"), + "Should return 504 for timeout" ); - // Should timeout around 3 seconds for IMDS endpoints (allow some margin) + // Should timeout around 100ms for IMDS endpoints (allow some margin) assert.ok( duration >= 80 && duration < 200, `IMDS timeout should be ~80-200ms, got ${duration}ms` @@ -280,10 +280,10 @@ describe("registryProxy.connectTunnel", () => { const duration = Date.now() - startTime; - // Should return 502 Bad Gateway (timeout) + // Should return 504 Gateway Timeout (not 502 - 504 is for actual timeouts) assert.ok( - responseData.includes("HTTP/1.1 502 Bad Gateway"), - "Should return 502 for timeout" + responseData.includes("HTTP/1.1 504 Gateway Timeout"), + "Should return 504 for timeout" ); // Should NOT be instant - it should retry the connection (taking ~500ms due to mock timeout) diff --git a/packages/safe-chain/src/registryProxy/tunnelRequestHandler.js b/packages/safe-chain/src/registryProxy/tunnelRequestHandler.js index bde9c17..5eac381 100644 --- a/packages/safe-chain/src/registryProxy/tunnelRequestHandler.js +++ b/packages/safe-chain/src/registryProxy/tunnelRequestHandler.js @@ -43,6 +43,7 @@ export function tunnelRequest(req, clientSocket, head) { function tunnelRequestToDestination(req, clientSocket, head) { const { port, hostname } = new URL(`http://${req.url}`); const isImds = isImdsEndpoint(hostname); + const targetPort = Number.parseInt(port) || 443; if (timedoutImdsEndpoints.includes(hostname)) { clientSocket.end("HTTP/1.1 502 Bad Gateway\r\n\r\n"); @@ -58,64 +59,77 @@ function tunnelRequestToDestination(req, clientSocket, head) { return; } - const serverSocket = net.connect( - Number.parseInt(port) || 443, - hostname, - () => { - clientSocket.write("HTTP/1.1 200 Connection Established\r\n\r\n"); - serverSocket.write(head); - serverSocket.pipe(clientSocket); - clientSocket.pipe(serverSocket); - } - ); - - // Set explicit connection timeout to avoid waiting for OS default (~2 minutes). - // IMDS endpoints get shorter timeout (3s) since they're commonly unreachable outside cloud environments. const connectTimeout = getConnectTimeout(hostname); - serverSocket.setTimeout(connectTimeout); - serverSocket.on("timeout", () => { - // Suppress error logging for IMDS endpoints - timeouts are expected when not in cloud + // Use JS setTimeout for true connection timeout (not idle timeout). + // socket.setTimeout() measures inactivity, not time since connection attempt. + const connectTimer = setTimeout(() => { if (isImds) { timedoutImdsEndpoints.push(hostname); ui.writeVerbose( - `Safe-chain: connect to ${hostname}:${ - port || 443 - } timed out after ${connectTimeout}ms` + `Safe-chain: connect to ${hostname}:${targetPort} timed out after ${connectTimeout}ms` ); } else { ui.writeError( - `Safe-chain: connect to ${hostname}:${ - port || 443 - } timed out after ${connectTimeout}ms` + `Safe-chain: connect to ${hostname}:${targetPort} timed out after ${connectTimeout}ms` ); } - serverSocket.destroy(); // Clean up socket to prevent event loop hanging - clientSocket.end("HTTP/1.1 502 Bad Gateway\r\n\r\n"); + serverSocket.destroy(); + if (clientSocket.writable) { + clientSocket.end("HTTP/1.1 504 Gateway Timeout\r\n\r\n"); + } + }, connectTimeout); + + const serverSocket = net.connect(targetPort, hostname, () => { + // Clear timer to prevent false timeout errors after successful connection + clearTimeout(connectTimer); + + clientSocket.write("HTTP/1.1 200 Connection Established\r\n\r\n"); + serverSocket.write(head); + serverSocket.pipe(clientSocket); + clientSocket.pipe(serverSocket); }); clientSocket.on("error", () => { // This can happen if the client TCP socket sends RST instead of FIN. // Not subscribing to 'error' event will cause node to throw and crash. + clearTimeout(connectTimer); + if (serverSocket.writable) { + serverSocket.end(); + } + }); + + clientSocket.on("close", () => { + // Client closed connection - clean up server socket + clearTimeout(connectTimer); if (serverSocket.writable) { serverSocket.end(); } }); serverSocket.on("error", (err) => { + clearTimeout(connectTimer); if (isImds) { ui.writeVerbose( - `Safe-chain: error connecting to ${hostname}:${port} - ${err.message}` + `Safe-chain: error connecting to ${hostname}:${targetPort} - ${err.message}` ); } else { ui.writeError( - `Safe-chain: error connecting to ${hostname}:${port} - ${err.message}` + `Safe-chain: error connecting to ${hostname}:${targetPort} - ${err.message}` ); } if (clientSocket.writable) { clientSocket.end("HTTP/1.1 502 Bad Gateway\r\n\r\n"); } }); + + serverSocket.on("close", () => { + // Server closed connection - clean up client socket + clearTimeout(connectTimer); + if (clientSocket.writable) { + clientSocket.end(); + } + }); } /**