enhance: classify packed extend status codes into segcore error codes - #575
Merged
jiaqizho merged 5 commits intoJul 7, 2026
Merged
Conversation
Collaborator
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: czs007 The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
Builds on the packed-specific V2 extend status codes (PackedInvalidArgs / PackedStorageIO / PackedMetadataCorrupted / PackedFileCorrupted / PackedArrowError / PackedUnexpected) so packed failures carry a structured classification instead of generic Arrow text. Reference: milvus-io#572. Adds the producer-owned mapping ToSegcoreErrorCode / ToSegcoreError so milvus-storage classifies its own codes once, at the source: * PackedInvalidArgs -> InvalidParameter (caller input, non-retriable) * PackedStorageIO -> StorageTransientError (2045, RETRIABLE) * PackedMetadataCorrupted -> DataFormatBroken (permanent corruption) * PackedFileCorrupted -> DataFormatBroken (permanent corruption) * PackedArrowError -> StorageError (permanent internal) * PackedUnexpected/AWS/Txn -> StorageError (permanent internal) The mapping is a switch with no default plus -Werror=switch, so a new ExtendStatusCode that is not classified breaks the build. The retriable verdict is load-bearing: a transient storage IO failure maps to the retriable StorageTransientError(2045) and must never collapse into the non-retriable StorageError(2044). The no-detail fallback keeps coarse arrow classification (IO -> transient, OOM -> mem-allocate, Invalid/Type/Key -> data corruption). Depends on milvus-common StorageTransientError(2045). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: zhenshan.cao <zhenshan.cao@zilliz.com> Co-authored-by: jiaqizho <jiaqi.zhou@zilliz.com>
czs007
force-pushed
the
enhance-packed-extend-status-codes
branch
from
July 1, 2026 07:29
971e6b5 to
7c17bee
Compare
jiaqizho
reviewed
Jul 1, 2026
jiaqizho
reviewed
Jul 1, 2026
Address review on the retriability semantics:
* PackedStorageIO -> StorageError(2044), non-retriable (review: v2 packed
consumers must not stack another retry loop). Note this switch branch is
currently dormant: no live consumer routes a Packed* status through
ToSegcoreErrorCode (the packed C-APIs hardcode FileReadFailed /
FileWriteFailed), so the mapping is reserved for future direct-link callers.
* Fix the retry rationale in comments: object-storage IO retry lives once in
the shared S3 ArrowFileSystem (AWS SDK DefaultRetryStrategy), not per read
generation -- there is no 'v2 retries, v3 does not' asymmetry. A plain
transient IO escapee stays retriable (StorageTransientError/2045) because a
querynode replica-reroute is a distinct upper layer, not a stacked retry.
* Tag permanently-failing S3 errors at the source so they can no longer be
misclassified as transient by the no-detail fallback:
- AwsErrorNotFound(104): NoSuchKey / NoSuchBucket / ResourceNotFound
(also the two hand-rolled 'Bucket not found' sites in s3_client.cpp)
- AwsErrorAccessDenied(105): AccessDenied / InvalidAccessKeyId /
SignatureDoesNotMatch
- AwsErrorNonRetryable(106): recognized error types the SDK itself judged
non-retryable
All three map to the non-retriable StorageError(2044); without this a read
of a deleted object classified as 2045 and querynode would retry-storm a
request that can never succeed.
* Keep the SDK verdict scoped to recognized error types: S3-compatible
backends (MinIO) surface genuine transients as UNKNOWN + non-retryable
('SlowDown' arrives exactly this way), so UNKNOWN and connect-style errors
still fall through to the retriable plain-IOError bucket.
* Tests: PermanentS3ErrorsAreNotRetriable, TestErrorToStatusPermanentVsTransient
(NotFound / AccessDenied / recognized-non-retryable / UNKNOWN-SlowDown stays
transient / SLOW_DOWN stays transient); TestErrorToStatus generic-IOError case
now uses an UNKNOWN error since ACCESS_DENIED is no longer generic.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Signed-off-by: zhenshan.cao <zhenshan.cao@zilliz.com>
Align with the segcore error-code design: a missing object/bucket keeps its fine-grained permanent code (ObjectNotExist/2017, already classified permanent by the Go-side table) instead of collapsing into the generic StorageError(2044). Consumers can then distinguish 'data missing' (stale loadinfo, GC'd file) from a generic storage failure. Retriability is unchanged: still permanent, never the retriable 2045. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: zhenshan.cao <zhenshan.cao@zilliz.com>
… P0)
An unguarded ValueOrDie aborts the whole process on failure -- no catch can
intercept it, so it is the one failure mode the Ring-3 boundary cannot
contain. Convert the remaining library-core sites to status propagation:
* SplitterPlugin::Split (+ both implementations and
SizeBasedSplitter::SplitRecordBatches): bare std::vector return forced
ValueOrDie on SelectColumns -> return arrow::Result<...> and propagate
with ARROW_ASSIGN_OR_RAISE. Also stop ignoring the arrow::Status
returned by ColumnGroup::AddRecordBatch in the accumulation loop.
Caller (PackedRecordBatchWriter::writeWithSplitIndex) adopts
ARROW_ASSIGN_OR_RAISE.
* parquet FileRowGroupReader's MatchSchemaAndFillNullColumns: void ->
arrow::Status; the MakeArrayOfNull allocation failure now propagates
instead of aborting.
packed/reader.cpp's ValueOrDie sites already carry .ok() guards (fixed in
an earlier commit of this PR); the FFI/JNI-layer sites were audited as
guarded and are untouched.
Verified: milvus_test Splitter/Packed/FileReader/FormatReader filters
122/122 pass; clang-format-18 clean.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Signed-off-by: zhenshan.cao <zhenshan.cao@zilliz.com>
jiaqizho
reviewed
Jul 3, 2026
jiaqizho
reviewed
Jul 3, 2026
Signed-off-by: jiaqizho <jiaqi.zhou@zilliz.com>
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #575 +/- ##
==========================================
- Coverage 75.09% 74.83% -0.27%
==========================================
Files 160 162 +2
Lines 15393 15610 +217
Branches 2347 2379 +32
==========================================
+ Hits 11559 11681 +122
- Misses 3834 3929 +95
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Collaborator
|
/lgtm |
This was referenced Jul 6, 2026
czs007
marked this pull request as ready for review
July 7, 2026 02:06
jiaqizho
added a commit
to jiaqizho/milvus-storage
that referenced
this pull request
Jul 7, 2026
…milvus-io#575) Add the producer-owned mapping `ToSegcoreErrorCode` / `ToSegcoreError` so milvus-storage classifies its own packed failures once, at the source, and milvus can consume a typed segcore `ErrorCode` instead of a generic Arrow status text. > Note: this **builds on milvus-io#572** (jiaqizho — packed `ExtendStatusCode` + `WrapExtendError`), which is not yet merged, so this branch currently includes those commits. Once milvus-io#572 lands this PR should be rebased onto it so it only carries the mapping delta. Happy to coordinate. | ExtendStatusCode | segcore ErrorCode | retry | |---|---|---| | PackedInvalidArgs | InvalidParameter (2042) | non-retriable (input) | | PackedStorageIO | **StorageTransientError (2045)** | **retriable** | | PackedMetadataCorrupted / PackedFileCorrupted | DataFormatBroken (2024) | permanent | | PackedArrowError / PackedUnexpected / AWS / Txn | StorageError (2044) | permanent | The retriable verdict is load-bearing: a transient storage IO failure maps to the retriable `StorageTransientError(2045)` and must never collapse into the non-retriable `StorageError(2044)`. The switch has no `default` + a post-switch fallback under `-Werror=switch`, so a new `ExtendStatusCode` breaks the build until classified. The no-detail path keeps a coarse arrow mapping (IO→transient, OOM→mem-allocate, Invalid/Type/Key→corruption). Depends on `milvus-common` `StorageTransientError(2045)`: zilliztech/milvus-common#102. Bump the `milvus-common` conan pin to the published 2045 revision before merge (the pin here points at a local build for verification). Design + tracking: milvus-io/milvus#50903. Consumed by milvus-io/milvus#50768. --------- Signed-off-by: zhenshan.cao <zhenshan.cao@zilliz.com> Signed-off-by: jiaqizho <jiaqi.zhou@zilliz.com> Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Co-authored-by: jiaqizho <jiaqi.zhou@zilliz.com> (cherry picked from commit 9c28242)
jiaqizho
added a commit
to jiaqizho/milvus-storage
that referenced
this pull request
Jul 7, 2026
…milvus-io#575) Add the producer-owned mapping `ToSegcoreErrorCode` / `ToSegcoreError` so milvus-storage classifies its own packed failures once, at the source, and milvus can consume a typed segcore `ErrorCode` instead of a generic Arrow status text. > Note: this **builds on milvus-io#572** (jiaqizho — packed `ExtendStatusCode` + `WrapExtendError`), which is not yet merged, so this branch currently includes those commits. Once milvus-io#572 lands this PR should be rebased onto it so it only carries the mapping delta. Happy to coordinate. | ExtendStatusCode | segcore ErrorCode | retry | |---|---|---| | PackedInvalidArgs | InvalidParameter (2042) | non-retriable (input) | | PackedStorageIO | **StorageTransientError (2045)** | **retriable** | | PackedMetadataCorrupted / PackedFileCorrupted | DataFormatBroken (2024) | permanent | | PackedArrowError / PackedUnexpected / AWS / Txn | StorageError (2044) | permanent | The retriable verdict is load-bearing: a transient storage IO failure maps to the retriable `StorageTransientError(2045)` and must never collapse into the non-retriable `StorageError(2044)`. The switch has no `default` + a post-switch fallback under `-Werror=switch`, so a new `ExtendStatusCode` breaks the build until classified. The no-detail path keeps a coarse arrow mapping (IO→transient, OOM→mem-allocate, Invalid/Type/Key→corruption). Depends on `milvus-common` `StorageTransientError(2045)`: zilliztech/milvus-common#102. Bump the `milvus-common` conan pin to the published 2045 revision before merge (the pin here points at a local build for verification). Design + tracking: milvus-io/milvus#50903. Consumed by milvus-io/milvus#50768. --------- Signed-off-by: zhenshan.cao <zhenshan.cao@zilliz.com> Signed-off-by: jiaqizho <jiaqi.zhou@zilliz.com> Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Co-authored-by: jiaqizho <jiaqi.zhou@zilliz.com> (cherry picked from commit 9c28242) Signed-off-by: jiaqizho <jiaqi.zhou@zilliz.com>
This was referenced Jul 24, 2026
xiaofan-luan
pushed a commit
to czs007/milvus-storage
that referenced
this pull request
Jul 29, 2026
…own) (milvus-io#596) ## What Adds a CI **ratchet** for error-handling discipline: a checked-in per-file baseline of `abort` sites (`ValueOrDie`/`ValueUnsafe`/`MoveValueUnsafe`) and `throw` sites in production code (`cpp/src` + `cpp/include`, tests excluded), enforced by a lightweight two-layer check: 1. **Exact match** against the committed baseline: any per-file count change fails CI — an increase must be fixed in code, a decrease must be recorded by regenerating the baseline (`make -C cpp update-error-ratchet`) in the same PR, so freed slack cannot silently grow back. 2. **Base-branch totals** (pull requests): per-category **totals must not increase** versus the baseline recorded on the PR's base commit. This is the machine-enforced ratchet direction: regenerating a *raised* baseline inside the PR keeps layer 1 green but fails layer 2. Baseline at introduction: **abort = 143, throw = 59** (git-tracked files only, so vendored/generated trees are excluded automatically). ## Why Ring-1 discipline for this library is that failures are reported as `arrow::Status` with structured classification, not by aborting the process or leaking exceptions to consumers (milvus links this C++ directly). Individual sweeps keep fixing sites — milvus-io#575 removed four unguarded `ValueOrDie` abort paths — but nothing prevents regressions, and the pattern keeps reappearing: - a fifth unguarded `ValueOrDie` (`format/parquet/file_reader.cpp`, reachable when the file schema lacks field-id metadata) predates milvus-io#575's sweep and was never on any list (fixed in milvus-io#598); - `packed/reader.cpp` still throws from a constructor; the lance/iceberg bridges threw across the library boundary until milvus-io#597 — the ratchet makes that burn-down visible and permanent in the baseline diff. ## How to work with it ```bash make -C cpp check-error-ratchet # what CI runs (also: bash cpp/scripts/error_handling_ratchet.sh check) make -C cpp update-error-ratchet # regenerate the baseline after a burn-down ``` The failure output is a unified diff against the baseline plus instructions for both directions; the base-totals failure names the category and both totals. ## Verification - `check` passes on the pristine tree; category totals reported (abort=143, throw=59). - Injecting a `throw std::runtime_error` fails layer 1 with a per-file diff (verified locally, then reverted). - The self-reference attack (add a throw → regenerate the baseline in the same PR → check against the pre-attack base) fails layer 2 with `throw: 59 (base) -> 60 (this PR)`; a genuine burn-down against the same base passes (both verified locally). - The workflow is toolchain-free (checkout + bash); layer 2 anchors to `github.event.pull_request.base.sha` and distinguishes bootstrap (no baseline on base, verified via `git cat-file`) from real failures. Not covered (honest scope) — this is a text-level ratchet, not a semantic linter: - the grep counts comments/strings too, so an equal-count swap (delete a commented `throw`, add a real one in the same file — or, for layer 2, in another file) is not caught; - `grep -c` counts matching **lines**, not call sites: two sites on one line count once, and a site added to an already-matching line does not trip the gate; - the checker runs from the PR checkout (standard in-repo lint-gate trust model — defense target is accidental regression; edits to the gate are visible in the diff); - migrating to clang-query later can reuse the same baseline flow. issue: milvus-io/milvus#50903 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Signed-off-by: zhenshan.cao <zhenshan.cao@zilliz.com> Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
jiaqizho
pushed a commit
to jiaqizho/milvus-storage
that referenced
this pull request
Jul 30, 2026
…e-destroying paths in packed (milvus-io#598) ## Problem Three related defects in the packed (V2 API) library-mode error paths — all instances of the "stringify-and-rethrow destroys the classification" / "abort instead of status" classes: 1. **`PackedRecordBatchReader`'s constructor throws.** On a failed open it does `throw std::runtime_error(status.ToString())` — the classification the lower layers carefully attached (`PackedInvalidArgs` / `PackedStorageIO` / `PackedMetadataCorrupted` details, the filesystem's ENOENT) is flattened into a string at the very last step. Direct-link consumers are forced to `try`-wrap construction and can only parse text. 2. **`ColumnGroup::Table()` does the same** (`throw std::runtime_error(result.status().message())` — even drops `ToString()`'s code prefix). 3. **A fifth unguarded `ValueOrDie` abort path** (missed by the milvus-io#575 sweep, which fixed four): `file_reader.cpp` `FileRecordBatchReader::init`'s `schema==nullptr` branch calls `FieldIDList::Make(schema_).ValueOrDie()`. A parquet file whose stored schema lacks `PARQUET:field_id` metadata **aborts the whole process** — a data-dependent abort. The sibling `schema!=nullptr` branch already used `ARROW_ASSIGN_OR_RAISE`. ## Change - **New `PackedRecordBatchReader::Make(...) → arrow::Result<std::unique_ptr<...>>`** — same parameters, reports open failures as a status with the classification intact. The throwing constructor is **kept** (documented as deprecated, "do not add new call sites") so direct-link consumers can migrate without a lockstep bump; it can be deleted once they have. - milvus side: the only constructor consumer is `segcore/packed_reader_c.cpp` (3 try-wrapped sites). Migrating those to `Make()` hands them a classified status — which is exactly the input the planned "wire the packed C-API through `ToSegcoreError`" follow-up needs (it currently hardcodes FileReadFailed/FileWriteFailed). Tracked for the milvus-side PR; not part of this change. - **`ColumnGroup::Table()` returns `arrow::Result`**, wrapping the cause without destroying it. `ColumnGroup::Schema()` now reads the schema from the first batch (all batches in a group share one schema) instead of materializing the merged table. In-tree callers updated; there are no milvus-side callers of either. - **`file_reader.cpp` abort path** → `ARROW_ASSIGN_OR_RAISE`, matching the sibling branch. ## Verification - Full `make build` clean. - New tests in `packed_error_status_test.cpp`: - `Make()` on a missing file → the wrap preserves the filesystem's not-found detail → `ToSegcoreError` = `ObjectNotExist` (the throwing constructor destroyed exactly this); - `Make()` with empty paths → `PackedInvalidArgs`; - `Make()` success path reads a batch. - Packed/column-group suites: 27/29 passed — the single failure is the pre-existing upstream `ColumnGroupTest.MemoryUsageCalculation` (fails 3/3 in isolation on unmodified main; not touched here), one skipped. Not covered (honest scope): the deprecated throwing constructor still exists (one `throw` site remains by design until milvus migrates); no fault-injection e2e. Refs: milvus-io/milvus#50903. Follows the classification taxonomy of milvus-io#574/milvus-io#575. 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Signed-off-by: zhenshan.cao <zhenshan.cao@zilliz.com> Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
xiaofan-luan
added a commit
to xiaofan-luan/milvus-storage
that referenced
this pull request
Jul 30, 2026
…nomy Azure tagged exactly one condition (PreconditionFailed, plus Conflict + BlobAlreadyExists) and let everything else fall through to a plain Status::IOError. An untagged status reaches segcore as StorageError/2044, permanent and non-retriable -- so on Azure a throttle or a 503 was reported as a permanent storage error and never retried, and a missing blob was indistinguishable from a generic storage failure. internal::ClassifyAzureError maps the failure onto the existing taxonomy: transport failure (no response, StatusCode None) -> 107; 408 -> 108; 429 and 503+ServerBusy -> 109; other 5xx -> 110; 404 -> 104; 401/403 -> 105; 412 and 409+BlobAlreadyExists -> 103. Anything not positively identified stays untagged and keeps today's conservative non-retriable behaviour -- retriability is never invented. It takes the raw HTTP status rather than the SDK exception type so the mapping is unit-testable without an Azure account; every credential-bearing Azure test is skipped in CI, so a mapping tested only through the filesystem would be untested in practice. Also fixes the one real GCS gap: the producer stringified an init failure into Status::Invalid, turning a credential/network problem into a data-class error (DataFormatBroken/2024) and destroying the cause's own classification. It now preserves the status code and detail. Two claims in milvus-io#595 do not hold and are deliberately NOT acted on: GCS is not unclassified (GcpFileSystemProducer builds milvus-storage's own S3FileSystem, so the data path already inherits ErrorToStatus), and azurefs's PathNotFound already carries an ENOENT detail, so it already reaches ObjectNotExist/2017. Closes milvus-io#595 Refs: milvus-io#574, milvus-io#575, milvus-io/milvus#50903 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011HDgMok3nR8ZNugKWWQQK2 Signed-off-by: xiaofanluan <xiaofan.luan@zilliz.com>
jiaqizho
pushed a commit
to jiaqizho/milvus-storage
that referenced
this pull request
Jul 30, 2026
…nomy (milvus-io#604) ## Problem The ExtendStatusCode taxonomy (milvus-io#574 transients 107–110, milvus-io#575 permanent S3 tags 104–106) only covers the S3 path. On Azure, `ExceptionToStatus` tagged exactly one condition — `PreconditionFailed`, plus `Conflict`+`BlobAlreadyExists` — and everything else fell through to a plain `Status::IOError`: https://github.com/milvus-io/milvus-storage/blob/d3eebb1ff653f023bb02c885e139d69daca0f1d2/cpp/src/filesystem/azure/azurefs.cc#L360-L375 An untagged status reaches segcore as `StorageError`/2044 — permanent and non-retriable. So on Azure a **throttle or a 503 was reported as a permanent storage error and never retried**, which is the availability-relevant half of the gap. Closes milvus-io#595. ## Change `internal::ClassifyAzureError(http_status, error_code)` maps the failure onto the existing taxonomy; `ExceptionToStatus` tags the status when it returns a code and keeps today's plain `IOError` when it does not. | Azure | ExtendStatusCode | segcore | retry | |---|---|---|---| | no response (transport: conn refused/reset, DNS, TLS) | `StorageTransientNetwork` 107 | 2045 | yes | | 408 Request Timeout | `StorageTransientTimeout` 108 | 2045 | yes | | 429 Too Many Requests | `StorageTransientThrottling` 109 | 2045 | yes | | 503 + `ServerBusy` | `StorageTransientThrottling` 109 | 2045 | yes | | 503 (other), 500, 502, 504 | `StorageTransientService` 110 | 2045 | yes | | 404 | `AwsErrorNotFound` 104 | 2017 `ObjectNotExist` | no | | 401, 403 | `AwsErrorAccessDenied` 105 | 2044 | no | | 412, 409 + `BlobAlreadyExists` | `AwsErrorPreConditionFailed` 103 | 2044 | no | | anything else (incl. 409 non-`BlobAlreadyExists`) | untagged | 2044 | no | Notes on the two judgement calls: - **Transport failures are the `StatusCode == None` (0) case.** `Azure::Core::Http::TransportException` derives from `RequestFailedException` and `ExceptionToStatus` is already called with it (the `catch` in the HNS check), so it arrives here with `StatusCode` left at its `None` default. That is the discriminator, and it is the one class where "no response at all" makes retry unambiguously right. - **503 is overloaded on Azure Storage**: `ServerBusy` is throttling, everything else is an availability blip. Both retriable, but keeping them apart lets a consumer apply throttle-aware backoff only where it belongs. Same split milvus-io#574 settled on for S3. Codes are cloud-neutral despite the `AwsError*` prefix (per the naming discussion in milvus-io#574) — no new values needed, as milvus-io#595 predicted. The classifier takes the raw HTTP status rather than the SDK exception type specifically so it can be unit-tested **without an Azure account**. Every credential-bearing Azure test is skipped in CI, so a mapping tested only through the filesystem would be untested in practice. ## Two corrections to milvus-io#595 Both were load-bearing for its proposed scope, and both turn out to be wrong. Verified against `d3eebb1`: 1. **"GCS: zero `MakeExtendError` call sites in the whole subtree. All GCS errors surface as untagged status."** — Not so. `GcpFileSystemProducer` builds **milvus-storage's own** `S3FileSystem`, the one carrying `ErrorToStatus`: https://github.com/milvus-io/milvus-storage/blob/d3eebb1ff653f023bb02c885e139d69daca0f1d2/cpp/src/filesystem/gcp/gcp_filesystem_producer.cpp#L255-L259 (`#include "milvus-storage/filesystem/s3/s3_filesystem.h"` at L41, inside `namespace milvus_storage` at L45.) So the GCS **data path already inherits the full S3 classification**, including the HTTP-status channel that is vendor-neutral. What is genuinely unclassified is only the init path, which this PR fixes as a one-liner: it was stringifying the cause into `Status::Invalid`, turning a credential/network init failure into a data-class error (`DataFormatBroken`/2024) and destroying whatever classification the cause carried. The remaining GCS gap is the exception-*name* channel (`SlowDown`, `XMinioServerNotInitialized` are AWS/MinIO dialect), which is a long tail, not the whole taxonomy. 2. **"`PathNotFound()` returns arrow's generic `PathNotFound` IOError with no `ExtendStatusDetail` and no errno detail, so it cannot reach `ObjectNotExist`/2017."** — It does carry an errno detail. `arrow::fs::internal::PathNotFound` is `Status::IOError(...).WithDetail(StatusDetailFromErrno(ENOENT))`, and `ToSegcoreError` checks ENOENT before anything else, so that path already reaches 2017. No change needed, and none made. Net: milvus-io#595's Azure analysis holds and is what this PR implements; its GCS half shrinks to the init-path one-liner. ## Verification - `make build` clean; clang-format-18 clean. - New `cpp/test/filesystem/azure_error_classification_test.cpp`, which runs in CI without credentials: every transient status is retriable and maps to 2045; every permanent one is non-retriable, maps to its expected segcore code, and is asserted **not** to be 2045; unidentified statuses stay untagged and land on 2044; and the two conditions milvus-io#595 named are stated as the end-to-end verdict a consumer sees. - Full suite: **1071 ran / 915 passed / 154 skipped (cloud credentials) / 2 failed.** Both failures — `MetadataTest.TestFieldIDList` and `FileReaderTest.SchemaEvolutionWithInvalidFieldID` — are pre-existing: I reverted the change in place, rebuilt, and reproduced them identically on unmodified `origin/main`. They are in the `FieldIDList` area milvus-io#598 is separately fixing. ## Not covered - The Azure **name** channel: this classifies on HTTP status plus `ErrorCode` only where the status is ambiguous (409, 503). Azure error codes such as `OperationTimedOut` (which arrives as 500) are covered by their status, but a name-only signal with no distinguishing status is not. - The Go/FFI consumer still discards the classification: milvus's `HandleLoonFFIResult` wraps every loon failure in one `ErrLoonTransient` sentinel and never reads `err_code`. The C++/segcore half is wired as of milvus-io/milvus#51530. So this makes Azure's errors *classifiable*, and they now reach segcore correctly, but the Go retry path still cannot tell them apart. Tracked in milvus-io/milvus#50903. - No fault-injection e2e against a live Azure account. Refs: milvus-io#595, milvus-io#574, milvus-io#575, milvus-io/milvus#50903. 🤖 Generated with [Claude Code](https://claude.com/claude-code) https://claude.ai/code/session_011HDgMok3nR8ZNugKWWQQK2 --------- Signed-off-by: xiaofanluan <xiaofan.luan@zilliz.com> Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
xiaofan-luan
added a commit
to xiaofan-luan/milvus-storage
that referenced
this pull request
Jul 30, 2026
…ability Errors returned to upper layers could say "retryable" but never "whose problem is this", and the retry bit was stored independently of the meaning, in three hand-synced tables. This adds the missing axis and makes the tables generated instead of hand-maintained. - One category per code: User / Transient / Permanent, crossing the C ABI as loon_ffi_error_category(). Retriability is now DERIVED (retryable == category == Transient), not a second stored bool. - Both code tables (LOON_* internal, ExtendStatusCode) move into X-macro lists in ffi_error_code.h. The exported loon_errcode_* constants, error_to_string, the category/retryable lookups, the ExtendStatusCode enum and its metadata are all generated from them, so a code cannot be classified two different ways in two different places. - The 11 internal LOON_* codes are classified for the first time: they were absent from the metadata table, so loon_ffi_is_retryable_errcode answered false for all of them by omission rather than by decision. - Codes are aligned with the AWS S3 / Aliyun OSS vocabulary, with the five deliberate divergences pinned by tests and documented. - New LOON_SOURCE_NOT_FOUND / LOON_SOURCE_ACCESS_DENIED: the same object-store condition is a system failure on an internally generated path and a user error on a path the user typed, and only the entry point knows which. Wired at loon_exttable_explore / loon_exttable_get_file_info, the two entry points that take a user-supplied location. - docs/error-codes.md enumerates every code with its category, retry verdict, S3/OSS equivalent and segcore mapping, plus an honest per-producer coverage table. Behaviour delta: loon_ffi_is_retryable_errcode(LOON_MEMORY_ERROR) now returns true (OOM is retriable, matching segcore 2034). Every other code keeps its previous verdict. No consumer reads these values yet, so the delta is inert today. Refs: milvus-io/milvus#50903 (the deferred "LOON_* enum-ization + category" item), milvus-io#574, milvus-io#575. Supersedes the classification half of milvus-io#568, whose IOError -> retriable mapping contradicts the conservative default milvus-io#574 settled on. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011HDgMok3nR8ZNugKWWQQK2 Signed-off-by: xiaofanluan <xiaofan.luan@zilliz.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
Add the producer-owned mapping
ToSegcoreErrorCode/ToSegcoreErrorso milvus-storage classifies its own packed failures once, at the source, and milvus can consume a typed segcoreErrorCodeinstead of a generic Arrow status text.Mapping (no-
defaultswitch +-Werror=switch)The retriable verdict is load-bearing: a transient storage IO failure maps to the retriable
StorageTransientError(2045)and must never collapse into the non-retriableStorageError(2044). The switch has nodefault+ a post-switch fallback under-Werror=switch, so a newExtendStatusCodebreaks the build until classified. The no-detail path keeps a coarse arrow mapping (IO→transient, OOM→mem-allocate, Invalid/Type/Key→corruption).Dependencies
Depends on
milvus-commonStorageTransientError(2045): zilliztech/milvus-common#102. Bump themilvus-commonconan pin to the published 2045 revision before merge (the pin here points at a local build for verification).Design + tracking: milvus-io/milvus#50903. Consumed by milvus-io/milvus#50768.