enhance: classify segcore errors across producers and enforce classification end-to-end

[czs007]

What

Consume the producer-owned error classification at the segcore boundary and make the whole C++→Go classification drift-proof, so a segcore error is classified as input (caller's fault, non-retriable), transient (retriable) or permanent (non-retriable) instead of flattening to UnexpectedError(2001) or carrying the wrong retry default.

Design + tracking: #50903.

Changes

• T1 — register the storage fallback pair in pkg/util/merr/segcore.go: StorageError(2044) non-retriable, StorageTransientError(2045) retriable.

• T2 — KnowhereStatusToErrorCode → a switch with no default + -Werror=switch over the full knowhere::Status; add build-path variant KnowhereBuildStatusToErrorCode so a build-time OOM / disk read stays retriable instead of collapsing into a permanent IndexBuildError.

• T3/T4 — ArrowStatusToErrorCode delegates to the producer's milvus_storage::ToSegcoreError (retires milvus's duplicate mapper); audited and routed 25 storage arrow-status sites that were collapsing to 2001 through the single mapper (extracted to storage/StatusToErrorCode.h), always preserving the arrow sub-code in the message.

• T5 — unmapped-code observability: UnmappedSegcoreCodeTotal{code} counter + rate-limited WARN via an observer hook (merr is a leaf package); registered on QueryNode and DataNode. Unknown code degrades to non-retriable, never panics.

