fix: Stop kill task if segment IDs that share load spec of killable segment could not be determined by kfaraz · Pull Request #19737 · apache/druid

kfaraz · 2026-07-23T17:33:40Z

Bug

When concurrent append and replace is enabled and kill tasks (or embedded kill tasks on Overlord) are used, there may be potential data loss if the task action retrieveUpgradedFromSegmentIds or retrieveUpgradedToSegmentIds fired by the kill task fails. This is because the failure of these task actions is currently ignored and we may end up removing segment files from deep store even if the same load specs are shared by other segments.

Changes

Stop kill task (and embedded kill tasks) if parent IDs of the killable unused segments could not be determined. There are 2 cases possible:
- Segment does not share load spec: Do not remove segment from metadata store, otherwise deep store files would become orphaned and will never be removed.
- Segment shares load spec with other segments: Do not remove segment files from deep store as the same files are needed by other segments.
Stop kill task (and embedded kill tasks) if sibling segment IDs or children segment IDs could not be determined. Same cases as above.
Improve performance of embedded kill tasks by avoiding extra DB call to fetch parent IDs
- pre-fetch the parent IDs in the previous call which identifies the killable unused segments in the first place.

This PR has:

abhishekrb19

Thanks for the fix @kfaraz! A few comments on test & logs.

abhishekrb19 · 2026-07-24T07:44:04Z

+          e,
+          "Could not retrieve parent segment ids using task action[retrieveUpgradedFromSegmentIds]."
+          + " Stopping kill task to avoid data loss in case the segment files"
+          + " are shared by other segments."


What do you think about adding a unit test in KillUnusedSegmentsTaskTest or UnusedSegmentsKillerTest to verify that segments aren't spuriously purged in this case?

Thanks for the suggestion! Added some tests.

FrankChen021

Severity	Findings
P0	0
P1	2
P2	0
P3	0
Total	2

Reviewed 12 of 12 changed files.

This is an automated review by Codex GPT-5.6-Sol

FrankChen021 · 2026-07-24T12:15:15Z


-      toolbox.getDataSegmentKiller().kill(segmentsToBeKilled);
-      emitMetric(toolbox.getEmitter(), TaskMetrics.SEGMENTS_DELETED_FROM_DEEPSTORE, segmentsToBeKilled.size());
+      // 3. Nuke all eligible unused segments


[P1] Close the sharing race before deleting deep storage

The new relationship lookup is only a preflight before SegmentNukeAction. With concurrent locks enabled this task holds a REPLACE lock, which can coexist with APPEND publication; an append publish or retry can therefore insert a parent or upgraded sibling sharing the load spec after step 2. Step 3 then removes the old metadata and step 4 deletes a file still referenced by the newly published segment. Keep the preflight, but perform a fail-closed sharing recheck after the nuke and before deep-storage deletion (ideally in a transaction/critical section that excludes a new publication window).

It is very unlikely for this to happen. It had been called out here as well.
#16362 (comment)

Essentially, the kill buffer period protects us against this.
Concurrent APPEND would only perform upgrades on a "used" segment.
And kill task would only nuke segments that have been been "unused" for a while (default is 30 days).

Adding a comment in javadoc.

FrankChen021 · 2026-07-24T12:15:16Z

        response.getUpgradedToSegmentIds().forEach((parent, children) -> {
-          if (!CollectionUtils.isNullOrEmpty(children)) {
-            // Do not kill segment if its parent or any of its siblings still exist in metadata store
+          if (!segmentIdsBeingKilled.containsAll(children)) {


[P1] Fail closed when the relationship response is null

If the action response itself or its upgradedToSegmentIds map is null, this guard skips the only new safety check and treats every candidate as unshared. Both the response DTO and remote conversion permit null, so a malformed or missing result can still proceed through metadata and deep-storage deletion. Throw before SegmentNukeAction unless a non-null map was returned.

kfaraz · 2026-07-24T16:06:08Z

@abhishekrb19 , thanks a lot for the prompt review! I have updated the tests and added a couple of new ones.

abhishekrb19

Some minor comments, but lgtm otherwise - thanks!

abhishekrb19 · 2026-07-25T05:41:24Z

+      final Set<DataSegment> unusedSegments = unusedSegmentsPlus.stream()
+                                                                 .map(DataSegmentPlus::getDataSegment)
+                                                                 .collect(Collectors.toSet());
+
      if (!TaskLocks.isLockCoversSegments(taskLockMap, unusedSegments)) {


nit: perhaps we could introduce a thin wrapper for isLockCoversSegments() that also accepts DataSegmentPlus that delegates to isLockCoversSegments(NavigableMap<DateTime, List>, Collection segments) to avoid another iteration over the set of segments.

The Set<DataSegment> unusedSegments created before this line is also used later in this method.

abhishekrb19 · 2026-07-25T05:45:15Z

+      final Set<String> segmentIds = unusedSegments.stream().map(
+          s -> s.getDataSegment().getId().toString()
+      ).collect(Collectors.toSet());


nit: We could compute this once in the caller and pass it to both this function and getKillableSegments.

Fix bug and improve embedded kill perf

626a66d

github-actions Bot added the Area - Ingestion label Jul 23, 2026

kfaraz added 2 commits July 23, 2026 23:12

minor fixes

392b190

More fixes

8f6c820

kfaraz added the Bug label Jul 23, 2026

kfaraz added this to the 38.0.0 milestone Jul 23, 2026

kfaraz changed the title ~~fix: Stop kill task if parent IDs of segment could not be determined~~ fix: Stop kill task if segment IDs that share load spec of killable segment could not be determined Jul 23, 2026

kfaraz commented Jul 23, 2026

View reviewed changes

Comment thread ...xing-service/src/main/java/org/apache/druid/indexing/common/task/KillUnusedSegmentsTask.java

Exclude self

4249ea9

abhishekrb19 reviewed Jul 24, 2026

View reviewed changes

kfaraz added 3 commits July 24, 2026 14:00

Fix tests

02612d9

Add some tests

cb45985

Add tests for UnusedSegmentsKiller

8cadbbe

FrankChen021 reviewed Jul 24, 2026

View reviewed changes

Add note in javadoc

0f2ee05

abhishekrb19 approved these changes Jul 25, 2026

View reviewed changes

fix docs, javadoc

0dd7e69

github-actions Bot added the Area - Documentation label Jul 25, 2026

Uh oh!

Conversation

kfaraz commented Jul 23, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Bug

Changes

Uh oh!

Uh oh!

abhishekrb19 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

abhishekrb19 Jul 24, 2026

Choose a reason for hiding this comment

Uh oh!

kfaraz Jul 24, 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!

FrankChen021 left a comment

Choose a reason for hiding this comment

Uh oh!

FrankChen021 Jul 24, 2026

Choose a reason for hiding this comment

Uh oh!

kfaraz Jul 24, 2026

Choose a reason for hiding this comment

Uh oh!

kfaraz Jul 24, 2026

Choose a reason for hiding this comment

Uh oh!

FrankChen021 Jul 24, 2026

Choose a reason for hiding this comment

Uh oh!

kfaraz commented Jul 24, 2026

Uh oh!

abhishekrb19 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

abhishekrb19 Jul 25, 2026

Choose a reason for hiding this comment

Uh oh!

kfaraz Jul 25, 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!

abhishekrb19 Jul 25, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

kfaraz commented Jul 23, 2026 •

edited

Loading

kfaraz Jul 24, 2026 •

edited

Loading

kfaraz Jul 25, 2026 •

edited

Loading