Skip to content

fix: [2.6] guard facade APIs from exceptions #1685

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
foxspy wants to merge 1 commit into
base: 2.6
Choose a base branch
from
Open

fix: [2.6] guard facade APIs from exceptions #1685

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 4 additions & 4 deletions include/knowhere/cluster/cluster.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -117,16 +117,16 @@ class Cluster {
}

expected<DataSetPtr>
Train(const DataSet& dataset, const Json& json);
Train(const DataSet& dataset, const Json& json) noexcept;

expected<DataSetPtr>
Assign(const DataSet& dataset);
Assign(const DataSet& dataset) noexcept;

expected<DataSetPtr>
GetCentroids() const;
GetCentroids() const noexcept;

std::string
Type() const;
Type() const noexcept;

~Cluster() {
if (node == nullptr)
Expand Down
4 changes: 2 additions & 2 deletions include/knowhere/cluster/cluster_factory.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,12 +24,12 @@ class ClusterFactory {
public:
template <typename DataType>
expected<Cluster<ClusterNode>>
Create(const std::string& name, const Object& object = nullptr);
Create(const std::string& name, const Object& object = nullptr) noexcept;
template <typename DataType>
const ClusterFactory&
Register(const std::string& name, std::function<Cluster<ClusterNode>(const Object&)> func);
static ClusterFactory&
Instance();
Instance() noexcept;

private:
struct FunMapValueBase {
Expand Down
17 changes: 9 additions & 8 deletions include/knowhere/comp/brute_force.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,43 +28,44 @@ class BruteForce {
template <typename DataType>
static expected<DataSetPtr>
Search(const DataSetPtr base_dataset, const DataSetPtr query_dataset, const Json& config, const BitsetView& bitset,
milvus::OpContext* op_context = nullptr);
milvus::OpContext* op_context = nullptr) noexcept;

template <typename DataType>
static Status
SearchWithBuf(const DataSetPtr base_dataset, const DataSetPtr query_dataset, int64_t* ids, float* dis,
const Json& config, const BitsetView& bitset, milvus::OpContext* op_context = nullptr);
const Json& config, const BitsetView& bitset, milvus::OpContext* op_context = nullptr) noexcept;

template <typename DataType>
static Status
SearchOnChunkWithBuf(const DataSetPtr base_dataset, const DataSetPtr query_dataset, int64_t* ids, float* dis,
const Json& config, const BitsetView& bitset, milvus::OpContext* op_context = nullptr);
const Json& config, const BitsetView& bitset,
milvus::OpContext* op_context = nullptr) noexcept;

template <typename DataType>
static expected<DataSetPtr>
RangeSearch(const DataSetPtr base_dataset, const DataSetPtr query_dataset, const Json& config,
const BitsetView& bitset, milvus::OpContext* op_context = nullptr);
const BitsetView& bitset, milvus::OpContext* op_context = nullptr) noexcept;

// Perform row oriented sparse vector brute force search.
static expected<DataSetPtr>
SearchSparse(const DataSetPtr base_dataset, const DataSetPtr query_dataset, const Json& config,
const BitsetView& bitset, milvus::OpContext* op_context = nullptr);
const BitsetView& bitset, milvus::OpContext* op_context = nullptr) noexcept;

static Status
SearchSparseWithBuf(const DataSetPtr base_dataset, const DataSetPtr query_dataset, sparse::label_t* ids, float* dis,
const Json& config, const BitsetView& bitset, milvus::OpContext* op_context = nullptr);
const Json& config, const BitsetView& bitset, milvus::OpContext* op_context = nullptr) noexcept;

template <typename DataType>
static expected<std::vector<IndexNode::IteratorPtr>>
AnnIterator(const DataSetPtr base_dataset, const DataSetPtr query_dataset, const Json& config,
const BitsetView& bitset, bool use_knowhere_search_pool = true,
milvus::OpContext* op_context = nullptr);
milvus::OpContext* op_context = nullptr) noexcept;

