---
tags: [project-drogheda, backend, code-review, scale]
project: Project Drogheda
author: Helgen CTO
date: 2026-06-20
status: Review — for the "add more units" test campaign
---

# Code Review — retranslator + monitor (scale readiness)

> Review of the **hardened** `retranslator.py`, `web_monitor.py`, and the
> `gps_logger.py` they depend on, ahead of bringing more units online. The three
> 🔴 field-killers (HOL blocking, anti-jump black-hole, CGNAT identity) are already
> fixed — this pass is about **what else bites as the fleet grows**. Nothing here is
> on fire; the top items are the ones you'll actually hit while testing.

Severity: 🔴 blocks/badly misleads at scale · 🟠 should fix before the fleet grows ·
🟡 polish.

> **Status (2026-06-20):** **R1, R2, G1, G2 are implemented and verified** — smoke
> (10/10, no regression), an R1/R2 focused test (7/7), and a standalone index+prune
> test all pass. Remaining: **M1** (dashboard auth — an nginx change, server-side) and
> the 🟡 polish items. New env knob: `GPS_RETENTION_DAYS` (default 30, 0 = off).

## Summary

The hardened code is solid and the concurrency model is sound. The remaining risks
are mostly **operational at scale**: a new unit that isn't in `device_config.json`
silently won't forward (and the monitor won't tell you why); the SQLite history has
no index and no retention (it will scan slowly and grow until the small disk fills);
the dashboard has no auth and re-renders 500 rows every 2 s; and a couple of latent
parsing/ack assumptions. No SQL injection (queries are parameterized) and no obvious
XSS.

## 🔴 / 🟠 Issues to fix before adding units

| # | File · location | Issue | Sev |
|---|---|---|---|
| R1 | `retranslator.py` · `_process_device_telemetry` (`wialon_id = device_cell_id_to_wialon_id.get(cell_id)`) | **An unconfigured `cell_id` is silently never forwarded.** A new unit's GPTXT marks it *online* (so it appears active in the monitor) but with no `wialon_id` mapping it's never sent to Wialon — and it shows as `wialon_state="idle"`, not a clear "unconfigured". This is the #1 thing you'll hit adding units. | 🔴 |
| G2 | `gps_logger.py` · whole file | **SQLite history grows unbounded.** ~288 rows/unit/day → ~144k/day at 500 units, no pruning. On the space-limited server this **fills the disk** over weeks. | 🔴 |
| G1 | `gps_logger.py` · `_create_table` | **No index** on `gps_positions`. `/api/availability` does `WHERE received_at … GROUP BY day,device` → full scan; at millions of rows the report crawls. | 🟠 |
| M1 | `web_monitor.py` / nginx | **No auth on the dashboard or API.** `monitor.helgen.dev` exposes live fleet positions + `/api/status` + `/api/availability` to anyone with the URL. Client-sensitive. | 🟠 |
| R2 | `retranslator.py` · `main()` UDP loop (`raw = udp_data.decode(...).strip()`) | **One NMEA sentence per UDP datagram is assumed.** If a unit ever batches sentences in one packet (`$GPTXT…\r\n$GNGGA…`), only the first is parsed. Latent today (firmware sends one per datagram) but a trap for the planned `$PHLG` injection. | 🟠 |
| R3 | `retranslator.py` · `WialonForwarder._deliver` (`response = s.recv(1024)`) | **Wialon data-ack isn't validated.** Only the *login* ack (`#AL#1`) is checked; the per-packet ack is read and discarded, so a **rejected** packet still counts as `acked` → monitor shows the unit healthy when Wialon isn't accepting it. | 🟠 |

## 🟡 Worth doing

