fix: Marking manual sessions as crashed (#3501)

Fix marking manual sessions as crashed when capturing crash events. Fixes GH-3498
getsentry · Dec 15, 2023 · aea5987 · aea5987
1 parent e02158b
commit aea5987
Show file tree

Hide file tree

Showing 3 changed files with 43 additions and 30 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -2,11 +2,15 @@
 
 ## Unreleased
 
-## Features
+### Features
 
 - Add frames delay to transactions (#3487)
 - Add slow and frozen frames to spans (#3450, #3478)
 
+### Fixes
+
+- **Fix marking manual sessions as crashed (#3501)**: When turning off autoSessionTracking and manually starting and ending sessions, the SDK didn't mark sessions as crashed when sending a crash event to Sentry. This is fixed now.
+
 ## 8.17.1
 
 ### Fixes 

diff --git a/Sources/Sentry/SentryHub.m b/Sources/Sentry/SentryHub.m
@@ -238,10 +238,10 @@ - (void)captureCrashEvent:(SentryEvent *)event
 }
 
 /**
- * If autoSessionTracking is enabled we want to send the crash and the event together to get proper
- * numbers for release health statistics. If there are multiple crash events to be sent on the start
- * of the SDK there is currently no way to know which one belongs to the crashed session so we just
- * send the session with the first crashed event we receive.
+ * We must send the crash and the event together to get proper numbers for release health
+ * statistics. If multiple crash events are to be dispatched at the start of the SDK, there is
+ * currently no way to know which one belongs to the crashed session, so we send the session with
+ * the first crash event we receive.
  */
 - (void)captureCrashEvent:(SentryEvent *)event withScope:(SentryScope *)scope
 {
@@ -252,21 +252,18 @@ - (void)captureCrashEvent:(SentryEvent *)event withScope:(SentryScope *)scope
         return;
     }
 
-    // Check this condition first to avoid unnecessary I/O
-    if (client.options.enableAutoSessionTracking) {
-        SentryFileManager *fileManager = [client fileManager];
-        SentrySession *crashedSession = [fileManager readCrashedSession];
+    SentryFileManager *fileManager = [client fileManager];
+    SentrySession *crashedSession = [fileManager readCrashedSession];
 
-        // It can be that there is no session yet, because autoSessionTracking was just enabled and
-        // there is a previous crash on disk. In this case we just send the crash event.
-        if (crashedSession != nil) {
-            [client captureCrashEvent:event withSession:crashedSession withScope:scope];
-            [fileManager deleteCrashedSession];
-            return;
-        }
+    // It can occur that there is no session yet because autoSessionTracking was just enabled or
+    // users didn't start a manual session yet, and there is a previous crash on disk. In this case,
+    // we just send the crash event.
+    if (crashedSession != nil) {
+        [client captureCrashEvent:event withSession:crashedSession withScope:scope];
+        [fileManager deleteCrashedSession];
+    } else {
+        [client captureCrashEvent:event withScope:scope];
     }
-
-    [client captureCrashEvent:event withScope:scope];
 }
 
 - (SentryId *)captureTransaction:(SentryTransaction *)transaction withScope:(SentryScope *)scope

diff --git a/Tests/SentryTests/SentryHubTests.swift b/Tests/SentryTests/SentryHubTests.swift
@@ -1,3 +1,4 @@
+import Nimble
 import Sentry
 import SentryTestUtils
 import XCTest
@@ -606,9 +607,28 @@ class SentryHubTests: XCTestCase {
 
         assertNoCrashedSessionSent()
 
+        let environment = "test"
+        sut.configureScope { $0.setEnvironment(environment) }
         sut.captureCrash(fixture.event)
+        assertEventSentWithSession(scopeEnvironment: environment)
 
-        assertEventSentWithSession()
+        // Make sure further crash events are sent
+        sut.captureCrash(fixture.event)
+        assertCrashEventSent()
+    }
+
+    func testCaptureCrashEvent_ManualSessionTracking_CrashedSessionExists() {
+        givenAutoSessionTrackingDisabled()
+
+        givenCrashedSession()
+
+        assertNoCrashedSessionSent()
+
+        let environment = "test"
+        sut.configureScope { $0.setEnvironment(environment) }
+        sut.captureCrash(fixture.event)
+
+        assertEventSentWithSession(scopeEnvironment: environment)
 
         // Make sure further crash events are sent
         sut.captureCrash(fixture.event)
@@ -640,15 +660,6 @@ class SentryHubTests: XCTestCase {
         assertCrashEventSent()
     }
 
-    func testCaptureCrashEvent_SessionExistsButAutoSessionTrackingDisabled() {
-        givenAutoSessionTrackingDisabled()
-        givenCrashedSession()
-
-        sut.captureCrash(fixture.event)
-
-        assertCrashEventSent()
-    }
-
     func testCaptureCrashEvent_ClientIsNil() {
         sut = fixture.getSut()
         sut.bindClient(nil)
@@ -989,7 +1000,7 @@ class SentryHubTests: XCTestCase {
         XCTAssertTrue(arguments.first?.event.isCrashEvent ?? false)
     }
 
-    private func assertEventSentWithSession() {
+    private func assertEventSentWithSession(scopeEnvironment: String) {
         let arguments = fixture.client.captureCrashEventWithSessionInvocations
         XCTAssertEqual(1, arguments.count)
 
@@ -1001,7 +1012,8 @@ class SentryHubTests: XCTestCase {
         XCTAssertEqual(SentrySessionStatus.crashed, session?.status)
         XCTAssertEqual(fixture.options.environment, session?.environment)
 
-        XCTAssertEqual(fixture.scope, argument?.scope)
+        let event = argument?.scope.applyTo(event: fixture.event, maxBreadcrumbs: 10)
+        expect(event?.environment) == scopeEnvironment
     }
 
     private func assertSessionWithIncrementedErrorCountedAdded() {