From 86b39b952d81c8aa6db56beb293e08befdca296e Mon Sep 17 00:00:00 2001 From: bitflash-maintainer Date: Sun, 23 Aug 2026 07:45:56 +0000 Subject: payout: flush the ledger to the device before the rename that publishes it WritePendingPayoutsFile, SavePayingLocked and SavePoolRoundsLocked all wrote a temp file and renamed it, with no fsync anywhere. That is atomic against the process dying -- which is what the crash test exercised -- and not against the machine dying. A power cut or a hard VM reset can land the rename on the platter ahead of the data it points at, and what comes back is a short or empty file. For pending_payouts.json that is not a corrupt ledger, it is an absent one: LoadPendingPayouts catches the parse error, logs one warning, and the node starts up owing nobody anything. Total silent loss of the unpaid ledger, from a clean-looking boot. WriteFileAtomic now fflush()es and fsync()s the temp file before renaming it, and fsync()s the containing directory afterwards so the new directory entry is durable as well. If the data cannot be made durable the rename does not happen at all: the previous file is still intact and still correct, and the next write will try again. Ordering the sync before the rename is the point -- publishing a name that points at nothing is the exact failure being prevented. The directory sync is best effort; a filesystem that will not let the directory be opened for it is not a reason to refuse to write the ledger. This is only affordable because the preceding commit moved all three writes out from under gPayoutLedgerMutex / gPayoutExecMutex / gPoolRoundsMutex. An fsync under any of those would park the pool event loop on the payout thread's disk, which is the contention the merge was right to remove. Applies to upstream too and is being reported there. No on-disk format change. --- src/rpc.cpp | 61 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++--- 1 file changed, 58 insertions(+), 3 deletions(-) diff --git a/src/rpc.cpp b/src/rpc.cpp index d655ad5..45130ff 100644 --- a/src/rpc.cpp +++ b/src/rpc.cpp @@ -31,6 +31,9 @@ #include #include #include +#include +#else +#include #endif #include @@ -457,9 +460,50 @@ struct VersionedBlob { uint64 seq = 0; }; -// Replace `path` with `data` atomically. Caller must hold the write mutex for -// `path`: the temp name is derived from the target, so two threads in here at -// once for one path is precisely the race this exists to prevent. +// Push a file's contents out of the C library's buffer AND out of the kernel's +// page cache, so the bytes exist on the device before anything points at them. +static bool SyncFileHandle(FILE* f) +{ + if (fflush(f) != 0) + return false; +#ifdef _WIN32 + return _commit(_fileno(f)) == 0; +#else + return fsync(fileno(f)) == 0; +#endif +} + +// Make the directory entry that rename() created durable too. +// +// A tmp+rename is only atomic with respect to a crash of the process. Against +// a power cut or a hard VM reset the rename can reach the platter while the +// data it points at has not, and the file that comes back is short or empty. +// For pending_payouts.json that is not a corrupt file, it is a silent one: +// LoadPendingPayouts catches the parse error, logs a single warning, and the +// node comes up owing nobody anything. +// +// Best effort -- a filesystem that does not allow the directory to be opened +// for this is not a reason to refuse to write the ledger. +static void SyncDirectoryOf(const std::string& path) +{ +#ifndef _WIN32 + size_t slash = path.find_last_of('/'); + std::string dir = (slash == std::string::npos) ? std::string(".") + : path.substr(0, slash); + if (dir.empty()) + dir = "/"; + int fd = open(dir.c_str(), O_RDONLY); + if (fd < 0) + return; + fsync(fd); + close(fd); +#endif +} + +// Replace `path` with `data` atomically and durably. Caller must hold the +// write mutex for `path`: the temp name is derived from the target, so two +// threads in here at once for one path is precisely the race this exists to +// prevent. static bool WriteFileAtomic(const std::string& path, const std::string& data, const char* what) { @@ -476,6 +520,16 @@ static bool WriteFileAtomic(const std::string& path, const std::string& data, fclose(f); return false; } + // Before the rename, not after: publishing a name that points at nothing + // is the failure this ordering exists to prevent. If the data cannot be + // made durable, do not rename -- the previous file is still intact and + // still correct, and the next write will try again. + if (!SyncFileHandle(f)) { + LogPrint("payout", "[payout] WARNING: could not flush %s to disk (%s)" + " -- keeping the previous file\n", tmp.c_str(), what); + fclose(f); + return false; + } // fclose() is where buffered data actually reaches the OS, so a failure // here means the .tmp is short -- renaming it over the real file would // destroy the good copy, which is exactly what these files exist to @@ -494,6 +548,7 @@ static bool WriteFileAtomic(const std::string& path, const std::string& data, path.c_str(), what); return false; } + SyncDirectoryOf(path); return true; } -- cgit v1.2.3