rpc: evict idle polling filters after a timeout, add subscription metrics#22261
Merged
Conversation
…viction Signed-off-by: hfuss <hayden.fuss@kaleido.io> self-review fixes Signed-off-by: hfuss <hayden.fuss@kaleido.io>
Conflict resolution: main refactored the WS subscription handlers into the generic subscribeRPC helper (#22180), so the SetSubscriptionProtocol calls from this PR were re-homed into each subscribe closure (NewHeads, subscribePendingTransactions, Logs). The new TransactionReceipts subscription stays untracked since this PR defines no receipts filter type.
Contributor
There was a problem hiding this comment.
Pull request overview
This PR adds timeout-based eviction for idle HTTP polling filters (matching geth-style behavior) and extends subscription tracking/metrics so operators can detect and prevent unbounded growth of unused filters.
Changes:
- Add per-subscription protocol + last-access tracking, plus a background eviction loop keyed off a new
rpc.subscription.filters.timeoutconfig/flag. - Touch HTTP filters on polling RPCs (
eth_getFilterChanges,eth_getFilterLogs) to keep actively-polled filters from being evicted. - Introduce protocol-labeled subscription metrics and counters for created/unsubscribed/reaped subscriptions.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| rpc/rpchelper/subscription.go | Adds protocol + last-access tracking and eviction helpers to subscription channels. |
| rpc/rpchelper/metrics.go | Adds protocol label support and counters for subscription lifecycle tracking. |
| rpc/rpchelper/filters.go | Starts eviction loop, implements stale-sub eviction, and wires metrics + tracking into unsubscribe paths. |
| rpc/rpchelper/config.go | Adds RpcSubscriptionFiltersTimeout to filter config with default 0 (disabled). |
| rpc/jsonrpc/eth_filters.go | Enables tracking/touching for HTTP filters and protocol labeling for WS subscriptions. |
| node/cli/flags.go | Adds rpc.subscription.filters.timeout duration flag for embedded node CLI. |
| node/cli/default_flags.go | Registers the new timeout flag in default CLI flags. |
| cmd/rpcdaemon/cli/config.go | Adds the timeout flag to rpcdaemon’s Cobra config. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…rChanges ReadPendingTxs consumes every batch accumulated since the last poll, but GetFilterChanges only reported txs[0], silently dropping the hashes of any later batches.
… check Replace ShouldEvict + re-check with CloseIfIdle, which decides staleness and closes under the same lock Touch uses, so a subscription polled within the timeout can never be evicted; this removes the TOCTOU race the old two-phase eviction acknowledged as unavoidable. lastAccess and protocol move from atomics to the existing chan_sub mutex. Also compute the eviction check interval once in New so the startup log matches the actual ticker (it previously logged timeout/2 while the loop floored to 1s), fix the evictStaleSubscriptions doc-comment name, and add eviction/touch coverage tests. Addresses Copilot review comments from #18591.
Member
Author
|
Addressed the Copilot review comments from #18591 in a05032f:
While touching those lines, two more fixes landed:
|
Each evicted logs filter rebuilt the aggregate request and issued its own remote update; one update at the end of the eviction cycle covers them all. Client-driven UnsubscribeLogs keeps its per-call update.
…eviction plumbing Drop late writes to the per-filter stores: the forwarding goroutine can drain channel-buffered items after unsubscribe/eviction has deleted the store, and the recreated entry is unreachable (reads are gated on the subscription existing), so it leaked forever. The Add* functions now check subscription existence under the store lock and delete instead of storing when it is gone. Deduplicate the tracking/eviction plumbing: one trackedSub resolver (via a new SubTracker interface) behind Touch/Track/SetSubscriptionProtocol, one generic evictIdleSubs for heads and pending txs, and one generic unsubscribeSubInternal for the channel-sub teardown. Make each eth_getFilterChanges poll cost a single subscription-map lookup: TouchSubscription reports existence, so the type dispatch probes it directly instead of Has* + Touch + re-Get. Metrics helpers switch to WithLabelValues, avoiding a per-call label-map allocation.
Dead since its import; erigon's filter machinery lives in rpc/rpchelper. Keeps the criteria types and FilterCriteria JSON decoding, and fixes the ReceiptsFilterCriteria doc-comment name.
yperbasis
changed the title
- Track pollable subscriptions in a single id->type registry: Subscribe* now takes the protocol at creation (http = evictable polling filter, ws = connection-scoped, empty = internal), so tracking cannot be forgotten; TouchSubscription resolves the filter type in one map lookup; one eviction loop replaces evictIdleSubs/evictIdleLogsSubs and captures the protocol during the Range. - Add metrics.CounterVec (mirroring GaugeVec) and cache the subscription lifecycle counters as package vars instead of building a label string and hitting the global registry on every event. - hasLogsFilter: RLock instead of exclusive Lock; it runs per delivered log inside AddLogs' orphan guard. - Drop the now test-only Has*Subscription methods; reshape UnsubscribeLogs to the early-return shape of its siblings.
…emote filter Previously SubscribeLogs/SubscribeReceipts removed the just-created filter on a failed remote-filter update but still returned the (closed) channel and dead id, so eth_newFilter appeared to succeed while every subsequent poll returned 'filter not found', and WS/sync subscribers waited on a channel that would never fire. They now return an error and install nothing; eth_newFilter, eth_subscribe (logs, receipts), and eth_sendRawTransactionSync propagate it to the client.
AskAlexSharov
approved these changes
Jul 7, 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
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.
Supersedes #18591 by @onelapahead (his branch lives on an org fork that maintainers cannot push to; the original commit is carried here with authorship preserved).
Polling filters (
eth_newFilter,eth_newBlockFilter,eth_newPendingTransactionFilter) previously lived untileth_uninstallFilteror a restart, accumulating buffered heap for clients that walked away. They are now evicted when not polled within--rpc.subscription.filters.timeout(default 5 minutes, matching the stale-filter deadline in geth'seth/filters—FilterAPI.timeoutLoopwithConfig.Timeout; 0 disables eviction). Any poll (eth_getFilterChanges,eth_getFilterLogs) resets the filter's deadline. WebSocket subscriptions are unaffected — they end with their connection.Behavior change: a client that polls slower than the timeout gets
filter not foundand must recreate its filter — same as geth.Metrics
subscriptions_active{filter,protocol}gauge — current live subscriptionssubscriptions_created_total/subscriptions_unsubscribed_totalcounters (active = created − unsubscribedholds exactly)subscriptions_reaped_total{filter}— the timeout-evicted subsetmetrics.GetOrCreateCounterVecis new indiagnostics/metrics, mirroringGaugeVec)Implementation notes
Subscribe*takes the protocol at creation (http= evictable polling filter,ws= connection-scoped; internal subscriptions are neither evicted nor counted), so a subscription cannot be created untracked. Polls resolve the filter type and reset the deadline in one map lookup, and one eviction loop iterates the registry for all filter types.eth_getFilterChangesnow returns all buffered pending-tx batches (previously only the first batch was reported while all were consumed — silent data loss), and each poll costs a single subscription-map lookup.eth_newFilterandeth_subscribe(logs/receipts) now return an error when the remote filter update fails, instead of handing out a dead filter id / never-firing subscription (that silent failure predates this PR).PublicFilterAPIport inrpc/filters/api.gois removed; the criteria types andFilterCriteriaJSON decoding stay.Covered by unit tests in
rpc/rpchelper/filters_eviction_test.go(eviction, touch-prevents-eviction, WS exemption, orphan guard, batched remote update, and a concurrent touch/evict exercise run under-race) plus aneth_getFilterChangesbatching test inrpc/jsonrpc. ChangeLog updated (breaking-change entry + new flag/metrics).