Skip to content

fix(tracing): make no-op starts idempotent - #4501

Closed
sylvesterkaczmarek wants to merge 3 commits into
openai:mainfrom
sylvesterkaczmarek:fix/noop-tracing-start-idempotent
Closed

fix(tracing): make no-op starts idempotent#4501
sylvesterkaczmarek wants to merge 3 commits into
openai:mainfrom
sylvesterkaczmarek:fix/noop-tracing-start-idempotent

Conversation

@sylvesterkaczmarek

Copy link
Copy Markdown
Contributor

Summary

  • make repeated NoOpTrace.start() calls idempotent
  • make repeated NoOpSpan.start() calls idempotent
  • prevent a second mark_as_current=True start from overwriting the original ContextVar token

Normal trace and span implementations already ignore repeated starts. The no-op implementations did not, so a second start replaced the saved token with one whose previous value was the no-op object itself. A single finish(reset_current=True) then restored that object as current instead of clearing it, leaking tracing context when tracing is disabled.

Test plan

  • added focused regression coverage in tests/test_noop_tracing_start_idempotent.py
  • GitHub Actions

Issue number

N/A

@sylvesterkaczmarek
sylvesterkaczmarek marked this pull request as ready for review August 17, 2026 21:35
@seratch

seratch commented Aug 17, 2026

Copy link
Copy Markdown
Member

I confirmed that explicitly starting the same no-op trace or span twice overwrites its saved context token and leaves the no-op object current after one finish. However, the documented lifecycle is either context-manager use or one balanced manual start()/finish() pair, and I could not find an SDK path that starts the same no-op object twice.

Please provide a concrete supported scenario that reaches this repeated-start sequence and shows the resulting context leak affecting later work. Without such a scenario, this currently protects only an unsupported lifecycle composition, so I would close this PR for now.

@seratch seratch closed this Aug 17, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants