From 2b50fc07329315725b068a53bc4cd4848ff36c86 Mon Sep 17 00:00:00 2001 From: Graham McIntire Date: Wed, 18 Mar 2026 20:17:39 -0500 Subject: [PATCH] fix agent check cascade and at-risk subscriber double-counting (#79) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Summary - **`list_checks_for_agent` cascade**: the agent query only matched checks with `agent_token_id` set directly on the check record — checks on devices inheriting their agent via site/org/global default were never sent to the Go agent. Now uses left joins with the full cascade (device assignment → site → org default → global default cloud poller), matching the pattern from `list_agent_polling_targets`. - **At-risk summary double-counting**: `get_impact_summary` called `analyze_device_impact` per down AP, and each returned the full site subscriber count. With 3 APs down at a 147-sub site it showed 441 subs / $31k instead of 147 / $10k. Fixed by deduplicating impact per site. Same fix applied to `calculate_down_impact` in site impact summaries. ## Test plan - [x] 3 new cascade tests for `list_checks_for_agent` (site, org default, global default) - [x] 3 new tests for `get_impact_summary` (single site dedup, multi-site sum, zero case) - [x] All 8,607 existing tests pass - [ ] Deploy to staging, verify At Risk numbers on dashboard - [ ] Verify Go agent receives DNS/HTTP checks via cascade Reviewed-on: https://git.mcintire.me/graham/towerops-web/pulls/79 --- lib/towerops/dashboard.ex | 34 +++++---- lib/towerops/monitoring.ex | 77 +++++++++++++++++++-- test/towerops/dashboard_test.exs | 111 ++++++++++++++++++++++++++++++ test/towerops/monitoring_test.exs | 50 ++++++++++++++ 4 files changed, 254 insertions(+), 18 deletions(-) diff --git a/lib/towerops/dashboard.ex b/lib/towerops/dashboard.ex index 4133daab..591ad07d 100644 --- a/lib/towerops/dashboard.ex +++ b/lib/towerops/dashboard.ex @@ -123,17 +123,27 @@ defmodule Towerops.Dashboard do def get_impact_summary(organization_id) do down_devices = Devices.list_organization_devices(organization_id, %{"status" => "down"}) - # Compute impact for each down device - impacts = - Enum.map(down_devices, fn device -> + # Deduplicate by site: site-level impact is the same for every AP at a site, + # so analyze one representative device per site to avoid counting subscribers N times. + {with_site, without_site} = Enum.split_with(down_devices, & &1.site_id) + + site_impacts = + with_site + |> Enum.group_by(& &1.site_id) + |> Enum.map(fn {_site_id, devices} -> + ImpactAnalysis.analyze_device_impact(organization_id, hd(devices).id) + end) + + no_site_impacts = + Enum.map(without_site, fn device -> ImpactAnalysis.analyze_device_impact(organization_id, device.id) end) - # Aggregate — count unique sites with outages + impacts = site_impacts ++ no_site_impacts + sites_with_outages = - down_devices + with_site |> Enum.map(& &1.site_id) - |> Enum.reject(&is_nil/1) |> Enum.uniq() |> length() @@ -266,13 +276,9 @@ defmodule Towerops.Dashboard do defp calculate_down_impact(_organization_id, _down_devices, 0), do: {0, Decimal.new("0")} defp calculate_down_impact(organization_id, down_devices, _down_count) do - impacts = - Enum.map(down_devices, fn device -> - ImpactAnalysis.analyze_device_impact(organization_id, device.id) - end) - - subs = impacts |> Enum.map(& &1.total_subscribers) |> Enum.sum() - mrr = impacts |> Enum.map(& &1.total_mrr) |> Enum.reduce(Decimal.new("0"), &Decimal.add/2) - {subs, mrr} + # All devices are at the same site, so site-level impact is identical for each. + # Use one device to avoid counting site subscribers once per AP. + impact = ImpactAnalysis.analyze_device_impact(organization_id, hd(down_devices).id) + {impact.total_subscribers, impact.total_mrr} end end diff --git a/lib/towerops/monitoring.ex b/lib/towerops/monitoring.ex index 19ef84a9..22300d38 100644 --- a/lib/towerops/monitoring.ex +++ b/lib/towerops/monitoring.ex @@ -7,6 +7,7 @@ defmodule Towerops.Monitoring do import Ecto.Query + alias Towerops.Agents.AgentAssignment alias Towerops.Devices.Device alias Towerops.Monitoring.Check alias Towerops.Monitoring.CheckResult @@ -50,22 +51,90 @@ defmodule Towerops.Monitoring do end @doc """ - Returns the list of service checks assigned to a specific agent. + Returns the list of service checks for devices assigned to a specific agent. - Only returns http/tcp/dns checks (service checks that agents can execute). + Uses the full agent assignment cascade (device → site → org → global default) + to find checks, not just the check's own agent_token_id field. + Only returns http/tcp/dns/ssl checks (service checks that agents can execute). """ def list_checks_for_agent(agent_token_id, opts \\ []) do + global_default = Towerops.Settings.get_global_default_cloud_poller() + is_global_default = agent_token_id == global_default + query = from c in Check, - where: c.agent_token_id == ^agent_token_id, + left_join: d in assoc(c, :device), + left_join: s in assoc(d, :site), + left_join: o in assoc(d, :organization), + left_join: aa in AgentAssignment, + on: aa.device_id == d.id and aa.enabled == true, + left_join: disabled in AgentAssignment, + on: + disabled.device_id == d.id and disabled.agent_token_id == ^agent_token_id and + disabled.enabled == false, where: c.check_type in ["http", "tcp", "dns", "ssl"], - order_by: [asc: c.name] + order_by: [asc: c.name], + distinct: c.id query + |> where_check_agent_matches(agent_token_id, is_global_default) |> maybe_filter_by_enabled(opts[:enabled]) |> Repo.all() end + # Builds the WHERE clause for matching checks to an agent via the full cascade: + # direct check assignment → device assignment → site → org default → global default + defp where_check_agent_matches(query, agent_token_id, is_global_default) do + condition = + [c] + |> dynamic(c.agent_token_id == ^agent_token_id) + |> or_device_assignment(agent_token_id) + |> or_site_assignment(agent_token_id) + |> or_org_default(agent_token_id) + |> or_global_default(is_global_default) + + from q in query, where: ^condition + end + + defp or_device_assignment(condition, agent_token_id) do + dynamic( + [c, d, s, o, aa, disabled], + ^condition or + (not is_nil(d.id) and aa.agent_token_id == ^agent_token_id and is_nil(disabled.id)) + ) + end + + defp or_site_assignment(condition, agent_token_id) do + dynamic( + [c, d, s, o, aa, disabled], + ^condition or + (not is_nil(d.id) and is_nil(aa.agent_token_id) and is_nil(disabled.id) and + not is_nil(s.id) and s.agent_token_id == ^agent_token_id) + ) + end + + defp or_org_default(condition, agent_token_id) do + dynamic( + [c, d, s, o, aa, disabled], + ^condition or + (not is_nil(d.id) and is_nil(aa.agent_token_id) and is_nil(disabled.id) and + (is_nil(s.id) or is_nil(s.agent_token_id)) and + o.default_agent_token_id == ^agent_token_id) + ) + end + + defp or_global_default(condition, false), do: condition + + defp or_global_default(condition, true) do + dynamic( + [c, d, s, o, aa, disabled], + ^condition or + (not is_nil(d.id) and is_nil(aa.agent_token_id) and is_nil(disabled.id) and + (is_nil(s.id) or is_nil(s.agent_token_id)) and + is_nil(o.default_agent_token_id)) + ) + end + @doc """ Gets a single check. """ diff --git a/test/towerops/dashboard_test.exs b/test/towerops/dashboard_test.exs index 66f606df..13a1d172 100644 --- a/test/towerops/dashboard_test.exs +++ b/test/towerops/dashboard_test.exs @@ -6,6 +6,7 @@ defmodule Towerops.DashboardTest do import Towerops.OrganizationsFixtures alias Towerops.Dashboard + alias Towerops.Gaiia alias Towerops.Integrations setup do @@ -143,6 +144,116 @@ defmodule Towerops.DashboardTest do end end + describe "get_impact_summary/1" do + test "does not multiply site subscribers by number of down APs", %{org: org} do + site = site_fixture(org.id) + + {:ok, _network_site} = + Gaiia.upsert_network_site(org.id, %{ + gaiia_id: "gaiia-site-1", + name: "Tower 3", + account_count: 147, + total_mrr: Decimal.new("10479.00") + }) + + Gaiia.update_network_site_mapping( + Gaiia.get_network_site(org.id, "gaiia-site-1"), + %{site_id: site.id} + ) + + # Create 3 APs at this site, all down + for i <- 1..3 do + device = + device_fixture(%{ + organization_id: org.id, + site_id: site.id, + name: "AP #{i}" + }) + + Towerops.Devices.update_device(device, %{status: :down}) + end + + summary = Dashboard.get_impact_summary(org.id) + + # Should be 147 subscribers (site total), NOT 147 * 3 = 441 + assert summary.subscribers_affected == 147 + assert Decimal.equal?(summary.mrr_at_risk, Decimal.new("10479.00")) + assert summary.down_device_count == 3 + assert summary.sites_with_outages == 1 + end + + test "sums subscribers across different sites correctly", %{org: org} do + # Site 1 with 100 subscribers, 2 APs down + site1 = site_fixture(org.id, %{name: "Tower A"}) + + {:ok, _} = + Gaiia.upsert_network_site(org.id, %{ + gaiia_id: "site-a", + name: "Tower A", + account_count: 100, + total_mrr: Decimal.new("5000.00") + }) + + Gaiia.update_network_site_mapping( + Gaiia.get_network_site(org.id, "site-a"), + %{site_id: site1.id} + ) + + for i <- 1..2 do + device = + device_fixture(%{ + organization_id: org.id, + site_id: site1.id, + name: "Site1 AP #{i}" + }) + + Towerops.Devices.update_device(device, %{status: :down}) + end + + # Site 2 with 50 subscribers, 1 AP down + site2 = site_fixture(org.id, %{name: "Tower B"}) + + {:ok, _} = + Gaiia.upsert_network_site(org.id, %{ + gaiia_id: "site-b", + name: "Tower B", + account_count: 50, + total_mrr: Decimal.new("2500.00") + }) + + Gaiia.update_network_site_mapping( + Gaiia.get_network_site(org.id, "site-b"), + %{site_id: site2.id} + ) + + device = + device_fixture(%{ + organization_id: org.id, + site_id: site2.id, + name: "Site2 AP 1" + }) + + Towerops.Devices.update_device(device, %{status: :down}) + + summary = Dashboard.get_impact_summary(org.id) + + # 100 + 50 = 150 total subscribers across 2 sites + assert summary.subscribers_affected == 150 + assert Decimal.equal?(summary.mrr_at_risk, Decimal.new("7500.00")) + assert summary.down_device_count == 3 + assert summary.sites_with_outages == 2 + end + + test "returns zero impact when no devices are down", %{org: org} do + summary = Dashboard.get_impact_summary(org.id) + + assert summary.subscribers_affected == 0 + assert Decimal.equal?(summary.mrr_at_risk, Decimal.new("0")) + assert summary.down_device_count == 0 + assert summary.sites_with_outages == 0 + end + end + describe "list_site_summaries/1" do test "returns enriched site data", %{org: org} do {:ok, site} = diff --git a/test/towerops/monitoring_test.exs b/test/towerops/monitoring_test.exs index 4e019940..e877ce01 100644 --- a/test/towerops/monitoring_test.exs +++ b/test/towerops/monitoring_test.exs @@ -185,6 +185,56 @@ defmodule Towerops.MonitoringTest do assert Monitoring.list_checks_for_agent(agent2.id) == [] end + + test "returns checks for devices inheriting agent via site", %{organization: org, device: device, site: site} do + {:ok, agent_token, _} = agent_token_fixture(org.id) + Towerops.Sites.update_site(site, %{agent_token_id: agent_token.id}) + + {:ok, dns_check} = + Monitoring.create_check( + valid_check_attrs(org.id, %{ + name: "DNS Check", + check_type: "dns", + config: %{"hostname" => "example.com", "record_type" => "A"}, + device_id: device.id + }) + ) + + checks = Monitoring.list_checks_for_agent(agent_token.id, enabled: true) + ids = Enum.map(checks, & &1.id) + assert dns_check.id in ids + end + + test "returns checks for devices inheriting agent via org default", %{organization: org, device: device} do + {:ok, agent_token, _} = agent_token_fixture(org.id) + Towerops.Organizations.update_organization(org, %{default_agent_token_id: agent_token.id}) + + {:ok, http_check} = + Monitoring.create_check(valid_check_attrs(org.id, %{name: "HTTP Check", device_id: device.id})) + + checks = Monitoring.list_checks_for_agent(agent_token.id, enabled: true) + ids = Enum.map(checks, & &1.id) + assert http_check.id in ids + end + + test "returns checks for devices inheriting global default cloud poller", %{organization: org, device: device} do + {:ok, cloud_poller, _token} = Towerops.Agents.create_cloud_poller("Global Cloud Poller") + Towerops.Settings.set_global_default_cloud_poller(cloud_poller.id) + + {:ok, dns_check} = + Monitoring.create_check( + valid_check_attrs(org.id, %{ + name: "DNS Check", + check_type: "dns", + config: %{"hostname" => "example.com", "record_type" => "A"}, + device_id: device.id + }) + ) + + checks = Monitoring.list_checks_for_agent(cloud_poller.id, enabled: true) + ids = Enum.map(checks, & &1.id) + assert dns_check.id in ids + end end describe "get_check/1 and get_check!/1" do