Merge bitcoin/bitcoin#28946: init: don't delete PID file if it was not generated

8f6ab31 init: don't delete PID file if it was not generated (willcl-ark)

Pull request description:

  In a similar vein to #28784, if a second `bitcoind` is started using the same datadir it will fail to start up, but during shutdown remove the PID file from the first `bitcoind` instance.

ACKs for top commit:
  achow101:
    ACK 8f6ab31
  andrewtoth:
    ACK 8f6ab31
  romanz:
    ACK bitcoin/bitcoin@8f6ab31

Tree-SHA512: c9af703cbfa179d33ef9580a51e86c1b0acbd28daa18c8d2e5e5ff796ab4d3e2009a962a47e6046a0e5ece936f8a06ee8af5fdf8ff4ae1e52cbcdbec4b942271

Author: achow101

src/init.cpp

@@ -155,6 +155,11 @@ static const char* DEFAULT_ASMAP_FILENAME="ip_asn.map";
  * The PID file facilities.
  */
 static const char* BITCOIN_PID_FILENAME = "bitcoind.pid";
+/**
+ * True if this process has created a PID file.
+ * Used to determine whether we should remove the PID file on shutdown.
+ */
+static bool g_generated_pid{false};
 
 static fs::path GetPidFile(const ArgsManager& args)
 {
@@ -170,12 +175,24 @@ static fs::path GetPidFile(const ArgsManager& args)
 #else
         tfm::format(file, "%d\n", getpid());
 #endif
+        g_generated_pid = true;
         return true;
     } else {
         return InitError(strprintf(_("Unable to create the PID file '%s': %s"), fs::PathToString(GetPidFile(args)), SysErrorString(errno)));
     }
 }
 
+static void RemovePidFile(const ArgsManager& args)
+{
+    if (!g_generated_pid) return;
+    const auto pid_path{GetPidFile(args)};
+    if (std::error_code error; !fs::remove(pid_path, error)) {
+        std::string msg{error ? error.message() : "File does not exist"};
+        LogPrintf("Unable to remove PID file (%s): %s\n", fs::PathToString(pid_path), msg);
+    }
+}
+
+
 //////////////////////////////////////////////////////////////////////////////
 //
 // Shutdown
@@ -352,13 +369,7 @@ void Shutdown(NodeContext& node)
     node.scheduler.reset();
     node.kernel.reset();
 
-    try {
-        if (!fs::remove(GetPidFile(*node.args))) {
-            LogPrintf("%s: Unable to remove PID file: File does not exist\n", __func__);
-        }
-    } catch (const fs::filesystem_error& e) {
-        LogPrintf("%s: Unable to remove PID file: %s\n", __func__, fsbridge::get_filesystem_error_message(e));
-    }
+    RemovePidFile(*node.args);
 
     LogPrintf("%s: done\n", __func__);
 }

test/functional/feature_filelock.py

@@ -30,8 +30,11 @@ def run_test(self):
         expected_msg = f"Error: Cannot obtain a lock on data directory {datadir}. {self.config['environment']['PACKAGE_NAME']} is probably already running."
         self.nodes[1].assert_start_raises_init_error(extra_args=[f'-datadir={self.nodes[0].datadir_path}', '-noserver'], expected_msg=expected_msg)
 
+        self.log.info("Check that cookie and PID file are not deleted when attempting to start a second bitcoind using the same datadir")
         cookie_file = datadir / ".cookie"
         assert cookie_file.exists()  # should not be deleted during the second bitcoind instance shutdown
+        pid_file = datadir / "bitcoind.pid"
+        assert pid_file.exists()
 
         if self.is_wallet_compiled():
             def check_wallet_filelock(descriptors):