enhance: expose retryable storage errors through extend status and C FFI

[jiaqizho]

Storage failures used to mostly come back through the C API as generic Arrow errors, so callers could not cleanly tell whether restarting the whole read or write was worth trying. This change adds explicit transient storage extend statuses and carries a retryable flag in the status detail, while keeping existing non-retryable S3 cases like conflicts and precondition failures separate.

S3 error translation now classifies the cases we actually want outer callers to retry after the AWS SDK has already returned its final error. NoSuchUpload is treated as a retryable upload-state failure, and transient network, timeout, throttling, and service-side failures are mapped into dedicated storage transient statuses. Other S3 failures still fall back to plain Arrow errors or existing non-retryable extend statuses, so the retry signal stays intentional instead of being inferred from every storage error.

The C FFI now exposes dedicated storage transient errcodes and adds loon_ffi_is_retryable_errcode, so callers can ask the API directly instead of duplicating the mapping. The FFI return helpers also route Arrow statuses with extend status details through the same conversion path across filesystem, reader, writer, manifest, and external table APIs, while preserving the normal fallback code for plain Arrow failures.

[codecov]

Codecov Report

❌ Patch coverage is 84.63542% with 59 lines in your changes missing coverage. Please review.
✅ Project coverage is 75.38%. Comparing base (9c28242) to head (d1cbd71).
⚠️ Report is 2 commits behind head on main.

Files with missing lines	Patch %	Lines
cpp/src/format/vortex/vortex_format_reader.cpp	76.59%	22 Missing ⚠️
cpp/src/ffi/filesystem_c.cpp	64.51%	11 Missing ⚠️
cpp/src/format/vortex/vortex_footer_reader.cpp	71.05%	11 Missing ⚠️
cpp/src/ffi/reader_c.cpp	83.33%	3 Missing ⚠️
python/milvus_storage/_ffi.py	70.00%	3 Missing ⚠️
cpp/src/ffi/v2_packed_writer_c.cpp	66.66%	2 Missing ⚠️
cpp/include/milvus-storage/format/format_reader.h	0.00%	1 Missing ⚠️
cpp/src/common/extend_status.cpp	96.15%	1 Missing ⚠️
cpp/src/ffi/result_c.cpp	87.50%	1 Missing ⚠️
cpp/src/ffi/segment_writer_c.cpp	88.88%	1 Missing ⚠️
... and 3 more

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #574      +/-   ##
==========================================
+ Coverage   74.82%   75.38%   +0.56%     
==========================================
  Files         162      164       +2     
  Lines       15600    15574      -26     
  Branches     2376     2336      -40     
==========================================
+ Hits        11672    11741      +69     
+ Misses       3928     3833      -95     

Flag	Coverage Δ
cpp	78.23% <85.02%> (+0.64%)	⬆️
python	44.44% <70.00%> (-0.33%)	⬇️

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]

Overall the direction is right — an explicit transient taxonomy plus a retryable signal through the C FFI is exactly what upper layers need, and replacing VortexException with arrow::Status across the Rust bridge is a solid cleanup. A few issues should be addressed before merge though. Note the milvus side also needs a companion change before this signal is actually consumed (register 2045 in merr's segcore code table, narrow classifyLoonErr's treat-everything-as-transient logic to use err_code / loon_ffi_is_retryable_errcode, and stop flattening err_code in ThrowIfFFIError); we'll track that in the milvus repo.

P0: AwsErrorNoSuchUpload should not be marked retryable

NoSuchUpload means the multipart upload id is gone (aborted/expired). Retrying the same call on the same writer fails deterministically forever — only restarting the whole write with a fresh upload can succeed. retryable=true invites consumers to spin on a dead upload id. Suggest changing it in all three places together so they stay consistent:

• kExtendStatusCodeMetadata: retryable=false
• s3_internal.h: move the NO_SUCH_UPLOAD case out of tryMakeRetryableExtendArrowError into the permanent classifier
• ToSegcoreErrorCode: map back to StorageError (2044) instead of 2045

Callers that recognize code 101 can still decide to rebuild the upload and rewrite at their own level.

P0: transient HTTP fallback is missing 429 (TOO_MANY_REQUESTS)

tryMakeRetryableExtendArrowError's HTTP fallback only covers 408/500/502/503/504. Rate limiting from S3-compatible backends often arrives as HTTP 429 with an exception name the SDK doesn't recognize (error_type == UNKNOWN):

