diff --git a/findings.md b/findings.md new file mode 100644 index 00000000..e916f1cb --- /dev/null +++ b/findings.md @@ -0,0 +1,97 @@ +# Findings — Bugs & Improvements (Non-Critical) + +## Bugs + +### 🟠 High + +| # | File | Line | Issue | Suggested Fix | +|---|------|------|-------|---------------| +| 1 | `mechanism_classifier.ex` | 314-317 | `get_lat`/`get_lon` crash on nil — `nil / 1.0` raises `ArithmeticError` if radar lookup returns nil coords | Add pattern match for nil or guard before division | +| 2 | `path_compute.ex` | 433 | `conditions.abs_humidity` nil crash — `nil * dist_km` raises if conditions map is missing `:abs_humidity` | Use `Map.get(conditions, :abs_humidity, 7.5)` with fallback | +| 3 | `beacon_live/index.ex` | 119 | `path_with_prefix` creates `//beacons` URLs — `"/" <> path` doubles slash on already-prefixed paths | `String.trim_leading(path, "/")` | +| 4 | `map_live.ex` | 1053 | `flash_group` called outside `layouts.ex` — violates AGENTS.md rule | Include equivalent in-line FlashGroup component or restructure to use `Layouts.app` | +| 5 | `config/runtime.exs` | 142, 386 | `EMAIL_SERVER` not validated — unset produces empty SNI causing silent TLS handshake failure | Raise at boot when required env vars are missing (like `DATABASE_URL` does) | +| 6 | `notify_listener.ex` | 119-126 | `Task.start/1` linked to caller — crash in task kills the GenServer, contradicting moduledoc | Use `Task.Supervisor.start_child` or wrap body in `try/rescue` | +| 7 | `notify_listener.ex` | 36, 40-51 | No `Process.monitor` on `Postgrex.Notifications` — if PG notify connection crashes, GenServer never learns | Add `Process.monitor` and handle `{:DOWN, ...}` in `handle_info` | + +### 🟡 Medium + +| # | File | Line | Issue | Suggested Fix | +|---|------|------|-------|---------------| +| 8 | `radio.ex` | 128, 157 | N+1 queries — `list_contacts_for_user` and `list_contacts_involving_callsign` don't preload `:user` | Add `|> Repo.preload(:user)` before `Repo.all()` | +| 9 | `radio.ex` | 751-757 | Missing indexes — dedup query filters on `band`, `qso_timestamp`, `flagged_invalid` with no covering index | Add partial index on `(band, qso_timestamp) WHERE flagged_invalid = false` | +| 10 | `grid.ex` | 31-36 | `conus_points/0` recomputes ~94k elements on every call — pure function, never memoized | Memoize with module attribute `@conus_points conus_points()` | +| 11 | `endpoint.ex` | 7-12 | Session cookie is signed but not encrypted — payload is readable though tamper-proof | Add `encryption_salt` to `@session_options` | +| 12 | `accounts.ex` | 528-538 | Fetches all tokens before deleting them — two queries where one suffices | Use `Repo.delete_all(from t in UserToken, where: t.user_id == ^user.id)` | +| 13 | `profiles_file.ex` | 169-177 | Unhandled `:zlib.gunzip/1` exception — corrupt gzip files raise through cache wrapper | Wrap in `try/rescue` | +| 14 | `scores_file.ex` | 515 | Direct ETS `match_delete` bypassing Cache API | Use `Cache.invalidate` or `Cache` public API | +| 15 | `profiles_file.ex` | 323 | Same direct ETS access as above | Same fix | +| 16 | `path_compute.ex` | 381-394 | Six redundant list traversals — 12 passes over 9 profiles for what could be 1 `Enum.reduce` | Single pass with `Enum.reduce` | +| 17 | `hf_muf.ex` | 51 | `sec_i_factor(@ref_distance_km)` recomputed on every call — constant input | Precompute with module attribute | +| 18 | `config/config.exs` | 163 | `EXLA.Backend` configured for all envs but `nx`/`exla` are dev/test-only deps | Guard with `if Mix.env() in [:dev, :test]` | + +### 🟢 Low + +| # | File | Line | Issue | Suggested Fix | +|---|------|------|-------|---------------| +| 19 | `router.ex` | 165 | Login rate limit 30/min per IP may be generous for brute-force | Consider lowering to 10-15/min | +| 20 | `application.ex` | 107-109 | Dev/test model loading blocks supervisor start | Wrap in `Task.start` | +| 21 | `scorer.ex` | 126-138 | `classify_time_period` guard boundaries have gap at exactly `-3.0` | Switch to `cond` with inclusive/exclusive clarity | +| 22 | `scorer.ex` | 636 | `||` for default when `//` is more intention-revealing | Replace `contact.pos1["lon"] \|\| -97.0` with `//` | +| 23 | `path_compute.ex` | 356-357 | Eager `Repo.get(Station, ...)` instead of preloading assoc | Preload `:station` upstream | +| 24 | `radio.ex` | 1203 | Hardcoded cache key `{ContactMapController, :gzipped_payload}` fragile to module rename | Extract to a named key in `ContactMapController` | +| 25 | `user.ex` | — | Missing `has_many :contacts` and `has_many :beacons` associations | Add for convenience (not a bug, test callers use raw queries) | +| 26 | `radio.ex` | 337 | Missing partial indexes for enrichment queries | Create filtered indexes on `(qso_timestamp) WHERE weather_status IN ('pending','failed')` | + +--- + +## Improvements + +### Architecture & Design + +| # | Area | Suggestion | +|---|------|------------| +| 1 | `contact_live_test.exs` (3505 lines) | Split into smaller files — currently 2.5x next largest file, uses `async: true` with shared state, relies on `send(lv.pid, ...)` / `:sys.get_state` | +| 2 | **18 `Process.sleep` usages** in tests | Replace with `Process.monitor` + `assert_receive` or `:sys.get_state` per AGENTS.md | +| 3 | **25 `Process.alive?` assertions** in tests | Assert on DOM output instead — only verifies process didn't crash, not that handler did anything | +| 4 | `route.ex` | `get "/"` page controller serves HTML _and_ markdown via `serve_markdown_if_requested` — bypasses secure headers on markdown path | +| 5 | `router.ex` | Some public routes (`/docs/api/openapi.yaml`) are inside `:browser` pipeline but outside `live_session` — comment explains it's intentional but fragile | + +### Test Coverage Gaps + +| Module | Status | +|--------|--------| +| `lib/microwaveprop/ionosphere.ex` | **Untested** | +| `lib/microwaveprop/mailer.ex` | **Untested** | +| `lib/microwaveprop/repo.ex` | **Untested** | +| `lib/microwaveprop/space_weather.ex` | **Untested** | +| `about_live.ex` | Real DB query logic, **untested** | + +### Config & Tooling + +| # | Finding | Suggested Fix | +|---|---------|---------------| +| 1 | `LIVE_VIEW_SIGNING_SALT` hardcoded in `config.exs`, not read from env | Read from `System.get_env` in `runtime.exs` | +| 2 | `:precommit` uses `deps.unlock --unused` (modifies lockfile) | Use `--check-unused` for read-only check | +| 3 | `Credo.Check.Warning.UnsafeToAtom` disabled | Re-enable to catch `String.to_atom(user_input)` DOS vector | +| 4 | `Credo.Check.Readability.Specs` disabled | Consider re-enabling for production codebase | +| 5 | `Credo.Check.Warning.LeakyEnvironment` disabled | Re-enable to catch accidental env var logging | + +### Performance Hotspots + +| # | Area | Impact | +|---|------|--------| +| 1 | `grid.ex:31` — `conus_points()` rebuilds ~94k items per call | **Highest impact fix** — memoize with `@conus_points` | +| 2 | `radio.ex` dedup queries — missing indexes | Growing table, will degrade over time | +| 3 | Enrichment queries — missing partial indexes | Each `contacts_needing_enrichment` call scans entire table | +| 4 | `path_compute.ex:381-394` — 12 list traversals for 9-element profile lists | Small today, but unnecessary overhead in hot path | + +### Security (Remaining) + +| # | Finding | Severity | +|---|---------|----------| +| 1 | Session cookie missing encryption salt (endpoint.ex:7-12) | Medium | +| 2 | `String.to_atom/1` not warned by Credo (`.credo.exs` has `UnsafeToAtom` disabled) | Low | +| 3 | Login rate limit at 30/min may be generous | Low | +| 4 | `build_contact_changes` in `radio.ex:1277` uses `String.to_existing_atom(key)` — safe due to whitelist, but fragile | Low | +| 5 | Markdown path bypasses secure browser headers | Low (mitigated by plain-text content type) | diff --git a/lib/microwaveprop/application.ex b/lib/microwaveprop/application.ex index 0828f498..14d8f879 100644 --- a/lib/microwaveprop/application.ex +++ b/lib/microwaveprop/application.ex @@ -75,8 +75,24 @@ defmodule Microwaveprop.Application do # the Oban supervisor starts (above in `children`). :ok = Microwaveprop.ObanErrorReporter.attach() - # Run migrations after Repo is started - _ = Microwaveprop.Release.migrate() + # Run migrations after Repo is started. Wrapped in a background + # Task so the application supervisor doesn't block on long-running + # migrations on pod boot. Errors are logged but don't crash boot — + # the health check will catch schema mismatches. + Task.start(fn -> + try do + case Microwaveprop.Release.migrate() do + results when is_list(results) -> + Logger.info("Database migrations completed (#{length(results)} repos)") + + other -> + Logger.warning("Database migrations returned: #{inspect(other)}") + end + rescue + e -> + Logger.error("Database migrations failed: #{Exception.format(:error, e, __STACKTRACE__)}") + end + end) # Warm GridCache from the latest persisted ProfilesFile so /weather # has data immediately after a pod restart. Runs after the GridCache diff --git a/lib/microwaveprop/radio.ex b/lib/microwaveprop/radio.ex index 0fecd8b9..26664f50 100644 --- a/lib/microwaveprop/radio.ex +++ b/lib/microwaveprop/radio.ex @@ -732,6 +732,15 @@ defmodule Microwaveprop.Radio do changeset = Ecto.Changeset.add_error(changeset, :grid1, "could not resolve grid to coordinates") {:error, %{changeset | action: :insert}} end + rescue + e in Ecto.ConstraintError -> + # Unique constraint violation from a concurrent insert — the + # dedup check at `insert_validated_contact` raced with another + # process. Re-check and return the existing contact. + case find_duplicate_contact(changeset) do + nil -> {:error, :constraint_error} + existing -> {:error, :duplicate, existing} + end end defp find_duplicate_contact(changeset) do diff --git a/lib/microwaveprop/radio/contact.ex b/lib/microwaveprop/radio/contact.ex index 3768aa60..d5cf0b24 100644 --- a/lib/microwaveprop/radio/contact.ex +++ b/lib/microwaveprop/radio/contact.ex @@ -82,7 +82,7 @@ defmodule Microwaveprop.Radio.Contact do @type t :: %__MODULE__{} @required_fields ~w(station1 station2 qso_timestamp band)a - @optional_fields ~w(grid1 grid2 pos1 pos2 distance_km user_id mode user_declared_prop_mode height1_ft height2_ft private notes)a + @optional_fields ~w(grid1 grid2 pos1 pos2 distance_km mode user_declared_prop_mode height1_ft height2_ft private notes)a @spec changeset(t() | Ecto.Changeset.t(), map()) :: Ecto.Changeset.t() def changeset(contact, attrs) do diff --git a/lib/microwaveprop_web/api/rate_limiter.ex b/lib/microwaveprop_web/api/rate_limiter.ex index 8594e638..c8bdb750 100644 --- a/lib/microwaveprop_web/api/rate_limiter.ex +++ b/lib/microwaveprop_web/api/rate_limiter.ex @@ -101,7 +101,7 @@ defmodule MicrowavepropWeb.Api.RateLimiter do defp bucket_for(conn, opts) do case conn.assigns[:current_api_token] do - %{id: id} when is_binary(id) -> + %{id: id} when not is_nil(id) -> {{:token, id}, Keyword.get(opts, :auth_limit, @default_auth_limit)} _ -> diff --git a/lib/microwaveprop_web/live/path_live.ex b/lib/microwaveprop_web/live/path_live.ex index 99cdfb4d..bab9e49d 100644 --- a/lib/microwaveprop_web/live/path_live.ex +++ b/lib/microwaveprop_web/live/path_live.ex @@ -107,9 +107,22 @@ defmodule MicrowavepropWeb.PathLive do is_gps = p["source"] == "gps" + # Clear stale results when URL params change — otherwise a cached + # result from a previous calculation persists when the user navigates + # to a different source/destination/band combination. + new_source = if(is_gps, do: socket.assigns.source, else: p["source"]) + params_changed? = params_differ?(socket, p, new_source) + + socket = + if params_changed? do + assign(socket, result: nil, error: nil) + else + socket + end + socket = assign(socket, - source: if(is_gps, do: socket.assigns.source, else: p["source"]), + source: new_source, destination: p["destination"], band: p["band"], src_height_ft: p["src_height_ft"], @@ -133,6 +146,18 @@ defmodule MicrowavepropWeb.PathLive do end end + defp params_differ?(socket, p, new_source) do + socket.assigns.result != nil and + (new_source != socket.assigns.source or + p["destination"] != socket.assigns.destination or + p["band"] != socket.assigns.band or + p["src_height_ft"] != socket.assigns.src_height_ft or + p["dst_height_ft"] != socket.assigns.dst_height_ft or + p["tx_power_dbm"] != socket.assigns.tx_power_dbm or + p["src_gain_dbi"] != socket.assigns.src_gain_dbi or + p["dst_gain_dbi"] != socket.assigns.dst_gain_dbi) + end + # Loads a cached rover-planning Path and rebuilds the @result map # PathLive's render expects. The blob lives under `result["term"]` # (Base64-encoded `:erlang.term_to_binary/1` of PathCompute's full diff --git a/priv/repo/migrations/20260529201755_add_contacts_dedup_index.exs b/priv/repo/migrations/20260529201755_add_contacts_dedup_index.exs new file mode 100644 index 00000000..8f49adbb --- /dev/null +++ b/priv/repo/migrations/20260529201755_add_contacts_dedup_index.exs @@ -0,0 +1,44 @@ +defmodule Microwaveprop.Repo.Migrations.AddContactsDedupIndex do + use Ecto.Migration + + def up do + # Remove existing duplicates before creating the unique index. + # Within each group of (band, qso_timestamp, normalized station pair, + # normalized grid pair), keep only the first-inserted contact and + # remove later duplicates. Flagged-invalid contacts are excluded + # from the index predicate, so they never cause violations. + execute """ + DELETE FROM contacts + WHERE id IN ( + SELECT id FROM ( + SELECT id, + ROW_NUMBER() OVER ( + PARTITION BY band, qso_timestamp, + LEAST(upper(station1), upper(station2)), + GREATEST(upper(station1), upper(station2)), + LEAST(upper(grid1), upper(grid2)), + GREATEST(upper(grid1), upper(grid2)) + ORDER BY inserted_at, id + ) AS rn + FROM contacts + WHERE flagged_invalid = false + ) dup + WHERE dup.rn > 1 + ) + """ + + execute """ + CREATE UNIQUE INDEX contacts_dedup_idx ON contacts + (band, qso_timestamp, + LEAST(upper(station1), upper(station2)), + GREATEST(upper(station1), upper(station2)), + LEAST(upper(grid1), upper(grid2)), + GREATEST(upper(grid1), upper(grid2))) + WHERE flagged_invalid = false + """ + end + + def down do + execute "DROP INDEX contacts_dedup_idx" + end +end diff --git a/test/microwaveprop/radio/contact_edit_test.exs b/test/microwaveprop/radio/contact_edit_test.exs index 629e56ba..7f2a42d2 100644 --- a/test/microwaveprop/radio/contact_edit_test.exs +++ b/test/microwaveprop/radio/contact_edit_test.exs @@ -331,8 +331,8 @@ defmodule Microwaveprop.Radio.ContactEditTest do defp owned_contact(user) do {:ok, contact} = - %Contact{} - |> Contact.changeset(Map.put(@contact_attrs, :user_id, user.id)) + %Contact{user_id: user.id} + |> Contact.changeset(@contact_attrs) |> Microwaveprop.Repo.insert() contact @@ -543,8 +543,8 @@ defmodule Microwaveprop.Radio.ContactEditTest do test "true when contact.user_id matches the user", %{user: user} do {:ok, contact} = - %Contact{} - |> Contact.changeset(Map.put(@contact_attrs, :user_id, user.id)) + %Contact{user_id: user.id} + |> Contact.changeset(@contact_attrs) |> Microwaveprop.Repo.insert() assert Radio.owner?(contact, user) diff --git a/test/microwaveprop/radio_test.exs b/test/microwaveprop/radio_test.exs index f56986f6..2004f574 100644 --- a/test/microwaveprop/radio_test.exs +++ b/test/microwaveprop/radio_test.exs @@ -21,9 +21,12 @@ defmodule Microwaveprop.RadioTest do distance_km: Decimal.new("295") } + merged = Map.merge(default, attrs) + {user_id, changeset_attrs} = Map.pop(merged, :user_id) + {:ok, contact} = - %Contact{} - |> Contact.changeset(Map.merge(default, attrs)) + %Contact{user_id: user_id} + |> Contact.changeset(changeset_attrs) |> Repo.insert() contact diff --git a/test/microwaveprop_web/live/contact_live_test.exs b/test/microwaveprop_web/live/contact_live_test.exs index f4d9e7eb..af776b7e 100644 --- a/test/microwaveprop_web/live/contact_live_test.exs +++ b/test/microwaveprop_web/live/contact_live_test.exs @@ -39,12 +39,12 @@ defmodule MicrowavepropWeb.ContactLiveTest do distance_km: Decimal.new("295") } - {changeset_attrs, direct_attrs} = Map.split(Map.merge(default, attrs), [:user_submitted]) + merged = Map.merge(default, attrs) + {user_id, changeset_attrs} = Map.pop(merged, :user_id) {:ok, contact} = - %Contact{} - |> Contact.changeset(direct_attrs) - |> Ecto.Changeset.change(changeset_attrs) + %Contact{user_id: user_id} + |> Contact.changeset(changeset_attrs) |> Repo.insert() contact diff --git a/test/microwaveprop_web/live/user_profile_live_test.exs b/test/microwaveprop_web/live/user_profile_live_test.exs index 6673fa9d..d2db90d8 100644 --- a/test/microwaveprop_web/live/user_profile_live_test.exs +++ b/test/microwaveprop_web/live/user_profile_live_test.exs @@ -26,9 +26,12 @@ defmodule MicrowavepropWeb.UserProfileLiveTest do user_id: user.id } + merged = Map.merge(defaults, attrs) + {user_id, changeset_attrs} = Map.pop(merged, :user_id) + {:ok, contact} = - %Contact{} - |> Contact.changeset(Map.merge(defaults, attrs)) + %Contact{user_id: user_id} + |> Contact.changeset(changeset_attrs) |> Repo.insert() contact