util: Abort in CheckDiskSpace/FlatFileSeq::Open on rare 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 all paths equally and abort the program.

Aborting the program here is fine, because the exceptions happen so rarely, that they are similar in frequency to other unclean shutdown reasons. The exceptions basically can only happen when the user unplugs the external USB drive, but this sounds as likely as an OOM kill (or other kill of the program), which also results in an abort.

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:

#35003 (validation: improve block data I/O error handling in P2P paths by furszy)
#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)

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 outdated

  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.

maflcko commented at 11:51 AM on July 10, 2026:

(solving for now, because this comment applies to prior code that was return false; here).

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, ...

josibake commented at 12:51 PM on July 9, 2026: member

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.

I think this is one of the strengths of exceptions: its impossible to ignore them, unlike error codes. So I think the correct thing is allow them to propagate up to the appropriate layer, where they can be handled. The question I'd pose instead is "what is the appropriate layer," or "are there multiple layers." If we can't find a layer to handle an exception, having it terminate seems the safest, no?

maflcko force-pushed on Jul 9, 2026

maflcko renamed this:
~~util: Return false in CheckDiskSpace on exceptions~~
util: Abort in CheckDiskSpace on rare exceptions
on Jul 9, 2026

DrahtBot renamed this:
~~util: Abort in CheckDiskSpace on rare exceptions~~
util: Abort in CheckDiskSpace on rare exceptions
on Jul 9, 2026

maflcko commented at 1:34 PM on July 9, 2026: member

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.

I think this is one of the strengths of exceptions: its impossible to ignore them

I am not sure if it is impossible to ignore exceptions. There are several catch-all in the codebase, which lead to exceptions sometimes being silently ignored, and other times lead to an abort if they are not caught, and it is not clear from a quick glance which code outcome will be hit.

If we think that a terminate is fine here, I think then it is fine to do it consistently. The patch would be trivial, and I've pushed it.

If we think a terminate is not fine here, and it should be a fatal shutdown, I think all possible paths should be fixed and handled and properly lead to a fatal shutdown.

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

maflcko force-pushed on Jul 9, 2026

DrahtBot added the label CI failed on Jul 9, 2026

DrahtBot commented at 1:54 PM on July 9, 2026: contributor

🚧 At least one of the CI tasks failed. <sub>Task lint: https://github.com/bitcoin/bitcoin/actions/runs/29021537412/job/86129674308</sub> <sub>LLM reason (✨ experimental): CI failed due to a Python lint (py_lint) error from ruff: an unused import (F401) in test/functional/feature_io_errors.py.</sub>

<details><summary>Hints</summary>

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.

</details>

util: Abort in CheckDiskSpace on rare exceptions fac9306686

maflcko force-pushed on Jul 9, 2026

josibake commented at 2:03 PM on July 9, 2026: member

There are several catch-all in the codebase

That's a different problem of catching the exception and doing nothing with it. My original comment was referring to impossible to ignore uncaught exceptions (perhaps poorly worded).

With your most recent push, I still think this PR is repeating the same bad pattern of having a policy decision being made in a low level primitive. The function should do what its asked (check the disk space) and give an exception back to the caller if it cannot complete the task. The caller then decides to either do something with the exception, or just let it propagate up if they don't have enough information, context, authority etc to do something.

If we keep following this pattern, over time this allows us to have all of our error handling logic and logging consolidated into boundaries where its much easier to reason about whats going on.

If we think that a terminate is fine here, I think then it is fine to do it consistently.

Not disagreeing with this, I just don't think this is the right way to enforce consistency. I'd rather we get rid of the do nothing catch alls, letting any unhandled exception terminate for free. For anything that can be handled, catch it at the appropriate layer and take a policy action owned by that boundary.

DrahtBot removed the label CI failed on Jul 9, 2026

in src/flatfile.cpp:50 in fac9306686

  46 | +    if (ec) {
  47 | +        const auto msg{tfm::format("Unable to create directory %s: %s", fs::PathToString(path.parent_path()), ec.message())};
  48 | +        std::cerr << msg << std::endl;
  49 | +        LogError("%s", msg);
  50 | +        std::abort();
  51 | +    }

furszy commented at 2:02 PM on July 10, 2026:

I don't think a low-level unclean shutdown is the right approach. An error accessing an index directory or another unrelated file shouldn't prevent the chain or wallet from attempting to flush their state to disk. They could be living in a different storage location.

maflcko commented at 2:18 PM on July 10, 2026:

Aborting the program here is fine, because the exceptions happen so rarely, that they are similar in frequency to other unclean shutdown reasons. The exceptions basically can only happen when the user unplugs the external USB drive, but this sounds as likely as an OOM kill (or other kill of the program), which also results in an abort.

Unclean shut-downs happen (and they should be supported), see e.g. #33208.

It is impossible to avoid all unclean shutdowns. There can always be a power outage, an overheated CPU, a kernel panic, an OOM, an assertion failure, a typo in the pid of kill -9, etc. So I think it is fine to add one more rare thing to the list here.

Also, this commit is trivial to revert, if another approach is taken, but such an approach will likely have to touch the whole codebase to bubble up the error/exception and handle (or not handle) it.

furszy commented at 2:49 PM on July 10, 2026:

