fix: prevent infinite recursion when looking up real paths

[BenMcLean]

This fixes https://github.com/npm/cli/issues/7309 which causes a "RangeError: Maximum call stack size exceeded" on line 38 on Windows 11 without this change.

References

@danFbach found the fix.

[wraithgar]

The == is probably going to fail linting (should be ===) and this will need a test.

[danFbach]

My npm just updated and undid my local patch. this is literally a one line change, can we get this PR merged?

[danFbach]

@wraithgar Does the subject of this pr just need to be changed to pass the lint test?

[wraithgar]

It's a little concerning that this change passes all test coverage without the need for a new test. This means one of two things:

• In tests we are hitting this new use-case, but not ending up in this infinite loop.
• In tests we are hitting this new use case, but it is already part of the infinite loop test.

Either situation means we've fixed this bug by accident, and not on purpose. At the very least we'll need a new test showing explicitly the issue this fix is addressing, and hopefully also be able to explain how the new code path in this PR is being hit in tests now w/o making the tests fail.

It's also telling that the error experienced is a maximum call stack error. There is already a guard in this file that's supposed to kick in before we get to that point. It's possible answering the question(s) above will help solve that mystery too.

This fix is a good start, but we need to be able to show in tests and/or comments why it works.

[danFbach]

@wraithgar

It's also telling that the error experienced is a maximum call stack error. There is already a guard in this file that's supposed to kick in before we get to that point. It's possible answering the question(s) above will help solve that mystery too.

when i was debugging, I was also puzzled by the fact that it never threw the eloop exception after 2k iterations as you should exceed that long before the maximum call stack is exceeded.

I can add the context that npm update does not throw the error, only npm update -g - and that my machine is using folder redirection. Just now, i added a local windows account (no folder redirection) and was able to run both npm update and npm update -g without issue.

I would agree that a test is missing or an existing test is flawed, which should currently be failing. Given what i've just tested, I'm guessing it is lacking test(s) related to folder redirection.

Let me know if i can help further. Perhaps, point me to where the unit tests are stored in this repo and I'll try to review them in my free time and see if there's anything that can be added or improved.

[wraithgar]

There are no unit tests for this util in isolation, we find that unit tests often invite dead code paths and don't usually test the code as it is used.

The two libraries that consume this helper are lib/arborist/load-actual.js and lib/arborist/build-ideal-tree.js Their test suites are in test/arborist/load-actual.js and test/arborist/build-ideal-tree.js respectively.

Unfortunately the tests in arborist are not the easiest to jump into without quite a bit of context. You're welcome to dive in but don't feel bad if you can't make a lot of headway.

If you can use isolated examples, including what values realpath.js is seeing in that loop, that may be enough for us to figure out what's going on and either add a good comment to that loop, or add a test.

[danFbach]

If you can use isolated examples, including what values realpath.js is seeing in that loop, that may be enough for us to figure out what's going on and either add a good comment to that loop, or add a test.

check the initial issue. #7309

i logged:

console.log(depth, dir, path, dir == path, base);

result:

undefined \\SOURCE2\Users\danF\AppData\Roaming \\SOURCE2\Users\danF\AppData\Roaming\npm false npm
NaN \\SOURCE2\Users\danF\AppData \\SOURCE2\Users\danF\AppData\Roaming false Roaming
NaN \\SOURCE2\Users\danF \\SOURCE2\Users\danF\AppData false AppData
NaN \\SOURCE2\Users\ \\SOURCE2\Users\danF false danF
NaN \\SOURCE2\Users\ \\SOURCE2\Users\ true Users
NaN \\SOURCE2\Users\ \\SOURCE2\Users\ true Users
NaN \\SOURCE2\Users\ \\SOURCE2\Users\ true Users
NaN \\SOURCE2\Users\ \\SOURCE2\Users\ true Users
NaN \\SOURCE2\Users\ \\SOURCE2\Users\ true Users
...repeats until max call stack exception

happy to log something else

[danFbach]

@wraithgar any movement on this?