Skip to content

Proxy /api/geocode to tower-finder-service on every tower vhost - #369

Open
jehanazad wants to merge 1 commit into
mainfrom
feat/proxy-geocode
Open

jehanazad wants to merge 1 commit into
mainfrom
feat/proxy-geocode

Conversation

@jehanazad

Copy link
Copy Markdown
Contributor

Why

tower-finder-service grew POST /api/geocode (offworldlabs/tower-finder-service#31): the search form's address field resolves a typed US address to coordinates through it. nginx here forwards only /api/towers, /api/elevation and /api/config, so on every public towers hostname the lookup falls through location / to the monolith and answers 404. Observed today on test-towers.retina.fm after a deploy of the service that carries the route. The service's own edge on 8443 answers it, but nothing routes there until the Cloudflare flip.

What

  • deploy/nginx/snippets/towers-proxy.conf: a fourth location /api/geocode block, same tower-finder.conf include as the other three. Rendered for the test environment it lands on all six vhosts that carry the snippet.
  • backend/tests/test_towers_vhost_coverage.py: /api/geocode joins _PROXIED_PATHS, so the snippet must carry it and no exact-match location may outrank it.
  • deploy/tower-contract.sh: assert_geocode_contract, probing with an empty query. The service's request model refuses that with 422 before either geocoder upstream (US Census, Nominatim) is asked, so the probe spends no third-party quota, has no degraded state to tolerate, and cannot be confused with the fallback's 404. Wired into the direct-run gate, staging-smoke-test.sh, the production smoke suite in ci.yml, and the pre-deploy check of staging's instance over retina-edge.
  • README, ONBOARDING and the compose/render comments now list four routes.

This repo's own bundle does not call the route yet; the vhosts must answer the same set either way, for the same reason the dashboard vhosts already include the snippet.

Verified

  • test_towers_vhost_coverage.py: 5 passed.
  • deploy/check-env-parity.py: staging and test in parity with production.
  • Rendered nginx.conf for the test overlay: location /api/geocode present on the same six vhosts as location /api/config.
  • pre-commit run --all-files: clean (ruff check, ruff format, vulture, ruff-config x2).
  • bash deploy/tower-contract.sh https://staging-towers.retina.fm/api/towers against current staging: towers, elevation and config OK; geocode FAILED with the new 404 diagnostic, which is the defect this PR fixes and what the check will pass once this deploys.

Note for the deploy

The pre-deploy gate asserts staging's tower-finder-service answers 422 on an empty geocode query. That instance must carry offworldlabs/tower-finder-service#31, which is on that repo's main.

🤖 Generated with Claude Code

tower-finder-service grew POST /api/geocode (its PR #31): the search form's
address field resolves a typed US address to coordinates through it. nginx
here forwarded only /api/towers, /api/elevation and /api/config, so on every
public towers hostname the lookup fell through `location /` to the monolith
and answered 404. The service's own edge on 8443 has the route, but nothing
routes there until the Cloudflare flip.

Add the fourth location to snippets/towers-proxy.conf, so the include keeps
carrying the whole tower stack rather than three quarters of it. This repo's
bundle does not call the route yet; the vhosts must answer the same set either
way, for the same reason the dashboard vhosts already include the snippet.

Guard it the way the other three are guarded:
- test_towers_vhost_coverage.py asserts the snippet proxies it and that no
  exact-match location outranks it.
- tower-contract.sh gains assert_geocode_contract, probing with an empty
  query. The service's request model refuses that with 422 before either
  geocoder upstream (US Census, Nominatim) is asked, so the probe spends no
  third-party quota and cannot be confused with the fallback's 404. The
  direct-run gate, the staging smoke test, the production smoke test and the
  pre-deploy check of staging's instance over retina-edge all call it.
- README, ONBOARDING and the compose/render comments list four routes.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
@claude

claude Bot commented Sep 14, 2026

Copy link
Copy Markdown

Claude finished @jehanazad's task in 3m 28s —— View job


Claude finished reviewing this PR

  • Gather context (PR body, diff, CLAUDE.md/ONBOARDING.md)
  • Understand the request (custom instruction: run /code-review on this PR)
  • Review changed files for correctness, security, consistency
  • Post review feedback

Summary

This is a clean, well-scoped change: a fourth location /api/geocode block added to the shared nginx snippet, plus matching updates to the vhost-coverage test, the tower contract library, both smoke suites, and docs. I traced the wiring end-to-end (nginx snippet → test_towers_vhost_coverage.pytower-contract.shstaging-smoke-test.shci.yml's pre-deploy and production-smoke jobs) and it's internally consistent — the new assert_geocode_contract follows the same retry/shape conventions as assert_elevation_contract/_assert_json_keys, the empty-query-→-422 probe design is sound (it never reaches a paid geocoder), and every place that enumerated "three" routes was updated to "four". No functional bugs found.

Two minor nits, neither blocking:

1. Stray literal newline in a printf stringdeploy/staging-smoke-test.sh:352-353

    echo "FAIL"; printf '    %s
' "$REASON"; FAIL=$((FAIL+1))

Every other FAIL/WARN print in this file (lines 94, 328, 330) and in tower-contract.sh writes printf ' %s\n' "$REASON" on one line. Here the \n escape got replaced with an actual line break inside the single-quoted string. Output is byte-identical either way (printf treats a literal \n byte in its format string the same as the two-character escape), so this isn't a functional bug, but it stands out as a copy/paste artifact against the file's own convention and is worth tidying for consistency.
Fix this →

2. Adjacent comment now slightly staledeploy/tower-contract.sh:121

# Elevation is the only one of the three routes that fans out to a third party,

This predates the PR and wasn't touched by it, but the PR's own description notes /api/geocode fans out to two geocoder upstreams (US Census, Nominatim) in real usage — it just never reaches them in assert_geocode_contract's probe because the empty query 422s at validation first. Worth a small wording tweak so a future reader doesn't take "only one" at face value now that there are four routes and a second one also touches a third party in production.
Fix this →

What I checked and found solid

  • deploy/nginx/snippets/towers-proxy.conf: new block is a longer prefix than location /api/ and there's no location = exact match anywhere in the template outranking it (confirmed via test_no_exact_match_location_outranks_the_proxied_paths).
  • backend/tests/test_towers_vhost_coverage.py: _PROXIED_PATHS and the docstrings were updated together; the exact-match and snippet-coverage tests iterate the whole tuple, so /api/geocode is enforced the same as the other three.
  • assert_geocode_contract in deploy/tower-contract.sh: retry-then-fail structure matches assert_tower_contract/assert_elevation_contract; success returns 0 on the first 422 seen, 404 gets its own diagnostic pointing at the snippet, and the direct-run loop (for check in elevation config geocode) correctly derives ${BASE}/api/geocode.
  • ci.yml: both the pre-deploy staging gate (base64 round-trip of TOWER_CONTRACT_GEOCODE_BODY over SSH) and the production smoke suite (dash/towers vhost loop) are wired consistently with the existing config/elevation checks.
  • deploy/staging-smoke-test.sh: new dash-vhost geocode check mirrors the existing elevation/config checks in placement and style.
  • Docs (README.md, ONBOARDING.md, docker-compose.local.yml) consistently updated from "three" to "four" routes; no stale reference to the old three-route set remains anywhere else in the repo.

Branch: feat/proxy-geocode

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