Repository navigation
feat(readiness): readiness follows reachability, three-valued probe (MAG-4641) - #586
Merged
Merged
Conversation
…MAG-4641) Readiness turned NotReady whenever the health probe got anything but a result. An upstream answering with an error, an error status or a rate limit answers every replica the same way, so all of them left rotation together and clients got connection errors from an empty Service instead of the upstream's reply; a cooldown rate limiter made it flap. The probe now ends in one of three verdicts. A result is healthy. No answer at all (timeouts, transport failures, nothing to send to) is unhealthy: that is what differs between replicas. An answer that is an error, an error status or a rate limit is inconclusive and leaves the chain's health as it was. An answer needs positive evidence (a node error, an HTTPStatusError, a rate limit, REST's node-error flags), so an uncatalogued failure still fails closed. Client relays follow the same rule: a result defers the probe, no answer brings it forward, and an upstream's error answer, a cache hit or a client that hung up does nothing.
Tomelia1999
approved these changes
Oct 26, 2026
nimrod-teich
added a commit
that referenced
this pull request
Oct 26, 2026
…AG-4639) (#584) * fix(readiness): give each health-check try its own relay processor (MAG-4639) Every try of sendRelayWithRetries shared one relay processor. An attempt cut off at its try's 5 s deadline still delivers its response, late, and the next try's WaitForResults returned on it within microseconds, took it as its own answer, and cut off its own attempt, whose late response the try after read in turn. One stalled upstream failed all six tries even when the others answered. Each try now builds its processor on its own context, so a late response lands where nothing reads it. The state machine and its used providers stay shared, so tries still rotate endpoints. * feat(readiness): readiness follows reachability, three-valued probe (MAG-4641) (#586) Readiness turned NotReady whenever the health probe got anything but a result. An upstream answering with an error, an error status or a rate limit answers every replica the same way, so all of them left rotation together and clients got connection errors from an empty Service instead of the upstream's reply; a cooldown rate limiter made it flap. The probe now ends in one of three verdicts. A result is healthy. No answer at all (timeouts, transport failures, nothing to send to) is unhealthy: that is what differs between replicas. An answer that is an error, an error status or a rate limit is inconclusive and leaves the chain's health as it was. An answer needs positive evidence (a node error, an HTTPStatusError, a rate limit, REST's node-error flags), so an uncatalogued failure still fails closed. Client relays follow the same rule: a result defers the probe, no answer brings it forward, and an upstream's error answer, a cache hit or a client that hung up does nothing.
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
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.
#Closes MAG-4641
Jira ticket: MAG-4641
Note
Left unmerged for review. The data plane: this changes when
/readyzturns 503.Merge order: after #585 (MAG-4355). Without it a refused gRPC dial reads as the node's answer, so a gRPC chain whose upstreams all refuse connections would keep its last verdict instead of turning unhealthy. The two branches merge cleanly together.
Stack (merge top to bottom):
Why
Since #581 a chain+interface turns unhealthy, and the pod NotReady, when its health probe gets anything but a result. That caught the replica that could not reach its upstreams in the null-route drill. It also catches an upstream that answers with an error, an error status or a rate limit. Every replica shares its upstreams and gets the same answer, so all of them left rotation together, and clients got connection errors from an empty Service where they used to get the upstream's reply. A rate limiter with a cooldown made readiness flap: pulled, traffic stops, the limit clears, back in, the limit trips again.
What differs between replicas is whether each one reaches its upstreams, and that is what readiness should say.
What changed
metrics.ProbeVerdict,RelaysMonitor.recordProbeResult):--relays-health-unhealthy-intervalwhile unhealthy.upstreamAnswerFrom,protocolErrorIsAnAnswer): a node error, an upstream's HTTP status (JSON-RPC'sHTTPStatusError; REST's node-error and rate-limit flags, since REST fails the attempt with a plain error), or a rate limit. Anything else is no answer, including a failure nobody has catalogued, so the rule fails closed. It readsrelay_end's inputs, not its attempt outcomes:relay_endfiles a JSON-RPC 5xx undertransport_error.sendRelayWithRetries) aggregates its tries: any result is healthy, otherwise any answer is inconclusive, otherwise unreachable. Each try reads only its own processor (fix(readiness): give each health-check try its own relay processor (MAG-4639) #584), so a try that timed out holds nothing and counts as no answer.relayHealthSignalOf, read from the relay's attempts rather than its returned result):LogRelay), as before;ProbeSoon), as before;Health Check Interval Checklog line gainsverdict;health resultis now the health after the probe.docs/METRICS.md("When they change") describes the three verdicts;docs/LOCAL-COMPOSE.mdanddocs/RESP-CACHE.mdsay what a lasting 503 means.Behaviour to know about
smartrouter_overall_healthalert does not fire for it; the tip, stuck-provider and non-200 alerts do.recordProbeResult(case ProbeHealthy, ProbeInconclusivewhen unhealthy); I kept the agreed design and left the call to review.How to verify
TestSendCraftedRelays_VerdictFollowsReachabilitydrives the real probe against a live upstream, about 8 s:Each rule was mutated and the change turned these tests red, then restored:
HTTPStatusErrorevidenceVerdictFollowsReachability/an_HTTP_503,UpstreamAnswerFrom/a_JSON-RPC_error_status,FeedsReadiness/an_error_status…UpstreamAnswerFrom/a_REST_error_statusUpstreamAnswerFrom/a_typed_rate_limit,…/a_classified_rate_limitVerdictFollowsReachability/a_dropped_connection,…/a_refused_connection, fourUpstreamAnswerFromrows,FeedsReadiness/no_answer…VerdictFollowsReachability: error body, 503, 403, rate limitFeedsReadiness/an_error_answer…,…/an_error_status…FeedsReadiness/no_answer…TestRelaysMonitor_InconclusiveProbeKeepsAHealthyChainHealthyTestRelaysMonitor_InconclusiveProbeKeepsAnUnhealthyChainUnhealthyREST's flags and a rate limit carried only by the classifier are covered by the unit table, not end to end: the probe fleet is JSON-RPC, where a 429 also arrives as an
HTTPStatusError.With #585 merged in locally, its real-gRPC harness gives no answer for a refused dial and an answer for a node-sent
UNAVAILABLEunder this rule. That check was not committed: it needs #585's helpers.Full runs:
go vet ./...passes;go test ./protocol/rpcsmartrouter/passes in full without-race(253 s);protocol/metricspasses with-race.protocol/metricsunder-race -count=3failsTestGetUniqueGuidResponseDeterministicon this branch and onmainalike:TestHappyFlowleaves a monitor logging oncontext.Background()while that test sets the global log level.Screenshots
No UI change.