Fixed PackageWatchdog health check state

1. Receiving List<PackageInfo>:
Since I29e2d619a5296716c29893ab3aa2f35f69bfb4d7, we now receive a
List of PackageInfo instead of Strings for packages supporting
explicit health checks. Now, we parse this List<PackageInfo> from
ExtServices instead of trying to parse List<String> and we use the
health check timeout in the PackageInfo as the health check expiry
deadline instead of using the total package expiry time.

2. Updating health check durations onSupportedPackages:
Before, we always updated the health check duration for a
package if the package is supported and the health check state is
not PASSED, this caused the health check duration for a package to
never reduce as long as we kept getting onSupportedPackages. Now, we
improved the readability of the state transitions onSupportedPackages.
We now correctly only update the health check duration for supported
packages in the INACTIVE state.

3. FAILED state:
Before we only had INACTIVE, ACTIVE and PASSED states. When a package
has failed the health check we could notify the observer multiple
times in quick succession and get into a bad internal state with
negative health check durations. Now we added check to ensure we
don't try to schedule with a Handler with a negative duration and we
defined a negative health check duration to be a new FAILED state if the
health check is not passed. This clearly defines the state transitions
as seen below:

+----------+     +---------+    +------+
|          |     |         |    |      |
| INACTIVE +---->+ ACTIVE  +--->+PASSED|
|          |     |         |    |      |
+-----+----+     +----+----+    +------+
      |               |
      |               |
      |               |
      |               |
      |          +----v----+
      |          |         |
      +----------> FAILED  |
                 |         |
                 +---------+

4. Uptime state:
Everytime we pruned observers, we scheduled the next prune and stored
the current SystemClock#uptimeMillis. This allowed us determine how
much time had elapsed for the next prune. The uptime was not correclty
updated when starting to observe already observed packages. With the
following sequence of events:

-monitor package A for 1hr
-30mins elapsed
-monitor package A again for 1hr

A would expire 30mins from the last event instead of 1hr.
This was because the second time around, we
saved the new state to disk but did not reschedule so did not update
the uptime at last schedule, so 1hr from the first event, we would
prune packages with the original uptime and incorrectly expire A
earlier. Now we update all internal state, fixed this and added a test
for this case.

5. Readability
Improved method variable names, logging and comments.

Bug: 120598832
Test: Manual testing && atest PackageWatcdogTest
Change-Id: I1512d5938848ad26b668636405fe9b0db50d3a2e

3 files changed, 436 insertions, 199 deletions

diff --git a/services/core/java/com/android/server/ExplicitHealthCheckController.java b/services/core/java/com/android/server/ExplicitHealthCheckController.java
index 27ad208ef8a1..19ab33e85f5e 100644
--- a/services/core/java/com/android/server/ExplicitHealthCheckController.java
+++ b/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);
                 }));
diff --git a/services/core/java/com/android/server/PackageWatchdog.java b/services/core/java/com/android/server/PackageWatchdog.java
index 0c681df036b4..7d0d8344a566 100644
--- a/services/core/java/com/android/server/PackageWatchdog.java
+++ b/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;
-    // 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 long mUptimeAtLastStateSync;
 
     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.
