Repository navigation
Conversation
…ument DictionaryV1.getData writes each batch's block range into its query text, so every batch is a new text. Parsing it with `gql` for Apollo Client kept every document alive: graphql-tag's docCache never evicts, and Apollo's QueryManager.transformCache retains each document too. A document's size grows with the project's handler filters, so the main thread's heap grew for as long as the node synced through the dictionary. DictionaryV1 now posts its queries as plain GraphQL over HTTP through a protected `query()` method, so nothing is parsed or cached. Failures throw a DictionaryQueryError carrying the dictionary's GraphQL errors, which the existing `distinct` and `startHeight` fallbacks still detect. The Substrate spec version query uses the same method, and neither package depends on @apollo/client any more.
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. 🧰 Additional context used📚 Code guidelines (1)No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configuration
⛔ Files ignored due to path filters (1)
📒 Files selected for processing (7)
💤 Files with no reviewable changes (2)
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 9 remain after this review. 📝 WalkthroughWalkthroughDictionary v1 replaces Apollo Client requests with direct HTTP GraphQL queries. The query method handles response errors and supplies metadata, batch, and spec-version queries. Both node packages remove ChangesDictionary v1 HTTP queries
Priority: ➖ Normal Estimated code review effort: 3 (Moderate) | ~20 minutes Change: Bug fix Merge Risk: ⚪ Minimal · up to No actionable issue was established in the dictionary query change; it is mergeable after normal checks. Security Architecture ReviewSecurity architecture risk: 🔵 Low · up to The change keeps requests within the existing dictionary boundary and preserves the inspected consumers’ failure handling. No introduced security concern was established. Some uncertainty remains around implicit HTTP behavior and deployment compatibility. Retained concerns Security review detailsSecurity Blast Radius
Trust Boundaries and Controls
Resilience and Maintainability Implications
Hardening Proposals
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
70602ae to
7087d3e
Compare
Description
Problem
While a node syncs through a v1 dictionary, its main thread's heap grows without bound and never recovers, until the process fails.
DictionaryV1.getDatabuilds a new GraphQL query for every batch. The filter values are passed as variables, butbuildDictQueryFragmentwrites the batch's block range into the query text:So every batch's query text is unique.
getDataparses it withgqland runs it through Apollo Client, and two caches keep every parsed document alive:docCache: keyed by query text, never evicted.gqladds one document per batch.QueryManager.transformCache: anAutoCleanedWeakCachemeant to drop entries once their document is collected. But each entry's value (nonReactiveQuery) references its own key, so the document is never collected. Only the cache's size limit bounds it, at 2,000 entries by default.A document's size is proportional to the project's handler filters. Small projects may never notice, but a project with many handlers leaks a large document per batch for as long as it syncs through the dictionary. A node following the chain head stops querying the dictionary, so it stops growing, but it keeps what it built up until it restarts.
This is present on
main(node-core 19.3.1).Fix
DictionaryV1now sends its queries as plain GraphQL over HTTP, through a new protectedquery<T>()method using the globalfetch(already used indictionary.service.ts). Nothing is parsed, so nothing is cached, whatever the query text contains.DictionaryQueryErrorwith the HTTP status and the dictionary'sgraphQLErrors. The existingdistinctandstartHeightfallbacks detect unsupported arguments exactly as before. A tested case covers thedistinctfallback end to end.getDatareads a typed result of_metadataplus each queried entity'snodes.SubstrateDictionaryV1's spec version query uses the same method.@apollo/clientis no longer a dependency of@subql/node-coreor@subql/node.Nothing the dictionary client did depended on Apollo:
fetchPolicy: 'no-cache';HttpLinkwith no retry or custom headers;Alternative considered: passing the block range as GraphQL variables would also make the query text constant. But the
blockHeightfilter's type differs between dictionary deployments (BigFloaton some,BigInton newer schemas), so the variable's type would have to be discovered per dictionary. Not parsing at all avoids that and removes the dependency.API change: the protected
clientgetter onDictionaryV1is replaced byquery(). I checked the other SubQuery chain repos that extendDictionaryV1(Cosmos, Algorand, NEAR, Starknet, Stellar, Concordium). None usesclientor touches_clientin its tests; only Substrate did, and it's updated here.Observed in practice
We hit this indexing Polymesh testnet (about 220 handler filters) from genesis with
@subql/node6.4.6 and--workers=4.--trace-gc, the main thread's heap after full collections rose steadily from about 225 MB to 1.6 GB over 3M blocks (about 450 MB per million blocks), while the workers stayed flat. It never stepped at runtime upgrades.docCacheandtransformCache, about 1.7 MB per document for that project.Fatal process out of memoryonce the heap's memory mappings reached the kernel'svm.max_map_countlimit (around 18 GB RSS).Fixes # (no issue opened; happy to open one if you prefer to track it there)
Type of change
The protected
clientgetter is removed (see API change above).Checklist
Testing
fetchmocked: a successful query, GraphQL errors (including what thedistinctfallback checks), a non-JSON response, and agetDataretry withoutdistinct.init()reads the metadata, andgetDatareturns the expected blocks withdistinct.main's.Two failures already present on
mainwill also show here:gateway.subquery.networkcurrently returns HTTP 503 ("failure to get a peer from the ring-balancer"). They fail identically onmain.yarn lintreports one error inpackages/node-core/src/indexer/unfinalizedBlocks.service.ts(import/order), also onmain. It's unrelated, so I haven't touched it here.Summary by CodeRabbit
distinct.