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

"wp-scripts build" returns status 0 / zero, even if the build fails #41712

Closed
sweber83 opened this issue Jun 14, 2022 · 14 comments · Fixed by #42396
Closed

"wp-scripts build" returns status 0 / zero, even if the build fails #41712

sweber83 opened this issue Jun 14, 2022 · 14 comments · Fixed by #42396
Assignees
Labels
Good First Issue An issue that's suitable for someone looking to contribute for the first time [Package] Scripts /packages/scripts [Type] Bug An existing feature does not function as intended

Comments

@sweber83
Copy link

sweber83 commented Jun 14, 2022

Description

We use wp-scripts build to create the webpack build in a CI pipeline.
The CI pipeline relies on the return code to determine the job status.
If the build fails with the "JavaScript heap out of memory" error, wp-scripts build returns status 0.
This causes the next pipeline stage to run, which leads to deployment of a corrupted build artifact.

Step-by-step reproduction instructions

In a project, which uses wp-scripts, run the following command:
NODE_OPTIONS="--max_old_space_size=64" yarn wp-scripts build
You may need to adjust the max_old_space_size for the "JavaScript heap out of memory" error to appear.
If you then echo $?, you get 0, when it should be > 0.

Screenshots, screen recording, code snippet

No response

Environment info

@wordpress/scripts: 23.2.0

Please confirm that you have searched existing issues in the repo.

Yes

Please confirm that you have tested with all plugins deactivated except Gutenberg.

Yes

@gziolo gziolo added [Package] Scripts /packages/scripts Needs Testing Needs further testing to be confirmed. labels Jun 15, 2022
@amustaque97
Copy link
Member

I would love to take a look at this issue.

@gziolo
Copy link
Member

gziolo commented Jul 11, 2022

@amustaque97, were you able to reproduce the issue and figure out what's causing it?

@amustaque97
Copy link
Member

amustaque97 commented Jul 12, 2022

@amustaque97, were you able to reproduce the issue and figure out what's causing it?

@gziolo, not yet. I will update this issue with my findings later today 🙂

@t-hamano
Copy link
Contributor

I was able to reproduce this error on my test repository.

Indeed, a memory leak does occur, but it still passes through the pipeline.
But I believe this is an error caused by the Node core and not a wp-scripts issue.

How about increasing the memory usage to prevent memory leaks?

{
  "scripts": {
    "build": "cross-env NODE_OPTIONS=\"--max-old-space-size=4096\" wp-scripts build index.js"
  },
  "devDependencies": {
    "@wordpress/scripts": "^23.4.0",
    "cross-env": "^7.0.3"
  }
}

@tw-360vier
Copy link

@t-hamano
The problem is not that it fails when it runs out of memory but that it does so with a zero exit code.

Because this means that no one notices that it failed and a following deployment step would deploy a broken build.

Increasing the memory does not solve that problem.

@t-hamano
Copy link
Contributor

Do you mean that there is a way for npm scripts to catch memory leaks caused by node and return an arbitrary status code?

@tw-360vier
Copy link

First off, let's keep the terminology straight:

The problem is not "memory leaks" (bugs that cause memory to be allocated but not freed up again)
but "JavaScript heap out of memory" situations where nodejs can't allocate any more memory.

In this situation i want the process to exit with an exit code that indicates a problem (exit code > 0).
With other scripts, this is possible. For example if i try to build a small example with webpack instead of wp-scripts, i can get a helpful exit code.

{
  "scripts": {
    "build:wp-scripts": "wp-scripts build",
    "build:webpack": "cross-env WP_SRC_DIRECTORY=\"./src\" webpack build"
  },
  "dependencies": {
    "@wordpress/scripts": "^23.4.0",
    "cross-env": "^7.0.3"
  }
}
// webpack.config.js
module.exports = require('@wordpress/scripts/config/webpack.config.js')
// src/index.js
console.log('this should end up in our bundle')

Now, running both these command yields pretty much the same result.

