From 872f717dba9af5667ed206c3531f8a8927ee8d72 Mon Sep 17 00:00:00 2001 From: Graham McIntire Date: Mon, 23 Mar 2026 17:03:41 -0500 Subject: [PATCH] Fix critical bugs, memory leaks, and N+1 queries (#132) CRITICAL FIXES: - Fix unsafe pattern matching in SNMP discovery that would crash on timeout - Changed {:ok, _} = ... to proper case statements with error handling - Extracted safe_discover/3 helper to reduce cyclomatic complexity - Added fallback to empty lists and warning logging for timeouts - Affects: discover_vlans, discover_ip_addresses, discover_processors, discover_storage MEMORY LEAK FIXES (JS Hooks): - GlobalSearchTrigger: Added destroyed() cleanup for click listener - SidebarCollapse: Added destroyed() cleanup for click listener - ThemeSelector: Added destroyed() cleanup for phx:set-theme window listener - NetworkMap: Added destroyed() cleanup for 3 button listeners (zoom in/out/fit) - All hooks now properly store handler references and remove listeners on unmount LIVEVIEW FIXES: - Added phx-update="ignore" to 5 SensorChart hooks to prevent chart re-initialization - Added catch-all handle_info(_msg, socket) to 5 LiveViews to prevent crashes on unexpected messages - device_live/show.ex, device_live/index.ex, alert_live/index.ex, activity_feed_live.ex, agent_live/index.ex N+1 QUERY FIXES: - Created Gaiia.get_site_subscriber_summaries/1 batch function - Refactored alert_live and device_live to use batch query instead of looping - Reduces queries from N to 1 when loading site subscriber data Impact: - Prevents discovery worker crashes during SNMP timeouts - Eliminates memory accumulation from leaked event handlers - Prevents chart state loss on LiveView updates - Prevents LiveView crashes from unexpected PubSub messages - Improves database performance by eliminating N+1 queries in alert/device views - Passes credo --strict with 0 issues Reviewed-on: https://git.mcintire.me/graham/towerops-web/pulls/132 --- assets/js/app.ts | 84 +++++++++++++++---- lib/towerops/gaiia.ex | 11 +++ lib/towerops/snmp/discovery.ex | 26 ++++-- lib/towerops_web/live/alert_live/index.ex | 14 ++-- lib/towerops_web/live/device_live/index.ex | 24 ++++-- lib/towerops_web/live/device_live/show.ex | 2 + .../live/device_live/show.html.heex | 5 ++ .../live/agent_live/helpers_test.exs | 2 +- 8 files changed, 137 insertions(+), 31 deletions(-) diff --git a/assets/js/app.ts b/assets/js/app.ts index 524ad01d..abfe6f97 100644 --- a/assets/js/app.ts +++ b/assets/js/app.ts @@ -576,6 +576,10 @@ const BetaBannerDismiss = { const NetworkMap = { cy: null as any, _topologyData: null as any, + _zoomInHandler: null as (() => void) | null, + _zoomOutHandler: null as (() => void) | null, + _fitHandler: null as (() => void) | null, + mounted(this: any) { const topologyData = JSON.parse(this.el.dataset.topology || '{}') this._topologyData = topologyData @@ -614,6 +618,24 @@ const NetworkMap = { }, destroyed(this: any) { + // Clean up event listeners + const zoomIn = document.getElementById('cy-zoom-in') + const zoomOut = document.getElementById('cy-zoom-out') + const fit = document.getElementById('cy-fit') + + if (zoomIn && this._zoomInHandler) { + zoomIn.removeEventListener('click', this._zoomInHandler) + this._zoomInHandler = null + } + if (zoomOut && this._zoomOutHandler) { + zoomOut.removeEventListener('click', this._zoomOutHandler) + this._zoomOutHandler = null + } + if (fit && this._fitHandler) { + fit.removeEventListener('click', this._fitHandler) + this._fitHandler = null + } + if (this.cy) { // Stop any running layout before destroying to prevent async callbacks // from accessing a destroyed renderer @@ -633,30 +655,33 @@ const NetworkMap = { const fit = document.getElementById('cy-fit') if (zoomIn) { - zoomIn.addEventListener('click', () => { + this._zoomInHandler = () => { if (!this.cy) return this.cy.zoom({ level: Math.min(this.cy.zoom() + ZOOM_STEP, this.cy.maxZoom()), renderedPosition: { x: this.cy.width() / 2, y: this.cy.height() / 2 } }) - }) + } + zoomIn.addEventListener('click', this._zoomInHandler) } if (zoomOut) { - zoomOut.addEventListener('click', () => { + this._zoomOutHandler = () => { if (!this.cy) return this.cy.zoom({ level: Math.max(this.cy.zoom() - ZOOM_STEP, this.cy.minZoom()), renderedPosition: { x: this.cy.width() / 2, y: this.cy.height() / 2 } }) - }) + } + zoomOut.addEventListener('click', this._zoomOutHandler) } if (fit) { - fit.addEventListener('click', () => { + this._fitHandler = () => { if (!this.cy) return this.cy.fit(undefined, 50) - }) + } + fit.addEventListener('click', this._fitHandler) } }, @@ -1693,11 +1718,21 @@ const userTimezone = Intl.DateTimeFormat().resolvedOptions().timeZone || "UTC" // Global Search (Cmd+K / Ctrl+K) hook const GlobalSearchTrigger = { - mounted() { - this.el.addEventListener("click", () => { + handleClick: null as ((e: Event) => void) | null, + + mounted(this: any) { + this.handleClick = () => { // Simulate Cmd+K to open global search document.dispatchEvent(new KeyboardEvent("keydown", { key: "k", metaKey: true })) - }) + } + this.el.addEventListener("click", this.handleClick) + }, + + destroyed(this: any) { + if (this.handleClick) { + this.el.removeEventListener("click", this.handleClick) + this.handleClick = null + } } } @@ -1737,23 +1772,44 @@ const StatusTitle = { // Sidebar collapse - toggle class on (which LiveView never patches) // so the state persists across all navigations without flicker. const SidebarCollapse = { + handleClick: null as ((e: Event) => void) | null, + mounted(this: any) { - this.el.addEventListener('click', (e: Event) => { + this.handleClick = (e: Event) => { if ((e.target as HTMLElement).closest('[data-sidebar-toggle]')) { const html = document.documentElement html.classList.toggle('sidebar-collapsed') localStorage.setItem('sidebarCollapsed', String(html.classList.contains('sidebar-collapsed'))) } - }) + } + this.el.addEventListener('click', this.handleClick) + }, + + destroyed(this: any) { + if (this.handleClick) { + this.el.removeEventListener('click', this.handleClick) + this.handleClick = null + } } } const ThemeSelector = { - mounted() { + handleThemeChange: null as (() => void) | null, + + mounted(this: any) { this.updateActive() - window.addEventListener("phx:set-theme", () => this.updateActive()) + this.handleThemeChange = () => this.updateActive() + window.addEventListener("phx:set-theme", this.handleThemeChange) }, - updateActive() { + + destroyed(this: any) { + if (this.handleThemeChange) { + window.removeEventListener("phx:set-theme", this.handleThemeChange) + this.handleThemeChange = null + } + }, + + updateActive(this: any) { const current = localStorage.getItem("phx:theme") || "system" this.el.querySelectorAll("[data-phx-theme]").forEach((btn) => { const isActive = btn.dataset.phxTheme === current diff --git a/lib/towerops/gaiia.ex b/lib/towerops/gaiia.ex index fce22f58..de7db9f1 100644 --- a/lib/towerops/gaiia.ex +++ b/lib/towerops/gaiia.ex @@ -288,6 +288,17 @@ defmodule Towerops.Gaiia do |> Repo.one() end + @doc "Get subscriber summaries for multiple sites in a single query to prevent N+1." + def get_site_subscriber_summaries(site_ids) when is_list(site_ids) do + NetworkSite + |> where([ns], ns.site_id in ^site_ids) + |> select([ns], {ns.site_id, %{account_count: ns.account_count, total_mrr: ns.total_mrr}}) + |> Repo.all() + |> Map.new() + end + + def get_site_subscriber_summaries(_), do: %{} + # --- Device Impact Queries --- @doc """ diff --git a/lib/towerops/snmp/discovery.ex b/lib/towerops/snmp/discovery.ex index 206c8cbd..3431a576 100644 --- a/lib/towerops/snmp/discovery.ex +++ b/lib/towerops/snmp/discovery.ex @@ -233,19 +233,21 @@ defmodule Towerops.Snmp.Discovery do [] end - # Secondary discovery: VLANs, IPs, processors, storage (already fall back to [] on timeout) + # Secondary discovery: VLANs, IPs, processors, storage Logger.info("Discovering VLANs...", device_id: device.id) - {:ok, vlans} = discover_vlans_with_timeout(client_opts, profile, timeouts) + vlans = safe_discover(fn -> discover_vlans_with_timeout(client_opts, profile, timeouts) end, "VLAN", device.id) Logger.info("Discovering IP addresses...", device_id: device.id) - {:ok, ip_addresses} = discover_ip_addresses_with_timeout(client_opts, timeouts) + ip_addresses = safe_discover(fn -> discover_ip_addresses_with_timeout(client_opts, timeouts) end, "IP", device.id) _ = log_ip_discovery_results(device, ip_addresses) Logger.info("Discovering processors...", device_id: device.id) - {:ok, processors} = discover_processors_with_timeout(client_opts, timeouts) + + processors = + safe_discover(fn -> discover_processors_with_timeout(client_opts, timeouts) end, "Processor", device.id) Logger.info("Discovering storage...", device_id: device.id) - {:ok, storage} = discover_storage_with_timeout(client_opts, timeouts) + storage = safe_discover(fn -> discover_storage_with_timeout(client_opts, timeouts) end, "Storage", device.id) Logger.info("Saving discovery results (including IP addresses and processors)...", device_id: device.id) @@ -1509,4 +1511,18 @@ defmodule Towerops.Snmp.Discovery do List.last(group) end) end + + defp safe_discover(discovery_fn, resource_type, device_id) do + case discovery_fn.() do + {:ok, result} -> + result + + {:error, :timeout} -> + [] + + {:error, reason} -> + Logger.warning("#{resource_type} discovery failed: #{inspect(reason)}", device_id: device_id) + [] + end + end end diff --git a/lib/towerops_web/live/alert_live/index.ex b/lib/towerops_web/live/alert_live/index.ex index 3df8c9e5..43b03888 100644 --- a/lib/towerops_web/live/alert_live/index.ex +++ b/lib/towerops_web/live/alert_live/index.ex @@ -191,6 +191,8 @@ defmodule ToweropsWeb.AlertLive.Index do {:noreply, load_alerts(socket, socket.assigns.current_scope.organization.id)} end + def handle_info(_msg, socket), do: {:noreply, socket} + defp load_alerts(socket, organization_id) do all_alerts = Alerts.list_organization_alerts(organization_id, %{"limit" => 500}) @@ -294,11 +296,13 @@ defmodule ToweropsWeb.AlertLive.Index do end defp load_site_subscribers(alerts) do - alerts - |> Enum.map(& &1.device.site_id) - |> Enum.uniq() - |> Enum.reject(&is_nil/1) - |> Map.new(fn site_id -> {site_id, Gaiia.get_site_subscriber_summary(site_id)} end) + site_ids = + alerts + |> Enum.map(& &1.device.site_id) + |> Enum.uniq() + |> Enum.reject(&is_nil/1) + + Gaiia.get_site_subscriber_summaries(site_ids) end @doc false diff --git a/lib/towerops_web/live/device_live/index.ex b/lib/towerops_web/live/device_live/index.ex index 3c589639..566ba49f 100644 --- a/lib/towerops_web/live/device_live/index.ex +++ b/lib/towerops_web/live/device_live/index.ex @@ -278,9 +278,19 @@ defmodule ToweropsWeb.DeviceLive.Index do @impl true def handle_info(:debounced_device_reload, socket) do - {:noreply, reload_device_stream(socket)} + organization = socket.assigns.current_scope.organization + devices = Devices.list_organization_devices(organization.id) + + socket = + socket + |> assign(:pending_reload_ref, nil) + |> stream(:device_rows, build_device_rows(devices), reset: true) + + {:noreply, socket} end + def handle_info(_msg, socket), do: {:noreply, socket} + defp tick_interval_ms do if Application.get_env(:towerops, :env) == :test, do: :infinity, else: to_timeout(second: 15) end @@ -464,11 +474,13 @@ defmodule ToweropsWeb.DeviceLive.Index do defp device_type_label(_), do: "Unknown" defp load_site_subscribers(devices) do - devices - |> Enum.map(& &1.site_id) - |> Enum.uniq() - |> Enum.reject(&is_nil/1) - |> Map.new(fn site_id -> {site_id, Gaiia.get_site_subscriber_summary(site_id)} end) + site_ids = + devices + |> Enum.map(& &1.site_id) + |> Enum.uniq() + |> Enum.reject(&is_nil/1) + + Gaiia.get_site_subscriber_summaries(site_ids) end defp device_row_title(device) do diff --git a/lib/towerops_web/live/device_live/show.ex b/lib/towerops_web/live/device_live/show.ex index 4c5a242f..7c454255 100644 --- a/lib/towerops_web/live/device_live/show.ex +++ b/lib/towerops_web/live/device_live/show.ex @@ -268,6 +268,8 @@ defmodule ToweropsWeb.DeviceLive.Show do |> assign(:edit_check, nil)} end + def handle_info(_msg, socket), do: {:noreply, socket} + # Private functions defp maybe_subscribe_and_schedule_refresh(socket, device_id) do diff --git a/lib/towerops_web/live/device_live/show.html.heex b/lib/towerops_web/live/device_live/show.html.heex index 2f599da0..c89b225b 100644 --- a/lib/towerops_web/live/device_live/show.html.heex +++ b/lib/towerops_web/live/device_live/show.html.heex @@ -636,6 +636,7 @@
@@ -890,6 +894,7 @@