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.

[nktnet1]

Hey @pimterry,

Looks like Node 24.0.0 changed it back to 1 arg.

You can see the fix upstream here:

• wallabyjs@0d676bf

[pimterry]

Thanks for the ping @nktnet1. I've just cherry-picked that change in (along with two other upstream commits that looked useful) and pushed that here. Can you test this again in your setup?

I've tried getting things set up myself but I hit Cannot destructure property 'dir' of 'shared.package' as it is undefined in import-sync, along with failures in all the tests here in Node v16 or modern releases. In general this is a bit of a mess now (e.g. the webpack v4 issue) and given recent Node changes I'm really not inclined to do any more than the bare minimum for this package. As of the end of April Node v18 is now end-of-life, which means all supported Node versions now support sync require of ESM out of the box, and I think this package is going to be formally deprecated in the not too distant future.

[nktnet1]

Hi @pimterry,

I've tested the changes - seems to be working fine on my end, thank you!

Make sense about deprecating the module - I'm happy to fork and maintain it afterwards for a little while if you no longer have interest in it. This is mainly because ESM support, even in node v24, is still only out as an RC:

But I do see the light at the end of the tunnel :).

Thanks for all the contributions to this thus far!

[pimterry]

Great, thanks! Published as v3.3.2.