Merge "Fixed PackageWatchdog health check state" into qt-dev

services/core/java/com/android/server/ExplicitHealthCheckController.java

@@ -35,7 +35,9 @@ import android.os.RemoteException;
 import android.os.UserHandle;
 import android.service.watchdog.ExplicitHealthCheckService;
 import android.service.watchdog.IExplicitHealthCheckService;
+import android.service.watchdog.PackageInfo;
 import android.text.TextUtils;
+import android.util.ArraySet;
 import android.util.Slog;
 
 import com.android.internal.annotations.GuardedBy;
@@ -69,7 +71,7 @@ class ExplicitHealthCheckController {
     // 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;
+    @GuardedBy("mLock") @Nullable private Consumer<List<PackageInfo>> 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
@@ -104,7 +106,7 @@ class ExplicitHealthCheckController {
      * 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) {
+            Consumer<List<PackageInfo>> supportedConsumer, Runnable notifySyncRunnable) {
         synchronized (mLock) {
             if (mPassedConsumer != null || mSupportedConsumer != null
                     || mNotifySyncRunnable != null) {
@@ -144,14 +146,18 @@ class ExplicitHealthCheckController {
             return;
         }
 
-        getSupportedPackages(supportedPackages -> {
+        getSupportedPackages(supportedPackageInfos -> {
             // Notify the watchdog without lock held
-            mSupportedConsumer.accept(supportedPackages);
+            mSupportedConsumer.accept(supportedPackageInfos);
             getRequestedPackages(previousRequestedPackages -> {
                 synchronized (mLock) {
                     // Hold lock so requests and cancellations are sent atomically.
                     // It is important we don't mix requests from multiple threads.
 
+                    Set<String> supportedPackages = new ArraySet<>();
+                    for (PackageInfo info : supportedPackageInfos) {
+                        supportedPackages.add(info.getPackageName());
+                    }
                     // Note, this may modify newRequestedPackages
                     newRequestedPackages.retainAll(supportedPackages);
 
@@ -229,7 +235,7 @@ class ExplicitHealthCheckController {
      * Returns the packages that we can request explicit health checks for.
      * The packages will be returned to the {@code consumer}.
      */
-    private void getSupportedPackages(Consumer<List<String>> consumer) {
+    private void getSupportedPackages(Consumer<List<PackageInfo>> consumer) {
         synchronized (mLock) {
             if (!prepareServiceLocked("get health check supported packages")) {
                 return;
@@ -238,7 +244,8 @@ class ExplicitHealthCheckController {
             Slog.d(TAG, "Getting health check supported packages");
             try {
                 mRemoteService.getSupportedPackages(new RemoteCallback(result -> {
-                    List<String> packages = result.getStringArrayList(EXTRA_SUPPORTED_PACKAGES);
+                    List<PackageInfo> packages =
+                            result.getParcelableArrayList(EXTRA_SUPPORTED_PACKAGES);
                     Slog.i(TAG, "Explicit health check supported packages " + packages);
                     consumer.accept(packages);
                 }));

services/core/java/com/android/server/PackageWatchdog.java

@@ -27,6 +27,7 @@ import android.os.Environment;
 import android.os.Handler;
 import android.os.Looper;
 import android.os.SystemClock;
+import android.service.watchdog.PackageInfo;
 import android.text.TextUtils;
 import android.util.ArrayMap;
 import android.util.ArraySet;
@@ -57,6 +58,7 @@ import java.util.ArrayList;
 import java.util.Collections;
 import java.util.Iterator;
 import java.util.List;
+import java.util.Map;
 import java.util.Set;
 
 /**
@@ -102,16 +104,10 @@ public class PackageWatchdog {
     private boolean mIsHealthCheckEnabled = true;
     @GuardedBy("mLock")
     private boolean mIsPackagesReady;
-    // SystemClock#uptimeMillis when we last executed #pruneObservers.
+    // SystemClock#uptimeMillis when we last executed #syncState
     // 0 if no prune is scheduled.
     @GuardedBy("mLock")
-    private long mUptimeAtLastPruneMs;
+    private long mUptimeAtLastStateSync;
-    // Duration in millis that the last prune was scheduled for.
-    // Used along with #mUptimeAtLastPruneMs after scheduling a prune to determine the remaining
-    // duration before #pruneObservers will be executed.
-    // 0 if no prune is scheduled.
-    @GuardedBy("mLock")
-    private long mDurationAtLastPrune;
 
     private PackageWatchdog(Context context) {
         // Needs to be constructed inline
@@ -156,7 +152,7 @@ public class PackageWatchdog {
             mHealthCheckController.setCallbacks(packageName -> onHealthCheckPassed(packageName),
                     packages -> onSupportedPackages(packages),
                     () -> syncRequestsAsync());
-            // Controller is initially disabled until here where we may enable it and sync requests
+            // Controller is initially disabled until here where we may enable it and sync our state
             setExplicitHealthCheckEnabled(mIsHealthCheckEnabled);
         }
     }
@@ -173,10 +169,6 @@ public class PackageWatchdog {
             if (internalObserver != null) {
                 internalObserver.mRegisteredObserver = observer;
             }
-            if (mDurationAtLastPrune == 0) {
-                // Nothing running, prune
-                pruneAndSchedule();
-            }
         }
     }
 
@@ -214,6 +206,12 @@ public class PackageWatchdog {
             packages.add(new MonitoredPackage(packageNames.get(i), durationMs, false));
         }
 
+        // Sync before we add the new packages to the observers. This will #pruneObservers,
+        // causing any elapsed time to be deducted from all existing packages before we add new
+        // packages. This maintains the invariant that the elapsed time for ALL (new and existing)
+        // packages is the same.
+        syncState("observing new packages");
+
         synchronized (mLock) {
             ObserverInternal oldObserver = mAllObservers.get(observer.getName());
             if (oldObserver == null) {
@@ -224,16 +222,16 @@ public class PackageWatchdog {
             } else {
                 Slog.d(TAG, observer.getName() + " added the following "
                         + "packages to monitor " + packageNames);
-                oldObserver.updatePackages(packages);
+                oldObserver.updatePackagesLocked(packages);
             }
         }
+
+        // Register observer in case not already registered
         registerHealthObserver(observer);
-        // Always prune because we may have received packges requiring an earlier
+
-        // schedule than we are currently scheduled for.
+        // Sync after we add the new packages to the observers. We may have received packges
-        pruneAndSchedule();
+        // requiring an earlier schedule than we are currently scheduled for.
-        Slog.i(TAG, "Syncing health check requests, observing packages " + packageNames);
+        syncState("updated observers");
-        syncRequestsAsync();
-        saveToFileAsync();
     }
 
     /**
@@ -245,7 +243,7 @@ public class PackageWatchdog {
         synchronized (mLock) {
             mAllObservers.remove(observer.getName());
         }
-        saveToFileAsync();
+        syncState("unregistering observer: " + observer.getName());
     }
 
     /**
@@ -296,7 +294,8 @@ public class PackageWatchdog {
                         ObserverInternal observer = mAllObservers.valueAt(oIndex);
                         PackageHealthObserver registeredObserver = observer.mRegisteredObserver;
                         if (registeredObserver != null
-                                && observer.onPackageFailure(versionedPackage.getPackageName())) {
+                                && observer.onPackageFailureLocked(
+                                        versionedPackage.getPackageName())) {
                             int impact = registeredObserver.onHealthCheckFailed(versionedPackage);
                             if (impact != PackageHealthObserverImpact.USER_IMPACT_NONE
                                     && impact < currentObserverImpact) {
@@ -321,9 +320,11 @@ public class PackageWatchdog {
     /** Writes the package information to file during shutdown. */
     public void writeNow() {
         synchronized (mLock) {
+            // Must only run synchronous tasks as this runs on the ShutdownThread and no other
+            // thread is guaranteed to run during shutdown.
             if (!mAllObservers.isEmpty()) {
-                mLongTaskHandler.removeCallbacks(this::saveToFile);
+                mLongTaskHandler.removeCallbacks(this::saveToFileAsync);
-                pruneObservers(SystemClock.uptimeMillis() - mUptimeAtLastPruneMs);
+                pruneObserversLocked();
                 saveToFile();
                 Slog.i(TAG, "Last write to update package durations");
             }
@@ -341,9 +342,8 @@ public class PackageWatchdog {
         synchronized (mLock) {
             mIsHealthCheckEnabled = enabled;
             mHealthCheckController.setEnabled(enabled);
-            Slog.i(TAG, "Syncing health check requests, explicit health check is "
+            // Prune to update internal state whenever health check is enabled/disabled
-                    + (enabled ? "enabled" : "disabled"));
+            syncState("health check state " + (enabled ? "enabled" : "disabled"));
-            syncRequestsAsync();
         }
     }
 
@@ -393,9 +393,8 @@ public class PackageWatchdog {
      * Serializes and syncs health check requests with the {@link ExplicitHealthCheckController}.
      */
     private void syncRequestsAsync() {
-        if (!mShortTaskHandler.hasCallbacks(this::syncRequests)) {
+        mShortTaskHandler.removeCallbacks(this::syncRequests);
-            mShortTaskHandler.post(this::syncRequests);
+        mShortTaskHandler.post(this::syncRequests);
-        }
     }
 
     /**
@@ -414,6 +413,7 @@ public class PackageWatchdog {
 
         // Call outside lock to avoid holding lock when calling into the controller.
         if (packages != null) {
+            Slog.i(TAG, "Syncing health check requests for packages: " + packages);
             mHealthCheckController.syncRequests(packages);
         }
     }
@@ -426,86 +426,73 @@ public class PackageWatchdog {
      * effectively behave as if the explicit health check hasn't passed for {@code packageName}.
      *
      * <p> {@code packageName} can still be considered failed if reported by
-     * {@link #onPackageFailure} before the package expires.
+     * {@link #onPackageFailureLocked} before the package expires.
      *
      * <p> Triggered by components outside the system server when they are fully functional after an
      * update.
      */
     private void onHealthCheckPassed(String packageName) {
         Slog.i(TAG, "Health check passed for package: " + packageName);
-        boolean shouldUpdateFile = false;
+        boolean isStateChanged = false;
+
         synchronized (mLock) {
             for (int observerIdx = 0; observerIdx < mAllObservers.size(); observerIdx++) {
                 ObserverInternal observer = mAllObservers.valueAt(observerIdx);
                 MonitoredPackage monitoredPackage = observer.mPackages.get(packageName);
-                if (monitoredPackage != null && !monitoredPackage.mHasPassedHealthCheck) {
+
-                    monitoredPackage.mHasPassedHealthCheck = true;
+                if (monitoredPackage != null) {
-                    shouldUpdateFile = true;
+                    int oldState = monitoredPackage.getHealthCheckStateLocked();
+                    int newState = monitoredPackage.tryPassHealthCheckLocked();
+                    isStateChanged |= oldState != newState;
                 }
             }
         }
 
-        // So we can unbind from the service if this was the last result we expected
+        if (isStateChanged) {
-        Slog.i(TAG, "Syncing health check requests, health check passed for " + packageName);
+            syncState("health check passed for " + packageName);
-        syncRequestsAsync();
-
-        if (shouldUpdateFile) {
-            saveToFileAsync();
         }
     }
 
-    private void onSupportedPackages(List<String> supportedPackages) {
+    private void onSupportedPackages(List<PackageInfo> supportedPackages) {
-        boolean shouldUpdateFile = false;
+        boolean isStateChanged = false;
-        boolean shouldPrune = false;
+
+        Map<String, Long> supportedPackageTimeouts = new ArrayMap<>();
+        Iterator<PackageInfo> it = supportedPackages.iterator();
+        while (it.hasNext()) {
+            PackageInfo info = it.next();
+            supportedPackageTimeouts.put(info.getPackageName(), info.getHealthCheckTimeoutMillis());
+        }
 
         synchronized (mLock) {
             Slog.d(TAG, "Received supported packages " + supportedPackages);
             Iterator<ObserverInternal> oit = mAllObservers.values().iterator();
             while (oit.hasNext()) {
-                ObserverInternal observer = oit.next();
+                Iterator<MonitoredPackage> pit = oit.next().mPackages.values().iterator();
-                Iterator<MonitoredPackage> pit =
-                        observer.mPackages.values().iterator();
                 while (pit.hasNext()) {
                     MonitoredPackage monitoredPackage = pit.next();
-                    String packageName = monitoredPackage.mName;
+                    String packageName = monitoredPackage.getName();
-                    int healthCheckState = monitoredPackage.getHealthCheckState();
+                    int oldState = monitoredPackage.getHealthCheckStateLocked();
+                    int newState;
 
-                    if (healthCheckState != MonitoredPackage.STATE_PASSED) {
+                    if (supportedPackageTimeouts.containsKey(packageName)) {
-                        // Have to update file, we will either transition state or reduce
+                        // Supported packages become ACTIVE if currently INACTIVE
-                        // health check duration
+                        newState = monitoredPackage.setHealthCheckActiveLocked(
-                        shouldUpdateFile = true;
+                                supportedPackageTimeouts.get(packageName));
-
-                        if (supportedPackages.contains(packageName)) {
-                            // Supports health check, transition to ACTIVE if not already.
-                            // We need to prune packages earlier than already scheduled.
-                            shouldPrune = true;
-
-                            // TODO: Get healthCheckDuration from supportedPackages
-                            long healthCheckDuration = monitoredPackage.mDurationMs;
-                            monitoredPackage.mHealthCheckDurationMs = Math.min(healthCheckDuration,
-                                    monitoredPackage.mDurationMs);
-                            Slog.i(TAG, packageName + " health check state is now: ACTIVE("
-                                    + monitoredPackage.mHealthCheckDurationMs + "ms)");
-                        } else {
-                            // Does not support health check, transistion to PASSED
-                            monitoredPackage.mHasPassedHealthCheck = true;
-                            Slog.i(TAG, packageName + " health check state is now: PASSED");
-                        }
                     } else {
-                        Slog.i(TAG, packageName + " does not support health check, state: PASSED");
+                        // Unsupported packages are marked as PASSED unless already FAILED
+                        newState = monitoredPackage.tryPassHealthCheckLocked();
                     }
+                    isStateChanged |= oldState != newState;
                 }
             }
         }
 
-        if (shouldUpdateFile) {
+        if (isStateChanged) {
-            saveToFileAsync();
+            syncState("updated health check supported packages " + supportedPackages);
-        }
-        if (shouldPrune) {
-            pruneAndSchedule();
         }
     }
 
+    @GuardedBy("mLock")
     private Set<String> getPackagesPendingHealthChecksLocked() {
         Slog.d(TAG, "Getting all observed packages pending health checks");
         Set<String> packages = new ArraySet<>();
@@ -516,8 +503,9 @@ public class PackageWatchdog {
                     observer.mPackages.values().iterator();
             while (pit.hasNext()) {
                 MonitoredPackage monitoredPackage = pit.next();
-                String packageName = monitoredPackage.mName;
+                String packageName = monitoredPackage.getName();
-                if (!monitoredPackage.mHasPassedHealthCheck) {
+                if (monitoredPackage.getHealthCheckStateLocked()
+                        != MonitoredPackage.STATE_PASSED) {
                     packages.add(packageName);
                 }
             }
@@ -525,88 +513,91 @@ public class PackageWatchdog {
         return packages;
     }
 
-    /** Executes {@link #pruneObservers} and schedules the next execution. */
+    /**
-    private void pruneAndSchedule() {
+     * Syncs the state of the observers.
+     *
+     * <p> Prunes all observers, saves new state to disk, syncs health check requests with the
+     * health check service and schedules the next state sync.
+     */
+    private void syncState(String reason) {
         synchronized (mLock) {
-            long nextDurationToScheduleMs = getNextPruneScheduleMillisLocked();
+            Slog.i(TAG, "Syncing state, reason: " + reason);
-            if (nextDurationToScheduleMs == Long.MAX_VALUE) {
+            pruneObserversLocked();
-                Slog.i(TAG, "No monitored packages, ending prune");
+
-                mDurationAtLastPrune = 0;
+            saveToFileAsync();
-                mUptimeAtLastPruneMs = 0;
+            syncRequestsAsync();
-                return;
+
-            }
+            // Done syncing state, schedule the next state sync
-            long uptimeMs = SystemClock.uptimeMillis();
+            scheduleNextSyncStateLocked();
-            // O if not running
+        }
-            long elapsedDurationMs = mUptimeAtLastPruneMs == 0
+    }
-                    ? 0 : uptimeMs - mUptimeAtLastPruneMs;
+
-            // Less than O if unexpectedly didn't run yet even though
+    private void syncStateWithScheduledReason() {
-            // we are past the last duration scheduled to run
+        syncState("scheduled");
-            long remainingDurationMs = mDurationAtLastPrune - elapsedDurationMs;
+    }
-            if (mUptimeAtLastPruneMs == 0
+
-                    || remainingDurationMs <= 0
+    @GuardedBy("mLock")
-                    || nextDurationToScheduleMs < remainingDurationMs) {
+    private void scheduleNextSyncStateLocked() {
-                // First schedule or an earlier reschedule
+        long durationMs = getNextStateSyncMillisLocked();
-                pruneObservers(elapsedDurationMs);
+        mShortTaskHandler.removeCallbacks(this::syncStateWithScheduledReason);
-                // We don't use Handler#hasCallbacks because we want to update the schedule delay
+        if (durationMs == Long.MAX_VALUE) {
-                mShortTaskHandler.removeCallbacks(this::pruneAndSchedule);
+            Slog.i(TAG, "Cancelling state sync, nothing to sync");
-                mShortTaskHandler.postDelayed(this::pruneAndSchedule, nextDurationToScheduleMs);
+            mUptimeAtLastStateSync = 0;
-                mDurationAtLastPrune = nextDurationToScheduleMs;
+        } else {
-                mUptimeAtLastPruneMs = uptimeMs;
+            Slog.i(TAG, "Scheduling next state sync in " + durationMs + "ms");
-            }
+            mUptimeAtLastStateSync = SystemClock.uptimeMillis();
+            mShortTaskHandler.postDelayed(this::syncStateWithScheduledReason, durationMs);
         }
     }
 
     /**
-     * Returns the next time in millis to schedule a prune.
+     * Returns the next duration in millis to sync the watchdog state.
      *
      * @returns Long#MAX_VALUE if there are no observed packages.
      */
-    private long getNextPruneScheduleMillisLocked() {
+    @GuardedBy("mLock")
+    private long getNextStateSyncMillisLocked() {
         long shortestDurationMs = Long.MAX_VALUE;
         for (int oIndex = 0; oIndex < mAllObservers.size(); oIndex++) {
             ArrayMap<String, MonitoredPackage> packages = mAllObservers.valueAt(oIndex).mPackages;
             for (int pIndex = 0; pIndex < packages.size(); pIndex++) {
                 MonitoredPackage mp = packages.valueAt(pIndex);
-                long duration = Math.min(mp.mDurationMs, mp.mHealthCheckDurationMs);
+                long duration = mp.getShortestScheduleDurationMsLocked();
                 if (duration < shortestDurationMs) {
                     shortestDurationMs = duration;
                 }
             }
         }
-        Slog.i(TAG, "Next prune will be scheduled in " + shortestDurationMs + "ms");
-
         return shortestDurationMs;
     }
 
     /**
-     * Removes {@code elapsedMs} milliseconds from all durations on monitored packages.
+     * Removes {@code elapsedMs} milliseconds from all durations on monitored packages
-     *
+     * and updates other internal state.
-     * <p> Prunes all observers with {@link ObserverInternal#prunePackages} and discards observers
-     * without any packages left.
      */
-    private void pruneObservers(long elapsedMs) {
+    @GuardedBy("mLock")
-        if (elapsedMs == 0) {
+    private void pruneObserversLocked() {
+        long elapsedMs = mUptimeAtLastStateSync == 0
+                ? 0 : SystemClock.uptimeMillis() - mUptimeAtLastStateSync;
+        if (elapsedMs <= 0) {
+            Slog.i(TAG, "Not pruning observers, elapsed time: " + elapsedMs + "ms");
             return;
         }
-        synchronized (mLock) {
+
-            Slog.d(TAG, "Removing expired packages after " + elapsedMs + "ms");
+        Slog.i(TAG, "Removing " + elapsedMs + "ms from all packages on all observers");
-            Iterator<ObserverInternal> it = mAllObservers.values().iterator();
+        Iterator<ObserverInternal> it = mAllObservers.values().iterator();
-            while (it.hasNext()) {
+        while (it.hasNext()) {
-                ObserverInternal observer = it.next();
+            ObserverInternal observer = it.next();
-                Set<MonitoredPackage> failedPackages =
+            Set<MonitoredPackage> failedPackages =
-                        observer.prunePackages(elapsedMs);
+                    observer.prunePackagesLocked(elapsedMs);
-                if (!failedPackages.isEmpty()) {
+            if (!failedPackages.isEmpty()) {
-                    onHealthCheckFailed(observer, failedPackages);
+                onHealthCheckFailed(observer, failedPackages);
-                }
+            }
-                if (observer.mPackages.isEmpty()) {
+            if (observer.mPackages.isEmpty()) {
-                    Slog.i(TAG, "Discarding observer " + observer.mName + ". All packages expired");
+                Slog.i(TAG, "Discarding observer " + observer.mName + ". All packages expired");
-                    it.remove();
+                it.remove();
-                }
             }
         }
-        Slog.i(TAG, "Syncing health check requests, pruned observers");
-        syncRequestsAsync();
-        saveToFileAsync();
     }
 
     private void onHealthCheckFailed(ObserverInternal observer,
@@ -618,7 +609,7 @@ public class PackageWatchdog {
                     PackageManager pm = mContext.getPackageManager();
                     Iterator<MonitoredPackage> it = failedPackages.iterator();
                     while (it.hasNext()) {
-                        String failedPackage = it.next().mName;
+                        String failedPackage = it.next().getName();
                         long versionCode = 0;
                         Slog.i(TAG, "Explicit health check failed for package " + failedPackage);
                         try {
@@ -673,6 +664,7 @@ public class PackageWatchdog {
      * Persists mAllObservers to file. Threshold information is ignored.
      */
     private boolean saveToFile() {
+        Slog.i(TAG, "Saving observer state to file");
         synchronized (mLock) {
             FileOutputStream stream;
             try {
@@ -689,7 +681,7 @@ public class PackageWatchdog {
                 out.startTag(null, TAG_PACKAGE_WATCHDOG);
                 out.attribute(null, ATTR_VERSION, Integer.toString(DB_VERSION));
                 for (int oIndex = 0; oIndex < mAllObservers.size(); oIndex++) {
-                    mAllObservers.valueAt(oIndex).write(out);
+                    mAllObservers.valueAt(oIndex).writeLocked(out);
                 }
                 out.endTag(null, TAG_PACKAGE_WATCHDOG);
                 out.endDocument();
@@ -730,7 +722,7 @@ public class PackageWatchdog {
 
         ObserverInternal(String name, List<MonitoredPackage> packages) {
             mName = name;
-            updatePackages(packages);
+            updatePackagesLocked(packages);
         }
 
         /**
@@ -738,20 +730,13 @@ public class PackageWatchdog {
          * Does not persist any package failure thresholds.
          */
         @GuardedBy("mLock")
-        public boolean write(XmlSerializer out) {
+        public boolean writeLocked(XmlSerializer out) {
             try {
                 out.startTag(null, TAG_OBSERVER);
                 out.attribute(null, ATTR_NAME, mName);
                 for (int i = 0; i < mPackages.size(); i++) {
                     MonitoredPackage p = mPackages.valueAt(i);
-                    out.startTag(null, TAG_PACKAGE);
+                    p.writeLocked(out);
-                    out.attribute(null, ATTR_NAME, p.mName);
-                    out.attribute(null, ATTR_DURATION, String.valueOf(p.mDurationMs));
-                    out.attribute(null, ATTR_EXPLICIT_HEALTH_CHECK_DURATION,
-                            String.valueOf(p.mHealthCheckDurationMs));
-                    out.attribute(null, ATTR_PASSED_HEALTH_CHECK,
-                            String.valueOf(p.mHasPassedHealthCheck));
-                    out.endTag(null, TAG_PACKAGE);
                 }
                 out.endTag(null, TAG_OBSERVER);
                 return true;
@@ -762,7 +747,7 @@ public class PackageWatchdog {
         }
 
         @GuardedBy("mLock")
-        public void updatePackages(List<MonitoredPackage> packages) {
+        public void updatePackagesLocked(List<MonitoredPackage> packages) {
             for (int pIndex = 0; pIndex < packages.size(); pIndex++) {
                 MonitoredPackage p = packages.get(pIndex);
                 mPackages.put(p.mName, p);
@@ -775,37 +760,24 @@ public class PackageWatchdog {
          * observation. If any health check duration is less than 0, the health check result
          * is evaluated.
          *
-         * @returns a {@link Set} of packages that were removed from the observer without explicit
+         * @return a {@link Set} of packages that were removed from the observer without explicit
          * health check passing, or an empty list if no package expired for which an explicit health
          * check was still pending
          */
         @GuardedBy("mLock")
-        private Set<MonitoredPackage> prunePackages(long elapsedMs) {
+        private Set<MonitoredPackage> prunePackagesLocked(long elapsedMs) {
             Set<MonitoredPackage> failedPackages = new ArraySet<>();
             Iterator<MonitoredPackage> it = mPackages.values().iterator();
             while (it.hasNext()) {
                 MonitoredPackage p = it.next();
-                int healthCheckState = p.getHealthCheckState();
+                int oldState = p.getHealthCheckStateLocked();
-
+                int newState = p.handleElapsedTimeLocked(elapsedMs);
-                // Handle health check timeouts
+                if (oldState != MonitoredPackage.STATE_FAILED
-                if (healthCheckState == MonitoredPackage.STATE_ACTIVE) {
+                        && newState == MonitoredPackage.STATE_FAILED) {
-                    // Only reduce duration if state is active
+                    Slog.i(TAG, "Package " + p.mName + " failed health check");
-                    p.mHealthCheckDurationMs -= elapsedMs;
+                    failedPackages.add(p);
-                    // Check duration after reducing duration
-                    if (p.mHealthCheckDurationMs <= 0) {
-                        failedPackages.add(p);
-                    }
                 }
-
+                if (p.isExpiredLocked()) {
-                // Handle package expiry
-                p.mDurationMs -= elapsedMs;
-                // Check duration after reducing duration
-                if (p.mDurationMs <= 0) {
-                    if (healthCheckState == MonitoredPackage.STATE_INACTIVE) {
-                        Slog.w(TAG, "Package " + p.mName
-                                + " expiring without starting health check, failing");
-                        failedPackages.add(p);
-                    }
                     it.remove();
                 }
             }
@@ -817,10 +789,10 @@ public class PackageWatchdog {
          * @returns {@code true} if failure threshold is exceeded, {@code false} otherwise
          */
         @GuardedBy("mLock")
-        public boolean onPackageFailure(String packageName) {
+        public boolean onPackageFailureLocked(String packageName) {
             MonitoredPackage p = mPackages.get(packageName);
             if (p != null) {
-                return p.onFailure();
+                return p.onFailureLocked();
             }
             return false;
         }
@@ -877,33 +849,45 @@ public class PackageWatchdog {
     }
 
     /**
-     * Represents a package along with the time it should be monitored for.
+     * Represents a package and its health check state along with the time
+     * it should be monitored for.
      *
      * <p> Note, the PackageWatchdog#mLock must always be held when reading or writing
      * instances of this class.
      */
-    //TODO(b/120598832): Remove 'm' from non-private fields
+    static class MonitoredPackage {
-    private static class MonitoredPackage {
         // Health check states
+        // TODO(b/120598832): Prefix with HEALTH_CHECK
         // mName has not passed health check but has requested a health check
-        public static int STATE_ACTIVE = 0;
+        public static final int STATE_ACTIVE = 0;
         // mName has not passed health check and has not requested a health check
-        public static int STATE_INACTIVE = 1;
+        public static final int STATE_INACTIVE = 1;
         // mName has passed health check
-        public static int STATE_PASSED = 2;
+        public static final int STATE_PASSED = 2;
+        // mName has failed health check
+        public static final int STATE_FAILED = 3;
 
-        public final String mName;
+        //TODO(b/120598832): VersionedPackage?
-        // Whether an explicit health check has passed
+        private final String mName;
+        // One of STATE_[ACTIVE|INACTIVE|PASSED|FAILED]. Updated on construction and after
+        // methods that could change the health check state: handleElapsedTimeLocked and
+        // tryPassHealthCheckLocked
+        private int mHealthCheckState = STATE_INACTIVE;
+        // Whether an explicit health check has passed.
+        // This value in addition with mHealthCheckDurationMs determines the health check state
+        // of the package, see #getHealthCheckStateLocked
         @GuardedBy("mLock")
-        public boolean mHasPassedHealthCheck;
+        private boolean mHasPassedHealthCheck;
-        // System uptime duration to monitor package
+        // System uptime duration to monitor package.
         @GuardedBy("mLock")
-        public long mDurationMs;
+        private long mDurationMs;
         // System uptime duration to check the result of an explicit health check
         // Initially, MAX_VALUE until we get a value from the health check service
         // and request health checks.
+        // This value in addition with mHasPassedHealthCheck determines the health check state
+        // of the package, see #getHealthCheckStateLocked
         @GuardedBy("mLock")
-        public long mHealthCheckDurationMs = Long.MAX_VALUE;
+        private long mHealthCheckDurationMs = Long.MAX_VALUE;
         // System uptime of first package failure
         @GuardedBy("mLock")
         private long mUptimeStartMs;
@@ -921,6 +905,20 @@ public class PackageWatchdog {
             mDurationMs = durationMs;
             mHealthCheckDurationMs = healthCheckDurationMs;
             mHasPassedHealthCheck = hasPassedHealthCheck;
+            updateHealthCheckStateLocked();
+        }
+
+        /** Writes the salient fields to disk using {@code out}. */
+        @GuardedBy("mLock")
+        public void writeLocked(XmlSerializer out) throws IOException {
+            out.startTag(null, TAG_PACKAGE);
+            out.attribute(null, ATTR_NAME, mName);
+            out.attribute(null, ATTR_DURATION, String.valueOf(mDurationMs));
+            out.attribute(null, ATTR_EXPLICIT_HEALTH_CHECK_DURATION,
+                    String.valueOf(mHealthCheckDurationMs));
+            out.attribute(null, ATTR_PASSED_HEALTH_CHECK,
+                    String.valueOf(mHasPassedHealthCheck));
+            out.endTag(null, TAG_PACKAGE);
         }
 
         /**
@@ -929,7 +927,7 @@ public class PackageWatchdog {
          * @return {@code true} if failure count exceeds a threshold, {@code false} otherwise
          */
         @GuardedBy("mLock")
-        public boolean onFailure() {
+        public boolean onFailureLocked() {
             final long now = SystemClock.uptimeMillis();
             final long duration = now - mUptimeStartMs;
             if (duration > TRIGGER_DURATION_MS) {
@@ -949,18 +947,141 @@ public class PackageWatchdog {
         }
 
         /**
-         * Returns any of the health check states of {@link #STATE_ACTIVE},
+         * Sets the initial health check duration.
+         *
+         * @return the new health check state
+         */
+        @GuardedBy("mLock")
+        public int setHealthCheckActiveLocked(long initialHealthCheckDurationMs) {
+            if (initialHealthCheckDurationMs <= 0) {
+                Slog.wtf(TAG, "Cannot set non-positive health check duration "
+                        + initialHealthCheckDurationMs + "ms for package " + mName
+                        + ". Using total duration " + mDurationMs + "ms instead");
+                initialHealthCheckDurationMs = mDurationMs;
+            }
+            if (mHealthCheckState == STATE_INACTIVE) {
+                // Transitions to ACTIVE
+                mHealthCheckDurationMs = initialHealthCheckDurationMs;
+            }
+            return updateHealthCheckStateLocked();
+        }
+
+        /**
+         * Updates the monitoring durations of the package.
+         *
+         * @return the new health check state
+         */
+        @GuardedBy("mLock")
+        public int handleElapsedTimeLocked(long elapsedMs) {
+            if (elapsedMs <= 0) {
+                Slog.w(TAG, "Cannot handle non-positive elapsed time for package " + mName);
+                return mHealthCheckState;
+            }
+            // Transitions to FAILED if now <= 0 and health check not passed
+            mDurationMs -= elapsedMs;
+            if (mHealthCheckState == STATE_ACTIVE) {
+                // We only update health check durations if we have #setHealthCheckActiveLocked
+                // This ensures we don't leave the INACTIVE state for an unexpected elapsed time
+                // Transitions to FAILED if now <= 0 and health check not passed
+                mHealthCheckDurationMs -= elapsedMs;
+            }
+            return updateHealthCheckStateLocked();
+        }
+
+        /**
+         * Marks the health check as passed and transitions to {@link #STATE_PASSED}
+         * if not yet {@link #STATE_FAILED}.
+         *
+         * @return the new health check state
+         */
+        @GuardedBy("mLock")
+        public int tryPassHealthCheckLocked() {
+            if (mHealthCheckState != STATE_FAILED) {
+                // FAILED is a final state so only pass if we haven't failed
+                // Transition to PASSED
+                mHasPassedHealthCheck = true;
+            }
+            return updateHealthCheckStateLocked();
+        }
+
+        /** Returns the monitored package name. */
+        private String getName() {
+            return mName;
+        }
+
+        //TODO(b/120598832): IntDef
+        /**
+         * Returns the current health check state, any of {@link #STATE_ACTIVE},
          * {@link #STATE_INACTIVE} or {@link #STATE_PASSED}
          */
         @GuardedBy("mLock")
-        public int getHealthCheckState() {
+        public int getHealthCheckStateLocked() {
+            return mHealthCheckState;
+        }
+
+        /**
+         * Returns the shortest duration before the package should be scheduled for a prune.
+         *
+         * @return the duration or {@link Long#MAX_VALUE} if the package should not be scheduled
+         */
+        @GuardedBy("mLock")
+        public long getShortestScheduleDurationMsLocked() {
+            return Math.min(toPositive(mDurationMs), toPositive(mHealthCheckDurationMs));
+        }
+
+        /**
+         * Returns {@code true} if the total duration left to monitor the package is less than or
+         * equal to 0 {@code false} otherwise.
+         */
+        @GuardedBy("mLock")
+        public boolean isExpiredLocked() {
+            return mDurationMs <= 0;
+        }
+
+        /**
+         * Updates the health check state based on {@link #mHasPassedHealthCheck}
+         * and {@link #mHealthCheckDurationMs}.
+         *
+         * @return the new health check state
+         */
+        @GuardedBy("mLock")
+        private int updateHealthCheckStateLocked() {
+            int oldState = mHealthCheckState;
             if (mHasPassedHealthCheck) {
-                return STATE_PASSED;
+                // Set final state first to avoid ambiguity
+                mHealthCheckState = STATE_PASSED;
+            } else if (mHealthCheckDurationMs <= 0 || mDurationMs <= 0) {
+                // Set final state first to avoid ambiguity
+                mHealthCheckState = STATE_FAILED;
             } else if (mHealthCheckDurationMs == Long.MAX_VALUE) {
-                return STATE_INACTIVE;
+                mHealthCheckState = STATE_INACTIVE;
             } else {
-                return STATE_ACTIVE;
+                mHealthCheckState = STATE_ACTIVE;
             }
+            Slog.i(TAG, "Updated health check state for package " + mName + ": "
+                    + toString(oldState) + " -> " + toString(mHealthCheckState));
+            return mHealthCheckState;
+        }
+
+        /** Returns a {@link String} representation of the current health check state. */
+        private static String toString(int state) {
+            switch (state) {
+                case STATE_ACTIVE:
+                    return "ACTIVE";
+                case STATE_INACTIVE:
+                    return "INACTIVE";
+                case STATE_PASSED:
+                    return "PASSED";
+                case STATE_FAILED:
+                    return "FAILED";
+                default:
+                    return "UNKNOWN";
+            }
+        }
+
+        /** Returns {@code value} if it is greater than 0 or {@link Long#MAX_VALUE} otherwise. */
+        private static long toPositive(long value) {
+            return value > 0 ? value : Long.MAX_VALUE;
         }
     }
 }

tests/PackageWatchdog/src/com/android/server/PackageWatchdogTest.java

@@ -16,6 +16,7 @@
 
 package com.android.server;
 
+import static com.android.server.PackageWatchdog.MonitoredPackage;
 import static com.android.server.PackageWatchdog.TRIGGER_FAILURE_COUNT;
 
 import static org.junit.Assert.assertEquals;
@@ -27,6 +28,7 @@ import android.content.Context;
 import android.content.pm.VersionedPackage;
 import android.os.Handler;
 import android.os.test.TestLooper;
+import android.service.watchdog.PackageInfo;
 import android.util.AtomicFile;
 
 import androidx.test.InstrumentationRegistry;
@@ -143,6 +145,31 @@ public class PackageWatchdogTest {
         assertNull(watchdog.getPackages(observer3));
     }
 
+    /** Observing already observed package extends the observation time. */
+    @Test
+    public void testObserveAlreadyObservedPackage() throws Exception {
+        PackageWatchdog watchdog = createWatchdog();
+        TestObserver observer = new TestObserver(OBSERVER_NAME_1);
+
+        // Start observing APP_A
+        watchdog.startObservingHealth(observer, Arrays.asList(APP_A), SHORT_DURATION);
+
+        // Then advance time half-way
+        Thread.sleep(SHORT_DURATION / 2);
+        mTestLooper.dispatchAll();
+
+        // Start observing APP_A again
+        watchdog.startObservingHealth(observer, Arrays.asList(APP_A), SHORT_DURATION);
+
+        // Then advance time such that it should have expired were it not for the second observation
+        Thread.sleep((SHORT_DURATION / 2) + 1);
+        mTestLooper.dispatchAll();
+
+        // Verify that APP_A not expired since second observation extended the time
+        assertEquals(1, watchdog.getPackages(observer).size());
+        assertTrue(watchdog.getPackages(observer).contains(APP_A));
+    }
+
     /**
      * Test package observers are persisted and loaded on startup
      */
@@ -577,6 +604,84 @@ public class PackageWatchdogTest {
         assertEquals(APP_C, observer.mFailedPackages.get(0));
     }
 
+    /**
+     * Tests failure when health check duration is different from package observation duration
+     * Failure is also notified only once.
+     */
+    @Test
+    public void testExplicitHealthCheckFailureBeforeExpiry() throws Exception {
+        TestController controller = new TestController();
+        PackageWatchdog watchdog = createWatchdog(controller, true /* withPackagesReady */);
+        TestObserver observer = new TestObserver(OBSERVER_NAME_1,
+                PackageHealthObserverImpact.USER_IMPACT_MEDIUM);
+
+        // Start observing with explicit health checks for APP_A and
+        // package observation duration == LONG_DURATION
+        // health check duration == SHORT_DURATION (set by default in the TestController)
+        controller.setSupportedPackages(Arrays.asList(APP_A));
+        watchdog.startObservingHealth(observer, Arrays.asList(APP_A), LONG_DURATION);
+
+        // Then APP_A has exceeded health check duration
+        Thread.sleep(SHORT_DURATION);
+        mTestLooper.dispatchAll();
+
+        // Verify that health check is failed
+        assertEquals(1, observer.mFailedPackages.size());
+        assertEquals(APP_A, observer.mFailedPackages.get(0));
+
+        // Then clear failed packages and start observing a random package so requests are synced
+        // and PackageWatchdog#onSupportedPackages is called and APP_A has a chance to fail again
+        // this time due to package expiry.
+        observer.mFailedPackages.clear();
+        watchdog.startObservingHealth(observer, Arrays.asList(APP_B), LONG_DURATION);
+
+        // Verify that health check failure is not notified again
+        assertTrue(observer.mFailedPackages.isEmpty());
+    }
+
+    /** Tests {@link MonitoredPackage} health check state transitions. */
+    @Test
+    public void testPackageHealthCheckStateTransitions() {
+        MonitoredPackage m1 = new MonitoredPackage(APP_A, LONG_DURATION,
+                false /* hasPassedHealthCheck */);
+        MonitoredPackage m2 = new MonitoredPackage(APP_B, LONG_DURATION, false);
+        MonitoredPackage m3 = new MonitoredPackage(APP_C, LONG_DURATION, false);
+        MonitoredPackage m4 = new MonitoredPackage(APP_D, LONG_DURATION, SHORT_DURATION, true);
+
+        // Verify transition: inactive -> active -> passed
+        // Verify initially inactive
+        assertEquals(MonitoredPackage.STATE_INACTIVE, m1.getHealthCheckStateLocked());
+        // Verify still inactive, until we #setHealthCheckActiveLocked
+        assertEquals(MonitoredPackage.STATE_INACTIVE, m1.handleElapsedTimeLocked(SHORT_DURATION));
+        // Verify now active
+        assertEquals(MonitoredPackage.STATE_ACTIVE, m1.setHealthCheckActiveLocked(SHORT_DURATION));
+        // Verify now passed
+        assertEquals(MonitoredPackage.STATE_PASSED, m1.tryPassHealthCheckLocked());
+
+        // Verify transition: inactive -> active -> failed
+        // Verify initially inactive
+        assertEquals(MonitoredPackage.STATE_INACTIVE, m2.getHealthCheckStateLocked());
+        // Verify now active
+        assertEquals(MonitoredPackage.STATE_ACTIVE, m2.setHealthCheckActiveLocked(SHORT_DURATION));
+        // Verify now failed
+        assertEquals(MonitoredPackage.STATE_FAILED, m2.handleElapsedTimeLocked(SHORT_DURATION));
+
+        // Verify transition: inactive -> failed
+        // Verify initially inactive
+        assertEquals(MonitoredPackage.STATE_INACTIVE, m3.getHealthCheckStateLocked());
+        // Verify now failed because package expired
+        assertEquals(MonitoredPackage.STATE_FAILED, m3.handleElapsedTimeLocked(LONG_DURATION));
+        // Verify remains failed even when asked to pass
+        assertEquals(MonitoredPackage.STATE_FAILED, m3.tryPassHealthCheckLocked());
+
+        // Verify transition: passed
+        assertEquals(MonitoredPackage.STATE_PASSED, m4.getHealthCheckStateLocked());
+        // Verify remains passed even if health check fails
+        assertEquals(MonitoredPackage.STATE_PASSED, m4.handleElapsedTimeLocked(SHORT_DURATION));
+        // Verify remains passed even if package expires
+        assertEquals(MonitoredPackage.STATE_PASSED, m4.handleElapsedTimeLocked(LONG_DURATION));
+    }
+
     private PackageWatchdog createWatchdog() {
         return createWatchdog(new TestController(), true /* withPackagesReady */);
     }
@@ -636,7 +741,7 @@ public class PackageWatchdogTest {
         private List<String> mSupportedPackages = new ArrayList<>();
         private List<String> mRequestedPackages = new ArrayList<>();
         private Consumer<String> mPassedConsumer;
-        private Consumer<List<String>> mSupportedConsumer;
+        private Consumer<List<PackageInfo>> mSupportedConsumer;
         private Runnable mNotifySyncRunnable;
 
         @Override
@@ -649,7 +754,7 @@ public class PackageWatchdogTest {
 
         @Override
         public void setCallbacks(Consumer<String> passedConsumer,
-                Consumer<List<String>> supportedConsumer, Runnable notifySyncRunnable) {
+                Consumer<List<PackageInfo>> supportedConsumer, Runnable notifySyncRunnable) {
             mPassedConsumer = passedConsumer;
             mSupportedConsumer = supportedConsumer;
             mNotifySyncRunnable = notifySyncRunnable;
@@ -661,7 +766,11 @@ public class PackageWatchdogTest {
             if (mIsEnabled) {
                 packages.retainAll(mSupportedPackages);
                 mRequestedPackages.addAll(packages);
-                mSupportedConsumer.accept(mSupportedPackages);
+                List<PackageInfo> packageInfos = new ArrayList<>();
+                for (String packageName: packages) {
+                    packageInfos.add(new PackageInfo(packageName, SHORT_DURATION));
+                }
+                mSupportedConsumer.accept(packageInfos);
             } else {
                 mSupportedConsumer.accept(Collections.emptyList());
             }