qa: Drop recursive deletes from test code, add lint checks.

davidgumberg commented at 3:59 am on January 29, 2026: contributor

Both fs::remove_all and shutil::rmtree() are a bit dangerous, and most of their uses are not necessary, this PR removes most instances of both.

remove_all() is still used in in src/test/util/setup_common.cpp as part of BasicTestingSetup::BasicTestingSetup’s constructor and destructor, and it is used in the kernel test code’s TestDirectory:

https://github.com/bitcoin/bitcoin/blob/734899a4c404f24379d0597744465c147d9ad024/src/test/kernel/test_kernel.cpp#L100-L112

In both cases, remove_all is likely necessary, but the kernel’s test code is RAII, ideally BasicTestingSetup could be made similar in a follow-up or in this PR if reviewers think it is important.

Similarly in the python code, most usage was unnecessary, but there are a few places where rmtree() was necessary, I have added sanity checks to make sure these are inside of the tmpdir before doing recursive delete there.

DrahtBot added the label Tests on Jan 29, 2026

DrahtBot commented at 4:00 am on January 29, 2026: contributor

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

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	hodlinator, sedited, achow101
Concept ACK	l0rinc
Approach ACK	w0xlt

If your review is incorrectly listed, please copy-paste <!–meta-tag:bot-skip–> into the comment that the bot should ignore.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#34372 (QA: wallet_migration: Test several more weird scenarios by luke-jr)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

davidgumberg force-pushed on Jan 29, 2026

DrahtBot added the label CI failed on Jan 29, 2026

DrahtBot commented at 4:05 am on January 29, 2026: contributor

🚧 At least one of the CI tasks failed. Task lint: https://github.com/bitcoin/bitcoin/actions/runs/21465095038/job/61825339241 LLM reason (✨ experimental): Lint check failed due to Ruff error: unused import shutil in test/functional/test_framework/test_node.py.

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.

DrahtBot removed the label CI failed on Jan 29, 2026

l0rinc commented at 10:38 am on January 29, 2026: contributor

Sounds reasonable, light concept ACK

w0xlt commented at 6:19 pm on January 29, 2026: contributor

Approach ACK

sedited approved

sedited commented at 10:47 am on March 8, 2026: contributor

lgtm

The changes to the cache directory logic in the last two commits were not that straight forward to understand for me. Can you elaborate on them a bit in the commit messages?

DrahtBot added the label Needs rebase on Mar 9, 2026

davidgumberg force-pushed on Mar 10, 2026

davidgumberg commented at 11:33 pm on March 10, 2026: contributor

The changes to the cache directory logic in the last two commits were not that straight forward to understand for me. Can you elaborate on them a bit in the commit messages?

Thanks for the feedback, I’ve (hopefully) improved the commit message for the one where I drop the --keepcache argument, and I’ve moved changes that I accidentally included in the last commit in the series back to their true home in that commit.

DrahtBot removed the label Needs rebase on Mar 10, 2026

DrahtBot added the label CI failed on Mar 11, 2026

davidgumberg force-pushed on Mar 12, 2026

DrahtBot removed the label CI failed on Mar 12, 2026

DrahtBot added the label Needs rebase on Mar 13, 2026

qa: Remove all instances of `remove_all` except test cleanup

Adds a lint check for `remove_all()`

`fs::remove_all()`/`std::filesystem::remove_all()` is extremely
dangerous, all user-facing instances of it have been removed, and it
also deserves to be removed from the places in our test code where it is
being used unnecessarily.

a7e4a59d6d

test: functional: drop unused --keepcache argument

At the time this was added in  #10197, building the test cache took 21
seconds, as described in that PR, but this is no longer true, as
demonstrated by running the functional test framework with and without
the --keepcache arguments on master prior to this commit:

```
hyperfine   --warmup 1  --export-markdown results.md --runs 3 \
    -n 'without --keepcache'    './build/test/functional/test_runner.py -j $(nproc)' \
    -n 'with --keepcache'       './build/test/functional/test_runner.py -j $(nproc) --keepcache'
```

| Command               | Mean [s]       | Min [s] | Max [s] | Relative |
|:----------------------|---------------:|--------:|---:|---:|
| `without --keepcache` | 76.373 ± 3.058 | 74.083  | 79.846  | 1.00 |
| `with --keepcache`    | 77.384 ± 1.836 | 75.952  | 79.454  | 1.01 ± 0.05 |

As a consequence, this argument can be removed from the test runner and
this also has the benefit of being able to use an RAII-like
`tempfile.TemporaryDirectory` instead of having to clean up the cache
manually at the end of test runs.

bitcoin/bitcoin#10197: https://github.com/bitcoin/bitcoin/pull/10197

8bfb422de8

test: functional: drop rmtree usage and add lint check

`shutil.rmtree` is dangerous because it recursively deletes. There are
not likely to be any issues with it's current uses, but it is possible
that some of the assumptions being made now won't always be true, e.g.
about what some of the variables being passed to `rmtree` represent.

For some remaining uses of rmtree that can't be avoided for now, use
`cleanup_dir` which asserts that the recursively deleted folder is a
child of the the `tmpdir` of the test run. Otherwise,
`tempfile.TemporaryDirectory` should be used which does it's own
deleting on being garbage collected, or old fashioned unlinking and
rmdir in the case of directories with known contents.

0d1301b47a

davidgumberg force-pushed on Mar 24, 2026

