fix: Postgres node with ssh tunnel getting into a broken state and not being recreated (#16054)

packages/nodes-base/nodes/Postgres/transport/index.ts

@@ -3,8 +3,9 @@ import type {
 	ICredentialTestFunctions,
 	ILoadOptionsFunctions,
 	ITriggerFunctions,
+	Logger,
 } from 'n8n-workflow';
-import { createServer, type AddressInfo } from 'node:net';
+import { createServer, type AddressInfo, type Server } from 'node:net';
 import pgPromise from 'pg-promise';
 
 import { ConnectionPoolManager } from '@utils/connection-pool-manager';
@@ -53,14 +54,37 @@ const getPostgresConfig = (
 	return dbConfig;
 };
 
+function withCleanupHandler(proxy: Server, abortController: AbortController, logger: Logger) {
+	proxy.on('error', (error) => {
+		logger.error('TCP Proxy: Got error, calling abort controller', { error });
+		abortController.abort();
+	});
+	proxy.on('close', () => {
+		logger.error('TCP Proxy: Was closed, calling abort controller');
+		abortController.abort();
+	});
+	proxy.on('drop', (dropArgument) => {
+		logger.error('TCP Proxy: Connection was dropped, calling abort controller', {
+			dropArgument,
+		});
+		abortController.abort();
+	});
+	abortController.signal.addEventListener('abort', () => {
+		logger.debug('Got abort signal. Closing TCP proxy server.');
+		proxy.close();
+	});
+
+	return proxy;
+}
+
 export async function configurePostgres(
 	this: IExecuteFunctions | ICredentialTestFunctions | ILoadOptionsFunctions | ITriggerFunctions,
 	credentials: PostgresNodeCredentials,
 	options: PostgresNodeOptions = {},
 ): Promise<ConnectionsData> {
-	const poolManager = ConnectionPoolManager.getInstance();
+	const poolManager = ConnectionPoolManager.getInstance(this.logger);
 
-	const fallBackHandler = async () => {
+	const fallBackHandler = async (abortController: AbortController) => {
 		const pgp = pgPromise({
 			// prevent spam in console "WARNING: Creating a duplicate database object for the same connection."
 			// duplicate connections created when auto loading parameters, they are closed immediately after, but several could be open at the same time
@@ -101,74 +125,33 @@ export async function configurePostgres(
 			if (credentials.sshAuthenticateWith === 'privateKey' && credentials.privateKey) {
 				credentials.privateKey = formatPrivateKey(credentials.privateKey);
 			}
-			const sshClient = await this.helpers.getSSHClient(credentials);
+			const sshClient = await this.helpers.getSSHClient(credentials, abortController);
 
 			// Create a TCP proxy listening on a random available port
-			const proxy = createServer();
+			const proxy = withCleanupHandler(createServer(), abortController, this.logger);
+
 			const proxyPort = await new Promise<number>((resolve) => {
 				proxy.listen(0, LOCALHOST, () => {
 					resolve((proxy.address() as AddressInfo).port);
 				});
 			});
 
-			const close = () => {
-				proxy.close();
-				sshClient.off('end', close);
-				sshClient.off('error', close);
-			};
-			sshClient.on('end', close);
-			sshClient.on('error', close);
-
-			await new Promise<void>((resolve, reject) => {
-				proxy.on('error', (err) => reject(err));
-				proxy.on('connection', (localSocket) => {
-					sshClient.forwardOut(
-						LOCALHOST,
-						localSocket.remotePort!,
-						credentials.host,
-						credentials.port,
-						(err, clientChannel) => {
-							if (err) {
-								proxy.close();
-								localSocket.destroy();
-							} else {
-								localSocket.pipe(clientChannel);
-								clientChannel.pipe(localSocket);
-							}
-						},
-					);
-				});
-				resolve();
-			}).catch((err) => {
-				proxy.close();
-
-				let message = err.message;
-				let description = err.description;
-
-				if (err.message.includes('ECONNREFUSED')) {
-					message = 'Connection refused';
-					try {
-						description = err.message.split('ECONNREFUSED ')[1].trim();
-					} catch (e) {}
-				}
-
-				if (err.message.includes('ENOTFOUND')) {
-					message = 'Host not found';
-					try {
-						description = err.message.split('ENOTFOUND ')[1].trim();
-					} catch (e) {}
-				}
-
-				if (err.message.includes('ETIMEDOUT')) {
-					message = 'Connection timed out';
-					try {
-						description = err.message.split('ETIMEDOUT ')[1].trim();
-					} catch (e) {}
-				}
-
-				err.message = message;
-				err.description = description;
-				throw err;
+			proxy.on('connection', (localSocket) => {
+				sshClient.forwardOut(
+					LOCALHOST,
+					localSocket.remotePort!,
+					credentials.host,
+					credentials.port,
+					(error, clientChannel) => {
+						if (error) {
+							this.logger.error('SSH Client: Port forwarding encountered an error', { error });
+							abortController.abort();
+						} else {
+							localSocket.pipe(clientChannel);
+							clientChannel.pipe(localSocket);
+						}
+					},
+				);
 			});
 
 			const db = pgp({
@@ -176,7 +159,20 @@ export async function configurePostgres(
 				port: proxyPort,
 				host: LOCALHOST,
 			});
-			return { db, pgp };
+
+			abortController.signal.addEventListener('abort', async () => {
+				this.logger.debug('configurePostgres: Got abort signal, closing pg connection.');
+				try {
+					if (!db.$pool.ended) await db.$pool.end();
+				} catch (error) {
+					this.logger.error('configurePostgres: Encountered error while closing the pool.', {
+						error,
+					});
+					throw error;
+				}
+			});
+
+			return { db, pgp, sshClient };
 		}
 	};
 
@@ -185,8 +181,10 @@ export async function configurePostgres(
 		nodeType: 'postgres',
 		nodeVersion: options.nodeVersion as unknown as string,
 		fallBackHandler,
-		cleanUpHandler: async ({ db }) => {
-			if (!db.$pool.ended) await db.$pool.end();
+		wasUsed: ({ sshClient }) => {
+			if (sshClient) {
+				this.helpers.updateLastUsed(sshClient);
+			}
 		},
 	});
 }

packages/nodes-base/utils/__tests__/connection-pool-manager.test.ts

@@ -1,26 +1,31 @@
+import { mock } from 'jest-mock-extended';
+import { OperationalError, type Logger } from 'n8n-workflow';
+
 import { ConnectionPoolManager } from '@utils/connection-pool-manager';
 
 const ttl = 5 * 60 * 1000;
 const cleanUpInterval = 60 * 1000;
 
+const logger = mock<Logger>();
+
 let cpm: ConnectionPoolManager;
 
 beforeAll(() => {
 	jest.useFakeTimers();
-	cpm = ConnectionPoolManager.getInstance();
+	cpm = ConnectionPoolManager.getInstance(logger);
 });
 
 beforeEach(async () => {
-	await cpm.purgeConnections();
+	cpm.purgeConnections();
 });
 
 afterAll(() => {
-	cpm.onShutdown();
+	cpm.purgeConnections();
 });
 
 test('getInstance returns a singleton', () => {
-	const instance1 = ConnectionPoolManager.getInstance();
-	const instance2 = ConnectionPoolManager.getInstance();
+	const instance1 = ConnectionPoolManager.getInstance(logger);
+	const instance2 = ConnectionPoolManager.getInstance(logger);
 
 	expect(instance1).toBe(instance2);
 });
@@ -29,25 +34,27 @@ describe('getConnection', () => {
 	test('calls fallBackHandler only once and returns the first value', async () => {
 		// ARRANGE
 		const connectionType = {};
-		const fallBackHandler = jest.fn().mockResolvedValue(connectionType);
-		const cleanUpHandler = jest.fn();
+		const fallBackHandler = jest.fn(async () => {
+			return connectionType;
+		});
+
 		const options = {
 			credentials: {},
 			nodeType: 'example',
 			nodeVersion: '1',
 			fallBackHandler,
-			cleanUpHandler,
+			wasUsed: jest.fn(),
 		};
 
 		// ACT 1
-		const connection = await cpm.getConnection<string>(options);
+		const connection = await cpm.getConnection(options);
 
 		// ASSERT 1
 		expect(fallBackHandler).toHaveBeenCalledTimes(1);
 		expect(connection).toBe(connectionType);
 
 		// ACT 2
-		const connection2 = await cpm.getConnection<string>(options);
+		const connection2 = await cpm.getConnection(options);
 		// ASSERT 2
 		expect(fallBackHandler).toHaveBeenCalledTimes(1);
 		expect(connection2).toBe(connectionType);
@@ -56,27 +63,29 @@ describe('getConnection', () => {
 	test('creates different pools for different node versions', async () => {
 		// ARRANGE
 		const connectionType1 = {};
-		const fallBackHandler1 = jest.fn().mockResolvedValue(connectionType1);
-		const cleanUpHandler1 = jest.fn();
+		const fallBackHandler1 = jest.fn(async () => {
+			return connectionType1;
+		});
 
 		const connectionType2 = {};
-		const fallBackHandler2 = jest.fn().mockResolvedValue(connectionType2);
-		const cleanUpHandler2 = jest.fn();
+		const fallBackHandler2 = jest.fn(async () => {
+			return connectionType2;
+		});
 
 		// ACT 1
-		const connection1 = await cpm.getConnection<string>({
+		const connection1 = await cpm.getConnection({
 			credentials: {},
 			nodeType: 'example',
 			nodeVersion: '1',
 			fallBackHandler: fallBackHandler1,
-			cleanUpHandler: cleanUpHandler1,
+			wasUsed: jest.fn(),
 		});
-		const connection2 = await cpm.getConnection<string>({
+		const connection2 = await cpm.getConnection({
 			credentials: {},
 			nodeType: 'example',
 			nodeVersion: '2',
 			fallBackHandler: fallBackHandler2,
-			cleanUpHandler: cleanUpHandler2,
+			wasUsed: jest.fn(),
 		});
 
 		// ASSERT
@@ -92,21 +101,52 @@ describe('getConnection', () => {
 	test('calls cleanUpHandler after TTL expires', async () => {
 		// ARRANGE
 		const connectionType = {};
-		const fallBackHandler = jest.fn().mockResolvedValue(connectionType);
-		const cleanUpHandler = jest.fn();
-		await cpm.getConnection<string>({
+		let abortController: AbortController | undefined;
+		const fallBackHandler = jest.fn(async (ac: AbortController) => {
+			abortController = ac;
+			return connectionType;
+		});
+		await cpm.getConnection({
 			credentials: {},
 			nodeType: 'example',
 			nodeVersion: '1',
 			fallBackHandler,
-			cleanUpHandler,
+			wasUsed: jest.fn(),
 		});
 
 		// ACT
 		jest.advanceTimersByTime(ttl + cleanUpInterval * 2);
 
 		// ASSERT
-		expect(cleanUpHandler).toHaveBeenCalledTimes(1);
+		if (abortController === undefined) {
+			fail("abortController haven't been initialized");
+		}
+		expect(abortController.signal.aborted).toBe(true);
+	});
+
+	test('throws OperationsError if the fallBackHandler aborts during connection initialization', async () => {
+		// ARRANGE
+		const connectionType = {};
+		const fallBackHandler = jest.fn(async (ac: AbortController) => {
+			ac.abort();
+			return connectionType;
+		});
+
+		// ACT
+		const connectionPromise = cpm.getConnection({
+			credentials: {},
+			nodeType: 'example',
+			nodeVersion: '1',
+			fallBackHandler,
+			wasUsed: jest.fn(),
+		});
+
+		// ASSERT
+
+		await expect(connectionPromise).rejects.toThrow(OperationalError);
+		await expect(connectionPromise).rejects.toThrow(
+			'Could not create pool. Connection attempt was aborted.',
+		);
 	});
 });
 
@@ -114,66 +154,115 @@ describe('onShutdown', () => {
 	test('calls all clean up handlers', async () => {
 		// ARRANGE
 		const connectionType1 = {};
-		const fallBackHandler1 = jest.fn().mockResolvedValue(connectionType1);
-		const cleanUpHandler1 = jest.fn();
-		await cpm.getConnection<string>({
+		let abortController1: AbortController | undefined;
+		const fallBackHandler1 = jest.fn(async (ac: AbortController) => {
+			abortController1 = ac;
+			return connectionType1;
+		});
+		await cpm.getConnection({
 			credentials: {},
 			nodeType: 'example',
 			nodeVersion: '1',
 			fallBackHandler: fallBackHandler1,
-			cleanUpHandler: cleanUpHandler1,
+			wasUsed: jest.fn(),
 		});
 
 		const connectionType2 = {};
-		const fallBackHandler2 = jest.fn().mockResolvedValue(connectionType2);
-		const cleanUpHandler2 = jest.fn();
-		await cpm.getConnection<string>({
+		let abortController2: AbortController | undefined;
+		const fallBackHandler2 = jest.fn(async (ac: AbortController) => {
+			abortController2 = ac;
+			return connectionType2;
+		});
+		await cpm.getConnection({
 			credentials: {},
 			nodeType: 'example',
 			nodeVersion: '2',
 			fallBackHandler: fallBackHandler2,
-			cleanUpHandler: cleanUpHandler2,
+			wasUsed: jest.fn(),
 		});
 
-		// ACT 1
-		cpm.onShutdown();
+		// ACT
+		cpm.purgeConnections();
 
 		// ASSERT
-		expect(cleanUpHandler1).toHaveBeenCalledTimes(1);
-		expect(cleanUpHandler2).toHaveBeenCalledTimes(1);
+		if (abortController1 === undefined || abortController2 === undefined) {
+			fail("abortController haven't been initialized");
+		}
+		expect(abortController1.signal.aborted).toBe(true);
+		expect(abortController2.signal.aborted).toBe(true);
 	});
 
 	test('calls all clean up handlers when `exit` is emitted on process', async () => {
 		// ARRANGE
 		const connectionType1 = {};
-		const fallBackHandler1 = jest.fn().mockResolvedValue(connectionType1);
-		const cleanUpHandler1 = jest.fn();
-		await cpm.getConnection<string>({
+		let abortController1: AbortController | undefined;
+		const fallBackHandler1 = jest.fn(async (ac: AbortController) => {
+			abortController1 = ac;
+			return connectionType1;
+		});
+		await cpm.getConnection({
 			credentials: {},
 			nodeType: 'example',
 			nodeVersion: '1',
 			fallBackHandler: fallBackHandler1,
-			cleanUpHandler: cleanUpHandler1,
+			wasUsed: jest.fn(),
 		});
 
 		const connectionType2 = {};
-		const fallBackHandler2 = jest.fn().mockResolvedValue(connectionType2);
-		const cleanUpHandler2 = jest.fn();
-		await cpm.getConnection<string>({
+		let abortController2: AbortController | undefined;
+		const fallBackHandler2 = jest.fn(async (ac: AbortController) => {
+			abortController2 = ac;
+			return connectionType2;
+		});
+		await cpm.getConnection({
 			credentials: {},
 			nodeType: 'example',
 			nodeVersion: '2',
 			fallBackHandler: fallBackHandler2,
-			cleanUpHandler: cleanUpHandler2,
+			wasUsed: jest.fn(),
 		});
 
-		// ACT 1
+		// ACT
 		// @ts-expect-error we're not supposed to emit `exit` so it's missing from
 		// the type definition
 		process.emit('exit');
 
 		// ASSERT
-		expect(cleanUpHandler1).toHaveBeenCalledTimes(1);
-		expect(cleanUpHandler2).toHaveBeenCalledTimes(1);
+		if (abortController1 === undefined || abortController2 === undefined) {
+			fail("abortController haven't been initialized");
+		}
+		expect(abortController1.signal.aborted).toBe(true);
+		expect(abortController2.signal.aborted).toBe(true);
+	});
+});
+
+describe('wasUsed', () => {
+	test('is called for every successive `getConnection` call', async () => {
+		// ARRANGE
+		const connectionType = {};
+		const fallBackHandler = jest.fn(async () => {
+			return connectionType;
+		});
+
+		const wasUsed = jest.fn();
+		const options = {
+			credentials: {},
+			nodeType: 'example',
+			nodeVersion: '1',
+			fallBackHandler,
+			wasUsed,
+		};
+
+		// ACT 1
+		await cpm.getConnection(options);
+
+		// ASSERT 1
+		expect(wasUsed).toHaveBeenCalledTimes(0);
+
+		// ACT 2
+		await cpm.getConnection(options);
+
+		// ASSERT 2
+		expect(wasUsed).toHaveBeenCalledTimes(1);
 	});
 });

packages/nodes-base/utils/connection-pool-manager.ts

@@ -1,4 +1,5 @@
 import { createHash } from 'crypto';
+import { OperationalError, type Logger } from 'n8n-workflow';
 
 let instance: ConnectionPoolManager;
 
@@ -15,19 +16,23 @@ type RegistrationOptions = {
 };
 
 type GetConnectionOption<Pool> = RegistrationOptions & {
-	/** When a node requests for a connection pool, but none is available, this handler is called to create new instance of the pool, which then cached and re-used until it goes stale.  */
-	fallBackHandler: () => Promise<Pool>;
+	/**
+	 * When a node requests for a connection pool, but none is available, this
+	 * handler is called to create new instance of the pool, which is then cached
+	 * and re-used until it goes stale.
+	 */
+	fallBackHandler: (abortController: AbortController) => Promise<Pool>;
 
-	/** When a pool hasn't been used in a while, or when the server is shutting down, this handler is invoked to close the pool */
-	cleanUpHandler: (pool: Pool) => Promise<void>;
+	wasUsed: (pool: Pool) => void;
 };
 
 type Registration<Pool> = {
 	/** This is an instance of a Connection Pool class, that gets reused across multiple executions */
 	pool: Pool;
 
-	/** @see GetConnectionOption['closeHandler'] */
-	cleanUpHandler: (pool: Pool) => Promise<void>;
+	abortController: AbortController;
+
+	wasUsed: (pool: Pool) => void;
 
 	/** We keep this timestamp to check if a pool hasn't been used in a while, and if it needs to be closed */
 	lastUsed: number;
@@ -38,9 +43,9 @@ export class ConnectionPoolManager {
 	 * Gets the singleton instance of the ConnectionPoolManager.
 	 * Creates a new instance if one doesn't exist.
 	 */
-	static getInstance(): ConnectionPoolManager {
+	static getInstance(logger: Logger): ConnectionPoolManager {
 		if (!instance) {
-			instance = new ConnectionPoolManager();
+			instance = new ConnectionPoolManager(logger);
 		}
 		return instance;
 	}
@@ -51,9 +56,12 @@ export class ConnectionPoolManager {
 	 * Private constructor that initializes the connection pool manager.
 	 * Sets up cleanup handlers for process exit and stale connections.
 	 */
-	private constructor() {
+	private constructor(private readonly logger: Logger) {
 		// Close all open pools when the process exits
-		process.on('exit', () => this.onShutdown());
+		process.on('exit', () => {
+			this.logger.debug('ConnectionPoolManager: Shutting down. Cleaning up all pools');
+			this.purgeConnections();
+		});
 
 		// Regularly close stale pools
 		setInterval(() => this.cleanupStaleConnections(), cleanUpInterval);
@@ -84,54 +92,67 @@ export class ConnectionPoolManager {
 		const key = this.makeKey(options);
 
 		let value = this.map.get(key);
-		if (!value) {
-			value = {
-				pool: await options.fallBackHandler(),
-				cleanUpHandler: options.cleanUpHandler,
-			} as Registration<unknown>;
+
+		if (value) {
+			value.lastUsed = Date.now();
+			value.wasUsed(value.pool);
+			return value.pool as T;
+		}
+
+		const abortController = new AbortController();
+		value = {
+			pool: await options.fallBackHandler(abortController),
+			abortController,
+			wasUsed: options.wasUsed,
+		} as Registration<unknown>;
+
+		// It's possible that `options.fallBackHandler` already called the abort
+		// function. If that's the case let's not continue.
+		if (abortController.signal.aborted) {
+			throw new OperationalError('Could not create pool. Connection attempt was aborted.', {
+				cause: abortController.signal.reason,
+			});
 		}
 
 		this.map.set(key, { ...value, lastUsed: Date.now() });
+		abortController.signal.addEventListener('abort', async () => {
+			this.logger.debug('ConnectionPoolManager: Got abort signal, cleaning up pool.');
+			this.cleanupConnection(key);
+		});
+
 		return value.pool as T;
 	}
 
+	private cleanupConnection(key: string) {
+		const registration = this.map.get(key);
+
+		if (registration) {
+			this.map.delete(key);
+			registration.abortController.abort();
+		}
+	}
+
 	/**
 	 * Removes and cleans up connection pools that haven't been used within the
 	 * TTL.
 	 */
 	private cleanupStaleConnections() {
 		const now = Date.now();
-		for (const [key, { cleanUpHandler, lastUsed, pool }] of this.map.entries()) {
+		for (const [key, { lastUsed }] of this.map.entries()) {
 			if (now - lastUsed > ttl) {
-				void cleanUpHandler(pool);
-				this.map.delete(key);
+				this.logger.debug('ConnectionPoolManager: Found stale pool. Cleaning it up.');
+				void this.cleanupConnection(key);
 			}
 		}
 	}
 
 	/**
 	 * Removes and cleans up all existing connection pools.
+	 * Connections are closed in the background.
 	 */
-	async purgeConnections(): Promise<void> {
-		await Promise.all(
-			[...this.map.entries()].map(async ([key, value]) => {
-				this.map.delete(key);
-
-				return await value.cleanUpHandler(value.pool);
-			}),
-		);
-	}
-
-	/**
-	 * Cleans up all connection pools when the process is shutting down.
-	 * Does not wait for cleanup promises to resolve also does not remove the
-	 * references from the pool.
-	 *
-	 * Only call this on process shutdown.
-	 */
-	onShutdown() {
-		for (const { cleanUpHandler, pool } of this.map.values()) {
-			void cleanUpHandler(pool);
+	purgeConnections(): void {
+		for (const key of this.map.keys()) {
+			this.cleanupConnection(key);
 		}
 	}
 }