fix: backup providers issues#2266
Merged
Merged
Conversation
…oss epochs Backup providers could never be blocked: blockProvider was a no-op for them since they are not in validAddresses, and the backup selection path never checked any blocked list. Additionally, the epoch-transition re-blocking and health-probe logic only covered the main pairing, leaving previously-blocked backup providers with a clean slate. - Add blockedBackupProviders map to ConsumerSessionManager - blockProvider now adds backup providers to blockedBackupProviders when they are not found in validAddresses - getValidConsumerSessionsWithProviderFromBackupProviderList skips providers in blockedBackupProviders - UpdateAllProviders merges blockedBackupProviders into previousEpochBlockedProviders and re-blocks them in the new epoch - checkAndUnblockHealthyReBlockedProviders handles backup providers in both probe passes, including cross-role transitions (normal→backup) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Review Summary by QodoFix backup provider blocking, metrics fan-out, and Solana chain tracker initialization
WalkthroughsDescription• Track and persist blocked backup provider state across epochs - Add blockedBackupProviders map to track backup providers blocked this epoch - Merge blocked backups into previousEpochBlockedProviders at epoch transition - Re-block backup providers that were blocked in previous epoch if still in backup list - Skip blocked backups in getValidConsumerSessionsWithProviderFromBackupProviderList • Handle backup providers in reconnection and health-check recovery paths - Update GenerateReconnectCallback to unblock backups via blockedBackupProviders - Extend checkAndUnblockHealthyReBlockedProviders to probe and unblock backup providers - Add comprehensive test coverage for backup provider blocking lifecycle • Fix metrics fan-out for shared endpoint URLs across multiple providers - Change urlToProviderName map to urlToProviderNames slice to support multiple providers per URL - Update SetEndpointLatestBlock and RecordBlockFetch to emit metrics for all providers on shared URL - Add deduplication logic in RegisterEndpoint to prevent duplicate provider entries • Fix endpoint health metric reset at epoch transitions for backups - Call SetEndpointOverallHealth during updateEpoch to reset Prometheus gauge alongside struct reset - Ensure backup providers with stuck unhealthy metrics recover at epoch boundary • Enable Solana chain tracker per-endpoint metrics via CustomMessage implementation - Implement CustomMessage in EndpointChainFetcher to support SVMChainTracker getLatestBlockhash calls - Force blocksToSave=1 for Solana-family chains to avoid SVMChainTracker slot-cache misses - Include backup providers in GetAllDirectRPCEndpoints for startup ChainTracker initialization Diagramflowchart LR
A["Backup Provider Blocked"] --> B["Added to blockedBackupProviders"]
B --> C["Skipped in Selection"]
B --> D["Merged to previousEpochBlockedProviders"]
D --> E["Re-blocked in New Epoch"]
E --> F["Comprehensive Probe"]
F --> G["Unblocked if Healthy"]
H["Shared Endpoint URL"] --> I["Multiple Providers Registered"]
I --> J["urlToProviderNames Map"]
J --> K["Fan-out Metrics to All Providers"]
L["Epoch Transition"] --> M["Reset Endpoint Health Struct"]
M --> N["Reset Prometheus Gauge"]
N --> O["Backup Metrics Recover"]
P["Solana Chain Tracker"] --> Q["CustomMessage Implementation"]
Q --> R["Per-endpoint Metrics Enabled"]
R --> S["Backup URLs in Startup Init"]
File Changes1. protocol/lavasession/consumer_session_manager.go
|
Code Review by Qodo
1.
|
Ports the GenerateReconnectCallback change from #2265 on top of #2257: when the periodic reconnect probe succeeds for a blocked backup provider, remove it from blockedBackupProviders instead of calling validateAndReturnBlockedProviderToValidAddressesList (a no-op for backups, which are not in validAddresses). Without this, periodic reconnection could never recover a blocked backup outside the epoch-transition path. Also expands the test suite: - TestCheckAndUnblock_BackupRoutedToComprehensiveProbe covers the !isBackup && !IsReported guard: confirms backups never take the immediate-unblock branch (which would not touch blockedBackupProviders and would silently stall the recovery). - TestCheckAndUnblock_BackupUnblockedWhenHealthy covers the positive path — a blocked backup with a reachable endpoint is unblocked via comprehensive probe at epoch transition. - TestGenerateReconnectCallback_BackupProviderUnblocked covers the new backup branch added by this commit. - TestGenerateReconnectCallback_NonBackupUsesValidAddressesPath guards against regressing the existing non-backup reconnect flow. Co-authored-by: avitenzer <avitenzer@users.noreply.github.com> Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
PR #2256 resets endpoint.ResetHealth() for both primary and backup providers on epoch transition, but only the in-memory struct was cleared — the lava_rpc_endpoint_overall_health Prometheus gauge stayed stuck at 0 (unhealthy) forever. The metric is only toggled on state transitions from relay outcomes (MarkUnhealthy/ResetHealth in the hot path), and backups rarely receive the successful relay needed to push the gauge back to 1. Net effect: operators see backups at 0% uptime on dashboards even though the router considers them healthy again after the epoch reset, which can mask real regressions and (worse) cause failover logic that reads the gauge to refuse routing to a provider that is in fact available. Emits SetEndpointOverallHealth(..., true) alongside the struct reset for both the primary and backup provider loops in updateEpoch, mirrored via rpsr.rpcServers[chainKey].smartRouterEndpointMetrics. Also adds TestUpdateEpoch_ResetsHealthMetric which seeds the gauge to 0 (via SetEndpointOverallHealth) for a primary and a backup, runs updateEpoch, then reads lava_rpc_endpoint_overall_health back from prometheus.DefaultGatherer and asserts both have transitioned to 1. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
ResetEndpointMetrics had no callers and, despite its name, set endpoint_overall_health to 0 instead of 1 — a footgun if ever wired up. The two actual consumers (relay hot path and epoch transition) already call SetEndpointOverallHealth with an explicit bool, so there's no need for a helper. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Two independent gaps combined to leave backup providers showing N/A latest-block on the dashboard: 1. GetAllDirectRPCEndpoints (consumer_session_manager.go) iterated only csm.pairing, so the smart router's initializeChainTrackers startup path skipped every backup. A ChainTracker for a dedicated-URL backup (e.g. base.lava.build with no primary sharing that URL) was only ever created lazily via the relay hot path — which backups rarely reach. Extended the function to iterate csm.backupProviders as well. 2. urlToProviderName was a map[string]string, so multiple providers configured on the same upstream URL collapsed to a single name (last writer wins). The ChainTracker's OnNewBlock callback keys emissions by URL and resolves to a provider name via this map — so every URL-keyed metric emission reached exactly one provider's label, leaving its peers stuck at zero on dashboards. Changed the map to map[string][]string with append+dedup in RegisterEndpoint, added resolveProviderNames returning all providers for a URL, and updated the two URL-keyed emitters (SetEndpointLatestBlock, RecordBlockFetch) to iterate and emit per provider. resolveProviderName is kept for callers that already pass a provider name (relay hot path) — it now returns the first name. Tests: - TestGetAllDirectRPCEndpoints_IncludesBackupProviders — primary and backup both appear in the startup iteration. - TestResolveProviderNames_FansOutAcrossSharedURL — both providers registered at the same URL resolve together. - TestResolveProviderNames_DeduplicatesSameProvider — repeated (URL, providerName) registrations (typical for providers whose node-urls lists the same URL twice) don't duplicate entries. - TestResolveProviderNames_FallsBackToInput — unknown inputs (provider names) round-trip as a single-element slice for backward compatibility with callers that pass names directly. - TestSetEndpointLatestBlock_FansOutAcrossSharedURL — one URL-keyed call populates the gauge for every provider sharing the URL. - TestRecordBlockFetch_FansOutAcrossSharedURL — same guarantee for the chain-tracker success/fail counters. The test files in protocol/metrics that constructed SmartRouterMetricsManager literals were updated to the new field name. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…for Solana
SVMChainTracker (protocol/chaintracker/svm_chain_tracker.go:55) fetches
latest block via CustomMessage with a getLatestBlockhash JSON-RPC body
— it needs the slot, block hash, and block height returned together,
which has no equivalent in the generic FetchLatestBlockNum path.
EndpointChainFetcher.CustomMessage was a stub returning
"CustomMessage not supported for EndpointChainFetcher". On every
Solana-family chain this caused the per-endpoint ChainTracker to fail
its first fetch — no OnNewBlock callbacks ever fired, so the metric
fan-out added earlier in this branch had nothing to fan out and every
per-endpoint metric for backups remained at N/A.
Delegate to the existing sendRawRequest helper: for POST (the SVM
case) the `data` argument is the JSON-RPC body and sendRawRequest
posts it through the direct RPC connection; for GET the `data`
argument is already used as the URL suffix per existing REST
convention, so the same delegation preserves GET semantics.
Tests:
- TestEndpointChainFetcher_CustomMessage_POSTDelegatesToConnection
exercises the SVMChainTracker code path — asserts the upstream
response body flows back through CustomMessage unchanged.
- TestEndpointChainFetcher_CustomMessage_PropagatesUnhealthyConnection
asserts the connection-health check still fails closed rather than
silently returning empty data.
Also fixes a latent compile-break in the package-shared
mockDirectRPCConnection: its GetNodeUrl signature was returning
interface{} while the lavasession.DirectRPCConnection interface
requires *common.NodeUrl. The existing tracker tests never passed the
mock through the interface, so it compiled on its own — but any new
test using it as a DirectRPCConnection (including this one) hit the
mismatch.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…ckers With CustomMessage now working for EndpointChainFetcher, the SVM ChainTracker path made it past its initial getLatestBlockhash call on Solana — only to die seconds later with "ChainTracker stopped with error: slot not found in cache". Root cause is a pre-existing limitation in SVMChainTracker: fetchLatestBlockNumInner populates the blockNum→slot cache for the latest block only (svm_chain_tracker.go:70), but the generic ChainTracker init loop (chain_tracker.go readHashes) iterates latestBlock down to latestBlock - blocksToSave + 1 calling FetchBlockHashByNum for each. On SVM every lookup after the first hits the cache-miss path and returns "slot not found in cache", killing the tracker permanently (StartAndServe returns the error and the goroutine exits). For per-endpoint tracking we don't need history — each tracker watches a single URL, so there's no cross-endpoint fork detection to do; the manager only uses GetLatestBlockNum via ValidateEndpointSync and the OnNewBlock callback for per-provider metric emission. Forcing blocksToSave=1 on Solana-family chains (SOLANA, SOLANAT, KOII, KOIIT) sidesteps the SVMChainTracker limitation entirely without losing any capability the manager actually uses. EVM chains keep their caller-requested value. Test: TestEndpointChainTrackerManager_ForcesBlocksToSave1ForSolana asserts the override fires for every SVM family id and leaves non-SVM chains unchanged. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
HTTPDirectRPCConnection.IsHealthy was hard-coded to return true, with a comment deferring health tracking to "the endpoint/QoS layer". But the comprehensive probe path used by checkAndUnblockHealthyReBlockedProviders (probeProvider → probeDirectRPCEndpoints for static providers) only consults conn.IsHealthy() — so for static HTTP/HTTPS backups the probe was a guaranteed pass regardless of reachability, and a blocked backup with an unreachable upstream would be silently unblocked at every epoch transition. Two complementary changes close this: 1. HTTPDirectRPCConnection grows an atomic healthy flag (initial true), flipped to false by transport-level failures in SendRequest and DoHTTPRequest (dial / TLS / body-read errors), flipped back to true on any successful exchange — including HTTP 4xx/5xx, which are *application* errors, not transport failures. Flipping on 4xx/5xx would make rate-limited endpoints flap unhealthy and starve the dashboard on routine server errors; the test TestHTTPDirectRPCConnection_IsHealthy_Stays4xxHealthy guards that carve-out explicitly. 2. probeDirectRPCEndpoints now also gates on endpoint.Enabled. This belt-and-suspenders with IsHealthy(): the endpoint struct accumulates relay-path failures via MarkUnhealthy → ConnectionRefusals ≥ MaxConsecutiveConnectionAttempts → Enabled=false. Reading that state here means a backup that the hot path has already declared dead won't be optimistically revived by a probe that happens to catch a lucky moment in IsHealthy's lifecycle. Known gap (flagged in the IsHealthy comment): a never-hit backup with an unreachable upstream still reports healthy on the very first epoch because nothing has exercised either signal yet. Closing that cold-start case requires an active probe request (e.g. chain-specific no-op via the ChainParser), which is a meaningfully bigger change and belongs in a separate follow-up. Tests: - TestHTTPDirectRPCConnection_IsHealthy_StartsTrue — documents the optimistic-initialization behavior. - TestHTTPDirectRPCConnection_IsHealthy_FlipsOnDialFailure — the core behavior: a transport error must flip IsHealthy to false. - TestHTTPDirectRPCConnection_IsHealthy_Stays4xxHealthy — guards against overreach on application-level HTTP errors. - TestHTTPDirectRPCConnection_IsHealthy_RecoversAfterFailure — after upstream recovery, a successful exchange must restore IsHealthy=true. - TestProbeDirectRPCEndpoints_RespectsDisabledEndpoint — the endpoint.Enabled gate blocks a disabled endpoint from passing the probe even when IsHealthy happens to say true. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
520c33c to
e03cc62
Compare
avitenzer
reviewed
Apr 13, 2026
The JSON decoder was reading r.Body unbounded, letting a caller stream an
arbitrarily large payload into an endpoint whose only legitimate content
is {"offset_seconds": N}. Wrap the body in http.MaxBytesReader(w, r.Body,
1024) before decoding.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
avitenzer
previously approved these changes
Apr 13, 2026
avitenzer
previously approved these changes
Apr 13, 2026
Node errors classified to a LavaError with Retryable=false (e.g. CHAIN_EXECUTION_REVERTED, CHAIN_OUT_OF_GAS, CHAIN_DOUBLE_SPEND) were retried because the state machine only short-circuited on IsUnsupportedMethod / IsUserError subcategories and ignored the registry's Retryable flag. Populate RelayResult.IsNonRetryable from the classifier on both consumer and smart-router paths and make HasNonRetryableUserFacingErrors rest solely on that flag. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
186d251 to
c9f19da
Compare
nimrod-teich
approved these changes
Apr 14, 2026
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
3 participants
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.
Description
Closes: #XXXX
Author Checklist
All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.
I have...
!in the type prefix if API or client breaking changemainbranchReviewers Checklist
All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.
I have...