[fix](test) Align two UT assertions with current master behavior - #67404
Open
bosswnx wants to merge 2 commits into
Open
[fix](test) Align two UT assertions with current master behavior#67404bosswnx wants to merge 2 commits into
bosswnx wants to merge 2 commits into
Conversation
Contributor
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
IcebergWritePlanProviderTest asserts baseSnapshotId == null for an explicitly pinned empty read, but apache#66348 intentionally changed this to keep the pinned -1 as an OCC fence; the test from apache#66345 was not updated. PropertyAnalyzerTest expects the partition-level "only supports V2, V3 and SNII" message for V1, but apache#64522 already throws the "deprecated" message inside analyzeInvertedIndexFileStorageFormat before the partition-level check can run, making the expected branch unreachable. Update the Iceberg assertion to expect -1 (matching the intent documented in apache#66348), and accept either V1-rejection message in the PropertyAnalyzer assertion. Test-only, no production code changed.
bosswnx
force-pushed
the
fix/ut-align-assertions-67403
branch
from
September 1, 2026 16:48
ab02e29 to
f4a5e3f
Compare
Contributor
Author
|
run buildall |
Contributor
TPC-H: Total hot run time: 16927 ms |
Contributor
TPC-DS: Total hot run time: 82557 ms |
Contributor
ClickBench: Total hot run time: 14.76 s |
Contributor
FE Regression Coverage ReportIncrement line coverage |
Closed
2 tasks
Contributor
Author
|
The NonConcurrent Regression failure on this PR is not caused by this change — it is a pre-existing BE crash on master, now tracked in #67428. Evidence:
The cascade of ~49 suite failures is downstream of the BE exit ("available backend num is 0"), not separate issues. Given the above, the NonConcurrent pipeline should fail for any PR based on current master. A re-run will hit the same crash until the upstream fix in #67428 lands — happy to help there. Everything else on this PR is green: FE UT pass, CheckStyle pass, P0 Regression pass, External Regression pass. |
Merged
16 tasks
morningman
pushed a commit
that referenced
this pull request
Sep 3, 2026
…lush task teardown (#67442) ### What problem does this PR solve? Issue Number: close #67428 Related PR: #66889 (introduced the UAF) Problem Summary: ASAN heap-use-after-free in `SharedMemtable::~SharedMemtable()` during group-commit memtable flush task teardown, introduced by #66889. `PartOfGroupMemtableFlushTask` holds its `SharedMemtable` by `shared_ptr` but only a `weak_ptr` to the `FlushToken`. When `run()`'s local `shared_ptr<FlushToken>` drops the last reference at the end of `run()`, destruction cascades `FlushToken` -> `RowsetWriter` -> `RowsetWriterContext`; the thread pool then destroys the task object, and `~SharedMemtable()` dereferences the dangling raw `RowsetWriterContext* rowset_ctx` in `remove_segment_allocated_lsns()`. Reproduced by the nonConcurrent pipeline on two unrelated PRs (#67404, #67402). ### What changed? - `SharedMemtable` now owns `std::shared_ptr<SegmentAllocatedLsnMap>` captured from the group writer context at submission; insert/remove go through the owned map. A null map stands for "no LSN allocation" (equivalent to `need_allocated_lsn()`, since `GroupRowsetWriter::init()` creates the map exactly when needed). This keeps the precise cleanup dependency alive without extending the whole `RowsetWriter` lifetime — the approach recommended in the #67428 triage. - Add `SegmentAllocatedLsnMap::contains_segment()` for test assertions. - Regression tests covering: (a) the last token/writer owner released while a group flush task finishes, (b) a queued subtask running after its weak token expired, (c) cancellation, plus LSN-entry cleanup assertions on the flush-error path. ### Verification ASAN BE UT, both directions: **without** the fix the new tests abort with the exact reported UAF (`SUMMARY: AddressSanitizer: heap-use-after-free rowset_writer_context.h:202:9 in doris::RowsetWriterContext::remove_segment_allocated_lsns`); **with** the fix the `MemTableFlushExecutor*` tests pass 8/8 (two runs).
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
2 participants
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.
What problem does this PR solve?
Issue Number: close #67403
Related PR: #66345 #66348 #64522 #66475
Problem Summary:
Two FE unit tests fail on current master because their assertions contradict behavior that later PRs deliberately introduced:
IcebergWritePlanProviderTest.planMergePreservesExplicitlyEmptyReadAcrossConcurrentFirstAppendassertsbaseSnapshotId == nullfor an explicitly pinned empty read, but [fix](iceberg) Harden external-write lifecycle, OCC, memory, and file ownership #66348 intentionally changed this to keep the pinned-1as an OCC fence (documented inIcebergConnectorTransaction.applyBeginGuards). The test from [fix](iceberg) Fix MVCC and nested schema evolution edge cases #66345 was not updated, so it has been failing since [fix](iceberg) Harden external-write lifecycle, OCC, memory, and file ownership #66348 landed.PropertyAnalyzerTest.testAnalyzePartitionInvertedIndexFileStorageFormatexpects the partition-level message "only supports V2, V3 and SNII" forV1, but [fix](fe) block inverted index V1 creation in FE #64522 already throws the "deprecated" message insideanalyzeInvertedIndexFileStorageFormatbefore the partition-level check can run, making the expected branch unreachable.What changed?
Test-only changes, no production code:
assertEquals(Long.valueOf(-1L), ...)matching the intent documented in [fix](iceberg) Harden external-write lifecycle, OCC, memory, and file ownership #66348, with a comment explaining the semantics.Both tests pass locally after the change:
IcebergWritePlanProviderTest79/79,PropertyAnalyzerTest20/20.Release note
None
Check List (For Author)
Test
Behavior changed:
Does this need documentation?