diff --git a/app/jobs/regular/problem_check.rb b/app/jobs/regular/problem_check.rb index f20e0cc5ad0..b4596a874c1 100644 --- a/app/jobs/regular/problem_check.rb +++ b/app/jobs/regular/problem_check.rb @@ -18,7 +18,12 @@ module Jobs problems = check.call raise RetrySignal if problems.present? && retry_count < check.max_retries - problems.each { |problem| AdminDashboardData.add_found_scheduled_check_problem(problem) } + if problems.present? + problems.each { |problem| AdminDashboardData.add_found_scheduled_check_problem(problem) } + ProblemCheckTracker[identifier].problem!(next_run_at: check.perform_every.from_now) + else + ProblemCheckTracker[identifier].no_problem!(next_run_at: check.perform_every.from_now) + end rescue RetrySignal Jobs.enqueue_in( check.retry_after, diff --git a/app/jobs/scheduled/problem_checks.rb b/app/jobs/scheduled/problem_checks.rb index 8950138faf5..89161925e49 100644 --- a/app/jobs/scheduled/problem_checks.rb +++ b/app/jobs/scheduled/problem_checks.rb @@ -14,7 +14,12 @@ module Jobs # not be re-added by the relevant checker, and will be cleared. AdminDashboardData.clear_found_scheduled_check_problems - ::ProblemCheck.scheduled.each do |check| + scheduled_checks = + ProblemCheckTracker.all.filter_map do |tracker| + tracker.check if tracker.check.scheduled? && tracker.ready_to_run? + end + + scheduled_checks.each do |check| Jobs.enqueue(:problem_check, check_identifier: check.identifier.to_s) end end diff --git a/app/models/problem_check_tracker.rb b/app/models/problem_check_tracker.rb new file mode 100644 index 00000000000..a8f19b1bb5c --- /dev/null +++ b/app/models/problem_check_tracker.rb @@ -0,0 +1,47 @@ +# frozen_string_literal: true + +class ProblemCheckTracker < ActiveRecord::Base + validates :identifier, presence: true, uniqueness: true + validates :blips, presence: true, numericality: { greater_than_or_equal_to: 0 } + + def self.[](identifier) + find_or_create_by(identifier:) + end + + def ready_to_run? + next_run_at.blank? || next_run_at.past? + end + + def problem!(next_run_at: nil) + now = Time.current + + update!(blips: blips + 1, last_run_at: now, last_problem_at: now, next_run_at:) + end + + def no_problem!(next_run_at: nil) + now = Time.current + + update!(blips: 0, last_run_at: now, last_success_at: now, next_run_at:) + end + + def check + ProblemCheck[identifier] + end +end + +# == Schema Information +# +# Table name: problem_check_trackers +# +# id :bigint not null, primary key +# identifier :string not null +# blips :integer default(0), not null +# last_run_at :datetime +# next_run_at :datetime +# last_success_at :datetime +# last_problem_at :datetime +# +# Indexes +# +# index_problem_check_trackers_on_identifier (identifier) UNIQUE +# diff --git a/db/migrate/20240223052820_create_problem_check_trackers.rb b/db/migrate/20240223052820_create_problem_check_trackers.rb new file mode 100644 index 00000000000..12e5ac5c0da --- /dev/null +++ b/db/migrate/20240223052820_create_problem_check_trackers.rb @@ -0,0 +1,16 @@ +# frozen_string_literal: true + +class CreateProblemCheckTrackers < ActiveRecord::Migration[7.0] + def change + create_table :problem_check_trackers do |t| + t.string :identifier, null: false, index: { unique: true } + + t.integer :blips, null: false, default: 0 + + t.datetime :last_run_at + t.datetime :next_run_at + t.datetime :last_success_at + t.datetime :last_problem_at + end + end +end diff --git a/spec/fabricators/problem_check_tracker_fabricator.rb b/spec/fabricators/problem_check_tracker_fabricator.rb new file mode 100644 index 00000000000..77f4a819f38 --- /dev/null +++ b/spec/fabricators/problem_check_tracker_fabricator.rb @@ -0,0 +1,3 @@ +# frozen_string_literal: true + +Fabricator(:problem_check_tracker) {} diff --git a/spec/jobs/problem_check_spec.rb b/spec/jobs/problem_check_spec.rb index 8f4d9d60b5f..86c6d128045 100644 --- a/spec/jobs/problem_check_spec.rb +++ b/spec/jobs/problem_check_spec.rb @@ -11,6 +11,7 @@ RSpec.describe Jobs::ProblemCheck do before do ::ProblemCheck::TestCheck = Class.new(::ProblemCheck) do + self.perform_every = 30.minutes self.max_retries = 0 def call @@ -39,6 +40,7 @@ RSpec.describe Jobs::ProblemCheck do before do ::ProblemCheck::TestCheck = Class.new(::ProblemCheck) do + self.perform_every = 30.minutes self.max_retries = 0 def call @@ -71,6 +73,7 @@ RSpec.describe Jobs::ProblemCheck do before do ::ProblemCheck::TestCheck = Class.new(::ProblemCheck) do + self.perform_every = 30.minutes self.max_retries = 2 def call @@ -79,6 +82,12 @@ RSpec.describe Jobs::ProblemCheck do end end + it "does not yet update the problem check tracker" do + expect { + described_class.new.execute(check_identifier: :test_check, retry_count: 1) + }.not_to change { ProblemCheckTracker.where("blips > ?", 0).count } + end + it "schedules a retry" do expect_enqueued_with( job: :problem_check, @@ -94,6 +103,7 @@ RSpec.describe Jobs::ProblemCheck do before do ::ProblemCheck::TestCheck = Class.new(::ProblemCheck) do + self.perform_every = 30.minutes self.max_retries = 1 def call @@ -102,9 +112,15 @@ RSpec.describe Jobs::ProblemCheck do end end + it "updates the problem check tracker" do + expect { + described_class.new.execute(check_identifier: :test_check, retry_count: 1) + }.to change { ProblemCheckTracker.where("blips > ?", 0).count }.by(1) + end + it "does not schedule a retry" do expect_not_enqueued_with(job: :problem_check) do - described_class.new.execute(check_identifier: :test_identifier, retry_count: 1) + described_class.new.execute(check_identifier: :test_check, retry_count: 1) end end end diff --git a/spec/jobs/problem_checks_spec.rb b/spec/jobs/problem_checks_spec.rb index 314e1968292..40c42b4fdfa 100644 --- a/spec/jobs/problem_checks_spec.rb +++ b/spec/jobs/problem_checks_spec.rb @@ -20,9 +20,43 @@ RSpec.describe Jobs::ProblemChecks do ProblemCheck.send(:remove_const, "NonScheduledCheck") end - it "schedules the individual scheduled checks" do - expect_enqueued_with(job: :problem_check, args: { check_identifier: "scheduled_check" }) do - described_class.new.execute([]) + context "when the tracker determines the check is ready to run" do + before do + ProblemCheckTracker.create!(identifier: "scheduled_check", next_run_at: 5.minutes.ago) + end + + it "schedules the individual scheduled checks" do + expect_enqueued_with(job: :problem_check, args: { check_identifier: "scheduled_check" }) do + described_class.new.execute([]) + end + end + end + + context "when the tracker determines the check shouldn't run yet" do + before do + ProblemCheckTracker.create!(identifier: "scheduled_check", next_run_at: 5.minutes.from_now) + end + + it "does not schedule any check" do + expect_not_enqueued_with( + job: :problem_check, + args: { + check_identifier: "scheduled_check", + }, + ) { described_class.new.execute([]) } + end + end + + context "when dealing with a non-scheduled check" do + before { ProblemCheckTracker.create!(identifier: "non_scheduled_check", next_run_at: nil) } + + it "does not schedule any check" do + expect_not_enqueued_with( + job: :problem_check, + args: { + check_identifier: "non_scheduled_check", + }, + ) { described_class.new.execute([]) } end end @@ -34,13 +68,4 @@ RSpec.describe Jobs::ProblemChecks do }, ) { described_class.new.execute([]) } end - - it "does not schedule non-scheduled checks" do - expect_not_enqueued_with( - job: :problem_check, - args: { - check_identifier: "non_scheduled_check", - }, - ) { described_class.new.execute([]) } - end end diff --git a/spec/models/problem_check_tracker_spec.rb b/spec/models/problem_check_tracker_spec.rb new file mode 100644 index 00000000000..6d0b3a9b800 --- /dev/null +++ b/spec/models/problem_check_tracker_spec.rb @@ -0,0 +1,98 @@ +# frozen_string_literal: true + +RSpec.describe ProblemCheckTracker do + describe "validations" do + let(:record) { described_class.new(identifier: "twitter_login") } + + it { expect(record).to validate_presence_of(:identifier) } + it { expect(record).to validate_uniqueness_of(:identifier) } + + it { expect(record).to validate_numericality_of(:blips).is_greater_than_or_equal_to(0) } + end + + describe ".[]" do + before { Fabricate(:problem_check_tracker, identifier: "twitter_login") } + + context "when the problem check tracker already exists" do + it { expect(described_class[:twitter_login]).not_to be_new_record } + end + + context "when the problem check tracker doesn't exist yet" do + it { expect(described_class[:facebook_login]).to be_previously_new_record } + end + end + + describe "#ready_to_run?" do + let(:problem_tracker) { described_class.new(next_run_at:) } + + context "when the next run timestamp is not set" do + let(:next_run_at) { nil } + + it { expect(problem_tracker).to be_ready_to_run } + end + + context "when the next run timestamp is in the past" do + let(:next_run_at) { 5.minutes.ago } + + it { expect(problem_tracker).to be_ready_to_run } + end + + context "when the next run timestamp is in the future" do + let(:next_run_at) { 5.minutes.from_now } + + it { expect(problem_tracker).not_to be_ready_to_run } + end + end + + describe "#problem!" do + let(:problem_tracker) do + Fabricate(:problem_check_tracker, identifier: "twitter_login", **original_attributes) + end + + let(:original_attributes) do + { + blips: 0, + last_problem_at: 1.week.ago, + last_success_at: 24.hours.ago, + last_run_at: 24.hours.ago, + next_run_at: nil, + } + end + + let(:updated_attributes) { { blips: 1 } } + + it do + freeze_time + + expect { problem_tracker.problem!(next_run_at: 24.hours.from_now) }.to change { + problem_tracker.attributes + }.to(hash_including(updated_attributes)) + end + end + + describe "#no_problem!" do + let(:problem_tracker) do + Fabricate(:problem_check_tracker, identifier: "twitter_login", **original_attributes) + end + + let(:original_attributes) do + { + blips: 0, + last_problem_at: 1.week.ago, + last_success_at: Time.current, + last_run_at: 24.hours.ago, + next_run_at: nil, + } + end + + let(:updated_attributes) { { blips: 0 } } + + it do + freeze_time + + expect { problem_tracker.no_problem!(next_run_at: 24.hours.from_now) }.to change { + problem_tracker.attributes + }.to(hash_including(updated_attributes)) + end + end +end