MULTI does not handle commands "runtime" errors

[LiraNuna]

Description

When using XAUTOCLAIM on a non-existant key, a TypeError is raised because the result is incorrectly mapped.

To reproduce:

const redis = require("redis");

async function reproduceBug() {
    const redisClient = redis.createClient();
    await redisClient.connect();

    return await redisClient.multi()
        .XAUTOCLAIM('does-not-exist', 'group', 'test', 15000, '0')
        .exec();
}

reproduceBug();

I believe the issue stems from transformReply not handling the exception when it comes through, but it could also be a problem with handleExecReplies not detecting the error.

Node.js Version

Any

Redis Server Version

Any

Node Redis Version

4.6.11

Platform

Any

Logs

node bug.js
node_modules/@redis/client/dist/lib/commands/generic-transformers.js:87
    return reply.map(transformStreamMessageNullReply);
                 ^

TypeError: Cannot read properties of undefined (reading 'map')
    at transformStreamMessagesNullReply (node_modules/@redis/client/dist/lib/commands/generic-transformers.js:87:18)
    at transformReply (node_modules/@redis/client/dist/lib/commands/XAUTOCLAIM.js:17:79)
    at node_modules/@redis/client/dist/lib/multi-command.js:67:37
    at Array.map (<anonymous>)
    at RedisMultiCommand.transformReplies (node_modules/@redis/client/dist/lib/multi-command.js:65:27)
    at RedisMultiCommand.handleExecReplies (node_modules/@redis/client/dist/lib/multi-command.js:62:21)
    at Commander.exec (node_modules/@redis/client/dist/lib/client/multi-command.js:81:82)
    at processTicksAndRejections (node:internal/process/task_queues:96:5)
    at async reproduceBug (bug.js:7:12)

[LiraNuna]

I think a possible fix for this would be this modification to transformReplies :

    transformReplies(rawReplies: Array<RedisCommandRawReply>): Array<RedisCommandRawReply> {
        return rawReplies.map((reply, i) => {
+           if (reply instanceof Error) {
+               throw reply;
+           }
+
            const { transformReply, args } = this.queue[i];
            return transformReply ? transformReply(reply, args.preserve) : reply;
        });
    }

I'm not very familiar with the architecture of the library to determine the best possible solution. Please note that this only occurs when using MULTI.

[leibale]

It seems like this is an issue with all commands that throws an error in "runtime", not something specific to XAUTOCLAIM:

// same bug...
await client.multi()
  .set('key', 'value')
  .hGetAll('key')
  .exec();

How do you expect the client to behave in this scenario? to resolve with the error in the respective index? reject with custom error that contains all the replies?

// resolve with the error:
const replies = await client.multi()
  .set('key', 'value')
  .hGetAll('key')
  .exec();
// replies = ['OK', ErrorReply('...')]

// reject with custom error:
try {
  await client.multi()
    .set('key', 'value')
    .hGetAll('key')
    .exec();
} catch (error) {
  if (error instanceof MultiReplyError) {
    error.replies;
    // replies = ['OK', ErrorReply('...')]
  }
}

possible fix:

transformReplies(rawReplies: Array<RedisCommandRawReply>): Array<RedisCommandRawReply> {
  const errorIndexes = [];
        
  const replies = rawReplies.map((reply, i) => {
    if (reply instanceof ReplyError) {
      errorIndexes.push(i);
      return reply;
    }
        
    const { transformReply, args } = this.queue[i];
    return transformReply ? transformReply(reply, args.preserve) : reply;
  });

  if (errorIndexes.length) throw new MultiErrorReply(replies, errorIndexes);
  return replies;
}

[LiraNuna]

It seems like this is an issue with all commands that throws an error in "runtime", not something specific to XAUTOCLAIM:

I suspected that but didn't have enough evidence. I just wanted to put out a bug report to make sure this is captured (notice above how I'm still experimenting).

How do you expect the client to behave in this scenario?

Ideally throw the exception reply from redis, as it stands currently the error is lost and it took us quite a while to figure out what happened.

The solution I proposed above isn't very good as it loses the stack trace from the original call (assuming it is uncaught). Your solution looks very nice, a wrapper exception of MultiErrorReply better explains what happened and gives the possibility of debugging.