fix(tracing): skip NoOpTrace context reset on GeneratorExit#3225
fix(tracing): skip NoOpTrace context reset on GeneratorExit#3225adityasingh2400 wants to merge 2 commits into
Conversation
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0693c700a2
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
|
||
| def __exit__(self, exc_type, exc_val, exc_tb): | ||
| self.finish(reset_current=True) | ||
| self.finish(reset_current=exc_type is not GeneratorExit) |
There was a problem hiding this comment.
When a generator using with trace(..., disabled=True) is closed normally in the same task, __exit__ receives GeneratorExit. This now skips Scope.reset_current_trace, leaving get_current_trace() as NoOpTrace after close. Since create_trace_for_run treats any current trace as active and spans under NoOpTrace become NoOpSpan, later tracing in that context can be silently suppressed.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in efa4c5ab: Try the context reset and swallow the cross-context ValueError so the trace is cleared on same-context close without inheriting NoOpTrace on later spans. Test covers the same-task generator close.
NoOpTrace.__exit__ unconditionally called finish(reset_current=True), unlike TraceImpl, ReattachedTrace, SpanImpl, and NoOpSpan which all skip the contextvars reset when exiting via GeneratorExit. When an async generator wrapped in a NoOpTrace is finalized by GC from a different task than the one that opened it, resetting a Token created in another Context raises ValueError. This aligns NoOpTrace with the other lifecycle types so disabled tracing behaves consistently with enabled tracing under generator-driven cleanup.
…alueError Skipping reset unconditionally on GeneratorExit handled the cross-task GC case but left NoOpTrace as the current trace after a normal same-context generator close, suppressing later tracing in that context. Always attempt the reset and swallow ValueError for the cross-Context case so both paths work correctly.
adityasingh2400
force-pushed
the
fix/tracing-noop-generator-exit
branch
from
c00dc93 to
efa4c5a
Compare
2 participants
Summary
NoOpTrace.__exit__unconditionally callsfinish(reset_current=True), unlike every other lifecycle type in this module:TraceImpl.__exit__(line 524):self.finish(reset_current=exc_type is not GeneratorExit)ReattachedTrace.__exit__(line 342):self.finish(reset_current=exc_type is not GeneratorExit)SpanImpl.__exit__(line 343):if exc_type is GeneratorExit: ... reset_current = FalseNoOpSpan.__exit__(line 234):if exc_type is GeneratorExit: ... reset_current = FalseWhen an async generator wrapped in a
NoOpTrace(i.e. tracing is disabled) is finalized by GC from a task other than the one that opened it, resetting thecontextvars.Tokenfrom a differentContextraisesValueError: <Token> was created in a different Context. The other lifecycle types already account for this;NoOpTraceshould match.The fix is a single-line change to follow the same pattern. Behavior under tracing-enabled paths is unchanged; behavior under tracing-disabled paths now matches tracing-enabled paths under
GeneratorExit.Test plan
NoOpTrace.__exit__(GeneratorExit, ...)skips the reset and preserves_prev_context_token.tests/tracing/(32) and trace processor / agent tracing tests (68) pass.