Skip to content

Bound per-track history and shrink the completed-track archive - #25

Merged
jehanazad merged 1 commit into
mainfrom
fix/bounded-track-history
Sep 6, 2026
Merged

jehanazad merged 1 commit into
mainfrom
fix/bounded-track-history

Conversation

@jehanazad

Copy link
Copy Markdown
Contributor

Summary

Two stores in the tracker grew without bound in a long-running server holding one Tracker per node (~60 pipelines, days of uptime): Track.history (five plain lists appended on every update and every coast step, ~1.5 MiB per track-hour at 1 Hz, worse for a persistent clutter track) and Tracker.completed_tracks (deque(maxlen=5000) of whole Track objects with full history per node, read by nothing in the server). This bounds both.

Changes

  • Track.history buffers are collections.deque(maxlen=TRACK_HISTORY_MAX); TRACK_HISTORY_MAX = 600 (10 min at 1 Hz), overridable per track via config tracker.track_history_max for batch consumers that want deep history. The deepest reader in the server asks for 20 samples.
  • get_recent_detections rewritten as a reversed zip over measurements/timestamps (deque indexing is O(n); semantics unchanged: newest-first scan, skips the None a missed frame appends, returns the newest n oldest-first).
  • to_dict materialises the deques (json.dumps cannot serialise a deque).
  • MAX_COMPLETED_TRACKS 5000 → 50, with a max_completed_tracks kwarg on Tracker.__init__ for consumers that want the deeper archive. Count-capping rather than stripping history keeps to_dict()/the CLI batch output meaningful.
  • _merge_track_pair now extends all five buffers (it skipped frames, leaving lengths unequal).

Measured

Track.history: 450 B per entry → 1.55 MiB per track-hour unbounded, now 264 KiB flat per track. completed_tracks: ~30 KiB per retired track → per-node ceiling 149 MiB → 1.5 MiB.

Tests

New tests/test_history_bounds.py (6): 5000-frame run keeps every buffer at the cap; get_recent_detections(20) still returns the newest 20 associated samples in order; coasted frames skipped; configurable cap; bounded archive with the escape hatch; retired tracks keep bounded history. Suite 108 passed; pre-commit clean.

Consumer: offworldlabs/retina-server pin bump follows.

🤖 Generated with Claude Code

Two per-Tracker stores grew far past anything that reads them, and the
server holds one Tracker per radar node with days of uptime, so both are
multiplied by the fleet.

`Track.history` was five plain lists appended on every processed frame,
on the update path and the coast path alike, with no cap anywhere. A
track that never satisfies should_delete -- a loitering aircraft, or a
persistent clutter return that coasts and re-associates forever --
therefore accumulated one timestamp, one frame number, one numpy state
copy, one detection dict and one status string per frame for its whole
life. Measured at 450 B per entry: 1.55 MiB per track-hour at 1 Hz, and
strictly monotonic. This is the one store here that produces sustained
growth rather than a plateau.

The five buffers become deque(maxlen=TRACK_HISTORY_MAX), default 600 --
10 minutes at 1 Hz, and about 60x the deepest reader. Every reader wants
only the tail. `_initiate_tracklets` fits the last 3 associated samples
and only ever runs on TENTATIVE tracks, which die at N_WINDOW frames.
`get_recent_detections` is called with n bounded by the tracker's
detection_window, 20 by default. `_merge_tracks` reads states[-1] and
states[0] across a 5 s window. The server backend reads no history
attribute at all: it goes through get_recent_detections, and asks for at
most N2_TRACK_HISTORY_MAX, which is 20. The cap is overridable per Track
with config tracker.track_history_max, for a batch consumer that wants
deeper history than a long-running server should hold.

Nothing slices the buffers, so the deque swap needed three adaptations
rather than a rewrite. `get_recent_detections` walked its two lists by
descending index, which on a deque is O(n) per access and would have
made the scan quadratic; it now walks reversed() iterators of the two
buffers in step, which preserves the semantics exactly -- newest-first,
skipping the None measurements that mark_missed appends for coasted
frames, stopping at n, returned oldest-first. `to_dict` now materializes
the timestamps and state_status buffers with list(), because json.dumps
cannot serialize a deque and the CLI dumps that structure. The
`.extend()` calls in `_merge_track_pair` need no change.

That merge also extended four of the five buffers and not "frames",
which left the buffers unequal in length. Nothing reads "frames" today,
so this was invisible, but with a shared maxlen the misalignment is the
kind that outlives its cause. It extends all five now.

