Ensure deletion of snapshot clone propagate state changes by ywangd · Pull Request #142192 · elastic/elasticsearch

ywangd · 2026-02-10T07:17:29Z

Previously, when a clone is running with additional snapshots queued behind it, deleting the clone can leave the snapshots queued without starting the first queued. With this PR, we ensure the state is propogated correctly in such case so that the next inline snapshot or clone can run.

The PR also updates the stress tests to excercise aborting running clones as well as deleting completed ones.

…etion-and-clone

elasticsearchmachine · 2026-02-10T07:17:54Z

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

elasticsearchmachine · 2026-02-10T07:17:54Z

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

ywangd · 2026-02-10T07:18:19Z

+                                            cloneName
+                                        );
+                                        if (abortSnapshot == false) {
+                                            snapshots.put(cloneName, new TrackedSnapshot(trackedRepository, cloneName));


Add clone to the list of completed snapshots so that it can deleted, restored and cloned again.

ywangd · 2026-02-10T07:29:34Z

+                        // The clone is running, submit a request to master to FAIL it.
+                        // It may race with SUCCESS. That's OK, we can take either final state.
+                        statusUpdater.sendUpdate(
+                            snapshot,
+                            null,
+                            shardEntry.getKey(),
+                            new ShardSnapshotStatus(nodeId, ShardState.FAILED, status.generation(), "aborted by snapshot deletion"),
+                            ActionListener.noop()
+                        );


I chose to not update the state here. So that it will remain as INIT and get updated to FAILED by the update request. The alternative is to update it to ABORTED here as well as sending the update. I didn't do it because (1) A racing SUCCESS update can change ABORTED to SUCESS which is a bit strange. We can avoid it in applyShardSnapshotUpdate but it's extra code. (2) We don't use ABORTED for clones anywhere else. It was not necessary since it runs on the master. So in summary, I decided to keep the state unchanged here which feels overall simpler.

I think we should be working towards making clones behave more similarly to regular snapshots wherever possible, and this moves us in the opposite direction. We can do away with a fair amount of this special-case handling if the master-as-cluster-state-custodian interacted with the master-as-shard-level-cloner more like how it interacts with data nodes taking shard-level snapshots.

To that end I'd prefer we used the ABORTED state "properly" here. FAILED generally means that nothing is writing to this location in the repository any more (ignoring nodes which left the cluster) but here AIUI we might mark the shard as FAILED even while its clone operation is in progress. In particular I think we could also fix this bug by marking the shard as ABORTED here and then teaching the shard-level cloner to react to this properly and move to FAILED (or SUCCESS) when its work is done.

Moving from ABORTED to SUCCESS is legitimate, we have that transition on non-clone snapshots too. Possibly that deserves a comment somewhere.

I pushed 3c4f215 based on your suggestion. Does it match what you have in mind? We still need one if/else to handle the clone entry since it uses repoShardId instead of shardId.

Moving from ABORTED to SUCCESS is legitimate

Indeed, I missed it. The method name IndexShardSnapshotStatus#abortIfNotCompleted already indicates that.

DaveCTurner

Thanks Yang, nice catch. I'm not sure this is quite how I'd want to fix this tho - see comments.

DaveCTurner · 2026-02-10T11:02:32Z

        }

