Merge "Refactor PackageWatchdog explicit health checks" into qt-dev

3 files changed, 334 insertions, 256 deletions

diff --git a/services/core/java/com/android/server/ExplicitHealthCheckController.java b/services/core/java/com/android/server/ExplicitHealthCheckController.java
index 164837ab98dd..27ad208ef8a1 100644
--- a/services/core/java/com/android/server/ExplicitHealthCheckController.java
+++ b/services/core/java/com/android/server/ExplicitHealthCheckController.java
@@ -41,10 +41,14 @@ import android.util.Slog;
 import com.android.internal.annotations.GuardedBy;
 import com.android.internal.util.Preconditions;
 
+import java.util.Collection;
 import java.util.Collections;
+import java.util.Iterator;
 import java.util.List;
+import java.util.Set;
 import java.util.function.Consumer;
 
+// TODO(b/120598832): Add tests
 /**
  * Controls the connections with {@link ExplicitHealthCheckService}.
  */
@@ -53,18 +57,30 @@ class ExplicitHealthCheckController {
     private final Object mLock = new Object();
     private final Context mContext;
 
-    // Called everytime the service is connected, so the watchdog can sync it's state with
-    // the health check service. In practice, should never be null after it has been #setEnabled.
-    @GuardedBy("mLock") @Nullable private Runnable mOnConnected;
     // Called everytime a package passes the health check, so the watchdog is notified of the
     // passing check. In practice, should never be null after it has been #setEnabled.
+    // To prevent deadlocks between the controller and watchdog threads, we have
+    // a lock invariant to ALWAYS acquire the PackageWatchdog#mLock before #mLock in this class.
+    // It's easier to just NOT hold #mLock when calling into watchdog code on this consumer.
     @GuardedBy("mLock") @Nullable private Consumer<String> mPassedConsumer;
+    // Called everytime after a successful #syncRequest call, so the watchdog can receive packages
+    // supporting health checks and update its internal state. In practice, should never be null
+    // after it has been #setEnabled.
+    // To prevent deadlocks between the controller and watchdog threads, we have
+    // a lock invariant to ALWAYS acquire the PackageWatchdog#mLock before #mLock in this class.
+    // It's easier to just NOT hold #mLock when calling into watchdog code on this consumer.
+    @GuardedBy("mLock") @Nullable private Consumer<List<String>> mSupportedConsumer;
+    // Called everytime we need to notify the watchdog to sync requests between itself and the
+    // health check service. In practice, should never be null after it has been #setEnabled.
+    // To prevent deadlocks between the controller and watchdog threads, we have
+    // a lock invariant to ALWAYS acquire the PackageWatchdog#mLock before #mLock in this class.
+    // It's easier to just NOT hold #mLock when calling into watchdog code on this runnable.
+    @GuardedBy("mLock") @Nullable private Runnable mNotifySyncRunnable;
     // Actual binder object to the explicit health check service.
     @GuardedBy("mLock") @Nullable private IExplicitHealthCheckService mRemoteService;
-    // Cache for packages supporting explicit health checks. This cache should not change while
-    // the health check service is running.
-    @GuardedBy("mLock") @Nullable private List<String> mSupportedPackages;
-    // Connection to the explicit health check service, necessary to unbind
+    // Connection to the explicit health check service, necessary to unbind.
+    // We should only try to bind if mConnection is null, non-null indicates we
+    // are connected or at least connecting.
     @GuardedBy("mLock") @Nullable private ServiceConnection mConnection;
     // Bind state of the explicit health check service.
     @GuardedBy("mLock") private boolean mEnabled;
@@ -73,23 +89,117 @@ class ExplicitHealthCheckController {
         mContext = context;
     }
 
+    /** Enables or disables explicit health checks. */
+    public void setEnabled(boolean enabled) {
+        synchronized (mLock) {
+            Slog.i(TAG, "Explicit health checks " + (enabled ? "enabled." : "disabled."));
+            mEnabled = enabled;
+        }
+    }
+
+    /**
+     * Sets callbacks to listen to important events from the controller.
+     *
+     * <p> Should be called once at initialization before any other calls to the controller to
+     * ensure a happens-before relationship of the set parameters and visibility on other threads.
+     */
+    public void setCallbacks(Consumer<String> passedConsumer,
+            Consumer<List<String>> supportedConsumer, Runnable notifySyncRunnable) {
+        synchronized (mLock) {
+            if (mPassedConsumer != null || mSupportedConsumer != null
+                    || mNotifySyncRunnable != null) {
+                Slog.wtf(TAG, "Resetting health check controller callbacks");
+            }
+
+            mPassedConsumer = Preconditions.checkNotNull(passedConsumer);
+            mSupportedConsumer = Preconditions.checkNotNull(supportedConsumer);
+            mNotifySyncRunnable = Preconditions.checkNotNull(notifySyncRunnable);
+        }
+    }
+
+    /**
+     * Calls the health check service to request or cancel packages based on
+     * {@code newRequestedPackages}.
+     *
+     * <p> Supported packages in {@code newRequestedPackages} that have not been previously
+     * requested will be requested while supported packages not in {@code newRequestedPackages}
+     * but were previously requested will be cancelled.
+     *
+     * <p> This handles binding and unbinding to the health check service as required.
+     *
+     * <p> Note, calling this may modify {@code newRequestedPackages}.
+     *
+     * <p> Note, this method is not thread safe, all calls should be serialized.
+     */
+    public void syncRequests(Set<String> newRequestedPackages) {
+        boolean enabled;
+        synchronized (mLock) {
+            enabled = mEnabled;
+        }
+
+        if (!enabled) {
+            Slog.i(TAG, "Health checks disabled, no supported packages");
+            // Call outside lock
+            mSupportedConsumer.accept(Collections.emptyList());
+            return;
+        }
+
+        getSupportedPackages(supportedPackages -> {
+            // Notify the watchdog without lock held
+            mSupportedConsumer.accept(supportedPackages);
+            getRequestedPackages(previousRequestedPackages -> {
+                synchronized (mLock) {
+                    // Hold lock so requests and cancellations are sent atomically.
+                    // It is important we don't mix requests from multiple threads.
+
+                    // Note, this may modify newRequestedPackages
+                    newRequestedPackages.retainAll(supportedPackages);
+
+                    // Cancel packages no longer requested
+                    actOnDifference(previousRequestedPackages,
+                            newRequestedPackages, p -> cancel(p));
+                    // Request packages not yet requested
+                    actOnDifference(newRequestedPackages,
+                            previousRequestedPackages, p -> request(p));
+
+                    if (newRequestedPackages.isEmpty()) {
+                        Slog.i(TAG, "No more health check requests, unbinding...");
+                        unbindService();
+                        return;
+                    }
+                }
+            });
+        });
+    }
+
+    private void actOnDifference(Collection<String> collection1, Collection<String> collection2,
+            Consumer<String> action) {
+        Iterator<String> iterator = collection1.iterator();
+        while (iterator.hasNext()) {
+            String packageName = iterator.next();
+            if (!collection2.contains(packageName)) {
+                action.accept(packageName);
+            }
+        }
+    }
+
     /**
      * Requests an explicit health check for {@code packageName}.
      * After this request, the callback registered on {@link #setCallbacks} can receive explicit
      * health check passed results.
-     *
-     * @throws IllegalStateException if the service is not started
      */
-    public void request(String packageName) throws RemoteException {
+    private void request(String packageName) {
         synchronized (mLock) {
-            if (!mEnabled) {
+            if (!prepareServiceLocked("request health check for " + packageName)) {
                 return;
             }
 
-            enforceServiceReadyLocked();
-
             Slog.i(TAG, "Requesting health check for package " + packageName);
-            mRemoteService.request(packageName);
+            try {
+                mRemoteService.request(packageName);
+            } catch (RemoteException e) {
+                Slog.w(TAG, "Failed to request health check for package " + packageName, e);
+            }
         }
     }
 
@@ -97,46 +207,46 @@ class ExplicitHealthCheckController {
      * Cancels all explicit health checks for {@code packageName}.
      * After this request, the callback registered on {@link #setCallbacks} can no longer receive
      * explicit health check passed results.
-     *
-     * @throws IllegalStateException if the service is not started
      */
-    public void cancel(String packageName) throws RemoteException {
+    private void cancel(String packageName) {
         synchronized (mLock) {
-            if (!mEnabled) {
+            if (!prepareServiceLocked("cancel health check for " + packageName)) {
                 return;
             }
 
-            enforceServiceReadyLocked();
-
             Slog.i(TAG, "Cancelling health check for package " + packageName);
-            mRemoteService.cancel(packageName);
+            try {
+                mRemoteService.cancel(packageName);
+            } catch (RemoteException e) {
+                // Do nothing, if the service is down, when it comes up, we will sync requests,
+                // if there's some other error, retrying wouldn't fix anyways.
+                Slog.w(TAG, "Failed to cancel health check for package " + packageName, e);
+            }
         }
     }
 
     /**
      * Returns the packages that we can request explicit health checks for.
      * The packages will be returned to the {@code consumer}.
-     *
-     * @throws IllegalStateException if the service is not started
      */
-    public void getSupportedPackages(Consumer<List<String>> consumer) throws RemoteException {
+    private void getSupportedPackages(Consumer<List<String>> consumer) {
         synchronized (mLock) {
-            if (!mEnabled) {
-                consumer.accept(Collections.emptyList());
+            if (!prepareServiceLocked("get health check supported packages")) {
                 return;
             }
 
-            enforceServiceReadyLocked();
-
-            if (mSupportedPackages == null) {
-                Slog.d(TAG, "Getting health check supported packages");
+            Slog.d(TAG, "Getting health check supported packages");
+            try {
                 mRemoteService.getSupportedPackages(new RemoteCallback(result -> {
-                    mSupportedPackages = result.getStringArrayList(EXTRA_SUPPORTED_PACKAGES);
-                    consumer.accept(mSupportedPackages);
+                    List<String> packages = result.getStringArrayList(EXTRA_SUPPORTED_PACKAGES);
+                    Slog.i(TAG, "Explicit health check supported packages " + packages);
+                    consumer.accept(packages);
                 }));
-            } else {
-                Slog.d(TAG, "Getting cached health check supported packages");
-                consumer.accept(mSupportedPackages);
+            } catch (RemoteException e) {
+                // Request failed, treat as if all observed packages are supported, if any packages
+                // expire during this period, we may incorrectly treat it as failing health checks
+                // even if we don't support health checks for the package.
+                Slog.w(TAG, "Failed to get health check supported packages", e);
             }
         }
     }
@@ -144,57 +254,42 @@ class ExplicitHealthCheckController {
     /**
      * Returns the packages for which health checks are currently in progress.
      * The packages will be returned to the {@code consumer}.
-     *
-     * @throws IllegalStateException if the service is not started
      */
-    public void getRequestedPackages(Consumer<List<String>> consumer) throws RemoteException {
+    private void getRequestedPackages(Consumer<List<String>> consumer) {
         synchronized (mLock) {
-            if (!mEnabled) {
-                consumer.accept(Collections.emptyList());
+            if (!prepareServiceLocked("get health check requested packages")) {
                 return;
             }
 
-            enforceServiceReadyLocked();
-
             Slog.d(TAG, "Getting health check requested packages");
-            mRemoteService.getRequestedPackages(new RemoteCallback(
-                    result -> consumer.accept(
-                            result.getStringArrayList(EXTRA_REQUESTED_PACKAGES))));
-        }
-    }
-
-    /** Enables or disables explicit health checks. */
-    public void setEnabled(boolean enabled) {
-        synchronized (mLock) {
-            if (enabled == mEnabled) {
-                return;
-            }
-
-            Slog.i(TAG, "Setting explicit health checks enabled " + enabled);
-            mEnabled = enabled;
-            if (enabled) {
-                bindService();
-            } else {
-                unbindService();
+            try {
+                mRemoteService.getRequestedPackages(new RemoteCallback(result -> {
+                    List<String> packages = result.getStringArrayList(EXTRA_REQUESTED_PACKAGES);
+                    Slog.i(TAG, "Explicit health check requested packages " + packages);
+                    consumer.accept(packages);
+                }));
+            } catch (RemoteException e) {
+                // Request failed, treat as if we haven't requested any packages, if any packages
+                // were actually requested, they will not be cancelled now. May be cancelled later
+                Slog.w(TAG, "Failed to get health check requested packages", e);
             }
         }
     }
 
     /**
-     * Sets callbacks to listen to important events from the controller.
-     * Should be called at initialization.
+     * Binds to the explicit health check service if the controller is enabled and
+     * not already bound.
      */
-    public void setCallbacks(Runnable onConnected, Consumer<String> passedConsumer) {
-        Preconditions.checkNotNull(onConnected);
-        Preconditions.checkNotNull(passedConsumer);
-        mOnConnected = onConnected;
-        mPassedConsumer = passedConsumer;
-    }
-
-    /** Binds to the explicit health check service. */
     private void bindService() {
         synchronized (mLock) {
-            if (mRemoteService != null) {
+            if (!mEnabled || mConnection != null || mRemoteService != null) {
+                if (!mEnabled) {
+                    Slog.i(TAG, "Not binding to service, service disabled");
+                } else if (mRemoteService != null) {
+                    Slog.i(TAG, "Not binding to service, service already connected");
+                } else {
+                    Slog.i(TAG, "Not binding to service, service already connecting");
+                }
                 return;
             }
             ComponentName component = getServiceComponentNameLocked();
@@ -205,14 +300,11 @@ class ExplicitHealthCheckController {
 
             Intent intent = new Intent();
             intent.setComponent(component);
-            // TODO: Fix potential race conditions during mConnection state transitions.
-            // E.g after #onServiceDisconected, the mRemoteService object is invalid until
-            // we get an #onServiceConnected.
             mConnection = new ServiceConnection() {
                 @Override
                 public void onServiceConnected(ComponentName name, IBinder service) {
-                    initState(service);
                     Slog.i(TAG, "Explicit health check service is connected " + name);
+                    initState(service);
                 }
 
                 @Override
@@ -221,19 +313,18 @@ class ExplicitHealthCheckController {
                     // Service crashed or process was killed, #onServiceConnected will be called.
                     // Don't need to re-bind.
                     Slog.i(TAG, "Explicit health check service is disconnected " + name);
+                    synchronized (mLock) {
+                        mRemoteService = null;
+                    }
                 }
 
                 @Override
                 public void onBindingDied(ComponentName name) {
                     // Application hosting service probably got updated
                     // Need to re-bind.
-                    synchronized (mLock) {
-                        if (mEnabled) {
-                            unbindService();
-                            bindService();
-                        }
-                    }
-                    Slog.i(TAG, "Explicit health check service binding is dead " + name);
+                    Slog.i(TAG, "Explicit health check service binding is dead. Rebind: " + name);
+                    unbindService();
+                    bindService();
                 }
 
                 @Override
@@ -243,9 +334,9 @@ class ExplicitHealthCheckController {
                 }
             };
 
-            Slog.i(TAG, "Binding to explicit health service");
-            mContext.bindServiceAsUser(intent, mConnection, Context.BIND_AUTO_CREATE,
-                    UserHandle.of(UserHandle.USER_SYSTEM));
+            mContext.bindServiceAsUser(intent, mConnection,
+                    Context.BIND_AUTO_CREATE, UserHandle.of(UserHandle.USER_SYSTEM));
+            Slog.i(TAG, "Explicit health check service is bound");
         }
     }
 
@@ -253,10 +344,11 @@ class ExplicitHealthCheckController {
     private void unbindService() {
         synchronized (mLock) {
             if (mRemoteService != null) {
-                Slog.i(TAG, "Unbinding from explicit health service");
                 mContext.unbindService(mConnection);
                 mRemoteService = null;
+                mConnection = null;
             }
+            Slog.i(TAG, "Explicit health check service is unbound");
         }
     }
 
@@ -301,40 +393,54 @@ class ExplicitHealthCheckController {
 
     private void initState(IBinder service) {
         synchronized (mLock) {
-            mSupportedPackages = null;
+            if (!mEnabled) {
+                Slog.w(TAG, "Attempting to connect disabled service?? Unbinding...");
+                // Very unlikely, but we disabled the service after binding but before we connected
+                unbindService();
+                return;
+            }
             mRemoteService = IExplicitHealthCheckService.Stub.asInterface(service);
             try {
                 mRemoteService.setCallback(new RemoteCallback(result -> {
                     String packageName = result.getString(EXTRA_HEALTH_CHECK_PASSED_PACKAGE);
                     if (!TextUtils.isEmpty(packageName)) {
-                        synchronized (mLock) {
-                            if (mPassedConsumer == null) {
-                                Slog.w(TAG, "Health check passed for package " + packageName
-                                        + "but no consumer registered.");
-                            } else {
-                                mPassedConsumer.accept(packageName);
-                            }
+                        if (mPassedConsumer == null) {
+                            Slog.wtf(TAG, "Health check passed for package " + packageName
+                                    + "but no consumer registered.");
+                        } else {
+                            // Call without lock held
+                            mPassedConsumer.accept(packageName);
                         }
                     } else {
-                        Slog.w(TAG, "Empty package passed explicit health check?");
+                        Slog.wtf(TAG, "Empty package passed explicit health check?");
                     }
                 }));
-                if (mOnConnected == null) {
-                    Slog.w(TAG, "Health check service connected but no runnable registered.");
-                } else {
-                    mOnConnected.run();
-                }
+                Slog.i(TAG, "Service initialized, syncing requests");
             } catch (RemoteException e) {
                 Slog.wtf(TAG, "Could not setCallback on explicit health check service");
             }
         }
+        // Calling outside lock
+        mNotifySyncRunnable.run();
     }
 
+    /**
+     * Prepares the health check service to receive requests.
+     *
+     * @return {@code true} if it is ready and we can proceed with a request,
+     * {@code false} otherwise. If it is not ready, and the service is enabled,
+     * we will bind and the request should be automatically attempted later.
+     */
     @GuardedBy("mLock")
-    private void enforceServiceReadyLocked() {
-        if (mRemoteService == null) {
-            // TODO: Try to bind to service
-            throw new IllegalStateException("Explicit health check service not ready");
+    private boolean prepareServiceLocked(String action) {
+        if (mRemoteService != null && mEnabled) {
+            return true;
+        }
+        Slog.i(TAG, "Service not ready to " + action
+                + (mEnabled ? ". Binding..." : ". Disabled"));
+        if (mEnabled) {
+            bindService();
         }
+        return false;
     }
 }
diff --git a/services/core/java/com/android/server/PackageWatchdog.java b/services/core/java/com/android/server/PackageWatchdog.java
index 2ba4d975a6b0..feffe2f72b6f 100644
--- a/services/core/java/com/android/server/PackageWatchdog.java
+++ b/services/core/java/com/android/server/PackageWatchdog.java
@@ -26,7 +26,6 @@ import android.content.pm.VersionedPackage;
 import android.os.Environment;
 import android.os.Handler;
 import android.os.Looper;
-import android.os.RemoteException;
 import android.os.SystemClock;
 import android.text.TextUtils;
 import android.util.ArrayMap;
@@ -55,12 +54,10 @@ import java.io.InputStream;
 import java.lang.annotation.Retention;
 import java.nio.charset.StandardCharsets;
 import java.util.ArrayList;
-import java.util.Collection;
 import java.util.Collections;
 import java.util.Iterator;
 import java.util.List;
 import java.util.Set;
-import java.util.function.Consumer;
 
 /**
  * Monitors the health of packages on the system and notifies interested observers when packages
@@ -154,10 +151,11 @@ public class PackageWatchdog {
     public void onPackagesReady() {
         synchronized (mLock) {
             mIsPackagesReady = true;
-            mHealthCheckController.setCallbacks(this::updateHealthChecks,
-                    packageName -> onHealthCheckPassed(packageName));
-            // Controller is disabled at creation until here where we may enable it
-            mHealthCheckController.setEnabled(mIsHealthCheckEnabled);
+            mHealthCheckController.setCallbacks(packageName -> onHealthCheckPassed(packageName),
+                    packages -> onSupportedPackages(packages),
+                    () -> syncRequestsAsync());
+            // Controller is initially disabled until here where we may enable it and sync requests
+            setExplicitHealthCheckEnabled(mIsHealthCheckEnabled);
         }
     }
 
@@ -196,41 +194,23 @@ public class PackageWatchdog {
      * @throws IllegalArgumentException if {@code packageNames} is empty
      * or {@code durationMs} is less than 1
      */
-    public void startObservingHealth(PackageHealthObserver observer, List<String> packages,
+    public void startObservingHealth(PackageHealthObserver observer, List<String> packageNames,
             long durationMs) {
-        if (packages.isEmpty() || durationMs < 1) {
-            throw new IllegalArgumentException("Observation not started, no packages specified"
-                    + "or invalid duration");
-        }
-        if (!mIsPackagesReady) {
-            // TODO: Queue observation requests when packages are not ready
-            Slog.w(TAG, "Attempt to observe when packages not ready");
+        if (packageNames.isEmpty()) {
+            Slog.wtf(TAG, "No packages to observe, " + observer.getName());
             return;
         }
-
-        try {
-            Slog.i(TAG, "Getting packages supporting explicit health check");
-            mHealthCheckController.getSupportedPackages(supportedPackages ->
-                    startObservingInner(observer, packages, durationMs, supportedPackages));
-        } catch (RemoteException e) {
-            Slog.wtf(TAG, "Failed to fetch supported explicit health check packages");
+        if (durationMs < 1) {
+            // TODO: Instead of failing, monitor for default? 48hrs?
+            throw new IllegalArgumentException("Invalid duration " + durationMs + "ms for observer "
+                    + observer.getName() + ". Not observing packages " + packageNames);
         }
-    }
 
-    private void startObservingInner(PackageHealthObserver observer,
-            List<String> packageNames, long durationMs,
-            List<String> healthCheckSupportedPackages) {
-        Slog.i(TAG, "Start observing packages " + packageNames
-                + ". Explicit health check supported packages " + healthCheckSupportedPackages);
         List<MonitoredPackage> packages = new ArrayList<>();
         for (int i = 0; i < packageNames.size(); i++) {
-            String packageName = packageNames.get(i);
-            boolean shouldEnableHealthCheck = healthCheckSupportedPackages.contains(packageName);
-            // If we should enable explicit health check for a package,
-            // MonitoredPackage#mHasHealthCheckPassed will be false
-            // until PackageWatchdog#onHealthCheckPassed
-            packages.add(new MonitoredPackage(packageName, durationMs, !shouldEnableHealthCheck));
+            packages.add(new MonitoredPackage(packageNames.get(i), durationMs, false));
         }
+
         synchronized (mLock) {
             ObserverInternal oldObserver = mAllObservers.get(observer.getName());
             if (oldObserver == null) {
@@ -248,97 +228,11 @@ public class PackageWatchdog {
         // Always reschedule because we may need to expire packages
         // earlier than we are already scheduled for
         rescheduleCleanup();
-        updateHealthChecks();
+        Slog.i(TAG, "Syncing health check requests, observing packages " + packageNames);
+        syncRequestsAsync();
         saveToFileAsync();
     }
 
-    private void requestCheck(String packageName) {
-        try {
-            Slog.d(TAG, "Requesting explicit health check for " + packageName);
-            mHealthCheckController.request(packageName);
-        } catch (RemoteException e) {
-            Slog.wtf(TAG, "Failed to request explicit health check for " + packageName, e);
-        }
-    }
-
-    private void cancelCheck(String packageName) {
-        try {
-            Slog.d(TAG, "Cancelling explicit health check for " + packageName);
-            mHealthCheckController.cancel(packageName);
-        } catch (RemoteException e) {
-            Slog.wtf(TAG, "Failed to cancel explicit health check for " + packageName, e);
-        }
-    }
-
-    private void actOnDifference(Collection<String> collection1, Collection<String> collection2,
-            Consumer<String> action) {
-        Iterator<String> iterator = collection1.iterator();
-        while (iterator.hasNext()) {
-            String packageName = iterator.next();
-            if (!collection2.contains(packageName)) {
-                action.accept(packageName);
-            }
-        }
-    }
-
-    private void updateChecksInner(List<String> supportedPackages,
-            List<String> previousRequestedPackages) {
-        boolean shouldUpdateFile = false;
-
-        synchronized (mLock) {
-            Slog.i(TAG, "Updating explicit health checks. Supported packages: " + supportedPackages
-                    + ". Requested packages: " + previousRequestedPackages);
-            Set<String> newRequestedPackages = new ArraySet<>();
-            Iterator<ObserverInternal> oit = mAllObservers.values().iterator();
-            while (oit.hasNext()) {
-                ObserverInternal observer = oit.next();
-                Iterator<MonitoredPackage> pit =
-                        observer.mPackages.values().iterator();
-                while (pit.hasNext()) {
-                    MonitoredPackage monitoredPackage = pit.next();
-                    String packageName = monitoredPackage.mName;
-                    if (!monitoredPackage.mHasPassedHealthCheck) {
-                        if (supportedPackages.contains(packageName)) {
-                            newRequestedPackages.add(packageName);
-                        } else {
-                            shouldUpdateFile = true;
-                            monitoredPackage.mHasPassedHealthCheck = true;
-                        }
-                    }
-                }
-            }
-            // TODO: Support ending the binding if newRequestedPackages is empty.
-            // Will have to re-bind when we #startObservingHealth.
-
-            // Cancel packages no longer requested
-            actOnDifference(previousRequestedPackages, newRequestedPackages, p -> cancelCheck(p));
-            // Request packages not yet requested
-            actOnDifference(newRequestedPackages, previousRequestedPackages, p -> requestCheck(p));
-        }
-
-        if (shouldUpdateFile) {
-            saveToFileAsync();
-        }
-    }
-
-    private void updateHealthChecks() {
-        mShortTaskHandler.post(() -> {
-            try {
-                Slog.i(TAG, "Updating explicit health checks for all available packages");
-                mHealthCheckController.getSupportedPackages(supported -> {
-                    try {
-                        mHealthCheckController.getRequestedPackages(
-                                requested -> updateChecksInner(supported, requested));
-                    } catch (RemoteException e) {
-                        Slog.e(TAG, "Failed to get requested health check packages", e);
-                    }
-                });
-            } catch (RemoteException e) {
-                Slog.e(TAG, "Failed to get supported health check package", e);
-            }
-        });
-    }
-
     /**
      * Unregisters {@code observer} from listening to package failure.
      * Additionally, this stops observing any packages that may have previously been observed
@@ -441,6 +335,9 @@ public class PackageWatchdog {
         synchronized (mLock) {
             mIsHealthCheckEnabled = enabled;
             mHealthCheckController.setEnabled(enabled);
+            Slog.i(TAG, "Syncing health check requests, explicit health check is "
+                    + (enabled ? "enabled" : "disabled"));
+            syncRequestsAsync();
         }
     }
 
@@ -487,6 +384,35 @@ public class PackageWatchdog {
     }
 
     /**
+     * Serializes and syncs health check requests with the {@link ExplicitHealthCheckController}.
+     */
+    private void syncRequestsAsync() {
+        if (!mShortTaskHandler.hasCallbacks(this::syncRequests)) {
+            mShortTaskHandler.post(this::syncRequests);
+        }
+    }
+
+    /**
+     * Syncs health check requests with the {@link ExplicitHealthCheckController}.
+     * Calls to this must be serialized.
+     *
+     * @see #syncRequestsAsync
+     */
+    private void syncRequests() {
+        Set<String> packages = null;
+        synchronized (mLock) {
+            if (mIsPackagesReady) {
+                packages = getPackagesPendingHealthChecksLocked();
+            } // else, we will sync requests when packages become ready
+        }
+
+        // Call outside lock to avoid holding lock when calling into the controller.
+        if (packages != null) {
+            mHealthCheckController.syncRequests(packages);
+        }
+    }
+
+    /**
      * Updates the observers monitoring {@code packageName} that explicit health check has passed.
      *
      * <p> This update is strictly for registered observers at the time of the call
@@ -512,11 +438,64 @@ public class PackageWatchdog {
                 }
             }
         }
+
+        // So we can unbind from the service if this was the last result we expected
+        Slog.i(TAG, "Syncing health check requests, health check passed for " + packageName);
+        syncRequestsAsync();
+
+        if (shouldUpdateFile) {
+            saveToFileAsync();
+        }
+    }
+
+    private void onSupportedPackages(List<String> supportedPackages) {
+        boolean shouldUpdateFile = false;
+
+        synchronized (mLock) {
+            Slog.i(TAG, "Received supported packages " + supportedPackages);
+            Iterator<ObserverInternal> oit = mAllObservers.values().iterator();
+            while (oit.hasNext()) {
+                ObserverInternal observer = oit.next();
+                Iterator<MonitoredPackage> pit =
+                        observer.mPackages.values().iterator();
+                while (pit.hasNext()) {
+                    MonitoredPackage monitoredPackage = pit.next();
+                    String packageName = monitoredPackage.mName;
+                    if (!monitoredPackage.mHasPassedHealthCheck
+                            && !supportedPackages.contains(packageName)) {
+                        // Hasn't passed health check but health check is not supported
+                        Slog.i(TAG, packageName + " does not support health checks, passing");
+                        shouldUpdateFile = true;
+                        monitoredPackage.mHasPassedHealthCheck = true;
+                    }
+                }
+            }
+        }
+
         if (shouldUpdateFile) {
             saveToFileAsync();
         }
     }
 
+    private Set<String> getPackagesPendingHealthChecksLocked() {
+        Slog.d(TAG, "Getting all observed packages pending health checks");
+        Set<String> packages = new ArraySet<>();
+        Iterator<ObserverInternal> oit = mAllObservers.values().iterator();
+        while (oit.hasNext()) {
+            ObserverInternal observer = oit.next();
+            Iterator<MonitoredPackage> pit =
+                    observer.mPackages.values().iterator();
+            while (pit.hasNext()) {
+                MonitoredPackage monitoredPackage = pit.next();
+                String packageName = monitoredPackage.mName;
+                if (!monitoredPackage.mHasPassedHealthCheck) {
+                    packages.add(packageName);
+                }
+            }
+        }
+        return packages;
+    }
+
     /** Reschedules handler to prune expired packages from observers. */
     private void rescheduleCleanup() {
         synchronized (mLock) {
@@ -591,7 +570,8 @@ public class PackageWatchdog {
                 }
             }
         }
-        updateHealthChecks();
+        Slog.i(TAG, "Syncing health check requests pruned packages");
+        syncRequestsAsync();
         saveToFileAsync();
     }
 
@@ -690,6 +670,7 @@ public class PackageWatchdog {
     }
 
     private void saveToFileAsync() {
+        // TODO(b/120598832): Use Handler#hasCallbacks instead of removing and posting
         mLongTaskHandler.removeCallbacks(this::saveToFile);
         mLongTaskHandler.post(this::saveToFile);
     }
diff --git a/tests/PackageWatchdog/src/com/android/server/PackageWatchdogTest.java b/tests/PackageWatchdog/src/com/android/server/PackageWatchdogTest.java
index 28af7cee8f38..33bb4cceb3a9 100644
--- a/tests/PackageWatchdog/src/com/android/server/PackageWatchdogTest.java
+++ b/tests/PackageWatchdog/src/com/android/server/PackageWatchdogTest.java
@@ -26,7 +26,6 @@ import static org.junit.Assert.assertTrue;
 import android.content.Context;
 import android.content.pm.VersionedPackage;
 import android.os.Handler;
-import android.os.RemoteException;
 import android.os.test.TestLooper;
 import android.util.AtomicFile;
 
@@ -43,6 +42,7 @@ import java.util.ArrayList;
 import java.util.Arrays;
 import java.util.Collections;
 import java.util.List;
+import java.util.Set;
 import java.util.concurrent.TimeUnit;
 import java.util.function.Consumer;
 
@@ -635,43 +635,34 @@ public class PackageWatchdogTest {
         private boolean mIsEnabled;
         private List<String> mSupportedPackages = new ArrayList<>();
         private List<String> mRequestedPackages = new ArrayList<>();
-        private Runnable mStateChangedRunnable;
         private Consumer<String> mPassedConsumer;
+        private Consumer<List<String>> mSupportedConsumer;
+        private Runnable mNotifySyncRunnable;
 
         @Override
-        public void request(String packageName) throws RemoteException {
-            if (!mRequestedPackages.contains(packageName)) {
-                mRequestedPackages.add(packageName);
+        public void setEnabled(boolean enabled) {
+            mIsEnabled = enabled;
+            if (!mIsEnabled) {
+                mSupportedPackages.clear();
             }
         }
 
         @Override
-        public void cancel(String packageName) throws RemoteException {
-            mRequestedPackages.remove(packageName);
-        }
-
-        @Override
-        public void getSupportedPackages(Consumer<List<String>> consumer) throws RemoteException {
-            consumer.accept(mIsEnabled ? mSupportedPackages : Collections.emptyList());
-        }
-
-        @Override
-        public void getRequestedPackages(Consumer<List<String>> consumer) throws RemoteException {
-            // Pass copy to prevent ConcurrentModificationException during test
-            consumer.accept(
-                    mIsEnabled ? new ArrayList<>(mRequestedPackages) : Collections.emptyList());
-        }
-
-        @Override
-        public void setEnabled(boolean enabled) {
-            mIsEnabled = enabled;
-            mStateChangedRunnable.run();
+        public void setCallbacks(Consumer<String> passedConsumer,
+                Consumer<List<String>> supportedConsumer, Runnable notifySyncRunnable) {
+            mPassedConsumer = passedConsumer;
+            mSupportedConsumer = supportedConsumer;
+            mNotifySyncRunnable = notifySyncRunnable;
         }
 
         @Override
-        public void setCallbacks(Runnable stateChangedRunnable, Consumer<String> passedConsumer) {
-            mStateChangedRunnable = stateChangedRunnable;
-            mPassedConsumer = passedConsumer;
+        public void syncRequests(Set<String> packages) {
+            mRequestedPackages.clear();
+            if (mIsEnabled) {
+                packages.retainAll(mSupportedPackages);
+                mRequestedPackages.addAll(packages);
+            }
+            mSupportedConsumer.accept(mSupportedPackages);
         }
 
         public void setSupportedPackages(List<String> packages) {