Carry a typed classification on storage failures - #491
Conversation
…rage Fifty call sites raised `OmniError::Lance` for failures that never touched storage: missing columns, unexpected Arrow array types, blob-descriptor shape mismatches, and row-alignment violations. These are engine invariant violations wearing a storage costume. They are harmless today because `OmniError::Lance` is an untyped string bucket that everything above treats as opaque. They stop being harmless the moment storage failures carry a typed retryability signal: a supervisor deciding "should I retry this graph?" would read an Arrow-shape bug as a storage condition and reason about it as transient. Retarget them to `OmniError::manifest_internal`, which is what they always were. No behavior change beyond the error's own classification: both variants surface as an opaque internal failure to every current consumer, and no test asserts on their text. The sites that legitimately wrap a `lance::Error` with context keep the storage variant.
Every Lance and object-store failure was flattened to a string
(`OmniError::Lance(String)`), so nothing above the substrate boundary could
tell a transient transport condition from a permanent one without parsing
error text — which no code was willing to do, correctly.
The cost is not hypothetical. `object_store` routes every retried-and-exhausted
HTTP condition — 5xx, connection resets, DNS, TLS, budget exhaustion — into
`Error::Generic`, and Lance keeps that value reachable through `IO { source }`.
All of it arrived upstream as an opaque string, indistinguishable from a
corrupt file. A caller wanting to retry an S3 blip had no signal to retry on.
Introduce `StorageFailureKind` — `Transient`, `Configuration`, `NotFound`,
`Permanent` — and derive it once, at each substrate boundary:
- `From<lance::Error>` classifies by variant and, for `IO`, downcasts through
the source chain to the originating `object_store::Error`. No text parsing.
- The storage adapter's `storage_backend_error` now takes the typed
`object_store::Error` instead of `impl Display`, so the classification
survives the boundary it previously died at. `LocalFileSystem` also reports
plain OS errors through `Generic`, so that path inspects the underlying
`io::Error` — a wrong path is not something retrying fixes.
- `From<ArrowError>` maps to a manifest-internal failure: an Arrow shape
violation is an engine bug and must never be classifiable as storage.
- `From<DataFusionError>` classifies by source chain, because the same type
carries both in-memory execution failures and failures raised during a Lance
scan.
The enum is closed on purpose: adding a class should be a compile error at
every consumer that switches on it. `OmniError` becomes `#[non_exhaustive]` so
the reverse is not true for the error as a whole; that costs one catch-all arm
in the server's translation.
`with_context` replaces the several sites that formatted an already-classified
error into a new error's message, which threw the classification away. Context
says where a failure happened; it never changes what it is.
Commit conflicts keep their dedicated variant — RFC-023's effect-free key
fence depends on telling them apart from arbitrary storage failures.
Display is byte-identical (`storage: <message>`), so logs, operator runbooks,
and the HTTP mapping are unchanged. This commit adds the signal; no caller
consumes it yet.
One deliberate omission: the publisher's namespace-validation conflicts keep
the storage variant as `Permanent` rather than becoming manifest conflicts.
That retargeting would change their HTTP status, which is a behavior change
that does not belong in a classification refactor.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 3 potential issues.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 496bee9. Configure here.
| object_store::Error::JoinError { .. } => StorageFailureKind::Transient, | ||
| object_store::Error::NotFound { .. } | object_store::Error::NotModified { .. } => { | ||
| StorageFailureKind::NotFound | ||
| } |
There was a problem hiding this comment.
NotModified mapped to NotFound
Medium Severity
object_store::Error::NotModified is classified as StorageFailureKind::NotFound. A 304 means the object exists and matched a precondition; treating it as absence can make the first consumer of kind take create-or-recreate paths for a live object.
Reviewed by Cursor Bugbot for commit 496bee9. Configure here.
| StorageError::Backend(StorageFailure::new( | ||
| kind, | ||
| format!("storage {} failed for '{}': {}", action, uri, err), | ||
| )) |
There was a problem hiding this comment.
Backend errors gain storage prefix
Medium Severity
Object-store failures from storage_backend_error now become OmniError::Storage, whose storage: {message} Display and HTTP mapping add another storage: on top of messages that already begin with storage … failed. Logs and HTTP bodies therefore change from the previous manifest-internal wording.
Additional Locations (2)
Reviewed by Cursor Bugbot for commit 496bee9. Configure here.
| return Ok(true); | ||
| }; | ||
| let kind = BlobKind::try_from(kind).map_err(|e| OmniError::Lance(e.to_string()))?; | ||
| let kind = BlobKind::try_from(kind).map_err(OmniError::from)?; |
There was a problem hiding this comment.
Blob kind still classified as storage
Medium Severity
Invalid BlobKind values still go through OmniError::from on lance::Error, so a descriptor shape mismatch becomes a classified Storage failure (typically Configuration) instead of a manifest-internal engine error like the other retargeted non-storage sites.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 496bee9. Configure here.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 496bee9ec1
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| object_store::Error::NotFound { .. } | object_store::Error::NotModified { .. } => { | ||
| StorageFailureKind::NotFound | ||
| } |
There was a problem hiding this comment.
When a conditional object-store read receives NotModified (HTTP 304 because the supplied ETag still matches), the object is known to exist, yet this arm reports StorageFailureKind::NotFound, whose public contract says the object genuinely does not exist. SDK consumers using storage_failure() can therefore take absence/recreation or recovery actions on a cache-freshness response; give NotModified a non-absence disposition instead of grouping it with NotFound.
Useful? React with 👍 / 👎.
Maintainer review: changes requestedThe typed storage-failure boundary is the right direction, but its current classifications are not safe enough to drive retries:
This is a public cross-cutting taxonomy used by #489, so it needs a design RFC covering retry semantics, compatibility, and unknown/future variants. Main already owns RFC-031; use the next available number. Please rebase onto current main after the taxonomy is settled, resolve the existing review threads, and rerun canonical workspace/failpoint tests, both Clippy graphs, format, and focused storage-classification tests. #488 and #489 should be restacked afterward. |
aaltshuler
left a comment
aaltshuler
left a comment
There was a problem hiding this comment.
Review of exact head 496bee9e. The typed-boundary direction is useful, but the current model conflates an observed storage condition with effect certainty and operation-specific retry safety. The six inline comments below cover source-level blockers not already raised by the existing 304, doubled-prefix, and Blob threads. This branch is also 58 main commits behind, targets Lance 9 while main uses Lance 10, and produces 11 content conflicts; please rebuild the change on current main rather than resolving it mechanically. The replacement should preserve current Blob/precondition/receipt mappings and run project CI.
| // remote stores carry a `RetryError` instead and fall through to | ||
| // `Transient`, which is the accurate reading for them. | ||
| object_store::Error::Generic { source, .. } => { | ||
| classify_io_source(source.as_ref(), 0).unwrap_or(StorageFailureKind::Transient) |
There was a problem hiding this comment.
[P1] Do not default Generic or future variants to Transient. In object_store 0.13.2, Generic also carries configuration parsing, credential/builder, checksum/URL/decoding failures, bare redirects, and non-retryable HTTP statuses. That is absence of classification evidence, not positive transient evidence. Add Unknown/Indeterminate, whitelist only typed transient sources, and keep write replay safety out of this condition enum.
| // `Fenced` belongs to Lance's MemWAL writer, which OmniGraph does not | ||
| // operate (see docs/dev/wal-removal.md). Unreachable in practice; | ||
| // classified conservatively rather than given a speculative retry. | ||
| _ => StorageFailureKind::Permanent, |
There was a problem hiding this comment.
[P1] Handle Lance::Namespace before this wildcard. Lance preserves the typed NamespaceError, whose codes distinguish service-unavailable/throttling, not-found, authentication/input/unsupported, concurrent-modification/already-exists, and internal/schema failures. Collapsing all of them to Permanent discards a complete upstream taxonomy and gives callers the wrong remedy.
| return Some(omnigraph_storage::classify_object_store_error(error)); | ||
| } | ||
| if let Some(error) = source.downcast_ref::<lance::Error>() { | ||
| return Some(classify_lance_error(error)); |
There was a problem hiding this comment.
[P2] The advertised depth bound resets here. classify_lance_error takes no remaining-depth argument, and a nested Lance::IO restarts classify_error_source(..., 0); an arbitrarily nested Lance chain can therefore bypass MAX_ERROR_SOURCE_DEPTH. The same early return also stops at source-bearing Lance External/Wrapped variants. Use one iterative or budget-threaded source walker and test exactly-at-limit and over-limit chains.
| source: &(dyn std::error::Error + 'static), | ||
| depth: usize, | ||
| ) -> StorageFailureKind { | ||
| find_substrate_kind(source, depth).unwrap_or(StorageFailureKind::Transient) |
There was a problem hiding this comment.
[P1] This fallback makes classification both unsafe and wrapper-dependent. Lance uses IO for protobuf decoding, URL/object-path parsing, SQL parsing, and direct std::io::Error sources, but this walker does not inspect direct std::io::Error; all unknown shapes become Transient. The same PermissionDenied can therefore be Configuration through object_store, Transient through Lance IO, or plain IO through StorageError. Unknown sources must remain Unknown, and the same underlying source should classify identically through every supported wrapper.
| lance::Error::RetryableCommitConflict { .. } | ||
| | lance::Error::TooMuchWriteContention { .. } | ||
| ) { | ||
| return Self::RetryableCommitConflict(error.to_string()); |
There was a problem hiding this comment.
[P1] Do not mint the whole-operation retry signal in the global From<lance::Error> conversion. A Lance commit conflict does not by itself prove that the enclosing graph operation is effect-free. That proof exists only inside the exact table-store commit adapter; mint RetryableCommitConflict there after the fence is established, and leave other Lance conflicts as typed precondition/unknown storage failures.
| /// They never describe the storage layer, so they must not be classifiable | ||
| /// as a storage condition a caller might retry. | ||
| fn from(error: arrow_schema::ArrowError) -> Self { | ||
| Self::manifest_internal(error.to_string()) |
There was a problem hiding this comment.
[P1] Arrow errors are not uniformly in-memory shape failures. Arrow 58 exposes IoError and ExternalError with source chains that may contain std::io, object-store, or Lance failures. Flattening every variant to manifest-internal makes classification depend on which wrapper carried the same substrate error. Walk typed external/IO sources first, then classify only pure shape/compute variants as internal.
2 participants
Summary
Every Lance and object-store failure was flattened into
OmniError::Lance(String), so nothing above the substrate boundary could distinguish a transient transport condition from a permanent one without parsing error text.That is not hypothetical.
object_storeroutes every retried-and-exhausted HTTP condition — 5xx, connection resets, DNS, TLS, retry-budget exhaustion — intoError::Generic, and Lance keeps that value reachable throughIO { source }. All of it arrived upstream as an opaque string, indistinguishable from a corrupt file.This adds
StorageFailureKind(Transient/Configuration/NotFound/Permanent) and derives it once, at each substrate boundary:From<lance::Error>classifies by variant and, forIO, downcasts through the source chain to the originatingobject_store::Error. No text parsing anywhere.storage_backend_errortakes the typedobject_store::Errorinstead ofimpl Display, so the classification survives the boundary it previously died at.LocalFileSystemreports plain OS errors throughGenerictoo, so that path inspects the underlyingio::Error— a wrong path is not something retrying fixes.From<ArrowError>maps to a manifest-internal failure: an Arrow shape violation is an engine bug and must never be classifiable as storage.From<DataFusionError>classifies by source chain, since the same type carries both in-memory execution failures and failures raised during a Lance scan.OmniErrorbecomes#[non_exhaustive](one catch-all arm added in the server).StorageFailureKindstays closed on purpose: adding a class should be a compile error at every consumer that switches on it.The first commit is separable and lands first: ~50 call sites raised
OmniError::Lancefor failures that never touched storage — missing columns, unexpected Arrow array types, blob-descriptor shape mismatches. Harmless while the variant was an untyped string bucket; actively wrong once storage failures carry a retryability signal.Behavior
Displayis byte-identical (storage: <message>), so logs, operator runbooks, and the HTTP mapping are unchanged. This PR adds the signal; no caller consumes it yet.One deliberate omission: the publisher's namespace-validation conflicts keep the storage variant as
Permanentrather than becoming manifest conflicts. That retargeting would change their HTTP status, which is a behavior change that does not belong in a classification refactor.Verification
lance::Errorandobject_store::Errorshapes (not approximations) and pin: transient survives the Lance boundary, permission/absence classify correctly, commit conflicts keep their dedicated variant, Arrow failures are never storage, DataFusion classifies by source not call site,Displayis unchanged, and context preserves classification.Genericstays transient while a local OS error through the same variant does not.cargo test --workspace --locked --features omnigraph-engine/failpoints,omnigraph-cluster/failpoints— 75 suites, 2019 tests, zero failures.cargo clippy --workspace --all-targets --locked -- -D warnings -W clippy::dbg_macro— clean.cargo fmt --all --check.No storage-format migration.
Note
Medium Risk
Wide mechanical refactor across manifest, recovery, exec, and HTTP error translation; wrong classification could mislead future retry logic, though current HTTP/logs behavior is intended to stay the same.
Overview
Replaces the flat
OmniError::Lance(String)bucket withOmniError::Storage(StorageFailure), carryingStorageFailureKind(Transient,Configuration,NotFound,Permanent) derived once at substrate boundaries—no error-text parsing above storage.omnigraph-storageaddsclassify_object_store_error(includingGeneric→ transient for remote S3 vs localio::Errorinspection), wraps failures inStorageError::Backend, and changesstorage_backend_errorto accept typedobject_store::Error.omnigraphcentralizes Lance mapping inFrom<lance::Error>(commit conflicts stayRetryableCommitConflict), walks source chains forDataFusionError, mapsArrowErrorto manifest-internal, and addsstorage_context/storage_permanent/with_context. Hundreds of call sites switch fromOmniError::Lance(...)toOmniError::fromormanifest_internalfor engine/shape bugs that were mislabeled as storage.omnigraph-servermapsOmniError::Storageto HTTP internal and adds a catch-all for#[non_exhaustive]OmniError. Operator-facingstorage: …display is unchanged; classification is additive (not yet consumed for retry/HTTP branching beyond mapping).Reviewed by Cursor Bugbot for commit 496bee9. Bugbot is set up for automated code reviews on this repo. Configure here.
Greptile Summary
The PR replaces string-flattened storage errors with typed failure classifications while preserving existing operator-facing messages.
OmniErrornon-exhaustive and updates the server fallback mapping.Confidence Score: 5/5
The PR appears safe to merge; no actionable changed-code defects were identified.
Storage errors are classified at typed substrate boundaries, non-storage failures remain separately represented, existing display behavior is preserved, and the dependency advisories inspected are unchanged from the base branch.
Important Files Changed
Flowchart
%%{init: {'theme': 'neutral'}}%% flowchart LR OS[object_store::Error] --> OC[classify_object_store_error] OC --> SF[StorageFailure] LE[lance::Error] --> LC[classify_lance_error] LC --> SF DF[DataFusionError] --> SC{Storage source chain?} SC -->|yes| SF SC -->|no| DE[DataFusion execution error] AE[ArrowError] --> ME[Manifest internal error] SF --> OE[OmniError::Storage] OE --> API[Existing HTTP 500 storage message]Reviews (1): Last reviewed commit: "feat(error): carry a typed classificatio..." | Re-trigger Greptile
Context used (5)