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.
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.
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.
5. Audit mobile and map search for unbounded scans. Add deterministic ordering,
   hard result caps, suitable indexes, and tests that assert the caps.
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.
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.