Skip to content

Session Replay is not cleaned up correctly after calling Sentry.close #5069

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.

Sign up for GitHub

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

Jump to bottom
Open
philprime opened this issue Apr 8, 2025 · 3 comments · May be fixed by #5121 or #5158
Open

Session Replay is not cleaned up correctly after calling Sentry.close #5069

philprime opened this issue Apr 8, 2025 · 3 comments · May be fixed by #5121 or #5158
Assignees
@philprime
Labels
Bug Cocoa Session Replay

Comments

@philprime
Copy link
Contributor

Description

I created a sample in #5055 which provides a function to reload the Sentry SDK:

static func reloadSentrySDK() {
    if SentrySDK.isEnabled {
        print("SentrySDK already started, closing it")
        SentrySDK.close()
    }

    SentrySDK.start { options in
        options.dsn = "https://6cc9bae94def43cab8444a99e0031c28@o447951.ingest.sentry.io/5428557"
        options.debug = true

        options.tracesSampleRate = 1.0
        options.profilesSampleRate = 1.0
        options.sessionReplay.sessionSampleRate = Self.isSessionReplayEnabled ? 1.0 : 0.0
        options.sessionReplay.enableExperimentalViewRenderer = Self.isExperimentalViewRendererEnabled
    }
}

When closing the SDK the on-demand replay capturing is not restarted.
An initial investigation showed that uninstalling the SentrySessionReplayIntegration calls the method SentrySessionReplay.pause() which invalidates the CADisplayLink used as the trigger for creating frames:

https://github.com/getsentry/sentry-cocoa/blob/0782001b8903e178a3bb8db34f2f366ce1cc04ab/Sources/Sentry/SentrySessionReplayIntegration.m#L523C9-L527

sentry-cocoa/Sources/Swift/Integrations/SessionReplay/SentrySessionReplay.swift

Lines 108 to 117 in 0782001

func pause() {
lock.lock()
defer { lock.unlock() }
displayLink.invalidate()
if isFullSession {
prepareSegmentUntil(date: dateProvider.date())
}
isSessionPaused = false
}

When installing the SDK again, there seems to be left-over state and the session replay recorder is not bootstrapped correctly.

When calling stop() instead of pause() in the SentrySessionReplayIntegration, the object gets deallocated and discarded, but it is not started again correctly.
@philprime philprime self-assigned this Apr 8, 2025
@github-project-automation github-project-automation bot moved this to Needs Discussion in [DEPRECATED] Mobile SDKs Apr 8, 2025
@philprime philprime changed the title On-demand Session Replay is not cleaned up correctly after calling Sentry.close Session Replay is not cleaned up correctly after calling Sentry.close Apr 8, 2025
@philprime philprime mentioned this issue Apr 8, 2025
Closed
@philprime philprime moved this from Needs Discussion to Todo in [DEPRECATED] Mobile SDKs Apr 9, 2025
@philprime
Copy link
Contributor Author

It looks to me like the change from stop to pause was introduced in #4326 without proper documentation what the difference between the two methods is

@philprime philprime moved this from Todo to In Progress in [DEPRECATED] Mobile SDKs Apr 14, 2025
@kahest kahest added the Platform: Cocoa label Apr 14, 2025 — with Linear
@philprime
Copy link
Contributor Author

I believe the issue lies in the starting of session replay capturing of frames.

The didBecomeActive notification is used as a trigger, therefore if the SDK is closed and reopened, this trigger won't start the capturing again as the app is already in foreground.

@philprime
Copy link
Contributor Author

Further investigations now surfaced that the SentrySessionTracker is not starting a new session when closing and restarting the SDK while app is already running, therefore causing session replay also not to start.

This comment states we do not want to automatically start a new session as soon as the SDK starts:

sentry-cocoa/Sources/Sentry/SentrySessionTracker.m

Lines 46 to 52 in da7b357

/**
* Can also be called when the system launches an app for a background task. We don't want to track
* sessions if an app is only in the background. Therefore we must not start a session in here. Such
* apps must do session tracking manually, see
* https://docs.sentry.io/workflow/releases/health/#session
*/
- (void)start

Therefore we instead rely on the didBecomeActive trigger from the application lifecycle or triggered by the Hybrid SDKs.

sentry-cocoa/Sources/Sentry/SentrySessionTracker.m

Lines 135 to 149 in da7b357

/**
* It is called when an App. is receiving events / It is in the foreground and when we receive a
* @c SentryHybridSdkDidBecomeActiveNotification. There is no guarantee that this method is called
* once or twice. We need to ensure that we execute it only once.
* @discussion This also works when using SwiftUI or Scenes, as UIKit posts a
* @c didBecomeActiveNotification regardless of whether your app uses scenes, see
* https://developer.apple.com/documentation/uikit/uiapplicationdelegate/1622956-applicationdidbecomeactive.
* @warning Hybrid SDKs must only post this notification if they are running in the foreground
* because the auto session tracking logic doesn't support background tasks. Posting the
* notification from the background would mess up the session stats. Hybrid SDKs must only post this
* notification if they are running in the foreground because the auto session tracking logic
* doesn't support background tasks. Posting the notification from the background would mess up the
* session stats.
*/
- (void)didBecomeActive

This method then calls [hub startSession] depending on if the app was not in the foreground yet, or if it has been in the background long enough:

sentry-cocoa/Sources/Sentry/SentrySessionTracker.m

Lines 163 to 179 in da7b357

if (nil == self.lastInForeground) {
// Cause we don't want to track sessions if the app is in the background we need to wait
// until the app is in the foreground to start a session.
[hub startSession];
} else {
// When the app was already in the foreground we have to decide whether it was long enough
// in the background to start a new session or to keep the session open. We don't want a new
// session if the user switches to another app for just a few seconds.
NSTimeInterval secondsInBackground =
[[SentryDependencyContainer.sharedInstance.dateProvider date]
timeIntervalSinceDate:self.lastInForeground];
if (secondsInBackground * 1000 >= (double)(self.options.sessionTrackingIntervalMillis)) {
[hub endSessionWithTimestamp:self.lastInForeground];
[hub startSession];
}
}

If [hub startSession] is invoked, it invokes the listener here:

sentry-cocoa/Sources/Sentry/SentryHub.m

Line 131 in da7b357

[_sessionListener sentrySessionStarted:_session];

Which will forward the calls to the session replay integration:

sentry-cocoa/Sources/Sentry/SentrySessionReplayIntegration.m

Lines 508 to 512 in da7b357

- (void)sentrySessionStarted:(SentrySession *)session
{
_rateLimited = NO;
[self startSession];
}

Therefore the first step must be fixing the session tracker to ensure that a "session" is actually starting.

@philprime philprime linked a pull request Apr 22, 2025 that will close this issue
Open
7 tasks
@philprime philprime moved this from In Progress to Needs Review in [DEPRECATED] Mobile SDKs Apr 23, 2025
@philprime philprime moved this from Needs Review to In Progress in [DEPRECATED] Mobile SDKs Apr 25, 2025
@philprime philprime linked a pull request Apr 30, 2025 that will close this issue
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@philprime philprime

Labels
Bug Cocoa Session Replay
Projects
[DEPRECATED] Mobile SDKs
Status: In Progress
Milestone
No milestone
3 participants
@kahest @stephanie-anderson @philprime