Delete cached error reports if an Exception is thrown during disk IO, preventing delivery of empty/partial reports on the next app launch

CHANGELOG.md

@@ -2,6 +2,9 @@
 
 ## TBD
 
+* Delete cached error reports if an Exception is thrown during disk IO, preventing delivery of empty/partial reports on the next app launch.
+  [#609](https://github.com/bugsnag/bugsnag-android/pull/609)
+
 * Prevent internal error reporting of FileNotFoundException during serialization
   [#605](https://github.com/bugsnag/bugsnag-android/pull/605)
 

bugsnag-android-core/src/androidTest/java/com/bugsnag/android/FileStoreTest.kt

@@ -0,0 +1,73 @@
+package com.bugsnag.android
+
+import android.content.Context
+import androidx.test.core.app.ApplicationProvider
+import org.junit.Assert.assertEquals
+import org.junit.Assert.assertTrue
+import org.junit.Test
+import java.io.File
+import java.io.FileNotFoundException
+import java.lang.Exception
+import java.lang.RuntimeException
+
+class FileStoreTest {
+    val appContext = ApplicationProvider.getApplicationContext<Context>()
+    val config = Configuration("api-key")
+
+    @Test
+    fun sendsInternalErrorReport() {
+        val delegate = CustomDelegate()
+        val dir = File(appContext.filesDir, "custom-store")
+        dir.mkdir()
+
+        val store = CustomFileStore(config, appContext, dir.absolutePath, 1, null, delegate)
+        val exc = RuntimeException("Whoops")
+        store.write(CustomStreamable(exc))
+
+        assertEquals("Crash report serialization", delegate.context)
+        assertEquals(File(dir, "foo.json"), delegate.errorFile)
+        assertEquals(exc, delegate.exception)
+        assertEquals(0, dir.listFiles().size)
+    }
+
+    @Test
+    fun sendsInternalErrorReportNdk() {
+        val delegate = CustomDelegate()
+        val dir = File(appContext.filesDir, "custom-store")
+        dir.mkdir()
+
+        val store = CustomFileStore(config, appContext, "", 1, null, delegate)
+        store.enqueueContentForDelivery("foo")
+
+        assertEquals("NDK Crash report copy", delegate.context)
+        assertEquals(File("/foo.json"), delegate.errorFile)
+        assertTrue(delegate.exception is FileNotFoundException)
+    }
+}
+
+class CustomDelegate: FileStore.Delegate {
+    var exception: Exception? = null
+    var errorFile: File? = null
+    var context: String? = null
+
+    override fun onErrorIOFailure(exception: Exception?, errorFile: File?, context: String?) {
+        this.exception = exception
+        this.errorFile = errorFile
+        this.context = context
+    }
+}
+
+class CustomStreamable(val exc: Throwable) : JsonStream.Streamable {
+    override fun toStream(stream: JsonStream) = throw exc
+}
+
+internal class CustomFileStore(
+    config: Configuration,
+    appContext: Context,
+    val folder: String?,
+    maxStoreCount: Int,
+    comparator: java.util.Comparator<File>?,
+    delegate: Delegate?
+) : FileStore<CustomStreamable>(config, appContext, folder, maxStoreCount, comparator, delegate) {
+    override fun getFilename(`object`: Any?) = "$folder/foo.json"
+}

bugsnag-android-core/src/main/java/com/bugsnag/android/FileStore.java

@@ -84,9 +84,13 @@ void enqueueContentForDelivery(String content) {
             out = new BufferedWriter(new OutputStreamWriter(fos, "UTF-8"));
             out.write(content);
         } catch (Exception exc) {
+            File errorFile = new File(filename);
+
             if (delegate != null) {
-                delegate.onErrorIOFailure(exc, new File(filename), "NDK Crash report copy");
+                delegate.onErrorIOFailure(exc, errorFile, "NDK Crash report copy");
             }
+
+            IOUtils.deleteFile(errorFile);
         } finally {
             try {
                 if (out != null) {
@@ -121,9 +125,13 @@ String write(@NonNull JsonStream.Streamable streamable) {
         } catch (FileNotFoundException exc) {
             Logger.warn("Ignoring FileNotFoundException - unable to create file", exc);
         } catch (Exception exc) {
+            File errorFile = new File(filename);
+
             if (delegate != null) {
-                delegate.onErrorIOFailure(exc, new File(filename), "Crash report serialization");
+                delegate.onErrorIOFailure(exc, errorFile, "Crash report serialization");
             }
+
+            IOUtils.deleteFile(errorFile);
         } finally {
             IOUtils.closeQuietly(stream);
             lock.unlock();

bugsnag-android-core/src/main/java/com/bugsnag/android/IOUtils.java

@@ -4,6 +4,7 @@
 import androidx.annotation.Nullable;
 
 import java.io.Closeable;
+import java.io.File;
 import java.io.IOException;
 import java.io.Reader;
 import java.io.Writer;
@@ -47,4 +48,14 @@ public static int copy(@NonNull final Reader input,
 
         return (int) count;
     }
+
+    static void deleteFile(File file) {
+        try {
+            if (!file.delete()) {
+                file.deleteOnExit();
+            }
+        } catch (Exception ex) {
+            Logger.warn("Failed to delete file", ex);
+        }
+    }
 }

features/internal_report.feature → features/cached_error_reports.feature

@@ -1,4 +1,10 @@
-Feature: Sending internal error reports
+Feature: Cached Error Reports
+
+Scenario: If an empty file is in the cache directory then zero requests should be made
+    When I run "EmptyReportScenario"
+    And I configure the app to run in the "non-crashy" state
+    And I relaunch the app
+    Then I should receive 0 requests
 
 @skip_above_android_7
 Scenario: Sending internal error reports on API <26
@@ -66,3 +72,9 @@ Scenario: Sending internal error reports with cache tombstone + groups enabled
     And the event "metaData.BugsnagDiagnostics.fileLength" equals 4
     And the event "metaData.BugsnagDiagnostics.cacheGroup" is true
     And the event "metaData.BugsnagDiagnostics.cacheTombstone" is true
+
+Scenario: If a file in the cache directory is deleted before a request completes, zero further requests should be made
+    When I run "DeletedReportScenario"
+    And I configure the app to run in the "non-crashy" state
+    And I relaunch the app
+    Then I should receive 0 requests

features/cached_session_reports.feature

@@ -0,0 +1,19 @@
+Feature: Cached Session Reports
+
+Scenario: If an empty file is in the cache directory then zero requests should be made
+    When I run "EmptySessionScenario"
+    And I configure the app to run in the "non-crashy" state
+    And I relaunch the app
+    Then I should receive 0 requests
+
+Scenario: Sending internal error reports on API <26
+    When I run "PartialSessionScenario"
+    And I configure the app to run in the "non-crashy" state
+    And I relaunch the app
+    Then I should receive 0 requests
+
+Scenario: If a file in the cache directory is deleted before a request completes, zero further requests should be made
+    When I run "DeletedSessionScenario"
+    And I configure the app to run in the "non-crashy" state
+    And I relaunch the app
+    Then I should receive 0 requests

features/fixtures/mazerunner/src/main/java/com/bugsnag/android/mazerunner/scenarios/DeletedReportScenario.kt

@@ -0,0 +1,58 @@
+package com.bugsnag.android.mazerunner.scenarios
+
+import android.app.Activity
+import android.content.Context
+import android.content.Intent
+import android.os.Build
+import android.os.storage.StorageManager
+import android.util.Log
+
+import com.bugsnag.android.*
+import com.bugsnag.android.Configuration
+import java.io.File
+
+internal class DeletedReportScenario(config: Configuration,
+                                     context: Context) : Scenario(config, context) {
+
+    init {
+        config.setAutoCaptureSessions(false)
+
+        if (context is Activity) {
+            eventMetaData = context.intent.getStringExtra("EVENT_METADATA")
+
+            if (eventMetaData != "non-crashy") {
+                disableAllDelivery(config)
+            } else {
+                val ctor = Class.forName("com.bugsnag.android.DefaultDelivery").declaredConstructors[0]
+                ctor.isAccessible = true
+                val baseDelivery = ctor.newInstance(null) as Delivery
+                val errDir = File(context.cacheDir, "bugsnag-errors")
+
+                config.delivery = object: Delivery {
+                    override fun deliver(payload: SessionTrackingPayload, config: Configuration) {
+                        baseDelivery.deliver(payload, config)
+                    }
+
+                    override fun deliver(report: Report, config: Configuration) {
+                        // delete files before they can be delivered
+                        val files = errDir.listFiles()
+                        files.forEach {
+                            Log.d("Bugsnag", "Deleting file: ${it.delete()}")
+                        }
+
+                        Log.d("Bugsnag", "Files available " + errDir.listFiles()[0].exists())
+                        baseDelivery.deliver(report, config)
+                    }
+                }
+            }
+        }
+    }
+
+    override fun run() {
+        super.run()
+
+        if (eventMetaData != "non-crashy") {
+            Bugsnag.notify(java.lang.RuntimeException("Whoops"))
+        }
+    }
+}

features/fixtures/mazerunner/src/main/java/com/bugsnag/android/mazerunner/scenarios/DeletedSessionScenario.kt

@@ -0,0 +1,67 @@
+package com.bugsnag.android.mazerunner.scenarios
+
+import android.app.Activity
+import android.content.Context
+import android.content.Intent
+import android.os.Build
+import android.os.Handler
+import android.os.HandlerThread
+import android.os.storage.StorageManager
+import android.util.Log
+
+import com.bugsnag.android.*
+import com.bugsnag.android.Configuration
+import java.io.File
+
+internal class DeletedSessionScenario(config: Configuration,
+                                      context: Context) : Scenario(config, context) {
+
+    init {
+        config.setAutoCaptureSessions(false)
+
+        if (context is Activity) {
+            eventMetaData = context.intent.getStringExtra("EVENT_METADATA")
+
+            if (eventMetaData != "non-crashy") {
+                disableAllDelivery(config)
+            } else {
+                val ctor = Class.forName("com.bugsnag.android.DefaultDelivery").declaredConstructors[0]
+                ctor.isAccessible = true
+                val baseDelivery = ctor.newInstance(null) as Delivery
+                val errDir = File(context.cacheDir, "bugsnag-sessions")
+
+                config.delivery = object: Delivery {
+                    override fun deliver(payload: SessionTrackingPayload, config: Configuration) {
+                        // delete files before they can be delivered
+                        val files = errDir.listFiles()
+                        files.forEach {
+                            Log.d("Bugsnag", "Deleting file: ${it.delete()}")
+                        }
+
+                        Log.d("Bugsnag", "Files available " + errDir.listFiles()[0].exists())
+                        baseDelivery.deliver(payload, config)
+                    }
+
+                    override fun deliver(report: Report, config: Configuration) {
+                        baseDelivery.deliver(report, config)
+                    }
+                }
+            }
+        }
+    }
+
+    override fun run() {
+        super.run()
+
+        if (eventMetaData != "non-crashy") {
+            Bugsnag.startSession()
+        }
+
+        val thread = HandlerThread("HandlerThread")
+        thread.start()
+
+        Handler(thread.looper).post {
+            flushAllSessions()
+        }
+    }
+}

features/fixtures/mazerunner/src/main/java/com/bugsnag/android/mazerunner/scenarios/EmptyReportScenario.kt

@@ -1,21 +1,40 @@
 package com.bugsnag.android.mazerunner.scenarios
 
+import android.app.Activity
 import android.content.Context
+import android.content.Intent
+import android.os.Build
+import android.os.storage.StorageManager
+
 import com.bugsnag.android.Bugsnag
 import com.bugsnag.android.Configuration
 import java.io.File
 
-/**
- * Verifies that if a report is empty and a beforeSend callback is set,
- * minimal information is still sent to bugsnag.
- */
 internal class EmptyReportScenario(config: Configuration,
                                    context: Context) : Scenario(config, context) {
 
     init {
         config.setAutoCaptureSessions(false)
         config.beforeSend { true }
-        val files = File(context.cacheDir, "bugsnag-errors").listFiles()
-        files.forEach { it.writeText("") }
+
+        if (context is Activity) {
+            eventMetaData = context.intent.getStringExtra("EVENT_METADATA")
+            val errDir = File(context.cacheDir, "bugsnag-errors")
+
+            if (eventMetaData != "non-crashy") {
+                disableAllDelivery(config)
+            } else {
+                val files = errDir.listFiles()
+                files.forEach { it.writeText("") }
+            }
+        }
+    }
+
+    override fun run() {
+        super.run()
+
+        if (eventMetaData != "non-crashy") {
+            Bugsnag.notify(java.lang.RuntimeException("Whoops"))
+        }
     }
 }