ls: properly release parent nodes in DFS

[Alonely0]

Fixes a regression introduced in #11386 (sorry) where DFS nodes did not properly free the recorded inodes of the parent directories. This caused critical errors in some instances of recursive hard links, and made memory usage increase with a combinatorical explosion.

[oech3]

Would you add test?

[Alonely0]

Would you add test?

I'm not even sure of how to build a minimum reproducible example lol, but I'll try. I found this by dumb luck.

[codspeed-hq]

Merging this PR will improve performance by ×15

⚡ 6 improved benchmarks
✅ 295 untouched benchmarks
⏩ 46 skipped benchmarks¹

Performance Changes

	Mode	Benchmark	BASE	HEAD	Efficiency
⚡	Memory	ls_recursive_long_all_deep_tree[(100, 4)]	126.3 KB	107.8 KB	+17.12%
⚡	Memory	ls_recursive_balanced_tree[(6, 4, 15)]	1,817.8 KB	117.3 KB	×15
⚡	Memory	ls_recursive_mixed_tree	133 KB	122.5 KB	+8.55%
⚡	Memory	ls_recursive_long_all_balanced_tree[(6, 4, 15)]	1,988.5 KB	288 KB	×6.9
⚡	Memory	ls_recursive_long_all_mixed_tree	133.6 KB	123.1 KB	+8.51%
⚡	Memory	ls_recursive_deep_tree[(200, 2)]	143.3 KB	106.9 KB	+33.98%

---

_{Comparing Alonely0:fix-ls-rec (47e2cbf) with main (319a06c)}

Footnotes

1. 46 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports. ↩

[Alonely0]

It appears this fixes this fully fixes the insane memory consumption introduced by the DFS PR lol.

[Alonely0]

Would you add test?

I've discovered I can't since this relies on hard links of directories, which are not a thing on most filesystems.

[github-actions]

GNU testsuite comparison:

GNU test failed: tests/tail/retry. tests/tail/retry is passing on 'main'. Maybe you have to rebase?
Congrats! The gnu test tests/tail/pipe-f2 is no longer failing!
Congrats! The gnu test tests/tail/tail-n0f is now passing!

[github-actions]

GNU testsuite comparison:

Skip an intermittent issue tests/tail/inotify-dir-recreate (fails in this run but passes in the 'main' branch)
Note: The gnu test tests/basenc/bounded-memory is now being skipped but was previously passing.

[oech3]

Windows has many hardlinks on WinSxS, or you can make hardlink at tests.

[Alonely0]

Pushed a quick fix of a bug I found while writing the test lol

[oech3]

Add WINDIR to spell-checker: ignore

[github-actions]

GNU testsuite comparison:

Skipping an intermittent issue tests/cut/bounded-memory (passes in this run but fails in the 'main' branch)
Skipping an intermittent issue tests/date/date-locale-hour (passes in this run but fails in the 'main' branch)
Skipping an intermittent issue tests/pr/bounded-memory (passes in this run but fails in the 'main' branch)
Skipping an intermittent issue tests/tail/follow-name (passes in this run but fails in the 'main' branch)
Skipping an intermittent issue tests/tty/tty-eof (passes in this run but fails in the 'main' branch)
Note: The gnu test tests/cp/link-heap is now being skipped but was previously passing.

[oech3]

thread 'test_ls::test_ls_recursive_hardlink_cycles' (8648) panicked at tests\uutests\src\lib\util.rs:2000:35:
called `Result::unwrap()` on an `Err` value: Custom { kind: Other, error: "wait: Timeout of '30s' reached" }

[Alonely0]

@oech3 I don't think there's a clear path to being able to reliably test this in an integration test because of how hard it is to replicate an environment like procfs or WinSxS across platforms (I have spent a couple of hours lol). However, it would not be hard at all to throw a debug assertion or a cfg(test)-gated assertion ensuring all file ancestors are released on all tests. This would catch the bug and similar variants across our test suite; I mean to add something like this at the end of list or in ListState::drop():

debug_assert!(state.listed_ancestors.is_empty());
// or similarly
#[cfg(test)]
impl Drop for ListState {
    fn drop(&mut self) {
        assert!(self.listed_ancestors.is_empty());
    }
}

Or whichever of the two-factorial solutions you find most suitable, if any.

[oech3]

debug_assert! is OK. We can improve test later if someoun found the way...

[github-actions]

GNU testsuite comparison:

Skipping an intermittent issue tests/cut/bounded-memory (passes in this run but fails in the 'main' branch)
Skipping an intermittent issue tests/date/date-locale-hour (passes in this run but fails in the 'main' branch)

[github-actions]

GNU testsuite comparison:

GNU test failed: tests/rm/isatty. tests/rm/isatty is passing on 'main'. Maybe you have to rebase?
Skipping an intermittent issue tests/date/resolution (passes in this run but fails in the 'main' branch)

[Alonely0]

@sylvestre should we add rm/isatty to intermittent?

[sylvestre]

Probably thanks

[anastygnome]

@sylvestre can we rerun this?

[anastygnome]

@Alonely0, could we make the dfs common with what we do in the tsort util?

[Alonely0]

@Alonely0, could we make the dfs common with what we do in the tsort util?

Sure thing, I'll look into it after my midterms.

[Alonely0]

@sylvestre can we rerun this?

I can just re-run it by rebasing :)

[github-actions]

GNU testsuite comparison:

Skipping an intermittent issue tests/date/date-locale-hour (passes in this run but fails in the 'main' branch)
Skipping an intermittent issue tests/pr/bounded-memory (passes in this run but fails in the 'main' branch)
Note: The gnu test tests/rm/many-dir-entries-vs-OOM is now being skipped but was previously passing.
Congrats! The gnu test tests/tail/tail-n0f is now passing!

[Alonely0]

@sylvestre sorry for the ping, is there something else you would like to see here? :) Lmk if there is anything I can do (also, the CI failure is unrelated).