fix: memoize legacyNodeResolve resolver to avoid native memory leak#481
fix: memoize legacyNodeResolve resolver to avoid native memory leak#481B4nan wants to merge 4 commits into
Conversation
`legacyNodeResolve` constructs a fresh `unrs-resolver` `ResolverFactory` on every import resolution via `createNodeResolver`. `unrs-resolver` is a Rust binding that allocates native memory invisible to V8's heap limits, so per-call construction causes RSS to grow without bound over a lint run. We hit this on a large lerna monorepo: a single ESLint worker peaked at ~19 GB RSS before the OOM killer fired. Bumping `--max-old-space-size` did not help because the leak is in native memory, not the JS heap. The fix memoizes resolver instances by their JSON-stringified options. In practice the options object is constant across calls within a single lint run, so the cache is effectively a singleton. Verified locally: with this patch + `--concurrency=auto` + 8 GB heap, peak RSS drops from OOM-territory to ~20 GB and the lint completes cleanly.
|
📝 WalkthroughWalkthroughAdded memoization for node resolvers in Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
|
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. |
Up to standards ✅🟢 Issues
|
| Metric | Results |
|---|---|
| Complexity | 11 |
| Duplication | 0 |
TIP This summary will be updated as you push new changes. Give us feedback
There was a problem hiding this comment.
The change LGTM after all.
But if you have faced the performance issue with the legacy node resolver, maybe you really should migrate to the next version of the resolver and give that a shot? With the newer version of resolver, we have already implemented instance reuse.
You can find more information about the next version of resolver here: https://github.com/un-ts/eslint-plugin-import-x/releases/tag/v4.6.0 and #192
|
Thanks for the review! Switched the cache key to And great pointer on I still think this PR is worth landing for the legacy path though: anyone still on Happy to close if you'd rather keep the legacy path stagnant as a nudge for people to migrate — just let me know. |
…stance `eslint-plugin-import-x`'s legacy `node` resolver path (the default fall-through when `import-x/resolver-next` is not configured) constructs a fresh `unrs-resolver.ResolverFactory` on every import resolution. `unrs-resolver` is a Rust binding that allocates native memory invisible to V8 heap limits, so per-call construction causes worker RSS to grow without bound over a lint run. On a large lerna monorepo (apify/apify-core, ~60 packages, thousands of files), this reliably OOM-killed CI workers within 2 minutes. `import-x` v4.6.0+ already has proper instance reuse on the new resolver path — you just have to opt in by passing a single `createNodeResolver()` instance via `import-x/resolver-next`. See: https://github.com/un-ts/eslint-plugin-import-x/releases/tag/v4.6.0 un-ts/eslint-plugin-import-x#481 (review) Set it in the shared config so every consumer gets the fix automatically and nobody has to rediscover it via an OOM.
…stance (#42) ## Summary \`eslint-plugin-import-x\`'s legacy \`node\` resolver path — the default fall-through when \`import-x/resolver-next\` is not configured — constructs a fresh \`unrs-resolver.ResolverFactory\` on every import resolution. \`unrs-resolver\` is a Rust binding that allocates native memory invisible to V8 heap limits, so per-call construction causes worker RSS to grow without bound over a lint run. On a large lerna monorepo (apify/apify-core, ~60 packages, thousands of files), this reliably OOM-killed CI workers within 2 minutes, regardless of how much JS heap we gave them. Bumping \`--max-old-space-size\` didn't help because the leak is in native memory. ## Fix \`eslint-plugin-import-x\` v4.6.0+ already has proper instance reuse — but only on the new resolver path. You opt in by passing a single reusable \`createNodeResolver()\` instance via \`import-x/resolver-next\`. Since our shared config is the obvious chokepoint, set it here so every consumer gets the fix automatically and nobody has to rediscover the leak via an OOM. ## Context - un-ts/eslint-plugin-import-x#481 (the leak in the legacy path, and the review feedback pointing to this as the proper fix) - https://github.com/un-ts/eslint-plugin-import-x/releases/tag/v4.6.0 (release notes where instance reuse landed) - un-ts/eslint-plugin-import-x#192 (the original upstream PR introducing the new resolver path) ## Verification Tested on apify-core by configuring the same thing locally in its \`eslint.config.mjs\`. \`npx eslint ./src/packages --concurrency=auto\` with 8 GB heap now completes cleanly where it previously OOM-killed the worker every time.
commit: |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/utils/resolve.ts (1)
128-156: Add a small regression test around cache reuse.This fix is centralized in a tiny helper, so it would be easy to regress later. A focused test that verifies two fresh-but-equivalent option objects reuse one resolver, while different option signatures allocate separate resolvers, would lock the behavior in without needing the large-scale OOM repro in CI.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/utils/resolve.ts` around lines 128 - 156, Add a focused regression test for getCachedNodeResolver/cachedNodeResolvers: create two distinct-but-equivalent option objects and assert getCachedNodeResolver(optsA) === getCachedNodeResolver(optsB) (same resolver instance), then assert that a call with a different option signature returns a different instance; do this by calling getCachedNodeResolver (or spying/mocking createNodeResolver) to verify reuse and distinct allocation behavior for different stableHash keys; reference getCachedNodeResolver, cachedNodeResolvers, createNodeResolver, and stableHash when locating the code to test.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/utils/resolve.ts`:
- Around line 128-156: Add a focused regression test for
getCachedNodeResolver/cachedNodeResolvers: create two distinct-but-equivalent
option objects and assert getCachedNodeResolver(optsA) ===
getCachedNodeResolver(optsB) (same resolver instance), then assert that a call
with a different option signature returns a different instance; do this by
calling getCachedNodeResolver (or spying/mocking createNodeResolver) to verify
reuse and distinct allocation behavior for different stableHash keys; reference
getCachedNodeResolver, cachedNodeResolvers, createNodeResolver, and stableHash
when locating the code to test.
The memoized `unrs-resolver` `ResolverFactory` instances cache filesystem lookups internally. When test files are renamed, the stale internal cache causes resolution failures. This adds a `clearCache()` method on the `NodeResolver` type (delegating to `ResolverFactory.clearCache()`) and exports `clearCachedNodeResolvers()` from utils to flush both the resolver map and each resolver's internal FS cache. The rename cache correctness tests now call this after the file rename. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
🧹 Nitpick comments (1)
test/utils/resolve.spec.ts (1)
525-532: Optional: co-locate rename and cache-clear in one hook for stronger ordering.Current behavior is fine, but merging these two
beforeAllhooks reduces future order-coupling risk if more hooks are inserted later.♻️ Suggested refactor
- beforeAll(() => - fs.promises.rename(testFilePath(original), testFilePath(changed)), - ) - - // The memoized unrs-resolver instances cache FS lookups internally. - // After a rename we must clear them so the resolver sees the new state. - beforeAll(() => clearCachedNodeResolvers()) + beforeAll(async () => { + await fs.promises.rename(testFilePath(original), testFilePath(changed)) + // The memoized unrs-resolver instances cache FS lookups internally. + // After a rename we must clear them so the resolver sees the new state. + clearCachedNodeResolvers() + })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/utils/resolve.spec.ts` around lines 525 - 532, Co-locate the filesystem rename and resolver cache clear into a single beforeAll hook to guarantee ordering: replace the two separate beforeAll(...) calls with one beforeAll that first calls fs.promises.rename(testFilePath(original), testFilePath(changed)) and then calls clearCachedNodeResolvers(), so the rename completes before the memoized unrs-resolver instances are cleared; refer to testFilePath(...) and clearCachedNodeResolvers() to locate the operations to combine.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@test/utils/resolve.spec.ts`:
- Around line 525-532: Co-locate the filesystem rename and resolver cache clear
into a single beforeAll hook to guarantee ordering: replace the two separate
beforeAll(...) calls with one beforeAll that first calls
fs.promises.rename(testFilePath(original), testFilePath(changed)) and then calls
clearCachedNodeResolvers(), so the rename completes before the memoized
unrs-resolver instances are cleared; refer to testFilePath(...) and
clearCachedNodeResolvers() to locate the operations to combine.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 62b3f996-352d-4499-97d6-74baeee84fae
📒 Files selected for processing (3)
src/node-resolver.tssrc/utils/resolve.tstest/utils/resolve.spec.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/utils/resolve.ts
| // The memoized unrs-resolver instances cache FS lookups internally. | ||
| // After a rename we must clear them so the resolver sees the new state. | ||
| beforeAll(() => clearCachedNodeResolvers()) | ||
|
|
There was a problem hiding this comment.
I don't get it. Previously, we didn't need to clear the cache, and the tests finished fine. What's going on here, why do we need to clear the cache now?
There was a problem hiding this comment.
Before this PR, legacyNodeResolve constructed a brand new ResolverFactory on every call, so each call got a fresh internal FS cache. After memoizing, the same ResolverFactory is reused — and unrs-resolver keeps its own internal Rust-side cache of FS lookups that survives across calls. The wrapper fileExistsCache has a TTL, but the resolver's internal cache doesn't, so once the wrapper misses and asks the resolver, the resolver still returns its stale answer.
The test sequence is:
- resolve
./CaseyKasem.js→ resolver caches "this file exists" - rename the file on disk
- resolve
./CASEYKASEM2.js→ before: new resolver, sees the rename. after: same cached resolver, internal cache says it doesn't exist → test fails
In a real lint run files don't get renamed mid-run, so this only matters in tests. clearCachedNodeResolvers() is just the test-side equivalent of "pretend this is a fresh process."
2 participants
Problem
legacyNodeResolveinsrc/utils/resolve.tsconstructs a freshunrs-resolverResolverFactoryviacreateNodeResolveron every import resolution.unrs-resolveris a Rust binding that allocates native memory which is not visible to V8's heap limits, so per-call construction causes RSS to grow without bound over the course of a lint run.We hit this on a large lerna monorepo (apify/apify-core, ~60 workspace packages, thousands of files). A single ESLint worker peaked at >19 GB RSS before being OOM-killed by the runner. Bumping
--max-old-space-sizedid not help — the leak is in native memory, not the JS heap.The dominant time-spender shown by
TIMING=allisimport-x/no-cycle, but the memory hot-spot is the resolver. Disablingno-cycleentirely reduced peak RSS by only ~2.5 GB (from ~12.5 GB to ~10 GB single-worker) — confirming the leak is in the per-call resolver allocation, not the cycle traversal.Fix
Memoize
unrs-resolverinstances by their option signature.In practice the resolver options object is constant across calls within a single lint run, so the cache is effectively a singleton — but JSON-stringifying the options is cheap enough to handle the multi-resolver-config case correctly, and doesn't risk a stale cache if options ever vary.
Verification
Local repro on apify/apify-core's
src/packages(~60 workspace packages):So this restores the ability to use
--concurrency=autowith the default 8 GB heap on a typical CI runner.Notes
legacyNodeResolveis the path taken when noimport-x/resolver-nextis configured and the resolver name is inLEGACY_NODE_RESOLVERS. That covers the defaultnoderesolver, which is what most consumers use, so this fix benefits the common case.Map<string, Resolver>keyed byJSON.stringify(opts). AWeakMapkeyed by the options object would be slightly more efficient but doesn't work because callers often pass freshly-constructed options objects rather than reusing the same reference.Summary by CodeRabbit
Refactor
Tests