`completed_tracks` is a deque of whole retired Track objects, each with
its history attached, and MAX_COMPLETED_TRACKS was 5000 -- about 149 MiB
per node at the measured 30 KiB for a 40-frame track, filled by ordinary
track churn within the hour and then never released. Nothing outside
this library and its tests reads it: get_confirmed_tracks and
completed_tracks have zero call sites in the server, which reads only
tracker.tracks. The ceiling drops to 50.

Capping the count rather than stripping history off retired tracks,
because the history is what makes the archive worth keeping: to_dict
serializes each confirmed track's full history, and that is the CLI's
entire batch output. A dropped payload would leave the retention tests
asserting over hollow objects. A consumer that genuinely wants a deeper
archive passes max_completed_tracks to Tracker(), so the ceiling is a
default rather than a limit.

The bound is pinned by tests over a 5000-frame run rather than by the
constant alone: the buffers stay at maxlen while n_frames runs past it,
and get_recent_detections still returns the newest 20 associated samples
oldest-first, contiguous, ending on the frame just processed. A separate
case marks three frames missed and asserts the None measurements are
skipped, since that reverse-scan semantic is what the server's freshness
signal depends on.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
@claude

claude Bot commented Sep 6, 2026

Copy link
Copy Markdown

Claude finished @jehanazad's task in 6m 21s —— View job


Code Review

  • Gather context (diff vs origin/main, all three changed files, call sites of Tracker(...))
  • Verify findings against source
  • Post review

Reviewed retina_tracker/track.py, retina_tracker/tracker.py, and tests/test_history_bounds.py against origin/main.

1. MAX_COMPLETED_TRACKS dropped 100x (5000 → 50) with no config wiring — unlike the sibling fix (retina_tracker/tracker.py:23)

track_history_max is readable from config['tracker'] in Track.__init__ (retina_tracker/track.py:70), giving deployments an escape hatch back to deeper history. max_completed_tracks got a constructor kwarg but no equivalent config path, and none of the three real construction sites pass it:

  • retina_tracker/__init__.py:16Tracker(event_writer=writer, config=config)
  • retina_tracker/server.py:48Tracker(...) (no kwarg)
  • retina_tracker/cli.py:58Tracker(event_writer=event_writer, detection_window=detection_window, config=get_config())

So every real deployment silently gets the new hardcoded default of 50 instead of 5000. Per the PR's own reasoning, this archive is what to_dict()/visualize_tracks/batch consumers read for anything beyond the last few seconds of a capture — dropping the cap 100x with zero config knob means a busy node over a multi-hour capture retains only the most recent 50 retired tracks. Worth either reading tracker.max_completed_tracks from config the same way track_history_max is, or explicitly noting in the PR why it's intentionally not configurable.

2. New test file adds inline comments, violating this repo's no-comments rule (tests/test_history_bounds.py:58,101,110)

Both CLAUDE.md ("No comments: Code should be self-documenting") and .claude/rules/code-style.md ("Do not add comments; let names and structure carry the intent") are explicit and apply repo-wide, not just to production code. Lines like # Oldest-first, contiguous, and ending on the frame just processed., # ...and a consumer that wants a deeper archive can still ask for one., and # Idle past N_DELETE and past the merge window so the track retires. should be removed or folded into names/asserts/docstrings.

3. The new history["frames"] merge fix has no direct test (retina_tracker/tracker.py:392)

_merge_track_pair previously skipped frames while merging the other four buffers — this PR fixes that (good catch, it was a real bug). But tests/test_history_bounds.py only exercises single, unmerged tracks (bounded growth, get_recent_detections). No test drives two tracks through _merge_tracks/_merge_track_pair and asserts history["frames"] stays the same length as timestamps/states/measurements/state_status afterward. Since CLAUDE.md requires test coverage for all business logic, and this is exactly the kind of silent-desync regression the fix addresses, a merge-path test would close the gap.

Everything else checks out: the list→deque migration doesn't break any reader (get_recent_detections, to_dict, tracklet fit, cli.py plotting), to_dict correctly re-wraps deques for JSON serialization, and index alignment across the five parallel history buffers is preserved.

@jehanazad
jehanazad merged commit d172e3e into main Sep 6, 2026
2 checks passed
@Purple10101
Purple10101 deleted the fix/bounded-track-history branch September 6, 2026 15:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant