Pool: failed scale-up connect() is an unhandled rejection, hides real error behind TimeoutError

[kyungseopk1m]

Description

RedisClientPool.execute() scales the pool up by calling #create() without awaiting or catching it:

// packages/client/lib/client/pool.ts:483-485
if (this.totalClients < this._self.#options.maximum) {
  this._self.#create();
}

#create() re-throws when client.connect() fails (pool.ts:443-448), so this becomes an unhandled rejection. Compare with connect(), in the same file, which calls the same #create() but properly awaits it inside try/catch (pool.ts:419-429):

const promises = [];
while (promises.length < this._self.#options.minimum) {
  promises.push(this._self.#create());
}
try {
  await Promise.all(promises);
} catch (err) {
  this.destroy();
  throw err;
}

Effect on the caller: with no idle client available, execute() pushes the task onto #tasksQueue and returns without ever finding out the scale-up connect failed. The task just sits there until acquireTimeout elapses, so the caller gets a TimeoutError instead of the real connection error, and with acquireTimeout: 0 the task never resolves at all.

Repro (published redis@6.2.1, no server needed, port 1 is never listening):

const { createClientPool } = require('redis');
process.on('unhandledRejection', e => console.log('[unhandledRejection]', e.constructor.name, e.message));

const pool = createClientPool(
  { socket: { host: '127.0.0.1', port: 1, reconnectStrategy: false } },
  { minimum: 0, maximum: 4, acquireTimeout: 500 }
);
await pool.connect(); // resolves immediately, minimum is 0
await pool.ping();

Output:

[unhandledRejection] Error connect ECONNREFUSED 127.0.0.1:1
execute() rejected after 502 ms with TimeoutError Timeout waiting for a client after 500ms

With acquireTimeout: 0 the ping() call above just hangs forever instead of rejecting, and tasksQueueLength stays at 1.

The obvious fix is to have execute()'s #create() call reject the queued task it triggered instead of letting it dangle. But that means a second call site starts removing tasks from #tasksQueue, and the removal API isn't safe for that:

// packages/client/lib/client/pool.ts:459-461, 467
const client = this._self.#idleClients.shift(),
  { tail } = this._self.#tasksQueue;
...
this._self.#tasksQueue.remove(task, tail);

tail is captured once, at the moment the task is pushed, and handed to SinglyLinkedList.remove(node, parent) (linked-list.ts:169-189), which trusts it as the node's current predecessor:

remove(node: SinglyLinkedNode<T>, parent: SinglyLinkedNode<T> | undefined) {
  if (node.removed) throw new Error("node already removed");
  ...
  } else if (this.#tail === node) {
    this.#tail = parent;
    parent!.next = undefined;
  } else {
    parent!.next = node.next;
  }
  node.removed = true;
}

If that captured predecessor is itself removed first, this corrupts the list instead of throwing. Using the actual SinglyLinkedList shipped in @redis/client/dist/lib/client/linked-list.js:

const { SinglyLinkedList } = require('@redis/client/dist/lib/client/linked-list.js');
const queue = new SinglyLinkedList();

const t1 = queue.tail; const n1 = queue.push({ id: 'task1' });
const t2 = queue.tail; const n2 = queue.push({ id: 'task2' });
const t3 = queue.tail; const n3 = queue.push({ id: 'task3' });

queue.remove(n2, t2); // task2's connect fails first, task1 still queued -> correct removal
queue.remove(n3, t3); // task3's connect fails next, using its captured predecessor (task2, already gone)

queue.push({ id: 'task4 (real pending task)' });

console.log(queue.shift()); // task1
console.log(queue.shift()); // task3 -- already-removed, already-rejected task runs again
console.log(queue.length);  // 0, even though task4 never ran

Output:

{ id: 'task1' }
{ id: 'task3' }
0

task4 is silently orphaned (unreachable from head, will never be shifted out and never gets a client), while task3, already marked removed and already rejected once, gets handed the next available client and runs a second time. tasksQueueLength reports 0 the whole time, so nothing about this is observable from the outside.

This is latent in the current code too, execute()'s own acquireTimeout handler is the only existing caller of #tasksQueue.remove and uses this same captured-tail pattern. It just never surfaces today because same-duration acquireTimeout timers always expire in push order, and removing the current head is always safe regardless of what parent is passed in. It stops being safe the moment a second, non-FIFO caller (like the obvious fix above) is added.

(This is a different code path from #3062/#3085, that one was DoublyLinkedList corruption in #clientsInUse via #returnClient, already fixed. This is SinglyLinkedList/#tasksQueue.)

Not sure what the preferred direction is here, doubly linking #tasksQueue or having remove() re-derive the real predecessor instead of trusting a captured one both seem like they'd close this, but wanted to raise it before assuming which. Happy to help once there's a direction.

Node.js Version

v22.16.0

Redis Server Version

n/a, reproduces without a reachable Redis server

Node Redis Version

6.2.1

Platform

macOS (darwin), also inspected against packages/client/lib/client/pool.ts / linked-list.ts in the repo directly

Logs

[unhandledRejection] Error connect ECONNREFUSED 127.0.0.1:1
execute() rejected after 502 ms with TimeoutError Timeout waiting for a client after 500ms

[nkaradzhov]

Thanks for the detailed report and repro — both defects confirmed. Split into two PRs:

• fix(client): propagate scale-up connect failure to the waiting pool task #3401 — scale-up #create() failure now rejects the waiting task with the real connect error instead of dangling / masking it behind TimeoutError.
• fix(client): back pool task queue with a doubly-linked list #3402 — #tasksQueue switched to DoublyLinkedList, so remove(node) uses the live predecessor and the stale-tail corruption is gone. You called the direction exactly right.

Good catch on the latent one — nice analysis.

[nkaradzhov]

@kyungseopk1m thanks again, PRs merged, this issue should be fixed. Closing now