Respond to standup at login correctly. #1877

tylerkaraszewski · 2024-09-23T21:43:07Z

Details

See investigation in comments here: https://github.com/Expensify/Expensify/issues/426227

When we connect to a peer, if we're STANDINGUP we intend to send a STATE message indicating that the remote node should approve or deny our standup:

Bedrock/sqlitecluster/SQLiteNode.cpp

Lines 1810 to 1831 in cedf037

    
           void SQLiteNode::_onConnect(SQLitePeer* peer) { 
        
               SASSERT(peer); 
        
               SASSERTWARN(!peer->loggedIn); 
        
               // Send the LOGIN 
        
               PINFO("Sending LOGIN"); 
        
               SData login("LOGIN"); 
        
               login["Priority"] = to_string(_priority); 
        
               login["State"] = stateName(_state); 
        
               login["Version"] = _version; 
        
               login["Permafollower"] = _originalPriority ? "false" : "true"; 
        
               _sendToPeer(peer, login); 
        
               // If we're STANDINGUP when a peer connects, send them a STATE message so they know they need to APPROVE or DENY the standup. 
        
               // Otherwise we will wait for their response that's not coming,and can eventually time out the standup. 
        
               if (_state == SQLiteNodeState::STANDINGUP) { 
        
                   SData state("STATE"); 
        
                   state["StateChangeCount"] = to_string(_stateChangeCount); 
        
                   state["State"] = stateName(_state); 
        
                   state["Priority"] = SToStr(_priority); 
        
                   _sendToPeer(peer, state); 
        
               } 
        
           }

However, because the LOGIN and STATE messages are sent back to back, they will always have the same state (which is STANDINGUP).

This means that when processing the STATE message we'll always hit this code:

Bedrock/sqlitecluster/SQLiteNode.cpp

