Exclude volatile per-request fields from Jobs crash blacklist

plugins/Jobs.cpp

@@ -116,15 +116,38 @@ void BedrockPlugin_Jobs::upgradeDatabase(SQLite& db)
     SASSERT(db.verifyIndex("jobsPriorityNextRunWWWStag", "jobs", "(priority, nextRun) WHERE state IN ('QUEUED', 'RUNQUEUED') AND name GLOB 'www-stag/*'", false, !BedrockPlugin_Jobs::isLive));
 }
 
+// ==========================================================================
+void BedrockJobsCommand::populateCrashIdentifyingValues()
+{
+    // We key the crash blacklist ("poison-pill" protection) on an explicit whitelist of the request
+    // fields that semantically identify a Jobs command: `name` and `data`. Everything else (e.g.
+    // `requestID`, `ID`, `lastIP`, `_source`) is volatile per-request data that is never identical
+    // across two requests.
+    //
+    // Whitelisting matters because of how BedrockServer::_wouldCrash treats multiple crashes of the
+    // same methodLine: if a command crashes with more than one distinct set of identifying values, it
+    // assumes we've already lost more than one node and fully blocks the command for everyone. If we
+    // keyed on volatile fields, two crashes of the *same* command would record two different sets
+    // (because e.g. `requestID` differs) and wrongly trip that block-everyone safeguard. Keying on
+    // just `name` and `data` means the same command crashing twice matches a single blacklist entry
+    // (blocking only that command), while two genuinely different commands crashing correctly trips
+    // the block-everyone safeguard.
+    //
+    // CrashMap::insert only records a field if it's actually set on the request, so listing a field
+    // that's absent is a no-op.
+    static const set<string, STableComp> crashIdentifyingFields = {"name", "data"};
+    for (const auto& field : crashIdentifyingFields) {
+        crashIdentifyingValues.insert(field);
+    }
+}
+
 // ==========================================================================
 bool BedrockJobsCommand::peek(SQLite& db)
 {
     const string& requestVerb = request.getVerb();
 
     // Jobs commands can only crash if they look identical.
-    for (const auto& name : request.nameValueMap) {
-        crashIdentifyingValues.insert(name.first);
-    }
+    populateCrashIdentifyingValues();
 
     // We can potentially change this, so we set it here.
     mockRequest = request.isSet("mockRequest");

plugins/Jobs.h

@@ -27,6 +27,12 @@ class BedrockPlugin_Jobs : public BedrockPlugin {
 class BedrockJobsCommand : public BedrockCommand {
 public:
     BedrockJobsCommand(SQLiteCommand&& baseCommand, BedrockPlugin* plugin);
+
+    // Populates crashIdentifyingValues with the request fields that identify this command for the
+    // crash blacklist ("poison-pill" protection), excluding volatile per-request fields (e.g.
+    // requestID) that would otherwise make the blacklist entry impossible to ever match.
+    void populateCrashIdentifyingValues();
+
     virtual bool peek(SQLite& db);
     virtual void process(SQLite& db);
     virtual void handleFailedReply();

test/tests/jobs/CrashIdentifyingValuesTest.cpp

@@ -0,0 +1,74 @@
+#include <libstuff/SData.h>
+#include <plugins/Jobs.h>
+#include <test/lib/BedrockTester.h>
+
+// Unit tests for BedrockJobsCommand::populateCrashIdentifyingValues(). The crash blacklist
+// ("poison-pill" protection) refuses any future command whose methodLine and crashIdentifyingValues
+// exactly match a command that previously crashed a node. crashIdentifyingValues is keyed on an
+// explicit whitelist of the fields that semantically identify a Jobs command -- `name` and `data`.
+// Everything else (e.g. `requestID`, `jobID`, `priority`, `lastIP`) is volatile or non-identifying
+// per-request data. Keying only on `name`/`data` keeps the same command's repeated crashes on a
+// single blacklist entry, and lets BedrockServer::_wouldCrash's "more than one identifying set ->
+// block everyone" safeguard fire only when genuinely different commands crash. These tests verify
+// only the whitelisted fields are kept and everything else is dropped.
+struct JobsCrashIdentifyingValuesTest : tpunit::TestFixture
+{
+    JobsCrashIdentifyingValuesTest()
+        : tpunit::TestFixture("JobsCrashIdentifyingValues",
+                              TEST(JobsCrashIdentifyingValuesTest::onlyKeepsWhitelistedFields),
+                              TEST(JobsCrashIdentifyingValuesTest::keepsOnlyPresentWhitelistedFields))
+    {
+    }
+
+    // Only the whitelisted fields (`name`, `data`) and their values are kept; every other field --
+    // including per-request volatile fields -- is dropped.
+    void onlyKeepsWhitelistedFields()
+    {
+        SData request("GetJob");
+        request["name"] = "TestJob";
+        request["data"] = "{\"key\":\"value\"}";
+        request["jobID"] = "12345";
+        request["priority"] = "500";
+        request["requestID"] = "abc123";
+        request["ID"] = "999";
+        request["lastIP"] = "127.0.0.1";
+        request["_source"] = "someSource";
+
+        BedrockJobsCommand command(SQLiteCommand(move(request)), nullptr);
+        command.populateCrashIdentifyingValues();
+
+        // Only the whitelisted fields are kept, with their original values.
+        ASSERT_EQUAL(command.crashIdentifyingValues.size(), 2u);
+        ASSERT_TRUE(command.crashIdentifyingValues.count("name"));
+        ASSERT_TRUE(command.crashIdentifyingValues.count("data"));
+        ASSERT_EQUAL(command.crashIdentifyingValues["name"], "TestJob");
+        ASSERT_EQUAL(command.crashIdentifyingValues["data"], "{\"key\":\"value\"}");
+
+        // Every non-whitelisted field must be excluded -- including non-volatile-but-non-identifying
+        // fields like jobID/priority, so the same command crashing twice stays on one blacklist entry.
+        ASSERT_FALSE(command.crashIdentifyingValues.count("jobID"));
+        ASSERT_FALSE(command.crashIdentifyingValues.count("priority"));
+        ASSERT_FALSE(command.crashIdentifyingValues.count("requestID"));
+        ASSERT_FALSE(command.crashIdentifyingValues.count("ID"));
+        ASSERT_FALSE(command.crashIdentifyingValues.count("lastIP"));
+        ASSERT_FALSE(command.crashIdentifyingValues.count("_source"));
+    }
+
+    // A whitelisted field that isn't set on the request is not recorded (CrashMap::insert skips
+    // fields the request doesn't have).
+    void keepsOnlyPresentWhitelistedFields()
+    {
+        SData request("CreateJob");
+        request["name"] = "AnotherJob";
+        request["priority"] = "500";
+
+        BedrockJobsCommand command(SQLiteCommand(move(request)), nullptr);
+        command.populateCrashIdentifyingValues();
+
+        // `data` was never set, so only `name` should be present; `priority` is not whitelisted.
+        ASSERT_EQUAL(command.crashIdentifyingValues.size(), 1u);
+        ASSERT_TRUE(command.crashIdentifyingValues.count("name"));
+        ASSERT_FALSE(command.crashIdentifyingValues.count("data"));
+        ASSERT_FALSE(command.crashIdentifyingValues.count("priority"));
+    }
+} __JobsCrashIdentifyingValuesTest;