Addresses PR 191 reviewer feedback:
1. Shutdown drain
stop() now waits up to 5 s for in-flight _route_packet tasks to finish,
then cancels any that remain. Previously only the queue-consumer loop was
cancelled; created tasks were abandoned with no guarantee they completed.
Mechanism: _route_tasks set tracks live tasks (added on create, discarded
in the done-callback). stop() takes a snapshot and calls asyncio.wait()
with timeout=5.0, then cancels the still-pending subset.
2. Drop counter
_cap_drop_count increments each time a packet is dropped at the cap.
The running total is included in every WARNING log line and also printed
at shutdown so operators can tell at a glance whether the safety valve is
actually firing in production.
3. Tests (tests/test_packet_router.py)
test_cap_drops_packets_when_full — cap=3, send 8 → 5 drops, 3 in-flight
test_cap_drop_count_increments — count increments by 1 per drop
test_cap_drop_count_zero_... — count stays 0 when cap never reached
test_stop_waits_for_in_flight_tasks — slow task (0.2 s) completes, not cancelled
test_stop_cancels_tasks_...timeout — hanging task cancelled after timeout
test_route_tasks_set_cleaned_up — set empty after all tasks finish
test_counter_matches_set_size — _in_flight == len(_route_tasks) at cap
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Merged this now. It’s a safe change with no behavioural impact, and it removes unnecessary work in the hot paths when DEBUG logging is off. Happy to revisit if we want to standardise on lazy formatting later, but this gives us an immediate win.
Problem
-------
update_packet_metrics() called rrdtool.info() (cached for 5 s) to get the
RRD's last_update timestamp. rrdtool.info() returns a massive Python dict:
17 data sources × 5 RRAs × ~8 fields each = ~700+ dict entries per call.
tracemalloc showed +10696 new allocations / +251 KB at this exact line,
flagged as "Investigate" in the memory diagnostics dashboard.
The rrdtool.info() approach was also unnecessarily complex: it required a
5-second secondary cache, a _pending_rrd_update buffer, and two extra
instance attributes — all to answer one question ("did we already write
this period?") that we can answer ourselves with a single integer.
Fix
---
Replace _last_rrd_info_cache / _last_rrd_info_time / _pending_rrd_update
with a single self._last_rrd_update: int = 0 that stores the timestamp of
the last successful rrdtool.update() call. The throttle check becomes:
if timestamp <= self._last_rrd_update:
return
On success: self._last_rrd_update = timestamp
Zero dict allocations per call. The only downside vs rrdtool.info() is
that _last_rrd_update resets to 0 on process restart, meaning the first
packet after a restart always triggers a write — correct behaviour.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Problem
-------
rrdtool.fetch() is a blocking C library call that reads 24 hours of RRD
data from disk. The dashboard can call get_data() on every page refresh.
On an SD card each fetch can cost several milliseconds of I/O, and because
the RRD step is 60 seconds the data cannot change more often than that —
any fetch within the same 60-second window returns identical data.
The combined-optimizations branch had a 60-second read cache; rightup's
batching refactor inadvertently removed it. This PR restores it.
Solution
--------
* Add self._get_data_cache: tuple = (0.0, None) to __init__
* In get_data(): set use_cache = (start_time is None and end_time is None)
- if use_cache and cache is < 60 s old: return cached result immediately
- after a successful live fetch with use_cache: store (now, result)
* Explicit start_time / end_time callers always bypass the cache so
fine-grained or historical queries are never stale
Why 60 s TTL?
The RRD step is 60 s, so the database cannot hold a newer sample until
the next step boundary. A 60-second cache is tight enough that the
dashboard always shows data ≤ one step stale, and loose enough that
a burst of refreshes costs one disk read instead of N.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Problem 1 — _recent_drops: the list was evicted with pop(0), which is an
O(n) memmove every time a drop is recorded. With maxlen=20 this is
negligible today, but pop(0) on a list is always O(n) and the pattern is
worth eliminating.
Problem 2 — _known_neighbors cap: the eviction path did
set(list(self._known_neighbors)[500:])
which first materialises the entire set as a list (O(n) allocation) before
slicing. itertools.islice works directly on the set iterator and only
allocates the 500 kept items, halving peak memory pressure during cleanup.
Changes:
* Import itertools (already absent from this file)
* Import deque from collections alongside OrderedDict
* self._recent_drops initialised as deque(maxlen=20); self._max_recent_drops
removed (maxlen is the single source of truth)
* Drop-recording block: rebuild deque from generator (preserves pubkey dedup
filter) then append — automatic eviction replaces the explicit pop(0) guard
* Known-neighbors cap: itertools.islice(self._known_neighbors, 500) replaces
list(self._known_neighbors)[500:]
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Lloyd