Skip to content

Exclude volatile per-request fields from Jobs crash blacklist#2673

Draft
MelvinBot wants to merge 4 commits into
mainfrom
claude-jobsCrashBlacklistVolatileFields
Draft

Exclude volatile per-request fields from Jobs crash blacklist#2673
MelvinBot wants to merge 4 commits into
mainfrom
claude-jobsCrashBlacklistVolatileFields

Conversation

@MelvinBot

@MelvinBot MelvinBot commented Jul 8, 2026

Copy link
Copy Markdown
Contributor

Details

The Jobs plugin's crash-command blacklist ("poison-pill" protection) was effectively broken. In BedrockJobsCommand::peek, crashIdentifyingValues was populated with every request field:

for (const auto& name : request.nameValueMap) {
    crashIdentifyingValues.insert(name.first);
}

That set included requestID, which is unique for every request. Since BedrockServer::_wouldCrash requires an exact match on every stored key/value (skipping only Content-Length), a single blacklist entry could never match a future command — no two requests share a requestID.

This change keys crashIdentifyingValues on an explicit whitelist of the fields that semantically identify a Jobs command — name and data — instead of a blacklist of volatile fields. This is the correct approach given how BedrockServer::_wouldCrash handles multiple crashes of the same methodLine:

  • If a command crashes with more than one distinct set of identifying values, _wouldCrash assumes we've already lost more than one node and blocks the command for everyone.
  • With a whitelist of just name/data, the same command crashing twice records the same identifying set (one entry → only that command is blocked), while two genuinely different commands crashing records two sets → the block-everyone safeguard fires as intended.
  • A blacklist that left in non-identifying fields (e.g. jobID, priority) would have recorded a different set on each crash of the same command and wrongly tripped the block-everyone path.

Fixed Issues

https://github.com/Expensify/Expensify/issues/656214

Tests

  • Updated the in-process unit test test/tests/jobs/CrashIdentifyingValuesTest.cpp to assert only name and data (and their values) are kept and every other field is dropped, including present-but-non-identifying fields like jobID/priority.
  • just docker rebuild-cpp — Bedrock + Auth compile cleanly with -Wall -Werror.
  • ./test -only JobsCrashIdentifyingValues → Passed: 2, Failed: 0.
  • ./test -only CreateJob (server-based, exercises the peek() path) → Passed: 17, Failed: 0.

// TODO: The human co-author must fill out the tests you ran before marking this PR as "ready for review".


Internal Testing Reminder: when changing bedrock, please compile auth against your new changes

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 <cead22@users.noreply.github.com>
@MelvinBot
MelvinBot requested a review from a team July 8, 2026 13:16
Comment thread plugins/Jobs.cpp Outdated
cead22 and others added 2 commits July 8, 2026 10:18
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 <cead22@users.noreply.github.com>
@cead22
cead22 requested a review from danieldoglas July 8, 2026 23:16
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 <cead22@users.noreply.github.com>
Comment thread plugins/Jobs.cpp
for (const auto& name : request.nameValueMap) {
crashIdentifyingValues.insert(name.first);
}
populateCrashIdentifyingValues();

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of doing the AI thing where it adds 50 lines to make a 3 line change, I would simply change the loop removed here with:

crashIdentifyingValues.insert("name");
crashIdentifyingValues.insert("data");

No changes to the .h file, -3/+2 here.

@@ -0,0 +1,74 @@
#include <libstuff/SData.h>

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I find this entire test quite useless. It inserts two items in a map and then spends 74 lines asserting that the map contains those two items. We already know that maps work without this test class.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants