enhance: define storage error taxonomy and preserve diagnostics

[xiaofan-luan]

Summary

This PR defines one native storage-error taxonomy and preserves it from the
producer to the Arrow, FFI and segcore boundaries.

• Every exported error code has exactly one category: User, Retryable,
  Conflict, DataFormat, or System.
• S3-compatible stores, Azure, filesystem configuration, packed IO, manifests,
  transactions and Vortex attach typed diagnostic codes where the cause is
  known. Unclassified failures remain conservative System errors.
• LoonFFIResult remains unchanged. Consumers derive category and retry signal
  from the existing error code and keep the code, name and message for
  diagnostics.
• External-source location/credential failures are re-tagged as user-owned only
  at entry points that know the source was supplied by the caller. Unknown file
  formats use LOON_USER_INVALID_ARGUMENT, not LOON_SOURCE_INVALID.

Aggregation contract

Independent packed, multipart and batch failures use one bounded aggregator:

• one failure preserves its original status;
• if every failure is Retryable, the retryable aggregate parent is returned;
• failures sharing a non-Retryable category select the most critical exact code
  using an explicit stable priority;
• mixed-category or unclassified failures use the non-retryable partial-failure
  parent;
• per-item messages and provider extra_info are retained with limits on item
  count, field length and total size.

This makes asynchronous S3/Azure callback order unable to change the final
error code. A retryable packed/write failure is intended for a bounded outer
operation retry, which recreates the request/writer rather than reusing a failed
writer object.

Consumer contract

• Retryable: an outer operation may retry with bounded backoff.
• Conflict: do not blindly replay; re-read/rebase through transaction-aware
  logic.
• DataFormat: persisted data could not be decoded.
• User: return the caller-owned error without treating it as a system alert.
• System and unknown future codes: do not retry generically; inspect the exact
  code and diagnostics.

Milvus companion adapters should preserve Code, Category, Name, and
Message. In particular, Conflict must not be globally mapped to a retriable
storage sentinel.

Verification

• C++ build: milvus_test and Test_FFI
• 313 focused C++ tests: 304 passed, 9 environment-dependent tests skipped
• Pure C FFI: 86 passed
• Python taxonomy: 3 passed
• cpp/scripts/check_error_table.py: 36 codes / 24 ExtendStatusCode values in sync
• clang-format 18 and git diff --check

The complete code/category table and consumer examples are in
docs/error-codes.md.

[chatgpt-codex-connector]

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 76.27%. Comparing base (63c29c6) to head (62b489e).
⚠️ Report is 5 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #603      +/-   ##
==========================================
+ Coverage   75.98%   76.27%   +0.29%     
==========================================
  Files         169      169              
  Lines       16937    16504     -433     
  Branches     2535     2473      -62     
==========================================
- Hits        12870    12589     -281     
+ Misses       4067     3915     -152     

