fix server: fix last error reply

[adiholden]

bug 1: in multi command squasher error message was not set therefore it was not printed to log on the relevant command only on exec, fixed by setting the last error in CapturingReplyBuilder::SendError
bug 2: I sometimes see error logs which does not make sence f.e mset will print key does not exist, I added a dcheck for before command execution that there is no error set and it fails. reproduced with test_reply_guard_oom. @kostasrim will you be able to ivestigate this?

[kostasrim]

@adiholden on it thank you

[kostasrim]

🚢 🇮🇹

[adiholden]

why do we need this here? what are the cases that we ignore?

[kostasrim]

Any error that has to do with parsing the arguments, unknown command etc -- we do the same in InvokeCmd. These are not interesting to us.

An example from a few lines below:

  1210   if (auto err = VerifyCommandState(cid, args_no_cmd, *dfly_cntx); err) {
  1211     if (auto& exec_info = dfly_cntx->conn_state.exec_info; exec_info.IsCollecting())
  1212       exec_info.state = ConnectionState::ExecInfo::EXEC_ERROR;
  1213 
  1214     // We need to skip this because ACK's should not be replied to
  1215     // Bonus points because this allows to continue replication with ACL users who got
  1216     // their access revoked and reinstated
  1217     if (cid->name() == "REPLCONF" && absl::EqualsIgnoreCase(ArgS(args_no_cmd, 0), "ACK")) {
  1218       server_family_.GetDflyCmd()->OnClose(dfly_cntx);
  1219       return;
  1220     }
  1221     dfly_cntx->SendError(std::move(*err));
  1222     return;
  1223   }

SendError will register an error message for us to print but 1. it's not really interesting 2. it won't be printed at the right time.

P.s. we do the same in InvokeCmd() for similar paths.

[adiholden]

what do you mean these are not interesting for us?
we want to see error messages for commands sent to dragonfly, so if we have problem we will know the command and the params sent and it will be easy to reproduce the problem and to know if this is bug on our side or on client side.

[kostasrim]

Because those paths are not bugs related to commands, they are "soft errors". For example, acl authentication failed has nothing really interesting for us. The person could not execute the command because of their ACL's. If for some reason the client thinks that a command got rejected because of a bug they can file an issue. On the other hand, I most certainty don't think it would be useful to have a bunch of warnings because a client could not execute something that they have not credentials for. Similarly, for VerifyCommandState I don't think it's interesting let's say that a user could not run a command because the command is not allowed to be run under a script. These are soft errors and if we do have bugs, they can file the issue and it will be immediately apparent. Same applies for a command that gets rejected for arity (because the user did not specify enough arguments). I basically classify all of these errors are "soft" and I wanted to avoid logging them because even if there are bugs it will be easy to spot them. Furthermore, we log every second and that means that an ACL error will hide a command error and I believe the second is more important