kaxil commented on code in PR #68039:
URL: https://github.com/apache/airflow/pull/68039#discussion_r3366172419
##########
airflow-core/src/airflow/api_fastapi/execution_api/app.py:
##########
@@ -253,9 +253,25 @@ async def _extract_w3c_trace_context(
token = otel_context.attach(ctx)
try:
yield
- finally:
+ except GeneratorExit:
+ # Cross-task force-close (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
+ except BaseException:
+ # A route handler raised: FastAPI throws the exception into this
generator at
+ # the yield, in the SAME task that attach ran in. Detach so the
upstream trace
+ # context does not stay attached for the exception handler, the error
response,
+ # and any spans/logs emitted while unwinding. Suppress any detach
error so it
+ # cannot mask the original exception being propagated.
with contextlib.suppress(Exception):
otel_context.detach(token)
+ raise
+ else:
+ otel_context.detach(token)
Review Comment:
Two things, neither blocking.
**Optional: the task-identity premise your comments already state can drive
the control flow directly.** Both new comments establish the real invariant --
detach is only valid in the Context (i.e. the asyncio Task) that `attach` ran
in: route errors unwind "in the SAME task," disconnects finalize "in a
different asyncio Task." The three-branch `GeneratorExit` / `BaseException` /
`else` split enumerates exception types as a stand-in for that. You can test it
directly:
```python
token = otel_context.attach(ctx)
attached_in = asyncio.current_task()
try:
yield
finally:
if asyncio.current_task() is attached_in:
otel_context.detach(token)
```
Normal completion and route-error both unwind in the request task (detach
runs); disconnect/cancel finalizes in a different task (detach skipped, so OTel
never hits its `Failed to detach context` log). One branch instead of three,
keyed on the cause rather than on which exception FastAPI happens to throw. The
current version is correct either way.
**Could you post before/after logs from a reproduced disconnect?** The
spurious line is emitted inside OTel -- `detach()` wraps the reset in
`try/except Exception: logger.exception("Failed to detach context")`, which is
why the original caller-side `suppress` never silenced it. The new unit tests
assert detach isn't *called*, but they can't show the ERROR line is actually
gone from the log output. A quick before/after from a real disconnect would
confirm the user-visible symptom is fixed end to end.
--
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]
