Use RemoteCall for quota exceeded in backups
For both key-value and full-backup. This makes the tasks wait for the
quota exceeded call, allowing the agent to complete before being torn
down (as described in bug). Also added a short time-out (3s) in case of
misbehaving agents.
Bug: 68762178
Bug: 110082831
Test: atest FrameworksServicesRoboTests
Test: 1. while true; do atest FullBackupQuotaTest; done
2. Stop after ~50 executions, verify all succeeded.
Test: 1. while true; do atest KeyValueQuotaTest; done
2. Stop after ~50 executions, verify all succeeded.
Test: CtsBackupTestCases
Change-Id: Ib582e75a4d317ab53a6df8cb261966a04ef085fb
diff --git a/core/java/android/app/IBackupAgent.aidl b/core/java/android/app/IBackupAgent.aidl
index 4517446..d3d7c68 100644
--- a/core/java/android/app/IBackupAgent.aidl
+++ b/core/java/android/app/IBackupAgent.aidl
@@ -48,8 +48,7 @@
* be echoed back to the backup service binder once the new
* data has been written to the data and newState files.
*
- * @param callbackBinder Binder on which to indicate operation completion,
- * passed here as a convenience to the agent.
+ * @param callbackBinder Binder on which to indicate operation completion.
*
* @param transportFlags Flags with additional information about the transport.
*/
@@ -133,8 +132,9 @@
* Could be less than total backup size if backup process was interrupted
* before finish of processing all backup data.
* @param quotaBytes Current amount of backup data that is allowed for the app.
+ * @param callbackBinder Binder on which to indicate operation completion.
*/
- void doQuotaExceeded(long backupDataBytes, long quotaBytes);
+ void doQuotaExceeded(long backupDataBytes, long quotaBytes, IBackupCallback callbackBinder);
/**
* Restore a single "file" to the application. The file was typically obtained from
diff --git a/core/java/android/app/backup/BackupAgent.java b/core/java/android/app/backup/BackupAgent.java
index 62332d6..df27d58 100644
--- a/core/java/android/app/backup/BackupAgent.java
+++ b/core/java/android/app/backup/BackupAgent.java
@@ -980,7 +980,7 @@
try {
callbackBinder.operationComplete(result);
} catch (RemoteException e) {
- // we'll time out anyway, so we're safe
+ // We will time out anyway.
}
// Don't close the fd out from under the system service if this was local
@@ -1162,10 +1162,16 @@
}
@Override
- public void doQuotaExceeded(long backupDataBytes, long quotaBytes) {
+ public void doQuotaExceeded(
+ long backupDataBytes,
+ long quotaBytes,
+ IBackupCallback callbackBinder) {
long ident = Binder.clearCallingIdentity();
+
+ long result = RESULT_ERROR;
try {
BackupAgent.this.onQuotaExceeded(backupDataBytes, quotaBytes);
+ result = RESULT_SUCCESS;
} catch (Exception e) {
Log.d(TAG, "onQuotaExceeded(" + BackupAgent.this.getClass().getName() + ") threw",
e);
@@ -1173,6 +1179,12 @@
} finally {
waitForSharedPrefs();
Binder.restoreCallingIdentity(ident);
+
+ try {
+ callbackBinder.operationComplete(result);
+ } catch (RemoteException e) {
+ // We will time out anyway.
+ }
}
}
}
diff --git a/services/backup/java/com/android/server/backup/BackupAgentTimeoutParameters.java b/services/backup/java/com/android/server/backup/BackupAgentTimeoutParameters.java
index 49fa1cc..df46d260b 100644
--- a/services/backup/java/com/android/server/backup/BackupAgentTimeoutParameters.java
+++ b/services/backup/java/com/android/server/backup/BackupAgentTimeoutParameters.java
@@ -57,6 +57,10 @@
public static final String SETTING_RESTORE_AGENT_FINISHED_TIMEOUT_MILLIS =
"restore_agent_finished_timeout_millis";
+ @VisibleForTesting
+ public static final String SETTING_QUOTA_EXCEEDED_TIMEOUT_MILLIS =
+ "quota_exceeded_timeout_millis";
+
// Default values
@VisibleForTesting public static final long DEFAULT_KV_BACKUP_AGENT_TIMEOUT_MILLIS = 30 * 1000;
@@ -71,6 +75,9 @@
@VisibleForTesting
public static final long DEFAULT_RESTORE_AGENT_FINISHED_TIMEOUT_MILLIS = 30 * 1000;
+ @VisibleForTesting
+ public static final long DEFAULT_QUOTA_EXCEEDED_TIMEOUT_MILLIS = 3 * 1000;
+
@GuardedBy("mLock")
private long mKvBackupAgentTimeoutMillis;
@@ -86,6 +93,9 @@
@GuardedBy("mLock")
private long mRestoreAgentFinishedTimeoutMillis;
+ @GuardedBy("mLock")
+ private long mQuotaExceededTimeoutMillis;
+
private final Object mLock = new Object();
public BackupAgentTimeoutParameters(Handler handler, ContentResolver resolver) {
@@ -118,6 +128,10 @@
parser.getLong(
SETTING_RESTORE_AGENT_FINISHED_TIMEOUT_MILLIS,
DEFAULT_RESTORE_AGENT_FINISHED_TIMEOUT_MILLIS);
+ mQuotaExceededTimeoutMillis =
+ parser.getLong(
+ SETTING_QUOTA_EXCEEDED_TIMEOUT_MILLIS,
+ DEFAULT_QUOTA_EXCEEDED_TIMEOUT_MILLIS);
}
}
@@ -170,4 +184,16 @@
return mRestoreAgentFinishedTimeoutMillis;
}
}
+
+ public long getQuotaExceededTimeoutMillis() {
+ synchronized (mLock) {
+ if (BackupManagerService.DEBUG_SCHEDULING) {
+ Slog.v(
+ TAG,
+ "getQuotaExceededTimeoutMillis(): "
+ + mQuotaExceededTimeoutMillis);
+ }
+ return mQuotaExceededTimeoutMillis;
+ }
+ }
}
diff --git a/services/backup/java/com/android/server/backup/fullbackup/FullBackupEngine.java b/services/backup/java/com/android/server/backup/fullbackup/FullBackupEngine.java
index 5694659..16906f7 100644
--- a/services/backup/java/com/android/server/backup/fullbackup/FullBackupEngine.java
+++ b/services/backup/java/com/android/server/backup/fullbackup/FullBackupEngine.java
@@ -46,6 +46,7 @@
import com.android.server.backup.BackupAgentTimeoutParameters;
import com.android.server.backup.BackupManagerService;
import com.android.server.backup.BackupRestoreTask;
+import com.android.server.backup.remote.RemoteCall;
import com.android.server.backup.utils.FullBackupUtils;
import java.io.BufferedOutputStream;
@@ -270,10 +271,12 @@
return result;
}
- public void sendQuotaExceeded(final long backupDataBytes, final long quotaBytes) {
+ public void sendQuotaExceeded(long backupDataBytes, long quotaBytes) {
if (initializeAgent()) {
try {
- mAgent.doQuotaExceeded(backupDataBytes, quotaBytes);
+ RemoteCall.execute(
+ callback -> mAgent.doQuotaExceeded(backupDataBytes, quotaBytes, callback),
+ mAgentTimeoutParameters.getQuotaExceededTimeoutMillis());
} catch (RemoteException e) {
Slog.e(TAG, "Remote exception while telling agent about quota exceeded");
}
diff --git a/services/backup/java/com/android/server/backup/fullbackup/PerformFullTransportBackupTask.java b/services/backup/java/com/android/server/backup/fullbackup/PerformFullTransportBackupTask.java
index f7c1c10..e108026 100644
--- a/services/backup/java/com/android/server/backup/fullbackup/PerformFullTransportBackupTask.java
+++ b/services/backup/java/com/android/server/backup/fullbackup/PerformFullTransportBackupTask.java
@@ -51,6 +51,7 @@
import com.android.server.backup.TransportManager;
import com.android.server.backup.internal.OnTaskFinishedListener;
import com.android.server.backup.internal.Operation;
+import com.android.server.backup.remote.RemoteCall;
import com.android.server.backup.transport.TransportClient;
import com.android.server.backup.transport.TransportNotAvailableException;
import com.android.server.backup.utils.AppBackupUtils;
@@ -739,7 +740,9 @@
Slog.d(TAG, "Package hit quota limit on preflight " +
pkg.packageName + ": " + totalSize + " of " + mQuota);
}
- agent.doQuotaExceeded(totalSize, mQuota);
+ RemoteCall.execute(
+ callback -> agent.doQuotaExceeded(totalSize, mQuota, callback),
+ mAgentTimeoutParameters.getQuotaExceededTimeoutMillis());
}
} catch (Exception e) {
Slog.w(TAG, "Exception preflighting " + pkg.packageName + ": " + e.getMessage());
diff --git a/services/backup/java/com/android/server/backup/keyvalue/KeyValueBackupReporter.java b/services/backup/java/com/android/server/backup/keyvalue/KeyValueBackupReporter.java
index 2f32775..54e6b1d 100644
--- a/services/backup/java/com/android/server/backup/keyvalue/KeyValueBackupReporter.java
+++ b/services/backup/java/com/android/server/backup/keyvalue/KeyValueBackupReporter.java
@@ -377,9 +377,9 @@
Slog.w(TAG, "Unable to contact transport for recommended backoff: " + e);
}
- void onRemoteCallReturned(RemoteResult result) {
+ void onRemoteCallReturned(RemoteResult result, String logIdentifier) {
if (MORE_DEBUG) {
- Slog.v(TAG, "Agent call returned " + result);
+ Slog.v(TAG, "Agent call " + logIdentifier + " returned " + result);
}
}
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 fbe0d6f..a4cd629 100644
--- a/services/backup/java/com/android/server/backup/keyvalue/KeyValueBackupTask.java
+++ b/services/backup/java/com/android/server/backup/keyvalue/KeyValueBackupTask.java
@@ -694,8 +694,6 @@
IBackupTransport transport = mTransportClient.connectOrThrow("KVBT.extractAgentData()");
long quota = transport.getBackupQuota(packageName, /* isFullBackup */ false);
int transportFlags = transport.getTransportFlags();
- long kvBackupAgentTimeoutMillis =
- mAgentTimeoutParameters.getKvBackupAgentTimeoutMillis();
callingAgent = true;
agentResult =
@@ -708,7 +706,8 @@
quota,
callback,
transportFlags),
- kvBackupAgentTimeoutMillis);
+ mAgentTimeoutParameters.getKvBackupAgentTimeoutMillis(),
+ "doBackup()");
} catch (Exception e) {
mReporter.onCallAgentDoBackupError(packageName, callingAgent, e);
errorCleanup();
@@ -908,14 +907,16 @@
return false;
}
- private void agentDoQuotaExceeded(
- @Nullable IBackupAgent agent, String packageName, long backupDataSize) {
+ private void agentDoQuotaExceeded(@Nullable IBackupAgent agent, String packageName, long size) {
if (agent != null) {
try {
IBackupTransport transport =
mTransportClient.connectOrThrow("KVBT.agentDoQuotaExceeded()");
long quota = transport.getBackupQuota(packageName, false);
- agent.doQuotaExceeded(backupDataSize, quota);
+ remoteCall(
+ callback -> agent.doQuotaExceeded(size, quota, callback),
+ mAgentTimeoutParameters.getQuotaExceededTimeoutMillis(),
+ "doQuotaExceeded()");
} catch (Exception e) {
mReporter.onAgentDoQuotaExceededError(e);
}
@@ -1056,11 +1057,12 @@
}
}
- private RemoteResult remoteCall(RemoteCallable<IBackupCallback> remoteCallable, long timeoutMs)
+ private RemoteResult remoteCall(
+ RemoteCallable<IBackupCallback> remoteCallable, long timeoutMs, String logIdentifier)
throws RemoteException {
mPendingCall = new RemoteCall(mCancelled, remoteCallable, timeoutMs);
RemoteResult result = mPendingCall.call();
- mReporter.onRemoteCallReturned(result);
+ mReporter.onRemoteCallReturned(result, logIdentifier);
mPendingCall = null;
return result;
}
diff --git a/services/backup/java/com/android/server/backup/remote/RemoteCall.java b/services/backup/java/com/android/server/backup/remote/RemoteCall.java
index b3e802e..3af9e1d 100644
--- a/services/backup/java/com/android/server/backup/remote/RemoteCall.java
+++ b/services/backup/java/com/android/server/backup/remote/RemoteCall.java
@@ -44,6 +44,21 @@
*/
// TODO: Kick-off callable in dedicated thread (because of local calls, which are synchronous)
public class RemoteCall {
+ /**
+ * Creates a {@link RemoteCall} object with {@code callable} and {@code timeoutMs} and calls
+ * {@link #call()} on it immediately after.
+ *
+ * <p>Note that you won't be able to cancel the call, to do that construct an object regularly
+ * first, then use {@link #call()}.
+ *
+ * @see #RemoteCall(RemoteCallable, long)
+ * @see #call()
+ */
+ public static RemoteResult execute(RemoteCallable<IBackupCallback> callable, long timeoutMs)
+ throws RemoteException {
+ return new RemoteCall(callable, timeoutMs).call();
+ }
+
private final RemoteCallable<IBackupCallback> mCallable;
private final CompletableFuture<RemoteResult> mFuture;
private final long mTimeoutMs;
diff --git a/services/robotests/src/com/android/server/backup/BackupAgentTimeoutParametersTest.java b/services/robotests/src/com/android/server/backup/BackupAgentTimeoutParametersTest.java
index 801451e..0d2c221 100644
--- a/services/robotests/src/com/android/server/backup/BackupAgentTimeoutParametersTest.java
+++ b/services/robotests/src/com/android/server/backup/BackupAgentTimeoutParametersTest.java
@@ -16,6 +16,8 @@
package com.android.server.backup;
+import static com.google.common.truth.Truth.assertThat;
+
import static org.junit.Assert.assertEquals;
import android.content.ContentResolver;
@@ -51,7 +53,6 @@
mContentResolver = context.getContentResolver();
mParameters = new BackupAgentTimeoutParameters(new Handler(), mContentResolver);
- mParameters.start();
}
/** Stop observing changes to the setting. */
@@ -61,8 +62,11 @@
}
/** Tests that timeout parameters are initialized with default values on creation. */
+ // TODO: Break down tests
@Test
public void testGetParameters_afterConstructorWithStart_returnsDefaultValues() {
+ mParameters.start();
+
long kvBackupAgentTimeoutMillis = mParameters.getKvBackupAgentTimeoutMillis();
long fullBackupAgentTimeoutMillis = mParameters.getFullBackupAgentTimeoutMillis();
long sharedBackupAgentTimeoutMillis = mParameters.getSharedBackupAgentTimeoutMillis();
@@ -86,13 +90,33 @@
restoreAgentFinishedTimeoutMillis);
}
+ @Test
+ public void testGetQuotaExceededTimeoutMillis_returnsDefaultValue() {
+ mParameters.start();
+
+ long timeout = mParameters.getQuotaExceededTimeoutMillis();
+
+ assertThat(timeout)
+ .isEqualTo(BackupAgentTimeoutParameters.DEFAULT_QUOTA_EXCEEDED_TIMEOUT_MILLIS);
+ }
+
+ @Test
+ public void testGetQuotaExceededTimeoutMillis_whenSettingSet_returnsSetValue() {
+ putStringAndNotify(
+ BackupAgentTimeoutParameters.SETTING_QUOTA_EXCEEDED_TIMEOUT_MILLIS + "=" + 1279);
+ mParameters.start();
+
+ long timeout = mParameters.getQuotaExceededTimeoutMillis();
+
+ assertThat(timeout).isEqualTo(1279);
+ }
+
/**
* Tests that timeout parameters are updated when we call start, even when a setting change
* occurs while we are not observing.
*/
@Test
public void testGetParameters_withSettingChangeBeforeStart_updatesValues() {
- mParameters.stop();
long testTimeout = BackupAgentTimeoutParameters.DEFAULT_KV_BACKUP_AGENT_TIMEOUT_MILLIS * 2;
final String setting =
BackupAgentTimeoutParameters.SETTING_KV_BACKUP_AGENT_TIMEOUT_MILLIS
@@ -112,6 +136,7 @@
*/
@Test
public void testGetParameters_withSettingChangeAfterStart_updatesValues() {
+ mParameters.start();
long testTimeout = BackupAgentTimeoutParameters.DEFAULT_KV_BACKUP_AGENT_TIMEOUT_MILLIS * 2;
final String setting =
BackupAgentTimeoutParameters.SETTING_KV_BACKUP_AGENT_TIMEOUT_MILLIS
diff --git a/services/robotests/src/com/android/server/backup/keyvalue/KeyValueBackupReporterTest.java b/services/robotests/src/com/android/server/backup/keyvalue/KeyValueBackupReporterTest.java
index 72ba439..21b90f1 100644
--- a/services/robotests/src/com/android/server/backup/keyvalue/KeyValueBackupReporterTest.java
+++ b/services/robotests/src/com/android/server/backup/keyvalue/KeyValueBackupReporterTest.java
@@ -27,6 +27,7 @@
import android.util.Log;
import com.android.server.backup.BackupManagerService;
+import com.android.server.backup.remote.RemoteResult;
import com.android.server.testing.FrameworkRobolectricTestRunner;
import com.android.server.testing.SystemLoaderPackages;
import com.android.server.testing.shadows.ShadowEventLog;
@@ -37,6 +38,9 @@
import org.junit.runner.RunWith;
import org.mockito.Mock;
import org.robolectric.annotation.Config;
+import org.robolectric.shadows.ShadowLog;
+
+import java.lang.reflect.Field;
@RunWith(FrameworkRobolectricTestRunner.class)
@Config(
@@ -77,4 +81,39 @@
assertThat(observer).isEqualTo(mObserver);
}
+
+ @Test
+ public void testOnRevertTask_logsCorrectly() throws Exception {
+ setMoreDebug(true);
+
+ mReporter.onRevertTask();
+
+ assertLogcat(TAG, Log.INFO);
+ }
+
+ @Test
+ public void testOnRemoteCallReturned_logsCorrectly() throws Exception {
+ setMoreDebug(true);
+
+ mReporter.onRemoteCallReturned(RemoteResult.of(3), "onFoo()");
+
+ assertLogcat(TAG, Log.VERBOSE);
+ ShadowLog.LogItem log = ShadowLog.getLogsForTag(TAG).get(0);
+ assertThat(log.msg).contains("onFoo()");
+ assertThat(log.msg).contains("3");
+ }
+
+ /**
+ * HACK: We actually want {@link KeyValueBackupReporter#MORE_DEBUG} to be a constant to be able
+ * to strip those lines at build time. So, we have to do this to test :(
+ */
+ private static void setMoreDebug(boolean value)
+ throws NoSuchFieldException, IllegalAccessException {
+ if (KeyValueBackupReporter.MORE_DEBUG == value) {
+ return;
+ }
+ Field moreDebugField = KeyValueBackupReporter.class.getDeclaredField("MORE_DEBUG");
+ moreDebugField.setAccessible(true);
+ moreDebugField.set(null, value);
+ }
}
diff --git a/services/robotests/src/com/android/server/backup/remote/RemoteCallTest.java b/services/robotests/src/com/android/server/backup/remote/RemoteCallTest.java
index 6434c4f..1d92bed 100644
--- a/services/robotests/src/com/android/server/backup/remote/RemoteCallTest.java
+++ b/services/robotests/src/com/android/server/backup/remote/RemoteCallTest.java
@@ -222,6 +222,37 @@
assertThat(result.get()).isEqualTo(RemoteResult.FAILED_CANCELLED);
}
+ @Test
+ public void testExecute_whenCallbackIsCalledBeforeTimeout_returnsResult() throws Exception {
+ RemoteResult result =
+ runInWorkerThread(
+ () -> RemoteCall.execute(callback -> callback.operationComplete(3), 1000));
+
+ assertThat(result.get()).isEqualTo(3);
+ }
+
+ @Test
+ public void testExecute_whenTimesOutBeforeCallback_returnsTimeOut() throws Exception {
+ ConditionVariable scheduled = new ConditionVariable(false);
+
+ Future<RemoteResult> result =
+ runInWorkerThreadAsync(
+ () ->
+ RemoteCall.execute(
+ callback -> {
+ postDelayed(
+ Handler.getMain(),
+ () -> callback.operationComplete(0),
+ 1000);
+ scheduled.open();
+ },
+ 500));
+
+ scheduled.block();
+ runToEndOfTasks(Looper.getMainLooper());
+ assertThat(result.get()).isEqualTo(RemoteResult.FAILED_TIMED_OUT);
+ }
+
private static <T> Future<T> runInWorkerThreadAsync(Callable<T> supplier) {
CompletableFuture<T> future = new CompletableFuture<>();
new Thread(() -> future.complete(uncheck(supplier)), "test-worker-thread").start();