fix: address 56 findings from code audit #37

graham · 2026-09-20T18:32:00-05:00

graham commented

2026-09-20 18:32:00 -05:00

Fixes all findings from the 2026-09-20 code audit (bugs.md): 6 high, 30 medium, 20 low across lib/ and assets/js/.

High

is.ex: {:tcp, ...}/tcp_closed/tcp_error now match state.socket (stale-socket messages ignored) and :reconnect is a no-op while connected — a tcp_error+tcp_closed pair can no longer spawn parallel APRS-IS sessions.
map_live: :cleanup_old_packets loop is scheduled once via a tracked cleanup_timer assign (cancelled before reschedule) — handle_params patches no longer accumulate parallel cleanup chains.
connection_monitor → map_live: drain broadcast is now {:drain_connections, percent} (1..100) consumed as :rand.uniform(100) <= percent — previously a count was multiplied by 10, dropping every client at 10.
leader_election: pid_alive?/2 returns :alive | :dead | :unknown; only confirmed-dead (:nodedown/:noproc) unregisters — a transient RPC timeout no longer deletes a live leader's registration (split-brain). The :no registration path now calls notify_leadership_change(false) when stepping down, and :check_leadership no longer schedules elections during the cluster-formation wait.

Medium (selected)

handle_params/3: trail_duration, map_center, map_zoom compared against pre-assign values (URL nav/back-forward actually applies); tracking state reconciled from the callsign param; tracking a callsign preserves lat/lng/z/trail/hist; get_assigns debug event removed (it crashed the LiveView).
packet_batcher: same-second distinct packets accepted (dedupe by packet id).
historical_loader: total_batches derived from the zoom packet limit (was hardcoded 1 → high-zoom history stopped at 500 rows); a fully-filtered page now advances loading instead of stalling to the 15s failsafe.
mobile_channel: tracked callsign matches base_callsign too — object/item packets (BALLOON) now reach subscribers.
packets: :start_time/:end_time honored (ApiDocsLive's 30-day window works); limit/hours_back clamped at the context boundary (negative LIMIT no longer reaches Postgres).
prepared_queries: get_latest_packet_for_callsign orders by received_at DESC only — newest packet wins regardless of position.
partition_manager: packets_default rows past retention are deleted (protected callsigns preserved first); packets_preserved dedupes on composite (id, received_at) — new migration 20260920000000_packets_preserved_composite_identity drops the id-only unique index and adds the composite one.
application: cluster_enabled + empty topologies logs a warning and falls back to the standalone IsSupervisor; RateLimiter starts before Endpoint.
connection_monitor: registering pids are monitored (:DOWN decrements — abnormal LiveView exits no longer leak the count); check_load gathers stats in a Task so the GenServer stays responsive; scheduler_wall_time enabled so the CPU fallback works.
circuit_breaker: record_success only closes from :half_open; a single probe is admitted while half-open.
device_cache: refresh casts are dropped while a load is in flight (no more full-table SELECT stampede).
error_notifier: mail delivered via BroadcastTaskSupervisor off the telemetry handler (sync when the Swoosh test adapter is configured).
device_identification: an empty/malformed feed returns {:error, :empty_device_payload} instead of wiping the devices table.
is.ex: liveness/keepalive timers carry tokens so stale in-flight messages are ignored; stats count parsed packets not TCP chunks; get_status uses the exit-safe stats path; admit_bad_packet survives a missing rate-limit ETS table; reset_failure keeps the actually-connected server.
broadcast_task_supervisor: the atomics slot is released when start_child exits (:noproc).
packet_consumer_pool: per-consumer max_demand clamped to ≥ 1.
map.ts: lat/lng == 0 accepted; removeMarkerWithoutTrail unregisters from OMS; stale _leaflet_id no longer detaches the container; init-retry timers cleared on destroy and gated on isDestroyed; antimeridian pruning wraps marker lng into the bounds range; long-press converts viewport→container coords; updateMarker writes back to markerStates.
weather_charts: updated() guarded until the bundle loads; chart destroyed only after new data validates.
app.ts: failed bundle loads reset the loading flag and keep queued callbacks so remounts retry.
error_boundary.ts: attribution checks the event target / composedPath against the boundary element.
map_drawer.ts: document-level Tab trap redirects focus into the drawer.

Low

map.ts: callsignIndex cleared on historical reload; highlight_packet fallback maps MarkerState → MarkerData correctly.
endpoint: :user_agent added to mobile socket connect_info.
user_session_controller: partial user params → clean 400.
error_boundary component: CSP-safe reload link replaces inline onclick.
aprs_symbol: packet-controlled symbol table/code (and callsign) escaped before innerHTML.
callsign_view: merged packet list sorts by real timestamps.
cache: ttl <= 0 does not store.
spatial_pubsub: efficiency metric clamped ≥ 0; unparseable coords skipped instead of broadcasting to Null Island.
geo_utils: haversine a clamped to [0,1]; maidenhead: boundary/out-of-domain inputs handled.

Verification

mix test: 2273/2273 (25 doctests, 27 properties)
mix credo --strict: clean
mix format: clean
pre-commit hooks (format, credo, dialyzer): green on 55634ad0

Notes

Migration 20260920000000_packets_preserved_composite_identity is required for the new ON CONFLICT (id, received_at) — it drops packets_preserved_id_index and adds the composite unique index.
Tests that pinned buggy behavior were updated: is_test.exs (socket identity, tagged timers, per-packet stats), prepared_queries_test.exs (newest-wins ordering), historical_loader_test.exs (batch counts), cache_test.exs, device_cache_test.exs, device_identification_test.exs, connection_monitor_test.exs (async check_load), circuit_breaker_test.exs (single half-open probe), get_assigns tests removed.
docs/tracked-callsign-behavior.md updated for the new ordering.

Fixes all findings from the 2026-09-20 code audit (`bugs.md`): 6 high, 30 medium, 20 low across `lib/` and `assets/js/`. ## High - **is.ex**: `{:tcp, ...}`/`tcp_closed`/`tcp_error` now match `state.socket` (stale-socket messages ignored) and `:reconnect` is a no-op while connected — a `tcp_error`+`tcp_closed` pair can no longer spawn parallel APRS-IS sessions. - **map_live**: `:cleanup_old_packets` loop is scheduled once via a tracked `cleanup_timer` assign (cancelled before reschedule) — `handle_params` patches no longer accumulate parallel cleanup chains. - **connection_monitor → map_live**: drain broadcast is now `{:drain_connections, percent}` (1..100) consumed as `:rand.uniform(100) <= percent` — previously a count was multiplied by 10, dropping every client at 10. - **leader_election**: `pid_alive?/2` returns `:alive | :dead | :unknown`; only confirmed-dead (`:nodedown`/`:noproc`) unregisters — a transient RPC timeout no longer deletes a live leader's registration (split-brain). The `:no` registration path now calls `notify_leadership_change(false)` when stepping down, and `:check_leadership` no longer schedules elections during the cluster-formation wait. ## Medium (selected) - `handle_params/3`: `trail_duration`, `map_center`, `map_zoom` compared against pre-assign values (URL nav/back-forward actually applies); tracking state reconciled from the `callsign` param; tracking a callsign preserves `lat/lng/z/trail/hist`; `get_assigns` debug event removed (it crashed the LiveView). - `packet_batcher`: same-second distinct packets accepted (dedupe by packet id). - `historical_loader`: `total_batches` derived from the zoom packet limit (was hardcoded 1 → high-zoom history stopped at 500 rows); a fully-filtered page now advances loading instead of stalling to the 15s failsafe. - `mobile_channel`: tracked callsign matches `base_callsign` too — object/item packets (`BALLOON`) now reach subscribers. - `packets`: `:start_time`/`:end_time` honored (ApiDocsLive's 30-day window works); `limit`/`hours_back` clamped at the context boundary (negative `LIMIT` no longer reaches Postgres). - `prepared_queries`: `get_latest_packet_for_callsign` orders by `received_at DESC` only — newest packet wins regardless of position. - `partition_manager`: `packets_default` rows past retention are deleted (protected callsigns preserved first); `packets_preserved` dedupes on composite `(id, received_at)` — **new migration** `20260920000000_packets_preserved_composite_identity` drops the id-only unique index and adds the composite one. - `application`: `cluster_enabled` + empty topologies logs a warning and falls back to the standalone `IsSupervisor`; `RateLimiter` starts before `Endpoint`. - `connection_monitor`: registering pids are monitored (`:DOWN` decrements — abnormal LiveView exits no longer leak the count); `check_load` gathers stats in a Task so the GenServer stays responsive; `scheduler_wall_time` enabled so the CPU fallback works. - `circuit_breaker`: `record_success` only closes from `:half_open`; a single probe is admitted while half-open. - `device_cache`: refresh casts are dropped while a load is in flight (no more full-table SELECT stampede). - `error_notifier`: mail delivered via `BroadcastTaskSupervisor` off the telemetry handler (sync when the Swoosh test adapter is configured). - `device_identification`: an empty/malformed feed returns `{:error, :empty_device_payload}` instead of wiping the devices table. - `is.ex`: liveness/keepalive timers carry tokens so stale in-flight messages are ignored; stats count parsed packets not TCP chunks; `get_status` uses the exit-safe stats path; `admit_bad_packet` survives a missing rate-limit ETS table; `reset_failure` keeps the actually-connected server. - `broadcast_task_supervisor`: the atomics slot is released when `start_child` exits (`:noproc`). - `packet_consumer_pool`: per-consumer `max_demand` clamped to ≥ 1. - `map.ts`: `lat/lng == 0` accepted; `removeMarkerWithoutTrail` unregisters from OMS; stale `_leaflet_id` no longer detaches the container; init-retry timers cleared on destroy and gated on `isDestroyed`; antimeridian pruning wraps marker lng into the bounds range; long-press converts viewport→container coords; `updateMarker` writes back to `markerStates`. - `weather_charts`: `updated()` guarded until the bundle loads; chart destroyed only after new data validates. - `app.ts`: failed bundle loads reset the loading flag and keep queued callbacks so remounts retry. - `error_boundary.ts`: attribution checks the event target / composedPath against the boundary element. - `map_drawer.ts`: document-level Tab trap redirects focus into the drawer. ## Low - `map.ts`: `callsignIndex` cleared on historical reload; `highlight_packet` fallback maps `MarkerState` → `MarkerData` correctly. - `endpoint`: `:user_agent` added to mobile socket `connect_info`. - `user_session_controller`: partial `user` params → clean 400. - `error_boundary` component: CSP-safe reload link replaces inline `onclick`. - `aprs_symbol`: packet-controlled symbol table/code (and callsign) escaped before `innerHTML`. - `callsign_view`: merged packet list sorts by real timestamps. - `cache`: `ttl <= 0` does not store. - `spatial_pubsub`: efficiency metric clamped ≥ 0; unparseable coords skipped instead of broadcasting to Null Island. - `geo_utils`: haversine `a` clamped to `[0,1]`; `maidenhead`: boundary/out-of-domain inputs handled. ## Verification - `mix test`: **2273/2273** (25 doctests, 27 properties) - `mix credo --strict`: clean - `mix format`: clean - pre-commit hooks (format, credo, dialyzer): green on `55634ad0` ## Notes - Migration `20260920000000_packets_preserved_composite_identity` is required for the new `ON CONFLICT (id, received_at)` — it drops `packets_preserved_id_index` and adds the composite unique index. - Tests that pinned buggy behavior were updated: `is_test.exs` (socket identity, tagged timers, per-packet stats), `prepared_queries_test.exs` (newest-wins ordering), `historical_loader_test.exs` (batch counts), `cache_test.exs`, `device_cache_test.exs`, `device_identification_test.exs`, `connection_monitor_test.exs` (async check_load), `circuit_breaker_test.exs` (single half-open probe), `get_assigns` tests removed. - `docs/tracked-callsign-behavior.md` updated for the new ordering.

graham added 1 commit

2026-09-20 18:32:00 -05:00

fix: address 56 findings from code audit

skippy-bot/review Skippy review: 2 blocking findings open — see the PR thread

Details

Elixir CI / Build and test (pull_request) Failing after 58s

Details

Elixir CI / Dialyzer (pull_request) Successful in 41s

Details

Elixir CI / Build and Push Docker Image (pull_request) Has been skipped

Details

55634ad0c6

See PR description for per-finding details.

skippy-bot reviewed

2026-09-20 18:44:55 -05:00

skippy-bot left a comment

🤖 Skippy PR review

4 findings — 2 blocking before merge.

Severity	Location	Issue
🟡 Warning	`lib/aprsme/packets/prepared_queries.ex:35`	Newest-wins ordering breaks map centring for tracked callsigns
🟡 Warning	`priv/repo/migrations/20260920000000_packets_preserved_composite_identity.exs:30`	down/0 cannot run once the archive holds repeat observations
🔵 Suggestion	`lib/aprsme/error_notifier.ex:54`	Saturated broadcast pool silently swallows the error email
🔵 Suggestion	`lib/aprsme_web/aprs_symbol.ex:203`	Overlay branch lost image-rendering: pixelated

_{Reviewed 55634ad0c61c. Comment skippy review to re-run.}

### 🤖 Skippy PR review **4 findings** — 2 blocking before merge. | Severity | Location | Issue | | --- | --- | --- | | 🟡 Warning | `lib/aprsme/packets/prepared_queries.ex:35` | Newest-wins ordering breaks map centring for tracked callsigns | | 🟡 Warning | `priv/repo/migrations/20260920000000_packets_preserved_composite_identity.exs:30` | down/0 cannot run once the archive holds repeat observations | | 🔵 Suggestion | `lib/aprsme/error_notifier.ex:54` | Saturated broadcast pool silently swallows the error email | | 🔵 Suggestion | `lib/aprsme_web/aprs_symbol.ex:203` | Overlay branch lost image-rendering: pixelated | <sub>Reviewed `55634ad0c61c`. Comment `skippy review` to re-run.</sub>

lib/aprsme/error_notifier.ex Outdated

					
				@ -44,0 +51,4 @@

				    if test_mailer?() do

				      maybe_send_error_email(occurrence)

				    else

				      Aprsme.BroadcastTaskSupervisor.async_execute(fn -> maybe_send_error_email(occurrence) end)

🔵 Suggestion — Saturated broadcast pool silently swallows the error email

async_execute/1 sheds work at its concurrency ceiling and returns :dropped (documented in Aprsme.BroadcastTaskSupervisor); the return value is discarded here, so during an error storm, exactly when notifications matter, a first-occurrence email can vanish with no log. occurrence_count == 1 is never retried.

Log the non-started result, e.g. case Aprsme.BroadcastTaskSupervisor.async_execute(...) do {:ok, _} -> :ok; other -> Logger.error("Error email not dispatched: #{inspect(other)}") end, or send synchronously on that path.

**🔵 Suggestion — Saturated broadcast pool silently swallows the error email** `async_execute/1` sheds work at its concurrency ceiling and returns `:dropped` (documented in `Aprsme.BroadcastTaskSupervisor`); the return value is discarded here, so during an error storm, exactly when notifications matter, a first-occurrence email can vanish with no log. `occurrence_count == 1` is never retried. Log the non-started result, e.g. `case Aprsme.BroadcastTaskSupervisor.async_execute(...) do {:ok, _} -> :ok; other -> Logger.error("Error email not dispatched: #{inspect(other)}") end`, or send synchronously on that path.

skippy-bot marked this conversation as resolved

lib/aprsme/packets/prepared_queries.ex

					
				@ -36,3 +35,1 @@

				          desc: fragment("CASE WHEN ? IS NULL THEN 0 ELSE 1 END", p.location),

				          desc: p.received_at

				        ],

				        order_by: [desc: p.received_at],

