telemetry: fix spurious 'Failed to detach context' error on shutdown - #77
telemetry: fix spurious 'Failed to detach context' error on shutdown#77andre-merzky wants to merge 2 commits into
Conversation
ContextVar tokens can only be reset from the contextvars.Context (i.e. the asyncio task) that created them. When TelemetryManager.start() and stop() run in different tasks -- the normal case under the ORBIT rhapsody plugin, where session init and close are separate requests -- the detach in stop() raised ValueError inside opentelemetry.context.detach(), which OTel catches and logs as 'ERROR: Failed to detach context' on every otherwise-clean shutdown. Record the attaching task in start() and detach only when stop() runs in that same task. Skipping the detach is safe: the attach only ever affected the starting task's context, and the tracer provider is shut down immediately afterwards. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request updates the telemetry manager to track the asyncio task that attached the OpenTelemetry session context token, ensuring that the context is only detached if stop() is called from the same task that executed start(). This prevents spurious ValueError logs at shutdown. The review feedback suggests using a weak reference (weakref.ref) to store the task instead of a strong reference to avoid potential memory leaks, along with the corresponding dereferencing logic during the identity check.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| self._session_ctx_token = otel_context.attach( | ||
| trace_mod.set_span_in_context(self._session_span) | ||
| ) | ||
| self._session_ctx_task = asyncio.current_task() |
There was a problem hiding this comment.
Storing a strong reference to an asyncio.Task object can prevent the task, its coroutine, and all local variables in its call stack from being garbage collected, potentially causing a memory leak in long-running applications. Using a weak reference instead avoids this issue while still allowing identity comparison.
| self._session_ctx_task = asyncio.current_task() | |
| import weakref | |
| _task = asyncio.current_task() | |
| self._session_ctx_task = weakref.ref(_task) if _task is not None else None |
There was a problem hiding this comment.
Done in 9161618 — task is now held as a weakref.ref, with the import at module level.
| if asyncio.current_task() is self._session_ctx_task: | ||
| otel_context.detach(self._session_ctx_token) |
There was a problem hiding this comment.
Dereference the weak reference to the task before performing the identity check. If the task has already been garbage collected, the weak reference will resolve to None, and the detach step will be safely skipped.
| if asyncio.current_task() is self._session_ctx_task: | |
| otel_context.detach(self._session_ctx_token) | |
| _attached_task = self._session_ctx_task() if self._session_ctx_task is not None else None | |
| if _attached_task is not None and asyncio.current_task() is _attached_task: | |
| otel_context.detach(self._session_ctx_token) |
There was a problem hiding this comment.
Done in 9161618 — the weakref is dereferenced before the identity check; a collected task resolves to None and the detach is skipped.
A strong reference would pin the starting task's coroutine frame and locals for the entire session lifetime; the weakref keeps the identity check and resolves to None once the task is gone, in which case the detach is skipped just like any other cross-task stop(). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request updates the telemetry manager to avoid spurious OpenTelemetry errors during shutdown. It introduces a weak reference to the asyncio task that starts the telemetry session, ensuring that detaching the OpenTelemetry context token in stop() is only attempted if it is executed within the same task that attached it. I have no feedback to provide as there are no review comments.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
1 participant
Summary
TelemetryManager.stop()logged a spurious OpenTelemetry error on every otherwise-clean shutdown whenever it ran in a different asyncio task thanstart():ContextVar tokens can only be reset from the contextvars.Context (i.e. the asyncio task) that created them. Under the ORBIT rhapsody plugin, session init and close are separate requests handled by separate tasks, so the cross-task detach failed every time. The
ValueErroris caught insideopentelemetry.context.detach()and logged as an ERROR there, so a try/except at the call site cannot silence it — the detach must be skipped instead.Fix
Record the attaching task in
start();stop()detaches only when it runs in that same task, and otherwise just drops the token. Skipping is safe: the attach only ever affected the starting task's context, and the tracer provider shuts down immediately afterwards.Verification
tests/unit/telemetry: 90 passed with opentelemetry installed (1 failure intest_emits_resource_updateis pre-existing — fails identically on unpatchedmainin this environment).main(remaining failures/errors are missing-dask / Dragon-runtime environment issues, verified pre-existing).🤖 Generated with Claude Code