Followup: ensure the LSM iterators are invalidated on exceptions#30362
Open
ballard26 wants to merge 3 commits intoredpanda-data:devfrom
Open
Followup: ensure the LSM iterators are invalidated on exceptions#30362ballard26 wants to merge 3 commits intoredpanda-data:devfrom
ballard26 wants to merge 3 commits intoredpanda-data:devfrom
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
Follow-up exception-safety hardening for LSM iterators: ensures valid() cannot remain true after an exception during a mutating iterator operation, preventing downstream use of stale iterator state.
Changes:
- Extracted a reusable
throwing_iteratortest utility and wired it into existing DB iterator tests. - Updated
two_level_iterator,merging_iterator, and the block reader iterator to invalidate state at the start of mutating operations and re-establish validity only on success. - Added targeted exception-safety tests for
two_level_iteratorandmerging_iterator.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| src/v/lsm/db/tests/iter_test.cc | Switches DB iterator exception-safety tests to shared throwing_iterator utility. |
| src/v/lsm/db/tests/BUILD | Adds dependency on the new throwing_iterator test library. |
| src/v/lsm/core/internal/two_level_iterator.cc | Adds explicit invalidation/revalidation so valid() can’t remain stale-true after exceptions. |
| src/v/lsm/core/internal/tests/two_level_iterator_test.cc | New tests validating two_level_iterator invalidates on exceptions across seek/next/prev paths. |
| src/v/lsm/core/internal/tests/throwing_iterator.h | New reusable throwing iterator for exception-path testing of higher-level iterators. |
| src/v/lsm/core/internal/tests/merging_iterator_test.cc | Adds exception-safety tests ensuring merging iterator becomes invalid after child exceptions. |
| src/v/lsm/core/internal/tests/BUILD | Adds throwing_iterator test library and new two_level_iterator_test target + deps. |
| src/v/lsm/core/internal/merging_iterator.cc | Invalidates _current early so valid() becomes false if an awaited child op throws. |
| src/v/lsm/block/reader.cc | Invalidates block iterator position during mutating operations so corruption exceptions don’t leave a stale-valid position. |
Comments suppressed due to low confidence (1)
src/v/lsm/db/tests/iter_test.cc:21
std::mapis used later in this test file (e.g. inDBIteratorExceptionSafetyTest::SetUp), but<map>is no longer included here. The file currently relies onthrowing_iterator.hto transitively include<map>, which is brittle if that header changes. Please add an explicit#include <map>in this file.
#include "lsm/core/internal/iterator.h"
#include "lsm/core/internal/keys.h"
#include "lsm/core/internal/options.h"
#include "lsm/core/internal/tests/throwing_iterator.h"
#include "lsm/db/iter.h"
#include "lsm/db/memtable.h"
#include <gtest/gtest.h>
#include <memory>
#include <stdexcept>
#include <string_view>
Collaborator
CI test resultstest results on build#83923
test results on build#83966
|
3ec9ba5 to
a3a1449
Compare
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 9 out of 9 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (2)
src/v/lsm/db/tests/iter_test.cc:21
iter_test.ccstill usesstd::map(e.g., inDBIteratorExceptionSafetyTest::SetUp) but the direct#include <map>was removed. Consider re-adding the explicit include to avoid relying on transitive includes fromthrowing_iterator.h.
#include <gtest/gtest.h>
#include <memory>
#include <stdexcept>
#include <string_view>
src/v/lsm/core/internal/tests/BUILD:111
- This BUILD file ends with multiple trailing blank lines. Running buildifier will typically remove these; consider trimming to a single trailing newline to avoid formatting-only diffs from automated tooling.
The merging iterator only set _current at the end of each mutating method, via find_smallest or find_largest. If a child threw partway through, those helpers never ran and _current still pointed at the previous child. valid() then returned true with stale state. Clear _current at the start of every mutating method so a thrown await leaves the iterator invalid. Also adds a shared throwing_iterator test helper.
The previous fix only cleared _data_iter inside init_data_block. Other throws in a mutating method could still leave _data_iter non-null with undefined inner state. The derived valid() check would then return true. Replace it with a cached _valid bool. Each mutating method clears _valid at the start and re-establishes it on the success path.
parse_next_key set _current to the next entry's offset before calling decode_entry. If decode_entry threw a corruption_exception, valid() returned true while _key and _value still held the previous entry's bytes. Move the _current assignment to the success tail of parse_next_key. Also invalidate at the start of every mutating method.
a3a1449 to
586ad29
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
A follow up to the previous PR on this matter. Goes through the remainder of the LSM iterators and ensures each of them are properly invalidated when an exception is thrown. See individual commits for details on each.
Backports Required
Release Notes