From 861f12fa45b439b2c1fd0f34a14632bcc7307a6a Mon Sep 17 00:00:00 2001 From: "zhenshan.cao" Date: Thu, 2 Jul 2026 16:16:01 -0700 Subject: [PATCH 1/4] fix: carry typed error codes on cachinglayer failure paths The two cancellation checks (CacheSlot load, Manager slot creation) threw a bare std::runtime_error, so a cancelled operation collapsed to UnexpectedError(2001) at the cgo boundary and FollyCancel(2038) -- defined exactly for this -- had zero producers. Throw via ThrowInfo(ErrorCode::FollyCancel) instead; the folly exception_wrapper chain preserves the typed SegcoreError to the consumer (verified by the existing cachinglayer future tests). ListNode's destroyed-while-loading guard also threw a bare runtime_error; that one is an internal invariant, not a cancellation, so it now wraps a typed SegcoreError(UnexpectedError) -- same class, but explicit and recognizable at the boundary. Verified: make test (all_tests + cachinglayer_test) 100% pass. Co-Authored-By: Claude Fable 5 Signed-off-by: zhenshan.cao --- src/cachinglayer/lrucache/ListNode.cpp | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/src/cachinglayer/lrucache/ListNode.cpp b/src/cachinglayer/lrucache/ListNode.cpp index 033979b..0f75b24 100644 --- a/src/cachinglayer/lrucache/ListNode.cpp +++ b/src/cachinglayer/lrucache/ListNode.cpp @@ -261,8 +261,12 @@ ListNode::mark_unload(std::function cb) { // NOTE: it should not happen, but if it does, we should not make the program deadlock. auto promise = std::move(load_promise_); lock.unlock(); - promise->setException(folly::exception_wrapper( - std::runtime_error("ListNode destroyed while loading, this should not happen"))); + // Wrap a typed SegcoreError (internal invariant, not a + // cancellation): a bare runtime_error rethrown from this + // promise cannot be recognized at the cgo boundary. + promise->setException(folly::exception_wrapper(milvus::SegcoreError( + milvus::ErrorCode::UnexpectedError, + "ListNode destroyed while loading, this should not happen"))); } break; } From bf09f9b4c2f1ffe54a278204a8e2e079b4469d9a Mon Sep 17 00:00:00 2001 From: "zhenshan.cao" Date: Thu, 2 Jul 2026 16:39:23 -0700 Subject: [PATCH 2/4] enhance: add an observer hook for untyped exceptions at the cgo boundary FailureCStatus is the single point that decides whether a typed error code survives to Go: an exception that is not a SegcoreError collapses to UnexpectedError(2001). Fire a registered observer on that branch so the consumer (milvus) can count it (UntypedCgoExceptionTotal) and log the message -- a shrinking hit rate is the measure that explicit error classification coverage is improving. The header stays free of any metrics dependency; same pattern as the Go-side unmapped-code observer. Co-Authored-By: Claude Fable 5 Signed-off-by: zhenshan.cao --- include/common/EasyAssert.h | 18 ++++++++++++++++++ src/common/EasyAssert.cpp | 23 +++++++++++++++++++++++ 2 files changed, 41 insertions(+) diff --git a/include/common/EasyAssert.h b/include/common/EasyAssert.h index bf489d2..95020fa 100644 --- a/include/common/EasyAssert.h +++ b/include/common/EasyAssert.h @@ -130,11 +130,29 @@ FailureCStatus(int code, const std::string& msg) { return CStatus{code, strdup(msg.data())}; } +// Observability hook for the cgo boundary: fired whenever FailureCStatus +// receives an exception that is NOT a SegcoreError (so its typed error code is +// lost and the failure collapses to UnexpectedError). The consumer (milvus) +// registers an observer that bumps a metric / logs; a shrinking hit rate means +// explicit error classification coverage is improving. This header stays free +// of any metrics dependency -- same observer pattern as the Go-side +// RegisterUnmappedSegcoreCodeObserver. +using UntypedCgoExceptionObserver = void (*)(const char* what); + +void +RegisterUntypedCgoExceptionObserver(UntypedCgoExceptionObserver observer); + +namespace impl { +void +NotifyUntypedCgoException(const char* what); +} // namespace impl + inline CStatus FailureCStatus(const std::exception* ex) { if (auto segcore_err = dynamic_cast(ex)) { return CStatus{static_cast(segcore_err->get_error_code()), strdup(segcore_err->what())}; } + impl::NotifyUntypedCgoException(ex->what()); return CStatus{static_cast(UnexpectedError), strdup(ex->what())}; } diff --git a/src/common/EasyAssert.cpp b/src/common/EasyAssert.cpp index 744b49f..0d8c512 100644 --- a/src/common/EasyAssert.cpp +++ b/src/common/EasyAssert.cpp @@ -16,12 +16,35 @@ #include "common/EasyAssert.h" +#include #include #include #include #include "fmt/format.h" +namespace milvus { + +namespace { +std::atomic untyped_cgo_exception_observer{nullptr}; +} // namespace + +void +RegisterUntypedCgoExceptionObserver(UntypedCgoExceptionObserver observer) { + untyped_cgo_exception_observer.store(observer, std::memory_order_release); +} + +namespace impl { +void +NotifyUntypedCgoException(const char* what) { + if (auto observer = untyped_cgo_exception_observer.load(std::memory_order_acquire)) { + observer(what); + } +} +} // namespace impl + +} // namespace milvus + namespace milvus::impl { std::string From e62d87ff5484f6f78a6eb88179c761c94ec43b43 Mon Sep 17 00:00:00 2001 From: "zhenshan.cao" Date: Sun, 26 Jul 2026 17:45:36 -0700 Subject: [PATCH 3/4] fix: satisfy clang-format in ListNode destructor error path The manually wrapped SegcoreError arguments fit within the 120-column limit, so clang-format joins them onto one line and the pre-commit hook fails in CI. Co-Authored-By: Claude Opus 5 (1M context) --- src/cachinglayer/lrucache/ListNode.cpp | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/cachinglayer/lrucache/ListNode.cpp b/src/cachinglayer/lrucache/ListNode.cpp index 0f75b24..4ce5b0e 100644 --- a/src/cachinglayer/lrucache/ListNode.cpp +++ b/src/cachinglayer/lrucache/ListNode.cpp @@ -265,8 +265,7 @@ ListNode::mark_unload(std::function cb) { // cancellation): a bare runtime_error rethrown from this // promise cannot be recognized at the cgo boundary. promise->setException(folly::exception_wrapper(milvus::SegcoreError( - milvus::ErrorCode::UnexpectedError, - "ListNode destroyed while loading, this should not happen"))); + milvus::ErrorCode::UnexpectedError, "ListNode destroyed while loading, this should not happen"))); } break; } From 03e3e2eaf89c1c71ed91d7b0d6d10f600611e063 Mon Sep 17 00:00:00 2001 From: "zhenshan.cao" Date: Sun, 26 Jul 2026 20:55:09 -0700 Subject: [PATCH 4/4] fix: make the untyped-cgo-exception observer call noexcept and best-effort Review finding: NotifyUntypedCgoException runs inside FailureCStatus's exception-to-CStatus conversion. Without a barrier, a throwing observer (e.g. allocation failure while formatting a log line) would replace the original failure and escape the cgo boundary, terminating the process. Mark it noexcept and swallow anything the observer throws so the conversion always completes. Co-Authored-By: Claude Fable 5 Signed-off-by: zhenshan.cao --- include/common/EasyAssert.h | 5 ++++- src/common/EasyAssert.cpp | 12 ++++++++++-- 2 files changed, 14 insertions(+), 3 deletions(-) diff --git a/include/common/EasyAssert.h b/include/common/EasyAssert.h index 95020fa..2f96dc4 100644 --- a/include/common/EasyAssert.h +++ b/include/common/EasyAssert.h @@ -143,8 +143,11 @@ void RegisterUntypedCgoExceptionObserver(UntypedCgoExceptionObserver observer); namespace impl { +// Best-effort, noexcept: runs inside FailureCStatus's conversion path, so a +// throwing observer must never replace the original failure or escape the +// cgo boundary. void -NotifyUntypedCgoException(const char* what); +NotifyUntypedCgoException(const char* what) noexcept; } // namespace impl inline CStatus diff --git a/src/common/EasyAssert.cpp b/src/common/EasyAssert.cpp index 0d8c512..1ff7a04 100644 --- a/src/common/EasyAssert.cpp +++ b/src/common/EasyAssert.cpp @@ -36,9 +36,17 @@ RegisterUntypedCgoExceptionObserver(UntypedCgoExceptionObserver observer) { namespace impl { void -NotifyUntypedCgoException(const char* what) { +NotifyUntypedCgoException(const char* what) noexcept { if (auto observer = untyped_cgo_exception_observer.load(std::memory_order_acquire)) { - observer(what); + try { + observer(what); + } catch (...) { + // The observer is metrics/logging only. It runs inside + // FailureCStatus's exception-to-CStatus conversion, so a throwing + // observer must never replace the original failure or let an + // exception escape the cgo boundary (which would terminate the + // process). Swallow and carry on with the conversion. + } } } } // namespace impl