Skip to content

fix(profiling): release the EventPipe session when the SDK shuts down - #5470

Open
jamescrosswell wants to merge 11 commits into
mainfrom
fix/5418-profiler-session-dispose
Open

fix(profiling): release the EventPipe session when the SDK shuts down#5470
jamescrosswell wants to merge 11 commits into
mainfrom
fix/5418-profiler-session-dispose

Conversation

@jamescrosswell

@jamescrosswell jamescrosswell commented Aug 5, 2026

Copy link
Copy Markdown
Collaborator

SamplingTransactionProfilerFactory.Dispose() disposed the antecedent Task<SampleProfilerSession> rather than the session it wraps, so the EventPipeSession was never released.

That line turned out to be the last of three independent defects sitting between "SDK shuts down" and "EventPipe session released" — fixing it alone would not have changed anything observable.

What was broken

  1. SampleProfilerSession.Stop() skipped its own disposals on the happy path. _processing was built with TaskContinuationOptions.OnlyOnFaulted. When eventSource.Process() returns normally the continuation criteria aren't met, so the continuation transitions to Canceled and _processing.Wait() throws AggregateException(TaskCanceledException). EventPipeSession.Dispose() and TraceLogEventSource.Dispose() sat after that call and were never reached — the catch logged Error during sampler profiler session shutdown. and swallowed it. This fired on every clean shutdown, not on some edge case.

  2. Nothing ever called Dispose() on the factory. ProfilingIntegration wasn't IDisposable, so Hub never added it to _integrationsToCleanup, and the factory's only other reference is options.TransactionProfilerFactory, which nothing disposes. Outside of tests, SamplingTransactionProfilerFactory.Dispose() was unreachable.

  3. The reported bug. The ContinueWith parameter is the antecedent task, so Dispose() called Task.Dispose() — a near no-op on a completed task — instead of disposing the session.

What this changes

ProfilingIntegration now owns the factory it creates. It implements IDisposable so the Hub tears it down on shutdown, and disposal also clears options.TransactionProfilerFactory, so a SentryOptions reused for a later hub gets a working profiler rather than a stale reference. It only touches a factory it created itself — one supplied from elsewhere is left alone.

SamplingTransactionProfilerFactory.Dispose() now actually stops the session. StartEventPipeSession() can block for up to 30 seconds and can't be cancelled, so disposal can land before there is any session to stop; a lock hands the session between the startup task and Dispose() so exactly one of the two tears it down. The wait for the first event is cancelled on disposal rather than left to block indefinitely. Both waits are bounded at 2 seconds so a wedged session can't hang shutdown.

SampleProfilerSession.Stop() moves the disposals into a finally, so the EventPipe connection is released even when stopping the session or draining its events fails.

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 the TraceLog regardless. Nothing accumulates while an application runs.

It matters where the process outlives the SDK: repeated Init/Close cycles strand one EventPipeSession plus its TraceLog per 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. The steady-state growth reported in #5469 is a separate mechanism and is not addressed here.

Notes

  • Defect 1 has a visible signature in diagnostic logs: Error during sampler profiler session shutdown. on every clean shutdown with profiling enabled. Worth asking anyone reporting profiler-related resource growth whether they see it.
  • Hub.Dispose() now does real work for profiling users where it previously did none.
  • Most of Sentry.Profiling.Tests is Skip.If(TestEnvironment.IsGitHubActions) because it opens real EventPipe sessions. The ProfilingIntegration tests added here are plain [Fact]s that do run in CI; the two session-level tests follow the existing skip pattern.
  • AGENTS.md gains a short section recording the repo's preference for code over comments. Happy to split that into its own PR if it doesn't belong here.

Fixes #5418

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

codecov Bot commented Aug 5, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 73.23944% with 19 lines in your changes missing coverage. Please review.
✅ Project coverage is 74.78%. Comparing base (2be688c) to head (d26d7a5).
⚠️ Report is 2 commits behind head on main.

Files with missing lines Patch % Lines
src/Sentry.Profiling/SampleProfilerSession.cs 35.29% 7 Missing and 4 partials ⚠️
...ry.Profiling/SamplingTransactionProfilerFactory.cs 79.48% 6 Missing and 2 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #5470      +/-   ##
==========================================
- Coverage   74.87%   74.78%   -0.09%     
==========================================
  Files         513      513              
  Lines       18659    18821     +162     
  Branches     3636     3678      +42     
