Skip to content

Commit 6d7edd2

Browse files
clauderobacarp
authored andcommitted
Fix PeriodicJob starvation by preventing duplicate enqueues
When executors are busy with long-running jobs, the coordinator would enqueue a new copy of each periodic job every interval, even though previous copies hadn't been executed yet. This caused periodic jobs to pile up in the queue, starving other work. Track the pending job run ID in the periodic job's metadata. Before enqueuing, check whether the previously enqueued run has finished. If it hasn't, skip the enqueue. The reference is cleared automatically when the job finishes or its config expires. https://claude.ai/code/session_01CGgGg8k3tV5QKYMZ5cFJxT
1 parent 6f54cba commit 6d7edd2

2 files changed

Lines changed: 115 additions & 5 deletions

File tree

spec/mosquito/periodic_job_run_spec.cr

Lines changed: 89 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -37,4 +37,93 @@ describe Mosquito::PeriodicJobRun do
3737
end
3838
end
3939
end
40+
41+
it "does not enqueue a second job run when one is already pending" do
42+
clean_slate do
43+
now = Time.utc.at_beginning_of_second
44+
periodic = PeriodicJobRun.new PeriodicTestJob, interval
45+
46+
# First execution should enqueue.
47+
Timecop.freeze(now) do
48+
periodic.last_executed_at = now - interval
49+
assert periodic.try_to_execute
50+
end
51+
52+
queue = PeriodicTestJob.queue
53+
first_size = queue.size(include_dead: false)
54+
assert first_size > 0, "Expected at least one job in the queue"
55+
56+
# Second execution after another interval should be skipped
57+
# because the first job run hasn't finished yet.
58+
Timecop.freeze(now + interval) do
59+
assert periodic.try_to_execute
60+
end
61+
62+
second_size = queue.size(include_dead: false)
63+
assert_equal first_size, second_size
64+
end
65+
end
66+
67+
it "enqueues again after the pending job run finishes" do
68+
clean_slate do
69+
now = Time.utc.at_beginning_of_second
70+
periodic = PeriodicJobRun.new PeriodicTestJob, interval
71+
72+
# Enqueue the first job run.
73+
Timecop.freeze(now) do
74+
periodic.last_executed_at = now - interval
75+
periodic.try_to_execute
76+
end
77+
78+
# Simulate the job finishing by writing finished_at to the backend.
79+
pending_id = periodic.metadata["pending_run_id"]?
80+
refute_nil pending_id
81+
Mosquito.backend.set(
82+
Mosquito::JobRun.config_key(pending_id.not_nil!),
83+
"finished_at",
84+
Time.utc.to_unix_ms.to_s
85+
)
86+
87+
queue = PeriodicTestJob.queue
88+
size_after_first = queue.size(include_dead: false)
89+
90+
# Now a new interval passes — should enqueue since the previous one finished.
91+
Timecop.freeze(now + interval) do
92+
assert periodic.try_to_execute
93+
end
94+
95+
size_after_second = queue.size(include_dead: false)
96+
assert size_after_second > size_after_first
97+
end
98+
end
99+
100+
it "enqueues again when the pending job run config has been cleaned up" do
101+
clean_slate do
102+
now = Time.utc.at_beginning_of_second
103+
periodic = PeriodicJobRun.new PeriodicTestJob, interval
104+
105+
# Enqueue the first job run.
106+
Timecop.freeze(now) do
107+
periodic.last_executed_at = now - interval
108+
periodic.try_to_execute
109+
end
110+
111+
pending_id = periodic.metadata["pending_run_id"]?
112+
refute_nil pending_id
113+
114+
# Simulate the job run config being deleted (e.g. TTL expiry).
115+
Mosquito.backend.delete Mosquito::JobRun.config_key(pending_id.not_nil!)
116+
117+
queue = PeriodicTestJob.queue
118+
size_before = queue.size(include_dead: false)
119+
120+
# Next interval should enqueue because the old run is gone.
121+
Timecop.freeze(now + interval) do
122+
assert periodic.try_to_execute
123+
end
124+
125+
size_after = queue.size(include_dead: false)
126+
assert size_after > size_before
127+
end
128+
end
40129
end

src/mosquito/periodic_job_run.cr

Lines changed: 26 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,7 @@
11
module Mosquito
22
class PeriodicJobRun
3+
Log = ::Log.for self
4+
35
property class : Mosquito::PeriodicJob.class
46
property interval : Time::Span | Time::MonthSpan
57
getter metadata : Metadata { Metadata.new(Mosquito.backend.build_key("periodic_jobs", @class.name)) }
@@ -47,24 +49,43 @@ module Mosquito
4749
now = Time.utc
4850

4951
if last_executed_at + interval <= now
50-
execute
52+
if pending_job_run?
53+
Log.info { "Skipping enqueue for #{@class.name}: a job run is already pending" }
54+
else
55+
execute
56+
end
5157

52-
# Weaknesses:
53-
# - If something interferes with the job run, it won't be correct that it was executed.
54-
# - if the worker is backlogged, the start time will be different from the last executed time.
5558
self.last_executed_at = now
5659
true
5760
else
5861
false
5962
end
6063
end
6164

62-
# Enqueues the job for execution
65+
# Returns true if a previously enqueued job run has not yet finished.
66+
# This prevents duplicate enqueues when executors are busy and the
67+
# periodic interval elapses multiple times before the job is run.
68+
def pending_job_run? : Bool
69+
if pending_id = metadata["pending_run_id"]?
70+
if job_run = JobRun.retrieve(pending_id)
71+
return true if job_run.finished_at.nil?
72+
end
73+
74+
# Job run has finished or was cleaned up; clear the stale reference.
75+
metadata["pending_run_id"] = nil
76+
end
77+
78+
false
79+
end
80+
81+
# Enqueues the job for execution and records the job run id so that
82+
# subsequent intervals can detect that a run is already pending.
6383
def execute
6484
job = @class.new
6585
job_run = job.build_job_run
6686
job_run.store
6787
@class.queue.enqueue job_run
88+
metadata["pending_run_id"] = job_run.id
6889
end
6990
end
7091
end

0 commit comments

Comments
 (0)