Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: increase maxBuffer of spawnSync #729

Merged
merged 7 commits into from
Apr 22, 2022

Conversation

gabrielrussoc
Copy link
Contributor

@gabrielrussoc gabrielrussoc commented Apr 21, 2022

nodeJS limits the size of the stdout of child processes to 1MB: https://nodejs.org/api/child_process.html#child_processspawnsynccommand-args-options

The output of git ls-files exceeds 1MB in my case, falling back to using glob. However the glob call hardcodes a list of files that doesn't respect .gitignore and causes weird behaviour.

I'm setting 100MB as an arbitrary large value. There is some discussion about the default value of this argument here: nodejs/node#9829

Motivated by #704

Tests

I tried to find tests for this behaviour but it looks like there are none. I manually ran the uploader on a large repository with more than 1MB output:

$ git ls-files > ls_files.txt
$ du -sh ls_files.txt
6.8M	ls_files.txt

Before:

./uploader ...
...
<--- Last few GCs --->

[22463:0x7fe065602300]   168398 ms: Mark-sweep 4052.5 (4135.6) -> 4042.5 (4141.9) MB, 2926.0 / 0.0 ms  (average mu = 0.584, current mu = 0.255) allocation failure scavenge might not succeed
[22463:0x7fe065602300]   172823 ms: Mark-sweep 4060.3 (4143.6) -> 4049.4 (4148.6) MB, 4104.4 / 0.0 ms  (average mu = 0.367, current mu = 0.072) allocation failure scavenge might not succeed


<--- JS stacktrace --->

FATAL ERROR: Reached heap limit Allocation failed - JavaScript heap out of memory

After:

./uploader ...
...
[2022-04-21T20:58:15.604Z] ['info'] Uploading...
[2022-04-21T20:58:17.644Z] ['info'] {"status":"success","resultURL":""}

@gabrielrussoc gabrielrussoc requested a review from a team as a code owner April 21, 2022 20:51
@drazisil-codecov
Copy link
Contributor

@gabrielrussoc It looks like @thomasrockhu-codecov may have fixed this in the latest version with #716

Do you mind pulling down 0.2.0 and testing?

@gabrielrussoc
Copy link
Contributor Author

I can confirm the issue still reproes on 0.2.0, looks like #716 missed this call to spawnSync

@gabrielrussoc
Copy link
Contributor Author

Here's a public repro:

On the linux repository: https://github.com/torvalds/linux (has more than 1MB files)

$ ln -s . INFINITE_LOOP_TO_BREAK_CODECOV

$ ./codecov-linux

The repo is too large, git ls-files fails, it falls back to glob, glob doesn't ignore the circular symlink, memory explodes

@drazisil-codecov
Copy link
Contributor

drazisil-codecov commented Apr 22, 2022

glob doesn't ignore the circular symlink

@mitchell-codecov Any thoughts?

@gabrielrussoc is there a setting to tell glob to ignore symlinks? I can't think of a use case where we would want to, seems that's almost guaranteed to not match a report. 🤔

@gabrielrussoc
Copy link
Contributor Author

I believe this is used to discover the file network and not to look for reports

update to 100mb
add missing usage
@drazisil-codecov
Copy link
Contributor

I believe this is used to discover the file network and not to look for reports

Yes, but the network is needed to match against file paths in the reports. It's not documented, but that is its purpose.

@drazisil-codecov
Copy link
Contributor

drazisil-codecov commented Apr 22, 2022

@gabrielrussoc I haven't got the hang of editing PRs from forks from the command line, so I sent your branch a PR :D

@drazisil-codecov drazisil-codecov merged commit 540fcc4 into codecov:main Apr 22, 2022
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