Donate to e Foundation | Murena handsets with /e/OS | Own a part of Murena! Learn more

Commit 6997f3c8 authored by Zimuzo's avatar Zimuzo
Browse files

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
parent e777220a

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

+13 −6
Original line number Diff line number Diff line
@@ -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

+311 −190

File changed.

Preview size limit exceeded, changes collapsed.

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

+112 −3
Original line number Diff line number Diff line
@@ -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());

            }