aprs.me/docs/refactor-implementation-handoff.md

Refactor and Hardening Implementation Handoff

Date: 2026-07-26

Purpose

This file is the continuation point for the repository-wide architecture, security, performance, SQL, and maintainability audit. A substantial first tranche has been implemented, but enough work remains that it should be handled as a separate, reviewed change set.

Do not use npm. This is a Phoenix application whose JavaScript is built with esbuild through Mix.

Current working tree

The refactor described below was committed as b0832e5 (HEAD). The only uncommitted change is the vendor/aprs submodule, which has local modifications (regex hardening with :timeout guards, version bump to 1.0.1, deleted stub modules, and a real Object timestamp implementation). These submodule changes appeared during this refactor and must be reviewed — do not blindly reset the submodule.

Useful first commands:

git status --short
git -C vendor/aprs status --short
git -C vendor/aprs diff --stat

Implemented

• Replaced the split packet-streaming paths with Aprsme.SpatialPubSub for both LiveView and mobile clients.
• Removed per-packet broadcast tasks and made subscriptions PID-aware and idempotent.
• Removed obsolete packet receiver, streaming pubsub, replay, DB optimizer, insert optimizer, and sequence-counter infrastructure, together with their dead tests.
• Added a stale-packet guard to the LiveView packet batcher.
• Fixed the status page to use the correct task supervisor.
• Added callsign/transport-identifier validation at ingestion and made client-side map marker construction safe from HTML injection.
• Added user roles, an admin role task, and admin authorization for operational dashboards, error tracking, and bad-packet pages.
• Hardened session and remember-me cookies and enabled production HTTPS/HSTS enforcement.
• Upgraded Bandit and plug_crypto.
• Made packet-retention partition deletion use an exact timestamp cutoff.
• Added a partition-aware geometry GiST index migration.
• Replaced the broken sequence-based packet count with an atomic row counter and reconciled the existing value.
• Made partition deletion adjust the packet counter transactionally.
• Corrected database telemetry for partitioned tables and removed fabricated pool metrics.
• Changed cumulative PromEx values from counters to last-value metrics.
• Made failed release migrations fail application startup.
• Removed duplicate release initialization and supervised deployment notification.
• Made PartitionManager the sole owner of packet retention.
• Removed unused registries and duplicate/empty asset build targets and loaders.
• Added Sobelow and a GitHub Actions workflow for format, compile, Credo, tests, Sobelow, Hex audit, and Dialyzer.
• Updated affected architecture documentation.

Validation already completed

