#33646 log: check fclose() results and report safely in logging.cpp
| Reviewability | Reviews | Agreement | Size | ||||
|---|---|---|---|---|---|---|---|
| #33646 log: check fclose() results and report safely in logging.cpp | Waiting on author | 0 | Positive | 29 | |||
|
|
|
|
| |||
Reviewability: ReadyStalePaused Agreement: StrongPositivePositive w/ caveatsNeutralMildDisputedBlockedCrickets Reviews: current code-review ACKs, then (+stale ACKs) and -NACKs; greener = more ACKs. Size: lines added or modified outside tests, then in tests; greener = smaller. | |||||||
Goal
Checks the return value of `fclose()` calls in `src/logging.cpp` via a helper function `CloseLogFile()`. Failures are formatted and written directly to stderr along with system error details to avoid re-entering the logger or taking locks during error reporting.
Problem: Errors encountered when closing log files during log rotation, shrink, or shutdown are currently ignored. If a filesystem writeback failure occurs on close, it is not reported anywhere and can lead to unnoticed diagnostics loss.
Reviewability
Paused · Waiting on author
Open objections from sedited with no author reply for 11 days; review would repeat what has been asked.
Waiting on (from the thread read):
- author: address sedited's open review comments from 2026-09-08 2026-10-21
- author: address sedited's open review comments on noexcept, unnecessary variable, and commit message 2026-10-21
Agreement
Positive
support without stated reasons, no open objection (sedited)
Thread notes: Added questions from ajtowns and sedited, along with maflcko's comment suggestion and sedited's recent review comments on noexcept and variable cleanup. / Brought the record up to date by recording questions and suggestions from ajtowns, sedited, and maflcko. The PR is waiting on the author to respond to sedited's open review comments.
Objections, suggestions, and questions:
| Reviewer | Type | Harm, request, or question | Status | Must be | Author replied | Quote |
|---|---|---|---|---|---|---|
| ajtowns (member) | question, approach | what is the value in having the fclose outside the m_cs guard | answered · author explained it avoids holding the mutex during slow I/O | answered | yes | 2025-12-11: 'What's the value in having this outside the `m_cs` guard?' Status from: 2025-12-11: cedwies: 'I put the `fclose` outside the lock to avoid holding the mutex during potentially slow I/O' |
| sedited (member) | question, approach | whether there is a way to consistently trigger an fclose error to test it locally | answered · maflcko and author noted unbuffered logging prevents flush errors, leaving only fault injection | answered | yes | 2026-03-05: 'is there a way to consistently trigger an fclose error so this might be tested locally?' Status from: 2026-03-24: maflcko: 'an alternative to mock the fs to return an IO error likely won't work, because there is no buffer to be flushed' |
| maflcko (member) | suggestion, maintenance | place the comment on the function rather than the namespace | fixed · fix pushed · author moved documentation onto CloseLogFile | changed | yes | 2026-05-16: 'The comment should be on the function, not the namespace?' Status from: 2026-07-28: cedwies: 'Done, the documentation is now attached directly to `CloseLogFile()`.' |
| sedited (member) | objection, approach | inconsistent mutex locking across close paths with poor motivation for the test-only change | fixed · fix pushed · author restored fclose under m_cs in DisconnectTestLogger | answered | yes | 2026-05-23: 'changing this for this single test-only path seems like a poor motivation' Status from: 2026-07-28: cedwies: 'Agreed. I restored the close in DisconnectTestLogger() to run under m_cs for consistency with the other call sites.' |
| sedited (member) | question, maintenance | clarify whether the commit message is stale | open | answered | no | 2026-09-08: 'Is the commit message stale? I'm having a hard time understanding the second paragraph.' |
| sedited (member) | suggestion, approach | drop noexcept from the function | open | answered | no | 2026-09-08: 'I don't think this function needs `noexcept`.' |
| sedited (member) | suggestion, maintenance | remove unnecessary extra variable | open | answered | no | 2026-09-08: 'This extra variable now seems unnecessary, no?' |
Support:
- sedited (member) (2026-03-05): This looks ok [not substantive]
Participants:
- ajtowns (member, question, 2 statements 2025-12-11 to 2025-12-11): asked why fclose was placed outside m_cs guard
- sedited (member, objection, 10 statements 2026-03-05 to 2026-10-21): objected to locking changes for test logger, asked questions on testing and code structure
- maflcko (member, neutral, 2 statements 2026-03-24 to 2026-05-16): discussed fclose error testing limitations and suggested moving comment to function
Files
0 lines under test/bench/ci.
- src/logging.cpp +29/-8
Card
The short text the ranking pass compares across PRs.
This PR introduces a CloseLogFile helper in src/logging.cpp to verify fclose return values and write error diagnostics to stderr. It prevents silent failures during log rotation and shutdown when closing the debug log. Because debug log files are unbuffered, close failures are rare, giving this patch marginal operational impact. The change is conceptually welcomed and reviewable, with only minor nits and commit message tweaks awaiting the author.
Data
thread read through 2026-10-21 (20 statements), head a41ae57707
labels: Utils/log/libs
Categories
Utilities (logging, arguments, libraries)
P3 · bug fix (ranking pass: Moved down: the card itself notes close failures are rare, making this the least consequential logging fix.)
Reports previously ignored fclose() errors during log rotation, shrink, or shutdown directly to stderr. This prevents silent diagnostic loss and handles I/O writeback failures safely without re-entering the logger or taking locks.
Membership: Matches Utils/log/libs label and directly modifies logging implementation in src/logging.cpp.
Factors: security/stability 1, bug 1, performance 0, user value 0, leverage 0