Skip to content

20260910 - Own the record, and serve it (re-target of #28) - #29

Closed
Purple10101 wants to merge 7 commits into
mainfrom
20260909-tracker-control-reset
Closed

Purple10101 wants to merge 7 commits into
mainfrom
20260909-tracker-control-reset

Conversation

@Purple10101

Copy link
Copy Markdown
Contributor

#28 was approved and merged, but it landed on 20260909-couple-delay-doppler-state rather than on main.

It was a stacked PR based on #27. #27 merged to main at 12:53:51 and #28 merged twenty seconds later, before GitHub had re-targeted it, so it merged into its original base branch. gh reports it MERGED, which is true, just not into main.

The result is that main carries #27 alone. retina_tracker/control.py and retina_tracker/history.py do not exist on it, along with the ingest rewrite, the detection classification work and five test files.

That is load-bearing right now: retina-gui and retina-node have both merged and both depend on the control surface this adds. retina-node passes --control-port to the tracker container, and main's CLI does not accept it, so argparse would exit 2 and the container would crash-loop.

What is in here

Nothing new. This is #28's content, unchanged:

  • control.py, the loopback HTTP surface: POST /reset, POST /history/clear, GET /health, and GET /events as an SSE stream with chunked framing
  • history.py, four hours of detections held in array.array columns across three classes, with snapshot and delta reads
  • the ingest rewrite, so the newest detection feed takes the socket
  • per-detection classification, so the record says what became of every detection rather than only the tracked ones
  • the events file carrying only what changed, rather than repeating each track's whole window

Verification

A merge of this branch into main produces no conflicts, and the resulting tree is byte-identical to the branch tree already reviewed on #28. There is no new code to look at.

Both gates pass on the branch head. claude-review will fail until CLAUDE_CODE_OAUTH_TOKEN is rotated: every review run across the org has failed since the token went stale yesterday evening, and it fails before any model call, so it is not reading this diff.

🤖 Generated with Claude Code

Purple10101 and others added 7 commits September 9, 2026 18:47
The one control operation the tracker supports, clearing state between search
geometries, has only ever been reachable as a {"type": "RESET"} message mixed
into the detection socket. That works while one process sends both the
detections and the controls, and that is exactly the arrangement being unwound.
blah2_api is taking over the detection socket so retina-gui stops being the
transport between blah2 and here; run_tcp_server accepts one connection at a
time, so the auto-calibration search cannot simply open its own alongside it.

So control gets its own door: POST /reset on a loopback HTTP surface, plus GET
/health, which reports frames seen and tracks held and is the one question
worth asking a node while the feeder is being switched over. Nothing here knows
who is calling. The auto-calibration search and the Tracker page are equal
consumers of a tracker that does not distinguish between them.

Loopback by default, for the same reason the ingest socket is: the sidecar runs
with network_mode host, where 0.0.0.0 would publish this on the LAN. Stdlib
only. This is a handful of routes with one or two clients, and a framework
would be a dependency and an image layer for nothing.

A reset from a request thread can now land in the middle of a frame, so both
paths take one lock. It is held for the duration of the reset, so a 200 means
the tracker is already clear rather than scheduled to be: a caller resetting
between candidate geometries waits on the next frame, and that frame must not
be able to associate into pre-reset state. Deferring to a flag the frame loop
reads would have made the response a promise, and would never have applied at
all on a node whose detections had stopped.

The socket RESET keeps working. It goes when retina-gui stops feeding this
socket, not before.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The ingest loop accepted one connection and then read it to EOF, with a
backlog of one behind it. A peer that vanishes without closing — a killed
container, a dropped link — leaves a half-open socket that never returns
from recv and never reaches EOF, so the loop sat there for as long as the
kernel took to notice while the next peer waited unserved.

That is not a hypothetical here. The handover from retina-gui to blah2_api
is exactly a new peer arriving while the old one may still be holding the
slot, and getting it wrong means a tracker that silently receives nothing.

So the loop selects on the listener as well as the connection, and the
newest connection wins: accepting closes the one it replaces and drops
whatever was half-read, since a partial line belongs to the peer that was
sending it. Replacing rather than multiplexing is deliberate. The tracker
does not deduplicate frames, so two live feeders would hand it every frame
twice at dt=0 and give every track twice the evidence it earned.

Two smaller things the rewrite fixes on the way past. The buffer holds bytes
and decodes per line, so a multi-byte character split across a recv boundary
no longer raises out of the loop. And one malformed frame can no longer end
detection ingest until the container is restarted: a line that is not JSON,
not an object, or missing what process_frame needs is logged and dropped.
Both were reachable from anything with access to the port.

serve_detections is split out from run_tcp_server and takes a stop_event, so
the tests drive it over real sockets on an ephemeral port. Nothing here is
observable against a stub: every case is about what the socket layer does.
Reverting to the old "stop watching the listener once connected" behaviour
fails three of them.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Every track event carried the track's whole rolling window, up to
detection_window points, to communicate the one point that was new. On a
300-frame run with three targets that is 1.38 MB where 277 KB says the same
thing: 5x, measured by running the same input through the tracker twice with
the filtering off and on.

Not the 20x the window size suggests, because at one detection per event the
per-event metadata is most of the line. If this file needs to shrink further,
that is where the bytes are now, not in the detections.

Safe because neither consumer reads an event as the track's state now.
live_score.load_tracks unions detections by timestamp across every event
mentioning a track, and retina-gui's buffer appends only timestamps it has
not seen; both reconstruct the same history from a delta stream as from a
repeating one, which the tests assert directly by loading both and comparing.

A high-water mark per track is enough. A track's history only grows forwards
for as long as it can emit: merging is the one thing that splices older
points in, and it operates on all_tracks, the post-mortem archive, after the
track has been deleted from self.tracks and can no longer produce an event.
The map is an LRU bounded at 512 tracks, since ids are unique for the life of
a run and this process runs for weeks. Evicting one costs a repeated window,
never a dropped detection, which is the right way round given both consumers
dedupe.

The event is still written when nothing is new. length, the anomaly flags and
shadow_fraction all move over a track's life, and a consumer that missed
those updates would be holding a stale opinion of a live track.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The tracker consumed detections and reported only the ones that ended up
inside a confirmed track. Two exits were silent. Anything below MIN_SNR was
dropped at the gate before the tracker looked at it, and anything that
started a tentative track which never promoted vanished with it. So nothing
downstream could tell "the tracker rejected everything it saw" from "there
was nothing to see", which are the two explanations for an empty page and
want opposite responses.

Every detection now leaves through an optional sink, classified as
associated, unassociated, or below_snr. The gate becomes a partition rather
than a filter, in one loop rather than two comprehensions so a NaN snr — which
fails both comparisons — still lands in exactly one bucket instead of
disappearing from the accounting.

The hard part is when a verdict is final. "Associated" means the detection
ended up in a track that was confirmed, and that is not knowable when the
detection arrives: one that starts a tentative track is unassociated at that
instant and becomes associated a few frames later if the track promotes.
Answering on arrival would be wrong for exactly the detections that matter
most. So a frame waits until each of its tracks has settled, promoted or
deleted, and is released with a final answer. A detection joining an
already-confirmed track settles immediately, so a steady feed is not delayed;
on a mixed 200-frame scene the queue sat 6 frames behind, which is N_WINDOW.

Frames are released only from the front. Out-of-order release would break the
one assumption a consumer buffering these wants to make, and it is why a
below-gate detection waits for its frame despite its own verdict being
obvious. One call per frame, in order, is worth a few frames of lag.

ever_confirmed latches in the state_status setter rather than at the
promotion site. Three routes reach ACTIVE — M-of-N promotion, a coasting
track re-associating, and tracklet initiation — and latching at one of them
was silently wrong for the other two: every track in a steady feed read as
never confirmed. Current status cannot stand in for the flag either, since a
track that was ACTIVE and has since been deleted reads the same as one that
never got there.

No sink means no bookkeeping, so the CLI and any node without a consumer pay
nothing. With one attached the cost was 3.8% on 200 frames.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The tracker's own Track objects keep a bounded per-track ring and drop
completed tracks after a short merge window, so nothing in it can answer what
was seen three hours ago. This is that record: every detection the tracker was
given, classified, plus the points and metadata of every track.

In memory, not on disk. It does not need to survive a restart, and the
alternative was several hundred megabytes a day onto SD cards the fleet cannot
afford to wear out.

Stored as array.array columns. A CPython tuple of four floats costs about 192
bytes once the list slot, the tuple header and four boxed floats are counted;
the same point here is 20, so a buffer that would have been 83 MB is 8.7.
Columns are also what goes on the wire, so serving a snapshot is a slice rather
than a transposition, and one point-set per class means it is a slice rather
than a filter over everything held.

Bounded twice. The window is the intent, but a window alone makes the footprint
a function of how busy the sky is, which is not something a node can promise.
max_points is a hard ceiling underneath it, defaulting to about 10 MB per
class, and whichever binds first wins. Tracks are capped too, evicting whichever
has the stalest last point so a live one is reached for last.

Values are rounded on the way out, to the precision the measurement has. That
is not only about wire size: 16.1 held as a float32 reads back as
16.100000381469727, which would be eighteen bytes for four bytes of meaning.

GET /events serves it: a snapshot on connect, then only what has been appended.
The cursor lives in the connection's own thread and nowhere else, so there is
no per-consumer state on the server to expire and nothing to negotiate — a
reconnect takes a fresh snapshot, which is always a valid place to start.
Sending the snapshot down the same stream is what removes the race between what
it contained and where the deltas began. Because pruning drops from the front,
a position is a monotonic count rather than an index, and clear() bumps a
generation that voids every outstanding cursor at once.

/health now reports the footprint, which is the question worth being able to
ask of a node whose memory matters.

Verified end to end against a running tracker: three detections a frame, one of
each class, arriving over a real SSE connection as a snapshot followed by
deltas, with the track's own points alongside.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…cord be cleared

Two additions the page needs.

POST /history/clear wipes the record without touching the tracker. That is
the distinction "Clear buffer" has always carried — clear what I am being
shown, keep tracking — and it survives the record moving here from
retina-gui, which no longer has a buffer to clear.

And the stream is chunked. It was sending "Connection: close" with no length,
which is legal and which curl and EventSource both handle, so it looked fine.
urllib3 does not: with no framing it reads to EOF to find a message boundary,
and on a stream that never ends that means blocking until the read timeout
instead of delivering each event as it arrives. retina-gui proxies this with
requests, so every message would have arrived only when the connection
finally gave up. Framing each message as its own HTTP/1.1 chunk makes it
stream to any client rather than only the forgiving ones.

Found by proxying it for real rather than by reading the spec, which is the
only way this one shows up: every direct client I had tried worked.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…work hooks

Two CI gates, neither of them a code problem.

ruff-format reflows the lines this branch added: joined argument lists that
fit, an implicit string concatenation collapsed to one literal, trailing
commas, and a space after a docstring's opening quotes. Token-for-token
identical otherwise.

The dead-code gate reported seven names in control.py. Six are called by
http.server and socketserver rather than by us: do_GET and do_POST are looked
up by name from the request line, log_message is an override the base class
calls, and protocol_version, daemon_threads and allow_reuse_address are class
attributes the servers read. Those go in the whitelist's contracts section,
which had been empty, each with the reason it cannot be deleted.

The seventh was real. log_message's fmt parameter went unused because the
override discards everything; the base class calls it positionally, so *args
covers the format string and the parameter can go.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@claude

claude Bot commented Sep 10, 2026

Copy link
Copy Markdown

Claude finished @Purple10101's task in 4m 3s —— View job


Reviewing PR #29

  • Gather context (PR description, changed files, CLAUDE.md rules)
  • Verify merge/conflict claim against origin/main
  • Review control.py (loopback HTTP surface)
  • Review history.py (rolling detection buffer)
  • Review ingest rewrite / classification changes (server.py, tracker.py, track.py, output.py, cli.py)
  • Review new tests
  • Run lint/format/tests per CLAUDE.md (blocked — see note)
  • Post final review

Summary

The re-target story checks out: gh pr view 28 shows its head branch is 20260909-tracker-control-reset (this PR's branch) merged into base 20260909-couple-delay-doppler-state, not main. git diff origin/main HEAD --stat here matches the PR's claimed 13 files / +2384/-40 exactly, and --control-port is present on the CLI (retina_tracker/cli.py:215-217), so the retina-node crash-loop concern is real and this does fix it.

I read all seven new/modified source files and the five new test files. The classification pipeline (tracker.py's _classify_frame/_drain_classifications) is the trickiest part — I traced landed_in through association, promotion, deletion and _merge_tracks, and the ordering holds: a track is marked retired = True before it can ever become eligible for _merge_tracks, so a merged-away track's classification is already settled and unaffected by the merge. I didn't find a correctness bug there.

Two things worth a look:

1. Every new/changed file is comment- and docstring-heavy, which conflicts with this repo's own stated rules. 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 unqualified. control.py, history.py, tracker.py, track.py, and output.py all carry module/class/method docstrings plus inline rationale comments throughout (e.g. retina_tracker/control.py:1-22, retina_tracker/history.py:1-36, retina_tracker/tracker.py:283-334). The content is genuinely good — it explains non-obvious why rather than restating the what — but it's a direct conflict with the letter of the repo's rules as written today, not a style nit I'm inventing. Since this is a straight re-target of already-approved #28 content, this was presumably accepted once already; flagging it here since the rule is unambiguous and machine-checkable, and future PRs may get held to it inconsistently otherwise.
Fix this →

2. Minor edge-case bug in the window clamp — retina_tracker/control.py:113-123.

return max(MIN_WINDOW_S, min(seconds, self.server.history.window_s))

The comment above (control.py:43-45) says a requested window "must never... ask for more than is held." But max(MIN_WINDOW_S, ...) is applied after clamping to history.window_s, so if the server were configured with history_window_s < MIN_WINDOW_S (60s) — possible via --history-window, which has no lower-bound validation in cli.py:218-223 — a client could get a window larger than what's actually held, back to the invariant the comment says shouldn't be violated. Not reachable with the default 4-hour window, so this is low severity, but the fix is a one-line reorder: min(max(seconds, MIN_WINDOW_S), self.server.history.window_s).

Note on verification: I could not run ruff check, ruff format --check, or pytest — the sandbox denied those Bash invocations ("this command requires approval") with no prompt to approve. Static reading of the diff didn't surface obvious lint/format violations, and the new test files (test_control.py, test_history.py, test_classification.py, test_event_deltas.py, test_ingest.py) look thorough — they specifically cover the tricky cases (aged-out pending classification, cursor invalidation on clear(), connection replacement mid-read, track eviction order). If CI on this branch is green (the PR says both required gates pass), that's stronger signal than I can independently reproduce here.

Nothing here blocks the merge — the two points above are a style-rule conflict worth a maintainer call and a low-severity edge case, not correctness issues in the load-bearing path.

@Purple10101

Copy link
Copy Markdown
Contributor Author

Reopening from 20260909-couple-delay-doppler-state instead, so both branches end up fully merged. Same resulting tree.

@Purple10101
Purple10101 deleted the 20260909-tracker-control-reset branch September 10, 2026 14:24
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