test(server): fix TracingTestGuard drop order and add force_flush - #2604
Open
krishicks wants to merge 1 commit into
Open
test(server): fix TracingTestGuard drop order and add force_flush#2604krishicks wants to merge 1 commit into
krishicks wants to merge 1 commit into
Conversation
krishicks
requested review from
a team,
derekwaynecarr,
maxamillion and
mrunalp
as code owners
August 3, 2026 23:18
krishicks
enabled auto-merge
August 3, 2026 23:19
Rust drops struct fields in declaration order. TracingTestGuard previously dropped _default (restoring the thread-local subscriber) before _provider (shutting down the provider). This created a window where the subscriber was gone but the provider was still live: any span closing in that gap could skew the GUARD_STACK thread-local used by tracing-opentelemetry's context_activation, leaving a stale OTel parent context visible to the next test. The result was spans that should be trace roots appearing as children, causing assert_is_root to fail intermittently on CI. Fix the field order so _provider drops first (while the test subscriber is still installed), then _default restores the subscriber, then _lock releases the serialization mutex. Also add TracingTestGuard::force_flush() and call it before every span assertion across all tracing tests. SimpleSpanProcessor exports synchronously so this is a no-op today, but it makes the synchronization intent explicit and guards against regressions if the processor type ever changes. Signed-off-by: Kris Hicks <khicks@nvidia.com>
krishicks
force-pushed
the
hicks/push-mnmulylrwmwv
branch
from
August 4, 2026 01:59
e0bd740 to
95788b1
Compare
| /// spans which close during shutdown export to the correct exporter rather | ||
| /// than leaking into the thread-local `OTel` context stack. `provider` is | ||
| /// therefore declared before `_default` — Rust drops fields in declaration | ||
| /// order, so the provider is torn down first. |
Contributor
There was a problem hiding this comment.
always something new to learn with rust
| impl TracingTestGuard { | ||
| /// Flush the provider so all pending spans are exported before reading. | ||
| /// | ||
| /// `SimpleSpanProcessor` exports synchronously, so this is a no-op for |
Contributor
There was a problem hiding this comment.
I assume rust has a BatchSpanProcessor like other langs that exports otel? using that in "prod" (as opposed to tests) I think is generally the blessed path, not sure if we're only using Simple in tests
Contributor
There was a problem hiding this comment.
my agent tells me we are indeed using Batch outside the test path 👍
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
2 participants
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.
Summary
Try to fix the flaky tracing tests.
Rust drops struct fields in declaration order. TracingTestGuard previously dropped _default (restoring the thread-local subscriber) before _provider (shutting down the provider). This created a window where the subscriber was gone but the provider was still live: any span closing in that gap could skew the GUARD_STACK thread-local used by tracing-opentelemetry's context_activation, leaving a stale OTel parent context visible to the next test. The result was spans that should be trace roots appearing as children, causing assert_is_root to fail intermittently on CI.
Fix the field order so _provider drops first (while the test subscriber is still installed), then _default restores the subscriber, then _lock releases the serialization mutex.
Also add TracingTestGuard::force_flush() and call it before every span assertion across all tracing tests. SimpleSpanProcessor exports synchronously so this is a no-op today, but it makes the synchronization intent explicit and guards against regressions if the processor type ever changes.
Related Issue
Changes
Testing
mise run pre-commitpassesChecklist