20260814 - Adopt spec v1.1.1, and fix four faults that silenced a node - #12
Merged
Merged
Conversation
The server author's 1.1.0 answers eight of our questions and breaks a generated client three ways. 1.1.1 adds the beam change they agreed with Josh: both antenna fields required and nullable, so an uncharacterised antenna sends two nulls rather than being unable to build a NodeConfig at all. Adopted by generation rather than by reading. The new spec went to disk, models were regenerated, and the suite located every place the contract moved — 80-odd failures and two collection errors, which is the drift check earning its keep. ## wire/serialise.py is back Deleted three days ago when the spec briefly had no required-nullable field. 1.1.1 has seven: both beam fields, HeartbeatRequest.config_version, and all four NodeHealth core values. The rule was correct the whole time; what changed was whether anything exercised it. That is worth remembering — the module went because its only subject disappeared, not because the reasoning was wrong. The canary written when it was deleted did its job: it fired on regeneration and pointed at the rule before anything shipped. tests/wire/test_serialise.py now pins the inventory by name, so a future revision adding or removing one of these is a visible edit rather than a silent behaviour change. ## What the revision answered Q10 — boot_id, accepted verbatim. Distinct per process start, in memory, never persisted, and required on frames as well as beats: a restart between two beats would otherwise corrupt the server's gap accounting for up to a minute. Q16 — HeartbeatRequest.config_version became nullable, which is the whole fix. may_heartbeat no longer requires a version, so a node whose config.yml is unreadable now beats and says why instead of going silent exactly when it most needed to be heard. Q11 — queue_depth removed. Q17 — `stalled` added, so a node whose radar stopped no longer reports `error` and no longer raises against the wrong team. Q7 and Q3 — the ADS-B tolerances and cpi_s are required now. All three were collected once, removed under "the spec is the scope", and have come back through the spec rather than by us deciding they looked useful. The discipline working, not failing. delay_tolerance is in kilometres: blah2-api compares it against computeBistaticDelay, which returns bistatic_range / 1000 from metric ECEF (api/bistatic.js:67). Converting gives 6.671 against the spec's own 6.67 example, which is confirmation rather than a coincidence. ## A gap of ours, found while auditing adsb_present was hardcoded None in the heartbeat, and had been since the service was first wired. An absent NodeHealth.adsb means *disabled* to the server, so every node was reporting ADS-B off — including nodes actively associating. The answer was sitting in the poll loop the whole time and never reached the beat. Blah2Client now tracks it alongside last_poll_ok, sticky across a failed poll because blah2 going away does not un-configure ADS-B. Worth noting why the audits missed it: the payload validated and the behavioural checks only asked whether the code path existed. A hardcoded None on an optional field is invisible to both. ## Two whitelist entries rather than suppressions Blah2.NoneType_None is a junk enum member the generator produced from `null` appearing in the enum list alongside a nullable type; it is redundant and worth asking the server author to drop. NodeConfig.delay_tolerance_us reads as unused because it only ever appears as a keyword argument — the source attribute is delay_tolerance_km, and that rename across the unit boundary is precisely what the naming convention is for. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
## The release gate was weaker than CI ci.yml grew a dead-code check and `ruff format --check`; release.yml never did. Both listed their gates inline, so the two drifted silently and a tag could have published an image CI would have rejected. Nothing shipped through the gap — there are no tags yet — but the gap was real for three days. Both now call tools/check.sh, which is also what to run by hand. Coverage stays CI-only: it is a review aid rather than a gate, and putting it in the script would slow the local loop for nothing. ## tools/check.sh --tracked Runs the gates against a clean copy of tracked files rather than the working tree, because the working tree lies in both directions. Gitignored scratch files are invisible to CI but present locally, so a local run can fail a gate CI would pass — and, far worse, pass one CI will fail. That second case happened twice this week: once because vulture was not installed and the script reported "no dead code found", and once because a `git add -A` swept four gitignored files into a commit. It found two more while being written. `RED` was undefined in summarise_run.py, missed because an earlier lint run had its output redirected to /dev/null. And --tracked itself was broken: it called $REPO_ROOT's copies of check-dead-code.sh and generate-models.sh, both of which cd to their own directory's parent, so they audited the working tree instead of the clean one — the exact false pass the mode exists to prevent. ## A live view of the mock http://127.0.0.1:<port>/ while any live script runs. Served by mock_server.py itself, stdlib only, polling its own control channel so it works against a run already in progress. A request log printed after a run answers "what happened"; a node under test raises "what is happening". Watching `streaming` turn to `stalled` as blah2 goes down, and the errors count tick up, is worth more than reconstructing it from JSON afterwards. ## Two live scripts live-stress.sh drives the paths that only exist across a restart and a failure: boot_id changing while seq starts over, a token reused rather than re-registering, and a node with an unreadable config.yml still heartbeating with config_version null. All three confirmed on Owl. live-stalled.sh stops blah2 to reach NodeState.stalled, which nothing else can. It is the only script here that writes to the node, so the safety is explicit: plain `docker stop` on four named containers, no sudo and no compose file, and `docker start` from an EXIT trap over its own ssh connection so a dead tunnel cannot strand the radar. It verifies blah2-api answers before exiting and fails loudly if not. Confirmed live: streaming -> stalled with blah2 "down", eight heartbeats held through the outage, detections stopped and resumed under one boot_id, and the node recovered without intervention. Also corrects the compose entry's "reflash window", which the spec renamed to operator reactivation. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The open-questions list and the implementation plan are deleted. Both had done their job and were now actively misleading. open-questions.md tracked eighteen asks. Spec v1.1.0 and v1.1.1 answered or accepted eight of them, four were withdrawn once checked, and the rest were closed by decisions taken here. What remained was a document whose numbering was load-bearing across four other files and whose content was largely historical — with 74 references to it in code, each one a pointer a reader would have to chase to learn something no longer true. implementation-plan.md described three build stages that are all built, a "Still to settle" list that is settled, and a module layout that CLAUDE.md already carries. Its one durable section, the table of decisions and the reasoning behind them, is folded into CLAUDE.md rather than lost: those are exactly the choices that get re-litigated when nobody wrote down why. Every Q-reference is gone from the code. The bare pointers were deleted; the ones carrying real meaning were rewritten to state the reason rather than cite a number. "See Q11" becomes what Q11 argued. Corrects the drift that prompted this: - CLAUDE.md claimed the ADS-B tolerances were removed under "the spec is the scope". They came back as required fields in v1.1.1, which is the rule working in both directions and worth saying so. - CLAUDE.md and data-sources.md described the beam fields as optional-and- omitted. They are nullable-and-null. - data-sources.md still argued for `cpi_s` as an open request; it is now a required field. - README claimed stages 1 and 2 built, 177 tests, stage 3 "does not exist yet". - The exception allowing us to edit the spec now records that it has already been silently reverted once by an upstream revision, and that 1.1.1 again carries a change of ours — so check `NodeConfig.beam_width_deg` on adoption. - Records that Owl runs dopplerMin/Max ±1000 rather than the standard ±200, so its measured frame rate is five times the Doppler bins' worth slower and is not a fleet figure. That nearly went to the server author as one. Four documents remain: the spec, data-sources, README and CLAUDE.md. Correspondence with the server author stays outside version control, because a draft rewritten between sends reads as a second, competing spec. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
… next A review found four defects, all verified before fixing. Two of them stopped a loop for the lifetime of the process, which is the failure mode the whole nullable-config_version design exists to prevent — a node that has gone quiet is indistinguishable from a node that has lost power. ## A long fault, seen twice, killed the heartbeat Errors.add truncated to exactly MAX_MESSAGE, then _render appended " (x2)". 517 characters against the spec's maxLength of 512, so HeartbeatRequest refused it; reliable.py calls payload_factory() unguarded, so the thread died. The bound now applies to the rendered string, and the repeat suffix is reserved so the *message* gives way rather than the count. "seen 300 times" is what separates a wedged node from a blip, and costs a handful of characters against a traceback nobody reads to the end. ## An out-of-range config killed registration _send_config was widened to `except ValueError` when IncompleteConfig went, on the reasoning that pydantic's ValidationError is a ValueError. The same widening was needed in _registration_payload and was never carried across — so a latitude past 90 parses as YAML, fails the spec's bounds, and takes the registration loop with it. Fixed, and it now reports through _config_rejected and errors[]. ## Every loop is now supervised The reason both bugs were silent rather than loud. A daemon thread that raises writes nothing to errors[] and nothing to the status document, and the process stays alive with one fewer loop. Loops are wrapped so an unexpected exception is recorded, written to the status document immediately, and reported as a detail that outranks every other — because a stopped loop means the node is not doing what the rest of the document claims. Deliberately no restart. A loop that died on deterministic input would spin; the useful outcome is a human seeing which one stopped and why. ## blah2-api's warmup read as a transport failure `var detection = ''` (api/server.js:69) until the first CPI, so an empty 200 on every stack start was decoded inside the transport try-block and reported blah2 "down" for a service that was answering. Body reading and decoding are now separate: reaching the service sets last_poll_ok regardless of what the body turns out to be, an empty body is the warmup case, and an unparseable one joins the MalformedFrame path. The test fake could not express this — it only ever returned a decoded object, never a body — which is why nothing caught it. FakeResponse now carries `text`. ## Also - DISK_PATH is passed in the compose entry. host.py defaults to /data, which the compose entry never mounts; only its subdirectories are, so statvfs measured the container overlay and disk_free_mb was fiction. - CLAUDE.md records that `stalled` ships ahead of the server confirming it, and what it costs if they decline: a heartbeat 400 is Kind.INVALID, not retryable, so it is dropped — silencing the node whose radar just died. - Repairs three sentences the Q-reference removal mangled in units.py and heartbeat.py, and clears the last stale references: serialise.py citing a test file that no longer exists and saying "six" where its own table says seven, client.py documenting the old serialisation, and two docs still describing the beam fields as omitted rather than null. - The outbound document cluster is deleted. It was gitignored and then never updated, so it had become the stale half — describing a spec two revisions old and restating questions the spec has since answered. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Two more defects of the same shape as the last set: bounds that each hold individually and compose past a third one nobody was checking. ## errors[] is bounded twice and still exceeded the body cap Twenty items of 512 characters is 10 KiB of errors alone, against the 8 KiB the spec caps a heartbeat body at *"at the origin, ahead of parsing"*. Measured at 10,599 bytes. A node carrying twenty distinct long faults is precisely the node whose heartbeat matters, and it would have been refused unread — no status code the client could interpret, because nothing parsed it. The rendered array now has a byte budget and drops least-frequent-first to fit, the same trade the count bound already makes: the persistent fault survives and the noise around it goes. 10,599 bytes becomes 5,938. The mock enforces the caps too. It validated schemas and accepted a payload production would have rejected, which is the circularity problem in miniature — a mock that only checks what we thought to check. ## inf and nan are not JSON `snr` is `10*log10(|x|) - noisePower` (CfarDetector1D.cpp:48), so a detection with zero magnitude gives -inf. pydantic accepts it, because the spec bounds these arrays' length rather than their values, and json.dumps then emits the bare tokens Infinity and NaN — which no strict parser accepts. One such value and the entire frame is refused, opaquely, at both ends. Non-finite detections are now dropped by index across all four parallel arrays, on the same reasoning as a malformed adsb_hex: one bad value must not cost the other detections in the CPI, and dropping it from every array is what keeps them parallel. ## Checked and found sound Detection volume needed no change. 5000 detections truncate to 512 and the worst case — 512 detections all ADS-B associated — is 18.5 KiB against a 64 KiB cap. One real risk found and not fixed here, because the fix is not ours: `tx_callsign` is capped at 32 characters by the spec and retina-gui puts **no length limit** on the tower-name field. A longer name now surfaces as a rejected configuration rather than a dead loop, but the node still cannot register. ## tools/live-edges.sh Drives all of it on a real node without touching blah2 — a synthetic blah2-api on a spare port serves each edge in turn, so the radar keeps running. Confirmed on Owl: 5000 truncated to 512 with a 14 KiB body, three non-finite detections dropped with zero reaching the mock and all four arrays parallel, an empty 200 read as warmup rather than as unreachable, and an unparseable body recorded as a data fault while the state stayed `streaming`. Its fifth phase did not do its job and is worth saying so: connection errors to one URL deduplicate to a single message, so it never produced twenty distinct faults. That bound is covered by unit tests and by the mock's new cap instead. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Four pieces of work live in retina-gui rather than here, and one of them is why
no node can register. Written down in CLAUDE.md next to the sibling-repo table
so it is read by anyone picking this up, rather than living in a conversation.
The vulture whitelist's UNREVIEWED section is gone. It listed five names as
"no reference found anywhere in the estate"; four of them were wrong.
_Handler dispatches endpoint handlers by name — getattr(self, f"_{endpoint}")
at mock_server.py:438 — so vulture sees four definitions and no callers, which
makes them contracts rather than a backlog. The fifth, probe_report.failures(),
was genuinely dead: summarise() reads the module global directly. Deleted rather
than whitelisted.
A whitelist of unreviewed entries is worse than no whitelist. Each one is a
claim, and these had been sitting there asserting something untrue.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Adopts
node-ingestv1.1.1, then fixes what adopting it — and a review — turnedup. Six commits, each a separate idea.
Adopting v1.1.1
Done by generation, not by reading: the new spec to disk, models regenerated,
and the suite located every place the contract moved — 80-odd failures and two
collection errors.
Eight of our questions answered.
boot_idaccepted verbatim (Q10).HeartbeatRequest.config_versionmade nullable (Q16).queue_depthremoved(Q11).
stalledadded (Q17).cpi_sand both ADS-B tolerances now required(Q3, Q7) — all three had been removed under "the spec is the scope" and came
back through the spec, which is that rule working in both directions.
wire/serialise.pyis back. It was deleted three days ago when the specbriefly had no required-nullable field; v1.1.1 has seven. The rule was correct
throughout — what changed was whether anything exercised it. The canary written
at deletion fired on regeneration and pointed at the rule before anything
shipped.
delay_toleranceis in kilometres: blah2-api compares it againstcomputeBistaticDelay, which returnsbistatic_range / 1000from metric ECEF(
api/bistatic.js:67). Converting gives 6.671 against the spec's own 6.67example — confirmation rather than coincidence.
Four faults that silenced a node
Two were found by a review, two by pushing on the edges. All four verified
before fixing.
Errors.addtruncated to exactly 512,_renderthen appended" (x2)"→ 517 against the spec'smaxLength: 512.reliable.pycallspayload_factory()unguarded, so the heartbeat thread died permanently_send_configwas widened toexcept ValueError; the same widening was never carried across to_registration_payload. A latitude past 90 killed the registration looperrors[]inf/nansnris10*log10(|x|) - noisePower, so a zero-magnitude detection gives-inf.json.dumpsemits the bare tokensInfinity/NaN, which no strict parser accepts — one value and the whole frame is refusedEach fix keeps the useful half: the repeat count survives truncation ("seen 300
times" separates a wedged node from a blip), errors drop least-frequent-first
so the persistent fault outlives the noise, and a non-finite detection is
dropped by index across all four arrays rather than costing the frame.
And a guard so the next one is loud
Every loop is now supervised. This is why all four were silent rather than
loud: a daemon thread that raises writes nothing to
errors[], nothing to thestatus document, and leaves the process alive with one fewer loop. A stopped
loop is now recorded, written immediately, and reported as a detail that
outranks every other — because the node is not doing what the rest of the
document claims.
No restart on purpose. A loop that died on deterministic input would spin; the
useful outcome is a human seeing which one stopped and why.
One our own audits missed
adsb_presentwas hardcodedNonein the heartbeat and had been since theservice was first wired. An absent
NodeHealth.adsbmeans disabled to theserver, so every node was reporting ADS-B off — including nodes actively
associating. The answer was in the poll loop and never reached the beat.
Verified on Owl
Every endpoint, every reachable state, and the edges:
seqruns both starting at 1 under differentboot_ids,one registration across three process starts. Without
boot_idthe serverwould see
50 → 1and could not tell a restart from catastrophic lossconfig_version: nulland the YAMLparse error in
errors[]. Under the old spec that node went silentstalled—streaming → stalledwithblah2: "down", 8 beats heldthrough the outage, recovered unaided. blah2 restored from an EXIT trap over a
separate ssh connection
3 non-finite dropped with zero reaching the mock, an empty 200 read as
warmup rather than unreachable
The mock now enforces the spec's body caps too. It validated schemas and
accepted a payload production refuses — the circularity problem in miniature.
Also
ci.ymlandrelease.ymlboth calltools/check.sh. They had drifted: the release gate was missing thedead-code check and
ruff format --checkfor three days, so a tag could havepublished an image CI would have rejected.
--trackedruns the gates against a clean copy of tracked files, becausethe working tree lies in both directions. It caught two false passes while
being written, including one in itself.
http://127.0.0.1:18080/— watchingstreamingturnstalledbeats reconstructing it from JSON.retired; the plan's decisions table is folded into CLAUDE.md rather than lost.
All 74 Q-references removed from code, rewritten to state the reason rather
than cite a number.
reference found anywhere in the estate"; four were dispatched by
getattr.The fifth was genuinely dead and is deleted.
Known, and deliberate
stalledships ahead of the server confirming it. The spec marks it"Proposed, confirm before implementing". We send it because the alternative,
error, raises against the node when the fault is the radar's. If the serverstill validates five values, a heartbeat
400isKind.INVALID— notretryable, so dropped — silencing the node whose radar just died. Recorded in
CLAUDE.md and worth confirming before this reaches a fleet.
docs/node-ingest-v1.ymlcarries a change of ours (both beam fieldsnullable, agreed with the server author). Their previous revision silently
reverted our last such edit, so check
NodeConfig.beam_width_degwhen adoptingthe next one.
Still blocked outside this repo. No node can register until retina-gui
persists the three consent records, and its tower-name field needs a 32-character
cap to match
tx_callsign. Both are now in CLAUDE.md.Still circular. Every live result is against our mock, which validates with
the same generated models the client builds from.
staging-api.retina.fmis theonly thing that closes it.
🤖 Generated with Claude Code