Make sure peek and process happen in the same state

Author: tylerkaraszewski

BedrockCommand.h

@@ -150,6 +150,9 @@ class BedrockCommand : public SQLiteCommand {
     // in `process` instead of peek, as it will always be escalated to leader 
     const bool escalateImmediately;
 
+    // Record the state we were acting under in the last call to `peek` or `process`.
+    SQLiteNode::State lastPeekedOrProcessedInState = SQLiteNode::UNKNOWN;
+
   protected:
     // The plugin that owns this command.
     BedrockPlugin* _plugin;

BedrockCore.cpp

@@ -68,6 +68,7 @@ bool BedrockCore::isTimedOut(unique_ptr<BedrockCommand>& command) {
 BedrockCore::RESULT BedrockCore::peekCommand(unique_ptr<BedrockCommand>& command, bool exclusive) {
     AutoTimer timer(command, BedrockCommand::PEEK);
     BedrockServer::ScopedStateSnapshot snapshot(_server);
+    command->lastPeekedOrProcessedInState = _server.getState();
 
     // Convenience references to commonly used properties.
     const SData& request = command->request;
@@ -176,6 +177,15 @@ BedrockCore::RESULT BedrockCore::processCommand(unique_ptr<BedrockCommand>& comm
     AutoTimer timer(command, BedrockCommand::PROCESS);
     BedrockServer::ScopedStateSnapshot snapshot(_server);
 
+    // We need to be leading (including standing down) and we need to have peeked this command in the same set of
+    // states, or we can't complete this command (we can't commit the command of we're not leading, and if we're
+    // leading but were following when we peeked, we may try to read HTTPS requests we never made).
+    if ((command->lastPeekedOrProcessedInState != SQLiteNode::LEADING && command->lastPeekedOrProcessedInState != SQLiteNode::STANDINGDOWN) ||
+        (_server.getState() != SQLiteNode::LEADING && _server.getState() != SQLiteNode::STANDINGDOWN)) {
+        return RESULT::SERVER_NOT_LEADING;
+    }
+    command->lastPeekedOrProcessedInState = _server.getState();
+
     // Convenience references to commonly used properties.
     const SData& request = command->request;
     SData& response = command->response;

BedrockCore.h

@@ -14,7 +14,8 @@ class BedrockCore : public SQLiteCore {
         SHOULD_PROCESS = 2,
         NEEDS_COMMIT = 3,
         NO_COMMIT_REQUIRED = 4,
-        ABANDONED_FOR_CHECKPOINT = 5
+        ABANDONED_FOR_CHECKPOINT = 5,
+        SERVER_NOT_LEADING = 6
     };
 
     // Automatic timing class that records an entry corresponding to its lifespan.

BedrockServer.cpp

@@ -1093,6 +1093,22 @@ void BedrockServer::worker(SQLitePool& dbPool,
                     } else if (result == BedrockCore::RESULT::NO_COMMIT_REQUIRED) {
                         // Nothing to do in this case, `command->complete` will be set and we'll finish as we fall out
                         // of this block.
+                    } else if (result == BedrockCore::RESULT::SERVER_NOT_LEADING) {
+                        // We won't write regardless.
+                        core.rollback();
+
+                        // If there are no HTTPS requests, we can just re-queue this command, otherwise, we will
+                        // potentially run the same HTTPS requests twice.
+                        if (command->httpsRequests.size()) {
+                            SALERT("Server stopped leading while running command with HTTPS requests!");
+                            command->response.methodLine = "500 Leader stopped leading";
+                            server._reply(command);
+                            break;
+                        } else {
+                            // Allow for an extra retry and start from the top, like with ABANDONED_FOR_CHECKPOINT.
+                            SINFO("State changed before 'processCommand' but no HTTPS requests so retrying.");
+                            ++retry;
+                        }
                     } else {
                         SERROR("processCommand (" << command->request.getVerb() << ") returned invalid result code: " << (int)result);
                     }