• when ShouldRetry() == true, it falls into the last fallback and gets mislabeled StorageTransientNetwork — throttling handled with fast network-style retries makes the throttling worse;
• when ShouldRetry() == false, it falls through to plain IOError and rate limiting becomes a permanent failure.

One-line fix: add TOO_MANY_REQUESTS -> StorageTransientThrottling to the HTTP switch.

Beyond 429, it's worth auditing the throttling signal of each object store we support, since every vendor throttles differently and the current recognizers only cover the AWS/MinIO shapes (THROTTLING/SLOW_DOWN enums, "SlowDown"/"SlowDownWrite" names): e.g. AWS S3 uses 503 SlowDown, GCS XML API uses 429, MinIO uses 503 SlowDown/SlowDownWrite, and OSS/COS have their own error codes on 429/503. Both channels (HTTP code and exception name) need entries per backend, otherwise a vendor's throttling silently degrades to non-retryable IOError.

Also, when the backend does throttle us, the response usually carries an explicit Retry-After hint (both 429 and 503 SlowDown may include it; the SDK exposes it via AWSError::GetResponseHeaders()). The translation layer currently discards it — the extend status only carries code + message, so the outer retry can only guess its own backoff, which tends to hammer an already-throttling backend. Suggest capturing Retry-After into ExtendStatusDetail (e.g. an optional retry_after_ms field) and exposing it through the FFI result, so consumers honor the server-requested delay instead of inventing their own schedule. Fine as a follow-up PR if it's too much scope here, but the header shouldn't be silently dropped at the translation layer.

P1: two not-found paths still bypass the structured codes (pre-existing, but worth closing in this PR since it establishes the taxonomy)

• EnsureHeadObject returns plain PathNotFound on not-found (s3_filesystem.cpp, both call sites), bypassing the AwsErrorNotFound tagging in ErrorToStatus — so a missing object surfaces as a generic storage error instead of ObjectNotExist.
• vortex_footer_reader.cpp ResolveFileSize turns a missing file into arrow::Status::Invalid, which the segcore mapping classifies as DataFormatBroken (2024, data corruption) — wrong semantics for "object does not exist".

P1: public C header lost its stable error-code constants

The #define LOON_* values moved to an internal header and the public ffi_c.h now only exposes extern const int symbols (txn codes also renumbered 10/11 -> 111/112). I verified no existing consumer hardcodes the old values and the codes are never persisted, so this isn't breaking today — but extern const int can't be used as C switch/case labels and macro-style access from cgo is gone, which is a usability regression for C/C++ consumers. Suggest keeping a stable enum/macro set in the public header (generated from the same source as ffi_error_code.h so they can't drift), with the exported symbols kept as the dynamic-language channel.

[jiaqizho]

Overall the direction is right — an explicit transient taxonomy plus a retryable signal through the C FFI is exactly what upper layers need, and replacing VortexException with arrow::Status across the Rust bridge is a solid cleanup. A few issues should be addressed before merge though. Note the milvus side also needs a companion change before this signal is actually consumed (register 2045 in merr's segcore code table, narrow classifyLoonErr's treat-everything-as-transient logic to use err_code / loon_ffi_is_retryable_errcode, and stop flattening err_code in ThrowIfFFIError); we'll track that in the milvus repo.

P0: AwsErrorNoSuchUpload should not be marked retryable

NoSuchUpload means the multipart upload id is gone (aborted/expired). Retrying the same call on the same writer fails deterministically forever — only restarting the whole write with a fresh upload can succeed. retryable=true invites consumers to spin on a dead upload id. Suggest changing it in all three places together so they stay consistent:

• kExtendStatusCodeMetadata: retryable=false
• s3_internal.h: move the NO_SUCH_UPLOAD case out of tryMakeRetryableExtendArrowError into the permanent classifier
• ToSegcoreErrorCode: map back to StorageError (2044) instead of 2045

Callers that recognize code 101 can still decide to rebuild the upload and rewrite at their own level.

P0: transient HTTP fallback is missing 429 (TOO_MANY_REQUESTS)

tryMakeRetryableExtendArrowError's HTTP fallback only covers 408/500/502/503/504. Rate limiting from S3-compatible backends often arrives as HTTP 429 with an exception name the SDK doesn't recognize (error_type == UNKNOWN):

• when ShouldRetry() == true, it falls into the last fallback and gets mislabeled StorageTransientNetwork — throttling handled with fast network-style retries makes the throttling worse;
• when ShouldRetry() == false, it falls through to plain IOError and rate limiting becomes a permanent failure.

One-line fix: add TOO_MANY_REQUESTS -> StorageTransientThrottling to the HTTP switch.

