diff --git a/app/src/main/java/org/thoughtcrime/securesms/jobs/FastJobStorage.kt b/app/src/main/java/org/thoughtcrime/securesms/jobs/FastJobStorage.kt index 229f4e12f6..7d59f4c260 100644 --- a/app/src/main/java/org/thoughtcrime/securesms/jobs/FastJobStorage.kt +++ b/app/src/main/java/org/thoughtcrime/securesms/jobs/FastJobStorage.kt @@ -486,11 +486,12 @@ class FastJobStorage(private val jobDatabase: JobDatabase) : JobStorage { /** * Note that this is currently only checking a specific kind of circular dependency -- ones that are - * created between dependencies and queues. + * created between dependencies, queues, and priorities. * * More specifically, dependencies where one job depends on another job in the same queue that was - * scheduled *after* it. These dependencies will never resolve. Under normal circumstances these - * won't occur, but *could* occur if the user changed their clock (either purposefully or automatically). + * scheduled *after* it, or if it depends on a job with a lower priority. These dependencies will + * never resolve. Under normal circumstances these won't occur, but *could* occur if the user changed + * their clock (either purposefully or automatically). * * Rather than go through and delete them from the database, removing them from memory at load time * serves the same effect and doesn't require new write methods. This should also be very rare. @@ -511,7 +512,7 @@ class FastJobStorage(private val jobDatabase: JobDatabase) : JobStorage { return false } - return dependsOnJob.createTime > job.createTime + return dependsOnJob.createTime > job.createTime || dependsOnJob.globalPriority < job.globalPriority || dependsOnJob.queuePriority < job.queuePriority } /** diff --git a/app/src/test/java/org/thoughtcrime/securesms/jobs/FastJobStorageTest.kt b/app/src/test/java/org/thoughtcrime/securesms/jobs/FastJobStorageTest.kt index d7ec5d8e76..98c4fa77a1 100644 --- a/app/src/test/java/org/thoughtcrime/securesms/jobs/FastJobStorageTest.kt +++ b/app/src/test/java/org/thoughtcrime/securesms/jobs/FastJobStorageTest.kt @@ -813,6 +813,22 @@ class FastJobStorageTest { assertThat(subject.getNextEligibleJob(20, NO_PREDICATE)).isEqualTo(fullSpec1.jobSpec) } + @Test + fun `getNextEligibleJob - same queue, with dependency, with later job having a higher global priority`() { + val fullSpec1 = FullSpec(jobSpec(id = "1", factoryKey = "f1", queueKey = "q1", createTime = 1, globalPriority = 0), emptyList(), emptyList()) + val fullSpec2 = FullSpec(jobSpec(id = "2", factoryKey = "f2", queueKey = "q1", createTime = 1, globalPriority = 1), emptyList(), listOf(DependencySpec(jobId = "2", dependsOnJobId = "1", isMemoryOnly = false))) + + val subject = FastJobStorage(mockDatabase(listOf(fullSpec1, fullSpec2))) + subject.init() + + // The init should find the "circular dependency" and remove it altogether, causing the higher priority job to run first, even though it had an (unsatisfiable) dependency on the other job + // Note that we prevent these types of jobs from being inserted now -- this is just for bad jobs that snuck in beforehand. + assertThat(subject.getNextEligibleJob(10, NO_PREDICATE)).isEqualTo(fullSpec2.jobSpec) + subject.deleteJob(fullSpec2.jobSpec.id) + + assertThat(subject.getNextEligibleJob(20, NO_PREDICATE)).isEqualTo(fullSpec1.jobSpec) + } + @Test fun `deleteJobs - writes to database`() { val database = mockDatabase(DataSet1.FULL_SPECS)