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: graham/towerops-web#132

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 <html> (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<HTMLElement>("[data-phx-theme]").forEach((btn) => {
       const isActive = btn.dataset.phxTheme === current

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 """

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

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

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

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

lib/towerops_web/live/device_live/show.html.heex

@@ -636,6 +636,7 @@
                   <div
                     id="traffic-chart"
                     phx-hook="SensorChart"
+                    phx-update="ignore"
                     data-chart={@traffic_chart_data}
                     data-unit="bps"
                     data-auto-scale="true"
@@ -673,6 +674,7 @@
                   <div
                     id="latency-chart"
                     phx-hook="SensorChart"
+                    phx-update="ignore"
                     data-chart={@latency_chart_data}
                     data-unit="ms"
                     data-auto-scale="true"
@@ -768,6 +770,7 @@
                     <div
                       id="processor-chart"
                       phx-hook="SensorChart"
+                      phx-update="ignore"
                       data-chart={@processor_chart_data}
                       data-unit="%"
                       style="height: 200px;"
@@ -826,6 +829,7 @@
                   <div
                     id="memory-chart"
                     phx-hook="SensorChart"
+                    phx-update="ignore"
                     data-chart={@memory_chart_data}
                     style="height: 300px;"
                   >
@@ -890,6 +894,7 @@
                     <div
                       id="storage-volume-chart"
                       phx-hook="SensorChart"
+                      phx-update="ignore"
                       data-chart={@storage_volume_chart_data}
                       data-unit="%"
                       style="height: 200px;"

test/towerops_web/live/agent_live/helpers_test.exs

@@ -18,7 +18,7 @@ defmodule ToweropsWeb.AgentLive.HelpersTest do
     end
 
     test "returns :online at the boundary just under 120 seconds" do
-      last_seen = DateTime.add(DateTime.utc_now(), -119, :second)
+      last_seen = DateTime.add(DateTime.utc_now(), -118, :second)
       agent_token = %{last_seen_at: last_seen}
 
       assert {:online, "Online"} = Helpers.agent_status(agent_token)