-        pruneAndSchedule();
-        Slog.i(TAG, "Syncing health check requests, observing packages " + packageNames);
-        syncRequestsAsync();
-        saveToFileAsync();
+
+        // Sync after we add the new packages to the observers. We may have received packges
+        // requiring an earlier schedule than we are currently scheduled for.
+        syncState("updated observers");
     }
 
     /**
@@ -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);
-                pruneObservers(SystemClock.uptimeMillis() - mUptimeAtLastPruneMs);
+                mLongTaskHandler.removeCallbacks(this::saveToFileAsync);
+                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 "
-                    + (enabled ? "enabled" : "disabled"));
-            syncRequestsAsync();
+            // Prune to update internal state whenever health check is enabled/disabled
+            syncState("health check state " + (enabled ? "enabled" : "disabled"));
         }
     }
 
@@ -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.post(this::syncRequests);
-        }
+        mShortTaskHandler.removeCallbacks(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;
-                    shouldUpdateFile = true;
+
+                if (monitoredPackage != null) {
+                    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
-        Slog.i(TAG, "Syncing health check requests, health check passed for " + packageName);
-        syncRequestsAsync();
-
-        if (shouldUpdateFile) {
-            saveToFileAsync();
+        if (isStateChanged) {
+            syncState("health check passed for " + packageName);
         }
     }
 
-    private void onSupportedPackages(List<String> supportedPackages) {
-        boolean shouldUpdateFile = false;
-        boolean shouldPrune = false;
+    private void onSupportedPackages(List<PackageInfo> supportedPackages) {
+        boolean isStateChanged = 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 =
-                        observer.mPackages.values().iterator();
+                Iterator<MonitoredPackage> pit = oit.next().mPackages.values().iterator();
                 while (pit.hasNext()) {
                     MonitoredPackage monitoredPackage = pit.next();
-                    String packageName = monitoredPackage.mName;
-                    int healthCheckState = monitoredPackage.getHealthCheckState();
-
-                    if (healthCheckState != MonitoredPackage.STATE_PASSED) {
-                        // Have to update file, we will either transition state or reduce
-                        // health check duration
-                        shouldUpdateFile = true;
-
-                        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");
-                        }
+                    String packageName = monitoredPackage.getName();
+                    int oldState = monitoredPackage.getHealthCheckStateLocked();
+                    int newState;
+
+                    if (supportedPackageTimeouts.containsKey(packageName)) {
+                        // Supported packages become ACTIVE if currently INACTIVE
+                        newState = monitoredPackage.setHealthCheckActiveLocked(
+                                supportedPackageTimeouts.get(packageName));
                     } 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) {
-            saveToFileAsync();
-        }
-        if (shouldPrune) {
-            pruneAndSchedule();
+        if (isStateChanged) {
+            syncState("updated health check supported packages " + supportedPackages);
         }
     }
 
+    @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;
-                if (!monitoredPackage.mHasPassedHealthCheck) {
+                String packageName = monitoredPackage.getName();
+                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();
-            if (nextDurationToScheduleMs == Long.MAX_VALUE) {
-                Slog.i(TAG, "No monitored packages, ending prune");
-                mDurationAtLastPrune = 0;
-                mUptimeAtLastPruneMs = 0;
-                return;
-            }
-            long uptimeMs = SystemClock.uptimeMillis();
-            // O if not running
-            long elapsedDurationMs = mUptimeAtLastPruneMs == 0
-                    ? 0 : uptimeMs - mUptimeAtLastPruneMs;
-            // Less than O if unexpectedly didn't run yet even though
-            // we are past the last duration scheduled to run
-            long remainingDurationMs = mDurationAtLastPrune - elapsedDurationMs;
-            if (mUptimeAtLastPruneMs == 0
-                    || remainingDurationMs <= 0
-                    || nextDurationToScheduleMs < remainingDurationMs) {
-                // First schedule or an earlier reschedule
-                pruneObservers(elapsedDurationMs);
-                // We don't use Handler#hasCallbacks because we want to update the schedule delay
-                mShortTaskHandler.removeCallbacks(this::pruneAndSchedule);
-                mShortTaskHandler.postDelayed(this::pruneAndSchedule, nextDurationToScheduleMs);
-                mDurationAtLastPrune = nextDurationToScheduleMs;
-                mUptimeAtLastPruneMs = uptimeMs;
-            }
+            Slog.i(TAG, "Syncing state, reason: " + reason);
+            pruneObserversLocked();
+
+            saveToFileAsync();
+            syncRequestsAsync();
+
+            // Done syncing state, schedule the next state sync
+            scheduleNextSyncStateLocked();
+        }
+    }
+
+    private void syncStateWithScheduledReason() {
+        syncState("scheduled");
+    }
+
+    @GuardedBy("mLock")
+    private void scheduleNextSyncStateLocked() {
+        long durationMs = getNextStateSyncMillisLocked();
+        mShortTaskHandler.removeCallbacks(this::syncStateWithScheduledReason);
+        if (durationMs == Long.MAX_VALUE) {
+            Slog.i(TAG, "Cancelling state sync, nothing to sync");
+            mUptimeAtLastStateSync = 0;
+        } else {
+            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.
-     *
-     * <p> Prunes all observers with {@link ObserverInternal#prunePackages} and discards observers
-     * without any packages left.
+     * Removes {@code elapsedMs} milliseconds from all durations on monitored packages
+     * and updates other internal state.
      */
