fix: dns and ping checks not executing in production (#71)

## Summary

- **Ping checks never scheduled**: `ensure_default_ping_check` created check records but never called `schedule_check`, so auto-created ping checks never executed
- **Skipped checks permanently orphaned**: `CheckExecutorWorker` only rescheduled checks that actually executed — skipped checks (agent-assigned, SNMP disabled) broke the scheduling chain permanently
- **No recovery mechanism**: `JobHealthCheckWorker` only recovered `DeviceMonitorWorker` jobs, not `CheckExecutorWorker` jobs — any lost check job was gone forever

## Changes

- `monitoring.ex`: Call `schedule_check` after creating ping check in `ensure_default_ping_check`
- `check_executor_worker.ex`: Reschedule skipped-but-enabled checks so conditions are re-evaluated next cycle
- `job_health_check_worker.ex`: Recover orphaned `CheckExecutorWorker` jobs for enabled checks (runs every 10 min)
- `devices.ex`: Wrap `ensure_default_ping_check` in try/rescue for deadlock resilience

## Test plan

- [x] New tests: ping check scheduling on creation, no duplicate scheduling on idempotent call
- [x] Updated tests: skipped checks now assert rescheduling instead of orphaning
- [x] New tests: health check worker recovers orphaned check executor jobs, skips disabled checks
- [x] All 177 targeted tests pass
- [x] Compile clean with warnings-as-errors
- [x] Pre-commit hooks pass (credo, format)

Reviewed-on: graham/towerops-web#71

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

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)
 

lib/towerops/monitoring.ex

@@ -434,17 +434,22 @@ defmodule Towerops.Monitoring do
 
     case existing do
       nil ->
-        create_check(%{
+        with {:ok, check} <-
-          organization_id: device.organization_id,
+               create_check(%{
-          device_id: device.id,
+                 organization_id: device.organization_id,
-          name: "ICMP Ping",
+                 device_id: device.id,
-          check_type: "ping",
+                 name: "ICMP Ping",
-          config: %{"host" => device.ip_address, "count" => 3},
+                 check_type: "ping",
-          enabled: true,
+                 config: %{"host" => device.ip_address, "count" => 3},
-          interval_seconds: 60,
+                 enabled: true,
-          timeout_ms: 5000,
+                 interval_seconds: 60,
-          source_type: "auto_discovery"
+                 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

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)

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
+  2. Find enabled checks with no active CheckExecutorWorker job
-  3. Log when missing jobs are found and recovered
+  3. Automatically create missing jobs
-
+  4. Log when missing jobs are found and recovered
-  Note: SNMP polling is handled by per-check CheckExecutorWorker jobs, not device-level jobs.
 
   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()

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

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)

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
+      # Verify Oban job was scheduled for the discovered sensor check
-      jobs = Repo.all(from j in Oban.Job, where: j.worker == "Towerops.Workers.CheckExecutorWorker")
+      jobs = Repo.all(from(j in Oban.Job, where: j.worker == "Towerops.Workers.CheckExecutorWorker"))
       assert length(jobs) == 1
 
       job = hd(jobs)

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

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
+      # Simulate missing DeviceMonitorWorker job for device 1 by deleting it
-      Repo.delete_all(from j in Oban.Job, where: fragment("args->>'device_id' = ?", ^device1.id))
+      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)
+      jobs_before = Repo.aggregate(Oban.Job, :count)
-      assert Repo.aggregate(Oban.Job, :count) == 1
 
       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)
+      # Should have exactly 1 more job (the recovered DeviceMonitorWorker)
-      # Total jobs should be 2 (1 existing for device3 + 1 new for device1)
+      assert Repo.aggregate(Oban.Job, :count) == jobs_before + 1
-      assert Repo.aggregate(Oban.Job, :count) == 2
     end
   end
 end