fix(tracing): close the default backend exporter on shutdown - #4712
fix(tracing): close the default backend exporter on shutdown#4712abhay-codes07 wants to merge 9 commits into
Conversation
The default BackendSpanExporter opens a pooled httpx client in its constructor and keeps it for the process lifetime, but nothing in the shutdown chain ever closed it. At interpreter teardown this left the TLS connection to api.openai.com open, emitting ResourceWarning under -W error::ResourceWarning / PYTHONDEVMODE=1 (which fails strict CI). Close the exporter at the setup boundary that owns it: the atexit handler _shutdown_global_trace_provider now closes the module-owned default exporter after the processors have drained, via a new _shutdown_default_exporter helper in the processors module. Only the exporter created by default_exporter/default_processor is closed, so an exporter injected into a caller-constructed BatchTraceProcessor is never closed on its behalf -- ownership is not inferred from the processor. The default singletons are cleared so a later trace rebuilds a fresh stack instead of reusing an exporter whose client is already closed.
![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: f91386c4de
ℹ️ 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".
seratch
left a comment
seratch
left a comment
There was a problem hiding this comment.
The cleanup is still attached only to the private atexit path.
A long-running application calling the public get_trace_provider().shutdown() still shuts down the processors without closing the module-owned default BackendSpanExporter HTTP client. Please attach default-exporter ownership to the public default-provider shutdown lifecycle, while keeping caller-injected exporters caller-owned.
Please also keep the global objects coherent when cleanup runs: clearing the processor/exporter singletons while GLOBAL_TRACE_PROVIDER still references the old processor creates two conflicting sources of truth. Add coverage for explicit public shutdown, repeated/idempotent shutdown, provider replacement, and preservation of injected exporters.
Address review feedback on the default-exporter close: - Attach the close to the public provider shutdown lifecycle instead of only the private atexit hook. BatchTraceProcessor gains an explicit _owns_exporter flag (set only by default_processor for the module-owned exporter); its shutdown() closes the exporter after the final drain. get_trace_provider().shutdown() -> MultiTracingProcessor.shutdown() -> BatchTraceProcessor.shutdown() now closes the client on both the public and atexit paths. - Keep caller-injected exporters caller-owned: a BatchTraceProcessor built by a caller has _owns_exporter=False and never closes its exporter. - Do not clear the module singletons, so GLOBAL_TRACE_PROVIDER and the default processor/exporter no longer become conflicting sources of truth. The setup atexit hook is reverted to its original form. - Closing is idempotent and a failing close is logged, not raised. Covers public shutdown, repeated/idempotent shutdown, provider replacement, and injected-exporter preservation.
|
Thanks for the detailed review — I reworked the fix along the lines you described. Public shutdown lifecycle, explicit ownership. The close now lives on Coherent globals. I no longer clear the module singletons, and the Idempotent / safe. Repeated shutdown keeps closing safely, and a failing New tests cover explicit public shutdown, repeated/idempotent shutdown, provider replacement preserving an injected exporter, and the default processor owning and closing its exporter. |
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f11e18947b
ℹ️ 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".
…down Addresses review follow-up: closing the default exporter left GLOBAL_TRACE_PROVIDER and the cached default processor pointing at the now-closed exporter, so a trace emitted after shutdown (e.g. from an atexit callback that runs after this one) reused the stale provider and silently dropped the final export. When the registered global default provider shuts down -- on both the public get_trace_provider().shutdown() and the atexit paths -- reset GLOBAL_TRACE_PROVIDER together with the module-owned default processor/exporter singletons. The three are released as one coherent unit, and the next trace lazily rebuilds a fresh stack. A caller's own provider that is not the registered global one is left untouched, and an injected exporter stays caller-owned.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 259e766207
ℹ️ 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".
…fault stack Follow-up: when set_trace_processors (or another public override) swaps the provider's processors after the default stack was bootstrapped, the module-owned default processor is no longer registered and never runs its shutdown, so it never closes its exporter. _reset_default_processor then dropped the singleton while the exporter's httpx client was still open, leaking one client per rebuild cycle and re-introducing the ResourceWarning this change fixes. Close the module-owned exporter before dropping the reference. close() is idempotent, so the normal path (default processor already closed it on shutdown) is unaffected.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4caee3f56d
ℹ️ 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".
Follow-up on the shutdown reset concurrency window. The reset cleared GLOBAL_TRACE_PROVIDER first and then, under a separate lock, cleared the default processor/exporter singletons. A thread calling get_trace_provider() in that gap saw no global provider and rebuilt one from the stale processor singleton, adopting the just-shut-down processor whose shutdown event is set and whose exporter is closed -- later traces through it could be dropped. Clear the module singletons before publishing GLOBAL_TRACE_PROVIDER = None, both inside the global-provider lock, so a concurrent rebuild observes either the still-registered provider or a fully fresh stack. _detach_default_processor clears and returns the exporter (acquiring the processor lock nested inside the provider lock, matching the get_trace_provider lock order); _close_default_exporter then closes it outside both locks since close() may do I/O.
|
@codex review |
|
Note You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
@abhay-codes07 This repo automatically triggers codex review for each push. No need to mention codex unless the auto trigger is temporarily unstable. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d48acc0d28
ℹ️ 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".
…lose The follow-up that rebuilt the default stack on shutdown (resetting GLOBAL_TRACE_PROVIDER and the module singletons) introduced regressions: it dropped the exporter that stores an explicit set_tracing_export_api_key value, and its identity-only check also reset a caller-installed DefaultTraceProvider, silently rebuilding the SDK backend provider and bypassing the caller's processors on a later trace. Revert to the narrower, coherent design: the module-owned default exporter is closed on shutdown via BatchTraceProcessor._owns_exporter (set only by default_processor), on both the public get_trace_provider().shutdown() and atexit paths. Nothing is cleared, so GLOBAL_TRACE_PROVIDER and the default processor/exporter stay a single coherent source of truth, the configured tracing API key survives, and a caller-installed provider is left untouched. A caller-injected exporter remains caller-owned and is never closed here.
|
Heads up on a design decision (latest commit I initially followed up the exporter-close by also rebuilding the default stack on shutdown (resetting
So I dropped the rebuild and kept the narrower change that matches your two review points directly:
Net effect vs |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b58f06b77f
ℹ️ 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".
…ops it set_trace_processors can replace the module-owned default processor after the default stack was already bootstrapped. That processor's shutdown -- the only path that closes the default BackendSpanExporter -- then never runs, leaking its HTTP client (ResourceWarning) for the rest of the process. When the replacement set no longer contains the default processor, shut it down (closing the exporter via _owns_exporter) and clear the module singletons so a later default_processor/default_exporter rebuilds a fresh stack. A default processor the caller keeps in the new set is left running, GLOBAL_TRACE_PROVIDER is untouched, and caller-injected exporters remain caller-owned.
|
Note You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d6c8b89852
ℹ️ 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".
…ocessor _detach_default_processor_if_replaced called processor.shutdown() with no timeout, so swapping processors via set_trace_processors could block on the detached default exporter's network flush and retries against a slow or unreachable backend. Use a zero timeout: the replaced default's queued work is abandoned, its retry backoff is cut short, and its exporter's client is still closed (the processor owns it).
Summary
The default
BackendSpanExporteropens a pooledhttpx2.Clientin its constructor and keeps it for the process lifetime, but nothing in the shutdown chain ever called its existingclose(). At interpreter teardown the TLS connection toapi.openai.comwas left open, emittingResourceWarning: unclosed <ssl.SSLSocket ...>under-W error::ResourceWarning/PYTHONDEVMODE=1— which fails strict CI setups — and leaking one client per default-stack rebuild.Fixes #4681.
Approach. As discussed on the earlier PR #4682,
BatchTraceProcessorcannot safely infer ownership of an exporter it was handed, so the leak is fixed at the setup boundary that creates and owns the default exporter, not by having a processor guess. The atexit handler_shutdown_global_trace_providernow closes the module-owned default exporter after the processors have drained, via a new_shutdown_default_exporterhelper in theprocessorsmodule:default_exporter/default_processoris closed. An exporter injected into a caller-constructedBatchTraceProcessoris left untouched — ownership is never inferred from the processor or from object identity._global_exporter/_global_processorsingletons are cleared under the existing lock, so a later trace rebuilds a fresh default stack rather than reusing an exporter whose client is already closed (no shutdown-then-reuse regression).finallyso a provider-shutdown failure can't leak the client, and it's a no-op when no default exporter was ever created.This is the narrower fix seratch left #4681 open for; it does not add a
close()ownership contract to injected exporters.Test plan
New tests in
tests/test_trace_processor.py:test_shutdown_default_exporter_closes_and_clears_singletons— closes the client, clears the singletons, and a laterdefault_exporter()returns a fresh instance.test_shutdown_default_exporter_is_noop_without_a_default— safe when no default was created.test_shutdown_default_exporter_leaves_injected_exporter_open— an injected exporter is not closed (ownership respected).test_global_provider_shutdown_closes_default_exporter— the atexit/setup path closes the default exporter's client.All four fail on
mainwithout the source change.uv run pytest tests/test_trace_processor.py tests/test_released_api_contract.py→ 177 passed. Full suite,ruff format --check,ruff check, andmypyon the changed files pass locally.Issue number
Fixes #4681
Checks
make lint,make formatandmake tests(ran targeted lint/format/typecheck + the trace and full suites locally)/reviewbefore submitting this PR