| # | File · location | Suggestion |
|---|---|---|
| R4 | `retranslator.py` · config load | New units need a **restart** (`device_config.json` read once). Add a `SIGHUP` handler to reload the map live — no fleet-wide blip every time you add a test unit. |
| M2 | `web_monitor.py` · `renderTable()` (`tbody.innerHTML = rows.map(...)`) | Re-renders **all 500 rows every 2 s**, churning the DOM and dropping scroll/selection. Diff rows, or only re-render on change. |
| R5 | `retranslator.py` · `_build_data_packet` (`datetime.utcnow()`) | Wialon timestamp = **server receive time**, not the GGA fix time. Sub-second for a slow fleet, but parse the GGA UTC if you want exact fix times. |
| R6 | `retranslator.py` · `WialonForwarder.forget` | A `forget()` on a full worker queue is silently dropped → a socket can leak until the next send error. Bounded; low impact. |
| R7 | `retranslator.py` · `_deliver` ack read | Residual per-worker HOL: one slow ack blocks that worker's ~8 devices for up to `TCP_RW_TIMEOUT` (15 s). Far better than global; consider lowering the read timeout. |
| G3 | `gps_logger.py` · `_insert` | `commit()` per row. Fine at the 300 s throttle; revisit if the throttle drops or inserts burst. |
| M3 | `web_monitor.py` · `sse_events` | Re-parses the whole status JSON every 2 s only to validate. Trivial now; grows with the device count. |

## What looks good ✅

- **Concurrency model** — UDP ingest fully decoupled from Wialon I/O; per-worker
  queues with drop-oldest backpressure; stats behind a lock; `list()` snapshots in
  the stats/cleanup threads (no "dict changed size").
- **Identity** — `(ip,port)` transport + `cell_id` state is the right CGNAT-safe shape.
- **Anti-jump** — time-aware + self-healing + cleared on disconnect; can't black-hole.
- **Safety** — atomic status-file write (`.tmp` + `rename`); fail-safe drops; SQL writer
  is a bounded background thread that never blocks the loop.
- **Security hygiene** — availability SQL is **parameterized** (`?`), DB opened
  read-only, free-text escaped (`esc()`), SSE degrades to "gateway down" cleanly.

## Before you test more units — do these first

1. **R1 — make unconfigured units obvious.** Two cheap parts: log a one-time
   `WARNING` per unknown `cell_id`, and surface a distinct `wialon_state="unconfigured"`
   in the monitor so a new unit that isn't forwarding is self-explanatory.
   ```python
   # in _process_device_telemetry, where wialon_id is looked up:
   wialon_id = device_cell_id_to_wialon_id.get(cell_id)
   if not wialon_id:
       if cell_id not in _warned_unconfigured:      # module-level set()
           _warned_unconfigured.add(cell_id)
           log.warning(f"[CONFIG] cell_id {cell_id} sin wialon_id — "
                       f"agrégalo a device_config.json o no se reenvía.")
       # still record telemetry/odometer; just don't forward
   ```
2. **G2 — cap the SQLite growth.** A nightly prune keeps the disk safe:
   ```sql
   DELETE FROM gps_positions WHERE received_at < datetime('now','-30 days');
   ```
   (run from the writer on a daily timer, or a cron `sqlite3` one-liner + `VACUUM`).
3. **G1 — add the index** (one line in `_create_table`, idempotent):
   ```sql
   CREATE INDEX IF NOT EXISTS idx_gps_recv_dev
     ON gps_positions(received_at, device_cell_id);
   ```
4. **M1 — gate the dashboard.** Fastest is nginx Basic Auth on `monitor.helgen.dev`
   (one `auth_basic` + htpasswd), or an IP allowlist for the office/VPN.
5. **R2 — split multi-sentence datagrams** (`for line in raw.split('\n'): …`) so the
   loop is robust before you wire any new sender (incl. `$PHLG`).

## Verdict

**Approve, with the R1/G1/G2 items recommended before the test campaign.** The core is
production-grade; the gaps are operational (config ergonomics, DB lifecycle, dashboard
auth) and quick to close. R1 and G2 are the two that will actually bite you while
adding units.

## Related
- [[02-Architecture-Software/backend-review|backend-review]] (the original 🔴 findings + resolution) ·
  `04-Source-Code/htracker/retranslator.py` · `web_monitor.py` · `gps_logger.py`