Skip to content

Fix fingerprint processor failures during concurrent bulk request - #23027

Merged
cwperks merged 1 commit into
opensearch-project:mainfrom
tanyabti:fingerprint-bug-fix
Sep 16, 2026
Merged

cwperks merged 1 commit into
opensearch-project:mainfrom
tanyabti:fingerprint-bug-fix

Conversation

@tanyabti

@tanyabti tanyabti commented Sep 14, 2026

Copy link
Copy Markdown
Contributor

Description

The fingerprint ingest processor failed under concurrent bulk ingestion, throwing ArrayIndexOutOfBoundsException from the native crypto provider and, once triggered, breaking all fingerprint hashing on the affected node until it was replaced.

Cause: TheHashMethod enum cached a single MessageDigest instance per algorithm and reused the same one for every request. MessageDigest is not thread-safe, so when multiple ingest threads used that shared instance at once, they corrupt its internal state.

Fix: Store a supplier per algorithm and resolve a fresh per-call (ThreadLocal-backed) digest on each execute(). Now each call gets its own fresh (thread-local) MessageDigest, so threads no longer share one. Added a test that runs the processor from many threads at once and checks the output stays correct with no errors.

Related Issues

Resolves #[Issue number to be closed when this PR is merged]

Check List

  • Functionality includes testing.
  • API changes companion pull request created, if applicable.
  • Public documentation issue/PR created, if applicable.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

Signed-off-by: tanyabti <tanyabti@amazon.com>
@tanyabti
tanyabti requested a review from a team as a code owner September 14, 2026 08:39
@github-actions

Copy link
Copy Markdown
Contributor

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

🧪 PR contains tests
🔒 No security concerns identified
✅ No TODO sections
🔀 No multiple PR themes
⚡ No major issues detected

@github-actions

Copy link
Copy Markdown
Contributor

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
General
Preserve stack traces from worker failures

If a thread throws (e.g. ArrayIndexOutOfBoundsException from a corrupted digest),
the loop aborts but the test still passes as long as mismatches is empty and
failures gets populated — however, the assertion on failures uses equalTo(List.of())
which will surface it. Still, consider also asserting non-empty progress (e.g. that
the loop actually ran) and rethrowing the first Throwable with its cause preserved
so the stack trace is visible in test output rather than just its toString().

modules/ingest-common/src/test/java/org/opensearch/ingest/common/FingerprintProcessorTests.java [203-213]

 for (int i = 0; i < iterationsPerThread; i++) {
     IngestDocument doc = new IngestDocument(
         new java.util.HashMap<>(Map.of(fieldName, fieldValue)),
         new java.util.HashMap<>()
     );
     processor.execute(doc);
     String actual = doc.getFieldValue(targetField, String.class);
     if (expectedFingerprint.equals(actual) == false) {
         mismatches.add(actual);
     }
 }
+if (failures.isEmpty() == false) {
+    throw new AssertionError("thread failed", failures.get(0));
+}
Suggestion importance[1-10]: 3

__

Why: Minor test-diagnostic improvement; the existing assertThat on failures will still surface the failure via toString(), so the added rethrow only marginally improves debugging output.

Low

@github-actions

Copy link
Copy Markdown
Contributor

✅ Gradle check result for 9d09030: SUCCESS

@codecov

codecov Bot commented Sep 14, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 71.76%. Comparing base (0249cde) to head (9d09030).
⚠️ Report is 7 commits behind head on main.

Additional details and impacted files
@@             Coverage Diff              @@
##               main   #23027      +/-   ##
============================================
+ Coverage     71.67%   71.76%   +0.09%     
- Complexity    77559    77640      +81     
============================================
  Files          6168     6168              
  Lines        360105   360105              
  Branches      52380    52380              
============================================
+ Hits         258095   258421     +326     
+ Misses        81495    81181     -314     
+ Partials      20515    20503      -12     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@cwperks
cwperks merged commit 475fcf2 into opensearch-project:main Sep 16, 2026
17 checks passed
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.

2 participants