Repository navigation
fix: map a failed outbound RPC instead of passing its status to the caller - #50
Merged
Merged
Conversation
…aller AsyncExceptionHandlerInterceptor re-raised every grpc.RpcError untouched. grpc.aio.AbortError is not an RpcError, so that clause only ever let through grpc.aio.AioRpcError from calls the handler made, and grpc.aio answered the caller with UNKNOWN and the downstream error's repr as details. Such an error now goes through the status map like any other exception: INTERNAL with the safe detail by default, logged with its traceback. Mapping grpc.RpcError in error_status_map picks another status. Fixes #48
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.
Summary
AsyncExceptionHandlerInterceptorre-raised everygrpc.RpcErroruntouched.grpc.aio.AbortErroris not anRpcError(AbortError -> BaseError -> Exception), and the onlyRpcErrorsubclass grpcio defines is the client-sidegrpc.aio.AioRpcError. So the clause never passed through a status this RPC chose; it only let through failures of calls the handler made to other services, and grpc.aio answered the caller withUNKNOWNand the downstream error'srepr(status, details, debug string) as details.The clause is gone; the
AbortErrorone stays. A failed outbound call now goes the same way as any other exception:find_mapped_status:INTERNALwith the default map, sinceRpcErroris not in it;detail_factory: "Internal server error" by default;logger.exceptionwith the traceback, so the downstream status and details are still in the log.Because it goes through the map,
{grpc.RpcError: grpc.StatusCode.UNAVAILABLE}inerror_status_mapreports every downstream failure asUNAVAILABLE(or whatever the service wants). No new option.The issue's script before and after:
Considered and rejected
INVALID_ARGUMENTorUNAUTHENTICATEDmeans this server sent a bad request or holds bad credentials, and passing it on blames the caller.INTERNALis the default; a service that knows a particular downstream code is the caller's business catches theAioRpcErrorin the handler and aborts with it, and the guide says so.AsyncSentryInterceptor, which also skipsgrpc.RpcError. Capturing every downstream outage as a new event is a separate decision. The failure is already logged at ERROR with its traceback by the exception handler. Left as it is; the observability guide now says where such failures go.Compatibility
This changes what callers receive, but only in one case: a response that was
UNKNOWNwith the downstream repr becomesINTERNALwith the safe detail. The old status carried no information (grpc.aio always answeredUNKNOWNfor these), so nothing could branch on it; what it did carry was the downstream details. Released as afix:, not a breaking change.Interceptors outside the handler (metrics, request logger, tracing) now record
INTERNAL, which is what the caller actually gets. Before, they recorded the downstream code fromexc.code()while the caller gotUNKNOWN. Tracing still records the originalAioRpcErroron the span through theAbortError's__context__.A custom
detail_factorynow also receivesAioRpcErrorinstances. It already had to handle arbitrary exceptions (any unmapped type reaches it), so a factory that echoesstr(exc)leaks here exactly as it leaks for any other exception.servicewright
servicewright#86 is the same bug in
UnhandledErrorInterceptor, which is its own implementation on top of this kit'sAsyncServerInterceptorbase, not onAsyncExceptionHandlerInterceptor. Both now behave the same way: an outboundRpcErroris logged with its traceback and answered with a maskedINTERNAL, a deliberateAbortErrorpasses through. The wording on the wire stays each library's own (Internal server errorhere;internal_errorplusx-error-codein servicewright, which has no counterpart in this kit).Tests
AioRpcError(downstreamINTERNALandPERMISSION_DENIED) aborts withINTERNAL/ "Internal server error" and logs the original exception with its traceback;grpc.RpcErrorinerror_status_mappicks the status.rpc_erroris dropped from the pass-through parametrization.AbortDirectly(PERMISSION_DENIED, "handler-abort"); its caller getsINTERNAL/ "Internal server error", and its metrics recordINTERNAL.exception_handler.pyput back, all four new tests fail (the integration one withUNKNOWN != INTERNAL).ruff check,ruff format --check,mypy grpc_server_kit: clean.pytest --cov: 441 passed, 97.60% coverage.Type of change
Checklist
make checkpasses locally (ruff+mypy)docs/agents.mdincludedRelated issues
Fixes #48