#35763 util: write JSON atomically in WriteJson

ReviewabilityReviewsAgreementSize
#35763 kevkevinpal util: write JSON atomically in WriteJsonReady1 (+1)Strong28 + 45 tests
  • Prevent banlist and settings files from being corrupted or truncated if a crash occurs during a write
  • Ensure settings and ban updates are replaced atomically rather than written directly in place
  • Ready for review
  • Strong support with all earlier concerns on API safety, temp files, and tests resolved
  • Tested on FreeBSD and approved the self-contained atomic write implementation (winterrdog)
  • Approved noting that the new test coverage is useful (Herb-ops)
  • 28 lines added or modified outside tests
  • 45 lines added or modified in tests
  • 24 lines removed in total
  • 7 files, 1 commits
Utilities (logging, arguments, libraries)#29 of 59
P3 · bug fix
  • P3 because atomic updates prevent corrupted or empty banlist files following unexpected shutdowns
  • The benefit is limited to protecting local state during rare crash or write-interruption scenarios
Ranking notes
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

Moves atomic file replacement logic directly into common::WriteSettings by writing to a temporary ".tmp" file and using RenameOver to replace the target destination. This makes banlist writes atomic and removes duplicate temporary-file handling from ArgsManager::WriteSettingsFile. A unit test is added to verify banlist roundtripping and rename-failure behavior.

Problem: Writing banlist.json previously wrote directly to the destination path. A crash, power loss, or write failure while persisting bans could leave the file empty or truncated. Additionally, ArgsManager had to implement its own atomic-write dance around settings.json rather than having the utility function handle it.

Evidence of importance:

  • PR description: "WriteJson writes to path + ".tmp" and RenameOver()s into place, so a failed or interrupted write cannot leave a truncated destination. Both settings.json and banlist.json use this helper."

Scope: The PR description mentions renaming common::WriteSettings to common::WriteJson, but the patch leaves the function name as WriteSettings.

Reviewability

Ready

Nothing known that would invalidate a review now.

Agreement

Strong

substantive support, no open objection (Herb-ops, winterrdog, winterrdog)

Thread notes: Initial record for this PR. All objections raised by winterrdog and Herb-ops regarding atomic write API safety, test verification, temp file handling, and naming symmetry were resolved, leading to winterrdog's tACK.

Objections, suggestions, and questions:

ReviewerTypeHarm, request, or questionStatusMust beAuthor repliedQuote
Herb-ops (none)objection, correctnessthe unit test does not verify rename behavior because it still passes when reverted to direct writingfixed · fix pushed · v1, not yet re-checkedansweredyes2026-07-28: 'does not verify the rename behavior named by the test'
Status from: 2026-09-07: kevkevinpal: 'can you review this again?'
winterrdog (contributor)objection, correctnessunverified test coverage and inaccurate error messages are blocking concernsfixed · fix pushed · v1, not yet re-checkedchangedyes2026-07-28: 'these need attention. so, i think they are blocking'
Status from: 2026-09-07: winterrdog: 'approach ACK'
winterrdog (contributor)objection, safetyexposing WriteJsonUnsafe allows future callers to accidentally bypass atomic write semantics and write straight to live filesfixed · fix pushed · v1, not yet re-checkedchangedyes2026-07-28: 'nothing stops a future caller from accidentally writing straight to a live file'
Status from: 2026-09-07: winterrdog: 'current implementation is self-contained and handles the "temp write + RenameOver()" itself'
winterrdog (contributor)objection, maintenanceleaving temporary files on failure paths could cause temp files to accumulate indefinitelyfixed · fix pushed · v1, not yet re-checkedansweredyes2026-09-07: 'these temp files are most likely to accumulate indefinitely'
Status from: 2026-09-08: winterrdog: 'makes sense. i concur'
winterrdog (contributor)objection, interfaceAPI naming asymmetry between WriteJson and ReadSettings when both operate on the same datafixed · fix pushed · v1, not yet re-checkedansweredyes2026-09-08: 'WriteJson / ReadSettings pair that operates on the same kind of data, but has different names (asymmetry)'
Status from: 2026-09-17: winterrdog: 'that is fine. we can do that'

Support:

  • winterrdog (contributor) (Concept ACK, 2026-07-21): Thanks for picking these up. [not substantive]
  • Herb-ops (none) (ACK, 2026-07-28): ACK 07ec5af152b5bcff1b74decb9c7b729dff6c2283, with two non-blocking observations: The test is useful
  • winterrdog (contributor) (Approach ACK, 2026-09-07): current implementation is self-contained and handles the "temp write + RenameOver()" itself
  • winterrdog (contributor) (Tested ACK, 2026-09-17): successfully built and tested on FreeBSD 15.0/clang++-19.1.7/x86_64. the changes herein LGTM as well

Participants:

  • winterrdog (contributor, objection, 9 statements 2026-07-21 to 2026-09-17): Concept ACK, raised concerns on unsafe write API, temp files, and naming symmetry, then tACKed after fixes
  • Herb-ops (none, objection, 1 statements 2026-07-28 to 2026-07-28): ACKed with non-blocking observations on test coverage and error log wording

Files

46 lines under test/bench/ci.

  • src/test/banman_tests.cpp +44/-0
  • src/common/args.cpp +6/-10
  • src/common/settings.cpp +10/-4
  • src/addrdb.cpp +6/-7
  • src/common/settings.h +5/-1
  • src/common/args.h +1/-1
  • src/test/argsman_tests.cpp +1/-1

Card

The short text the ranking pass compares across PRs.

This pull request makes common::WriteSettings write to a temporary file and atomically replace the target destination with RenameOver. Previously, banlist.json was written directly in place, leaving node operators vulnerable to a truncated or corrupted banlist if the write was interrupted by a crash or power failure. It also removes duplicate temporary-file management logic in ArgsManager::WriteSettingsFile and adds unit tests covering atomic banlist writes and rename failure handling.

Data

dossier JSON · extract JSON · model ledger, generated 2026-09-18T03:38, confidence high, input hash 1fd4cce03a277b88
thread read through 2026-09-17 (14 statements), head ed55f14404
Discussion read under schema 1 rules: suggestions and questions may be missing, and an objection may combine several harms (schema history).
labels: Utils/log/libs

Categories

Utilities (logging, arguments, libraries)

P3 · bug fix

Prevents potential file truncation or corruption by ensuring common JSON files are written atomically. The PR description notes that banlist writes previously wrote directly to destination, where "a failed or interrupted write cannot leave a truncated destination" once converted to write-and-rename semantics.

Membership: Matches the Utils/log/libs label and modifies common settings serialization utilities in src/common/settings.cpp and args.cpp.

Factors: security/stability 1, bug 1, performance 0, user value 1, leverage 0