enhance: route arrow Status to finer LOON FFI error codes

[czs007]

What

Every arrow failure crossing the loon FFI boundary collapsed to LOON_ARROW_ERROR, so a consumer (milvus segcore) could not tell a transient failure from a permanent one and had to treat them all as one opaque error.

This PR adds an ArrowStatusToLoonCode helper (ffi_internal/result.h) and routes the RETURN_ERROR(LOON_ARROW_ERROR, ...) sites through it so the failure category survives the FFI boundary:

arrow::Status	LOON code	meaning
IsOutOfMemory()	LOON_MEMORY_ERROR	transient, retriable
IsIOError()	LOON_IO_ERROR (new)	transient object-storage failure (throttling / timeout / reset), retriable
IsInvalid() / IsTypeError() / IsKeyError()	LOON_DATA_ERROR (new)	malformed / corrupt data on disk, permanent
everything else	LOON_ARROW_ERROR	unclassified

Two new codes (LOON_IO_ERROR, LOON_DATA_ERROR) are added to the FFI enum (ffi_c.h) with their error_to_string entries. Applied across all FFI entry points (reader / writer / segment / manifest / exttable / filesystem).

Why

The codes are consumed on the milvus side, which maps each LOON code onto the matching segcore ErrorCode (LOON_IO_ERROR -> FileReadFailed retriable, LOON_DATA_ERROR -> DataFormatBroken permanent, ...). Without the finer LOON codes a transient S3 IO failure was indistinguishable from permanent data corruption and could not be retried / rerouted.

Pairs with

The milvus-side consumer change (separate PR on milvus-io/milvus) that reads these LOON codes at segcore's loon FFI boundary. Context: milvus error-handling standardization, milvus-io/milvus#47420.

Verified

• Builds cleanly (make build, produces libmilvus-storage.so).
• clang-format-18 clean.

[sre-ci-robot]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: czs007
To complete the pull request process, please assign tedxu after the PR has been reviewed.
You can assign the PR to them by writing /assign @tedxu 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

[codecov]

Codecov Report

❌ Patch coverage is 15.95745% with 79 lines in your changes missing coverage. Please review.
✅ Project coverage is 74.93%. Comparing base (73d564c) to head (ce7da87).

Files with missing lines	Patch %	Lines
cpp/src/ffi/filesystem_c.cpp	17.85%	23 Missing ⚠️
cpp/src/ffi/reader_c.cpp	23.52%	13 Missing ⚠️
cpp/src/ffi/segment_reader_c.cpp	0.00%	12 Missing ⚠️
cpp/src/ffi/exttable_c.cpp	0.00%	8 Missing ⚠️
cpp/src/ffi/segment_writer_c.cpp	0.00%	8 Missing ⚠️
cpp/src/ffi/v2_packed_writer_c.cpp	0.00%	6 Missing ⚠️
cpp/src/ffi/writer_c.cpp	0.00%	6 Missing ⚠️
cpp/src/ffi/manifest_c.cpp	0.00%	3 Missing ⚠️

❌ Your patch check has failed because the patch coverage (15.95%) is below the target coverage (60.00%). You can increase the patch coverage or adjust the target coverage.

Additional details and impacted files

@@           Coverage Diff           @@
##             main     #568   +/-   ##
=======================================
  Coverage   74.93%   74.93%           
=======================================
  Files         160      160           
  Lines       15355    15359    +4     
  Branches     2340     2341    +1     
=======================================
+ Hits        11507    11510    +3     
- Misses       3848     3849    +1     

Flag	Coverage Δ
cpp	77.77% <15.95%> (-0.01%)	⬇️
python	44.76% <ø> (ø)

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.

[xiaofan-luan]

Review: right direction, but the classification is done at the FFI boundary while the error category is already destroyed in the core/format layers

This PR splits LOON_ARROW_ERROR by arrow StatusCode (IO / DATA / MEMORY) — the direction is exactly right, and the static_assert + unit test are solid. But after tracing the whole error path, ArrowStatusToLoonCode is either bypassed or actively misclassifies on the hottest read path, because it relies on the lower layers preserving the StatusCode, and the core/format layers do not.

One guiding principle (for all follow-up changes)

1. Storage's own code must not throw — always return arrow::Status / arrow::Result and preserve the correct StatusCode. In particular, never write throw std::runtime_error(status.ToString()), which stringifies an arrow::Status and throws away its StatusCode.
2. Exceptions thrown by external systems (the Rust cxx bridges vortex/lance/iceberg, avro, arrow internals) must be caught at the immediate C++ call site, and must distinguish an IO exception from a format/data exception, converting to Status::IOError (transient, retriable) or Status::Invalid (permanent data error) respectively, with distinct error messages. Only then can this PR's ArrowStatusToLoonCode carry the category through to segcore.

---

P0 — correctness bugs that exist today (and that this PR will misclassify or be bypassed by)

1. src/packed/reader.cpp:69-72 — an IO failure is rewritten to Status::Invalid, which this PR turns into a permanent error

auto result = MakeArrowFileReader(*fs, path, ...);   // opens a parquet file on object storage
if (!result.ok()) {
  return arrow::Status::Invalid(fmt::format("Error making file reader ... {}", result.status().ToString()));
}

The most common failure of MakeArrowFileReader is S3 throttling / timeout / reset, which arrow surfaces as Status::IOError. Unconditionally rewriting it to Invalid means, after this PR, Invalid → LOON_DATA_ERROR (permanent, not retriable). This is the exact opposite of the PR's goal: a transient S3 read failure that should be retried gets labeled "permanent on-disk data corruption". Should just return result.status(); to preserve the IOError.

2. src/packed/reader.cpp:51-55 — the constructor does throw status.ToString(), bypassing the whole classification

auto status = init(...);
if (!status.ok()) { LOG_STORAGE_ERROR_ << ...; throw std::runtime_error(status.ToString()); }

The arrow::StatusCode is stringified and lost here, so the FFI boundary can only catch it as LOON_GOT_EXCEPTION(5) — ArrowStatusToLoonCode never runs. The packed reader, a core read path, completely bypasses this PR. Should become a factory returning arrow::Result<> (or carry the Status in the exception), not stringify it.

3. src/format/iceberg/iceberg_format.cpp:27,38 (+ iceberg_common.cpp:153,155) — throw escapes through an arrow::Result function
IcebergFormat::explore returns arrow::Result<> but calls two throwing functions with no protection:

• iceberg::ToStorageOptions (iceberg_common.cpp:153/155 does throw std::runtime_error for an unknown cloud provider)
• iceberg::PlanFiles (throws IcebergException)

Fix: change ToStorageOptions to return arrow::Result<...> and return Status::Invalid for an unknown provider (a genuine config error); wrap PlanFiles in try/catch at the call site and convert to Status::IOError (external-system exception, classified by IO vs format), matching how the vortex/lance readers already do it.

4. src/format/lance/lance_common.cpp:132,134 — same as iceberg
ToStorageOptions does throw std::runtime_error for an unknown provider, called unprotected from lance_table_reader.cpp:132's load_metadata (arrow::Result). Change it to return Result, with Status::Invalid for an unknown provider.

5. src/format/vortex/vortex_translater.cpp:180 — another throw status.ToString()
On the read path, stringifies an arrow::Status and throws it — same as #2. Should propagate/return the Status, not throw runtime_error(status.ToString()).

---

P1

6. ArrowStatusToLoonCode only looks at StatusCode, never reads ExtendStatusDetail
common/extend_status.h already carries finer categories via StatusDetail (TxnExhaustedRetry/TxnResolutionFailed, AwsErrorConflict/PreConditionFailed). ArrowStatusToLoonCode ignores the detail, so those finer categories are lost when mapping to a LOON code. Suggest checking ExtendStatusDetail first, then falling back to StatusCode, and mapping Txn/Aws to the matching LOON codes.

7. src/packed/column_group.cpp:35,50 — a logic error misusing Status::IOError

return arrow::Status::IOError("ColumnGroup::AddRecordBatch: batch is null");

A null batch is a logic error, but after this PR it becomes LOON_IO_ERROR (retriable) → the caller will keep retrying a logic error that can never succeed. Should be Status::Invalid.

