add tests and add snmp neighbor discovery

lib/towerops/monitoring/supervisor.ex

@@ -5,6 +5,7 @@ defmodule Towerops.Monitoring.Supervisor do
   use Supervisor
 
   alias Towerops.Monitoring.EquipmentMonitor
+  alias Towerops.Snmp.NeighborCleanupWorker
   alias Towerops.Snmp.PollerRegistry
   alias Towerops.Snmp.PollerSupervisor
   alias Towerops.Snmp.PollerWorker
@@ -17,7 +18,7 @@ defmodule Towerops.Monitoring.Supervisor do
 
   @impl true
   def init(_init_arg) do
-    children = [
+    base_children = [
       # Registry for naming monitor processes
       {Registry, keys: :unique, name: Towerops.Monitoring.Registry},
       # Registry for naming SNMP poller processes
@@ -28,6 +29,14 @@ defmodule Towerops.Monitoring.Supervisor do
       {DynamicSupervisor, name: PollerSupervisor, strategy: :one_for_one}
     ]
 
+    # Only start cleanup worker in non-test environments
+    children =
+      if test_mode?() do
+        base_children
+      else
+        base_children ++ [NeighborCleanupWorker]
+      end
+
     # Start all monitors after supervisor is initialized (but not in test mode)
     # In test mode, the Repo uses Sandbox pool which we can detect
     _ =

lib/towerops/snmp/discovery.ex

@@ -260,10 +260,11 @@ defmodule Towerops.Snmp.Discovery do
 
       # Delete old interfaces/sensors and insert new ones
       delete_old_data(device)
-      insert_interfaces(device, interfaces)
+      new_interfaces = insert_interfaces(device, interfaces)
       insert_sensors(device, sensors)
 
-      device
+      # Return device with interfaces loaded and equipment_id added
+      %{device | interfaces: Enum.map(new_interfaces, &Map.put(&1, :equipment_id, equipment.id))}
     end)
   end
 

lib/towerops/snmp/neighbor_cleanup_worker.ex

@@ -0,0 +1,72 @@
+defmodule Towerops.Snmp.NeighborCleanupWorker do
+  @moduledoc """
+  GenServer that periodically cleans up stale neighbor discovery records.
+
+  Neighbors that haven't been discovered in the last 24 hours are considered stale
+  and will be automatically removed from the database.
+  """
+  use GenServer
+
+  alias Towerops.Equipment
+  alias Towerops.Snmp
+
+  require Logger
+
+  # Check for stale neighbors every hour
+  @cleanup_interval to_timeout(hour: 1)
+
+  # Consider neighbors stale if not seen in 24 hours
+  @stale_threshold_hours 24
+
+  def start_link(opts \\ []) do
+    GenServer.start_link(__MODULE__, opts, name: __MODULE__)
+  end
+
+  @impl true
+  def init(_opts) do
+    # Schedule first cleanup shortly after startup
+    schedule_cleanup(10_000)
+    {:ok, %{}}
+  end
+
+  @impl true
+  def handle_info(:cleanup, state) do
+    cleanup_stale_neighbors()
+    schedule_cleanup(@cleanup_interval)
+    {:noreply, state}
+  end
+
+  defp cleanup_stale_neighbors do
+    cutoff = DateTime.add(DateTime.utc_now(), -@stale_threshold_hours, :hour)
+
+    Logger.info("Starting neighbor cleanup for records older than #{cutoff}")
+
+    # Get all equipment IDs with SNMP enabled
+    equipment_list = Equipment.list_snmp_enabled_equipment()
+
+    total_deleted =
+      Enum.reduce(equipment_list, 0, fn equipment, acc ->
+        case Snmp.delete_stale_neighbors(equipment.id, cutoff) do
+          {count, _} when count > 0 ->
+            Logger.info("Deleted #{count} stale neighbors for equipment #{equipment.name} (#{equipment.id})")
+
+            acc + count
+
+          _ ->
+            acc
+        end
+      end)
+
+    if total_deleted > 0 do
+      Logger.info("Neighbor cleanup completed: removed #{total_deleted} stale records")
+    else
+      Logger.debug("Neighbor cleanup completed: no stale records found")
+    end
+
+    :ok
+  end
+
+  defp schedule_cleanup(delay) do
+    Process.send_after(self(), :cleanup, delay)
+  end
+end

lib/towerops/snmp/neighbor_discovery.ex

@@ -48,7 +48,7 @@ defmodule Towerops.Snmp.NeighborDiscovery do
       {:ok, entries} when entries != [] ->
         parse_lldp_neighbors(entries, interfaces)
 
-      {:ok, []} ->
+      {:ok, entries} when entries == %{} ->
         Logger.debug("No LLDP neighbors found")
         []
 
@@ -59,10 +59,13 @@ defmodule Towerops.Snmp.NeighborDiscovery do
   end
 
   defp parse_lldp_neighbors(entries, interfaces) do
+    # Convert map from Client.walk to list of maps
+    entry_list = Enum.map(entries, fn {oid, value} -> %{oid: oid, value: value} end)
+
     # Group by local port index (time, local_port_num, rem_index)
     # OIDs are in format: 1.0.8802.1.1.2.1.4.1.1.X.time.local_port.rem_index
     grouped =
