fix(tracing): release the span scope when a generator is closed - #4233
Merged
seratch merged 2 commits intoAug 6, 2026
Conversation
SpanImpl.__exit__ and NoOpSpan.__exit__ passed reset_current=False whenever the with block unwound on GeneratorExit, so closing a generator from the task that advanced it left the ended span current and nested every later span under it. The token saved by start is still valid on that path because the body resumes in the caller's own context, so the reset both can and should run. openai#4221 made the same correction for TraceImpl, ReattachedTrace, and NoOpTrace, and this brings the span level back in line with it by reusing that fix's shape: reset normally, and tolerate only the cross-context ValueError that an abandoned generator finalized from another task cannot avoid.
Wrapping the whole finish call in except ValueError would swallow a processor that raises ValueError from on_span_end, drop the saved token, and leave the finished span current for everything that ran afterwards. On main that failure propagates, because the GeneratorExit path passes reset_current=False and never guards the call, so catching it would be a new regression rather than a fix. Run finish without the reset, then guard only Scope.reset_current_span, which is the single call that can raise on a foreign token.
seratch
approved these changes
Aug 6, 2026
seratch
left a comment
Member
There was a problem hiding this comment.
Thanks, this addresses the concern. _finish_on_generator_exit now calls finish(reset_current=False) before handling the token and scopes the ValueError suppression to Scope.reset_current_span, so processor failures still propagate instead of being mistaken for foreign-context resets. The new regression exercises that exact path, while the existing tests retain same-task, cross-task, and explicit wrong-context coverage.
I do not see any further code changes needed. The latest CI run is still in progress, with seven checks passing and no failures at the time of review.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
SpanImpl.__exit__andNoOpSpan.__exit__passedreset_current=Falsewhenever thewithblock unwound onGeneratorExit, so closing a generator from the task that advanced it left the ended span current and nested every span opened afterwards under it. The token saved bystartis still valid on that path, because the generator body resumes in the caller's own context, so the reset both can and should run.#4221 made exactly this correction at the trace level for
TraceImpl,ReattachedTrace, andNoOpTrace, and the span level was left behind. This restores the pairing, with the tolerance narrowed to the reset itself:finishruns without the reset, and onlyScope.reset_current_spanis guarded, since that is the one call that can raise on a foreign token. Guarding the wholefinishwould swallow a processor that raisesValueErrorfromon_span_end, which propagates onmaintoday, so that would be a new regression rather than a fix.Fail-before evidence, in both directions. With
src/agents/tracing/spans.pyreverted tomainand the new tests kept,test_generator_close_in_the_same_task_releases_the_span_scopefails for both parameters withassert <agents.tracing.spans.SpanImpl object at 0x...> is None, giving 2 failed and 5 passed. With the helper written as a singletryaround the wholefinishcall instead,test_generator_close_surfaces_processor_failurefails withFailed: DID NOT RAISE <class 'ValueError'>. The committed form gives 7 passed, and the full suite plusmake lintandmake typecheckare green.