Repository navigation
fix(client): keep the selected database when a pipelined command fails - #3484
Conversation
_executePipeline recorded the database from multi().select() only after every command in the batch had resolved. When a later command failed, Promise.all rejected and the record was skipped, although the server had already run the SELECT. The connection stayed on the new database, the client still held the old one, and the next reconnect's handshake went back to database 0 without an error. The database is now recorded when the SELECT's own reply arrives. A SELECT that fails, such as an index out of range, is still not recorded.
The previous commit recorded the database from multi().select() when the reply to the last SELECT in the batch arrived. When that last SELECT failed, as in select(1).select(99), nothing was recorded, although the server had run select(1) and stayed on database 1, so the next reconnect still went back to 0. Each SELECT now records the database from its own argument when its reply arrives, so the last SELECT the server ran is the one kept, and a SELECT that fails is never recorded. A pipelined SELECT added with addCommand() is recorded the same way. _executePipeline keeps its selectedDB parameter, no longer read, because cluster passes slotNumber after it.
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 bbf3472. Configure here.
nkaradzhov
left a comment
There was a problem hiding this comment.
Thanks for the fix. A failed command can leave #selectedDB out of sync after an earlier SELECT succeeds. Recording the database from each successful SELECT reply addresses that bug.
Before we merge this, please update the command-name check to handle lowercase and Buffer names in raw multi().addCommand() calls. Redis accepts those forms, but the current check misses them and can still restore the wrong database after reconnect. Please add a reconnect test for one of those forms.
The pipeline recorded a SELECT only when its command name was the string 'SELECT'. Redis also runs select and a Buffer name sent through multi().addCommand(), so the client kept its old database and the next reconnect went back to database 0. The name is now compared as String(args[0]).toUpperCase(), as the HIMPORT check in the same file already does.
|
Done in ffafe20: the pipeline now compares |

Description
A database selected with
multi().select(db)is lost on the next reconnect when the batch runs withexecAsPipeline()and one of its commands fails._executePipelinesets#selectedDBonly afterawait Promise.all(...), so a rejected command skips the assignment, though Redis already ran the SELECT and the connection sits on the new database. The handshake re-sends SELECT only while#selectedDB !== 0, so after a reconnect the client is back on database 0 with no error. The same batch run withexec()keeps its database. #3469 made the pool forwardselectedDBto this path, so pooled clients lose it the same way.#selectedDBis now set from each SELECT's own reply, with the database taken from that command's arguments, so the last SELECT Redis ran is the one recorded. A SELECT that fails is not recorded: afterselect(1).select(99)the client reconnects to database 1, where master goes back to 0. A raw SELECT added withaddCommand()in a pipeline is now recorded too, with its name in any case and as a string or aBuffer. TheselectedDBparameter is no longer read, and it stays in the signature because cluster passesslotNumberafter it.Redis 8.2.2,
multi().select(2).set('s', 'text').incr('s').execAsPipeline()andmulti().select(1).select(99).set('d', '1').execAsPipeline(), each followed byCLIENT KILLfrom a second client:The first new case, next to multi
should remember selected db, pipelinesselect(2), a failing INCR,select(1)and an out-of-range SELECT and fails on master with0 !== 1. A second case pipelinesaddCommand([Buffer.from('select'), '2'])and fails on master with0 !== 2. Both kill the connection withCLIENT KILL IDfrom a duplicate, as the PubSub and MONITOR cases do, because the QUIT-basedkillClientfails on a clean master on my Windows machine. The wholepackages/clientsuite, run with CI's test command including the cluster and sentinel specs, has no failure on this branch that master does not have: on my Windows machine 9 cases fail on both (the threekillClientreconnect cases, fourHIMPORTreconnect cases and twosocketTimeoutcases). After the Buffer change,lib/client/index.spec.tson Redis 8.2.2 fails the same four cases on this branch and on master.Checklist
npm testpass with this change (including linting)?