8000 nodejs v22.10.0 onward give errors: void node::fs::InternalModuleStat(const v8::FunctionCallbackInfo<v8::Value>&) · Issue #1180 · shelljs/shelljs · GitHub
[go: up one dir, main page]
Skip to content

nodejs v22.10.0 onward give errors: void node::fs::InternalModuleStat(const v8::FunctionCallbackInfo<v8::Value>&) #1180

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.

Sign up for GitHub

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

Jump to bottom
Closed
kmashint opened this issue Feb 14, 2025 · 10 comments · Fixed by #1218 or #1219
Closed

nodejs v22.10.0 onward give errors: void node::fs::InternalModuleStat(const v8::FunctionCallbackInfo<v8::Value>&) #1180

kmashint opened this issue Feb 14, 2025 · 10 comments · Fixed by #1218 or #1219
Labels
fix Bug/defect, or a fix for such a problem test

Comments

@kmashint
Copy link
Contributor
kmashint commented Feb 14, 2025 •
edited
Loading

When creating a PR for an improvement to carry forward the exit code from a failed sync exec() to the thrown Error, CI tests in the cloud were failing for nodejs v22.31.1 on Ubuntu, OSX, and Windows.
#1179

Sample errors are below:

// From Ubuntu:
TypeError [ERR_INVALID_ARG_TYPE]: The "code" argument must be of type number. Received type string ('128SIGABRT')
    at process.set [as exitCode] (node:internal/bootstrap/node:123:9)
    at ChildProcess.<anonymous> (/home/runner/work/shelljs/shelljs/node_modules/foreground-child/index.js:63:22)

// From Windows:
  #  C:\WINDOWS\system32\cmd.exe [8364]: void __cdecl node::fs::InternalModuleStat(const class v8::FunctionCallbackInfo<class v8::Value> &) at c:\ws\src\node_file.cc:1040
  #  Assertion failed: (args.Length()) >= (2)

Some research showed that:

  1. The problems started with nodejs v22.10.0. v22.9.0 ran the shelljs tests successfully.
  2. There seems to be a new incompatibility between the newer nodejs signals and forground-child@2.0.0, where forground-child@3.3.0 adjusts its handling (see below).
  3. spawn-wrap and signal-exit may also be related, but the most obvious problem is the foreground-child and 128SIGABRT, although there it be some deeper error.
  4. The nyc dependency which has a new version 17.1.0 that requires newer foreground-child@3.3.0. But underlying that is spawn-wrap@2.0.0 that has no newer version and uses the problematic foreground-child@2.0.0. I tried forcing some version overrides in package.json but it was still broken. And with nyc 17.1.0, the test runs were breaking on older nodejs versions as well, so there's some incompatibility there.
