Harden RPC-fault reliability: bound locks/timeouts and cap fan-out (7 fixes)#1430
Open
kAIPraxisBot wants to merge 12 commits into
Open
Conversation
…ng callers getStatus, GetBlockHash and DownloadGenesis used the package-level http.Get (http.DefaultClient), which sets no timeout, so a node that accepts the connection but never responds hangs the caller indefinitely. Route all three through a shared client that bounds the dial (10s) and the wait-for-response headers (30s) without capping the body, so large genesis downloads still work. Add tests covering the happy/error paths and a stalled-node bound check.
…dline handleTransferRequest and validateTimestampNonce called recorder.Status with context.Background(); the cometbft client has no default timeout, so a stalled node could pile up request goroutines/connections indefinitely (line 361 runs before the bandwidth limiter, so it is not capacity-capped). Wrap both calls with a statusQueryTimeout deadline (request-scoped where an echo context is available). Add a test asserting the Status context now carries a deadline.
scheduleDepletedEscrowReplacement and scheduleAutoSettlement spawned one goroutine and chain tx per escrow with only per-id dedup and no global cap. A mass-depletion event (100+ escrows) fanned out that many broadcasts at once, driving the RPC node into 503s and a self-amplifying crash loop (the next tick re-fires the whole batch). Add a bounded escrowTxSem acquired around the actual broadcast (released during settlement back-off so slots are not starved). The helpers are nil-safe so Gateway literals in tests run unbounded. Add tests asserting concurrency reaches but never exceeds the cap, and nil-safety.
…ution failure The once-per-epoch whitelist sync overwrote the nginx whitelist with whatever IPs resolved. A transient DNS outage at that single sync moment resolved zero IPs, yet syncWhitelist returned nil, so the caller advanced BlockHeightSynced and did not re-sync until the next epoch - de-whitelisting every validator, dropping their rate-limit exemption and accruing fail2ban bans. The 404 path already returns an error to preserve state; mirror it: when participants exist but none resolve to a public IP, return an error so the whitelist is preserved and the sync retries within the same epoch. Add a test for the guard.
…rotocol settleDevshardOnChain held the process-wide finalizeMu across the multi-round network Finalize. finalizeMu serialises every finalize, including user /v1/finalize, so a hung Finalize blocked all of them - and the admin settle path (gateway.go) passes only the request context, which has no server-side deadline, so the hold could be indefinite. Route the call through finalizeUnderLock, which holds the lock but bounds the Finalize with rotationFinalizeTimeout, capping the worst-case hold without changing the serialisation invariant. Add tests for the timeout, mutex release, and the bounded context passed through.
GetCurrentEpochGroupData held the write lock c.mu across CurrentEpochGroupData with context.Background() (no timeout). At an epoch boundary every in-flight request misses at once; one hung query pins the mutex and stalls all inference validation. Coalesce concurrent misses with singleflight, run the query with a bounded context OUTSIDE the lock, and take the lock only for the brief cache read/write. Apply the same shape to GetEpochGroupData. Add tests asserting single-RPC coalescing, that the mutex is not held across the query, and the bounded context - verified under -race.
…ries getSigners held the write lock c.mu across two sequential chain queries (GranteesByMessageType + AccountByAddress), so one slow granter throttled signature verification for every granter, and the 2-minute TTL made the misses recur. Coalesce concurrent misses for the same key with singleflight and run the queries OUTSIDE the lock, taking it only for the brief cache read/write. Add tests for single-pair coalescing, that the mutex is not held across the queries, fetch-path correctness/caching, and error propagation - under -race.
…headers Review follow-up: the transport-level ResponseHeaderTimeout does not bound the body read, so a node that returns 200 headers and then stalls mid-body would still hang the caller. Give each fetch a per-request context deadline covering the entire request (short for the tiny status/block responses, generous for a genesis download). Add a headers-then-body-stall test.
Review follow-up: getSigners and GetEpochGroupData executed the singleflight query on whichever caller became leader, so that caller cancelling (client disconnect or its own deadline) failed every caller sharing the coalesced result - most likely exactly at an epoch-boundary burst. Run the coalesced query under a fresh bounded context instead, matching GetCurrentEpochGroupData.
…t have a URL Review follow-up: gate the transient-resolution-failure guard on the count of participants that actually advertise a URL, not the raw total, so a set where nobody has a URL yet (a benign empty state) is not mistaken for a resolution outage and does not loop forever without updating nginx.
…n wait Review follow-up: with Do the caller's ctx was unused after decoupling the query context, and a caller could not stop waiting on its own cancellation. Switch to DoChan and select on the caller ctx: the coalesced query still runs under a fresh decoupled context (one caller cancelling never fails the shared flight), but each caller can now return early on its own ctx without affecting others.
Review follow-up: gating on 'participants have URLs but zero public IPs' still looped forever for an all-private set (URLs resolve to private IPs by design). Count actual DNS resolution failures instead, so the guard fires only on a real transient outage; a set that resolves cleanly to private/no IPs falls through and advances as before. Add a test for the all-private (no-failure) path.
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.
Summary
Hardens a class of reliability bugs where a transient RPC / chain-node failure (503, timeout, connection refused, tx error) turns into a disproportionate, long-lived outage. Three shapes recur across the services:
The recurring aggravator is
context.Background()on chain calls (the cometbft/cosmos clients set no default timeout). Each fix is one commit; every changed path is covered by a test, and the concurrency changes were run under-race.Fixes
1.
epoch_group_cache.go— don't hold the cache lock across the chain query (HIGH)GetCurrentEpochGroupDataheld the write lockc.muacrossCurrentEpochGroupData(context.Background()). At an epoch boundary every in-flight request misses at once; one hung query pins the mutex and stalls all inference validation. Now:singleflightcoalesces concurrent misses into one query, the query runs with a bounded context outside the lock, and the lock is taken only for the brief cache read/write. The same shape inGetEpochGroupDatais fixed too. Tested: single-RPC coalescing, mutex not held across the query, bounded context — under-race.2.
authzcache/cache.go— don't hold the cache lock across the signer queries (HIGH)getSignersheld the write lock across two sequential chain queries (GranteesByMessageType+AccountByAddress), so one slow granter throttled signature verification for every granter, and the 2-minute TTL made the misses recur. Now:singleflight+ both queries run outside the lock under a bounded context. Tested: single-pair coalescing, mutex not held across the queries, fetch-path correctness/caching, error propagation — under-race.3.
devshard/gateway.go— bound concurrent escrow-rotation chain transactions (HIGH)scheduleDepletedEscrowReplacementandscheduleAutoSettlementspawned one goroutine and chain tx per escrow with only per-id dedup and no global cap. A mass-depletion event (100+ escrows) fanned out that many broadcasts at once, driving the node into 503s and a self-amplifying crash loop. Now: a boundedescrowTxSemacquired around the actual broadcast (released during settlement back-off so slots are not starved); the helpers are nil-safe. Tested: concurrency reaches but never exceeds the cap; nil-safety.4.
proxy/sidecar/main.go— don't wipe the validator whitelist on a transient resolution failure (MEDIUM)The once-per-epoch whitelist sync overwrote the nginx whitelist with whatever resolved. A transient DNS outage at that single moment resolved zero IPs, yet
syncWhitelistreturned nil, so the caller advancedBlockHeightSyncedand did not re-sync until the next epoch — de-whitelisting every validator and accruing fail2ban bans. The 404 path already returns an error to preserve state; this mirrors it: when participants that advertise a URL exist but none resolve to a public IP, return an error so the whitelist is preserved and the sync retries within the same epoch. Tested: the guard fires on an all-unresolvable set.5.
devshard/escrow_rotator.go— bound thefinalizeMuhold across the settlementFinalize(MEDIUM)settleDevshardOnChainheld the process-widefinalizeMuacross the multi-round networkFinalize.finalizeMuserialises every finalize (user/v1/finalizeincluded), so a hungFinalizeblocked all of them — and the admin settle path passes only the request context, which has no server-side deadline, so the hold could be indefinite. Now:finalizeUnderLockstill holds the lock but bounds theFinalizewithrotationFinalizeTimeout, capping the worst-case hold without changing the serialisation invariant. Tested: the timeout cuts off a hung finalize and the mutex is released.6.
post_chat_handler.go— bound the request-pathStatus()lookups with a deadline (MEDIUM)handleTransferRequestandvalidateTimestampNoncecalledrecorder.Status(context.Background()); the cometbft client has no default timeout, so a stalled node could pile up request goroutines/connections (line 361 runs before the bandwidth limiter, so it is not capacity-capped). Both calls now carry astatusQueryTimeoutdeadline (request-scoped where an echo context is available). Tested: theStatuscontext now carries a deadline.7.
inference-chain/internal/rpc/client.go— bound the RPC HTTP client (LOW)getStatus,GetBlockHashandDownloadGenesisusedhttp.Get(http.DefaultClient), which sets no timeout, so a node that accepts the connection but never responds hangs the caller. Now: a shared client bounds dial + response headers, and each request additionally carries a per-request context deadline covering the whole request including the body read (short for the tiny status/block responses, generous for a genesis download), so a headers-then-body-stall is bounded too. Tested: happy/error paths, a never-responds node, and a headers-then-body-stall.Testing
-race.decentralized-api,devshard,inference-chain, and theproxy/sidecarbinary.DoChanso one caller's cancellation cannot fail the shared flight while each caller can still abandon its own wait; and key the whitelist-preserve guard on actual DNS resolution failures so an all-private participant set does not loop.