2 files changed, 72 insertions, 12 deletions

diff --git a/services/backup/java/com/android/server/backup/keyvalue/KeyValueBackupTask.java b/services/backup/java/com/android/server/backup/keyvalue/KeyValueBackupTask.java
index 5e10916c4491..a4e58a1faeac 100644
--- a/services/backup/java/com/android/server/backup/keyvalue/KeyValueBackupTask.java
+++ b/services/backup/java/com/android/server/backup/keyvalue/KeyValueBackupTask.java
@@ -649,16 +649,33 @@ public class KeyValueBackupTask implements BackupRestoreTask, Runnable {
         mReporter.onStartPackageBackup(PM_PACKAGE);
         mCurrentPackage = new PackageInfo();
         mCurrentPackage.packageName = PM_PACKAGE;
-
         try {
-            extractPmAgentData(mCurrentPackage);
+            // If we can't even extractPmAgentData(), then we treat the local state as
+            // compromised, just in case. This means that we will clear data and will
+            // start from a clean slate in the next attempt. It's not clear whether that's
+            // the right thing to do, but matches what we have historically done.
+            try {
+                extractPmAgentData(mCurrentPackage);
+            } catch (TaskException e) {
+                throw TaskException.stateCompromised(e); // force stateCompromised
+            }
+            // During sendDataToTransport, we generally trust any thrown TaskException
+            // about whether stateCompromised because those are likely transient;
+            // clearing state for those would have the potential to lead to cascading
+            // failures, as discussed in http://b/144030477.
+            // For specific status codes (e.g. TRANSPORT_NON_INCREMENTAL_BACKUP_REQUIRED),
+            // cleanUpAgentForTransportStatus() or theoretically handleTransportStatus()
+            // still have the opportunity to perform additional clean-up tasks.
             int status = sendDataToTransport(mCurrentPackage);
             cleanUpAgentForTransportStatus(status);
         } catch (AgentException | TaskException e) {
             mReporter.onExtractPmAgentDataError(e);
             cleanUpAgentForError(e);
-            // PM agent failure is task failure.
-            throw TaskException.stateCompromised(e);
+            if (e instanceof TaskException) {
+                throw (TaskException) e;
+            } else {
+                throw TaskException.stateCompromised(e); // PM agent failure is task failure.
+            }
         }
     }
 
diff --git a/services/robotests/backup/src/com/android/server/backup/keyvalue/KeyValueBackupTaskTest.java b/services/robotests/backup/src/com/android/server/backup/keyvalue/KeyValueBackupTaskTest.java
index ec56e1ebc8e0..b5c9375fcc0d 100644
--- a/services/robotests/backup/src/com/android/server/backup/keyvalue/KeyValueBackupTaskTest.java
+++ b/services/robotests/backup/src/com/android/server/backup/keyvalue/KeyValueBackupTaskTest.java
@@ -122,6 +122,7 @@ import com.android.server.testing.shadows.ShadowBackupDataOutput;
 import com.android.server.testing.shadows.ShadowEventLog;
 import com.android.server.testing.shadows.ShadowSystemServiceRegistry;
 
+import com.google.common.base.Charsets;
 import com.google.common.truth.IterableSubject;
 
 import org.junit.After;
@@ -1910,7 +1911,8 @@ public class KeyValueBackupTaskTest  {
     }
 
     @Test
-    public void testRunTask_whenTransportReturnsError_updatesFilesAndCleansUp() throws Exception {
+    public void testRunTask_whenTransportReturnsErrorForGenericPackage_updatesFilesAndCleansUp()
+            throws Exception {
         TransportMock transportMock = setUpInitializedTransport(mTransport);
         when(transportMock.transport.performBackup(
                         argThat(packageInfo(PACKAGE_1)), any(), anyInt()))
@@ -1926,6 +1928,39 @@ public class KeyValueBackupTaskTest  {
         assertCleansUpFilesAndAgent(mTransport, PACKAGE_1);
     }
 
+    /**
+     * Checks that TRANSPORT_ERROR during @pm@ backup keeps the state file untouched.
+     * http://b/144030477
+     */
+    @Test
+    public void testRunTask_whenTransportReturnsErrorForPm_updatesFilesAndCleansUp()
+            throws Exception {
+        // TODO(tobiast): Refactor this method to share code with
+        //  testRunTask_whenTransportReturnsErrorForGenericPackage_updatesFilesAndCleansUp
+        // See patchset 7 of http://ag/11762961
+        final PackageData packageData = PM_PACKAGE;
+        TransportMock transportMock = setUpInitializedTransport(mTransport);
+        when(transportMock.transport.performBackup(
+                argThat(packageInfo(packageData)), any(), anyInt()))
+                .thenReturn(BackupTransport.TRANSPORT_ERROR);
+
+        byte[] pmStateBytes = "fake @pm@ state for testing".getBytes(Charsets.UTF_8);
+
+        Path pmStatePath = createPmStateFile(pmStateBytes.clone());
+        PackageManagerBackupAgent pmAgent = spy(createPmAgent());
+        KeyValueBackupTask task = createKeyValueBackupTask(transportMock, packageData);
+        runTask(task);
+        verify(pmAgent, never()).onBackup(any(), any(), any());
+
+        assertThat(Files.readAllBytes(pmStatePath)).isEqualTo(pmStateBytes.clone());
+
+        boolean existed = deletePmStateFile();
+        assertThat(existed).isTrue();
+        // unbindAgent() is skipped for @pm@. Comment in KeyValueBackupTask.java:
+        // "For PM metadata (for which applicationInfo is null) there is no agent-bound state."
+        assertCleansUpFiles(mTransport, packageData);
+    }
+
     @Test
     public void testRunTask_whenTransportGetBackupQuotaThrowsForPm() throws Exception {
         TransportMock transportMock = setUpInitializedTransport(mTransport);
@@ -2707,21 +2742,29 @@ public class KeyValueBackupTaskTest  {
      *       </ul>
      * </ul>
      */
-    private void createPmStateFile() throws IOException {
-        createPmStateFile(mTransport);
+    private Path createPmStateFile() throws IOException {
+        return createPmStateFile("pmState".getBytes());
+    }
+
+    private Path createPmStateFile(byte[] bytes) throws IOException {
+        return createPmStateFile(bytes, mTransport);
+    }
+
+    private Path createPmStateFile(TransportData transport) throws IOException {
+        return createPmStateFile("pmState".getBytes(), mTransport);
     }
 
-    /** @see #createPmStateFile() */
-    private void createPmStateFile(TransportData transport) throws IOException {
-        Files.write(getStateFile(transport, PM_PACKAGE), "pmState".getBytes());
+    /** @see #createPmStateFile(byte[]) */
+    private Path createPmStateFile(byte[] bytes, TransportData transport) throws IOException {
+        return Files.write(getStateFile(transport, PM_PACKAGE), bytes);
     }
 
     /**
      * Forces transport initialization and call to {@link
      * UserBackupManagerService#resetBackupState(File)}
      */
-    private void deletePmStateFile() throws IOException {
-        Files.deleteIfExists(getStateFile(mTransport, PM_PACKAGE));
+    private boolean deletePmStateFile() throws IOException {
+        return Files.deleteIfExists(getStateFile(mTransport, PM_PACKAGE));
     }
 
     /**