Skip to content

Stop another service's outage rolling this repo's deploy back - #392

Merged
Babissimo merged 1 commit into
mainfrom
fix/prod-smoke-off-towers-host
Sep 15, 2026
Merged

Babissimo merged 1 commit into
mainfrom
fix/prod-smoke-off-towers-host

Conversation

@Babissimo

@Babissimo Babissimo commented Sep 15, 2026

Copy link
Copy Markdown
Contributor

Ticket: https://app.clickup.com/t/123zgec2zeq

Independent of #389 and #391 — branched from main, mergeable on its own.

The problem

production-smoke-tests ends in a Rollback production on smoke-test failure step that SSHes into production and reverts retina-server. Two things were wrong with what it gated on.

Three probes named towers.retina.fm. Since the origin flip of 2026-09-14, Cloudflare routes that name to tower-finder-service's own edge, so those probes stopped traversing our nginx at all. They asserted nothing about what this repo does, while still being able to roll it back.

And repointing them is not sufficient. An earlier revision of this PR claimed it was; that was wrong. The probes reach the same service through our proxy whichever hostname they use, and assert_tower_contract failed on any non-200, so a tower-finder-service outage still failed the smoke and still reverted a healthy retina-server release.

The fix

The probes move to map.retina.fm, which we do serve, and the two causes are told apart.

A gateway status (502, 503, 504) means our nginx matched the location and forwarded, and the far side did not answer. That is warned, not failed. Anything else stays fatal.

A broken forward does not look like a gateway status: with no location /api/towers the request falls through location / to the app, whose tower stack went with the monolith, so it answers 404. That is the regression these probes exist to catch and it is untouched.

This follows a precedent already eight lines below in the same block. /api/elevation has warned on its upstream's refusal since 2026-08-27, when open-meteo's quota rolled production back on a deploy that was fine (86cbaxrhp, and again on 2026-09-14 as 123zgec2qqa). The reasoning was never specific to elevation, only to a fault we do not own.

Two consequences carried deliberately

  • That elevation comment justified its own leniency by saying /api/towers and /api/config stay strict. No longer true, so it now rests on the 404 case instead. Comment updated rather than left to mislead.
  • The direct-run gate in tower-contract.sh still treats a config gateway status as fatal. The production smoke asks "should this deploy be reverted"; that gate asks "may a vhost be pointed at this instance", and an instance that is not answering is not ready. Without this the tower-service-contract job would have started passing against a dead service.

Staging's suite warns on the same terms: a red staging smoke skips the production deploy, so left strict it would hold releases behind an outage this repo cannot fix.

Coverage after this change

question covered by can revert production?
does the service honour the contract? its own CI smoke, plus our tower-service-contract job no
do our vhosts forward, in config? test_towers_vhost_coverage.py no
do our vhosts actually forward, live? these probes, via the 404 case yes, and only for our own fault

Verification

Nothing in CI validates a workflow edit in this repo — no actionlint step, no pre-commit hook, and the smoke jobs only run on push to main. So a green tick here proves little. Checked by hand:

  • Return codes proven against a local server returning each status: assert_tower_contract and assert_config_contract give rc 2 for 502/503/504 and rc 1 for 404 and 500.
  • actionlint clean; bash -n clean on both scripts; shellcheck reports nothing in tower-contract.sh and nothing in the lines this touches in staging-smoke-test.sh.
  • pre-commit run --all-files passes. test_towers_vhost_coverage.py and test_nginx_rewrite_ordering.py pass (8 tests).
  • Every repointed endpoint probed live against production: map.retina.fm/ 200, /api/towers 200 on map, api, dash; /api/elevation and /api/config 200 on dash and map. admin answers 302 without credentials, which is expected — it is behind Cloudflare Access and the job supplies a service token.

The real proof is the first production deploy after merge; worth watching that run.

🤖 Generated with Claude Code

@claude

This comment has been minimized.

@Babissimo
Babissimo force-pushed the fix/prod-smoke-off-towers-host branch from 5ea8b30 to d8a8487 Compare September 15, 2026 14:31
@claude

This comment has been minimized.

@Babissimo
Babissimo force-pushed the fix/prod-smoke-off-towers-host branch from d8a8487 to 814e84c Compare September 15, 2026 15:06
@claude

This comment has been minimized.

@Babissimo Babissimo changed the title Stop the production smoke asserting another service's health Stop another service's outage rolling this repo's deploy back Sep 15, 2026
Three probes in production-smoke-tests named towers.retina.fm. Since the origin
flip of 2026-09-14 Cloudflare routes that name to tower-finder-service's own
edge, so they no longer traversed our nginx at all and asserted nothing about
what this repo does. They now point at map.retina.fm, which we do serve.

Repointing alone would not have been enough, and the first draft of this change
claimed otherwise. The probes reach the same service through our proxy either
way, and assert_tower_contract failed on any non-200, so a service outage still
failed the smoke — and this job answers a failed smoke by SSHing into production
and reverting retina-server. A dependency being down was grounds for rolling
back a healthy release.

