Skip to content

Fix S3 deletion bugs #129891

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 10 commits into
base: main
Choose a base branch
from

Conversation

DaveCTurner
Copy link
Contributor

Bug the first: S3BlobContainer#delete spuriously attempts to delete a
blob named after the container, as if it were a directory in a
filesystem. This makes sense for filesystem repositories but in S3 the
blob key is an opaque string, may be a prefix of other blob keys, and
can legitimately end in a /. We never create such a blob in the first
place, but this was hidden because AWS S3 silently ignores these
deletion requests, and also because...

Bug the second: S3HttpHandler would delete extant blobs but ignore
nonexistent blobs when processing a multi-object delete request. This is
apparently how S3 behaves in practice but it's not documented as such so
we cannot rely on it and must be stricter in our tests. Fixing this
exposed...

Bug the third: S3BlobContainer#deleteBlobsIgnoringIfNotExists wasn't
actually ignoring NoSuchKey errors should any arise, and the S3
reference documentation does not proscribe this behaviour, so we must
handle it properly.

Bug the first: `S3BlobContainer#delete` spuriously attempts to delete a
blob named after the container, as if it were a directory in a
filesystem. This makes sense for filesystem repositories but in S3 the
blob key is an opaque string, may be a prefix of other blob keys, and
can legitimately end in a `/`. We never create such a blob in the first
place, but this was hidden because AWS S3 silently ignores these
deletion requests, and also because...

Bug the second: `S3HttpHandler` would delete extant blobs but ignore
nonexistent blobs when processing a multi-object delete request. This is
apparently how S3 behaves in practice but it's not documented as such so
we cannot rely on it and must be stricter in our tests. Fixing this
exposed...

Bug the third: `S3BlobContainer#deleteBlobsIgnoringIfNotExists` wasn't
actually ignoring `NoSuchKey` errors should any arise, and the S3
reference documentation does not proscribe this behaviour, so we must
handle it properly.
@DaveCTurner DaveCTurner added >bug :Distributed Coordination/Snapshot/Restore Anything directly related to the `_snapshot/*` APIs v9.1.0 labels Jun 23, 2025
@elasticsearchmachine elasticsearchmachine added the Team:Distributed Coordination Meta label for Distributed Coordination team label Jun 23, 2025
@elasticsearchmachine
Copy link
Collaborator

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

@elasticsearchmachine
Copy link
Collaborator

Hi @DaveCTurner, I've created a changelog YAML for you.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
>bug :Distributed Coordination/Snapshot/Restore Anything directly related to the `_snapshot/*` APIs Team:Distributed Coordination Meta label for Distributed Coordination team v9.2.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants