apache / apache/druid

Partial segment loading: bootstrap-restored cache entries can fail to download after restart (reused BootstrapRangeReader throws)

Open
#19,680 1 comment 0 reactions 0 assignees View on GitHub
Dominant language
Java
Stars
14.1k
Forks
3.8k
Avg merge
2d 58m
Merged PRs (30d)
233

Description

### Affected version

`master` (feature introduced in #19535, "on-demand partial segment loading"). The feature is gated off by default behind `druid.segmentCache.virtualStorage` + `druid.segmentCache.virtualStoragePartialDownloadsEnabled`.

> Note: this was found by static analysis while reading the #19535 change; I have **not** reproduced it at runtime yet (a repro sketch is included below). Filing it because the code path looks deterministically broken across restarts and is currently untested.

### Description

With partial downloads enabled, a segment can be cached with only some of its containers on local disk. After a process restart, `PartialSegmentCacheBootstrap.reserveFromDisk` restores such entries from disk — but builds each restored `PartialSegmentMetadataCacheEntry` with `BootstrapRangeReader.INSTANCE`, whose `readRange()` unconditionally throws:

```java
// PartialSegmentCacheBootstrap.BootstrapRangeReader
public InputStream readRange(String filename, long offset, long length) {
throw DruidException.defensive(
"BootstrapRangeReader was asked to fetch [%s] @[%d:%d]; bootstrap should only read from local disk", ...);
}
```

A partially-downloaded segment restores with `isFullyDownloaded() == false` (that requires `downloadedFiles.containsAll(metadata.getFiles().keySet())`, while `restoreBundlesFromDisk` admits any bundle whose container files merely exist). When a query later acquires that segment, `SegmentLocalCacheManager.acquireSegment` opens a real working reader via `tryOpenRangeReader`, but `findOrReservePartial` reuses the pre-existing restored entry and **discards** that reader:

```java
final ReservedPartial existing = findExistingPartialWithHold(dataSegment.getId());
if (existing != null) { return existing; } // the real rangeReader arg is dropped
return reservePartial(dataSegment, rangeReader); // only reached when no entry exists yet
```

The reader can't be replaced afterward — the entry's and the file mapper's `rangeReader` fields are both `final`, and `doMount` builds the mapper exactly once. So the download then runs through the restored entry's throw-only `BootstrapRangeReader`:

- **`AcquireMode.FULL`:** `ensureAllDownloaded()` → `downloadContainer` → `streamRangeIntoContainer` → `rangeReader.readRange(...)` → throws.
- **`AcquireMode.PARTIAL`:** at cursor build, `mapFile` → `ensureFileDownloaded` → `streamRangeIntoContainer` → `rangeReader.readRange(...)` → throws.

### Impact

After any restart, a query that needs not-yet-cached bytes of a partially-downloaded segment fails with a `DruidException` (defensive) instead of fetching the missing bytes from deep storage — i.e., the feature's core lazy-load path is broken across restarts. Only deployments running partial downloads (the off-by-default experimental flags) are affected.

### Existing mitigation doesn't cover this path

`doMount`'s cleanup (and `testMountFailureRemovesLingeringWeakEntry`) handle the **mount-time** header-refetch case: on mount failure, `removeUnheldWeakEntry` drops the stale entry. But that only fires on mount failure. When the header file is present the mount **succeeds**, the entry persists, and the later container/file download throws with no fallback. `SegmentLocalCacheManagerPartialAcquireTest` exercises `acquireSegment` + download only via **fresh** reservations (real reader), so the restore-then-download path is untested.

### Suggested fix

When an on-demand acquire reuses a pre-existing (bootstrap-restored) metadata entry, replace its `BootstrapRangeReader` with the real reader from `tryOpenRangeReader` — or evict + re-reserve the entry via `reservePartial` — so restored partial segments can complete downloads. A regression test that restores a partially-downloaded entry and then drives both a FULL and a PARTIAL acquire would lock it in.

### Suggested reproduction (not yet run)

1. Enable `druid.segmentCache.virtualStorage=true` and `druid.segmentCache.virtualStoragePartialDownloadsEnabled=true` on a historical/MSQ worker (non-ephemeral, the default).
2. Run a query that lazily downloads only part of a segment, so its on-disk state is partial.
3. Restart the process (triggering `bootstrap()` → `reserveFromDisk`).
4. Run a query touching a column/range that wasn't previously downloaded.
5. Expected: the missing bytes are fetched from deep storage. Observed (per this trace): `DruidException` from `BootstrapRangeReader.readRange`.

*Traced across `PartialSegmentCacheBootstrap`, `SegmentLocalCacheManager`, `PartialSegmentMetadataCacheEntry`, and `PartialSegmentFileMapperV10` by inspection.*

Contributor guide

Open the contributing guide

Research direction

Start with PartialSegmentCacheBootstrap.reserveFromDisk and SegmentLocalCacheManager.findOrReservePartial, then inspect PartialSegmentMetadataCacheEntry and PartialSegmentFileMapperV10. Extend SegmentLocalCacheManagerPartialAcquireTest to restore a partially downloaded entry and exercise both FULL and PARTIAL acquisition; done means missing bytes download successfully instead of BootstrapRangeReader throwing.

Written by the indexing model from the issue text.

Assessment

Tech stack
java
Domain
databases
Issue type
Bug
Difficulty
4/5
Estimated time
3-5 days
Activity status
Quiet
Clarity
Mostly clear
Newbie friendliness
58/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.