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

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.

# 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

G2 — cap the SQLite growth. A nightly prune keeps the disk safe:
```
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).

G1 — add the index (one line in _create_table, idempotent):

CREATE INDEX IF NOT EXISTS idx_gps_recv_dev
  ON gps_positions(received_at, device_cell_id);

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

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

7.4 KiB Raw Blame History