-    private void pruneObservers(long elapsedMs) {
-        if (elapsedMs == 0) {
+    @GuardedBy("mLock")
+    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");
-            Iterator<ObserverInternal> it = mAllObservers.values().iterator();
-            while (it.hasNext()) {
-                ObserverInternal observer = it.next();
-                Set<MonitoredPackage> failedPackages =
-                        observer.prunePackages(elapsedMs);
-                if (!failedPackages.isEmpty()) {
-                    onHealthCheckFailed(observer, failedPackages);
-                }
-                if (observer.mPackages.isEmpty()) {
-                    Slog.i(TAG, "Discarding observer " + observer.mName + ". All packages expired");
-                    it.remove();
-                }
+
+        Slog.i(TAG, "Removing " + elapsedMs + "ms from all packages on all observers");
+        Iterator<ObserverInternal> it = mAllObservers.values().iterator();
+        while (it.hasNext()) {
+            ObserverInternal observer = it.next();
+            Set<MonitoredPackage> failedPackages =
+                    observer.prunePackagesLocked(elapsedMs);
+            if (!failedPackages.isEmpty()) {
+                onHealthCheckFailed(observer, failedPackages);
+            }
+            if (observer.mPackages.isEmpty()) {
+                Slog.i(TAG, "Discarding observer " + observer.mName + ". All packages expired");
+                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);
-                    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);
+                    p.writeLocked(out);
                 }
                 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();
-
-                // Handle health check timeouts
-                if (healthCheckState == MonitoredPackage.STATE_ACTIVE) {
-                    // Only reduce duration if state is active
-                    p.mHealthCheckDurationMs -= elapsedMs;
-                    // Check duration after reducing duration
-                    if (p.mHealthCheckDurationMs <= 0) {
-                        failedPackages.add(p);
-                    }
+                int oldState = p.getHealthCheckStateLocked();
+                int newState = p.handleElapsedTimeLocked(elapsedMs);
+                if (oldState != MonitoredPackage.STATE_FAILED
+                        && newState == MonitoredPackage.STATE_FAILED) {
+                    Slog.i(TAG, "Package " + p.mName + " failed health check");
+                    failedPackages.add(p);
                 }
-
-                // 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);
-                    }
+                if (p.isExpiredLocked()) {
                     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
-    private static class MonitoredPackage {
+    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 final String mName;
-        // Whether an explicit health check has passed
+        public static final int STATE_PASSED = 2;
+        // mName has failed health check
+        public static final int STATE_FAILED = 3;
+
+        //TODO(b/120598832): VersionedPackage?
+        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;
-        // System uptime duration to monitor package
+        private boolean mHasPassedHealthCheck;
+        // 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;
+        }
     }
 }
diff --git a/tests/PackageWatchdog/src/com/android/server/PackageWatchdogTest.java b/tests/PackageWatchdog/src/com/android/server/PackageWatchdogTest.java
index b308982c2343..fa7bf61a07b2 100644
--- a/tests/PackageWatchdog/src/com/android/server/PackageWatchdogTest.java
+++ b/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());
             }