template <typename DataType>
static expected<std::vector<IndexNode::IteratorPtr>>
AnnIteratorOnChunk(const DataSetPtr base_dataset, const DataSetPtr query_dataset, const Json& config,
const BitsetView& bitset, bool use_knowhere_search_pool = true,
milvus::OpContext* op_context = nullptr);
milvus::OpContext* op_context = nullptr) noexcept;
};

} // namespace knowhere
Expand Down
163 changes: 162 additions & 1 deletion include/knowhere/expected.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,9 +13,21 @@
#define EXPECTED_H

#include <cassert>
#include <functional>
#include <iostream>
#include <memory>
#include <new>
#include <optional>
#include <stdexcept>
#include <string>
#include <type_traits>
#include <utility>

#if defined(SWIG)
#define KNOWHERE_NODISCARD
#else
#define KNOWHERE_NODISCARD [[nodiscard]]
#endif

namespace knowhere {

Expand Down Expand Up @@ -52,8 +64,67 @@ enum class Status {
brute_force_inner_error = 30,
emb_list_inner_error = 31,
aisaq_error = 32,
knowhere_inner_error = 33,
};

enum class StatusCategory {
success = 0,
input_error = 1,
inner_error = 2,
};

inline constexpr StatusCategory
StatusCategoryOf(knowhere::Status status) {
switch (status) {
case knowhere::Status::success:
return StatusCategory::success;
case knowhere::Status::invalid_args:
case knowhere::Status::invalid_param_in_json:
case knowhere::Status::out_of_range_in_json:
case knowhere::Status::type_conflict_in_json:
case knowhere::Status::invalid_metric_type:
case knowhere::Status::empty_index:
case knowhere::Status::not_implemented:

@liliu-z liliu-z Jun 22, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

index_not_trained/index_already_trained map to input_error (:87-89) and timeout maps to inner_error (:106), and that category directly drives the retry-vs-reject decision. These are state/deadline conditions rather than obvious user-input, so confirm the mapping matches intended Milvus semantics — otherwise a not-trained or timeout outcome could be retried or rejected incorrectly. This is a verify-before-merge sign-off, not a confirmed bug.

case knowhere::Status::index_not_trained:
case knowhere::Status::index_already_trained:
case knowhere::Status::invalid_value_in_json:
case knowhere::Status::arithmetic_overflow:
case knowhere::Status::invalid_binary_set:
case knowhere::Status::invalid_instruction_set:
case knowhere::Status::invalid_index_error:
case knowhere::Status::invalid_cluster_error:
case knowhere::Status::invalid_serialized_index_type:
return StatusCategory::input_error;
case knowhere::Status::faiss_inner_error:
case knowhere::Status::hnsw_inner_error:
case knowhere::Status::malloc_error:
case knowhere::Status::diskann_inner_error:
case knowhere::Status::disk_file_error:
case knowhere::Status::cuvs_inner_error:
case knowhere::Status::cuda_runtime_error:
case knowhere::Status::cluster_inner_error:
case knowhere::Status::timeout:
case knowhere::Status::internal_error:
case knowhere::Status::sparse_inner_error:
case knowhere::Status::brute_force_inner_error:
case knowhere::Status::emb_list_inner_error:
case knowhere::Status::aisaq_error:
case knowhere::Status::knowhere_inner_error:
default:
return StatusCategory::inner_error;
}
}

inline constexpr bool
IsInputError(knowhere::Status status) {
return StatusCategoryOf(status) == StatusCategory::input_error;
}

inline constexpr bool
IsInnerError(knowhere::Status status) {
return StatusCategoryOf(status) == StatusCategory::inner_error;
}

inline std::string
Status2String(knowhere::Status status) {
switch (status) {
Expand Down Expand Up @@ -113,13 +184,15 @@ Status2String(knowhere::Status status) {
return "emb_list inner error";
case knowhere::Status::aisaq_error:
return "internal AiSAQ error";
case knowhere::Status::knowhere_inner_error:
return "knowhere inner error";
default:
return "unexpected status";
}
}

template <typename T>
class expected {
class KNOWHERE_NODISCARD expected {

@liliu-z liliu-z Jun 22, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Marking expected<T> [[nodiscard]] (via KNOWHERE_NODISCARD) is a real compile-surface change, and the in-repo test_diskann.cc fix shows discard sites exist. But knowhere's own build will not break: host C++ -Werror is commented out (compile_flags.cmake:19) so a discard is only a warning, and the GPU build suppresses host warnings via -Xcompiler=-w / --diag-suppress (compile_flags.cmake:31-33), so the cuVS -Werror=all-warnings path is unaffected. The only real risk is downstream Milvus if it compiles with -Werror — sweep it for discarded expected<T> returns before merge.

public:
template <typename... Args>
expected(Args&&... args) : val(std::make_optional<T>(std::forward<Args>(args)...)), err(Status::success) {
Expand Down Expand Up @@ -202,6 +275,94 @@ class expected {
std::string msg;
};

#if !defined(SWIG)

namespace detail {

template <typename T>
struct is_expected : std::false_type {};

template <typename T>
struct is_expected<expected<T>> : std::true_type {};

template <typename T>
inline constexpr bool is_expected_v = is_expected<std::decay_t<T>>::value;

template <typename T>
struct expected_value;

template <typename T>
struct expected_value<expected<T>> {
using type = T;
};

inline std::string
ExceptionMessage(const char* prefix, const std::string& what) {
if (what.empty()) {
return prefix;
}
return std::string(prefix) + ": " + what;
}

template <typename R>
std::decay_t<R>
GuardedFailure(Status status, std::string msg) noexcept {
using Result = std::decay_t<R>;
if constexpr (std::is_same_v<Result, Status>) {
return status;
} else if constexpr (is_expected_v<Result>) {
using Value = typename expected_value<Result>::type;
return expected<Value>::Err(status, std::move(msg));
} else if constexpr (std::is_same_v<Result, bool>) {
return false;
} else if constexpr (std::is_integral_v<Result> || std::is_floating_point_v<Result>) {
return Result{};
} else if constexpr (std::is_same_v<Result, std::string>) {
return {};
} else if constexpr (std::is_default_constructible_v<Result>) {
return Result{};
} else {
static_assert(std::is_default_constructible_v<Result>,
"GuardedCall requires Status, expected<T>, or a default-constructible return type");
}
}

template <typename R>
std::decay_t<R>
GuardedCallFailure(Status status, std::string msg) noexcept {
if constexpr (std::is_void_v<R>) {
return;
} else {
return GuardedFailure<R>(status, std::move(msg));
}
}

} // namespace detail

template <typename Func, typename... Args>
std::decay_t<std::invoke_result_t<Func, Args...>>
GuardedCall(Func&& func, Args&&... args) noexcept {
using Result = std::invoke_result_t<Func, Args...>;
try {
if constexpr (std::is_void_v<Result>) {
std::invoke(std::forward<Func>(func), std::forward<Args>(args)...);
return;
} else {
return std::invoke(std::forward<Func>(func), std::forward<Args>(args)...);
}
} catch (const std::bad_alloc& e) {
return detail::GuardedCallFailure<Result>(Status::malloc_error,

@liliu-z liliu-z Jun 22, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The failure-construction path here can itself allocate, so under memory pressure that allocation throws and the exception escapes the guard, reaching terminate — a hole in the exact no-throw guarantee this PR exists to provide. On OOM, a guarded call that should have returned a clean error status instead crashes the whole process. Make the failure path allocation-free / noexcept so it cannot throw.

detail::ExceptionMessage("bad alloc", e.what()));
} catch (const std::exception& e) {
return detail::GuardedCallFailure<Result>(Status::knowhere_inner_error,
detail::ExceptionMessage("unhandled exception", e.what()));
} catch (...) {
return detail::GuardedCallFailure<Result>(Status::knowhere_inner_error, "unknown exception");
}
}

#endif

// Evaluates expr that returns a Status. Does nothing if the returned Status is
// a Status::success, otherwise returns the Status from the current function.
#define RETURN_IF_ERROR(expr) \
Expand Down
40 changes: 20 additions & 20 deletions include/knowhere/index/index.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -140,70 +140,70 @@ class Index {
}

Status
Build(const DataSetPtr dataset, const Json& json, bool use_knowhere_build_pool = true);
Build(const DataSetPtr dataset, const Json& json, bool use_knowhere_build_pool = true) noexcept;

#ifdef KNOWHERE_WITH_CARDINAL
const std::shared_ptr<Interrupt>
BuildAsync(const DataSetPtr dataset, const Json& json,
const std::chrono::seconds timeout = std::chrono::seconds::max());
const std::chrono::seconds timeout = std::chrono::seconds::max()) noexcept;
#else
const std::shared_ptr<Interrupt>
BuildAsync(const DataSetPtr dataset, const Json& json, bool use_knowhere_build_pool = true);
BuildAsync(const DataSetPtr dataset, const Json& json, bool use_knowhere_build_pool = true) noexcept;
#endif

Status
Train(const DataSetPtr dataset, const Json& json, bool use_knowhere_build_pool = true);
Train(const DataSetPtr dataset, const Json& json, bool use_knowhere_build_pool = true) noexcept;

Status
Add(const DataSetPtr dataset, const Json& json, bool use_knowhere_build_pool = true);
Add(const DataSetPtr dataset, const Json& json, bool use_knowhere_build_pool = true) noexcept;

expected<DataSetPtr>
Search(const DataSetPtr dataset, const Json& json, const BitsetView& bitset,
milvus::OpContext* op_context = nullptr) const;
milvus::OpContext* op_context = nullptr) const noexcept;

expected<std::vector<IndexNode::IteratorPtr>>
AnnIterator(const DataSetPtr dataset, const Json& json, const BitsetView& bitset,
bool use_knowhere_search_pool = true, milvus::OpContext* op_context = nullptr) const;
bool use_knowhere_search_pool = true, milvus::OpContext* op_context = nullptr) const noexcept;

expected<DataSetPtr>
RangeSearch(const DataSetPtr dataset, const Json& json, const BitsetView& bitset,
milvus::OpContext* op_context = nullptr) const;
milvus::OpContext* op_context = nullptr) const noexcept;

expected<DataSetPtr>
GetVectorByIds(const DataSetPtr dataset, milvus::OpContext* op_context = nullptr) const;
GetVectorByIds(const DataSetPtr dataset, milvus::OpContext* op_context = nullptr) const noexcept;

bool
HasRawData(const std::string& metric_type) const;
HasRawData(const std::string& metric_type) const noexcept;

bool
IsAdditionalScalarSupported(bool is_mv_only) const;
IsAdditionalScalarSupported(bool is_mv_only) const noexcept;

expected<DataSetPtr>
GetIndexMeta(const Json& json) const;
GetIndexMeta(const Json& json) const noexcept;

Status
Serialize(BinarySet& binset) const;
Serialize(BinarySet& binset) const noexcept;

Status
Deserialize(const BinarySet& binset, const Json& json = {});
Deserialize(const BinarySet& binset, const Json& json = {}) noexcept;

Status
DeserializeFromFile(const std::string& filename, const Json& json = {});
DeserializeFromFile(const std::string& filename, const Json& json = {}) noexcept;

int64_t
Dim() const;
Dim() const noexcept;

int64_t
Size() const;
Size() const noexcept;

int64_t
Count() const;
Count() const noexcept;

std::string
Type() const;
Type() const noexcept;

[[nodiscard]] bool
LoadIndexWithStream() const;
LoadIndexWithStream() const noexcept;

~Index() {
if (node == nullptr)
Expand Down
Loading
Loading