davidgumberg commented at 11:06 pm on March 24, 2026: contributor

Rebased to address merge conflict.

DrahtBot removed the label Needs rebase on Mar 25, 2026

in test/functional/test_framework/test_node.py:253 in 0d1301b47a

245@@ -248,9 +246,6 @@ def __del__(self):
246             # this destructor is called.
247             print(self._node_msg("Cleaning up leftover process"), file=sys.stderr)
248             self.process.kill()
249-        if self.ipc_tmp_dir:
250-            print(self._node_msg(f"Cleaning up ipc directory {str(self.ipc_tmp_dir)!r}"))
251-            shutil.rmtree(self.ipc_tmp_dir)

hodlinator commented at 9:31 am on March 25, 2026:

remark: This just being straight up removed had me concerned but then I read the docs for https://docs.python.org/3/library/tempfile.html#tempfile.TemporaryDirectory and compared to mkdtemp on the same page.

Similar case in test/functional/test_runner.py.

in test/lint/test_runner/src/lint_cpp.rs:130 in 0d1301b47a

121@@ -122,6 +122,32 @@ fs:: namespace, which has unsafe filesystem functions marked as deleted.
122     }
123 }
124 
125+pub fn lint_remove_all() -> LintResult {
126+    let found = git()
127+        .args([
128+            "grep",
129+            "--line-number",
130+            "remove_all(.*)",

hodlinator commented at 9:52 am on March 25, 2026:

Fails to catch:

0    uintmax_t (*sneaky)(const std::filesystem::path&) = &fs::remove_all;
1    sneaky("asd");

But it’s probably too contrived anyways. Could possibly change to this for a wider search:

0            "\bremove_all\b",

No strong opinion.

hodlinator approved

hodlinator commented at 11:22 am on March 25, 2026: contributor

ACK 0d1301b47a35f1ff04c30d104f774b5f8f0c6995

Strongly agree with us being more careful when deleting files (similar vein as recently merged #34776).

Verified linters found violations.

Ran benchmark on master to verify removal of --keepcache is fine:

 0₿ hyperfine   --warmup 1  --export-markdown results.md --runs 3     -n 'without --keepcache'    './build/test/functional/test_runner.py -j $(nproc)'     -n 'with --keepcache'       './build/test/functional/test_runner.py -j $(nproc) --keepcache'
 1Benchmark 1: without --keepcache
 2  Time (mean ± σ):     177.599 s ±  1.659 s    [User: 890.268 s, System: 92.331 s]
 3  Range (min … max):   176.573 s … 179.513 s    3 runs
 4 
 5  Warning: The first benchmarking run for this command was significantly slower than the rest (179.513 s). This could be caused by (filesystem) caches that were not filled until after the first run. You are already using the '--warmup' option which helps to fill these caches before the actual benchmark. You can either try to increase the warmup count further or re-run this benchmark on a quiet system in case it was a random outlier. Alternatively, consider using the '--prepare' option to clear the caches before each timing run.
 6 
 7Benchmark 2: with --keepcache
 8  Time (mean ± σ):     179.919 s ±  7.237 s    [User: 894.839 s, System: 91.573 s]
 9  Range (min … max):   175.105 s … 188.242 s    3 runs
10 
11Summary
12  without --keepcache ran
13    1.01 ± 0.04 times faster than with --keepcache

DrahtBot requested review from w0xlt on Mar 25, 2026

DrahtBot requested review from l0rinc on Mar 25, 2026

sedited approved

sedited commented at 3:57 pm on March 25, 2026: contributor

ACK 0d1301b47a35f1ff04c30d104f774b5f8f0c6995

achow101 commented at 7:58 pm on March 26, 2026: member

ACK 0d1301b47a35f1ff04c30d104f774b5f8f0c6995

achow101 merged this on Mar 26, 2026

achow101 closed this on Mar 26, 2026

davidgumberg deleted the branch on Mar 26, 2026

in test/lint/test_runner/src/lint_py.rs:92 in 0d1301b47a

87+        .status()
88+        .expect("command error")
89+        .success();
90+    if found {
91+        Err(r#"
92+Use of shutil.rmtree() is dangerous and should be avoided.

maflcko commented at 6:45 am on March 27, 2026:

nit: I think it is fine to flag this, but any error ideally comes with a solution. cleanup_folder is called a bunch of times in the test, so it can not be avoided all the time. I think it would be more friendly and pro-active to just say so, instead of letting the dev figure it out by themselves every time:

0Use of shutil.rmtree() is dangerous and should be avoided. If it is really required for the test, use self.cleanup_folder(_).

maflcko commented at 8:25 am on April 1, 2026:

done in https://github.com/bitcoin/bitcoin/pull/34976

in test/lint/test_runner/src/lint_cpp.rs:142 in 0d1301b47a

137+        .status()
138+        .expect("command error")
139+        .success();
140+    if found {
141+        Err(r#"
142+Use of fs::remove_all or std::filesystem::remove_all is dangerous and should be avoided.

maflcko commented at 6:46 am on March 27, 2026:

0Use of fs::remove_all or std::filesystem::remove_all is dangerous and should be avoided. If removal is required, use fs::remove.

nit: Same here, albeit one can probably guess it.

maflcko commented at 6:48 am on March 27, 2026: member

lgtm, but I think the error messages could also mention the solution instead of only the problem

qa: Drop recursive deletes from test code, add lint checks. #34439

Reviews

Conflicts