Add full conditional write support to S3 test fixture #138542

DaveCTurner · 2025-11-24T20:10:08Z

In #133030 we added limited support for conditional writes in
S3HttpHandler, allowing callers to prevent overwriting an existing
blob with an If-None-Match: * precondition header. This commit extends
the implementation to include support for the If-Match: <etag>
precondition header allowing callers to perform atomic compare-and-set
operations which overwrite existing objects.

In elastic#133030 we added limited support for conditional writes in `S3HttpHandler`, allowing callers to prevent overwriting an existing blob with an `If-None-Match: *` precondition header. This commit extends the implementation to include support for the `If-Match: <etag>` precondition header allowing callers to perform atomic compare-and-set operations which overwrite existing objects.

elasticsearchmachine · 2025-11-24T20:10:33Z

Pinging @elastic/es-distributed-coordination (Team:Distributed Coordination)

mhl-b · 2025-11-24T22:41:37Z

test/fixtures/s3-fixture/src/main/java/fixture/s3/S3HttpHandler.java

+        if (requireExistingETag != null) {
+            final var success = new AtomicBoolean(true);
+            blobs.compute(path, (ignoredPath, existingContents) -> {
+                if (existingContents != null && requireExistingETag.equals(getEtagFromContents(existingContents))) {


If there is no existing object and it's If-Match it should return 404. I assume in this case it would return 412 precondition failed.

https://docs.aws.amazon.com/AmazonS3/latest/userguide/conditional-writes.html#conditional-error-response

If there's no current object version with the same name, or if the current object version is a delete marker, the operation fails with a 404 Not Found error.

Maybe a test for If-Match for non-existing object.

Ah thanks, well spotted. Bit of an odd choice tbh but I'll try and find a way to match that behaviour.

joshua-adams-1 · 2025-11-25T11:04:59Z

modules/repository-s3/src/main/java/org/elasticsearch/repositories/s3/S3BlobContainer.java

+                        if (e instanceof SdkServiceException sdkServiceException
+                            && sdkServiceException.statusCode() == RestStatus.NOT_FOUND.getStatus()) {
+                            // NOT_FOUND is what we wanted
+                            logger.atDebug()


I think there is a change of behaviour. CMIIW, but previously if abortMultiPartUpload threw a SdkServiceException then the exception would propagate upwards and get caught in the outer catch block here, and then throw a NoSuchFileException to the user. Now, since we're swallowing the exception, this isn't true.

If this is an intentional change in behaviour then I think it worthy enough for a unit test (as we don't want this code to change in future and then start returning an exception were previously there was none)

That's correct. This is covered by tests already in this PR - it's the reason why 5a9eace failed.

I'll open a separate PR to address this, it's somewhat orthogonal to the point of this PR.

Ok see #138569

joshua-adams-1 · 2025-11-25T11:13:17Z

modules/repository-s3/src/main/java/org/elasticsearch/repositories/s3/S3BlobContainer.java

+                                .log("multipart upload of [{}] with ID [{}] not found on abort", blobName, uploadId);
+                        } else {
+                            // aborting the upload on failure is a best-effort cleanup step - if it fails then we must just move on
+                            logger.atWarn()


TIL that there was an alternate logging format!

joshua-adams-1 · 2025-11-25T11:28:42Z

test/fixtures/s3-fixture/src/test/java/fixture/s3/S3HttpHandlerTests.java

            if (task.status == RestStatus.PRECONDITION_FAILED) {
                assertNotNull(handler.getUpload(task.uploadId));
            } else {
+                assertThat(task.status, oneOf(RestStatus.OK, RestStatus.CONFLICT));


Technically, this test is only checking that of the four requests, exactly one succeeds, but it doesn't assert on the order we expect the rest statuses to be returned.

List<TestWriteTask> successfulTasks = tasks.stream().filter(task -> task.status == RestStatus.OK).toList(); assertThat(successfulTasks, hasSize(1));

passes if the first three requests fail and the fourth succeeds, and then again

assertThat(task.status, oneOf(RestStatus.OK, RestStatus.CONFLICT));

is indifferent to the order.

Is there value in making stronger assertions about when we expect the RestStatus to be OK versus CONFLICT? Ie, we put an object that doesn't exist, then expect OK, then put the same object again N times each expecting CONFLICT?

We're running the requests in parallel and have no expectations about which one of them might win the race to succeed. We just need to know that the other 3 fail.

joshua-adams-1

LGTM but best to wait for @mhl-b to approve too in case I've missed something

Now that #138569 is merged this is simpler, but at least different enough to deserve another look.

mhl-b · 2025-11-25T21:18:51Z

test/fixtures/s3-fixture/src/main/java/fixture/s3/S3HttpHandler.java

+        final var iterator = ifMatch.iterator();
+        if (iterator.hasNext()) {
+            final var result = iterator.next();
+            if (iterator.hasNext() == false) {
+                return result;
+            }
+        }


nit: I think it should be exactly-one-item, not last-item.

mhl-b · 2025-11-25T21:24:37Z

test/fixtures/s3-fixture/src/main/java/fixture/s3/S3HttpHandler.java

+                responseCode.set(
+                    ESTestCase.randomFrom(
+                        existingContents == null ? RestStatus.NOT_FOUND : RestStatus.PRECONDITION_FAILED,
+                        RestStatus.CONFLICT
+                    )
+                );


Why do you need randomization for the conflict? Is it because we synchronize on blobs and cant detect conflicting uploads, i.e. handler linearize writes?

The AWS docs are not 100% clear on exactly what conditions lead to a 409 here, but it doesn't really matter from our point of view anyway: we just need to make sure we handle both 409 and 412s as acceptable failures.

mhl-b

LGTM

In elastic#133030 we added limited support for conditional writes in `S3HttpHandler`, allowing callers to prevent overwriting an existing blob with an `If-None-Match: *` precondition header. This commit extends the implementation to include support for the `If-Match: <etag>` precondition header allowing callers to perform atomic compare-and-set operations which overwrite existing objects.

DaveCTurner requested review from joshua-adams-1 and mhl-b November 24, 2025 20:10

DaveCTurner added >test Issues or PRs that are addressing/adding tests :Distributed Coordination/Snapshot/Restore Anything directly related to the `_snapshot/*` APIs v9.3.0 labels Nov 24, 2025

elasticsearchmachine added the Team:Distributed Coordination Meta label for Distributed Coordination team label Nov 24, 2025

mhl-b reviewed Nov 24, 2025

View reviewed changes

DaveCTurner added 4 commits November 25, 2025 08:26

Merge branch 'main' into 2025/11/24/s3-fixture-conditional-writes

8b14e66

Rework to return 404 and 409 too

3b4477f

Fix comment

5a9eace

Accept 404 for abort-on-cleanup response

86363d1

joshua-adams-1 reviewed Nov 25, 2025

View reviewed changes

Merge branch 'main' into 2025/11/24/s3-fixture-conditional-writes

185a32e

joshua-adams-1 previously approved these changes Nov 25, 2025

View reviewed changes

joshua-adams-1 mentioned this pull request Nov 25, 2025

Ignore abort-on-cleanup failure in S3 repo #138569

Merged

Merge branch 'main' into 2025/11/24/s3-fixture-conditional-writes

0030552

DaveCTurner requested review from joshua-adams-1 and mhl-b November 25, 2025 15:15

Merge branch 'main' into 2025/11/24/s3-fixture-conditional-writes

5c0542c

mhl-b reviewed Nov 25, 2025

View reviewed changes

mhl-b approved these changes Nov 26, 2025

View reviewed changes

DaveCTurner merged commit 594a373 into elastic:main Nov 26, 2025
34 checks passed

Add full conditional write support to S3 test fixture #138542

Add full conditional write support to S3 test fixture #138542

Conversation

DaveCTurner commented Nov 24, 2025

Uh oh!

elasticsearchmachine commented Nov 24, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

joshua-adams-1 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mhl-b left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants