Snapshot track history deques before off-thread iteration

[jehanazad]

Summary

#25 turned the five Track.history buffers from lists into deque(maxlen=TRACK_HISTORY_MAX). A deque raises RuntimeError: deque mutated during iteration if it is appended to while a Python-level loop walks it; a list tolerated exactly that. Only the frame worker appends to a given node's tracker, but two Track readers are called from other threads, and both were raising in production — each failure dropping one consumer tick.

list(some_deque) is a single C call, so under the GIL it cannot be interleaved by another Python thread: it is an atomic snapshot. Both off-thread readers now snapshot before iterating.

Changes

retina_tracker/track.py

• get_recent_detections — snapshot history["measurements"] and history["timestamps"] back to back with list(), then run the identical zip(reversed(...), reversed(...)) reverse scan with the same early exit. Same output.
  The zip is deliberately left alone: the mutator appends timestamps → frames → states → measurements, so a reader that lands mid-append can always see one more timestamp than measurement. That was already true when these were lists, and zip truncating to the shorter buffer is the pre-existing behaviour.
• to_dict — snapshot the four emitted buffers (timestamps, states, measurements, state_status) before the comprehensions, then truncate to their common length so the diagnostic arrays stay aligned with one another.
• tracker.py's own history readers are not changed: they run on the mutating thread and cannot race.

Test coverage

New tests/test_history_snapshot.py::test_readers_survive_concurrent_appends: a background thread drives track.update() / track.mark_missed() in a tight loop for ~0.5 s while the main thread repeatedly calls get_recent_detections(n=5) and to_dict(), asserting no exception, well-formed detection dicts (timestamp/delay/doppler/snr), and mutually aligned to_dict history arrays. ~0.6 s wall.

• Without the fix the test failed on 5 of 5 runs (RuntimeError: deque mutated during iteration).
• With the fix: suite is 109 passed (108 before).

Review notes

• No behaviour change for single-threaded callers; the only new cost is one list() copy per call of each reader, bounded by TRACK_HISTORY_MAX.
• to_dict's new truncation can drop the newest sample from a dump when a reader lands mid-append. That is the intended trade for aligned arrays in a diagnostic payload.

🤖 Generated with Claude Code

[claude]

Claude finished @jehanazad's task in 3m 52s —— View job

---

Reviewing PR #26

• Gather context (PR description, changed files)
• Read retina_tracker/track.py diff
• Read tests/test_history_snapshot.py
• Check tracker.py/cli.py callers for other potential race sites
• Post review feedback

Review

Correctness of the core fix: sound. list(deque) is implemented entirely in C (the deque's native tp_iternext), so it never re-enters the bytecode eval loop and can't be interrupted by a GIL switch — it's a genuine atomic snapshot, unlike iterating the live deque. get_recent_detections (retina_tracker/track.py:795-821) and to_dict (retina_tracker/track.py:823-837) now snapshot before iterating/comprehending, which is exactly the right fix for RuntimeError: deque mutated during iteration.

to_dict truncation logic (retina_tracker/track.py:829-837): correct for the common case. Since the mutator appends timestamps → frames → states → measurements → state_status (retina_tracker/track.py:700-704, 723-732), a reader landing mid-update()/mark_missed() sees at most a 1-entry skew across the four snapshotted buffers, and truncating to min(...) keeps them aligned — matching the PR's own stated trade-off (may drop the newest sample from a dump).

One theoretical edge case worth being aware of (not a blocker, given this is diagnostic-only output): the four list() calls aren't a single atomic unit — they're four separate atomic snapshots taken in sequence. If a GIL switch happened between them and the writer thread got to run several more update()/mark_missed() cycles before the reader resumed (enough to evict more than one entry off the left of a bounded deque(maxlen=600)), the naive "skew is always ≤1" assumption could be violated, and [:n_hist] would no longer line up entries by original frame — not just be short one one sample. In practice this requires the reader to be preempted for many iterations mid-to_dict(), which is very unlikely (four back-to-back C calls rarely span a scheduler quantum), so this is a low-severity nit rather than something that needs fixing now.

get_recent_detections: the two independent snapshots (meas, ts_hist) can differ by the same kind of one-timestamp skew; the comment correctly notes zip truncates to the shorter one, preserving pre-existing (list-based) behavior. No issue.

Scope check: confirmed via grep that tracker.py's own direct history[...] reads (retina_tracker/tracker.py:287-288, 377-378, 391-395) run inside process_frame/_merge_track_pair, i.e. on the same thread that mutates history, so leaving them unchanged is correct. cli.py:131 reads history["measurements"] directly, but that's an offline plotting script with no concurrent writer — also fine to leave alone. No other off-thread Track history readers exist in this repo (the frame-worker/analytics/admin threads described in the PR body live in a consuming service, not in this repository), so the fix's scope matches what's actually here.

Style note: the project's .claude/rules/code-style.md says "do not add comments; let names and structure carry the intent," and this PR adds two multi-line comment blocks explaining the GIL/atomicity rationale. That said, this is a legitimate exception per the general guidance ("only add a comment when the WHY is non-obvious" — a hidden threading invariant like this qualifies), and the comments are load-bearing for future maintainers who might otherwise "simplify" the double-snapshot back to a single-pass iteration. I'd leave them, but flagging since it's a literal deviation from the repo's stated zero-comments policy.

Tests: tests/test_history_snapshot.py is a solid stress test — real Tracker/Track objects (not mocks), a genuine competing writer thread hammering update()/mark_missed() for ~0.5s while the main thread repeatedly calls both fixed readers, with alignment assertions on to_dict()'s emitted arrays. Verified the API usage (Tracker(config=...), process_frame, track.update(detection, ts, frame=i), n_frames/n_associated bookkeping) matches the current code. The PR's claim of 5/5 pre-fix failures and 109-passed post-fix is credible from reading the change.

Note: I wasn't able to execute ruff check/ruff format --check/pytest in this sandbox (tool invocations for python3/ruff were blocked pending approval that isn't available in this automated context), so I could not independently confirm the "109 passed" claim or lint/format cleanliness — this was a static read-through only. Recommend CI confirms these before merge.

Overall: a well-targeted, correctly-reasoned concurrency fix with good test coverage. No blocking issues found.