diff --git a/plugins/Jobs.cpp b/plugins/Jobs.cpp index 6ce15bf68..e1e7c4bea 100644 --- a/plugins/Jobs.cpp +++ b/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 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"); diff --git a/plugins/Jobs.h b/plugins/Jobs.h index bbdc37dce..b0d1c4643 100644 --- a/plugins/Jobs.h +++ b/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(); diff --git a/test/tests/jobs/CrashIdentifyingValuesTest.cpp b/test/tests/jobs/CrashIdentifyingValuesTest.cpp new file mode 100644 index 000000000..70ba57b7c --- /dev/null +++ b/test/tests/jobs/CrashIdentifyingValuesTest.cpp @@ -0,0 +1,74 @@ +#include +#include +#include + +// 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;