fix(tracing): skip NoOpTrace context reset on GeneratorExit#3225
fix(tracing): skip NoOpTrace context reset on GeneratorExit#3225adityasingh2400 wants to merge 3 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.
c00dc93 to
efa4c5a
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 84e7e07717
ℹ️ 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".
| # the trace was entered in another task and the generator is | ||
| # closing from the parent's context). Skipping reset here is | ||
| # safe: that other Context owns its own contextvar copy. | ||
| pass |
There was a problem hiding this comment.
When an async generator enters a disabled trace in one task and a child task created from that context calls aclose(), reset_current_trace() raises ValueError because the token belongs to the opener's Context, but the closer task's copied Context can still have this same NoOpTrace as the current trace. This new pass clears only the saved token, so the closer task continues with a no-op current trace; subsequent Runner.run/create_trace_for_run in that task sees an active trace and skips creating a real one. Please restore or clear the current Context in this branch when it still points at this trace, instead of only swallowing the error.
Useful? React with 👍 / 👎.
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.