Lease based read-only request without check_quorum behaves wrong.

[rustonaut]

Lease based read-only request without check_quorum behaves wrong, at least in outdated leader in a minority partition.

In step_leader for handling MsgReadIndex messages.

https://github.com/pingcap/raft-rs/blob/3799cfa482e288a0fc4a3a6c365c6681afd85b3a/src/raft.rs#L1429-L1449

I think line 1437 should use read_index instead of self.raft_log.commited, if
I'm not wrong (and if I'm wrong the read_index variable should be created in
the else branch to prevent confusion).

If my understanding of the code is correct then lease base read requests allow
you to read up to the commit index iff self.check_quorum is set as this would
make sure that the leader still has the majority behind them and return INVALID_INDEX
if not (which btw. seems to be kind of a strange, shouldn't then Config::validate at last
warn on using a leas based reads without check_quorum).

[Hoverbear]

@rustonaut Thanks for this report. 🙇‍♀️ It's very good and clear. :)

You can see that etcd's Raft does similar...: https://github.com/etcd-io/etcd/blob/83304cfc808cf6303d48c45a696f169fae422e68/raft/raft.go#L1010-L1037

	case pb.MsgReadIndex:
		if r.quorum() > 1 {
			if r.raftLog.zeroTermOnErrCompacted(r.raftLog.term(r.raftLog.committed)) != r.Term {
				// Reject read only request when this leader has not committed any log entry at its term.
				return nil
			}

			// thinking: use an interally defined context instead of the user given context.
			// We can express this in terms of the term and index instead of a user-supplied value.
			// This would allow multiple reads to piggyback on the same message.
			switch r.readOnly.option {
			case ReadOnlySafe:
				r.readOnly.addRequest(r.raftLog.committed, m)
				r.bcastHeartbeatWithCtx(m.Entries[0].Data)
			case ReadOnlyLeaseBased:
				ri := r.raftLog.committed
				if m.From == None || m.From == r.id { // from local member
					r.readStates = append(r.readStates, ReadState{Index: r.raftLog.committed, RequestCtx: m.Entries[0].Data})
				} else {
					r.send(pb.Message{To: m.From, Type: pb.MsgReadIndexResp, Index: ri, Entries: m.Entries})
				}
			}
		} else {
			r.readStates = append(r.readStates, ReadState{Index: r.raftLog.committed, RequestCtx: m.Entries[0].Data})
		}

		return nil
}

I'm going to investigate this a bit more.

[Hoverbear]

Hi @rustonaut , can you take a look at #141 . I think you are correct.

[rustonaut]

I think it's correct, long term we might want to consider to maybe warn if
lease based reads are used without check_quorum as this would be a
non-time limited lease in case of a long-term/permanent network partition.