110 lines
7.4 KiB
Markdown
110 lines
7.4 KiB
Markdown
---
|
|
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`
|