payout: leave no temporary file behind, and say so where it will be read

Every failure path in WriteFileAtomic returned without removing
"<path>.tmp". A disk that filled up once, or a single EIO, left a short
file called pending_payouts.json.tmp sitting next to a good ledger for
as long as the node ran. That file is worse than no file: it carries the
name of a half-finished save, and the operator cleaning up after the
incident is precisely the person who will move it into place and publish
a ledger missing whatever had not been written yet.

The failures also logged under LogPrint("payout", ...). pool_status.json
is written through the same function but everything else about it is
logged under "pool", so an operator running -debug=pool was told nothing
when the status file could not be replaced -- the message existed and
was discarded for having the wrong category. They now print
unconditionally: a write failure is not a trace of normal operation, it
is the file that is about to be read not being there, and gating it on a
category chosen before the failure happened is how it went missing. The
brief's alternative, a category argument, would still have required the
right guess in advance.

Properties: no failure of WriteFileAtomic leaves a .tmp behind, and no
failure of it is invisible at default log settings.

1 files changed, 28 insertions, 10 deletions

diff --git a/src/rpc.cpp b/src/rpc.cpp
index f853e78..be265db 100644
--- a/src/rpc.cpp
+++ b/src/rpc.cpp
@@ -506,20 +506,35 @@ static void SyncDirectoryOf(const std::string& path)
 // 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.
+//
+// Every failure removes the temp file before returning. It used to be left
+// where it fell, so a disk that filled up once left a short, valid-looking
+// "pending_payouts.json.tmp" next to the good ledger for as long as the node
+// ran. That file is a trap: it has the name of a half-finished save, and an
+// operator cleaning up after an incident is exactly the person who will move
+// it into place.
+//
+// Failures print unconditionally rather than through LogPrint(). These three
+// files are written under two different debug categories ("payout" for the
+// ledgers, "pool" for the status file), and a write failure is not a trace of
+// normal operation -- it is the file the operator is about to read not being
+// there. Gating it behind a category that had to be chosen before the failure
+// happened is how "could not replace pool_status.json" went missing.
 static bool WriteFileAtomic(const std::string& path, const std::string& data,
                             const char* what)
 {
     std::string tmp = path + ".tmp";
     FILE* f = fopen(tmp.c_str(), "w");
     if (!f) {
-        LogPrint("payout", "[payout] WARNING: could not write %s (%s)\n",
-                 tmp.c_str(), what);
+        printf("[payout] WARNING: could not write %s (%s)\n",
+               tmp.c_str(), what);
         return false;
     }
     if (!data.empty() && fwrite(data.c_str(), 1, data.size(), f) != data.size()) {
-        LogPrint("payout", "[payout] WARNING: short write to %s (%s)\n",
-                 tmp.c_str(), what);
+        printf("[payout] WARNING: short write to %s (%s)\n",
+               tmp.c_str(), what);
         fclose(f);
+        remove(tmp.c_str());
         return false;
     }
     // Before the rename, not after: publishing a name that points at nothing
@@ -527,9 +542,10 @@ static bool WriteFileAtomic(const std::string& path, const std::string& data,
     // 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);
+        printf("[payout] WARNING: could not flush %s to disk (%s)"
+               " -- keeping the previous file\n", tmp.c_str(), what);
         fclose(f);
+        remove(tmp.c_str());
         return false;
     }
     // fclose() is where buffered data actually reaches the OS, so a failure
@@ -537,8 +553,9 @@ static bool WriteFileAtomic(const std::string& path, const std::string& data,
     // destroy the good copy, which is exactly what these files exist to
     // prevent.
     if (fclose(f) != 0) {
-        LogPrint("payout", "[payout] WARNING: could not close %s (%s)\n",
-                 tmp.c_str(), what);
+        printf("[payout] WARNING: could not close %s (%s)\n",
+               tmp.c_str(), what);
+        remove(tmp.c_str());
         return false;
     }
 #ifdef _WIN32
@@ -546,8 +563,9 @@ static bool WriteFileAtomic(const std::string& path, const std::string& data,
     remove(path.c_str());
 #endif
     if (rename(tmp.c_str(), path.c_str()) != 0) {
-        LogPrint("payout", "[payout] WARNING: could not replace %s (%s)\n",
-                 path.c_str(), what);
+        printf("[payout] WARNING: could not replace %s (%s)\n",
+               path.c_str(), what);
+        remove(tmp.c_str());
         return false;
     }
     SyncDirectoryOf(path);