Wait for the heap analysis instead of deleting the heap dump

docs/changelog.md

@@ -25,6 +25,11 @@ Releases before 2.8.1 predate these markers.
     * 🔀 Heap dumps taken by `leakcanary-android-test` and `leakcanary-android-instrumentation` move from the files directory of the app under test to its no backup directory, so they aren't backed up either. Heap dumps taken by `leakcanary-android-uiautomator` stay in `/data/local/tmp`: they're written by `am dumpheap` running as the shell user, which can't write to an app's private directory.
     * 🔀 `adb pull /sdcard/Download/leakcanary-com.example/…` doesn't work anymore. Share the heap dump from the LeakCanary UI, or run `adb exec-out run-as com.example cat no_backup/leakcanary/<name>.hprof > dump.hprof`.
     * 🔀 Heap dumps written by an earlier version of LeakCanary are left where they are. LeakCanary only cleans up the directory it writes to, and on API 30 and above it couldn't delete the ones it no longer owns anyway. Delete the `Download/leakcanary-com.example` directory by hand to reclaim that space.
+* 🐛 [#2790](https://github.com/square/leakcanary/issues/2790) [#2770](https://github.com/square/leakcanary/issues/2770) [#1670](https://github.com/square/leakcanary/issues/1670) LeakCanary deleted heap dumps whose analysis hadn't run yet, and the analysis then failed with *"Hprof file … missing"*. It keeps at most `LeakCanary.Config.maxStoredHeapDumps` heap dumps, 7 by default, and enforced that by deleting the oldest ones every time it created a new heap dump file, with nothing checking whether an analysis was still waiting for any of them. Nothing stopped those from piling up either: dumping the heap was rate limited to one a minute and was otherwise blind to how many analyses were already in flight, so a device dumping the heap faster than it could analyze it deleted heap dumps out from under its own queued analyses. LeakCanary now waits for the analysis instead: it doesn't dump the heap while an earlier heap dump is still waiting for one, and when the cleanup does have to delete something it takes the heap dumps that were already analyzed first, oldest first, leaving the ones still waiting for last.
+    * 🔀 One heap dump and one analysis at a time, so heap dumps are further apart on a device where the analysis is slow. Waiting until every stored heap dump has been analyzed also keeps LeakCanary from dumping the heap while an analysis is holding its index of another heap dump in memory, which would make the new heap dump bigger, slower to analyze, and full of LeakCanary's own objects. While LeakCanary is waiting, the retained objects notification says so, and tapping it dumps the heap right away as it always did.
+    * 🔀 A heap dump that has no stored analysis has its analysis dispatched again the next time the app becomes visible. Otherwise it stays a heap dump that nothing will ever read, and since LeakCanary now waits for a pending analysis before dumping the heap again, that one heap dump would stop it from ever dumping the heap again. WorkManager already re-runs an analysis that was interrupted by the process dying, so this is about what it doesn't cover: an analysis it considers failed rather than interrupted, which is what a dead `:leakcanary` process looks like to the main process, and WorkManager not being there at all — `leakcanary-android` declares it `compileOnly`, so unless your app depends on `androidx.work` itself, the analyzer is `BackgroundThreadHeapAnalyzer`, a plain background thread with nothing persisted and nothing to resume from. Only the oldest waiting heap dump is dispatched, and the next one goes as soon as that analysis is done, so several heap dumps left behind are analyzed back to back rather than in parallel. Asking again for an analysis that did complete gives back the analysis that was already stored instead of analyzing the same heap dump twice.
+    * 🔀 LeakCanary gives up on a heap dump once 3 analyses of it have started and none have finished, which is what happens when parsing that heap dump gets the process killed, and stores a failure saying so rather than retrying it forever and never dumping the heap again. The heap dump file is left in place, so it can still be shared for a bug report.
+* 🔨 [#2790](https://github.com/square/leakcanary/issues/2790) A heap analysis that finds its heap dump file gone now says why the file went away, including when LeakCanary deleted it in an earlier process. The reason used to come from two in-memory lists, which are empty in every process other than the one that did the deleting — and a queued analysis outliving the deletion is precisely the case where the process was killed and restarted, so the failure read *"deleted because: unknown"* for exactly the people who report it. Deletions are now recorded in LeakCanary's database, which outlives the process, and cover the `maxStoredHeapDumps` cleanup, deleting one analysis from the LeakCanary UI, and deleting all of them. When there's no record, the failure says so and says what that leaves — the app's data being cleared, or something else in the app deleting the file — instead of claiming LeakCanary deleted it for an unknown reason.
 * ⚠️ The *"Render Heap Dump"* screen in the LeakCanary UI is gone, along with its *"Generate HQ Bitmap"* action. The action rendered the heap dump to a PNG in the public `Download` folder, gated on the `WRITE_EXTERNAL_STORAGE` permission — which apps targeting Android 11 and above can't be granted, so since 2020 tapping it only ever showed a toast asking for a permission that would have bought nothing.
 * 💥 [#2789](https://github.com/square/leakcanary/issues/2789) [#2773](https://github.com/square/leakcanary/issues/2773) The heap analysis of a large heap failed with an `OutOfMemoryError` while growing the set of objects the path finding traversal has already visited. That set was keyed by object id, so it held every reachable object in an 8 byte per slot hash table at a 0.75 load factor, sized from a guess (`instanceCount / 2`) that is always too small: on the Android heap dumps in our test resources the traversal ends up visiting 0.65x to 1.08x of `instanceCount`. Growing doubles the table and rehashes into it while the old one is still referenced, so the moment of growth needs 1.5x the new table — on a heap of 4 million objects, a 33.6 MB table and a 67.1 MB one live at the same time, in an app capped at a 512 MB growth limit. The visited set is now one bit per object in the heap dump, keyed by the object's index rather than by its id, allocated once at a size that's known upfront, so it can't grow and can't rehash: 509 KB rather than a 100.7 MB peak on that 4 million object heap. On a 4.4 million object Android heap dump, the smallest heap the analysis completes in goes from 513 MB to 385 MB.
     * 🔀 Mapping an object id to its index is a binary search where a hash lookup used to do, and the traversal does that once per reference it reads, so the analysis is about 2% slower end to end (4% to 8% of the path finding step) on the Android heap dumps in our test resources.

leakcanary/leakcanary-android-core/src/androidTest/java/leakcanary/DatabaseMigrationTest.kt

@@ -51,6 +51,19 @@ class DatabaseMigrationTest {
     }
   }
 
+  /**
+   * The retention cleanup keeps heap dumps that no stored analysis was run on, so an analysis stored
+   * before the heap dump file path column existed has to be backfilled, or the heap dump it already
+   * analyzed would look like it's still waiting for an analysis.
+   */
+  @Test fun v24_heap_dumps_are_known_to_have_been_analyzed() {
+    DB_V24 upgrade {
+      val analyzedFilePaths = HeapAnalysisTable.retrieveAnalyzedHeapDumpFilePaths(this)
+
+      analyzedFilePaths.size assertEquals 1
+    }
+  }
+
   @Test fun v24_has_3_leak_types() {
     DB_V24 upgrade {
       LeakTable.retrieveAllLeaks(this).size assertEquals 3

leakcanary/leakcanary-android-core/src/androidTest/java/leakcanary/HeapDumpRetentionTest.kt

@@ -0,0 +1,188 @@
+package leakcanary
+
+import androidx.test.platform.app.InstrumentationRegistry
+import java.io.File
+import leakcanary.internal.LeakDirectoryProvider
+import leakcanary.internal.activity.db.HeapAnalysisTable
+import leakcanary.internal.activity.db.HeapDumpTable
+import leakcanary.internal.activity.db.ScopedLeaksDb
+import org.assertj.core.api.Assertions.assertThat
+import org.junit.After
+import org.junit.Before
+import org.junit.Rule
+import org.junit.Test
+import shark.HeapAnalysis
+import shark.HeapAnalysisException
+import shark.HeapAnalysisFailure
+
+/**
+ * [LeakDirectoryProvider] deletes heap dumps to keep at most `maxStoredHeapDumps` of them. A heap
+ * dump whose analysis hasn't run yet has to survive that, because that analysis can run long after
+ * the process that created the heap dump is gone.
+ */
+internal class HeapDumpRetentionTest {
+
+  @get:Rule
+  var databaseRule = DatabaseRule()
+
+  private val context
+    get() = InstrumentationRegistry.getInstrumentation().targetContext
+
+  private val heapDumpDirectory
+    get() = File(context.noBackupFilesDir, "leakcanary")
+
+  @Before @After fun emptyHeapDumpDirectory() {
+    heapDumpDirectory.listFiles()?.forEach { it.delete() }
+  }
+
+  @Test fun heap_dumps_under_the_limit_are_kept() {
+    val kept = writeHeapDump("kept.hprof", lastModifiedMillis = 1000)
+
+    newHeapDumpFile(maxStoredHeapDumps = 1)
+
+    assertThat(kept).exists()
+  }
+
+  @Test fun analyzed_heap_dumps_are_deleted_before_ones_waiting_for_analysis() {
+    val waiting = writeHeapDump("waiting.hprof", lastModifiedMillis = 1000)
+    val analyzed = writeHeapDump("analyzed.hprof", lastModifiedMillis = 2000)
+    markAnalyzed(analyzed)
+
+    newHeapDumpFile(maxStoredHeapDumps = 1)
+
+    assertThat(analyzed).doesNotExist()
+    assertThat(waiting).exists()
+  }
+
+  @Test fun oldest_heap_dump_waiting_for_analysis_is_deleted_when_none_were_analyzed() {
+    val oldest = writeHeapDump("oldest.hprof", lastModifiedMillis = 1000)
+    val newest = writeHeapDump("newest.hprof", lastModifiedMillis = 2000)
+
+    newHeapDumpFile(maxStoredHeapDumps = 1)
+
+    assertThat(oldest).doesNotExist()
+    assertThat(newest).exists()
+  }
+
+  @Test fun heap_dumps_with_no_analysis_are_the_ones_waiting_for_one() {
+    val analyzed = writeHeapDump("analyzed.hprof", lastModifiedMillis = 1000)
+    markAnalyzed(analyzed)
+    val newerWaiting = writeHeapDump("newer-waiting.hprof", lastModifiedMillis = 3000)
+    val olderWaiting = writeHeapDump("older-waiting.hprof", lastModifiedMillis = 2000)
+
+    val waitingForAnalysis = LeakDirectoryProvider(context) { 7 }
+      .heapDumpFilesWaitingForAnalysis()
+
+    assertThat(waitingForAnalysis).containsExactly(olderWaiting, newerWaiting)
+  }
+
+  @Test fun deleting_a_heap_dump_waiting_for_analysis_records_that_it_was_waiting() {
+    val waiting = writeHeapDump("waiting.hprof", lastModifiedMillis = 1000)
+    writeHeapDump("newer.hprof", lastModifiedMillis = 2000)
+
+    newHeapDumpFile(maxStoredHeapDumps = 1)
+
+    assertThat(deletionReason(waiting))
+      .contains("maxStoredHeapDumps limit of 1")
+      .contains("still waiting to be analyzed")
+  }
+
+  @Test fun deleting_an_analyzed_heap_dump_records_that_it_was_analyzed() {
+    val analyzed = writeHeapDump("analyzed.hprof", lastModifiedMillis = 1000)
+    markAnalyzed(analyzed)
+    writeHeapDump("newer.hprof", lastModifiedMillis = 2000)
+
+    newHeapDumpFile(maxStoredHeapDumps = 1)
+
+    assertThat(deletionReason(analyzed)).contains("the oldest one it had already analyzed")
+  }
+
+  @Test fun deleting_an_analysis_records_why_its_heap_dump_went_away() {
+    val heapDumpFile = writeHeapDump("analyzed.hprof", lastModifiedMillis = 1000)
+    val analysisId = markAnalyzed(heapDumpFile)
+
+    ScopedLeaksDb.writableDatabase(context) { db ->
+      HeapAnalysisTable.delete(db, analysisId, heapDumpFile)
+    }
+
+    assertThat(heapDumpFile).doesNotExist()
+    assertThat(deletionReason(heapDumpFile)).contains("deleted from the LeakCanary UI")
+  }
+
+  @Test fun deleting_all_analyses_records_why_their_heap_dumps_went_away() {
+    val heapDumpFile = writeHeapDump("analyzed.hprof", lastModifiedMillis = 1000)
+    markAnalyzed(heapDumpFile)
+
+    ScopedLeaksDb.writableDatabase(context) { db ->
+      HeapAnalysisTable.deleteAll(db)
+    }
+
+    assertThat(heapDumpFile).doesNotExist()
+    assertThat(deletionReason(heapDumpFile)).contains("All heap analyses were deleted")
+  }
+
+  @Test fun a_heap_dump_LeakCanary_never_deleted_has_no_recorded_reason() {
+    val neverDeleted = writeHeapDump("kept.hprof", lastModifiedMillis = 1000)
+
+    assertThat(rawDeletionReason(neverDeleted)).isNull()
+  }
+
+  @Test fun what_LeakCanary_records_about_heap_dumps_doesnt_grow_without_bound() {
+    val heapDumpCount = 300
+    ScopedLeaksDb.writableDatabase(context) { db ->
+      repeat(heapDumpCount) { index ->
+        HeapDumpTable.recordDeletion(db, File(heapDumpDirectory, "$index.hprof"), "Deleted.")
+      }
+      val rowCount = db.rawQuery("SELECT COUNT(*) FROM heap_dump", null).use { cursor ->
+        cursor.moveToNext()
+        cursor.getInt(0)
+      }
+      assertThat(rowCount).isLessThan(heapDumpCount)
+    }
+    assertThat(rawDeletionReason(File(heapDumpDirectory, "${heapDumpCount - 1}.hprof"))).isNotNull()
+    assertThat(rawDeletionReason(File(heapDumpDirectory, "0.hprof"))).isNull()
+  }
+
+  private fun newHeapDumpFile(maxStoredHeapDumps: Int) {
+    LeakDirectoryProvider(context) { maxStoredHeapDumps }.newHeapDumpFile()
+  }
+
+  private fun writeHeapDump(
+    name: String,
+    lastModifiedMillis: Long
+  ): File {
+    heapDumpDirectory.mkdirs()
+    val heapDumpFile = File(heapDumpDirectory, name)
+    heapDumpFile.writeText("Stands in for a heap dump: nothing here reads the contents.")
+    check(heapDumpFile.setLastModified(lastModifiedMillis)) {
+      "Could not set the last modified time of $heapDumpFile"
+    }
+    return heapDumpFile
+  }
+
+  /**
+   * Stores an analysis of [heapDumpFile], which is what makes it a heap dump that isn't waiting for
+   * one anymore. A failure is enough: the analysis ran, so the heap dump has been read.
+   */
+  private fun markAnalyzed(heapDumpFile: File): Long {
+    val analysis: HeapAnalysis = HeapAnalysisFailure(
+      heapDumpFile = heapDumpFile,
+      createdAtTimeMillis = 42,
+      analysisDurationMillis = 10,
+      exception = HeapAnalysisException(RuntimeException("Boom"))
+    )
+    return ScopedLeaksDb.writableDatabase(context) { db ->
+      HeapAnalysisTable.insert(db, analysis)
+    }
+  }
+
+  private fun deletionReason(heapDumpFile: File): String =
+    checkNotNull(rawDeletionReason(heapDumpFile)) {
+      "No recorded deletion for $heapDumpFile"
+    }
+
+  private fun rawDeletionReason(heapDumpFile: File): String? =
+    ScopedLeaksDb.readableDatabase(context) { db ->
+      HeapDumpTable.retrieveDeletionReason(db, heapDumpFile)
+    }
+}

leakcanary/leakcanary-android-core/src/androidTest/java/leakcanary/MissingHeapDumpFailureTest.kt

@@ -0,0 +1,75 @@
+package leakcanary
+
+import android.app.Application
+import androidx.test.platform.app.InstrumentationRegistry
+import java.io.File
+import leakcanary.EventListener.Event.HeapAnalysisDone.HeapAnalysisFailed
+import leakcanary.EventListener.Event.HeapDump
+import leakcanary.internal.AndroidDebugHeapAnalyzer
+import leakcanary.internal.activity.db.HeapDumpTable
+import leakcanary.internal.activity.db.ScopedLeaksDb
+import org.assertj.core.api.Assertions.assertThat
+import org.junit.Before
+import org.junit.Rule
+import org.junit.Test
+import shark.HeapAnalysisFailure
+
+/**
+ * An analysis is queued on WorkManager, so it can run in a later process than the one that dumped
+ * the heap, and find that the heap dump file is gone. What it can say about that comes from the
+ * database, which is what makes the answer survive the process that did the deleting.
+ */
+internal class MissingHeapDumpFailureTest {
+
+  @get:Rule
+  var databaseRule = DatabaseRule()
+
+  private val context
+    get() = InstrumentationRegistry.getInstrumentation().targetContext
+
+  @Before fun installAppWatcher() {
+    // AndroidDebugHeapAnalyzer reads InternalLeakCanary.application, which requires an install.
+    if (!AppWatcher.isInstalled) {
+      InstrumentationRegistry.getInstrumentation().runOnMainSync {
+        AppWatcher.manualInstall(
+          context.applicationContext as Application,
+          watchersToInstall = emptyList()
+        )
+      }
+    }
+  }
+
+  @Test fun failure_names_the_recorded_reason_LeakCanary_deleted_the_heap_dump() {
+    val heapDumpFile = File(context.noBackupFilesDir, "leakcanary/deleted.hprof")
+    ScopedLeaksDb.writableDatabase(context) { db ->
+      HeapDumpTable.recordDeletion(db, heapDumpFile, "A recorded reason.")
+    }
+
+    val failure = analyzeMissingHeapDump(heapDumpFile)
+
+    assertThat(failure.exception.cause).hasMessageContaining("A recorded reason.")
+  }
+
+  @Test fun failure_says_LeakCanary_has_no_record_of_deleting_the_heap_dump() {
+    val heapDumpFile = File(context.noBackupFilesDir, "leakcanary/vanished.hprof")
+
+    val failure = analyzeMissingHeapDump(heapDumpFile)
+
+    assertThat(failure.exception.cause).hasMessageContaining("no record of deleting it")
+  }
+
+  private fun analyzeMissingHeapDump(heapDumpFile: File): HeapAnalysisFailure {
+    check(!heapDumpFile.exists()) {
+      "$heapDumpFile should not exist"
+    }
+    val done = AndroidDebugHeapAnalyzer.runAnalysisBlocking(
+      HeapDump(
+        uniqueId = "unique-id",
+        file = heapDumpFile,
+        durationMillis = 10,
+        reason = "Testing a heap dump that isn't there"
+      )
+    ) { }
+    return (done as HeapAnalysisFailed).heapAnalysis
+  }
+}