+        private Runnable createAbortRunnable(boolean abortSnapshot, TrackedRepository trackedRepository, String snapshotName) {
+            final boolean isClone = snapshotName.contains("-clone-");


I don't really like this, but it seems it's only there for slightly more refined logs. Yet the log messages contain the snapshot name anyway. I think I'd rather stick with snapshot throughout (a clone is a special kind of snapshot) instead of doing this string-matching.

Sure I removed it. Added it originally because the logs in startCloner do spell out clone.

DaveCTurner · 2026-02-10T11:09:37Z

+        private Runnable createAbortRunnable(boolean abortSnapshot, TrackedRepository trackedRepository, String snapshotName) {
+            final boolean isClone = snapshotName.contains("-clone-");
+            final Runnable abortRunnable;
+            if (abortSnapshot) {


Possible bikeshedding but also not a fan of this. createAbortRunnable sounds like it will create a runnable which aborts the snapshot. Possibly renaming it to maybeCreateAbortRunnable is enough? Or move this branch back out to the callers?

If we do keep it here, could we have a constant NO_OP_ABORT_RUNNABLE = () -> {} and check for equality with that rather than having a separate bool?

Yeah good point. I renamed the method and used a noop runnable for the check.

DaveCTurner · 2026-02-10T11:15:26Z

+                                public void onFailure(Exception e) {
+                                    final Throwable cause = ExceptionsHelper.unwrapCause(e);
+                                    if (cause instanceof SnapshotException
+                                        && Regex.simpleMatch("*" + cloneName + "*Snapshot was aborted by deletion", cause.getMessage())) {


Bleurgh why is this reported specially? 🤦

Could we use org.elasticsearch.cluster.SnapshotsInProgress#ABORTED_FAILURE_TEXT rather than just the literal message?

Can we also check abortSnapshot in this if condition?

I used ABORTED_FAILURE_TEXT as suggested. Also add check for the abort runnable.

why is this reported specially

Not sure I follow. Do you mean we should remove the message match or something else?

DaveCTurner · 2026-02-10T11:28:56Z

+                        // The clone is running, submit a request to master to FAIL it.
+                        // It may race with SUCCESS. That's OK, we can take either final state.
+                        statusUpdater.sendUpdate(
+                            snapshot,
+                            null,
+                            shardEntry.getKey(),
+                            new ShardSnapshotStatus(nodeId, ShardState.FAILED, status.generation(), "aborted by snapshot deletion"),
+                            ActionListener.noop()
+                        );


I think we should be working towards making clones behave more similarly to regular snapshots wherever possible, and this moves us in the opposite direction. We can do away with a fair amount of this special-case handling if the master-as-cluster-state-custodian interacted with the master-as-shard-level-cloner more like how it interacts with data nodes taking shard-level snapshots.

To that end I'd prefer we used the ABORTED state "properly" here. FAILED generally means that nothing is writing to this location in the repository any more (ignoring nodes which left the cluster) but here AIUI we might mark the shard as FAILED even while its clone operation is in progress. In particular I think we could also fix this bug by marking the shard as ABORTED here and then teaching the shard-level cloner to react to this properly and move to FAILED (or SUCCESS) when its work is done.

Moving from ABORTED to SUCCESS is legitimate, we have that transition on non-clone snapshots too. Possibly that deserves a comment somewhere.

Co-authored-by: David Turner <david.turner@elastic.co>

…etion-and-clone

DaveCTurner

LGTM, one nit

DaveCTurner · 2026-02-16T19:11:04Z

+            }
+            return Entry.createClone(
+                snapshot,
+                completed(clonesBuilder.values()) ? (hasFailures(clonesBuilder) ? State.FAILED : State.SUCCESS) : State.ABORTED,


Do we need to check hasFailures here or could we just say State.FAILED in the completed branch? AIUI State.SUCCESS means we finalize the clone and then delete it, whereas State.FAILED means we skip finalization, which will leak data on any shards that did succeed. We need to be ok with this kind of leak (it gets cleaned up by subsequent deletes) so I think we could make this slightly less branchy here.

(yet another deviation in behaviour, we should finalize either way so that the follow-up delete does the cleanup, but again no action to take here, just a comment)

could we just say State.FAILED in the completed branch

Yeah we can do that for simplicity. See a143e80

we should finalize either way

I didn't notice the difference behaviour in finalization. Thanks! Yeah consistency would be better.

…etion-and-clone

ywangd added 6 commits February 5, 2026 14:19

wip

52d08b9

Merge remote-tracking branch 'origin/main' into bugs-for-snapshot-del…

08e5ce1

…etion-and-clone

wip

ac6fad8

add abort clone

b32e582

interface

26970b4

remove not-needed test

16ae2e1

ywangd requested a review from DaveCTurner February 10, 2026 07:17

ywangd added >bug :Distributed/Snapshot/Restore Anything directly related to the `_snapshot/*` APIs v9.4.0 labels Feb 10, 2026

elasticsearchmachine added the Team:Distributed Meta label for distributed team. label Feb 10, 2026

Update docs/changelog/142192.yaml

56bcd20

ywangd commented Feb 10, 2026

View reviewed changes

Comment thread server/src/main/java/org/elasticsearch/cluster/SnapshotsInProgress.java

Merge branch 'main' into bugs-for-snapshot-deletion-and-clone

354868e

ywangd commented Feb 10, 2026

View reviewed changes

DaveCTurner changed the title ~~Ensure deletion of snapshot clone propogate state changes~~ Ensure deletion of snapshot clone propagate state changes Feb 10, 2026

DaveCTurner reviewed Feb 10, 2026

View reviewed changes

ywangd and others added 7 commits February 11, 2026 20:15

Update docs/changelog/142192.yaml

5c4a5f8

Co-authored-by: David Turner <david.turner@elastic.co>

remove isClone

38ce2a3

noop runnable

d471e0a

abort text

ad3a8d0

fix check for noop

0f88312

aborted then failed

3c4f215

Merge remote-tracking branch 'origin/main' into bugs-for-snapshot-del…

3c13158

…etion-and-clone

ywangd requested a review from DaveCTurner February 11, 2026 10:22

ywangd added 2 commits February 11, 2026 23:23

fix test

c2cf710

Merge branch 'main' into bugs-for-snapshot-deletion-and-clone

79c35d7

DaveCTurner approved these changes Feb 16, 2026

View reviewed changes

ywangd added 3 commits February 18, 2026 11:14

Merge remote-tracking branch 'origin/main' into bugs-for-snapshot-del…

7e1ca8a

…etion-and-clone

feedback

a143e80

tweak assertion

fced14d

ywangd added the auto-merge-without-approval Automatically merge pull request when CI checks pass (NB doesn't wait for reviews!) label Feb 18, 2026

elasticsearchmachine merged commit 0ec7dda into elastic:main Feb 18, 2026
35 checks passed

ywangd deleted the bugs-for-snapshot-deletion-and-clone branch February 18, 2026 02:56

This was referenced Feb 24, 2026

Failed clone can lead to stucked snapshot deletion #142919

Closed

Synthesize ShardId for SnapshotInfo of failed clones #142920

Merged

Conversation

ywangd commented Feb 10, 2026

Uh oh!

elasticsearchmachine commented Feb 10, 2026

Uh oh!

elasticsearchmachine commented Feb 10, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ywangd Feb 10, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

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!

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!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

ywangd Feb 10, 2026 •

edited

Loading