Skip to content

fix(node-core): stop dictionary v1 queries retaining every parsed document - #3048

Open
F-OBrien wants to merge 1 commit into
subquery:mainfrom
F-OBrien:fix/dictionary-v1-unbounded-memory
Open

F-OBrien wants to merge 1 commit into
subquery:mainfrom
F-OBrien:fix/dictionary-v1-unbounded-memory

Conversation

@F-OBrien

@F-OBrien F-OBrien commented Oct 5, 2026 •

Copy link
Copy Markdown

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.getData builds a new GraphQL query for every batch. The filter values are passed as variables, but buildDictQueryFragment writes the batch's block range into the query text:

blockHeight: {greaterThanOrEqualTo: "<startBlock>", lessThan: "<queryEndBlock>"}

So every batch's query text is unique. getData parses it with gql and runs it through Apollo Client, and two caches keep every parsed document alive:

  • graphql-tag's docCache: keyed by query text, never evicted. gql adds one document per batch.
  • Apollo Client's QueryManager.transformCache: an AutoCleanedWeakCache meant 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

DictionaryV1 now sends its queries as plain GraphQL over HTTP, through a new protected query<T>() method using the global fetch (already used in dictionary.service.ts). Nothing is parsed, so nothing is cached, whatever the query text contains.

  • Errors: a failed query throws a DictionaryQueryError with the HTTP status and the dictionary's graphQLErrors. The existing distinct and startHeight fallbacks detect unsupported arguments exactly as before. A tested case covers the distinct fallback end to end.
  • Results are typed: getData reads a typed result of _metadata plus each queried entity's nodes.
  • Substrate: SubstrateDictionaryV1's spec version query uses the same method.
  • Dependencies: @apollo/client is no longer a dependency of @subql/node-core or @subql/node.

Nothing the dictionary client did depended on Apollo:

  • both queries ran with fetchPolicy: 'no-cache';
  • the link was a plain HttpLink with no retry or custom headers;
  • the default error policy already rejected on any GraphQL error.

Alternative considered: passing the block range as GraphQL variables would also make the query text constant. But the blockHeight filter's type differs between dictionary deployments (BigFloat on some, BigInt on 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 client getter on DictionaryV1 is replaced by query(). I checked the other SubQuery chain repos that extend DictionaryV1 (Cosmos, Algorand, NEAR, Starknet, Stellar, Concordium). None uses client or touches _client in 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/node 6.4.6 and --workers=4.

  • Growth: with --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.
  • What heap snapshots showed: the growth was GraphQL syntax trees held by docCache and transformCache, about 1.7 MB per document for that project.
  • The failure: each genesis resync eventually crashed with Fatal process out of memory once the heap's memory mappings reached the kernel's vm.max_map_count limit (around 18 GB RSS).
  • With both caches neutralised: using an interim workaround (clearing graphql-tag's cache after each query and capping Apollo's document caches), the main thread's heap stayed flat at about 155 MB across the same range, with sync speed unchanged. That workaround is what this PR makes unnecessary. I'll add a measurement with this exact change once it has run.

Fixes # (no issue opened; happy to open one if you prefer to track it there)

Type of change

  • Bug fix (non-breaking change which fixes an issue)

The protected client getter is removed (see API change above).

Checklist

  • I have tested locally
  • I have performed a self review of my changes
  • Updated any relevant documentation (none affected)
  • Linked to any relevant issues (none open)
  • I have added tests relevant to my changes
  • Any dependent changes have been merged and published in downstream modules (none needed)
  • My code is up to date with the base branch
  • I have updated relevant changelogs

Testing

  • New unit tests, with fetch mocked: a successful query, GraphQL errors (including what the distinct fallback checks), a non-JSON response, and a getData retry without distinct.
  • Against a live v1 dictionary (Polymesh testnet): init() reads the metadata, and getData returns the expected blocks with distinct.
  • Build, lint and Prettier pass for the changed files, and the changed directories' lint output is identical to main's.

Two failures already present on main will also show here:

  • The network-backed dictionary v1 tests fail because the test dictionary on gateway.subquery.network currently returns HTTP 503 ("failure to get a peer from the ring-balancer"). They fail identically on main.
  • yarn lint reports one error in packages/node-core/src/indexer/unfinalizedBlocks.service.ts (import/order), also on main. It's unrelated, so I haven't touched it here.

