From fa6ffaf94e148e66bf3158167311dc1fcef7666c Mon Sep 17 00:00:00 2001 From: Babissimo Date: Mon, 7 Sep 2026 10:54:10 +0100 Subject: [PATCH 01/10] Accept a node that cannot say where it is 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 --- backend/services/node_config.py | 69 ++++++++++- backend/services/tcp_handler.py | 30 +++-- backend/tests/test_node_config_validation.py | 124 ++++++++++++++++++- backend/tests/test_tcp_validate_config.py | 32 +++++ 4 files changed, 240 insertions(+), 15 deletions(-) diff --git a/backend/services/node_config.py b/backend/services/node_config.py index 4ff73702..3c4fb7a6 100644 --- a/backend/services/node_config.py +++ b/backend/services/node_config.py @@ -12,7 +12,7 @@ """ import math -from typing import Any +from typing import Any, Literal # About 0.11 m. Below this the receiver and illuminator are the same point as far as # the solver is concerned, whatever the node believes it measured. @@ -48,6 +48,13 @@ def __init__(self, field: str, reason: str = "out of range") -> None: "doppler_tolerance_hz": (0, math.inf, False, True), } +# Nullable since 1.1.3. An owner setting a node up cannot always supply the +# geometry, and a substituted coordinate would be wrong data the server could +# not later tell apart from a survey. Latitude and longitude are a pair; +# altitude stands alone, because it is a small term that already defaults to +# zero wherever the geodesy reads it. +_NULLABLE = {"rx_lat", "rx_lon", "rx_alt_ft", "tx_lat", "tx_lon", "tx_alt_ft"} + _REQUIRED = set(_NUMERIC_BOUNDS) | {"tx_callsign", "beam_width_deg", "beam_azimuth_deg"} @@ -86,6 +93,9 @@ def validate_config(payload: dict[str, Any]) -> dict[str, Any]: out: dict[str, Any] = {} for field, (low, high, low_inclusive, high_inclusive) in _NUMERIC_BOUNDS.items(): + if payload[field] is None and field in _NULLABLE: + out[field] = None + continue value = _number(field, payload[field]) below = value < low if low_inclusive else value <= low above = value > high if high_inclusive else value >= high @@ -123,10 +133,63 @@ def validate_config(payload: dict[str, Any]) -> dict[str, Any]: raise ConfigInvalid("beam_azimuth_deg") out["beam_azimuth_deg"] = azimuth + # A latitude without its longitude places nothing, so a half-supplied side + # is a bug upstream rather than a state worth representing. + for lat_field, lon_field in (("rx_lat", "rx_lon"), ("tx_lat", "tx_lon")): + if (out[lat_field] is None) != (out[lon_field] is None): + unpaired = lat_field if out[lat_field] is None else lon_field + raise ConfigInvalid(unpaired, "latitude and longitude must be given together") + if ( - abs(out["rx_lat"] - out["tx_lat"]) < _MIN_BASELINE_DEG - and abs(out["rx_lon"] - out["tx_lon"]) < _MIN_BASELINE_DEG + out["rx_lat"] is not None + and out["tx_lat"] is not None + and ( + abs(out["rx_lat"] - out["tx_lat"]) < _MIN_BASELINE_DEG + and abs(out["rx_lon"] - out["tx_lon"]) < _MIN_BASELINE_DEG + ) ): raise ConfigInvalid("tx_lat", "receiver and illuminator are at the same point") return out + + +PositionStatus = Literal["positioned", "missing_rx", "missing_tx", "missing_both"] + + +def _is_num(v: Any) -> bool: + return isinstance(v, (int, float)) and not isinstance(v, bool) and math.isfinite(v) + + +def _is_placed(lat: Any, lon: Any) -> bool: + """A pair given as a real position: a usable number in both slots, and + not the (0, 0) sentinel. + + The same rule lives in services.geo.valid_latlon and in + retina_analytics.constants.has_full_geometry. Not shared from either: this + module is a leaf that takes a dict and returns a dict, and must stay + importable with neither retina_analytics nor a database on the path. + valid_latlon pulls in retina_analytics.constants, which would break that. + + A config straight out of connected_nodes is unvalidated JSON rather than + validate_config's output, so lat/lon may be any type; _is_num keeps a + non-numeric value reading as not placed rather than raising. + """ + return _is_num(lat) and _is_num(lon) and not (lat == 0.0 and lon == 0.0) + + +def position_status(config: dict[str, Any]) -> PositionStatus: + """Which ends of the bistatic pair this config places. + + One value for consumers to branch on, rather than four fields each of them + has to recombine. Keyed on latitude and longitude alone: a node with a + position and no altitude is positioned. + """ + has_rx = _is_placed(config.get("rx_lat"), config.get("rx_lon")) + has_tx = _is_placed(config.get("tx_lat"), config.get("tx_lon")) + if has_rx and has_tx: + return "positioned" + if has_rx: + return "missing_tx" + if has_tx: + return "missing_rx" + return "missing_both" diff --git a/backend/services/tcp_handler.py b/backend/services/tcp_handler.py index 6b21be77..a2375364 100644 --- a/backend/services/tcp_handler.py +++ b/backend/services/tcp_handler.py @@ -59,17 +59,25 @@ def _log_event(category: str, message: str, severity: str = "info", meta: dict | def _validate_node_config(config: dict) -> str | None: """Return an error message if the node config is invalid, else None.""" - # Accept both flat lat/lon and rx_lat/rx_lon forms - lat = config.get("rx_lat", config.get("lat")) - lon = config.get("rx_lon", config.get("lon")) - if lat is None or lon is None: - return "missing lat/lon (expected rx_lat/rx_lon or lat/lon)" - try: - lat, lon = float(lat), float(lon) - except (TypeError, ValueError): - return f"non-numeric lat/lon: {lat!r}, {lon!r}" - if not (-90 <= lat <= 90) or not (-180 <= lon <= 180): - return f"lat/lon out of range: {lat}, {lon}" + # rx_lat/rx_lon present and explicitly null is a positionless + # registration, not a missing one: dict.get's single-default form cannot + # tell that apart from the keys being absent, which still falls back to + # the legacy flat lat/lon form. + _explicit_positionless = ( + "rx_lat" in config and "rx_lon" in config and config["rx_lat"] is None and config["rx_lon"] is None + ) + if not _explicit_positionless: + # Accept both flat lat/lon and rx_lat/rx_lon forms + lat = config.get("rx_lat", config.get("lat")) + lon = config.get("rx_lon", config.get("lon")) + if lat is None or lon is None: + return "missing lat/lon (expected rx_lat/rx_lon or lat/lon)" + try: + lat, lon = float(lat), float(lon) + except (TypeError, ValueError): + return f"non-numeric lat/lon: {lat!r}, {lon!r}" + if not (-90 <= lat <= 90) or not (-180 <= lon <= 180): + return f"lat/lon out of range: {lat}, {lon}" bw = config.get("beam_width_deg") if bw is not None: try: diff --git a/backend/tests/test_node_config_validation.py b/backend/tests/test_node_config_validation.py index 19d890e4..91ebc208 100644 --- a/backend/tests/test_node_config_validation.py +++ b/backend/tests/test_node_config_validation.py @@ -2,7 +2,7 @@ import pytest -from services.node_config import ConfigInvalid, validate_config +from services.node_config import ConfigInvalid, position_status, validate_config VALID = { "rx_lat": 51.42, @@ -356,3 +356,125 @@ def test_the_field_named_is_always_a_string(): with pytest.raises(ConfigInvalid) as excinfo: validate_config([1, 2]) assert isinstance(excinfo.value.field, str) + + +# --- Nullable coordinates ------------------------------------------------------------ + + +@pytest.mark.parametrize( + "overrides,expected", + [ + ({}, "positioned"), + ({"rx_lat": None, "rx_lon": None}, "missing_rx"), + ({"tx_lat": None, "tx_lon": None}, "missing_tx"), + ({"rx_lat": None, "rx_lon": None, "tx_lat": None, "tx_lon": None}, "missing_both"), + ({"rx_alt_ft": None, "tx_alt_ft": None}, "positioned"), + ({"rx_alt_ft": None}, "positioned"), + ], + ids=["full", "no-rx", "no-tx", "neither", "no-altitude", "one-altitude"], +) +def test_null_coordinates_are_accepted(overrides, expected): + out = validate_config(dict(VALID, **overrides)) + for key, value in overrides.items(): + assert out[key] is value + assert position_status(out) == expected + + +@pytest.mark.parametrize( + "overrides,expected", + [ + ({"rx_lat": 0.0, "rx_lon": 0.0}, "missing_rx"), + ({"tx_lat": 0.0, "tx_lon": 0.0}, "missing_tx"), + ({"rx_lat": 0.0}, "positioned"), + ({"tx_lon": 0.0}, "positioned"), + ], + ids=["rx-null-island", "tx-null-island", "rx-on-the-equator", "tx-on-the-prime-meridian"], +) +def test_position_status_treats_the_zero_pair_as_absent(overrides, expected): + """(0, 0) is the legacy broken-config sentinel, not a real position in the + Gulf of Guinea, matching has_full_geometry in retina-analytics. A single + zero axis is still a real coordinate, so it must not read as absent.""" + out = validate_config(dict(VALID, **overrides)) + assert position_status(out) == expected + + +@pytest.mark.parametrize( + "overrides,field", + [ + ({"rx_lat": None}, "rx_lat"), + ({"rx_lon": None}, "rx_lon"), + ({"tx_lat": None}, "tx_lat"), + ({"tx_lon": None}, "tx_lon"), + ], +) +def test_half_a_position_is_rejected(overrides, field): + with pytest.raises(ConfigInvalid) as excinfo: + validate_config(dict(VALID, **overrides)) + assert excinfo.value.field == field + assert excinfo.value.reason == "latitude and longitude must be given together" + + +def test_a_missing_key_is_still_an_error(): + payload = dict(VALID) + del payload["rx_lat"] + with pytest.raises(ConfigInvalid) as excinfo: + validate_config(payload) + assert excinfo.value.reason == "missing" + + +def test_baseline_check_is_skipped_when_a_side_is_null(): + # Identical rx and tx would be a degenerate baseline, but with no tx there + # is no baseline to be degenerate. + out = validate_config(dict(VALID, tx_lat=None, tx_lon=None)) + assert out["tx_lat"] is None + + +def test_an_out_of_range_coordinate_is_reported_before_a_missing_pair(): + """The bounds loop runs before the pair rule, so an out-of-range rx_lat is + reported as out-of-range, not as an unpaired coordinate, even though its + own pair (rx_lon) is null in the same payload.""" + with pytest.raises(ConfigInvalid) as excinfo: + validate_config(dict(VALID, rx_lat=91.0, rx_lon=None)) + assert excinfo.value.field == "rx_lat" + assert excinfo.value.reason == "out of range" + + +@pytest.mark.parametrize( + "config", + [ + {}, + {"node_id": "x"}, + {"rx_lat": 1.0}, + ], + ids=["empty", "unrelated-keys-only", "latitude-without-longitude"], +) +def test_position_status_on_a_config_that_never_saw_validate_config(config): + """_refresh_analytics_and_nodes calls position_status on connected_nodes + configs directly, which never necessarily passed through validate_config: + a legacy node's config can carry no geometry keys at all, and a + bulk-ingested one can carry a lone coordinate. A side with only one of its + two coordinates places nothing, so all three of these read as + missing_both.""" + assert position_status(config) == "missing_both" + + +@pytest.mark.parametrize("field", ["rx_lat", "rx_lon", "tx_lat", "tx_lon"]) +@pytest.mark.parametrize( + "value", + [ + pytest.param("abc", id="string"), + pytest.param("", id="empty-string"), + pytest.param([], id="list"), + pytest.param(True, id="bool-true"), + pytest.param(False, id="bool-false"), + ], +) +def test_a_non_numeric_coordinate_reads_as_not_placed_rather_than_raising(field, value): + """A connected_nodes config is unvalidated JSON, so a garbage value can sit + in any coordinate slot: float("") and float([]) both raise, and bool is a + subclass of int, so a naive isinstance(x, (int, float)) check would accept + True as a latitude. position_status must read past all of that as merely + unplaced, not raise.""" + config = {"rx_lat": 51.42, "rx_lon": -0.91, "tx_lat": 51.37, "tx_lon": -0.88, field: value} + side = "rx" if field.startswith("rx") else "tx" + assert position_status(config) == f"missing_{side}" diff --git a/backend/tests/test_tcp_validate_config.py b/backend/tests/test_tcp_validate_config.py index 71893f00..72c366fb 100644 --- a/backend/tests/test_tcp_validate_config.py +++ b/backend/tests/test_tcp_validate_config.py @@ -323,3 +323,35 @@ def test_scientific_notation_lat_lon(self): config = {"lat": 4.0e1, "lon": -7.4e1} result = _validate_node_config(config) assert result is None + + +class TestExplicitNullIsPositionless: + """rx_lat/rx_lon present with an explicit null is a positionless + registration, distinct from the keys being absent.""" + + def test_explicit_null_rx_lat_rx_lon_is_accepted(self): + config = {"rx_lat": None, "rx_lon": None} + assert _validate_node_config(config) is None + + def test_other_fields_are_still_validated(self): + config = {"rx_lat": None, "rx_lon": None, "beam_width_deg": "invalid"} + result = _validate_node_config(config) + assert result is not None + assert "non-numeric beam_width_deg" in result + + def test_rx_lat_null_alone_is_still_missing(self): + """Only rx_lon absent, not null: a genuinely absent key is still + rejected, matching test_only_rx_lat_present.""" + config = {"rx_lat": None} + result = _validate_node_config(config) + assert result is not None + assert "missing lat/lon" in result + + def test_flat_lat_lon_null_is_not_treated_as_positionless(self): + """The positionless carve-out is for rx_lat/rx_lon specifically: the + legacy lat/lon flat form predates this feature and still means + missing when null.""" + config = {"lat": None, "lon": None} + result = _validate_node_config(config) + assert result is not None + assert "missing lat/lon" in result From 4cf2d3e6e6f0bb93eea6f711cabebb386e885f3b Mon Sep 17 00:00:00 2001 From: Babissimo Date: Mon, 7 Sep 2026 10:54:10 +0100 Subject: [PATCH 02/10] Store a position we do not have as null, not as (0, 0) 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 --- backend/core/nodes.py | 15 +++++--- .../0005_nullable_node_coordinates.py | 37 +++++++++++++++++++ backend/routes/nodes.py | 7 +++- backend/tests/test_node_config_store.py | 36 ++++++++++++++++++ contracts/nodes-v1.openapi.yaml | 2 +- 5 files changed, 89 insertions(+), 8 deletions(-) create mode 100644 backend/migrations/versions/0005_nullable_node_coordinates.py diff --git a/backend/core/nodes.py b/backend/core/nodes.py index 5fb37391..6f435e87 100644 --- a/backend/core/nodes.py +++ b/backend/core/nodes.py @@ -53,12 +53,15 @@ class NodeConfig(Base): id: Mapped[int] = mapped_column(Integer, primary_key=True, autoincrement=True) node_id: Mapped[str] = mapped_column(String(32), ForeignKey("nodes.node_id"), index=True) version: Mapped[int] = mapped_column(Integer) - rx_lat: Mapped[float] = mapped_column(Float) - rx_lon: Mapped[float] = mapped_column(Float) - rx_alt_ft: Mapped[float] = mapped_column(Float) - tx_lat: Mapped[float] = mapped_column(Float) - tx_lon: Mapped[float] = mapped_column(Float) - tx_alt_ft: Mapped[float] = mapped_column(Float) + # Nullable since contract 1.1.3: an owner cannot always supply the geometry + # at setup, and such a node is carried without being placed. Latitude and + # longitude are validated as a pair; altitude stands alone. + rx_lat: Mapped[float | None] = mapped_column(Float, nullable=True) + rx_lon: Mapped[float | None] = mapped_column(Float, nullable=True) + rx_alt_ft: Mapped[float | None] = mapped_column(Float, nullable=True) + tx_lat: Mapped[float | None] = mapped_column(Float, nullable=True) + tx_lon: Mapped[float | None] = mapped_column(Float, nullable=True) + tx_alt_ft: Mapped[float | None] = mapped_column(Float, nullable=True) tx_callsign: Mapped[str] = mapped_column(String(32)) fc_hz: Mapped[float] = mapped_column(Float) fs_hz: Mapped[float] = mapped_column(Float) diff --git a/backend/migrations/versions/0005_nullable_node_coordinates.py b/backend/migrations/versions/0005_nullable_node_coordinates.py new file mode 100644 index 00000000..9e629bf9 --- /dev/null +++ b/backend/migrations/versions/0005_nullable_node_coordinates.py @@ -0,0 +1,37 @@ +"""The six coordinate columns become nullable on node_configs. + +Revision ID: 0005 +Revises: 0004 +""" + +import sqlalchemy as sa +from alembic import op + +revision = "0005" +down_revision = "0004" +branch_labels = None +depends_on = None + +# A downgrade cannot express a null, and code predating 1.1.3 has no +# null-handling for these six columns, so a rollback across this revision must +# be surfaced to a human rather than served as safe. +rollback_safety = "destructive" + +_COLUMNS = ("rx_lat", "rx_lon", "rx_alt_ft", "tx_lat", "tx_lon", "tx_alt_ft") + + +def upgrade() -> None: + # Existing rows are left exactly as they are. A row that declared (0, 0) + # stays as declared: the table is append-only, so this governs new rows + # only, and rewriting history would be guessing at what a node meant. + with op.batch_alter_table("node_configs") as batch: + for column in _COLUMNS: + batch.alter_column(column, existing_type=sa.Float(), nullable=True) + + +def downgrade() -> None: + # A null cannot be expressed under the old constraint, so a downgrade with + # positionless rows present will fail loudly rather than invent coordinates. + with op.batch_alter_table("node_configs") as batch: + for column in _COLUMNS: + batch.alter_column(column, existing_type=sa.Float(), nullable=False) diff --git a/backend/routes/nodes.py b/backend/routes/nodes.py index 9514966a..a2deaf6a 100644 --- a/backend/routes/nodes.py +++ b/backend/routes/nodes.py @@ -44,7 +44,12 @@ # # Publishing NodeConfig would be the minor bump, since that is the one thing # here a client cannot already do (86cb6d7he). -NODE_API_VERSION = "1.1.2" +# +# 1.1.3 makes the six coordinate fields of NodeConfig nullable, so a node whose +# owner cannot supply the geometry can still register. A patch rather than a +# minor bump for the same reason as above: NodeConfig is not published, so the +# document gains no field and no capability a client can read (86cb6d7he). +NODE_API_VERSION = "1.1.3" # No tag here: each sub-router carries the contract's own grouping, since those # are what a generated client is built around. diff --git a/backend/tests/test_node_config_store.py b/backend/tests/test_node_config_store.py index 6124d68d..b29f2479 100644 --- a/backend/tests/test_node_config_store.py +++ b/backend/tests/test_node_config_store.py @@ -233,3 +233,39 @@ def test_the_compared_fields_are_every_geometry_column(): assert set(_CONFIG_FIELDS) == set(validate_config(dict(CONFIG))) assert len(_CONFIG_FIELDS) == 15 assert set(CHANGES) == set(_CONFIG_FIELDS) + + +async def test_a_null_position_round_trips(node_session): + """node_configs.node_id is a foreign key, so the row it hangs off has to + exist first, the same as the `node` fixture gives every other test here.""" + node_session.add(Node(node_id="test-null-pos", node_ref=mint_node_ref(), board_model="raspberrypi5-4gb")) + await node_session.flush() + + row = NodeConfig( + node_id="test-null-pos", + version=1, + rx_lat=None, + rx_lon=None, + rx_alt_ft=None, + tx_lat=34.90, + tx_lon=-82.45, + tx_alt_ft=1200.0, + tx_callsign="WSPA", + fc_hz=195e6, + fs_hz=2.4e6, + beam_width_deg=None, + beam_azimuth_deg=None, + max_range_km=150.0, + cpi_s=0.5, + delay_tolerance_us=10.0, + doppler_tolerance_hz=5.0, + ) + node_session.add(row) + await node_session.commit() + + # expire_on_commit=False leaves row fully populated from what was just + # constructed, so get()/select() hand it back unread; refresh() is what + # actually re-queries the columns. + await node_session.refresh(row) + assert row.rx_lat is None + assert row.tx_lat == 34.90 diff --git a/contracts/nodes-v1.openapi.yaml b/contracts/nodes-v1.openapi.yaml index cc984b4c..d83fc7c3 100644 --- a/contracts/nodes-v1.openapi.yaml +++ b/contracts/nodes-v1.openapi.yaml @@ -1,7 +1,7 @@ openapi: 3.1.0 info: title: RETINA node ingest - version: 1.1.2 + version: 1.1.3 description: | The RETINA server's HTTP API. The paths under `/v1/nodes` are the RETINA node ingest contract, generated from the server and versioned as a unit; everything From ad5621173ae61cf7d31081b50776a18c8ba26142 Mon Sep 17 00:00:00 2001 From: Babissimo Date: Mon, 7 Sep 2026 10:54:11 +0100 Subject: [PATCH 03/10] Carry position_status to the dashboard, and prove a null node stays off 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 --- backend/routes/auth.py | 2 + backend/services/adsb_regions.py | 3 +- backend/services/tasks/analytics_refresh.py | 2 + backend/tests/test_auth_routes.py | 25 +++- backend/tests/test_positionless_node.py | 132 ++++++++++++++++++ .../tests/test_unpositioned_registration.py | 2 +- frontend/src/components/map/hooks.ts | 16 ++- libs/retina-analytics | 2 +- 8 files changed, 173 insertions(+), 11 deletions(-) create mode 100644 backend/tests/test_positionless_node.py diff --git a/backend/routes/auth.py b/backend/routes/auth.py index 124b72c9..817b4465 100644 --- a/backend/routes/auth.py +++ b/backend/routes/auth.py @@ -33,6 +33,7 @@ get_jwt_strategy, get_or_create_oauth_user, ) +from services.node_config import position_status logger = logging.getLogger(__name__) router = APIRouter(prefix="/api/auth", tags=["auth"]) @@ -262,6 +263,7 @@ async def my_nodes(request: Request): "is_synthetic": info.get("is_synthetic", False), "rx_lat": cfg.get("rx_lat"), "rx_lon": cfg.get("rx_lon"), + "position_status": position_status(cfg), "frequency": cfg.get("FC", cfg.get("frequency")), } ) diff --git a/backend/services/adsb_regions.py b/backend/services/adsb_regions.py index 0e12c61d..7dcde857 100644 --- a/backend/services/adsb_regions.py +++ b/backend/services/adsb_regions.py @@ -249,7 +249,8 @@ def is_position_absent(lat, lon) -> bool: which no node and no aircraft occupies. Only the exact pair reads as absence: the equator and the prime meridian are each perfectly good coordinates on their own. This is the convention retina_analytics applies - in _has_receiver_position, and every backend site must agree with it. + in has_full_geometry, which holds it for both ends of the bistatic pair, + and every backend site must agree with it. A bool is never the sentinel even though `bool` is an `int` subclass and `False == 0.0`: a node reporting a boolean is sending malformed config, diff --git a/backend/services/tasks/analytics_refresh.py b/backend/services/tasks/analytics_refresh.py index ff3b525d..9aa1c8ff 100644 --- a/backend/services/tasks/analytics_refresh.py +++ b/backend/services/tasks/analytics_refresh.py @@ -24,6 +24,7 @@ from services.geo import bearing_deg, bistatic_delay_us, haversine_km, node_beam_params, point_in_beam from services.geo import valid_latlon as _valid_latlon from services.id_utils import multinode_hex_from_key +from services.node_config import position_status from services.node_sites import log_colocation_audit from services.public_location import ( fuzz_enabled, @@ -382,6 +383,7 @@ def _refresh_analytics_and_nodes(): ), "sample_rate": (info.get("config", {}).get("Fs") or info.get("config", {}).get("fs_hz")), "location": _public_location_block(nid, info.get("config", {})), + "position_status": position_status(info.get("config", {})), } for nid, info in _published_nodes }, diff --git a/backend/tests/test_auth_routes.py b/backend/tests/test_auth_routes.py index f130e887..47669f75 100644 --- a/backend/tests/test_auth_routes.py +++ b/backend/tests/test_auth_routes.py @@ -138,11 +138,34 @@ def test_my_nodes_entry_has_expected_fields(self, client): try: nodes = client.get("/api/auth/me/nodes").json() node = next(n for n in nodes if n["node_id"] == "field-check-node") - for field in ("node_id", "name", "status", "is_synthetic"): + for field in ("node_id", "name", "status", "is_synthetic", "position_status"): assert field in node, f"Missing field: {field}" finally: asyncio.run(set_node_owner("field-check-node", None)) + def test_my_nodes_entry_carries_position_status_for_a_private_node(self, client): + """A private node is filtered out of /api/radar/nodes entirely, so + its owner has nowhere else to learn it needs a position.""" + from core import state + from core.auth import set_node_owner + from core.users import ANONYMOUS_USER + + node_id = "position-status-node" + asyncio.run(set_node_owner(node_id, ANONYMOUS_USER["id"])) + with state.connected_nodes_lock: + state.connected_nodes[node_id] = { + "status": "active", + "config": {"rx_lat": None, "rx_lon": None, "tx_lat": None, "tx_lon": None}, + } + try: + nodes = client.get("/api/auth/me/nodes").json() + node = next(n for n in nodes if n["node_id"] == node_id) + assert node["position_status"] == "missing_both" + finally: + asyncio.run(set_node_owner(node_id, None)) + with state.connected_nodes_lock: + state.connected_nodes.pop(node_id, None) + # ── OAuth state token (CSRF + open-redirect) ────────────────────────────────── diff --git a/backend/tests/test_positionless_node.py b/backend/tests/test_positionless_node.py new file mode 100644 index 00000000..f1df8ecb --- /dev/null +++ b/backend/tests/test_positionless_node.py @@ -0,0 +1,132 @@ +"""A node that registers with null coordinates is carried, not placed. + +The sibling of test_unpositioned_registration, which covers coordinates that +are absent. Here they are explicitly null, which is what contract 1.1.3 added: +the node is counted and visible in the dashboard, and takes no part in the map +or the solver. +""" + +import pytest + +from core import state +from services.node_config import position_status + +_CONFIG = { + "rx_lat": None, + "rx_lon": None, + "rx_alt_ft": None, + "tx_lat": None, + "tx_lon": None, + "tx_alt_ft": None, + "tx_callsign": "WSPA", + "fc_hz": 195e6, + "fs_hz": 2.4e6, + "beam_width_deg": None, + "beam_azimuth_deg": None, + "max_range_km": 150.0, + "cpi_s": 0.5, + "delay_tolerance_us": 10.0, + "doppler_tolerance_hz": 5.0, +} + + +# Enumerated explicitly rather than scanned from connected_nodes: not every +# test below registers through it, so cleanup can't be derived from it. +_NODE_IDS = ("test-null-1", "test-null-2", "test-null-3") + +# A single node can't discriminate the overlap guard: with nobody to pair +# against, no zone forms whether or not the guard excludes it. Ten can: were +# has_full_geometry not excluding them, all ten would collapse onto the same +# undefined geometry and pair into every one of the 45 possible zones. +_OVERLAP_IDS = [f"test-null-overlap-{i}" for i in range(10)] + + +@pytest.fixture(autouse=True) +def _clean(): + yield + for node_id in (*_NODE_IDS, *_OVERLAP_IDS): + state.connected_nodes.pop(node_id, None) + state.node_pipelines.pop(node_id, None) + state.node_associator.unregister_node(node_id) + state.node_analytics.retire_node(node_id) + + +def test_positionless_node_is_counted_but_not_placed(): + node_id = "test-null-1" + state.node_analytics.register_node(node_id, dict(_CONFIG)) + state.node_associator.register_node(node_id, dict(_CONFIG)) + + assert node_id in state.node_analytics.metrics + assert "detection_area" not in state.node_analytics.get_node_summary(node_id) + + # A metrics entry alone doesn't show frames are counted, which is the + # promise this feature makes to the owner of a node we cannot place. + assert state.node_analytics.record_detection_frame(node_id, {"timestamp": 1.0, "detections": []}) is True + assert state.node_analytics.metrics[node_id].total_frames == 1 + + +def test_ten_positionless_nodes_form_no_overlap_zones(): + for node_id in _OVERLAP_IDS: + state.node_analytics.register_node(node_id, dict(_CONFIG)) + state.node_associator.register_node(node_id, dict(_CONFIG)) + + wanted = set(_OVERLAP_IDS) + assert sum(1 for pair in state.node_associator.overlap_zones if wanted & set(pair)) == 0 + + +def test_positionless_node_builds_no_solver_pipeline(): + """The never-solve half: a positionless node builds no solver pipeline. + + Holds today because get_or_create_node_pipeline's guard at + frame_processor.py:171, `if cfg.get("rx_lat") and cfg.get("tx_lat")`, + falls through to the shared default pipeline only because None is falsy: + a known truthiness bug, 86cbavanm. + + Worth pinning because a correct repair to `is not None` preserves this + fall-through, but a repair that tests key presence (`"rx_lat" in cfg`) + instead of value nullity does not: the key is present, carrying None, so + pipeline construction proceeds and raises a TypeError. + """ + from services.frame_processor import get_or_create_node_pipeline + + node_id = "test-null-3" + with state.connected_nodes_lock: + state.connected_nodes[node_id] = { + "config": dict(_CONFIG), + "config_hash": "", + "status": "active", + "last_heartbeat": None, + "peer": "test", + "is_synthetic": False, + "capabilities": {}, + } + + # It falls back to the shared default rather than returning None, so assert + # identity against a sentinel: a node-specific pipeline would be a new + # object, and would also register itself in state.node_pipelines. + sentinel = object() + assert get_or_create_node_pipeline(node_id, sentinel) is sentinel + assert node_id not in state.node_pipelines + + +def test_position_status_reaches_the_nodes_payload(): + import orjson + + from services.tasks.analytics_refresh import _refresh_analytics_and_nodes + + node_id = "test-null-2" + with state.connected_nodes_lock: + state.connected_nodes[node_id] = { + "config": dict(_CONFIG), + "config_hash": "", + "status": "active", + "last_heartbeat": None, + "peer": "test", + "is_synthetic": False, + "capabilities": {}, + } + _refresh_analytics_and_nodes() + payload = orjson.loads(state.latest_nodes_bytes) + assert payload["nodes"][node_id]["position_status"] == "missing_both" + assert payload["nodes"][node_id]["location"]["rx_lat"] is None + assert position_status(_CONFIG) == "missing_both" diff --git a/backend/tests/test_unpositioned_registration.py b/backend/tests/test_unpositioned_registration.py index 00a21829..98097dad 100644 --- a/backend/tests/test_unpositioned_registration.py +++ b/backend/tests/test_unpositioned_registration.py @@ -8,7 +8,7 @@ and the solver queue backs up behind candidates that are pure artefact (86cb5hef4). -The guard itself lives in retina-analytics (`_has_receiver_position`), so what +The guard itself lives in retina-analytics (`has_full_geometry`), so what is pinned here is the behaviour this repo depends on rather than its implementation: nothing but the submodule revision stands between main and a repeat, and the failure mode is a saturated solver rather than an exception. diff --git a/frontend/src/components/map/hooks.ts b/frontend/src/components/map/hooks.ts index 36e5a0dd..ff3977d0 100644 --- a/frontend/src/components/map/hooks.ts +++ b/frontend/src/components/map/hooks.ts @@ -331,13 +331,15 @@ export function useNodes() { const da = (info as any).detection_area; const ec = (info as any).empirical_coverage; if (da) { - // Skip null-island nodes (rx=(0,0)) that result from backend - // register_node() defaulting missing rx/tx coords to 0. These - // show up after HTTP-registration without a config block - // (notably e2e bulk tests) and render as a stray marker in the - // Atlantic Ocean. Use a small epsilon so we still allow a real - // node legitimately near the equator/prime-meridian, but - // dismiss the exact-zero default sentinel. + // Defence in depth, not the primary guard: the analytics library + // only builds a detection_area when has_full_geometry(config) is + // true, and that already rejects the exact rx=(0,0) sentinel, so + // a null-island node should never reach here with one. Kept in + // case some other path ever hands us a detection_area without + // going through that check. Use a small epsilon so we still allow + // a real node legitimately near the equator/prime meridian, but + // dismiss the exact-zero sentinel that would otherwise render as + // a stray marker in the Gulf of Guinea. const rxLat = da.rx.lat; const rxLon = da.rx.lon; if (Math.abs(rxLat) < 1e-6 && Math.abs(rxLon) < 1e-6) continue; diff --git a/libs/retina-analytics b/libs/retina-analytics index 76dfb450..13bad15d 160000 --- a/libs/retina-analytics +++ b/libs/retina-analytics @@ -1 +1 @@ -Subproject commit 76dfb4503ad68f39f88ad7d90225dc0ccce4b899 +Subproject commit 13bad15dfc07bad78146a2a0c2c50d889053b229 From 2b1153713d7709b39e4441e85846722b94b5243f Mon Sep 17 00:00:00 2001 From: Babissimo Date: Mon, 7 Sep 2026 10:54:11 +0100 Subject: [PATCH 04/10] Flag a node we cannot place, without calling it broken 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 --- .../src/components/PositionStatusBadge.tsx | 28 +++++++++++ .../src/pages/admin/NetworkHealthPage.tsx | 4 +- .../src/pages/admin/NodeManagementPage.tsx | 2 + dashboard/src/pages/user/NodeDetailPage.tsx | 27 +++++++++++ dashboard/src/pages/user/OverviewPage.tsx | 47 ++++++++++++++++++- .../src/test/PositionStatusBadge.test.tsx | 20 ++++++++ dashboard/src/types.ts | 5 ++ 7 files changed, 130 insertions(+), 3 deletions(-) create mode 100644 dashboard/src/components/PositionStatusBadge.tsx create mode 100644 dashboard/src/test/PositionStatusBadge.test.tsx diff --git a/dashboard/src/components/PositionStatusBadge.tsx b/dashboard/src/components/PositionStatusBadge.tsx new file mode 100644 index 00000000..d7a1be19 --- /dev/null +++ b/dashboard/src/components/PositionStatusBadge.tsx @@ -0,0 +1,28 @@ +import type { PositionStatus } from "../types"; + +const LABELS: Record, string> = { + missing_both: "No position", + missing_rx: "No receiver position", + missing_tx: "No illuminator position", +}; + +// Shared with NodeDetailPage's banner (verbatim) and OverviewPage's section +// header (paraphrased there until it started drifting from this wording). +export const POSITION_STATUS_EXPLANATION = + "Detections from this node are counted and archived. It needs a " + + "position before they can be placed on the map or contribute to solves."; + +/** Sits beside `status`, never inside it: liveness and position completeness + * are separate questions, and a node detecting without a position is healthy. */ +export function PositionStatusBadge({ status }: { status: PositionStatus }) { + const label = LABELS[status as keyof typeof LABELS]; + // Renders for a status in the label map (i.e. not "positioned"); anything + // else, including an unexpected or absent value, renders nothing rather + // than an empty chip. + if (!label) return null; + return ( + + {label} + + ); +} diff --git a/dashboard/src/pages/admin/NetworkHealthPage.tsx b/dashboard/src/pages/admin/NetworkHealthPage.tsx index 159ef604..09ca0edf 100644 --- a/dashboard/src/pages/admin/NetworkHealthPage.tsx +++ b/dashboard/src/pages/admin/NetworkHealthPage.tsx @@ -134,7 +134,9 @@ export default function NetworkHealthPage() { {/* Node location map */} {(() => { - const geoNodes = nodes.filter((n) => n.location?.rx_lat && n.location?.rx_lon); + const geoNodes = nodes.filter( + (n) => n.location?.rx_lat != null && n.location?.rx_lon != null, + ); if (geoNodes.length === 0) return null; const avgLat = geoNodes.reduce((s, n) => s + n.location.rx_lat, 0) / geoNodes.length; const avgLon = geoNodes.reduce((s, n) => s + n.location.rx_lon, 0) / geoNodes.length; diff --git a/dashboard/src/pages/admin/NodeManagementPage.tsx b/dashboard/src/pages/admin/NodeManagementPage.tsx index a70b1aac..7c72f899 100644 --- a/dashboard/src/pages/admin/NodeManagementPage.tsx +++ b/dashboard/src/pages/admin/NodeManagementPage.tsx @@ -1,6 +1,7 @@ import { useState, useEffect } from "react"; import { useNavigate } from "react-router-dom"; import { api } from "../../api/client"; +import { PositionStatusBadge } from "../../components/PositionStatusBadge"; const PAGE_SIZE = 25; @@ -87,6 +88,7 @@ export default function NodeManagementPage() { {online ? "Online" : "Offline"} + {node.name || id}
diff --git a/dashboard/src/pages/user/NodeDetailPage.tsx b/dashboard/src/pages/user/NodeDetailPage.tsx index 08164ffd..ee29a928 100644 --- a/dashboard/src/pages/user/NodeDetailPage.tsx +++ b/dashboard/src/pages/user/NodeDetailPage.tsx @@ -4,6 +4,14 @@ import { BarChart, Bar, XAxis, YAxis, CartesianGrid, Tooltip, ResponsiveContainer, } from "recharts"; import { api } from "../../api/client"; +import { POSITION_STATUS_EXPLANATION } from "../../components/PositionStatusBadge"; +import type { PositionStatus } from "../../types"; + +const POSITION_FIX_HINT: Record, string> = { + missing_rx: "Add its receiver position in the node configuration.", + missing_tx: "Add its illuminator position in the node configuration.", + missing_both: "Add its receiver and illuminator positions in the node configuration.", +}; export default function NodeDetailPage() { const { nodeId } = useParams(); @@ -136,6 +144,25 @@ export default function NodeDetailPage() {
)} + {/* Position completeness is orthogonal to the node's liveness (`status`): + a positionless node can be actively detecting and perfectly healthy. */} + {nodeInfo?.position_status && nodeInfo.position_status !== "positioned" && ( +
+ Position not configured.{" "} + {POSITION_STATUS_EXPLANATION}{" "} + {POSITION_FIX_HINT[nodeInfo.position_status as Exclude]} +
+ )} + {/* RF Configuration */} {nodeInfo && (
diff --git a/dashboard/src/pages/user/OverviewPage.tsx b/dashboard/src/pages/user/OverviewPage.tsx index bb78d25d..6fdea0e6 100644 --- a/dashboard/src/pages/user/OverviewPage.tsx +++ b/dashboard/src/pages/user/OverviewPage.tsx @@ -4,9 +4,11 @@ import { AreaChart, Area, XAxis, YAxis, CartesianGrid, Tooltip, ResponsiveContainer, } from "recharts"; import { api } from "../../api/client"; +import { PositionStatusBadge, POSITION_STATUS_EXPLANATION } from "../../components/PositionStatusBadge"; export default function OverviewPage() { const [nodes, setNodes] = useState([]); + const [myNodes, setMyNodes] = useState([]); const [analytics, setAnalytics] = useState(null); const [aircraftCount, setAircraftCount] = useState(0); const [loading, setLoading] = useState(true); @@ -14,8 +16,10 @@ export default function OverviewPage() { const timerRef = useRef>(undefined); const fetchData = () => { - Promise.all([api.nodes(), api.analytics(), api.aircraft()]) - .then(([n, a, ac]) => { + // myNodes fails soft: it is only needed for the needs-attention list, and + // an unauthenticated view of this page must still render the rest. + Promise.all([api.nodes(), api.analytics(), api.aircraft(), api.myNodes().catch(() => [])]) + .then(([n, a, ac, mine]) => { // n.nodes is a dict {node_id: {status, ...}} const nodeMap = n.nodes || {}; // a.nodes is a dict {node_id: {trust, metrics, detection_area, reputation}} @@ -26,6 +30,7 @@ export default function OverviewPage() { _analytics: analyticsMap[id] || {}, })); setNodes(nodeList); + setMyNodes(Array.isArray(mine) ? mine : []); setAnalytics(a); setAircraftCount((ac.aircraft || []).length); }) @@ -43,6 +48,12 @@ export default function OverviewPage() { const nodeList = Array.isArray(nodes) ? nodes : []; const onlineCount = nodeList.filter((n) => n.status !== "disconnected" && n.status != null).length; + // Merged with the owner's own nodes, because /api/radar/nodes drops private + // ones: a private node with no position would otherwise appear nowhere its + // owner looks, and this list is the only place they are told. + const byId = new Map(nodeList.map((n) => [n.node_id, n])); + for (const n of myNodes) if (!byId.has(n.node_id)) byId.set(n.node_id, n); + const needsAttention = [...byId.values()].filter((n) => n.position_status && n.position_status !== "positioned"); // detection_area.n_detections is the most reliably populated counter const totalFrameDetections = nodeList.reduce( (s, n) => s + (n._analytics?.metrics?.total_detections || n._analytics?.detection_area?.n_detections || 0), @@ -82,6 +93,38 @@ export default function OverviewPage() {
+ {needsAttention.length > 0 && ( +
+
+

Needs Attention

+ + {POSITION_STATUS_EXPLANATION} + +
+
+ + + + + + + + + {needsAttention.map((node) => { + const id = node.node_id || node.id; + return ( + navigate(`/nodes/${id}`)}> + + + + ); + })} + +
NodePosition
{node.name || id}
+
+
+ )} + {chartData.length > 0 && (
diff --git a/dashboard/src/test/PositionStatusBadge.test.tsx b/dashboard/src/test/PositionStatusBadge.test.tsx new file mode 100644 index 00000000..8b548e93 --- /dev/null +++ b/dashboard/src/test/PositionStatusBadge.test.tsx @@ -0,0 +1,20 @@ +import { render, screen } from "@testing-library/react"; +import { describe, expect, it } from "vitest"; + +import { PositionStatusBadge } from "../components/PositionStatusBadge"; + +describe("PositionStatusBadge", () => { + it("renders nothing for a positioned node", () => { + const { container } = render(); + expect(container).toBeEmptyDOMElement(); + }); + + it.each([ + ["missing_both", "No position"], + ["missing_rx", "No receiver position"], + ["missing_tx", "No illuminator position"], + ])("labels %s", (status, label) => { + render(); + expect(screen.getByText(label)).toBeInTheDocument(); + }); +}); diff --git a/dashboard/src/types.ts b/dashboard/src/types.ts index ba70e232..e2b26f32 100644 --- a/dashboard/src/types.ts +++ b/dashboard/src/types.ts @@ -63,6 +63,11 @@ export interface RadarNode { empirical_n_points: number; } +/** Which ends of a node's bistatic pair have coordinates. Orthogonal to a + * node's `status` (liveness): a node can be actively detecting and still + * be anything but "positioned". */ +export type PositionStatus = "positioned" | "missing_rx" | "missing_tx" | "missing_both"; + /* ---- Dashboard / fleet ---- */ export interface FleetDashboard { From 17514401c7192e6e6cced799dfbbca5774386ddb Mon Sep 17 00:00:00 2001 From: Babissimo Date: Mon, 7 Sep 2026 10:54:11 +0100 Subject: [PATCH 05/10] Build a solver pipeline only for a node we can place 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 --- backend/routes/node_stream.py | 12 +-- backend/services/frame_processor.py | 132 ++++++++++++++---------- backend/tests/test_adsb_seed_backend.py | 4 + backend/tests/test_dark_follow.py | 12 ++- backend/tests/test_frame_processor.py | 38 ++++++- backend/tests/test_node_streaming.py | 11 +- backend/tests/test_positionless_node.py | 20 ++-- 7 files changed, 142 insertions(+), 87 deletions(-) diff --git a/backend/routes/node_stream.py b/backend/routes/node_stream.py index 4f6c211b..8fbf6ffc 100644 --- a/backend/routes/node_stream.py +++ b/backend/routes/node_stream.py @@ -134,13 +134,11 @@ def _file_frame(node_id: str, frame: DetectionFrame) -> int: not at all: the count is the array length or nothing. A node absent from `state.connected_nodes` is declined rather than queued. - frame_processor has no geometry for such a node and falls back to the - process-wide default pipeline, so the frame would be solved against - somebody else's receiver and transmitter and reach the map as a plausible - detection in the wrong place, while the ack claimed it was accepted. - Declining costs at most one heartbeat interval of this node's data, and the - next beat restores it by re-registering the node. Queueing costs - correctness, silently, which is the worse trade. + This server holds no configuration for such a node at all, so there is no + config_hash to check staleness against and nothing to place, count or + attribute the frame under. Declining costs at most one heartbeat interval + of this node's data, and the next beat restores it by re-registering the + node. Queueing costs correctness, silently, which is the worse trade. Recovery deliberately does not happen here. It needs a database read and a write to the registries, and this is the path that runs at the fleet's frame diff --git a/backend/services/frame_processor.py b/backend/services/frame_processor.py index a05b115e..a364d576 100644 --- a/backend/services/frame_processor.py +++ b/backend/services/frame_processor.py @@ -28,6 +28,7 @@ ) from services.id_utils import normalize_hex_key as _normalize_hex_key from services.known_claiming import claim_known_targets, strip_claimed_detections +from services.node_config import position_status from services.storage import archive_detections # ── Archive batching ────────────────────────────────────────────────────────── @@ -238,23 +239,32 @@ def configs_for_solver_input(node_cfgs: dict[str, dict], s_in: dict) -> dict[str def get_or_create_node_pipeline( node_id: str, default_pipeline: PassiveRadarPipeline, -) -> PassiveRadarPipeline: +) -> PassiveRadarPipeline | None: + """The node's own pipeline, built from its config and cached from then on. + + None for a node this server cannot place: solving its frames against + default_pipeline's fixed geometry would geolocate them at somebody else's + receiver and illuminator and publish them under this node's id. + """ pipeline = state.node_pipelines.get(node_id) if pipeline is not None: return pipeline cfg = state.connected_nodes.get(node_id, {}).get("config", {}) - if cfg.get("rx_lat") and cfg.get("tx_lat"): + if position_status(cfg) == "positioned": pipeline_cfg = { "node_id": node_id, "Fs": cfg.get("fs_hz", cfg.get("Fs", 2_000_000)), "FC": cfg.get("fc_hz", cfg.get("FC", 195_000_000)), "rx_lat": cfg["rx_lat"], "rx_lon": cfg["rx_lon"], - "rx_alt_ft": cfg.get("rx_alt_ft", 900), + # Independently nullable: `.get(key, default)` would not apply the + # default to an explicit null, and a real altitude of 0.0 (sea + # level) rules out `or` as well. + "rx_alt_ft": 900 if cfg.get("rx_alt_ft") is None else cfg["rx_alt_ft"], "tx_lat": cfg["tx_lat"], "tx_lon": cfg["tx_lon"], - "tx_alt_ft": cfg.get("tx_alt_ft", 1200), + "tx_alt_ft": 1200 if cfg.get("tx_alt_ft") is None else cfg["tx_alt_ft"], "doppler_min": cfg.get("doppler_min", -300), "doppler_max": cfg.get("doppler_max", 300), "min_doppler": cfg.get("min_doppler", 15), @@ -272,7 +282,7 @@ def get_or_create_node_pipeline( state.node_pipelines[node_id] = pipeline return pipeline - return default_pipeline + return None # ── Per-frame processing (runs in thread pool) ─────────────────────────────── @@ -481,7 +491,11 @@ def process_one_frame(node_id: str, frame: dict, default_pipeline: PassiveRadarP # association directly, so there is no inert way to shadow this. if state.ADSB_SEED_MODE == "active" and not _pframe.get("adsb"): _geo = state.node_associator.node_geometries.get(node_id) - if _geo is not None: + # A geometry exists even for a node this server cannot place (see + # get_or_create_node_pipeline's docstring), so presence alone is not + # enough; the node must also be positioned. + _cfg = state.node_associator.node_configs.get(node_id, {}) + if _geo is not None and position_status(_cfg) == "positioned": # Own-world states only: this is a cache-wide assignment for a # node with no receiver, so every other-world entry is a decoy # its detections can bind to on a delay/Doppler coincidence — @@ -500,60 +514,66 @@ def process_one_frame(node_id: str, frame: dict, default_pipeline: PassiveRadarP if _tags is not None: _pframe["adsb"] = _tags state.bump_counter("adsb_seed_frames_autotagged") + # None for a node this server cannot place (see get_or_create_node_pipeline): + # its frame is still counted above by record_detection_frame, but it is + # not geolocated, tracked, associated or handed to the solver: there is + # no geometry to place any of that against. pipeline = get_or_create_node_pipeline(node_id, default_pipeline) - pipeline.process_frame(_pframe) + if pipeline is not None: + pipeline.process_frame(_pframe) _d_pipeline = time.thread_time() - _t3 - _d_known _t2 = time.thread_time() - _ts_ms_assoc = frame.get("timestamp", 0) - # Track-level association. The detection-level path it replaced now lives - # in retina_analytics.detection_association, reachable only from the - # offline bench, which keeps it as the A/B baseline. - _track_views = _node_track_views(pipeline, _ts_ms_assoc or None) - # Feed the per-node distinct-track counters — total_tracks / - # geolocated_tracks were exported (and read by the admin API) but never - # written anywhere. - state.node_analytics.record_node_tracks( - node_id, - (v["track_id"] for v in _track_views), - list(pipeline.geolocated_tracks.keys()), - ) - round_ = state.node_associator.submit_tracks_round( - node_id, - _track_views, - _ts_ms_assoc, - ) - # anchored_inputs (top-down claiming, ASSOC_CLAIM_MODE=active) and - # adsb_inputs (ADS-B seeding, ADSB_SEED_MODE=active) are already in - # solver-input shape — see _claim_round / _adsb_seed_round — so they - # join the bottom-up pairs' formatted output directly. Both empty in - # off/shadow mode. - solver_inputs = ( - (state.node_associator.format_track_pairs_for_solver(round_.pairs) if round_.pairs else []) - + round_.anchored_inputs - + round_.adsb_inputs - ) - if solver_inputs: - node_cfgs = get_node_configs() - for s_in in solver_inputs: - if s_in["n_nodes"] < 2: - continue - try: - state.solver_queue.put_nowait((s_in, configs_for_solver_input(node_cfgs, s_in), time.time())) - except Exception: - state.bump_counter("solver_queue_drops") - if state.solver_queue_drops % 100 == 1: - logging.warning( - "Solver queue full — dropped %d candidates total", - state.solver_queue_drops, - ) - from services.alerting import send_alert - - send_alert( - "solver_queue_drops", - f"Solver queue full — {state.solver_queue_drops} candidates dropped", - {"total_drops": state.solver_queue_drops}, - ) + if pipeline is not None: + _ts_ms_assoc = frame.get("timestamp", 0) + # Track-level association. The detection-level path it replaced now lives + # in retina_analytics.detection_association, reachable only from the + # offline bench, which keeps it as the A/B baseline. + _track_views = _node_track_views(pipeline, _ts_ms_assoc or None) + # Feed the per-node distinct-track counters — total_tracks / + # geolocated_tracks were exported (and read by the admin API) but never + # written anywhere. + state.node_analytics.record_node_tracks( + node_id, + (v["track_id"] for v in _track_views), + list(pipeline.geolocated_tracks.keys()), + ) + round_ = state.node_associator.submit_tracks_round( + node_id, + _track_views, + _ts_ms_assoc, + ) + # anchored_inputs (top-down claiming, ASSOC_CLAIM_MODE=active) and + # adsb_inputs (ADS-B seeding, ADSB_SEED_MODE=active) are already in + # solver-input shape — see _claim_round / _adsb_seed_round — so they + # join the bottom-up pairs' formatted output directly. Both empty in + # off/shadow mode. + solver_inputs = ( + (state.node_associator.format_track_pairs_for_solver(round_.pairs) if round_.pairs else []) + + round_.anchored_inputs + + round_.adsb_inputs + ) + if solver_inputs: + node_cfgs = get_node_configs() + for s_in in solver_inputs: + if s_in["n_nodes"] < 2: + continue + try: + state.solver_queue.put_nowait((s_in, configs_for_solver_input(node_cfgs, s_in), time.time())) + except Exception: + state.bump_counter("solver_queue_drops") + if state.solver_queue_drops % 100 == 1: + logging.warning( + "Solver queue full — dropped %d candidates total", + state.solver_queue_drops, + ) + from services.alerting import send_alert + + send_alert( + "solver_queue_drops", + f"Solver queue full — {state.solver_queue_drops} candidates dropped", + {"total_drops": state.solver_queue_drops}, + ) _d_assoc = time.thread_time() - _t2 # ADS-B extraction: TCP handler runs _apply_synthetic_adsb for synth nodes diff --git a/backend/tests/test_adsb_seed_backend.py b/backend/tests/test_adsb_seed_backend.py index 77b3fc86..80b6964a 100644 --- a/backend/tests/test_adsb_seed_backend.py +++ b/backend/tests/test_adsb_seed_backend.py @@ -364,6 +364,10 @@ def test_adsb_inputs_reach_the_solver_queue(self, monkeypatch): ) monkeypatch.setattr(state, "solver_queue", queue.Queue()) + # A positioned node: process_one_frame only reaches submit_tracks_round + # (where the stub above is installed) for a node it can place. + with state.connected_nodes_lock: + state.connected_nodes["test-adsb-seed-queue"] = {"config": dict(_NODE_CFG)} default = PassiveRadarPipeline(DEFAULT_NODE_CONFIG) process_one_frame("test-adsb-seed-queue", _make_frame(), default) diff --git a/backend/tests/test_dark_follow.py b/backend/tests/test_dark_follow.py index 1e8f46de..011ce4d4 100644 --- a/backend/tests/test_dark_follow.py +++ b/backend/tests/test_dark_follow.py @@ -37,10 +37,11 @@ from pipeline.passive_radar import DEFAULT_NODE_CONFIG, PassiveRadarPipeline from services import dark_follow, track_filter from services import known_claiming as kc -from services.frame_processor import process_one_frame +from services.frame_processor import get_or_create_node_pipeline, process_one_frame from services.geo import offset_latlon_m from services.tasks import known_lane from services.tasks import solver as solver_mod +from tests.node_helpers import register_test_node _NODE_CFG = { "rx_lat": 34.85, @@ -647,6 +648,10 @@ class TestModesInProcessOneFrame: processes; shadow leaves the frame whole.""" def _run(self, monkeypatch, mode): + # Registered through the shared helper, not the associator alone: a node + # absent from connected_nodes cannot be placed, so it gets no pipeline + # and process_one_frame skips every per-node branch this class names. + register_test_node(_NODE_ID, _NODE_CFG) ts = int(time.time() * 1000) geo = _install(monkeypatch, ts - 2000, mode=mode) monkeypatch.setattr(state, "KNOWN_LANE_MODE", "binding") @@ -654,8 +659,11 @@ def _run(self, monkeypatch, mode): frame = _frame(ts, [pd, pd + 500.0], [pf, pf + 500.0]) default = PassiveRadarPipeline(DEFAULT_NODE_CONFIG) + # The node's own pipeline, built from its own geometry, is what + # process_one_frame hands the frame to; `default` never sees it. seen = [] - monkeypatch.setattr(default, "process_frame", lambda f: seen.append(f)) + node_pipeline = get_or_create_node_pipeline(_NODE_ID, default) + monkeypatch.setattr(node_pipeline, "process_frame", lambda f: seen.append(f)) process_one_frame(_NODE_ID, frame, default) assert len(seen) == 1 return frame, seen[0] diff --git a/backend/tests/test_frame_processor.py b/backend/tests/test_frame_processor.py index b824071b..efb7592f 100644 --- a/backend/tests/test_frame_processor.py +++ b/backend/tests/test_frame_processor.py @@ -195,10 +195,34 @@ def test_returns_cached_pipeline(self): p2 = get_or_create_node_pipeline("test-cached", default) assert p1 is p2 - def test_falls_back_to_default(self): + def test_returns_none_for_a_node_with_no_usable_position(self): + """Not a fall-back to `default`: solving an unplaceable node's frames + against the shared pipeline's fixed geometry would geolocate them at + somebody else's receiver and illuminator.""" default = PassiveRadarPipeline(DEFAULT_NODE_CONFIG) p = get_or_create_node_pipeline("test-noconfig", default) - assert p is default + assert p is None + + def test_null_altitudes_default_rather_than_reach_the_geolocator_as_none(self): + """rx_alt_ft/tx_alt_ft are independently nullable; `cfg.get(key, default)` + does not apply the default when the key is present with value None, and + PassiveRadarPipeline._init_geolocator multiplies the altitude by + FT_TO_M unconditionally, so a null here must resolve before construction + rather than reach it.""" + default = PassiveRadarPipeline(DEFAULT_NODE_CONFIG) + state.connected_nodes["test-null-altitude"] = { + "config": { + "rx_lat": 34.0, + "rx_lon": -84.0, + "rx_alt_ft": None, + "tx_lat": 33.8, + "tx_lon": -83.8, + "tx_alt_ft": None, + }, + } + p = get_or_create_node_pipeline("test-null-altitude", default) + assert p.config["rx_alt_ft"] == 900 + assert p.config["tx_alt_ft"] == 1200 # ── Frame processing ───────────────────────────────────────────────────────── @@ -270,6 +294,16 @@ def test_claiming_anchored_inputs_reach_the_solver_queue(self, monkeypatch): # also guarantees only this frame's item is seen. monkeypatch.setattr(state, "solver_queue", queue.Queue()) + # A positioned node: process_one_frame only reaches submit_tracks_round + # (where the stub above is installed) for a node it can place. + state.connected_nodes["test-anchor"] = { + "config": { + "rx_lat": 34.0, + "rx_lon": -84.0, + "tx_lat": 33.8, + "tx_lon": -83.8, + }, + } default = PassiveRadarPipeline(DEFAULT_NODE_CONFIG) process_one_frame("test-anchor", _make_frame(), default) diff --git a/backend/tests/test_node_streaming.py b/backend/tests/test_node_streaming.py index 690de68d..644df5d0 100644 --- a/backend/tests/test_node_streaming.py +++ b/backend/tests/test_node_streaming.py @@ -305,13 +305,12 @@ async def test_a_blocked_node_s_frames_stay_out_of_the_pipeline(registered_node, async def test_a_frame_from_a_node_absent_from_the_pipeline_is_not_filed(registered_node, node_client): - """Not filed, because frame_processor would solve it against the wrong geometry. + """Not filed, because this server holds no configuration for the node at all. - `get_or_create_node_pipeline` falls back to the process-wide default - pipeline for a node it holds no configuration for, so the frame would reach - the map as a plausible detection against somebody else's receiver and - transmitter, with an ack claiming it was accepted. Wrong data is worse than - the gap, and the gap is at most one heartbeat interval wide. + There is no config_hash to check staleness against and nothing to place, + count or attribute the frame under, with an ack claiming it was accepted + regardless. Wrong data is worse than the gap, and the gap is at most one + heartbeat interval wide. """ token, _ = registered_node state.connected_nodes.clear() diff --git a/backend/tests/test_positionless_node.py b/backend/tests/test_positionless_node.py index f1df8ecb..c8636b79 100644 --- a/backend/tests/test_positionless_node.py +++ b/backend/tests/test_positionless_node.py @@ -77,15 +77,10 @@ def test_ten_positionless_nodes_form_no_overlap_zones(): def test_positionless_node_builds_no_solver_pipeline(): """The never-solve half: a positionless node builds no solver pipeline. - Holds today because get_or_create_node_pipeline's guard at - frame_processor.py:171, `if cfg.get("rx_lat") and cfg.get("tx_lat")`, - falls through to the shared default pipeline only because None is falsy: - a known truthiness bug, 86cbavanm. - - Worth pinning because a correct repair to `is not None` preserves this - fall-through, but a repair that tests key presence (`"rx_lat" in cfg`) - instead of value nullity does not: the key is present, carrying None, so - pipeline construction proceeds and raises a TypeError. + get_or_create_node_pipeline returns None rather than falling back to the + shared default pipeline: solving this node's frames against the default's + fixed geometry would geolocate them at somebody else's receiver and + illuminator, and publish them under this node's id. """ from services.frame_processor import get_or_create_node_pipeline @@ -101,11 +96,8 @@ def test_positionless_node_builds_no_solver_pipeline(): "capabilities": {}, } - # It falls back to the shared default rather than returning None, so assert - # identity against a sentinel: a node-specific pipeline would be a new - # object, and would also register itself in state.node_pipelines. - sentinel = object() - assert get_or_create_node_pipeline(node_id, sentinel) is sentinel + default = object() + assert get_or_create_node_pipeline(node_id, default) is None assert node_id not in state.node_pipelines From bf07d9deaf4f7530e7ad32a75d0932042fc3d21b Mon Sep 17 00:00:00 2001 From: Babissimo Date: Mon, 7 Sep 2026 10:54:11 +0100 Subject: [PATCH 06/10] Stop a zero coordinate vanishing and a null one raising 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 --- backend/services/known_claiming.py | 12 +- backend/services/tasks/analytics_refresh.py | 14 +-- backend/tests/test_known_claiming.py | 28 +++++ backend/tests/test_periodic_adsb_bbox.py | 105 ++++++++++++++++++ .../src/pages/user/RFEnvironmentPage.tsx | 4 +- frontend/src/components/map/geo.ts | 2 +- 6 files changed, 151 insertions(+), 14 deletions(-) create mode 100644 backend/tests/test_periodic_adsb_bbox.py diff --git a/backend/services/known_claiming.py b/backend/services/known_claiming.py index b5dac45c..9f460c62 100644 --- a/backend/services/known_claiming.py +++ b/backend/services/known_claiming.py @@ -85,6 +85,7 @@ from core import state from services import dark_follow, track_filter from services.id_utils import normalize_hex_key +from services.node_config import position_status # Same base constants as the seeding path: the comparison is the identical # "measurement vs dead-reckoned ADS-B fix" shape, so a different base gate @@ -803,10 +804,11 @@ def claim_known_targets(node_id: str, frame: dict, follow_claimed: set[int] | No lane's claims from the frame without the other's. Omit it and path 3 does not run at all — a caller that cannot receive the split cannot honour it. - Claims nothing without a registered geometry: the registry contract - requires the predicted observation, and there is nothing to predict - with. Fail toward dark, the same discipline every ADS-B doubt-case in - this pipeline follows. + Claims nothing without a positioned node: predict_observation needs both + ends of the bistatic pair, and a node this server cannot place has + nothing to predict against, whatever its geometry entry coerced an + unplaced coordinate to. Fail toward dark, the same discipline every + ADS-B doubt-case in this pipeline follows. """ delays = frame.get("delay") or [] dopplers = frame.get("doppler") or [] @@ -815,6 +817,8 @@ def claim_known_targets(node_id: str, frame: dict, follow_claimed: set[int] | No geo = state.node_associator.node_geometries.get(node_id) if geo is None: return set() + if position_status(state.node_associator.node_configs.get(node_id, {})) != "positioned": + return set() ts_ms = int(frame.get("timestamp", 0)) frame_ts_s = ts_ms / 1000.0 diff --git a/backend/services/tasks/analytics_refresh.py b/backend/services/tasks/analytics_refresh.py index 9aa1c8ff..6a868ea2 100644 --- a/backend/services/tasks/analytics_refresh.py +++ b/backend/services/tasks/analytics_refresh.py @@ -611,12 +611,12 @@ def _refresh_missed_detections(nodes_snapshot: list): if info.get("status") == "disconnected": continue cfg = info.get("config", {}) + if position_status(cfg) != "positioned": + continue rx_lat = cfg.get("rx_lat") rx_lon = cfg.get("rx_lon") tx_lat = cfg.get("tx_lat") tx_lon = cfg.get("tx_lon") - if not all((rx_lat, rx_lon, tx_lat, tx_lon)): - continue # Resolved the same way every module resolves it: explicit aim, else # broadside off the RX→TX baseline (Yagi sits perpendicular to it), @@ -959,12 +959,12 @@ def _refresh_node_verification(node_id: str): if not measured_delay_us or measured_delay_us <= 0: continue - tx_lat = cfg.get("tx_lat") or 0.0 - tx_lon = cfg.get("tx_lon") or 0.0 - rx_lat = cfg.get("rx_lat") or 0.0 - rx_lon = cfg.get("rx_lon") or 0.0 - if not tx_lat or not rx_lat: + if position_status(cfg) != "positioned": continue + tx_lat = cfg.get("tx_lat") + tx_lon = cfg.get("tx_lon") + rx_lat = cfg.get("rx_lat") + rx_lon = cfg.get("rx_lon") solver_lat = getattr(track, "lat", 0.0) or 0.0 solver_lon = getattr(track, "lon", 0.0) or 0.0 diff --git a/backend/tests/test_known_claiming.py b/backend/tests/test_known_claiming.py index c99d4ff2..b06f4eb0 100644 --- a/backend/tests/test_known_claiming.py +++ b/backend/tests/test_known_claiming.py @@ -128,6 +128,18 @@ def test_no_geometry_claims_nothing(self): assert claimed == set() assert state.known_claims == {} + def test_positionless_node_claims_nothing(self): + """register_node still builds a NodeGeometry for a node missing a + coordinate (rx_lat/rx_lon coerced to 0.0), so `geo is not None` alone + would admit it; the node must also read as positioned.""" + node_id = "test-known-claiming-positionless" + state.node_associator.register_node(node_id, dict(_NODE_CFG, rx_lat=None, rx_lon=None)) + ts = int(time.time() * 1000) + _cache_state("aaa111", ts) + claimed = kc.claim_known_targets(node_id, _frame(ts, [50.0], [10.0])) + assert claimed == set() + assert state.known_claims == {} + class TestGateVsFixAge: """The allowance doubles linearly toward the 45 s age cap: a residual the @@ -417,6 +429,17 @@ def _cloud(self, rng, geo, frame_ts_s): def test_prescreen_never_disagrees_with_the_gate(self, name, monkeypatch): geo = self._GEOMETRIES[name] state.node_associator.node_geometries[_NODE_ID] = geo + # coverage_limit/fov are callables the config dict has no way to carry, + # so this builds NodeGeometry directly rather than through + # register_node, which is also claim_known_targets's other source of + # "is this node positioned"; without an entry here every geometry + # variant would read as unplaced regardless of its own coordinates. + state.node_associator.node_configs[_NODE_ID] = { + "rx_lat": geo.rx_lat, + "rx_lon": geo.rx_lon, + "tx_lat": geo.tx_lat, + "tx_lon": geo.tx_lon, + } ts = int(time.time() * 1000) frame_ts_s = ts / 1000.0 # String seed, not hash(name): str hashing is salted per interpreter, @@ -581,6 +604,11 @@ def _run(self, monkeypatch, mode): default = PassiveRadarPipeline(DEFAULT_NODE_CONFIG) seen = [] monkeypatch.setattr(default, "process_frame", lambda f: seen.append(f)) + # Pre-seed the pipeline cache: get_or_create_node_pipeline no longer + # falls back to `default` for a node it holds no connected_nodes + # config for (_register only registers it with the associator), so + # this stands in for the node's own, already-built pipeline. + state.node_pipelines[_NODE_ID] = default process_one_frame(_NODE_ID, frame, default) return frame, seen[0] diff --git a/backend/tests/test_periodic_adsb_bbox.py b/backend/tests/test_periodic_adsb_bbox.py new file mode 100644 index 00000000..e4b18366 --- /dev/null +++ b/backend/tests/test_periodic_adsb_bbox.py @@ -0,0 +1,105 @@ +"""_fetch_external_adsb's node positions, in services/tasks/periodic.py. + +The query regions are built from connected_nodes' configs, which since 1.1.3 +may carry a null position. Two layers drop one: the node loop here, and +regions_for_nodes via is_position_absent / is_usable. These pin the outcome +rather than either mechanism, so removing one layer leaves them green and +removing both fails them with the TypeError being guarded against, raised in +cell_of. That is the failure worth a test: _fetch_external_adsb's caller +swallows the exception and never retries, so one unplaced node would cost the +whole fleet its ADS-B ground truth silently and for good. +""" + +import asyncio + +import pytest + +from core import state +from services.tasks import periodic + + +class _FakeResponse: + status_code = 200 + + def __init__(self, states): + self._states = states + + def json(self): + return {"states": self._states} + + +class _FakeOpenSkyClient: + """Stands in for httpx.AsyncClient, capturing each box a call requests.""" + + is_closed = False + + def __init__(self, states): + self._states = states + self.calls: list[dict] = [] + + async def get(self, url, params=None): + self.calls.append(params) + return _FakeResponse(self._states) + + +# One minimal OpenSky state vector: [icao, callsign, origin, ts, ts, lon, lat, alt, ...]. +_ONE_STATE = [["abc123", "TST1", None, None, None, -84.5, 33.85, 1000.0, False, 100.0, 90.0]] + +_POSITIONED = (33.9, -84.6) + + +@pytest.fixture(autouse=True) +def _clean_nodes(): + yield + with state.connected_nodes_lock: + for node_id in ("test-bbox-positionless", "test-bbox-positioned"): + state.connected_nodes.pop(node_id, None) + + +@pytest.fixture +def _no_fallback(monkeypatch): + """Keep the adsb.lol fallback off the network for a partially covered region.""" + + async def _none(_uncovered): + return {}, set() + + monkeypatch.setattr(periodic, "_fetch_adsb_lol", _none) + + +def _add_node(node_id, lat, lon): + with state.connected_nodes_lock: + state.connected_nodes[node_id] = { + "status": "active", + "is_synthetic": False, + "config": {"rx_lat": lat, "rx_lon": lon}, + } + + +def test_a_positionless_node_does_not_cost_the_fleet_its_regions(monkeypatch, _no_fallback): + """A mixed fleet still queries, from the positioned node alone.""" + _add_node("test-bbox-positionless", None, None) + _add_node("test-bbox-positioned", *_POSITIONED) + + fake_client = _FakeOpenSkyClient(_ONE_STATE) + monkeypatch.setattr(periodic, "_opensky_client", fake_client) + + rate_limited = asyncio.run(periodic._fetch_external_adsb()) + + assert rate_limited is False + assert fake_client.calls, "the positioned node should still have been queried" + lat, lon = _POSITIONED + assert any(p["lamin"] <= lat <= p["lamax"] and p["lomin"] <= lon <= p["lomax"] for p in fake_client.calls), ( + f"no requested box covers the positioned node: {fake_client.calls}" + ) + + +def test_all_nodes_positionless_skips_the_fetch(monkeypatch, _no_fallback): + _add_node("test-bbox-positionless", None, None) + + fake_client = _FakeOpenSkyClient(_ONE_STATE) + monkeypatch.setattr(periodic, "_opensky_client", fake_client) + + rate_limited = asyncio.run(periodic._fetch_external_adsb()) + + assert rate_limited is False + assert fake_client.calls == [] diff --git a/dashboard/src/pages/user/RFEnvironmentPage.tsx b/dashboard/src/pages/user/RFEnvironmentPage.tsx index c195e0da..d5d89480 100644 --- a/dashboard/src/pages/user/RFEnvironmentPage.tsx +++ b/dashboard/src/pages/user/RFEnvironmentPage.tsx @@ -165,8 +165,8 @@ export default function RFEnvironmentPage() { Average SNR{(metrics.avg_snr || 0).toFixed(2)} dB Total Frames Processed{(metrics.total_frames || 0).toLocaleString()} Detection Rate{metrics.total_frames ? ((metrics.total_detections / metrics.total_frames) * 100).toFixed(1) + "%" : "—"} - RX Location{location.rx_lat && location.rx_lon ? `${location.rx_lat.toFixed(4)}, ${location.rx_lon.toFixed(4)}` : "—"} - TX Location{location.tx_lat && location.tx_lon ? `${location.tx_lat.toFixed(4)}, ${location.tx_lon.toFixed(4)}` : "—"} + RX Location{location.rx_lat != null && location.rx_lon != null ? `${location.rx_lat.toFixed(4)}, ${location.rx_lon.toFixed(4)}` : "—"} + TX Location{location.tx_lat != null && location.tx_lon != null ? `${location.tx_lat.toFixed(4)}, ${location.tx_lon.toFixed(4)}` : "—"}
diff --git a/frontend/src/components/map/geo.ts b/frontend/src/components/map/geo.ts index a8f3d3ea..076c16bb 100644 --- a/frontend/src/components/map/geo.ts +++ b/frontend/src/components/map/geo.ts @@ -66,7 +66,7 @@ export function getFocusPoints(aircraft, nodes, selectedHex) { } return nodes - .filter((n) => n.rx_lat && n.rx_lon) + .filter((n) => validLatLon(n.rx_lat, n.rx_lon)) .map((n) => [n.rx_lat, n.rx_lon]); } From a8d7b0c92d79a5d3582020d7528a6d0edac8a3dc Mon Sep 17 00:00:00 2001 From: Babissimo Date: Mon, 7 Sep 2026 10:54:12 +0100 Subject: [PATCH 07/10] Correct a beam-width comment and ONBOARDING's editable installs 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 --- ONBOARDING.md | 3 ++- backend/routes/node_config.py | 6 ------ 2 files changed, 2 insertions(+), 7 deletions(-) diff --git a/ONBOARDING.md b/ONBOARDING.md index 47b09fdd..91302e89 100644 --- a/ONBOARDING.md +++ b/ONBOARDING.md @@ -71,7 +71,8 @@ git submodule update --init --recursive cd backend python -m venv .venv && source .venv/bin/activate pip install -r requirements.txt -r requirements-dev.txt -pip install -e ../libs/retina-geolocator -e ../libs/retina-tracker +pip install -e ../libs/retina-geolocator -e ../libs/retina-tracker \ + -e ../libs/retina-custody -e ../libs/retina-simulation -e ../libs/retina-analytics cp .env.example .env # fill in what you need (see below) RETINA_ENV=dev AUTH_ALLOW_ANONYMOUS_ADMIN=1 SYNTHETIC_FLEET_ENABLED=1 uvicorn main:app --reload ``` diff --git a/backend/routes/node_config.py b/backend/routes/node_config.py index 3bbf32d5..81e79a30 100644 --- a/backend/routes/node_config.py +++ b/backend/routes/node_config.py @@ -158,12 +158,6 @@ async def _hand_to_pipeline(session: AsyncSession, node: Node, version: int) -> retried. The version is committed, so the node row already reads it, and the next identical resend finds nothing changed and never reaches here. The alert is therefore the whole of the recovery path, which is why it carries the version. - - One failure is live rather than hypothetical: retina-analytics reads - `config.get("beam_width_deg", 41)`, so an explicit null passes its default by, and - the unchanged-geometry comparison in association.py then subtracts it. Every node - in the fleet sends a null width under contract 1.1.1. Tracked in 86cb5dakr, with - the ordering above; neither is this endpoint's to fix. """ from services.alerting import send_alert From 57104d722faffa4f4530d0be6e10469cbbd58247 Mon Sep 17 00:00:00 2001 From: Babissimo Date: Mon, 7 Sep 2026 10:54:26 +0100 Subject: [PATCH 08/10] Give a node's config one canonical shape, established at every entry "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 --- backend/routes/admin.py | 3 +- backend/routes/radar.py | 10 +- backend/scripts/association_bench.py | 8 + backend/services/blah2_bridge.py | 8 +- backend/services/frame_processor.py | 10 +- backend/services/geo.py | 20 +- backend/services/node_config.py | 118 +++++++++--- backend/services/node_pipeline.py | 9 +- backend/services/node_registration.py | 9 +- backend/services/tasks/analytics_refresh.py | 14 +- backend/services/tcp_handler.py | 12 +- backend/services/track_gates.py | 13 +- backend/tests/node_helpers.py | 35 ++++ backend/tests/test_adsb_seed_backend.py | 10 +- backend/tests/test_frame_processor.py | 40 ++-- backend/tests/test_known_claiming.py | 15 +- backend/tests/test_node_config_validation.py | 183 ++++++++++++++++++- backend/tests/test_node_pipeline.py | 46 +++++ backend/tests/test_storage.py | 30 +++ backend/tests/test_tcp_handler.py | 31 +++- 20 files changed, 527 insertions(+), 97 deletions(-) create mode 100644 backend/tests/node_helpers.py diff --git a/backend/routes/admin.py b/backend/routes/admin.py index bb884276..3659bb0b 100644 --- a/backend/routes/admin.py +++ b/backend/routes/admin.py @@ -423,7 +423,8 @@ async def get_tower_config(_admin=Depends(require_admin)): cfg = info.get("config", {}) tx_lat = cfg.get("tx_lat") tx_lon = cfg.get("tx_lon") - if tx_lat and tx_lon: + # A transmitter on the equator or the prime meridian is a real tower. + if tx_lat is not None and tx_lon is not None: key = f"{tx_lat:.4f},{tx_lon:.4f}" if key not in towers: towers[key] = { diff --git a/backend/routes/radar.py b/backend/routes/radar.py index c99ab526..56498e33 100644 --- a/backend/routes/radar.py +++ b/backend/routes/radar.py @@ -15,6 +15,7 @@ from core.users import require_admin from pipeline.passive_radar import PassiveRadarPipeline from services import node_registration +from services.node_config import canonical_config from services.node_pipeline import config_hash from services.public_location import public_latlon from services.publication import is_private @@ -129,17 +130,20 @@ async def ingest_detections( frames = body.frames if body.frames is not None else [body_dict] if node_id not in state.connected_nodes: + # This path carries no geometry at all: the node is counted, and stays + # unplaced until it configures itself over TCP or the v1 API. + legacy_config = canonical_config({"node_id": node_id}) with state.connected_nodes_lock: state.connected_nodes[node_id] = { "config_hash": "", - "config": {"node_id": node_id}, + "config": legacy_config, "status": "active", "last_heartbeat": datetime.now(timezone.utc).isoformat(), "peer": "http", "is_synthetic": is_synthetic_node(node_id), "capabilities": {}, } - await node_registration.register_node(node_id, {"node_id": node_id}) + await node_registration.register_node(node_id, legacy_config) else: with state.connected_nodes_lock: state.connected_nodes[node_id]["status"] = "active" @@ -189,7 +193,9 @@ async def ingest_detections_bulk( changed = False else: entry_config = entry.config or {"node_id": node_id} + # Hashed as declared, stored canonical: see register_with_pipeline. entry_hash = config_hash(entry_config) + entry_config = canonical_config(entry_config) # A hash mismatch only triggers re-registration for a node this # endpoint itself created. Otherwise a caller holding RADAR_API_KEY # could strip a live v1 or TCP node's geometry by naming it in a diff --git a/backend/scripts/association_bench.py b/backend/scripts/association_bench.py index f392efad..aee1e586 100644 --- a/backend/scripts/association_bench.py +++ b/backend/scripts/association_bench.py @@ -96,6 +96,7 @@ node_beam_params, # noqa: E402 ) from services.geo import haversine_km as _haversine_km # noqa: E402 +from services.node_config import position_status # noqa: E402 from services.tasks.solver import ( # noqa: E402 _ewma_smooth_track, claim_decision, @@ -422,6 +423,13 @@ def _beam_gate_ok(out: dict, s_in: dict, node_cfgs: dict, fov_provider) -> bool: cfg = node_cfgs.get(nid) if not cfg: continue + # The same placement guard solver.py applies before its range/bearing + # work: node_beam_params stopped coercing a missing coordinate to 0.0, + # so an unplaced node reaches the haversine below as None. A snapshot + # read from a live server carries only placed nodes, but this leg is + # also pointed at recorded ones. + if position_status(cfg) not in ("positioned", "missing_tx"): + continue p = node_beam_params(cfg) rx_lat, rx_lon = p["rx_lat"], p["rx_lon"] range_km = _haversine_km(rx_lat, rx_lon, out["lat"], out["lon"]) diff --git a/backend/services/blah2_bridge.py b/backend/services/blah2_bridge.py index 7a1938ff..e854998c 100644 --- a/backend/services/blah2_bridge.py +++ b/backend/services/blah2_bridge.py @@ -48,6 +48,7 @@ from core.runtime_config import default_source_path, runtime_path from core.task_registry import register_task from services import node_registration +from services.node_config import canonical_config log = logging.getLogger("blah2_bridge") @@ -218,18 +219,21 @@ def load_nodes(path: Path | None = None) -> list[Blah2Node]: async def _register_node(node: Blah2Node): """Register a node in state as a real (non-synthetic) connected node.""" + # Hashed over the file's own config, so a node's hash tracks the file + # rather than the normaliser. cfg_hash = hashlib.sha256(json.dumps(node.config, sort_keys=True).encode()).hexdigest()[:16] + config = canonical_config(node.config) with state.connected_nodes_lock: state.connected_nodes[node.node_id] = { "config_hash": cfg_hash, - "config": node.config, + "config": config, "status": "active", "last_heartbeat": "", "peer": node.peer, "is_synthetic": False, "capabilities": {"adsb_report": True}, } - await node_registration.register_node(node.node_id, node.config) + await node_registration.register_node(node.node_id, config) log.info("blah2_bridge: registered node %s", node.node_id) diff --git a/backend/services/frame_processor.py b/backend/services/frame_processor.py index a364d576..4e5ecf39 100644 --- a/backend/services/frame_processor.py +++ b/backend/services/frame_processor.py @@ -250,6 +250,9 @@ def get_or_create_node_pipeline( if pipeline is not None: return pipeline + # Canonical: every config in connected_nodes goes in through + # services.node_config.canonical_config, so a placed node has four float + # coordinates and two float altitudes, and no default is applied here. cfg = state.connected_nodes.get(node_id, {}).get("config", {}) if position_status(cfg) == "positioned": pipeline_cfg = { @@ -258,13 +261,10 @@ def get_or_create_node_pipeline( "FC": cfg.get("fc_hz", cfg.get("FC", 195_000_000)), "rx_lat": cfg["rx_lat"], "rx_lon": cfg["rx_lon"], - # Independently nullable: `.get(key, default)` would not apply the - # default to an explicit null, and a real altitude of 0.0 (sea - # level) rules out `or` as well. - "rx_alt_ft": 900 if cfg.get("rx_alt_ft") is None else cfg["rx_alt_ft"], + "rx_alt_ft": cfg["rx_alt_ft"], "tx_lat": cfg["tx_lat"], "tx_lon": cfg["tx_lon"], - "tx_alt_ft": 1200 if cfg.get("tx_alt_ft") is None else cfg["tx_alt_ft"], + "tx_alt_ft": cfg["tx_alt_ft"], "doppler_min": cfg.get("doppler_min", -300), "doppler_max": cfg.get("doppler_max", 300), "min_doppler": cfg.get("min_doppler", 15), diff --git a/backend/services/geo.py b/backend/services/geo.py index 460e8d51..da573d76 100644 --- a/backend/services/geo.py +++ b/backend/services/geo.py @@ -88,9 +88,14 @@ def node_beam_params(node_cfg: dict) -> dict: ``beam_azimuth_deg`` is None when the node declares no aim *and* has no TX to derive broadside from; callers should then skip the bearing test rather than invent a direction. + + The four coordinates are passed through as the caller's config holds them, + a float or None each; see services.node_config.canonical_config, which + every in-process config goes through. Callers wanting a placed node should + gate on ``position_status`` first. """ - rx_lat = float(node_cfg.get("rx_lat") or node_cfg.get("lat") or 0) - rx_lon = float(node_cfg.get("rx_lon") or node_cfg.get("lon") or 0) + rx_lat = node_cfg.get("rx_lat") + rx_lon = node_cfg.get("rx_lon") tx_lat = node_cfg.get("tx_lat") tx_lon = node_cfg.get("tx_lon") @@ -110,10 +115,13 @@ def node_beam_params(node_cfg: dict) -> dict: if explicit_az is not None and not math.isfinite(explicit_az): explicit_az = None + # A broadside aim needs both ends of the baseline. Truthiness here scored a + # transmitter on the equator or the prime meridian as no transmitter at + # all, and the node came out omnidirectional. if explicit_az is not None: beam_az = explicit_az - elif tx_lat and tx_lon: - beam_az = (bearing_deg(rx_lat, rx_lon, float(tx_lat), float(tx_lon)) + 90.0) % 360.0 + elif None not in (rx_lat, rx_lon, tx_lat, tx_lon): + beam_az = (bearing_deg(rx_lat, rx_lon, tx_lat, tx_lon) + 90.0) % 360.0 else: beam_az = None @@ -144,8 +152,8 @@ def node_beam_params(node_cfg: dict) -> dict: return { "rx_lat": rx_lat, "rx_lon": rx_lon, - "tx_lat": float(tx_lat) if tx_lat else None, - "tx_lon": float(tx_lon) if tx_lon else None, + "tx_lat": tx_lat, + "tx_lon": tx_lon, "beam_azimuth_deg": beam_az, "beam_width_deg": beam_width_deg, "max_range_km": max_range_km, diff --git a/backend/services/node_config.py b/backend/services/node_config.py index 3c4fb7a6..251b3e24 100644 --- a/backend/services/node_config.py +++ b/backend/services/node_config.py @@ -1,4 +1,5 @@ -"""The one configuration validator, shared by registration and PUT /nodes/config. +"""The one configuration validator, shared by registration and PUT /nodes/config, +and the one normaliser every in-process copy of a node's config passes through. Bounds are the wire contract's, at version 1.1.1. Three checks are here and not there because a JSON schema cannot express them: a receiver and illuminator at @@ -8,7 +9,7 @@ A leaf on purpose. It takes a dict and returns a dict, knowing nothing of identity, HTTP or status codes, so both callers can share it and it stays testable without a -database. +database. Nothing beyond the standard library may be imported here. """ import math @@ -58,17 +59,30 @@ def __init__(self, field: str, reason: str = "out of range") -> None: _REQUIRED = set(_NUMERIC_BOUNDS) | {"tx_callsign", "beam_width_deg", "beam_azimuth_deg"} -def _number(field: str, value: Any) -> float: +def _as_finite_float(value: Any) -> tuple[float | None, str]: + """The value as a finite float, with the reason when it cannot be one. + + Shared by the raising and non-raising doors below so the two cannot drift + on what counts as a real number, which is the property this whole module + turns on. The reason is what _number reports and _finite_float discards. + """ if isinstance(value, bool) or not isinstance(value, (int, float)): - raise ConfigInvalid(field, "not a number") + return None, "not a number" try: number = float(value) except OverflowError: # JSON puts no ceiling on integer literals, so a node can send an int with no # float representation. Rejected rather than left to raise out of the route. - raise ConfigInvalid(field, "out of range") from None + return None, "out of range" if not math.isfinite(number): - raise ConfigInvalid(field, "not a finite number") + return None, "not a finite number" + return number, "" + + +def _number(field: str, value: Any) -> float: + number, reason = _as_finite_float(value) + if number is None: + raise ConfigInvalid(field, reason) return number @@ -153,28 +167,82 @@ def validate_config(payload: dict[str, Any]) -> dict[str, Any]: return out -PositionStatus = Literal["positioned", "missing_rx", "missing_tx", "missing_both"] - +_COORDINATE_PAIRS = (("rx_lat", "rx_lon"), ("tx_lat", "tx_lon")) -def _is_num(v: Any) -> bool: - return isinstance(v, (int, float)) and not isinstance(v, bool) and math.isfinite(v) +# The flat spelling nodes predating rx_/tx_ still send. services.tcp_handler +# accepts it, so a node using it is placed and must read as placed. +_LEGACY_COORDINATES = (("rx_lat", "lat"), ("rx_lon", "lon")) +# Terrain figures, not measurements. pipeline.passive_radar and +# retina_geolocator.multinode_solver both multiply an altitude by a metre +# conversion the moment they are handed one, so neither may ever see a null. +_ALTITUDE_DEFAULT_FT = {"rx_alt_ft": 900.0, "tx_alt_ft": 1200.0} -def _is_placed(lat: Any, lon: Any) -> bool: - """A pair given as a real position: a usable number in both slots, and - not the (0, 0) sentinel. - The same rule lives in services.geo.valid_latlon and in - retina_analytics.constants.has_full_geometry. Not shared from either: this - module is a leaf that takes a dict and returns a dict, and must stay - importable with neither retina_analytics nor a database on the path. - valid_latlon pulls in retina_analytics.constants, which would break that. +def _finite_float(value: Any) -> float | None: + """The value as a float, or None when it cannot be one. - A config straight out of connected_nodes is unvalidated JSON rather than - validate_config's output, so lat/lon may be any type; _is_num keeps a - non-numeric value reading as not placed rather than raising. + The non-raising sibling of _number, for config dicts that never passed + through validate_config and may hold anything JSON can express. Unlike + _number it parses a numeric string first, which such a dict does carry. + """ + if isinstance(value, str): + try: + value = float(value) + except ValueError: + return None + return _as_finite_float(value)[0] + + +def canonical_config(raw: Any) -> dict[str, Any]: + """The one in-memory shape of a node's config, for every consumer to read. + + Returns a new dict, leaving ``raw`` untouched, in which: + + - ``rx_lat``, ``rx_lon``, ``tx_lat`` and ``tx_lon`` are each a float or + None, always present. None means the end is not placed, covering absent, + null, unusable, half a pair, and the exact (0, 0) sentinel that was the + only representable "unknown" while the columns were NOT NULL. A single + zero axis is a real coordinate and survives. + - ``rx_alt_ft`` and ``tx_alt_ft`` are floats, always present. + - the legacy flat ``lat``/``lon`` fold into ``rx_lat``/``rx_lon`` and are + gone from the result. + - every other key passes through unchanged. + + Never raises, for any input, including a non-dict. + + Called wherever a config enters shared in-process state, so downstream code + may read a coordinate as a number or a null and nothing else. The durable + ``node_configs`` row is not canonicalised: it keeps its honest nulls. """ - return _is_num(lat) and _is_num(lon) and not (lat == 0.0 and lon == 0.0) + if not isinstance(raw, dict): + return {} + config = dict(raw) + + # Keyed on absence, not falsiness: rx_lat present and explicitly null is a + # positionless registration, which a stray legacy lat must not overrule. + for field, legacy in _LEGACY_COORDINATES: + if field not in config and legacy in config: + config[field] = config[legacy] + config.pop("lat", None) + config.pop("lon", None) + + for lat_field, lon_field in _COORDINATE_PAIRS: + lat = _finite_float(config.get(lat_field)) + lon = _finite_float(config.get(lon_field)) + if lat is None or lon is None or (lat == 0.0 and lon == 0.0): + lat = lon = None + config[lat_field] = lat + config[lon_field] = lon + + for field, default in _ALTITUDE_DEFAULT_FT.items(): + altitude = _finite_float(config.get(field)) + config[field] = default if altitude is None else altitude + + return config + + +PositionStatus = Literal["positioned", "missing_rx", "missing_tx", "missing_both"] def position_status(config: dict[str, Any]) -> PositionStatus: @@ -183,9 +251,11 @@ def position_status(config: dict[str, Any]) -> PositionStatus: One value for consumers to branch on, rather than four fields each of them has to recombine. Keyed on latitude and longitude alone: a node with a position and no altitude is positioned. + + Reads a canonical_config, where an unplaced end is None on both axes. """ - has_rx = _is_placed(config.get("rx_lat"), config.get("rx_lon")) - has_tx = _is_placed(config.get("tx_lat"), config.get("tx_lon")) + has_rx = config.get("rx_lat") is not None and config.get("rx_lon") is not None + has_tx = config.get("tx_lat") is not None and config.get("tx_lon") is not None if has_rx and has_tx: return "positioned" if has_rx: diff --git a/backend/services/node_pipeline.py b/backend/services/node_pipeline.py index 578e9a66..5991b6e1 100644 --- a/backend/services/node_pipeline.py +++ b/backend/services/node_pipeline.py @@ -17,6 +17,7 @@ from core import state from core.nodes import Node, NodeConfig from services import node_registration +from services.node_config import canonical_config if TYPE_CHECKING: from routes.node_schemas import DetectionFrame @@ -98,9 +99,15 @@ def config_hash(config: dict) -> str: async def register_with_pipeline(session: AsyncSession, node: Node) -> None: config = await _pipeline_config(session, node.node_id) + # Hashed before canonicalisation, and it must stay that way: the TCP + # heartbeat compares a node's own hash against this one, and hashing the + # canonical form would report config drift across the whole fleet on the + # deploy that introduced it. + declared_hash = config_hash(config) + config = canonical_config(config) with state.connected_nodes_lock: state.connected_nodes[node.node_id] = { - "config_hash": config_hash(config), + "config_hash": declared_hash, "config": config, "status": "active", "last_heartbeat": "", diff --git a/backend/services/node_registration.py b/backend/services/node_registration.py index bbb602ee..19b1e4b2 100644 --- a/backend/services/node_registration.py +++ b/backend/services/node_registration.py @@ -14,13 +14,20 @@ from concurrent.futures import ThreadPoolExecutor from core import state +from services.node_config import canonical_config _registration_executor = ThreadPoolExecutor(max_workers=1, thread_name_prefix="node-reg") def register_node_blocking(node_id: str, config: dict) -> None: """Register with analytics and the associator. Callers on the event loop - want `register_node`, not this.""" + want `register_node`, not this. + + The single door into both library registries, so the config is + canonicalised here as well as at each call site: neither library defends + against a null altitude or a coordinate that is not a number. + """ + config = canonical_config(config) state.node_analytics.register_node(node_id, config) state.node_associator.register_node(node_id, config) diff --git a/backend/services/tasks/analytics_refresh.py b/backend/services/tasks/analytics_refresh.py index 6a868ea2..342ab6d5 100644 --- a/backend/services/tasks/analytics_refresh.py +++ b/backend/services/tasks/analytics_refresh.py @@ -1619,19 +1619,15 @@ def _min_truth_dist_km(kv: tuple) -> float: max_bistatic_deg: float | None = None for cid in r.get("contributing_node_ids", []): cfg = node_cfg_snap.get(cid, {}) - t_tx_lat = cfg.get("tx_lat") - t_tx_lon = cfg.get("tx_lon") - t_rx_lat = cfg.get("rx_lat") - t_rx_lon = cfg.get("rx_lon") - if not all((t_tx_lat, t_tx_lon, t_rx_lat, t_rx_lon)): + if position_status(cfg) != "positioned": continue ang = _bistatic_angle_deg( solver_lat, solver_lon, - float(t_tx_lat), - float(t_tx_lon), - float(t_rx_lat), - float(t_rx_lon), + cfg["tx_lat"], + cfg["tx_lon"], + cfg["rx_lat"], + cfg["rx_lon"], ) if max_bistatic_deg is None or ang > max_bistatic_deg: max_bistatic_deg = ang diff --git a/backend/services/tcp_handler.py b/backend/services/tcp_handler.py index a2375364..1f4efafd 100644 --- a/backend/services/tcp_handler.py +++ b/backend/services/tcp_handler.py @@ -19,6 +19,7 @@ from services.feed_helpers import adsb_capture_ts_ms, adsb_store from services.geo import valid_latlon from services.id_utils import normalize_hex_key +from services.node_config import canonical_config # Optional shared token for node authentication. If not set, any node can connect. _RADAR_NODE_TOKEN: str | None = os.getenv("RADAR_NODE_TOKEN") @@ -251,13 +252,18 @@ async def handle_tcp_client(reader: asyncio.StreamReader, writer: asyncio.Stream logging.warning("Radar TCP: rejected CONFIG from %s: %s", node_id, cfg_err) await _send_msg(writer, {"type": "CONFIG_NACK", "error": cfg_err}) continue + # config_hash stays the node's own, computed over what it + # sent: the heartbeat drift check compares against it. + canonical = canonical_config(config_payload) is_synth = msg.get("is_synthetic", is_synthetic_node(node_id)) _was_disconnected = state.connected_nodes.get(node_id, {}).get("status") == "disconnected" - _config_changed = state.connected_nodes.get(node_id, {}).get("config") != config_payload + # Both sides canonical, or every reconnect would look like a + # config change and evict the node's pipeline. + _config_changed = state.connected_nodes.get(node_id, {}).get("config") != canonical with state.connected_nodes_lock: state.connected_nodes[node_id] = { "config_hash": config_hash, - "config": config_payload, + "config": canonical, "status": "active", "last_heartbeat": datetime.now(timezone.utc).isoformat(), "peer": str(peer), @@ -308,7 +314,7 @@ async def handle_tcp_client(reader: asyncio.StreamReader, writer: asyncio.Stream "server_capabilities": SERVER_CAPABILITIES, }, ) - await node_registration.register_node(node_id, config_payload) + await node_registration.register_node(node_id, canonical) continue # ── REGISTER_KEY (chain of custody) ──────────────── diff --git a/backend/services/track_gates.py b/backend/services/track_gates.py index 71c0eec8..2e9731cd 100644 --- a/backend/services/track_gates.py +++ b/backend/services/track_gates.py @@ -36,6 +36,7 @@ offset_latlon_m, valid_latlon, ) +from services.node_config import position_status from services.public_location import fuzz_enabled, fuzz_node_cfg, public_point_delta @@ -110,12 +111,14 @@ def _build_single_node_arc( if delay_us is None or delay_us <= 0: return None - rx_lat = node_cfg.get("rx_lat") - rx_lon = node_cfg.get("rx_lon") - tx_lat = node_cfg.get("tx_lat") - tx_lon = node_cfg.get("tx_lon") - if None in (rx_lat, rx_lon, tx_lat, tx_lon): + # An arc has foci at both ends of the baseline, so a node missing either + # end draws nothing. + if position_status(node_cfg) != "positioned": return None + rx_lat = node_cfg["rx_lat"] + rx_lon = node_cfg["rx_lon"] + tx_lat = node_cfg["tx_lat"] + tx_lon = node_cfg["tx_lon"] # One source of truth for what the node can see — the same resolution # rules (broadside default, zero-width means missing, bistatic limit) diff --git a/backend/tests/node_helpers.py b/backend/tests/node_helpers.py new file mode 100644 index 00000000..3acd3681 --- /dev/null +++ b/backend/tests/node_helpers.py @@ -0,0 +1,35 @@ +"""Put a node into the in-process registries the way an entry point does. + +A test that hand-seeds only `state.connected_nodes`, or only the associator, +gets a node the frame path treats as half-registered: `get_or_create_node_pipeline` +returns None and every per-node branch downstream of it is skipped, so the test +passes without entering the code it names. +""" + +from core import state +from services import node_registration +from services.node_config import canonical_config +from services.tcp_handler import is_synthetic_node + + +def register_test_node(node_id: str, config: dict, **overrides) -> dict: + """Register `config` for `node_id`, and return the canonical form stored. + + The same two steps every writer of state.connected_nodes takes: store the + canonical config, then register that same config with analytics and the + associator. + """ + canonical = canonical_config(config) + with state.connected_nodes_lock: + state.connected_nodes[node_id] = { + "config_hash": "", + "config": canonical, + "status": "active", + "last_heartbeat": "", + "peer": "test", + "is_synthetic": is_synthetic_node(node_id), + "capabilities": {}, + **overrides, + } + node_registration.register_node_blocking(node_id, canonical) + return canonical diff --git a/backend/tests/test_adsb_seed_backend.py b/backend/tests/test_adsb_seed_backend.py index 80b6964a..b7759606 100644 --- a/backend/tests/test_adsb_seed_backend.py +++ b/backend/tests/test_adsb_seed_backend.py @@ -26,6 +26,7 @@ confirmed_track_views, process_one_frame, ) +from tests.node_helpers import register_test_node _NODE_CFG = { "rx_lat": 34.85, @@ -366,8 +367,7 @@ def test_adsb_inputs_reach_the_solver_queue(self, monkeypatch): # A positioned node: process_one_frame only reaches submit_tracks_round # (where the stub above is installed) for a node it can place. - with state.connected_nodes_lock: - state.connected_nodes["test-adsb-seed-queue"] = {"config": dict(_NODE_CFG)} + register_test_node("test-adsb-seed-queue", _NODE_CFG) default = PassiveRadarPipeline(DEFAULT_NODE_CONFIG) process_one_frame("test-adsb-seed-queue", _make_frame(), default) @@ -389,10 +389,14 @@ def _boom(node_id, frame): monkeypatch.setattr(fp, "claim_known_targets", _boom) before = state.known_claims_errors + # Placed, so the dark lane the frame is meant to continue down is + # actually there to continue down. + register_test_node("test-claim-fail-open", _NODE_CFG) default = PassiveRadarPipeline(DEFAULT_NODE_CONFIG) process_one_frame("test-claim-fail-open", _make_frame(), default) assert state.known_claims_errors == before + 1 + assert "test-claim-fail-open" in state.node_pipelines def test_empty_adsb_inputs_add_nothing(self, monkeypatch): monkeypatch.setattr( @@ -402,6 +406,8 @@ def test_empty_adsb_inputs_add_nothing(self, monkeypatch): ) monkeypatch.setattr(state, "solver_queue", queue.Queue()) + # Placed, so the round the stub above returns is really consulted. + register_test_node("test-adsb-seed-empty", _NODE_CFG) default = PassiveRadarPipeline(DEFAULT_NODE_CONFIG) process_one_frame("test-adsb-seed-empty", _make_frame(), default) diff --git a/backend/tests/test_frame_processor.py b/backend/tests/test_frame_processor.py index efb7592f..6fcc299f 100644 --- a/backend/tests/test_frame_processor.py +++ b/backend/tests/test_frame_processor.py @@ -29,9 +29,20 @@ process_one_frame, resolve_ground_truth_hex, ) +from tests.node_helpers import register_test_node # ── Helpers ─────────────────────────────────────────────────────────────────── +# A placed node, for the paths process_one_frame only enters for one. +_PLACED_CFG = { + "rx_lat": 34.0, + "rx_lon": -84.0, + "tx_lat": 33.8, + "tx_lon": -83.8, + "fc_hz": 195e6, + "max_range_km": 150.0, +} + def _make_frame(ts: int = None, n: int = 3) -> dict: if ts is None: @@ -204,14 +215,14 @@ def test_returns_none_for_a_node_with_no_usable_position(self): assert p is None def test_null_altitudes_default_rather_than_reach_the_geolocator_as_none(self): - """rx_alt_ft/tx_alt_ft are independently nullable; `cfg.get(key, default)` - does not apply the default when the key is present with value None, and - PassiveRadarPipeline._init_geolocator multiplies the altitude by - FT_TO_M unconditionally, so a null here must resolve before construction - rather than reach it.""" + """PassiveRadarPipeline._init_geolocator multiplies the altitude by + FT_TO_M unconditionally, so a null altitude must be resolved before it + gets here. Registration is what resolves it, so the node is registered + rather than written straight into connected_nodes.""" default = PassiveRadarPipeline(DEFAULT_NODE_CONFIG) - state.connected_nodes["test-null-altitude"] = { - "config": { + register_test_node( + "test-null-altitude", + { "rx_lat": 34.0, "rx_lon": -84.0, "rx_alt_ft": None, @@ -219,7 +230,7 @@ def test_null_altitudes_default_rather_than_reach_the_geolocator_as_none(self): "tx_lon": -83.8, "tx_alt_ft": None, }, - } + ) p = get_or_create_node_pipeline("test-null-altitude", default) assert p.config["rx_alt_ft"] == 900 assert p.config["tx_alt_ft"] == 1200 @@ -231,9 +242,13 @@ def test_null_altitudes_default_rather_than_reach_the_geolocator_as_none(self): class TestProcessOneFrame: def test_process_valid_frame(self): default = PassiveRadarPipeline(DEFAULT_NODE_CONFIG) + register_test_node("test-proc", _PLACED_CFG) frame = _make_frame() # Should not raise process_one_frame("test-proc", frame, default) + # The node's own pipeline saw the frame; an unregistered node would + # have had none and the whole per-node half would have been skipped. + assert state.node_pipelines["test-proc"].config["rx_lat"] == 34.0 def test_sets_aircraft_dirty_with_adsb(self): default = PassiveRadarPipeline(DEFAULT_NODE_CONFIG) @@ -296,14 +311,7 @@ def test_claiming_anchored_inputs_reach_the_solver_queue(self, monkeypatch): # A positioned node: process_one_frame only reaches submit_tracks_round # (where the stub above is installed) for a node it can place. - state.connected_nodes["test-anchor"] = { - "config": { - "rx_lat": 34.0, - "rx_lon": -84.0, - "tx_lat": 33.8, - "tx_lon": -83.8, - }, - } + register_test_node("test-anchor", _PLACED_CFG) default = PassiveRadarPipeline(DEFAULT_NODE_CONFIG) process_one_frame("test-anchor", _make_frame(), default) diff --git a/backend/tests/test_known_claiming.py b/backend/tests/test_known_claiming.py index b06f4eb0..d33d94d0 100644 --- a/backend/tests/test_known_claiming.py +++ b/backend/tests/test_known_claiming.py @@ -26,7 +26,8 @@ from core import state from pipeline.passive_radar import DEFAULT_NODE_CONFIG, PassiveRadarPipeline from services import known_claiming as kc -from services.frame_processor import process_one_frame +from services.frame_processor import get_or_create_node_pipeline, process_one_frame +from tests.node_helpers import register_test_node _NODE_CFG = { "rx_lat": 34.85, @@ -595,20 +596,18 @@ class TestModesInProcessOneFrame: that: the original frame (archive, ADS-B extraction) stays whole.""" def _run(self, monkeypatch, mode): - _register() + register_test_node(_NODE_ID, _NODE_CFG) monkeypatch.setattr(state, "KNOWN_LANE_MODE", mode) ts = int(time.time() * 1000) tag = {"hex": "bind01", "lat": _LAT, "lon": _LON, "alt_baro": _ALT_BARO_FT, "gs": 0, "track": 0} frame = _frame(ts, [50.0, 52.0], [10.0, 15.0], adsb=[tag, None]) default = PassiveRadarPipeline(DEFAULT_NODE_CONFIG) + # The node's own pipeline, built from its own geometry, is what + # process_one_frame hands the frame to; `default` never sees it. seen = [] - monkeypatch.setattr(default, "process_frame", lambda f: seen.append(f)) - # Pre-seed the pipeline cache: get_or_create_node_pipeline no longer - # falls back to `default` for a node it holds no connected_nodes - # config for (_register only registers it with the associator), so - # this stands in for the node's own, already-built pipeline. - state.node_pipelines[_NODE_ID] = default + node_pipeline = get_or_create_node_pipeline(_NODE_ID, default) + monkeypatch.setattr(node_pipeline, "process_frame", lambda f: seen.append(f)) process_one_frame(_NODE_ID, frame, default) return frame, seen[0] diff --git a/backend/tests/test_node_config_validation.py b/backend/tests/test_node_config_validation.py index 91ebc208..843b6001 100644 --- a/backend/tests/test_node_config_validation.py +++ b/backend/tests/test_node_config_validation.py @@ -2,7 +2,7 @@ import pytest -from services.node_config import ConfigInvalid, position_status, validate_config +from services.node_config import ConfigInvalid, canonical_config, position_status, validate_config VALID = { "rx_lat": 51.42, @@ -393,9 +393,12 @@ def test_null_coordinates_are_accepted(overrides, expected): def test_position_status_treats_the_zero_pair_as_absent(overrides, expected): """(0, 0) is the legacy broken-config sentinel, not a real position in the Gulf of Guinea, matching has_full_geometry in retina-analytics. A single - zero axis is still a real coordinate, so it must not read as absent.""" + zero axis is still a real coordinate, so it must not read as absent. + + validate_config keeps the pair as sent, because the durable row records + what the node claimed; canonical_config is what collapses it.""" out = validate_config(dict(VALID, **overrides)) - assert position_status(out) == expected + assert position_status(canonical_config(out)) == expected @pytest.mark.parametrize( @@ -455,7 +458,7 @@ def test_position_status_on_a_config_that_never_saw_validate_config(config): bulk-ingested one can carry a lone coordinate. A side with only one of its two coordinates places nothing, so all three of these read as missing_both.""" - assert position_status(config) == "missing_both" + assert position_status(canonical_config(config)) == "missing_both" @pytest.mark.parametrize("field", ["rx_lat", "rx_lon", "tx_lat", "tx_lon"]) @@ -470,11 +473,171 @@ def test_position_status_on_a_config_that_never_saw_validate_config(config): ], ) def test_a_non_numeric_coordinate_reads_as_not_placed_rather_than_raising(field, value): - """A connected_nodes config is unvalidated JSON, so a garbage value can sit - in any coordinate slot: float("") and float([]) both raise, and bool is a - subclass of int, so a naive isinstance(x, (int, float)) check would accept - True as a latitude. position_status must read past all of that as merely - unplaced, not raise.""" - config = {"rx_lat": 51.42, "rx_lon": -0.91, "tx_lat": 51.37, "tx_lon": -0.88, field: value} + """A config arriving over the wire is unvalidated JSON, so a garbage value + can sit in any coordinate slot: float("") and float([]) both raise, and + bool is a subclass of int, so a naive isinstance(x, (int, float)) check + would accept True as a latitude. canonical_config must read past all of + that as merely unplaced, not raise.""" + config = canonical_config({"rx_lat": 51.42, "rx_lon": -0.91, "tx_lat": 51.37, "tx_lon": -0.88, field: value}) side = "rx" if field.startswith("rx") else "tx" assert position_status(config) == f"missing_{side}" + + +# --- canonical_config ---------------------------------------------------------------- + + +def test_canonical_config_leaves_the_input_dict_alone(): + raw = {"rx_lat": "51.42", "rx_lon": "-0.91", "rx_alt_ft": None, "node_id": "n1"} + before = dict(raw) + canonical_config(raw) + assert raw == before + + +@pytest.mark.parametrize( + "raw", + [None, "config", 7, [], ("rx_lat", 1.0)], + ids=["none", "string", "int", "list", "tuple"], +) +def test_canonical_config_never_raises_on_a_non_dict(raw): + assert canonical_config(raw) == {} + + +def test_non_geometry_keys_pass_through_unchanged(): + raw = {"tx_callsign": "WSPA", "fc_hz": 195e6, "beam_azimuth_deg": None, "capabilities": {"adsb": True}} + out = canonical_config(raw) + for key, value in raw.items(): + assert out[key] == value + + +def test_the_legacy_flat_spelling_becomes_the_rx_pair(): + """services.tcp_handler accepts lat/lon, so a node sending it is placed and + must read as placed. The flat keys do not survive alongside the folded ones.""" + out = canonical_config({"lat": 51.42, "lon": -0.91}) + assert (out["rx_lat"], out["rx_lon"]) == (51.42, -0.91) + assert "lat" not in out and "lon" not in out + assert position_status(out) == "missing_tx" + + +def test_an_explicit_null_rx_is_not_overruled_by_a_stray_legacy_lat(): + """rx_lat present and null is a positionless registration, which the flat + spelling must not undo: the fold keys on the canonical key being absent.""" + out = canonical_config({"rx_lat": None, "rx_lon": None, "lat": 51.42, "lon": -0.91}) + assert out["rx_lat"] is None and out["rx_lon"] is None + assert position_status(out) == "missing_both" + + +@pytest.mark.parametrize( + "value,expected", + [ + pytest.param(51.42, 51.42, id="float"), + pytest.param(51, 51.0, id="int"), + pytest.param("51.42", 51.42, id="numeric-string"), + pytest.param("-0.91", -0.91, id="negative-numeric-string"), + pytest.param("abc", None, id="non-numeric-string"), + pytest.param("", None, id="empty-string"), + pytest.param(True, None, id="bool-true"), + pytest.param(False, None, id="bool-false"), + pytest.param(None, None, id="null"), + pytest.param([], None, id="list"), + pytest.param(float("nan"), None, id="nan"), + pytest.param(float("inf"), None, id="infinity"), + pytest.param(float("-inf"), None, id="negative-infinity"), + pytest.param(10**400, None, id="int-too-large-for-a-float"), + ], +) +def test_a_coordinate_becomes_a_float_or_none(value, expected): + """10**400 has no float representation: float() raises OverflowError on it + rather than returning inf, and JSON puts no ceiling on an integer literal.""" + out = canonical_config({"rx_lat": value, "rx_lon": value}) + if expected is None: + assert out["rx_lat"] is None and out["rx_lon"] is None + else: + assert out["rx_lat"] == expected and isinstance(out["rx_lat"], float) + + +@pytest.mark.parametrize("present", ["rx_lat", "rx_lon", "tx_lat", "tx_lon"]) +def test_half_a_pair_nulls_the_other_half(present): + out = canonical_config({present: 51.42}) + for field in ("rx_lat", "rx_lon", "tx_lat", "tx_lon"): + assert out[field] is None + assert position_status(out) == "missing_both" + + +def test_an_unusable_axis_nulls_its_partner(): + out = canonical_config({"rx_lat": float("nan"), "rx_lon": -0.91, "tx_lat": 51.37, "tx_lon": -0.88}) + assert out["rx_lat"] is None and out["rx_lon"] is None + assert (out["tx_lat"], out["tx_lon"]) == (51.37, -0.88) + assert position_status(out) == "missing_rx" + + +@pytest.mark.parametrize( + "overrides,expected", + [ + pytest.param({"rx_lat": 0.0, "rx_lon": 0.0}, "missing_rx", id="rx-null-island"), + pytest.param({"tx_lat": 0.0, "tx_lon": 0.0}, "missing_tx", id="tx-null-island"), + pytest.param({"rx_lat": 0.0}, "positioned", id="rx-on-the-equator"), + pytest.param({"rx_lon": 0.0}, "positioned", id="rx-on-the-prime-meridian"), + pytest.param({"tx_lat": 0.0}, "positioned", id="tx-on-the-equator"), + pytest.param({"tx_lon": 0.0}, "positioned", id="tx-on-the-prime-meridian"), + ], +) +def test_the_zero_pair_collapses_but_a_single_zero_axis_survives(overrides, expected): + """(0, 0) was the only representable "unknown" while the columns were + NOT NULL. The equator and the prime meridian are legitimate on their own.""" + placed = {"rx_lat": 51.42, "rx_lon": -0.91, "tx_lat": 51.37, "tx_lon": -0.88} + out = canonical_config(dict(placed, **overrides)) + assert position_status(out) == expected + if expected == "positioned": + for field, value in overrides.items(): + assert out[field] == value + + +@pytest.mark.parametrize( + "value", + [ + pytest.param(None, id="null"), + pytest.param("abc", id="non-numeric-string"), + pytest.param(float("nan"), id="nan"), + pytest.param(True, id="bool"), + pytest.param(10**400, id="int-too-large-for-a-float"), + ], +) +def test_an_unusable_altitude_becomes_the_default(value): + out = canonical_config({"rx_alt_ft": value, "tx_alt_ft": value}) + assert out["rx_alt_ft"] == 900.0 + assert out["tx_alt_ft"] == 1200.0 + + +def test_an_absent_altitude_becomes_the_default(): + out = canonical_config({}) + assert out["rx_alt_ft"] == 900.0 + assert out["tx_alt_ft"] == 1200.0 + + +@pytest.mark.parametrize( + "value,expected", + [ + pytest.param(0.0, 0.0, id="sea-level"), + pytest.param(-40, -40.0, id="below-sea-level"), + pytest.param("1250", 1250.0, id="numeric-string"), + ], +) +def test_a_usable_altitude_is_kept_as_a_float(value, expected): + """Zero is a real altitude, so a truthiness fallback cannot resolve these.""" + out = canonical_config({"rx_alt_ft": value, "tx_alt_ft": value}) + assert out["rx_alt_ft"] == expected and isinstance(out["rx_alt_ft"], float) + assert out["tx_alt_ft"] == expected and isinstance(out["tx_alt_ft"], float) + + +def test_a_validated_config_survives_canonicalisation_unchanged(): + """The ordinary case: nothing about a well-formed config moves.""" + out = canonical_config(validate_config(dict(VALID))) + for field in ("rx_lat", "rx_lon", "tx_lat", "tx_lon", "rx_alt_ft", "tx_alt_ft"): + assert out[field] == float(VALID[field]) + assert position_status(out) == "positioned" + + +def test_canonicalising_twice_changes_nothing(): + raw = {"lat": "51.42", "lon": "-0.91", "tx_lat": 0.0, "tx_lon": 0.0, "rx_alt_ft": None} + once = canonical_config(raw) + assert canonical_config(once) == once diff --git a/backend/tests/test_node_pipeline.py b/backend/tests/test_node_pipeline.py index cf2f11bc..507c7b66 100644 --- a/backend/tests/test_node_pipeline.py +++ b/backend/tests/test_node_pipeline.py @@ -10,6 +10,8 @@ """ import asyncio +import hashlib +import json import logging from datetime import UTC, datetime @@ -24,7 +26,9 @@ from core.nodes import Node, NodeConfig from pipeline.passive_radar import DEFAULT_NODE_CONFIG, PassiveRadarPipeline from services.frame_processor import get_or_create_node_pipeline +from services.node_config import position_status from services.node_pipeline import ( + _pipeline_config, prime_pipeline, prime_pipeline_at_startup, register_with_pipeline, @@ -109,6 +113,48 @@ async def test_the_pipeline_config_carries_the_defaults_blah2_bridge_supplies(no assert config["min_doppler"] == 15 +async def test_a_row_with_no_position_registers_unplaced_with_resolved_altitudes(node_session): + """The row keeps its honest nulls; the in-memory copy does not. + + A null altitude reaches PassiveRadarPipeline and the multinode solver as a + multiplicand, so registration resolves it. A null coordinate stays null and + the node is carried unplaced.""" + unplaced = await _seed( + node_session, + NODE_ID, + rx_lat=None, + rx_lon=None, + rx_alt_ft=None, + tx_lat=None, + tx_lon=None, + tx_alt_ft=None, + ) + + await register_with_pipeline(node_session, unplaced) + + config = state.connected_nodes[NODE_ID]["config"] + assert position_status(config) == "missing_both" + assert config["rx_alt_ft"] == 900.0 + assert config["tx_alt_ft"] == 1200.0 + assert get_or_create_node_pipeline(NODE_ID, PassiveRadarPipeline(DEFAULT_NODE_CONFIG)) is None + + +async def test_the_config_hash_is_computed_before_canonicalisation(node_session): + """The TCP heartbeat compares a node's own hash against the stored one, so + canonicalisation must not move it: the whole fleet would report config drift + on the deploy that introduced it.""" + node = await _seed(node_session, NODE_ID, rx_alt_ft=None, tx_alt_ft=None) + row_config = await _pipeline_config(node_session, NODE_ID) + expected = hashlib.sha256(json.dumps(row_config, sort_keys=True).encode()).hexdigest()[:16] + + await register_with_pipeline(node_session, node) + + entry = state.connected_nodes[NODE_ID] + assert entry["config_hash"] == expected + stored = hashlib.sha256(json.dumps(entry["config"], sort_keys=True).encode()).hexdigest()[:16] + assert stored != expected, "the two forms must differ here, or this pins nothing" + + async def test_an_aimed_node_keeps_the_azimuth_it_was_configured_with(node_session): aimed = await _seed(node_session, NODE_ID, beam_azimuth_deg=200.0) diff --git a/backend/tests/test_storage.py b/backend/tests/test_storage.py index 0be98866..55320750 100644 --- a/backend/tests/test_storage.py +++ b/backend/tests/test_storage.py @@ -2,6 +2,7 @@ import pytest +from services.node_config import canonical_config from services.storage import archive_detections, list_archived_files, read_archived_file @@ -32,6 +33,35 @@ def test_archive_returns_key(self): assert isinstance(key, str) and "/" in key assert "test-storage-node" in key + def test_a_legacy_spelled_node_archives_its_real_position(self): + """The archive snapshots the canonical config, so the legacy flat + lat/lon a node may still send is folded before it is written. + + Archive rows are permanent and not correctable once published, and a + null here is indistinguishable from a node that genuinely declared no + position. Reading an un-normalised config instead wrote nulls for a + fully placed node, which is unrecoverable after the fact.""" + from core import state + + state.connected_nodes["test-legacy-node"] = { + "config": canonical_config({"lat": 51.5, "lon": -0.12, "tx_lat": 51.6, "tx_lon": -0.2}), + "status": "active", + } + try: + archive_detections( + "test-legacy-node", + [{"delay": [10.0], "doppler": [50.0], "snr": [12.0], "timestamp": 1000}], + ) + result = list_archived_files(node_id="test-legacy-node") + data = read_archived_file(result["files"][0]["key"]) + finally: + state.connected_nodes.pop("test-legacy-node", None) + + row = data["detections"][0] + assert row["rx_lat"] is not None and row["rx_lon"] is not None + # Fuzzed or not, the published receiver stays within a few km of truth. + assert abs(row["rx_lat"] - 51.5) < 0.1 + def test_list_finds_archived(self): archive_detections( "test-storage-node", diff --git a/backend/tests/test_tcp_handler.py b/backend/tests/test_tcp_handler.py index e29c913b..e5c81230 100644 --- a/backend/tests/test_tcp_handler.py +++ b/backend/tests/test_tcp_handler.py @@ -11,6 +11,7 @@ import pytest from core import state +from services.node_config import position_status from services.tcp_handler import ( _apply_synthetic_adsb, _enqueue_detection, @@ -37,14 +38,15 @@ def _make_hello(node_id: str = "test-node-1", is_synthetic: bool = False) -> byt ) -def _make_config(node_id: str = "test-node-1", is_synthetic: bool = False) -> bytes: +def _make_config(node_id: str = "test-node-1", is_synthetic: bool = False, config: dict | None = None) -> bytes: return _msg( { "type": "CONFIG", "node_id": node_id, "config_hash": "abc123", "is_synthetic": is_synthetic, - "config": { + "config": config + or { "node_id": node_id, "rx_lat": 33.94, "rx_lon": -84.65, @@ -160,6 +162,31 @@ def test_hello_config_registers_node(self): assert node["config_hash"] == "abc123" assert node["status"] == "disconnected" # set in finally block after EOF + def test_the_stored_config_is_canonical(self): + """_validate_node_config accepts the legacy flat spelling, so a node + sending it is placed and must read as placed everywhere downstream. The + handler stores the canonical form: folded, coerced, altitudes resolved.""" + reader = MockStreamReader( + [ + _make_hello("test-node-1"), + _make_config( + "test-node-1", + config={"node_id": "test-node-1", "lat": "33.94", "lon": "-84.65", "tx_lat": 0.0, "tx_lon": 0.0}, + ), + b"", + ] + ) + writer = MockStreamWriter() + + asyncio.run(handle_tcp_client(reader, writer)) + + config = state.connected_nodes["test-node-1"]["config"] + assert (config["rx_lat"], config["rx_lon"]) == (33.94, -84.65) + assert "lat" not in config and "lon" not in config + assert config["tx_lat"] is None and config["tx_lon"] is None + assert config["rx_alt_ft"] == 900.0 and config["tx_alt_ft"] == 1200.0 + assert position_status(config) == "missing_tx" + def test_config_ack_sent(self): """Server replies with CONFIG_ACK after receiving CONFIG.""" reader = MockStreamReader( From ae4860d1cfd5943dbfacb27df43f5f1f1eb4b381 Mon Sep 17 00:00:00 2001 From: Babissimo Date: Mon, 7 Sep 2026 10:54:26 +0100 Subject: [PATCH 09/10] Default a null altitude at the geometry boundary, not on the way in MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- backend/services/blah2_bridge.py | 17 ++++++- backend/services/frame_processor.py | 14 ++++-- backend/services/node_config.py | 50 +++++++++++++++++--- backend/services/node_registration.py | 10 +++- backend/services/tasks/analytics_refresh.py | 4 +- backend/tests/test_blah2_bridge.py | 14 ++++++ backend/tests/test_frame_processor.py | 35 ++++++++++++++ backend/tests/test_node_config_validation.py | 16 ++++--- backend/tests/test_node_pipeline.py | 23 +++++---- backend/tests/test_tcp_handler.py | 8 ++-- 10 files changed, 156 insertions(+), 35 deletions(-) diff --git a/backend/services/blah2_bridge.py b/backend/services/blah2_bridge.py index e854998c..361a0754 100644 --- a/backend/services/blah2_bridge.py +++ b/backend/services/blah2_bridge.py @@ -60,10 +60,14 @@ # Fields every node must supply — without these the bistatic solve is undefined. _REQUIRED = ("node_id", "detection_url", "rx_lat", "rx_lon", "tx_lat", "tx_lon", "fc_hz") +# Altitude is deliberately absent from the defaults below: it is resolved at +# the geometry boundary (node_config.resolve_altitudes) instead, so a node that +# declares none archives and publishes a null rather than a figure nothing +# downstream could later tell apart from a survey. +_OPTIONAL_ALTITUDES = ("rx_alt_ft", "tx_alt_ft") + # Optional fields and the defaults applied when a node omits them. _OPTIONAL_DEFAULTS = { - "rx_alt_ft": 0.0, - "tx_alt_ft": 0.0, "fs_hz": 2_000_000, "doppler_min": -300, "doppler_max": 300, @@ -145,6 +149,15 @@ def _build_node(entry: dict) -> Blah2Node: cfg[key] = float(raw) except (TypeError, ValueError) as exc: raise Blah2ConfigError(f"{node_id}: {key} is not a number: {raw!r}") from exc + for key in _OPTIONAL_ALTITUDES: + raw = entry.get(key) + if raw is None: + cfg[key] = None + continue + try: + cfg[key] = float(raw) + except (TypeError, ValueError) as exc: + raise Blah2ConfigError(f"{node_id}: {key} is not a number: {raw!r}") from exc for key, lo, hi in (("rx_lat", -90, 90), ("tx_lat", -90, 90), ("rx_lon", -180, 180), ("tx_lon", -180, 180)): if not lo <= cfg[key] <= hi: diff --git a/backend/services/frame_processor.py b/backend/services/frame_processor.py index 4e5ecf39..da725add 100644 --- a/backend/services/frame_processor.py +++ b/backend/services/frame_processor.py @@ -28,7 +28,7 @@ ) from services.id_utils import normalize_hex_key as _normalize_hex_key from services.known_claiming import claim_known_targets, strip_claimed_detections -from services.node_config import position_status +from services.node_config import position_status, resolve_altitudes from services.storage import archive_detections # ── Archive batching ────────────────────────────────────────────────────────── @@ -195,13 +195,18 @@ def _reset_for_tests() -> None: def get_node_configs() -> dict[str, dict]: + """Every connected node's config, altitudes resolved. + + The solver's snapshot: retina_geolocator multiplies an altitude by a metre + conversion as soon as it is handed one, so a null must not reach it. + """ configs = {} with state.connected_nodes_lock: snapshot = list(state.connected_nodes.items()) for nid, info in snapshot: cfg = info.get("config") if cfg: - configs[nid] = cfg + configs[nid] = resolve_altitudes(cfg) return configs @@ -252,8 +257,9 @@ def get_or_create_node_pipeline( # Canonical: every config in connected_nodes goes in through # services.node_config.canonical_config, so a placed node has four float - # coordinates and two float altitudes, and no default is applied here. - cfg = state.connected_nodes.get(node_id, {}).get("config", {}) + # coordinates. Altitude is resolved here, at the boundary, because + # passive_radar subscripts it and converts it to metres. + cfg = resolve_altitudes(state.connected_nodes.get(node_id, {}).get("config", {})) if position_status(cfg) == "positioned": pipeline_cfg = { "node_id": node_id, diff --git a/backend/services/node_config.py b/backend/services/node_config.py index 251b3e24..2147245d 100644 --- a/backend/services/node_config.py +++ b/backend/services/node_config.py @@ -174,9 +174,39 @@ def validate_config(payload: dict[str, Any]) -> dict[str, Any]: _LEGACY_COORDINATES = (("rx_lat", "lat"), ("rx_lon", "lon")) # Terrain figures, not measurements. pipeline.passive_radar and -# retina_geolocator.multinode_solver both multiply an altitude by a metre -# conversion the moment they are handed one, so neither may ever see a null. -_ALTITUDE_DEFAULT_FT = {"rx_alt_ft": 900.0, "tx_alt_ft": 1200.0} +# retina_geolocator.multinode_solver each multiply an altitude by a metre +# conversion the moment they are handed one, so neither may ever see a null; +# retina_analytics.association takes one too, spelled `or 0`, which survives a +# null by silently reading it as sea level. +# +# Applied at the geometry boundary, never on the way in: a config that reaches +# publication or the Parquet archive must carry the altitude the node declared, +# nulls included, because nothing downstream could later tell a working figure +# apart from a survey and archive rows are not correctable once published. +# resolve_altitudes below is the only caller. +ALTITUDE_DEFAULT_FT = {"rx_alt_ft": 900.0, "tx_alt_ft": 1200.0} + + +def resolve_altitudes(cfg: dict) -> dict: + """``cfg`` with a null altitude replaced by its terrain default. + + The geometry boundary, and the counterpart to canonical_config: that keeps + a declared null null, because publication and the archive must not carry an + invented figure, and this resolves it for the geodesy, which cannot take + one. Apply it at every door into geometry and nowhere earlier, so that one + missing altitude cannot become 900 ft in one subsystem and 0 ft in another. + + Keyed on None, not falsiness: a receiver at 0 ft is at sea level, not + unsurveyed, and ``or`` would silently lift it to 900. + + Copies, so resolving cannot write the working figure back into the dict + that publication and the archive read. + """ + resolved = dict(cfg) + for field, default in ALTITUDE_DEFAULT_FT.items(): + if resolved.get(field) is None: + resolved[field] = default + return resolved def _finite_float(value: Any) -> float | None: @@ -204,13 +234,20 @@ def canonical_config(raw: Any) -> dict[str, Any]: null, unusable, half a pair, and the exact (0, 0) sentinel that was the only representable "unknown" while the columns were NOT NULL. A single zero axis is a real coordinate and survives. - - ``rx_alt_ft`` and ``tx_alt_ft`` are floats, always present. + - ``rx_alt_ft`` and ``tx_alt_ft`` are each a float or None, always present. + None is the honest answer and is left standing here; geometry resolves it + through ``resolve_altitudes`` at the three doors that cannot take a null. - the legacy flat ``lat``/``lon`` fold into ``rx_lat``/``rx_lon`` and are gone from the result. - every other key passes through unchanged. Never raises, for any input, including a non-dict. + Corrects, never invents. Every transformation above turns an unusable value + into the null it already meant, or a coordinate into the number it already + was, so the result is safe to publish and to archive as well as to solve on + — which is why there is one shape here and not a canonical/declared pair. + Called wherever a config enters shared in-process state, so downstream code may read a coordinate as a number or a null and nothing else. The durable ``node_configs`` row is not canonicalised: it keeps its honest nulls. @@ -235,9 +272,8 @@ def canonical_config(raw: Any) -> dict[str, Any]: config[lat_field] = lat config[lon_field] = lon - for field, default in _ALTITUDE_DEFAULT_FT.items(): - altitude = _finite_float(config.get(field)) - config[field] = default if altitude is None else altitude + for field in ALTITUDE_DEFAULT_FT: + config[field] = _finite_float(config.get(field)) return config diff --git a/backend/services/node_registration.py b/backend/services/node_registration.py index 19b1e4b2..78b24fa0 100644 --- a/backend/services/node_registration.py +++ b/backend/services/node_registration.py @@ -14,7 +14,7 @@ from concurrent.futures import ThreadPoolExecutor from core import state -from services.node_config import canonical_config +from services.node_config import canonical_config, resolve_altitudes _registration_executor = ThreadPoolExecutor(max_workers=1, thread_name_prefix="node-reg") @@ -26,8 +26,14 @@ def register_node_blocking(node_id: str, config: dict) -> None: The single door into both library registries, so the config is canonicalised here as well as at each call site: neither library defends against a null altitude or a coordinate that is not a number. + + A geometry door, so the altitude is resolved too. The associator reads it + as ``(config.get("rx_alt_ft") or 0) * 0.3048``, which quietly places an + unsurveyed receiver at sea level while the pipeline and the solver put the + same node at 900 ft. Neither registry publishes an altitude, so resolving + here cannot leak a working figure into a payload. """ - config = canonical_config(config) + config = resolve_altitudes(canonical_config(config)) state.node_analytics.register_node(node_id, config) state.node_associator.register_node(node_id, config) diff --git a/backend/services/tasks/analytics_refresh.py b/backend/services/tasks/analytics_refresh.py index 342ab6d5..c8af4517 100644 --- a/backend/services/tasks/analytics_refresh.py +++ b/backend/services/tasks/analytics_refresh.py @@ -621,8 +621,8 @@ def _refresh_missed_detections(nodes_snapshot: list): # Resolved the same way every module resolves it: explicit aim, else # broadside off the RX→TX baseline (Yagi sits perpendicular to it), # else omnidirectional; width falls back to the shared YAGI default. - # tx_lat/tx_lon are already known truthy from the `all(...)` check - # above, so beam_azimuth can't come back None here. + # The position_status gate above admits only a node with both ends + # placed, so beam_azimuth cannot come back None here. params = node_beam_params(cfg) beam_width = params["beam_width_deg"] max_range = params["max_range_km"] diff --git a/backend/tests/test_blah2_bridge.py b/backend/tests/test_blah2_bridge.py index 0ff5c83e..22d05aa0 100644 --- a/backend/tests/test_blah2_bridge.py +++ b/backend/tests/test_blah2_bridge.py @@ -314,3 +314,17 @@ async def test_older_frame_dropped(self, monkeypatch): async def test_newer_frame_after_older_still_passes(self, monkeypatch): """An out-of-order frame must not wedge the node against later good ones.""" assert await self._enqueued_timestamps(monkeypatch, [1000, 500, 2000]) == [1000, 2000] + + +def test_an_omitted_altitude_stays_null(): + """No invented altitude on the way in. + + resolve_altitudes supplies the terrain figure at the geometry boundary, so + a node that declares none must reach publication and the Parquet archive + with a null: those rows are permanent, and a fabricated figure there cannot + afterwards be told apart from a survey. + """ + cfg = _build_node(MINIMAL).config + + assert cfg["rx_alt_ft"] is None + assert cfg["tx_alt_ft"] is None diff --git a/backend/tests/test_frame_processor.py b/backend/tests/test_frame_processor.py index 6fcc299f..4d1c6541 100644 --- a/backend/tests/test_frame_processor.py +++ b/backend/tests/test_frame_processor.py @@ -27,6 +27,7 @@ normalize_hex_key, position_distance_km, process_one_frame, + resolve_altitudes, resolve_ground_truth_hex, ) from tests.node_helpers import register_test_node @@ -169,6 +170,40 @@ def test_skips_missing_config(self): configs = get_node_configs() assert "test-cfg-2" not in configs + def test_resolves_a_null_altitude(self): + """The solver's snapshot feeds retina_geolocator, which multiplies an + altitude by a metre conversion the moment it is handed one.""" + state.connected_nodes["test-cfg-3"] = { + "config": {"rx_lat": 33.9, "rx_lon": -84.6, "rx_alt_ft": None, "tx_alt_ft": None}, + "status": "active", + } + configs = get_node_configs() + assert configs["test-cfg-3"]["rx_alt_ft"] == 900.0 + assert configs["test-cfg-3"]["tx_alt_ft"] == 1200.0 + + def test_leaves_the_stored_config_alone(self): + """resolve_altitudes copies. Defaulting in place would write the + working figure back into the dict publication and the archive read.""" + stored = {"rx_lat": 33.9, "rx_lon": -84.6, "rx_alt_ft": None} + state.connected_nodes["test-cfg-4"] = {"config": stored, "status": "active"} + get_node_configs() + assert stored["rx_alt_ft"] is None + + +class TestResolveAltitudes: + def test_sea_level_survives(self): + """0 ft is a real altitude, not an absent one: a truthiness fallback + would silently lift every sea-level receiver to 900 ft.""" + assert resolve_altitudes({"rx_alt_ft": 0.0})["rx_alt_ft"] == 0.0 + + def test_a_null_takes_the_terrain_default(self): + resolved = resolve_altitudes({"rx_alt_ft": None, "tx_alt_ft": None}) + assert resolved["rx_alt_ft"] == 900.0 + assert resolved["tx_alt_ft"] == 1200.0 + + def test_an_absent_altitude_takes_the_terrain_default(self): + assert resolve_altitudes({})["rx_alt_ft"] == 900.0 + # ── Pipeline factory ───────────────────────────────────────────────────────── diff --git a/backend/tests/test_node_config_validation.py b/backend/tests/test_node_config_validation.py index 843b6001..77b14e23 100644 --- a/backend/tests/test_node_config_validation.py +++ b/backend/tests/test_node_config_validation.py @@ -602,16 +602,20 @@ def test_the_zero_pair_collapses_but_a_single_zero_axis_survives(overrides, expe pytest.param(10**400, id="int-too-large-for-a-float"), ], ) -def test_an_unusable_altitude_becomes_the_default(value): +def test_an_unusable_altitude_becomes_null(value): + """Null, not the terrain default. These configs reach publication and the + Parquet archive, where an invented 900 ft is indistinguishable from a + survey and the rows cannot be corrected once published. Geometry resolves + the null at its own boundary: see node_config.resolve_altitudes.""" out = canonical_config({"rx_alt_ft": value, "tx_alt_ft": value}) - assert out["rx_alt_ft"] == 900.0 - assert out["tx_alt_ft"] == 1200.0 + assert out["rx_alt_ft"] is None + assert out["tx_alt_ft"] is None -def test_an_absent_altitude_becomes_the_default(): +def test_an_absent_altitude_becomes_null(): out = canonical_config({}) - assert out["rx_alt_ft"] == 900.0 - assert out["tx_alt_ft"] == 1200.0 + assert out["rx_alt_ft"] is None + assert out["tx_alt_ft"] is None @pytest.mark.parametrize( diff --git a/backend/tests/test_node_pipeline.py b/backend/tests/test_node_pipeline.py index 507c7b66..d417bda0 100644 --- a/backend/tests/test_node_pipeline.py +++ b/backend/tests/test_node_pipeline.py @@ -113,12 +113,14 @@ async def test_the_pipeline_config_carries_the_defaults_blah2_bridge_supplies(no assert config["min_doppler"] == 15 -async def test_a_row_with_no_position_registers_unplaced_with_resolved_altitudes(node_session): - """The row keeps its honest nulls; the in-memory copy does not. - - A null altitude reaches PassiveRadarPipeline and the multinode solver as a - multiplicand, so registration resolves it. A null coordinate stays null and - the node is carried unplaced.""" +async def test_a_row_with_no_position_registers_unplaced_and_keeps_its_nulls(node_session): + """The in-memory copy keeps the row's honest nulls, altitude included. + + Registration resolves nothing: this config is what /api/radar/nodes + publishes and what the Parquet archive snapshots, and a terrain default + written here would be indistinguishable downstream from a survey. Geometry + resolves the altitude at its own boundary instead, and a node with no + coordinates builds no pipeline at all.""" unplaced = await _seed( node_session, NODE_ID, @@ -134,8 +136,8 @@ async def test_a_row_with_no_position_registers_unplaced_with_resolved_altitudes config = state.connected_nodes[NODE_ID]["config"] assert position_status(config) == "missing_both" - assert config["rx_alt_ft"] == 900.0 - assert config["tx_alt_ft"] == 1200.0 + assert config["rx_alt_ft"] is None + assert config["tx_alt_ft"] is None assert get_or_create_node_pipeline(NODE_ID, PassiveRadarPipeline(DEFAULT_NODE_CONFIG)) is None @@ -143,7 +145,10 @@ async def test_the_config_hash_is_computed_before_canonicalisation(node_session) """The TCP heartbeat compares a node's own hash against the stored one, so canonicalisation must not move it: the whole fleet would report config drift on the deploy that introduced it.""" - node = await _seed(node_session, NODE_ID, rx_alt_ft=None, tx_alt_ft=None) + # The (0, 0) sentinel, which canonicalisation collapses to a null pair. A + # null altitude no longer serves here: it is left null on both sides now, + # so the two hashes would agree and the assertion below would pin nothing. + node = await _seed(node_session, NODE_ID, rx_lat=0.0, rx_lon=0.0) row_config = await _pipeline_config(node_session, NODE_ID) expected = hashlib.sha256(json.dumps(row_config, sort_keys=True).encode()).hexdigest()[:16] diff --git a/backend/tests/test_tcp_handler.py b/backend/tests/test_tcp_handler.py index e5c81230..26e182ae 100644 --- a/backend/tests/test_tcp_handler.py +++ b/backend/tests/test_tcp_handler.py @@ -164,8 +164,10 @@ def test_hello_config_registers_node(self): def test_the_stored_config_is_canonical(self): """_validate_node_config accepts the legacy flat spelling, so a node - sending it is placed and must read as placed everywhere downstream. The - handler stores the canonical form: folded, coerced, altitudes resolved.""" + sending it is placed and must read as placed everywhere downstream — + the archive included, which is a permanent record. The handler stores + the canonical form: folded and coerced, with the declared altitude left + alone for geometry to resolve at its own boundary.""" reader = MockStreamReader( [ _make_hello("test-node-1"), @@ -184,7 +186,7 @@ def test_the_stored_config_is_canonical(self): assert (config["rx_lat"], config["rx_lon"]) == (33.94, -84.65) assert "lat" not in config and "lon" not in config assert config["tx_lat"] is None and config["tx_lon"] is None - assert config["rx_alt_ft"] == 900.0 and config["tx_alt_ft"] == 1200.0 + assert config["rx_alt_ft"] is None and config["tx_alt_ft"] is None assert position_status(config) == "missing_tx" def test_config_ack_sent(self): From f0d5b10329eeb586b0bf9d97d2d5e5c7e3ad45f9 Mon Sep 17 00:00:00 2001 From: Babissimo Date: Mon, 7 Sep 2026 10:54:26 +0100 Subject: [PATCH 10/10] Gate an unplaced node out of the solver's snapshot 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 --- backend/services/frame_processor.py | 62 ++++++++++++++++++++------- backend/services/tasks/known_lane.py | 9 ++-- backend/services/tasks/solver.py | 10 +++++ backend/tests/test_frame_processor.py | 26 +++++++++++ backend/tests/test_solver_trimming.py | 32 ++++++++++++++ 5 files changed, 120 insertions(+), 19 deletions(-) diff --git a/backend/services/frame_processor.py b/backend/services/frame_processor.py index da725add..00ff0ebe 100644 --- a/backend/services/frame_processor.py +++ b/backend/services/frame_processor.py @@ -194,18 +194,44 @@ def _reset_for_tests() -> None: # ── Node configs helper ────────────────────────────────────────────────────── -def get_node_configs() -> dict[str, dict]: - """Every connected node's config, altitudes resolved. +def _solver_input_node_ids(s_in: dict) -> set[str]: + """The node ids a solver input can reach. - The solver's snapshot: retina_geolocator multiplies an altitude by a metre - conversion as soon as it is handed one, so a null must not reach it. + Its measurements, plus the pool's spare ones: _adopt_pool_nodes re-solves + with those once the first solve vouches for them, so a config missing for + one is silently dropped by the epoch alignment and the solver's NodeSetups. + """ + return {m.get("node_id") for m in (s_in.get("measurements") or ())} | { + m.get("node_id") for m in (s_in.get("pool_measurements") or ()) + } + + +def get_node_configs(wanted: set[str] | None = None) -> dict[str, dict]: + """Every *placed* connected node's config, altitudes resolved. + + The solver's snapshot, and the placement gate for everything drawn from + it: an unplaced node has no geometry to solve against, so membership here + means the node can be solved with and a consumer needs no check of its + own. Gated here rather than at each of them because the snapshot is the + one place that knows, and a consumer testing `nid in node_cfgs` reads as + though it already had (see known_lane's dark-follow claim filter). + + Altitudes are resolved because retina_geolocator multiplies one by a metre + conversion as soon as it is handed it, so a null must not reach it. That + is a copy per node, so `wanted` narrows it to the ids a caller can use; + the default is the whole placed fleet. """ configs = {} with state.connected_nodes_lock: snapshot = list(state.connected_nodes.items()) for nid, info in snapshot: + if wanted is not None and nid not in wanted: + continue cfg = info.get("config") - if cfg: + # missing_tx is placed enough: the range circle and the bearing wedge + # are both about the receiver, and the bistatic paths test the + # transmitter separately. + if cfg and position_status(cfg) in ("positioned", "missing_tx"): configs[nid] = resolve_altitudes(cfg) return configs @@ -227,14 +253,11 @@ def configs_for_solver_input(node_cfgs: dict[str, dict], s_in: dict) -> dict[str is unaffected — it fetches its own configs (known_lane.run_known_lane_pass) rather than reusing what was queued here. """ - wanted = {m.get("node_id") for m in (s_in.get("measurements") or ())} # The pool's spare measurements are the one thing downstream that CAN - # widen the set: solver._adopt_pool_nodes re-solves with them once the - # first solve vouches for them, and a node without a config there is - # silently dropped by the epoch alignment and the solver's NodeSetups — - # measured live, that left 120 of 309 "widened" candidates solving on - # their original two nodes. A pool is 0-6 extra configs, not 50. - wanted |= {m.get("node_id") for m in (s_in.get("pool_measurements") or ())} + # widen the set, which is why _solver_input_node_ids counts them: measured + # live, omitting them left 120 of 309 "widened" candidates solving on their + # original two nodes. A pool is 0-6 extra configs, not 50. + wanted = _solver_input_node_ids(s_in) return {nid: cfg for nid, cfg in node_cfgs.items() if nid in wanted} @@ -257,10 +280,15 @@ def get_or_create_node_pipeline( # Canonical: every config in connected_nodes goes in through # services.node_config.canonical_config, so a placed node has four float - # coordinates. Altitude is resolved here, at the boundary, because - # passive_radar subscripts it and converts it to metres. - cfg = resolve_altitudes(state.connected_nodes.get(node_id, {}).get("config", {})) + # coordinates. + cfg = state.connected_nodes.get(node_id, {}).get("config", {}) if position_status(cfg) == "positioned": + # Altitude is resolved here, at the boundary, because passive_radar + # subscripts it and converts it to metres. After the placement test, + # not before: only this branch caches anything, so an unplaced node + # reaches this line on every frame it ever sends, and the copy would + # be thrown away every time. + cfg = resolve_altitudes(cfg) pipeline_cfg = { "node_id": node_id, "Fs": cfg.get("fs_hz", cfg.get("Fs", 2_000_000)), @@ -560,7 +588,9 @@ def process_one_frame(node_id: str, frame: dict, default_pipeline: PassiveRadarP + round_.adsb_inputs ) if solver_inputs: - node_cfgs = get_node_configs() + # Only what these inputs name: the snapshot copies a config per + # node, and the fleet is far larger than any one candidate. + node_cfgs = get_node_configs(set().union(*(_solver_input_node_ids(s) for s in solver_inputs))) for s_in in solver_inputs: if s_in["n_nodes"] < 2: continue diff --git a/backend/services/tasks/known_lane.py b/backend/services/tasks/known_lane.py index e2a9335e..8d227acd 100644 --- a/backend/services/tasks/known_lane.py +++ b/backend/services/tasks/known_lane.py @@ -817,9 +817,12 @@ def run_dark_follow_pass(solve_fn, node_cfgs: dict | None = None, mode: str | No from services.frame_processor import get_node_configs node_cfgs = get_node_configs() - # A node whose config has gone (disconnected since the claim) cannot be - # solved with: the LM needs its geometry. Drop the node rather than - # the key — the remaining nodes are still a solve if there are two. + # A node absent from the snapshot cannot be solved with: the LM needs + # its geometry, and get_node_configs returns the placed nodes only, so + # this drops both a node that disconnected since the claim and one that + # re-registered without its position while these claims were held. + # Drop the node rather than the key: the remaining nodes are still a + # solve if there are two of them. claims = {nid: c for nid, c in claims.items() if nid in node_cfgs} if len(claims) < 2: continue diff --git a/backend/services/tasks/solver.py b/backend/services/tasks/solver.py index d565e864..b1a93d51 100644 --- a/backend/services/tasks/solver.py +++ b/backend/services/tasks/solver.py @@ -32,6 +32,7 @@ from services.geo import bearing_deg, bistatic_differential_km, node_beam_params, offset_latlon_m from services.geo import haversine_km as _haversine_km from services.id_utils import is_transponder_hex, multinode_hex_from_key, normalize_hex_key +from services.node_config import position_status from services.solve_uncertainty import solve_sigma_m _N_SOLVER_WORKERS = int(os.getenv("SOLVER_WORKERS", "2")) @@ -3401,6 +3402,15 @@ def _process_solver_item( cfg = node_cfgs.get(nid) if not cfg: continue + # A receiver is enough: the range circle and the bearing wedge + # are both about it, and the bistatic branch below tests its + # transmitter separately. Gated at all because node_cfgs is an + # unfiltered snapshot of every connected node and nothing from + # submit_tracks_round to here checks placement, so a node + # re-registered without its position while its retained tracks + # were being paired arrives here unplaced. + if position_status(cfg) not in ("positioned", "missing_tx"): + continue p = node_beam_params(cfg) rx_lat, rx_lon = p["rx_lat"], p["rx_lon"] range_km = _haversine_km(rx_lat, rx_lon, result["lat"], result["lon"]) diff --git a/backend/tests/test_frame_processor.py b/backend/tests/test_frame_processor.py index 4d1c6541..cb8df5ab 100644 --- a/backend/tests/test_frame_processor.py +++ b/backend/tests/test_frame_processor.py @@ -189,6 +189,32 @@ def test_leaves_the_stored_config_alone(self): get_node_configs() assert stored["rx_alt_ft"] is None + def test_omits_an_unplaced_node(self): + """The snapshot is the placement gate for everything drawn from it. + + Consumers test `nid in node_cfgs` and treat that as "can be solved + with", which is only true if an unplaced node never appears: it has no + geometry to solve against, and its None coordinates would otherwise + reach the solver queue through known_lane's dark-follow claim filter. + """ + state.connected_nodes["test-cfg-5"] = { + "config": {"rx_lat": None, "rx_lon": None, "tx_lat": None, "tx_lon": None}, + "status": "active", + } + assert "test-cfg-5" not in get_node_configs() + + def test_wanted_narrows_the_snapshot(self): + """A config is copied per node, so a caller that can only reach a + handful says so rather than paying for the fleet.""" + for nid in ("test-cfg-6", "test-cfg-7"): + state.connected_nodes[nid] = { + "config": {"rx_lat": 33.9, "rx_lon": -84.6}, + "status": "active", + } + configs = get_node_configs({"test-cfg-6"}) + assert "test-cfg-6" in configs + assert "test-cfg-7" not in configs + class TestResolveAltitudes: def test_sea_level_survives(self): diff --git a/backend/tests/test_solver_trimming.py b/backend/tests/test_solver_trimming.py index 0f529848..7bb11eaa 100644 --- a/backend/tests/test_solver_trimming.py +++ b/backend/tests/test_solver_trimming.py @@ -18,6 +18,7 @@ import time from core import state +from services.node_config import canonical_config from services.tasks import solver as solver_mod LAT, LON = 35.0, -82.0 @@ -599,6 +600,37 @@ def solve_fn(_s_in, _cfgs): failure = rec["beam_failures"][0] assert failure["rule"] == "range" + def test_a_node_with_no_receiver_is_skipped_rather_than_gated_on(self): + """node_cfgs is an unfiltered snapshot of every connected node, and + nothing between submit_tracks_round and the beam gate checks placement, + so a node re-registered without its position while its retained tracks + were being paired arrives here unplaced. There is no receiver to measure + a range or a bearing from, so it contributes no verdict at all rather + than a range computed against a stand-in coordinate.""" + s_in = { + "n_nodes": 2, + "measurements": [ + {"node_id": "n1", "delay_us": 10.0, "doppler_hz": 1.0, "snr": 15.0}, + {"node_id": "unplaced", "delay_us": 12.0, "doppler_hz": 2.0, "snr": 14.0}, + ], + "timestamp_ms": int(time.time() * 1000), + } + cfgs = { + "n1": {"rx_lat": 35.0, "rx_lon": -82.0, "max_range_km": 500.0}, + # Canonical form of a node that declared no position: the keys are + # present and null, so a `.get(key, 0)` default cannot rescue it. + "unplaced": canonical_config({"rx_lat": None, "rx_lon": None, "max_range_km": 5.0}), + } + + def solve_fn(_s_in, _cfgs): + return _stub_result(["n1", "unplaced"], rms_delay=1.0, lat=35.1, lon=-82.0, n_nodes=2) + + result = self._run(s_in, solve_fn, cfgs=cfgs) + + # n1 passes its range test and the unplaced node is skipped, so the + # solve survives. Without the gate this raises TypeError instead. + assert result is not None and result["success"] + class _StubFov: """Duck-types EmpiricalCoverageState's beam-gate surface — enough for