Repository navigation
fix(protocol)!: answer a pinned request from its node or fail, with lava-pin-allow-retry to opt out (MAG-2280, MAG-4123) - #578
Tomelia1999 wants to merge 2 commits into
Conversation
ccfd503 to
4584142
Compare
| // to the caller with Lava-Retries 0. Checked before the stateful rate-limit carve-out, so a | ||
| // pinned write refused for rate is not broadcast elsewhere either. Cross-validation overrides | ||
| // pins and stopped above. | ||
| if input.Pinned { |
There was a problem hiding this comment.
@Tomelia1999 A question from QA automation, to resolve before this merges.
This line makes a pinned request (lava-select-provider) stop after the pinned provider: it is answered by that provider, or it fails with that provider's own error. It is never retried on another provider.
Our failover tests use the pin to make the first attempt go to a chosen provider, and then check that the retry goes to another provider. A text search of the automation repository (Magma-Devs/smart-router-automation) finds 21 failover test files that pin a request. 14 of them also assert a retry or a second provider in the reply (counted by the search, not read one by one). Examples:
- test_a_jsonrpc_router_fails_over_at_once_when_a_provider_port_is_closed.py
- test_rest_and_grpc_requests_move_to_another_primary_when_the_pinned_primary_goes_quiet.py
- test_the_last_chance_with_a_backup_tier.py
- test_a_retry_goes_to_another_primary_and_not_to_a_backup_when_only_the_backups_offer_archive.py (merged on 2026-10-26)
From the description of this PR, these tests will fail once it merges. The failover regression runs on router main at 03:00 UTC from Sunday to Thursday (its image variable FAILOVER_REGRESSION_IMAGE is not set), so the next run after the merge goes red.
Question: after this change, what is the supported way for a test to make the first attempt of a request go to a chosen provider, and still let the retry go to another provider? For example an override that holds for the first attempt only, or a debug setting.
QA needs your answer to change the tests in time. Please answer here and resolve this thread before you merge.
Tomelia1999
changed the title
…from another node (MAG-2280, MAG-4123) A request pinned with lava-select-provider or lava-stickiness is answered by the pinned upstream or fails with that upstream's error. The router no longer retries it, hedges it, or re-selects for it on another node, and no longer drops the pin when its own consistency gate rejects the pinned node. Three paths quietly served a pinned request from another node: - MAG-2280: the pinned node answered with a retryable error (a 429, say) and the retry batch dropped the pin (MAG-2228). For the head-then-fetch sequence lava-stickiness exists for, the fallback node was never checked to have reached the block, so the retry produced the very false gap the header was meant to prevent; and the pin stayed on the failing node for the next request. - MAG-4123: the pre-dispatch consistency gate judged the pinned node too far behind on a `latest` call, released it, and the resend ran selection without the pin: another node answered with Lava-Retries 0 and nothing in the reply said so. - The attempt window hedged a slow pinned request onto another node. Now the policy stops a pinned request after a node error and at the attempt window (DecisionInput.Pinned, reason "Pinned"); a selection error that names the pin stops the send stage at once (reason "PinnedProviderUnavailable") instead of after SendRelayAttempts resends; the consistency gate fails a pinned request with "pinned provider is too far behind the chain tip" instead of starting an unpinned retry; the pin resolver honours the headers on every attempt; and a sticky provider that already failed this request is refused like a named pin, not replaced. The session keeps its node through these failures. A node that keeps failing is still blocked by the health machinery, which invalidates the pin as before. Client-visible: a pinned request that used to be served by another node now returns the pinned node's error (or a router error for the consistency case) with Lava-Retries 0. The docs that promised the old failover are updated in Magma-Devs/docs#77. Tests: policy (pinned never retries, hedges or broadcasts; pin errors stop the send stage), relaycore helpers, the real state machine over the REST mock chain (node error ends on "Pinned" with one batch; a pin refusal ends on the first send), the gate's terminal error, and the sticky already-failed refusal. The hedge-accounting end-to-end test steered its first attempt with a pin; it now uses a fixed selection order, since a pinned request no longer hedges. Two relay_end goldens carry the new stop reason. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
… on request (MAG-2280) With the pin now holding on every attempt, a caller who prefers a node but would rather get an answer elsewhere than an error had no way to say so, and the QA suite had no way to steer a first attempt without forbidding failover. lava-pin-allow-retry: true, sent with lava-select-provider or lava-stickiness, restores the first-attempt-only pin for that request: retries, hedges and the consistency gate's retry may reach other providers and are counted in Lava-Retries. The session's pin is never moved. Only the value "true" (any case) enables it; alone it does nothing. relaycore.PinAllowsRetry reads it; PinnedByDirectives returns false when it is set, so the policy and the consistency gate treat the request as unpinned; resolvePinDirectives takes firstAttempt again and drops the pin on later attempts only for such a request. Verified locally against the provider simulator: pinned+429 retried on a peer (200, retries 1) with the header and failed closed without it or with "false"; a slow pinned node hedged with the header and waited without it; the sticky pin stayed on its node after a retried failure. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
7a13ece to
f111606
Compare
|
Blocked until QA agrees. Moved to draft on purpose: this PR changes the pinned-request contract, and the automation suite needs adjusting first. QA task: MAG-4674. It waits for Victoria to confirm that the pin helpers send |
|
@Tomelia1999 A proposal that keeps the fix and drops most of the breakage: keep The two headers serve two purposes:
What it costs on each side:
So the trade is a few lines in this PR against 500-plus tests that are not easy to change, and the customer-facing fix stays exactly as strong for sticky sessions. The question: can the default be flipped per header in this PR, stickiness strict and select-provider as today? |
|
To be clear on the proposal above: it needs no new header.
So the only change is which behaviour each existing header gets by default. |
Warning
Blocked: do not merge until QA agrees. This PR changes how pinned requests behave, and about 90 automation tests depend on the old behaviour. It waits until QA (Victoria) confirms the test changes in MAG-4674 are in: the pin helpers send
lava-pin-allow-retry: true, and the pin-contract tests are ready to flip.#Closes MAG-2280
#Closes MAG-4123
Jira ticket: MAG-2280
Summary
The rule. A request pinned with
lava-select-providerorlava-stickinessis answered by the pinned upstream or fails with that upstream's own error. The router never quietly answers it from another node. Same rule for both headers.Three paths broke it, all closed here:
lava-stickinessexists for, the fallback node was never checked to have reached the block, so this produced the very false gap the header was bought to prevent; the pin also stayed on the failing node for the next request. Now the policy stops with reasonPinnedand the caller gets the node's error,Lava-Retries: 0.latestcall whose node lagged more than the threshold was released and resent without the pin: another node answered withLava-Retries: 0, invisible. Now the gate fails the request withpinned provider is too far behind the chain tip; a pinned request is not served by another provider(lag and threshold logged). Option A from the ticket. Concrete-block requests are unchanged.Opt-out per request:
lava-pin-allow-retry: true(second commit). Sent with either pin header, it keeps the pin to the first attempt only: retries, hedges and the consistency gate's retry may reach other providers and are counted inLava-Retries; the session's pin is never moved. Onlytrue(any case) enables it; alone it does nothing. For customers who prefer a node but would rather get an answer elsewhere than an error, and for the QA suite, which can add this one header to its pin helpers instead of rewriting ~90 tests that use the pin to steer the first attempt.Where it lives.
relaycore:DecisionInput.Pinned(from the request's original directive headers) and two helpers,PinnedByDirectivesandPinnedUpstreamUnservable; the send-stop arm recordsPinnedProviderUnavailable.relaypolicy:Decidestops a pinned request right after the cross-validation check, so before the stateful rate-limit carve-out (a pinned write refused for rate is not broadcast elsewhere either);OnSendRelayResultstops at once on a selection error that names the pin instead of afterSendRelayAttemptsidentical resends.rpcsmartrouter:resolvePinDirectiveshonours the headers on every attempt (thefirstAttemptparameter is gone);sendRelayToDirectEndpointsreturnspinnedConsistencyRejectioninstead of the retry-triggering gate error when the request is pinned and not cross-validated.lavasession: selection receivestriedByThisRequest; a sticky provider that already failed this request is refused withSelectedProviderAlreadyFailedError, like a named pin, instead of falling through to another provider. The busy-refill fall-through is untouched.Not in this PR, on purpose: a bounded wait-and-retry on the same node for a 429 with a short Retry-After; and whether a pod-local sticky session whose node is merely busy (out of CU, session cap) should fail rather than re-pin. Both are separate decisions.
Known, handled in MAG-4481: a pinned node's 429 reaches the client as HTTP 503 with the router's text, because the router stamps "all rate-limited → 503" (
RelayProcessor.buildFailureResult). That is MAG-4481 decision B (return the node's 429 as is), not a pin issue. The fix lands with MAG-4481, after which a pinned 429 goes back to the caller as the node's own 429.Docs: Magma-Devs/docs#77 (draft until this is in a release) rewrites the three sentences that promised the old failover.
QA follow-ups (automation repo): the strict xfail on
test_stickiness_preserved_across_retries_when_pinned_provider_returns_retryable_errorasserts a contract nobody designed; the expected history is now[pinned=error]withLava-Retries: 0and the node's error returned. The observation test "retry of a sticky request leaves the shared claim unchanged" (P then Q,Lava-Retries ≥ 1) flips to the new contract.test_pinned_call_stays_on_a_lagging_pinned_provider.py(expected failure today) should pass.Verification
go build ./...,go vet ./...clean,gofmt -lclean.policy_pinned_test.go(pinned never retries, hedges or broadcasts; pin errors stop the send stage at once),relaycore/pinned_test.go,rpcsmartrouter/pinned_request_test.go(real state machine over the REST mock chain: a node error ends onPinnedafter exactly one batch; a pin refusal ends on the first send withPinnedProviderUnavailable; the gate's error is terminal and names the lag),lavasession/sticky_pinned_retry_test.go.TestResolvePinDirectives(pins read on every attempt), the tworelay_endgoldens for an unknown pinned provider (stop_reasonis nowPinnedProviderUnavailable), the hang-marks list of non-blaming stop reasons, andTestRESTListener_HedgedLoserCountsAsARetry, which steered its first attempt with a pin and now uses a fixed selection order.relaypolicy,relaycore,lavasessionandrpcsmartrouterpass.Author Checklist
I have...
!in the type prefix if API or client breaking change (a pinned request that used to be served by another node now returns an error)mainbranchReviewers Checklist
I have...
🤖 Generated with Claude Code