From 8789f0be6fbcc2831739c94a7f3a02413e6a196a Mon Sep 17 00:00:00 2001 From: Mike Horgan Date: Tue, 5 May 2026 15:43:29 -0400 Subject: [PATCH] fix(s3): guard ETag and version-id reads against missing headers handleResponseHeaders unconditionally recorded the part ETag into the parent MPU task's context map (a ConcurrentHashMap, which rejects null values). On any non-2xx part-PUT response the ETag header is absent, producing a NullPointerException that surfaced as misleading "Premature channel closure" warnings and obscured the underlying server failure. The versioning branch had a parallel issue: a missing x-amz-version-id header caused the literal string "null" to be appended to the item name, compounding "~null~null~..." across retries. Both branches now skip mutation when the header is absent. Adds a regression test covering each case. --- .../coop/netty/http/s3/S3ResponseHandler.java | 31 +++++---- .../http/s3/S3ResponseHandlerMpuPartTest.java | 63 +++++++++++++++++++ 2 files changed, 83 insertions(+), 11 deletions(-) create mode 100644 engine/extensions/storage-drivers/implementations/s3/src/test/java/com/dell/spt/storage/driver/coop/netty/http/s3/S3ResponseHandlerMpuPartTest.java diff --git a/engine/extensions/storage-drivers/implementations/s3/src/main/java/com/dell/spt/storage/driver/coop/netty/http/s3/S3ResponseHandler.java b/engine/extensions/storage-drivers/implementations/s3/src/main/java/com/dell/spt/storage/driver/coop/netty/http/s3/S3ResponseHandler.java index 244b519..f4f29d6 100644 --- a/engine/extensions/storage-drivers/implementations/s3/src/main/java/com/dell/spt/storage/driver/coop/netty/http/s3/S3ResponseHandler.java +++ b/engine/extensions/storage-drivers/implementations/s3/src/main/java/com/dell/spt/storage/driver/coop/netty/http/s3/S3ResponseHandler.java @@ -53,22 +53,31 @@ public S3ResponseHandler(final S3StorageDriver driver, final boolean verif @Override protected final void handleResponseHeaders(final Channel channel, final O op, final HttpHeaders respHeaders) { if (op instanceof PartialDataOperation && OpType.CREATE.equals(op.type())) { - // Capture part ETags for MPU write — needed for CompleteMultipartUpload XML body - final PartialDataOperation subTask = (PartialDataOperation) op; + // Capture part ETags for MPU write — needed for CompleteMultipartUpload XML body. + // On error responses the server omits the ETag header; skip recording in that case + // (the part will be retried, and the contextData map rejects null values). final String eTag = respHeaders.get(HttpHeaderNames.ETAG); - final CompositeDataOperation mpuTask = subTask.parent(); - final int partNum = subTask.partNumber() + 1; - mpuTask.put(Integer.toString(partNum), eTag); - // Capture per-part checksum value if the server echoed one back - if (checksumHeader != null) { - final String checksumVal = respHeaders.get(checksumHeader); - if (checksumVal != null) { - mpuTask.put(S3Api.KEY_PART_CHECKSUM_PREFIX + partNum, checksumVal); + if (eTag != null) { + final PartialDataOperation subTask = (PartialDataOperation) op; + final CompositeDataOperation mpuTask = subTask.parent(); + final int partNum = subTask.partNumber() + 1; + mpuTask.put(Integer.toString(partNum), eTag); + // Capture per-part checksum value if the server echoed one back + if (checksumHeader != null) { + final String checksumVal = respHeaders.get(checksumHeader); + if (checksumVal != null) { + mpuTask.put(S3Api.KEY_PART_CHECKSUM_PREFIX + partNum, checksumVal); + } } } } if (versioningEnabled) { - op.item().name(op.item().name() + "~" + respHeaders.get("x-amz-version-id")); + // Skip on error responses (no x-amz-version-id header); otherwise the literal + // string "null" gets appended to the item name, compounding on each retry. + final String versionId = respHeaders.get("x-amz-version-id"); + if (versionId != null) { + op.item().name(op.item().name() + "~" + versionId); + } } } diff --git a/engine/extensions/storage-drivers/implementations/s3/src/test/java/com/dell/spt/storage/driver/coop/netty/http/s3/S3ResponseHandlerMpuPartTest.java b/engine/extensions/storage-drivers/implementations/s3/src/test/java/com/dell/spt/storage/driver/coop/netty/http/s3/S3ResponseHandlerMpuPartTest.java new file mode 100644 index 0000000..efb3994 --- /dev/null +++ b/engine/extensions/storage-drivers/implementations/s3/src/test/java/com/dell/spt/storage/driver/coop/netty/http/s3/S3ResponseHandlerMpuPartTest.java @@ -0,0 +1,63 @@ +package com.dell.spt.storage.driver.coop.netty.http.s3; + +import static org.junit.jupiter.api.Assertions.assertDoesNotThrow; +import static org.junit.jupiter.api.Assertions.assertEquals; + +import com.dell.spt.base.item.DataItemImpl; +import com.dell.spt.base.item.op.OpType; +import com.dell.spt.base.item.op.composite.data.CompositeDataOperationImpl; +import com.dell.spt.base.item.op.data.DataOperationImpl; +import com.dell.spt.base.item.op.partial.data.PartialDataOperationImpl; +import com.dell.spt.base.storage.Credential; + +import io.netty.handler.codec.http.DefaultHttpHeaders; +import io.netty.handler.codec.http.HttpHeaders; + +import org.junit.jupiter.api.Test; + +/** + * Regression tests covering response-header handling on error responses, where the + * server omits headers the success path would normally provide (ETag for MPU parts, + * x-amz-version-id when versioning is enabled). Both branches must tolerate the + * missing headers without throwing or silently corrupting operation state. + */ +final class S3ResponseHandlerMpuPartTest { + + private static final long PART_SIZE = 10L * 1024 * 1024; + private static final long ITEM_SIZE = 20L * 1024 * 1024; + + @Test + void handleResponseHeadersDoesNotThrowWhenPartResponseHasNoETag() { + final var handler = new S3ResponseHandler>( + null, false, false, null); + + final var partItem = new DataItemImpl(0L, PART_SIZE); + final var parentItem = new DataItemImpl(0L, ITEM_SIZE); + final var parent = new CompositeDataOperationImpl( + 0, OpType.CREATE, parentItem, null, "/streaming/20MB/63/", Credential.NONE, + null, 0, PART_SIZE); + final var partOp = new PartialDataOperationImpl( + 0, OpType.CREATE, partItem, null, "/streaming/20MB/63/", Credential.NONE, + 0, parent); + + final HttpHeaders emptyHeaders = new DefaultHttpHeaders(); + + assertDoesNotThrow(() -> handler.handleResponseHeaders(null, partOp, emptyHeaders)); + } + + @Test + void handleResponseHeadersDoesNotCorruptItemNameWhenVersionHeaderMissing() { + final var handler = new S3ResponseHandler>( + null, false, true, null); + + final var item = new DataItemImpl("foo", 0L, ITEM_SIZE); + final var op = new DataOperationImpl( + 0, OpType.CREATE, item, null, "/bucket/", Credential.NONE, null, 0); + + final HttpHeaders emptyHeaders = new DefaultHttpHeaders(); + + handler.handleResponseHeaders(null, op, emptyHeaders); + + assertEquals("foo", item.name()); + } +}