Merge 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 bitcoin#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@8f6ab31

Tree-SHA512: c9af703cbfa179d33ef9580a51e86c1b0acbd28daa18c8d2e5e5ff796ab4d3e2009a962a47e6046a0e5ece936f8a06ee8af5fdf8ff4ae1e52cbcdbec4b942271

Author: achow101

src/init.cpp

@@ -178,6 +178,11 @@ static const char* DEFAULT_ASMAP_FILENAME="ip_asn.map";
  * The PID file facilities.
  */
 static const char* BITCOIN_PID_FILENAME = "dashd.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)
 {
@@ -193,12 +198,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
@@ -419,13 +436,7 @@ void Shutdown(NodeContext& node)
     node.chainman.reset();
     node.scheduler.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);
 
     node.args = nullptr;
     LogPrintf("%s: done\n", __func__);

test/functional/feature_filelock.py

@@ -1,4 +1,4 @@
-#!/usr/bin/env python3
+#\!/usr/bin/env python3
 # Copyright (c) 2018-2020 The Bitcoin Core developers
 # Distributed under the MIT software license, see the accompanying
 # file COPYING or http://www.opensource.org/licenses/mit-license.php.
@@ -28,6 +28,12 @@ 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}', '-noserver'], expected_msg=expected_msg)
 
+        self.log.info("Check that cookie and PID file are not deleted when attempting to start a second dashd using the same datadir")
+        cookie_file = os.path.join(datadir, ".cookie")
+        assert os.path.exists(cookie_file)  # should not be deleted during the second dashd instance shutdown
+        pid_file = os.path.join(datadir, "dashd.pid")
+        assert os.path.exists(pid_file)
+
         if self.is_wallet_compiled():
             def check_wallet_filelock(descriptors):
                 wallet_name = ''.join([random.choice(string.ascii_lowercase) for _ in range(6)])
@@ -47,3 +53,4 @@ def check_wallet_filelock(descriptors):
 
 if __name__ == '__main__':
     FilelockTest().main()
+EOF < /dev/null