fix(tracing): clear the exporter's shutdown request when a processor attaches - #4684
fix(tracing): clear the exporter's shutdown request when a processor attaches#4684rajarshidattapy wants to merge 4 commits into
Conversation
…attaches BackendSpanExporter._shutdown_event is one-way: BatchTraceProcessor.shutdown() sets it through _request_shutdown() so an in-flight export abandons its retry backoff, and nothing ever clears it. default_exporter() caches a module-level singleton, so that instance outlives the processor that shut it down. Every later export through it gave up on the first 5xx or network error instead of retrying, and logged a warning blaming a shutdown that was already over. Clear the request as a new processor attaches to the exporter, which is the point where the exporter is known to be in service again. Kept duck-typed like the existing _request_shutdown call, since TracingExporter requires neither. Fixes openai#4683
![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: 4fb0dd57a1
ℹ️ 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".
…topped Clearing the exporter's request as a new processor attached could take the cancellation away from a worker that still owned it: if shutdown(timeout=...) timed out while a worker was exporting a failed batch, and a replacement BatchTraceProcessor was constructed before that worker reached _sleep_before_retry, the worker slept through its backoff and kept retrying after the shutdown that was meant to stop it had already returned. Make ownership explicit instead. The processor that requests the shutdown remembers it, and releases it only when its own worker is gone: at the end of _run for a worker that stopped, or after the synchronous drain when no worker was ever started. A worker abandoned by a timed-out shutdown therefore keeps the exporter cancelled until it exits, and hands it back on the way out, so the next processor still gets its retries. The request is now made before the processor's own shutdown event is set, so a worker that exits immediately always sees who owns it.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8760e90e7f
ℹ️ 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".
Processors share an exporter, so a single exporter-wide flag let one release speak for all of them: a provider shutting its processors down in turn could time out on a processor whose worker was still inside an HTTP request, then shut down a second processor whose worker exited and cleared the shared cancellation. When the blocked request finally returned a 5xx, its worker found the request gone and retried, after its own shutdown had returned. Count the outstanding requests instead. _request_shutdown() increments and sets, _reset_shutdown() decrements and only clears the event when the count reaches zero, both under a lock. Each processor asks at most once, so its single release balances its request and never takes the cancellation away from another processor that still needs it.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 560d2b1bbe
ℹ️ 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".
BatchTraceProcessor.shutdown is safe to call from more than one thread, and the check that kept the exporter request to one per processor was an unsynchronized read-then-set: two concurrent callers could both find the flag unset and both increment the exporter's request count, while the worker released only one of them on its way out. The count never reached zero, so the shared cancellation stayed in force and every later processor gave up on its first transient failure instead of retrying. Move the request behind a per-processor lock, alongside the release that balances it, and release after a successful join as well -- covering a worker that had already exited when the request was made, which would otherwise leave nobody to release it.
|
Thanks for the careful concurrency work. I do not think we should merge this lifecycle contract as-is. The reproducer establishes that exporter reuse after My preference is one explicit terminal ownership model: the owner closes the default exporter, and a later tracing initialization creates a fresh exporter and processor. That avoids adding a ref-counted cross-processor cancellation protocol for a reuse path we have not committed to support. I suggest closing this PR for now while keeping the underlying lifecycle question open. |
2 participants
Fixes #4683
Problem
BackendSpanExporter._shutdown_eventis one-way.BatchTraceProcessor.shutdown(timeout=...)sets it through
_request_shutdown()so an in-flight export abandons its retry backoffinstead of sleeping past the shutdown deadline — but nothing ever clears it, and
default_exporter()caches a module-level singleton, so the exporter outlives the processorthat shut it down.
Every export through that exporter afterwards gives up on the first 5xx or network error
rather than retrying:
The batch is dropped, and the warning blames a shutdown that finished long ago.
Fix
Make ownership of the request explicit: the processor that requests the exporter's shutdown
releases it, and only once its own worker has stopped.
BackendSpanExportercounts outstanding requests:_request_shutdown()increments andsets the event,
_reset_shutdown()decrements and clears it only when the count reacheszero, both under a lock. Processors share an exporter, so one processor's release must not
speak for another's still-running worker.
BatchTraceProcessorrequests at most once — the request and the release that balances itare both taken under a per-processor lock, since
shutdownmay be called from severalthreads — and releases it once its worker is done: at the end of
_run, or inshutdown()itself whenever the worker is not running by the time it returns. Both lookups stay
duck-typed like the existing
_request_shutdowncall, sinceTracingExporterrequiresneither.
that exits immediately always sees that the request is owned.
Releasing on worker exit rather than when the next processor attaches is what keeps a
timed-out shutdown safe: a worker that outlived its
shutdown(timeout=...)still owns thecancellation, keeps abandoning its retries, and hands the exporter back only when it stops —
so a replacement processor can never resurrect the backoff of a worker that is supposed to be
going away. The existing
test_batch_trace_processor_shutdown_interrupts_exporter_retry_backoffcontinues to coverthe abort itself.
No public API changes.
Tests
Two regression tests in
tests/test_trace_processor.py, both failing onmainand passingwith this change:
test_new_processor_restores_exporter_retries_after_a_previous_shutdown— shuts aprocessor down with a timeout, attaches a new one to the same exporter, and asserts the
next 504 batch is retried
max_retriestimes instead of once (1 → 3).test_worker_that_outlives_shutdown_keeps_the_exporter_cancelled— pins the ownershiprule: with the worker stuck inside a request,
shutdown(timeout=0.05)returns while it isstill running, a replacement processor attaches, and the abandoned worker must still find
the exporter cancelled, give up without retrying, and release the request as it exits.
test_concurrent_shutdowns_leave_the_exporter_request_balanced— pins the serialization:two threads shut the same processor down and rendezvous inside the window between the
request check and the request itself, so an unsynchronized decision makes two requests
against the single release the worker performs; the exporter's request count must be back
to zero afterwards.
test_shared_exporter_stays_cancelled_until_every_shutdown_releases— pins the counting:two processors share an exporter, the first times out with its worker inside a request and
the second shuts down cleanly; the shared cancellation must survive the second processor's
release and lift only once the blocked worker has stopped too.
tests/test_trace_processor.py,tests/tracing/,tests/test_tracing.pyandtests/test_tracing_errors.pypass (tests/tracing/test_import_side_effects.py::test_core_imports_do_not_require_legacy_httpxfails identically on
mainin my environment).ruff check,ruff format --checkandmypyare clean on both changed files.