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 👍 / 👎.
1 participant
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)