🟡 Warning — Newest-wins ordering breaks map centring for tracked callsigns

get_latest_packet_for_callsign/1 no longer prefers a positioned row, so it can now return a status/object/item packet whose lat/lon are nil (the select pulls them from location). Every caller that centres on the tracked station needs a position, and all of them silently no-op on nil: Navigation.handle_callsign_tracking/4, Index.center_on_latest_packet/5 and Events.zoom_to_latest_packet/2. The live path still disagrees with the query: Index.preferred_tracked_packet/2 explicitly prefers CoordinateUtils.has_position_data?.

Concretely: opening /W5ISP-1 (search, refresh, or back/forward) no longer centres or zooms the map whenever that station's newest packet is not a position packet, which is normal for status/object-heavy stations.

Keep newest-wins for display, but give the centring paths a positioned fallback, e.g. Packets.get_latest_positioned_packet_for_callsign/1 (same query plus where: not is_nil(p.location)) used by handle_callsign_tracking/4, center_on_latest_packet/5 and zoom_to_latest_packet/2.

**🟡 Warning — Newest-wins ordering breaks map centring for tracked callsigns** `get_latest_packet_for_callsign/1` no longer prefers a positioned row, so it can now return a status/object/item packet whose `lat`/`lon` are nil (the select pulls them from `location`). Every caller that centres on the tracked station needs a position, and all of them silently no-op on nil: `Navigation.handle_callsign_tracking/4`, `Index.center_on_latest_packet/5` and `Events.zoom_to_latest_packet/2`. The live path still disagrees with the query: `Index.preferred_tracked_packet/2` explicitly prefers `CoordinateUtils.has_position_data?`. Concretely: opening `/W5ISP-1` (search, refresh, or back/forward) no longer centres or zooms the map whenever that station's newest packet is not a position packet, which is normal for status/object-heavy stations. Keep newest-wins for display, but give the centring paths a positioned fallback, e.g. `Packets.get_latest_positioned_packet_for_callsign/1` (same query plus `where: not is_nil(p.location)`) used by `handle_callsign_tracking/4`, `center_on_latest_packet/5` and `zoom_to_latest_packet/2`.