8. src/filesystem/gcp/gcp_filesystem_producer.cpp:183 / s3/s3_filesystem_producer.cpp:128 — init errors wrapped as Status::Invalid
Filesystem init failures (credentials / network) are wrapped as Invalid → permanent DATA_ERROR after this PR. Also the GCP message says "failed to initialize S3" (copy-paste). Classify by nature: network/IO → IOError, config → Invalid.

9. Local catching of external-system exceptions must be completed and made uniform
vortex (vortex_format_reader.cpp:206, etc.) and lance (lance_table_reader.cpp:135, etc.) already catch → Status at the bridge's C++ call site; iceberg does not (i.e. #3). Make it a uniform rule: every Rust cxx bridge / avro throw must be caught at the immediate call site and converted to a Status that distinguishes IO vs format exceptions.

10. Fault-injection Status::IOError gets re-classified
writer.cpp:327/364/401/507 and manifest.cpp:603 use Status::IOError for fault injection, which after this PR now becomes LOON_IO_ERROR instead of LOON_FAULT_INJECT_ERROR. Please confirm the fault-injection tests don't assert on the specific code and break.

11. Two gaps at the FFI boundary

• loon_reader_set_keyretriever (reader_c.cpp:429) has no try/catch; the registered lambda, if it throws, crosses the FFI boundary = UB. Needs a try/catch. (reader_c.cpp:462 even left a // TODO: make sure which exception will be throw.)
• ArrowStatusToLoonCode doesn't cover IsCancelled (cancellation) / IsCapacityError (capacity, usually retriable); both fall into the generic LOON_ARROW_ERROR today and could be mapped.
• catch style is inconsistent: reader/manifest/writer use catch (std::exception&) (non-const), filesystem/bridge use const std::exception&; suggest standardizing on const ref.

---

In one sentence

This PR does the classification at the FFI boundary, but Invalid is used as a generic catch-all throughout this repo (IO failures get wrapped into it too), and the core/format layers frequently throw status.ToString(), destroying the StatusCode upstream. For this error-code rework to actually land, the key fixes are the P0 items in the core/format layers (don't throw, don't stringify, classify by IO vs format) — not just the boundary helper. Suggest landing P0 as a prerequisite/follow-up PR, and adding a CI grep that blocks throw (excluding the vendored azurefs.cc, the cxx bridges, and tests).

[xiaofan-luan]

Follow-up: a full-chain retryability audit — the codebase misclassifies in both directions depending on the format

After going through every arrow::Status construction site in src/ with one lens — can the consumer tell a transient failure (object storage unavailable → retry) from a permanent one (corrupt data → don't retry)? — the picture is worse than a few isolated sites: the same "object storage is down" failure is classified into opposite categories depending on which format path it travels through.

First, the correct baseline: Parquet

The Parquet read path is essentially right and is the model to copy. It uses ARROW_ASSIGN_OR_RAISE / ARROW_RETURN_NOT_OK, which propagate arrow's underlying IOError unchanged; the Status::Invalid("Failed to ...") sites in parquet_format_reader.cpp (e.g. :87, :300, :305, :311, :466, :599) are null/range guards that run after ARROW_RETURN_NOT_OK — i.e. genuine logic/data errors, correctly permanent. ✅

The systemic problem: every other path either drops the code or hard-codes one

Path	What it does with an IO failure	Effect	Sites
Parquet ✅	ARROW_RETURN_NOT_OK propagates IOError; Invalid only as null/range guard	correct	—
Packed reader ❌ → permanent	manual if (!ok) return Status::Invalid(... status.ToString()), discards IOError	transient S3 failure → DATA_ERROR, not retried	packed/reader.cpp:71 (already noted P0)
FS producers (S3/GCP init) ❌ → permanent	wraps init failure as Invalid	credential/network blip → permanent	s3_filesystem_producer.cpp:128, gcp_filesystem_producer.cpp:183 (already noted P1)
Vortex ❌ → transient	every catch (VortexException) → Status::IOError	a corrupt vortex file (permanent) is labeled retriable → retried forever	vortex_format_reader.cpp:210,219,228,629,655,680,727; vortex_footer_reader.cpp:311,382,407,418,537; vortex_writer.cpp:68,86
Lance ❌ → transient	same catch → IOError	corrupt lance fragment retried forever	lance_table_reader.cpp:138,147,158,226,257,285

So Packed / FS-init freeze transient errors into permanent ones (won't retry when they should), while Vortex / Lance treat permanent errors as transient (retry forever on corrupt data). Both violate the retry-vs-don't-retry contract, in opposite directions.

Root cause for Vortex/Lance: the Rust bridge throws away the category (can't be fixed in C++)

class VortexException : public std::runtime_error { ... };   // message string only, no kind
class LanceException  : public std::runtime_error { ... };   // same

The Rust side is built on opendal (opendal 0.55), which carries an ErrorKind that does separate transient IO (RateLimited/Unexpected/timeout) from NotFound/decode errors. But the cxx bridge collapses every Rust error into a string-only exception, so the C++ catch has no category to act on and blindly picks IOError.

This means the Vortex/Lance retry classification cannot be fixed at the C++ layer — the fix has to be in the Rust bridge: surface the opendal ErrorKind (either as distinct exception types, or an is_transient/kind field on VortexException/LanceException), then have the C++ catch map opendal-IO → IOError and decode/footer-corruption → Invalid. Until that exists, the blanket IOError is a default that retries permanently-corrupt data forever, and it should be called out as such in this PR.

Smaller but real

• s3_client.cpp:182, :202 — "Bucket not found" → Status::IOError (retriable). A missing bucket is a permanent config error; retrying will never make it appear. Should be a non-retriable category.
• packed/column_group.cpp:35, :50 — null batch / merge failure → IOError (a logic error mislabeled retriable); same inverse error as above (already noted P1).
• common/arrow_util.cpp:185,187,193 — env-var-too-long → CapacityError (unmapped by ArrowStatusToLoonCode, falls to generic); env-var-undefined → KeyError (→ DATA_ERROR, acceptable as a permanent config error). Worth an explicit mapping.
• Correct examples to copy (no change needed): vortex_footer_reader.cpp:196 and vortex_format_reader.cpp:114 use Status::KeyError for a missing field/projection → DATA_ERROR (correctly permanent); all the Parquet Invalid guards are correct.

Two items to add to the plan

• New P0 — Vortex/Lance bridge: surface opendal ErrorKind across the cxx boundary and classify IO vs decode at the catch site. Until then, explicitly document the blanket IOError as a known "will retry corrupt data" behavior.
• New P1 — s3_client.cpp bucket-not-found: reclassify as non-retriable.

One sentence

The retry contract this PR is trying to establish at the FFI boundary is undermined upstream by a layer that classifies in both wrong directions — Packed/FS-init force transient → permanent, Vortex/Lance force permanent → transient — and the Vortex/Lance half is structurally unfixable in C++ because the Rust bridge discards opendal's ErrorKind. Parquet already shows the right pattern (propagate arrow status via ARROW_RETURN_NOT_OK, reserve Invalid for true logic/data errors); the others should converge on it.

[liliu-z]

result.h:84 maps arrow Invalid/TypeError/KeyError to LOON_DATA_ERROR, but these statuses originate from caller/config validation paths (schema import, ConvertWriterConfig, URI parse, policy create), not on-disk corruption. LOON_DATA_ERROR signals to callers that their stored data is permanently corrupt, so a user who merely passes a bad schema or misconfigured writer receives a data-corruption error instead of an actionable invalid-argument one. Route these statuses to a validation/invalid-argument code rather than LOON_DATA_ERROR.

[liliu-z]

This PR adds LOON_IO_ERROR (13) and LOON_DATA_ERROR (14) and bumps LOON_ERRORCODE_MAX to 15 in ffi_c.h, but the Python mirror of this enum in python/milvus_storage/_ffi.py:27 still hardcodes LOON_ERRORCODE_MAX = 13 and omits both new codes. There is no runtime effect today because check_result only embeds the raw err_code in the message and never branches on the constants, but the mirror is now stale and will mislead anyone reading or extending the bindings. Add LOON_IO_ERROR/LOON_DATA_ERROR and set LOON_ERRORCODE_MAX = 15 in _ffi.py.

[jiaqizho]

In ARROW, arrow::Status::IOError is not broken down into sub-errors. For example, incorrect formatting, S3 permissions, and most S3 issues are wrapped as arrow::Status::IOError. So LOON_IO_ERROR is not retryable actully.

I think only two types of errors can be retried here:

1. The Filesystem retry failure, then the caller can retry.
2. Multiupload related errors, such as no_such_upload (frequent in Minio), these require the upper layer to retry.

I will correct this part of error in extend_status.h and expose it in FFI.

[xiaofan-luan]

Follow-up: the deserialization layer signals data corruption via C++ exceptions, not arrow::Status — and an exception must still become LOON_DATA_ERROR, not LOON_GOT_EXCEPTION

There's a layer below the arrow::Status paths this PR touches: the on-disk parquet/packed metadata deserialization code expresses "the data is corrupt" by throwing a C++ exception, not by returning a Status. So a corrupt file — which is the textbook permanent, non-retriable LOON_DATA_ERROR — never reaches ArrowStatusToLoonCode at all.

// common/metadata.cpp:203,206 — RowGroupMetadata::Deserialize (returns a value, not Status)
if (tokens.size() != 3) throw std::runtime_error("Invalid row group metadata format");
return RowGroupMetadata(std::stoull(tokens[0]), std::stoll(tokens[1]), std::stoll(tokens[2]));
                     //  ↑ a non-numeric token → std::stoll throws std::invalid_argument

Same class of site (all parse disk- or property-controlled input, all can throw):

• common/metadata.cpp:61,155 — std::stoll(field_id) parsing field ids; :206 — row-group metadata
• common/metadata.cpp:39,118,216 — FieldIDList::Get(i) / RowGroupMetadataVector::Get(i) throw std::out_of_range, reached from PackedFileMetadata::Make (returns arrow::Result) at :321
• format/parquet/file_reader.cpp:170 — field_id_mapping.at(field_id); :169 — field_id_list.Get(i)
• format/iceberg/iceberg_format.cpp:30 — std::stoll(snapshot_str); format/lance/lance_common.cpp:154 — std::stoull(...); properties.cpp:675 — property_infos.at(key)

Two consequences, both hitting the retry contract

1. The corruption signal is downgraded to "unknown exception". These all sit inside arrow::Result call chains, but there's no try/catch on the way up, so the exception propagates to the FFI boundary, gets caught by catch (std::exception&), and RETURN_EXCEPTION collapses it to LOON_GOT_EXCEPTION. A clean "file is corrupt → permanent" becomes an uncategorized error the consumer can't act on.
2. On a reader worker thread it's std::terminate. This layer is reached from parallel take / get_chunks running on the thread pool. The FFI try/catch is on the calling thread and cannot catch an exception thrown on a worker thread → the process aborts. A single corrupt parquet file can crash the process instead of returning an error code.

The ask: even on the exception path, surface LOON_DATA_ERROR

Two levels, do both:

• Preferred — don't throw for corrupt data. Add a small helper, e.g. arrow::Result<int64_t> ParseInt64(std::string_view) that wraps std::stoll in try/catch and returns Status::Invalid on failure; make Get(i) / Deserialize(...) return arrow::Result<...>. Corrupt input then flows as Status::Invalid → LOON_DATA_ERROR through the existing ArrowStatusToLoonCode. This is exactly the "corrupt data = permanent" semantics this PR is establishing.

• Also — make the FFI exception path classify, not collapse. Even after the above, an exception can still escape (a .at(), a third-party throw). Right now RETURN_EXCEPTION maps every exception to LOON_GOT_EXCEPTION. Add an ExceptionToLoonCode(const std::exception&) companion to ArrowStatusToLoonCode so the catch blocks classify by type instead of collapsing — at minimum:

  • std::invalid_argument / std::out_of_range / a dedicated data-corruption exception type → LOON_DATA_ERROR
  • std::bad_alloc → LOON_MEMORY_ERROR
  • everything else → LOON_GOT_EXCEPTION

  Best is a typed exception (e.g. throw a DataCorruptionError from the deserialization layer) so the mapping is intentional rather than guessing from std types.

• Independently, the worker-thread terminate risk should be closed by making the parse path return Status (the preferred fix) or by catching within the worker before the result is joined.

Net: a corrupt file must end up as LOON_DATA_ERROR whether it travels the Status path or the exception path — never as LOON_GOT_EXCEPTION, and never as a process abort.