Node internal crash node::fs::InternalModuleStat starting from Node 22.10

[arthur-fontaine]

I use this library through import-sync, but I can't import this library anymore.

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

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

 1: 0xfb795c node::Assert(node::AssertionInfo const&) [node]
 2: 0xfc0a74  [node]
 3: 0x1f209b8  [node]

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

1: /workspaces/barebones-nodejs/@httptoolkit/esm/esm.js:2375:36
2: /workspaces/barebones-nodejs/@httptoolkit/esm/esm.js:11144:39
3: /workspaces/barebones-nodejs/@httptoolkit/esm/esm.js:11147:27
4: /workspaces/barebones-nodejs/@httptoolkit/esm/esm.js:11160:21
5: /workspaces/barebones-nodejs/@httptoolkit/esm/esm.js:11222:28
6: /workspaces/barebones-nodejs/@httptoolkit/esm/esm.js:11243:33
7: /workspaces/barebones-nodejs/@httptoolkit/esm/esm.js:11515:30
8: /workspaces/barebones-nodejs/@httptoolkit/esm/esm.js:11512:21
9: onceWrapper (node:events:622:26)
10: emit (node:events:507:28)


Aborted

I debugged it and found it comes from this exact line.

esm/src/fs/stat-fast.js

Line 70 in aa8723a

? binding.fs.internalModuleStat(toNamespacedPath(thePath))

I checked what should be the second argument (as told by the error)

https://github.com/nodejs/node/blob/9b6cea6ebe1942e24dc096b1bae53ecfacb781b2/lib/internal/modules/cjs/loader.js#L239C55-L239C72

The patch should be

+ ? binding.fs.internalModuleStat(binding.fs, toNamespacedPath(thePath))
- ? binding.fs.internalModuleStat(toNamespacedPath(thePath))

But the binding.fs parameter should ONLY be added if the version is greater or equal to 22.10. Maybe we should try to find the commit in Node to ensure that there is no others conditions.

You can find a reproduction at https://github.com/arthur-fontaine/htk-esm-internal-crash-repro. It doesn't use the @httptoolkit/esm library, but I copied its code and tried to reduce it to localize the bug. (look for the "HERE IS THE BUG" comment in esm/loader.js)

EDIT:

I think this is the commit where internalModuleStat started to require 2 parameters nodejs/node@f5d454a.

EDIT2:

The PR on Node that added the second parameter on internalModuleStat: nodejs/node#54408

EDIT3:

Checking if Node is version 22.10 or greater seems to be the only condition. Then the patch should look like

+ const nodeVersion = process.version.slice(1).split('.'); // Ex. "22.10.0" -> ["22", "10", "0"]
+ const major = parseInt(nodeVersion[0], 10);
+ const minor = parseInt(nodeVersion[1], 10);

const result = typeof thePath === "string"
-    ? binding.fs.internalModuleStat(toNamespacedPath(thePath))
+    ? (major > 22 || (major === 22 && minor >= 10))
+        ? binding.fs.internalModuleStat(binding.fs, toNamespacedPath(thePath))
+        : binding.fs.internalModuleStat(toNamespacedPath(thePath))
    : -1;

[nktnet1]

@pimterry the wallaby repo from upstream has fixed this exact issue - could you sync the fork?

• master...wallabyjs:esm:master

and thanks @arthur-fontaine for the investigation 👍

[pimterry]

Sure, now done @nktnet1, I've pulled that commit it. Can you give it a quick test? If it's all working for you I can ship a 3.3.1 release shortly.

[nktnet1]

I've tested it by first publishing with yalc using node v16, to get around the envelop issue in node v18+ you mentioned here: #2 (comment))

# @httptoolkit/esm repo
fnm use 16
npx --yes yalc publish --private

then switching back to node v22 to install:

# import-sync repo
fnm use 22
npx yalc add @httptoolkit/esm

One test that I have, which passes up until node v20,

https://github.com/nktnet1/import-sync/blob/7d6f64d309eab65081c16c462547f863c0cdd504/tests/basic/basic.test.ts#L12-L14

now fails on node v22:

$ npx yalc add @httptoolkit/esm
Package @httptoolkit/esm@3.3.0 added ==> /home/nktnet/personal/projects/import-sync/node_modules/@httptoolkit/esm

$ import-sync(main)$ npm t                        

> import-sync@2.2.2 test
> jest

(node:41109) [DEP0040] DeprecationWarning: The `punycode` module is deprecated. Please use a userland alternative instead.
(Use `node --trace-deprecation ...` to show where the warning was created)
(node:41109) [DEP0179] DeprecationWarning: crypto.Hash constructor is deprecated.
(node:41109) ExperimentalWarning: CommonJS module /home/nktnet/personal/projects/import-sync/src/import.ts is loading ES Module /home/nktnet/personal/projects/import-sync/tests/basic/basic.mjs using require().
Support for loading ES Module in require() is an experimental feature and might change at any time
 FAIL  tests/basic/basic.test.ts
  ● ESM import shallow object

    expect(received).toStrictEqual(expected) // deep equality

    Expected: {"key1": "value1", "key2": 2}
    Received: serializes to the same string

      11 |
      12 | test('ESM import shallow object', () => {
    > 13 |   expect(basic.object).toStrictEqual({ key1: 'value1', key2: 2 });
         |                        ^
      14 | });
      15 |
      16 | test('ESM import function', () => {

      at Object.<anonymous> (tests/basic/basic.test.ts:13:24)

 PASS  tests/resolver/resolver.test.ts
 PASS  tests/esmlibs/esmlibs.test.ts
 PASS  tests/edge/edge.test.ts
 PASS  tests/path/path.test.ts

Test Suites: 1 failed, 4 passed, 5 total
Tests:       1 failed, 19 passed, 20 total
Snapshots:   0 total
Time:        1.515 s, estimated 2 s
Ran all test suites.

Still, before the change, import-sync did not run on node v22 at all, so this is an improvement. This issue is most likely a Jest-related one too (relating to this article) so I wouldn't worry about it (will investigate another time).

Thanks @pimterry!

[pimterry]

Cool, thanks @nktnet1! Now published as 3.3.1.