Skip to content

fix(readiness): give each health-check try its own relay processor (MAG-4639) - #584

Merged
nimrod-teich merged 2 commits into
mainfrom
mag-4639
Oct 26, 2026
Merged

nimrod-teich merged 2 commits into
mainfrom
mag-4639

Conversation

@nimrod-teich

@nimrod-teich nimrod-teich commented Oct 26, 2026 •

Copy link
Copy Markdown
Contributor

#Closes MAG-4639

Jira ticket: MAG-4639

Note

Left unmerged for review. It changes how the health check, and so /readyz, judges a chain after one of its tries stalls.

Stack (merge top to bottom):

  1. fix(readiness): give each health-check try its own relay processor (MAG-4639) #584 fix(readiness): give each health-check try its own relay processor (MAG-4639)
  2. feat(readiness): readiness follows reachability, three-valued probe (MAG-4641) #586 feat(readiness): readiness follows reachability, three-valued probe (MAG-4641)

Why

The health check (sendRelayWithRetries) makes up to six tries, each bounded by a 5 s deadline, and every try shared one relay processor. An attempt cut off at its try's deadline still delivers its response, late, in the defer that ends it. The next try's WaitForResults returns on the first response it reads, so it took that late response as its own answer within microseconds, gave up, and cut off its own attempt. That attempt's late response went to the try after it, and so on down all six. Only the first try ever waited on its own attempt.

The test below reproduces it against main:

[-] failed sending init relay  cancelled relay processor {total responses:0}             try 1, stalled 5 s
[-] failed sending init relay  ... context deadline exceeded: Caller's context ...      try 2 reads try 1's
[-] failed sending init relay  ... context canceled: Request context was canceled ...   tries 3 to 6, each the previous try's

The first try stalls, so one stalled upstream fails the whole check even when the other upstreams answer. Since MAG-4617 the probe alone decides readiness and runs within 15 s of a failed client relay, so on a quiet chain one stall can turn the interface unhealthy until the next probe.

What changed

  • One processor per try (rpcsmartrouter_server.go, sendRelayWithRetries), built on that try's context. A late response lands in a processor nothing reads.
  • The state machine and its used providers stay shared, so each try still goes to an endpoint the earlier tries did not use, as before.
  • Each try reports its own result. With initialRelays, a later try no longer re-reports an earlier try's success as its own.

How to verify

go test -race -count=1 ./protocol/rpcsmartrouter/ -run 'TestSendCraftedRelays|TestHealthRelay'

TestSendCraftedRelays_AStalledTryDoesNotAnswerTheNext (about 5 s) puts an upstream that stalls on its first request, then answers each request after 50 ms, behind three providers. It fails on main at ec999d5 and passes here: 5 of 5 under -race. The 50 ms is slower than reading a response already waiting and far faster than the try deadline, so the shared processor fails the check every time.

The full go test ./protocol/rpcsmartrouter/ (without -race) passes on the branch in 254 s.

TestHealthCommand_RejectsUnbootableCrossValidation can fail under -race when it runs after any crafted-relay test. The health command sets the global log level while a previous test's connector goroutine is still logging its shutdown. It fails 2 of 4 runs on main paired with TestSendCraftedRelays_ResultAnswerIsHealthy, and 2 of 4 here paired with the new test, so this change does not cause it.

Screenshots

No UI change. The evidence is the log excerpt under Why.

…AG-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.
…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.
@nimrod-teich
nimrod-teich merged commit bb43ae9 into main Oct 26, 2026
19 checks passed
@nimrod-teich
nimrod-teich deleted the mag-4639 branch October 26, 2026 21:07
nimrod-teich added a commit that referenced this pull request Oct 29, 2026
…(MAG-4720) (#615)

The generated v1.6.5 Highlights said the router turns NotReady "when
all of its relays fail", which is #581's rule. MAG-4641 (#586) replaced
it before the release: a chain turns unhealthy only when no upstream
answers, and an error, an error status or a rate limit leaves it as it
was. #586 merged into #584's branch and reached main inside #584's
squash commit, so the generator gave it no line.

Lead the Highlights with the shipped rule and who notices, drop the
wrong sentence, add the MAG-4641 line pointing at bb43ae9, and note on
the #581 and #584 lines how they relate to it.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants