Skip to content

Commit 7273f9f

Browse files
snalliclaude
andauthored
Log and meter S3 batch-delete request-body parse failures (#3255)
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>
1 parent f3a50ff commit 7273f9f

3 files changed

Lines changed: 12 additions & 1 deletion

File tree

ambry-frontend/src/main/java/com/github/ambry/frontend/FrontendMetrics.java

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -265,6 +265,8 @@ public class FrontendMetrics {
265265
// GetReplicasHandler
266266
public final Counter invalidBlobIdError;
267267
public final Counter responseConstructionError;
268+
// S3BatchDeleteHandler
269+
public final Counter s3BatchDeleteRequestParseError;
268270
// Other
269271
// FrontendRestRequestService
270272
public final Histogram restRequestServiceStartupTimeInMs;
@@ -721,6 +723,9 @@ public FrontendMetrics(MetricRegistry metricRegistry, FrontendConfig frontendCon
721723
invalidBlobIdError = metricRegistry.counter(MetricRegistry.name(GetReplicasHandler.class, "InvalidBlobIdError"));
722724
responseConstructionError =
723725
metricRegistry.counter(MetricRegistry.name(GetReplicasHandler.class, "ResponseConstructionError"));
726+
// S3BatchDeleteHandler
727+
s3BatchDeleteRequestParseError =
728+
metricRegistry.counter(MetricRegistry.name(S3BatchDeleteHandler.class, "RequestParseError"));
724729

725730
// Other
726731
restRequestServiceStartupTimeInMs =

ambry-frontend/src/main/java/com/github/ambry/frontend/s3/S3BatchDeleteHandler.java

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -116,6 +116,8 @@ private Callback<Long> parseRequestBodyAndDeleteCallback(RetainingAsyncWritableC
116116
throw new RestServiceException(batchSizeErrorMessage, RestServiceErrorCode.BadRequest);
117117
}
118118
} catch (Exception e) {
119+
metrics.s3BatchDeleteRequestParseError.inc();
120+
logger.warn("Failed to parse S3 batch-delete request body, returning malformed-body error", e);
119121
S3MessagePayload.Error response = new S3MessagePayload.Error();
120122
response.setCode(ERR_MALFORMED_REQUEST_BODY_CODE);
121123
response.setMessage(ERR_MALFORMED_REQUEST_BODY_MESSAGE);

ambry-frontend/src/test/java/com/github/ambry/frontend/S3BatchDeleteHandlerTest.java

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -71,6 +71,7 @@ public class S3BatchDeleteHandlerTest {
7171
private final Account account;
7272
private final Container container;
7373
private FrontendConfig frontendConfig;
74+
private FrontendMetrics metrics;
7475
private NamedBlobPutHandler namedBlobPutHandler;
7576
private S3BatchDeleteHandler s3BatchDeleteHandler;
7677
private final NettyByteBufLeakHelper nettyByteBufLeakHelper = new NettyByteBufLeakHelper();
@@ -233,6 +234,7 @@ public void malformedXMLRequestTest() throws Exception {
233234
RestResponseChannel restResponseChannel = new MockRestResponseChannel();
234235
request.setArg(RestUtils.InternalKeys.REQUEST_PATH,
235236
RequestPath.parse(request, frontendConfig.pathPrefixesToRemove, CLUSTER_NAME));
237+
long parseErrorsBefore = metrics.s3BatchDeleteRequestParseError.getCount();
236238
FutureResult<ReadableStreamChannel> futureResult = new FutureResult<>();
237239
s3BatchDeleteHandler.handle(request, restResponseChannel, futureResult::done);
238240
ReadableStreamChannel readableStreamChannel = futureResult.get();
@@ -245,6 +247,8 @@ public void malformedXMLRequestTest() throws Exception {
245247
assertEquals(response.getMessage(), ERR_MALFORMED_REQUEST_BODY_MESSAGE);
246248
assertEquals(response.getCode(), ERR_MALFORMED_REQUEST_BODY_CODE);
247249
assertEquals("Mismatch on status", ResponseStatus.Ok, restResponseChannel.getStatus());
250+
assertEquals("Parse-error counter should increment by 1", parseErrorsBefore + 1,
251+
metrics.s3BatchDeleteRequestParseError.getCount());
248252
}
249253

250254
@Test
@@ -314,7 +318,7 @@ private void setup() throws Exception {
314318
CommonTestUtils.populateRequiredRouterProps(properties);
315319
VerifiableProperties verifiableProperties = new VerifiableProperties(properties);
316320
frontendConfig = new FrontendConfig(verifiableProperties);
317-
FrontendMetrics metrics = new FrontendMetrics(new MetricRegistry(), frontendConfig);
321+
metrics = new FrontendMetrics(new MetricRegistry(), frontendConfig);
318322
AccountAndContainerInjector injector = new AccountAndContainerInjector(ACCOUNT_SERVICE, metrics, frontendConfig);
319323
IdSigningService idSigningService = new AmbryIdSigningService();
320324
AmbrySecurityServiceFactory securityServiceFactory =

0 commit comments

Comments
 (0)