fix(profiling): release the EventPipe session when the SDK shuts down - #5470
Draft
jamescrosswell wants to merge 1 commit into
Draft
fix(profiling): release the EventPipe session when the SDK shuts down#5470jamescrosswell wants to merge 1 commit into
jamescrosswell wants to merge 1 commit into
Conversation
SamplingTransactionProfilerFactory.Dispose() disposed the antecedent Task<SampleProfilerSession> rather than the session it wraps. Fixing that alone changes nothing observable, because two further defects sit between "SDK shuts down" and "EventPipe session released": - SampleProfilerSession.Stop() built _processing as an OnlyOnFaulted continuation, which transitions to Canceled when Process() returns normally - so Wait() threw on every clean shutdown and the disposals after it were never reached. - ProfilingIntegration was not IDisposable, so Hub never registered it for cleanup and the factory's Dispose() was only ever called by tests. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #5470 +/- ##
==========================================
+ Coverage 74.71% 74.74% +0.03%
==========================================
Files 513 513
Lines 18729 18758 +29
Branches 3663 3669 +6
==========================================
+ Hits 13993 14021 +28
+ Misses 3865 3858 -7
- Partials 871 879 +8 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
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
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.
SamplingTransactionProfilerFactory.Dispose()disposed the antecedentTask<SampleProfilerSession>rather than the session it wraps, so theEventPipeSessionwas never stopped.Tracing it through, that line is the last of three defects sitting between "SDK shuts down" and "EventPipe session released" — fixing it alone would not have changed anything observable:
SampleProfilerSession.Stop()skipped its own disposals on the happy path._processingwas built withTaskContinuationOptions.OnlyOnFaulted; wheneventSource.Process()returns normally the continuation criteria aren't met, so the continuation transitions toCanceledand_processing.Wait()throwsAggregateException(TaskCanceledException).EventPipeSession.Dispose()andTraceLogEventSource.Dispose()sat after that call and were never reached — thecatchloggedError during sampler profiler session shutdown.and swallowed it. The continuation is now unconditional, and the disposals moved into afinallywith a bounded drain wait so shutdown can't hang.Nothing ever called
Dispose()on the factory.ProfilingIntegrationwas notIDisposable, soHubnever added it to_integrationsToCleanup, and the factory's only other reference isoptions.TransactionProfilerFactory, which nothing disposes. The integration is nowIDisposableand disposes the factory — but only one it created itself, sinceTransactionProfilerFactorymay have been supplied elsewhere.The reported bug.
Dispose()now waits (bounded) for an in-flight startup and then disposes the session. It also cancels theWaitForFirstEventAsyncwait, which could otherwise block indefinitely, and observes the startup task's exception if it failed.Impact
Low, and narrower than the issue suggests. The session is created once and lives for the SDK's lifetime —
SamplingTransactionProfiler.Stop()only clears_inProgress, it never touches the session — so the only disposal point is SDK shutdown, which is normally process shutdown, where the OS reclaims the handle and theTraceLogregardless. Nothing accumulates while an application runs.Where it does bite is when the process outlives the SDK: repeated
Init/Closecycles strand oneEventPipeSessionplus itsTraceLogper cycle, and hosts that close Sentry but keep running never release the pipe.The issue links this to the Windows Service memory growth in #3375. That link doesn't hold up — a service that initialises Sentry once would never reach any of these paths.
Notes
Error during sampler profiler session shutdown.on every clean shutdown with profiling enabled. Worth asking anyone reporting profiler-related growth whether they see it.Hub.Dispose()now does real work for profiling users where it previously did none. Both new waits are bounded at 2s so a wedged EventPipe session can't hang shutdown.Sentry.Profilingtests are largelySkip.If(TestEnvironment.IsGitHubActions), so the twoProfilingIntegrationtests are plain[Fact]s that do run in CI; only the session-level test follows the existing skip pattern.Fixes #5418