Skip to content

Cache the single-node ellipse arc per track - #138

Merged
Babissimo merged 3 commits into
mainfrom
arc-cache-single-node-ellipse
Jul 28, 2026
Merged

Cache the single-node ellipse arc per track#138
Babissimo merged 3 commits into
mainfrom
arc-cache-single-node-ellipse

Conversation

@Babissimo

@Babissimo Babissimo commented Jul 28, 2026

Copy link
Copy Markdown
Contributor

Summary

Adds a backend compute-cache that memoises _build_single_node_arc per (hex, node_id) track, fingerprinted on (delay_us, lat, lon) (None-safe, tight epsilon) and evicted each flush to the set of currently-seen keys. This removes the redundant per-flush arc recompute that produced the ~4s map-ellipse pulse for single-node tracks.

ClickUp: 86caxu6vc (subtask of umbrella 86cau8uu2)

Changes

  • b98b494 — cache plus _cached_single_node_arc helper, with unit tests
  • 6552887 — wire the memo into build_combined_aircraft_json, evict per flush, integration tests
  • 6fb4ab3 — drive the flush-loop cadence from AIRCRAFT_FLUSH_INTERVAL_S (previously dead) instead of a hardcoded sleep(1.0), correct the docstrings to ~1 Hz, drop the now-redundant vulture whitelist entry

The flush/broadcast loop stays at 1 Hz. Real RETINA nodes report detections at ~2 Hz, but the server broadcast is deliberately throttled to 1 Hz; the cache removes the redundant per-flush recompute independently of either rate.

Benchmark

Single-threaded build-compute only (not full prod wall-clock), median build_combined_aircraft_json time over a fixed 200-node synthetic fleet. The axis is the fleet --interval (seconds between fresh detections per track), which maps directly to cache hit rate:

Fleet interval main branch speedup
40 s (≈39/40 cache hits) 87.4 ms 5.9 ms 14.9x
1 s (≈all misses) 92.4 ms 89.8 ms ~1.03x

At the real ~2 Hz node cadence the detection changes every flush, so the cache is invalidated almost every flush and helps only idle/aging tracks (the 1 s row is a close proxy). The clear win is the 40 s test fleet (testmap), which is what this branch is for.

Testing

