Skip to content

fix: convert lint results per chunk to avoid OOM on large repos - #5018

Merged
lolgab merged 4 commits into
masterfrom
fix/oom-convert-results-per-chunk
Aug 28, 2026
Merged

fix: convert lint results per chunk to avoid OOM on large repos#5018
lolgab merged 4 commits into
masterfrom
fix/oom-convert-results-per-chunk

Conversation

@lolgab

@lolgab lolgab commented Aug 28, 2026

Copy link
Copy Markdown
Contributor

Summary

  • lintFilesChunkByChunk accumulated every chunk's ESLint.LintResult[] (each carrying the full file source text) into one array for the whole repo, then converted everything at the end. Memory grew with repo size regardless of the 5MB per-chunk cap, causing JavaScript heap out of memory crashes on large repos even after the recent --max-old-space-size bump (Increase Node.js memory limits in entrypoint.sh #5017).
  • Convert (and discard) each chunk's results immediately, keeping only lightweight ToolResult[] around for the rest of the run.

Test plan

  • CI test suite
  • npm run build (typecheck) passes locally

lintFilesChunkByChunk kept every chunk's ESLint.LintResult[] (each
carrying full file source text) in memory for the whole repo before
converting, so memory grew with repo size regardless of chunk size.
Convert and discard each chunk's results immediately instead.
@codacy-production

codacy-production Bot commented Aug 28, 2026

Copy link
Copy Markdown

Up to standards ✅

🟢 Issues 0 issues

Results:
0 new issues

View in Codacy

🟢 Metrics 0 complexity · 0 duplication

Metric Results
Complexity 0
Duplication 0

View in Codacy

AI Reviewer: first review requested successfully. AI can make mistakes. Always validate suggestions.

Run reviewer

TIP This summary will be updated as you push new changes.

@codacy-production codacy-production Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

While the PR correctly identifies the memory bottleneck caused by accumulating heavy ESLint.LintResult objects, the implementation contains a logic gap where glob patterns bypass the chunking logic entirely, likely leaving the OOM risk unresolved for many users. Additionally, the final processing steps create a secondary memory peak by mapping the full results array at once. The Codacy analysis is currently 'up to standards', but the 'MissingRequirements' coverage status and lack of new tests suggest a high risk of regression. Addressing the glob expansion and optimizing the final collection phase is recommended before merging.

About this PR

  • No unit tests or integration tests were provided to verify that the chunking logic correctly converts results or that memory is actually freed. The 'Test plan' in the PR description was also left empty.

Test suggestions

  • Verify that results from each chunk are converted to ToolResult before the next chunk starts.
  • Verify that the final output from engineImpl contains the expected relative paths for all ToolResults.
  • Verify that globs in the input files still result in converted ToolResult objects.
Prompt proposal for missing tests
Consider implementing these tests if applicable:
1. Verify that results from each chunk are converted to ToolResult before the next chunk starts.
2. Verify that the final output from engineImpl contains the expected relative paths for all ToolResults.
3. Verify that globs in the input files still result in converted ToolResult objects.

TIP Improve review quality by adding custom instructions
TIP How was this review? Give us feedback

Comment thread src/engineImpl.ts
Comment on lines +33 to +34
const results = files.some((file: string) => /\*|\?|\[/.test(file))
? convertResults(await eslint.lintFiles(files))

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔴 HIGH RISK

The OOM prevention logic is bypassed when glob patterns are detected (e.g., **/*.js). Since default configurations frequently use globs, eslint.lintFiles(files) will still load all results into memory at once, defeating the purpose of this fix. Consider expanding globs into a flat list of explicit file paths before the chunking logic to ensure consistent memory usage. You can use a library like fast-glob to resolve these patterns.

Comment thread src/engineImpl.ts
const chunkResults = await eslint.lintFiles(chunkOfFiles)
lintResults.push.apply(lintResults, chunkResults)
filesLinted += chunkResults.length
results.push(...convertResults(chunkResults))

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 MEDIUM RISK

Suggestion: Using the spread operator (...) with results.push can trigger a RangeError: Maximum call stack size exceeded in Node.js if a chunk contains a very large number of linting issues (typically > 65k). A loop is safer for merging large result sets:

Suggested change
results.push(...convertResults(chunkResults))
convertResults(chunkResults).forEach((r) => results.push(r))

Comment thread src/engineImpl.ts

debug("engine: finished")
return convertResults(lintResults).map((r) => r.relativeTo(srcDirPath))
return results.map((r) => r.relativeTo(srcDirPath))

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 MEDIUM RISK

Suggestion: Calling .map() on the full results array at the end doubles the memory required for the results list right before the function returns. To minimize memory pressure, consider mapping to relative paths earlier within the chunking loop so the final array only contains the finished objects.

lolgab added 3 commits August 28, 2026 14:45
…are memory-bounded too

Codacy review correctly flagged that the glob branch bypassed chunking
entirely — since the default files-to-analyze list ("**/*.ts", etc.)
is globs, most repos with no explicit codacyrc.files never hit the
chunked path at all, leaving the OOM risk unresolved for the common
case. Expand globs up front (excluding node_modules/dist/build/.git)
and always run the chunked, per-chunk-converted lint path.
Addresses reviewer request for test coverage on the new chunking path.
The glob-expansion path added extra risk (mixed explicit+glob file
lists could drop explicitly-requested files under dist/build-named
folders) for a case that doesn't occur in normal usage — Codacy always
passes an explicit file list, filtering is done upstream. Reverting to
just the per-chunk result conversion, which is the actual OOM fix.
@lolgab
lolgab enabled auto-merge August 28, 2026 12:52
@lolgab
lolgab merged commit 0dd201f into master Aug 28, 2026
5 checks passed
@lolgab
lolgab deleted the fix/oom-convert-results-per-chunk branch August 28, 2026 13:15
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.

2 participants