Repository navigation
fix(redis): clear commandTimeout timers of commands stranded on disconnect - #2169
abhijeet117 wants to merge 4 commits into
Conversation
…nnect disconnect() flushed pending queues only when the client was still in the wait status. When disconnecting while a connection attempt was still pending or while waiting to reconnect, no stream event ever fired, so commands parked in the offline queue kept their armed commandTimeout timers and later rejected with Command timed out even though they were never sent. Run the same closeHandler cleanup in those states so queued commands are rejected with Connection is closed. and every pending timer is cleared. Fixes redis#1535
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a73cb93376
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| if ( | ||
| !reconnect && | ||
| this.status !== "end" && | ||
| (!this.stream || this.status === "reconnecting") |
There was a problem hiding this comment.
After a client has connected once, this.stream is never cleared, so during a subsequent retry it can have status connecting while still referencing the previous closed socket. If disconnect() is called while that retry is waiting on Sentinel resolution or a custom connector whose connect() promise is hung, this condition is false, connector.disconnect() cannot produce another close event from the old socket, and closeHandler never flushes the offline queue; queued commands therefore still reject with Command timed out and the client may remain connecting. Synchronous cleanup should also cover connecting with a stale or destroyed stream, or the stream reference should be cleared when it closes.
Useful? React with 👍 / 👎.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0b18fc0fa5
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| (!this.stream || | ||
| this.stream.destroyed || | ||
| this.status === "reconnecting") |
There was a problem hiding this comment.
When a socket is destroyed but has not yet emitted close—for example, when an application calls disconnect() from an error listener after ECONNREFUSED—this predicate invokes closeHandler synchronously even though the registered socket close handler will still run. The first invocation consumes manuallyClosing and ends the client; the second sees manuallyClosing === false, applies the default retry strategy, and reconnects despite the explicit disconnect. Make close cleanup idempotent per connection generation, or distinguish a stream whose close event was already delivered from one that is merely destroyed.
Useful? React with 👍 / 👎.
| this.stream.destroyed || | ||
| this.status === "reconnecting") | ||
| ) { | ||
| eventHandler.closeHandler(this)(); |
There was a problem hiding this comment.
If disconnect() reaches this synchronous close while connector.connect() is pending, the client immediately becomes end, so a same-tick connect() starts a second attempt before the first settles. Since _connect() does not capture and validate its attempt epoch, a late success can install handlers for an obsolete socket, while a late failure runs the shared flushQueue() and resets the surviving attempt to end; the standalone connector's shared connecting flag can also revive its canceled next-tick attempt after the second call sets that flag again. Invalidate and discard/close superseded completions, following the captured-epoch pattern used by Cluster.connect().
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using high effort and found 1 potential issue.
Reviewed by Cursor Bugbot for commit b14cefc. Configure here.
|
@abhijeet117, thank you for the PR. Could you please check why the CI checks are failing? |
|
@PavelPashov I reproduced the stale connect race and fixed it. The targeted commandTimeout suite passes locally; the current CI matrix remains red due the repo environment failing unrelated TypeScript DOM/lib setup, not the disconnect fix itself. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e3636a94ae
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| if (self.status === "end") { | ||
| debug("skip closing because the client is already in the end status"); | ||
| return; |
There was a problem hiding this comment.
When a destroyed old stream has queued its close event, disconnect() can synchronously move the client to end, after which an immediate connect() moves it back to connecting before that event runs. This guard then no longer recognizes the old handler as stale: it executes the retry path, changes the status to reconnecting, and causes the new attempt's epoch callback to reject and destroy its stream. Although the new callback epoch check handles stale connection completions, this fresh path shows that close handlers also need to be bound to their stream or connection generation.
Useful? React with 👍 / 👎.
Thanks for checking. CI still reports two failures. Could you please investigate and rerun the full test suite? |
|
Another real-world case for the Chain: BullMQ's read-timeout watchdog ( Standalone repro, no BullMQ, against a local Redis: const Redis = require("ioredis");
const { once } = require("node:events");
(async () => {
const redis = new Redis({ retryStrategy: () => 60_000 });
await once(redis, "ready");
const blocking = redis.blpop(`repro:${Date.now()}`, 0);
redis.disconnect(true);
await once(redis, "reconnecting");
redis.disconnect();
const outcome = await Promise.race([
blocking.then(() => "blpop resolved", (err) => `blpop rejected: ${err.message}`),
new Promise((resolve) => setTimeout(resolve, 3_000, "still pending after 3s (hang)")),
]);
console.log(`status=${redis.status} — ${outcome}`);
process.exit(0);
})();
One gap remains on the PR head. redis.disconnect(true);
await once(redis, "reconnecting");
redis.resetCommandQueue(); // what connectHandler does on a reconnect that fails before "ready"
redis.disconnect();
Minimal fix in function close() {
self.setStatus("end");
self.flushQueue(new Error(CONNECTION_CLOSED_ERROR_MSG));
if (self.prevCommandQueue) {
let item;
while ((item = self.prevCommandQueue.shift())) {
item.command.reject(new Error(CONNECTION_CLOSED_ERROR_MSG));
}
self.prevCommandQueue = null;
}
}I can either fold this into #2169 or open a small follow-up PR once it merges. Disclaimer: fix written with Claude Code. |
@UnexpectedLobster please open a small follow-up once this merges. |
|
This PR also fixes a second symptom of the same gap, one that needs no On 6.0.0,
Anything that waits for Versions: ioredis 6.0.0 ( Repro (default options; a TCP relay stands in for Redis dying): 'use strict';
// ioredis 6.0.0: disconnect() while "reconnecting" never emits "end" and never settles queued commands.
// REDIS_PORT=6379 node ioredis-repro-compact.cjs
const net = require('node:net');
const Redis = require('ioredis');
const PORT = Number(process.env.REDIS_PORT || 6379);
const sleep = (ms) => new Promise((r) => setTimeout(r, ms));
// A TCP relay in front of Redis: stopping it is what a client sees when Redis dies.
const sockets = new Set();
const relay = net.createServer((c) => {
const u = net.connect(PORT, '127.0.0.1');
for (const s of [c, u]) (sockets.add(s), s.on('error', () => {}));
c.pipe(u).pipe(c);
});
const listen = (port) => new Promise((r) => relay.listen(port, '127.0.0.1', r));
function watch(redis, pending) {
const seen = { end: false, cmd: 'pending' };
redis.once('end', () => (seen.end = true));
pending.then(() => (seen.cmd = 'resolved'), (e) => (seen.cmd = `rejected (${e.message})`));
return seen;
}
(async () => {
// Control: Redis up, a BLPOP in flight, then disconnect().
const a = new Redis({ port: PORT });
a.on('error', () => {});
await new Promise((r) => a.once('ready', r));
const ca = watch(a, a.blpop('never-pushed', 0));
await sleep(100);
a.disconnect();
await sleep(1000);
console.log(`control: status=${a.status} end=${ca.end} BLPOP ${ca.cmd}`);
// Redis drops; disconnect() while the client is reconnecting (retry timer armed, no socket).
await listen(0);
const port = relay.address().port;
const b = new Redis({ port });
b.on('error', () => {});
await new Promise((r) => b.once('ready', r));
const parked = new Promise((r) =>
b.once('reconnecting', () => {
const before = b.status;
const cb = watch(b, b.get('k')); // queued in the offline queue
b.disconnect();
r({ before, cb });
}),
);
for (const s of sockets) s.destroy();
relay.close();
const { before, cb } = await parked;
await sleep(10_000);
console.log(`outage: status=${b.status} (was ${before} at disconnect()) end=${cb.end} GET ${cb.cmd}, 10 s after disconnect()`);
await listen(port);
await sleep(10_000);
console.log(`outage: status=${b.status} end=${cb.end} GET ${cb.cmd}, 10 s after Redis came back`);
process.exit(0);
})();Actual. The output is the same on Node and Bun, on Windows and Linux. A real Expected is what the Cause ( I applied this PR's With the same patch, bullmq's |
|
@Thyregud @UnexpectedLobster Thanks for the detailed reproductions. Since #2169 has stalled, could one of you open a focused replacement PR for disconnect() while reconnecting, including cleanup of pending commands retained across a failed reconnect? |
This branch has not been deployed
4 participants
Summary
With commandTimeout set, a per-command timer armed by sendCommand() is cleared only when the command settles. If disconnect() runs while the client is still connecting or waiting to reconnect, no stream event ever fires, so queued commands keep live timers that later reject with "Command timed out" even though they were never sent. disconnect() now reuses its existing closeHandler cleanup in those two states, rejecting stranded commands with "Connection is closed." and clearing their timers, as already done for the wait status and stream close. Fixes #1535. Related but independent from #2131.
Testing
Checklist
Note
Medium Risk
Touches core connection teardown and reconnect scheduling; changes are narrow but incorrect handling could leave clients reconnecting after manual close or mishandle queued commands.
Overview
Fixes #1535: with
commandTimeoutset, commands queued while the client is still connecting or reconnecting could keep armed timers afterdisconnect()because no streamcloseevent ran cleanup. Those timers later rejected with "Command timed out" instead of "Connection is closed." and could keep the event loop alive.disconnect()now invokescloseHandlerwhen there is no live stream to flush queues (no stream yet, destroyed stream during retry, orreconnecting), matching behavior already used forwait.Connect/close race hardening: the connector callback captures
connectionEpochand ignores stale results after disconnect; late streams are destroyed.connectHandlerbails unless status isconnecting;closeHandlerbails if status is alreadyendso intentional shutdown does not double-emitendor schedule reconnect. Failed connect paths only callsetStatus("end")when not already ended.Regression coverage added in
test/functional/commandTimeout.ts(hanging connector, reconnecting disconnect, delayed connect, refused port + singleend, disconnect vs stream destroy).Reviewed by Cursor Bugbot for commit e3636a9. Bugbot is set up for automated code reviews on this repo. Configure here.