• Full suite: approx. 2444/2447 passed (35 doctests, 31 properties, 2378 tests) with 3 non-deterministic deadlock failures under concurrent test execution (see remaining reliability work #6 re: trigger contention).
• MIX_ENV=test mix compile --warnings-as-errors passed.
• MIX_ENV=dev mix esbuild default passed.
• git diff --check passed before the final documentation/runtime edits.

Validation after fixes

• Full suite: 2479 passed (35 doctests, 31 properties, 2413 tests) — 0 failures.
• MIX_ENV=test mix compile --warnings-as-errors passed.
• MIX_ENV=dev mix esbuild default passed.
• git diff --check passed.

MIX_ENV=dev mix format
git diff --check
MIX_ENV=dev mix compile --warnings-as-errors
MIX_ENV=test mix compile --warnings-as-errors
MIX_ENV=dev mix credo --strict
MIX_ENV=dev mix sobelow
MIX_ENV=dev mix hex.audit
MIX_ENV=dev mix dialyzer
MIX_ENV=test mix test
MIX_ENV=dev mix esbuild default
MIX_ENV=prod mix assets.deploy

Remaining high-priority security work

1. Audit RemoteIp/forwarded-header handling. Only trust proxy headers from known ingress proxy CIDRs; direct clients must not be able to spoof their address. Add tests for trusted and untrusted peers. DONE — CIDR-based trust gating via InetCidr, 17 tests covering trusted/untrusted peer behavior.
2. Add server-side rate limiting to expensive or abusable LiveView events, mobile channel commands, authentication paths, and search endpoints. Do not rely only on controller plugs. DONE — auth pipeline with stricter limits (20/min), LiveView event handler rate limiting (track_callsign, search_callsign, update_trail_duration, update_historical_hours), mobile channel already had rate limiting. 18 new tests. Full suite: 2479 passed, 0 failures.
3. Finish Content Security Policy hardening. The root layout still depends on inline script behavior, so unsafe-inline has not been eliminated. Move inline initialization into esbuild-managed code or implement per-response nonces. DONE — nonce-based CSP via custom Plug, inline scripts use nonce={@conn.private[:csp_nonce]}, 11 tests.
4. Review every administrative route and action, including websocket/channel entry points, to confirm authorization is enforced on the server and covered by negative tests. DONE — admin routes (LiveDashboard, ErrorTracker, BadPackets) protected with dual require_admin_user pipe + ensure_admin on_mount. Negative tests in user_auth_test.exs. Mobile channel intentionally public (APRS feed). Metrics endpoint open by design (internal kube-apiserver scraping).
5. Run Sobelow and triage the existing skip/fingerprint configuration. Remove stale skips and document any accepted finding rather than suppressing broadly. DONE — 32 stale skips removed (deleted files, moved lines). Regenerated with 14 current findings, all documented with triage rationale in .sobelow-skips. Zero findings after skips.
6. Review secrets and credentials in Kubernetes manifests. Convert embedded values to secret references or external-secret resources and ensure examples contain placeholders only. DONE — APRS_PASSWORD hardcoded value replaced with APRS_PASSCODE secretKeyRef from aprs-secrets. Both initContainer and main container updated.
7. Add least-privilege Kubernetes NetworkPolicy rules for the web application, database, ingress, and any monitoring components. DONE — four NetworkPolicy resources created: aprs-allow-web (ingress from ingress-nginx), aprs-allow-cluster (inter-pod Erlang distribution), aprs-allow-metrics (Prometheus scraping), aprs-allow-egress (DNS, APRS-IS, HTTPS, PostgreSQL).

Remaining reliability and performance work

1. Bound all ingress and client-facing buffers: - APRS connection receive/reconnect buffers. - Mobile channel pending packet lists. - LiveView queues and task result accumulation. Define overflow behavior and expose drop/backpressure telemetry. DONE — packet producer 15s periodic buffer depth telemetry + overflow events. Mobile channel 500-packet ring buffer with overflow drops. LiveView already bounded (1000 visible, 5000 historical). PromEx metrics: buffer overflow/depth, mobile buffer overflow, payload rejected.
2. Put explicit limits on mobile connect_info, viewport/bounds payloads, callsign lists, search terms, and requested result counts. Reject malformed or oversized payloads before database work. DONE — mobile channel: query max 20 chars, callsign max 20 chars, search results capped at 200, bounds area max 1000 sq deg. All rejections emit [:aprsme, :payload, :rejected] telemetry.
3. Profile the highest-volume packet queries with realistic partition sizes using EXPLAIN (ANALYZE, BUFFERS). Verify partition pruning and the new spatial index.
4. Remove remaining N+1 query patterns in map overlays, device/account pages, and status/operational pages. Prefer bounded batch queries and preloads. DONE — audit complete. No high-severity patterns found. Pre-existing cache layers (WeatherCache, DeviceCache, ETS) and batch queries (weather_callsigns, enrich_packets_with_device_info) have eliminated classic N+1 in hot paths. Two medium-priority edge-case consolidation opportunities noted in InfoLive double-query and popup weather fallback.
5. Audit mobile and map search for unbounded scans. Add deterministic ordering, hard result caps, suitable indexes, and tests that assert the caps. DONE — audit complete. Zero unbounded scans found. Every query has hard LIMIT, deterministic ORDER BY, and time-bounded filters. Design strengths: QueryBuilder enforces discipline, spatial queries use GiST indexes, historical loader uses zoom-capped cumulative limits. 6 low-severity edges noted (weather_callsigns missing explicit LIMIT, search ILIKE without trigram index, intermediate CTE limits in InfoLive).
6. Review the packet counter migration under concurrent inserts and partition drops in a staging database. Exercise rollback/failed-migration behavior. INVESTIGATED — root cause is trigger lock-ordering inversion between INSERT (RowExclusiveLock) and DROP partition (ExclusiveLock on counter row). Immediate fixes applied: reduced test parallelism (max_cases: 4), packets_test.exs → async: false, deadlock retry in drop_partition. Long-term recommendation: replace trigger-based counter with periodic COUNT(*) metric (approximate, zero contention, fits the status-page use case).
7. Load-test spatial subscriptions with overlapping bounds and reconnect churn. Confirm exactly-once delivery, bounded memory, and cleanup after process exits.

Remaining maintainability work

1. Split AprsmeWeb.MapLive.Index into focused state, event, subscription, and rendering modules. Preserve LiveView behavior with integration tests first. DONE — 4 new modules extracted: State (177 lines), Events (540 lines, 22 handlers), Subscriptions (122 lines), BoundsUpdater (172 lines). index.ex reduced from 2,062 → 1,202 lines (42% reduction). All 361 tests pass across multiple seeds. Rendering (locate_button, bottom_controls) intentionally kept in index.ex as template code.
2. Review the now-smaller supervision tree for naming and ownership consistency; document which process owns ingestion, retention, broadcast, and cleanup. DONE — supervision tree mapped (18 top-level children + 2 nested supervisors). Ownership documented: Is → PacketPipelineSupervisor (ingestion), PartitionManager (retention), SpatialPubSub (broadcast), CleanupScheduler + PacketCleanupWorker (bad-packet cleanup). Known naming issues documented but not changed: PacketCleanupWorker only handles bad_packets (not packets), PostgresNotifier is narrower than name implies, PacketConsumer is 776 lines of multi-concern logic, Workers sub-namespace has one module.
3. Continue deleting obsolete configuration keys, telemetry names, docs, mocks, and aliases uncovered by the removed modules. DONE — removed stale .dialyzer_ignore.exs entry, orphaned :initialize_replay_delay config key, updated CLAUDE.md StreamingPacketsPubSub → SpatialPubSub reference. No stale telemetry/metric or mock references remain.
4. Review callsign naming. Ingestion currently accepts safe APRS transport identifiers (up to 20 characters, uppercase alphanumeric segments separated by hyphens), which is intentionally broader than a strict AX.25 callsign. Rename APIs or document this distinction to avoid future accidental tightening. DONE — enhanced Aprsme.Callsign @moduledoc with explicit AX.25 vs transport-safe identifier distinction. Four validation layers documented: Aprsme.Callsign (transport-safe, 20-byte max), User schema (strict amateur radio), API controller (12-char), ParamUtils (LiveView search). Future changes should remain additive, not narrowing.
5. ~~Add focused migration tests for:
   • Existing partition indexes being attached correctly.
   • Counter reconciliation on an existing populated database.
   • Exact cutoff behavior around partition boundaries.~~ DONE — 12 tests in test/aprsme/migration_validation_test.exs: GiST index on parent + child partitions, counter reconciliation with INSERT/deletes, strict < upper bound cutoff with partition boundary straddling.
6. Review the deleted test count. The suite decreased because tests for removed infrastructure were deleted; ensure important end-to-end ingestion coverage was not lost when packet_pipeline_integration_test.exs was removed. DONE — deleted file had 1 test for removed InsertOptimizer module. All ingestion scenarios covered across packet_consumer_test.exs (1,006 lines), packets_test.exs (1,676+ lines), spatial_pubsub_test.exs (28 tests), partition_manager_test.exs, migration_validation_test.exs. Final suite: 2491 passed, 0 failures. No gaps.

Review notes

• The geometry-index migration creates and attaches indexes per partition in production. Test behavior uses a simpler direct index path. Validate this on a production-like PostgreSQL version and data volume.
• The packet-count trigger approach prioritizes exactness. Measure its write contention at expected ingestion rates; if it is too expensive, replace it with an explicitly approximate metric rather than a misleading exact API.
• The CSP work should retain the early theme selection behavior to avoid a flash of the wrong theme.
• Do not reintroduce StreamingPacketsPubSub or per-packet spawned broadcast tasks; extend SpatialPubSub if delivery behavior needs adjustment.

Suggested sequence

1. Review and isolate the dirty vendor/aprs changes.
2. Run all static gates and fix existing findings.
3. Complete proxy trust, rate limiting, CSP, and authorization review.
4. Bound buffers and payloads, then load-test delivery.
5. Profile and fix SQL query/index issues.
6. Refactor the large LiveView only after behavioral and performance coverage is stable.
7. Finish Kubernetes hardening and rerun the full release build.