fix(tracing): catch only the context reset on generator close - #4232
Conversation
_finish_on_generator_exit wrapped the whole finish() call, but TraceImpl.finish runs the processor's on_trace_end before resetting the scope. A processor that raised ValueError was therefore mistaken for a foreign context token: the error was swallowed, the saved token dropped, and the finished trace left current for everything that ran after the close. Run finish(reset_current=False) so processor failures surface, then reset the scope separately and tolerate only that failure, which is the one that genuinely cannot succeed when another task finalizes the generator.
seratch
left a comment
There was a problem hiding this comment.
Please run the reset cleanup in a finally block so that a finish() failure does not leave the finished trace current in the same task. Also clear _prev_context_token only after the reset succeeds or its expected foreign-context ValueError has been handled; clearing it before the reset loses the ownership handle if any unexpected reset error occurs. Update the regression to assert that the original failure propagates while Scope.get_current_trace() is restored and the token is cleared.
This keeps the cross-task behavior from #4221 while preserving cleanup ownership on failure. After that focused change and green CI, this should be ready for another review.
A failing finish() skipped the reset entirely, so a processor error left the finished trace current in the same task. Run the reset in a finally so the scope is released either way, and clear _prev_context_token only once the reset has succeeded or hit the foreign-context ValueError it expects, so an unexpected reset failure keeps the ownership handle.
|
All three points done in b15119f. You are right that my version traded one leak for another: a failing try:
trace.finish(reset_current=False)
finally:
token = trace._prev_context_token
if token is not None:
try:
Scope.reset_current_trace(token)
except ValueError:
logger.debug("Skipping trace context reset, token belongs to another context")
trace._prev_context_token = NoneThe The regression now asserts what you asked for. It still expects the processor's which is exactly the finished trace being left current. Verification: Cross-task behavior from #4221 is unchanged, and I confirmed this does not overlap #4233, which touches |
Follow-up to #4221, requested by @seratch there.
#4221 moved the foreign-token tolerance into
_finish_on_generator_exit, but that helper wraps the wholefinish()call.TraceImpl.finishruns the processor before resetting the scope:So a processor whose
on_trace_endraisesValueErroris mistaken for a foreign context token. The error is swallowed, the saved token is dropped, and the reset never runs, which leaves the finished trace current for everything that runs after the close. Closing from the same task returns normally while the scope is quietly wrong.The change
Run
finish(reset_current=False)so processor failures propagate normally, then reset the scope separately and tolerate only that failure, which is the one that genuinely cannot succeed when another task finalizes the generator:Verified
test_generator_close_surfaces_processor_failurefails on current main withDID NOT RAISE <class 'ValueError'>and passes here. It also asserts the token is left in place rather than silently dropped, since the reset never ran.tests/tracingis 49 passed, withruff check,ruff format --check, andmypyclean on both changed files.