But when i provoke a "JavaScript heap out of memory" situation (Which can happen during a normal CI build after any change to my git repository since we don't know beforehand how much memory the build will need. So there is no point in just increasing "max-old-space-size" to some arbitrary amount)
there is a significant difference:

$ NODE_OPTIONS="--max-old-space-size=32" yarn build:webpack; echo $? 
yarn run v1.22.18
warning package.json: No license field
$ cross-env WP_SRC_DIRECTORY="./src" webpack build

<--- Last few GCs --->

[1513248:0x27c7960]      848 ms: Mark-sweep (reduce) 29.5 (36.8) -> 29.2 (33.1) MB, 12.3 / 0.0 ms  (+ 0.0 ms in 0 steps since start of marking, biggest step 0.0 ms, walltime since start of marking 13 ms) (average mu = 0.583, current mu = 0.187) allocation[1513248:0x27c7960]      867 ms: Mark-sweep (reduce) 30.9 (33.8) -> 30.0 (34.0) MB, 2.8 / 0.0 ms  (+ 1.2 ms in 11 steps since start of marking, biggest step 0.3 ms, walltime since start of marking 15 ms) (average mu = 0.678, current mu = 0.798) finalize i

<--- JS stacktrace --->

FATAL ERROR: Reached heap limit Allocation failed - JavaScript heap out of memory
 1: 0xa673d8 node::Abort() [webpack]
 2: 0x95bb2e  [webpack]
 3: 0xc73b32 v8::Utils::ReportOOMFailure(v8::internal::Isolate*, char const*, bool) [webpack]
 4: 0xc73ec7 v8::internal::V8::FatalProcessOutOfMemory(v8::internal::Isolate*, char const*, bool) [webpack]
 5: 0xe31465  [webpack]
 6: 0xe46c62  [webpack]
 7: 0xe46c8c v8::internal::Heap::CollectAllGarbage(int, v8::internal::GarbageCollectionReason, v8::GCCallbackFlags) [webpack]
 8: 0xdd7060 v8::internal::StackGuard::HandleInterrupts() [webpack]
 9: 0x119fc7a v8::internal::Runtime_StackGuard(int, unsigned long*, v8::internal::Isolate*) [webpack]
10: 0x1597fd9  [webpack]
error Command failed with exit code 1.
info Visit https://yarnpkg.com/en/docs/cli/run for documentation about this command.
1

Building with webpack failed with exit code 1, indicating that something went wrong.
My CI pipeline would stop here and notify me with an e-mail.

$ NODE_OPTIONS="--max-old-space-size=32" yarn build:wp-scripts; echo $? 
yarn run v1.22.18
warning package.json: No license field
$ wp-scripts build

<--- Last few GCs --->
t of marking 14 ms) (average mu = 0.835, current mu = 0.812) finalize i[1512968:0x4116860]      804 ms: Mark-sweep (reduce) 30.3 (33.6) -> 29.5 (33.1) MB, 11.4 / 0.0 ms  (+ 0.0 ms in 0 steps since start of marking, biggest step 0.0 ms, walltime since start of marking 12 ms) (average mu = 0.644, current mu = 0.253) allocation[1512968:0x4116860]      827 ms: Mark-sweep (reduce) 30.9 (33.3) -> 30.0 (33.8) MB, 11.5 / 0.0 ms  (average mu = 0.573, current mu = 0.502) allocation failure scavenge might not succeed


<--- JS stacktrace --->

FATAL ERROR: Reached heap limit Allocation failed - JavaScript heap out of memory
 1: 0xa673d8 node::Abort() [webpack]
 2: 0x95bb2e  [webpack]
 3: 0xc73b32 v8::Utils::ReportOOMFailure(v8::internal::Isolate*, char const*, bool) [webpack]
 4: 0xc73ec7 v8::internal::V8::FatalProcessOutOfMemory(v8::internal::Isolate*, char const*, bool) [webpack]
 5: 0xe31465  [webpack]
 6: 0xe46c62  [webpack]
 7: 0xe48d49 v8::internal::Heap::AllocateRawWithLightRetrySlowPath(int, v8::internal::AllocationType, v8::internal::AllocationOrigin, v8::internal::AllocationAlignment) [webpack]
 8: 0xe48dd7 v8::internal::Heap::AllocateRawWithRetryOrFailSlowPath(int, v8::internal::AllocationType, v8::internal::AllocationOrigin, v8::internal::AllocationAlignment) [webpack]
 9: 0xe0a0a0 v8::internal::Factory::AllocateRaw(int, v8::internal::AllocationType, v8::internal::AllocationAlignment) [webpack]