==========================================
+ Hits        13970    14076     +106     
- Misses       3819     3867      +48     
- Partials      870      878       +8     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Comment thread src/Sentry.Profiling/SampleProfilerSession.cs Outdated
Comment thread src/Sentry.Profiling/SampleProfilerSession.cs Outdated
Comment thread src/Sentry.Profiling/SampleProfilerSession.cs Outdated
Comment thread src/Sentry.Profiling/SamplingTransactionProfilerFactory.cs Outdated
Comment thread src/Sentry.Profiling/SamplingTransactionProfilerFactory.cs Outdated
Comment thread src/Sentry.Profiling/SamplingTransactionProfilerFactory.cs Outdated
Comment thread src/Sentry.Profiling/SamplingTransactionProfilerFactory.cs Outdated
Co-authored-by: James Crosswell <jamescrosswell@users.noreply.github.com>
@jamescrosswell
jamescrosswell marked this pull request as ready for review August 19, 2026 09:24
@github-actions github-actions Bot added the risk: medium PR risk score: medium label Aug 19, 2026
Comment thread src/Sentry.Profiling/SamplingTransactionProfilerFactory.cs
Comment thread src/Sentry.Profiling/SamplingTransactionProfilerFactory.cs
jamescrosswell and others added 2 commits August 19, 2026 22:02
Both review bots flagged the same window from different angles, and both
were right:

- The startup task read _shutdownCts.Token, which throws
  ObjectDisposedException once Dispose() has disposed the CTS. The token is
  now captured up front so the task never touches the source.
- StartEventPipeSession() can block for up to 30s and isn't cancellable, so
  Dispose() could run before _session was ever assigned - disposing nothing
  and leaving the session that arrived later running forever. A lock now
  hands the session between the two sides so exactly one of them stops it.

Adds test seams to SampleProfilerSession so the race can be exercised
deterministically rather than by timing.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…e' into fix/5418-profiler-session-dispose

# Conflicts:
#	src/Sentry.Profiling/SamplingTransactionProfilerFactory.cs
Comment thread src/Sentry.Profiling/SampleProfilerSession.cs Outdated
Comment thread src/Sentry.Profiling/SampleProfilerSession.cs Outdated
Comment thread src/Sentry.Profiling/SampleProfilerSession.cs Outdated
Comment thread src/Sentry.Profiling/SamplingTransactionProfilerFactory.cs Outdated
Comment thread src/Sentry.Profiling/SamplingTransactionProfilerFactory.cs Outdated
jamescrosswell and others added 4 commits August 19, 2026 22:20
Co-authored-by: James Crosswell <jamescrosswell@users.noreply.github.com>
Documents the convention in AGENTS.md: this repo favours code that needs no
comments, with the code and the PR description as the documentation. Minimal
comments only where something genuinely isn't obvious.

Applies it to this branch - 34 added comment lines down to 12. What survives is
non-intuitive framework behaviour (a ContinueWith whose criteria aren't met is
Canceled; reading CancellationTokenSource.Token after Dispose throws) and the
justification for an empty catch. Test rationale moved into FluentAssertions
"because" strings, where it shows up in failure output instead of sitting in a
comment.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
disposedDuringStartup read as "SDK startup". It means Dispose() ran while the
EventPipe session was still being created, so disposedWhileSessionStarting.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@jamescrosswell
jamescrosswell marked this pull request as draft August 23, 2026 23:40
jamescrosswell and others added 3 commits August 24, 2026 11:48
ProfilingIntegration.Dispose() disposed the factory but left
options.TransactionProfilerFactory pointing at it. Reusing the same
SentryOptions for a later Hub meant Register() saw a non-null factory, skipped
creating a working one, and profiling silently never started.

Reported by Warden on #5470.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
An earlier commit on this branch staged a stale submodule working copy,
reverting the pointer that came in from main and leaving the committed Cocoa
bindings out of step with it. That fails dirty-check.ps1 on the macOS build.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@jamescrosswell
jamescrosswell marked this pull request as ready for review August 25, 2026 04:39

@cursor cursor Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cursor Bugbot has reviewed your changes and found 1 potential issue.

Fix All in Cursor

❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.

Reviewed by Cursor Bugbot for commit d26d7a5. Configure here.

factory.Dispose();
_ownedFactory = null;
_options = null;
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re-init disposes the replacement hub profiler

Medium Severity

SentrySdk.Init constructs the new Hub (and calls Register) before disposing the previous one. Register keeps the existing factory, then the old hub's Dispose stops that factory and clears TransactionProfilerFactory. The replacement hub is left with no profiler, so a second Init silently disables profiling.

Additional Locations (1)
Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit d26d7a5. Configure here.

@vaind vaind left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd look at bugbot's comment, other than that, lgtm

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

risk: medium PR risk score: medium

Projects

None yet

Development

Successfully merging this pull request may close these issues.

SamplingTransactionProfilerFactory.Dispose disposes the Task, not the SampleProfilerSession (resource leak)

2 participants