From f3cf1a9b3c7b00c61cc2214cf170b52bead81f20 Mon Sep 17 00:00:00 2001 From: Graham McIntire Date: Wed, 18 Mar 2026 17:29:36 -0500 Subject: [PATCH] skip all server-side checks when any agent is assigned (#77) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Summary - Add `device_has_effective_agent?/1` to Agents context that checks full cascade (device → site → org → global default) without distinguishing between cloud pollers and local agents - Update check_executor_worker, device_poller_worker, and device_monitor_worker to use new function - Phoenix never executes checks (DNS, ping, HTTP, TCP, SSL, SNMP) when any agent handles the device ## Problem The global default cloud poller was not being checked in `should_phoenix_poll_device?/1`, causing Phoenix to run DNS/ping/service checks server-side even when the Go agent was handling the device. This produced duplicate results with NXDOMAIN errors from the Erlang DNS resolver. ## Test plan - [x] 6 new tests for `device_has_effective_agent?/1` covering all cascade levels - [x] Full test suite passes (8601 tests) - [ ] Verify staging DNS check stops showing NXDOMAIN errors after deploy Reviewed-on: https://git.mcintire.me/graham/towerops-web/pulls/77 --- lib/towerops/agents.ex | 36 +++++++++ lib/towerops/workers/check_executor_worker.ex | 12 +-- lib/towerops/workers/device_monitor_worker.ex | 8 +- lib/towerops/workers/device_poller_worker.ex | 14 ++-- test/towerops/agents_test.exs | 77 +++++++++++++++++++ 5 files changed, 130 insertions(+), 17 deletions(-) diff --git a/lib/towerops/agents.ex b/lib/towerops/agents.ex index b641b4bb..d296bded 100644 --- a/lib/towerops/agents.ex +++ b/lib/towerops/agents.ex @@ -1011,6 +1011,42 @@ defmodule Towerops.Agents do end end + @doc """ + Returns true if the device has any effective agent assigned (cloud or local) + at any level: device, site, organization, or global default. + + Unlike `should_phoenix_poll_device?/1`, this makes no distinction between + cloud pollers and local agents. Any agent means the Go agent handles polling. + + Used by the check executor to decide whether to skip server-side execution. + """ + @spec device_has_effective_agent?(Ecto.UUID.t()) :: boolean() + def device_has_effective_agent?(device_id) do + query = + from d in Device, + left_join: aa in AgentAssignment, + on: aa.device_id == d.id and aa.enabled == true, + left_join: s in assoc(d, :site), + left_join: o in assoc(d, :organization), + where: d.id == ^device_id, + select: %{ + has_device_assignment: not is_nil(aa.agent_token_id), + has_site_token: not is_nil(s.agent_token_id), + has_org_token: not is_nil(o.default_agent_token_id) + } + + case Repo.one(query) do + nil -> + false + + result -> + result.has_device_assignment or + result.has_site_token or + result.has_org_token or + Towerops.Settings.get_global_default_cloud_poller() != nil + end + end + ## PubSub notifications @doc """ diff --git a/lib/towerops/workers/check_executor_worker.ex b/lib/towerops/workers/check_executor_worker.ex index dc2fa152..139e085f 100644 --- a/lib/towerops/workers/check_executor_worker.ex +++ b/lib/towerops/workers/check_executor_worker.ex @@ -24,7 +24,6 @@ defmodule Towerops.Workers.CheckExecutorWorker do max_attempts: 3 alias Towerops.Agents - alias Towerops.Devices.Device alias Towerops.Monitoring alias Towerops.Monitoring.Executors.DnsExecutor alias Towerops.Monitoring.Executors.HttpExecutor @@ -78,21 +77,22 @@ defmodule Towerops.Workers.CheckExecutorWorker do defp should_skip_snmp_check?(%{check_type: check_type} = check) when check_type in @snmp_check_types do Client.phoenix_snmp_disabled() or - (check.device_id != nil and not Agents.should_phoenix_poll_device?(%Device{id: check.device_id})) + (check.device_id != nil and Agents.device_has_effective_agent?(check.device_id)) end defp should_skip_snmp_check?(_check), do: false defp should_skip_service_check?(%{check_type: check_type} = check) when check_type in @service_check_types do - # Skip service checks (HTTP/TCP/DNS) if device is assigned to an agent - # The agent will execute these checks from the device's network location - check.device_id != nil and not Agents.should_phoenix_poll_device?(%Device{id: check.device_id}) + # Skip service checks (HTTP/TCP/DNS/SSL) if device has any agent (cloud or local) + # The agent handles all check execution - no fallback to Phoenix + check.device_id != nil and Agents.device_has_effective_agent?(check.device_id) end defp should_skip_service_check?(_check), do: false defp should_skip_ping_check?(%{check_type: "ping"} = check) do - check.device_id != nil and not Agents.should_phoenix_poll_device?(%Device{id: check.device_id}) + # Skip ping if device has any agent (cloud or local) + check.device_id != nil and Agents.device_has_effective_agent?(check.device_id) end defp should_skip_ping_check?(_check), do: false diff --git a/lib/towerops/workers/device_monitor_worker.ex b/lib/towerops/workers/device_monitor_worker.ex index 4fdd8c29..2822815f 100644 --- a/lib/towerops/workers/device_monitor_worker.ex +++ b/lib/towerops/workers/device_monitor_worker.ex @@ -60,10 +60,10 @@ defmodule Towerops.Workers.DeviceMonitorWorker do end # Check if device should continue being monitored - # Returns false if Phoenix SNMP is disabled, monitoring is disabled, or device is assigned to an agent + # Returns false if Phoenix SNMP is disabled, monitoring is disabled, or device has any agent defp should_continue_monitoring?(device) do !Client.phoenix_snmp_disabled() && device.monitoring_enabled && - Agents.should_phoenix_poll_device?(device) + !Agents.device_has_effective_agent?(device.id) end defp maybe_perform_check(device) do @@ -74,8 +74,8 @@ defmodule Towerops.Workers.DeviceMonitorWorker do not device.monitoring_enabled -> :ok - not Agents.should_phoenix_poll_device?(device) -> - Logger.debug("Skipping Phoenix monitoring for device #{device.name} - assigned to non-cloud-poller agent") + Agents.device_has_effective_agent?(device.id) -> + Logger.debug("Skipping Phoenix monitoring for device #{device.name} - device has agent assigned") :ok true -> diff --git a/lib/towerops/workers/device_poller_worker.ex b/lib/towerops/workers/device_poller_worker.ex index a8428fd2..ec23500f 100644 --- a/lib/towerops/workers/device_poller_worker.ex +++ b/lib/towerops/workers/device_poller_worker.ex @@ -77,10 +77,10 @@ defmodule Towerops.Workers.DevicePollerWorker do end # Check if device should continue being polled - # Returns false if Phoenix SNMP is disabled, SNMP is disabled on device, or device is assigned to an agent + # Returns false if Phoenix SNMP is disabled, SNMP is disabled on device, or device has any agent defp should_continue_polling?(device) do !Client.phoenix_snmp_disabled() && device.snmp_enabled && - Agents.should_phoenix_poll_device?(device) + !Agents.device_has_effective_agent?(device.id) end defp maybe_poll_device(device) do @@ -91,8 +91,8 @@ defmodule Towerops.Workers.DevicePollerWorker do not device.snmp_enabled -> :ok - not Agents.should_phoenix_poll_device?(device) -> - Logger.debug("Skipping Phoenix poll for device #{device.name} - assigned to non-cloud-poller agent") + Agents.device_has_effective_agent?(device.id) -> + Logger.debug("Skipping Phoenix poll for device #{device.name} - device has agent assigned") :ok true -> @@ -272,10 +272,10 @@ defmodule Towerops.Workers.DevicePollerWorker do :reassigned device -> - if Agents.should_phoenix_poll_device?(device) do - :ok - else + if Agents.device_has_effective_agent?(device.id) do :reassigned + else + :ok end end end diff --git a/test/towerops/agents_test.exs b/test/towerops/agents_test.exs index c57b5277..7bd116b0 100644 --- a/test/towerops/agents_test.exs +++ b/test/towerops/agents_test.exs @@ -1814,6 +1814,83 @@ defmodule Towerops.AgentsTest do end end + describe "device_has_effective_agent?/1" do + setup %{organization: org} do + {:ok, site} = + Towerops.Sites.create_site(%{ + name: "Test Site", + organization_id: org.id + }) + + {:ok, device} = + Towerops.Devices.create_device(%{ + name: "Test Device", + ip_address: "192.168.1.1", + site_id: site.id, + organization_id: org.id + }) + + {:ok, local_agent, _} = Agents.create_agent_token(org.id, "Local Agent") + {:ok, cloud_poller, _} = Agents.create_cloud_poller("Cloud Poller") + + %{ + site: site, + device: device, + local_agent: local_agent, + cloud_poller: cloud_poller + } + end + + test "returns false when device has no agent assigned", %{device: device} do + assert Agents.device_has_effective_agent?(device.id) == false + end + + test "returns true when device is assigned to local agent", %{ + device: device, + local_agent: local_agent + } do + {:ok, _} = Agents.assign_device_to_agent(local_agent.id, device.id) + assert Agents.device_has_effective_agent?(device.id) == true + end + + test "returns true when device is assigned to cloud poller", %{ + device: device, + cloud_poller: cloud_poller + } do + {:ok, _} = Agents.assign_device_to_agent(cloud_poller.id, device.id) + assert Agents.device_has_effective_agent?(device.id) == true + end + + test "returns true when site has agent assigned", %{ + device: device, + site: site, + local_agent: local_agent + } do + {:ok, _} = Towerops.Sites.update_site(site, %{agent_token_id: local_agent.id}) + assert Agents.device_has_effective_agent?(device.id) == true + end + + test "returns true when organization has default agent", %{ + organization: org, + device: device, + local_agent: local_agent + } do + {:ok, _} = Towerops.Organizations.update_organization(org, %{default_agent_token_id: local_agent.id}) + assert Agents.device_has_effective_agent?(device.id) == true + end + + test "returns true when global default cloud poller is set", %{ + device: device, + cloud_poller: cloud_poller + } do + {:ok, _} = Towerops.Settings.set_global_default_cloud_poller(cloud_poller.id) + assert Agents.device_has_effective_agent?(device.id) == true + + # Clean up + {:ok, _} = Towerops.Settings.set_global_default_cloud_poller(nil) + end + end + describe "list_updatable_agents/0" do test "returns agents with recent last_seen_at", %{organization: org} do {:ok, agent, _token} = Agents.create_agent_token(org.id, "Active Agent")