util: Return false in CheckDiskSpace on exceptions #35676

maflcko commented at 12:29 PM on July 7, 2026: member

Currently, exceptions from CheckDiskSpace calls in the scheduler thread (e.g. from events for blockfilterindex or the scheduled disk space check) are not caught, leading to a program termination:

terminate called after throwing an instance of 'std::filesystem::__cxx11::filesystem_error'
  what():  filesystem error: cannot get free space: No such file or directory [/tmp/bitcoin_func_test_1xy14g2w/node0/regtest/indexes/blockfilter/basic]

Immediate termination is fine, because those exceptions are unexpected, but it could make sense to be consistent and treat the exception like any other fatal error and instead shut down the program.

So shut down the program with a fatal error instead.

DrahtBot added the label Utils/log/libs on Jul 7, 2026

DrahtBot commented at 12:29 PM on July 7, 2026: 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/35676.

Reviews

See the guideline and AI policy for information on the review process.

Type	Reviewers
Concept ACK	furszy, l0rinc

If your review is incorrectly listed, please copy-paste <code></code> into the comment that the bot should ignore.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#34132 (coins: drop error catcher, centralize fatal read handling by l0rinc)

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.

maflcko force-pushed on Jul 7, 2026

DrahtBot added the label CI failed on Jul 7, 2026

maflcko removed the label CI failed on Jul 7, 2026

maflcko force-pushed on Jul 7, 2026

DrahtBot added the label CI failed on Jul 7, 2026

furszy commented at 2:00 PM on July 7, 2026: member

Concept ACK, related to #35003 (review)

test: Check handling of I/O erros from CheckDiskSpace (via Allocate) fa67548d1d

util: Return false in CheckDiskSpace on exceptions fa12aabc66

maflcko force-pushed on Jul 7, 2026

maflcko commented at 2:39 PM on July 7, 2026: member

Concept ACK, related to #35003 (comment)

Thx for the context. Reminded me that adding a test for the scheduler check is trivial, so I did that as well.

sedited commented at 2:45 PM on July 7, 2026: contributor

I'm not sure about this change, and the functional test is basically illustrating my hesitation. This now clobbers a potential existence, or permissions error into a disk space fault. The goal in my mind should be to make diagnostics easier. If we really want to clobber all error categories and report which function failed, rather than why something went wrong, shouldn't we at least be logging some detail? I'm starting to think we have these concepts backwards, maybe crashing is actually preferable in some places.

in src/util/fs_helpers.cpp:101 in fa12aabc66

  93 | @@ -94,7 +94,11 @@ bool CheckDiskSpace(const fs::path& dir, uint64_t additional_bytes)
  94 |  {
  95 |      constexpr uint64_t min_disk_space{50_MiB};
  96 |  
  97 | -    uint64_t free_bytes_available = fs::space(dir).available;
  98 | +    std::error_code ec;
  99 | +    const uint64_t free_bytes_available{fs::space(dir, ec).available};
 100 | +    if (ec) {
 101 | +        return false;
 102 | +    }

josibake commented at 2:47 PM on July 7, 2026:

This now conflates a program logic question, "Is there enough disk space", with an actual programming bug, e.g., not satisfying the function's preconditions by passing in a directory that doesn't exist. I understand the desire for consistency, but I don't think this is the way to do it.

furszy commented at 2:52 PM on July 7, 2026: member

shouldn't we at least be logging some detail?

+1, it would be nice to log what caused the error at least.

DrahtBot removed the label CI failed on Jul 7, 2026

maflcko commented at 3:48 PM on July 7, 2026: member

I'm starting to think we have these concepts backwards, maybe crashing is actually preferable in some places.

I am not opposed to crashing. I just wonder which errors should lead to a crash and which errors should lead to a fatal shutdown (with a best-effort flush). I can see that trying to gracefully try to flush in case of an out-of-space could make sense to avoid data loss on a best-effort basis. I can also see that crashing in case of hardware or software corruption is preferable.

For me, it just seems odd that the same i/o exception can sometimes be caught and be handled, and other times it leads to a terminate. Maybe this is just me, and if other reviewers are fine with that and think that those different handling paths are irrelevant or preferable, then we can probably close this (and other such cleanups).

l0rinc commented at 10:02 PM on July 7, 2026: contributor

Concept ACK

maflcko commented at 5:23 AM on July 8, 2026: member

To expand my earlier comment: If we don't care about those types of exceptions and think that crashing is preferable, the two functions should be marked noexcept. This also follows the pattern of other throwing functions that are marked so: GenerateRandomGarbage() noexcept, SQLiteDatabase::Cleanup() noexcept, ...