-
Notifications
You must be signed in to change notification settings - Fork 168
🐛 [RUM Profiler] Fix profiler stuck when session expires #4152
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
🐛 [RUM Profiler] Fix profiler stuck when session expires #4152
Conversation
When the profiler was paused (tab hidden) and the session expired, stopProfilerInstance would return early without updating instance.state from 'paused' to 'stopped'. This caused the session renewal check to fail, leaving the profiler stuck.
Bundles Sizes Evolution
🚀 CPU Performance
🧠 Memory Performance
|
When SESSION_RENEWED fires while the async stopProfiling is still in progress, the check for instance.state === 'stopped' fails because the state hasn't been updated yet. This causes the profiler to not restart. Use flags to track when SESSION_RENEWED fires during the stop process and restart the profiler after stop completes if needed.
31d5c0f to
e1f9df7
Compare
… collection Replace flag-based approach with simpler sync design: - stopProfilerInstance/pauseProfilerInstance update state synchronously - Data collection continues in background (fire-and-forget) - Eliminates race condition by design: SESSION_RENEWED always sees correct state - Change stop() return type from Promise<void> to void
|
/to-staging |
|
View all feedbacks in Devflow UI.
Commit 7e19e6260a will soon be integrated into staging-07.
Commit 7e19e6260a has been merged into staging-07 in merge commit b6bcfd462c. Check out the triggered pipeline on Gitlab 🦊 If you need to revert this integration, you can use the following command: |
… operations - Use waitNextMicrotask() instead of Promise.resolve() for explicitness - Remove unnecessary waitForBoolean() after profiler.stop() and SESSION_EXPIRED since state changes are now synchronous - Add comments explaining microtask flushes needed for async data collection
Motivation
The profiler can get stuck in "stopped" status after periods of inactivity, preventing it from collecting profiling data. Two race conditions were identified:
Session expires while profiler is paused: When the tab is hidden, the profiler pauses. If the session expires during this time,
stopProfilerInstancereturned early without updating the instance state from 'paused' to 'stopped', causing the session renewal check to fail.Session renews while stop is in progress: When
SESSION_RENEWEDfires while the asyncstopProfilingwas still executing, the checkinstance.state === 'stopped'failed because the state hadn't been updated yet.Changes
Fix 1: Handle paused state in
stopProfilerInstanceWhen
stopProfilerInstanceis called while the profiler is paused, properly transition the state to 'stopped' with the appropriatestateReason.Fix 2: Sync state changes with fire-and-forget data collection
Instead of using
awaitto wait for profiler to stop (with data being sent), we use a syncstopProfiling:stopProfilerInstanceandpauseProfilerInstancenow update state synchronouslycollectProfilerInstance) continues in the background as fire-and-forgetSESSION_RENEWEDfires, the state is already 'stopped'stop()return type fromPromise<void>tovoidThis approach is simpler and more robust than the flag-based solution (commit 2 in this PR) because:
Unit tests
Since we changed the
stopandstopProfilerInstancefunctions to be sync instead of async, and we don't wait for the profiles to be sent, we needed to update the unit tests to account for:stopis updating synchronously the status of the profiler.isStoppedno longer tells you the latest "Profile" was sent.So overall we needed to await the requests to make sure it was properly sent.
Test instructions
SESSION_EXPIREDFor the race condition test:
SESSION_EXPIREDimmediately followed bySESSION_RENEWEDBoth of theses have unit test to cover it.
Checklist