fix(scopes): apply withScope data to events in globalHubMode

.cursor/rules/scopes.mdc

@@ -90,15 +90,20 @@ For JVM Backend applications (servers) we discourage enabling `globalHubMode` si
 
 ### Enabled
 
-If `globalHubMode` is enabled, the SDK avoids forking scopes.
+If `globalHubMode` is enabled, the SDK avoids forking scopes _implicitly_.
 
 This means, retrieving current scopes on a thread where specific scopes do not exist yet for the thread, the root scopes are not forked but returned directly.
-The SDK also doesn't fork scopes when `Sentry.pushScope`, `Sentry.pushIsolation`, `Sentry.withScope` or `Sentry.withIsolationScope` are executed.
+`Sentry.pushScope`, `Sentry.pushIsolationScope` and `Sentry.popScope` are no-ops.
+They are unbalanced API: the caller may never restore the previous scopes, or restore them on a different thread, which would corrupt the globally shared scopes.
+
+`Sentry.withScope` and `Sentry.withIsolationScope` do fork, also when `globalHubMode` is enabled.
+They are balanced by construction and always restore the previous scopes before returning, so they cannot corrupt the shared scopes.
+This matches the cross SDK spec ("fork the current scope, invoke callback, discard the fork when done") and what the SDK did before major version 8.
 
 The suppression of forking via `globalHubMode` only applies when using `Sentry` static API or `ScopesAdapter`.
 In case the `Scopes` instance is accessed directly, forking will happen as if `globalHubMode` is disabled.
-However, while it's possible to use `Sentry.setCurrentScopes` it does not have any effect due to `Sentry.getCurrentScopes` directly returning `rootScopes` if `globalHubMode` is enabled.
-This means the forked scopes have to be managed manually, e.g. by keeping a reference and accessing Sentry API via the reference instead of using static API.
+`Sentry.setCurrentScopes`, and thus `Scopes.makeCurrent`, does have an effect when `globalHubMode` is enabled, but only for scopes that descend from the current `rootScopes`.
+`Sentry.getCurrentScopes` ignores all other scopes, because scopes left over from a closed or re-initialized SDK would otherwise be read back from a thread local that the SDK cannot clean up.
 
 `ScopesAdapter` makes use of the static `Sentry` API internally. It allows us to access the correct scopes for the current context without passing it along explicitly. It also makes testing easier.
 

CHANGELOG.md

