ref(service): Classify service errors by kind#493
Open
lcian wants to merge 20 commits into
Open
Conversation
This comment has been minimized.
This comment has been minimized.
9ca4473 to
2953792
Compare
Add service-level error kinds so API handlers can map failures without matching every backend-specific variant. Keep client metadata errors distinct from internal metadata failures and classify transient backend failures as retryable/service-unavailable paths. Co-Authored-By: OpenAI Codex <noreply@openai.com>
2953792 to
445c724
Compare
Remove private wrapper constructor helpers that only duplicated the public Error helpers. Keep classification policy in the public constructors so call sites have fewer names to reason about. Co-Authored-By: OpenAI Codex <noreply@openai.com>
Remove the service Error conversion from objectstore metadata errors. Map metadata parsing and serialization failures at each backend call site so the context and classification stay visible. Co-Authored-By: OpenAI Codex <noreply@openai.com>
Update stream documentation links after the client stream error rename. This keeps rustdoc clean under CI's denied warnings. Co-Authored-By: OpenAI Codex <noreply@openai.com>
Move reqwest retryability out of the shared error wrapper and back next to the GCS retry loop. Let ReqwestError handle only construction and service-level classification. Co-Authored-By: OpenAI Codex <noreply@openai.com>
lcian
commented
Jun 8, 2026
| let metadata_headers = metadata.to_headers("").map_err(ServiceError::from)?; | ||
| let metadata_headers = metadata | ||
| .to_headers("") | ||
| .map_err(|cause| ServiceError::metadata("failed to serialize object metadata", cause))?; |
Member
Author
There was a problem hiding this comment.
Improvement: here we have a valid Metadata, it's therefore our fault if we fail to serialize it, and now we can correctly map this to an internal error as opposed to always blaming the client.
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
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.
This is a different take on giving us more control over HTTP status codes when erroring out from the
servicelayer that doesn't make callers more verbose, but actually less so in some cases.ServiceErrorvariants gain a kind that's used to determine log level and HTTP status code when converting toApiErrorResponse. Thekindis implemented via https://crates.io/crates/derive-error-kind which lets us do it concisely via aerror_kindattribute on each variant. This is a very simple proc macro that we could also vendor in.Existing constructors for specific
Errorvariants remain, and map toErrorKind::Internal.For errors that could map to different kinds depending on the context, we also introduce additional intermediate wrapper types (needed to support different kinds) and constructors on
ServiceError.Namely:
reqwest_transparent(alternative to the existingreqwesthelper),serde_client(alternative to the existingserdehelper), and themetadata/metadata_clientpair.As a special case, we implement
From<std::io::Error> for Errorso that callers can keep using?to propagate io errors. That's because so far we always wantio::Errorto map toErrorKind::internal, so there's no reason to make callers more verbose in this case.This has the added benefit that the impl can encapsulate the
unpack_client_errorlogic, to avoid repeating it in every caller.Also renames
ClientErrortoClientStreamErroras that's a less confusing name.Refs FS-358