10: 0xe02664 v8::internal::FactoryBase<v8::internal::Factory>::AllocateRawWithImmortalMap(int, v8::internal::AllocationType, v8::internal::Map, v8::internal::AllocationAlignment) [webpack]
11: 0xe0448f v8::internal::FactoryBase<v8::internal::Factory>::NewRawOneByteString(int, v8::internal::AllocationType) [webpack]
12: 0xe0c3ed v8::internal::Factory::NewStringFromUtf8(v8::base::Vector<char const> const&, v8::internal::AllocationType) [webpack]
13: 0xc863dd v8::String::NewFromUtf8(v8::Isolate*, char const*, v8::NewStringType, int) [webpack]
14: 0xb6dfb8 node::StringBytes::Encode(v8::Isolate*, char const*, unsigned long, node::encoding, v8::Local<v8::Value>*) [webpack]
15: 0xa413c3  [webpack]
16: 0xcd45ce v8::internal::FunctionCallbackArguments::Call(v8::internal::CallHandlerInfo) [webpack]
17: 0xcd4f2f  [webpack]
18: 0xcd542f v8::internal::Builtin_HandleApiCall(int, unsigned long*, v8::internal::Isolate*) [webpack]
19: 0x15980b9  [webpack]
Done in 1.71s.
0

Building with wp-scripts also failed, but return exit code 0, indicating that everything was okay.
My CI pipeline would go on deploying whatever was the result of this half-finished build.

And since webpack fails with a proper exit code, i would expect wp-scripts to be able to do the same.

@t-hamano
Copy link
Contributor

Thank you for your detailed explanation.
If webpack returns the correct error code, it means that wp-scripts could be able to return the same code.

@gziolo
If you have a good idea, please let us know 🙏

@gziolo
Copy link
Member

gziolo commented Jul 13, 2022

I have no idea, but clearly, the status code returned from the spawn call is incorrect:

const { status } = spawn( resolveBin( 'webpack' ), getWebpackArgs(), {
stdio: 'inherit',
} );
process.exit( status );

@amustaque97
Copy link
Member

I'm not able to reproduce the issue. I'm getting the exit status code as 1. Is it platform-specific? @t-hamano

Let me try it in the Ubuntu docker instance once.

Here is the screenshot for reference.
Screenshot 2022-07-13 at 6 51 27 PM

@t-hamano
Copy link
Contributor

Okay, my machine is Windows, so I will try it with Windows OS and WSL2 (Ubuntu).

@amustaque97
Copy link
Member

I found the issue. In case of failure status value is null. We need to handle it conditionally.

You can read more about it here: https://nodejs.org/api/child_process.html#child_processspawnsynccommand-args-options

Check the return values in the above link.

const { status } = spawn( resolveBin( 'webpack' ), getWebpackArgs(), {
stdio: 'inherit',
} );
process.exit( status );

Attaching a screenshot for reference.
Screenshot 2022-07-13 at 7 17 22 PM

@amustaque97 amustaque97 added Good First Issue An issue that's suitable for someone looking to contribute for the first time and removed Needs Testing Needs further testing to be confirmed. labels Jul 13, 2022
@amustaque97 amustaque97 added the [Type] Bug An existing feature does not function as intended label Jul 13, 2022
@amustaque97 amustaque97 self-assigned this Jul 13, 2022
@github-actions github-actions bot added the [Status] In Progress Tracking issues with work in progress label Jul 13, 2022
@gziolo
Copy link
Member

gziolo commented Jul 14, 2022

#42396 should fix this issue. We missed the window for npm publishing that happened yesterday, so it should be available on npm in 2 weeks.

@amustaque97 amustaque97 removed the [Status] In Progress Tracking issues with work in progress label Jul 14, 2022
@tw-360vier
Copy link

Thank you very much for your help!
Soon i will be sleeping better knowing our pipelines will fail when they should. 💤

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Good First Issue An issue that's suitable for someone looking to contribute for the first time [Package] Scripts /packages/scripts [Type] Bug An existing feature does not function as intended
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants