improve pidfile handling

This patch centralize the logic of creating & reading pid files into the
new pid_file module and improves upon / makes explicit a few race conditions
that existed with the previous code.

Starting Processes / Creating Pidfiles
======================================

Before this patch, we had three places that had very similar-looking
    match lock_file::create_lock_file { ... }
blocks.
After this change, they can use a straight-forward call provided
by the pid_file:
    pid_file::claim_pid_file_for_pid()

Stopping Processes / Reading Pidfiles
=====================================

The new pid_file module provides a function to read a pidfile,
called read_pidfile(), that returns a

  pub enum PidFileRead {
      NotExist,
      NotHeldByAnyProcess(PidFileGuard),
      LockedByOtherProcess(Pid),
  }

If we get back NotExist, there is nothing to kill.

If we get back NotHeldByAnyProcess, the pid file is stale and we must
ignore its contents.

If it's LockedByOtherProcess, it's either another pidfile reader
or, more likely, the daemon that is still running.
In this case, we can read the pid in the pidfile and kill it.
There's still a small window where this is racy, but it's not a
regression compared to what we have before.

The NotHeldByAnyProcess is an improvement over what we had before
this patch. Before, we would blindly read the pidfile contents
and kill, even if no other process held the flock.
If the pidfile was stale (NotHeldByAnyProcess), then that kill
would either result in ESRCH or hit some other unrelated process
on the system. This patch avoids the latter cacse by grabbing
an exclusive flock before reading the pidfile, and returning the
flock to the caller in the form of a guard object, to avoid
concurrent reads / kills.
It's hopefully irrelevant in practice, but it's a little robustness
that we get for free here.

Maintain flock on Pidfile of ETCD / any InitialPidFile::Create()
================================================================

Pageserver and safekeeper create their pidfiles themselves.
But for etcd, neon_local creates the pidfile (InitialPidFile::Create()).

Before this change, we would unlock the etcd pidfile as soon as
`neon_local start` exits, simply because no-one else kept the FD open.

During `neon_local stop`, that results in a stale pid file,
aka, NotHeldByAnyProcess, and it would henceforth not trust that
the PID stored in the file is still valid.

With this patch, we make the etcd process inherit the pidfile FD,
thereby keeping the flock held until it exits.

control_plane/src/background_process.rs

@@ -14,17 +14,19 @@
 
 use std::ffi::OsStr;
 use std::io::Write;
-use std::path::Path;
+use std::os::unix::prelude::AsRawFd;
+use std::os::unix::process::CommandExt;
+use std::path::{Path, PathBuf};
 use std::process::{Child, Command};
 use std::time::Duration;
 use std::{fs, io, thread};
 
-use anyhow::{anyhow, bail, Context, Result};
+use anyhow::Context;
 use nix::errno::Errno;
+use nix::fcntl::{FcntlArg, FdFlag};
 use nix::sys::signal::{kill, Signal};
 use nix::unistd::Pid;
-
-use utils::lock_file;
+use utils::pid_file::{self, PidFileRead};
 
 // These constants control the loop used to poll for process start / stop.
 //
@@ -86,6 +88,14 @@ where
     let filled_cmd = fill_aws_secrets_vars(fill_rust_env_vars(background_command));
     filled_cmd.envs(envs);
 
+    let pid_file_to_check = match initial_pid_file {
+        InitialPidFile::Create(path) => {
+            pre_exec_create_pidfile(filled_cmd, path);
+            path
+        }
+        InitialPidFile::Expect(path) => path,
+    };
+
     let mut spawned_process = filled_cmd.spawn().with_context(|| {
         format!("Could not spawn {process_name}, see console output and log files for details.")
     })?;
@@ -95,29 +105,8 @@ where
             .with_context(|| format!("Subprocess {process_name} has invalid pid {pid}"))?,
     );
 