// forground-child@2.0.0 near index.js:63
  child.on('close', (code, signal) => {
    // Allow the callback to inspect the child’s exit code and/or modify it.
    process.exitCode = signal ? 128 + signal : code

// https://github.com/tapjs/foreground-child/blob/v3.3.0/src/index.ts#L152
  const childHangup = () => {
    try {
      child.kill('SIGHUP')

      /* c8 ignore start */
    } catch (_) {
      // SIGHUP is weird on windows
      child.kill('SIGTERM')
    }
    /* c8 ignore stop */
  }
@kmashint kmashint changed the title nodejs v22.10.0 onward give errors on exec() and/or ChildProcess related tests nodejs v22.10.0 onward give errors on exec(), ChildProcess related tests Feb 14, 2025
@nfischer
Copy link
Member

Thanks for the report. I'm going to see if I can reproduce the error locally.

@nfischer
Copy link
Member

Hmm I get a different error when I run this locally:

$ nvm use v22.10.0 --delete-prefix
$ npm install
$ npm run test

> shelljs@0.8.3 test
> ava test/*.js


  #  node[14062]: void node::fs::InternalModuleStat(const v8::FunctionCallbackInfo<v8::Value>&) at ../src/node_file.cc:1037
  #  Assertion failed: (args.Length()) >= (2)

----- Native stack trace -----

 1: 0xf76517 node::Assert(node::AssertionInfo const&) [node]
 2: 0xf7dd17  [node]
 3: 0x7f972140f5e2

----- JavaScript stack trace -----

1: [PATH_REDACTED]/shelljs/node_modules/esm/esm.js:1:34535
2: [PATH_REDACTED]/shelljs/node_modules/esm/esm.js:1:34176
3: [PATH_REDACTED]/shelljs/node_modules/esm/esm.js:1:34506
4: [PATH_REDACTED]/shelljs/node_modules/esm/esm.js:1:173374
5: [PATH_REDACTED]/shelljs/node_modules/esm/esm.js:1:173420
6: [PATH_REDACTED]/shelljs/node_modules/esm/esm.js:1:173521
7: [PATH_REDACTED]/shelljs/node_modules/esm/esm.js:1:258942
8: [PATH_REDACTED]/shelljs/node_modules/esm/esm.js:1:261569
9: e ([PATH_REDACTED]/shelljs/node_modules/esm/esm.js:1:262673)
10: get ([PATH_REDACTED]/shelljs/node_modules/esm/esm.js:1:262740)


Aborted (core dumped)

@nfischer
Copy link
Member

Oh I see that you pointed out Windows and Ubuntu have different errors. I'm running on Ubuntu, but I see the error you found on the Windows bot.

Not sure what the 128SIGABRT string is about. I guess we need to solve the node::fs::InternalModuleStat assertion error first.

@nfischer
Copy link
Member

When I run this on v22.9.0, I also get failed tests but with a different error.

$ npm run test

> shelljs@0.8.3 test
> ava test/*.js

(node:20194) [DEP0040] DeprecationWarning: The `punycode` module is deprecated. Please use a userland alternative instead.
(Use `node --trace-deprecation ...` to show where the warning was created)

⠦ which › basic usage

  2 tests failed
  6 tests skipped

  pushd › quiet mode off

  [PATH_REDACTED]/shelljs/test/resources/pushd/test/pushd.js:343

  Difference:

  - `[PATH_REDACTED]/shelljs/test/resources/pushd [PATH_REDACTED]/shelljs␊
  + `[PATH_REDACTED]/shelljs/test/resources/pushd [PATH_REDACTED]/shelljs␊
    `



  popd › quiet mode off

  [PATH_REDACTED]/shelljs/test/popd.js:130

   129:     t.is(stdout, '');
   130:     t.is(stderr, `${rootDir}\n`);
   131:     t.is(process.cwd(), trail[0]);

  Difference:

  - `[PATH_REDACTED]/shelljs␊
  + `[PATH_REDACTED]/shelljs␊
    `

@nfischer
Copy link
Member

This is the upstream issue: standard-things/esm#931

This is unfortunately going to be very difficult for me to solve. ava depends on esm. The only way I see to solve this is to upgrade ava to a very high version, but this is going to require rewriting a huge amount of code to get my tests compatible with the new ava version.

I'll accept community contributions to upgrade ava and rewrite all of the tests. I just don't have time for this right now. Best I can do is upgrade ava to 2.0, but that's not enough to fix this. https://github.com/shelljs/shelljs/pull/1181/files

nfischer added a commit that referenced this issue Feb 14, 2025
@nfischer
chore: pin node v22.9.0
37f7fa0 
There's a bug when running on node v22.10. This temporarily pins 22 to
22.9.0 to workaround the problem and keep tests passing on CI.

Mitigation for issue #1180.
@nfischer nfischer mentioned this issue Feb 14, 2025
Merged
@nfischer nfischer added the fix Bug/defect, or a fix for such a problem label Feb 14, 2025
nfischer added a commit that referenced this issue Feb 14, 2025
@nfischer
chore: pin node v22.9.0
93909ce 
There's a bug when running on node v22.10. This temporarily pins 22 to
22.9.0 to workaround the problem and keep tests passing on CI.

Mitigation for issue #1180.
@nfischer nfischer changed the title nodejs v22.10.0 onward give errors on exec(), ChildProcess related tests nodejs v22.10.0 onward give errors: void node::fs::InternalModuleStat(const v8::FunctionCallbackInfo<v8::Value>&) Feb 14, 2025
@kmashint
Copy link
Contributor Author
kmashint commented Feb 14, 2025 •
edited
Loading

Yes, I actually saw something similar on Windows with node v22.9.0 about punycode, pushd, popd, but that wasn't as drastic as the crash in 22.10.0 and beyond. The 128SIGINT came from the cloud build on Ubuntu (unsure of the version) with node v22.13.1 (I think).

kmashint pushed a commit to kmashint/shelljs that referenced this issue Feb 15, 2025
@nfischer @kmashint
chore: pin node v22.9.0
8e6cb9d 
There's a bug when running on node v22.10. This temporarily pins 22 to
22.9.0 to workaround the problem and keep tests passing on CI.

Mitigation for issue shelljs#1180.
nfischer added a commit that referenced this issue Feb 16, 2025
@nfischer
chore: pin node v22.9.0
81ed122 
There's a bug when running on node v22.10. This temporarily pins 22 to
22.9.0 to workaround the problem and keep tests passing on CI.

Mitigation for issue #1180.
kmashint pushed a commit to kmashint/shelljs that referenced this issue Feb 16, 2025
@nfischer @kmashint
chore: pin node v22.9.0
5e97491 
There's a bug when running on node v22.10. This temporarily pins 22 to
22.9.0 to workaround the problem and keep tests passing on CI.

Mitigation for issue shelljs#1180.
This was referenced Mar 19, 2025
Closed
Closed
@Wassap124
Copy link

@nfischer so if 0.8.5 does not require esm,
but 0.9.x does,

that should be a breaking change, and communicated as such, no?

maybe 0.9.0 should've been 1.0.0

@kmashint
Copy link
Contributor Author
kmashint commented Mar 20, 2025 •
edited
Loading

@Wassap124 For semver at 0.minor.patch, major zero, the minor version changes allow for breaking changes.

@nfischer
Copy link
Member

@Wassap124

so if 0.8.5 does not require esm, but 0.9.x does

Are you referring to shelljs? Just to be clear, I believe that shelljs v0.9.0 does NOT require ESM. I believe you should be able to use shelljs v0.9.0 from within either commonjs modules OR ecmascript modules, exactly the same way that shelljs v0.8 worked. If that doesn't match up with what you're seeing on your end, please file a new GitHub issue so that I can investigate with high priority.

What I was discussing above was that the ava project requires ESM now, and if we want to update ava then I need to rewrite shelljs. But to be clear, I did not do that rewrite and I did not update ava yet.

For semver at 0.minor.patch, major zero, the minor version changes allow for breaking changes.

@kmashint is correct. The upgrade from shelljs v0.8.0 to v0.9.0 does in fact contain some breaking changes, and I have versioned the package appropriately to indicate this. If you want to see the breaking changes, take a look through https://github.com/shelljs/shelljs/wiki/Migrating-from-v0.8-to-v0.9 (and if I missed anything, please feel free to update the wiki).

@nfischer nfischer added the test label Mar 21, 2025
@Wassap124
Copy link
Wassap124 commented Mar 23, 2025 •
edited
Loading

@kmashint it was only a suggestion, but at least notify about a breaking change in the changelog

@nfischer thanks for the clarification!

If that doesn't match up with what you're seeing on your end, please file a new GitHub issue so that I can investigate with high priority.

it indeed does not, I opened an issue with my current experience,
apologies for cluttering this issue

nfischer added a commit that referenced this issue Apr 10, 2025
@nfischer
chore: update ava
b8b07c1 
This updates ava to v6. This resolves some `npm audit` warnings in our
project's devDependencies.

This is a partial fix #1180, although I found more errors when I ran the
tests locally on v22.10.0.

Test: npm audit
@nfischer nfischer mentioned this issue Apr 10, 2025
Merged
nfischer added a commit that referenced this issue Apr 10, 2025
@nfischer
chore: update ava
5e3d331 
This updates ava to v6. This resolves some `npm audit` warnings in our
project's devDependencies. This adds `workerThreads: false` because
operations like `process.chdir()` don't work in workers (but are
required for shelljs).

This is a partial fix #1180, although I found more errors when I ran the
tests locally on v22.10.0.

Test: npm audit
nfischer added a commit that referenced this issue Apr 10, 2025
@nfischer
fix: cmd is compatible with node 22.10
1d9485f 
Starting in node >= 22.10.0, a commandNotFound error will have
`undefined` values for stdout/stderr, whereas in earlier versions these
values were `null`.

Fixes #1180
@nfischer nfischer mentioned this issue Apr 10, 2025
Merged
nfischer added a commit that referenced this issue Apr 10, 2025
@nfischer
fix: cmd is compatible with node 22.10
75d26de 
Starting in node >= 22.10.0, a commandNotFound error will have
`undefined` values for stdout/stderr, whereas in earlier versions these
values were `null`.

Fixes #1180
kmashint pushed a commit to kmashint/shelljs that referenced this issue Apr 10, 2025
@nfischer @kmashint
chore: update ava
5088aea 
This updates ava to v6. This resolves some `npm audit` warnings in our
project's devDependencies. This adds `workerThreads: false` because
operations like `process.chdir()` don't work in workers (but are
required for shelljs).

This is a partial fix shelljs#1180, although I found more errors when I ran the
tests locally on v22.10.0.

Test: npm audit
kmashint pushed a commit to kmashint/shelljs that referenced this issue Apr 10, 2025
@nfischer @kmashint
fix: cmd is compatible with node 22.10
2b70555 
Starting in node >= 22.10.0, a commandNotFound error will have
`undefined` values for stdout/stderr, whereas in earlier versions these
values were `null`.

Fixes shelljs#1180
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
fix Bug/defect, or a fix for such a problem test
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

chore: update ava shelljs/shelljs
fix: cmd is compatible with node 22.10 shelljs/shelljs
3 participants
@kmashint @nfischer @Wassap124
0