• T6 — codegen + compile-time enforcement: a generated SegcoreCode type (from milvus-common's EasyAssert.h) + an exhaustive classForCode switch marked //exhaustive:enforce, with the exhaustive golangci-lint enabled opt-in — a new C++ code that is not classified fails lint (the C++→Go analog of -Werror=switch).

• §3 B-tier — classify marisa and simdjson errors (build/load/parse) instead of collapsing to 2001, sub-code in the message; simdjson optional-access (NO_SUCH_FIELD/INCORRECT_TYPE) stays a benign skip; the loon_ffi FFI boundary is untouched.

• Boundary hardening (adversarial self-review of this PR's own diff) — closed the escapes that would defeat the mapping above: a throw e; slicing rethrow in LoadWithStrategy that destroyed the very codes the columnar-read mapping attaches (bare throw; now), the same slice in MinioChunkManager::PreCheck; GetCoreMetrics / EstimateLoadIndexResource / init-and-config entry points that could let an exception cross the C ABI and terminate the process; and every remaining extern-C entry that caught only std::exception now ends in catch(...) via the shared CGoCatch.h macros.

• Pin + semantics — bump milvus-storage_VERSION to 11f8a36 (the enhance: expose retryable storage errors through extend status and C FFI milvus-storage#574 merge, which also contains Add AVX flags when building faiss #575) and align the no-detail IOError expectation with the settled semantics: the producer tags every known-transient failure with a retryable ExtendStatusDetail, so a bare IOError with no detail is unclassified and deliberately falls back to permanent StorageError(2044) — a stripped-detail NotFound now degrades to non-retriable (safe) instead of retriable (retry storm on a permanent 404).

• Wire pass-through (client-visible) — a segcore error now reaches the client with its ORIGINAL code (2009 stays 2009, 2024 stays 2024) instead of collapsing to the ErrSegcore(2000) umbrella with the real code buried in the message. Family identity for errors.Is is preserved via inner/Unwrap; input/system/retriable classification unchanged. Guardrails: only in-band (2000-2099) codes pass through (garbage still collapses to 2000); cross-family mappings (2046 → wire 110) keep their sentinel's code. ErrSegcoreUnsupported/ErrSegcorePretendFinished move to the C++ values they represent (2001→2003, 2002→2033) — their old numbers squatted on C++ UnexpectedError/NotImplemented and would false-match under code-based errors.Is. Verified end-to-end on a live standalone (ef<k reaches the client as 2042, unsupported tokenizer as 2001); the three e2e assertions pinning the old 2000 updated.

• Remaining code-destroying sites — the three classes that still swallowed a producer's classification before the cgo boundary are now gone from internal/core/src and internal/core/thirdparty: status-consuming AssertInfo (104 → 0, incl. ~47 arrow builder paths whose commonest failure is OOM, now retriable MemAllocateFailed instead of a permanent 2001), bare throw std::runtime_error/logic_error/bad_alloc (68 → 0 — these were not SegcoreError, so they collapsed to 2001 and falsely fired the untyped-exception observer), and throw fmt::format(...) (12 → 0 — it throws a std::string, which catch (std::exception&) cannot see at all). tantivy's 73 AssertInfo(res.result_->success, ...) (plus 10 raw-RustResult stragglers found later) now classify the rust error — originally by its Display prefix, since replaced by a proper #[repr(i32)] discriminant carried in RustResult.error_code (see the Aug-10 update below). Typed ThrowInfo sites: 894 → 1081. The ~1500 genuine invariant asserts are untouched — 2001 is correct for them. The long-standing FIXME about err_code not surviving the nested LOON FFI boundary is also resolved, delegating to milvus_storage::ToSegcoreErrorCode rather than duplicating its table.

Verification

Verified in this PR:

• Mapping correctness (unit-tested, in-process): test_knowhere_status_mapping.cpp / test_storage_error_code.cpp / test_exec.cpp cover every mapper branch (knowhere Status incl. the build variant, arrow/extend status incl. AwsErrorNotFound→ObjectNotExist(2017), permanent-S3 vs transient), plus FailureCStatus code preservation and both observer hooks firing.
• Code projection to Go (one hop, unit-tested): segcore_test.go pins classForCode for every generated code and asserts merr.Status(err).GetRetriable() for transient codes; the T6 generator is idempotent and the exhaustive lint fails on an unclassified code.
• Full C++ suite: 8213/8223 unit tests pass locally (10 skipped; Azure connectivity tests excluded), 8648 in CI, rebased on current master (one pre-existing, unrelated concurrency test excluded: GrowingConcurrentReopenTest deadlocks deterministically on current master with or without this PR — rwlock writer starvation in growing-segment reopen code this PR does not touch; reported separately).
• Static audit (grep-verifiable): every storage arrow-status consumption site on the read path routes through ArrowStatusToErrorCode, and every extern-C boundary ends in a catch(...) tail.

Explicitly NOT verified here (follow-up):

• Runtime fault injection. No S3 throttle / 404 / OOM / corrupt-file failure has been triggered end-to-end in a running cluster. Transient codes reach Go with retriable=true (unit-tested projection), but the downstream consumption — lb_policy replica reroute on merr.IsRetryableErr, index/analyze scheduler retry — is pre-existing logic from enhance: standardize error handling on merr + Sys/Input classification #50221 and has not been driven by a real segcore transient error in this PR. This PR preserves classification for observability and correct retry defaults; the retry behavior itself is exercised only by its own pre-existing tests.

Dependencies

• milvus-common StorageTransientError(2045) — enhance: add retriable StorageTransientError(2045) error code zilliztech/milvus-common#102 merged.
• milvus-storage ToSegcoreError / packed ExtendStatusCode — enhance: classify packed extend status codes into segcore error codes milvus-storage#575 + [skip ci](shards): remove color decorator in log file, fixes #573 #574 merged; pin bumped in-tree to 11f8a36.
• knowhere three-way classification — enhance: three-way status classification; own the segcore code mapping zilliztech/knowhere#1704 merged (the milvus-side KnowhereStatusToErrorCode → thin delegate to knowhere's own ToSegcoreErrorCode is a follow-up, gated on a knowhere version bump).
• milvus-common untyped-cgo-exception observer — enhance: carry typed error codes on cachinglayer failure paths and observe untyped cgo exceptions zilliztech/milvus-common#112 merged and released as 1.0.0-1fd1160; the pin now points at the published package. All dependencies are in.

Update (Aug 10) — full-population audit, LOON path, runtime observability

The originally deferred FFI/LOON path is now done on the milvus side, and the audit was extended from the three grep-able classes to the entire 2001-producing population:

• Every remaining 2001 site read. All 1,517 AssertInfo (four sweeps: errno fingerprint, failure-keyword messages, condition morphology, and finally data provenance — does the guarded value come from disk/network?) and all 198 explicit ThrowInfo(UnexpectedError) sites. ~290 were externally-triggerable and now carry typed codes: file/remote IO -> FileOpen/Create/Read/WriteFailed (retriable), mmap/allocation -> MmapError/MemAllocateFailed (retriable), persisted-format damage (CRC/magic/parquet meta/index-meta keys) -> DataFormatBroken, deployment config -> ConfigInvalid, request content -> InvalidParameter, a cancel-race -> FollyCancel. The ~1,400 kept sites are genuine invariants or cgo contracts where 2001 is the correct report.
• Two infinite-retry bugs. Statically-impossible conditions (index_type x metric blacklist, per-type metric allowlists, json/geometry index gates) threw 2001 -> generic retry -> the build task spun forever; they now throw Unsupported, which getStateFromError maps to a terminal JobStateFailed. Missing index_type/metric_type/min_gram/max_gram keys in persisted index meta had the same loop on the load path; they are DataFormatBroken now.
• knowhere expected<> bypasses closed (8 sites in QueryResult.h/CachedSearchIterator): iterator failures went through AssertInfo and discarded the Status knowhere had already classified; they now route through KnowhereStatusToErrorCode, so an OOM/disk failure during search iteration stays retriable. Preflight rewraps in segment_c/boost_score similarly preserved the original SegcoreError code instead of flattening to 2001+string.
• tantivy discriminant over the FFI. RustResult now carries error_code (#[repr(i32)] TantivyBindingErrorCode, cbindgen-exported); the C++ mapper switches on the enum instead of parsing the Display text, and the inner tantivy::TantivyError is discriminated too (IoError/Open*Error -> Io/retriable, DataCorruption/IncompatibleIndex -> DataCorruption). Wording changes on the rust side can no longer silently degrade classification.
• LOON / FFI path (the deferred item), milvus side complete. The Go funnel HandleLoonFFIResult dropped err_code entirely and wrapped every failure as ErrLoonTransient — a 404/access-denied/corrupt-data retried as transient. It now classifies by the producer's own loon_ffi_is_retryable_errcode; permanent failures carry the new ErrLoonPermanent and terminate retry loops (pack_writer_v3 via retry.Unrecoverable; the external-refresh manager guard extended so behavior does not invert). On the C++ side LoonErrCodeToErrorCode is the single classification entry (low band -> hand table, extend band -> producer's ToSegcoreErrorCode, unknown -> producer's retryable probe), unifying the two previously-divergent ThrowIfFFIError helpers — LOON_FILE_NOT_FOUND(12) now converges to ObjectNotExist(2017) on both integration paths. Remaining LOON items (e.g. promoting FileNotFound into ExtendStatusCode) live in the milvus-storage repo.
• Regression guards. scripts/check_segcore_error_boundaries.sh wired into make static-check: every throw in internal/core/src must carry a milvus ErrorCode (zero-tolerance; currently 0 violations); vendored fmindex:: is confined to its boundary files; knowhere/arrow/milvus_storage/tantivy are ratcheted by a checked-in file-set baseline (new consumer files fail the check; shrinking is free).
• Runtime observability for what is left. milvus_cgo_unexpected_segcore_origin_total{origin="<file>:<line>"} counts every 2001 crossing the cgo boundary by its C++ source location (parsed from the at file:line suffix AssertInfo already emits, build paths collapsed to repo-relative). A site that fires in production names itself — reclassification becomes evidence-driven instead of re-reading ~1,400 asserts.

Site count for the 2001 family: 1,955 on master -> 1,525 on this branch; the delta is reclassification into actionable codes, not deletion of checks.

Deferred

• milvus-storage-side LOON improvements: promote LOON_FILE_NOT_FOUND into ExtendStatusCode, category byte (design §4.7) — tracked in the storage repo.
• knowhere-side: thin-delegate KnowhereStatusToErrorCode to knowhere's own ToSegcoreErrorCode, gated on a knowhere version bump.

issue: #50903

[czs007]

Re: the LoonResultToErrorCode build break (raised by @xiaocai)

Confirmed, and this is expected for the current draft state of this PR.

The break is real: this PR consumes LOON_IO_ERROR / LOON_DATA_ERROR, which are not defined in the pinned milvus-storage (df61720). They are added by milvus-io/milvus-storage#568. This PR is intentionally a draft blocked on #568 (see the banner at the top of the description) for exactly this reason.

On the two suggested fixes:

• Bumping the pin (your first suggestion) is the intended resolution. Once [skip ci] (shards) format logging #568 merges I'll bump milvus-storage_VERSION in internal/core/thirdparty/milvus-storage/CMakeLists.txt:17 to its merge commit and CI will go green. Re: confirming result.err_code returns these — yes: [skip ci] (shards) format logging #568's ArrowStatusToLoonCode returns LOON_IO_ERROR / LOON_DATA_ERROR, and CreateFFIResult(code, ...) writes code into result.err_code.
• Dropping the two cases would defeat the purpose of the change. Folding IO failures back into LOON_ARROW_ERROR → UnexpectedError(2001) loses their retriability (the main goal here), and routing data errors to default → UnexpectedError loses the permanent-vs-transient distinction. So I'd prefer to keep the cases and resolve via the pin bump.

Holding this PR as a draft until #568 lands.

[czs007]

Re: retry behavior change in lb_policy.go (raised by @yezh)

Confirmed — it's intended, bounded, and acceptable. One correction to the persistent-failure example.

• Bounded: retryTimes = len(shardLeaders) + max(retryOnReplica, 1) (lb_policy.go:356) — a fixed extra count, not amplification.
• Input errors still fast-fail: merr.GetErrorType(err) == InputError aborts immediately at :335 without retrying or touching the blacklist.
• Correction on "real disk corruption": that surfaces as arrow Invalid → DataFormatBroken (2024), which is not marked retriable (segcore.go:142), so it still fast-fails / blacklists exactly as before. Only transient IO (FileReadFailed 2014) and OOM (MemAllocateFailed 2034) enter the retriable branch — i.e. precisely the cases where a replica reroute can plausibly succeed.
• All-replica OOM: yes, this now takes up to retryTimes rounds before failing instead of failing fast. That's acceptable: OOM is genuinely transient (memory pressure may ease, another replica may have headroom), the request was going to fail regardless, and the extra cost is bounded.

Worth noting the prior behavior was arguably worse here: these transient failures collapsed to UnexpectedError (2001) and blacklisted the serving node — a persistent penalty on a healthy node for a transient blip. This PR stops that; a transient failure now only excludes the replica for the current request.

[xiaofan-luan]

Requesting changes. I verified each of the four findings below against the current PR head, plus the merged milvus-storage#575 and milvus-common#102. All four reproduce; file:line references are corrected to the current tree. P1-2 and P1-3 mean the PR cannot reach green CI once the dependency pins are bumped, so I'd hold merge until they're resolved.

---

P1 — DataTypeInvalid / FieldIDInvalid / FieldAlreadyExist classified as InputError, but they carry mixed semantics

pkg/util/merr/segcore.go classForCode maps CodeDataTypeInvalid(2007), CodeFieldIDInvalid(2020), CodeFieldAlreadyExist(2021) to inputError: true unconditionally. A producer audit shows these are predominantly or exclusively thrown for internal conditions:

• FieldIDInvalid(2020): 5 producer sites, all internal (load-order guards in load_field_data_c.cpp, "unsupported system field id" in SegmentInterface.cpp:725) — zero user-input sites.
• FieldAlreadyExist(2021): 2 sites, both internal — internal/core/src/segcore/load_field_data_c.cpp:65 "append same field info multi times", internal/core/src/common/GroupChunk.h:51 "Field {} already exists in GroupChunk".
• DataTypeInvalid(2007): ~90 production sites, ~90% internal default: / "logical error" guards (e.g. internal/core/src/segcore/SegmentGrowingImpl.cpp:1732 ThrowInfo(DataTypeInvalid, "logical error")); only ~7 are genuine request validation (JSON cast-type params, aggregation-over-type, search/metric mismatch).

Because they're InputError, internal/proxy/shardclient/lb_policy.go:334 aborts the cross-replica sweep:

if merr.GetErrorType(err) == merr.InputError {
    return false, err
}

So an internal/transient failure on one replica that surfaces as 2007/2020/2021 is never rerouted to a healthy replica — an availability regression.

Recommendation: default 2020 and 2021 to a non-retriable system error (they have no user-input producers at all); for 2007, either default to system error or apply WrapErrAsInputErrorWhen only at the ~7 boundaries where the value provably came from the request. Better still, split the mixed-semantics codes at the C++ source.

---

P1 — Plain Arrow IOError: test/comment claim retriable StorageTransientError(2045), but the delegated (merged) storage mapper returns permanent StorageError(2044)

internal/core/unittest/test_storage_error_code.cpp:54-55 asserts ArrowStatusToErrorCode(arrow::Status::IOError(...)) == StorageTransientError(2045) (retriable), and internal/core/src/storage/StatusToErrorCode.h comments the same. But ArrowStatusToErrorCode is a pure delegate to milvus_storage::ToSegcoreError, and the merged milvus-storage#575 (9c28242) maps it the other way, in the no-detail path:

// cpp/src/common/extend_status.cpp:203-204
} else if (status.IsIOError()) {
    code = milvus::StorageError;  // 2044, non-retriable
}

This is already an internal contradiction in this PR — the test is green only because CI still pins a pre-#575 storage build; bumping the pin to the merged revision breaks ArrowStatusToErrorCodeMapping. Net effect: the 25 plain-arrow sites stop collapsing to UnexpectedError(2001) (a real win), but plain IO lands on non-retriable 2044, so the T3/T4 retriable benefit for the live plain-arrow read path is over-stated. (#575 deliberately moved the transient/permanent split upstream into ExtendStatusDetail tags, so the no-detail path is always 2044 — there is no retriable-IO subset left here.)

Recommendation: #575's author already settled on plain-IO = permanent (the S3 SDK retry budget is already spent by the time the error propagates). So align this PR to that: change test_storage_error_code.cpp:55 to expect StorageError, fix the StatusToErrorCode.h comment, and adjust the PR description to "classification preserved; plain IO stays permanent (2044)." (If plain IO should be retriable, that's a change to #575, not here.)

---

P1 — RegisterUntypedCgoExceptionObserver is called but defined nowhere → link error

internal/core/src/segcore/segcore_init_c.cpp (SegcoreInit, hunk @@ -34,6 +36,17 @@) calls milvus::RegisterUntypedCgoExceptionObserver(...), but the symbol is not defined anywhere in this PR (only 3 call sites — 1 prod + 2 in test_storage_error_code.cpp), not in milvus-common master, and not in the merged milvus-common#102 (which only adds StorageTransientError = 2045). The ## Dependencies section doesn't list it and the conan pin isn't bumped in this diff. As it stands this fails to link (undefined reference) when building the segcore lib / all_tests, even after the #102/#575 pin bumps.

Recommendation: land the milvus-common side first (the declaration + definition, and its invocation from FailureCStatus) and add it to ## Dependencies; or provide the definition within this PR.

---

P1 — T6 drift-proofing is not a closed CI loop

generate-segcore-codes (Makefile) is a manual-only target — it is not wired into verifiers, static-check, or any workflow. The exhaustive lint and TestSegcoreCodeTableCoverage both run against the committed snapshot pkg/util/merr/segcore_codes_gen.go. So if a milvus-common pin bump adds an ErrorCode but the file isn't regenerated, the const set is unchanged, the classForCode switch is still "exhaustive", and lint + tests pass — the .golangci.yml / segcore.go comments promising "a new C++ code cannot ship unclassified" don't hold at the point that actually matters (the pin bump).

Recommendation: this repo already has the exact pattern — .github/workflows/code-checker.yaml:63 runs make check-proto-product, which regenerates and git diff --exit-codes the .pb.go files. Add an analogous check-segcore-codes-product: generate-segcore-codes that runs git diff --exit-code -- pkg/util/merr/segcore_codes_gen.go, and chain it into that same CI step. Until the loop is wired, please soften the two in-code guarantee comments. (Runtime blast radius is contained — an unmapped code degrades to non-retriable ErrSegcore and fires the observer — so this is P1 on the false in-code guarantee, not a P0.)

---

Overall: solid direction (producer-owns-classification + compile-time exhaustiveness is the right shape), but P1-2/P1-3 block a green build after the pin bumps and P1-1 is a live retry-correctness regression. Happy to pair on any of these.

[czs007]

@xiaofan-luan Thanks for the thorough pass — all four findings verified and addressed. Point-by-point (note your review raced my rebase push, so two were already fixed in flight):

P1-1 (mixed-semantics InputError → lb_policy stops rerouting): fixed in b141748. Re-ran the producer audit and it matches yours: 2020 (4 sites) and 2021 (2 sites) have zero user-input producers; 2007 is overwhelmingly internal default:/"logical error" guards. All three now default to plain non-retriable system errors (cross-replica sweep keeps going), with a comment documenting the audit and a regression test (mixed_semantics_codes_stay_system) locking them as SystemError so re-classification has to be a conscious decision. The handful of genuine request-validation sites for 2007 can be tagged at the request boundary as a follow-up.

P1-2 (plain Arrow IOError 2045 vs merged #575's 2044): already fixed in dd43e1b-lineage (now 9330aef after rebase) — the test expects StorageError(2044), StatusToErrorCode.h's comment now documents the settled semantics (producer tags known-transients with a retryable ExtendStatusDetail; bare no-detail IOError = unclassified → conservative permanent), and the PR description's Verification section states "classification preserved; no-detail IO stays permanent". Verified against the merged pin: the branch is rebased onto master's e658197 storage bump and the full unittest suite is green with it.

P1-3 (RegisterUntypedCgoExceptionObserver undefined): the definition is zilliztech/milvus-common#112 (declaration + definition + the FailureCStatus invocation), now listed in ## Dependencies. The conan pin moves to the official package once it merges — exactly your option A; the PR stays draft until then.

P1-4 (T6 not a closed CI loop): fixed in b141748 — added check-segcore-codes-product (regenerate + git diff --exit-code on segcore_codes_gen.go, mirroring check-proto-product) and chained it into the code-checker workflow step you pointed at. Verified locally: the generator resolves the pinned header and exits 0 on no drift.

One related note from the rebase: master's kCollectionSchemaVersionNotReady (2046) is minted in ChunkedSegmentSealedImpl.cpp outside milvus-common's ErrorCode enum, so the generator can't see it — it's classified via an explicit documented special-case next to the exhaustive switch, and folding it into EasyAssert.h is flagged as a follow-up. It's a live specimen of exactly the drift P1-4's gate now catches.

[xiaofan-luan]

Two additional P1 classification issues found in the current error buckets.

[xiaofan-luan]

[P1] Keep mixed-semantics native codes as SystemError

OpTypeInvalid(2022) and DataIsEmpty(2023) are not unambiguous caller-input errors. For example, PlanProto.cpp throws OpTypeInvalid while decoding an internally generated plan, where an unknown enum can indicate an internal protocol or rolling-upgrade mismatch; index builders throw DataIsEmpty when an internally scheduled build task yields zero rows. Globally setting inputError: true makes these failures reach lb_policy as InputError, which immediately aborts the cross-replica sweep. Please classify both codes as non-retriable SystemError by default. A boundary that can prove the value came directly from the user may explicitly mark that specific error as input, but the global table should remain system-classified.

[czs007]

Fixed in 3fe5bae. Producer audit confirms both: 2022 is thrown from PlanProto/expr op switches decoding internally generated plans, 2023 from index builders on internally scheduled zero/null-row builds. Both moved to the mixed-semantics system group (comment documents the audit) and the mixed_semantics_codes_stay_system regression test now locks all five codes (2007/2020/2021/2022/2023).

[xiaofan-luan]

[P1] Keep index-state Knowhere statuses as SystemError

The pinned Knowhere producers use empty_index, index_not_trained, and index_already_trained for server-side index lifecycle/state failures, while invalid_binary_set is primarily produced when deserializing a persisted index. Mapping them all to InvalidParameter(2042) incorrectly blames the request and causes lb_policy to stop replica failover. Please map the three lifecycle statuses to a non-retriable SystemError bucket (for example KnowhereError) and map invalid_binary_set to DataFormatBroken; both mappings should remain system-classified.

[czs007]

Fixed in 3fe5bae. Verified against the pinned knowhere producers (empty_index = "DiskANN not loaded" — pure server-side index state): empty_index/index_not_trained/index_already_trained → KnowhereError (system, non-retriable), invalid_binary_set → DataFormatBroken. The mapping test pins all four so a future regrouping is explicit.

[xiaofan-luan]

One additional P1 was found in the StorageV2 manifest error paths.

[xiaofan-luan]

[P1] Preserve structured StorageV2 errors on every manifest path

This overload correctly uses ArrowStatusToErrorCode, but the sibling LoadColumnGroup overload at lines 8380–8386 still checks get_chunk_reader with AssertInfo. In addition, ManifestGroupTranslator.cpp:133–145 converts failures from get_chunk_size and get_chunk_rows into std::runtime_error. Those paths erase the structured Arrow/storage status before it reaches the C ABI catch, so transient storage errors such as 2045, OOM, and corruption classifications collapse to UnexpectedError(2001). Please route all three failures through ThrowInfo(milvus::storage::ArrowStatusToErrorCode(status), ...), consistent with this overload.

[czs007]

Fixed in 3fe5bae. Both the sibling LoadColumnGroup overload (AssertInfo → ThrowInfo(ArrowStatusToErrorCode(...))) and the two ManifestGroupTranslator sites (bare std::runtime_error → same). These came in with the rebase onto current master and the earlier sweep missed them — thanks for the catch. error_code suites 10/10 after a full rebuild.

[liliu-z]

The new try/catch that wraps PayloadReader::init (lines 57-215) was added to classify malformed-file exceptions as DataFormatBroken instead of collapsing to 2001, but std::stoi on the stored dim/elementType metadata (lines 115, 165, 172) throws std::invalid_argument/std::out_of_range, which is not a parquet::ParquetException — so a corrupt dim string escapes the sole catch (line 215) and still surfaces as UnexpectedError(2001) at the cgo boundary. Widen the catch (or wrap the stoi) so a malformed dim maps to DataFormatBroken like the rest of the block. Impact is limited to error labeling/observability since both 2001 and 2024 are permanent, non-retriable codes; framing it as unrelated/pre-existing understates that the PR now owns this try/catch.

[czs007]

Fixed in 9be7068 — the three bare std::stoi calls now go through a ParseMetadataInt helper that classifies a malformed dim/elementType as DataFormatBroken; the catch itself stays narrow so unrelated failures in the 160-line block are not mislabeled as data corruption.

Also swept the rest of the PR for the same class (narrow catches whose try block can throw uncovered types, std parser additions, callee-internal parsers on the covered read paths): the eight marisa catches each wrap a single trie call, the two remaining parquet catches wrap only parquet/arrow calls, and no other std parser exists in the diff — this was the only instance. Thanks for the catch.

[xiaofan-luan]

Reviewed the boundary layer once more after the last push. Five findings — the first three are places where this PR's own contract isn't met yet, the last two are pre-existing but in the same "exception crosses the C ABI and kills the QueryNode" class this PR exists to close, so flagging them here rather than in a separate issue.

1. [P1] Analyzer FFI still hardcodes Internal, losing the discriminant this PR carries over the FFI
internal/core/thirdparty/tantivy/tantivy-binding/src/tokenizer_c.rs:23,41,67 and index_writer_text_c.rs:48

These sites convert failures via RustResult::from_error(String), which hardcodes error_code = Internal (array.rs). So a user-supplied bad analyzer_params reaches AssertTantivyOk as Internal and collapses to UnexpectedError(2001) — on the analyzer path, which is exactly where the bulk of the InvalidArgument classification in this PR lives. index_reader_c.rs already does it right via .into(); the same underlying error currently gets different codes depending on entry point. Suggest a from_binding_error_msg(&TantivyBindingError, String) (keep error.code(), allow the contextual message) and switching these four sites.

2. [P2] Four extern-C entries still catch only std::exception
internal/core/src/segcore/boost_score.cpp:277,344, segment_c.cpp:1752,1902

The catch-hardening commit message states every remaining extern-C entry that caught only std::exception now ends in catch (...), but these four were missed — a non-std exception (folly) still escapes and terminates. The two segment_c getters can just switch to CGO_CATCH_AND_RETURN_CSTATUS (also picks up the bad_alloc arm); the async boost_score preflight needs a hand-written catch (...) mirroring its existing error-future construction.

3. [P2] Delete: proto parse sits outside the try
internal/core/src/segcore/segment_c.cpp:791

make_unique<IDs>(), ParseFromArray and AssertInfo(suc, ...) run before the try, so a malformed IDs blob throws a SegcoreError straight out of a CStatus-returning function. Insert and LoadDeletedRecord parse inside their try. Fix is moving the try up three lines.

4. [P1, pre-existing] GetNumOfQueries: an empty placeholder group kills the QueryNode
internal/core/src/segcore/plan_c.cpp:121

No try/catch, and the impl is group->at(0) — std::out_of_range on an empty group crosses the C ABI. ParsePlaceholderGroup returns an empty group for a valid zero-placeholder protobuf (the loop body just never runs), and the Go side calls this entry right after a successful parse. The only guard today is proxy-side nq>0 validation; the querynode itself is one malformed internal RPC away from dying. The invariant's owner is ParsePlaceholderGroup — throw InvalidParameter there when placeholders_size() == 0 (that path is inside a CStatus channel), which fixes every downstream consumer at once (AsyncSearch currently re-guards this case by hand inside its future lambda).

5. [P1, pre-existing] HasRawData: schema-evolution race throws across a bool boundary
internal/core/src/segcore/segment_c.cpp:729

ChunkedSegmentSealedImpl::HasRawData does snapshot->schema->operator[](fieldID), which AssertInfo-throws for a field not in the published schema, plus two more reachable AssertInfos ("vector index is not ready" / "interim index is not ready"); the growing impl has the same shape. Querying a just-added field before the segment's published schema catches up (the #51062/#51814 race family) terminates the process — no malformed data needed, just normal schema-evolution timing. HasFieldData right next to it handles this via field_exists_in_schema. Either return false on schema miss in the impls, or wrap the entry with CGO_CATCH_AND_LOG + return false — "no raw data" is the safe fallback (the caller just loads the field data).

Storage-side classification comments (PayloadWriter / IndexEntryReader / loon_ffi) intentionally withheld for now — those should follow the storage error-taxonomy rework rather than being settled piecemeal here.

[sre-ci-robot]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
To complete the pull request process, please ask for approval from czs007 after the PR has been reviewed.

The full list of commands accepted by this bot can be found here.

Details

Needs approval from an approver in each of these files:

• OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[czs007]

All five addressed in 0e802b7 — rebased on top of your two commits (ee3e1e0, 028a1f3), which touch a disjoint file set, so nothing of yours was overwritten.

1. Analyzer FFI discriminant — added RustResult::from_binding_error_msg(&err, msg) (keeps error.code(), allows the contextual message) and switched all four sites: tokenizer_c.rs x3 and index_writer_text_c.rs. A bad analyzer_params now reaches AssertTantivyOk as InvalidArgument -> InvalidParameter instead of Internal -> 2001, matching what index_reader_c.rs already did.

2. Four extern-C entries — segment_c's two getters and boost_score's synchronous entry now use CGO_CATCH_AND_RETURN_CSTATUS (picks up the bad_alloc arm too); the async preflight got a hand-written catch (...) mirroring its error-future construction. You were right that the earlier commit message overclaimed. Swept the rest of segcore afterwards: plan_c/packed_writer_c/tokenizer_c already end in catch (...).

3. Delete parses inside the try now — moved as suggested.

4. Empty placeholder group — fixed at the invariant's owner as you proposed: ParsePlaceholderGroup throws InvalidParameter when placeholders_size() == 0, inside a CStatus channel, so every downstream consumer (including the AsyncSearch hand-guard) is covered at once.

5. HasRawData — entry now logs and returns false. One correction to the finding: only the sealed impl can throw here. SegmentGrowingImpl::HasRawData never consults the schema (it checks indexing_record_/insert_record_ and otherwise returns true), so the schema-miss throw is ChunkedSegmentSealedImpl::HasRawDataFromState only — my first regression test used a growing segment and passed vacuously until I noticed. The fix covers both anyway, and the test now uses a sealed segment.

Verification: new test_cabi_exception_containment.cpp covers 4 and 5, both negative-controlled by reverting the fixes (they fail without them); full C++ suite 8222 passed / 0 failed; Go analyzer/function/querynodev2-segments suites pass. TestQueryNode/TestBasic fails on this branch and on pristine master alike (wal_selector.go:56, "mq rocksmq is only valid in standalone mode") — pre-existing, unrelated to this PR.

Agreed on holding the storage-side classification (PayloadWriter / IndexEntryReader / loon_ffi) for the taxonomy rework.

[sre-ci-robot]

✅ CI Loop Results 028a1f3

Stage	Result	Duration	Tests
✅ Build	SUCCESS	16.2min	-
✅ Code-Check	SUCCESS	7.9min	-
✅ UT-Integration	SUCCESS	26.0min	-
✅ UT-GO	SUCCESS	23.4min	-
✅ UT-CPP-Cov	SUCCESS	60.7min	8655 total, 8655 passed, 0 failed

Total: 85min | Pipeline | Artifacts

Overall Coverage: 74.0%
Diff Coverage: CPP 41.8% (671 hit, 934 miss, 1605 measurable lines, 2994 unmeasured) | Go 44.5% (101 hit, 126 miss, 227 measurable lines, 4572 unmeasured)
Diff Coverage HTML: view changed lines
Go Patch Warning: WARNING: Go patch coverage is partial; 4572 changed lines were unmeasured.
Total Patch Coverage: 42.1% (772/1832 measurable lines, 7566 unmeasured)

[sre-ci-robot]

✅ CI Loop Results 0e802b7

Stage	Result	Duration	Tests
✅ Build	SUCCESS	16.0min	-
✅ Code-Check	SUCCESS	7.3min	-
✅ UT-Integration	SUCCESS	26.1min	-
✅ UT-GO	SUCCESS	23.7min	-
✅ UT-CPP-Cov	SUCCESS	61.2min	8657 total, 8657 passed, 0 failed

Total: 85min | Pipeline | Artifacts

Overall Coverage: 74.0%
Diff Coverage: CPP 41.9% (684 hit, 947 miss, 1631 measurable lines, 3116 unmeasured) | Go 44.5% (101 hit, 126 miss, 227 measurable lines, 4572 unmeasured)
Diff Coverage HTML: view changed lines
Go Patch Warning: WARNING: Go patch coverage is partial; 4572 changed lines were unmeasured.
Total Patch Coverage: 42.2% (785/1858 measurable lines, 7688 unmeasured)