skippy-bot marked this conversation as resolved

lib/aprsme_web/aprs_symbol.ex

					
				@ -198,3 +202,2 @@

				          background-repeat: no-repeat, no-repeat;

				          image-rendering: pixelated;

				        " title="#{symbol_table}#{symbol_code}">

				        " title="#{title}">

🔵 Suggestion — Overlay branch lost image-rendering: pixelated

The escaping change also removed image-rendering: pixelated; from the overlay branch only. The non-overlay branch below (line 215) still sets it, as does render_style/3, so overlay symbols (tables A-Z / 0-9) are now smooth-scaled off the sprite sheet while every other sprite stays crisp. Restore the declaration if the removal was not deliberate.

**🔵 Suggestion — Overlay branch lost image-rendering: pixelated** The escaping change also removed `image-rendering: pixelated;` from the overlay branch only. The non-overlay branch below (line 215) still sets it, as does `render_style/3`, so overlay symbols (tables `A`-`Z` / `0`-`9`) are now smooth-scaled off the sprite sheet while every other sprite stays crisp. Restore the declaration if the removal was not deliberate.

skippy-bot marked this conversation as resolved

priv/repo/migrations/20260920000000_packets_preserved_composite_identity.exs

					
				@ -0,0 +27,4 @@

				  end

				  def down do

				    create unique_index(:packets_preserved, [:id])

