diff --git a/CHANGELOG.txt b/CHANGELOG.txt index 83fd4ac0..4a2f73e0 100644 --- a/CHANGELOG.txt +++ b/CHANGELOG.txt @@ -1,3 +1,21 @@ +2026-03-17 +fix: DNS and ping checks not executing in production + - ensure_default_ping_check now calls schedule_check after creating the check + - CheckExecutorWorker now reschedules skipped checks (agent-assigned, SNMP disabled) + so the scheduling chain is not permanently broken + - JobHealthCheckWorker now recovers orphaned CheckExecutorWorker jobs (enabled checks + with no active Oban job) in addition to DeviceMonitorWorker jobs + - Wrapped ensure_default_ping_check in try/rescue during device creation to prevent + transient DB deadlocks from failing the entire device creation + Files: lib/towerops/monitoring.ex, + lib/towerops/devices.ex, + lib/towerops/workers/check_executor_worker.ex, + lib/towerops/workers/job_health_check_worker.ex, + test/towerops/monitoring_test.exs, + test/towerops/workers/check_executor_worker_test.exs, + test/towerops/workers/job_health_check_worker_test.exs, + test/towerops/snmp_test.exs + 2026-03-16 feat: add service checks REST API, GraphQL API, and ping executor - New REST API: full CRUD at /api/v1/checks for HTTP, TCP, DNS, and ping checks diff --git a/lib/towerops/devices.ex b/lib/towerops/devices.ex index b94009cb..6942bc08 100644 --- a/lib/towerops/devices.ex +++ b/lib/towerops/devices.ex @@ -600,6 +600,8 @@ defmodule Towerops.Devices do * `:bypass_limits` - When true, bypasses subscription device limits (for superusers) """ def create_device(attrs, opts \\ []) do + require Logger + bypass_limits = Keyword.get(opts, :bypass_limits, false) # Check device quota before insertion (unless bypassing) @@ -612,7 +614,14 @@ defmodule Towerops.Devices do end # Auto-create ICMP ping check for all devices - _ = Monitoring.ensure_default_ping_check(device) + # Best-effort: if this fails (e.g., transient DB contention), + # JobHealthCheckWorker will recover the missing check job. + try do + _ = Monitoring.ensure_default_ping_check(device) + rescue + e -> + Logger.warning("Failed to create ping check for device #{device.id}: #{Exception.message(e)}") + end broadcast_device_change(device.organization_id, :device_created) diff --git a/lib/towerops/monitoring.ex b/lib/towerops/monitoring.ex index db80e12b..0ab168b9 100644 --- a/lib/towerops/monitoring.ex +++ b/lib/towerops/monitoring.ex @@ -434,17 +434,22 @@ defmodule Towerops.Monitoring do case existing do nil -> - create_check(%{ - organization_id: device.organization_id, - device_id: device.id, - name: "ICMP Ping", - check_type: "ping", - config: %{"host" => device.ip_address, "count" => 3}, - enabled: true, - interval_seconds: 60, - timeout_ms: 5000, - source_type: "auto_discovery" - }) + with {:ok, check} <- + create_check(%{ + organization_id: device.organization_id, + device_id: device.id, + name: "ICMP Ping", + check_type: "ping", + config: %{"host" => device.ip_address, "count" => 3}, + enabled: true, + interval_seconds: 60, + timeout_ms: 5000, + source_type: "auto_discovery" + }) do + # Best-effort scheduling; JobHealthCheckWorker recovers orphaned jobs + _ = schedule_check(check) + {:ok, check} + end check -> if check.config["host"] == device.ip_address do diff --git a/lib/towerops/workers/check_executor_worker.ex b/lib/towerops/workers/check_executor_worker.ex index 8aa9c30a..e5108d36 100644 --- a/lib/towerops/workers/check_executor_worker.ex +++ b/lib/towerops/workers/check_executor_worker.ex @@ -57,9 +57,11 @@ defmodule Towerops.Workers.CheckExecutorWorker do should_skip_snmp_check?(check) -> Logger.debug("Skipping SNMP check #{check_id} - Phoenix SNMP disabled or device assigned to agent") + schedule_next_check(check) should_skip_service_check?(check) -> Logger.debug("Skipping service check #{check_id} - device assigned to agent") + schedule_next_check(check) true -> execute_and_record(check) diff --git a/lib/towerops/workers/job_health_check_worker.ex b/lib/towerops/workers/job_health_check_worker.ex index 38c3b428..07bf8647 100644 --- a/lib/towerops/workers/job_health_check_worker.ex +++ b/lib/towerops/workers/job_health_check_worker.ex @@ -1,13 +1,12 @@ defmodule Towerops.Workers.JobHealthCheckWorker do @moduledoc """ - Oban worker that ensures all enabled devices have active monitoring jobs. + Oban worker that ensures all enabled jobs have active workers. Runs every 10 minutes via Oban.Plugins.Cron as a safety net to: 1. Find devices with monitoring_enabled=true but no active DeviceMonitorWorker job - 2. Automatically create missing jobs - 3. Log when missing jobs are found and recovered - - Note: SNMP polling is handled by per-check CheckExecutorWorker jobs, not device-level jobs. + 2. Find enabled checks with no active CheckExecutorWorker job + 3. Automatically create missing jobs + 4. Log when missing jobs are found and recovered This handles edge cases where jobs might get cancelled unexpectedly or database inconsistencies occur during deployments or pod failures. @@ -17,6 +16,8 @@ defmodule Towerops.Workers.JobHealthCheckWorker do import Ecto.Query alias Towerops.Devices + alias Towerops.Monitoring + alias Towerops.Monitoring.Check alias Towerops.Repo alias Towerops.Snmp.Client alias Towerops.Workers.DeviceMonitorWorker @@ -26,21 +27,60 @@ defmodule Towerops.Workers.JobHealthCheckWorker do @impl Oban.Worker @spec perform(Oban.Job.t()) :: :ok def perform(%Oban.Job{}) do + check_recoveries = recover_missing_check_executor_jobs() + + if check_recoveries > 0 do + Logger.warning("Job health check recovered #{check_recoveries} missing check executor job(s)") + end + if Client.phoenix_snmp_disabled() do - Logger.debug("Job health check skipped: Phoenix SNMP polling is disabled") + Logger.debug("Job health check: skipping monitor job recovery (Phoenix SNMP polling disabled)") else monitor_recoveries = recover_missing_monitor_jobs() if monitor_recoveries > 0 do Logger.warning("Job health check recovered #{monitor_recoveries} missing monitor job(s)") - else - Logger.debug("Job health check completed: all jobs healthy") end end + if check_recoveries == 0 do + Logger.debug("Job health check completed: all jobs healthy") + end + :ok end + defp recover_missing_check_executor_jobs do + active_check_ids = get_active_check_executor_check_ids() + + orphaned_checks = + Check + |> where([c], c.enabled == true) + |> Repo.all() + |> Enum.reject(&MapSet.member?(active_check_ids, &1.id)) + + Enum.each(orphaned_checks, fn check -> + Logger.debug( + "Check '#{check.name}' (#{check.check_type}) enabled but has no active executor job, scheduling", + check_id: check.id, + check_type: check.check_type + ) + + Monitoring.schedule_check(check) + end) + + length(orphaned_checks) + end + + defp get_active_check_executor_check_ids do + Oban.Job + |> where([j], j.worker == "Towerops.Workers.CheckExecutorWorker") + |> where([j], j.state in ["available", "scheduled", "executing", "retryable"]) + |> select([j], fragment("args->>'check_id'")) + |> Repo.all() + |> MapSet.new() + end + defp recover_missing_monitor_jobs do devices_with_monitoring = Devices.list_monitored_devices() active_monitor_job_device_ids = get_active_monitor_job_device_ids() diff --git a/priv/static/changelog.txt b/priv/static/changelog.txt index d5f7da37..cc5cd2a0 100644 --- a/priv/static/changelog.txt +++ b/priv/static/changelog.txt @@ -1,3 +1,19 @@ +2026-03-17 — Check Execution Fixes +* Fixed DNS and ping checks not executing in production +* Ping checks now automatically schedule when created for new devices +* Service checks (HTTP, TCP, DNS) now properly reschedule after being skipped +* Added automatic recovery for orphaned check jobs after pod restarts or deployments + +2026-03-17 — SSL Certificate Monitoring +* New SSL/TLS certificate expiration check type +* Monitors certificate validity and warns before expiration +* Service checks can now be delegated to remote agents for local network execution + +2026-03-17 — UI Improvements +* Fixed sticky header overlapping page content +* Removed device type column from device listings +* Skip SNMP discovery for devices with SNMP disabled + 2026-03-16 — Service Checks API * New REST API for managing service checks (HTTP, TCP, DNS, ping) * New GraphQL queries and mutations for service check management diff --git a/test/towerops/monitoring_test.exs b/test/towerops/monitoring_test.exs index 14f1c0e7..4ce3f0ce 100644 --- a/test/towerops/monitoring_test.exs +++ b/test/towerops/monitoring_test.exs @@ -1,6 +1,7 @@ defmodule Towerops.MonitoringTest do use Towerops.DataCase + import Ecto.Query import Towerops.AccountsFixtures import Towerops.AgentsFixtures import Towerops.OrganizationsFixtures @@ -8,6 +9,7 @@ defmodule Towerops.MonitoringTest do alias Towerops.Monitoring alias Towerops.Monitoring.Check alias Towerops.Monitoring.CheckResult + alias Towerops.Repo setup do user = user_fixture() @@ -561,6 +563,43 @@ defmodule Towerops.MonitoringTest do assert check.timeout_ms == 5000 end + test "schedules a CheckExecutorWorker job when creating a new ping check", %{device: device} do + # Delete existing ping check created by device setup, and all jobs + Repo.delete_all(from(c in Check, where: c.device_id == ^device.id and c.check_type == "ping")) + Repo.delete_all(Oban.Job) + + {:ok, check} = Monitoring.ensure_default_ping_check(device) + + jobs = + Repo.all( + from(j in Oban.Job, + where: j.worker == "Towerops.Workers.CheckExecutorWorker", + where: fragment("args->>'check_id' = ?", ^check.id) + ) + ) + + assert length(jobs) == 1 + end + + test "does not schedule duplicate job when check already exists", %{device: device} do + # Delete existing ping check created by device setup, and all jobs + Repo.delete_all(from(c in Check, where: c.device_id == ^device.id and c.check_type == "ping")) + Repo.delete_all(Oban.Job) + + {:ok, _check} = Monitoring.ensure_default_ping_check(device) + {:ok, _check} = Monitoring.ensure_default_ping_check(device) + + jobs = + Repo.all( + from(j in Oban.Job, + where: j.worker == "Towerops.Workers.CheckExecutorWorker" + ) + ) + + # Only one job from the initial creation, not a second from the idempotent call + assert length(jobs) == 1 + end + test "is idempotent - does not create duplicate", %{device: device} do {:ok, first} = Monitoring.ensure_default_ping_check(device) {:ok, second} = Monitoring.ensure_default_ping_check(device) diff --git a/test/towerops/snmp_test.exs b/test/towerops/snmp_test.exs index 032d5dd7..ee2839b2 100644 --- a/test/towerops/snmp_test.exs +++ b/test/towerops/snmp_test.exs @@ -2874,10 +2874,13 @@ defmodule Towerops.SnmpTest do }) |> Repo.insert!() + # Clear pre-existing jobs (e.g., from auto ping check creation) + Repo.delete_all(from(j in Oban.Job, where: j.worker == "Towerops.Workers.CheckExecutorWorker")) + assert {:ok, _counts} = Snmp.create_checks_from_discovery(device, snmp_device) - # Verify Oban job was scheduled - jobs = Repo.all(from j in Oban.Job, where: j.worker == "Towerops.Workers.CheckExecutorWorker") + # Verify Oban job was scheduled for the discovered sensor check + jobs = Repo.all(from(j in Oban.Job, where: j.worker == "Towerops.Workers.CheckExecutorWorker")) assert length(jobs) == 1 job = hd(jobs) diff --git a/test/towerops/workers/check_executor_worker_test.exs b/test/towerops/workers/check_executor_worker_test.exs index c5e5c727..ca645cd4 100644 --- a/test/towerops/workers/check_executor_worker_test.exs +++ b/test/towerops/workers/check_executor_worker_test.exs @@ -314,7 +314,7 @@ defmodule Towerops.Workers.CheckExecutorWorkerTest do end describe "perform/1 - SNMP check skipping" do - test "skips SNMP check when device is assigned to a local agent", %{ + test "skips SNMP check execution when device is assigned to agent but still reschedules", %{ device: device, organization: organization, snmp_device: snmp_device @@ -359,12 +359,14 @@ defmodule Towerops.Workers.CheckExecutorWorkerTest do results = Repo.all(from(r in CheckResult, where: r.check_id == ^check.id)) assert results == [] - # Should NOT reschedule + # Should still reschedule so the check runs again later + # (agent might disconnect, conditions might change) jobs = Repo.all(from(j in Oban.Job, where: j.worker == "Towerops.Workers.CheckExecutorWorker")) - assert jobs == [] + assert length(jobs) == 1 + assert hd(jobs).args["check_id"] == check.id end - test "skips snmp_interface check when device is assigned to agent", %{ + test "skips snmp_interface check execution when device is assigned to agent but still reschedules", %{ device: device, organization: organization } do @@ -390,9 +392,13 @@ defmodule Towerops.Workers.CheckExecutorWorkerTest do results = Repo.all(from(r in CheckResult, where: r.check_id == ^check.id)) assert results == [] + + # Should still reschedule + jobs = Repo.all(from(j in Oban.Job, where: j.worker == "Towerops.Workers.CheckExecutorWorker")) + assert length(jobs) == 1 end - test "skips service checks (HTTP/TCP/DNS) when device is assigned to agent", %{ + test "skips service check execution when device is assigned to agent but still reschedules", %{ device: device, organization: organization } do @@ -419,9 +425,14 @@ defmodule Towerops.Workers.CheckExecutorWorkerTest do # Should have no result since check was skipped results = Repo.all(from(r in CheckResult, where: r.check_id == ^check.id)) assert results == [] + + # Should still reschedule so conditions are re-evaluated later + jobs = Repo.all(from(j in Oban.Job, where: j.worker == "Towerops.Workers.CheckExecutorWorker")) + assert length(jobs) == 1 + assert hd(jobs).args["check_id"] == check.id end - test "skips SNMP check when phoenix_snmp_disabled is true", %{ + test "skips SNMP check execution when phoenix_snmp_disabled but still reschedules", %{ device: device, snmp_device: snmp_device } do @@ -476,9 +487,10 @@ defmodule Towerops.Workers.CheckExecutorWorkerTest do results = Repo.all(from(r in CheckResult, where: r.check_id == ^check.id)) assert results == [] - # No rescheduling + # Should still reschedule (SNMP may be re-enabled later) jobs = Repo.all(from(j in Oban.Job, where: j.worker == "Towerops.Workers.CheckExecutorWorker")) - assert jobs == [] + assert length(jobs) == 1 + assert hd(jobs).args["check_id"] == check.id end end end diff --git a/test/towerops/workers/job_health_check_worker_test.exs b/test/towerops/workers/job_health_check_worker_test.exs index 602a1146..607ce755 100644 --- a/test/towerops/workers/job_health_check_worker_test.exs +++ b/test/towerops/workers/job_health_check_worker_test.exs @@ -4,10 +4,128 @@ defmodule Towerops.Workers.JobHealthCheckWorkerTest do import Ecto.Query alias Towerops.DevicesFixtures + alias Towerops.Monitoring alias Towerops.Repo alias Towerops.Workers.JobHealthCheckWorker - describe "perform/1" do + describe "perform/1 - check executor recovery" do + test "recovers enabled checks that have no active CheckExecutorWorker job" do + device = DevicesFixtures.device_fixture(%{monitoring_enabled: true}) + + {:ok, check} = + Monitoring.create_check(%{ + organization_id: device.organization_id, + device_id: device.id, + name: "DNS Check", + check_type: "dns", + source_type: "manual", + interval_seconds: 60, + enabled: true, + config: %{"hostname" => "example.com"} + }) + + # Remove any jobs that were created + Repo.delete_all( + from(j in Oban.Job, + where: j.worker == "Towerops.Workers.CheckExecutorWorker" + ) + ) + + assert :ok = JobHealthCheckWorker.perform(%Oban.Job{}) + + # Should have created a CheckExecutorWorker job for the orphaned check + jobs = + Repo.all( + from(j in Oban.Job, + where: j.worker == "Towerops.Workers.CheckExecutorWorker", + where: fragment("args->>'check_id' = ?", ^check.id) + ) + ) + + assert length(jobs) == 1 + end + + test "does not create duplicate CheckExecutorWorker jobs for checks that already have one" do + device = DevicesFixtures.device_fixture(%{monitoring_enabled: true}) + + {:ok, check} = + Monitoring.create_check(%{ + organization_id: device.organization_id, + device_id: device.id, + name: "HTTP Check", + check_type: "http", + source_type: "manual", + interval_seconds: 60, + enabled: true, + config: %{"url" => "https://example.com"} + }) + + # Ensure there's already a job for this check + Monitoring.schedule_check(check) + + initial_count = + Repo.aggregate( + from(j in Oban.Job, + where: j.worker == "Towerops.Workers.CheckExecutorWorker" + ), + :count + ) + + assert :ok = JobHealthCheckWorker.perform(%Oban.Job{}) + + final_count = + Repo.aggregate( + from(j in Oban.Job, + where: j.worker == "Towerops.Workers.CheckExecutorWorker" + ), + :count + ) + + assert final_count == initial_count + end + + test "does not recover disabled checks" do + device = DevicesFixtures.device_fixture(%{monitoring_enabled: true}) + + # Disable the auto-created ping check so only the disabled DNS check exists + Repo.update_all( + from(c in Towerops.Monitoring.Check, where: c.device_id == ^device.id), + set: [enabled: false] + ) + + {:ok, _check} = + Monitoring.create_check(%{ + organization_id: device.organization_id, + device_id: device.id, + name: "Disabled DNS", + check_type: "dns", + source_type: "manual", + interval_seconds: 60, + enabled: false, + config: %{"hostname" => "example.com"} + }) + + Repo.delete_all( + from(j in Oban.Job, + where: j.worker == "Towerops.Workers.CheckExecutorWorker" + ) + ) + + assert :ok = JobHealthCheckWorker.perform(%Oban.Job{}) + + check_executor_jobs = + Repo.aggregate( + from(j in Oban.Job, + where: j.worker == "Towerops.Workers.CheckExecutorWorker" + ), + :count + ) + + assert check_executor_jobs == 0 + end + end + + describe "perform/1 - device monitor recovery" do test "recovers missing monitor jobs" do device = DevicesFixtures.device_fixture(%{monitoring_enabled: true}) @@ -33,10 +151,8 @@ defmodule Towerops.Workers.JobHealthCheckWorkerTest do test "does not create duplicate jobs if they already exist" do _device = DevicesFixtures.device_fixture(%{monitoring_enabled: true, snmp_enabled: true}) - # Only DeviceMonitorWorker job is created (SNMP polling via per-check jobs now) - + # Device creation creates DeviceMonitorWorker + CheckExecutorWorker (for ping check) initial_job_count = Repo.aggregate(Oban.Job, :count) - assert initial_job_count == 1 assert :ok = JobHealthCheckWorker.perform(%Oban.Job{}) @@ -45,33 +161,38 @@ defmodule Towerops.Workers.JobHealthCheckWorkerTest do end test "handles mixed scenario correctly" do - # Device 1: Monitoring enabled (should have 1 job - DeviceMonitorWorker) + # Device 1: Monitoring enabled (DeviceMonitorWorker + CheckExecutorWorker for ping) device1 = DevicesFixtures.device_fixture(%{monitoring_enabled: true, snmp_enabled: false}) - # Device 2: SNMP enabled (should have 0 device-level jobs - checks have their own jobs) + # Device 2: SNMP enabled (CheckExecutorWorker for ping only) _device2 = DevicesFixtures.device_fixture(%{monitoring_enabled: false, snmp_enabled: true}) - # Device 3: Both enabled (should have 1 job - DeviceMonitorWorker only) + # Device 3: Both enabled (DeviceMonitorWorker + CheckExecutorWorker for ping) _device3 = DevicesFixtures.device_fixture(%{monitoring_enabled: true, snmp_enabled: true}) - # Simulate missing job for device 1 by deleting it - Repo.delete_all(from j in Oban.Job, where: fragment("args->>'device_id' = ?", ^device1.id)) + # Simulate missing DeviceMonitorWorker job for device 1 by deleting it + Repo.delete_all( + from(j in Oban.Job, + where: j.worker == "Towerops.Workers.DeviceMonitorWorker", + where: fragment("args->>'device_id' = ?", ^device1.id) + ) + ) - # Ensure we only have jobs for device 3 left (1 job - DeviceMonitorWorker) - assert Repo.aggregate(Oban.Job, :count) == 1 + jobs_before = Repo.aggregate(Oban.Job, :count) assert :ok = JobHealthCheckWorker.perform(%Oban.Job{}) - # Verify job created for device 1 + # Verify DeviceMonitorWorker job created for device 1 assert Repo.one( - from j in Oban.Job, + from(j in Oban.Job, where: - j.worker == "Towerops.Workers.DeviceMonitorWorker" and fragment("args->>'device_id' = ?", ^device1.id) + j.worker == "Towerops.Workers.DeviceMonitorWorker" and + fragment("args->>'device_id' = ?", ^device1.id) + ) ) - # Device 2 has no device-level jobs (SNMP polling via per-check jobs) - # Total jobs should be 2 (1 existing for device3 + 1 new for device1) - assert Repo.aggregate(Oban.Job, :count) == 2 + # Should have exactly 1 more job (the recovered DeviceMonitorWorker) + assert Repo.aggregate(Oban.Job, :count) == jobs_before + 1 end end end