Restore TODO.md with remaining items at top, completed at bottom
This commit is contained in:
parent
9886c9171f
commit
1832a8ef04
1 changed files with 47 additions and 217 deletions
264
TODO.md
264
TODO.md
|
|
@ -1,35 +1,5 @@
|
|||
# Code Review Findings
|
||||
|
||||
## Critical Bugs
|
||||
|
||||
### ~~1. LiveView: Missing `{:noreply, socket}` in drain handler — crashes LiveView~~ FIXED
|
||||
|
||||
### ~~2. ConnectionMonitor: Deadlock via self-RPC every 30 seconds~~ FIXED
|
||||
**File:** `lib/aprsme/connection_monitor.ex:126-141`
|
||||
|
||||
~~`gather_cluster_stats/1` iterates `[Node.self() | Node.list()]` and calls `:rpc.call(node, __MODULE__, :get_stats, [])` for the local node. Since `get_stats/0` does a `GenServer.call(__MODULE__, :get_stats)`, and `gather_cluster_stats` is called from within `handle_info`, the GenServer is blocked waiting for the RPC which is waiting for the GenServer. Classic deadlock — times out every 30s.~~
|
||||
|
||||
**Fixed:** Now computes local stats directly from state instead of RPC to self. Remote nodes still use RPC.
|
||||
|
||||
### ~~3. LiveView: `get_callsign_key` generates random keys for Ecto structs — breaks all dedup~~ FIXED
|
||||
Now matches atom `:id`, string `"id"`, and falls back to `:sender`/`"sender"` before random key.
|
||||
|
||||
### ~~4. Cluster: `PacketDistributor` never calls `SpatialPubSub.broadcast_packet`~~ FIXED
|
||||
|
||||
### ~~5. LeaderElection: `pid_alive?/2` treats `{:badrpc, _}` as truthy~~ FIXED
|
||||
|
||||
### ~~5b. LeaderElection: Loser never detects lost registration after `:global` conflict resolution~~ FIXED
|
||||
**File:** `lib/aprsme/cluster/leader_election.ex`
|
||||
|
||||
When two pods start in isolation (before the Erlang cluster forms), both register as leader in their own `:global` namespace. When the cluster merges, `:global` calls `resolve_conflict/3` and silently unregisters the loser — but the loser's `check_leadership` handler only re-attempted election for non-leaders, so it stayed `is_leader: true` forever with an active APRS-IS connection. This caused both pods to connect to APRS-IS with the same callsign, creating a reconnect loop where they kicked each other off every 5 seconds.
|
||||
|
||||
**Fixed:** Added `verify_leadership/1` which checks `:global.whereis_name` on every `check_leadership` tick. If the registration no longer points to `self()`, the node steps down and notifies `ConnectionManager` to stop the APRS-IS connection.
|
||||
|
||||
### ~~6. SpatialPubSub: `ensure_float/1` crashes on integer strings~~ FIXED
|
||||
Now uses `Float.parse/1` which handles both `"42"` and `"42.5"`.
|
||||
|
||||
---
|
||||
|
||||
## Dual Packet State System (Architectural Bug)
|
||||
|
||||
### 7. LiveView maintains two unsynchronized packet tracking systems
|
||||
|
|
@ -46,211 +16,71 @@ These are never synchronized:
|
|||
|
||||
---
|
||||
|
||||
## Performance Issues
|
||||
|
||||
### 8. N+1 query: `has_weather_packets?` per marker
|
||||
**File:** `lib/aprsme_web/live/map_live/data_builder.ex:334`
|
||||
|
||||
Every non-weather marker on the map triggers an individual `Repo.exists?` query. With hundreds of visible markers, this is hundreds of DB round-trips per map load. Should batch-query weather callsigns upfront.
|
||||
|
||||
### ~~9. Duplicate packet transformation — every packet processed twice~~ FIXED
|
||||
Removed `extract_additional_data` and `normalize_data_type` from `Is.dispatch/1` — `PacketConsumer` already handles these.
|
||||
|
||||
### 10. `batch_length/1` recomputes O(n) list length on every event arrival
|
||||
**File:** `lib/aprsme/packet_consumer.ex:64-68, 130`
|
||||
|
||||
`batch_length(batch)` calls `length/1` (O(n)) on every `handle_events` call instead of tracking the count as an integer in state.
|
||||
|
||||
### 11. `check_batch_utilization` calls `length/1` four times in guards and body
|
||||
**File:** `lib/aprsme/packet_consumer.ex:174-180`
|
||||
|
||||
`length/1` in a guard traverses the entire list. Called again 3 more times in the body. For 1000-element batches, this is 4 full traversals just for a warning log.
|
||||
|
||||
### 12. `StreamingPacketsPubSub` does full ETS scan on every packet
|
||||
**File:** `lib/aprsme/streaming_packets_pubsub.ex:108-115`
|
||||
|
||||
`:ets.select` with no match conditions returns every subscriber for every packet. Geographic filtering happens afterward. With N subscribers and M packets/sec, this is O(N*M). A plain map in GenServer state would be equivalent.
|
||||
|
||||
### 13. `SpatialPubSub` receives every packet twice in non-cluster mode
|
||||
**File:** `lib/aprsme/spatial_pubsub.ex:66` and `lib/aprsme/packet_consumer.ex:378`
|
||||
|
||||
Receives packets from both `PacketConsumer.broadcast_single_packet` (direct cast) and `PostgresNotifier` (via PubSub). Duplicate processing and potential duplicate delivery.
|
||||
|
||||
### 14. `remove_markers_batch` sends one WebSocket event per marker
|
||||
**File:** `lib/aprsme_web/live/map_live/display_manager.ex:120-124`
|
||||
|
||||
Each marker removal is a separate `push_event`. For 100 markers, that's 100 separate WebSocket events. Should batch into a single `push_event("remove_markers_batch", %{ids: marker_ids})`.
|
||||
|
||||
### 15. `Encoding.clean_control_characters` allocates per grapheme for every string field
|
||||
**File:** `lib/aprsme/encoding.ex:147-153`
|
||||
|
||||
Decomposes into grapheme clusters, filters, joins — even when the string is clean ASCII (common case). A fast-path check (`String.printable?/1`) before decomposition would avoid allocation.
|
||||
|
||||
### 16. `truncate_datetimes_to_second` called twice per packet
|
||||
**File:** `lib/aprsme/packet_consumer.ex:278` and `lib/aprsme/packet_consumer.ex:426`
|
||||
|
||||
Every datetime traversed and pattern-matched twice through the same function.
|
||||
|
||||
### 17. Spatial queries don't use PostGIS GiST index
|
||||
**File:** `lib/aprsme/packets/query_builder.ex:139-149`
|
||||
|
||||
`within_bounds` filters on `p.lat`/`p.lon` B-tree columns instead of using `ST_Within` or `&&` operator on the `location` geometry column with a GiST index. Similarly `PreparedQueries` uses `ST_Y(location)` / `ST_X(location)` with BETWEEN, which also prevents spatial index use.
|
||||
|
||||
### 18. `weather_only` query uses 10-way OR — can't use indexes
|
||||
**File:** `lib/aprsme/packets/query_builder.ex:78-94`
|
||||
|
||||
10-way OR across 10 columns forces sequential scan. The `has_weather` boolean column exists on the Packet schema but is not used by this query.
|
||||
|
||||
### 19. `SpatialPubSub.get_intersecting_grid_cells` generates 341 cells for date-line-crossing viewports
|
||||
**File:** `lib/aprsme/spatial_pubsub.ex:284-295`
|
||||
|
||||
When west=170, east=-170, the range `170..-170` generates 341 cells instead of ~21. Functionally correct (filtered by `point_in_bounds?` later) but very wasteful.
|
||||
|
||||
### 20. Cleanup O(n^2) packet comparison in LiveView
|
||||
**File:** `lib/aprsme_web/live/map_live/index.ex:1756-1766`
|
||||
|
||||
`Enum.reject` with `Enum.any?` inside = O(n*m). Combined with bug #3 (random keys), this always marks all packets as expired.
|
||||
|
||||
---
|
||||
|
||||
## Memory Issues
|
||||
|
||||
### 21. ETS cache has no TTL — unbounded growth
|
||||
**File:** `lib/aprsme/cache.ex:20-27`
|
||||
|
||||
`Cache.put/4` accepts TTL opts but ignores them. `ttl/2` always returns `{:ok, nil}`. `:query_cache`, `:device_cache`, and `:symbol_cache` grow without bound.
|
||||
|
||||
### 22. Monitor reference leak in `StreamingPacketsPubSub`
|
||||
**File:** `lib/aprsme/streaming_packets_pubsub.ex:77`
|
||||
|
||||
Each `subscribe_to_bounds` creates a new monitor but the ref is never stored. `update_subscription_bounds` creates new monitors without demonitoring old ones. Accumulates dangling monitors per client session.
|
||||
|
||||
### 23. `all_packets` map in LiveView assigns holds 2000 full Ecto structs
|
||||
**File:** `lib/aprsme_web/live/map_live/packet_processor.ex:27-37`
|
||||
|
||||
Each packet has 100+ columns. 2000 full structs per connected client. Never cleaned by `handle_cleanup_old_packets` (which only touches `packet_state`).
|
||||
|
||||
### 24. `broadcast_async` uses `async_nolink` but never awaits — orphaned reply messages
|
||||
**File:** `lib/aprsme/broadcast_task_supervisor.ex:47-58`
|
||||
|
||||
`async_nolink` sends `{ref, result}` and `{:DOWN, ...}` messages to the calling GenServer. These pile up in the mailbox. Should use `start_child` (fire-and-forget) instead.
|
||||
|
||||
---
|
||||
|
||||
## Dead Code
|
||||
|
||||
### ~~25. `handle_info({:ssl, ...})` in Is module — connection uses `:gen_tcp`, not SSL~~ FIXED
|
||||
Removed dead SSL handler. Consolidated TCP handler into `handle_socket_data/2` with nil-safe timer cancel.
|
||||
|
||||
### 26. `PacketPipelineSetup` module — not in supervision tree, references unnamed consumers
|
||||
**File:** `lib/aprsme/packet_pipeline_setup.ex`
|
||||
|
||||
### 27. `Archiver` module — subscribes to PubSub but discards all messages
|
||||
**File:** `lib/aprsme/archiver.ex`
|
||||
|
||||
### 28. `SystemMonitor` — not in supervision tree, entirely inert
|
||||
**File:** `lib/aprsme/system_monitor.ex`
|
||||
|
||||
### 29. `DbOptimizer.copy_insert` — `COPY FROM STDIN` doesn't work through Postgrex
|
||||
**File:** `lib/aprsme/db_optimizer.ex:23-39`
|
||||
|
||||
Always fails and falls through to rescue which calls `regular_batch_insert`. Dead code path masked by rescue.
|
||||
|
||||
### 30. Router `/health` route — shadowed by endpoint HealthCheck plug
|
||||
**File:** `lib/aprsme_web/router.ex:56` vs `lib/aprsme_web/endpoint.ex:65`
|
||||
|
||||
### 31. `DeviceIdentification.enqueue_refresh_job/0` — commented-out implementation
|
||||
**File:** `lib/aprsme/device_identification.ex:192-194`
|
||||
|
||||
### 32. `RateLimiterWrapper.count/2` and `reset/1` — stubs returning hardcoded values
|
||||
**File:** `lib/aprsme/rate_limiter_wrapper.ex:16-29`
|
||||
|
||||
### 33. `pubsub_config/0` branches return identical values
|
||||
**File:** `lib/aprsme/application.ex:179-196`
|
||||
|
||||
Both `if cluster_enabled` and `else` return `{Phoenix.PubSub, name: Aprsme.PubSub}`.
|
||||
|
||||
### 34. Three no-op `send_heat_map_for_current_bounds` functions
|
||||
**Files:** `lib/aprsme_web/live/map_live/packet_processor.ex:147`, `historical_loader.ex:414`, `display_manager.ex:127`
|
||||
|
||||
Heat map never updates from real-time packets, historical loading at low zoom, or zoom threshold crossing.
|
||||
|
||||
### ~~35. `placeholders` option passed to `Repo.insert_all` — not a valid option~~ FIXED
|
||||
Removed from both `packet_consumer.ex` and `db_optimizer.ex`.
|
||||
Each packet has 100+ columns. 2000 full structs per connected client. Never cleaned by `handle_cleanup_old_packets` (which only touches `packet_state`). Appears to be dead state — written to but never read.
|
||||
|
||||
---
|
||||
|
||||
## Correctness Issues
|
||||
|
||||
### 36. Circuit breaker half-open state never persisted — allows unlimited concurrent probes
|
||||
**File:** `lib/aprsme/circuit_breaker.ex:89-93`
|
||||
|
||||
`calculate_current_state` returns `:half_open` but never writes it back to state. Multiple callers can all enter half-open simultaneously.
|
||||
|
||||
### ~~37. `Process.cancel_timer(nil)` crash risk in Is module~~ FIXED
|
||||
Consolidated into `handle_socket_data/2` with `if state.timer` guard.
|
||||
|
||||
### ~~38. `PacketPipelineSupervisor` uses `:one_for_one` — producer restart orphans consumers~~ FIXED
|
||||
Changed to `:rest_for_one` so consumer pool restarts when producer restarts.
|
||||
|
||||
### 39. `handle_update_time_display` doesn't trigger re-renders
|
||||
**File:** `lib/aprsme_web/live/map_live/index.ex:1774-1781`
|
||||
|
||||
Returns `{:noreply, socket}` with unchanged socket — LiveView diff is empty. Time-ago displays never update.
|
||||
|
||||
### 40. `PacketStore` uses global named ETS table shared across all LiveViews
|
||||
**File:** `lib/aprsme_web/live/map_live/packet_store.ex:12, 114`
|
||||
|
||||
Different LiveViews can overwrite each other's stored packets. TTL cleanup runs globally.
|
||||
|
||||
### ~~41. ConnectionMonitor memory calculation is inverted~~ FIXED
|
||||
Now computes `processes / total` — process memory as fraction of total VM memory.
|
||||
|
||||
### 42. `Callsign.valid?/1` accepts any non-empty string — docstring examples are wrong
|
||||
**File:** `lib/aprsme/callsign.ex:40-45`
|
||||
|
||||
Docstring says `valid?("A")` returns false, but implementation returns true.
|
||||
|
||||
### 43. `Encoding.latin1_to_utf8` and `clean_control_characters` fight each other
|
||||
**File:** `lib/aprsme/encoding.ex:126-170`
|
||||
|
||||
`latin1_to_utf8` converts bytes 128-159 to Unicode U+0080-U+009F, then `clean_control_characters` immediately strips them.
|
||||
|
||||
### 44. `DeviceCache` has unreachable outer `try/rescue`
|
||||
**File:** `lib/aprsme/device_cache.ex:109-132`
|
||||
|
||||
Inner rescue catches all exceptions (including the ones the outer rescue targets). Outer rescue for `Postgrex.Error`/`DBConnection.ConnectionError` is dead code.
|
||||
|
||||
---
|
||||
|
||||
## Security / Operational
|
||||
|
||||
### 45. LiveDashboard and ErrorTracker are publicly accessible
|
||||
**File:** `lib/aprsme_web/router.ex:40-43`
|
||||
|
||||
`/dashboard` and `/errors` are in a scope with only the `:browser` pipeline — no auth required. Exposes system metrics, process info, ETS tables, and stack traces.
|
||||
## Completed
|
||||
|
||||
### 1. LiveView: Missing `{:noreply, socket}` in drain handler — crashes LiveView
|
||||
### 2. ConnectionMonitor: Deadlock via self-RPC every 30 seconds
|
||||
### 3. LiveView: `get_callsign_key` generates random keys for Ecto structs — breaks all dedup
|
||||
### 4. Cluster: `PacketDistributor` never calls `SpatialPubSub.broadcast_packet`
|
||||
### 5. LeaderElection: `pid_alive?/2` treats `{:badrpc, _}` as truthy
|
||||
### 5b. LeaderElection: Loser never detects lost registration after `:global` conflict resolution
|
||||
### 6. SpatialPubSub: `ensure_float/1` crashes on integer strings
|
||||
### 8. N+1 query: `has_weather_packets?` per marker — batch query replaces N+1 individual calls
|
||||
### 9. Duplicate packet transformation — every packet processed twice
|
||||
### 10. `batch_length/1` recomputes O(n) list length on every event arrival
|
||||
### 11. `check_batch_utilization` calls `length/1` four times in guards and body
|
||||
### 12. `StreamingPacketsPubSub` does full ETS scan on every packet — replaced with `tab2list`
|
||||
### 13. `SpatialPubSub` receives every packet twice in non-cluster mode — removed PubSub subscription
|
||||
### 14. `remove_markers_batch` sends one WebSocket event per marker
|
||||
### 15. `Encoding.clean_control_characters` allocates per grapheme for every string field — added fast-path
|
||||
### 16. `truncate_datetimes_to_second` called twice per packet
|
||||
### 17. Spatial queries don't use PostGIS GiST index — rewritten to use ST_MakeEnvelope with && operator
|
||||
### 18. `weather_only` query uses 10-way OR — now uses indexed `has_weather` column
|
||||
### 19. `SpatialPubSub.get_intersecting_grid_cells` generates 341 cells for date-line-crossing viewports
|
||||
### 20. Cleanup O(n^2) packet comparison in LiveView — fixed with MapSet
|
||||
### 21. ETS cache has no TTL — added TTL with lazy expiration
|
||||
### 22. Monitor reference leak in `StreamingPacketsPubSub`
|
||||
### 24. `broadcast_async` uses `async_nolink` but never awaits — orphaned reply messages
|
||||
### 25. `handle_info({:ssl, ...})` in Is module — connection uses `:gen_tcp`, not SSL
|
||||
### 26. `PacketPipelineSetup` module — deleted
|
||||
### 27. `Archiver` module — deleted
|
||||
### 28. `SystemMonitor` — deleted
|
||||
### 29. `DbOptimizer.copy_insert` — `COPY FROM STDIN` doesn't work through Postgrex
|
||||
### 30. Router `/health` route — shadowed by endpoint HealthCheck plug
|
||||
### 31. `DeviceIdentification.enqueue_refresh_job/0` — removed commented-out stub
|
||||
### 32. `RateLimiterWrapper.count/2` and `reset/1` — removed dead stubs
|
||||
### 33. `pubsub_config/0` branches return identical values — simplified
|
||||
### 34. Three no-op `send_heat_map_for_current_bounds` functions — wired up to DisplayManager
|
||||
### 35. `placeholders` option passed to `Repo.insert_all` — not a valid option
|
||||
### 36. Circuit breaker half-open state never persisted
|
||||
### 37. `Process.cancel_timer(nil)` crash risk in Is module
|
||||
### 38. `PacketPipelineSupervisor` uses `:one_for_one` — changed to `:rest_for_one`
|
||||
### 39. `handle_update_time_display` doesn't trigger re-renders — now touches assign
|
||||
### 41. ConnectionMonitor memory calculation is inverted
|
||||
### 42. `Callsign.valid?/1` accepts any non-empty string — docstring examples are wrong
|
||||
### 43. `Encoding.latin1_to_utf8` and `clean_control_characters` fight each other — fixed C1 range
|
||||
### 44. `DeviceCache` has unreachable outer `try/rescue` — removed
|
||||
### 45. LiveDashboard and ErrorTracker are publicly accessible — moved behind auth
|
||||
### 46. SQL injection in `DbOptimizer` table/column name interpolation
|
||||
**File:** `lib/aprsme/db_optimizer.ex:29, 107, 126`
|
||||
|
||||
`table_name` and `columns` interpolated directly into SQL. Internal-only callers today, but the pattern is dangerous.
|
||||
|
||||
### 47. `/ready` and `/status.json` are rate-limited — risk of false health check failures
|
||||
**File:** `lib/aprsme_web/router.ex:54-59`
|
||||
|
||||
Piped through `:public_api` which includes `RateLimiter` at 100 req/min. Kubernetes probes hitting from same IP could get rate-limited.
|
||||
|
||||
### 47. `/ready` and `/status.json` are rate-limited — moved to separate unrated pipeline
|
||||
### 48. `ShutdownHandler.terminate` blocks 30s but scheduled messages never processed
|
||||
**File:** `lib/aprsme/shutdown_handler.ex:69-92`
|
||||
|
||||
`Process.send_after` in terminate is pointless — no more messages processed after terminate. The 30s sleep just delays shutdown.
|
||||
|
||||
### 49. DatabaseMetrics hardcodes pool metrics
|
||||
**File:** `lib/aprsme/telemetry/database_metrics.ex:77-85`
|
||||
|
||||
Reports `busy: 2` and `idle: pool_size - 2` as actual telemetry. Misleads monitoring dashboards.
|
||||
|
||||
### 50. `PacketBatcher` started outside supervision tree
|
||||
**File:** `lib/aprsme_web/live/map_live/index.ex:202`
|
||||
|
||||
If the batcher crashes, it won't restart. LiveView continues running but silently stops processing real-time packets.
|
||||
### 50. `PacketBatcher` started outside supervision tree — added crash recovery with Process.monitor
|
||||
|
|
|
|||
Loading…
Add table
Reference in a new issue