streams: avoid termination on buffered write failure #36049

pull l0rinc wants to merge 3 commits into bitcoin:master from l0rinc:l0rinc/streams-buffered-write-failure changing 3 files +55 −5
  1. l0rinc commented at 10:06 PM on August 20, 2026: contributor

    Problem: Block and undo writers buffer serialized data, then rely on the BufferedWriter destructor to write the remaining bytes. fwrite() can report fewer bytes written than requested after a local storage error, e.g., if the filesystem fills up while a block is being written (my 1 TB benchmarking servers have recently been filling up, so I've been seeing these failures more often). AutoFile::write_buffer() converts that result into an exception, and because the destructor is implicitly non-throwing, the exception invokes std::terminate instead of reaching normal storage-error handling. Calling flush() before destruction is insufficient on its own because a failed flush leaves the same bytes pending, so the destructor retries the write while the original exception unwinds.

    I introduced this bug in #31551 when adding the BufferedWriter.

    Fix: BufferedWriter now clears the pending byte count before writing, preventing a destructor retry after a failed explicit flush. Block and undo writers flush before destruction, so write failures follow normal storage-error handling instead of terminating the process.

    <details> <summary>Manual short-write reproducer</summary>

    sed -i '' '122s/src.size()/0/' src/streams.cpp
    cmake -B build && cmake --build build -j && build/bin/bitcoind -regtest -datadir="$(mktemp -d)"; echo "exit=$?"
    

    With implicit flushing, bitcoind aborts:

    libc++abi: terminating due to uncaught exception of type std::__1::ios_base::failure: AutoFile::write_buffer: write failed: unspecified iostream_category error zsh: abort build/bin/bitcoind -regtest -datadir="$(mktemp -d)" exit=134

    With explicit flushing, it reports Failed to write genesis block and exits 1 instead of terminating the process:

    Shutdown done exit=1

    </details>

  2. DrahtBot commented at 10:06 PM on August 20, 2026: contributor

    <!--e57a25ab6845829454e8d69fc972939a-->

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

    <!--006a51241073e994b41acfe9ec718e94-->

    Code Coverage & Benchmarks

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

    <!--021abf342d371248e50ceaed478a90ca-->

    Reviews

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

    Type Reviewers
    ACK w0xlt

    If your review is incorrectly listed, please copy-paste <code>&lt;!--meta-tag:bot-skip--&gt;</code> into the comment that the bot should ignore.

    <!--174a7506f384e20aa4161008e828411d-->

    Conflicts

    Reviewers, this pull request conflicts with the following ones:

    • #36210 <sub><img src="https://drahtbot.space/ack_count/bitcoin/bitcoin/36210.svg"></sub> (streams: Include the OS error when AutoFile I/O fails by pablomartin4btc)

    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.

    <!--5faf32d7da4f0f540f40219e4f7537a3-->

  3. andrewtoth commented at 10:21 PM on August 20, 2026: contributor

    the exception invokes std::terminate instead of reaching normal storage-error handling.

    Can you expand on why this is a problem? If there is a storage error, don't we have to crash anyways?

  4. l0rinc commented at 10:35 PM on August 20, 2026: contributor

    If there is a storage error, don't we have to crash anyways?

    Yes, the node still has to stop, but std::terminate bypasses the existing error handling.

    Can you expand on why this is a problem?

    Handling the exception lets the node report the storage error and run its normal shutdown cleanup.

  5. maflcko commented at 11:56 AM on August 21, 2026: member

    This reminds me of the other "exceptions can terminate" topics.

    It would be good to find a code pattern that avoids this class of problem wholesale. It doesn't seem a great use of human (or LLM) review time to spend on such issues one-by-one. If we care about those issues, it would be better if the compiler or a clang-tidy analysis could find and prevent all of them.

  6. w0xlt commented at 8:25 PM on September 5, 2026: contributor

    Concept ACK

    I ran into the same bug while working in this area and implemented an alternative fix before finding this PR.

    My version removes writes from the BufferedWriter destructor and requires explicit final flushing. It also adjusts AutoFile cleanup during exception unwinding and adds an exception handler for undo-write failures.

    https://github.com/w0xlt/bitcoin/tree/streams/buffered-write-errors

    Feel free to reuse anything you find useful.

  7. test: characterize failed buffered flush
    A failed explicit `BufferedWriter` flush leaves bytes pending, so the destructor retries the write while the exception unwinds.
    
    Record both writes while allowing the retry to succeed. This establishes the behavior that must change before callers can safely flush before destruction.
    
    Cover successful destructor fallback on normal exit and exception unwinding, and make the existing buffered stream tests flush explicitly.
    c960b44397
  8. streams: do not retry failed flushes
    A failed `BufferedWriter::flush()` leaves bytes pending, so the destructor retries the write while the exception unwinds.
    
    Clear the pending byte count before writing to the underlying stream. The original exception can then propagate without another write.
    
    Document explicit flushing for error handling and destructor flushing as a fallback for pending bytes.
    
    Co-authored-by: woltx <94266259+w0xlt@users.noreply.github.com>
    bb373c3b4b
  9. blockstorage: handle buffered write failures
    The `BufferedWriter` destructor is implicitly non-throwing, so a failure while it writes pending block or undo bytes invokes `std::terminate` before normal storage handling can report it.
    
    Flush both writers explicitly, catch the resulting exception inside each storage method, log it, request fatal shutdown, and return the existing failure result.
    
    Co-authored-by: woltx <94266259+w0xlt@users.noreply.github.com>
    cac9501d35
  10. l0rinc force-pushed on Sep 23, 2026
  11. l0rinc commented at 10:31 PM on September 23, 2026: contributor

    Thanks @w0xlt, I used your explicit-flush and undo-write error-handling ideas and credited you as a co-author.

    But I think forgetting to call flush() is a bigger practical risk than the rare exception from a destructor write, so I kept destructor flushing as a fallback. Block and undo writes now flush explicitly and handle failures, and a failed flush is no longer retried during exception unwinding.

  12. w0xlt commented at 5:22 AM on September 24, 2026: contributor

    ACK cac9501d355d9853c17dcfa874d85c8e952cece0


github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2026-10-11 08:51 UTC

This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me