streams: Include the OS error when AutoFile I/O fails #36210

pablomartin4btc commented at 8:08 PM on September 9, 2026: member

Include the OS error (errno) in AutoFile/ BufferedFile exception messages when fread()/ fwrite()/ fseek() calls fail. I noticed these were missing while reviewing #35492, which moves DumpWallet from std::ofstream to AutoFile. With this PR, callers get the actual reason why the call failed (disk full, permission denied, bad file descriptor...) instead of just a fixed string like "AutoFile::write: write failed".

-Notes:

The first commit adds streams_tests.cpp coverage asserting the current behaviour on master, with a TODO marking the assertion that the second commit (the actual fix) flips.
Using this in DumpWallet is left for later: the AutoFile-based dump.cpp from #35492 isn't on master yet, so it can be done in whichever PR gets merged second.

DrahtBot commented at 8:08 PM on September 9, 2026: contributor

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

External sites

Code coverage & benchmarks: https://corecheck.dev/bitcoin/bitcoin/pulls/36210
Ralph review <img src=https://review.fish.foo/assets/bitcoin/bitcoin/36210.png alt="Ralph review status"> https://git.fish.foo/bitcoin/bitcoin/pulls/36210#author-ralph

Reviews

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

Type	Reviewers
Concept ACK	w0xlt

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:

#36049 <img src="https://drahtbot.space/ack_count/bitcoin/bitcoin/36049.svg"> (streams: avoid termination on buffered write failure 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.

LLM Linter (✨ experimental)

Possible places where named args for integral literals may be used (e.g. func(x, /*named_arg=*/0) in C++, and func(x, named_arg=0) in Python):

src/test/streams_tests.cpp: BufferedFile bf{f, 8, 4}

2026-10-29 15:47:50

DrahtBot added the label CI failed on Sep 9, 2026

DrahtBot commented at 9:20 PM on September 9, 2026: contributor

🚧 At least one of the CI tasks failed. Task Alpine (musl): https://github.com/bitcoin/bitcoin/actions/runs/34399219812/job/102626611981 LLM reason (✨ experimental): CI failed because the CTest run reported streams_tests as failed (CTest exit status 8).

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

pablomartin4btc commented at 3:00 AM on September 10, 2026: member

Fixed the Alpine (musl) CI failure. The test was forcing an I/O error by opening the file in the wrong mode (e.g. writing to a read-only file). glibc sets errno when that happens, but musl doesn't: it just marks the stream as failed and returns (see __towrite.c/ __toread.c). So on POSIX the test now opens the file in the right mode and closes its file descriptor underneath, so the read/ write actually reaches the kernel and fails with a real EBADF, whatever the libc. Windows is unchanged, since its C runtime does set errno for the wrong-mode case.

pablomartin4btc marked this as a draft on Sep 10, 2026

pablomartin4btc force-pushed on Sep 10, 2026

DrahtBot removed the label CI failed on Sep 10, 2026

pablomartin4btc marked this as ready for review on Sep 10, 2026

pablomartin4btc commented at 4:28 AM on September 10, 2026: member

-<ins>Updates</ins>:

Fixed Alpine (musl) CI failure as described above.

w0xlt commented at 10:03 PM on September 22, 2026: contributor

Concept ACK

maflcko commented at 2:46 PM on October 22, 2026: member

Can you please write the pull description and any comments without using an LLM? see https://github.com/bitcoin/bitcoin/blob/master/doc/AI_POLICY.md

AI should not be used to generate comments when communicating with maintainers and other contributors. Comments are expected to be written by humans.

test: AutoFile/BufferedFile I/O failures discard the OS error

AutoFile::write(), write_buffer(), read() (via detail_fread), ignore(),
and BufferedFile::Fill() all throw a fixed, generic message when the
underlying fread()/fwrite() fails for a real OS reason (as opposed to
reaching end-of-file), discarding the errno that explains why.

Force such a failure with a genuine kernel-level I/O error rather than
relying on a libc-internal check: on POSIX, open the file in the
correct direction and close its underlying fd out from under the
FILE*, so the next read/write hits a real EBADF from the kernel; musl's
own __towrite()/__toread() never touch errno for a same-direction
mismatch, unlike glibc's, so a libc-internal check isn't reliable here.
On Windows, opening in the wrong direction already produces a genuine,
errno-bearing failure.

TODO: flip check_io_failure() to assert the OS error IS present once
AutoFile/BufferedFile are fixed to include it.

379dacf050

streams: include the OS error when AutoFile I/O fails

AutoFile::write(), write_buffer(), read() (via detail_fread), ignore(),
seek() and BufferedFile::Fill() throw a fixed message on a genuine
fread()/fwrite()/fseek() failure (not end-of-file), discarding the errno
that explains why the call actually failed. Callers surfacing e.what()
to logs or users lose that detail.

Capture errno right after the failing call and fold SysErrorString()
into the thrown message, consistent with the rest of the codebase's
error reporting (lint-locale-dependence.py restricts raw strerror use
to SysErrorString's own implementation).

Flips check_io_failure()'s assertion from the previous commit to
confirm the OS error is now present, and adds a seek() case: seeking
to a negative offset fails with EINVAL on all supported platforms.

5686bd642a

pablomartin4btc force-pushed on Oct 29, 2026

pablomartin4btc commented at 4:46 PM on October 29, 2026: member

-<ins>Updates</ins>:

Rebased due to a conflict in streams.cpp caused by #36669;
Included the OS error in AutoFile::seek() (and a test for it) and updated the PR description accordingly;
Reworded PR description and a comment as requested by @maflcko.