Stop spurious Failed to detach context errors from the OpenTelemetry interceptors - #1911
Conversation
The tracing interceptor's best-effort detach checked that the attached Context was still current, so a generator finalized by GC on another thread would skip the detach instead of using a token that is invalid there. OpenTelemetry's threading instrumentation, which strands enables whenever an Agent is created, copies the same Context object into new threads, so that check passes on the wrong thread and detach logs "Failed to detach context". Record the attaching thread with the token and skip the detach anywhere else. The regression test runs the existing safe-detach scenario with the instrumentation active; it fails without this change.
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
The plugin interceptor remains vulnerable, and recyclable thread identifiers do not reliably establish thread identity.
Review effort: Balanced
Findings: 1
What changed in this PR
Adds thread-aware OpenTelemetry context detachment to prevent foreign-thread token errors.
Changes:
- Introduces shared context attach/detach helpers.
- Adds threading-instrumentation regression coverage.
- Documents the fix.
| File | Description |
|---|---|
temporalio/contrib/opentelemetry/_interceptor.py |
Adds thread-aware detachment. |
tests/contrib/opentelemetry/test_opentelemetry.py |
Adds regression coverage. |
CHANGELOG.md |
Records the fix. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
OpenTelemetryPlugin installs OpenTelemetryInterceptor, whose four attach/detach sites had the same context-identity guard. Move the helpers into _context.py and use them from both interceptors. Compare the attaching threading.Thread object rather than its id, which the OS can reuse. The regression test is now a matrix over interceptor and threading instrumentation; the OpenTelemetryInterceptor case fails without this.
brianstrauch
left a comment
brianstrauch
left a comment
There was a problem hiding this comment.
Requesting changes for the P2 below: the thread identity guard skips valid context cleanup when workflow activations move between executor threads. All 80 existing OpenTelemetry tests passed, but a targeted workflow reproduction fails for both interceptors with the PR guard and passes with the original guard.
Workflow activations run on a thread pool while the asyncio task keeps its contextvars.Context, so a context attached in one activation is legitimately detached in a later one on another thread. The thread check skipped those detaches and left the inner context attached for outer interceptors. Let contextvars decide instead: perform the reset that opentelemetry.context.detach performs and treat its ValueError for a foreign Context as nothing to detach. Covers the thread change with a unit test on both interceptors and a worker test whose executor alternates activations between two threads.
DABH
changed the title
mypy rejects assigning to a method (method-assign); pytest's monkeypatch does the same thing and restores it on teardown.
|
|
||
|
|
||
| @dataclass(frozen=True) | ||
| class AttachedContext: |
There was a problem hiding this comment.
This seems like a context manager.
|
I don't totally follow. The description refers to "since" a PR that was never completed. And the title is incorrect, it is still detaching, just catching a theoretical failure to suppress a log. How is this doing anything about any test failures? If the goal is to not have that log statement, that seems to be the only thing it is accomplishing. Edit: I see, the test which is checking for the log. |
attached_context(context) replaces the seven attach/try/finally/detach sites; the guarded detach moves to _detach, which the leak test counts.
DABH
changed the title
Failed to detach context errors from the OpenTelemetry interceptors
The test patched opentelemetry.context.attach/detach process-wide, which also counts the threading instrumentation's attach/detach around every thread's run(). A pool thread started before the patch and exiting during the test adds a detach with no counted attach (9 attaches vs 10 detaches on 3.10 macos-arm). Record the interceptors' own detach results instead.
What was changed
TracingInterceptorandOpenTelemetryInterceptor(the oneOpenTelemetryPlugininstalls) attach an OpenTelemetry context around workflow, query, update, activity, and Nexus handler code and detach it in afinally. "Context" means two different things in that sentence:Context: an immutable value holding the current span and baggage;contextvars.Context: the runtime container a thread or asyncio task runs in, which holds the current OpenTelemetryContextin aContextVar.opentelemetry.context.attachsets thatContextVarin the current container and returns a token;detachresets it with the token, which is only valid inside the container that created it. If thefinallyruns in another container,detachcatches theValueErrorand logsFailed to detach contextat ERROR with a traceback. Nothing is wrong at that point: there is nothing to detach there. That happens when the context manager is finalized by garbage collection on another thread (an abandoned workflow coroutine, for example). #1174 added a guard for it, detach only if the attachedContextvalue is still the current one, withtest_opentelemetry_safe_detachasserting the log is gone. #1596 proposed a second guard for a copied container and was closed.The guard compares the value, but validity depends on the container, and the two can disagree. OpenTelemetry's threading instrumentation, which strands enables for every
Agent, propagates the sameContextvalue into every thread started while it is current, so on such a thread the guard passes and the invalid detach is attempted.test_opentelemetry_safe_detachfails deterministically whenever a strands test ran earlier in the same xdist worker (10 of 13 macos-arm attempts on #1868), and strands +OpenTelemetryPluginusers get the same error in their logs. Since #1868 merged, both macos-arm lanes onmainfail that test on every run (for example run 36895669970), together withtest_opentelemetry_context_restored_after_activity, which counts process-global attach/detach calls and so also sees the instrumentation's per-thread pair.The thread is not a usable test either (Brian's review): activations move between pool threads while the asyncio task keeps its container, and those detaches must happen. Only
contextvarsknows which container a token belongs to. Soattached_context, a context manager replacing the seven try/finally sites, keeps the value check and then performs the sameContextVar.resetthatopentelemetry.context.detachperforms, ignoring theValueErrorfor a token from another container. Valid detaches are unchanged. The only behavior change is that the spurious error is no longer logged.Testing
test_opentelemetry_safe_detach: interceptor ×ThreadingInstrumentormatrix; the instrumented cases fail onmain.test_opentelemetry_detach_after_thread_changeandtest_opentelemetry_context_restored_after_activation_thread_change: enter on one thread, leave on another, both interceptors; fail with a thread-based guard.test_opentelemetry_context_restored_after_activitynow records the interceptors' own detach results instead of patchingopentelemetry.context.attach/detachprocess-wide, so a thread started before the test and exiting during it no longer skews the count.tests/contrib/opentelemetry84 passed;poe lintclean.