Wait for the heap analysis instead of deleting the heap dump - #2920
Open
pyricau wants to merge 4 commits into
Open
Wait for the heap analysis instead of deleting the heap dump#2920pyricau wants to merge 4 commits into
pyricau wants to merge 4 commits into
Conversation
LeakCanary keeps at most 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 queued for any of them. An analysis runs on WorkManager, which survives process death and reboots, so it can start minutes or days after the heap dump was taken, find nothing to read, and fail with "Hprof file ... missing". That's #2790, and #1670 before it. A heap dump no stored analysis was run on is still waiting for its analysis, so the cleanup now deletes the ones a stored analysis was already run on first, and only takes a waiting one when there's no analyzed heap dump left. The limit stays a hard limit: a device dumping the heap faster than it can analyze it would otherwise fill up the disk. "Was this heap dump analyzed" has to survive process death, so it comes from a queryable heap_dump_file_path column on heap_analysis rather than from any separate bookkeeping that could drift from the analyses themselves. Analyses stored before this version get the column backfilled from their serialized blob, or the heap dumps they analyzed would look like they're still waiting. The other half of #2790 is what the failure says when the file really is gone. The reason came from two in-memory lists on LeakDirectoryProvider, which are empty in every process other than the one that did the deleting -- and a queued analysis outliving the deletion is exactly the case where the process was killed and restarted, so the failure read "deleted because: unknown" for precisely the people who report it. Deletions now go to a heap_dump_deletion table, trimmed to its 100 most recent rows on insert so it can't grow without bound, written by the maxStoredHeapDumps cleanup and by both UI delete paths. No record means LeakCanary didn't delete it, and the directory is private to the app, so the failure says what that leaves instead of blaming an unknown deletion. Two things fell out of deleteAll along the way: it no longer deserializes every analysis blob just to find the heap dump file path, and it now deletes rows whose blob fails to deserialize, which it used to leave behind. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Keeping the heap dumps that are still waiting for an analysis, which the previous commit does, only helps while there is an analyzed heap dump left to delete instead. Nothing bounded how many could pile up: dumping the heap was rate limited to one a minute and was otherwise blind to how many analyses were already in flight, so on a device where the analysis takes longer than that, LeakCanary dumped the heap again while the first analysis was still running, and again behind that. That's #2770, and it's also what makes the retention cleanup delete a heap dump an analysis still needs. So LeakCanary now waits: it doesn't dump the heap while any stored heap dump has no stored analysis. Tapping the notification still dumps the heap right away, since an explicit user request isn't something this gate should refuse. That gate turns one dropped analysis into a heap dump that would stop LeakCanary from ever dumping the heap again, so two things have to close it off. Dropped analyses are dispatched again when the app becomes visible, by sending a HeapDump event rather than by requeueing on WorkManager, because BackgroundThreadHeapAnalyzer is what LeakCanary falls back to when androidx.work isn't in the classpath and has no persisted queue to resume from. Re-dispatching is free when the analysis was never dropped: the WorkManager analyzers enqueue under a name unique to the heap dump with ExistingWorkPolicy.KEEP, and runAnalysisBlocking dispatches the analysis it finds already stored rather than parsing the heap dump a second time. And LeakCanary gives up eventually. The heap_dump table, which the previous commit added as heap_dump_deletion, now also counts how many analyses of each heap dump have started, so a heap dump whose analysis reliably kills the process stores a failure after 3 starts that never finished rather than being retried forever. Counting starts rather than dispatches is what makes a KEEP no-op and a slow analysis cost nothing. Giving up stores an analysis rather than deleting the file, because storing one is what ends the pendency and lets the retention cap delete that heap dump as analyzed, while the file stays on disk to be shared for a bug report. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Dispatching every heap dump that has no analysis handed WorkManager several analyses at once, and its default executor is a pool of 2 to 4 threads, so they ran in parallel: several parsed heaps in memory racing each other for CPU, which is the thing waiting for the analysis was added to prevent. Only the oldest is dispatched now, and the next one goes on the next check, once that one is no longer waiting. The KDoc claimed WorkManager has nothing to re-run an interrupted analysis with, which is wrong: killing the :leakcanary process mid parse on an emulator showed WorkManager reset the still RUNNING work and re-run the same work id a minute later. What it doesn't re-run is work it considers failed, which is what a dead :leakcanary process looks like to a main process that survives to commit it, and it isn't there at all when androidx.work isn't in the classpath, which is the default: it's a compileOnly dependency, so leakcanary-android-sample resolves to BackgroundThreadHeapAnalyzer, a bare background thread that loses the analysis with the process. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Heap dumps waiting for an analysis are dispatched one at a time, so something has to dispatch the next one, and until now the only things that did were the next retained object check and the next process becoming visible. That left the analyses to trickle out a minute apart at best, and not at all while no object was retained, with automated heap dumping gated the whole time. Every analyzer funnels its done event through InternalLeakCanary.sendEvent in the main process, including the remote one, whose HeapAnalysisDoneDispatchWorker exists to do exactly that. So that's where the trigger now hears that an analysis is over and dispatches the next heap dump waiting for one. Dumping the heap stays blocked until nothing is waiting for an analysis at all, not just until the next analysis is dispatched. Beyond keeping two parsed heaps from racing each other for CPU, that's what keeps the index Shark builds of the heap dump being analyzed out of the next heap dump: it's one of the largest things in the heap while an analysis runs, so dumping then would produce a bigger heap dump, slower to analyze, whose leak traces are full of LeakCanary's own objects. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #2790, #2770, and #1670 before them.
Both halves of the "HEAP ANALYSIS FAILED —
Hprof file … missing, deleted because: unknown" report in #2790 are about a heap dump that isn't there when its analysis runs. The first half was where heap dumps were stored, fixed by #2918. This is the other half: LeakCanary deleting a heap dump whose analysis hadn't run yet, not stopping those from piling up in the first place, and then not being able to say what happened.LeakCanary deleted heap dumps its own queued analyses still needed
LeakDirectoryProvider.cleanupOldHeapDumps()keeps at mostLeakCanary.Config.maxStoredHeapDumpsheap dumps, 7 by default, and enforced that by deleting the oldest files every time it created a new heap dump file, with nothing checking whether an analysis was still queued for any of them. An analysis is queued on WorkManager, which survives process death and reboots, so it can start minutes or days after the heap dump was taken, find nothing to read, and fail. Deleting a single analysis from the LeakCanary UI, and the clear-all, could take a queued analysis's heap dump out from under it the same way.A heap dump that no stored analysis was run on is still waiting for its analysis, so the cleanup now deletes the heap dumps that were already analyzed first, oldest first, and only takes one that's still waiting when there's no analyzed heap dump left to delete. The limit stays a hard limit: when every stored heap dump is waiting the oldest one goes anyway, and the failure then names
LeakCanary.Config.maxStoredHeapDumpsas the knob to raise, which is a much better thing to read than a file that vanished for no reason.maxStoredHeapDumpsand itsConfigproperty are unchanged."Was this heap dump analyzed" has to survive process death, so it comes from a queryable
heap_dump_file_pathcolumn onheap_analysis— the same derived-column pattern that table already uses forleak_countandexception_summary— rather than from separate lifecycle bookkeeping that could drift from the analyses themselves. Analyses stored before this version get the column backfilled from their serialized blob during the migration, or the heap dumps they already analyzed would look like they're still waiting and would be the last thing the cleanup deletes.Nothing stopped heap dumps from piling up faster than they could be analyzed
Ordering the cleanup is only half of it. Dumping the heap was rate limited to one a minute by an in-memory
lastHeapDumpUptimeMillisand was otherwise blind to how many analyses were already in flight, so on a device where the analysis takes longer than a minute LeakCanary would take a second heap dump while the first was still being analyzed, and a third behind that — each analysis holding a parsed heap in memory and racing the others for CPU. That's #2770, and #1670 is the same thing seen from the other end: the pile-up is what makes the cleanup delete a heap dump an analysis still needs, and no ordering rule can help once every stored heap dump is waiting.So LeakCanary now waits:
checkRetainedObjects()doesn't dump the heap while any stored heap dump has no stored analysis, and re-checks a minute later. The retained objects notification says "Waiting for the previous heap dump to be analyzed" while that's the case, and tapping it still dumps the heap right away —onDumpHeapReceivedgoes straight todumpHeap, so an explicit user request is not something this gate is entitled to refuse.To be clear about what this is and isn't: it's a policy on dumping, not a lock on analyzing. Since a heap dump only stops being pending once its analysis is stored, the steady state is one heap dump and one analysis at a time. What can still overlap is what deliberately bypasses the gate — a heap dump you ask for by tapping the notification or calling
LeakCanary.dumpHeap(), and importing an hprof from the LeakCanary UI.Waiting is only safe if the wait always ends
A gate on "an analysis is still pending" hands a new failure mode to anything that can drop an analysis: one heap dump nobody will ever analyze would stop LeakCanary from ever dumping the heap again. Two things close that off.
A heap dump with no analysis has one dispatched again. WorkManager does more here than I first assumed, and it's worth being precise about what's left for LeakCanary to do. I killed the
:leakcanaryprocess mid-parse on an emulator: the main process loggedWork [...RemoteHeapAnalyzerWorker] failed because it threw an exception/error: java.lang.RuntimeException: Binder died, which is WorkManager's terminal failure path, but because the main process was dying at the same moment that state never committed, and the restarted process reset the still-RUNNINGwork and re-ran the same work id 51 seconds later, successfully. So an analysis interrupted by process death is re-run by WorkManager itself, and for those users this resume is a backstop rather than the mechanism.What it isn't a backstop for:
leakcanary-android-coredeclaresandroidx.workcompileOnly, so unless the app depends on WorkManager itself the analyzer isBackgroundThreadHeapAnalyzer, a barepostto aHandlerThreadwith nothing persisted and nothing to resume from.samples:leakcanary-android-sampleis exactly this: noandroidx.workanywhere indebugRuntimeClasspath, and the device log confirmsBackgroundThreadHeapAnalyzerin the resolved config. Any process death loses that analysis permanently.:leakcanaryprocess looks like to a main process that survives to commit it — theBinder diedpath above.Result.failure()is terminal in WorkManager; it is never retried.eventListenersanalyzers, and apps that cancel all their WorkManager work.So the resume is analyzer-agnostic: it dispatches a
HeapDumpevent, which reaches whichever analyzer is configured. It runs when the app becomes visible, once per process, because whether an earlier process died mid-analysis is settled before this one starts, and on the visible transition rather than at startup so it stays out of app startup and out of the:leakcanaryprocess. Only the oldest waiting heap dump is dispatched: dispatching all of them would hand WorkManager several analyses at once for its 2-to-4-thread pool, which is the parallel analysis this whole gate exists to prevent. The next one goes on the next check, so several heap dumps left behind by repeated deaths drain one at a time rather than all at once.Re-dispatching has to be free when the analysis was never dropped at all, and it is, in two layers. The WorkManager analyzers now enqueue under a name unique to the heap dump file with
ExistingWorkPolicy.KEEP, so WorkManager keeps the work it already has. AndrunAnalysisBlockingis now idempotent for every analyzer: if an analysis of that heap dump is already stored, it dispatches that stored analysis instead of parsing the heap dump again and storing a second copy of the same result.And LeakCanary gives up eventually. A heap dump whose analysis reliably gets the process killed — the out-of-memory reports on the tracker are full of these — would otherwise be re-dispatched forever, and the gate would mean the heap is never dumped again. So a
heap_dumptable counts how many analyses of each heap dump have started: after 3 starts with nothing ever finishing, the 4th stores aHeapAnalysisFailuresaying so. Counting starts rather than dispatches is what makes aKEEPno-op and a legitimately slow analysis cost nothing, and a cancelled analysis decrements the count, since being asked to stop isn't a failed attempt. Giving up stores an analysis rather than deleting the file: storing one is what ends the pendency, so the gate opens and the retention cap is free to delete that heap dump as analyzed, while the file itself stays on disk for as long as the cap keeps it, because a heap dump that kills the analyzer is exactly what a bug report needs.The failure now says why the file went away
The reason came from
LeakDirectoryProvider.hprofDeleteReason(file), which consulted two in-memorymutableListOf<String>()companion fields and otherwise returnedunknown. Those lists are empty in every process other than the one that did the deleting, and a queued analysis outliving the deletion is exactly the case where the process was killed and restarted — so the failure read "deleted because: unknown" for precisely the people who report it. The clear-all didn't add to either list at all. #1670 sketched a shared-preferences marker for this and called the approach convoluted; the database LeakCanary already has is a better home.Deletions now go to the
heap_dumptable alongside the analysis start count, written by themaxStoredHeapDumpscleanup, by deleting one analysis from the UI, and by the clear-all, each with the reason spelled out in the message the failure will carry. It's bounded: writing a row trims the table to its 100 most recent heap dumps in the same transaction, because an analysis queued behind that many heap dumps is never going to run. A reason is only recorded whenFile.delete()actually returned true, so a failed delete can't overwrite the correct reason for a deletion that already happened.When there's no record, the failure says so and says what that leaves — heap dumps live in a directory private to the app, so either the app's data was cleared or something else in the app deleted the file — instead of asserting an unknown LeakCanary deletion.
Before:
After, when the retention cleanup took it:
After, when LeakCanary didn't delete it:
Two things that fell out of
deleteAllBoth in code this change had to touch anyway: it no longer deserializes every analysis blob just to find the heap dump file path, and it now deletes rows whose blob fails to deserialize, which it used to silently leave behind.
Testing
20 new instrumentation tests in
leakcanary-android-core, all against the real database.HeapDumpRetentionTestcovers the cleanup ordering (analyzed heap dumps go before waiting ones, the oldest waiting one goes when none were analyzed, nothing goes while under the limit), which heap dumps count as waiting, each recorded reason, and that the record stays bounded when 300 deletions are inserted.RepeatedHeapAnalysisTestcovers the resume and everything that has to make it safe: a waiting heap dump is dispatched again on the visible transition and an analyzed one isn't, only the oldest of several waiting ones is dispatched, asking again returns the analysis that was already stored rather than storing a second one, a heap dump is given up on after 3 starts that never finished while one that completes never is, a cancelled analysis doesn't count towards giving up, and the file survives being given up on. It runs on real heap dumps built with thedump { }DSL, and — sinceandroidx.workisn't on the androidTest classpath — it exercises theBackgroundThreadHeapAnalyzerconfiguration, which is the one with nothing to resume from.MissingHeapDumpFailureTestdrivesAndroidDebugHeapAnalyzer.runAnalysisBlockingon a heap dump that isn't there and asserts the failure names a recorded reason, and says it has no record when there is none.DatabaseMigrationTestgains a case asserting the v24 database's one stored analysis is known to have been analyzed after the migration../gradlew build detektis green on JDK 17 with noapi/*.apichange, and the 49 instrumentation tests pass on API 34 and on API 24, the minSdk. (The 48 that predate the one-at-a-time test also passed on API 36 and API 26.)Verified end to end on an emulator with
samples:leakcanary-android-process-sample, readingleaks.dboff the device between steps.First, the whole flow: a leak triggered a heap dump, whose analysis ran in the
:leakcanaryprocess and wroteheap_dump_file_pathcross-process, and once nothing was waiting the retention cap deleted the oldest heap dumps it had already analyzed and recorded that reason, never one that was waiting.Then the orphan path, with three heap dumps staged into
no_backup/leakcanarywith distinct modification times to stand in for what a process that died mid-analysis leaves behind. After a force-stop and relaunch, the log shows exactly one dispatch — the oldest — and thenWaiting for the analysis of [10-00, 10-10, 10-20] to finish before dumping the heap againwhen a new leak asked for a heap dump. The oldest was dispatched twice (once on the visible transition, once on the retained-objects check) andanalysis_start_countstill ended at 1, soKEEPplus the already-stored short circuit really do make the second ask free. Two more leak cycles drained the other two in order, one per cycle, each reachinganalysis_start_count = 1with exactly one stored analysis. Draining is deliberately lazy like this: it's driven by the visible transition and by the retained-objects check, so leftover heap dumps are analyzed one per cycle rather than all at once.🤖 Generated with Claude Code