From 1909b834c93a6f90359bd0138841da0e1b76a2e6 Mon Sep 17 00:00:00 2001 From: "Carlos Alvarez (via MelvinBot)" Date: Wed, 8 Jul 2026 13:15:52 +0000 Subject: [PATCH 1/4] Exclude volatile per-request fields from Jobs crash blacklist The Jobs plugin populated crashIdentifyingValues with every request field, including requestID (unique per request). Since _wouldCrash requires an exact match on every stored key/value, the blacklist entry could never match a future command, making the poison-pill protection a no-op for Jobs commands. Exclude volatile per-request fields (requestID, ID, lastIP, _source) so the blacklist keys on semantically meaningful fields (e.g. job name and data). Co-authored-by: Carlos Alvarez --- plugins/Jobs.cpp | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/plugins/Jobs.cpp b/plugins/Jobs.cpp index 6ce15bf68..d9ec178e8 100644 --- a/plugins/Jobs.cpp +++ b/plugins/Jobs.cpp @@ -121,8 +121,15 @@ bool BedrockJobsCommand::peek(SQLite& db) { const string& requestVerb = request.getVerb(); - // Jobs commands can only crash if they look identical. + // Jobs commands can only crash if they look identical. We key the crash blacklist on the + // semantically meaningful request fields, but exclude volatile, per-request fields that are + // never identical across two requests (e.g. `requestID`). Including them would make the + // blacklist entry impossible to match, rendering the poison-pill protection a no-op. + static const set volatileFields = {"requestID", "ID", "lastIP", "_source"}; for (const auto& name : request.nameValueMap) { + if (volatileFields.count(name.first)) { + continue; + } crashIdentifyingValues.insert(name.first); } From 6e3398543cb40b49e78bc44d1e656d5a08805681 Mon Sep 17 00:00:00 2001 From: Carlos Alvarez Date: Wed, 8 Jul 2026 10:18:12 -0400 Subject: [PATCH 2/4] Update plugins/Jobs.cpp --- plugins/Jobs.cpp | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/plugins/Jobs.cpp b/plugins/Jobs.cpp index d9ec178e8..5d3eab9d0 100644 --- a/plugins/Jobs.cpp +++ b/plugins/Jobs.cpp @@ -121,10 +121,8 @@ bool BedrockJobsCommand::peek(SQLite& db) { const string& requestVerb = request.getVerb(); - // Jobs commands can only crash if they look identical. We key the crash blacklist on the - // semantically meaningful request fields, but exclude volatile, per-request fields that are - // never identical across two requests (e.g. `requestID`). Including them would make the - // blacklist entry impossible to match, rendering the poison-pill protection a no-op. + // Jobs commands can only crash if they look identical. + // We exclude unique request fields so blacklist entries can be matched static const set volatileFields = {"requestID", "ID", "lastIP", "_source"}; for (const auto& name : request.nameValueMap) { if (volatileFields.count(name.first)) { From b6583abd456820b476dc47d1e757429265bafca7 Mon Sep 17 00:00:00 2001 From: "Carlos Alvarez (via MelvinBot)" Date: Wed, 8 Jul 2026 14:32:47 +0000 Subject: [PATCH 3/4] Add unit test for Jobs crash-blacklist volatile-field exclusion Extract the crashIdentifyingValues population into a testable helper (BedrockJobsCommand::populateCrashIdentifyingValues) and add CrashIdentifyingValuesTest covering that volatile per-request fields (requestID, ID, lastIP, _source) are excluded while semantically meaningful fields are retained. Co-authored-by: Carlos Alvarez --- plugins/Jobs.cpp | 19 ++++-- plugins/Jobs.h | 6 ++ .../tests/jobs/CrashIdentifyingValuesTest.cpp | 68 +++++++++++++++++++ 3 files changed, 88 insertions(+), 5 deletions(-) create mode 100644 test/tests/jobs/CrashIdentifyingValuesTest.cpp diff --git a/plugins/Jobs.cpp b/plugins/Jobs.cpp index 5d3eab9d0..d2df9e80a 100644 --- a/plugins/Jobs.cpp +++ b/plugins/Jobs.cpp @@ -117,12 +117,12 @@ void BedrockPlugin_Jobs::upgradeDatabase(SQLite& db) } // ========================================================================== -bool BedrockJobsCommand::peek(SQLite& db) +void BedrockJobsCommand::populateCrashIdentifyingValues() { - const string& requestVerb = request.getVerb(); - - // Jobs commands can only crash if they look identical. - // We exclude unique request fields so blacklist entries can be matched + // We key the crash blacklist on the semantically meaningful request fields, but exclude + // volatile, per-request fields that are never identical across two requests (e.g. `requestID`). + // Including them would make the blacklist entry impossible to match, rendering the poison-pill + // protection a no-op. static const set volatileFields = {"requestID", "ID", "lastIP", "_source"}; for (const auto& name : request.nameValueMap) { if (volatileFields.count(name.first)) { @@ -130,6 +130,15 @@ bool BedrockJobsCommand::peek(SQLite& db) } crashIdentifyingValues.insert(name.first); } +} + +// ========================================================================== +bool BedrockJobsCommand::peek(SQLite& db) +{ + const string& requestVerb = request.getVerb(); + + // Jobs commands can only crash if they look identical. + 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..fb44bfedf --- /dev/null +++ b/test/tests/jobs/CrashIdentifyingValuesTest.cpp @@ -0,0 +1,68 @@ +#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. If a volatile, per-request field such as +// `requestID` is included, the entry can never match a future command (no two requests share a +// requestID), making the protection a no-op. These tests verify those volatile fields are excluded +// while the semantically meaningful fields are retained. +struct JobsCrashIdentifyingValuesTest : tpunit::TestFixture +{ + JobsCrashIdentifyingValuesTest() + : tpunit::TestFixture("JobsCrashIdentifyingValues", + TEST(JobsCrashIdentifyingValuesTest::excludesVolatileFields), + TEST(JobsCrashIdentifyingValuesTest::retainsAllFieldsWhenNoneAreVolatile)) + { + } + + // Volatile per-request fields are dropped; meaningful fields (and their values) are kept. + void excludesVolatileFields() + { + SData request("GetJob"); + request["name"] = "TestJob"; + request["data"] = "{\"key\":\"value\"}"; + request["jobID"] = "12345"; + request["requestID"] = "abc123"; + request["ID"] = "999"; + request["lastIP"] = "127.0.0.1"; + request["_source"] = "someSource"; + + BedrockJobsCommand command(SQLiteCommand(move(request)), nullptr); + command.populateCrashIdentifyingValues(); + + // Volatile per-request fields must be excluded so the blacklist entry can actually match a + // genuinely repeated command. + ASSERT_FALSE(command.crashIdentifyingValues.count("requestID")); + ASSERT_FALSE(command.crashIdentifyingValues.count("ID")); + ASSERT_FALSE(command.crashIdentifyingValues.count("lastIP")); + ASSERT_FALSE(command.crashIdentifyingValues.count("_source")); + + // Semantically meaningful fields must be retained, with their original values. + ASSERT_TRUE(command.crashIdentifyingValues.count("name")); + ASSERT_TRUE(command.crashIdentifyingValues.count("data")); + ASSERT_TRUE(command.crashIdentifyingValues.count("jobID")); + ASSERT_EQUAL(command.crashIdentifyingValues["name"], "TestJob"); + ASSERT_EQUAL(command.crashIdentifyingValues["data"], "{\"key\":\"value\"}"); + ASSERT_EQUAL(command.crashIdentifyingValues["jobID"], "12345"); + + // Only the three non-volatile fields should be present. + ASSERT_EQUAL(command.crashIdentifyingValues.size(), 3u); + } + + // A command with no volatile fields keeps every field. + void retainsAllFieldsWhenNoneAreVolatile() + { + SData request("CreateJob"); + request["name"] = "AnotherJob"; + request["priority"] = "500"; + + BedrockJobsCommand command(SQLiteCommand(move(request)), nullptr); + command.populateCrashIdentifyingValues(); + + ASSERT_EQUAL(command.crashIdentifyingValues.size(), 2u); + ASSERT_TRUE(command.crashIdentifyingValues.count("name")); + ASSERT_TRUE(command.crashIdentifyingValues.count("priority")); + } +} __JobsCrashIdentifyingValuesTest; From fc76b0dc7bd2736adbd9a5530d39265b1ca46e70 Mon Sep 17 00:00:00 2001 From: "Carlos Alvarez (via MelvinBot)" Date: Fri, 10 Jul 2026 17:17:28 +0000 Subject: [PATCH 4/4] Whitelist name and data for Jobs crash-command identifier Switch crashIdentifyingValues from a blacklist of volatile fields to an explicit whitelist of the semantically identifying fields (name, data). This makes BedrockServer::_wouldCrash's "more than one identifying set -> block everyone" safeguard behave correctly: the same command crashing twice now stays on a single blacklist entry (blocking only that command), while two genuinely different commands crashing trips the block-everyone path (we've already lost two nodes). A blacklist that left in non- identifying fields like jobID/priority would have wrongly tripped that safeguard on repeats of the same command. Co-authored-by: Carlos Alvarez --- plugins/Jobs.cpp | 29 ++++++---- .../tests/jobs/CrashIdentifyingValuesTest.cpp | 54 ++++++++++--------- 2 files changed, 49 insertions(+), 34 deletions(-) diff --git a/plugins/Jobs.cpp b/plugins/Jobs.cpp index d2df9e80a..e1e7c4bea 100644 --- a/plugins/Jobs.cpp +++ b/plugins/Jobs.cpp @@ -119,16 +119,25 @@ void BedrockPlugin_Jobs::upgradeDatabase(SQLite& db) // ========================================================================== void BedrockJobsCommand::populateCrashIdentifyingValues() { - // We key the crash blacklist on the semantically meaningful request fields, but exclude - // volatile, per-request fields that are never identical across two requests (e.g. `requestID`). - // Including them would make the blacklist entry impossible to match, rendering the poison-pill - // protection a no-op. - static const set volatileFields = {"requestID", "ID", "lastIP", "_source"}; - for (const auto& name : request.nameValueMap) { - if (volatileFields.count(name.first)) { - continue; - } - crashIdentifyingValues.insert(name.first); + // 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); } } diff --git a/test/tests/jobs/CrashIdentifyingValuesTest.cpp b/test/tests/jobs/CrashIdentifyingValuesTest.cpp index fb44bfedf..70ba57b7c 100644 --- a/test/tests/jobs/CrashIdentifyingValuesTest.cpp +++ b/test/tests/jobs/CrashIdentifyingValuesTest.cpp @@ -4,26 +4,31 @@ // 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. If a volatile, per-request field such as -// `requestID` is included, the entry can never match a future command (no two requests share a -// requestID), making the protection a no-op. These tests verify those volatile fields are excluded -// while the semantically meaningful fields are retained. +// 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::excludesVolatileFields), - TEST(JobsCrashIdentifyingValuesTest::retainsAllFieldsWhenNoneAreVolatile)) + TEST(JobsCrashIdentifyingValuesTest::onlyKeepsWhitelistedFields), + TEST(JobsCrashIdentifyingValuesTest::keepsOnlyPresentWhitelistedFields)) { } - // Volatile per-request fields are dropped; meaningful fields (and their values) are kept. - void excludesVolatileFields() + // 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"; @@ -32,27 +37,26 @@ struct JobsCrashIdentifyingValuesTest : tpunit::TestFixture BedrockJobsCommand command(SQLiteCommand(move(request)), nullptr); command.populateCrashIdentifyingValues(); - // Volatile per-request fields must be excluded so the blacklist entry can actually match a - // genuinely repeated command. - ASSERT_FALSE(command.crashIdentifyingValues.count("requestID")); - ASSERT_FALSE(command.crashIdentifyingValues.count("ID")); - ASSERT_FALSE(command.crashIdentifyingValues.count("lastIP")); - ASSERT_FALSE(command.crashIdentifyingValues.count("_source")); - - // Semantically meaningful fields must be retained, with their original values. + // 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_TRUE(command.crashIdentifyingValues.count("jobID")); ASSERT_EQUAL(command.crashIdentifyingValues["name"], "TestJob"); ASSERT_EQUAL(command.crashIdentifyingValues["data"], "{\"key\":\"value\"}"); - ASSERT_EQUAL(command.crashIdentifyingValues["jobID"], "12345"); - // Only the three non-volatile fields should be present. - ASSERT_EQUAL(command.crashIdentifyingValues.size(), 3u); + // 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 command with no volatile fields keeps every field. - void retainsAllFieldsWhenNoneAreVolatile() + // 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"; @@ -61,8 +65,10 @@ struct JobsCrashIdentifyingValuesTest : tpunit::TestFixture BedrockJobsCommand command(SQLiteCommand(move(request)), nullptr); command.populateCrashIdentifyingValues(); - ASSERT_EQUAL(command.crashIdentifyingValues.size(), 2u); + // `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_TRUE(command.crashIdentifyingValues.count("priority")); + ASSERT_FALSE(command.crashIdentifyingValues.count("data")); + ASSERT_FALSE(command.crashIdentifyingValues.count("priority")); } } __JobsCrashIdentifyingValuesTest;