enhance: classify lance/iceberg bridge errors and stop leaking exceptions

[czs007]

Problem

The lance/iceberg cxx bridges reported every failure by throwing a string-only exception (LanceException / IcebergException) out of the library:

1. Exceptions escaped the library boundary. LanceTableReader::get_chunk/get_chunks/take/read_with_range and the whole api::Reader chain had no catch — a bridge error during a lance read left milvus-storage as a foreign exception and could only collapse to a generic internal error (2001) at the consumer's outermost boundary.
2. The error class was destroyed. lance::Error distinguishes not-found / corruption / lance-declared-retryable contention, and its IO variant wraps a typed object_store::Error — but the cxx boundary flattened all of it into one opaque string, so every failure surfaced as one bucket. A corrupt lance file was indistinguishable from an S3 throttle: permanent errors could be retried forever, transients could never be classified retriable. (Both this and the root cause — the bridge discards the typed error — were called out in the enhance: route arrow Status to finer LOON FFI error codes #568 review audit.)

Approach

Same mechanism the vortex bridge already established (#574): the Rust side embeds a classification code into the error string with a marker; the C++ side parses it back into a structured arrow::Status. One marker, one parser, now shared.

Rust (producer owns classification) — bridge_error.rs:

• classify_lance_error: DatasetNotFound alone → dataset-missing/ENOENT (12); internal resource-not-found variants (NotFound/IndexNotFound/RefNotFound/VersionNotFound and IO↳object_store::NotFound) → LanceResourceNotFound (114, non-retryable, no ENOENT); CorruptFile → data-corrupt; NotSupported → not-supported; lance-declared-retryable RetryableCommitConflict/TooMuchWriteContention → LanceWriteContention (113). Object-store throttling keeps its existing 109 category; IO{source} still maps PermissionDenied/Unauthenticated → 105 and Precondition → 103.
• Conservative discipline: anything not positively identified stays untagged → non-retriable fallback; no retriability is invented. InvalidInput is deliberately not tagged as caller input pending a producer-site audit (avoiding the mixed-semantics misclassification class).
• BatchFutStreamReader::next() wraps stream errors in BridgeError — the single choke point mid-scan read errors (the hot transient case) pass through before arrow FFI stringifies them.

C++ — shared bridge_error.{h,cpp}:

• Decodes: 12 → IOError+ENOENT detail (→ ObjectNotExist/2017); extend codes → ExtendStatusDetail (transients → 2045 retriable); bridge-private 1001 data-corrupt → Status::Invalid (→ 2024); 1002 → NotImplemented; no/unknown marker → plain IOError (→ 2044, conservative). Bridge-private codes (≥1000) never cross the C ABI.
• WrapBridgeRecordBatchReader translates live streams (read_with_range); drained-stream paths (get_chunk/get_chunks/take) decode on failure.
• The vortex bridge now delegates to the shared helpers — vortex public API and behavior unchanged, its error tests untouched and passing.
• lance_bridge / iceberg_bridge APIs are now arrow::Result/arrow::Status; all 23 throw sites and both exception types are gone. EstimateFragmentColumnMemory/EstimateFragmentMemory/IOStatsIncremental keep their best-effort degrade semantics (enhance: add per-column memory size estimates for chunks #586).

Behavior fix (intentional, please review): LanceTableWriter::Close fell back to creating a fresh dataset on any open failure (catch (std::exception) = "dataset does not exist"). The fallback now triggers only on classified DatasetNotFound / ENOENT; internal resource-not-found, auth failures, corruption, and transient IO propagate instead of silently creating a new dataset.

Classification table

lance error	surfaces as	segcore
DatasetNotFound	IOError + ENOENT detail	2017 ObjectNotExist (non-retriable; the only create-if-missing signal)
NotFound / IndexNotFound / RefNotFound / VersionNotFound; IO↳object_store NotFound	LanceResourceNotFound detail 114 (no ENOENT)	2017 ObjectNotExist (non-retriable)
RetryableCommitConflict / TooMuchWriteContention (lance's own classification)	LanceWriteContention detail 113	2045 retriable
object-store throttling / HTTP 429	StorageTransientThrottling detail 109	2045 retriable
IO↳PermissionDenied/Unauthenticated	detail 105	2044
IO↳Precondition	detail 103	2044
CorruptFile	Status::Invalid	2024 DataFormatBroken
NotSupported	Status::NotImplemented	—
everything else (incl. FieldNotFound, Schema/SchemaMismatch, InvalidInput, plain IO)	plain IOError	2044 (conservative)

Verification

• cargo check clean; full make build (lib + tests + benchmarks + tools) clean.
• New lance_bridge_error_test pins the decoder table (not-found → 2017, transient → 2045+retryable detail, corrupt → 2024, untagged/unknown-code → 2044, marker never leaks into messages, context preserved through translation) and an end-to-end open of a nonexistent dataset classifying as not-found through real lance.
• lance/iceberg/bridge suites: 78 ran / 69 passed / 9 skipped (cloud-credential tests, no env locally). LanceBasicTest 11/11 — real local write/read exercises the new writer fallback (not-found → create).
• Full milvus_test regression: 1074 ran / 919 passed / 154 skipped (cloud-credential) / 1 failed — the single failure is ColumnGroupTest.MemoryUsageCalculation, a pre-existing upstream test bug (dangling-pointer dedup in Buffer::Wrap), reproduced 3/3 in isolation and unrelated to this change (no packed/column_group code is touched here).
• clang-format clean on all touched files.
• Post-review taxonomy fix (cd7a578): rebased onto enhance: add error-handling ratchet gate (abort/throw baseline burn-down) #596, regenerated the error-handling ratchet baseline (lance bridge 21→0, iceberg bridge 2→0), reserved 109 for storage throttling, and added 113/114 with end-to-end Rust/C++/FFI coverage. Local Release build passed; focused GTest 24/24 and FFI 77/77.

Not covered (honest scope): iceberg-rust error kinds are not yet classified on the Rust side — iceberg bridge errors surface as plain non-retriable IOError (no worse than before, minus the exception); fault-injection e2e for mid-scan transient classification not added (the decoder path is unit-tested via synthetic markers). The rustfmt of lance_bridgeimpl.rs adds some formatting-only churn in that one file.

Refs: #595 (azure/GCS follow-up), milvus-io/milvus#50903 (error-handling tracking). Builds on the taxonomy from #574/#575.

🤖 Generated with Claude Code

[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 73.13433% with 18 lines in your changes missing coverage. Please review.
✅ Project coverage is 76.02%. Comparing base (d3eebb1) to head (3df645b).
⚠️ Report is 11 commits behind head on main.

Files with missing lines	Patch %	Lines
cpp/src/format/lance/lance_table_reader.cpp	72.34%	13 Missing ⚠️
cpp/src/format/lance/lance_format.cpp	0.00%	3 Missing ⚠️
cpp/src/format/iceberg/iceberg_format.cpp	0.00%	1 Missing ⚠️
cpp/src/format/lance/lance_table_writer.cpp	93.75%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #597      +/-   ##
==========================================
+ Coverage   75.95%   76.02%   +0.06%     
==========================================
  Files         168      168              
  Lines       16672    16681       +9     
  Branches     2509     2517       +8     
==========================================
+ Hits        12664    12681      +17     
+ Misses       4008     4000       -8     

Flag	Coverage Δ
cpp	78.72% <73.13%> (+0.06%)	⬆️
python	44.45% <ø> (ø)

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.

[chyezh]

Adversarial review of the error-handling refactor confirmed 1 high, 4 medium, and 1 low issue, centered on error classification fidelity in the bridge layer and error-path resource cleanup in the Lance writer.

High

• cpp/src/format/bridge/rust/src/bridge_error.rs:107 — Classification of LanceError::IO { source } relies on source.downcast_ref::<object_store::Error>(), which silently fails if the bridge crate and lance depend on different object_store versions, dropping all IO errors into the unclassified fallback. Additionally, object_store::Error::Generic — the form in which throttling/transient errors (429/503) surface after object_store exhausts its internal retries — explicitly falls into _ => None, and the C++ side maps unmarked messages to a plain non-retryable IOError (bridge_error.cpp:120), so genuine rate-limit errors become non-retryable. The comment at L120-122 shows this is a deliberate conservative choice, but 429/503 remain recoverable signals for upper-layer retry policy; please either classify Generic throttling errors as retryable or explain why staying untagged is acceptable, and pin/align the object_store dependency version to avoid the downcast coupling. (raised by chyezh, xiaocai2333)

Medium

• cpp/src/format/lance/lance_table_writer.cpp:118 — ExportRecordBatchReader(batch_iterator, &array_stream) creates an owning C stream with a release callback before OpenUnique, but the newly added non-ENOENT error return (L144) and the GetAllFragmentIds() failure path (L132) return early without calling array_stream.release (no release call exists anywhere in the file). The exported stream and its captured shared_ptr<BatchIterator> — holding every buffered RecordBatch — leak; repeated failed writes against a cloud dataset (auth/throttling failures) can retain entire write payloads. This is a behavior change introduced by this PR: the previous catch(std::exception) fallback always consumed the stream via WriteDataset. Move the export to immediately before WriteArrowArrayStream/WriteDataset, or wrap it in an RAII guard that releases the stream if unconsumed; also confirm whether the Rust write_stream consumes the stream on the WriteArrowArrayStream failure paths (L133/L147). (raised by chyezh, xiaocai2333)

• cpp/src/format/bridge/rust/src/bridge_error.rs:93 — FieldNotFound (dataset exists, field does not) is grouped with NotFound/DatasetNotFound and mapped to LOON_FILE_NOT_FOUND, which the C++ side decodes as IOError with an ENOENT detail (bridge_error.cpp:107-108). Beyond misleading semantics, LanceTableWriter::Close() uses ErrnoFromStatus == ENOENT to decide "table missing → create it" (lance_table_writer.cpp:134), so a field-level error surfacing through the open path would be misread as a missing table and trigger the dataset-creation path. Give FieldNotFound its own non-ENOENT classification. (raised by chyezh, xiaocai2333)

• cpp/src/format/bridge/rust/src/bridge_error.rs:96 — SchemaMismatch/Schema errors are classified together with CorruptFile as BRIDGE_ERRCODE_DATA_CORRUPT, decoded as arrow::Status::Invalid and further mapped to milvus::DataFormatBroken per the new test assertions. Schema mismatches are a normal caller-side condition during append/write, not data corruption; reporting them as "data format broken" can misdirect operators toward corruption remediation. Map schema errors to a distinct invalid-argument-style classification. (raised by chyezh, xiaocai2333)

• cpp/src/format/lance/lance_table_writer.cpp:134 — This PR narrows the "create table if missing" decision to ErrnoFromStatus == ENOENT only, propagating all other open errors (L143-145), but no test exercises this at the LanceTableWriter::Close() level. The new lance_bridge_error_test.cpp only covers the decode table and BlockingDataset::Open ENOENT classification. Please add tests anchoring both behaviors: (a) ENOENT correctly triggers dataset creation, and (b) a non-ENOENT open error (auth failure/throttling) propagates instead of silently creating a new dataset — this is the core semantic change of the PR. (raised by chyezh, xiaocai2333)

Low

• cpp/src/format/bridge/rust/src/bridge_error.cpp:137 — TranslateBridgeStatus only recognizes ExtendStatusDetail and ENOENT as "already structured" (L141); every other failing status is re-parsed via MakeBridgeErrorStatus(status.message()), which returns IOError for any unmarked message (L120), discarding the original StatusCode. Non-bridge Arrow errors reaching call sites like lance_table_reader.cpp's get_chunk/take/read_ranges (e.g. Status::Invalid or OutOfMemory from ImportChunkedArray) are silently rewritten to IOError and then mismapped to StorageError in segcore; already-decoded bridge Invalid/NotImplemented statuses flowing through a second time are likewise downgraded. Extend the structured check at L141 to pass through non-IOError status codes, or only attempt re-decoding on statuses where IsIOError holds. (surfaced during verification)

[czs007]

All six findings verified and addressed in af9fb35 — thanks, several of these were real catches:

• High (downcast coupling + untagged 429/503): the typed carrier (client::retry::RetryError, which has .status()) turned out to be pub(crate) in object_store, so a typed recovery is impossible from outside. The status is instead recovered from RequestError::Status's stable Display pattern ("non-2xx status code: NNN"): 408→transient-timeout(108), 429→throttling(109), 5xx→service(110) — all StorageTransientError/2045 downstream. Fail-safe by construction: if object_store rewords the message, classification degrades to untagged→non-retriable, never the reverse; unknown 4xx stay untagged (test-pinned). The version-coupling risk is now a compile-time pin: a unit test constructs LanceError::from(object_store::Error), which stops compiling if the bridge's object_store diverges from lance's — no silent runtime downcast failure possible.
• Writer stream leak: RAII guard added. Verified the Rust entry points take ownership immediately (ptr::replace with an empty stream ⇒ release == nullptr ⇒ guard is a no-op on consumed paths), so the guard fires exactly on the pre-handoff error returns this PR introduced.
• FieldNotFound→ENOENT: moved to untagged — agreed the create-if-missing coupling made this dangerous; rust test pins non-ENOENT.
• SchemaMismatch≠corruption: Schema family moved to untagged (producer sites are mixed library/user, so no input-blame either); CorruptFile alone remains data-corrupt.
• Writer Close tests: both directions now anchored — classified not-found creates the dataset; an EACCES open failure propagates and creates nothing (chmod-based, root-skipped).
• Low (Translate downgrade): non-IOError statuses now pass through untouched — the OutOfMemory→2044 downgrade you identified is test-pinned against regression.

Verification: cargo test --release 4/4 classification tests (note: rust unit tests are not yet wired into CI — CMake only drives cargo build; happy to add a cargo test step in a follow-up); full make build clean; lance/bridge suites 66 ran / 56 passed / 10 cloud-credential skips; the three new tests pass in isolation.

[xiaocai2333]

Adversarial review confirmed 1 high and 2 medium severity issues, all in error classification paths; the high one is a blocking regression on the mid-scan read path.

High

• cpp/src/format/bridge/rust/src/bridge_error.cpp:142 — The !status.IsIOError() short-circuit in TranslateBridgeStatus prevents mid-scan bridge errors from ever being decoded (regression). Mid-scan stream errors are wrapped as ArrowError::ExternalError (lance_bridgeimpl.rs:678), which arrow-rs's C-stream exporter maps to EINVAL and arrow C++ imports as StatusCode::Invalid — so S3 429/503/timeouts (codes 108/109/110) and mid-scan not-found (code 12) arrive as Status::Invalid still carrying the __LOON_VORTEX_FFI_ERRCODE__ marker, and the guard passes them through undecoded. The internal marker leaks into user-facing messages, and transient throttles collapse into milvus::DataFormatBroken (permanent), reintroducing the exact "corrupt file indistinguishable from throttle" bug this PR targets. The removed vortex decoder re-parsed the message unconditionally, so this is a behavioral regression. Affected call sites: lance_table_reader.cpp:396-398, 443-445, 463-465, and the wrapped reader at :494. Suggestion: discriminate on marker presence instead of StatusCode — decode whenever the message contains kBridgeErrCodeMarker, and only pass through marker-less statuses; add a test asserting arrow::Status::Invalid(marker + "109; ...") decodes to a retryable transient (existing tests only fabricate the marker inside IOError, lance_bridge_error_test.cpp:93). (raised by xaxys, xiaocai2333)

Medium

• cpp/src/format/bridge/rust/src/gcp_impersonation.rs:166 — HTTP transient errors on the GCP impersonation token paths are never classified as transient. Both the metadata-server (:161-174) and IAM generateAccessToken (:187-201) paths stringify reqwest's error_for_status() into object_store::Error::Generic, whose wording doesn't match the "non-2xx status code: NNN" pattern that classify_http_status_in_message (bridge_error.rs:155-166) requires — so 429/503 are not tagged LOON_TRANSIENT_* and 403 is not mapped to AccessDenied. Fail-safe (falls into the conservative non-retriable bucket), but real. Suggestion: classify at the producer while the typed reqwest::StatusCode is still in hand, instead of relying on message-pattern matching downstream. (raised by xaxys, xiaocai2333)

• cpp/src/format/bridge/rust/src/aliyun_oss_provider.rs:397 — Aliyun OSS STS/OIDC credential fetch 429/503s are not marked transient and 403 is not mapped to AccessDenied. Both :397 and :1292 capture resp.status() but then stringify it via Err(format!("... HTTP {status}: {text}")), wrapped as object_store::Error::Generic (:594-621), which the "non-2xx status code: NNN" classifier cannot match — so a throttled first fetch or refresh becomes a non-retriable StorageError. This occurs in ObjectStoreProvider::new_store and is independent of the streaming FFI issue above. Suggestion: classify directly while the typed StatusCode is still available. (raised by xaxys)

[czs007]

Round-2 findings addressed (commit just pushed):

• High — confirmed regression, good catch. The round-1 !IsIOError() guard was written without verifying what StatusCode the arrow C-stream import assigns to Rust stream errors — it's EINVAL ⇒ Invalid, exactly as you traced, so mid-scan errors bypassed decoding. Discrimination is now on marker presence: any status carrying the marker decodes regardless of StatusCode; marker-less statuses pass through untouched, so the no-downgrade property from round 1 is preserved (both directions test-pinned — new test drives Invalid(marker+"109") → retryable transient detail with no marker leakage, and Invalid(marker+"12") → ENOENT).
• GCP impersonation / Aliyun STS: both now emit the canonical non-2xx status code: NNN: pattern at the producer while the typed StatusCode is in hand (reqwest's e.status() / resp.status()), so the downstream classifier recovers the class; 401/403 additionally map to access-denied(105) rather than staying untagged.

Verification: cargo test --release 4/4 (403 case added); full build clean; bridge error suites 9/9; lance suites 27/27.

[jiaqizho]

This logical is vortex only. Because vortex using the filesystem_c as the obejct storage accessor.

But lance/iceberg won't use the filesystem_c. the error from obejct-store won't got any extend_status from the error message. So bring the vortex error convertor out the bridge file is messlesss.

[czs007]

You're describing the pre-PR world accurately: the marker used to have exactly one producer — vortex's filesystem_c.rs (LOON codes carried back from the C++ filesystem layer), and lance/iceberg, which use their own Rust-native object_store, never produced it. If that were still true, sharing the decoder would indeed be pointless.

The core of this PR is that it adds a second, lance-native producer that doesn't involve filesystem_c at all — bridge_error.rs:

• classify_lance_error() classifies the typed lance::Error (NotFound family → 12, CorruptFile → data-corrupt, lance's own RetryableCommitConflict/TooMuchWriteContention → transient, and the IO variant downcast to object_store::Error → NotFound/PermissionDenied/Precondition, plus the post-retry HTTP status recovered from object_store's message pattern);
• BridgeError's Display embeds the same marker; every lance bridge fn now returns Result<T, BridgeError>, and BatchFutStreamReader::next() wraps mid-scan stream errors the same way before arrow FFI stringifies them.

So the decoder now has two producers feeding one wire format: vortex (LOON codes via filesystem_c) and lance (classified lance::Error via bridge_error.rs) — that's what motivated hoisting it. It's verified end-to-end on the lance path: LanceBridgeErrorTest.OpenNonexistentDatasetClassifiesNotFound opens a nonexistent dataset through real lance, and the real DatasetNotFound arrives in C++ as ENOENT-detail → ObjectNotExist; BridgeErrorTest.TranslateDecodesMarkerRegardlessOfStatusCode covers the mid-scan form.

Where you're fully right today: iceberg. Its bridge errors carry no classification yet (stated in the PR's honest-scope section) — for iceberg this PR only removes the exception leak; classifying iceberg-rust error kinds is follow-up. If the sticking point is naming/placement rather than the mechanism, happy to rename or move things (e.g. keep bridge_error.{h,cpp} but note vortex/lance as the two current producers in the header comment).

[jiaqizho]

Then u should not use the __LOON_VORTEX_FFI_ERRCODE__ as marker. maybe use __LOON_RUST_BRIDGE_ERRCODE__ to replace it.

[czs007]

Done — renamed to __LOON_RUST_BRIDGE_ERRCODE__= across all producers and the decoder in lockstep (vortex's filesystem_c.rs, lance's bridge_error.rs, the C++ bridge_error.cpp, and both test suites). The marker never persists to storage nor crosses a process boundary (encoded and decoded within one call stack), so the rename is compatibility-free. Verified: cargo 4/4, bridge/lance/vortex error suites 13/13.

[jiaqizho]

Does it really make sense to distinguish between internal errors within the Lance object store?

From the perspective of the interface, both Lance and Iceberg are accessed via table-level APIs rather than file-level APIs. This implies that internal errors within Lance or Iceberg might be unrecoverable for the milvus-storage or milvus.

[czs007]

Good question — it goes to the heart of why this classification exists. The key point: the consumer of retriability is not milvus-storage retrying a file operation; it's the milvus querynode deciding whether to re-route the whole request to another replica. That decision point exists at exactly the table-API granularity you describe:

• A table-level take/scan fails mid-stream because S3 returned 429/503 → the error IS unrecoverable for this call, agreed — but re-issuing the whole request (against another replica / a moment later) very likely succeeds. That's StorageTransientError/2045, and the retry loop that consumes it lives in milvus's lb_policy, not inside this library.
• The same table-level call fails because a fragment file is corrupt → re-routing hits the same shared object store and fails identically. Never retry: DataFormatBroken/2024.
• DatasetNotFound → ObjectNotExist/2017 matters operationally regardless of retriability: milvus can tell "the data is gone (stale loadinfo / GC'd)" apart from "storage is misbehaving" — different alarms, different remediation.

Same failure surface at the interface, opposite correct reactions — one bit ("error happened") cannot carry that. This is also the empirically hot case: the #574 review flagged mid-scan throttling as the most common transient in production, and it is precisely a table-level scan that hits it.

Where your instinct is fully encoded in the table already: everything not positively identified defaults to non-retriable 2044 — Lance-internal errors with no clear signal (the Internal/Generic variants) land exactly where you'd put them, unrecoverable. Only positive signals (HTTP 429/503, lance's own RetryableCommitConflict, typed NotFound) get distinguished. And iceberg today matches your description completely: no classification, everything non-retriable — follow-up only if a consumer shows up.

[jiaqizho]

I don't think the error-code contract is actually unified here. filesystem_c.rs forwards LoonFFIResult.err_code verbatim, while the Lance bridge independently classifies LanceError into a subset of LOON/ExtendStatus codes plus bridge-private codes. Therefore the shared marker/parser currently unifies the wire format, but not the error taxonomy.

For example:

• Vortex may surface an S3 not-found as 104 (AwsErrorNotFound), while Lance maps object_store::Error::NotFound to 12 (ENOENT). These converge at the segcore boundary today, but carry different Arrow status details in intermediate layers.
• 109 means storage throttling in the filesystem FFI, but Lance also uses it for RetryableCommitConflict and TooMuchWriteContention. Both become retryable, but unrelated failures are reported under the same throttling category.
• The filesystem FFI may also emit base codes such as 1-9; the shared decoder does not interpret them and silently falls back to a plain IOError.

This does not cause an immediate parsing failure because the decoder accepts the union of known codes and conservatively falls back for unknown ones. However, it can produce different structured statuses for equivalent failures and overload the same retry/metrics category with different semantics. More importantly, broad Lance not-found variants are collapsed into code 12, while LanceTableWriter consumes ENOENT as the create-if-missing signal; an internal object/manifest/ref/version not-found could therefore be mistaken for dataset absence.

Could we define one canonical BridgeErrorCode taxonomy (for example, separating DatasetNotFound, ObjectNotFound, RetryableCommitConflict, and storage throttling), or use Lance-specific private codes and normalize them explicitly in the C++ decoder? At minimum, I think we should avoid reusing 109 for commit conflicts and avoid using the dataset-creation ENOENT signal for every Lance not-found variant.

[xiaofan-luan]

Thanks — agreed that the previous version unified the marker/wire format but still overloaded the taxonomy. Fixed in cd7a578 after rebasing onto current main:

• 109 is now reserved exclusively for object-store/storage throttling (StorageTransientThrottling).
• Added 113 (LanceWriteContention, retryable → segcore 2045) for RetryableCommitConflict / TooMuchWriteContention.
• Added 114 (LanceResourceNotFound, non-retryable → segcore 2017) for Lance internal resource-not-found variants and wrapped object_store::NotFound.
• Only LanceError::DatasetNotFound emits code 12 / ENOENT. NotFound, IndexNotFound, RefNotFound, VersionNotFound, and object-store not-found no longer trigger the writer's create-if-missing path.
• The new codes are wired through the C ABI/export maps, Python bindings, ExtendStatusDetail, retryability, and Segcore projection, with Rust/C++/FFI tests.

Also regenerated the #596 error-handling ratchet baseline after the rebase: lance_bridge.cpp 21→0 and iceberg_bridge.cpp 2→0.

[xiaofan-luan]

Code review

Mechanism and direction look right to me — one marker, one shared decoder, classification stays with the producer. I reviewed dcc5179 and re-checked against the current head cfee57f; several things I had flagged you had already fixed in cfee57f (the unclassified-stream case in particular — BRIDGE_ERRCODE_UNCLASSIFIED closes exactly the Invalid/EINVAL → DataFormatBroken hole).

Two CI blockers, one of which is not obvious from the summary line:

1. check — the error-handling ratchet, and it needs a rebase before it can even be fixed

-throw	cpp/src/format/bridge/rust/src/iceberg_bridge.cpp	2
-throw	cpp/src/format/bridge/rust/src/lance_bridge.cpp	21

The gate requires an exact match, so a burn-down has to be recorded in the same PR. The reason there was no local signal: this PR branches from 4e697a0, one commit before #596 added the gate, so cpp/scripts/error_handling_baseline.tsv does not exist on the branch at all. Rebase onto main, then cpp/scripts/error_handling_ratchet.sh update and commit the regenerated baseline. Expected diff is exactly those two lines (throw total 49 → 26).

2. build — the new Rust test step fails on missing protoc, not on an assertion

The failing step is the one this PR adds, and the log tail is:

lance-encoding build-script-build (exit status: 1)
Error: Could not find `protoc`. If `protoc` is installed, try setting the `PROTOC`
environment variable to the path of the protoc binary.

milvus-storage/.github/workflows/cpp-ci.yml

Lines 69 to 77 in cfee57f

	- name: Test Rust bridge error classifier
	working-directory: ./cpp
	env:
	CARGO_TARGET_DIR: ${{ github.workspace }}/cpp/build/Release/cargo/build
	RUSTFLAGS: -C force-frame-pointers=yes
	run: |
	cargo test --locked --manifest-path src/format/bridge/rust/Cargo.toml --lib bridge_error::tests

make build passes because it runs inside the conan environment; the standalone cargo test step does not inherit it, and the debug profile rebuilds the dependency tree, so lance-encoding's build script runs without protoc on PATH. Either install protobuf-compiler in that step, set PROTOC from the conan-provided one, or run the test through the same environment as the build.

3. 109 for commit conflicts — @jiaqizho's comment is still unanswered and I think it is right on this point

RetryableCommitConflict / TooMuchWriteContention → LOON_TRANSIENT_THROTTLING:

milvus-storage/cpp/src/format/bridge/rust/src/bridge_error.rs

Lines 113 to 121 in cfee57f

	LanceError::CorruptFile { .. } => Some(BRIDGE_ERRCODE_DATA_CORRUPT),
	LanceError::NotSupported { .. } => Some(BRIDGE_ERRCODE_NOT_SUPPORTED),
	// Lance itself declares these retryable: the failed attempt is spent,
	// but a fresh attempt (new commit round) can succeed. This is the
	// producer's own classification, not invented here.
	LanceError::RetryableCommitConflict { .. } | LanceError::TooMuchWriteContention { .. } => {
	Some(LOON_TRANSIENT_THROTTLING)
	}
	// IO wraps the underlying object_store error as a boxed source;

The retriability itself is defensible — AwsErrorNoSuchUpload sets the precedent for "the producer's budget is spent but a fresh outer attempt succeeds", and I checked lance 7.0.0: RetryableCommitConflict escapes via commit.rs:979 without the retry loop consuming it, so "Lance itself declares these retryable" is accurate. What does not hold up is the code: 109 is reserved for object-store rate limiting (s3_internal.h maps THROTTLING/SLOW_DOWN/HTTP 429 to it), and error_to_string(109) prints StorageTransientThrottling, so a write-commit conflict now shows up in logs and metrics as throttling. TxnExhaustedRetry/TxnResolutionFailed (111/112) already exist for this shape — though they map to permanent 2044, so if you want it retriable it needs a new code rather than reusing 109.

4. Two nits

EstimateFragmentColumnMemory is the only remaining catch (rust::cxxbridge1::Error) in the lance/iceberg bridges that does not route e.what() through MakeBridgeErrorStatus, so a classified failure ships the raw __LOON_RUST_BRIDGE_ERRCODE__=NNN; text in the message. Only reaches a debug log today, but it is a public-header API:

milvus-storage/cpp/src/format/bridge/rust/src/lance_bridge.cpp

Lines 125 to 130 in cfee57f

	}
	return memory_sizes;
	} catch (const rust::cxxbridge1::Error& e) {
	return arrow::Status::NotImplemented("Lance column memory size estimation is not available: ", e.what());
	}
	}

And lance guarantees a marker by construction (lance_bridgeimpl.rs:678 wraps every stream error in BridgeError), while vortex does not (vortex_bridgeimpl.rs:1834 wraps in ArrowError::ExternalError directly) — vortex only gets tagged because filesystem_c.rs happens to embed the marker itself. Worth making that uniform so the guarantee is structural rather than incidental.

For what it is worth, several things that looked alarming did not survive checking, so nobody needs to re-raise them: the find()-anywhere marker match is #574's design and this PR's marker-presence gate is strictly narrower than what MakeVortexErrorStatus did on main; classify_http_status_in_message using find rather than rfind is correct, since the pattern contains spaces (object keys are percent-encoded, http::Uri cannot hold one) and every producer in this PR puts the authoritative status first and untrusted text last; and the missing 404 arm is unreachable because object_store maps 404 to typed Error::NotFound before Generic.

Happy to push the two mechanical fixes (1) and (2) myself if that is easier — maintainerCanModify is on.

🤖 Generated with Claude Code

[xiaofan-luan]

Follow-up — my review above crossed with cd7a578, so two of its items are already stale: the ratchet baseline (fixed by the rebase) and the 109 overload (fixed by the new 113/114). Please ignore those two.

What is still red is only the third one, and it is unchanged on cd7a578:

Test Rust bridge error classifier
  lance-encoding build-script-build (exit status: 1)
  Error: Could not find `protoc`. ... try setting the `PROTOC` environment variable

Worth noting from the same log: conan does provide it in this job — -- Conan: Component target declared 'protobuf::libprotoc' and protobuf-conan-protoc-target.cmake both appear during the Build step. So protoc exists on the runner; the new cargo test step just doesn't inherit the environment that points at it, and its debug profile rebuilds the dependency tree so lance-encoding's build script runs. Exporting PROTOC (or apt-get install -y protobuf-compiler in that step) should be all it takes.

Still open from the review, both minor: EstimateFragmentColumnMemory not routing e.what() through MakeBridgeErrorStatus, and vortex's iterator not wrapping in BridgeError the way lance's does.

One heads-up for whoever merges second: this now overlaps #603, which converts ffi_error_code.h / extend_status.{h,cpp} / result_c.cpp / both export maps / the python cdef to a single generated table. 113/114 are added by hand to those same files here, so they will conflict textually and the new codes will need a row in the generated table. Happy to rebase #603 on top of this one once it lands — it is the later PR.

[xiaofan-luan]

Pushed 9b6230a to this branch (maintainerCanModify) — one line, installing protobuf-compiler next to libaio-dev in the build job, plus a comment recording why.

Revert it if you would rather fix it differently; the alternative I considered was adding --release to the cargo test step so it reuses corrosion's artifacts instead of rebuilding the dependency tree in the dev profile (which is also why that step takes ~20 min). I did not take that route because the step sets its own RUSTFLAGS: -C force-frame-pointers=yes, and any flag mismatch with the Release build forces the rebuild anyway.

Worth flagging while CI re-runs: unittest has been skipped on every run so far because build failed first, so the C++ suite has not actually executed for this PR yet. This run should be the first real signal.

[xiaofan-luan]

Pushed 3df645b — one real gap left in classify_lance_error, and it is on the hottest retriable path rather than a corner.

LanceError::IO was handled but Wrapped / External fell through to _ => None, which throws away a classification that is already sitting one box deeper. lance-io's batch read scheduler stashes the failing task's error and re-wraps it when the batch drops:

// lance-io-7.0.0/src/scheduler.rs
Err(err) => { self.err.get_or_insert(Box::new(err)); }
...
impl Drop for MutableBatch<F> {
    fn drop(&mut self) {
        let result = if self.err.is_some() { Err(Error::wrapped(self.err.take().unwrap())) }

and impl From<object_store::Error> for lance::Error yields IO { source }. So on any batched read an S3 throttle arrives as Wrapped(IO(object_store::Generic)), not IO(..), and was classified permanent. The encoding decoder wraps the same way (lance-encoding/src/decoder.rs:1496,1902). That is a fair chunk of the "a corrupt file and an S3 throttle look identical" problem this PR set out to fix, coming back one layer up.

Both wrappers now downcast: object_store::Error first, then LanceError recursively (each step strips a layer, so it terminates). The object_store match is extracted into classify_object_store_error so the two paths share one table. New test wrapped_errors_keep_the_inner_classification pins single wrap, double wrap, External holding the store error directly, and an opaque box staying untagged.

Cloned { message: String } is left alone deliberately — lance stringifies errors when cloning them across task boundaries, so the type is gone before the bridge can see it. Not fixable here.

Verified locally: cargo test --lib bridge_error::tests → 8 passed. (On macOS that step needs SDKROOT=$(xcrun --show-sdk-path) or ring's build script fails on TargetConditionals.h — unrelated to CI.)

Remaining from my earlier review, both still open and both minor: EstimateFragmentColumnMemory not routing through MakeBridgeErrorStatus (raw marker leaks into the message), and vortex's iterator not wrapping in BridgeError the way lance's does.

[bigsheeper]

Must-fix issues introduced by this PR: 0
Merge recommendation: Mergeable as-is — this review found no must-fix issue introduced by this PR.

Adversarial review found no issues requiring changes.

Verified:

• cpp/src/format/bridge/rust/src/bridge_error.cpp:142-159 — TranslateBridgeStatus no longer short-circuits on !status.IsIOError(); it checks the message for kBridgeErrCodeMarker and decodes marked errors regardless of the Arrow status code, so an Invalid produced from FFI EINVAL is correctly restored to the embedded bridge classification.
• cpp/src/format/bridge/rust/src/lance_bridgeimpl.rs:668-679 — mid-scan iteration errors are still wrapped in BridgeError, keeping the producer side of the marker protocol connected.
• cpp/src/format/lance/lance_table_reader.cpp:493-495 — the live stream reader runs the translation on every ReadNext, so mid-scan errors cannot bypass decoding.
• cpp/test/format/lance/lance_bridge_error_test.cpp:117-131 — regression coverage confirms Invalid(marker + 109) becomes a retryable StorageTransientThrottling status, Invalid(marker + 12) regains ENOENT, and an unknown code (marker + 1000) falls back to a plain IOError.

The previously reported high-severity finding is confirmed fixed, and no new issues were introduced by the incremental commits.