🟡 Warning — down/0 cannot run once the archive holds repeat observations

up widens the idempotency key to (id, received_at) precisely so packets_preserved can keep the same raw packet re-received at a different timestamp. down then recreates the id-only unique index, and it is the first statement in the rollback: as soon as one repeat exists, create unique_index(:packets_preserved, [:id]) raises a duplicate-key error and mix ecto.rollback fails, leaving the DB on the new schema with no way back.

Either dedupe first in down (keep the newest received_at per id, which is an explicit data loss), or drop the id-only index recreation and make the function raise/document that the migration is not reversible.

**🟡 Warning — down/0 cannot run once the archive holds repeat observations** `up` widens the idempotency key to `(id, received_at)` precisely so `packets_preserved` can keep the same raw packet re-received at a different timestamp. `down` then recreates the id-only unique index, and it is the first statement in the rollback: as soon as one repeat exists, `create unique_index(:packets_preserved, [:id])` raises a duplicate-key error and `mix ecto.rollback` fails, leaving the DB on the new schema with no way back. Either dedupe first in `down` (keep the newest `received_at` per `id`, which is an explicit data loss), or drop the id-only index recreation and make the function raise/document that the migration is not reversible.

skippy-bot marked this conversation as resolved

graham referenced this pull request from a commit

