Do not report cached events as lost

CHANGELOG.md

@@ -26,6 +26,8 @@
   ```
 - Fix abstract method error in `SentrySupportSQLiteDatabase` ([#4597](https://github.com/getsentry/sentry-java/pull/4597))
 - Ensure frame metrics listeners are registered/unregistered on the main thread ([#4582](https://github.com/getsentry/sentry-java/pull/4582))
+- Do not report cached events as lost ([#4575](https://github.com/getsentry/sentry-java/pull/4575))
+  - Previously events were recorded as lost early despite being retried later through the cache
 
 ## 8.18.0
 

sentry-android-core/api/sentry-android-core.api

@@ -483,6 +483,7 @@ public final class io/sentry/android/core/cache/AndroidEnvelopeCache : io/sentry
 	public static fun hasStartupCrashMarker (Lio/sentry/SentryOptions;)Z
 	public static fun lastReportedAnr (Lio/sentry/SentryOptions;)Ljava/lang/Long;
 	public fun store (Lio/sentry/SentryEnvelope;Lio/sentry/Hint;)V
+	public fun storeEnvelope (Lio/sentry/SentryEnvelope;Lio/sentry/Hint;)Z
 }
 
 public class io/sentry/android/core/performance/ActivityLifecycleCallbacksAdapter : android/app/Application$ActivityLifecycleCallbacks {

sentry-android-core/src/main/java/io/sentry/android/core/cache/AndroidEnvelopeCache.java

@@ -47,9 +47,19 @@ public AndroidEnvelopeCache(final @NotNull SentryAndroidOptions options) {
     this.currentDateProvider = currentDateProvider;
   }
 
+  @SuppressWarnings("deprecation")
   @Override
   public void store(@NotNull SentryEnvelope envelope, @NotNull Hint hint) {
-    super.store(envelope, hint);
+    storeInternalAndroid(envelope, hint);
+  }
+
+  @Override
+  public boolean storeEnvelope(@NotNull SentryEnvelope envelope, @NotNull Hint hint) {
+    return storeInternalAndroid(envelope, hint);
+  }
+
+  private boolean storeInternalAndroid(@NotNull SentryEnvelope envelope, @NotNull Hint hint) {
+    final boolean didStore = super.storeEnvelope(envelope, hint);
 
     final SentryAndroidOptions options = (SentryAndroidOptions) this.options;
     final TimeSpan sdkInitTimeSpan = AppStartMetrics.getInstance().getSdkInitTimeSpan();
@@ -83,6 +93,7 @@ public void store(@NotNull SentryEnvelope envelope, @NotNull Hint hint) {
 
           writeLastReportedAnrMarker(timestamp);
         });
+    return didStore;
   }
 
   @TestOnly

sentry-android-core/src/test/java/io/sentry/android/core/cache/AndroidEnvelopeCacheTest.kt

@@ -1,7 +1,9 @@
 package io.sentry.android.core.cache
 
+import io.sentry.ISerializer
 import io.sentry.NoOpLogger
 import io.sentry.SentryEnvelope
+import io.sentry.SentryOptions
 import io.sentry.UncaughtExceptionHandlerIntegration.UncaughtExceptionHint
 import io.sentry.android.core.AnrV2Integration.AnrV2Hint
 import io.sentry.android.core.SentryAndroidOptions
@@ -18,7 +20,9 @@ import kotlin.test.assertFalse
 import kotlin.test.assertTrue
 import org.junit.Rule
 import org.junit.rules.TemporaryFolder
+import org.mockito.kotlin.any
 import org.mockito.kotlin.mock
+import org.mockito.kotlin.same
 import org.mockito.kotlin.whenever
 
 class AndroidEnvelopeCacheTest {
@@ -35,8 +39,10 @@ class AndroidEnvelopeCacheTest {
       dir: TemporaryFolder,
       appStartMillis: Long? = null,
       currentTimeMillis: Long? = null,
+      optionsCallback: ((SentryOptions) -> Unit)? = null,
     ): AndroidEnvelopeCache {
       options.cacheDirPath = dir.newFolder("sentry-cache").absolutePath
+      optionsCallback?.invoke(options)
       val outboxDir = File(options.outboxPath!!)
       outboxDir.mkdirs()
 
@@ -82,7 +88,7 @@ class AndroidEnvelopeCacheTest {
     val cache = fixture.getSut(tmpDir)
 
     val hints = HintUtils.createWithTypeCheckHint(UncaughtHint())
-    cache.store(fixture.envelope, hints)
+    cache.storeEnvelope(fixture.envelope, hints)
 
     assertFalse(fixture.startupCrashMarkerFile.exists())
   }
@@ -92,7 +98,7 @@ class AndroidEnvelopeCacheTest {
     val cache = fixture.getSut(dir = tmpDir, appStartMillis = 1000L, currentTimeMillis = 5000L)
 
     val hints = HintUtils.createWithTypeCheckHint(UncaughtHint())
-    cache.store(fixture.envelope, hints)
+    cache.storeEnvelope(fixture.envelope, hints)
 
     assertFalse(fixture.startupCrashMarkerFile.exists())
   }
@@ -104,7 +110,7 @@ class AndroidEnvelopeCacheTest {
     fixture.options.cacheDirPath = null
 
     val hints = HintUtils.createWithTypeCheckHint(UncaughtHint())
-    cache.store(fixture.envelope, hints)
+    cache.storeEnvelope(fixture.envelope, hints)
 
     assertFalse(fixture.startupCrashMarkerFile.exists())
   }
@@ -114,7 +120,7 @@ class AndroidEnvelopeCacheTest {
     val cache = fixture.getSut(dir = tmpDir, appStartMillis = 1000L, currentTimeMillis = 2000L)
 
     val hints = HintUtils.createWithTypeCheckHint(UncaughtHint())
-    cache.store(fixture.envelope, hints)
+    cache.storeEnvelope(fixture.envelope, hints)
 
     assertTrue(fixture.startupCrashMarkerFile.exists())
   }
@@ -138,7 +144,7 @@ class AndroidEnvelopeCacheTest {
       HintUtils.createWithTypeCheckHint(
         AnrV2Hint(0, NoOpLogger.getInstance(), 12345678L, false, false)
       )
-    cache.store(fixture.envelope, hints)
+    cache.storeEnvelope(fixture.envelope, hints)
 
     assertFalse(fixture.lastReportedAnrFile.exists())
   }
@@ -151,7 +157,7 @@ class AndroidEnvelopeCacheTest {
       HintUtils.createWithTypeCheckHint(
         AnrV2Hint(0, NoOpLogger.getInstance(), 12345678L, false, false)
       )
-    cache.store(fixture.envelope, hints)
+    cache.storeEnvelope(fixture.envelope, hints)
 
     assertTrue(fixture.lastReportedAnrFile.exists())
     assertEquals("12345678", fixture.lastReportedAnrFile.readText())
@@ -189,5 +195,17 @@ class AndroidEnvelopeCacheTest {
     assertEquals(87654321L, lastReportedAnr)
   }
 
+  @Test
+  fun `returns false if storing fails`() {
+    val serializer = mock<ISerializer>()
+    val cache = fixture.getSut(tmpDir) { options -> options.setSerializer(serializer) }
+    whenever(serializer.serialize(same(fixture.envelope), any()))
+      .thenThrow(RuntimeException("forced ex"))
+    val hints = HintUtils.createWithTypeCheckHint(UncaughtHint())
+
+    val didStore = cache.storeEnvelope(fixture.envelope, hints)
+    assertFalse(didStore)
+  }
+
   internal class UncaughtHint : UncaughtExceptionHint(0, NoOpLogger.getInstance())
 }

sentry/api/sentry.api

@@ -4354,13 +4354,15 @@ public class io/sentry/cache/EnvelopeCache : io/sentry/cache/IEnvelopeCache {
 	public static fun getPreviousSessionFile (Ljava/lang/String;)Ljava/io/File;
 	public fun iterator ()Ljava/util/Iterator;
 	public fun store (Lio/sentry/SentryEnvelope;Lio/sentry/Hint;)V
+	public fun storeEnvelope (Lio/sentry/SentryEnvelope;Lio/sentry/Hint;)Z
 	public fun waitPreviousSessionFlush ()Z
 }
 
 public abstract interface class io/sentry/cache/IEnvelopeCache : java/lang/Iterable {
 	public abstract fun discard (Lio/sentry/SentryEnvelope;)V
 	public fun store (Lio/sentry/SentryEnvelope;)V
 	public abstract fun store (Lio/sentry/SentryEnvelope;Lio/sentry/Hint;)V
+	public fun storeEnvelope (Lio/sentry/SentryEnvelope;Lio/sentry/Hint;)Z
 }
 
 public final class io/sentry/cache/PersistingOptionsObserver : io/sentry/IOptionsObserver {
@@ -6663,6 +6665,7 @@ public final class io/sentry/transport/NoOpEnvelopeCache : io/sentry/cache/IEnve
 	public static fun getInstance ()Lio/sentry/transport/NoOpEnvelopeCache;
 	public fun iterator ()Ljava/util/Iterator;
 	public fun store (Lio/sentry/SentryEnvelope;Lio/sentry/Hint;)V
+	public fun storeEnvelope (Lio/sentry/SentryEnvelope;Lio/sentry/Hint;)Z
 }
 
 public final class io/sentry/transport/NoOpTransport : io/sentry/transport/ITransport {

sentry/src/main/java/io/sentry/cache/EnvelopeCache.java

@@ -93,8 +93,18 @@ public EnvelopeCache(
     previousSessionLatch = new CountDownLatch(1);
   }
 
+  @SuppressWarnings("deprecation")
   @Override
   public void store(final @NotNull SentryEnvelope envelope, final @NotNull Hint hint) {
+    storeInternal(envelope, hint);
+  }
+
+  @Override
+  public boolean storeEnvelope(final @NotNull SentryEnvelope envelope, final @NotNull Hint hint) {
+    return storeInternal(envelope, hint);
+  }
+
+  private boolean storeInternal(final @NotNull SentryEnvelope envelope, final @NotNull Hint hint) {
     Objects.requireNonNull(envelope, "Envelope is required.");
 
     rotateCacheIfNeeded(allEnvelopeFiles());
@@ -171,19 +181,20 @@ public void store(final @NotNull SentryEnvelope envelope, final @NotNull Hint hi
               WARNING,
               "Not adding Envelope to offline storage because it already exists: %s",
               envelopeFile.getAbsolutePath());
-      return;
+      return true;
     } else {
       options
           .getLogger()
           .log(DEBUG, "Adding Envelope to offline storage: %s", envelopeFile.getAbsolutePath());
     }
 
-    writeEnvelopeToDisk(envelopeFile, envelope);
+    final boolean didWriteToDisk = writeEnvelopeToDisk(envelopeFile, envelope);
 
     // write file to the disk when its about to crash so crashedLastRun can be marked on restart
     if (HintUtils.hasType(hint, UncaughtExceptionHandlerIntegration.UncaughtExceptionHint.class)) {
       writeCrashMarkerFile();
     }
+    return didWriteToDisk;
   }
 
   /**
@@ -295,7 +306,7 @@ private void updateCurrentSession(
     }
   }
 
-  private void writeEnvelopeToDisk(
+  private boolean writeEnvelopeToDisk(
       final @NotNull File file, final @NotNull SentryEnvelope envelope) {
     if (file.exists()) {
       options
@@ -312,7 +323,9 @@ private void writeEnvelopeToDisk(
       options
           .getLogger()
           .log(ERROR, e, "Error writing Envelope %s to offline storage", file.getAbsolutePath());
+      return false;
     }
+    return true;
   }
 
   private void writeSessionToDisk(final @NotNull File file, final @NotNull Session session) {

sentry/src/main/java/io/sentry/cache/IEnvelopeCache.java

@@ -6,10 +6,17 @@
 
 public interface IEnvelopeCache extends Iterable<SentryEnvelope> {
 
+  @Deprecated
   void store(@NotNull SentryEnvelope envelope, @NotNull Hint hint);
 
+  default boolean storeEnvelope(@NotNull SentryEnvelope envelope, @NotNull Hint hint) {
+    store(envelope, hint);
+    return true;
+  }
+
+  @Deprecated
   default void store(@NotNull SentryEnvelope envelope) {
-    store(envelope, new Hint());
+    storeEnvelope(envelope, new Hint());
   }
 
   void discard(@NotNull SentryEnvelope envelope);

sentry/src/main/java/io/sentry/transport/AsyncHttpTransport.java

@@ -139,7 +139,7 @@ private static QueuedThreadPoolExecutor initExecutor(
             final EnvelopeSender envelopeSender = (EnvelopeSender) r;
 
             if (!HintUtils.hasType(envelopeSender.hint, Cached.class)) {
-              envelopeCache.store(envelopeSender.envelope, envelopeSender.hint);
+              envelopeCache.storeEnvelope(envelopeSender.envelope, envelopeSender.hint);
             }
 
             markHintWhenSendingFailed(envelopeSender.hint, true);
@@ -271,7 +271,7 @@ public void run() {
       TransportResult result = this.failedResult;
 
       envelope.getHeader().setSentAt(null);
-      envelopeCache.store(envelope, hint);
+      boolean cached = envelopeCache.storeEnvelope(envelope, hint);
 
       HintUtils.runIfHasType(
           hint,
@@ -311,14 +311,17 @@ public void run() {
 
             // ignore e.g. 429 as we're not the ones actively dropping
             if (result.getResponseCode() >= 400 && result.getResponseCode() != 429) {
-              HintUtils.runIfDoesNotHaveType(
-                  hint,
-                  Retryable.class,
-                  (hint) -> {
-                    options
-                        .getClientReportRecorder()
-                        .recordLostEnvelope(DiscardReason.NETWORK_ERROR, envelopeWithClientReport);
-                  });
+              if (!cached) {
+                HintUtils.runIfDoesNotHaveType(
+                    hint,
+                    Retryable.class,
+                    (hint) -> {
+                      options
+                          .getClientReportRecorder()
+                          .recordLostEnvelope(
+                              DiscardReason.NETWORK_ERROR, envelopeWithClientReport);
+                    });
+              }
             }
 
             throw new IllegalStateException(message);
@@ -332,10 +335,12 @@ public void run() {
                 retryable.setRetry(true);
               },
               (hint, clazz) -> {
-                LogUtils.logNotInstanceOf(clazz, hint, options.getLogger());
-                options
-                    .getClientReportRecorder()
-                    .recordLostEnvelope(DiscardReason.NETWORK_ERROR, envelopeWithClientReport);
+                if (!cached) {
+                  LogUtils.logNotInstanceOf(clazz, hint, options.getLogger());
+                  options
+                      .getClientReportRecorder()
+                      .recordLostEnvelope(DiscardReason.NETWORK_ERROR, envelopeWithClientReport);
+                }
               });
           throw new IllegalStateException("Sending the event failed.", e);
         }
@@ -348,10 +353,12 @@ public void run() {
               retryable.setRetry(true);
             },
             (hint, clazz) -> {
-              LogUtils.logNotInstanceOf(clazz, hint, options.getLogger());
-              options
-                  .getClientReportRecorder()
-                  .recordLostEnvelope(DiscardReason.NETWORK_ERROR, envelope);
+              if (!cached) {
+                LogUtils.logNotInstanceOf(clazz, hint, options.getLogger());
+                options
+                    .getClientReportRecorder()
+                    .recordLostEnvelope(DiscardReason.NETWORK_ERROR, envelope);
+              }
             });
       }
       return result;

sentry/src/main/java/io/sentry/transport/NoOpEnvelopeCache.java

@@ -14,9 +14,15 @@ public static NoOpEnvelopeCache getInstance() {
     return instance;
   }
 
+  @SuppressWarnings("deprecation")
   @Override
   public void store(@NotNull SentryEnvelope envelope, @NotNull Hint hint) {}
 
+  @Override
+  public boolean storeEnvelope(@NotNull SentryEnvelope envelope, @NotNull Hint hint) {
+    return false;
+  }
+
   @Override
   public void discard(@NotNull SentryEnvelope envelope) {}