Summary by CodeRabbit

  • Bug Fixes
    • Fixed excessive memory growth during Dictionary v1 queries.
    • Improved handling of GraphQL errors, invalid responses, and queries that don’t support distinct.
    • Fixed extraction of metadata and block heights from dictionary results.

…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.
@coderabbitai

coderabbitai Bot commented Oct 5, 2026 •

Copy link
Copy Markdown

Review in Change Stack →

Navigate logical layers of code changes, visualize relationships, and explore their blast radius.

🧰 Additional context used
📚 Code guidelines (1)
CLAUDE.md — auto-discovered

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration
  • Configuration used: defaults
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: cf06b53d-aa58-4ae0-a3ca-d6f34759b7b3
📥 Commits

Reviewing files that changed from the base of the PR and between 51e2a2c and 7087d3e.

⛔ Files ignored due to path filters (1)
  • yarn.lock is excluded by !**/yarn.lock, !**/*.lock
📒 Files selected for processing (7)
  • packages/node-core/CHANGELOG.md
  • packages/node-core/package.json
  • packages/node-core/src/indexer/dictionary/v1/dictionaryV1.spec.ts
  • packages/node-core/src/indexer/dictionary/v1/dictionaryV1.ts
  • packages/node/CHANGELOG.md
  • packages/node/package.json
  • packages/node/src/indexer/dictionary/v1/substrateDictionaryV1.ts
💤 Files with no reviewable changes (2)
  • packages/node/package.json
  • packages/node-core/package.json

Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 9 remain after this review.


📝 Walkthrough

Walkthrough

Dictionary 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 @apollo/client.

Changes

Dictionary v1 HTTP queries

Layer / File(s) Summary
Fetch-based query method
packages/node-core/src/indexer/dictionary/v1/dictionaryV1.ts, packages/node-core/src/indexer/dictionary/v1/dictionaryV1.spec.ts, packages/node-core/package.json, packages/node-core/CHANGELOG.md
DictionaryV1 adds a protected query() method that posts query text and variables as JSON and handles invalid JSON, GraphQL errors, non-OK responses, and missing data. Metadata initialization and tests use the method. node-core removes @apollo/client.
Batch and spec-version query consumers
packages/node-core/src/indexer/dictionary/v1/dictionaryV1.ts, packages/node-core/src/indexer/dictionary/v1/dictionaryV1.spec.ts, packages/node/src/indexer/dictionary/v1/substrateDictionaryV1.ts, packages/node/package.json, packages/node/CHANGELOG.md
Batch retrieval and spec-version queries use the inherited query() method. The retry test checks a second event query without unsupported distinct. node removes @apollo/client.

Priority: ➖ Normal

Estimated code review effort: 3 (Moderate) | ~20 minutes

Change: Bug fix

Merge Risk: ⚪ Minimal · up to 7087d

No actionable issue was established in the dictionary query change; it is mergeable after normal checks.

Security Architecture Review

Security architecture risk: 🔵 Low · up to 7087d

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
No architecture-level concerns identified.

Security review details

Security Blast Radius

  • inferred — The relevant exposure remains the indexing process using a configured or registry-discovered dictionary. Control of that endpoint or its responses can influence dictionary-driven block selection and spec-version data. The inspected migration adds neither a new endpoint source nor additional tenant, credential, or service authority.

Trust Boundaries and Controls

  • observed — Both transports target the same constructor-supplied endpoint. The head explicitly sends JSON headers and a query payload without adding authentication headers or explicit credential, redirect, or retry options; the base HttpLink configuration likewise supplied only the URI. Endpoint trust remains an existing assumption, while exact implicit transport defaults remain unverified.

Resilience and Maintainability Implications

  • inferred — The unchanged timeout limits caller waiting rather than cancelling the HTTP request. A late response cannot newly commit metadata or batch results because those operations occur after the timeout resolves successfully, and query itself does not mutate dictionary state. Continued resource consumption by an abandoned request remains an existing limitation.

Hardening Proposals

  • proposed — As follow-up hardening of existing behavior, bound capability retries to a single state transition and cancel outstanding HTTP work when its deadline expires. These proposals improve failure containment; they are not findings introduced by this PR.
🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly identifies the Dictionary v1 query memory-retention fix, which is the main change.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Analyzed 0 functions across 3…
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create a new PR
  • Autopilot · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant