tiered_storage: fix bad_optional_access#30303
Conversation
There was a problem hiding this comment.
Pull request overview
Fixes a crash in tiered-storage fetches when retention advances _start_offset past all segments but stale segment metadata remains, and later uploads introduce a gap such that no segment exists exactly at _start_offset.
Changes:
- Update
partition_manifest::compute_start_kafka_offset_local()to use_segments.lower_bound(_start_offset)so stale segments below_start_offsetdon’t cause anulloptresult. - Add a regression test that reproduces the “start beyond all segments + stale segments + new segment with gap” scenario and asserts the fetch path no longer throws.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
src/v/cloud_storage/partition_manifest.cc |
Fixes start-offset translation fallback logic to skip stale segments and avoid returning nullopt in the described retention/GC failure scenario. |
src/v/cloud_storage/tests/remote_partition_test.cc |
Adds a regression test covering the crash scenario and validates reads proceed without std::bad_optional_access. |
| auto it = _segments.lower_bound(_start_offset); it != segments_end) { | ||
| local_start_offset = it->base_offset - it->delta_offset; |
There was a problem hiding this comment.
This seems reasonable to me, but also makes me wonder, should the caller be doing something different than a bad optional access? My guess it that it'll end up as a thrown exception / error case anyway, but wondering if there is a less surprising error scenario worth ironing out
It seems like this
redpanda/src/v/cloud_storage/remote_partition.cc
Lines 141 to 143 in bcaa00a
manifest.begin() and I'm wondering if we need a lower_bound there too
There was a problem hiding this comment.
cc @Lazin for thoughts, given you authored this code and the remote_partition.cc snippet
There was a problem hiding this comment.
I think it doesn't solve all edge cases. It will work if the manifest is not logically empty (all segments are below _start_offset). If the lower_bound returns end there will be bad optional access anyway.
There was a problem hiding this comment.
Re: Andrew's concern: there's code a bit below that skips forward and handles this case.
Re: Evgeny's: You're right that if all segments are below _start_offset that this function would return nullopt. That'd be correct as there's no offset to return. The problem would be the .value call in first_uploaded_offset. However, where that's called it's guarded by an is_data_available vassert, which guarantees there is some segment at or above the start offset to be found, so the option will not be empty. I pushed the vassert into the first_uploaded_offset call and documented it as a precondition. Bottom line, I think this handles all bad optional accesses. The other outcome is a vassert trip indicating a specific bug.
When retention advances _start_offset past all segments (the "delete everything" case) but GC fails to truncate the manifest, stale segments accumulate below _start_offset. When a new segment then arrives with a base_offset that doesn't match _start_offset exactly (there is a gap due to config batches), the fallback branch in compute_start_kafka_offset_local() used _segments.begin() and checked base_offset >= _start_offset. With stale segments present, begin() points below _start_offset, the condition is false, and the function returns nullopt. The caller (first_uploaded_offset) then calls .value() on the nullopt, throwing std::bad_optional_access through the Kafka fetch path. Fix by using lower_bound(_start_offset) instead of begin(), which skips any stale segments and finds the first valid one at or above _start_offset.
Retry command for Build#85065please wait until all jobs are finished before running the slash command |
|
/ci-repeat 1 |
|
/backport v26.1.x |
|
/backport v25.3.x |
|
/backport v25.2.x |
When retention advances _start_offset past all segments (the "delete everything" case) but GC fails to truncate the manifest, stale segments accumulate below _start_offset. When a new segment then arrives with a base_offset that doesn't match _start_offset exactly (there is a gap due to config batches), the fallback branch in compute_start_kafka_offset_local() used _segments.begin() and checked base_offset >= _start_offset. With stale segments present, begin() points below _start_offset, the condition is false, and the function returns nullopt. The caller (first_uploaded_offset) then calls .value() on the nullopt, throwing std::bad_optional_access through the Kafka fetch path.
Fix by using lower_bound(_start_offset) instead of begin(), which skips any stale segments and finds the first valid one at or above _start_offset.
Backports Required
Release Notes
Bug Fixes