@@ -5,6 +5,10 @@
 ### Fixes
 
 - Keep dropped tombstone and ANR events dropped, instead of reporting the same app exit again at every app start ([#6002](https://github.com/getsentry/sentry-java/pull/6002))
+- Apply `Sentry.withScope` and `Sentry.withIsolationScope` data to events captured inside the callback when `globalHubMode` is enabled ([#6004](https://github.com/getsentry/sentry-java/pull/6004))
+  - `globalHubMode` is enabled by default on Android, where tags, extras, contexts and level set inside the callback were silently dropped
+  - Scopes that are explicitly made current, e.g. via `Sentry.setCurrentScopes` or the `SentryContext` coroutine integration, are now also honoured when `globalHubMode` is enabled
+  - `Sentry.pushScope`, `Sentry.pushIsolationScope` and `Sentry.popScope` remain no-ops when `globalHubMode` is enabled
 
 ## 8.54.0
 

sentry-kotlin-extensions/src/test/java/io/sentry/kotlin/SentryContextTest.kt

@@ -11,18 +11,20 @@ import kotlin.test.assertNotEquals
 import kotlin.test.assertNotNull
 import kotlin.test.assertNull
 import kotlinx.coroutines.CoroutineName
+import kotlinx.coroutines.Dispatchers
 import kotlinx.coroutines.joinAll
 import kotlinx.coroutines.launch
 import kotlinx.coroutines.runBlocking
 
 class SentryContextTest {
-  // TODO [HSM] In global hub mode SentryContext behaves differently
-  // because Sentry.getCurrentScopes always returns rootScopes
-  // What's the desired behaviour?
+  // SentryContext makes its scopes current, which is honoured in global hub mode as well, so it
+  // isolates the same way there. Only implicit forking stays suppressed, see the last two tests.
+
+  private val dsn = "https://key@sentry.io/123"
 
   @BeforeTest
   fun init() {
-    initForTest("https://key@sentry.io/123")
+    initForTest(dsn)
   }
 
   @AfterTest
@@ -264,6 +266,46 @@ class SentryContextTest {
     assertEquals(initialContextElement, mergedContextElement)
   }
 
+  @Test
+  fun `current scope is isolated between coroutines in global hub mode`() = runBlocking {
+    initForTest({ it.dsn = dsn }, true)
+    Sentry.configureScope(ScopeType.CURRENT) { scope -> scope.setTag("parent", "parentValue") }
+
+    val c1 =
+      launch(SentryContext()) {
+        Sentry.configureScope(ScopeType.CURRENT) { scope -> scope.setTag("c1", "c1value") }
+        assertEquals("c1value", getTag("c1", ScopeType.CURRENT))
+        assertEquals("parentValue", getTag("parent", ScopeType.CURRENT))
+        assertNull(getTag("c2", ScopeType.CURRENT))
+      }
+    val c2 =
+      launch(SentryContext()) {
+        Sentry.configureScope(ScopeType.CURRENT) { scope -> scope.setTag("c2", "c2value") }
+        assertEquals("c2value", getTag("c2", ScopeType.CURRENT))
+        assertEquals("parentValue", getTag("parent", ScopeType.CURRENT))
+        assertNull(getTag("c1", ScopeType.CURRENT))
+      }
+    listOf(c1, c2).joinAll()
+
+    assertNotNull(getTag("parent", ScopeType.CURRENT))
+    assertNull(getTag("c1", ScopeType.CURRENT))
+    assertNull(getTag("c2", ScopeType.CURRENT))
+  }
+
+  @Test
+  fun `coroutines without SentryContext share the root scopes in global hub mode`() = runBlocking {
+    initForTest({ it.dsn = dsn }, true)
+    Sentry.configureScope(ScopeType.CURRENT) { scope -> scope.setTag("parent", "parentValue") }
+
+    launch(Dispatchers.Default) {
+        assertEquals("parentValue", getTag("parent", ScopeType.CURRENT))
+        Sentry.configureScope(ScopeType.CURRENT) { scope -> scope.setTag("child", "childValue") }
+      }
+      .join()
+
+    assertEquals("childValue", getTag("child", ScopeType.CURRENT))
+  }
+
   private fun getTag(tag: String, scopeType: ScopeType = ScopeType.ISOLATION): String? {
     var value: String? = null
     Sentry.configureScope(scopeType) { value = it.tags[tag] }

sentry-spring-7/src/test/kotlin/io/sentry/spring7/webflux/SentryWebfluxIntegrationTest.kt

@@ -127,11 +127,14 @@ class SentryWebfluxIntegrationTest {
       .expectStatus()
       .isOk
 
-    verify(transport)
-      .send(
-        checkTransaction { event -> assertEquals("GET /hello", event.transaction) },
-        anyOrNull(),
-      )
+    // the transaction is finished asynchronously, after the response has been sent
+    await.untilAsserted {
+      verify(transport)
+        .send(
+          checkTransaction { event -> assertEquals("GET /hello", event.transaction) },
+          anyOrNull(),
+        )
+    }
   }
 }
 

sentry-spring/src/test/kotlin/io/sentry/spring/webflux/SentryWebfluxIntegrationTest.kt

@@ -127,11 +127,14 @@ class SentryWebfluxIntegrationTest {
       .expectStatus()
       .isOk
 
-    verify(transport)
-      .send(
-        checkTransaction { event -> assertEquals("GET /hello", event.transaction) },
-        anyOrNull(),
-      )
+    // the transaction is finished asynchronously, after the response has been sent
+    await.untilAsserted {
+      verify(transport)
+        .send(
+          checkTransaction { event -> assertEquals("GET /hello", event.transaction) },
+          anyOrNull(),
+        )
+    }
   }
 }
 

sentry/src/main/java/io/sentry/Sentry.java

@@ -114,15 +114,24 @@ private Sentry() {}
   @ApiStatus.Internal
   @SuppressWarnings("deprecation")
   public static @NotNull IScopes getCurrentScopes(final boolean ensureForked) {
+    // read the volatile rootScopes once, so a concurrent Sentry.init cannot make the check below
+    // disagree with what we return
+    final @NotNull IScopes root = rootScopes;
+    @Nullable IScopes scopes = getScopesStorage().get();
     if (globalHubMode) {
-      return rootScopes;
+      // in global hub mode we never fork implicitly, but scopes that have explicitly been made
+      // current (e.g. by withScope) must still be honoured. Anything that did not originate from
+      // the present rootScopes is stale (SDK closed or re-initialized) and gets ignored.
+      if (scopes != null && !scopes.isNoOp() && root.isAncestorOf(scopes)) {
+        return scopes;
+      }
+      return root;
     }
-    @Nullable IScopes scopes = getScopesStorage().get();
     if (scopes == null || scopes.isNoOp()) {
       if (!ensureForked) {
         return NoOpScopes.getInstance();
       } else {
-        scopes = rootScopes.forkedScopes("getCurrentScopes");
+        scopes = root.forkedScopes("getCurrentScopes");
         getScopesStorage().set(scopes);
       }
     }
@@ -1060,7 +1069,13 @@ public static void removeExtra(final @Nullable String key) {
     return getCurrentScopes().getLastEventId();
   }
 
-  /** Pushes a new scope while inheriting the current scope's data. */
+  /**
+   * Pushes a new scope while inheriting the current scope's data.
+   *
+   * <p>This is a no-op in global hub mode, as the caller may never pop the scope again, or pop it
+   * on a different thread, which would corrupt the globally shared scopes. Use {@link
+   * Sentry#withScope(ScopeCallback)} if you need forking that also works in global hub mode.
+   */
   public static @NotNull ISentryLifecycleToken pushScope() {
     // pushScope is no-op in global hub mode
     if (!globalHubMode) {
@@ -1069,7 +1084,12 @@ public static void removeExtra(final @Nullable String key) {
     return NoOpScopesLifecycleToken.getInstance();
   }
 
-  /** Pushes a new isolation and current scope while inheriting the current scope's data. */
+  /**
+   * Pushes a new isolation and current scope while inheriting the current scope's data.
+   *
+   * <p>This is a no-op in global hub mode, for the same reason as {@link Sentry#pushScope()}. Use
+   * {@link Sentry#withIsolationScope(ScopeCallback)} instead.
+   */
   public static @NotNull ISentryLifecycleToken pushIsolationScope() {
     // pushScope is no-op in global hub mode
     if (!globalHubMode) {
@@ -1093,7 +1113,10 @@ public static void popScope() {
   }
 
   /**
-   * Runs the callback with a new current scope which gets dropped at the end
+   * Runs the callback with a new current scope which gets dropped at the end.
+   *
+   * <p>Unlike {@link Sentry#pushScope()} this also forks in global hub mode, since the previous
+   * scopes are always restored once the callback returns.
    *
    * @param callback the callback
    */
@@ -1105,6 +1128,9 @@ public static void withScope(final @NotNull ScopeCallback callback) {
    * Runs the callback with a new isolation scope which gets dropped at the end. Current scope is
    * also forked.
    *
+   * <p>Unlike {@link Sentry#pushIsolationScope()} this also forks in global hub mode, since the
+   * previous scopes are always restored once the callback returns.
+   *
    * @param callback the callback
    */
   public static void withIsolationScope(final @NotNull ScopeCallback callback) {

sentry/src/test/java/io/sentry/SentryTest.kt

@@ -1,5 +1,6 @@
 package io.sentry
 
+import com.google.common.truth.Truth.assertThat
 import io.sentry.SentryFeedbackOptions.IFormHandler
 import io.sentry.SentryOptions.ProfilesSamplerCallback
 import io.sentry.SentryOptions.TracesSamplerCallback
@@ -1434,6 +1435,93 @@ class SentryTest {
     assertNotSame(s1, s2)
   }
 
+  private fun initCapturingEvents(globalHubMode: Boolean): MutableList<SentryEvent> {
+    val events = mutableListOf<SentryEvent>()
+    initForTest(
+      { o ->
+        o.dsn = dsn
+        o.beforeSend = SentryOptions.BeforeSendCallback { event, _ ->
+          events.add(event)
+          null
+        }
+      },
+      globalHubMode,
+    )
+    return events
+  }
+
+  @Test
+  fun `withScope data is applied to events captured inside the callback`() {
+    for (globalHubMode in listOf(false, true)) {
+      val events = initCapturingEvents(globalHubMode)
+
+      Sentry.withScope { scope ->
+        scope.setTag("http-url", "https://example.com")
+        Sentry.captureException(RuntimeException("failed"))
+      }
+
+      assertThat(events.single().tags).containsEntry("http-url", "https://example.com")
+    }
+  }
+
+  @Test
+  fun `withIsolationScope data is applied to events captured inside the callback`() {
+    for (globalHubMode in listOf(false, true)) {
+      val events = initCapturingEvents(globalHubMode)
+
+      Sentry.withIsolationScope { scope ->
+        scope.setTag("http-url", "https://example.com")
+        Sentry.captureException(RuntimeException("failed"))
+      }
+
+      assertThat(events.single().tags).containsEntry("http-url", "https://example.com")
+    }
+  }
+
+  @Test
+  fun `withScope data does not leak into events captured after the callback`() {
+    for (globalHubMode in listOf(false, true)) {
+      val events = initCapturingEvents(globalHubMode)
+
+      Sentry.withScope { scope -> scope.setTag("http-url", "https://example.com") }
+      Sentry.captureException(RuntimeException("failed"))
+
+      assertThat(events.single().tags?.get("http-url")).isNull()
+    }
+  }
+
+  @Test
+  fun `in globalHubMode scopes are not forked for a thread that has none`() {
+    initForTest({ o -> o.dsn = dsn }, true)
+
+    val fromOtherThread = CompletableFuture.supplyAsync { Sentry.getCurrentScopes() }.get()
+
+    assertSame(Sentry.getCurrentScopes(), fromOtherThread)
+  }
+
+  @Test
+  fun `in globalHubMode scopes left over from a closed SDK are ignored`() {
+    initForTest({ o -> o.dsn = dsn }, true)
+    val stale = Sentry.forkedCurrentScope("stale")
+
+    Sentry.close()
+    // close only clears the storage of the calling thread, other threads may still hold stale ones
+    Sentry.setCurrentScopes(stale)
+
+    assertTrue(Sentry.getCurrentScopes().isNoOp)
+  }
+
+  @Test
+  fun `in globalHubMode scopes left over from a previous init are ignored`() {
+    initForTest({ o -> o.dsn = dsn }, true)
+    val stale = Sentry.forkedCurrentScope("stale")
+
+    initForTest({ o -> o.dsn = dsn }, true)
+    Sentry.setCurrentScopes(stale)
+
+    assertNotSame(stale, Sentry.getCurrentScopes())
+  }
+
   @Test
   fun `startProfiler starts the continuous profiler`() {
     val profiler = mock<IContinuousProfiler>()