Lines 1364 to 1368 in cedf037

    
           if (from == to) { 
        
               // No state change, just new commits? 
        
               PINFO("Peer received new commit in state '" << stateName(from) << "', commit #" << message["CommitCount"] << " (" 
        
                     << message["Hash"] << ")"); 
        
           } else {

Rather than this code:

Bedrock/sqlitecluster/SQLiteNode.cpp

Line 1424 in cedf037

SData response("STANDUP_RESPONSE");

We should probably send STANDUP_RESPONSE in response to LOGIN when the node is STANDINGUP and remove the second STATE message here.

This will fix the race condition where a late login during standup prevents the cluster from forming which happens because of this line:

Bedrock/sqlitecluster/SQLiteNode.cpp

Line 2033 in cedf037

_forkedFrom.clear();

However, I think that just puts us in a different bad state in the case we are forked from the node that does the late connect. Now it will respond to the LOGIN with a STANDUP_RESPONSE but this will be a DENY which is just as bad.

I think we should adjust the logic to not clear the _forkedFrom list until we are actually LEADING or FOLLOWING. I can imagine some scenario where clearing this in STANDINGUP gets us to reset a node that was forked and no longer is, i.e., it was just restored from backup, and it was the final node required to reach quorum. In this case, we would still not form a cluster, we'd need to restart the node that is attempting to stand up.

Thoughts on this?

EDIT: Looking at the code, we won't actually DENY due to a hash mismatch, only if we think some other node is leading. So a node that's forked from us (but hasn't realized that yet) can still approve the standup, and it will abstain if it does realize it's forked.

NOTE: The code in _sendStandupResponse is exactly the block deleted from _onMESSAGE with no changes except indentation.

Fixed Issues

Fixes https://github.com/Expensify/Expensify/issues/426227

Tests

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

rafecolton

The code change is simple enough and LGTM 👍 Have some questions.

However, I think that just puts us in a different bad state in the case we are forked from the node that does the late connect. Now it will respond to the LOGIN with a STANDUP_RESPONSE but this will be a DENY which is just as bad.

EDIT: Looking at the code, we won't actually DENY due to a hash mismatch, only if we think some other node is leading. So a node that's forked from us (but hasn't realized that yet) can still approve the standup, and it will abstain if it does realize it's forked.

This is not actually a problem due to the EDIT: portion, right?

I think we should adjust the logic to not clear the _forkedFrom list until we are actually LEADING or FOLLOWING. I can imagine some scenario where clearing this in STANDINGUP gets us to reset a node that was forked and no longer is, i.e., it was just restored from backup, and it was the final node required to reach quorum. In this case, we would still not form a cluster, we'd need to restart the node that is attempting to stand up.

Can you elaborate a bit and state if you still think this is necessary? I'm not understanding how the scenario above would cause us to not form a cluster

cead22 · 2024-09-24T09:23:47Z

This will fix the race condition where a late login during standup prevents the cluster from forming which happens because of this line:

I read everything a few times, but the one thing I don't understand is this scenario. Can you break it down?

Regardless, the change looks good to me and it seem like a simplification

danieldoglas

Changes LGTM, just a few questions so I can better understand how bedrock works

danieldoglas · 2024-09-24T14:16:34Z

sqlitecluster/SQLiteNode.cpp

+                if (otherPeer->state == SQLiteNodeState::STANDINGUP || otherPeer->state == SQLiteNodeState::LEADING || otherPeer->state == SQLiteNodeState::STANDINGDOWN) {
+                    // We need to contest this standup
+                    response["Response"] = "deny";
+                    response["Reason"] = "peer '" + otherPeer->name + "' is '" + stateName(otherPeer->state) + "'";
+                    break;
+                }


I know this is not related to the changes you're applying, but just wanted to understand this part. In the code above, if a peer is trying to standup and we're standing down, we add a warn (line 2762) but don't really set anything in the response. But in this block, if we see another peer standing down, we actually deny it.

Why do we have that different behavior between both?

I think this is just an oversight where we've used a definition of leading in this case that includes standingup and standingdown. We could probably exclude it here as well and only use deny if the peer is standingup or leading.

danieldoglas · 2024-09-24T14:41:12Z

sqlitecluster/SQLiteNode.cpp

-
-    // If we're STANDINGUP when a peer connects, send them a STATE message so they know they need to APPROVE or DENY the standup.
-    // Otherwise we will wait for their response that's not coming,and can eventually time out the standup.
-    if (_state == SQLiteNodeState::STANDINGUP) {
-        SData state("STATE");
-        state["StateChangeCount"] = to_string(_stateChangeCount);
-        state["State"] = stateName(_state);
-        state["Priority"] = SToStr(_priority);
-        _sendToPeer(peer, state);
-    }


Since we're not using any locks here, would it be possible that we have a state while sending login and another when sending state?

And in that case, considering that the _changeState would have been called somewhere else, would everything still work out fine since _changeState also sends a STATE message?

It would have been a bug if state changed between these two messages, though that couldn't happen any more since this change as we only send one now, right?

But state couldn't change between these two messages as only the sync thread can change state, and only the sync thread calls _onConnect.

Does that adequately answer your question?

It does, thanks!

chiragsalian

Im not sure if i 100% understand, but the code changes here LGTM. Is there some way to simulate postPoll and peer state changes in our vm?

tylerkaraszewski · 2024-09-30T23:11:14Z

This is not actually a problem due to the EDIT: portion, right?

Yes, that seems to be the case, this isn't actually a problem.

Can you elaborate a bit and state if you still think this is necessary? I'm not understanding how the scenario above would cause us to not form a cluster

Ok, let me quote myself for quick reference:

I think we should adjust the logic to not clear the _forkedFrom list until we are actually LEADING or FOLLOWING. I can imagine some scenario where clearing this in STANDINGUP gets us to reset a node that was forked and no longer is, i.e., it was just restored from backup, and it was the final node required to reach quorum. In this case, we would still not form a cluster, we'd need to restart the node that is attempting to stand up.

I believe I intended the "edit" block to apply to this, meaning that we can ignore the "I think we should... " sentence. But let's clarify the rest.

Let's say we are 1.sjc, and we are connected to 1.lax. Neither node is forked from the other, but 1.sjc thinks it is forked from 2.sjc. There is no cluster, say all other nodes are offline.

When 2.sjc comes back up (and is no longer forked), does 1.sjc exclude it from approving the standup because it's forked? If so, we will never reach "LEADING" because we will never have our standup approved. However, if we clear the list of forked nodes at STANDINGUP, then we can count 2.sjc's approval because it is no longer forked.

I don't think this actually matters though, since we don't send DENY due to being forked.

tylerkaraszewski · 2024-09-30T23:32:03Z

I read everything a few times, but the one thing I don't understand is this scenario. Can you break it down?
Regardless, the change looks good to me and it seem like a simplification

So the investigation for that was actually in this comment: https://github.com/Expensify/Expensify/issues/426227

2.sjc tries to stand up.
It does not get a response from 1.sjc
it goes searching.
It picks 1.sjc as a sync peer.
When it tries to sync, It notices it's forked from 1.sjc
This triggers a reconnect to 1.sjc
It tries to stand up again, because the rest of the cluster is available.
1.sjc reconnects after it starts standing up.
The whole thing repeats.

If we did not remove 1.sjc from the list of forked peers, we wouldn't try to choose it as a sync peer and reconnect to it.

I think that's complete.

tylerkaraszewski · 2024-09-30T23:32:23Z

I've going to merge, feel free to leave more comments if anything was inadequately answered.

Notes

f46df8e

tylerkaraszewski self-assigned this Sep 23, 2024

tylerkaraszewski added 3 commits September 23, 2024 14:56

Move standup response to separate function

1b4028f

Don't send separate state message

0a63e7e

Cleanup

28ce2a3

tylerkaraszewski requested review from cead22, rafecolton, coleaeason and chiragsalian September 23, 2024 22:00

tylerkaraszewski changed the title ~~[WIP] Respond to standup at login correctly.~~ Respond to standup at login correctly. Sep 23, 2024

rafecolton reviewed Sep 24, 2024

View reviewed changes

cead22 approved these changes Sep 24, 2024

View reviewed changes

danieldoglas self-requested a review September 24, 2024 14:06

danieldoglas approved these changes Sep 24, 2024

View reviewed changes

chiragsalian approved these changes Sep 24, 2024

View reviewed changes

coleaeason approved these changes Sep 24, 2024

View reviewed changes

rafecolton approved these changes Sep 26, 2024

View reviewed changes

tylerkaraszewski merged commit 1ef3e2d into main Sep 30, 2024
1 check passed

tylerkaraszewski deleted the tyler-fix-connect-while-standing-up branch September 30, 2024 23:32

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Respond to standup at login correctly. #1877

Respond to standup at login correctly. #1877

tylerkaraszewski commented Sep 23, 2024 •

edited

Loading

rafecolton left a comment

cead22 commented Sep 24, 2024

danieldoglas left a comment

danieldoglas Sep 24, 2024

tylerkaraszewski Sep 30, 2024

danieldoglas Sep 24, 2024

tylerkaraszewski Sep 30, 2024

danieldoglas Oct 1, 2024

chiragsalian left a comment

tylerkaraszewski commented Sep 30, 2024

tylerkaraszewski commented Sep 30, 2024

tylerkaraszewski commented Sep 30, 2024

	void SQLiteNode::_onConnect(SQLitePeer* peer) {
	SASSERT(peer);
	SASSERTWARN(!peer->loggedIn);
	// Send the LOGIN
	PINFO("Sending LOGIN");
	SData login("LOGIN");
	login["Priority"] = to_string(_priority);
	login["State"] = stateName(_state);
	login["Version"] = _version;
	login["Permafollower"] = _originalPriority ? "false" : "true";
	_sendToPeer(peer, login);

	// If we're STANDINGUP when a peer connects, send them a STATE message so they know they need to APPROVE or DENY the standup.
	// Otherwise we will wait for their response that's not coming,and can eventually time out the standup.
	if (_state == SQLiteNodeState::STANDINGUP) {
	SData state("STATE");
	state["StateChangeCount"] = to_string(_stateChangeCount);
	state["State"] = stateName(_state);
	state["Priority"] = SToStr(_priority);
	_sendToPeer(peer, state);
	}
	}

	if (from == to) {
	// No state change, just new commits?
	PINFO("Peer received new commit in state '" << stateName(from) << "', commit #" << message["CommitCount"] << " ("
	<< message["Hash"] << ")");
	} else {

Respond to standup at login correctly. #1877

Respond to standup at login correctly. #1877

Conversation

tylerkaraszewski commented Sep 23, 2024 • edited Loading

Details

Fixed Issues

Tests

rafecolton left a comment

Choose a reason for hiding this comment

cead22 commented Sep 24, 2024

danieldoglas left a comment

Choose a reason for hiding this comment

danieldoglas Sep 24, 2024

Choose a reason for hiding this comment

tylerkaraszewski Sep 30, 2024

Choose a reason for hiding this comment

danieldoglas Sep 24, 2024

Choose a reason for hiding this comment

tylerkaraszewski Sep 30, 2024

Choose a reason for hiding this comment

danieldoglas Oct 1, 2024

Choose a reason for hiding this comment

chiragsalian left a comment

Choose a reason for hiding this comment

tylerkaraszewski commented Sep 30, 2024

tylerkaraszewski commented Sep 30, 2024

tylerkaraszewski commented Sep 30, 2024

tylerkaraszewski commented Sep 23, 2024 •

edited

Loading