Full backend suite green: 991 passed, 81% coverage (the 1 Hz reconciliation changes only the interval constant's value and two docstrings, which no test references).

Deferred (non-blocking, tracked)

  • Node geometry sits outside the fingerprint. Safe: node reconfig is not a live operation and is guarded by the <=120s stale prune.
  • The cached arc list is aliased by identity into the emitted dict. Safe today (read-only consumer).

Follow-ups filed: 86cawjevd (realistic-cadence validation plus the GIL/compute fix), 86cawmutv (frontend coalesce), 86caxc0yg (radar-only to ADS-B re-key duplication / hex_id collisions).

@claude

claude Bot commented Jul 28, 2026

Copy link
Copy Markdown

Claude finished @Babissimo's task in 3m 24s —— View job


Code Review

Tasks

  • Read trigger request and repo guidelines
  • Review diff (commits ffe5fe4..2ddf5da)
  • Read changed source files in full context
  • Check tests for adequacy
  • Post review feedback

CI is green (991 passed). I was not able to execute the test suite myself in this sandbox (bash/pytest execution requires approval that isn't grantable here) — review below is static, informed by CI's passing result.

Summary

This is a well-scoped, well-documented change. _build_single_node_arc is genuinely a pure function of (delay_us, target_lat, target_lon, node_cfg), and the memo in frame_processor.py:567-611 keys/fingerprints/evicts exactly as the design doc (docs/superpowers/specs/2026-07-27-arc-caching-design.md) describes. I traced the call sites and confirmed:

  • _track_entry (frame_processor.py:667-…) is only reached once per ac_hex per build (guarded by seen_hex at lines 933/951), so the (ac_hex, node_id) key is never written twice in one flush.
  • build_combined_aircraft_json only ever runs on the single-worker _aircraft_flush_executor (aircraft_flush.py:18-19, max_workers=1), so the module-level _single_node_arc_cache dict has no concurrent-access hazard.
  • Eviction (frame_processor.py:1088-1090) correctly prunes anything not in _touched_arc_keys, bounding the cache to the live fleet with no timer.
  • Removal of AIRCRAFT_FLUSH_INTERVAL_S is clean — no remaining references anywhere in backend/.

Observations (non-blocking)

  1. Aliased arc list, already disclosed — the cached arc list is returned by reference and embedded directly into the emitted dict (ambiguity_arc = _cached_single_node_arc(...)), which also becomes state.latest_aircraft_json. The PR description already flags this as safe today because nothing mutates it in place. Worth keeping in mind if a future consumer (e.g. an in-place trim/transform on the served payload) is added later — that would silently corrupt the cached entry for subsequent builds. No action needed now, just flagging so it doesn't get forgotten when 86cawmutv (frontend coalescing) lands.
  2. Node-geometry outside the fingerprint, already disclosed — a live node_cfg change (rx/tx/beam) wouldn't invalidate a cached arc for that node_id until some other input also changes. PR body correctly notes this is fine because node reconfig isn't a live operation and is bounded by the 120s stale prune.
  3. Both of the above match the "Deferred (non-blocking, tracked)" section in the PR body, so nothing here is new — just confirming the review reaches the same conclusion the author already documented.

Tests

backend/tests/test_arc_builder.py covers exactly the matrix in the design doc's §8: hit/miss on delay, miss on position beyond epsilon, jitter-within-epsilon hit, None-delay caching, cached-vs-fresh purity, and the integration-level eviction + zero-rebuild-on-unchanged-fleet behavior. That's solid coverage for a compute-cache — no gaps I'd ask for.

Nice work — the design doc anticipated essentially every correctness question I'd otherwise raise (hex-collision safety, re-key duplication, cache growth bound).

@Babissimo
Babissimo force-pushed the arc-cache-single-node-ellipse branch from 2ddf5da to c30e66a Compare July 28, 2026 10:25
@claude

claude Bot commented Jul 28, 2026

Copy link
Copy Markdown

Claude finished @Babissimo's task in 3m 27s —— View job


Code Review

Tasks

  • Read trigger request and repo guidelines
  • Review diff (e69443d...HEAD, the branch's actual merge-base — shallow clone made origin/main unusable directly)
  • Read changed source files in full context
  • Check tests for adequacy
  • Post review feedback

Summary

The arc-caching core (_cached_single_node_arc, eviction, integration into build_combined_aircraft_json) is unchanged since the prior review pass and still holds up — key/fingerprint/eviction logic is correct, single-worker executor means no concurrency hazard, and backend/tests/test_arc_builder.py covers the full matrix (hit/miss/jitter/None-delay/purity/eviction/behavioral). Nothing new to add there.

This sync added one new commit, c30e66a ("run the aircraft flush loop at 2 Hz via the interval constant"), which is worth a closer look.

Finding: flush-cadence change contradicts this PR's own design doc

c30e66a changes backend/config/constants.py:34 from AIRCRAFT_FLUSH_INTERVAL_S = 1.0 to 0.5, and wires it into the sleep in backend/services/tasks/aircraft_flush.py:125 (previously hardcoded asyncio.sleep(1.0)). This is a real, system-wide behavior change: the aircraft.json write + WebSocket broadcast loop now genuinely runs at 2 Hz instead of 1 Hz — double the disk writes and double the WS broadcast frequency to every connected client, not something scoped to arc caching.

That would be fine on its own merits, except this same PR ships a design doc and implementation plan that explicitly documented the opposite decision:

  • docs/superpowers/specs/2026-07-27-arc-caching-design.md:134-137: "Fix the aircraft_flush.py docstrings that claim '~2 Hz' while the loop sleeps 1.0s → '~1 Hz' ... Remove the unused AIRCRAFT_FLUSH_INTERVAL_S..."
  • docs/superpowers/plans/2026-07-27-arc-caching.md:440-441: "The flush loop sleeps 1.0s (~1 Hz) but its docstrings claimed ~2 Hz, and AIRCRAFT_FLUSH_INTERVAL_S was unused..."
  • Both docs explicitly mark realistic 1-2 Hz cadence validation as out of scope for this branch (tracked in 86cawjevd).

An earlier commit on this branch (2ddf5da, described in the PR body) implemented exactly that documented plan — correct the docstrings to ~1 Hz, drop the dead constant. c30e66a reverses it: instead of fixing the docs to match the (previously real) 1 Hz behavior, it changes the code to match the docstrings, doubling the actual cadence. The commit message justifies this on new grounds ("Real RETINA nodes report detections at ~2 Hz"), which may well be correct — but the design doc and plan files committed in this same PR still describe the old, now-contradicted decision and weren't updated to match.

Two independent things worth reconciling before merge:

  1. Docs vs. code: docs/superpowers/specs/2026-07-27-arc-caching-design.md §7 and docs/superpowers/plans/.../2026-07-27-arc-caching.md still tell a reader the "correct" fix was ~1 Hz + dead-constant removal, which is no longer what the code does. If the 2 Hz change is intentional and correct, those docs should be updated (or a follow-up note added) rather than left describing a superseded plan.
  2. Scope and impact on the PR's own numbers: doubling the flush cadence is unrelated to the ellipse-arc-cache feature and enlarges the blast radius of this PR (disk I/O + WS bandwidth for the whole aircraft feed, not just single-node tracks). It also sharpens the PR's own disclosed caveat — the benchmark table shows the cache is ~useless at 1 s intervals (~all misses); locking the flush loop to a true 2 Hz now makes that the realistic operating point rather than a pessimistic edge case, which is worth a line in the PR description so reviewers/future readers aren't surprised the map-pulse fix has diminished real-world effect at the cadence the code now actually runs at.

Neither point blocks on correctness grounds — the arc-cache logic itself is unaffected and still correct at any cadence per its design. This is a documentation/scope-consistency issue, not a bug.

Fix this →

Babissimo and others added 2 commits July 28, 2026 11:42
The flush build calls _build_single_node_arc for every aircraft every
cycle, uncached, and that pure-Python scalar-trig recompute is what
overruns the ~1s flush and pulses the map ellipses. Add a per-(hex,node)
memo fingerprinted on the arc's non-static inputs (delay, lat, lon) so it
rebuilds only when a new detection changes them. This commit adds the
mechanism and its unit tests; wiring into the build follows.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Route the per-track arc through _cached_single_node_arc and prune the
cache each build to the (hex, node) keys actually seen, so an unchanged
fleet does zero arc rebuilds and cache size stays bounded to the live
fleet without a timer. The arc is still emitted every flush, so the
frontend afterglow is unchanged.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@Babissimo
Babissimo force-pushed the arc-cache-single-node-ellipse branch from c30e66a to 11322d3 Compare July 28, 2026 10:42
@claude

claude Bot commented Jul 28, 2026

Copy link
Copy Markdown

Claude finished @Babissimo's task in 2m 52s —— View job


Code Review

Tasks

  • Read trigger request and repo guidelines
  • Review full diff (e69443d...HEAD, the branch's actual merge-base — shallow clone makes origin/main diff unusable directly)
  • Read changed source files in full context
  • Check tests for adequacy
  • Post review feedback

Summary

Re-reviewed at HEAD (11322d3). The arc-cache core is unchanged since the earlier passes on this PR and still holds up on a fresh read:

  • _cached_single_node_arc (backend/services/frame_processor.py:579-611) keys on (ac_hex, node_id) and fingerprints exactly the non-static inputs (latest_delay_us, lat, lon), None-safe for ADS-B-only tracks.
  • The sole call site that feeds the per-track "single-node ellipse" (frame_processor.py:706-708) routes through the memo; the unrelated "pending arcs" path (frame_processor.py:1063, radar-only detections without a known position) calls _build_single_node_arc directly and already has its own independent 5s cache (_ARC_REFRESH_S) — untouched by this PR, no interaction with the new memo.
  • build_combined_aircraft_json only ever runs on the single-worker _aircraft_flush_executor (max_workers=1 in aircraft_flush.py:19-21), so the module-level _single_node_arc_cache dict has no concurrent-access hazard.
  • Eviction (frame_processor.py:1088-1090) prunes anything not in _touched_arc_keys from this build, bounding the cache to the live fleet with no timer.
  • backend/tests/test_arc_builder.py's new TestSingleNodeArcCache / TestArcCacheIntegration classes cover hit/miss on delay, miss on position beyond epsilon, jitter-within-epsilon hit, None-delay caching, cached-vs-fresh purity, eviction on hex drop, and zero-rebuild on an unchanged fleet — solid coverage for a compute-cache.

Correction to my prior review pass on this PR

My previous pass on this branch cited specific line numbers in docs/superpowers/specs/2026-07-27-arc-caching-design.md and docs/superpowers/plans/2026-07-27-arc-caching.md to argue the 2 Hz flush-cadence commit (c30e66a/11322d3) contradicts this PR's own design doc. On this checkout, neither file exists anywhere in the branch's tracked history (git ls-tree/git log on those paths turn up nothing, and the 5-file diff vs. the merge-base — constants.py, frame_processor.py, aircraft_flush.py, test_arc_builder.py, vulture_whitelist.py — matches the PR's own stated file list, with no docs/ entries). The PR description states the design spec and plan "are included on the branch under docs/superpowers/," but they aren't part of any commit reachable from this branch in this checkout. I can't verify my earlier line-number citations against what's actually committed, so treat that specific claim from my prior review as unconfirmed rather than fact. If those docs exist locally for the author but were never pushed, worth adding them (or dropping the "included on the branch" wording from the description).

Still-valid, non-blocking observation

Independent of the doc question above, c30e66a (backend/config/constants.py:34, backend/services/tasks/aircraft_flush.py:125) doubles the aircraft-flush cadence from a hardcoded 1.0s sleep to AIRCRAFT_FLUSH_INTERVAL_S = 0.5s — a real, system-wide change (2x aircraft.json disk writes, 2x WebSocket broadcast frequency to every connected client) bundled into a PR framed around single-node arc caching. On correctness grounds this is fine and the commit message's rationale (RETINA nodes report at ~2 Hz) is reasonable. Two things worth a line in the PR description before merge, since they shape how reviewers should read the benchmark table:

  1. This changes production behavior for the whole aircraft feed, not just single-node tracks — worth calling out explicitly as in-scope rather than leaving it to be inferred from the commit log.
  2. It sharpens the PR's own disclosed caveat: the benchmark shows the cache is ~all-misses at 1s intervals. Locking the flush loop to a true 2 Hz makes that row the realistic production operating point rather than a pessimistic edge case — i.e. in production this cache mainly helps idle/aging tracks between fresh detections, not the common case. Not a reason to hold the PR, just worth being explicit about the expected real-world win.

Neither point is a correctness issue — the arc-cache logic itself is correct at any flush cadence.

The aircraft.json write + WebSocket broadcast loop runs at ~1 Hz. Replace
its hardcoded sleep(1.0) with the AIRCRAFT_FLUSH_INTERVAL_S constant (which
was dead, kept alive only by the vulture whitelist), correct the docstrings
that still claimed 2 Hz, and drop the now-redundant whitelist entry (the
constant is referenced now, not dead).

Real RETINA nodes report detections at ~2 Hz, but the server flush stays a
deliberate 1 Hz; the arc cache earlier in this branch removes the redundant
per-flush arc recompute independently of that rate.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@Babissimo
Babissimo force-pushed the arc-cache-single-node-ellipse branch from 11322d3 to 6fb4ab3 Compare July 28, 2026 12:11
@claude

claude Bot commented Jul 28, 2026

Copy link
Copy Markdown

Claude finished @Babissimo's task in 1m 39s —— View job


Code Review

Tasks

  • Read trigger request and repo guidelines
  • Review full diff (e69443d...HEAD, the branch's actual merge-base — shallow clone makes origin/main diff unusable directly)
  • Read changed source files in full context
  • Check tests for adequacy
  • Post review feedback

I was not able to execute pytest myself in this sandbox (command execution requires approval that isn't grantable here) — this review is static, informed by CI's passing result (991 passed).

Summary

Re-reviewed at HEAD (6fb4ab3). The arc-cache core is unchanged since earlier passes on this PR and still holds up:

  • _cached_single_node_arc (backend/services/frame_processor.py:579-611) keys on (ac_hex, node_id) and fingerprints exactly the non-static inputs (latest_delay_us, lat, lon), None-safe for ADS-B-only tracks.
  • _track_entry calls round(track.lat, 6) unconditionally just above the cache call (frame_processor.py:674), confirming track.lat/lon are never None at that call site — the fingerprint's round(...) calls can't raise.
  • build_combined_aircraft_json only ever runs on the single-worker _aircraft_flush_executor (max_workers=1), so the module-level _single_node_arc_cache dict has no concurrent-access hazard.
  • Eviction (frame_processor.py:1088-1090) prunes anything not in _touched_arc_keys from the current build, bounding the cache to the live fleet with no timer.
  • backend/tests/test_arc_builder.py's TestSingleNodeArcCache / TestArcCacheIntegration cover hit/miss on delay, miss on position beyond epsilon, jitter-within-epsilon hit, None-delay caching, cached-vs-fresh purity, eviction on hex drop, and zero-rebuild on an unchanged fleet — solid coverage for a compute-cache.

Previous finding: resolved

My prior review pass flagged commit c30e66a/11322d3, which doubled the aircraft-flush cadence to a true 2 Hz (AIRCRAFT_FLUSH_INTERVAL_S = 0.5) — a real, system-wide behavior change (2x disk writes, 2x WS broadcast frequency) bundled into a PR framed around single-node arc caching.

The latest commit, 6fb4ab3 ("chore: drive the flush-loop cadence from the interval constant (1 Hz)"), resolves this cleanly: AIRCRAFT_FLUSH_INTERVAL_S stays at 1.0, the loop's asyncio.sleep(1.0) is now driven from the constant instead of hardcoded, docstrings are corrected to ~1 Hz consistently (aircraft_flush.py:1,122), and the now-referenced constant is dropped from vulture_whitelist.py. No remaining 2 Hz references anywhere in backend/ — checked. This is exactly the kind of dead-code/docstring cleanup the design intended, without changing production cadence. Good fix.

Remaining observations (non-blocking, already disclosed in the PR body)

  1. Aliased arc list — the cached arc list is returned by reference and embedded directly into the emitted dict, which becomes state.latest_aircraft_json. Safe today since nothing mutates it in place; worth remembering if a future consumer (e.g. 86cawmutv frontend coalescing) ever does an in-place transform on the served payload.
  2. Node geometry outside the fingerprint — a live node_cfg change wouldn't invalidate a cached arc until some other input also changes. Fine because node reconfig isn't a live operation and is bounded by the 120s stale prune.

Both match the PR's own "Deferred (non-blocking, tracked)" section — nothing new here.

Tests

Coverage is thorough and matches the cache's correctness matrix (hit/miss/jitter/None-delay/purity/eviction/behavioral). No gaps to flag.

Nice work resolving the cadence issue — the arc-cache feature and the flush-loop cleanup are now cleanly separated in effect (cadence unchanged, only the dead-constant/docstring drift is fixed).

@jonnyspicer jonnyspicer left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm, this method of testing is a bit funny, at some point we'll want some sort of observability on the frontend to detect slowdowns but not urgent

@Babissimo
Babissimo merged commit a2cd924 into main Jul 28, 2026
12 checks passed
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.

2 participants