Skip to content

enhance: carry typed error codes on cachinglayer failure paths and observe untyped cgo exceptions - #112

Merged
zhengbuqian merged 4 commits into
zilliztech:masterfrom
czs007:enhance-typed-cachinglayer-errors
Jul 27, 2026
Merged

enhance: carry typed error codes on cachinglayer failure paths and observe untyped cgo exceptions#112
zhengbuqian merged 4 commits into
zilliztech:masterfrom
czs007:enhance-typed-cachinglayer-errors

Conversation

@czs007

@czs007 czs007 commented Jul 24, 2026

Copy link
Copy Markdown
Contributor

Part of the error-handling hardening tracked in milvus-io/milvus#50903 (see also milvus-io/milvus#50768, which consumes both changes).

1. ListNode: loading-in-destructor is an invariant violation, not a cancel

ListNode::~ListNode finding a cell still in LOADING state is an internal invariant violation — the slot must not be torn down while a load is in flight. It previously threw a bare std::runtime_error, which collapses to UnexpectedError(2001) at the cgo boundary; now it throws a typed SegcoreError(UnexpectedError) explicitly, keeping the intent readable and the throw site consistent with the cancel paths that #111 already typed as FollyCancel.

2. Observer hook for untyped exceptions at the cgo boundary

FailureCStatus recovers the typed code via dynamic_cast<SegcoreError>; anything else (a raw std::exception from a third-party lib, a sliced rethrow) silently collapses to 2001 and is invisible in production.

This adds RegisterUntypedCgoExceptionObserver(cb): FailureCStatus invokes the callback whenever it hits a non-SegcoreError exception. milvus registers a Prometheus counter (internal_cgo_untyped_exception_total) plus a rate-limited warn at SegcoreInit, so "untyped exception rate should trend to zero" becomes a measurable claim instead of a hope. No behavior change when no observer is registered; a single atomic load on the failure path only.

Verification

🤖 Generated with Claude Code

czs007 and others added 2 commits July 24, 2026 14:15
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 <noreply@anthropic.com>
Signed-off-by: zhenshan.cao <zhenshan.cao@zilliz.com>
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 <noreply@anthropic.com>
Signed-off-by: zhenshan.cao <zhenshan.cao@zilliz.com>
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) <noreply@anthropic.com>
Comment thread src/common/EasyAssert.cpp Outdated
void
NotifyUntypedCgoException(const char* what) {
if (auto observer = untyped_cgo_exception_observer.load(std::memory_order_acquire)) {
observer(what);

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.

[P1] Prevent observer exceptions from escaping the cgo boundary

FailureCStatus must complete the exception-to-CStatus conversion, but this synchronous metric/log observer is called without an exception barrier. If the observer throws, for example during allocation or formatting, the original failure is replaced and the exception can escape the cgo boundary, terminating the process instead of returning UnexpectedError. Make NotifyUntypedCgoException best-effort and noexcept, and catch all exceptions from the observer.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Good catch — fixed in 03e3e2e. NotifyUntypedCgoException is now noexcept and wraps the observer call in catch (...): the observer is metrics/logging only, so a throwing observer must never replace the original failure or let an exception escape mid-conversion at the cgo boundary. make test green.

…ffort

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 <noreply@anthropic.com>
Signed-off-by: zhenshan.cao <zhenshan.cao@zilliz.com>
@zhengbuqian
zhengbuqian merged commit 1fd1160 into zilliztech:master Jul 27, 2026
3 checks passed
czs007 pushed a commit to czs007/milvus that referenced this pull request Jul 30, 2026
zilliztech/milvus-common#112 (the untyped-cgo-exception observer this PR
registers at SegcoreInit) is released as 1.0.0-1fd1160, so the pin can
move off the pre-release placeholder. Without it the C++ build failed to
link with "'RegisterUntypedCgoExceptionObserver' is not a member of
'milvus'".

Verified locally against the published recipe with a cleaned conan
generator directory: full build clean, error-code suites 10/10.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Signed-off-by: Zack <noreply@zilliz.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants