Fixes #22714: replace stale scheduled jobs in enqueue_once() - #22952
Fixes #22714: replace stale scheduled jobs in enqueue_once()#22952mderekasir wants to merge 1 commit into
Conversation
|
Thanks for working on this! Would you mind adding a brief PR description that summarizes the approach and the testing performed? The linked issue explains the problem well, but a short explanation of how this change identifies a stale job, including how it considers the database and RQ states, would make the review easier and preserve useful context for future maintainers. Thank you! |
pheus
left a comment
pheus
left a comment
There was a problem hiding this comment.
Thanks for working on this and for adding the pending and running coverage. The immediate recovery behavior for the reported stale database record looks sensible.
I found one blocking concern with using the timestamp alone to classify the job as stale. An overdue job can still be active in RQ while the NetBox record remains scheduled, so this path can replace healthy work and allow duplicate execution. I think the decision needs to include whether an active RQ job still exists, with coverage for both active and genuinely stale states.
| # Redis restart) and must be replaced rather than reused, even though its parameters match. | ||
| # Running/pending jobs are exempt from this check: their `scheduled` timestamp is expected to be | ||
| # in the past (or unset) once they've started, and that must not be mistaken for staleness. | ||
| is_stale = job.status == JobStatusChoices.STATUS_SCHEDULED and job.scheduled <= timezone.now() |
There was a problem hiding this comment.
Could we include the corresponding RQ job's active state when deciding whether this record is stale?
A scheduled time in the past is not sufficient on its own. The RQ job may still be scheduled, waiting in the queue, or starting on another worker while the NetBox record still says scheduled. Deleting it in that window can create a replacement while the original continues executing, and the original worker's later job.start() can recreate the deleted database row.
I think recovery should be limited to cases where no active RQ job remains. It would also be good to handle Redis lookup failures conservatively so that an uncertain RQ state does not cause healthy work to be replaced.
There was a problem hiding this comment.
During issue investigation, I did some tests on RQ validation by adding a property to the Job class, see #22714 (comment).
This seemed to detect jobs entirely missing in RQ, but broke some tests. And it didn't check the status of a found RQ job. But it might be a starting point.
| self.assertRaises(Job.DoesNotExist, job1.refresh_from_db) | ||
| self.assertEqual(TestJobRunner.get_jobs(instance).count(), 1) | ||
|
|
||
| def test_enqueue_once_replaces_stale_scheduled_job(self): |
There was a problem hiding this comment.
Could we add the complementary cases where the RQ job still exists?
The current tests create database records with random job_id values, so they cover only missing Redis state. I think we should verify that an overdue job is preserved while its RQ job is still active, and replaced when the RQ job is missing or terminal.
It would also be helpful to assert that the recovered job is pending with scheduled set to None, since the intended recovery is an immediate run.
3 participants
Closes: #22714