-    let pid_file_to_check = match initial_pid_file {
-        InitialPidFile::Create(target_pid_file_path) => {
-            match lock_file::create_lock_file(target_pid_file_path, pid.to_string()) {
-                lock_file::LockCreationResult::Created { .. } => {
-                    // We use "lock" file here only to create the pid file. The lock on the pidfile will be dropped as soon
-                    // as this CLI invocation exits, so it's a bit useless, but doesn't any harm either.
-                }
-                lock_file::LockCreationResult::AlreadyLocked { .. } => {
-                    anyhow::bail!("Cannot write pid file for {process_name} at path {target_pid_file_path:?}: file is already locked by another process")
-                }
-                lock_file::LockCreationResult::CreationFailed(e) => {
-                    return Err(e.context(format!(
-                    "Failed to create pid file for {process_name} at path {target_pid_file_path:?}"
-                )))
-                }
-            }
-            None
-        }
-        InitialPidFile::Expect(pid_file_path) => Some(pid_file_path),
-    };
-
     for retries in 0..RETRIES {
-        match process_started(pid, pid_file_to_check, &process_status_check) {
+        match process_started(pid, Some(pid_file_to_check), &process_status_check) {
             Ok(true) => {
                 println!("\n{process_name} started, pid: {pid}");
                 return Ok(spawned_process);
@@ -165,12 +154,27 @@ pub fn send_stop_child_process(child: &std::process::Child) -> anyhow::Result<()
 
 /// Stops the process, using the pid file given. Returns Ok also if the process is already not running.
 pub fn stop_process(immediate: bool, process_name: &str, pid_file: &Path) -> anyhow::Result<()> {
-    if !pid_file.exists() {
-        println!("{process_name} is already stopped: no pid file {pid_file:?} is present");
-        return Ok(());
-    }
-    let pid = read_pidfile(pid_file)?;
+    let pid = match pid_file::read(pid_file)
+        .with_context(|| format!("read pid_file {pid_file:?}"))?
+    {
+        PidFileRead::NotExist => {
+            println!("{process_name} is already stopped: no pid file present at {pid_file:?}");
+            return Ok(());
+        }
+        PidFileRead::NotHeldByAnyProcess(_) => {
+            // Don't try to kill according to file contents beacuse the pid might have been re-used by another process.
+            // Don't delete the file either, it can race with new pid file creation.
+            // Read `pid_file` module comment for details.
+            println!(
+                "No process is holding the pidfile. The process must have already exited. Leave in place to avoid race conditions: {pid_file:?}"
+            );
+            return Ok(());
+        }
+        PidFileRead::LockedByOtherProcess(pid) => pid,
+    };
+    // XXX the pid could become invalid (and recycled) at any time before the kill() below.
 
+    // send signal
     let sig = if immediate {
         print!("Stopping {process_name} with pid {pid} immediately..");
         Signal::SIGQUIT
@@ -182,8 +186,9 @@ pub fn stop_process(immediate: bool, process_name: &str, pid_file: &Path) -> any
     match kill(pid, sig) {
         Ok(()) => (),
         Err(Errno::ESRCH) => {
+            // Again, don't delete the pid file. The unlink can race with a new pid file being created.
             println!(
-                "{process_name} with pid {pid} does not exist, but a pid file {pid_file:?} was found"
+                "{process_name} with pid {pid} does not exist, but a pid file {pid_file:?} was found. Likely the pid got recycled. Lucky we didn't harm anyone."
             );
             return Ok(());
         }
@@ -252,6 +257,69 @@ fn fill_aws_secrets_vars(mut cmd: &mut Command) -> &mut Command {
     cmd
 }
 
+/// Add a `pre_exec` to the cmd that, inbetween fork() and exec(),
+/// 1. Claims a pidfile with a fcntl lock on it and
+/// 2. Sets up the pidfile's file descriptor so that it (and the lock)
+///    will remain held until the cmd exits.
+fn pre_exec_create_pidfile<P>(cmd: &mut Command, path: P) -> &mut Command
+where
+    P: Into<PathBuf>,
+{
+    let path: PathBuf = path.into();
+    // SAFETY
+    // pre_exec is marked unsafe because it runs between fork and exec.
+    // Why is that dangerous in various ways?
+    // Long answer:  https://github.com/rust-lang/rust/issues/39575
+    // Short answer: in a multi-threaded program, other threads may have
+    // been inside of critical sections at the time of fork. In the
+    // original process, that was allright, assuming they protected
+    // the critical sections appropriately, e.g., through locks.
+    // Fork adds another process to the mix that
+    //   1. Has a single thread T
+    //   2. In an exact copy of the address space at the time of fork.
+    // A variety of problems scan occur now:
+    //   1. T tries to grab a lock that was locked at the time of fork.
+    //      It will wait forever since in its address space, the lock
+    //      is in state 'taken' but the thread that would unlock it is
+    //      not there.
+    //   2. A rust object that represented some external resource in the
+    //      parent now got implicitly copied by the the fork, even though
+    //      the object's type is not `Copy`. The parent program may use
+    //      non-copyability as way to enforce unique ownership of an
+    //      external resource in the typesystem. The fork breaks that
+    //      assumption, as now both parent and child process have an
+    //      owned instance of the object that represents the same
+    //      underlying resource.
+    // While these seem like niche problems, (1) in particular is
+    // highly relevant. For example, `malloc()` may grab a mutex internally,
+    // and so, if we forked while another thread was mallocing' and our
+    // pre_exec closure allocates as well, it will block on the malloc
+    // mutex forever
+    //
+    // The proper solution is to only use C library functions that are marked
+    // "async-signal-safe": https://man7.org/linux/man-pages/man7/signal-safety.7.html
+    //
+    // With this specific pre_exec() closure, the non-error path doesn't allocate.
+    // The error path uses `anyhow`, and hence does allocate.
+    // We take our chances there, hoping that any potential disaster is constrained
+    // to the child process (e.g., malloc has no state ourside of the child process).
+    // Last, `expect` prints to stderr, and stdio is not async-signal-safe.
+    // Again, we take our chances, making the same assumptions as for malloc.
+    unsafe {
+        cmd.pre_exec(move || {
+            let file = pid_file::claim_for_current_process(&path).expect("claim pid file");
+            // Remove the FD_CLOEXEC flag on the pidfile descriptor so that the pidfile
+            // remains locked after exec.
+            nix::fcntl::fcntl(file.as_raw_fd(), FcntlArg::F_SETFD(FdFlag::empty()))
+                .expect("remove FD_CLOEXEC");
+            // Don't run drop(file), it would close the file before we actually exec.
+            std::mem::forget(file);
+            Ok(())
+        });
+    }
+    cmd
+}
+
 fn process_started<F>(
     pid: Pid,
     pid_file_to_check: Option<&Path>,
@@ -262,14 +330,11 @@ where
 {
     match status_check() {
         Ok(true) => match pid_file_to_check {
-            Some(pid_file_path) => {
-                if pid_file_path.exists() {
-                    let pid_in_file = read_pidfile(pid_file_path)?;
-                    Ok(pid_in_file == pid)
-                } else {
-                    Ok(false)
-                }
-            }
+            Some(pid_file_path) => match pid_file::read(pid_file_path)? {
+                PidFileRead::NotExist => Ok(false),
+                PidFileRead::LockedByOtherProcess(pid_in_file) => Ok(pid_in_file == pid),
+                PidFileRead::NotHeldByAnyProcess(_) => Ok(false),
+            },
             None => Ok(true),
         },
         Ok(false) => Ok(false),
@@ -277,21 +342,6 @@ where
     }
 }
 
-/// Read a PID file
-///
-/// We expect a file that contains a single integer.
-fn read_pidfile(pidfile: &Path) -> Result<Pid> {
-    let pid_str = fs::read_to_string(pidfile)
-        .with_context(|| format!("failed to read pidfile {pidfile:?}"))?;
-    let pid: i32 = pid_str
-        .parse()
-        .map_err(|_| anyhow!("failed to parse pidfile {pidfile:?}"))?;
-    if pid < 1 {
-        bail!("pidfile {pidfile:?} contained bad value '{pid}'");
-    }
-    Ok(Pid::from_raw(pid))
-}
-
 fn process_has_stopped(pid: Pid) -> anyhow::Result<bool> {
     match kill(pid, None) {
         // Process exists, keep waiting

control_plane/src/bin/neon_local.rs

@@ -324,7 +324,7 @@ fn handle_init(init_match: &ArgMatches) -> anyhow::Result<LocalEnv> {
             pg_version,
         )
         .unwrap_or_else(|e| {
-            eprintln!("pageserver init failed: {e}");
+            eprintln!("pageserver init failed: {e:?}");
             exit(1);
         });