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:

```sh
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.

```sh
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.