Skip to content

Make main green again + warm-up hardening from the #254 audit - #255

Merged
jehanazad merged 3 commits into
mainfrom
fix/main-ci-warmup-audit
Aug 26, 2026
Merged

jehanazad merged 3 commits into
mainfrom
fix/main-ci-warmup-audit

Conversation

@jehanazad

Copy link
Copy Markdown
Contributor

Main has been red since the #251 merge, on two independent counts. This fixes both, plus two hardening items a post-merge audit of #254 surfaced.

Unbreak CI

Warm-up hardening (audit findings on #254)

  • Atomic border publish: _load_borders() filled _geoms feature-by-feature behind an unlocked fast-path check. A caller concurrent with the parse could observe {"us"} only, skip the load, and silently 422 a valid Canadian point. Unreachable while every caller sat on the event loop — but Parse the border polygons at startup, not on the first request #254 moved the parse onto a threadpool thread, removing that structural guarantee. The dict is now built locally and published in one update.
  • Guarded startup warm-up: the lifespan called warm_borders() unguarded, turning a missing/corrupt geojson into a whole-API boot failure (plus deploy rollback) where it used to be a 500 on /api/towers alone — the exact blast radius prime_pipeline_at_startup documents refusing, ten lines up. Now swallowed and logged; classify_region still loads on demand.

Verified locally: the 3 KeyError cases fail on clean main and pass here; 172 tests across the affected files pass; full pre-commit run --all-files (ruff check/format, vulture, config drift) green.

🤖 Generated with Claude Code

jehanazad and others added 3 commits August 26, 2026 21:50
Since b9fd39b sim_push_adsb_positions rejects any hex that fails
is_transponder_hex(), so the readable "sim{name}" fixtures are dropped
before they reach state.adsb_aircraft and the test dies on the KeyError
instead of testing the writer. The tcp/fp writers don't filter, so only
this class needs real-looking 24-bit addresses.

Main has been red on exactly these three cases since the #253 merge.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The combined `with` fits on one line at the repo's 120-char limit. Caught by
CI's pre-commit gate, which I should have run before pushing rather than
`ruff check` alone.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Two follow-ups from auditing #254:

_load_borders() filled _geoms feature-by-feature behind an unlocked
fast-path check, so a caller running concurrently with the parse could
observe {"us"} only, skip the load, and silently classify a Canadian
point as unsupported. Unreachable while every caller sat on the event
loop, but the warm-up moved the parse onto a threadpool thread, which
removed that structural guarantee. Build the dict locally and publish it
in one update instead.

The lifespan called warm_borders() unguarded, turning a missing or
corrupt geojson into a whole-API boot failure where it used to be a 500
on /api/towers alone - the exact blast radius prime_pipeline_at_startup
documents refusing, ten lines up. Swallow and log; classify_region still
loads on demand.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@claude

claude Bot commented Aug 26, 2026

Copy link
Copy Markdown

Claude finished @jehanazad's task in 5m 57s —— View job


Claude finished reviewing this PR

  • Gather context (PR body, changed files)
  • Read changed files in full (backend/main.py, backend/services/region_lookup.py, both test files) plus the code they reference (node_pipeline.py, id_utils.py, sim_ingest.py)
  • Verify the CI-fix and hardening claims against the actual diff
  • Post review findings

Summary

Both fixes and both hardening changes hold up. Verified against git diff origin/main...HEAD:

  • Sim-ingest test fixtures (test_adsb_seed_backend.py): _SIM_CASE_HEX now produces 6-hex-digit addresses (a1b2c0, a1b2c1, a1b2c2) that pass is_transponder_hex's fullmatch, while the tcp/fp writer tests correctly keep their unfiltered tcp{name}/fp{name} labels. Matches the described bug.
  • test_region_lookup_warm.py: pure ruff-format whitespace join, no logic change.
  • Atomic border publish (region_lookup.py:37-56): building loaded locally and publishing via one _geoms.update(loaded) is the right minimal fix — a single dict.update under one GIL hold means the unlocked fast-path (if _geoms: return) can no longer observe a partially-filled dict. Sound.
  • Guarded startup warm-up (main.py:159-167): correctly swallows a warm_borders() failure instead of aborting the lifespan, consistent with how other startup/shutdown steps in this same function (snapshot save, HTTP client shutdown) already log-and-continue.

Findings

1. Comment overclaims parity with prime_pipeline_at_startupbackend/main.py:159-167

The new except block's comment says "Same policy as prime_pipeline_at_startup above", but prime_pipeline_at_startup (backend/services/node_pipeline.py:126-132) also calls send_alert("node_priming_failed", ...) so on-call gets paged. This block only does logging.exception(...) — no alert.

Failure scenario: the borders GeoJSON goes missing or gets corrupted in a deploy (e.g. a .dockerignore/build-context change), /api/towers region detection silently degrades, and nothing pages anyone — the only signal is a log line that has to be noticed in aggregate logs, unlike the pipeline-priming failure this comment claims to mirror.

This may be an intentional severity call (smaller blast radius than losing the whole v1 fleet), in which case the comment should say the policy differs rather than claiming parity. Either way it's worth a one-line fix.

Fix this →

2. Latent overflow in the sim-hex fixture generator — backend/tests/test_adsb_seed_backend.py:246 (low severity, not currently triggered)

_SIM_CASE_HEX = {name: f"a1b2c{i}" for i, (name, _kin) in enumerate(_WRITE_CASES)}

This is exactly 6 hex chars only while i stays single-digit. _WRITE_CASES has 3 entries today, so it's fine, but once it grows to an 11th case (i=10), f"a1b2c{i}" becomes 7 chars, is_transponder_hex's fullmatch fails, sim_push_adsb_positions silently increments rejected and skips the write, and _assert_derived then raises a bare KeyError — an opaque failure far from the real cause (hex-length overflow). A fixed-width format (e.g. f"a1{i:04x}") would remove the ceiling cheaply now.

Fix this →

3. Minor duplication (not a fix request) — the new main.py try/except and prime_pipeline_at_startup's try/except are structurally identical (log, optionally alert, swallow) but implemented independently. Not worth a shared helper for two call sites with meaningfully different messages.

Notes

  • No CLAUDE.md rule is implicated: the diff doesn't touch /v1/nodes routes/contracts, backend/.env, or host references.
  • I couldn't run pre-commit run --all-files or the test suite here (no backend/.venv in this checkout), so the "172 tests pass" / "pre-commit green" claims in the PR description are taken on trust rather than independently re-verified.

@jehanazad
jehanazad merged commit ac3f034 into main Aug 26, 2026
14 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant