From e4aaa237d6372b87c95962f7f9f96e975d043be4 Mon Sep 17 00:00:00 2001 From: askalf <263217947+askalf@users.noreply.github.com> Date: Mon, 14 Sep 2026 13:14:45 +0000 Subject: [PATCH 1/6] fix: handle rejection from the db-restoring select on reconnectOnError `handleReconnection` re-issues SELECT to restore the command's db before resending it when `reconnectOnError` returns 2, but that call is fire-and-forget. If it fails - the ACL changed, the db index is out of range, or the socket is not writeable and the offline queue is disabled - it becomes an unhandled rejection, which terminates the process on Node 15+ even when an error listener is registered on the client. Route it through silentEmit("error", err), matching the seven sibling call sites in readyHandler and the "select" handshake command in getHandshakeCommands. --- lib/Redis.ts | 4 +- test/unit/reconnectOnError.ts | 134 ++++++++++++++++++++++++++++++++++ 2 files changed, 137 insertions(+), 1 deletion(-) create mode 100644 test/unit/reconnectOnError.ts diff --git a/lib/Redis.ts b/lib/Redis.ts index 4f5dbe8e7..e4e00ac18 100644 --- a/lib/Redis.ts +++ b/lib/Redis.ts @@ -853,7 +853,9 @@ class Redis this.condition?.select !== item.select && item.command.name !== "select" ) { - this.select(item.select); + this.select(item.select).catch((err) => + this.silentEmit("error", err) + ); } // TODO // @ts-expect-error diff --git a/test/unit/reconnectOnError.ts b/test/unit/reconnectOnError.ts new file mode 100644 index 000000000..b3d5f7bf2 --- /dev/null +++ b/test/unit/reconnectOnError.ts @@ -0,0 +1,134 @@ +import { expect } from "chai"; +import MockServer from "../helpers/mock_server"; +import Redis from "../../lib/Redis"; + +const READONLY_ERROR = "READONLY You can't write against a read only replica."; +const INVALID_DB_INDEX = "ERR DB index is out of range"; + +// The restoring `SELECT` is protocol-independent, but the connection setup +// around it is not: under RESP3 the client sends HELLO before anything else. +// Both are covered, with RESP3 first since it is the default in v6. +const PROTOCOLS = [3, 2] as const; + +PROTOCOLS.forEach((protocol, index) => { + describe(`reconnectOnError db restoration (RESP${protocol})`, () => { + const basePort = 17930 + index * 10; + let savedListeners: any[] = []; + let unhandled: string[] = []; + + beforeEach(() => { + unhandled = []; + // test/helpers/global.ts installs an unhandledRejection listener that + // throws, which would turn a rejection here into an uncaught exception + // attributed to whichever test happens to be running. Swap it out for a + // recorder and restore it afterwards. + savedListeners = process.listeners("unhandledRejection"); + process.removeAllListeners("unhandledRejection"); + process.on("unhandledRejection", (reason) => { + unhandled.push(String(reason)); + }); + }); + + afterEach(() => { + process.removeAllListeners("unhandledRejection"); + for (const listener of savedListeners) { + process.on("unhandledRejection", listener); + } + }); + + // Fails `get` on the first connection only, so reconnectOnError triggers + // once and the resent command succeeds instead of looping. `select` fails + // from the reconnect onwards, which is what an ACL change or an + // out-of-range db index looks like to the reconnecting client. + function serverRejectingSelect(port: number) { + let connections = 0; + const server = new MockServer(port, (argv) => { + const name = String(argv[0]).toLowerCase(); + if (name === "info") { + return "# Server\r\nredis_version:7.0.0\r\n"; + } + if (name === "get" && connections < 2) { + return new Error(READONLY_ERROR); + } + if (name === "select" && connections >= 2) { + return new Error(INVALID_DB_INDEX); + } + return "OK"; + }); + server.on("connect", () => connections++); + return server; + } + + function client(port: number, enableOfflineQueue: boolean) { + return new Redis({ + port, + protocol, + lazyConnect: true, + enableOfflineQueue, + retryStrategy: () => 40, + // 2 means "reconnect, then resend the failed command", which is the + // branch that re-issues SELECT to restore the command's db. Keyed on + // the READONLY reply like the README's ElastiCache sample, so the + // restoring SELECT's own failure does not re-enter this branch. + reconnectOnError: (err: Error) => + err.message.startsWith("READONLY") ? 2 : false, + }); + } + + // Makes `condition.select` diverge from the in-flight command's `select`, + // which is the guard handleReconnection uses to decide whether the db has + // to be restored before the command is resent. + async function failCommandAfterSwitchingDb(redis: any) { + const errors: string[] = []; + redis.on("error", (err: Error) => errors.push(err.message)); + + const failing = redis.get("foo").catch(() => {}); + // Sent before the `get` reply arrives, so `condition.select` is already 2 + // by the time reconnectOnError sees the READONLY error for a command + // that was issued against db 0. + const switching = redis.select(2).catch(() => {}); + + await Promise.all([failing, switching]); + await new Promise((resolve) => setTimeout(resolve, 500)); + return errors; + } + + it("surfaces a failing db-restoring SELECT as an error event", async () => { + const server = serverRejectingSelect(basePort); + const redis = client(basePort, true); + await redis.connect(); + + const errors = await failCommandAfterSwitchingDb(redis); + + redis.disconnect(); + await server.disconnectPromise(); + + expect(unhandled, "must not surface as an unhandled rejection").to.eql( + [] + ); + expect(errors, "the client's error listener must receive it").to.include( + INVALID_DB_INDEX + ); + }); + + it("surfaces an unwritable db-restoring SELECT as an error event when the offline queue is disabled", async () => { + // Without the offline queue the restoring SELECT cannot be buffered, so + // it is rejected inline by sendCommand instead of by the server. + const server = serverRejectingSelect(basePort + 1); + const redis = client(basePort + 1, false); + await redis.connect(); + + const errors = await failCommandAfterSwitchingDb(redis); + + redis.disconnect(); + await server.disconnectPromise(); + + expect(unhandled, "must not surface as an unhandled rejection").to.eql( + [] + ); + expect(errors, "the client's error listener must receive it").to.include( + "Stream isn't writeable and enableOfflineQueue options is false" + ); + }); + }); +}); From 96cf3e5575a10fc173fb63132eec92045940bd8a Mon Sep 17 00:00:00 2001 From: askalf <263217947+askalf@users.noreply.github.com> Date: Mon, 14 Sep 2026 15:14:15 +0000 Subject: [PATCH 2/6] test: cover autopipelining and the select-command guard on reconnect Adds two cases to the reconnectOnError db restoration suite: the restoring SELECT under enableAutoPipelining, where the autopipeline must flush the failing command before the db is switched for the guard to be reached at all, and a control for the item.command.name !== "select" branch, which keeps the restoring call from firing when the failed command is itself a SELECT. --- test/unit/reconnectOnError.ts | 195 +++++++++++++++++++++++++++++++++- 1 file changed, 194 insertions(+), 1 deletion(-) diff --git a/test/unit/reconnectOnError.ts b/test/unit/reconnectOnError.ts index b3d5f7bf2..ed40b19b8 100644 --- a/test/unit/reconnectOnError.ts +++ b/test/unit/reconnectOnError.ts @@ -59,7 +59,56 @@ PROTOCOLS.forEach((protocol, index) => { return server; } - function client(port: number, enableOfflineQueue: boolean) { + // As above, but only the named db is rejected. Lets a test drive the + // restoring `SELECT` to a non-zero db without the reconnect's own + // handshake `SELECT` failing for the same reason, which would satisfy the + // assertions on both arms and prove nothing. + function serverRejectingDb(port: number, db: number) { + let connections = 0; + const server = new MockServer(port, (argv) => { + const name = String(argv[0]).toLowerCase(); + if (name === "info") { + return "# Server\r\nredis_version:7.0.0\r\n"; + } + if (name === "get" && connections < 2) { + return new Error(READONLY_ERROR); + } + if ( + name === "select" && + connections >= 2 && + String(argv[1]) === String(db) + ) { + return new Error(INVALID_DB_INDEX); + } + return "OK"; + }); + server.on("connect", () => connections++); + return server; + } + + // Never rejects `select`, so the db restoration succeeds. Used to check + // that the added `.catch` does not disturb the path it guards. + function serverAcceptingSelect(port: number) { + let connections = 0; + const server = new MockServer(port, (argv) => { + const name = String(argv[0]).toLowerCase(); + if (name === "info") { + return "# Server\r\nredis_version:7.0.0\r\n"; + } + if (name === "get" && connections < 2) { + return new Error(READONLY_ERROR); + } + return "OK"; + }); + server.on("connect", () => connections++); + return server; + } + + function client( + port: number, + enableOfflineQueue: boolean, + extraOptions: Record = {} + ) { return new Redis({ port, protocol, @@ -72,6 +121,7 @@ PROTOCOLS.forEach((protocol, index) => { // restoring SELECT's own failure does not re-enter this branch. reconnectOnError: (err: Error) => err.message.startsWith("READONLY") ? 2 : false, + ...extraOptions, }); } @@ -130,5 +180,148 @@ PROTOCOLS.forEach((protocol, index) => { "Stream isn't writeable and enableOfflineQueue options is false" ); }); + + it("surfaces a failing db-restoring SELECT when no error listener is registered", async () => { + // silentEmit falls back to logging when nothing is listening, so the + // rejection must still be consumed. This is the configuration that made + // the unguarded call fatal: a client with no `error` listener has + // nothing that could have handled the promise either. + const server = serverRejectingSelect(basePort + 2); + const redis = client(basePort + 2, true); + await redis.connect(); + + const failing = redis.get("foo").catch(() => {}); + const switching = redis.select(2).catch(() => {}); + await Promise.all([failing, switching]); + await new Promise((resolve) => setTimeout(resolve, 500)); + + redis.disconnect(); + await server.disconnectPromise(); + + expect(unhandled, "must not surface as an unhandled rejection").to.eql( + [] + ); + }); + + it("surfaces a failing db-restoring SELECT for a non-zero db", async () => { + // The db being restored is the command's own `select`, not the client's + // configured `db`. Restoring a non-zero db exercises a different value + // than the other cases, which all restore db 0. + const server = serverRejectingDb(basePort + 3, 5); + const redis = client(basePort + 3, true); + await redis.connect(); + + const errors: string[] = []; + redis.on("error", (err: Error) => errors.push(err.message)); + + await redis.select(5); + const failing = redis.get("foo").catch(() => {}); + const switching = redis.select(2).catch(() => {}); + await Promise.all([failing, switching]); + await new Promise((resolve) => setTimeout(resolve, 500)); + + redis.disconnect(); + await server.disconnectPromise(); + + expect(unhandled, "must not surface as an unhandled rejection").to.eql( + [] + ); + // The emitted error is the restoring SELECT's own, not the READONLY + // reply that triggered the reconnection: that one is not rejected here, + // its command is resent. + expect( + errors[0], + "the first error must be the restoring SELECT's own" + ).to.eql(INVALID_DB_INDEX); + }); + + it("surfaces a failing db-restoring SELECT when auto pipelining is enabled", async () => { + // `select` is one of notAllowedAutoPipelineCommands, so the restoring + // call goes through sendCommand and yields a Command promise even with + // enableAutoPipelining on -- which is what makes `.catch` available on + // it. The autopipeline must be allowed to flush `get` before `select(2)` + // is issued, otherwise both are batched into one tick, the in-flight + // command's db never diverges from `condition.select`, and the restoring + // call is not reached at all. + const server = serverRejectingSelect(basePort + 5); + const redis = client(basePort + 5, true, { + enableAutoPipelining: true, + }); + await redis.connect(); + + const errors: string[] = []; + redis.on("error", (err: Error) => errors.push(err.message)); + + const failing = redis.get("foo").catch(() => {}); + await new Promise((resolve) => setImmediate(resolve)); + const switching = redis.select(2).catch(() => {}); + + await Promise.all([failing, switching]); + await new Promise((resolve) => setTimeout(resolve, 500)); + + redis.disconnect(); + await server.disconnectPromise(); + + expect(unhandled, "must not surface as an unhandled rejection").to.eql( + [] + ); + expect(errors, "the client's error listener must receive it").to.include( + INVALID_DB_INDEX + ); + }); + + it("does not restore the db when the failed command is itself a SELECT", async () => { + // Control: green with and without the fix. `item.command.name !== + // "select"` keeps the restoring call from firing at all here, so the + // guarded line is never reached and the resent SELECT settles normally. + const server = serverAcceptingSelect(basePort + 6); + const redis = client(basePort + 6, true); + await redis.connect(); + + const errors: string[] = []; + redis.on("error", (err: Error) => errors.push(err.message)); + + const selects: number[] = []; + redis.on("select", (db: number) => selects.push(db)); + + const result = await redis.select(3); + await new Promise((resolve) => setTimeout(resolve, 500)); + + redis.disconnect(); + await server.disconnectPromise(); + + expect(unhandled, "must not surface as an unhandled rejection").to.eql( + [] + ); + expect(errors, "a plain SELECT must not emit an error").to.eql([]); + expect(result, "the SELECT must settle").to.eql("OK"); + expect(selects, "the db must be switched exactly once").to.eql([3]); + }); + + it("resends the command after a successful db restoration", async () => { + // Control: green with and without the fix. Pins that guarding the + // restoring SELECT does not disturb the path it guards -- no spurious + // error event, and the resent command still settles. + const server = serverAcceptingSelect(basePort + 4); + const redis = client(basePort + 4, true); + await redis.connect(); + + const errors: string[] = []; + redis.on("error", (err: Error) => errors.push(err.message)); + + const failing = redis.get("foo"); + const switching = redis.select(2).catch(() => {}); + const [result] = await Promise.all([failing, switching]); + await new Promise((resolve) => setTimeout(resolve, 500)); + + redis.disconnect(); + await server.disconnectPromise(); + + expect(unhandled, "must not surface as an unhandled rejection").to.eql( + [] + ); + expect(errors, "a successful SELECT must not emit an error").to.eql([]); + expect(result, "the resent command must settle").to.eql("OK"); + }); }); }); From d7b9d3a9296ca38a397ad409e69de4a8dc81b30d Mon Sep 17 00:00:00 2001 From: askalf <263217947+askalf@users.noreply.github.com> Date: Mon, 14 Sep 2026 17:10:03 +0000 Subject: [PATCH 3/6] test: drop the non-discriminating select-command case The "failed command is itself a SELECT" case never entered the reconnectOnError branch at all: a plain SELECT that succeeds is not rejected, so handleReconnection is never called for it and none of its assertions could distinguish the guarded call from the bare one. Remove it rather than keep a case that pins nothing. Keep the successful-restoration case, which does execute the guarded call, and make that explicit: name it a control and assert on the "select" events so the restoring SELECT it is controlling for is observed rather than assumed. --- test/unit/reconnectOnError.ts | 47 ++++++++++++----------------------- 1 file changed, 16 insertions(+), 31 deletions(-) diff --git a/test/unit/reconnectOnError.ts b/test/unit/reconnectOnError.ts index ed40b19b8..53d7a09db 100644 --- a/test/unit/reconnectOnError.ts +++ b/test/unit/reconnectOnError.ts @@ -270,45 +270,26 @@ PROTOCOLS.forEach((protocol, index) => { ); }); - it("does not restore the db when the failed command is itself a SELECT", async () => { - // Control: green with and without the fix. `item.command.name !== - // "select"` keeps the restoring call from firing at all here, so the - // guarded line is never reached and the resent SELECT settles normally. - const server = serverAcceptingSelect(basePort + 6); - const redis = client(basePort + 6, true); + it("resends the command after a successful db restoration (control)", async () => { + // Green on both arms by design: it reaches the guarded call and comes + // out the same, which is what makes it a control rather than a + // regression case. It pins that routing the restoring SELECT through + // `.catch` does not disturb the path it guards -- no spurious error + // event, and the resent command still settles. + const server = serverAcceptingSelect(basePort + 4); + const redis = client(basePort + 4, true); await redis.connect(); const errors: string[] = []; redis.on("error", (err: Error) => errors.push(err.message)); + // `select` is emitted whenever a SELECT actually changes the connection's + // db, so this records the restoration itself rather than assuming it. A + // control that never executed the guarded line would prove nothing, and + // the trailing 0 is the restoring SELECT the guard wraps. const selects: number[] = []; redis.on("select", (db: number) => selects.push(db)); - const result = await redis.select(3); - await new Promise((resolve) => setTimeout(resolve, 500)); - - redis.disconnect(); - await server.disconnectPromise(); - - expect(unhandled, "must not surface as an unhandled rejection").to.eql( - [] - ); - expect(errors, "a plain SELECT must not emit an error").to.eql([]); - expect(result, "the SELECT must settle").to.eql("OK"); - expect(selects, "the db must be switched exactly once").to.eql([3]); - }); - - it("resends the command after a successful db restoration", async () => { - // Control: green with and without the fix. Pins that guarding the - // restoring SELECT does not disturb the path it guards -- no spurious - // error event, and the resent command still settles. - const server = serverAcceptingSelect(basePort + 4); - const redis = client(basePort + 4, true); - await redis.connect(); - - const errors: string[] = []; - redis.on("error", (err: Error) => errors.push(err.message)); - const failing = redis.get("foo"); const switching = redis.select(2).catch(() => {}); const [result] = await Promise.all([failing, switching]); @@ -317,6 +298,10 @@ PROTOCOLS.forEach((protocol, index) => { redis.disconnect(); await server.disconnectPromise(); + expect( + selects, + "the guarded restoring SELECT must have run, back to db 0" + ).to.eql([2, 0]); expect(unhandled, "must not surface as an unhandled rejection").to.eql( [] ); From f5002bc369193338d1568f9cfbb6f6f47b353252 Mon Sep 17 00:00:00 2001 From: askalf <263217947+askalf@users.noreply.github.com> Date: Mon, 14 Sep 2026 19:13:49 +0000 Subject: [PATCH 4/6] test: cover several commands dropped by one reconnect --- test/unit/reconnectOnError.ts | 43 +++++++++++++++++++++++++++++++++++ 1 file changed, 43 insertions(+) diff --git a/test/unit/reconnectOnError.ts b/test/unit/reconnectOnError.ts index 53d7a09db..47dcedc4c 100644 --- a/test/unit/reconnectOnError.ts +++ b/test/unit/reconnectOnError.ts @@ -270,6 +270,49 @@ PROTOCOLS.forEach((protocol, index) => { ); }); + it("issues one restoring SELECT for several commands dropped together", async () => { + // handleReconnection runs once per dropped command, so three in flight + // walk the guarded line three times. Only the first one restores: the + // `SELECT` updates `condition.select` synchronously in sendCommand + // (lib/Redis.ts:655-662), so the `this.condition?.select !== item.select` + // guard is already false for the other two. That is worth pinning -- + // it is what keeps the failure to a single error event rather than one + // per command -- and all three commands must still be resent. + const server = serverRejectingSelect(basePort + 6); + const redis = client(basePort + 6, true); + await redis.connect(); + + const errors: string[] = []; + redis.on("error", (err: Error) => errors.push(err.message)); + + const results = [ + redis.get("foo").catch((err: Error) => err.message), + redis.get("bar").catch((err: Error) => err.message), + redis.get("baz").catch((err: Error) => err.message), + ]; + const switching = redis.select(2).catch(() => {}); + + const settled = await Promise.all(results); + await new Promise((resolve) => setTimeout(resolve, 500)); + await switching; + + redis.disconnect(); + await server.disconnectPromise(); + + expect(unhandled, "must not surface as an unhandled rejection").to.eql( + [] + ); + expect( + errors.filter((message) => message === INVALID_DB_INDEX).length, + "the guard disarms after the first restore, so exactly one error" + ).to.eql(1); + expect(settled, "all three dropped commands must be resent").to.eql([ + "OK", + "OK", + "OK", + ]); + }); + it("resends the command after a successful db restoration (control)", async () => { // Green on both arms by design: it reaches the guarded call and comes // out the same, which is what makes it a control rather than a From f2ef94fa1f5cca5e8a0a07319385e417a6850e7a Mon Sep 17 00:00:00 2001 From: askalf <263217947+askalf@users.noreply.github.com> Date: Tue, 15 Sep 2026 00:38:47 +0000 Subject: [PATCH 5/6] test: cover the restoring SELECT abandoned when reconnecting is given up --- test/unit/reconnectOnError.ts | 35 +++++++++++++++++++++++++++++++++++ 1 file changed, 35 insertions(+) diff --git a/test/unit/reconnectOnError.ts b/test/unit/reconnectOnError.ts index 47dcedc4c..2b44deac8 100644 --- a/test/unit/reconnectOnError.ts +++ b/test/unit/reconnectOnError.ts @@ -312,6 +312,41 @@ PROTOCOLS.forEach((protocol, index) => { "OK", ]); }); + it("surfaces a db-restoring SELECT abandoned when the client gives up reconnecting", async () => { + // The restoring SELECT does not have to be rejected by the server: with + // the offline queue on it is buffered while the client reconnects, and if + // `retryStrategy` declines to retry, closeHandler's close() sets the + // status to "end" and flushes the queues with Connection is closed + // (lib/redis/event_handler.ts:427-429). That rejection reaches the same + // guarded promise, so it is the second way the unguarded call crashed. + // `silentEmit` deliberately swallows it -- the client is ending and the + // user asked for that -- so this pins the no-unhandled-rejection half + // only, which is the half that was fatal. + const server = serverAcceptingSelect(basePort + 7); + const redis = client(basePort + 7, true, { + retryStrategy: () => null, + }); + await redis.connect(); + + const errors: string[] = []; + redis.on("error", (err: Error) => errors.push(err.message)); + + const failing = redis.get("foo").catch((err: Error) => err.message); + const switching = redis.select(2).catch(() => {}); + + const failed = await failing; + await new Promise((resolve) => setTimeout(resolve, 300)); + await switching; + await server.disconnectPromise(); + + expect(unhandled, "must not surface as an unhandled rejection").to.eql( + [] + ); + expect( + failed, + "the dropped command is rejected by the queue flush, not resent" + ).to.eql("Connection is closed."); + }); it("resends the command after a successful db restoration (control)", async () => { // Green on both arms by design: it reaches the guarded call and comes From 652b8f051c0713c04b346461066eac9f5c58f220 Mon Sep 17 00:00:00 2001 From: askalf <263217947+askalf@users.noreply.github.com> Date: Mon, 5 Oct 2026 15:13:38 -0400 Subject: [PATCH 6/6] test: keep the one case the fix needs The restoring SELECT rejected by the server reaches the error listener instead of escaping as an unhandled rejection, under both protocols. The other cases were checked during verification and are not committed. --- test/unit/reconnectOnError.ts | 335 ++-------------------------------- 1 file changed, 14 insertions(+), 321 deletions(-) diff --git a/test/unit/reconnectOnError.ts b/test/unit/reconnectOnError.ts index 2b44deac8..1de3bf787 100644 --- a/test/unit/reconnectOnError.ts +++ b/test/unit/reconnectOnError.ts @@ -7,21 +7,18 @@ const INVALID_DB_INDEX = "ERR DB index is out of range"; // The restoring `SELECT` is protocol-independent, but the connection setup // around it is not: under RESP3 the client sends HELLO before anything else. -// Both are covered, with RESP3 first since it is the default in v6. const PROTOCOLS = [3, 2] as const; PROTOCOLS.forEach((protocol, index) => { describe(`reconnectOnError db restoration (RESP${protocol})`, () => { - const basePort = 17930 + index * 10; + const port = 17930 + index; let savedListeners: any[] = []; let unhandled: string[] = []; beforeEach(() => { unhandled = []; // test/helpers/global.ts installs an unhandledRejection listener that - // throws, which would turn a rejection here into an uncaught exception - // attributed to whichever test happens to be running. Swap it out for a - // recorder and restore it afterwards. + // throws; record rejections here instead and restore it afterwards. savedListeners = process.listeners("unhandledRejection"); process.removeAllListeners("unhandledRejection"); process.on("unhandledRejection", (reason) => { @@ -36,11 +33,9 @@ PROTOCOLS.forEach((protocol, index) => { } }); - // Fails `get` on the first connection only, so reconnectOnError triggers - // once and the resent command succeeds instead of looping. `select` fails - // from the reconnect onwards, which is what an ACL change or an - // out-of-range db index looks like to the reconnecting client. - function serverRejectingSelect(port: number) { + it("surfaces a failing db-restoring SELECT as an error event", async () => { + // `get` fails on the first connection only, so reconnectOnError fires + // once; `select` fails from the reconnect onwards. let connections = 0; const server = new MockServer(port, (argv) => { const name = String(argv[0]).toLowerCase(); @@ -56,335 +51,33 @@ PROTOCOLS.forEach((protocol, index) => { return "OK"; }); server.on("connect", () => connections++); - return server; - } - - // As above, but only the named db is rejected. Lets a test drive the - // restoring `SELECT` to a non-zero db without the reconnect's own - // handshake `SELECT` failing for the same reason, which would satisfy the - // assertions on both arms and prove nothing. - function serverRejectingDb(port: number, db: number) { - let connections = 0; - const server = new MockServer(port, (argv) => { - const name = String(argv[0]).toLowerCase(); - if (name === "info") { - return "# Server\r\nredis_version:7.0.0\r\n"; - } - if (name === "get" && connections < 2) { - return new Error(READONLY_ERROR); - } - if ( - name === "select" && - connections >= 2 && - String(argv[1]) === String(db) - ) { - return new Error(INVALID_DB_INDEX); - } - return "OK"; - }); - server.on("connect", () => connections++); - return server; - } - - // Never rejects `select`, so the db restoration succeeds. Used to check - // that the added `.catch` does not disturb the path it guards. - function serverAcceptingSelect(port: number) { - let connections = 0; - const server = new MockServer(port, (argv) => { - const name = String(argv[0]).toLowerCase(); - if (name === "info") { - return "# Server\r\nredis_version:7.0.0\r\n"; - } - if (name === "get" && connections < 2) { - return new Error(READONLY_ERROR); - } - return "OK"; - }); - server.on("connect", () => connections++); - return server; - } - function client( - port: number, - enableOfflineQueue: boolean, - extraOptions: Record = {} - ) { - return new Redis({ + const redis = new Redis({ port, protocol, lazyConnect: true, - enableOfflineQueue, retryStrategy: () => 40, - // 2 means "reconnect, then resend the failed command", which is the - // branch that re-issues SELECT to restore the command's db. Keyed on - // the READONLY reply like the README's ElastiCache sample, so the - // restoring SELECT's own failure does not re-enter this branch. + // 2 = reconnect and resend, the branch that restores the command's db. reconnectOnError: (err: Error) => err.message.startsWith("READONLY") ? 2 : false, - ...extraOptions, }); - } - - // Makes `condition.select` diverge from the in-flight command's `select`, - // which is the guard handleReconnection uses to decide whether the db has - // to be restored before the command is resent. - async function failCommandAfterSwitchingDb(redis: any) { const errors: string[] = []; redis.on("error", (err: Error) => errors.push(err.message)); - - const failing = redis.get("foo").catch(() => {}); - // Sent before the `get` reply arrives, so `condition.select` is already 2 - // by the time reconnectOnError sees the READONLY error for a command - // that was issued against db 0. - const switching = redis.select(2).catch(() => {}); - - await Promise.all([failing, switching]); - await new Promise((resolve) => setTimeout(resolve, 500)); - return errors; - } - - it("surfaces a failing db-restoring SELECT as an error event", async () => { - const server = serverRejectingSelect(basePort); - const redis = client(basePort, true); - await redis.connect(); - - const errors = await failCommandAfterSwitchingDb(redis); - - redis.disconnect(); - await server.disconnectPromise(); - - expect(unhandled, "must not surface as an unhandled rejection").to.eql( - [] - ); - expect(errors, "the client's error listener must receive it").to.include( - INVALID_DB_INDEX - ); - }); - - it("surfaces an unwritable db-restoring SELECT as an error event when the offline queue is disabled", async () => { - // Without the offline queue the restoring SELECT cannot be buffered, so - // it is rejected inline by sendCommand instead of by the server. - const server = serverRejectingSelect(basePort + 1); - const redis = client(basePort + 1, false); await redis.connect(); - const errors = await failCommandAfterSwitchingDb(redis); - - redis.disconnect(); - await server.disconnectPromise(); - - expect(unhandled, "must not surface as an unhandled rejection").to.eql( - [] - ); - expect(errors, "the client's error listener must receive it").to.include( - "Stream isn't writeable and enableOfflineQueue options is false" - ); - }); - - it("surfaces a failing db-restoring SELECT when no error listener is registered", async () => { - // silentEmit falls back to logging when nothing is listening, so the - // rejection must still be consumed. This is the configuration that made - // the unguarded call fatal: a client with no `error` listener has - // nothing that could have handled the promise either. - const server = serverRejectingSelect(basePort + 2); - const redis = client(basePort + 2, true); - await redis.connect(); - - const failing = redis.get("foo").catch(() => {}); - const switching = redis.select(2).catch(() => {}); - await Promise.all([failing, switching]); - await new Promise((resolve) => setTimeout(resolve, 500)); - - redis.disconnect(); - await server.disconnectPromise(); - - expect(unhandled, "must not surface as an unhandled rejection").to.eql( - [] - ); - }); - - it("surfaces a failing db-restoring SELECT for a non-zero db", async () => { - // The db being restored is the command's own `select`, not the client's - // configured `db`. Restoring a non-zero db exercises a different value - // than the other cases, which all restore db 0. - const server = serverRejectingDb(basePort + 3, 5); - const redis = client(basePort + 3, true); - await redis.connect(); - - const errors: string[] = []; - redis.on("error", (err: Error) => errors.push(err.message)); - - await redis.select(5); - const failing = redis.get("foo").catch(() => {}); - const switching = redis.select(2).catch(() => {}); - await Promise.all([failing, switching]); - await new Promise((resolve) => setTimeout(resolve, 500)); - - redis.disconnect(); - await server.disconnectPromise(); - - expect(unhandled, "must not surface as an unhandled rejection").to.eql( - [] - ); - // The emitted error is the restoring SELECT's own, not the READONLY - // reply that triggered the reconnection: that one is not rejected here, - // its command is resent. - expect( - errors[0], - "the first error must be the restoring SELECT's own" - ).to.eql(INVALID_DB_INDEX); - }); - - it("surfaces a failing db-restoring SELECT when auto pipelining is enabled", async () => { - // `select` is one of notAllowedAutoPipelineCommands, so the restoring - // call goes through sendCommand and yields a Command promise even with - // enableAutoPipelining on -- which is what makes `.catch` available on - // it. The autopipeline must be allowed to flush `get` before `select(2)` - // is issued, otherwise both are batched into one tick, the in-flight - // command's db never diverges from `condition.select`, and the restoring - // call is not reached at all. - const server = serverRejectingSelect(basePort + 5); - const redis = client(basePort + 5, true, { - enableAutoPipelining: true, - }); - await redis.connect(); - - const errors: string[] = []; - redis.on("error", (err: Error) => errors.push(err.message)); - - const failing = redis.get("foo").catch(() => {}); - await new Promise((resolve) => setImmediate(resolve)); - const switching = redis.select(2).catch(() => {}); - - await Promise.all([failing, switching]); - await new Promise((resolve) => setTimeout(resolve, 500)); - - redis.disconnect(); - await server.disconnectPromise(); - - expect(unhandled, "must not surface as an unhandled rejection").to.eql( - [] - ); - expect(errors, "the client's error listener must receive it").to.include( - INVALID_DB_INDEX - ); - }); - - it("issues one restoring SELECT for several commands dropped together", async () => { - // handleReconnection runs once per dropped command, so three in flight - // walk the guarded line three times. Only the first one restores: the - // `SELECT` updates `condition.select` synchronously in sendCommand - // (lib/Redis.ts:655-662), so the `this.condition?.select !== item.select` - // guard is already false for the other two. That is worth pinning -- - // it is what keeps the failure to a single error event rather than one - // per command -- and all three commands must still be resent. - const server = serverRejectingSelect(basePort + 6); - const redis = client(basePort + 6, true); - await redis.connect(); - - const errors: string[] = []; - redis.on("error", (err: Error) => errors.push(err.message)); - - const results = [ - redis.get("foo").catch((err: Error) => err.message), - redis.get("bar").catch((err: Error) => err.message), - redis.get("baz").catch((err: Error) => err.message), - ]; - const switching = redis.select(2).catch(() => {}); - - const settled = await Promise.all(results); - await new Promise((resolve) => setTimeout(resolve, 500)); - await switching; - - redis.disconnect(); - await server.disconnectPromise(); - - expect(unhandled, "must not surface as an unhandled rejection").to.eql( - [] - ); - expect( - errors.filter((message) => message === INVALID_DB_INDEX).length, - "the guard disarms after the first restore, so exactly one error" - ).to.eql(1); - expect(settled, "all three dropped commands must be resent").to.eql([ - "OK", - "OK", - "OK", + // `select(2)` is sent before the `get` reply arrives, so the failed + // `get` (issued against db 0) needs its db restored before the resend. + await Promise.all([ + redis.get("foo").catch(() => {}), + redis.select(2).catch(() => {}), ]); - }); - it("surfaces a db-restoring SELECT abandoned when the client gives up reconnecting", async () => { - // The restoring SELECT does not have to be rejected by the server: with - // the offline queue on it is buffered while the client reconnects, and if - // `retryStrategy` declines to retry, closeHandler's close() sets the - // status to "end" and flushes the queues with Connection is closed - // (lib/redis/event_handler.ts:427-429). That rejection reaches the same - // guarded promise, so it is the second way the unguarded call crashed. - // `silentEmit` deliberately swallows it -- the client is ending and the - // user asked for that -- so this pins the no-unhandled-rejection half - // only, which is the half that was fatal. - const server = serverAcceptingSelect(basePort + 7); - const redis = client(basePort + 7, true, { - retryStrategy: () => null, - }); - await redis.connect(); - - const errors: string[] = []; - redis.on("error", (err: Error) => errors.push(err.message)); - - const failing = redis.get("foo").catch((err: Error) => err.message); - const switching = redis.select(2).catch(() => {}); - - const failed = await failing; - await new Promise((resolve) => setTimeout(resolve, 300)); - await switching; - await server.disconnectPromise(); - - expect(unhandled, "must not surface as an unhandled rejection").to.eql( - [] - ); - expect( - failed, - "the dropped command is rejected by the queue flush, not resent" - ).to.eql("Connection is closed."); - }); - - it("resends the command after a successful db restoration (control)", async () => { - // Green on both arms by design: it reaches the guarded call and comes - // out the same, which is what makes it a control rather than a - // regression case. It pins that routing the restoring SELECT through - // `.catch` does not disturb the path it guards -- no spurious error - // event, and the resent command still settles. - const server = serverAcceptingSelect(basePort + 4); - const redis = client(basePort + 4, true); - await redis.connect(); - - const errors: string[] = []; - redis.on("error", (err: Error) => errors.push(err.message)); - - // `select` is emitted whenever a SELECT actually changes the connection's - // db, so this records the restoration itself rather than assuming it. A - // control that never executed the guarded line would prove nothing, and - // the trailing 0 is the restoring SELECT the guard wraps. - const selects: number[] = []; - redis.on("select", (db: number) => selects.push(db)); - - const failing = redis.get("foo"); - const switching = redis.select(2).catch(() => {}); - const [result] = await Promise.all([failing, switching]); await new Promise((resolve) => setTimeout(resolve, 500)); redis.disconnect(); await server.disconnectPromise(); - expect( - selects, - "the guarded restoring SELECT must have run, back to db 0" - ).to.eql([2, 0]); - expect(unhandled, "must not surface as an unhandled rejection").to.eql( - [] - ); - expect(errors, "a successful SELECT must not emit an error").to.eql([]); - expect(result, "the resent command must settle").to.eql("OK"); + expect(unhandled).to.eql([]); + expect(errors).to.include(INVALID_DB_INDEX); }); }); });