diff --git a/contentgrid-appserver-contentstore-api/src/main/java/com/contentgrid/appserver/contentstore/api/ContentStoreException.java b/contentgrid-appserver-contentstore-api/src/main/java/com/contentgrid/appserver/contentstore/api/ContentStoreException.java new file mode 100644 index 000000000..b5cfd1ab8 --- /dev/null +++ b/contentgrid-appserver-contentstore-api/src/main/java/com/contentgrid/appserver/contentstore/api/ContentStoreException.java @@ -0,0 +1,17 @@ +package com.contentgrid.appserver.contentstore.api; + +/** + * Unchecked exception wrapping a {@link ContentIOException} for propagation through + * layers that do not declare checked exceptions. + */ +public class ContentStoreException extends RuntimeException { + + public ContentStoreException(ContentIOException cause) { + super(cause); + } + + @Override + public ContentIOException getCause() { + return (ContentIOException) super.getCause(); + } +} diff --git a/contentgrid-appserver-contentstore-impl-s3/src/main/java/com/contentgrid/appserver/contentstore/impl/s3/S3ContentStore.java b/contentgrid-appserver-contentstore-impl-s3/src/main/java/com/contentgrid/appserver/contentstore/impl/s3/S3ContentStore.java index 52bb9815f..3effc0ad1 100644 --- a/contentgrid-appserver-contentstore-impl-s3/src/main/java/com/contentgrid/appserver/contentstore/impl/s3/S3ContentStore.java +++ b/contentgrid-appserver-contentstore-impl-s3/src/main/java/com/contentgrid/appserver/contentstore/impl/s3/S3ContentStore.java @@ -92,6 +92,7 @@ private static long contentSize(GetObjectResponse response) { } @Override + @SneakyThrows(InterruptedException.class) public ContentAccessor writeContent(@NonNull InputStream inputStream) throws UnwritableContentException { var contentReference = ContentReference.of(UUID.randomUUID().toString()); try { @@ -100,24 +101,25 @@ public ContentAccessor writeContent(@NonNull InputStream inputStream) throws Unw .object(contentReference.getValue()) .stream(inputStream, -1, PART_SIZE) .build()) - .join(); + .get(); return new S3ContentAccessor(contentReference); } catch (InsufficientDataException | InternalException | InvalidKeyException | IOException | - NoSuchAlgorithmException | XmlParserException e) { + NoSuchAlgorithmException | XmlParserException | ExecutionException e) { throw new UnwritableContentException(contentReference, e); } } @Override + @SneakyThrows(InterruptedException.class) public void remove(@NonNull ContentReference contentReference) throws UnwritableContentException { try { client.removeObject(RemoveObjectArgs.builder() .bucket(bucketName) .object(contentReference.getValue()) .build()) - .join(); + .get(); } catch (InsufficientDataException | InternalException | InvalidKeyException | IOException | - NoSuchAlgorithmException | XmlParserException e) { + NoSuchAlgorithmException | XmlParserException | ExecutionException e) { throw new UnwritableContentException(contentReference, e); } diff --git a/contentgrid-appserver-contentstore-impl-s3/src/test/java/com/contentgrid/appserver/contentstore/impl/s3/S3ContentStoreWriteFailureTest.java b/contentgrid-appserver-contentstore-impl-s3/src/test/java/com/contentgrid/appserver/contentstore/impl/s3/S3ContentStoreWriteFailureTest.java new file mode 100644 index 000000000..2787b2658 --- /dev/null +++ b/contentgrid-appserver-contentstore-impl-s3/src/test/java/com/contentgrid/appserver/contentstore/impl/s3/S3ContentStoreWriteFailureTest.java @@ -0,0 +1,125 @@ +package com.contentgrid.appserver.contentstore.impl.s3; + +import static org.junit.jupiter.api.Assertions.assertThrows; + +import com.contentgrid.appserver.contentstore.api.UnwritableContentException; +import io.minio.MakeBucketArgs; +import io.minio.MinioAsyncClient; +import io.minio.RemoveBucketArgs; +import java.io.ByteArrayInputStream; +import java.io.IOException; +import java.io.InputStream; +import java.nio.charset.StandardCharsets; +import java.util.UUID; +import java.util.concurrent.CompletionException; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; +import org.testcontainers.containers.MinIOContainer; +import org.testcontainers.junit.jupiter.Container; +import org.testcontainers.junit.jupiter.Testcontainers; + +/** + * Reproducer for content upload bug where S3 write failures escape as uncaught + * {@link CompletionException} instead of being wrapped as {@link UnwritableContentException}. + *

+ * When {@link S3ContentStore#writeContent} fails (e.g. broken pipe to S3), the + * {@code CompletionException} from {@code CompletableFuture.join()} is not caught. + * This causes the exception to propagate uncaught through the entire request processing chain, + * bypassing the content store error handling in ContentUploadAttributeMapper and preventing + * the database transaction from ever starting. The result is a PUT that appears to succeed + * (HTTP 200 with default status) but content metadata is never persisted, causing subsequent + * GET requests to return 404. + */ +@Testcontainers +class S3ContentStoreWriteFailureTest { + + @Container + private static final MinIOContainer minioContainer = new MinIOContainer("minio/minio:RELEASE.2025-07-23T15-54-02Z"); + + private MinioAsyncClient client; + + @BeforeEach + void setUp() { + client = MinioAsyncClient.builder() + .endpoint(minioContainer.getS3URL()) + .credentials(minioContainer.getUserName(), minioContainer.getPassword()) + .build(); + } + + /** + * When the S3 bucket does not exist, putObject fails and .join() throws CompletionException. + * S3ContentStore.writeContent() should catch this and wrap it as UnwritableContentException, + * but currently the CompletionException escapes uncaught. + */ + @Test + void writeContent_whenS3OperationFails_shouldThrowUnwritableContentException() { + var store = new S3ContentStore(client, "non-existent-bucket-" + UUID.randomUUID()); + + assertThrows(UnwritableContentException.class, () -> store.writeContent( + new ByteArrayInputStream("test data".getBytes(StandardCharsets.UTF_8)))); + } + + /** + * Simulates a broken pipe during content upload by using an InputStream that fails mid-read. + * This mirrors the production scenario where the connection to S3 breaks during upload. + * The resulting CompletionException (wrapping an IOException) should be caught and wrapped + * as UnwritableContentException. + */ + @Test + void writeContent_whenInputStreamFailsDuringUpload_shouldThrowUnwritableContentException() throws Exception { + var bucketName = "test-" + UUID.randomUUID(); + client.makeBucket(MakeBucketArgs.builder().bucket(bucketName).build()).join(); + + var store = new S3ContentStore(client, bucketName); + + var brokenInputStream = new InputStream() { + private int bytesRead = 0; + + @Override + public int read() throws IOException { + if (bytesRead++ > 100) { + throw new IOException("Broken pipe"); + } + return 'x'; + } + + @Override + public int read(byte[] b, int off, int len) throws IOException { + if (bytesRead > 100) { + throw new IOException("Broken pipe"); + } + int toRead = Math.min(len, 50); + for (int i = 0; i < toRead; i++) { + b[off + i] = 'x'; + } + bytesRead += toRead; + return toRead; + } + }; + + assertThrows(UnwritableContentException.class, () -> store.writeContent(brokenInputStream)); + } + + /** + * Verifies that remove() has the same problem: CompletionException from .join() + * is not caught when the bucket doesn't exist. + */ + @Test + void remove_whenS3OperationFails_shouldThrowUnwritableContentException() throws Exception { + var bucketName = "test-" + UUID.randomUUID(); + client.makeBucket(MakeBucketArgs.builder().bucket(bucketName).build()).join(); + + var store = new S3ContentStore(client, bucketName); + + // Write content, then delete the bucket to force the remove to fail + var accessor = store.writeContent( + new ByteArrayInputStream("test".getBytes(StandardCharsets.UTF_8))); + + // Remove the bucket's contents and the bucket itself to cause subsequent operations to fail + store.remove(accessor.getReference()); + client.removeBucket(RemoveBucketArgs.builder().bucket(bucketName).build()).join(); + + // Now the store references a non-existent bucket, so remove should fail with UnwritableContentException + assertThrows(UnwritableContentException.class, () -> store.remove(accessor.getReference())); + } +} diff --git a/contentgrid-appserver-domain/build.gradle b/contentgrid-appserver-domain/build.gradle index 47ca17e69..5ed0be7e6 100644 --- a/contentgrid-appserver-domain/build.gradle +++ b/contentgrid-appserver-domain/build.gradle @@ -11,7 +11,7 @@ dependencies { api 'com.fasterxml.jackson.core:jackson-core' implementation 'org.slf4j:slf4j-api' - implementation project(':contentgrid-appserver-contentstore-api') + api project(':contentgrid-appserver-contentstore-api') implementation 'org.springframework:spring-core' implementation 'com.contentgrid.hateoas:contentgrid-pagination-offset:0.0.4' diff --git a/contentgrid-appserver-domain/src/main/java/com/contentgrid/appserver/domain/data/mapper/ContentUploadAttributeMapper.java b/contentgrid-appserver-domain/src/main/java/com/contentgrid/appserver/domain/data/mapper/ContentUploadAttributeMapper.java index 5d36c86f8..fbe62e286 100644 --- a/contentgrid-appserver-domain/src/main/java/com/contentgrid/appserver/domain/data/mapper/ContentUploadAttributeMapper.java +++ b/contentgrid-appserver-domain/src/main/java/com/contentgrid/appserver/domain/data/mapper/ContentUploadAttributeMapper.java @@ -4,6 +4,7 @@ import com.contentgrid.appserver.application.model.attributes.ContentAttribute; import com.contentgrid.appserver.application.model.attributes.SimpleAttribute; import com.contentgrid.appserver.application.model.values.AttributePath; +import com.contentgrid.appserver.contentstore.api.ContentStoreException; import com.contentgrid.appserver.contentstore.api.ContentStore; import com.contentgrid.appserver.contentstore.api.UnwritableContentException; import com.contentgrid.appserver.domain.data.DataEntry; @@ -78,8 +79,10 @@ protected Optional mapCompositeAttributeUnsupportedDatatype(Attribute } return Optional.of(builder.build()); - } catch (UnwritableContentException|IOException e) { - throw new RuntimeException(e); + } catch (UnwritableContentException e) { + throw new ContentStoreException(e); + } catch (IOException e) { + throw new java.io.UncheckedIOException(e); } } diff --git a/contentgrid-appserver-rest/src/main/java/com/contentgrid/appserver/rest/problem/ProblemDetailsExceptionHandler.java b/contentgrid-appserver-rest/src/main/java/com/contentgrid/appserver/rest/problem/ProblemDetailsExceptionHandler.java index 27334bf08..69bf04cbd 100644 --- a/contentgrid-appserver-rest/src/main/java/com/contentgrid/appserver/rest/problem/ProblemDetailsExceptionHandler.java +++ b/contentgrid-appserver-rest/src/main/java/com/contentgrid/appserver/rest/problem/ProblemDetailsExceptionHandler.java @@ -1,5 +1,6 @@ package com.contentgrid.appserver.rest.problem; +import com.contentgrid.appserver.contentstore.api.ContentStoreException; import com.contentgrid.appserver.application.model.Application; import com.contentgrid.appserver.domain.data.InvalidDataFormatException; import com.contentgrid.appserver.domain.data.InvalidDataTypeException; @@ -390,6 +391,12 @@ ResponseEntity integrity(EntityLinkedByRequiredRelationException except ))); } + @ExceptionHandler + ResponseEntity contentStoreError(ContentStoreException exception) { + return createResponse(problemFactory.createProblem(ProblemType.INTERNAL_CONTENT_STORE) + .withStatus(HttpStatus.INTERNAL_SERVER_ERROR)); + } + @ExceptionHandler ResponseEntity permissionDenied(PermissionDeniedException exception) { return ResponseEntity.status(HttpStatus.FORBIDDEN).build(); diff --git a/contentgrid-appserver-rest/src/main/java/com/contentgrid/appserver/rest/problem/ProblemType.java b/contentgrid-appserver-rest/src/main/java/com/contentgrid/appserver/rest/problem/ProblemType.java index ff5b8c29e..897cd61ca 100644 --- a/contentgrid-appserver-rest/src/main/java/com/contentgrid/appserver/rest/problem/ProblemType.java +++ b/contentgrid-appserver-rest/src/main/java/com/contentgrid/appserver/rest/problem/ProblemType.java @@ -36,6 +36,8 @@ public enum ProblemType implements ProblemTypeResolvable { INTEGRITY_RELATION_BLIND_OVERWRITE("integrity", "blind-relation-overwrite"), INTEGRITY_REQUIRED_RELATION("integrity", "required-relation"), + + INTERNAL_CONTENT_STORE("internal", "content-store"), ; ProblemType(String... params) { diff --git a/contentgrid-appserver-rest/src/main/resources/com/contentgrid/appserver/rest/problem/messages.properties b/contentgrid-appserver-rest/src/main/resources/com/contentgrid/appserver/rest/problem/messages.properties index efcbdc7e0..1145d5593 100644 --- a/contentgrid-appserver-rest/src/main/resources/com/contentgrid/appserver/rest/problem/messages.properties +++ b/contentgrid-appserver-rest/src/main/resources/com/contentgrid/appserver/rest/problem/messages.properties @@ -68,3 +68,9 @@ com.contentgrid.appserver.rest.problem.ProblemType.title.integrity.blind-relatio com.contentgrid.appserver.rest.problem.ProblemType.detail.integrity.blind-relation-overwrite={1} is already referenced by {2}, it can not be referenced from {0} as well com.contentgrid.appserver.rest.problem.ProblemType.title.integrity.required-relation=Relation is required com.contentgrid.appserver.rest.problem.ProblemType.detail.integrity.required-relation={0} is required + +# Internal +com.contentgrid.appserver.rest.problem.ProblemType.title.internal=Internal server error +com.contentgrid.appserver.rest.problem.ProblemType.detail.internal= +com.contentgrid.appserver.rest.problem.ProblemType.title.internal.content-store=Content store error +com.contentgrid.appserver.rest.problem.ProblemType.detail.internal.content-store= diff --git a/contentgrid-appserver-rest/src/test/java/com/contentgrid/appserver/rest/property/ContentRestControllerTest.java b/contentgrid-appserver-rest/src/test/java/com/contentgrid/appserver/rest/property/ContentRestControllerTest.java index f413cf7c7..66b9b5d90 100644 --- a/contentgrid-appserver-rest/src/test/java/com/contentgrid/appserver/rest/property/ContentRestControllerTest.java +++ b/contentgrid-appserver-rest/src/test/java/com/contentgrid/appserver/rest/property/ContentRestControllerTest.java @@ -24,6 +24,8 @@ import java.io.StringReader; import java.nio.charset.StandardCharsets; import java.util.UUID; +import com.contentgrid.appserver.contentstore.api.ContentReference; +import com.contentgrid.appserver.contentstore.api.UnwritableContentException; import java.util.stream.Stream; import org.apache.tomcat.util.http.parser.ContentRange; import org.junit.jupiter.api.AfterEach; @@ -954,6 +956,69 @@ void upload_multipart_empty_file_success(HttpMethod method) throws Exception { .andExpect(content().bytes(new byte[0])); } + /** + * Reproducer: when the content store write fails with a CompletionException + * (e.g. broken pipe to S3), the exception escapes uncaught through S3ContentStore, + * ContentUploadAttributeMapper, and the controller. The response should be a server + * error (5xx), but because the CompletionException bypasses all catch blocks, the + * response status depends on unspecified error handling behavior. + * + * Additionally, the content must not be persisted in the database — a subsequent GET + * should return 404. + */ + @ParameterizedTest + @CsvSource({"PUT", "POST"}) + void upload_contentStoreWriteFails_returnsServerError(HttpMethod method) throws Exception { + String invoiceId = createInvoice(null); + + // Simulate S3 broken pipe: S3ContentStore catches the CompletionException from + // MinioAsyncClient.putObject().get() and wraps it as UnwritableContentException + Mockito.doThrow(new UnwritableContentException( + ContentReference.of("test"), new java.io.IOException("Broken pipe"))) + .when(contentStoreSpy).writeContent(Mockito.any()); + + mockMvc.perform(request(method, "/invoices/{instanceId}/content", invoiceId) + .contentType(INVOICE_CONTENT_FILE.getContentType()) + .content(INVOICE_CONTENT_FILE.getBytes())) + .andExpect(status().is5xxServerError()); + + Mockito.reset(contentStoreSpy); + + // Content must not have been persisted + mockMvc.perform(get("/invoices/{instanceId}/content", invoiceId)) + .andExpect(status().isNotFound()); + } + + /** + * Same as above but with multipart upload, which is the exact path triggered + * by the Python client in production. + */ + @Test + void upload_multipart_contentStoreWriteFails_returnsServerError() throws Exception { + String invoiceId = createInvoice(null); + + Mockito.doThrow(new UnwritableContentException( + ContentReference.of("test"), new java.io.IOException("Broken pipe"))) + .when(contentStoreSpy).writeContent(Mockito.any()); + + MockMultipartFile file = new MockMultipartFile( + "file", + INVOICE_CONTENT_FILE.getOriginalFilename(), + INVOICE_CONTENT_FILE.getContentType(), + INVOICE_CONTENT_FILE.getBytes() + ); + + mockMvc.perform(multipart("/invoices/{instanceId}/content", invoiceId) + .file(file)) + .andExpect(status().is5xxServerError()); + + Mockito.reset(contentStoreSpy); + + // Content must not have been persisted + mockMvc.perform(get("/invoices/{instanceId}/content", invoiceId)) + .andExpect(status().isNotFound()); + } + @Test void upload_partial_put_not_supported() throws Exception { String invoiceId = createInvoice(INVOICE_CONTENT_FILE);