Accept a node that cannot say where it is - #278
Merged
Merged
Conversation
Babissimo
marked this pull request as draft
August 28, 2026 23:00
Babissimo
force-pushed
the
feat/nullable-node-coordinates
branch
2 times, most recently
from
September 7, 2026 12:27
0009fbd to
0de654a
Compare
This comment has been minimized.
This comment has been minimized.
Babissimo
force-pushed
the
feat/nullable-node-coordinates
branch
from
September 7, 2026 14:50
0de654a to
74f9dd0
Compare
This comment has been minimized.
This comment has been minimized.
Babissimo
force-pushed
the
feat/nullable-node-coordinates
branch
from
September 7, 2026 15:35
74f9dd0 to
c825652
Compare
This comment has been minimized.
This comment has been minimized.
Babissimo
force-pushed
the
feat/nullable-node-coordinates
branch
from
September 7, 2026 16:41
c825652 to
e97a1b0
Compare
This comment has been minimized.
This comment has been minimized.
rx_lat, rx_lon, rx_alt_ft, tx_lat, tx_lon and tx_alt_ft were required and
non-null, so an owner who cannot yet survey a node's geometry had no way
to register it at all. They join beam_width_deg and beam_azimuth_deg as
required-but-nullable: still keys every payload must send, still bounds-
checked when present, but null is no longer an error, since a substituted
coordinate would be wrong data the server could not later tell apart from
a real survey.
Latitude and longitude move as a pair per side, because a lone coordinate
places nothing and a half-supplied side is a bug upstream rather than a
state worth keeping. Altitude stays independently nullable: it is a small
term the geodesy already defaults to zero. The degenerate-baseline check
is guarded to fire only once both sides are present, so a node still
missing its far end no longer trips a check meant for two real points.
The bounds loop runs before the pair rule, so an out-of-range rx_lat whose
rx_lon is null is reported as out-of-range rather than as an unpaired
coordinate. That ordering is the useful one (the specific fault beats the
structural one) and is pinned by a test, since nothing else fixes it.
position_status folds has-rx/has-tx into the one value a caller needs
("positioned", "missing_rx", "missing_tx" or "missing_both"), rather than
four nullable fields every consumer would otherwise recombine itself. It
is keyed on latitude and longitude together for each side, so a config
carrying a latitude and no longitude reads as missing on that side rather
than as positioned. It also has to hold for a raw dict that never passed
through validate_config: connected_nodes configs are read directly, and a
legacy or bulk-ingested one may carry no geometry at all, or only one
coordinate of a pair.
_is_placed is total: a non-numeric coordinate (a string, a list, a bool)
reads as not placed rather than raising. It runs against unvalidated
connected_nodes configs on every analytics-refresh cycle, where a config
like {"tx_lat": "", "tx_lon": ""}, accepted verbatim by the bulk-detections
route, would otherwise take the whole cycle down: nodes and overlaps
payloads, accuracy stats, missed-detection tracking, per-node and MLAT
verification and stale pipeline eviction all stopping silently, every 30s,
for as long as that config stayed connected. position_status also treats a
coordinate pair at exactly (0, 0) as absent on either side, matching the
legacy broken-config sentinel rule retina-analytics applies, since
rx=(0, 0) with a real tx otherwise passed validation and read "positioned"
on the dashboard while the map, associator and solver all excluded the
node: unplaced and unflagged at once, the exact failure this feature exists
to prevent. Its return type is a Literal, so a fifth state cannot drift in
silently.
tcp_handler._validate_node_config is the second validation door, and it
takes the same view: an explicit rx_lat/rx_lon null is a positionless
registration rather than a "missing lat/lon" NACK of the exact state this
change exists to allow. A genuinely absent key still NACKs, unchanged.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
validate_config now accepts a null rx/tx lat, lon and altitude, but node_configs still declared all six NOT NULL, so a config that actually carried one would fail on insert rather than register. This closes that gap: the six columns become nullable, migration 0005 widens the constraint without touching a row that already exists, and the wire contract moves to 1.1.3. The table is append-only, one row per configuration version, because an archived detection refers to the version it was computed under. That is why the migration is a widening only: it does not backfill or normalise anything, and a row that declared (0, 0) stays exactly as declared. Downgrading past 0005 fails loudly if a positionless row exists rather than inventing coordinates for it, the same trade-off 0004 made for beam_width_deg. 0005 is graded rollback_safety = "destructive". The classifier's definition of safe is that the restored code does not read what a revision added, and nothing is added here, no column and no table. What changes is the value space of six columns that every line of pre-1.1.3 code has always read unconditionally as required floats. Once any node registers without coordinates, which 1.1.3 exists to allow and which continuous auto-accept makes routine rather than rare, that row is permanent: the database is then ahead in a way old code cannot safely serve, and the downgrade path fails on the same row rather than offering an escape. Both halves of that gap need a human, which is what "destructive" is for. 1.1.3 is a patch rather than a minor bump for the same reason 1.1.2 was: NodeConfig's fields are not published, so nothing about this is visible to a client, and RegisterRequest.config and the PUT /config body both stay free-form (additionalProperties: true) in the contract. Regenerating it changes only the version string. test_a_null_position_round_trips calls refresh() before reading the row back. The session is built with expire_on_commit=False, so get() or select() after commit returns the same identity-mapped Python object the test just constructed, never read from the database at all. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…ff the map The library's has_full_geometry/detection_area gating and validate_config's position_status now have something to join: pin retina-analytics to the commit offworldlabs/retina-analytics#24 landed on main and surface position_status on every node in /api/radar/nodes, which the dashboard reads. /api/auth/me/nodes carries it too, computed the same way: the published feed filters out private nodes, so a private positionless node's owner otherwise had no way to learn it needed a position. position_status is imported at module level in analytics_refresh, since node_config imports only math and typing and so cannot cycle with it. The new test file pins the two behaviours that must hold end to end: a null-geometry node is counted and visible, but builds no solver pipeline and joins no overlap zone. The pipeline guard's correctness is currently a truthiness accident (86cbavanm), and these assertions catch it if the repair regresses. Only a repair that checks key presence rather than value nullity would start building pipelines for positionless nodes: an is-not-None repair would not, since None is not None is False exactly as bool(None) is. The overlap test registers ten nodes, following test_unpositioned_registration's shape, so its zero-zones assertion can actually fail rather than hold vacuously for a single node. The _clean fixture enumerates its three node IDs explicitly rather than scanning connected_nodes for a name prefix. That scan is safe in test_unpositioned_registration, where every test registers through the real detections route, but test_positionless_node_is_counted_but_not_placed registers directly against node_analytics/node_associator and never touches connected_nodes, so the scan would not find it and the node would leak into the analytics and associator registries. Nothing would break today, because conftest's autouse reset wipes both before every test, but the fixture should not depend on that safety net. _CONFIG is a shared module-level dict and association.py keeps a bare reference to whatever it is passed rather than copying it, so the four call sites that hand it to something stateful pass dict(_CONFIG). "Counted" is asserted as record_detection_frame incrementing total_frames, not merely as the node having a metrics entry: what the owner needs to see is that a working receiver's frames are tallied even though it cannot be placed. Two comments elsewhere described the library as it was before this pin: a guard function that no longer exists, and a default-to-zero mechanism that no longer runs. Both now describe has_full_geometry's both-ends rule. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
position_status now reaches every dashboard surface that lists or details a node: a warning-style badge beside the liveness badge on NodeManagementPage (reusing the existing .badge.warning idiom, not a hand-rolled style), a banner above NodeDetailPage's RF configuration table, and a new "Needs Attention" section on OverviewPage that renders only when a node actually needs it. The badge renders nothing at all for a status outside its label map, rather than an empty chip. OverviewPage's list merges the owner's own nodes, from /api/auth/me/nodes, into the published payload. /api/radar/nodes drops a private node, so without that merge a private node with no position appears on no surface its owner reads, which is the one case the flag exists for. The copy is deliberately reassuring rather than alarming, and states the position fact (the node's detections are counted and archived) rather than current activity: /api/radar/nodes includes disconnected nodes, so it cannot promise that anything is being recorded right now. All three surfaces that tell an owner their node needs a position carried the same sentence separately, two of them already drifting apart; the badge exports the string and the other two import it, so they agree by construction. ConfigPage is deliberately not one of those surfaces. It renders /api/admin/config/nodes, which has two response shapes (nodes_config.json verbatim where that file exists, a payload built from connected_nodes otherwise), so position_status cannot be added to it server-side consistently: it would exist in the second shape only. Deriving it client-side would put a second definition of "positioned" beside services/node_config.py:position_status, which is exactly what this feature exists to remove, and would buy little, since that table already shows a missing coordinate in its own column. NetworkHealthPage's node-location map filtered nodes on `rx_lat && rx_lon`, which drops a node genuinely at latitude or longitude 0 the same way it drops one with no position at all. It now checks for null explicitly. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
get_or_create_node_pipeline fell back to the shared default pipeline for a node without a usable position (86cbavanm). Such a node's detections were geolocated against DEFAULT_NODE_CONFIG's fixed receiver and published on the public map stamped with whichever node last shared that pipeline, which included legacy nodes carrying no coordinates at all, registered through POST /api/radar/detections. It now returns None, and process_one_frame skips geolocation, tracking, association and solver-queueing for the frame while still counting it through record_detection_frame: an unplaced node stays visible and stays counted, and contributes nothing positional. The altitude default applies to an explicit null as well as an absent key, and without `or`, since 0.0 is sea level and a real altitude. A positioned node declaring a null altitude previously built no pipeline whatsoever and produced zero tracks for good, while reading as healthy throughout. The ADS-B seeding block gates on the node actually being positioned rather than merely present in the associator's geometry registry, which stores a NodeGeometry with coordinates coerced to 0.0 even for an unplaced node. KNOWN_LANE_MODE defaults to binding, so this lane was live: a positionless node's detections were matched against a fabricated Null Island baseline and the residual charged to the node's trust bias. Tests that reached the code under test through the old fall-through now register their node with a real position, through tests/node_helpers, or with a pre-seeded pipeline. That includes test_dark_follow's TestModesInProcessOneFrame, which registered with the associator alone: such a node is half-registered as far as the frame path is concerned, so it took the fall-through to reach the lane it names. It now captures the node's own pipeline, rather than pre-seeding node_pipelines with the shared default, which would hard-code the arrangement this commit abolishes. Two comments describing the fallback, in node_stream and in the test mirroring it, are corrected to match. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Consumers that read a position straight from a config predate both nullable coordinates and, in several cases, any care about latitude 0. Each is now keyed on the node being positioned, through position_status, rather than on a second null-handling scheme of its own. claim_known_targets gates on the node being positioned rather than on its presence in the geometry registry, for the same reason the frame processor's seeding block does: the registry holds coordinates coerced to 0.0 for a node that has none. Miss-rate statistics, solver-accuracy verification and the map and RF environment location displays stop treating a coordinate of exactly 0.0 as falsy, which silently dropped a fully positioned equatorial or prime-meridian node from each of them. _fetch_external_adsb needs no change of its own: region building already drops an absent or unusable position, in its node loop and again in regions_for_nodes, so the TypeError that would otherwise take the fleet's ADS-B ground truth cache offline cannot arise. Adding position_status there as well would be the second scheme this commit exists to remove. test_periodic_adsb_bbox pins that outcome end to end instead, because a positionless fleet is what this feature makes routine, and the failure is silent and unrecoverable: the caller's except never retries. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The comment on the beam-width default described a path that is never reached the way it claims: resolve_beam_width_deg already handles the null itself, and did so before this branch existed. ONBOARDING.md's backend setup installs all five libs editable, matching the worktree setup section below it and both justfile and ci.yml. An implementer working on this branch tripped on the two-lib version. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
"Absent", "null", "exactly (0,0)", "a numeric string", "NaN" and the legacy flat lat/lon spelling were each conflated differently across about eight places that had to agree and did not. Patching the leaf sites cannot settle that, because there is no single point where the shape is decided: every consumer re-derives it from unvalidated JSON, and a null altitude was read as 900 ft by the pipeline, as 0 by the consensus solver, and crashed the multinode solver outright. canonical_config in services/node_config.py is now that point. It folds the legacy spelling, coerces each coordinate to a float or None, nulls a half-given pair, collapses the (0,0) sentinel the NOT NULL columns once forced, and resolves both altitudes to floats. It is applied at all five writers of state.connected_nodes and again in register_node_blocking, the single door into both library registries. After it, no consumer can see anything but a float or None in a coordinate slot, and never None in an altitude slot, which makes those four disagreements unrepresentable rather than individually guarded. The predicates that were guarding for themselves collapse onto it: position_status becomes four is-None checks, and node_beam_params, track_gates, analytics_refresh and admin drop their falsy-coordinate spellings. That last one mattered: a transmitter on the equator or the prime meridian scored as no transmitter at all, so the node came out omnidirectional and its miss rate blew up. node_beam_params no longer coerces a missing coordinate to 0.0, so its callers must gate on placement first. All four in the backend already do; association_bench's copy of the solver's range/bearing rule now does too, which is what its docstring promises of it. The config_hash is still computed from the pre-canonical config at both the v1 and blah2 doors, and the TCP handler compares canonical against canonical, so no live node sees a spurious config-changed signal on deploy. The durable node_configs row is untouched: canonicalisation is what happens on the way into memory, not into storage. test_storage.py pins the archive against the regression this makes impossible, which would be silent and permanent: a node sending the legacy flat spelling, which tcp_handler still accepts, archiving null rx_lat and rx_lon despite being fully placed, because the snapshot was taken from a config that had not been folded. Parquet rows cannot be corrected once published, and a null there cannot afterwards be told apart from a node that genuinely declared no position. Four tests had been left vacuous, their node absent from connected_nodes so the pipeline was None and the path each named was never entered. They now register a node properly, through a shared helper that does what an entry point does. TestModesInProcessOneFrame had been repaired by pre-seeding node_pipelines with the shared default, which hard-codes the exact arrangement this change abolishes; it now captures the node's own pipeline. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
canonical_config was resolving a null altitude to its terrain figure, which
is right for the geodesy and wrong for everyone else: /api/radar/nodes and
the Parquet archive both read a config from connected_nodes, and both were
reporting 900 ft for a node that had declared nothing. Nothing downstream
can tell an invented figure apart from a survey, and archive rows are
permanent and not correctable once published.
Carrying a second dict beside the canonical one, and routing those two
consumers to it, would put the archive and the published location block on
an un-normalised payload: a node using the legacy flat lat/lon spelling,
which tcp_handler still accepts, would archive null geometry despite being
fully positioned, and a coordinate arriving as a numeric string would reach
public_latlon, which returns a non-number untouched and so would publish the
operator's true receiver position unfuzzed. Defaulting at the boundary
removes the need for a second form at all.
canonical_config therefore corrects and never invents: a coordinate becomes
the number it already was or the null it already meant, and an altitude
keeps its declared null. One shape is safe to solve on, to publish and to
archive, which is what the design spec asked for in §3.
node_config.resolve_altitudes is the boundary, applied at the three doors a
null cannot pass. get_or_create_node_pipeline subscripts the altitude and
hands it to passive_radar; get_node_configs, the solver's snapshot, has a
consumer that multiplies it by a metre conversion; register_node_blocking is
the single entry to both library registries, and the associator reads
`(config.get("rx_alt_ft") or 0) * 0.3048`. That third door matters most
quietly: spelled `or 0` rather than as a subscript, it survives a null
instead of raising, so an unsurveyed receiver would sit at sea level in the
registries while the pipeline and the solver placed the same node at 900 ft.
One missing altitude, two different answers, and neither figure is ever
printed. Neither registry publishes an altitude, so resolving at that door
cannot leak a working figure into a payload the way resolving on the way in
did.
resolve_altitudes lives in node_config beside the constant it reads and
opposite canonical_config, its counterpart; node_config is a leaf, so a
caller reaches it without pulling in the frame pipeline. It copies rather
than defaulting in place, or the working figure would be written back into
the dict the archive reads, and it keys on None rather than falsiness,
because a receiver at 0 ft is at sea level and not unsurveyed.
blah2_bridge stops substituting 0.0 for an altitude a node omits. That was
an invention on the way in, and it reached publication and the Parquet
archive as though surveyed; the key is left null now for resolve_altitudes to
fill at a door.
The config-hash test moves to the (0, 0) sentinel to keep its own guard
honest: it asserts the declared and canonical forms differ, and a null
altitude no longer makes them.
test_the_stored_config_is_canonical keeps its folding and coercion
assertions and gives up only the altitude one, which now belongs to the
geometry boundary rather than to canonical_config.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
get_node_configs returned every connected node, so nothing drawn from it could assume placement. The FOV block checked for itself; _stamp_contamination did not, and judged an unplaced node's coerced (0, 0) geometry against the solve; known_lane's dark-follow claim filter kept a claim on `nid in node_cfgs` under a comment saying the node was dropped because "the LM needs its geometry", which stopped being true the moment a coordinate could be null. That is three consumers of one snapshot, each owing the same check, and a fourth would have owed it too. The snapshot is the place that knows, so it decides: get_node_configs now returns the placed nodes only, membership means the node can be solved with, and known_lane's filter means what it always claimed. missing_tx counts as placed, because the range circle and the bearing wedge are both about the receiver and the bistatic paths test the transmitter separately. The FOV block keeps its own check. Its configs arrive pickled through the solver queue, so it cannot see which producer built them, and a boundary that takes work from a queue is worth guarding whatever today's producers do. A config is copied per node, for the altitude resolution, so get_node_configs takes the ids a caller can actually reach: process_one_frame passes the union of its solver inputs' node ids, which is 2 to 8 against a fleet of about 58, where before it copied the fleet and configs_for_solver_input discarded most of them a line later. _solver_input_node_ids is that extraction, shared with configs_for_solver_input rather than spelled twice. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Babissimo
force-pushed
the
feat/nullable-node-coordinates
branch
from
September 9, 2026 09:18
e97a1b0 to
f0d5b10
Compare
|
Claude finished @Babissimo's task in 2m 48s —— View job Review of PR #278
· Branch: SummaryThis is a careful, well-scoped piece of work. The No new issues found in this pass. One item from the prior review remains open:
{sn.tx_lat && sn.tx_lon && ( ... )}15 lines below (line 2062), the RX→TX baseline already uses Spot checks specific to this pass
|
Babissimo
marked this pull request as ready for review
September 9, 2026 09:23
Babissimo
added a commit
that referenced
this pull request
Sep 9, 2026
main moved a long way under this branch. Resolutions: blah2_bridge.py and its tests are deleted, against main's changes to them from #278 (null altitudes) and 408e764 (atomic error counters). Only the bridge's share of that work goes; node_config.canonical_config and resolve_altitudes, which #278 added and every node path uses, stay. main relocated _pipeline_frame to node_pipeline.pipeline_frame and its docstring still described the frame shape as the one blah2_bridge puts on the queue. Taken from main and reworded in its new home, since the module it cites is gone. frame_processor keeps main's adsb_capture_ts_ms work; only the comment naming a blah2 node is reworded. solverflow's tables are main's. It converted the whole file from line numbers to symbol references, which this branch had started doing by hand for two anchors; main's version is better and wins. The bridge is then removed from it. services/node_sites.py is left as main wrote it. It reads blah2_nodes.json from the runtime overlay and skips it cleanly when absent, so it keeps working while the droplets still hold their orphaned copies and goes quiet once those are cleared. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Babissimo
added a commit
that referenced
this pull request
Sep 9, 2026
pipeline_frame moved to node_pipeline in #278 and brought its docstring with it, which still described the frame shape as the one blah2_bridge puts on the queue. The merge took main's version of the function and left the text; this finishes that. node_sites reads blah2_nodes.json and keeps doing so: while a droplet still holds a seeded copy, that file is what tells the module radar3 and radar3a share a roof, which is what keeps their published positions fuzzed as one site. Only the comments change, to stop calling it the bridge's list now that nothing writes it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Server and dashboard half of the nullable-node-coordinates work.
Ticket: https://app.clickup.com/t/86cbauhxd
Depends on: offworldlabs/retina-analytics#24 — merge that first, then this
PR's
libs/retina-analyticspin is re-set to the merged SHA.Why
A node's config required receiver and illuminator coordinates before the server
would accept it, which put the coordinates between a working receiver and a
registered one whenever an owner could not supply them at setup.
Coordinates are now nullable. A node with no position registers, streams, has
its detections counted and archived, and shows in the dashboard flagged as
missing configuration; it contributes nothing to the map, the overlap graph or
the solver.
statuskeeps meaning liveness, so such a node reads as healthyand unplaced rather than broken.
Shape
Two functions in
services.node_config, and the split between them is thedesign:
canonical_configcorrects and never invents. It folds the legacy flatlat/lonspelling, coerces each coordinate to a float or None, andcollapses the
(0, 0)sentinel. Applied wherever a config enters sharedmemory. Because it invents nothing, its output is safe to solve on, to
publish and to archive, so there is one config shape rather than a
canonical/declared pair.
resolve_altitudessupplies what the geodesy cannot do without, at thethree doors into geometry and nowhere earlier: the pipeline builder, the
solver's snapshot (
get_node_configs), andregister_node_blocking, thesingle entry to both library registries. Defaulting any earlier puts an
invented terrain figure into
/api/radar/nodesand into Parquet rows thatare not correctable once published.
It keys on
Nonerather than falsiness throughout: a receiver at 0 ft is atsea level, not unsurveyed, and the equator and prime meridian are real
coordinates. That is the bug class tracked as 86cbavanm.
Also here
0005, gradeddestructive: a downgrade cannot express a null,so a rollback with positionless rows fails loudly rather than inventing
coordinates. Existing
(0, 0)rows are left exactly as declared.NodeConfigis deliberately notpublished in the contract.
node_cfgsis an unfiltered snapshot andnothing between
submit_tracks_roundand that block checks placement, so anode re-registered without its position while its retained tracks are being
paired arrives there unplaced.
statusplus a needs-attention list.Verification
Backend suite green, analytics library 438 green, dashboard 6/6, typecheck
clean,
pre-commit5/5, contract conformance green.Two regressions are pinned by new tests because both were silent and one was
unrecoverable: a node using the legacy flat spelling archiving null geometry
despite being fully placed, and a positionless node building no solver
pipeline.
Known gap
Nothing node-side or in retina-gui can currently produce a positionless node,
so this ships complete and unexercised until 86cbbfav4 lands.
🤖 Generated with Claude Code