From 753c1f5825eb6a5e2941ad8f38d945a08bb12f52 Mon Sep 17 00:00:00 2001 From: Spade A <71589810+SpadeA-Tang@users.noreply.github.com> Date: Wed, 22 Apr 2026 10:41:44 +0800 Subject: [PATCH] fix: preserve temp text index state across sealed segment Reopen (#49192) issue: https://github.com/milvus-io/milvus/issues/49076 Signed-off-by: SpadeA --- .../src/segcore/ChunkedSegmentSealedImpl.cpp | 17 ++++- .../src/segcore/ChunkedSegmentSealedImpl.h | 7 +++ internal/core/src/segcore/SegmentLoadInfo.h | 5 ++ internal/core/unittest/test_sealed.cpp | 63 +++++++++++++++++++ 4 files changed, 91 insertions(+), 1 deletion(-) diff --git a/internal/core/src/segcore/ChunkedSegmentSealedImpl.cpp b/internal/core/src/segcore/ChunkedSegmentSealedImpl.cpp index 5ed73f0ff9..e73085df01 100644 --- a/internal/core/src/segcore/ChunkedSegmentSealedImpl.cpp +++ b/internal/core/src/segcore/ChunkedSegmentSealedImpl.cpp @@ -2616,6 +2616,14 @@ ChunkedSegmentSealedImpl::CreateTextIndex(FieldId field_id, std::unique_lock lck(mutex_); + // Guard against re-entry on a field whose temp text index was already + // built: the path `/_` is shared with the + // live holder in text_indexes_, and rebuilding there races with its + // destructor's RemoveDir (issue #49076). + AssertInfo(text_indexes_.find(field_id) == text_indexes_.end(), + "text index for field {} already exists, refusing to rebuild", + field_id.get()); + const auto& field_meta = schema_->operator[](field_id); auto& cfg = storage::MmapManager::GetInstance().GetMmapConfig(); std::unique_ptr index; @@ -2701,6 +2709,9 @@ ChunkedSegmentSealedImpl::CreateTextIndex(FieldId field_id, text_indexes_[field_id] = std::make_shared( std::move(index), cfg.GetScalarIndexEnableMmap()); + // Record under mutex_ together with text_indexes_ write — concurrent + // readers of segment_load_info_ see a consistent view. + segment_load_info_.SetTextIndexCreated(field_id); } void @@ -3870,6 +3881,11 @@ ChunkedSegmentSealedImpl::Reopen( std::unique_lock lck(mutex_); SegmentLoadInfo current(segment_load_info_); + // Carry runtime-only state forward so subsequent Reopens don't + // re-schedule CreateTextIndex on a field whose temp index was built. + for (auto fid : current.GetCreatedTextIndexes()) { + new_seg_load_info.SetTextIndexCreated(fid); + } segment_load_info_ = new_seg_load_info; use_take_for_output_ = segment_load_info_.GetUseTakeForOutput(); lck.unlock(); @@ -4006,7 +4022,6 @@ ChunkedSegmentSealedImpl::ApplyLoadDiff(milvus::OpContext* op_ctx, if (!diff.text_indexes_to_create.empty()) { for (const auto& field_id : diff.text_indexes_to_create) { CreateTextIndex(field_id, op_ctx); - segment_load_info.SetTextIndexCreated(field_id); } } diff --git a/internal/core/src/segcore/ChunkedSegmentSealedImpl.h b/internal/core/src/segcore/ChunkedSegmentSealedImpl.h index e9d3ae7100..9de5889961 100644 --- a/internal/core/src/segcore/ChunkedSegmentSealedImpl.h +++ b/internal/core/src/segcore/ChunkedSegmentSealedImpl.h @@ -1291,6 +1291,13 @@ class ChunkedSegmentSealedImpl : public SegmentSealed { SetUseTakeForOutputForTesting(bool val) { use_take_for_output_ = val; } + + // Test-only: direct access to segment_load_info_ for asserting Reopen + // preserves runtime-only state (e.g. created_text_indexes_). + SegmentLoadInfo& + TestGetSegmentLoadInfo() { + return segment_load_info_; + } #endif private: diff --git a/internal/core/src/segcore/SegmentLoadInfo.h b/internal/core/src/segcore/SegmentLoadInfo.h index 1d1afb41c0..7b1265e80d 100644 --- a/internal/core/src/segcore/SegmentLoadInfo.h +++ b/internal/core/src/segcore/SegmentLoadInfo.h @@ -830,6 +830,11 @@ class SegmentLoadInfo { created_text_indexes_.end(); } + [[nodiscard]] const std::unordered_set& + GetCreatedTextIndexes() const { + return created_text_indexes_; + } + // ==================== Diff Computation ==================== /** diff --git a/internal/core/unittest/test_sealed.cpp b/internal/core/unittest/test_sealed.cpp index 2915c72ad5..ab7e46eada 100644 --- a/internal/core/unittest/test_sealed.cpp +++ b/internal/core/unittest/test_sealed.cpp @@ -70,6 +70,7 @@ #include "segcore/SegcoreConfig.h" #include "segcore/SegmentGrowing.h" #include "segcore/SegmentGrowingImpl.h" +#include "segcore/SegmentLoadInfo.h" #include "segcore/SegmentSealed.h" #include "segcore/Types.h" #include "storage/FileManager.h" @@ -2744,3 +2745,65 @@ TEST(SealedDropFieldData, PKFieldStillDropsBinlogIndex) { segment->DropFieldData(pk_id); EXPECT_TRUE(segment->HasFieldData(pk_id)); } + +// Reproducer for issue #49076. +// +// Reopen on a sealed segment does: +// SegmentLoadInfo new_seg_load_info(new_load_info, schema_); +// segment_load_info_ = new_seg_load_info; // <-- Bug 1 +// diff = current.ComputeDiff(new_seg_load_info); +// ApplyLoadDiff(op_ctx, new_seg_load_info, diff); // <-- Bug 2 +// +// `created_text_indexes_` is runtime-only (not proto-backed), so the RHS is +// always empty; the copy-assign wipes the member (Bug 1). And any Create +// done during this Reopen writes to the local new_seg_load_info, lost on +// return (Bug 2). A later Reopen's ComputeDiff then re-schedules +// CreateTextIndex for the same field at the same deterministic path, +// racing with the still-live holder and causing +// TantivyError: FileDoesNotExist("meta.json"). +TEST(SealedSegmentReopen, TextIndexCreatedWipedByReopen) { + auto schema = std::make_shared(); + schema->AddDebugField("pk", DataType::INT64); // 100 + schema->AddDebugField( + "vec", DataType::VECTOR_FLOAT, 128, knowhere::metric::L2); // 101 + std::map analyzer_params; + schema->AddDebugVarcharField(FieldName("text_field"), + DataType::VARCHAR, + /*max_length=*/65535, + /*nullable=*/false, + /*enable_match=*/true, + /*enable_analyzer=*/true, + analyzer_params, + std::nullopt); // 102 + schema->set_primary_field_id(FieldId(100)); + + const FieldId text_fid(102); + + auto segment = CreateSealedSegment(schema); + auto* sealed = dynamic_cast(segment.get()); + ASSERT_NE(sealed, nullptr); + + proto::segcore::SegmentLoadInfo proto; + proto.set_segmentid(49076); + proto.set_num_of_rows(0); + sealed->SetLoadInfo(proto); + + // Simulate the state a prior Load would leave behind after running + // CreateTextIndex for the enable_match field. + sealed->TestGetSegmentLoadInfo().SetTextIndexCreated(text_fid); + ASSERT_TRUE(sealed->TestGetSegmentLoadInfo().HasTextIndexCreated(text_fid)); + + // Reopen must preserve the runtime-only created_text_indexes_. Before + // the fix this fails (member wiped by `segment_load_info_ = + // new_seg_load_info`); after the fix it passes. The wipe happens before + // ApplyLoadDiff, so even though ApplyLoadDiff throws here (num_rows_ is + // unset because no real data was loaded), the state under test is + // already settled. + milvus::OpContext op_ctx; + try { + sealed->Reopen(&op_ctx, proto); + } catch (...) { + } + + EXPECT_TRUE(sealed->TestGetSegmentLoadInfo().HasTextIndexCreated(text_fid)); +}