Aborting the program here is fine, because the exceptions happen so rarely, that they are similar in frequency to other unclean shutdown reasons. The exceptions basically can only happen when the user unplugs the external USB drive, but this sounds as likely as an OOM kill (or other kill of the program), which also results in an abort.

Downloading + validating the chain is expensive, and ending up with a wallet in an inconsistent state due to an unclean shutdown is dangerous. They could easily be residing in different storage devices, we do have options for it. Attempting a clean shutdown, even during rare errors like this one, seems the safest approach for soft that is responsible of storing users' funds. At least that is my view.

Unclean shut-downs happen (and they should be supported), see e.g. #33208.

That shouldn't be the case anymore, we just merged #34897 which avoids reaching the situation of persisting a locator from a block that hasn't been flushed to disk yet.

Also, this commit is trivial to revert, if another approach is taken, but such an approach will likely have to touch the whole codebase to bubble up the error/exception and handle (or not handle) it.

I don't think this is a strong argument, I could have used the same argument to answer to your message #35003 (comment) too? at least #35003 introduces coverage to all the affected areas while this one doesn't.

maflcko commented at 3:14 PM on July 10, 2026:

Downloading + validating the chain is expensive, and ending up with a wallet in an inconsistent state due to an unclean shutdown is dangerous.

I don't think this abort will delete any previously downloaded and flushed block files, so no re-download is needed. Also, validation progress is flushed regularly during IBD, catching up a few minutes after a rare unclean shutdown (for whatever reason) should be fine.

Unclean shut-downs happen (and they should be supported), see e.g. #33208.

That shouldn't be the case anymore

Yes, that is the point. I just wanted to show an example that unclean shutdowns are not recommended, but supported. That is, if a bug is hit after restart after an unclean shutdown, it is a bug that should be fixed. (And such bugs have been fixed in the past.)

at least #35003 introduces coverage to all the affected areas while this one doesn't.

Happy to cherry-pick the test over here. It should be trivial. Otherwise, i think the downsides of 35003 in its current form are:

35003 doesn't deal with CheckDiskSpace, so it is incomplete and a separate pull will be needed anyway
35003 doesn't deal with p2p logic, so the node may keep running and may leave peers connected and silently not reply to them
35003 doesn't deal with indexes: It silently returns that a blockchain transaction does not exist in the chain, when it does exist in the chain.

So all of those will need another pull to take care of. My main concern is that 35003 will be merged and the stuff isn't followed-up on. My view is that in this area, we need to fix all issues (or at least agree on how to fix them) without regressing in the meantime.

furszy commented at 4:12 PM on July 10, 2026:

I don't think this abort will delete any previously downloaded and flushed block files, so no re-download is needed. Also, validation progress is flushed regularly during IBD, catching up a few minutes after a rare unclean shutdown (for whatever reason) should be fine.

Catching up can be painful too, especially if inconsistent state forces a full chain re-download for some reason, but that is not the main concern. An unclean shutdown could leave the wallet in an inconsistent state too, which seems far more serious to me. Even if the scenario is rare, I think it is worth avoiding an abrupt abort whenever possible. That is one of the reasons behind 35003 replacing the assertion for FatalError call.

So all of those will need another pull to take care of. My main concern is that 35003 will be merged and the stuff isn't followed-up on. My view is that in this area, we need to fix all issues (or at least agree on how to fix them) without regressing in the meantime.

Do you think that is the underlying reason we are not moving forward? Because that seems easy to address by opening an issue that lists all the remaining items and then crossing them off one by one.

We are not leaving the repository in a worse state with each change. In fact, the opposite is true. Each PR introduces strict improvements over the current state. The repository wouldn't be in a worse state after #35003, unless you see a regression introduced there? In such case, it would be good to know it to learn from it.

furszy commented at 2:18 PM on July 10, 2026: member

letting any unhandled exception terminate for free

I would be careful with this sentence, as it is a strong general statement. At the p2p layer, this could result in a remotely triggerable abortion. We cannot know where or how the code may be used in the future, but we can do the work to provide a safe interface that prevents such issues. I would take the safer approach and handle exceptions consistently.

maflcko renamed this:
~~util: Abort in CheckDiskSpace on rare exceptions~~
util: Abort in CheckDiskSpace/FlatFileSeq::Open on rare exceptions
on Jul 10, 2026

sedited commented at 10:52 AM on July 11, 2026: contributor

I think @furszy raises some good points here. It is one thing to posit that crashing should be fine, but another to actually being able to guarantee that. I think this is also close to impossible to guarantee.

At the p2p layer, this could result in a remotely triggerable abortion.

I'm not too worried about this, at least when it comes to filesystem exceptions. This might not be true for others, like socket-level exceptions. I find the problem as sketched in #35003, i.e. entering ant mill-like doom loops, much more likely and bad. In my opinion this is most clearly solved by throwing exceptions and catching them as close to the module/thread boundaries as possible.

Architecturally, I don't think we are in a horrible situation either. We have a main thread that gracefully handles shutdown when notified. On an error, it gives us a second chance at cleanly flushing the state. We have shutdown and interrupt notification objects available pretty much for the entire runtime of the program. Naively, it doesn't seem overly complicated to catch exceptions at the top level of a thread and then trigger a shutdown notification.

EDIT: Wanted to also add that with the guidance for using exceptions being cited here and in the other pull request, actual suggestions on where to handle exceptions is surprisingly slim.