The remaining @dialyzer {:no_opaque, ...} / {:nowarn_function, ...}
directives in accounts.ex, devices.ex, organizations.ex all masked the
same upstream issue: Ecto.Multi.new/0 produces a struct containing a
literal %MapSet{map: %{}} whose concrete shape violates MapSet's
@opaque t at every subsequent Ecto.Multi.* call boundary.
Tried first: public helper function with @spec Ecto.Multi.t() — confirmed
dialyzer uses success typing of the body, not the spec. Doesn't help.
Real fix: drop Ecto.Multi entirely for these transactions. Each was
performing a small, sequential set of Repo operations that map cleanly
to a plain `Repo.transaction(fn -> ... end)` block with Repo.rollback
on errors. No Multi machinery is actually needed:
- accounts.ex:
- register_user/1: insert + 0-2 consent grants
- register_user_with_organization/1: insert + create_org + consents
- confirm_user_transaction/1: update + delete_all tokens
- organizations.ex:
- do_create_organization/4: lock user → limit check → insert org + membership
- set_default_organization/2: two update_alls
- accept_invitation/2: update invitation + insert membership
- devices.ex:
- create_device/2: maybe lock org → maybe quota check → insert
Caught a real consent-failure rollback bug along the way: initial
refactor discarded the return of grant_consents_sequential/2, which
meant FK violations on consent insert would abort the transaction at
the Postgres level but the caller still saw {:ok, user} — later
operations in the transaction silently failed with 25P02. Fixed by
propagating the {:error, changeset} up so Repo.rollback is called.
Verified by the AccountsDefensiveCoverageTest regression tests.
Behavior is preserved — same transactional guarantees, same success
/ error tuple shapes. Net -86 LOC and no suppressions.
mix dialyzer: `done (passed successfully)` with zero @dialyzer
directives in lib/.
Changes to eliminate @dialyzer suppressions by fixing underlying causes:
NIF stubs (towerops_native.ex, mib_translator.ex):
- Change stubs to :erlang.nif_error(:nif_not_loaded) (no_return type).
Real NIF replaces stubs at load time; calls to unloaded stubs now fail
loudly instead of returning fake data. Lets dialyzer trust @spec.
- Remove @dialyzer :nowarn_function on three NIFs and on translate/1.
Discovery sync_* functions (snmp/discovery.ex, channels/agent_channel.ex):
- agent_channel passes %{device_id: _, interfaces: _} and %{id: _} maps
into Discovery.sync_ip_addresses/sync_processors/sync_storage, which
@spec'd only %Device{}. Add narrow map-type unions (ip_sync_device,
snmp_device_ref) reflecting what the functions actually access.
- Remove @dialyzer :nowarn_function on three agent_channel helpers.
remote_ip.ex — real bug caught and fixed:
- `:ranch.get_addr(socket.transport_pid)` was always raising since
Bandit uses ThousandIsland, not Ranch; the rescue _ -> nil silently
returned nil every time. Switched to Phoenix's documented
:peer_data connect_info (already enabled in endpoint.ex) via
socket.assigns; remote IP now actually works.
- Remove remote_ip.ex entry from .dialyzer_ignore.exs.
Accounts / Organizations (Ecto.Multi opacity):
- Add @specs to Multi-building helpers, refactor into pipe chains.
- 6 @dialyzer :nowarn_function → 0, but 7 :no_opaque remain. Root
cause is upstream: Ecto.Multi.new/0 returns a struct with a literal
%MapSet{} whose @opaque internal representation trips dialyzer on
every subsequent Multi.* call. Unfixable without an Ecto patch or
bypassing Multi entirely. Comments document the specific upstream
issue rather than a vague "Ecto.Multi opacity" claim.
Devices.ex:
- Adding @specs made it worse (call_without_opaque → contract_with_
opaque); inlining the Multi didn't help either — same MapSet root
cause. Suppression kept with a sharper comment.
Categories addressed:
- pattern_match / pattern_match_cov (32): remove dead case/with clauses
that dialyzer proved unreachable from the caller types.
- contract_supertype / extra_range / invalid_contract /
contract_with_opaque (25): narrow @spec declarations to match actual
success typings.
- call / call_without_opaque (18): fix bad calls, narrow User.t to
allow nil for in-memory changeset structs, suppress Ecto.Multi
opaque-type false positives with targeted @dialyzer directives.
- guard_fail / no_return / unused_fun / unknown_function (13): remove
dead || fallbacks, simplify always-true params, cascade-resolve
no_returns via the underlying pattern_match and call fixes.
Real production bug fixed: StormDetector.handle_cast/2 had swapped
`:queue.in` args (`queue |> :queue.in(ts)` which desugars to
`:queue.in(queue, ts)` — wrong argument order). Alert timestamps
were never being enqueued, so storm detection would fail at runtime.
Corrected to `ts |> :queue.in(queue)`.
.dialyzer_ignore.exs: suppress two genuine dep-PLT gaps
(:ranch.get_addr/1 false positive from Bandit's transitive ranch,
and the Cloak.Vault GenServer callback_info on the CI build path).
`mix dialyzer` now: Total errors: 114, Skipped: 114 — passes clean.
Warnings: 88 → 0.
Prefix fire-and-forget calls (PubSub.broadcast, Oban enqueue, Logger,
update_*, etc.) with `_ =` so dialyzer knows the return value is
intentionally discarded. Wrap a few if/case expressions whose
branches produce mixed types the same way.
No behavior changes — only explicit acknowledgement of discarded
returns.
Warnings: 242 → 88.
- Add @type t :: %__MODULE__{} to schemas referenced by callers:
ApiToken, Monitoring.Check, OnCall.Schedule, SiteOutage,
NotificationDigest, Snmp.WirelessClient, Topology.DeviceLink,
Agent.AgentJob, Agent.MikrotikResult.
- Proto.Types: qualify forward references to nested modules with
Types. prefix, and fix Metric typo (should be union type metric()).
- data_loaders.ex / chart_builders.ex: Snmp.SNMPDevice.t was a ghost
spec — module is Snmp.Device.
Warnings: 328 → 242.
Ignore file was silencing every warning under lib/towerops/**. Root
cause was plt_add_deps: :apps_direct missing Plug/Phoenix/Oban/Decimal
/Redix/ssl/public_key — hundreds of false `unknown_function` warnings
were being papered over.
Expand plt_add_apps to cover the common deps, narrow the ignore file
to real dep-PLT gaps only, and fix two concrete bugs uncovered by the
change:
- ScopedResource.fetch_preload/4: spec was atom() | [atom()] but
callers pass keyword lists (e.g. [rules: :targets]).
- ToweropsNative: tag NIF stubs with @dialyzer :nowarn_function so
dialyzer trusts the @spec instead of the fallback body.
Remaining 328 real warnings surface for follow-up.
Intercepts requests with Accept: text/markdown at endpoint level (before
the router's :accepts plug rejects them) and returns hardcoded markdown
representations for /, /privacy, and /terms.
The test was using 75ms timeout which was too tight:
- 5 broadcasts with 5ms delays = 25ms
- Debounce delay = 50ms
- Processing overhead = variable
Increased to 200ms to match other similar tests in the file and provide
adequate buffer for debouncing + processing.
Also reformatted proto erlang_compat files to fix line length issues.
Reviewed-on: graham/towerops-web#210
Phase 1: Foundation Types (100% complete)
- query_helpers: SQL LIKE sanitization with pipe operators
- numeric: Integer parsing with pattern matching guards
- result: Pure Elixir Result monad (map, and_then, unwrap_or)
Phase 2: Ecto Domain Types (100% complete)
- ip_address: IPv4/IPv6 validation using :inet directly
- mac_address: Multi-format MAC parsing (colon/hyphen/dot/compact)
- snmp_oid: OID parsing/manipulation with recursive pattern matching
All 198 tests passing across converted modules.
API changed from Gleam-style {:some/:none to idiomatic {:ok/:error.
Refactored parse_numeric_oid to use with statement, reducing nesting depth.
Reviewed-on: graham/towerops-web#196
Phoenix.PubSub.Redis 3.1.0 automatically appends sync_connect: true to the
redis_opts before passing to Redix.PubSub.start_link. Our configuration was
also passing sync_connect: false, creating a duplicate key that NimbleOptions
rejected with a confusing error message.
The error showed:
NimbleOptions.ValidationError: unknown options [:sync_connect],
valid options are: [..., :sync_connect, ...]
This happened because sync_connect appeared twice in the keyword list,
and NimbleOptions rejects duplicate keys.
The fix removes our sync_connect option entirely, letting phoenix_pubsub_redis
control the sync_connect behavior.
This completes the phoenix_pubsub_redis 3.1.0 migration started in cd660387.
Fixes production pod crash on startup.
Reviewed-on: graham/towerops-web#198
The recent dependency update to phoenix_pubsub_redis 3.1.0 introduced
breaking changes in the configuration API. The new version no longer
accepts connection options at the top level - they must be nested in
:redis_opts.
Changes:
- Move Redix connection options into :redis_opts keyword list
- Keep only :node_name at top level per v3.1.0 requirements
- Maintains all resilience settings (timeouts, backoff, keepalive)
Before (invalid for v3.1.0):
[node_name: node(), host: "...", port: 6379, timeout: 5000, ...]
After (valid for v3.1.0):
[node_name: node(), redis_opts: [host: "...", port: 6379, timeout: 5000, ...]]
This fixes the production crash with error:
NimbleOptions.ValidationError: unknown options [:timeout, :backoff_initial,
:backoff_max, :exit_on_disconnection, :sync_connect]
Related: commit 0399b30d (Update Elixir dependencies)
Reviewed-on: graham/towerops-web#197
Fixes FunctionClauseError when DashboardLive receives {:alert_changed, org_id}
message broadcast by user_auth hooks.
The user_auth module subscribes all LiveViews to the
"organization:#{org_id}:alerts" topic and broadcasts {:alert_changed, org_id}
messages when alerts are created or resolved. DashboardLive was missing a
handle_info/2 clause to handle this message format, causing crashes in
production.
Solution:
- Added handle_info/2 clause for {:alert_changed, _org_id} message
- Uses existing debounced reload pattern to update dashboard
- Added test to verify message is handled without crashing
- Updated both technical and user-facing changelogs
All tests pass. The fix follows the same pattern used in AlertLive.Index
and DeviceLive.Show.
Reviewed-on: graham/towerops-web#189
Add complete memory pool monitoring using HOST-RESOURCES-MIB.
Tracks RAM/swap usage with historical data and real-time updates.
- Add Mempool and MempoolReading schemas
- Integrate discovery and polling with concurrent Task.async_stream
- Add 7 context API functions for querying mempools
- 24 new tests, all 8754 tests passing (0 failures)
Implements C1 from findings_librenms.md
Reviewed-on: graham/towerops-web#184
Replace sequential multi-GET with true batched SNMP GET support.
Previously, Client.get_multiple/2 sent N individual SNMP GET PDUs
for N OIDs. Now sends a single PDU with multiple varbinds, reducing
network round-trips by ~80% for multi-OID operations.
Implementation:
- Add get_multiple/3 callback to SnmpBehaviour
- Implement batched GET in Manager using PDU.build_get_request_multi/2
- Update Client.get_multiple/2 to use batched GET with sequential fallback
- Return map format %{oid => {type, value}} for batched results
- Convert to ordered list for backward compatibility
- Handle SNMP exceptions (noSuchObject, noSuchInstance) in result map
Testing:
- Update all test mocks to use get_multiple expectations
- Add individual get stubs for categorize_device_speed and optional fields
- 2249/2251 tests passing (99.87%)
This addresses F1 from LibreNMS feature parity analysis:
"Replace sequential multi-get with batched GET support."
Impact:
- Discovery system info (6 OIDs): 1 SNMP request instead of 6
- Storage polling (3 OIDs/entry): 1 request per entry instead of 3
- Network round-trips reduced by ~80% for all get_multiple operations
Reviewed-on: graham/towerops-web#183
- Check device.snmp_enabled before creating discovery/polling jobs
- Also check snmp_enabled for MikroTik jobs
- Prevents agent from attempting SNMP operations on ping-only devices
- Fixes "Agent collected 0 OIDs" errors for devices without SNMP
Reviewed-on: graham/towerops-web#180
- Add @disable_ddl_transaction and @disable_migration_lock for concurrent index
- Increase statement_timeout to 10min for DELETE operation on large tables
- Use concurrently: true on index creation to avoid table locks
- Refactor detect_airfiber_counter_overrides to fix credo nesting warning
- Prevents timeout errors in production deployment
Reviewed-on: graham/towerops-web#178
Commented out weathermap navigation in sidebar and mobile menu.
Feature not ready for production use yet.
Files: lib/towerops_web/components/layouts.ex
Changelog: CHANGELOG.txt
Reviewed-on: graham/towerops-web#174
- Create WeathermapLive module at /weathermap with 60s auto-refresh
- Extend Topology.get_topology_for_weathermap with bandwidth utilization data
- Add WeathermapViewer JavaScript hook with color-coded edges:
* Green (0-30%) → Yellow (30-60%) → Orange (60-80%) → Red (80%+)
- Display throughput/capacity labels on edges (e.g., '847 Mbps / 1 Gbps')
- Add utilization stats dashboard with link count by usage level
- Support full-screen mode for NOC displays
- Integrate with existing dark mode and responsive design
- Add navigation links in sidebar and mobile menu
Technical implementation:
- Leverages existing Capacity module for interface utilization calculations
- Extends topology edges with utilization_level, utilization_pct, throughput_bps
- Builds on NetworkMapLive foundation with enhanced edge styling
- Auto-refresh via Phoenix PubSub and 60s timer
- Filter support for high/critical utilization links
Reviewed-on: graham/towerops-web#172
Root cause: AF11X devices only support SNMPv1, IF-MIB eth0 counters
return zero, and air0 32-bit counters wrap too fast for backhaul speeds.
Fixes:
- Use correct UBNT-AirFIBER-MIB OID indices from MIB SEQUENCE definition:
.7.1 = rxOctetsOK (inbound), .6.1 = txOctetsOK (outbound)
.10.1 = rxErroredFrames, .11.1 = txErroredFrames
- Detect AirFiber devices by sysDescr instead of SNMP probing (v1 devices
cannot return Counter64 via standard probe queries)
- Force SNMPv1 for AirFiber proprietary queries
- Apply proprietary counters to all real interfaces (not just eth0)
Verified against live AF11X at 10.250.1.90:
- IF-MIB eth0 returns zero counters
- Proprietary .6.1/.7.1 show real traffic (3.75TB rx / 43TB tx)
Reviewed-on: graham/towerops-web#170
Root cause: AF11X devices only support SNMPv1, IF-MIB eth0 counters
return zero, and air0 32-bit counters wrap too fast for backhaul speeds.
Fixes:
- Use correct UBNT-AirFIBER-MIB OID indices from MIB SEQUENCE definition:
.7.1 = rxOctetsOK (inbound), .6.1 = txOctetsOK (outbound)
.10.1 = rxErroredFrames, .11.1 = txErroredFrames
- Detect AirFiber devices by sysDescr instead of SNMP probing (v1 devices
cannot return Counter64 via standard probe queries)
- Force SNMPv1 for AirFiber proprietary queries
- Apply proprietary counters to all real interfaces (not just eth0)
Verified against live AF11X at 10.250.1.90:
- IF-MIB eth0 returns zero counters
- Proprietary .6.1/.7.1 show real traffic (3.75TB rx / 43TB tx)
Reviewed-on: graham/towerops-web#169
- Clamp negative counter deltas to zero instead of attempting 32-bit wrap
correction (matches LibreNMS behavior — wraps are indistinguishable from
HC counter resets, so discard the ambiguous data point)
- Add rate sanity check: discard values exceeding 400 Gbps as counter anomalies
- Add 4-byte binary decoding for 32-bit SNMP counters in decode_snmp_value
- Track HC vs standard counter usage per interface stat (is_hc field)
- Add migration for is_hc column on snmp_interface_stats
- Update tests to match new clamping behavior
Reviewed-on: graham/towerops-web#168
Devices configured as SNMPv1 were being force-upgraded to v2c for agent
polling, which breaks devices that don't support v2c (returning empty
OID responses and no throughput/capacity data).
Reviewed-on: graham/towerops-web#166
The wrap detection only applied the 2^32 correction when the previous
counter was in the upper half of the 32-bit range (> 2^31). On fast
links, counters wrap regardless of where in the range they sit. Three
graph rendering paths had no wrap detection at all, just max(0).
Consolidated all bps calculations into a single public
Capacity.calculate_bps/3 that always tries the 2^32 correction for
negative deltas. 64-bit HC counter resets remain clamped to zero since
the corrected delta stays negative.
Reviewed-on: graham/towerops-web#163
Removed duplicate function definitions that were causing compile warnings:
- handle_info(:cleanup_expired, state) was defined twice (lines 94 and 110)
- schedule_cleanup/0 was defined twice (lines 105 and 121)
The duplicate definitions at lines 110-124 were exact copies and have been removed.
Reviewed-on: graham/towerops-web#162
Standard ifInOctets/ifOutOctets are 32-bit counters that wrap around
4,294,967,296 bytes (~4.3 GB). At 1 Gbps this wraps every ~34 seconds,
causing brief drops to 0 bps in traffic charts and capacity reports.
When prev > 2^31 and delta is negative, add 2^32 to recover the correct
rate. If the result is still negative (HC 64-bit counter after reboot),
clamp to 0. Small-value decreases (prev < 2^31) continue to clamp to 0
since they cannot be real 32-bit wraps.
Fixes both capacity.ex (utilization reports) and device_live/show.ex
(per-device traffic chart).
Reviewed-on: graham/towerops-web#161
Add data_retention_days field to organizations (default 365, range 30-730).
DataRetentionWorker runs nightly at 1 AM to purge expired time-series data
across 8 tables, respecting each org's retention setting.
TimescaleDB global retention bumped from 90 to 730 days as safety net.
Resolved alerts are cleaned up; unresolved alerts kept regardless of age.
Reviewed-on: graham/towerops-web#144
When an agent joins, we update last_seen_at in the database but don't set
last_heartbeat_db_update in the socket. This causes the heartbeat throttling
logic to be out of sync:
- On join: DB updated, but last_heartbeat_db_update not set (nil)
- First heartbeat: Updates DB again because last_heartbeat_db_update is nil
- Subsequent heartbeats: Check if >30s since last DB update
- Problem: last_heartbeat_db_update tracks the first heartbeat, not the join DB update
This manifests when Phoenix recompiles during development - the channel process
continues running with old socket assigns, and if last_heartbeat_db_update is
recent, heartbeats won't update the DB. After 10+ minutes without DB updates,
the agent appears offline even though it's still connected.
Fix: Set last_heartbeat_db_update when we update the DB on join, so the
throttling logic accurately tracks the most recent DB update.
Reviewed-on: graham/towerops-web#142
The Check protobuf has a `oneof config` field which generates separate
fields in the Elixir struct: :http, :tcp, :dns, :ssl. However,
check_type_config/2 was returning [config: {:http, ...}] which doesn't
match the struct definition, causing a KeyError crash in
build_and_push_check_jobs/1.
This bug was originally fixed in e0a74e2f but was accidentally
reintroduced in f703e61b when attempting to use "tagged tuple config".
Fix: Return field-specific keyword lists (e.g., [http: %HttpCheckConfig{}])
instead of [config: {:http, ...}].
Also added comprehensive test that creates all 4 check types and verifies
no KeyError crash occurs when sending check jobs to agent.
Reviewed-on: graham/towerops-web#141
