pipeline: detection freshness, ADS-B field coercion, and monotonic blah2 frames (tranche 1) - #235
Conversation
|
Claude finished @Babissimo's task in 16m 51s —— View job Claude finished reviewing PR #235Todo list
SummaryAll four fixes described in the PR body check out against the code:
Finding
_gs_raw = entry.get("gs", 0) or 0
gs_ms = (_gs_raw if isinstance(_gs_raw, (int, float)) else 0.0) * 0.514444
Minor nit
Already covered by disclosed follow-ups (verified, not re-flagged)The multi-agent pass also surfaced several additional un-migrated |
|
Claude finished @Babissimo's task in 2m 23s —— View job Claude finished reviewing PR #235Todo list
SummaryThis sync added one commit,
Both are exactly what my last pass asked for, nothing more, nothing less. Fresh pass over the full diffI re-read every changed source file against
No new issues found. I wasn't able to execute |
|
Both review findings were real, and both were inconsistencies this branch introduced. Fixed in The MLAT live truth pool now uses It also matters more than a consistency nit. Added a regression test asserting the four published figures stay finite; it fails against the The While in the block I also dropped the incident narration from the comment above it, keeping only the constraint that still binds. The workspace rules say to delete a passage describing a fixed fault rather than keep it. Full suite green (82.54%), Thanks for checking the follow-up tickets rather than re-flagging what they cover — that is exactly the right call on the sites under |
|
Claude finished @Babissimo's task in 2m 49s —— View job Claude finished reviewing PR #235Todo list
SummaryHEAD is still
I re-verified all four headline fixes against the code directly rather than re-trusting the prior passes: the New finding
_gs_ms = (adsb.get("gs", 0) or 0) * 0.514444
_trk = math.radians(adsb.get("track", 0) or 0)
existing.alt_m = as_num(adsb.get("alt_baro")) * FT_TO_M # <- fixed by this PR
This one, unlike the sibling sites your review labeled "no evidence it is live" ( Everything else re-checked, no issues
|
The duplicate guard tested `ts_ms != last_ts`, which catches an exact repeat but passes an older frame straight through to the tracker. Nothing between `state.frame_queue` and `Tracker.process_frame` re-orders or checks monotonicity, and `process_frame` computes dt by bare subtraction, so an older frame yields a negative dt -- the head of the chain the tracker-side fixes close. `>` is safe here only because `_convert_frame`'s staleness gate already clamps every accepted timestamp to within STALE_THRESHOLD_S of our own clock, which bounds a backwards clock step to a stall of twice that rather than a permanent one. That coupling is not obvious from the call site, so it is recorded there: the gate has to stay above this guard. blah2's timestamps are POSIX epoch from system_clock, not uptime, so a blah2 restart cannot regress them; and at a 0.5 s CPI against 1 ms resolution two distinct frames cannot share a timestamp, so `>` discards nothing that `!=` kept. `blah2_bridge_task` had no test coverage at all -- it is imported only by main.py -- so the guard's behaviour was unobservable in either direction. Four cases now pin it, including that a rejected out-of-order frame does not wedge the node against the next good one. This reduces one source of out-of-order frames; it does not eliminate them. FRAME_WORKERS runs several unkeyed workers over one queue, so two frames from the same node can still reach the same tracker out of order. That is the per-node executor work, deliberately out of this tranche. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…bqgf) `Track.get_recent_detections` builds its result newest-first and then reverses before returning, so it hands back oldest-first. Nothing on the path from tracker to solver re-sorts: the event writer stores the list verbatim, GeoTrack keeps the same object, and the geolocator only reads it. So `[0]` is the OLDEST detection in the window, not the newest. A comment above the site asserted the opposite and a 2026-08-06 commit changed a correct `[-1]` to `[0]` on the strength of it. Three other places in the estate already treat the same call as oldest-first (frame_processor's `hist[-N:]`, analytics' `hist[-1]`, aircraft_feed's `reversed(meas)`), so the convention was never in doubt anywhere else. `latest_delay_us` builds the ambiguity arc, and for single-node tracks track_gates replaces the icon position with that arc's midpoint. Reading the oldest detection therefore drew the aircraft up to a full window of differential range behind itself, of order 6 km at ~1 µs/s over a 20-detection window. Expected effect, corrected from the audit's framing: the accuracy endpoints will NOT simply improve. `_record_accuracy_sample` and `_refresh_node_verification` both score `solver_lat`/`solver_lon`, which track_gates captures BEFORE the arc-midpoint override, so they never saw the biased position. What does move is delay matching -- truth association gates on `abs(measured - expected) < 15 µs`, and a stale delay drifts outside that window -- so expect n_matched to rise, and mean error possibly with it as tracks that previously failed to match rejoin the sample. The arc is still anchored at the oldest epoch inside the solver itself (lm_solver_track's t0), which is a separate ticket. The fresh-fix injection writes the current position onto that same oldest epoch, so its guess carries a window of lag. Left as it stands, since LM refines the guess, with the ordering recorded in a comment so the site stops reading as though `[0]` meant newest. The test fixtures asserted the inverted convention deliberately, so they had to be rebuilt rather than adjusted: the module docstring stated it as a finding, `_event` took its timestamp from `detections[0]`, and the one ordering-sensitive fixture was written descending. Two cases are added that a single-detection event could not distinguish -- one for the multi-detection materialised path, which is what would have caught the second site, and one for the ADS-B identity evidence derived from the same value, since that authorises calibration points and its derivation was previously uncovered. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…(86cb9bqd9) tar1090 reports alt_baro as the literal string "ground" for aircraft on the ground, and readsb sends a null gs for aircraft reporting no velocity. A bare multiply on either raises, and nothing between `_run_geolocation` and `frame_loop`'s catch-all wraps it, so one such aircraft costs the entire frame: track association, the `state.adsb_aircraft` refresh and the archive append are all skipped, and `frames_processed` never advances. The record keeps its freshness stamp, so it stays inside the 60 s window and every subsequent frame dies the same way until it ages out. This is the failure already recorded at analytics_refresh.py and frame_processor.py, which took out /api/test/mlat-accuracy and, on retina-test, the whole map. The audit found two sites. There are four, and the two it missed are the ones that matter, because both hand the value to the geolocator, which multiplies alt_baro, gs and track bare and outside the solver's try: - the fresh-fix injection, which overwrites detection[0]'s tag; - every other detection's inline tag, which is what reaches the solver when the injection does not fire -- no live fix, one older than 60 s, or a hex whose case does not match, since the three writers normalise the key while the tracker carries `adsb["hex"]` raw. Coercing at both boundaries rather than in the geolocator: that is a separate submodule, and this repo should not hand a library a value the library cannot use. The library stays fragile for its other callers, which is a ticket of its own. The tag coercion rewrites only keys already present. The geolocator branches on `"gs" in adsb` to choose its velocity path, so adding a key would change behaviour rather than preserve it. Uses `_as_num` from the seeding path rather than the isinstance guard the audit prescribed, and lifts it to config.constants as `as_num` so there is one definition beside FT_TO_M instead of a private helper reached through a deferred import. It rejects NaN as well as strings; the prescribed variant passes NaN through, which would poison alt_m silently rather than loudly. The audit's third site, in core/state.py, was fixed upstream by e48cde1 the day after the audit was written. `_coerced_adsb` guards on `isinstance(tag, dict)`, not truthiness. A node can put anything in `frame["adsb"][i]` -- passive_radar copies it onto the detection with no type check, and `DetectionRequest.frames` is `list[dict]` with extras allowed, so nothing validates the entries inside a frame; `tcp_handler` already skips non-dicts on the same list. Unpacking a truthy non-dict raises `'str' object is not a mapping` from `_geolocate_track_event`, which sits outside both trys in the file, so the coercion would have caused the very frame loss it exists to prevent. Passing the value through restores what happened before this helper existed: the geolocator's `"lat" not in adsb` is a valid membership test on a string or a list, and yields no ADS-B guess. At the verification refresh the original was `float()`, not a bare multiply, so `as_num` narrows the domain as well as fixing it: a numeric-string altitude now reads as 0 rather than converting. That is deliberate. Every other reader of `state.adsb_aircraft` already resolves a numeric string to 0, so this makes the published truth agree with the altitude the pipeline itself used; and the alternative, a try/except around `float()`, would readmit NaN and inf into `altitude_error_m` silently, which is the failure mode as_num exists to avoid. The ticket's criterion is the whole backend, not the frame path, so the sweep was re-run across it and three more raw multiplies came up. The node-supplied tag in the known lane fails open since e48cde1, so a grounded aircraft there degrades that lane silently and ticks `known_claims_errors` rather than killing the frame. The truth candidate in the per-node verification refresh goes through `float()`, which raises ValueError rather than TypeError, and the caller's blanket except then leaves that node with no verification payload at all. `POST /api/test/validate` guards on truthiness, which "ground" passes. A fourth, the external-cache fallback in `_refresh_mlat_verification`, is coerced for the grep's sake only: that cache is written solely by periodic.py, whose entries carry alt_m and no alt_baro key at all, so there is no failure for a test to reproduce. The geolocator's own multiplies are not in the backend and stay on their own ticket. The two `or 0` sites on gs and track in `_tag_velocity` keep it. Every comment in the estate names alt_baro alone, that matches the readsb schema where the string sentinel is an altitude convention, and the only non-numeric gs or track anywhere in the repo is an invented test fixture. What is different at the two sites changed here is that the value crosses into code that cannot guard itself. `services/blah2_bridge.py` comes off the coverage omit list: it was excluded as needing live blah2 hardware, which the tests added with the timestamp guard show is no longer true. It now reports 89%. Every test here reproduces the production failure when its guard is removed. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…cb9ut1w)
tar1090 reports alt_baro as "ground" for an aircraft on the surface. That
is a statement about position, not altitude: the aircraft is at field
elevation, roughly 313 m at Atlanta and 0 m almost nowhere. Coercing the
sentinel to 0.0 is sound for a solver seed, where the value is a starting
point the solve moves off, and wrong at the two sites that score the
solver against it, where it charges the whole of the field elevation to
altitude error and drags the published mean down with it.
Before this branch, float("ground") raised and the caller's blanket except
dropped the node's entire verification payload, so a grounded aircraft
never reached altitude_error_m at all. Coercing the sentinel silently
admitted a class of aircraft the accuracy sample had never contained. This
restores that exclusion deliberately, and narrows it to the altitude term
alone: a grounded aircraft is good position and velocity truth, so it
stays in those samples, and only the altitude comparison skips it.
The predicate is is_num() on the raw field, not on as_num()'s output,
which flattens absent, non-numeric and a genuine 0 ft to the same 0.0. It
is not keyed on the literal "ground": under tar1090's schema a present
alt_baro that is not a finite number is the sentinel, and for a truth
comparison dropping any other non-numeric value is the conservative
reading. as_num() is now defined through is_num() so the two cannot drift.
Exclusion is null in both payloads, matching the else-None arm truth_alt_m
already carried. POST /api/test/validate gains n_altitude_samples, since
its mean now runs over a denominator smaller than the match count, and its
altitude aggregates report null rather than 0 when nothing carried
altitude truth: 0 m of error would read as a perfect altitude solve.
The frontend is unaffected. Its one altitude_error_m reader takes the MLAT
payload, built by a separate emitter from its own truth pool.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
`_refresh_mlat_verification` builds its truth pool from two sources. The external one was moved onto `as_num` earlier in this branch; the live one, twenty lines above it, was left on a hand-rolled isinstance test. That split was introduced here, so it is closed here. The two are not equivalent. `isinstance(v, (int, float))` admits NaN and infinity, which is exactly what `as_num` exists to reject and the reason this branch chose it over the guard the audit prescribed. `json.loads` parses a bare `NaN` literal by default, so a node emitting non-standard JSON puts a genuine float NaN into `state.adsb_aircraft`, and the live pool reads that store directly. A NaN in the truth pool does not raise. It propagates through every comparison as NaN, so the aircraft still matches, still publishes, and the altitude and velocity errors on /api/test/mlat-verification go quietly non-numeric. The regression test asserts the four published figures stay finite, and fails against the isinstance form. Also drops the incident narration from the comment above it, keeping only the constraint that still binds: the sentinel arrives, and an isinstance test lets NaN through. The history is in git. `_tag_velocity`'s docstring claimed it applied the same conversion as `state._adsb_for_seeding`. Moving that function onto `as_num` earlier in this branch made the claim false, so it now says what it actually does and points at the ticket that would close the gap. Both found by the automated review on the PR. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
4838bdf to
d3da82f
Compare
|
Claude finished @Babissimo's task in 1m 47s —— View job Claude finished reviewing PR #235Todo list
SummaryHEAD ( Re-verified independently rather than trusting that:
Outstanding finding (raised in the previous pass, still unaddressed)
_gs_ms = (adsb.get("gs", 0) or 0) * 0.514444
_trk = math.radians(adsb.get("track", 0) or 0)
existing.alt_m = as_num(adsb.get("alt_baro")) * FT_TO_M # <- this line was fixed by this PR(the second site at
Existing tests only cover the Everything else re-checked, no issues
|
2 participants
Tranche 1 of the pipeline accuracy audit. Parent: 86cb9bp4q.
Tickets: 86cb9bqkh, 86cb9bqgf, 86cb9bqd9, 86cb9ut1w.
Independent of the tracker PR (retina-tracker#24); the submodule pointer is untouched here and its bump is a separate PR.
Four changes
Only forward blah2 timestamps are accepted (
86cb9bqkh). The guard wasts_ms != last_ts, which catches an exact repeat but passes an older frame straight through to the tracker. Nothing betweenstate.frame_queueandTracker.process_framere-orders or checks monotonicity.>is safe here only because_convert_frame's staleness gate already clamps every accepted timestamp to withinSTALE_THRESHOLD_Sof our own clock, bounding a backwards clock step to a ~20 s self-healing stall rather than a permanent one. That coupling is recorded at the call site, because86cb9bqj3edits the line directly above it.This reduces one source of out-of-order frames; it does not eliminate them.
FRAME_WORKERSruns several unkeyed workers over one queue, so two frames from the same node can still reach the same tracker out of order. That is the per-node executor work, deliberately out of this tranche.The newest detection's delay is published, not the oldest (
86cb9bqgf).Track.get_recent_detectionsbuilds its result newest-first then reverses, so it returns oldest-first. A comment asserted the opposite and a 2026-08-06 commit changed a correct[-1]to[0]on the strength of it. Three other places in the estate already treat the same call as oldest-first.latest_delay_usbuilds the ambiguity arc, and for single-node trackstrack_gatesreplaces the icon position with that arc's midpoint, so the aircraft was drawn up to a full window of differential range behind itself, of order 6 km.Corrected expectation, which differs from the audit's: the accuracy endpoints will not simply improve.
_record_accuracy_sampleand_refresh_node_verificationboth scoresolver_lat/solver_lon, captured before the arc-midpoint override, so they never saw the biased position. What moves is delay matching, since truth association gates onabs(measured - expected) < 15 µs. Expectn_matchedto rise, and mean error possibly with it as tracks that previously failed to match rejoin the sample.ADS-B fields are coerced before arithmetic (
86cb9bqd9). tar1090 reportsalt_baroas the literal string"ground"; readsb sends a nullgs. A bare multiply raises, nothing between_run_geolocationandframe_loop's catch-all wraps it, and the record keeps its freshness stamp, so one such aircraft costs every frame until it ages out. Already recorded twice in this codebase as having taken out/api/test/mlat-accuracyand, on retina-test, the whole map.The audit found three sites. There were eight, and one of its three was already fixed by #233 the day after it was written. The two it missed that matter most hand the raw value to
retina-geolocator, which multiplies it bare outside the solver'stry, so it kills the frame from inside the library; one of those is reachable precisely when the obvious site is not. A seventh usedfloat()and so raisesValueError, which a grep shaped on the knownTypeErrormisses entirely.Grounded aircraft are dropped from the altitude truth term (
86cb9ut1w). Coercing"ground"to0.0is right for a solver seed and wrong for a truth comparison:"ground"means on the surface, not 0 m MSL, about 313 m at Atlanta field elevation. It also silently changed the sample population, since these aircraft were previously excluded by crashing. They are now excluded deliberately from altitude while still counted for position and velocity.services/blah2_bridge.pyalso comes off the coverage omit list, where it was excluded as needing live hardware. It now reports 89%.Verification
Full suite exit 0 under randomised order, coverage 82.5%,
pre-commit run --all-filesgreen, node contract confirmed current. Every production change is mutation-tested: revert it and a named test fails, reproducing the originalTypeErrororValueError.blah2_bridge_taskhad no coverage at all before this (it is imported only bymain.py), so the timestamp guard's behaviour was unobservable in either direction.Known follow-ups, ticketed not folded in
86cb9t72j the geolocator still multiplies raw fields and is fragile for its other callers, guarded here at the boundary. 86cb9v3jt a third grounded-aircraft altitude site remains in the MLAT verification path, and it is the one the map UI renders. 86cb9t7c4
gs/trackarithmetic elsewhere in the backend, no evidence it is live.Note for reviewers
backend/config/constants.pyoverlaps with the ADS-B truth query-regions branch. My hunk is additive apart from one module-docstring line; happy to be the one who rebases.🤖 Generated with Claude Code