tracker: clamp frame dt and reject non-positive-definite gates (86cb9bq7p) - #24
Conversation
|
Claude finished @Babissimo's task in 3m 16s —— View job Claude finished the code reviewTask list
SummaryThis is a solid, well-reasoned fix. I traced the three changes against the actual code paths rather than just the PR description, and they hold up:
Sylvester's criterion (
Cost lower bound ( Tests ( No correctness bugs found. One non-blocking observation:
Verification noteI was not able to execute |
|
Noting the one observation so a human reviewer does not re-raise it: The review is right that a counter reachable only by direct attribute access on the On the verification note: CI ran Thanks for tracing the |
Babissimo
left a comment
Babissimo
left a comment
There was a problem hiding this comment.
Read through and ran it. The approach is right; one point below should be settled before this lands.
The approach
The diagnosis holds at the level that matters. F P Fᵀ is PSD by congruence for any real F, so it cannot be the source, while Q carries q·dt³/3 and q·dt on its diagonal, both negative for dt < 0, making each block negative definite with a positive determinant. That is exactly the state described, so the invalid input is dt itself and clamping at the boundary is the minimal fix rather than a workaround.
Sylvester over the audit's prescribed det_S > 1e-15 is a genuine improvement on what was asked for, and the justification checks out independently: both init paths are np.diag(...) (track.py:646, :650), F and Q are block-diagonal and (I − KH)P preserves that, so b = c = 0 structurally and det_S = a·d exactly.
The high-water mark needs a bound
math.isfinite shows the unpassable-mark hazard was seen, but it is bounded at infinity rather than at anything reachable. A finite implausible timestamp pins the clock permanently, every later frame clamps to dt = 0, and the filter never predicts again. Sixty ordinary frames after a single frame stamped a day ahead:
main clock back on the real base: yes live tracks: 1
this PR clock back on the real base: no live tracks: 10 dt clamped: 61 of 61
That is a trade rather than a plain regression, transient severe corruption exchanged for permanent silent degradation, but permanent and silent is the worse of the two to operate. It is reachable: tcp_handler.py:513 checks only that timestamp is present, never its value.
The bridge solves the same shape one layer up. In retina-server#235, ts_ms > last_ts is safe only because _convert_frame's abs(age_s) > STALE_THRESHOLD_S (blah2_bridge.py:248) sits above it, and the comment there says so. The tracker cannot use that gate, being a pure library driven by synthetic timestamps, so it wants the in-band equivalent: a run of backwards frames is the mark being wrong rather than the frames.
MAX_BACKWARDS_RUN = 3
...
self.n_backwards = self.n_backwards + 1 if not raw_dt > 0 else 0 # `not >` so NaN counts
...
resync = self.n_backwards >= MAX_BACKWARDS_RUN
if math.isfinite(timestamp) and (self.last_timestamp is None or timestamp > self.last_timestamp or resync):
if resync:
self.n_clock_resyncs += 1
self.n_backwards = 0
self.last_timestamp = timestampMeasured against that case, and against two clocks 30 s apart interleaved into one tracker, which is what nodes sharing default_pipeline produce:
wedge: on base / tracks interleaved: tracks / clamped / resyncs
this PR no / 10 2 / 60 / n/a
with the run bound yes / 1 2 / 60 / 0
Full recovery, four frames of frozen prediction instead of forever, and the interleaved case is untouched because alternating clocks reset the counter rather than accumulate it. All 75 existing tests pass unchanged, test_frame_after_a_clamped_one_gets_the_ordinary_dt included, which is the property the mark was added for.
A run of three rather than one is load-bearing: at one this is just deleting the mark, and the next frame computes the whole excursion as its dt, inside the bounds so neither clamped nor counted.
Capping how far the mark may advance per step is the other candidate and is worse. After a genuine long outage it crawls forward 60 s per frame, with dt pinned at the clamp and the gate at its widest the whole way.
Worth counting resyncs separately from clamps: clamping says frames are arriving out of order, resyncing says a node changed time base.
Smaller
MAX_FRAME_DT's comment rests on empty frames being dropped upstream, which is the audit's §3 defect. Once empty frames pass through, the tracker sees roughly 1 s cadence and 60 s stops being routine, so the comment reads as wrong from that point on. Worth pointing it at 86cb9bqj3 so it is revisited rather than surviving as folklore.
n_dt_clamped has no consumer in either repo yet. Tracked separately, noting it here only so it is not assumed covered by #235, which fixes the bridge guard but does not read the counter.
For the record, I checked whether the bogus timestamp also poisons the merge working set (all_tracks drains on death_timestamp) or the track ID date prefix. Neither reproduces: that frame's track stays tentative and never gets an ID. The clock wedge is the whole of it.
93c736f to
7146667
Compare
|
Claude finished @Babissimo's task in 3m 1s —— View job Claude's review of PR #24Task list
SummaryThis push resolves the one substantive concern raised in Babissimo's own review comment (the unbounded high-water mark) and goes further by hardening non-finite-timestamp handling. Both changes are correct and well tested.
This is exactly the mechanism proposed in the review thread: The two edge cases the reviewer's own writeup called out are both covered and correct:
Non-finite timestamp now rejects the whole frame ( This is a bigger change than the "clamp dt to 0" approach the original review verified — it now rejects the frame before computing One consequence worth being aware of (not a bug, just noting it's deliberate): a rejected non-finite frame still increments Minor:
No correctness issues found in the updated logic. The test coverage in Verification noteAs with the prior review pass, I could not execute |
7146667 to
c4955d2
Compare
|
Claude finished @Babissimo's task in 2m 17s —— View job Claude's review of PR #24Task list
SummaryThis push resolves the one non-blocking observation from the prior review pass. No new issues found.
The previous review noted that a non-finite-timestamp rejection and an out-of-order-but-usable clamp were both being folded into I re-traced the rest of the logic against this exact commit (
No correctness issues found in this diff. Verification noteAs with prior passes, I could not execute |
…bq7p) One chain, three links, no symptom at any of them: an out-of-order frame makes dt negative, a negative dt makes the innovation covariance non-positive-definite, the gate check tested the wrong property so such a track kept gating, and its Mahalanobis distances came out negative -- which wins a minimisation outright. The corrupted track then took detections from every healthy track it could reach, with no exception, no log and no counter. In a 40-frame run it absorbs another target's detections permanently and ends up ACTIVE, positive definite and indistinguishable from healthy, reporting the wrong aircraft. dt is clamped to [0, 60] and clamps are counted in `n_dt_clamped`. Zero rather than a nominal step for the lower bound: at dt=0, F is the identity and Q is zero, so the predict is a bitwise no-op and the update runs from the last good state. An out-of-order frame carries no usable time relationship, and substituting a nominal +0.5 s would advance the prediction in the wrong temporal direction while inflating P, loosening the gate at exactly the wrong moment. Repeated zero-dt cycles decay P harmonically rather than geometrically and one ordinary frame reinflates it, so there is no collapse risk. Nothing divides by this dt; the four anomaly detectors compute their own from raw timestamps and already guard `dt > 0`. Sixty for the upper bound, which is a cost trade rather than a safe point. Q grows as dt cubed, so the gate already spans the whole observed delay axis by about 23 s and no tolerable bound prevents that -- only wall-clock track ageing does, and that is a ticket of its own. What the bound decides is which failure you take: the clamp under-predicts by the truncated interval while injecting only the truncated process noise, so too tight a bound is its own regression. At 10 s tracking dies once a real gap exceeds ~20 s, and since empty frames are dropped upstream, multi-second droughts are ordinary -- that would cause by another route the very behaviour change the empty-frame ticket defers. Sixty keeps drought recovery to ~120 s. The clock now advances as a high-water mark rather than to whatever the last frame happened to carry, because clamping alone closes only half the path. A frame 6 s late is clamped to 0 and counted, but still leaves `last_timestamp` at T-6 s, so the next legitimate frame at T+1 computes 7 s. That is inside the bounds, so it is neither clamped nor counted, and Q[0,0] comes out 343x too large with a gate to match: one out-of-order frame corrupted two frames while the counter owned up to one. Nothing depends on the old semantics -- `last_timestamp` is read only to form this dt and as the clock in tests, and no other repo touches it. A non-finite timestamp is rejected outright, at the top of process_frame, before anything reads it. `min(max(nan, 0.0), 60.0)` returns nan, because Python's min and max yield the first operand whenever the comparison is false and every comparison against a NaN is, so the derived dt needed a guard either way. Guarding the dt and the clock alone was not enough: the raw stamp also reaches track IDs, `death_timestamp` and the merge-window cutoff. A NaN there sets `death_timestamp` on every coasting track, so `get_quality_score` returns nan; it makes the cutoff nan, so every comparison against it is false and `all_tracks` drains into `completed_tracks` in a single frame, emptying the merge working set; and on the frame a tentative track promotes, `datetime.fromtimestamp` raises straight out of `process_frame`, ValueError for a NaN and OverflowError for an infinity. One rejection at entry closes all three, and keeps the guard in one place rather than in each consumer of the stamp. Rejections are counted in `n_frames_rejected` rather than folded into `n_dt_clamped`: an unusable frame and an out-of-order but usable one are different failures, and a single counter for both cannot tell an operator which is happening. The mark yields after MAX_BACKWARDS_RUN consecutive non-advancing frames, because a one-way ratchet is a failure of its own. Barring an infinity from the mark is not sufficient: any implausible but finite future stamp pins it just as well, and every later frame then clamps to dt=0, so the filter never predicts again. Nothing upstream stops one arriving; the TCP ingest path checks only that a timestamp is present, never its value. Measured over 60 ordinary frames after a single stamp a day ahead: with the bare ratchet the clock stays pinned, 61 of 61 frames clamp and one aircraft fragments into 10 tracks, permanently; with the run bound it resyncs once and recovers to a single track. Three rather than one, because at one this is just deleting the mark, and because two interleaved node clocks alternate rather than run, so they keep it. The rationale above lives here rather than beside the code, because `CLAUDE.md` and `.claude/rules/code-style.md` both bar comments outright and ask names and structure to carry the intent. So `MAX_FRAME_DT` gains its unit as `MAX_FRAME_DT_S`, `MAX_BACKWARDS_RUN` says what it triggers as `BACKWARDS_RUN_BEFORE_RESYNC`, and the gate's mask is `pos_def` rather than `valid`. Both constants are new here, so neither rename can reach a consumer. Two things the deleted comments had been carrying are now carried by code instead. The gate omits the `d > 0` term of Sylvester's criterion, which is sound only while S is diagonal, so `TestGateStructuralInvariant` pins that the covariance stays block-diagonal through predict and update rather than leaving the precondition to prose: injecting a 1e-9 cross term into P fails it. And the entry guard is widened to `numbers.Real`, because `math.isfinite` raises off a real number rather than returning False, and `process_streaming_frame` reads the stamp out of an external frame without checking its type, so a node sending an ISO-8601 string crashed the frame rather than having it dropped. Test isolation goes in alongside, the suite having had none. `set_config` writes a module global that every accessor reads at call time, and with no conftest a file that set it supplied the config for every later file in collection order: 74 of the 75 tests ran under a config inherited from somewhere else. The fixture snapshots and restores it, reading the private global rather than `get_config()`, which assigns when the value is None and would corrupt the state it is meant to save. The gate now applies Sylvester's criterion rather than the sign of the determinant alone. The audit prescribed `det_S > 1e-15`, which is necessary but not sufficient: H P Hᵀ is symmetric and neither F nor Q couples delay to Doppler, so b = c = 0 and det_S = a*d exactly. Two negative diagonal entries therefore multiply to a positive determinant, and that -- not an indefinite matrix -- is what a late frame actually produces on a converged track inside the bridge's ±10 s staleness window. The determinant test cannot see it. The last test here fails with `det_S > 1e-15` and passes with `a > 0` added, which is the whole argument in one assertion. The cost lower bound goes in as prescribed. Costs are a Mahalanobis distance times a strictly positive SNR weight, so a negative cost is never legitimate however it arose. It cannot be reached through `_associate` while the gate holds, since a positive-definite S cannot yield a negative distance, and mutation-testing confirms as much: reverting it alone leaves the whole suite green. No test can protect it, so the comment beside it states the constraint instead: a distance times a positive weight is never negative. Association counts will shift slightly: detections that previously received a garbage negative distance are now excluded rather than preferred. That is the intent, not a regression. None of this had test coverage -- every existing frame steps 500 ms -- so the behaviours are pinned here rather than left to be rediscovered. Every production change is mutation-tested except the cost bound, which is the one the gate makes unreachable. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
c4955d2 to
42d897c
Compare
|
Claude finished @Babissimo's task in 6m 23s —— View job Claude's review of PR #24Task list
SummaryThis is the fourth review pass on this PR, and the code at dt clamp + non-finite rejection ( High-water mark + resync ( Sylvester's criterion ( Cost lower bound ( Tests — both new files are well-targeted at the exact failure modes in the PR body rather than generic coverage: e.g. CI — green for this exact commit: Carried forward from prior passes (non-blocking, already acknowledged by the author): No new issues found in this pass. The PR looks ready to merge on its technical merits. |
|
Claude finished @Babissimo's task in 2m 13s —— View job Claude's review of PR #24Task list
SummaryRe-verified the current HEAD ( Traced by hand against the code, not just the tests:
Test coverage is thorough and pins behavior rather than just "doesn't crash" — e.g. Verification
No blocking issues. This is ready from a code-review standpoint. |
An out-of-order frame made dt negative, which made F P Fᵀ + Q non-positive-definite. The association gate tested only that S was invertible, so such a track kept gating and its Mahalanobis distances came out negative, which wins a minimisation outright: the corrupted track took detections from every healthy track it could reach, with no exception, no log and no counter, and ended up reporting the wrong aircraft under the original track ID. This repo is where that bites. blah2_bridge's duplicate guard admitted an older frame, and FRAME_WORKERS drain a single queue with no per-node keying, so backwards timestamps reach a node's tracker under ordinary operation rather than as an edge case. The bump carries six intervening commits besides the fix. Five are tooling and formatting (ruff configuration, the vulture dead-code gate, running both through pre-commit) and one adds events.jsonl rotation, which this repo never reaches since it supplies its own InMemoryEventWriter. The fix is the only behavioural change that lands: across the range track.py sees import reordering plus one `if/return` pair collapsed to `return expr`, and config.py gains an additive `output` section with no existing default altered. offworldlabs/retina-tracker#24 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
1 participant
Ticket: 86cb9bq7p (the dt and gating halves; the per-node executor half stays open there)
Parent: 86cb9bp4q, pipeline accuracy audit of 2026-08-24
What this closes
One chain, three links, with no symptom at any of them.
An out-of-order frame makes
dtnegative. A negativedtmakesF P Fᵀ + Qnon-positive-definite. The gate tested only thatSwas invertible, so such a track kept gating. Its Mahalanobis distances then came out negative, and a negative cost wins a minimisation outright.The corrupted track takes detections from every healthy track it can reach, with no exception, no log and no counter. In a 40-frame run it absorbs another target's detections permanently and ends up
ACTIVE, positive definite and indistinguishable from healthy, reporting the wrong aircraft under the original track ID.Four changes
dtis clamped to[0, 60]and clamps are counted inn_dt_clamped.Zero rather than a nominal step for the lower bound: at
dt = 0the predict is a bitwise no-op (F = I,Q = 0), so the update runs from the last good state. An out-of-order frame carries no usable time relationship, and substituting a nominal+0.5 swould advance the prediction in the wrong temporal direction while inflatingP, loosening the gate at exactly the wrong moment.Sixty for the upper bound, as a cost trade rather than a safe point.
Qgrows asdt³, so the gate already spans the whole observed delay axis by about 23 s and no tolerable bound prevents that. What the bound decides is which failure you take: too tight and the clamp under-predicts while injecting only the truncated process noise. At 10 s, tracking dies once a real gap exceeds ~20 s, and empty frames are dropped upstream so multi-second droughts are ordinary. That would cause by another route the very behaviour change 86cb9bqj3 defers.A non-finite timestamp is rejected at entry to
process_frame, before anything reads it.min(max(nan, 0.0), 60.0)returnsnan, because Python'sminandmaxyield the first operand whenever the comparison is false, and every comparison against a NaN is. So the deriveddtneeded a guard either way. Guarding thedtand the clock alone was not sufficient: the raw stamp also reaches track IDs,death_timestampand the merge-window cutoff, and a NaN at each of those does separate damage.mark_missed(nan)setsdeath_timestampon every coasting track, soget_quality_score()returnsnan.nan, so everydeath_timestamp >= cutoffis false andall_tracksdrains intocompleted_tracksin a single frame, emptying the merge working set.datetime.fromtimestampraises straight out ofprocess_frame:ValueErrorfor a NaN,OverflowErrorfor an infinity.One rejection at entry closes all three and keeps the guard in one place rather than in each consumer of the stamp.
Rejections are counted in
n_frames_rejected, not folded inton_dt_clamped. An unusable frame and an out-of-order but usable one are different failures, and a single counter for both cannot tell an operator which is happening.The clock is a high-water mark that resyncs after
MAX_BACKWARDS_RUNnon-advancing frames.Without the high-water mark, a clamped late frame still rewinds the clock and the next frame computes the whole excursion as its
dt, inside the bounds so neither clamped nor counted.But a bare ratchet is a failure of its own. Barring an infinity from the mark is not enough: any implausible yet finite future stamp pins it just as well, and every later frame then clamps to
dt = 0, so the filter never predicts again. Nothing upstream stops one arriving, since the TCP ingest path checks only that a timestamp is present, never its value. Measured over 60 ordinary frames after a single stamp a day ahead:Three rather than one, because at one this is just deleting the mark, and because two interleaved node clocks alternate rather than run, so they keep it. An interleaved pair 30 s apart still gives 0 resyncs.
The gate applies Sylvester's criterion, not the sign of the determinant.
This deviates from the audit, which prescribed
det_S > 1e-15. That is necessary but not sufficient:H P Hᵀis symmetric and neitherFnorQcouples delay to Doppler, sob = c = 0anddet_S = a*dexactly. Two negative diagonal entries multiply to a positive determinant, and that, rather than an indefinite matrix, is what a late frame actually produces on a converged track inside the bridge's ±10 s staleness window. Measured: corruption fromdt = -6 sat snr 10, from-3 sat snr 20.test_corrupt_track_does_not_steal_a_healthy_track_detectionfails with the determinant form and passes witha > 0added. That single assertion is the whole argument.A lower bound on the accepted cost, as prescribed. Costs are a Mahalanobis distance times a strictly positive SNR weight, so a negative one is never legitimate. It cannot be reached through
_associatewhile the gate holds, and is kept as a second layer.Test isolation
The suite had none.
set_configwrites a module global that every accessor reads at call time, and with noconftest.pya file that set it supplied the config for every later file in collection order: 74 of the 75 existing tests ran under a config inherited from somewhere else. The new fixture snapshots and restores it, reading the private global rather thanget_config(), which assigns when the value isNoneand would corrupt the state it is meant to save.Expected effect
Association counts shift slightly. Detections that previously received a garbage negative distance are now excluded rather than preferred. That is the intent, not a regression.
Verification
96 tests (was 59),
pre-commit run --all-filesgreen. Reverting the entry rejection fails 11 tests; reverting the resync fails 3; reverting the counter split fails 3; reverting the clamp, the gate term and the high-water mark fails 9, 1 and 4 respectively. The cost bound stays green by construction, which is documented rather than hidden.Nothing in the
dt, gating or non-finite paths had coverage before: every pre-existing frame in the suite steps 500 ms, so negativedt, largedt, thedet_Ssign, negative costs and every non-finite case were all unexercised.Merge order
This merges first. A follow-up PR then bumps the submodule pointer in retina-server, which is currently pinned at
04ec70b(v0.2.1).Branched from
main(784d68d) rather than the pin, deliberately:retina_tracker/tracker.pyis byte-identical across that range, so there is no conflict risk and the audit's line numbers hold verbatim, while the pin predates.pre-commit-config.yamland so has no local gate at all. The bump will therefore carry six intervening commits: four ruff and dead-code tooling changes, and anevents.jsonlrotation the server never reaches, since it defines its ownInMemoryEventWriter.🤖 Generated with Claude Code