test(deps-installer): isolate the heavy deepRecursive test in its own process#12388
Merged
Conversation
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughThis PR refactors the test execution strategy for ChangesTest Execution Strategy Refactoring
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested reviewers
Poem
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint install timed out. The project may have too many dependencies for the sandbox. 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 |
zkochan
force-pushed
the
raise-deps-installer-test-heap
branch
from
June 13, 2026 18:16
6d159d4 to
e0bfe30
Compare
zkochan
changed the title
… process test/install/deepRecursive.ts resolves @teambit/bit's enormous circular and peer-dependency graph. Measured in a CI-faithful container (Linux, Node 22.13.0, amd64, default ~4 GB heap) it peaks at ~3.6 GB — it fits the default heap on its own, but not with the memory the other deps-installer test files leave behind in the same jest process (the --experimental-vm-modules module registry is not reclaimed between files). That overflow is the "FATAL ERROR: Reached heap limit" the CI suite hit. Run deepRecursive in a dedicated jest process (.test:heavy) so it gets the whole default heap to itself, and run the rest (.test:rest) in a separate process with it excluded via a negative-lookahead path pattern. The two runs cover every test file exactly once. This makes the earlier OOM workarounds unnecessary, so they are reverted: - the 5-way sharding of the suite (deepRecursive was the sole culprit; the remaining files are a subset of what historically ran in one process), and - the workerIdleMemoryLimit in the with-registry jest preset. No global heap bump: every run stays within Node's default ~4 GB, matching the budget pnpm has in production.
zkochan
force-pushed
the
raise-deps-installer-test-heap
branch
from
June 13, 2026 19:31
e0bfe30 to
c365e6a
Compare
zkochan
marked this pull request as ready for review
June 13, 2026 20:21
Code Review by Qodo
1. Worker recycling removed
|
PR Summary by QodoIsolate deps-installer deepRecursive test into its own Jest process to avoid CI OOM WalkthroughsDescription• Split deps-installer tests into heavy vs rest Jest runs to avoid heap accumulation OOM. • Remove prior OOM workarounds (test sharding and workerIdleMemoryLimit) as unnecessary. • Keep Node default heap budget by isolating the high-memory deepRecursive registry resolution test. Diagramgraph TD
A["CI: deps-installer suite"] --> B["pn .test"] --> H["Test env (NODE_OPTIONS + registry port)"] --> C[".test:heavy (jest)"] --> D["deepRecursive.ts"]
H --> E[".test:rest (jest)"] --> F["all other tests"]
C --> G["with-registry preset"]
E --> G
High-Level AssessmentThe following are alternative approaches to this PR: 1. Increase Node heap size globally
2. Keep worker recycling via workerIdleMemoryLimit
3. Continue sharding the suite (e.g., 5-way)
Recommendation: Prefer the PR’s approach: run the known high-memory deepRecursive test in a dedicated Jest process and run the rest separately. It directly targets the root cause (heap accumulation across files under --experimental-vm-modules) while preserving Node’s default heap budget and removing prior, broader workarounds (sharding and worker recycling) that add CI/config complexity. File ChangesTests (1)
Other (2)
|
![qodo-free-for-open-source-projects[bot]](https://avatars.githubusercontent.com/in/1068000?s=60&v=4){kind=small}
Comment on lines
7
to
12
| // Unfortunately, this means that if two such tests will run at the same time, | ||
| // they may break each other. | ||
| maxWorkers: 1, | ||
| // Recycle the test worker once its heap crosses this limit. Under | ||
| // `--experimental-vm-modules` Jest's VM module registry is never released | ||
| // between test files, so a long-running process climbs to Node's ~4 GB | ||
| // old-space ceiling and dies with an out-of-memory FATAL ERROR. Setting a | ||
| // limit is also what keeps `maxWorkers: 1` from collapsing to in-band | ||
| // execution (see `shouldRunInBand`): a recyclable worker is spawned instead, | ||
| // still serial, so the leaked memory is reclaimed between files without | ||
| // reintroducing the dist-tag races `maxWorkers: 1` prevents. | ||
| workerIdleMemoryLimit: '1500MB', | ||
| // Force Jest to exit after globalTeardown completes. The Verdaccio server | ||
| // and lifecycle child-processes spawned during tests may leave ref'd handles | ||
| // that prevent the process from exiting on its own. |
There was a problem hiding this comment.
1. Worker recycling removed 🐞 Bug ☼ Reliability
@pnpm/jest-config/with-registry no longer configures workerIdleMemoryLimit, so with `maxWorkers: 1 and --experimental-vm-modules` the same long-lived Jest process retains memory across many test files and can hit Node’s heap limit. This affects every package using the with-registry preset, not just @pnpm/installing.deps-installer.
Agent Prompt
### Issue description
`__utils__/jest-config/with-registry/jest-preset.js` removed `workerIdleMemoryLimit`, which was the only built-in mechanism to recycle the Jest worker process and reclaim heap between test files when running under `--experimental-vm-modules`.
### Issue Context
- The repo’s test scripts commonly run Jest with `--experimental-vm-modules`.
- The PR itself documents that under this mode, Jest’s VM module registry is not reclaimed between files.
- The `with-registry` preset is used across many packages, so this is a cross-suite reliability regression.
### Fix Focus Areas
- __utils__/jest-config/with-registry/jest-preset.js[4-16]
- .meta-updater/src/index.ts[368-386]
### Implementation notes
- Re-add `workerIdleMemoryLimit` to the `with-registry` preset (potentially keeping the previous value) so long suites don’t accumulate unreclaimed VM-module memory across files.
- If the intent is to avoid recycling for specific suites, consider making it configurable via env var (default on), rather than removing it globally.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
2 tasks
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
1 participant
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.
Problem
@pnpm/installing.deps-installerOOMs in CI (FATAL ERROR: Reached heap limit Allocation failed) in the shard that runstest/install/deepRecursive.ts, which resolves@teambit/[email protected]— an enormous circular + peer-dependency graph — from the live registry.Root cause (reproduced in a CI-faithful container)
I couldn't reproduce this on macOS/Node-24 (it uses less heap there, and my default heap is actually smaller than CI's). Reproduced it in a
linux/amd64+node:22.13.0container (CI's exact platform, default heap 4144 MB, matching CI):deepRecursivepeak RSSSo
deepRecursiveneeds ~3.6 GB on CI — it fits the default 4 GB heap on its own (passes), but not with the memory the other ~14 test files in its shard leave behind in the same jest process (jest's--experimental-vm-modulesregistry isn't reclaimed between files). That overflow is the OOM. There is no recent code regression — peak memory is unchanged across the last 30 commits; the live graph simply grew untildeepRecursive+ accumulation crossed 4 GB.Fix
Run
deepRecursivein a dedicated jest process (.test:heavy) so it gets the whole default heap to itself, and run the rest (.test:rest) in a separate process with it excluded (negative-lookahead path pattern). The two runs cover every test file exactly once (verified: 1 + 75 = 76).Crucially, no
--max-old-space-sizebump — every run stays within Node's default ~4 GB, matching the budget pnpm has in production.deepRecursivealone passing at the default heap was confirmed in the container.Because
deepRecursivewas the sole culprit, the two earlier OOM workarounds are reverted as unnecessary:workerIdleMemoryLimitin thewith-registryjest preset.Caveat
deepRecursive's ~3.6 GB leaves ~0.5 GB of margin under the 4 GB default. Since it resolves a live, growing graph, if it later exceeds 4 GB on its own that's a real pnpm problem (it would OOM in production too), to be fixed by reducing resolver memory — not by raising the limit.Written by an agent (Claude Code, claude-opus-4-8).
Summary by CodeRabbit