dheerajturaga commented on code in PR #68039:
URL: https://github.com/apache/airflow/pull/68039#discussion_r3360020080
##########
airflow-core/src/airflow/api_fastapi/execution_api/app.py:
##########
@@ -253,9 +252,16 @@ async def _extract_w3c_trace_context(
token = otel_context.attach(ctx)
try:
yield
- finally:
- with contextlib.suppress(Exception):
- otel_context.detach(token)
+ except GeneratorExit:
+ # The dependency generator was force-closed (client disconnect /
request
+ # cancellation): the finalizer runs in a different asyncio Task — and
thus a
+ # different contextvars.Context — than attach did, so detaching the
token would
+ # raise "Token was created in a different Context" (which OTel logs at
ERROR
+ # before any suppression here could see it). The attached Context is
being
+ # discarded with the dying task, so detaching is unnecessary; skip it
and re-raise.
+ raise
+ else:
+ otel_context.detach(token)
Review Comment:
Great catch — you're right, and it was a regression. FastAPI finalizes this
dependency by `athrow`-ing the route exception into the generator at the
`yield` (confirmed via `_solve_generator` -> `asynccontextmanager` ->
`AsyncExitStack`), which took neither branch, so detach was skipped on a
same-task path where it both should and would have succeeded — leaking the
upstream W3C context into the exception handler / 500 response / error-path
spans.
Fixed by adding an `except BaseException:` arm (ordered after `except
GeneratorExit:`, since `GeneratorExit` is a `BaseException`) that detaches and
re-raises. The detach there is wrapped in `contextlib.suppress(Exception)` so a
detach failure can't mask the original route exception (preserving the
`test_route_exception_not_masked_by_detach_error` invariant). Only the
cross-task `GeneratorExit` force-close skips detach now.
---
Drafted-by: Claude Code (Opus 4.8) (reviewed by @dheerajturaga before
posting)
##########
airflow-core/src/airflow/api_fastapi/execution_api/app.py:
##########
@@ -253,9 +252,16 @@ async def _extract_w3c_trace_context(
token = otel_context.attach(ctx)
try:
yield
- finally:
- with contextlib.suppress(Exception):
- otel_context.detach(token)
+ except GeneratorExit:
+ # The dependency generator was force-closed (client disconnect /
request
+ # cancellation): the finalizer runs in a different asyncio Task — and
thus a
+ # different contextvars.Context — than attach did, so detaching the
token would
+ # raise "Token was created in a different Context" (which OTel logs at
ERROR
+ # before any suppression here could see it). The attached Context is
being
+ # discarded with the dying task, so detaching is unnecessary; skip it
and re-raise.
+ raise
+ else:
+ otel_context.detach(token)
Review Comment:
Great catch — you're right, and it was a regression. FastAPI finalizes this
dependency by `athrow`-ing the route exception into the generator at the
`yield` (confirmed via `_solve_generator` -> `asynccontextmanager` ->
`AsyncExitStack`), which took neither branch, so detach was skipped on a
same-task path where it both should and would have succeeded — leaking the
upstream W3C context into the exception handler / 500 response / error-path
spans.
Fixed by adding an `except BaseException:` arm (ordered after `except
GeneratorExit:`, since `GeneratorExit` is a `BaseException`) that detaches and
re-raises. The detach there is wrapped in `contextlib.suppress(Exception)` so a
detach failure can't mask the original route exception (preserving the
`test_route_exception_not_masked_by_detach_error` invariant). Only the
cross-task `GeneratorExit` force-close skips detach now.
---
Drafted-by: Claude Code (Opus 4.8) (reviewed by @dheerajturaga)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