Beyond 429, it's worth auditing the throttling signal of each object store we support, since every vendor throttles differently and the current recognizers only cover the AWS/MinIO shapes (THROTTLING/SLOW_DOWN enums, "SlowDown"/"SlowDownWrite" names): e.g. AWS S3 uses 503 SlowDown, GCS XML API uses 429, MinIO uses 503 SlowDown/SlowDownWrite, and OSS/COS have their own error codes on 429/503. Both channels (HTTP code and exception name) need entries per backend, otherwise a vendor's throttling silently degrades to non-retryable IOError.

Also, when the backend does throttle us, the response usually carries an explicit Retry-After hint (both 429 and 503 SlowDown may include it; the SDK exposes it via AWSError::GetResponseHeaders()). The translation layer currently discards it — the extend status only carries code + message, so the outer retry can only guess its own backoff, which tends to hammer an already-throttling backend. Suggest capturing Retry-After into ExtendStatusDetail (e.g. an optional retry_after_ms field) and exposing it through the FFI result, so consumers honor the server-requested delay instead of inventing their own schedule. Fine as a follow-up PR if it's too much scope here, but the header shouldn't be silently dropped at the translation layer.

P1: two not-found paths still bypass the structured codes (pre-existing, but worth closing in this PR since it establishes the taxonomy)

• EnsureHeadObject returns plain PathNotFound on not-found (s3_filesystem.cpp, both call sites), bypassing the AwsErrorNotFound tagging in ErrorToStatus — so a missing object surfaces as a generic storage error instead of ObjectNotExist.
• vortex_footer_reader.cpp ResolveFileSize turns a missing file into arrow::Status::Invalid, which the segcore mapping classifies as DataFormatBroken (2024, data corruption) — wrong semantics for "object does not exist".

P1: public C header lost its stable error-code constants

The #define LOON_* values moved to an internal header and the public ffi_c.h now only exposes extern const int symbols (txn codes also renumbered 10/11 -> 111/112). I verified no existing consumer hardcodes the old values and the codes are never persisted, so this isn't breaking today — but extern const int can't be used as C switch/case labels and macro-style access from cgo is gone, which is a usability regression for C/C++ consumers. Suggest keeping a stable enum/macro set in the public header (generated from the same source as ffi_error_code.h so they can't drift), with the exported symbols kept as the dynamic-language channel.

1. HTTP 429 and not-found handling

   The HTTP 429 TOO_MANY_REQUESTS fallback now maps to StorageTransientThrottling, with regression coverage added.

   The not-found paths have also been fixed. ResolveFileSize now returns PathNotFound, and plain Arrow ENOENT is
   preserved and correctly mapped at the Segcore, C FFI, and Vortex bridge boundaries. For EnsureHeadObject, we
   intentionally preserve PathNotFound/ENOENT because Arrow/Parquet relies on that semantic.

2. AwsErrorNoSuchUpload

   We intentionally keep this retryable. The retry happens at the business-operation level, rather than retrying the
   same S3 request against the same multipart upload ID. A retry starts a new multipart upload and obtains a fresh
   upload ID, so it will not repeatedly operate on the expired or aborted upload.

3. Public C error-code constants

   Using exported extern const int symbols is intentional. C preprocessor macros cannot be consumed directly through
   FFI by languages such as Python and Rust, while exported symbols allow all language bindings to load the same
   values at runtime. The fact that these symbols cannot be used as C switch/case constant expressions is an expected
   tradeoff of this cross-language interface design.

[xiaofan-luan]

Thanks for the quick turnaround on the round-1 comments — verified the fixes in e58b107: 429 → StorageTransientThrottling with regression coverage, and the not-found paths now flow consistently (ResolveFileSize → PathNotFound, ENOENT → LOON_FILE_NOT_FOUND at FFI / ObjectNotExist at segcore, get_file_info → file_not_found), each with a test at the boundary. Also accepting both pushbacks: the business-operation-level retry semantics for NoSuchUpload is coherent (worth one doc line on loon_ffi_is_retryable_errcode stating that "retryable" means restart-the-operation, not retry-the-same-handle), and the extern const int rationale for cross-language FFI makes sense.

A second pass over the new head found one blocking issue and two that I'd strongly prefer to see in this PR. Ranked:

---

P0: the streaming read path silently drops the retryable classification (and leaks the marker string)

MakeVortexBridgeErrorStatus is the only decoder of the __LOON_VORTEX_FFI_ERRCODE__= marker, and it is only reachable from MakeVortexErrorStatus call sites. On the streaming path the error never gets there:

