Skip to content

Conversation

@pralkarz
Copy link
Contributor

@pralkarz pralkarz commented Mar 19, 2025

Fixes #135.

The current behavior makes it so that when we're crawling "/" with some symlinks and resolvePath: false, state.root is an empty string in

while (parent !== state.root && depth < 2) {
, hence we get stuck in an infinite loop.

Tests pass locally both on Linux (WSL) and Windows.

@43081j
Copy link
Contributor

43081j commented Mar 24, 2025

there's actually a bug here i think. you would've uncovered it if your test asserted anything

if you crawl / with a symlink, it seems sync will return the entry twice and withPromise (async) will return it once

this seems to be because we're checking the visited set of nodes once we resolve the symlink, to avoid visiting it twice. in sync mode, we can visit the symlink before the target file has been visited and so visited won't contain it. in async mode, we resolve the symlink after already visiting non-symlink nodes, so visited is already populated and we return early.

i suspect this is a bug and is because we do the visited check when resolving a symlink , but not when visiting the target file. so we end up adding both

edit going to open a standalone issue for this

@pralkarz
Copy link
Contributor Author

pralkarz commented Mar 24, 2025

there's actually a bug here i think. you would've uncovered it if your test asserted anything

if you crawl / with a symlink, it seems sync will return the entry twice and withPromise (async) will return it once

this seems to be because we're checking the visited set of nodes once we resolve the symlink, to avoid visiting it twice. in sync mode, we can visit the symlink before the target file has been visited and so visited won't contain it. in async mode, we resolve the symlink after already visiting non-symlink nodes, so visited is already populated and we return early.

i suspect this is a bug and is because we do the visited check when resolving a symlink , but not when visiting the target file. so we end up adding both

edit going to open a standalone issue for this

Hmm, do you have any steps to reproduce this? I checked locally, and with a minimally mocked filesystem, both async and sync return [ '/usr/lib/file-1', '/lib/file-1' ]. Similarly, when using the full mock, files.length is the same in both cases.

@benmccann
Copy link
Contributor

More details here: #138

@thecodrr thecodrr changed the title fix: don't slice the root when it's the root directory Fix hang when crawling root with direct symlinks Mar 25, 2025
@thecodrr thecodrr merged commit b2ee512 into thecodrr:master Mar 25, 2025
6 of 9 checks passed
@SuperchupuDev
Copy link
Contributor

looks like this made windows ci hang 🤔

@pralkarz pralkarz deleted the iss135 branch March 25, 2025 07:38
@pralkarz pralkarz restored the iss135 branch March 25, 2025 07:39
@pralkarz pralkarz deleted the iss135 branch March 25, 2025 08:13
thecodrr pushed a commit that referenced this pull request Apr 8, 2025
* fix: add a failing test

* fix: don't slice the root when it's the root directory

* fix: test both `sync` and `withPromise` properly

* chore: revert formatting changes

* fix: handle Windows root directories correctly

* chore: revert the formatting change

* chore: revert the formatting change

* chore: re-add the whitespace after the comma

* test: add an assertion to the test

* chore: revert the formatting change (sorry)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

withSymlinks({ resolvePaths: false }) hang when crawling /

5 participants