Fix fork when COMMIT_TRANSACTION is dropped

[tylerkaraszewski]

Details

This fixes a possible fork where followers fail to handle a commit sent by leader.

The existing behavior is that leader sends a BEGIN_TRANSACTION followed by a COMMIT_TRANSACTION to followers. When it is shutting down, one of these pairs is the final transaction sent to followers, and then the sockets to followers are closed.

The leader behavior is correct, the problem exists on followers.

When the follower is reading data off the socket, it reads the data that was sent before closing, and then marks the data as closed:

Bedrock/libstuff/STCPManager.cpp

Lines 119 to 139 in a88cf54

	if (SFDAnySet(fdm, socket.s, SREADEVTS | SWRITEEVTS)) {
	// Do both
	aliveAfterRecv = socket.recv();
	aliveAfterSend = socket.send();
	}
	} else {
	// Only send/recv if the socket is ready
	if (SFDAnySet(fdm, socket.s, SREADEVTS)) {
	aliveAfterRecv = socket.recv();
	}
	if (SFDAnySet(fdm, socket.s, SWRITEEVTS)) {
	aliveAfterSend = socket.send();
	}
	}
	// If we died, update
	if (!aliveAfterRecv || !aliveAfterSend) {
	// How did we die?
	SDEBUG("Connection to '" << socket.addr << "' died (recv=" << aliveAfterRecv << ", send="
	<< aliveAfterSend << ")");
	socket.state.store(Socket::CLOSED);

When the SQLitePeer object sees that the socket has been closed, it calls reset() on it's socket and returns SOCKET_CLOSED to SQLiteNode.

The problem is that it's possible that the COMMIT_TRANSACTION message is read immediately before the close on the socket, in the same call to postPoll (it's feasible even more messages are read on the same call, but they would all have the same issue).

We only tried to handle messages in the case that SQLitePeer::postPoll returned PeerPostPollStatus::OK:

Bedrock/sqlitecluster/SQLiteNode.cpp

Line 2673 in a88cf54

_onMESSAGE(peer, message);

So in this case, the COMMIT_TRANSACTION message was discarded.

However, this could lead to a fork, because the follower would never commit the last transaction sent by leader. Then when leader came back up, it could be out of sync with the follower.

This was easily reproducible on the HC-Tree branch, not because of any property of HC-Tree, but because the tests start and stop leader a lot more often for HC-Tree, making this occurrence more likely.

The fix was to:

1. Handle any messages that were received in the last postPoll even for PeerPostPollStatus::JUST_CONNECTED, PeerPostPollStatus::SOCKET_ERROR, and PeerPostPollStatus::SOCKET_CLOSED return values from SQLitePeer::postPoll (only SOCKET_CLOSED actually exhibited the problem here, but the others are for completeness). I extracted the logic here to a method called _processPeerMessages for this.
2. The peer messages need to still exist when we go to process them, which means we can't call reset on the SQLitePeer's socket before handling them. I moved the call to reset() out of SQLitePeer::postPoll and into SQLiteNode::postPoll.

Fixed Issues

https://github.com/Expensify/Expensify/issues/337537

Tests

---

Internal Testing Reminder: when changing bedrock, please compile auth against your new changes

[cead22]

NAB but why not make unlimited mandatory?

[tylerkaraszewski]

No particular reason I guess, I feel like we often supply defaults for things like this but I don't see any particular reason to do or not to do it here.

[danieldoglas]

Changes LGTM! Great catch!

[pecanoro]

Manually commenting since the deploy script is broken, this was deployed to webrock