util-genai: record cancellation as a failure, not a success - #520
Conversation
There was a problem hiding this comment.
Pull request overview
This PR fixes util-genai’s invocation context-manager finalization so cancellations and other BaseException cases are recorded as failures (via fail()), preventing cancelled operations from being exported indistinguishably from successful ones.
Changes:
- Update
GenAIInvocation.__exit__to treat any non-Noneexc_value: BaseExceptionas failure, aligning behavior with existing stream wrapper guards. - Add new unit tests covering
CancelledError,KeyboardInterrupt,SystemExit,GeneratorExit, and ensuring cancellation is not suppressed. - Add a towncrier changelog fragment describing the bug fix.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| util/opentelemetry-util-genai/src/opentelemetry/util/genai/_invocation.py | Treat BaseException in __exit__ as failure to correctly mark cancellation/interruption as errors. |
| util/opentelemetry-util-genai/tests/test_invocation_base_exception.py | New tests verifying __exit__ failure semantics for BaseException and that cancellation still propagates. |
| util/opentelemetry-util-genai/.changelog/PRNUMBER.fixed | Changelog fragment documenting the cancellation-as-failure fix. |
Suppressed comments (2)
util/opentelemetry-util-genai/tests/test_invocation_base_exception.py:162
test_cancellation_of_a_task_still_cancels_itcan hang for up to 60s (or indefinitely onstarted.wait()) if cancellation doesn’t propagate as expected. Adding a shortasyncio.wait_fortimeout makes the test fail fast instead of stalling the suite.
task = asyncio.ensure_future(work())
await started.wait()
task.cancel()
with pytest.raises(asyncio.CancelledError):
await task
util/opentelemetry-util-genai/tests/test_invocation_base_exception.py:119
- This assertion hardcodes the
CancelledErrormodule path (asyncio.exceptions...), which can change between Python versions. Compute the expectederror.typeusingfq_exception_typeto avoid version-specific failures.
self.assertEqual(
span.attributes["error.type"], "asyncio.exceptions.CancelledError"
)
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| task = asyncio.ensure_future(work()) | ||
| await started.wait() | ||
| task.cancel() | ||
| with pytest.raises(asyncio.CancelledError): | ||
| await task |
There was a problem hiding this comment.
Good catch, applied. Both scenarios now bound the startup wait and the awaited
task with asyncio.wait_for on a shared 5s constant, at both sites.
The assertions are unchanged. I checked that wait_for is transparent here: when
the task is cancelled externally it still propagates CancelledError and
task.cancelled() is still true, on 3.10 through 3.14. I also checked the guard
can actually fire. With task.cancel() removed, the two tests fail in about 10s
total instead of stalling for two minutes.
| self.assertEqual( | ||
| self._only_span().attributes["error.type"], | ||
| "asyncio.exceptions.CancelledError", | ||
| ) |
There was a problem hiding this comment.
Declining this one.
Deriving the expected value with fq_exception_type, the same helper the code
under test uses to produce error.type, would make the assertion tautological. It
would pass even if fq_exception_type returned the wrong string, which is
precisely what the test exists to pin. The literal is the oracle.
On brittleness: I ran the string myself on CPython 3.10.21, 3.11.16, 3.12.2,
3.13.15 and 3.14.7, which cover every version in the package's classifiers. It is
asyncio.exceptions.CancelledError on all five. If it ever does change, this
assertion failing is the correct outcome, since error.type is an exported
attribute value and a change there is a change users would see.
| @@ -0,0 +1 @@ | |||
| Record cancellation as a failure: the invocation context manager finalized any non-`Exception` `BaseException` (`asyncio.CancelledError`, `KeyboardInterrupt`, `SystemExit`, `GeneratorExit`) through the success path, so cancelled operations were exported with an unset span status and no `error.type`. | |||
There was a problem hiding this comment.
Already fixed. The fragment is .changelog/520.fixed on the current head; this
review ran against the first push.
02d832e to
009e789
Compare
Pull request dashboard statusWaiting on maintainers · refreshed 2026-09-02 14:52 UTC Merge when ready. Status above doesn't look right?
|
| # Guard on BaseException, not Exception: asyncio.CancelledError, | ||
| # KeyboardInterrupt, SystemExit and GeneratorExit do not derive from | ||
| # Exception, and treating them as success reports a cancelled or | ||
| # interrupted operation as a completed one. Matches the guard already | ||
| # used by the stream wrappers in ``stream.py``. | ||
| # | ||
| # Returning None (never a truthy value) is load-bearing: suppressing | ||
| # CancelledError here would break asyncio cancellation for every caller. |
There was a problem hiding this comment.
| # Guard on BaseException, not Exception: asyncio.CancelledError, | |
| # KeyboardInterrupt, SystemExit and GeneratorExit do not derive from | |
| # Exception, and treating them as success reports a cancelled or | |
| # interrupted operation as a completed one. Matches the guard already | |
| # used by the stream wrappers in ``stream.py``. | |
| # | |
| # Returning None (never a truthy value) is load-bearing: suppressing | |
| # CancelledError here would break asyncio cancellation for every caller. |
I think this code is self-explanatory
There was a problem hiding this comment.
Agreed, applied. Comment block removed.
| @@ -0,0 +1,187 @@ | |||
| # Copyright The OpenTelemetry Authors | |||
There was a problem hiding this comment.
can you please write this test against the actual langgraph public API. I think it's an existing pattern, but this is very synthetic, does not show how it'd work in real life and we're trying to stop using it.
There was a problem hiding this comment.
Happy to. I went to write it and hit something I wanted to check with you first.
langgraph is instrumented through the langchain package, and that instrumentation
is callback-based: on_chain_end calls invocation.stop() and on_chain_error
calls invocation.fail(error) directly. It never uses the invocation as a context
manager, so a langgraph-driven test doesn't reach GenAIInvocation.__exit__, and
it passes the same with and without this fix. on_chain_error is already typed to
take a BaseException and hands it straight to fail().
The place a real-API cancellation test would land in __exit__ is agno:
_tool_call_aexecute awaits inside with _start_tool_invocation(...). Happy to
open that as a separate PR against agno.
Want me to add the langgraph test anyway, or go the agno route?
`GenAIInvocation.__exit__` finalized the invocation through `fail()` only when the in-flight exception derived from `Exception`. `asyncio.CancelledError`, `KeyboardInterrupt`, `SystemExit` and `GeneratorExit` derive from `BaseException` but not from `Exception`, so they took the `stop()` branch: a cancelled operation was exported with `StatusCode.UNSET`, no `error.type`, and a duration metric carrying no error dimension. Guard on `BaseException` instead, matching the guard the stream wrappers in `stream.py` already use (`if exc_val is not None:`). `fail()` already accepts a `BaseException` and routes it through `Error.from_exception`, so no other change is needed; `error.type` resolves via `fq_exception_type` and stays consistent with the exception event. `__exit__` still returns `None`, so nothing is suppressed: suppressing `CancelledError` would break asyncio cancellation for every caller. Tests cover that explicitly, including a real cancelled task that must remain cancelled. Assisted-by: Claude Opus 5
009e789 to
39fbbab
Compare
Description
GenAIInvocation.__exit__calledfail()only when the in-flight exception derived fromException, soasyncio.CancelledErrortook thestop()branch: a cancelled operation was exportedwith
StatusCode.UNSET, noerror.type, and a duration metric with no error dimension —indistinguishable from one that completed. This guards on
BaseExceptioninstead, matching the guardthe stream wrappers in
stream.pyalready use.fail()already accepts aBaseException, so nothing else changed.__exit__still returnsNone— suppressing
CancelledErrorwould break asyncio cancellation for every caller — and two testscover that, one of them cancelling a real task mid-
await.Known gaps
KeyboardInterrupt,SystemExitandGeneratorExit.GeneratorExitis not reachable from any shipped instrumentation today — no
with <invocation>:block in thisrepository contains a
yield— but it is covered by a test. Happy to narrow this; it is one line.except Exception:handler are worse off than unfixed, not equal: on cancellation neither branch runs and the span
is never ended, so nothing is exported at all. Happy to follow up separately.
Type of change
How has this been tested?
New tests in
util/opentelemetry-util-genai/tests/test_invocation_base_exception.py, written beforethe fix and confirmed red against unmodified
main: 6 failed, 342 passed — five withAssertionError: <StatusCode.UNSET: 0> != <StatusCode.ERROR: 2>, one withKeyError: 'error.type'.The file's other tests passed before and after, so it discriminates rather than failing wholesale.
tox -e py310-test-util-genai— 348 passedtox -e py314-test-util-genai— 348 passedtox -e py310-test-instrumentation-genai-agno-oldest— 20 passedtox -e py310-test-instrumentation-genai-agno-latest— 20 passedtox -e py314-test-instrumentation-genai-agno-latest— 20 passedgoogle-genaiandsmolagentsoldest / latest — passedweaverbinary is notinstalled, so no assertion ran. Relying on CI for those.
Checklist