Flag	Coverage Δ
python	45.00% <100.00%> (+0.54%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[jiaqizho]

For milvus-storage, the upper layers are segcore and some Go code that calls it through FFI. Is the purpose of classifying errors as User here to consistently report configuration errors to Milvus users?

If so, at the storage layer we cannot actually distinguish whether an error was caused by invalid user input or by an internal Milvus error.

For example, LOON_INVALID_PROPERTIES is not necessarily caused by properties set by a Milvus user; most of these properties are hard-coded in segcore. Similarly, LOON_INVALID_ARGS could also be caused by segcore calling the API incorrectly, in which case it would be an internal logic error.

[xiaofan-luan]

You're right, and this is the sharpest problem with the PR as written. Let me answer the question directly and then say what I think the fix is.

No — the category is not meant to say "show this to a human." It says where the fault lies relative to storage: outside this library, in the request we were handed. Whether that request originated from a person typing a URI or from segcore passing a null pointer is knowledge only segcore has, and you're correct that we cannot recover it here.

The problem is that the PR doesn't stop at saying that. ToSegcoreErrorCode maps every User code to 2042 InvalidParameter, and 2042 in milvus does mean "the end user's parameter is bad" and gets surfaced as such. So the mapping asserts exactly the thing you're saying storage can't know. That's a real defect, not a naming quibble.

Concretely, the User codes today are two different things:

Code	What actually happened	Who must act
LOON_SOURCE_NOT_FOUND 13, LOON_SOURCE_ACCESS_DENIED 14, LOON_SOURCE_URI_INVALID 116	a location string the user supplied	the end user
LOON_INVALID_ARGS 1, LOON_INVALID_PROPERTIES 7, PackedInvalidArgs 50	the API was called wrong	a developer

The first group is safe because those codes are only minted at the two entry points that take a user-supplied location (loon_exttable_explore, loon_exttable_get_file_info) — storage doesn't guess there, the entry point knows. The second group is exactly your case: LOON_INVALID_PROPERTIES fires on loon_properties_create's duplicate-key and bad-index checks, and most of those properties are hard-coded in segcore, so calling it a user error is wrong.

By this PR's own stated rule — one category per consumer action — those two rows need different categories, because "return it to the requester, don't alert" and "alert a developer, this is a bug" are opposite actions. So the fix I'd propose:

• 1, 7, 50 become Permanent ("our bug or the data is gone → alert a developer, never retry"), and map to 2001 UnexpectedError rather than 2042 InvalidParameter. Non-retriable either way, so no retry-behaviour change — but segcore is told "you called us wrong" instead of milvus telling its user "your parameter is bad."
• User stays, and shrinks to 13/14/116 — the codes where storage provably knows the input was a user-supplied location. User → 2042 then becomes true rather than aspirational, and the invariant test can keep enforcing it.

This costs no new category and removes the guess you're pointing at. It does change the segcore code for LOON_INVALID_ARGS, which is the highest-frequency code at the boundary, so I'd rather have your agreement before pushing it than surprise you with it.

One thing I'd push back on slightly: LOON_NOT_SUPPORT (9) is also currently User, and I think that one is defensible where it is — asking for a format this build lacks is a property of the request, and no developer action fixes it. Happy to be argued out of that too.

Separately, thanks for reading closely enough to hit this — the invariant tests in this PR would never have caught it, since they only check that the tables agree with each other.

[xiaofan-luan]

On DefaultRetryableForExtendStatusCode (raised in rounds 4-7) — settling it here.

The mechanical point is correct: the old symbol was out-of-line, so the [[deprecated]] inline forwarder an earlier revision carried restored source compatibility only — the exported symbol was gone either way. The shim was removed rather than "fixed" because the obligation itself does not exist here:

• Zero call sites. Not in this repo, not in milvus (the only grep hit is the vendored copy of this header itself), not in any downstream we ship.
• This library makes no ABI promise. There is no SOVERSION, and every consumer (milvus, via conan) pins a commit and rebuilds from source in lockstep.
• Keeping a permanently dead compatibility surface for a never-called function is exactly the kind of second source of truth this PR exists to remove.

If an out-of-tree consumer ever surfaces, the fix on their side is a one-line rename. Please treat this as settled for this PR; happy to revisit if someone can name a real consumer.

On structured Java exceptions (round 7, P2) — agreed on the end state, and it is deliberately a follow-up rather than part of this PR: java/ currently has no exception classes at all, so doing it right needs a new LoonStorageException carrying errCode / category / retryable, JNI construction plumbing, and Java build wiring — a reviewable unit of its own. Today's boundary maps invalid-args → IllegalArgumentException and OOM → OutOfMemoryError; everything else is a RuntimeException whose code lives only in the message text. Will be tracked as a follow-up issue.

[xiaofan-luan]

On the DefaultRetryableForExtendStatusCode API/ABI break — accepting it explicitly, per the waive option.

The finding is factually right and I want to record it as such rather than argue it away. At the merge base (63c29c67) the function is declared in the installed header (extend_status.h:101) and defined out-of-line (extend_status.cpp:119), so it is a real exported C++ symbol, and the current dylib exports only the new name — nm -gU libmilvus-storage.dylib | grep DefaultRetryable returns nothing. Anything compiled against the old header and linked against the new library gets an undefined symbol. That is a break, not a theoretical one.

Accepting it, for three reasons that are specific to this symbol rather than general hand-waving:

1. No consumer exists. At the merge base the only callers are inside this repo (result_c.cpp:103 and three assertions in azure_error_classification_test.cpp), all renamed by this PR. milvus consumes the C ABI (loon_*), not the C++ API; the sole grep hit there is the vendored copy of this header. No out-of-tree consumer has been identified in four rounds of looking.
2. It was already invisible in the shipped configuration. cpp/ffi_exports.map lists only loon_* symbols, so under WITH_PYTHON_BINDING the version script hid this mangled C++ symbol at the merge base too.
3. This library makes no ABI promise. No SOVERSION; consumers pin a commit via conan and rebuild in lockstep.

Restoring a non-inline forwarder would mean keeping a permanently dead exported symbol for a function nothing calls — the kind of second surface this PR exists to remove. If a real consumer surfaces the fix on their side is a one-line rename. Marking this waived.

[jiaqizho]

Is the current PR ready for review?

[sre-ci-robot]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: xiaofan-luan
To complete the pull request process, please assign sunby after the PR has been reviewed.
You can assign the PR to them by writing /assign @sunby in a comment when ready.

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

[sre-ci-robot]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: xiaofan-luan
To complete the pull request process, please assign sunby after the PR has been reviewed.
You can assign the PR to them by writing /assign @sunby in a comment when ready.

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