2026-09-21 16:10:12 -05:00

fix: address Skippy review findings on #37

graham added 1 commit

2026-09-21 16:10:12 -05:00

fix: address Skippy review findings on #37

skippy-bot/review Skippy review: clean — no open findings

Details

Elixir CI / Build and test (pull_request) Failing after 4m29s

Details

Elixir CI / Dialyzer (pull_request) Successful in 7m11s

Details

Elixir CI / Build and Push Docker Image (pull_request) Has been skipped

Details

0529d68c6c

- prepared_queries: add get_latest_positioned_packet_for_callsign/1 and
  use it in the map-centering paths (Navigation.handle_callsign_tracking,
  Index.center_on_latest_packet, Events.zoom_to_latest_packet) so
  tracking still centres when the newest packet has no position
- migration down/0: dedupe repeat observations (keep newest per id)
  before recreating the id-only unique index so rollback can't fail
- error_notifier: log when the broadcast pool drops/fails the dispatch
  instead of silently swallowing a first-occurrence email
- aprs_symbol: restore image-rendering: pixelated in the overlay branch

skippy-bot commented

2026-09-21 16:12:45 -05:00

First-time contributor

Resolved 4 of 4 earlier findings in 0529d68:

prepared_queries.ex + the three centring paths (Navigation.handle_callsign_tracking/4, Index.center_on_latest_packet/5, Events.zoom_to_latest_packet/2) - positioned-packet fallback added.
packets_preserved migration down/0 - repeats are deduped (newest received_at per id) before the id-only index is recreated; received_at is NOT NULL through LIKE packets, so no NULL duplicate can slip past the a.received_at < b.received_at predicate.
error_notifier.ex - a :dropped/{:error, _} dispatch is now logged; async_execute/1 does return {:ok, pid} on success, so the new case is correct.
aprs_symbol.ex - image-rendering: pixelated; restored on the overlay branch.