• streaming_read (vortex_format_reader.cpp:674) only wraps the ImportRecordBatchReader call and returns the reader raw. Errors raised later from ReadNext — where the vortex scan actually performs its S3 I/O — bypass translation entirely.
• Rust side (filesystem_c.rs), LoonFfiError's Display embeds the marker into the message; VortexRecordBatchReader::next() maps it to ArrowError::ExternalError and arrow-rs stringifies it into the stream's get_last_error.
• Consumers get a plain IOError with the raw marker text and no ExtendStatusDetail: column_group_reader.cpp:418 (ToRecordBatches() behind loon_get_chunk/loon_get_chunks) falls through to the FFI fallback LOON_ARROW_ERROR, so loon_ffi_is_retryable_errcode returns false; loon_get_record_batch_reader (reader_c.cpp:442) exports the stream directly, so the internal marker string reaches user-visible messages verbatim.

blocking_read and take are fine — ImportChunkedArray drains the stream inside the wrapped call. The asymmetry is specific to streaming.

Why this is blocking: a mid-scan S3 throttle or network blip is the single most common transient failure (large scans spend ~all their time in stream iteration, and throttling correlates with heavy chunk reads). Today the milvus side masks this by retrying every loon error, but the whole point of this PR is to let classifyLoonErr narrow to err_code / loon_ffi_is_retryable_errcode — and the moment that lands, mid-scan transients on the primary read path become permanent failures (QueryNode load hard-fails on a 429 instead of backing off). The PR's core deliverable is inverted exactly where it matters most, and it's invisible to current tests because they only inject open-time errors.

Suggested fix — translate at the layer where bridge errors first enter C++, so both consumers are covered for free:

class VortexErrorTranslatingReader : public arrow::RecordBatchReader {
 public:
  explicit VortexErrorTranslatingReader(std::shared_ptr<arrow::RecordBatchReader> inner)
      : inner_(std::move(inner)) {}
  std::shared_ptr<arrow::Schema> schema() const override { return inner_->schema(); }
  arrow::Status ReadNext(std::shared_ptr<arrow::RecordBatch>* batch) override {
    return MakeVortexErrorStatus("Failed to read vortex record batch", inner_->ReadNext(batch));
  }
  arrow::Status Close() override {
    return MakeVortexErrorStatus("Failed to close vortex record batch reader", inner_->Close());
  }
 private:
  std::shared_ptr<arrow::RecordBatchReader> inner_;
};

and return std::make_shared<VortexErrorTranslatingReader>(reader_result.ValueOrDie()) from streaming_read. MakeVortexErrorStatus passes OK through, so the success path costs nothing. One caveat worth a comment in the code: for the exported-stream surface (loon_get_record_batch_reader) the errcode still can't reach LoonFFIResult (ArrowArrayStream has no side channel), but the message becomes clean and the in-process consumers — the dominant loon_get_chunk(s) path — get the correct retryable code. A regression test that injects a failure mid-iteration (not at open) would lock this in.

---

P1: WrapExtendError destroys the ENOENT channel — make not-found a first-class ExtendStatusCode

The new ENOENT signal rides an errno side-channel that only survives while nobody wraps the status. WrapExtendError (extend_status.cpp:132) recognizes only ExtendStatusDetail; arrow statuses hold a single detail, so MakeExtendError silently drops the errno detail. Concrete in-tree case: packed/reader.cpp:115/152 wrap MakeArrowFileReader failures as PackedStorageIO — a missing local file comes out as StorageError/2044 instead of ObjectNotExist/2017, and the ENOENT branches added in this commit (extend_status.cpp:240, result.h:89) are unreachable for every wrapped path. Note the asymmetry: an S3 AwsErrorNotFound survives wrapping because it is an ExtendStatusDetail; only the errno flavor of the same semantic condition gets destroyed.

Suggested fix — promote it into the taxonomy instead of patching each boundary:

1. ExtendStatusCode::FileNotFound = LOON_FILE_NOT_FOUND (the FFI constant already exists), one row in kExtendStatusCodeMetadata (retryable=false), one case in ToSegcoreErrorCode → milvus::ObjectNotExist (the no-default switch will enforce this). FFIErrorCodeFromExtendStatusCode is a raw-int passthrough, so FFI needs no change, and loon_ffi_is_retryable_errcode(12) becomes an explicit table hit instead of the unknown-code fallback.
2. A single shared helper IsPathNotFound(const arrow::Status&) (= ErrnoFromStatus(status) == ENOENT) as the one owner of "what counts as not-found", and one line in WrapExtendError:

   auto wrapped_code = detail ? detail->code()
                     : IsPathNotFound(cause) ? ExtendStatusCode::FileNotFound
                                             : code;

   With that, packed/reader.cpp needs no change at all.
