fix: an unhandled exception over gRPC is masked instead of sent as UNKNOWN plus its repr - #52
Merged
Merged
Conversation
added 2 commits
September 7, 2026 15:17
…KNOWN plus its repr ServiceErrorInterceptor claimed ServiceError and everything else fell through to grpc.aio, whose answer for an exception it was never told about is UNKNOWN with repr() of it. The HTTP side masks the same exception to a 500 with code=internal_error and keeps the message in the log, so the promise of the shared error model held for the errors a service declared and broke for the ones it did not — and those are the ones whose wording nobody reviewed. In the service the report came from, that text carried a DSN with a password. UnhandledErrorInterceptor is the gRPC counterpart of the HTTP stack's UnhandledErrorMiddleware and sits at the same depth: inside UnitScopeInterceptor so the traceback is logged with the RPC's correlation ids, and outside the metrics interceptor and the caller's own, so an exception type you map yourself still reaches your interceptor first. It aborts with INTERNAL, detail internal_error and x-error-code: internal_error, which is byte for byte what a public=False ServiceError already produced, and re-raises AbortError and RpcError — a status somebody chose deliberately. CancelledError is a BaseException and never reaches it. It is installed unconditionally, exactly as its HTTP counterpart is. map_service_errors=False still turns off only the mapping of declared errors; an unmapped ServiceError then arrives as a masked INTERNAL rather than as its own message. ErrorKind.CONFLICT keeps mapping to ALREADY_EXISTS. ABORTED also renders as 409 but tells clients to retry at a higher level, which a state conflict will not survive, and FAILED_PRECONDITION is spoken for by PRECONDITION_FAILED; the reason now sits next to the table instead of nowhere. Refs #50
…nsports The errors page said public=False masks at every transport and left the other half unsaid, so the pages a reader lands on described the gap as if it were the design. They now carry the masked-by-default table for an undeclared exception, the note that neither default_exception_handlers=False nor map_service_errors=False removes that layer, and the reason CONFLICT maps to ALREADY_EXISTS rather than to ABORTED. The gRPC adapter page gains the section on anything that is not a ServiceError and the interceptor chain grows its new row, as do the blueprint's two renderings of it. The runbook entry for a domain error becoming INTERNAL says what map_service_errors=False now leaves in place, the checklist gains the line about catching Exception and returning its str(), and agents.md carries the new name, the chain and a widened rule 16. Refs #50
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
An exception that is not a
ServiceErrorreached a gRPC caller with its own message. Over HTTP thesame exception is a masked 500; over gRPC it fell through to
grpc.aio, whose answer for anexception it was never told about is
UNKNOWNwithrepr()of it. The row from the issue's lab,before and after, same call over both transports:
The
unexpectedrow is now identical to theledgerrow, which is aServiceErrorwithpublic=False. That is the property worth having: a caller cannot tell an error the service hidfrom an error it never knew about, and neither carries a sentence nobody wrote. In the service the
lab is modelled on, the exception text held a DSN with a password.
The design
UnhandledErrorInterceptoris the gRPC counterpart of the HTTP stack'sUnhandledErrorMiddleware,and it sits at the same depth of the chain:
It aborts with
INTERNAL, detailinternal_errorandx-error-code: internal_error— byte forbyte what a
public=FalseServiceErroralready produced — and logs the exception with itstraceback.
grpc.aio.AbortErrorandgrpc.RpcErrorare re-raised, because a handler that abortedalready chose its status, and
asyncio.CancelledErroris aBaseException, so a caller walkingaway never reaches it.
Installed unconditionally, with no new keyword argument, exactly as
UnhandledErrorMiddlewareisover HTTP: not leaking is not a preference.
map_service_errors=Falsekeeps meaning what it meant —it turns off the mapping of the errors you declared — with the consequence, now documented, that an
unmapped
ServiceErrorarrives as a maskedINTERNALrather than as its own message.What I rejected
Folding the catch-all into
ServiceErrorInterceptor. Shortest diff, wrong position.That interceptor is innermost by design, so it would take every exception before the caller's own
interceptors could map theirs. Starlette dispatches handlers by type, so on HTTP a handler for your
exception type always wins over the
Exceptioncatch-all; a separate outer layer is what reproducesthat ordering here. It also covers an exception raised by an interceptor rather than by a servicer.
Installing grpc-server-kit's
AsyncExceptionHandlerInterceptorby default. It maps unknownexceptions to
INTERNAL, but it also mapsValueErrortoINVALID_ARGUMENT,TimeoutErrortoDEADLINE_EXCEEDEDandFileNotFoundErrortoNOT_FOUND, where the HTTP side masks all three to500. That would close the leak and re-open the disagreement somewhere else, and it sends no
x-error-code. Because it composes inside the new layer, using it deliberately still worksexactly as before.
ErrorKind.CONFLICTstays onALREADY_EXISTSThe issue asked whether it should. It should.
ABORTEDalso renders as 409, but it is thetransaction-conflict code and the gRPC contract tells clients to retry it at a higher level — wrong
advice for "this order was already paid", which will never succeed on a retry.
FAILED_PRECONDITIONis spoken for byPRECONDITION_FAILED, and keeping the table injective iswhat lets a gateway translate in both directions. A client that must tell a duplicate key from a
state conflict branches on
x-error-code, which carries the exact code either way. Changing itwould be a wire change that buys nothing; the reasoning is now written next to the table instead of
nowhere.
Tests
tests/unit/test_error_parity.py— new. One property over both transports: apublic=FalseServiceErrorand an undeclaredRuntimeErroreach produce kindINTERNAL, codeinternal_errorand no trace of the secret, over HTTP and over gRPC; a publicNOT_FOUNDstillarrives intact on both. Four of its six cases pass on
master; the fifth is thenever-declaredgRPC one, which fails.
tests/unit/test_grpc.py— the interceptor in isolation: the masked abort, the traceback and themethod name on the log record, an unmapped
ServiceError,AbortError/RpcError/CancelledErrorpassing through, and its position in the chain both with and withoutmap_service_errors.tests/integration/test_grpc_integration.py— the reporter's call over a real socket: a servicerraising
RuntimeError, a real client,INTERNALandinternal_erroron the wire.Type of change
A caller that was branching on
UNKNOWNto detect a server crash now seesINTERNAL, which is thecorrection being made. Nothing else on the wire changes.
Checklist
make checkpasses locally (ruff+mypy)CHANGELOG.mdleft alone — Release Please generates it from the Conventional Commit subjects (and from the PR title on a squash merge)docs/agents.mdincludedRelated issues
Closes #50