fix(grpc): finish aio client spans on abandoned calls - #19716
Conversation
![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: 28383fe535
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| _handle_rpc_error(span, rpc_error) | ||
| raise | ||
| except asyncio.CancelledError: | ||
| span.finish() |
There was a problem hiding this comment.
When a unary-unary or stream-unary operation is cancelled, including the new asyncio.wait_for timeout case, this branch flushes the span without setting grpc.status.code, the error flag, or error details, so the failed RPC is reported as a successful span. Streaming cancellations already use _handle_cancelled_error and are asserted as StatusCode.CANCELLED with error == 1; apply equivalent cancellation metadata here before finishing.
Useful? React with 👍 / 👎.
| client_call_details, | ||
| ) | ||
| call = await continuation(client_call_details, request) | ||
| _handle_add_callback(call, _done_callback_stream(span)) |
There was a problem hiding this comment.
If the returned unary-stream or stream-stream wrapper is never iterated and the RPC terminates with cancellation, a deadline, or a server error, this newly registered callback reaches _done_callback_stream but returns for every non-OK code because it assumes _wrap_stream_response will run an awaited handler. With no consumer that handler never runs, leaving the span unfinished, so the abandoned-call fix currently covers only successful streams; the callback path needs completion ownership for non-OK abandoned calls too, and its associated AIDEV lifecycle note should be updated accordingly.
AGENTS.md reference: AGENTS.md:L67-L70
Useful? React with 👍 / 👎.
Signed-off-by: Liam Abourousse <75449798+Bardakor@users.noreply.github.com>
Signed-off-by: Liam Abourousse <75449798+Bardakor@users.noreply.github.com>
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a10ba9234b
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| # A stream may never be consumed, so there may be no iterator-side error handler to own | ||
| # finalization. Schedule an async finalizer that can read authoritative code/details. | ||
| # `_claim_stream_error` makes this race-safe with `_wrap_stream_response`: whichever path | ||
| # starts handling the terminal state first owns tagging and finishing the span. |
There was a problem hiding this comment.
This edit removes both pre-existing AIDEV-NOTE anchors documenting the gRPC terminal-state race and replaces them with ordinary comments. These anchors are protected and must be updated rather than removed when their associated lifecycle logic changes; restore the AIDEV-NOTE markers so future anchor scans continue to surface this concurrency invariant.
AGENTS.md reference: AGENTS.md:L67-L69
Useful? React with 👍 / 👎.
Signed-off-by: Liam Abourousse <75449798+Bardakor@users.noreply.github.com>
|
@codex review |
|
Codex Review: Didn't find any major issues. More of your lovely PRs please. Reviewed commit: ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback". |
emmettbutler
left a comment
emmettbutler
left a comment
There was a problem hiding this comment.
Deferring review until conversations are resolved
2 participants
Description
Fixes #19600.
gRPC asyncio client spans could remain unfinished along two paths: a unary call cancelled before its done callback was registered, and a streaming call whose lazy response wrapper was never iterated.
This change makes completion ownership explicit once an interceptor creates a span:
grpc.status.code, error type/details, and finishes the span before re-raisingCancelledError;continuationreturns the call, before returning the lazy wrapper;call.code()/call.details()are captured even when no consumer ever iterates the stream;Testing
Original validation before the review follow-up:
grpcio~=1.59.0: 58 passed, 13 skipped.grpcio~=1.59.0: 67 passed, 4 skipped.scripts/lint checksgit diff --checkReview follow-up adds focused regressions for:
The updated branch has not been re-run locally in the current environment; a fresh Codex review has been requested and upstream CI still requires maintainer approval.
Risks
Low to moderate. The callback path now schedules a small async finalizer for non-OK terminal streams. The ownership flag prevents that task from racing the iterator-side error handler into a double finish. Successful stream callbacks remain synchronous.
Additional Notes
A local attempt to run the oldest Python 3.9 /
grpcio==1.34.1environment did not reach pytest because grpcio's isolated source build failed while importingpkg_resources. No project configuration was changed to bypass that build failure.