3. This also lets you delete code: the LOON_FILE_NOT_FOUND special case in MakeVortexBridgeErrorStatus (the int → marker → re-encoded-ENOENT → int triple hop collapses into the normal MakeExtendError path via ExtendStatusCodeFromInt), and the five hand-written ENOENT → RETURN_ERROR(LOON_FILE_NOT_FOUND) pre-checks in filesystem_c.cpp (259/321/493/630/925) which duplicate what FFIErrorCodeFromExtendStatus already does on the next line.
4. Where not to translate: agreed with your reasoning on EnsureHeadObject — inside the filesystem layer ENOENT/PathNotFound stays raw because arrow/parquet plumbing depends on it. The rule is simply: errno is the filesystem layer's dialect; it becomes FileNotFound the moment it crosses a classification boundary (wrap / segcore / FFI / vortex bridge).
5. Test to lock the bug: WrapExtendError(PackedStorageIO, ..., enoent_status) → ToSegcoreError must yield ObjectNotExist (today it yields 2044).

---

P1: 503 throttle classification — the name allowlist is currently dead code; a coarse split inside the 503 case does the job

Three facts (verified against aws-sdk-cpp 1.11.692, the version this repo pins, plus vendor docs/source):

• The SDK maps SlowDown to S3Errors::SLOW_DOWN, so the first switch always catches it — the "SlowDown" entry in the name branch can never fire. MinIO master meanwhile no longer emits bare SlowDown at all; it emits SlowDownRead/SlowDownWrite (cmd/api-errors.go), which the SDK does not know.
• The name branch sits after the HTTP switch, and GetResponseCode() is always populated when the server responded — so "SlowDownWrite" (503) is also caught by the 503 case first. Both entries in the current allowlist are effectively unreachable.
• Retryability is therefore already complete via enum + HTTP (429 is throttling by HTTP semantics regardless of body; every vendor throttle code rides 429 or 503). What's lost is only precision: 503-borne throttles — MinIO SlowDownRead/SlowDownWrite, Alibaba OSS's whole family (TotalQpsLimitExceeded, Download/UploadTrafficRateLimitExceeded, MetaOperationQpsLimitExceeded, ActiveRequestLimitExceeded, CpuLimitExceeded, all 503) — classify as StorageTransientService instead of StorageTransientThrottling. ToSegcoreErrorCode collapses all four transients into 2045 anyway, so the distinction only survives on the FFI err_code channel (109 vs 110) — which is exactly what a throttle-aware backoff policy on the Go side will want.

Suggested shape — no standalone name branch, just split the 503 case coarsely:

case Aws::Http::HttpResponseCode::SERVICE_UNAVAILABLE: {
  // 503 is ambiguous: vendors carry both throttling (MinIO SlowDownRead/Write,
  // OSS *Exceeded, COS SlowDown) and genuine unavailability on it. Coarse split
  // by name; a miss only costs label precision (Service vs Throttling), never
  // retryability, since everything here is transient.
  const auto& name = error.GetExceptionName();
  if (name.starts_with("SlowDown") || name.ends_with("Exceeded")) {
    return MakeExtendError(ExtendStatusCode::StorageTransientThrottling, message, message);
  }
  return MakeExtendError(ExtendStatusCode::StorageTransientService, message, message);
}

starts_with("SlowDown") covers AWS/GCS/COS + MinIO read/write variants; ends_with("Exceeded") covers the OSS family plus GCS RateLimitExceeded in one rule (safe because it's scoped to 503 — permanent *Exceeded codes like MaxMessageLengthExceeded arrive on 400 and never enter this case). The existing standalone name branch (SlowDown/SlowDownWrite/XMinioServerNotInitialized) can then be deleted — the first two are dead as shown, and XMinioServerNotInitialized lands in Service via the 503 fallthrough with the same retry outcome. IsConnectError (s3_internal.h:77) carries a copy of the same dead list and can be simplified the same way.

---

(Also noting for a future PR, not this one: the azure filesystem (azurefs.cc) has no extend-status classification at all — once the taxonomy is consumed on the milvus side, Azure's ServerBusy/503 will need an equivalent mapping, likely worth a tracking issue.)

[xiaofan-luan]

/approve

[sre-ci-robot]

[APPROVALNOTIFIER] This PR is NOT APPROVED

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

[shaoting-huang]

/lgtm