-      Enum.group_by(entries, fn %{oid: oid} ->
+      Enum.group_by(entry_list, fn %{oid: oid} ->
         # Extract local port num (second to last index) and remote index (last index)
         parts = String.split(oid, ".")
         [rem_index, local_port, _time | _] = Enum.reverse(parts)
@@ -80,7 +83,9 @@ defmodule Towerops.Snmp.NeighborDiscovery do
     neighbor_map =
       Enum.reduce(entries, %{}, fn %{oid: oid, value: value}, acc ->
         parts = String.split(oid, ".")
-        field_oid = Enum.at(parts, 11)
+        field_oid = Enum.at(parts, 10)
+        # Always extract local_port from OID, then parse the field
+        acc = extract_local_port(acc, oid)
         parse_lldp_field(field_oid, value, oid, acc)
       end)
 
@@ -99,7 +104,7 @@ defmodule Towerops.Snmp.NeighborDiscovery do
 
   defp parse_lldp_field("12", value, _oid, acc), do: Map.put(acc, :remote_capabilities, parse_lldp_capabilities(value))
 
-  defp parse_lldp_field(_field_oid, _value, oid, acc), do: extract_local_port(acc, oid)
+  defp parse_lldp_field(_field_oid, _value, _oid, acc), do: acc
 
   defp extract_local_port(acc, oid) do
     parts = String.split(oid, ".")
@@ -141,7 +146,7 @@ defmodule Towerops.Snmp.NeighborDiscovery do
       {:ok, entries} when entries != [] ->
         parse_cdp_neighbors(entries, interfaces)
 
-      {:ok, []} ->
+      {:ok, entries} when entries == %{} ->
         Logger.debug("No CDP neighbors found")
         []
 
@@ -152,10 +157,13 @@ defmodule Towerops.Snmp.NeighborDiscovery do
   end
 
   defp parse_cdp_neighbors(entries, interfaces) do
+    # Convert map from Client.walk to list of maps
+    entry_list = Enum.map(entries, fn {oid, value} -> %{oid: oid, value: value} end)
+
     # Group by interface index and device index
     # OIDs are in format: 1.3.6.1.4.1.9.9.23.1.2.1.1.X.if_index.device_index
     grouped =
-      Enum.group_by(entries, fn %{oid: oid} ->
+      Enum.group_by(entry_list, fn %{oid: oid} ->
         parts = String.split(oid, ".")
         # Get last two parts (interface index and device index)
         [device_idx | rest] = Enum.reverse(parts)
@@ -174,7 +182,9 @@ defmodule Towerops.Snmp.NeighborDiscovery do
     neighbor_map =
       Enum.reduce(entries, %{}, fn %{oid: oid, value: value}, acc ->
         parts = String.split(oid, ".")
-        field_oid = Enum.at(parts, 12)
+        field_oid = Enum.at(parts, 13)
+        # Always extract local_if_index from OID, then parse the field
+        acc = extract_cdp_local_interface(acc, oid)
         parse_cdp_field(field_oid, value, oid, acc)
       end)
 
@@ -191,7 +201,7 @@ defmodule Towerops.Snmp.NeighborDiscovery do
 
   defp parse_cdp_field("9", value, _oid, acc), do: Map.put(acc, :remote_capabilities, parse_cdp_capabilities(value))
 
-  defp parse_cdp_field(_field_oid, _value, oid, acc), do: extract_cdp_local_interface(acc, oid)
+  defp parse_cdp_field(_field_oid, _value, _oid, acc), do: acc
 
   defp extract_cdp_local_interface(acc, oid) do
     parts = String.split(oid, ".")
@@ -251,9 +261,7 @@ defmodule Towerops.Snmp.NeighborDiscovery do
   @lldp_cap_docsis 0x40
   @lldp_cap_station 0x80
 
-  defp parse_lldp_capabilities(value) when is_binary(value) do
-    <<caps::integer-size(16)>> = value
-
+  defp parse_lldp_capabilities(<<caps::integer-size(16)>>) do
     []
     |> maybe_add_cap(caps, @lldp_cap_other, "other")
     |> maybe_add_cap(caps, @lldp_cap_repeater, "repeater")

priv/repo/migrations/20260116185112_create_snmp_neighbors.exs

@@ -2,7 +2,9 @@ defmodule Towerops.Repo.Migrations.CreateSnmpNeighbors do
   use Ecto.Migration
 
   def change do
-    create table(:snmp_neighbors) do
+    create table(:snmp_neighbors, primary_key: false) do
+      add :id, :binary_id, primary_key: true
+
       add :equipment_id, references(:equipment, type: :binary_id, on_delete: :delete_all),
         null: false
 

test/towerops/snmp/discovery_test.exs

@@ -207,8 +207,8 @@ defmodule Towerops.Snmp.DiscoveryTest do
       end)
 
       # Mock interface discovery - no interfaces for simplicity
-      # Walk called 3 times: interfaces (1), cisco sensors (1), base sensors fallback (1)
-      expect(SnmpMock, :walk, 3, fn _, _, _ ->
+      # Walk called 5 times: interfaces (1), cisco sensors (1), base sensors fallback (1), LLDP neighbors (1), CDP neighbors (1)
+      expect(SnmpMock, :walk, 5, fn _, _, _ ->
         {:ok, []}
       end)
 
@@ -272,12 +272,22 @@ defmodule Towerops.Snmp.DiscoveryTest do
         {:error, :timeout}
       end)
 
-      # Mock failed sensor discovery
-      expect(SnmpMock, :walk, fn _, oid, _ ->
-        if String.starts_with?(oid, "1.3.6.1.2.1.99") do
-          {:error, :timeout}
-        else
-          {:error, :no_such_object}
+      # Mock failed sensor discovery and neighbor discovery (LLDP + CDP)
+      expect(SnmpMock, :walk, 3, fn _, oid, _ ->
+        cond do
+          String.starts_with?(oid, "1.3.6.1.2.1.99") ->
+            {:error, :timeout}
+
+          String.starts_with?(oid, "1.0.8802") ->
+            # LLDP - return empty
+            {:ok, []}
+
+          String.starts_with?(oid, "1.3.6.1.4.1.9.9.23") ->
+            # CDP - return empty
+            {:ok, []}
+
+          true ->
+            {:error, :no_such_object}
         end
       end)
 

test/towerops/snmp/neighbor_cleanup_worker_test.exs

@@ -0,0 +1,301 @@
+defmodule Towerops.Snmp.NeighborCleanupWorkerTest do
+  use Towerops.DataCase, async: false
+
+  import Towerops.AccountsFixtures
+
+  alias Towerops.Snmp
+  alias Towerops.Snmp.Device
+  alias Towerops.Snmp.Interface
+  alias Towerops.Snmp.Neighbor
+  alias Towerops.Snmp.NeighborCleanupWorker
+
+  setup do
+    user = user_fixture()
+    {:ok, organization} = Towerops.Organizations.create_organization(%{name: "Test Org"}, user.id)
+
+    {:ok, site} =
+      Towerops.Sites.create_site(%{
+        name: "Test Site",
+        organization_id: organization.id
+      })
+
+    {:ok, equipment1} =
+      Towerops.Equipment.create_equipment(%{
+        name: "Test Switch 1",
+        ip_address: "192.168.1.1",
+        snmp_enabled: true,
+        snmp_version: "2c",
+        snmp_community: "public",
+        snmp_port: 161,
+        site_id: site.id
+      })
+
+    {:ok, equipment2} =
+      Towerops.Equipment.create_equipment(%{
+        name: "Test Switch 2",
+        ip_address: "192.168.1.2",
+        snmp_enabled: true,
+        snmp_version: "2c",
+        snmp_community: "public",
+        snmp_port: 161,
+        site_id: site.id
+      })
+
+    device1 =
+      %Device{}
+      |> Device.changeset(%{
+        equipment_id: equipment1.id,
+        sys_name: "test-switch-1",
+        sys_descr: "Test Switch 1"
+      })
+      |> Repo.insert!()
+
+    device2 =
+      %Device{}
+      |> Device.changeset(%{
+        equipment_id: equipment2.id,
+        sys_name: "test-switch-2",
+        sys_descr: "Test Switch 2"
+      })
+      |> Repo.insert!()
+
+    interface1 =
+      %Interface{}
+      |> Interface.changeset(%{
+        snmp_device_id: device1.id,
+        if_index: 1,
+        if_descr: "GigabitEthernet0/1",
+        if_name: "Gi0/1",
+        if_type: 6,
+        if_oper_status: "up"
+      })
+      |> Repo.insert!()
+
+    interface2 =
+      %Interface{}
+      |> Interface.changeset(%{
+        snmp_device_id: device2.id,
+        if_index: 1,
+        if_descr: "GigabitEthernet0/1",
+        if_name: "Gi0/1",
+        if_type: 6,
+        if_oper_status: "up"
+      })
+      |> Repo.insert!()
+
+    %{
+      equipment1: equipment1,
+      equipment2: equipment2,
+      interface1: interface1,
+      interface2: interface2
+    }
+  end
+
+  describe "cleanup process" do
+    test "worker starts successfully" do
+      {:ok, pid} = NeighborCleanupWorker.start_link()
+      assert Process.alive?(pid)
+      GenServer.stop(pid)
+    end
+
+    test "cleanup removes stale neighbors across all equipment", %{
+      equipment1: equipment1,
+      equipment2: equipment2,
+      interface1: interface1,
+      interface2: interface2
+    } do
+      # Create old neighbors on both equipment
+      two_days_ago = DateTime.add(DateTime.utc_now(), -48, :hour)
+      one_hour_ago = DateTime.add(DateTime.utc_now(), -1, :hour)
+
+      # Old neighbor on equipment1
+      {:ok, old_neighbor1} =
+        Snmp.upsert_neighbor(%{
+          equipment_id: equipment1.id,
+          interface_id: interface1.id,
+          protocol: "lldp",
+          remote_chassis_id: "old-neighbor-1",
+          remote_system_name: "old-neighbor-1",
+          last_discovered_at: two_days_ago
+        })
+
+      # Old neighbor on equipment2
+      {:ok, old_neighbor2} =
+        Snmp.upsert_neighbor(%{
+          equipment_id: equipment2.id,
+          interface_id: interface2.id,
+          protocol: "cdp",
+          remote_chassis_id: "old-neighbor-2",
+          remote_system_name: "old-neighbor-2",
+          last_discovered_at: two_days_ago
+        })
+
+      # Recent neighbor on equipment1
+      {:ok, recent_neighbor} =
+        Snmp.upsert_neighbor(%{
+          equipment_id: equipment1.id,
+          interface_id: interface1.id,
+          protocol: "cdp",
+          remote_chassis_id: "recent-neighbor",
+          remote_system_name: "recent-neighbor",
+          last_discovered_at: one_hour_ago
+        })
+
+      # Start worker and trigger cleanup immediately
+      {:ok, pid} = NeighborCleanupWorker.start_link()
+      send(pid, :cleanup)
+
+      # Wait for cleanup to complete
+      Process.sleep(100)
+
+      # Old neighbors should be deleted
+      assert Repo.get(Neighbor, old_neighbor1.id) == nil
+      assert Repo.get(Neighbor, old_neighbor2.id) == nil
+
+      # Recent neighbor should still exist
+      assert Repo.get(Neighbor, recent_neighbor.id)
+
+      GenServer.stop(pid)
+    end
+
+    test "cleanup handles equipment with no neighbors gracefully", %{equipment1: equipment1} do
+      # Ensure no neighbors exist
+      assert Snmp.list_neighbors(equipment1.id) == []
+
+      {:ok, pid} = NeighborCleanupWorker.start_link()
+      send(pid, :cleanup)
+
+      # Should not crash
+      Process.sleep(100)
+      assert Process.alive?(pid)
+
+      GenServer.stop(pid)
+    end
+
+    test "cleanup preserves neighbors within 24 hour threshold", %{
+      equipment1: equipment1,
+      interface1: interface1
+    } do
+      # Create neighbors at various ages
+      now = DateTime.utc_now()
+      twenty_three_hours_ago = DateTime.add(now, -23, :hour)
+      twenty_five_hours_ago = DateTime.add(now, -25, :hour)
+
+      # Neighbor just within threshold
+      {:ok, recent_neighbor} =
+        Snmp.upsert_neighbor(%{
+          equipment_id: equipment1.id,
+          interface_id: interface1.id,
+          protocol: "lldp",
+          remote_chassis_id: "recent",
+          remote_system_name: "recent",
+          last_discovered_at: twenty_three_hours_ago
+        })
+
+      # Neighbor just outside threshold
+      {:ok, stale_neighbor} =
+        Snmp.upsert_neighbor(%{
+          equipment_id: equipment1.id,
+          interface_id: interface1.id,
+          protocol: "cdp",
+          remote_chassis_id: "stale",
+          remote_system_name: "stale",
+          last_discovered_at: twenty_five_hours_ago
+        })
+
+      {:ok, pid} = NeighborCleanupWorker.start_link()
+      send(pid, :cleanup)
+
+      Process.sleep(100)
+
+      # Recent neighbor should still exist
+      assert Repo.get(Neighbor, recent_neighbor.id)
+
+      # Stale neighbor should be deleted
+      assert Repo.get(Neighbor, stale_neighbor.id) == nil
+
+      GenServer.stop(pid)
+    end
+
+    test "cleanup does not affect equipment without SNMP enabled" do
+      user = user_fixture()
+      {:ok, org} = Towerops.Organizations.create_organization(%{name: "Test Org 2"}, user.id)
+
+      {:ok, site} =
+        Towerops.Sites.create_site(%{
+          name: "Test Site 2",
+          organization_id: org.id
+        })
+
+      # Create equipment without SNMP enabled
+      {:ok, equipment} =
+        Towerops.Equipment.create_equipment(%{
+          name: "No SNMP Equipment",
+          ip_address: "192.168.1.100",
+          snmp_enabled: false,
+          site_id: site.id
+        })
+
+      device =
+        %Device{}
+        |> Device.changeset(%{
+          equipment_id: equipment.id,
+          sys_name: "no-snmp-device",
+          sys_descr: "No SNMP Device"
+        })
+        |> Repo.insert!()
+
+      interface =
+        %Interface{}
+        |> Interface.changeset(%{
+          snmp_device_id: device.id,
+          if_index: 1,
+          if_descr: "eth0",
+          if_type: 6
+        })
+        |> Repo.insert!()
+
+      # Create old neighbor
+      two_days_ago = DateTime.add(DateTime.utc_now(), -48, :hour)
+
+      {:ok, neighbor} =
+        Snmp.upsert_neighbor(%{
+          equipment_id: equipment.id,
+          interface_id: interface.id,
+          protocol: "lldp",
+          remote_chassis_id: "neighbor",
+          remote_system_name: "neighbor",
+          last_discovered_at: two_days_ago
+        })
+
+      {:ok, pid} = NeighborCleanupWorker.start_link()
+      send(pid, :cleanup)
+
+      Process.sleep(100)
+
+      # Neighbor should still exist (equipment not included in cleanup)
+      # because cleanup only processes SNMP-enabled equipment
+      assert Repo.get(Neighbor, neighbor.id)
+
+      GenServer.stop(pid)
+    end
+  end
+
+  describe "scheduled cleanup" do
+    test "worker schedules periodic cleanup" do
+      {:ok, pid} = NeighborCleanupWorker.start_link()
+
+      # Check that worker is alive and has scheduled a cleanup message
+      assert Process.alive?(pid)
+
+      # Get process info
+      {:messages, _messages} = Process.info(pid, :messages)
+
+      # Should eventually have a :cleanup message scheduled
+      # (may take a moment for the initial schedule)
+      Process.sleep(20)
+
+      GenServer.stop(pid)
+    end
+  end
+end

test/towerops/snmp/neighbor_discovery_test.exs

@@ -0,0 +1,544 @@
+defmodule Towerops.Snmp.NeighborDiscoveryTest do
+  use Towerops.DataCase
+
+  import Mox
+  import Towerops.AccountsFixtures
+
+  alias Towerops.Snmp
+  alias Towerops.Snmp.Device
+  alias Towerops.Snmp.Interface
+  alias Towerops.Snmp.Neighbor
+  alias Towerops.Snmp.NeighborDiscovery
+  alias Towerops.Snmp.SnmpMock
+
+  setup :verify_on_exit!
+
+  setup do
+    user = user_fixture()
+    {:ok, organization} = Towerops.Organizations.create_organization(%{name: "Test Org"}, user.id)
+
+    {:ok, site} =
+      Towerops.Sites.create_site(%{
+        name: "Test Site",
+        organization_id: organization.id
+      })
+
+    {:ok, equipment} =
+      Towerops.Equipment.create_equipment(%{
+        name: "Test Switch",
+        ip_address: "192.168.1.1",
+        snmp_enabled: true,
+        snmp_version: "2c",
+        snmp_community: "public",
+        snmp_port: 161,
+        site_id: site.id
+      })
+
+    device =
+      %Device{}
+      |> Device.changeset(%{
+        equipment_id: equipment.id,
+        sys_name: "test-switch",
+        sys_descr: "Test Switch"
+      })
+      |> Repo.insert!()
+
+    interface1 =
+      %Interface{}
+      |> Interface.changeset(%{
+        snmp_device_id: device.id,
+        if_index: 1,
+        if_descr: "GigabitEthernet0/1",
+        if_name: "Gi0/1",
+        if_type: 6,
+        if_oper_status: "up"
+      })
+      |> Repo.insert!()
+
+    interface2 =
+      %Interface{}
+      |> Interface.changeset(%{
+        snmp_device_id: device.id,
+        if_index: 2,
+        if_descr: "GigabitEthernet0/2",
+        if_name: "Gi0/2",
+        if_type: 6,
+        if_oper_status: "up"
+      })
+      |> Repo.insert!()
+
+    %{
+      equipment: equipment,
+      device: device,
+      interface1: interface1,
+      interface2: interface2,
+      organization: organization
+    }
+  end
+
+  describe "discover_neighbors/2" do
+    test "discovers LLDP neighbors", %{equipment: equipment, interface1: interface1} do
+      # Mock LLDP walk response
+      expect(SnmpMock, :walk, fn _, _, _ ->
+        {:ok,
+         [
+           # LLDP remote chassis ID (field 4) for timemark.1.1
+           %{oid: "1.0.8802.1.1.2.1.4.1.1.4.0.1.1", value: <<0xAA, 0xBB, 0xCC, 0xDD, 0xEE, 0xFF>>},
+           # LLDP remote port ID (field 5)
+           %{oid: "1.0.8802.1.1.2.1.4.1.1.5.0.1.1", value: "Gi0/1"},
+           # LLDP remote port description (field 6)
+           %{oid: "1.0.8802.1.1.2.1.4.1.1.6.0.1.1", value: "GigabitEthernet0/1"},
+           # LLDP remote system name (field 7)
+           %{oid: "1.0.8802.1.1.2.1.4.1.1.7.0.1.1", value: "neighbor-switch"},
+           # LLDP remote system description (field 8)
+           %{oid: "1.0.8802.1.1.2.1.4.1.1.8.0.1.1", value: "Cisco IOS Software"}
+         ]}
+      end)
+
+      # Mock CDP walk response (empty)
+      expect(SnmpMock, :walk, fn _, _, _ ->
+        {:ok, []}
+      end)
+
+      client_opts = [
+        ip: equipment.ip_address,
+        community: equipment.snmp_community,
+        version: equipment.snmp_version,
+        port: equipment.snmp_port
+      ]
+
+      interfaces = [Map.put(interface1, :equipment_id, equipment.id)]
+
+      {:ok, neighbors} = NeighborDiscovery.discover_neighbors(client_opts, interfaces)
+
+      assert length(neighbors) == 1
+      neighbor = List.first(neighbors)
+
+      assert neighbor.protocol == "lldp"
+      assert neighbor.interface_id == interface1.id
+      assert neighbor.equipment_id == equipment.id
+      assert neighbor.remote_system_name == "neighbor-switch"
+      assert neighbor.remote_port_id == "Gi0/1"
+      assert neighbor.remote_port_description == "GigabitEthernet0/1"
+      assert neighbor.remote_system_description == "Cisco IOS Software"
+    end
+
+    test "discovers CDP neighbors", %{equipment: equipment, interface2: interface2} do
+      # Mock LLDP walk response (empty)
+      expect(SnmpMock, :walk, fn _, _, _ ->
+        {:ok, []}
+      end)
+
+      # Mock CDP walk response
+      expect(SnmpMock, :walk, fn _, _, _ ->
+        {:ok,
+         [
+           # CDP remote address (field 4) for if_index.2.device_index.1
+           %{oid: "1.3.6.1.4.1.9.9.23.1.2.1.1.4.2.1", value: <<192, 168, 1, 10>>},
+           # CDP remote device ID (field 6)
+           %{oid: "1.3.6.1.4.1.9.9.23.1.2.1.1.6.2.1", value: "neighbor-router.example.com"},
+           # CDP remote port (field 7)
+           %{oid: "1.3.6.1.4.1.9.9.23.1.2.1.1.7.2.1", value: "FastEthernet0/1"},
+           # CDP remote platform (field 8)
+           %{oid: "1.3.6.1.4.1.9.9.23.1.2.1.1.8.2.1", value: "cisco ISR4331/K9"}
+         ]}
+      end)
+
+      client_opts = [
+        ip: equipment.ip_address,
+        community: equipment.snmp_community,
+        version: equipment.snmp_version,
+        port: equipment.snmp_port
+      ]
+
+      interfaces = [Map.put(interface2, :equipment_id, equipment.id)]
+
+      {:ok, neighbors} = NeighborDiscovery.discover_neighbors(client_opts, interfaces)
+
+      assert length(neighbors) == 1
+      neighbor = List.first(neighbors)
+
+      assert neighbor.protocol == "cdp"
+      assert neighbor.interface_id == interface2.id
+      assert neighbor.equipment_id == equipment.id
+      assert neighbor.remote_system_name == "neighbor-router.example.com"
+      assert neighbor.remote_port_id == "FastEthernet0/1"
+      assert neighbor.remote_platform == "cisco ISR4331/K9"
+      assert neighbor.remote_address == "192.168.1.10"
+    end
+
+    test "discovers both LLDP and CDP neighbors", %{
+      equipment: equipment,
+      interface1: interface1,
+      interface2: interface2
+    } do
+      # Mock LLDP walk response
+      expect(SnmpMock, :walk, fn _, _, _ ->
+        {:ok,
+         [
+           %{oid: "1.0.8802.1.1.2.1.4.1.1.4.0.1.1", value: <<0xAA, 0xBB, 0xCC, 0xDD, 0xEE, 0xFF>>},
+           %{oid: "1.0.8802.1.1.2.1.4.1.1.7.0.1.1", value: "lldp-neighbor"}
+         ]}
+      end)
+
+      # Mock CDP walk response
+      expect(SnmpMock, :walk, fn _, _, _ ->
+        {:ok,
+         [
+           %{oid: "1.3.6.1.4.1.9.9.23.1.2.1.1.6.2.1", value: "cdp-neighbor"},
+           %{oid: "1.3.6.1.4.1.9.9.23.1.2.1.1.7.2.1", value: "Gi0/1"}
+         ]}
+      end)
+
+      client_opts = [
+        ip: equipment.ip_address,
+        community: equipment.snmp_community,
+        version: equipment.snmp_version,
+        port: equipment.snmp_port
+      ]
+
+      interfaces = [
+        Map.put(interface1, :equipment_id, equipment.id),
+        Map.put(interface2, :equipment_id, equipment.id)
+      ]
+
+      {:ok, neighbors} = NeighborDiscovery.discover_neighbors(client_opts, interfaces)
+
+      assert length(neighbors) == 2
+      protocols = Enum.map(neighbors, & &1.protocol)
+      assert "lldp" in protocols
+      assert "cdp" in protocols
+    end
+
+    test "returns empty list when no neighbors found", %{equipment: equipment} do
+      # Mock LLDP walk response (empty)
+      expect(SnmpMock, :walk, fn _, _, _ ->
+        {:ok, []}
+      end)
+
+      # Mock CDP walk response (empty)
+      expect(SnmpMock, :walk, fn _, _, _ ->
+        {:ok, []}
+      end)
+
+      client_opts = [
+        ip: equipment.ip_address,
+        community: equipment.snmp_community,
+        version: equipment.snmp_version,
+        port: equipment.snmp_port
+      ]
+
+      {:ok, neighbors} = NeighborDiscovery.discover_neighbors(client_opts, [])
+
+      assert neighbors == []
+    end
+
+    test "handles SNMP errors gracefully", %{equipment: equipment, interface1: interface1} do
+      # Mock LLDP walk error
+      expect(SnmpMock, :walk, fn _, _, _ ->
+        {:error, :timeout}
+      end)
+
+      # Mock CDP walk error
+      expect(SnmpMock, :walk, fn _, _, _ ->
+        {:error, :timeout}
+      end)
+
+      client_opts = [
+        ip: equipment.ip_address,
+        community: equipment.snmp_community,
+        version: equipment.snmp_version,
+        port: equipment.snmp_port
+      ]
+
+      interfaces = [Map.put(interface1, :equipment_id, equipment.id)]
+
+      {:ok, neighbors} = NeighborDiscovery.discover_neighbors(client_opts, interfaces)
+
+      # Should return empty list on error
+      assert neighbors == []
+    end
+  end
+
+  describe "upsert_neighbor/1" do
+    test "creates a new neighbor record", %{equipment: equipment, interface1: interface1} do
+      attrs = %{
+        equipment_id: equipment.id,
+        interface_id: interface1.id,
+        protocol: "lldp",
+        remote_chassis_id: "aa:bb:cc:dd:ee:ff",
+        remote_system_name: "test-neighbor",
+        remote_system_description: "Test Device",
+        remote_platform: "Test Platform",
+        remote_port_id: "Gi0/1",
+        remote_port_description: "GigabitEthernet0/1",
+        remote_address: "192.168.1.100",
+        remote_capabilities: ["router", "bridge"],
+        last_discovered_at: DateTime.utc_now()
+      }
+
+      assert {:ok, neighbor} = Snmp.upsert_neighbor(attrs)
+      assert neighbor.protocol == "lldp"
+      assert neighbor.remote_system_name == "test-neighbor"
+      assert neighbor.remote_chassis_id == "aa:bb:cc:dd:ee:ff"
+      assert neighbor.remote_capabilities == ["router", "bridge"]
+    end
+
+    test "updates existing neighbor record", %{equipment: equipment, interface1: interface1} do
+      attrs = %{
+        equipment_id: equipment.id,
+        interface_id: interface1.id,
+        protocol: "cdp",
+        remote_chassis_id: "neighbor-switch",
+        remote_system_name: "old-name",
+        remote_platform: "Old Platform",
+        last_discovered_at: DateTime.utc_now()
+      }
+
+      {:ok, original} = Snmp.upsert_neighbor(attrs)
+
+      # Update with new data
+      updated_attrs = %{
+        equipment_id: equipment.id,
+        interface_id: interface1.id,
+        protocol: "cdp",
+        remote_chassis_id: "neighbor-switch",
+        remote_system_name: "new-name",
+        remote_platform: "New Platform",
+        last_discovered_at: DateTime.utc_now()
+      }
+
+      {:ok, updated} = Snmp.upsert_neighbor(updated_attrs)
+
+      # Should be the same record, just updated
+      assert updated.id == original.id
+      assert updated.remote_system_name == "new-name"
+      assert updated.remote_platform == "New Platform"
+    end
+
+    test "creates separate records for different protocols", %{
+      equipment: equipment,
+      interface1: interface1
+    } do
+      # Create LLDP neighbor
+      lldp_attrs = %{
+        equipment_id: equipment.id,
+        interface_id: interface1.id,
+        protocol: "lldp",
+        remote_chassis_id: "same-chassis-id",
+        remote_system_name: "neighbor-switch",
+        last_discovered_at: DateTime.utc_now()
+      }
+
+      # Create CDP neighbor with same chassis ID
+      cdp_attrs = %{
+        equipment_id: equipment.id,
+        interface_id: interface1.id,
+        protocol: "cdp",
+        remote_chassis_id: "same-chassis-id",
+        remote_system_name: "neighbor-switch",
+        last_discovered_at: DateTime.utc_now()
+      }
+
+      {:ok, lldp_neighbor} = Snmp.upsert_neighbor(lldp_attrs)
+      {:ok, cdp_neighbor} = Snmp.upsert_neighbor(cdp_attrs)
+
+      # Should be different records
+      assert lldp_neighbor.id != cdp_neighbor.id
+      assert lldp_neighbor.protocol == "lldp"
+      assert cdp_neighbor.protocol == "cdp"
+    end
+  end
+
+  describe "list_neighbors/1" do
+    test "returns all neighbors for equipment", %{
+      equipment: equipment,
+      interface1: interface1,
+      interface2: interface2
+    } do
+      # Create neighbors on different interfaces
+      {:ok, _neighbor1} =
+        Snmp.upsert_neighbor(%{
+          equipment_id: equipment.id,
+          interface_id: interface1.id,
+          protocol: "lldp",
+          remote_chassis_id: "aa:bb:cc:dd:ee:ff",
+          remote_system_name: "neighbor1",
+          last_discovered_at: DateTime.utc_now()
+        })
+
+      {:ok, _neighbor2} =
+        Snmp.upsert_neighbor(%{
+          equipment_id: equipment.id,
+          interface_id: interface2.id,
+          protocol: "cdp",
+          remote_chassis_id: "neighbor2",
+          remote_system_name: "neighbor2",
+          last_discovered_at: DateTime.utc_now()
+        })
+
+      neighbors = Snmp.list_neighbors(equipment.id)
+
+      assert length(neighbors) == 2
+      assert Enum.all?(neighbors, &(&1.equipment_id == equipment.id))
+      # Check that interface is preloaded
+      assert Enum.all?(neighbors, &Ecto.assoc_loaded?(&1.interface))
+    end
+
+    test "returns empty list for equipment with no neighbors", %{equipment: equipment} do
+      neighbors = Snmp.list_neighbors(equipment.id)
+      assert neighbors == []
+    end
+
+    test "orders neighbors by protocol and system name", %{
+      equipment: equipment,
+      interface1: interface1
+    } do
+      # Create neighbors in reverse alphabetical order
+      {:ok, _} =
+        Snmp.upsert_neighbor(%{
+          equipment_id: equipment.id,
+          interface_id: interface1.id,
+          protocol: "lldp",
+          remote_chassis_id: "chassis-z",
+          remote_system_name: "zebra",
+          last_discovered_at: DateTime.utc_now()
+        })
+
+      {:ok, _} =
+        Snmp.upsert_neighbor(%{
+          equipment_id: equipment.id,
+          interface_id: interface1.id,
+          protocol: "cdp",
+          remote_chassis_id: "chassis-a",
+          remote_system_name: "alpha",
+          last_discovered_at: DateTime.utc_now()
+        })
+
+      neighbors = Snmp.list_neighbors(equipment.id)
+
+      # Should be ordered by protocol, then system name
+      assert length(neighbors) == 2
+      [first, second] = neighbors
+      assert first.protocol == "cdp"
+      assert second.protocol == "lldp"
+    end
+  end
+
+  describe "delete_stale_neighbors/2" do
+    test "deletes neighbors not seen since cutoff", %{equipment: equipment, interface1: interface1} do
+      # Create old neighbor
+      two_days_ago = DateTime.add(DateTime.utc_now(), -48, :hour)
+
+      {:ok, old_neighbor} =
+        Snmp.upsert_neighbor(%{
+          equipment_id: equipment.id,
+          interface_id: interface1.id,
+          protocol: "lldp",
+          remote_chassis_id: "old-neighbor",
+          remote_system_name: "old-neighbor",
+          last_discovered_at: two_days_ago
+        })
+
+      # Create recent neighbor
+      {:ok, recent_neighbor} =
+        Snmp.upsert_neighbor(%{
+          equipment_id: equipment.id,
+          interface_id: interface1.id,
+          protocol: "cdp",
+          remote_chassis_id: "recent-neighbor",
+          remote_system_name: "recent-neighbor",
+          last_discovered_at: DateTime.utc_now()
+        })
+
+      # Delete neighbors older than 24 hours
+      cutoff = DateTime.add(DateTime.utc_now(), -24, :hour)
+      {count, _} = Snmp.delete_stale_neighbors(equipment.id, cutoff)
+
+      assert count == 1
+
+      # Old neighbor should be deleted
+      assert Repo.get(Neighbor, old_neighbor.id) == nil
+
+      # Recent neighbor should still exist
+      assert Repo.get(Neighbor, recent_neighbor.id)
+    end
+
+    test "only deletes neighbors for specified equipment", %{
+      interface1: interface1,
+      equipment: equipment
+    } do
+      user = user_fixture()
+      {:ok, org} = Towerops.Organizations.create_organization(%{name: "Org 2"}, user.id)
+      {:ok, site} = Towerops.Sites.create_site(%{name: "Site 2", organization_id: org.id})
+
+      {:ok, equipment2} =
+        Towerops.Equipment.create_equipment(%{
+          name: "Other Equipment",
+          ip_address: "192.168.1.2",
+          site_id: site.id
+        })
+
+      device2 =
+        %Device{}
+        |> Device.changeset(%{
+          equipment_id: equipment2.id,
+          sys_name: "other-device",
+          sys_descr: "Other Device"
+        })
+        |> Repo.insert!()
+
+      interface2 =
+        %Interface{}
+        |> Interface.changeset(%{
+          snmp_device_id: device2.id,
+          if_index: 1,
+          if_descr: "eth0",
+          if_type: 6
+        })
+        |> Repo.insert!()
+
+      two_days_ago = DateTime.add(DateTime.utc_now(), -48, :hour)
+
+      # Create old neighbors on both equipment
+      {:ok, neighbor1} =
+        Snmp.upsert_neighbor(%{
+          equipment_id: equipment.id,
+          interface_id: interface1.id,
+          protocol: "lldp",
+          remote_chassis_id: "neighbor1",
+          remote_system_name: "neighbor1",
+          last_discovered_at: two_days_ago
+        })
+
+      {:ok, neighbor2} =
+        Snmp.upsert_neighbor(%{
+          equipment_id: equipment2.id,
+          interface_id: interface2.id,
+          protocol: "lldp",
+          remote_chassis_id: "neighbor2",
+          remote_system_name: "neighbor2",
+          last_discovered_at: two_days_ago
+        })
+
+      # Delete only from first equipment
+      cutoff = DateTime.add(DateTime.utc_now(), -24, :hour)
+      {count, _} = Snmp.delete_stale_neighbors(equipment.id, cutoff)
+
+      assert count == 1
+
+      # First neighbor should be deleted
+      assert Repo.get(Neighbor, neighbor1.id) == nil
+
+      # Second neighbor should still exist
+      assert Repo.get(Neighbor, neighbor2.id)
+    end
+
+    test "returns count of 0 when no stale neighbors", %{equipment: equipment} do
+      cutoff = DateTime.add(DateTime.utc_now(), -24, :hour)
+      {count, _} = Snmp.delete_stale_neighbors(equipment.id, cutoff)
+
+      assert count == 0
+    end
+  end
+end