Skip to content

Log and meter S3 batch-delete request-body parse failures#3255

Merged
snalli merged 1 commit into
linkedin:masterfrom
snalli:snalli/log-s3-batch-delete-parse-error
May 11, 2026
Merged

Log and meter S3 batch-delete request-body parse failures#3255
snalli merged 1 commit into
linkedin:masterfrom
snalli:snalli/log-s3-batch-delete-parse-error

Conversation

@snalli
Copy link
Copy Markdown
Contributor

@snalli snalli commented May 6, 2026

JIRA: ACTIONITEM-16798

Summary

S3BatchDeleteHandler.parseRequestBodyAndDeleteCallback catches every
Exception thrown while validating the request body and maps it to a
generic MalformedRequestBody S3 error. The caught exception is never
logged, no metric is incremented, and the callback is invoked with a
null exception argument — so a genuine server-side bug (NPE, IO
failure, deserializer crash, etc.) is operationally indistinguishable
from a client sending malformed XML.

This change:

  • Adds FrontendMetrics.s3BatchDeleteRequestParseError (Counter).
  • In the catch block, increments the counter and logs the exception at
    WARN with logger.warn("Failed to parse S3 batch-delete request body, returning malformed-body error", e).
  • Leaves the S3 protocol response (200 OK with XML error body) unchanged
    — that's a protocol requirement.

Testing Done

  • S3BatchDeleteHandlerTest.malformedXMLRequestTest now also asserts
    the parse-error counter increments by exactly 1 when a malformed body
    fires the catch.
  • All 6 tests in S3BatchDeleteHandlerTest pass:
    ./gradlew :ambry-frontend:test --tests "com.github.ambry.frontend.S3BatchDeleteHandlerTest"
  • :ambry-frontend:compileJava and :ambry-frontend:compileTestJava are clean.

Durability risk analysis

No write/metadata/ordering paths touched. This is a pure observability
change in the request-validation path of an S3-compat batch delete
handler. The on-the-wire response is byte-for-byte identical to before.

🤖 Generated with Claude Code

The catch in S3BatchDeleteHandler.parseRequestBodyAndDeleteCallback
mapped every Exception to a generic MalformedRequestBody S3 error
without logging the cause or incrementing any metric. A genuine
server-side bug (NPE, IOException, etc.) was indistinguishable from a
client sending malformed XML.

Add FrontendMetrics.s3BatchDeleteRequestParseError counter, increment it
in the catch, and log the exception at WARN. The S3 protocol response
(200 OK with XML error body) is unchanged.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@codecov-commenter
Copy link
Copy Markdown

codecov-commenter commented May 6, 2026

Codecov Report

❌ Patch coverage is 50.00000% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 51.22%. Comparing base (52ba813) to head (803f947).
⚠️ Report is 384 commits behind head on master.

Files with missing lines Patch % Lines
...github/ambry/frontend/s3/S3BatchDeleteHandler.java 0.00% 2 Missing ⚠️
Additional details and impacted files
@@              Coverage Diff              @@
##             master    #3255       +/-   ##
=============================================
- Coverage     64.24%   51.22%   -13.03%     
+ Complexity    10398     8666     -1732     
=============================================
  Files           840      931       +91     
  Lines         71755    79486     +7731     
  Branches       8611     9515      +904     
=============================================
- Hits          46099    40716     -5383     
- Misses        23004    35380    +12376     
- Partials       2652     3390      +738     

☔ View full report in Codecov by Sentry.
📢 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.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@snalli snalli requested a review from satishkotha May 7, 2026 17:46
@snalli snalli merged commit 7273f9f into linkedin:master May 11, 2026
11 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.

3 participants