fix(flute-gateway): /healthz probes concurrently — 1,950 false unhealthy verdicts - #2578
Merged
Merged
Conversation
…thy verdicts
On B850 the container has been marked unhealthy for 26 hours straight (failing
streak 1,950) while serving correctly the entire time. Measured cause:
curl /healthz -> HTTP 200 in 15.038s
healthcheck timeout: 15s
It failed by 38 milliseconds.
/healthz awaited five provider probes and a Supabase probe strictly in
sequence, so every dependency that was DOWN added its own full timeout to the
wall clock. With whisper (DNS resolution failure), voicebox and omnivoice all
unreachable, that summed past the timeout. Bringing Supabase back up did NOT
help — measured again at 15.107s — which confirms the providers, not Supabase,
were the cost.
asyncio.gather makes the wall clock the SLOWEST probe instead of their sum, so
the endpoint stops degrading linearly with the number of dependencies that are
down — precisely when a health endpoint matters most and can least afford to
hang. return_exceptions=True also stops one exploding provider from 500-ing
the only endpoint that can tell an operator which provider exploded, and a
probe that RAISED is now logged rather than silently flattened to False.
Two regression tests added. The existing health tests asserted status codes and
payload keys and never once asserted how long the answer took, so they passed
throughout the outage. Latency is part of this endpoint's contract now.
Verified:
with fix 2 passed in 0.50s
without fix FAILED — "/healthz took 1.82s; serial execution would be
~1.80s. The probes are running one after another again."
(negative control: main.py reverted, tests kept)
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Contributor
|
Important Review available on request
Reviews should be triggered manually for repositories with fewer than 10 stars. Select Trigger review above or comment ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
github-actions
Bot
added
chit-aware
Contributor
🔏 CHIT-aware change — control-body routingThis PR touches a CHIT-aware service (ports 8086/8087/8103/8106/8113/9224 surface). Before a Control-Body ACK:
Advisory routing only — the blocking contract check is CHIT Contract. |
CI failed both new concurrency tests with DuplicateTimeseries. Not a defect in
the code under test, and not reproducible by any local invocation I tried —
single file, whole file, whole directory, and from the repo root all passed.
Root cause: main defines its Prometheus Counters at MODULE scope while
prometheus_client.REGISTRY is process-GLOBAL. The two outlive each other badly.
If anything evicts 'main' from sys.modules — other service test groups sharing a
worker do — a plain `import main` re-runs those definitions against a registry
that still holds them and raises DuplicateTimeseries.
Reproduced deliberately rather than guessed, via a pytest plugin that imports
main and then deletes it from sys.modules:
plain re-import -> DuplicateTimeseries: {'flute_requests',
'flute_requests_total', 'flute_requests_created'}
after unregistering flute_* -> import OK, app = FastAPI
_load_main() reuses the cached module and, only if it is genuinely gone, drops
the stale flute_* collectors first. Tests now patch via patch.object on the
resolved module rather than the "main." string, so they no longer depend on that
name resolving to a freshly executed module.
Verified:
full directory 228 passed, 38 skipped, 6 failed (all 6 pre-existing
auth failures, baselined in CI — unchanged by this PR)
under eviction plugin 2 passed (was: 2 failed, DuplicateTimeseries)
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…t main
CI still failed both new tests, but with a DIFFERENT error — DuplicateTimeseries
is gone, replaced by:
RuntimeError: Form data requires "python-multipart" to be installed.
Root cause is in the CI collector, not this service.
.github/workflows/python-tests.yml:151 builds the test install list by reading
each service requirements.txt line by line:
if not line or line.startswith("#") or line.startswith("-"):
continue # Skip comments, empty lines, and pip options
`-r requirements.lock` starts with "-", so it is silently dropped. This service's
requirements.txt is that include plus one package, so CI installs exactly ONE
dependency for flute-gateway (google-genai) and none of the 1,387 locked ones —
fastapi, httpx, prometheus-client and python-multipart arrive only if some other
service happens to name them. main.py uses fastapi Form(), and FastAPI raises at
route-definition time without python-multipart, so `import main` fails outright.
Restating the pin is the narrowest fix that makes the collector see it (verified
by running the collector's own parsing logic: it now yields ['google-genai',
'python-multipart']). Teaching the collector to follow `-r` includes is the real
fix, but that pulls the whole lock into the CI install and is not this PR's
blast radius — flagged for follow-up.
Also corrects my own earlier read: I reported python-multipart as undeclared
after grepping only requirements.txt. It is pinned at requirements.lock:858; the
gap was never the service's packaging, only the collector's view of it.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…ind spot
The previous commit fixed the wrong input. The failing check `python-tests` is
the merge-gate job (merge-gate.yml:47), which installs
.github/requirements-tests.txt and runs pytest_ratchet.py. The per-service
collector I patched lives in python-tests.yml, which triggers only on push to
main/hardened — it never ran on this PR. That collector's `-r` bug is real and
the pin there is still correct for post-merge, but it was not this failure.
Root cause here is a structural limit of how requirements-tests.txt is derived.
Its header says the contents are "evidence-based — derived by parsing the
imports of all 264 test_*.py files", which is sound and is why the file exists.
But NOTHING imports `multipart` by name. FastAPI requires it at
ROUTE-DEFINITION time for any endpoint declaring Form(...), so importing
flute-gateway's main raises
RuntimeError: Form data requires "python-multipart" to be installed.
before a single test executes. An import scan cannot see that dependency, so it
was never going to appear no matter how carefully the scan was run.
Verified against a venv built from ONLY .github/requirements-tests.txt, which is
what CI installs — not the ad-hoc `uv run --with` list I had been using, and
which is why four earlier local runs all passed while CI failed:
with python-multipart 2 passed
without it (uninstalled) 2 failed — RuntimeError: Form data requires
"python-multipart" to be installed.
full flute-gateway dir 230 passed, 36 skipped, 6 failed (all 6 pre-existing
auth failures, baselined; unchanged by this PR)
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…kip .claude CI now reports "125 total, 125 baselined, 0 new" — the new tests pass and nothing regressed. The gate is red only because the baseline still lists 57 entries that no longer fail, which is the ratchet working as designed. Those 57 were one missing dependency. flute-gateway/main.py declares a fastapi Form() endpoint and FastAPI raises at route-definition time without python-multipart, so importing the module failed and its whole test suite was recorded as broken. 57 of 182 entries — nearly a third of the entire baseline — were that, not 57 broken tests. Two tool fixes fall out of trying to follow the gate's own advice: 1. `.claude` added to EXCLUDE_PARTS. Discovery walks git worktrees under `.claude/worktrees/`, each a full repo copy. CI's checkout has none, so this is a no-op there; on a machine with worktrees it took discovery from 29 groups to 427. The gate tells you to run `make -C pmoves python-tests-baseline` — doing that locally would have written a baseline of duplicated worktree-scoped keys CI can never reproduce. 2. The stale list is no longer truncated at 40. `--write-baseline` cannot be run anywhere but CI regardless (discovery also walks populated submodules, which CI checks out empty — a developer machine reported 398 groups), so the operator must prune by hand from the CI log. Truncating at 40 removed exactly the information needed to do it. A list you are told to act on should not be abbreviated. This commit drops the 40 stale entries CI printed (182 -> 142). The remaining 17 were behind the truncation and will be listed in full by the next run. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…ching CI The previous commit's un-truncation immediately paid for itself: CI listed all 17 remaining stale entries in full instead of hiding them behind "... and 17 more", so they could be pruned in one pass rather than binary-searched. All 17 are services.flute-gateway.tests.test_voice_profiles — the last of the suite that could not import without python-multipart. Baseline is now 125 entries, exactly the "125 total, 125 baselined, 0 new" CI reported. Net for this PR: 182 -> 125, a 31% reduction, all from one missing dependency rather than any test being fixed. 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.
The measurement
pmoves-flute-gateway-1on B850 has been marked unhealthy for 26 hours — failing streak 1,950 — while serving correctly the whole time.It failed by 38 milliseconds.
Cause
/healthzawaited five provider probes and a Supabase probe strictly in sequence, so every dependency that is DOWN adds its own full timeout to the wall clock. Withwhisper(DNS resolution failure),voiceboxandomnivoiceall unreachable on this node, that summed past the timeout.Bringing the Supabase stack back up did not help — re-measured at 15.107s — which is what confirms the providers, not Supabase, were the cost.
Fix
asyncio.gathermakes the wall clock the slowest probe rather than their sum, so the endpoint stops degrading linearly with the number of dependencies that are down — precisely when a health endpoint matters most and can least afford to hang.return_exceptions=Trueadditionally stops one exploding provider from 500-ing the only endpoint that could tell an operator which provider exploded. A probe that raised is now logged, rather than silently flattened to the sameFalsea provider returns when it simply isn't ready — those are different facts and only one is a bug.Why the tests didn't catch it
The existing
/healthztests assert status codes and payload keys. None asserted how long the answer took, so all of them passed for the full 26 hours. A health endpoint that is correct but slower than the thing measuring it is indistinguishable from a dead one — latency is part of this endpoint's contract, and is now tested.2 passed in 0.50smain.pyreverted, tests kept)FAILED: /healthz took 1.82s; serial execution would be ~1.80s. The probes are running one after another again.Context
Found while bringing up the Supabase stack on B850. Same class as the findings in #2572: an instrument reporting confidently and wrongly, here for 1,950 consecutive samples that changed nobody's behaviour.
🤖 Generated with Claude Code