Partial segment loading: bootstrap-restored cache entries can fail to download after restart (reused BootstrapRangeReader throws)
- 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
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