So the two causes are now told apart. A gateway status (502, 503, 504) means
our nginx matched the location and forwarded, and the far side did not answer:
warned, not failed. Anything else stays fatal, and a broken forward is not a
gateway status anyway — with no `location /api/towers` the request falls through
`location /` to the app, whose tower stack went with the monolith, so it 404s.
That is the regression these probes exist to catch and it is untouched.

This follows the precedent already in the same block. /api/elevation has warned
on its upstream's refusal since 2026-08-27, when open-meteo's quota rolled
production back on a deploy that was fine. The reasoning there was never
specific to elevation; it was specific to a fault we do not own.

Two consequences carried deliberately. That comment justified elevation's
leniency by saying towers and config stay strict, which is no longer true, so it
now rests on the 404 case instead. And the direct-run gate in tower-contract.sh
keeps treating a config gateway status as fatal: the production smoke is asking
"should this deploy be reverted", but that gate is asking "may a vhost be pointed
at this instance", and an instance that is not answering is not ready.

Staging's suite warns on the same terms. A red staging smoke skips the
production deploy, so left strict it would hold releases behind an outage this
repo cannot fix.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@Babissimo
Babissimo force-pushed the fix/prod-smoke-off-towers-host branch from 814e84c to c2b2247 Compare September 15, 2026 15:18
@claude

claude Bot commented Sep 15, 2026

Copy link
Copy Markdown

Claude finished @Babissimo's task in 2m 27s —— View job


Review of PR #392 (updated diff, commit c2b2247)

  • Gather context (diff, related files)
  • Understand the request (custom instruction: run /code-review on this PR)
  • Run code review
  • Post findings

This update is on top of the prior review pass (which flagged the diff at 814e84c). Diffed the actual PR commit against its parent (b114652) to confirm scope: .github/workflows/ci.yml, deploy/staging-smoke-test.sh, deploy/tower-contract.sh — matching the PR's stated file list.

Prior findings — all addressed

All three blocking/medium findings from the previous pass are fixed in this revision:

  1. Staging's /api/config check (deploy/staging-smoke-test.sh:409-421) now goes through assert_config_contract with the same OK/WARN/FAIL three-way dispatch as the elevation check beside it, instead of the old hard-failing check_status. A tower-finder-service gateway status here no longer blocks deploy-production.
  2. assert_elevation_contract (deploy/tower-contract.sh:226) now calls the shared _is_upstream_failure helper instead of its old inline 503|502 check, so it tolerates 504 the same way assert_tower_contract and _assert_json_keys do.
  3. The stale ci.yml comment (~1113, now 1104-1107) has been rewritten to say routing coverage rests on the 404 case, matching the updated comment already in tower-contract.sh.

The previously-noted lower-priority item — the direct-run gate's first assert_tower_contract call not capturing $? — is also fixed: tower-contract.sh:270-287 now captures TARGET_RC and gives a distinct ::error:: message for rc=2 ("not answering") vs rc=1 ("contract wrong").

New observations (non-blocking)

  • deploy/tower-contract.sh:297-320 — in the direct-run gate's elevation/config loop, a config check returning rc=2 (upstream gateway status) still prints the same generic ::error:: text used for rc=1 ("does not yet answer... the way retina's callers read it" / "pointing one at this instance would break it"), which describes a shape mismatch rather than "not answering." The REASON line printed just above it does correctly explain the gateway status, so this isn't misleading in practice — just less precise than the distinct messaging the TARGET case above it now gets for the same rc=2/rc=1 split. Not worth a fix on its own.
  • deploy/staging-smoke-test.sh:90-91check_contract's comment ("adapts it to the PASS/FAIL tally") predates the WARN branch added a few lines below it. Cosmetic only.

Neither of these affects correctness — the deploy gate still fails config non-200 either way (which is the intended behavior per the PR description), and both are wording nits inside comments/log text.

Checked, no issue

  • bash -n-equivalent read-through of both scripts: no syntax issues, braces/quoting balanced, all if REASON=$(...); then rc=0; else rc=$?; fi capture sites (7 total, all three files) are correct and consistent with each other.
  • WARN counter is pre-existing shared tally infrastructure (smoke-tally.sh/smoke_summary) in both ci.yml and staging-smoke-test.sh — not something this PR needed to introduce.
  • No CLAUDE.md rule implicated: no /v1/nodes contract touched, no raw host/address committed (all hostnames are existing public vhosts), no new .env keys.
  • The still-outstanding docs/runbook.md:262-270 reference to towers.retina.fm (flagged in the previous pass) remains unchanged and out of scope for this diff.

Overall: this revision closes out every finding from the last pass; nothing new rises above a wording nit.

@Babissimo
Babissimo merged commit 63d59a9 into main Sep 15, 2026
14 checks passed
@Babissimo
Babissimo deleted the fix/prod-smoke-off-towers-host branch September 15, 2026 15:21
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