Correct ae.commit on recovery to equal call to applyCommit(index)

[MauriceVanVeen]

A stream would be removed from the cluster when hard-killing all servers directly after the stream has been added (or at least before meta was snapshotted).

To reproduce:

# setup cluster of 3 servers
# once ready, add a R3 stream
nats str add stream --defaults --subjects stream --replicas 3

# now hard kill all servers (I had them running locally with nats-0 on 4222, nats-1 on 4333, and nats-2 on 4444)
fuser 4444/tcp -k -SIGKILL
fuser 4333/tcp -k -SIGKILL
fuser 4222/tcp -k -SIGKILL

Then restart all servers and notice the stream being removed once a new meta leader has been chosen:

$ nats server report jetstream
╭───────────────────────────────────────────────────────────────────────────────────────────────────────────────╮
│                                               JetStream Summary                                               │
├─────────┬──────────────┬─────────┬───────────┬──────────┬───────┬────────┬──────┬─────────┬─────────┬─────────┤
│ Server  │ Cluster      │ Streams │ Consumers │ Messages │ Bytes │ Memory │ File │ API Req │ API Err │ Pending │
├─────────┼──────────────┼─────────┼───────────┼──────────┼───────┼────────┼──────┼─────────┼─────────┼─────────┤
│ nats-0* │ nats-cluster │ 0       │ 0         │ 0        │ 0 B   │ 0 B    │ 0 B  │ 0       │ 0       │       0 │
│ nats-1  │ nats-cluster │ 0       │ 0         │ 0        │ 0 B   │ 0 B    │ 0 B  │ 0       │ 0       │       0 │
│ nats-2  │ nats-cluster │ 0       │ 0         │ 0        │ 0 B   │ 0 B    │ 0 B  │ 0       │ 0       │       0 │
├─────────┼──────────────┼─────────┼───────────┼──────────┼───────┼────────┼──────┼─────────┼─────────┼─────────┤
│         │              │ 0       │ 0         │ 0        │ 0 B   │ 0 B    │ 0 B  │ 0       │ 0       │       0 │
╰─────────┴──────────────┴─────────┴───────────┴──────────┴───────┴────────┴──────┴─────────┴─────────┴─────────╯

On recovery the only snapshot the server has doesn't contain the stream, as the snapshot was created before the stream was. When a new meta leader is chosen it will create a snapshot and send out to the followers, but because that snapshot is empty the stream will be removed.

This PR ensures that the stream addition is properly replayed on recovery.
Also added a test that reproduces the hard kill scenario, by copying and reverting storage directories to previous state.

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

[MauriceVanVeen]

didRemove used to be set a long time ago, but now it would always be false.

To ensure the meta snapshot is written both for creation and removal, return true at the end instead.

[derekcollison]

We do not need to snapshot as the log with the add will be replayed. Do you mean the whole machine goes away?

[MauriceVanVeen]

We do not need to snapshot as the log with the add will be replayed.

I haven't seen that to be the case.

Same machine and same storage, just a non-graceful shutdown so the meta snapshot can't be written before restart.

[derekcollison]

Most of the time the snapshot is not written, and the log remaining after any given snapshot is replayed.

I was testing something the other day and disabled meta snapshots in the monitorCluster (and setLeader) calls and behavior was ok.

Do we have a test that can show what you are seeing?

[MauriceVanVeen]

Do we have a test that can show what you are seeing?

Yeah, this PR has a test that simulates a hard kill by copying the storage directories after AddStream returns successfully.

As well as spinning up a cluster myself locally and doing a real hard kill, results in an empty snapshot being sent and the stream being removed.
Recovery did not show the stream assignment to be replayed.

[derekcollison]

Sending empty snapshot could be a problem. We should protect against that for sure, but not force a snapshot IMO on add stream.

[neilalexander]

This might be related to those calls to SendSnapshot() that we definitely need to revisit.

[MauriceVanVeen]

and the log remaining after any given snapshot is replayed

Looked at the replaying, and it does replay it at least.
But when it calls into processAppendEntry nothing really seems to happen there. I would have expected applyCommit to be called?

[MauriceVanVeen]

Pushed a fix for an off-by-one that fixes this issue as well.

index=6 for example, so we would normally call applyCommit(index). But the AppendEntry has commit=5. So if the n.commit=5 on recovery it would not be applied since ae.commit > n.commit is false. Correcting ae.commit to equal to index, that ensures we'll call applyCommit in processAppendEntry.
https://github.com/nats-io/nats-server/pull/5946/files#diff-0a3d4e9b87bb3880bdb735fa489ef618b571de3370dae31d8f509b26e2fb852eR475

[MauriceVanVeen]

The above was an incorrect fix, instead came to another alternative which actually turned out to be @neilalexander's PR #5700.

This PR can be closed when Neil's PR is approved/merged 🎉

[MauriceVanVeen]

Test was included in #5700 and got merged, closing.