Implement failIfAlreadyExists in S3 repositories #133030

kvanerum · 2025-08-16T14:56:25Z

Today, S3-backed repositories ignore the failIfAlreadyExists flag and
may therefore overwrite a blob which already exists, potentially
corrupting a repository subject to concurrent writes, rather than
failing the second write.

AWS S3 now supports writes conditional on the non-existence of an object
via the If-None-Match: * HTTP header. This commit adjusts the
S3-backed repository implementation to respect the failIfAlreadyExists
flag using these conditional writes, eliminating the possibility of
overwriting blobs which should not be overwritten.

Relates #128565

elasticsearchmachine · 2025-08-18T09:18:04Z

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

DaveCTurner

Looks like a good approach to me. I left a few small comments.

DaveCTurner · 2025-08-18T10:34:55Z

...ework/src/main/java/org/elasticsearch/repositories/AbstractThirdPartyRepositoryTestCase.java

        }
    }

+    public void testFailIfAlreadyExists() throws IOException {


Excellent, so nice to see this test being added at last.

Could we make it slightly stronger and instead start by performing two writes concurrently, asserting that exactly one of them succeeds, and then follow up with the check that we can overwrite blobs?

I updated the test to include the changes you requested, but now it fails in HdfsRepositoryTests. Both writes succeed. I also tried using blobStore.writeBlobAtomic(...), without luck.
From what I can tell, TestingFs might not provide atomic renaming. Could you advise on how best to proceed?

Ohh HDFS how we love thee. I'd suggest suppressing the test thusly:

diff --git a/plugins/repository-hdfs/src/test/java/org/elasticsearch/repositories/hdfs/HdfsRepositoryTests.java b/plugins/repository-hdfs/src/test/java/org/elasticsearch/repositories/hdfs/HdfsRepositoryTests.java index 7961ca0257be..3d75d9915bf7 100644 --- a/plugins/repository-hdfs/src/test/java/org/elasticsearch/repositories/hdfs/HdfsRepositoryTests.java +++ b/plugins/repository-hdfs/src/test/java/org/elasticsearch/repositories/hdfs/HdfsRepositoryTests.java @@ -62,4 +62,9 @@ public class HdfsRepositoryTests extends AbstractThirdPartyRepositoryTestCase { assertThat(response.result().blobs(), equalTo(0L)); } } + + @Override + public void testFailIfAlreadyExists() { + // HDFS does not implement failIfAlreadyExists correctly + } }

DaveCTurner · 2025-08-18T10:37:55Z

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

-                            + "</Key>\n"
-                            + "</CompleteMultipartUploadResult>").getBytes(StandardCharsets.UTF_8);
+
+                        if (isProtectOverwrite(exchange) && blobs.containsKey(request.path())) {


Checking containsKey before the put is racy. I think we need a putIfAbsent if If-None-Match: * is specified, and therefore also a test which tries to catch a race here.

DaveCTurner · 2025-08-18T10:39:18Z

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

                // a copy request is a put request with an X-amz-copy-source header
                final var copySource = copySourceName(exchange);
-                if (copySource != null) {
+                if (isProtectOverwrite(exchange) && blobs.containsKey(request.path())) {


Likewise here, we need to use putIfAbsent to avoid the race on this path.

...third-party/src/test/java/org/elasticsearch/repositories/s3/S3RepositoryThirdPartyTests.java

DaveCTurner · 2025-08-18T10:42:57Z

...c/internalClusterTest/java/org/elasticsearch/repositories/s3/S3BlobStoreRepositoryTests.java

+            // initial write blob
+            writeBlob(container, blobName, new BytesArray(data), true);


This would be a good spot to check for potential races in S3HttpHandler, if we initially wrote two blobs concurrently and verified that exactly one of those writes succeeded.

Could we also sometimes write a much larger blob to trigger the multipart upload path?

DaveCTurner · 2025-08-18T11:00:12Z

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


    }

+    public void testPreventObjectOverwrite() {


Actually I'd like us to test for races here too. Could we concurrently issue one or two PutObject requests, and one or two CompleteMultipartUpload requests, and verify that exactly one of them succeeds? And ideally that doing a GetObject afterwards returns the contents of the object which succeeded?

… S3 repository test

…UploadRequest

… and concurrent write

…yExists

DaveCTurner

This PR solves #128565

Not quite, we also need to verify the failIfAlreadyExists behaviour in repo analysis before we can close this issue. But I'm ok to merge this PR to implement the flag even without the repo analysis changes.

I left a few small comments but structurally this all looks good now.

DaveCTurner · 2025-09-01T09:43:29Z

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

        final String destinationBlobName,
-        final long blobSize
+        final long blobSize,
+        final boolean failIfAlreadyExists


This parameter is always false (in production code anyway) and the true case isn't really tested - I'd rather we inlined it, passing the literal false to executeMultipart, instead.

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

DaveCTurner · 2025-09-01T09:52:00Z

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

-                        exchange.sendResponseHeaders(RestStatus.OK.getStatus(), response.length);
-                        exchange.getResponseBody().write(response);
+                        boolean preconditionFailed = false;
+                        if (isProtectOverwrite(exchange)) {


I don't think there's a way for this to be true with a request from Elasticsearch as it stands today, nor does it look like this is covered by unit tests, so this is effectively dead code. I'd rather leave this area alone for now, except perhaps to throw something (AssertionError would be fine) to document that If-None-Match: * isn't supported here if ever we change the callers in future.

Just to make sure I understand, are you suggesting I revert the changes to the request.isPutObjectRequest() case? That would also mean updating S3HttpHandlerTests.testPreventObjectOverwrite().

No, the PutObject request handling is doing what we want and is adequately tested. It's just the CopyObject API here where we're not actually using or even testing the If-None-Match: * option.

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

DaveCTurner · 2025-09-01T09:57:07Z

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

+
+        TestWriteTask(Consumer<TestWriteTask> consumer, Consumer<TestWriteTask> prepare) {
+            this(consumer);
+            prepare.accept(this);


Could we just call the prepare steps directly in the caller? It's kinda hard to follow the flow as written, it looks as if we're doing both the prepare and complete steps concurrently (and in the wrong order too!)

DaveCTurner · 2025-09-01T09:59:45Z

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

+        try (var executor = Executors.newVirtualThreadPerTaskExecutor()) {
+            tasks.forEach(task -> executor.submit(task.consumer));
+            executor.shutdown();
+            var done = executor.awaitTermination(1, TimeUnit.SECONDS);


1s is too short to be a reliable timeout in tests (we push our CI workers pretty hard sometimes):

Suggested change

var done = executor.awaitTermination(1, TimeUnit.SECONDS);

var done = executor.awaitTermination(SAFE_AWAIT_TIMEOUT.seconds(), TimeUnit.SECONDS);

DaveCTurner · 2025-09-01T10:04:39Z

...ework/src/main/java/org/elasticsearch/repositories/AbstractThirdPartyRepositoryTestCase.java

+            ex2 = e;
+        }
+
+        assertTrue("Exactly one of the writes must fail", ex1 != null ^ ex2 != null);


Technically it's that exactly one must succeed, it just happens that that's equivalent to exactly one failing when there's only two requests (and ^ is a little unfriendly to readers, I'd recommend !=)

Suggested change

assertTrue("Exactly one of the writes must fail", ex1 != null ^ ex2 != null);

assertTrue("Exactly one of the writes must succeed", (ex1 == null) != (ex2 == null));

…hods

…fAlreadyExists

…dition check logic

DaveCTurner

LGTM (assuming CI is happy, I'll take care of that) - thanks for your work on this @kvanerum

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

DaveCTurner · 2025-09-12T12:05:35Z

@elasticmachine test this please

Today, S3-backed repositories ignore the `failIfAlreadyExists` flag and may therefore overwrite a blob which already exists, potentially corrupting a repository subject to concurrent writes, rather than failing the second write. AWS S3 now supports writes conditional on the non-existence of an object via the `If-None-Match: *` HTTP header. This commit adjusts the S3-backed repository implementation to respect the `failIfAlreadyExists` flag using these conditional writes, eliminating the possibility of overwriting blobs which should not be overwritten. Relates elastic#128565

Implement failIfAlreadyExists in S3 repositories

f3c380d

elasticsearchmachine added needs:triage Requires assignment of a team area label external-contributor Pull request authored by a developer outside the Elasticsearch team v9.2.0 labels Aug 16, 2025

szybia added :Distributed Coordination/Snapshot/Restore Anything directly related to the `_snapshot/*` APIs and removed needs:triage Requires assignment of a team area label labels Aug 18, 2025

elasticsearchmachine added the Team:Distributed Coordination Meta label for Distributed Coordination team label Aug 18, 2025

DaveCTurner reviewed Aug 18, 2025

View reviewed changes

DaveCTurner self-assigned this Aug 18, 2025

DaveCTurner added the >enhancement label Aug 18, 2025

DaveCTurner reviewed Aug 18, 2025

View reviewed changes

kvanerum and others added 8 commits August 18, 2025 16:57

Test failIfAlreadyExists for concurrent writes

35d427c

Disable testFailIfAlreadyExists for HdfsRepositoryTests

3d54119

Fix race in S3HttpHandler overwrite protection

8e8f3e7

Replace failIfAlreadyExists variable with inline randomBoolean in…

3c2c833

… S3 repository test

Don't remove uploads when precondition failed on S3 completeMultipart…

c1974ad

…UploadRequest

Test failIfAlreadyExists in S3BlobStoreRepositoryTests with multipart…

4c479f4

… and concurrent write

Merge branch 'main' into s3-failIfAlreadyExists

a903309

Cleanup

ad360ca

kvanerum requested a review from DaveCTurner August 27, 2025 06:57

DaveCTurner added 2 commits September 1, 2025 10:34

Merge branch 'main' into 0reviews/2025/09/01/kvanerum/s3-failIfAlread…

cef778c

…yExists

Add changelog

6979fbe

DaveCTurner reviewed Sep 1, 2025

View reviewed changes

kvanerum added 6 commits September 3, 2025 20:25

Inline failIfAlreadyExists in executeMultipartCopy

9c860cd

Throw error on multiple If-None-Match headers in S3HttpHandler

b5c0088

Refactor S3HttpHandlerTests to simplify task creation with helper met…

75a91cf

…hods

Use constant for safe await timeout in S3HttpHandlerTests

c975a60

Improve readability in AbstractThirdPartyRepositoryTestCase.testFailI…

249e8bb

…fAlreadyExists

Simplify copy object logic in S3HttpHandler by removing unused precon…

884dcc8

…dition check logic

Merge branch 'main' into s3-failIfAlreadyExists

a839a47

kvanerum requested a review from DaveCTurner September 11, 2025 06:28

DaveCTurner approved these changes Sep 12, 2025

View reviewed changes

test/fixtures/s3-fixture/src/main/java/fixture/s3/S3HttpHandler.java Show resolved Hide resolved

Merge branch 'main' into s3-failIfAlreadyExists

419f4ab

DaveCTurner merged commit 025396b into elastic:main Sep 12, 2025
35 checks passed

This was referenced Sep 12, 2025

Verify failIfAlreadyExists behaviour in repository analyzer #134632

Open

Implement failIfAlreadyExists in S3 repositories #128565

Closed

DiannaHohensee mentioned this pull request Sep 12, 2025

[CI] S3BlobStoreRepositoryTests testFailIfAlreadyExists failing #134669

Closed

		// initial write blob
		writeBlob(container, blobName, new BytesArray(data), true);

	var done = executor.awaitTermination(1, TimeUnit.SECONDS);
	var done = executor.awaitTermination(SAFE_AWAIT_TIMEOUT.seconds(), TimeUnit.SECONDS);

	assertTrue("Exactly one of the writes must fail", ex1 != null ^ ex2 != null);
	assertTrue("Exactly one of the writes must succeed", (ex1 == null) != (ex2 == null));

Implement failIfAlreadyExists in S3 repositories #133030

Implement failIfAlreadyExists in S3 repositories #133030

Uh oh!

Conversation

kvanerum commented Aug 16, 2025 • edited by DaveCTurner Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

elasticsearchmachine commented Aug 18, 2025

Uh oh!

DaveCTurner 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DaveCTurner left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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!

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!

DaveCTurner left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

DaveCTurner commented Sep 12, 2025

Uh oh!

Uh oh!

Uh oh!

kvanerum commented Aug 16, 2025 •

edited by DaveCTurner

Loading