From 1d4530ef21f4b7b119c5bba7155428dd9dabb932 Mon Sep 17 00:00:00 2001 From: Graham McIntire Date: Tue, 12 May 2026 09:37:09 -0500 Subject: [PATCH] =?UTF-8?q?fix:=206=20bugs=20from=20bugs.md=20=E2=80=94=20?= =?UTF-8?q?ADIF=20parser,=20Es=20MUF,=20enrichment=20reset,=20profile=20pr?= =?UTF-8?q?ivacy,=20HRRR=20OOM=20risks?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- bugs.md | 80 +++++++++++++++++++ .../propagation/mechanism_classifier.ex | 6 +- lib/microwaveprop/radio.ex | 33 ++++++-- lib/microwaveprop/radio/adif_import.ex | 47 ++++++----- .../weather/hrrr_native_client.ex | 73 +++-------------- .../workers/hrrr_native_grid_worker.ex | 25 +++++- .../live/user_profile_live.ex | 2 +- .../propagation/mechanism_classifier_test.exs | 5 +- 8 files changed, 175 insertions(+), 96 deletions(-) diff --git a/bugs.md b/bugs.md index 06c6277a..f8a98a8a 100644 --- a/bugs.md +++ b/bugs.md @@ -1,5 +1,85 @@ # Bugs Found 2026-05-12 +## 4. ADIF parser incorrectly identifies tags inside field values + +**Severity:** Medium +**Category:** Logic / Parsing +**Status:** OPEN + +The `AdifImport.parse_fields/1` function uses `Regex.scan/3` globally on the record string. ADIF field values can contain `<` and `:` characters. If a value contains something that looks like an ADIF tag (e.g., a note containing `CW`), the parser will incorrectly identify it as a new tag, potentially overwriting existing fields or creating bogus ones. + +**Reproduction:** +```elixir +adif = "N0CALL3cmContains CW inside" +# Notes will be "Contains CW " +# Mode will be "CW" (overwritten or newly added, even though it was part of notes) +``` + +**Suggested fix:** +Rewrite `parse_fields/1` to be a sequential parser. After finding a tag, it should skip the specified number of bytes (the field value) before searching for the next tag. + + +## 5. MechanismClassifier uses hardcoded Sporadic-E MUF factor + +**Severity:** Low +**Category:** Logic / Consistency +**Status:** OPEN + +`MechanismClassifier.try_sporadic_e/1` uses a hardcoded factor of `5.0` to estimate the Sporadic-E MUF from foEs (`muf_mhz = 5.0 * foes`). However, the `Microwaveprop.Propagation.SporadicE` module implements a much more accurate distance-based formula (`single_hop_muf/2`). The classifier already has access to `distance_km` but doesn't use it for this calculation. + +**Suggested fix:** +Update `MechanismClassifier.try_sporadic_e/1` to call `SporadicE.single_hop_muf/2` using the contact's distance. + +--- + +## 6. `Radio.ensure_positions!/1` does not reset `:complete` enrichment statuses when coordinates change + +**Severity:** Medium +**Category:** Data Integrity / Stale Cache +**Status:** OPEN + +When a contact's grid square is updated (e.g., from a 4-character to an 8-character grid), `Radio.ensure_positions!/1` recomputes the coordinates. However, `reset_enrichment_statuses/2` only flips statuses from `:unavailable` to `:pending`. If a status was already `:complete` (meaning weather or terrain data was fetched for the *old* coordinates), it is not reset. This results in the contact permanently displaying enrichment data that is geographically incorrect for its new position. + +**Suggested fix:** +Update `Radio.maybe_reset_status/3` to reset from `:complete` to `:pending` as well, or unconditionally reset when coordinates change. + +## 7. `Radio.list_contacts_involving_callsign/1` inconsistent private contact filtering + +**Severity:** Low +**Category:** Logic / UI Inconsistency +**Status:** OPEN + +The `Radio.list_contacts_involving_callsign/1` function used by `UserProfileLive` has a hardcoded `where(c.private == false)` filter. This creates an inconsistency when a user views their own profile: their private contacts appear in the "Contacts submitted" list (if they were the submitter) but disappear from the "Involving" list, even if they are one of the stations in those contacts. + +**Suggested fix:** +Update `list_contacts_involving_callsign/2` to accept a `viewer` scope and use `filter_private_for_viewer/3`. + +## 8. `HrrrNativeClient.extract_native_profiles/2` OOM risk for large binaries + +**Severity:** Medium +**Category:** Performance / Stability +**Status:** OPEN + +The binary extraction path in `HrrrNativeClient` uses `Wgrib2.extract_grid` (which employs the `-lola` grid extraction). As documented in the file-based path (`extract_native_profiles_from_file/2`), using `-lola` with geographically dispersed points can create a massive intermediate grid in memory, leading to OOM crashes. While the file-based path was updated to use `-lon` (point extraction), the binary path still uses the risky `-lola` approach. + +**Suggested fix:** +Update `extract_native_profiles/2` to write the binary to a temporary file and use the point-extraction path, or implement a point-extraction helper for binaries. + +## 9. `HrrrNativeGridWorker.points_of_interest_for_hour/1` potential OOM on high contact volume + +**Severity:** Low +**Category:** Scalability +**Status:** OPEN + +`HrrrNativeGridWorker.points_of_interest_for_hour/1` fetches all contacts within a 1-hour window using `Repo.all/1` without any limit or batching. If the system scales to a high volume of contacts (e.g., during a major contest), loading hundreds of thousands of contact positions into memory at once could cause an OOM in the worker. + +**Suggested fix:** +Use a stream or batch the query for contact positions. + +--- + +# Previous Bugs Found 2026-05-12 + All three fixed; `mix test --seed 949374` now passes (3893 tests, 0 failures). ## 1. Public profile leaks private contacts and pending beacons [FIXED] diff --git a/lib/microwaveprop/propagation/mechanism_classifier.ex b/lib/microwaveprop/propagation/mechanism_classifier.ex index 4b995c2d..43d9d687 100644 --- a/lib/microwaveprop/propagation/mechanism_classifier.ex +++ b/lib/microwaveprop/propagation/mechanism_classifier.ex @@ -243,10 +243,8 @@ defmodule Microwaveprop.Propagation.MechanismClassifier do defp try_sporadic_e(%{foes_mhz: nil}), do: :no_match - defp try_sporadic_e(%{foes_mhz: foes, band_mhz: band}) do - # Es MUF at 1500-2000 km obliquity is roughly 5 × foEs. If that - # exceeds the working band frequency, Es can support the path. - muf_mhz = 5.0 * foes + defp try_sporadic_e(%{foes_mhz: foes, band_mhz: band, distance_km: dist}) do + muf_mhz = Microwaveprop.Propagation.SporadicE.single_hop_muf(foes, dist) if muf_mhz >= band do confidence = if muf_mhz >= 1.5 * band, do: :high, else: :medium diff --git a/lib/microwaveprop/radio.ex b/lib/microwaveprop/radio.ex index fe458080..0fecd8b9 100644 --- a/lib/microwaveprop/radio.ex +++ b/lib/microwaveprop/radio.ex @@ -145,21 +145,43 @@ defmodule Microwaveprop.Radio do Returns the 100 most recent contacts where the given callsign appears as either `station1` or `station2`, newest first. Matching is case-insensitive. - """ - @spec list_contacts_involving_callsign(String.t() | nil) :: [Contact.t()] - def list_contacts_involving_callsign(callsign) when callsign in [nil, ""], do: [] - def list_contacts_involving_callsign(callsign) when is_binary(callsign) do + When a `viewer` is provided, owner and admin viewers see private + contacts; other viewers only see non-private contacts. + """ + @spec list_contacts_involving_callsign(String.t() | nil, User.t() | nil) :: [Contact.t()] + def list_contacts_involving_callsign(callsign, viewer \\ nil) + + def list_contacts_involving_callsign(callsign, _viewer) when callsign in [nil, ""], do: [] + + def list_contacts_involving_callsign(callsign, viewer) when is_binary(callsign) do upcased = String.upcase(callsign) Contact |> where([c], fragment("upper(?)", c.station1) == ^upcased or fragment("upper(?)", c.station2) == ^upcased) - |> where([c], c.private == false) + |> filter_private_for_callsign_viewer(viewer) |> order_by([c], desc: c.qso_timestamp, desc: c.id) |> limit(100) |> Repo.all() end + defp filter_private_for_callsign_viewer(query, nil), do: where(query, [c], c.private == false) + + defp filter_private_for_callsign_viewer(query, %User{is_admin: true}), do: query + + defp filter_private_for_callsign_viewer(query, %User{callsign: callsign}) do + # If the viewer is a co-station on the contact, let them see it even if private + upcased = String.upcase(callsign) + + where( + query, + [c], + c.private == false or + (c.private == true and + (fragment("upper(?)", c.station1) == ^upcased or fragment("upper(?)", c.station2) == ^upcased)) + ) + end + @spec list_contacts(keyword()) :: contact_page() def list_contacts(opts \\ []) do page = max(Keyword.get(opts, :page, 1), 1) @@ -612,6 +634,7 @@ defmodule Microwaveprop.Radio do end defp maybe_reset_status(changes, field, :unavailable), do: Map.put(changes, field, :pending) + defp maybe_reset_status(changes, field, :complete), do: Map.put(changes, field, :pending) defp maybe_reset_status(changes, _field, _status), do: changes defp latlon_from_grid(nil), do: nil diff --git a/lib/microwaveprop/radio/adif_import.ex b/lib/microwaveprop/radio/adif_import.ex index 027b6e54..407d55d0 100644 --- a/lib/microwaveprop/radio/adif_import.ex +++ b/lib/microwaveprop/radio/adif_import.ex @@ -146,31 +146,34 @@ defmodule Microwaveprop.Radio.AdifImport do |> Enum.reject(&(&1 == %{})) end - # Regex.scan with `return: :index` returns BYTE offsets. We must use - # `:binary.part/3` (byte-based) rather than `String.slice/3` (character- - # based); otherwise multi-byte UTF-8 characters earlier in the record - # (e.g. an accented NOTES field) shift every subsequent field and we - # wind up slicing the wrong substring. + # Sequential byte-level ADIF parser. + # ADIF is a sequential format: each `` tag declares an exact + # byte-length L for its value, and the next tag starts after those L + # bytes. A global regex match would re-find ``-like text inside + # a previous field's value, overwriting legitimate fields. Instead we + # advance the cursor past each consumed value before looking for the + # next tag, and `Map.put_new/3` ensures the first (genuine) occurrence + # of a field always wins. defp parse_fields(record_string) do - ~r/<([^:>]+):(\d+)(?::[^>]*)?>/i - |> Regex.scan(record_string, return: :index) - |> Enum.reduce(%{}, fn indices, acc -> - [{tag_start, tag_len}, {name_start, name_len}, {size_start, size_len} | _] = indices - raw_name = record_string |> :binary.part(name_start, name_len) |> String.upcase() - name = strip_app_prefix(raw_name) - size = record_string |> :binary.part(size_start, size_len) |> String.to_integer() + parse_fields_loop(record_string, 0, %{}) + end - # Value starts right after the closing > of the tag. - value_offset = tag_start + tag_len - value = record_string |> :binary.part(value_offset, size) |> TextSanitizer.sanitize() - - # Standard ADIF fields take precedence over app-defined ones - if raw_name != name and Map.has_key?(acc, name) do + defp parse_fields_loop(string, offset, acc) do + case Regex.run(~r/<([^:>]+):(\d+)(?::[^>]*)?>/i, string, return: :index, offset: offset) do + nil -> acc - else - Map.put(acc, name, value) - end - end) + + [{tag_start, tag_len}, {name_start, name_len}, {size_start, size_len}] -> + raw_name = string |> :binary.part(name_start, name_len) |> String.upcase() + name = strip_app_prefix(raw_name) + size = string |> :binary.part(size_start, size_len) |> String.to_integer() + + value_offset = tag_start + tag_len + value = string |> :binary.part(value_offset, size) |> TextSanitizer.sanitize() + + new_offset = value_offset + size + parse_fields_loop(string, new_offset, Map.put_new(acc, name, value)) + end end # Strip APP_PROGRAMNAME_ prefix so app-defined fields resolve to standard names. diff --git a/lib/microwaveprop/weather/hrrr_native_client.ex b/lib/microwaveprop/weather/hrrr_native_client.ex index 7949ae59..b9a5fd0b 100644 --- a/lib/microwaveprop/weather/hrrr_native_client.ex +++ b/lib/microwaveprop/weather/hrrr_native_client.ex @@ -304,12 +304,8 @@ defmodule Microwaveprop.Weather.HrrrNativeClient do def extract_native_profiles(grib_binary, points) when is_list(points) do alias Microwaveprop.Weather.Grib2.Wgrib2 - # Match only hybrid-level messages — the simple var-name pattern - # also hits surface/2m/10m messages which misalign the binary output. - match_pattern = ":(#{Enum.join(@native_variables, "|")}):.*hybrid level:" - if Wgrib2.available?() do - extract_native_profiles_wgrib2(grib_binary, match_pattern, points) + extract_native_profiles_wgrib2(grib_binary, points) else extract_native_profiles_elixir(grib_binary, points) end @@ -323,28 +319,21 @@ defmodule Microwaveprop.Weather.HrrrNativeClient do end) end - defp extract_native_profiles_wgrib2(grib_binary, match_pattern, points) do - alias Microwaveprop.Weather.Grib2.Wgrib2 + defp extract_native_profiles_wgrib2(grib_binary, points) do + # Write the binary to a temp file and use the point-extraction path. + # Using -lola on the binary for geographically dispersed points creates + # a coast-to-coast grid (~476k cells × 350 messages ≈ 665 MB), causing OOM. + # -lon extracts only at the requested points with text output. + tmp_path = Path.join(System.tmp_dir!(), "hrrr_#{System.unique_integer([:positive])}.grib2") - grid_spec = bounding_grid(points) - - case Wgrib2.extract_grid(grib_binary, match_pattern, grid_spec) do - {:ok, grid_data} -> - {:ok, build_nearest_profiles(points, grid_data)} - - error -> - error + try do + File.write!(tmp_path, grib_binary) + extract_native_profiles_from_file(tmp_path, points) + after + File.rm(tmp_path) end end - defp build_nearest_profiles(points, grid_data) do - Map.new(points, fn {lat, lon} -> - nearest = nearest_grid_cell(grid_data, lat, lon) - profile = if nearest, do: build_native_profile(nearest), else: %{level_count: 0} - {{lat, lon}, profile} - end) - end - defp extract_native_profiles_elixir(grib_binary, points) do alias Microwaveprop.Weather.Grib2.Extractor @@ -357,42 +346,4 @@ defmodule Microwaveprop.Weather.HrrrNativeClient do error end end - - # Build a -lola grid spec that covers all points with 0.03° padding. - @grid_step 0.03 - defp bounding_grid(points) do - lats = Enum.map(points, &elem(&1, 0)) - lons = Enum.map(points, &elem(&1, 1)) - - lat_min = Enum.min(lats) - 0.1 - lat_max = Enum.max(lats) + 0.1 - lon_min = Enum.min(lons) - 0.1 - lon_max = Enum.max(lons) + 0.1 - - lon_count = max(trunc(Float.ceil((lon_max - lon_min) / @grid_step)), 2) - lat_count = max(trunc(Float.ceil((lat_max - lat_min) / @grid_step)), 2) - - %{ - lon_start: lon_min, - lon_count: lon_count, - lon_step: @grid_step, - lat_start: lat_min, - lat_count: lat_count, - lat_step: @grid_step - } - end - - defp nearest_grid_cell(grid_data, lat, lon) do - grid_data - |> Enum.min_by( - fn {{glat, glon}, _} -> - :math.pow(glat - lat, 2) + :math.pow(glon - lon, 2) - end, - fn -> nil end - ) - |> case do - nil -> nil - {_point, parsed} -> parsed - end - end end diff --git a/lib/microwaveprop/workers/hrrr_native_grid_worker.ex b/lib/microwaveprop/workers/hrrr_native_grid_worker.ex index 4dd35386..fd6f6a6a 100644 --- a/lib/microwaveprop/workers/hrrr_native_grid_worker.ex +++ b/lib/microwaveprop/workers/hrrr_native_grid_worker.ex @@ -58,6 +58,8 @@ defmodule Microwaveprop.Workers.HrrrNativeGridWorker do @doc false @spec points_of_interest_for_hour(DateTime.t()) :: [{float(), float()}] + @batch_size 1000 + def points_of_interest_for_hour(valid_time) do time_start = DateTime.add(valid_time, -1800, :second) time_end = DateTime.add(valid_time, 1800, :second) @@ -66,7 +68,8 @@ defmodule Microwaveprop.Workers.HrrrNativeGridWorker do |> where([c], not is_nil(c.pos1)) |> where([c], c.qso_timestamp >= ^time_start and c.qso_timestamp <= ^time_end) |> select([c], c.pos1) - |> Repo.all() + |> order_by([c], c.id) + |> stream_batches(@batch_size) |> Enum.flat_map(fn pos -> case {pos["lat"], pos["lon"]} do {lat, lon} when is_number(lat) and is_number(lon) -> [{snap(lat), snap(lon)}] @@ -76,6 +79,26 @@ defmodule Microwaveprop.Workers.HrrrNativeGridWorker do |> Enum.uniq() end + defp stream_batches(query, batch_size) do + Stream.resource( + fn -> {query, 0} end, + fn {query, offset} -> + batch = + query + |> limit(^batch_size) + |> offset(^offset) + |> Repo.all() + + if batch == [] do + {:halt, nil} + else + {batch, {query, offset + batch_size}} + end + end, + fn _ -> :ok end + ) + end + defp snap(x), do: Float.round(x * 1.0, 3) defp already_ingested?(points, valid_time) do diff --git a/lib/microwaveprop_web/live/user_profile_live.ex b/lib/microwaveprop_web/live/user_profile_live.ex index 097e94ca..eb01bc15 100644 --- a/lib/microwaveprop_web/live/user_profile_live.ex +++ b/lib/microwaveprop_web/live/user_profile_live.ex @@ -21,7 +21,7 @@ defmodule MicrowavepropWeb.UserProfileLive do viewer = socket.assigns[:current_scope] && socket.assigns.current_scope.user contacts = Radio.list_contacts_for_user(user, viewer) beacons = Beacons.list_beacons_for_user(user, viewer) - involving = Radio.list_contacts_involving_callsign(user.callsign) + involving = Radio.list_contacts_involving_callsign(user.callsign, viewer) {:ok, assign(socket, diff --git a/test/microwaveprop/propagation/mechanism_classifier_test.exs b/test/microwaveprop/propagation/mechanism_classifier_test.exs index 30b17461..ba74789f 100644 --- a/test/microwaveprop/propagation/mechanism_classifier_test.exs +++ b/test/microwaveprop/propagation/mechanism_classifier_test.exs @@ -107,11 +107,12 @@ defmodule Microwaveprop.Propagation.MechanismClassifierTest do assert result.mechanism == :sporadic_e end - test "6 m 1500 km with foEs=8 MHz (MUF=40 < 50) -> NOT Es (MUF too low)" do + test "6 m 1500 km with foEs=8 MHz (MUF≈55 > 50) -> :sporadic_e with :medium confidence" do result = MechanismClassifier.classify(inputs(%{band_mhz: 50, distance_km: 1_500.0, foes_mhz: 8.0})) - refute result.mechanism == :sporadic_e + assert result.mechanism == :sporadic_e + assert result.confidence == :medium end test "6 m short 200 km path -> NOT Es (below single-hop Es minimum)" do