subprocess: replace fs::directory_iterator with readdir #32529

1. 
   laanwj commented at 12:56 pm on May 16, 2025: member

   Alternate implementation of fd iteration for Linux.

   It looks like std::filesystem::directory_iterator, introduced in #32343, has an intermittent issue that causes a hang of the CI (#32524). i have not been able to reproduce this locally, and the CI situation makes it difficult to get more details about the problem.

   So to work around that, this PR replaces the use of directory_iterator with the equivalent POSIX opendir/readdir//closedir usage. It is only slightly more verbose. In all testing up to now this appears to make the problem go away.

2. laanwj added the label Tests on May 16, 2025
3. laanwj added the label Utils/log/libs on May 16, 2025
4. 
   DrahtBot commented at 12:56 pm on May 16, 2025: contributor

   The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

   Code Coverage & Benchmarks

   For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/32529.

   Reviews

   See the guideline for information on the review process. A summary of reviews will appear here.

   Conflicts

   No conflicts as of last run.

5. laanwj force-pushed on May 16, 2025
6. 
   laanwj commented at 1:09 pm on May 16, 2025: member

   ARM run failure is interesting:

   0fatal error: in "system_tests/run_command": subprocess::CalledProcessError: /proc/<pid>/fd iteration failed: No such file or directory (2)
   

   (proabably /proc isn’t mounted there, but why wasn’t it erroring with the previous implementation)

   Edit: i think this was an implementation problem, according to the documentation on the horrors of POSIX, errno needs to be set to zero explicitly to be able to detect errors during readdir.

7. DrahtBot added the label CI failed on May 16, 2025
8. 
   DrahtBot commented at 4:00 pm on May 16, 2025: contributor

   🚧 At least one of the CI tasks failed. Task no wallet, libbitcoinkernel: https://github.com/bitcoin/bitcoin/runs/42360298658 LLM reason (✨ experimental): The CI failure is due to the “system_tests” failing.

   Try to run the tests locally, according to the documentation. However, a CI failure may still happen due to a number of reasons, for example:

   • Possibly due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.

   • A sanitizer issue, which can only be found by compiling with the sanitizer and running the affected test.

   • An intermittent issue.

   Leave a comment here, if you need help tracking down a confusing failure.

9. laanwj force-pushed on May 16, 2025
10. laanwj force-pushed on May 16, 2025
11. DrahtBot removed the label CI failed on May 17, 2025
12. 
    maflcko commented at 10:40 am on May 17, 2025: member

    I haven’t looked at the code, but the CI passed 50 times: https://cirrus-ci.com/task/6662743735926784
13. 
    laanwj commented at 1:23 pm on May 17, 2025: member

    I haven’t looked at the code, but the CI passed 50 times: https://cirrus-ci.com/task/6662743735926784

    Okay. So unless the original code is using directory_iterator wrong (which i don’t think so), we can be reasonably sure there’s an intermittent bug in the implementation of it on at least some libc++ versions.

    i’ll clean up the code ~monday and open it for review.

14. subprocess: replace `fs::directory_iterator` with `readdir` 7177fbed0d
15. laanwj force-pushed on May 18, 2025
16. laanwj renamed this:
    [DONOTMERGE] subprocess: replace `fs::directory_iterator` with `readdir`
    subprocess: replace `fs::directory_iterator` with `readdir`
    on May 19, 2025
17. laanwj added the label Linux/Unix on May 19, 2025
18. laanwj closed this on May 19, 2025

    ---

19. 
    laanwj commented at 10:50 am on May 19, 2025: member

    This isn’t necessary if it’s worked around some other way #32524 (comment)

Contributors
laanwj DrahtBot maflcko

Labels
Linux/Unix Tests Utils/log/libs