Nothing new in 55634ad..0529d68. 0 still open.

_{Reviewed 0529d68c6c7b.}

**Resolved 4 of 4 earlier findings** in `0529d68`: - `prepared_queries.ex` + the three centring paths (`Navigation.handle_callsign_tracking/4`, `Index.center_on_latest_packet/5`, `Events.zoom_to_latest_packet/2`) - positioned-packet fallback added. - `packets_preserved` migration `down/0` - repeats are deduped (newest `received_at` per `id`) before the id-only index is recreated; `received_at` is `NOT NULL` through `LIKE packets`, so no NULL duplicate can slip past the `a.received_at < b.received_at` predicate. - `error_notifier.ex` - a `:dropped`/`{:error, _}` dispatch is now logged; `async_execute/1` does return `{:ok, pid}` on success, so the new `case` is correct. - `aprs_symbol.ex` - `image-rendering: pixelated;` restored on the overlay branch. Nothing new in `55634ad..0529d68`. 0 still open. <sub>Reviewed `0529d68c6c7b`.</sub>

graham merged commit 693cbea227 into main

2026-09-21 16:15:01 -05:00

graham deleted branch fix/bug-audit-batch

2026-09-21 16:15:01 -05:00

graham referenced this pull request from a commit

2026-09-21 16:15:01 -05:00

fix: address 56 findings from code audit (#37)

Sign in to join this conversation.

No reviewers

No labels

No milestone

No project