NRG (2.11): Signal on initial applied messages, ensure 'expected per subject' consistency

[MauriceVanVeen]

This PR ensures consistent 'expected per subject' updates by introducing two new errors:

• expected last sequence per subject temporarily unavailable
  (when we have entries in our log after becoming leader, don't accept updates before we've applied all of them, because we could have pending updates in our log still)
• subject for expected last sequence is in process
  (don't allow multiple updates for the same subject until the first has been fully processed, essentially de-duping)

Without these it would mean multiple updates for the same subject could go through. For example having 2 updates for the same subject. The leader would accept the first, and fail on the second, upping mset.clfs/failed. In certain scenarios we could get the leader to do that, and have a follower accept both messages, leading to desync.

By introducing these two new errors, we ensure only one update per subject goes through at a time. And we can't desync based on this anymore.

---

For a full explanation of how it would go wrong before:

1. Send message foo.foo, gets accepted by the leader.
2. Send message foo.bar with expected subject sequence of 0, gets accepted by the leader.
3. Send message foo.foo, gets accepted by the leader.
4. The leader makes a snapshot now.
5. Send message foo.bar with expected subject sequence of 0, fails on the leader. This duplicate would not be blocked before if these messages were sent quickly enough.
6. Send message foo.foo, gets accepted by the leader.
7. Send another message for foo.bar with subject sequence of 1, gets accepted by the leader.

If we now restart a follower in a way where it requires the snapshot that was made at step 4, the following would happen:

1. Snapshot is installed, and upper layer catchup gives foo.foo at seq 1, no message at seq 2 (because foo.bar was updated in step 7), another foo.foo at seq 3.
2. Now we replay step 5, message foo.bar with expected subject sequence of 0. This failed on the leader.. but because the replica must now make an assumption what the leader did and we have no message for this sequence. It assumes this message must be applied, resulting in the first step of the stream desyncing.
3. Step 6 gets applied, but message foo.foo will have a sequence one higher than on the leader.
4. Step 7 gets executed, but fails on the follower because the subject sequence is not 1, but 4 due to the follower's step 2.
5. Now the stream has desynced in a way that could not be detected (if bytes were also the same). If more updates based on sequence would be done, the streams would continue desyncing, essentially a snowball effect.

Because we now never allow multiple updates for the same subject to go through, this can't happen anymore.

Signed-off-by: Maurice van Veen github@mauricevanveen.com

[derekcollison]

LGTM

[derekcollison]

Question I have is of the error message when we have some messages in flight. We know the sequence if it is accepted, etc. Should we just do normal processing with what we think the sequence will be?

[MauriceVanVeen]

We know the sequence if it is accepted, etc. Should we just do normal processing with what we think the sequence will be?

Not sure what you mean? We can't know what the sequence will be until it has fully gone through proposals and is persisted into the stream.

If we get two messages on the same subject with a expected subject sequence of 10, we must allow one to be persisted and we must reject one. If we don't reject the second one before proposing we could run into the issue described in the description.

Having errors before proposing and with the guarantee the one inflight message will be persisted is sadly the only way to guard against this issue.
(because a stream snapshot doesn't contain the messages themselves, and messages are fully deleted and not marked as deleted, there's no way to know as a follower after processing a snapshot what to do with a message if the message it's pointing to is deleted)

[derekcollison]

We do know the proposed sequence of all items proposed to the NRTG layer. So we could say that we track that and assume it will succeed so we can early error on the next message if it does not align with the expected sequence.

[MauriceVanVeen]

So we could say that we track that and assume it will succeed so we can early error on the next message if it does not align with the expected sequence.

We indeed know the proposed sequence. But until it's fully propagated and persisted that sequence will only be just that, an assumption. It's very likely to become the correct sequence, but that guarantee depends on which stream settings you use whether it's very likely or less likely to be right (never 100%).

So it's still dangerous as we shouldn't rely on an assumption, it should be 100% guaranteed for there to never be stream desync (otherwise we can't fix the desync). Back to what's mentioned in the description; If you have a snapshot and any chance of mset.clfs needing to be upped because that assumption was incorrect, then the stream will desync.

For more context about the validity of this fix. With a customer's repro that results in KV desyncing, and running the changes from this PR, #6213 and #6216 combined. I have been running that repro locally for more than 3 hours and it doesn't desync anymore. (Whereas it used to desync very often just within 15-30 minutes)

[derekcollison]

I still think an error saying wrong last sequence when rejecting a message would be more meaningful than the temporary status error, which would result in a retry of which 99.999% of the time would return wrong sequence error.

Hence why I am proposing we just say wrong sequence right away based on knowledge we have about inflight.

[MauriceVanVeen]

I still think an error saying wrong last sequence when rejecting a message would be more meaningful than the temporary status error, which would result in a retry of which 99.999% of the time would return wrong sequence error.

Agree, that sounds better. Have changed the error message to be just wrong last sequence.

[derekcollison]

LGTM