Merge pull request #2016 from Expensify/tyler-remove-node-login

Stop sending NODE_LOGIN (still accept it from existing nodes)
Expensify · Dec 12, 2024 · 9b6a7ab · 9b6a7ab
2 parents 5e04561 + 9326901
commit 9b6a7ab
Show file tree

Hide file tree

Showing 2 changed files with 11 additions and 24 deletions.
diff --git a/sqlitecluster/SQLiteNode.cpp b/sqlitecluster/SQLiteNode.cpp
@@ -56,20 +56,6 @@
 // dbCountAtStart:   The highest committed transaction in the DB at the start of this transaction on leader, for
 //                   optimizing replication.
 
-// NOTE: This comment as well as NODE_LOGIN should be removed after https://github.com/Expensify/Bedrock/pull/1999 is deployed.
-// On LOGIN vs NODE_LOGIN.
-// _onConnect sends a LOGIN message.
-// _onConnect is called in exctly two places:
-// 1. In response to a NODE_LOGIN message received on a newly connected socket on the sync port. It's expected when
-//    establishing a connection, a node sends this NODE_LOGIN as its first message.
-// 2. Immediately following establishing a TCP connection to another node and sending a NODE_LOGIN message. In the case that
-//    we are the initiating node, we immediately queue three messages:
-//    1. NODE_LOGIN
-//    2. PING
-//    3. LOGIN
-//
-// When we receive a NODE_LOGIN, we immediately respond with a PING followed by a LOGIN (by calling _onConnect).
-
 #undef SLOGPREFIX
 #define SLOGPREFIX "{" << _name << "/" << SQLiteNode::stateName(_state) << "} "
 
@@ -1269,7 +1255,10 @@ void SQLiteNode::_onMESSAGE(SQLitePeer* peer, const SData& message) {
             SINFO("Received PONG from peer '" << peer->name << "' (" << peer->latency/1000 << "ms latency)");
             return;
         } else if (SIEquals(message.methodLine, "NODE_LOGIN")) {
-            // Do nothing, this keeps this code from warning until NODE_LOGIN is deprecated.
+            // We need to return early here to ignore this deprecated message and avoid throwing:
+            // STHROW("not logged in");
+            // Below. We can remove this check after one more deploy cycle.
+            // https://github.com/Expensify/Expensify/issues/450953
             return;
         }
 
@@ -2553,7 +2542,7 @@ void SQLiteNode::postPoll(fd_map& fdm, uint64_t& nextActivity) {
     // with peers, so we store any that we can remove in this list.
     list<Socket*> socketsToRemove;
 
-    // Check each new connection for a NODE_LOGIN message.
+    // Check each new connection for a LOGIN message.
     for (auto socket : _unauthenticatedIncomingSockets) {
         STCPManager::postPoll(fdm, *socket);
         try {
@@ -2565,7 +2554,9 @@ void SQLiteNode::postPoll(fd_map& fdm, uint64_t& nextActivity) {
             int messageSize = message.deserialize(socket->recvBuffer);
             if (messageSize) {
                 socket->recvBuffer.consumeFront(messageSize);
-                // Allow either LOGIN or NODE_LOGIN until we deprecate NODE_LOGIN.
+                // Old nodes, for one more upgrade cycle, will still send `NODE_LOGIN`. We can remove this check after this
+                // code is deployed.
+                // See: https://github.com/Expensify/Expensify/issues/450953
                 if (SIEquals(message.methodLine, "NODE_LOGIN") || SIEquals(message.methodLine, "LOGIN")) {
                     SQLitePeer* peer = getPeerByName(message["Name"]);
                     if (peer) {
@@ -2591,7 +2582,7 @@ void SQLiteNode::postPoll(fd_map& fdm, uint64_t& nextActivity) {
                         STHROW("Unauthenticated node '" + message["Name"] + "' attempted to connected, rejecting.");
                     }
                 } else {
-                    STHROW("expecting LOGIN or NODE_LOGIN");
+                    STHROW("expecting LOGIN");
                 }
             } else if (STimeNow() > socket->lastRecvTime + 5'000'000) {
                 STHROW("Incoming socket didn't send a message for over 5s, closing.");
@@ -2614,10 +2605,6 @@ void SQLiteNode::postPoll(fd_map& fdm, uint64_t& nextActivity) {
         switch (result) {
             case SQLitePeer::PeerPostPollStatus::JUST_CONNECTED:
             {
-                // When NODE_LOGIN is deprecated, we can remove the next 3 lines.
-                SData login("NODE_LOGIN");
-                login["Name"] = _name;
-                _sendToPeer(peer, login);
                 _onConnect(peer);
                 _sendPING(peer);
             }

diff --git a/sqlitecluster/SQLiteNode.h b/sqlitecluster/SQLiteNode.h
@@ -181,7 +181,7 @@ class SQLiteNode : public STCPManager {
     // would be a good idea for the caller to read any new commands or traffic from the network.
     bool update();
 
-    // Look up the correct peer by the name it supplies in a NODE_LOGIN
+    // Look up the correct peer by the name it supplies in a LOGIN
     // message. Does not lock, but this method is const and all it does is
     // access _peerList and peer->name, both of which are const. So it is safe
     // to call from other public functions.
@@ -293,7 +293,7 @@ class SQLiteNode : public STCPManager {
     const string _version;
 
     // These are sockets that have been accepted on the node port but have not yet been associated with a peer (because
-    // they need to send a NODE_LOGIN message with their name first).
+    // they need to send a LOGIN message with their name first).
     set<Socket*> _unauthenticatedIncomingSockets;
 
     // The write consistency requested for the current in-progress commit.