Try to avoid overlapping job execution.

In certain situtations, when an app schedules a new job using the same
job ID as an already executing job, JobScheduler may start running the
new job before it has fully stopped the old job and cleaned up state.
This may cause some incorrect behavior within apps that don't properly
handle this possibility. To alleviate that, try to avoid executing the
new job until the old job has finished, and document the behavior as
much as possible.

We still continue to start TOP+EJs immediately so that developers can
operate with that guarantee.

Bug: 231148615
Test: atest frameworks/base/services/tests/mockingservicestests/src/com/android/server/job
Test: atest frameworks/base/services/tests/servicestests/src/com/android/server/job
Test: atest CtsJobSchedulerTestCases
Change-Id: Ieedd7a9b91c350e0b835163fa9a73913d1edd345

5 files changed, 93 insertions, 11 deletions

diff --git a/apex/jobscheduler/framework/java/android/app/job/JobInfo.java b/apex/jobscheduler/framework/java/android/app/job/JobInfo.java
index f49cdbf403f0..4710322db283 100644
--- a/apex/jobscheduler/framework/java/android/app/job/JobInfo.java
+++ b/apex/jobscheduler/framework/java/android/app/job/JobInfo.java
@@ -1777,7 +1777,10 @@ public class JobInfo implements Parcelable {
          * {@link Build.VERSION_CODES#S}, but starting from Android version
          * {@link Build.VERSION_CODES#TIRAMISU}, expedited jobs for the foreground app are
          * guaranteed to be started before {@link JobScheduler#schedule(JobInfo)} returns (assuming
-         * all requested constraints are satisfied), similar to foreground services.
+         * all requested constraints are satisfied), similar to foreground services. However, this
+         * start guarantee means there is a higher chance of overlapping executions, as noted in
+         * {@link JobService}, so be sure to handle that properly if you intend to reschedule the
+         * job while it's actively running.
          *
          * @see JobInfo#isExpedited()
          */
diff --git a/apex/jobscheduler/framework/java/android/app/job/JobScheduler.java b/apex/jobscheduler/framework/java/android/app/job/JobScheduler.java
index 1f4ef0470ebd..388bbf1b26a0 100644
--- a/apex/jobscheduler/framework/java/android/app/job/JobScheduler.java
+++ b/apex/jobscheduler/framework/java/android/app/job/JobScheduler.java
@@ -107,7 +107,9 @@ public abstract class JobScheduler {
     /**
      * Schedule a job to be executed.  Will replace any currently scheduled job with the same
      * ID with the new information in the {@link JobInfo}.  If a job with the given ID is currently
-     * running, it will be stopped.
+     * running, it will be stopped. Note that in some cases, the newly scheduled job may be started
+     * before the previously running job has been fully stopped. See {@link JobService} for
+     * additional details.
      *
      * <p class="caution"><strong>Note:</strong> Scheduling a job can have a high cost, even if it's
      * rescheduling the same job and the job didn't execute, especially on platform versions before
@@ -131,7 +133,7 @@ public abstract class JobScheduler {
      * job.  If a job with the same ID is already scheduled, it will be replaced with the
      * new {@link JobInfo}, but any previously enqueued work will remain and be dispatched the
      * next time it runs.  If a job with the same ID is already running, the new work will be
-     * enqueued for it.
+     * enqueued for it without stopping the job.
      *
      * <p>The work you enqueue is later retrieved through
      * {@link JobParameters#dequeueWork() JobParameters.dequeueWork}.  Be sure to see there
diff --git a/apex/jobscheduler/framework/java/android/app/job/JobService.java b/apex/jobscheduler/framework/java/android/app/job/JobService.java
index e5b07429a5c6..7ed4b62ae7e4 100644
--- a/apex/jobscheduler/framework/java/android/app/job/JobService.java
+++ b/apex/jobscheduler/framework/java/android/app/job/JobService.java
@@ -31,6 +31,17 @@ import android.os.IBinder;
  * in blocking any future callbacks from the JobManager - specifically
  * {@link #onStopJob(android.app.job.JobParameters)}, which is meant to inform you that the
  * scheduling requirements are no longer being met.</p>
+ *
+ * As a subclass of {@link Service}, there will only be one active instance of any JobService
+ * subclasses, regardless of job ID. This means that if you schedule multiple jobs with different
+ * job IDs but using the same JobService class, that JobService may receive multiple calls to
+ * {@link #onStartJob(JobParameters)} and {@link #onStopJob(JobParameters)}, with each call being
+ * for the separate jobs.
+ *
+ * <p class="note">Note that if you cancel and reschedule an already executing job,
+ * there may be a small period of time where {@link #onStartJob(JobParameters)} has been called for
+ * the newly scheduled job instance before {@link #onStopJob(JobParameters)} has been called or
+ * fully processed for the old job.</p>
  */
 public abstract class JobService extends Service {
     private static final String TAG = "JobService";
diff --git a/apex/jobscheduler/service/java/com/android/server/job/JobConcurrencyManager.java b/apex/jobscheduler/service/java/com/android/server/job/JobConcurrencyManager.java
index c86353c84467..afe36b5fa25a 100644
--- a/apex/jobscheduler/service/java/com/android/server/job/JobConcurrencyManager.java
+++ b/apex/jobscheduler/service/java/com/android/server/job/JobConcurrencyManager.java
@@ -542,6 +542,22 @@ class JobConcurrencyManager {
         return mRunningJobs.contains(job);
     }
 
+    /**
+     * Returns true if a job that is "similar" to the provided job is currently running.
+     * "Similar" in this context means any job that the {@link JobStore} would consider equivalent
+     * and replace one with the other.
+     */
+    @GuardedBy("mLock")
+    private boolean isSimilarJobRunningLocked(JobStatus job) {
+        for (int i = mRunningJobs.size() - 1; i >= 0; --i) {
+            JobStatus js = mRunningJobs.valueAt(i);
+            if (job.getUid() == js.getUid() && job.getJobId() == js.getJobId()) {
+                return true;
+            }
+        }
+        return false;
+    }
+
     /** Return {@code true} if the state was updated. */
     @GuardedBy("mLock")
     private boolean refreshSystemStateLocked() {
@@ -699,6 +715,21 @@ class JobConcurrencyManager {
                 continue;
             }
 
+            final boolean isTopEj = nextPending.shouldTreatAsExpeditedJob()
+                    && nextPending.lastEvaluatedBias == JobInfo.BIAS_TOP_APP;
+            // Avoid overlapping job execution as much as possible.
+            if (!isTopEj && isSimilarJobRunningLocked(nextPending)) {
+                if (DEBUG) {
+                    Slog.w(TAG, "Delaying execution of job because of similarly running one: "
+                            + nextPending);
+                }
+                // It would be nice to let the JobService running the other similar job know about
+                // this new job so that it doesn't unbind from the JobService and we can call
+                // onStartJob as soon as the older job finishes.
+                // TODO: optimize the job reschedule flow to reduce service binding churn
+                continue;
+            }
+
             // Find an available slot for nextPending. The context should be one of the following:
             // 1. Unused
             // 2. Its job should have used up its minimum execution guarantee so it
@@ -707,8 +738,6 @@ class JobConcurrencyManager {
             ContextAssignment selectedContext = null;
             final int allWorkTypes = getJobWorkTypes(nextPending);
             final boolean pkgConcurrencyOkay = !isPkgConcurrencyLimitedLocked(nextPending);
-            final boolean isTopEj = nextPending.shouldTreatAsExpeditedJob()
-                    && nextPending.lastEvaluatedBias == JobInfo.BIAS_TOP_APP;
             final boolean isInOverage = projectedRunningCount > STANDARD_CONCURRENCY_LIMIT;
             boolean startingJob = false;
             if (idle.size() > 0) {
@@ -1177,6 +1206,15 @@ class JobConcurrencyManager {
                     continue;
                 }
 
+                // Avoid overlapping job execution as much as possible.
+                if (isSimilarJobRunningLocked(nextPending)) {
+                    if (DEBUG) {
+                        Slog.w(TAG, "Avoiding execution of job because of similarly running one: "
+                                + nextPending);
+                    }
+                    continue;
+                }
+
                 if (worker.getPreferredUid() != nextPending.getUid()) {
                     if (backupJob == null && !isPkgConcurrencyLimitedLocked(nextPending)) {
                         int allWorkTypes = getJobWorkTypes(nextPending);
@@ -1260,6 +1298,15 @@ class JobConcurrencyManager {
                     continue;
                 }
 
+                // Avoid overlapping job execution as much as possible.
+                if (isSimilarJobRunningLocked(nextPending)) {
+                    if (DEBUG) {
+                        Slog.w(TAG, "Avoiding execution of job because of similarly running one: "
+                                + nextPending);
+                    }
+                    continue;
+                }
+
                 if (isPkgConcurrencyLimitedLocked(nextPending)) {
                     continue;
                 }
diff --git a/apex/jobscheduler/service/java/com/android/server/job/JobSchedulerService.java b/apex/jobscheduler/service/java/com/android/server/job/JobSchedulerService.java
index 358c32722b8b..cd70e88b18aa 100644
--- a/apex/jobscheduler/service/java/com/android/server/job/JobSchedulerService.java
+++ b/apex/jobscheduler/service/java/com/android/server/job/JobSchedulerService.java
@@ -1208,12 +1208,22 @@ public class JobSchedulerService extends com.android.server.SystemService
             // This may throw a SecurityException.
             jobStatus.prepareLocked();
 
+            final boolean canExecuteImmediately;
             if (toCancel != null) {
                 // Implicitly replaces the existing job record with the new instance
-                cancelJobImplLocked(toCancel, jobStatus, JobParameters.STOP_REASON_CANCELLED_BY_APP,
-                        JobParameters.INTERNAL_STOP_REASON_CANCELED, "job rescheduled by app");
+                final boolean wasJobExecuting = cancelJobImplLocked(toCancel, jobStatus,
+                        JobParameters.STOP_REASON_CANCELLED_BY_APP,
+                        JobParameters.INTERNAL_STOP_REASON_CANCELED,
+                        "job rescheduled by app");
+                // Avoid overlapping job executions. Don't push for immediate execution if an old
+                // job with the same ID was running, but let TOP EJs start immediately.
+                canExecuteImmediately = !wasJobExecuting
+                        || (jobStatus.isRequestedExpeditedJob()
+                        && mUidBiasOverride.get(jobStatus.getSourceUid(), JobInfo.BIAS_DEFAULT)
+                        == JobInfo.BIAS_TOP_APP);
             } else {
                 startTrackingJobLocked(jobStatus, null);
+                canExecuteImmediately = true;
             }
 
             if (work != null) {
@@ -1256,7 +1266,12 @@ public class JobSchedulerService extends com.android.server.SystemService
                 // list and try to run it.
                 mJobPackageTracker.notePending(jobStatus);
                 mPendingJobQueue.add(jobStatus);
-                maybeRunPendingJobsLocked();
+                if (canExecuteImmediately) {
+                    // Don't ask the JobConcurrencyManager to try to run the job immediately. The
+                    // JobServiceContext will ask the JobConcurrencyManager for another job once
+                    // it finishes cleaning up the old job.
+                    maybeRunPendingJobsLocked();
+                }
             } else {
                 evaluateControllerStatesLocked(jobStatus);
             }
@@ -1377,8 +1392,10 @@ public class JobSchedulerService extends com.android.server.SystemService
      * is null, the cancelled job is removed outright from the system.  If
      * {@code incomingJob} is non-null, it replaces {@code cancelled} in the store of
      * currently scheduled jobs.
+     *
+     * @return true if the cancelled job was running
      */
-    private void cancelJobImplLocked(JobStatus cancelled, JobStatus incomingJob,
+    private boolean cancelJobImplLocked(JobStatus cancelled, JobStatus incomingJob,
             @JobParameters.StopReason int reason, int internalReasonCode, String debugReason) {
         if (DEBUG) Slog.d(TAG, "CANCEL: " + cancelled.toShortString());
         cancelled.unprepareLocked();
@@ -1389,7 +1406,7 @@ public class JobSchedulerService extends com.android.server.SystemService
         }
         mChangedJobList.remove(cancelled);
         // Cancel if running.
-        mConcurrencyManager.stopJobOnServiceContextLocked(
+        boolean wasRunning = mConcurrencyManager.stopJobOnServiceContextLocked(
                 cancelled, reason, internalReasonCode, debugReason);
         // If this is a replacement, bring in the new version of the job
         if (incomingJob != null) {
@@ -1397,6 +1414,7 @@ public class JobSchedulerService extends com.android.server.SystemService
             startTrackingJobLocked(incomingJob, cancelled);
         }
         reportActiveLocked();
+        return wasRunning;
     }
 
     void updateUidState(int uid, int procState) {
@@ -1755,7 +1773,7 @@ public class JobSchedulerService extends com.android.server.SystemService
             // same job ID), we remove it from the JobStore and tell the JobServiceContext to stop
             // running the job. Once the job stops running, we then call this method again.
             // TODO: rework code so we don't intentionally call this method twice for the same job
-            Slog.w(TAG, "Job didn't exist in JobStore");
+            Slog.w(TAG, "Job didn't exist in JobStore: " + jobStatus.toShortString());
         }
         if (mReadyToRock) {
             for (int i = 0; i < mControllers.size(); i++) {
@@ -3813,6 +3831,7 @@ public class JobSchedulerService extends com.android.server.SystemService
                     // Double indent for readability
                     pw.increaseIndent();
                     pw.increaseIndent();
+                    pw.println(job.toShortString());
                     job.dump(pw, true, nowElapsed);
                     pw.decreaseIndent();
                     pw.decreaseIndent();