From 35e5520291f491f41c725949203537c15d3b7656 Mon Sep 17 00:00:00 2001
From: Martin Stjernholm <mast@google.com>
Date: Mon, 18 Nov 2024 12:16:59 +0000
Subject: Don't reuse the old snapshot after a long running bg dexopt to do the
 file cleanup.

In particular this avoids deleting the dexopt artifacts for any new
apps installed since the background dexopt started. It may still
incorrectly delete files for new installs during the cleanup run, but
that time window is much shorter (on the order of 5-10 secs, vs 5-20
mins for a whole background dexopt run).

The dexopt and the cleanup are two independent operations, so they
don't need to use the same snapshot.

Test: atest ArtServiceTests
Bug: 378773852
Change-Id: I75c8f5b4934fb86facdd8d31ae18aab63426a346
---
 .../com/android/server/art/BackgroundDexoptJob.java   | 19 ++++++++++++-------
 .../android/server/art/BackgroundDexoptJobTest.java   | 12 +++++++-----
 2 files changed, 19 insertions(+), 12 deletions(-)

(limited to 'libartservice')

diff --git a/libartservice/service/java/com/android/server/art/BackgroundDexoptJob.java b/libartservice/service/java/com/android/server/art/BackgroundDexoptJob.java
index 526a08eb48..330940f9d5 100644
--- a/libartservice/service/java/com/android/server/art/BackgroundDexoptJob.java
+++ b/libartservice/service/java/com/android/server/art/BackgroundDexoptJob.java
@@ -234,14 +234,19 @@ public class BackgroundDexoptJob implements ArtServiceJobInterface {
             dexoptResultByPass = mInjector.getArtManagerLocal().dexoptPackages(snapshot,
                     ReasonMapping.REASON_BG_DEXOPT, cancellationSignal, Runnable::run,
                     progressCallbacks);
+        }
 
-            // For simplicity, we don't support cancelling the following operation in the middle.
-            // This is fine because it typically takes only a few seconds.
-            if (!cancellationSignal.isCanceled()) {
-                // We do the cleanup after dexopt so that it doesn't affect the `getSizeBeforeBytes`
-                // field in the result that we send to callbacks. Admittedly, this will cause us to
-                // lose some chance to dexopt when the storage is very low, but it's fine because we
-                // can still dexopt in the next run.
+        // For simplicity, we don't support cancelling the following operation in the middle.
+        // This is fine because it typically takes only a few seconds.
+        if (!cancellationSignal.isCanceled()) {
+            // We do the cleanup after dexopt so that it doesn't affect the `getSizeBeforeBytes`
+            // field in the result that we send to callbacks. Admittedly, this will cause us to
+            // lose some chance to dexopt when the storage is very low, but it's fine because we
+            // can still dexopt in the next run.
+            //
+            // Take a new snapshot since the one used for dexoptPackages above is old by now and
+            // new packages may have been installed.
+            try (var snapshot = mInjector.getPackageManagerLocal().withFilteredSnapshot()) {
                 long freedBytes = mInjector.getArtManagerLocal().cleanup(snapshot);
                 AsLog.i(String.format("Freed %d bytes", freedBytes));
             }
diff --git a/libartservice/service/javatests/com/android/server/art/BackgroundDexoptJobTest.java b/libartservice/service/javatests/com/android/server/art/BackgroundDexoptJobTest.java
index 85ae079c77..93c2aed7eb 100644
--- a/libartservice/service/javatests/com/android/server/art/BackgroundDexoptJobTest.java
+++ b/libartservice/service/javatests/com/android/server/art/BackgroundDexoptJobTest.java
@@ -79,7 +79,7 @@ public class BackgroundDexoptJobTest {
     @Mock private BackgroundDexoptJob.Injector mInjector;
     @Mock private ArtManagerLocal mArtManagerLocal;
     @Mock private PackageManagerLocal mPackageManagerLocal;
-    @Mock private PackageManagerLocal.FilteredSnapshot mSnapshot;
+    @Mock private PackageManagerLocal.FilteredSnapshot mSnapshot1, mSnapshot2;
     @Mock private JobScheduler mJobScheduler;
     @Mock private BackgroundDexoptJobService mJobService;
     @Mock private JobParameters mJobParameters;
@@ -94,8 +94,6 @@ public class BackgroundDexoptJobTest {
                 .when(SystemProperties.getBoolean(eq("pm.dexopt.disable_bg_dexopt"), anyBoolean()))
                 .thenReturn(false);
 
-        lenient().when(mPackageManagerLocal.withFilteredSnapshot()).thenReturn(mSnapshot);
-
         mConfig = new Config();
 
         lenient().when(mInjector.getArtManagerLocal()).thenReturn(mArtManagerLocal);
@@ -123,15 +121,19 @@ public class BackgroundDexoptJobTest {
 
     @Test
     public void testStart() {
+        when(mPackageManagerLocal.withFilteredSnapshot())
+                .thenReturn(mSnapshot1)
+                .thenReturn(mSnapshot2);
+
         when(mArtManagerLocal.dexoptPackages(
-                     same(mSnapshot), eq(ReasonMapping.REASON_BG_DEXOPT), any(), any(), any()))
+                     same(mSnapshot1), eq(ReasonMapping.REASON_BG_DEXOPT), any(), any(), any()))
                 .thenReturn(mDexoptResultByPass);
 
         Result result = Utils.getFuture(mBackgroundDexoptJob.start());
         assertThat(result).isInstanceOf(CompletedResult.class);
         assertThat(((CompletedResult) result).dexoptResultByPass()).isEqualTo(mDexoptResultByPass);
 
-        verify(mArtManagerLocal).cleanup(same(mSnapshot));
+        verify(mArtManagerLocal).cleanup(same(mSnapshot2));
     }
 
     @Test
-- 
cgit v1.2.3-59-g8ed1b