Script to clean up old files that should only run one instance

Question

I'm writing a bash script for deleting old files on a Linux server. Here are some relevant facts about this script:

• As currently written, it does work as expected. However, it's entirely possible there's some edge case I haven't considered (which is part of my reason for posting here).
• It will be run as root once a day, scheduled via cron.
• Only one instance should run at a time. If another instance is started, it should just exit.
• It will delete any file whose mtime is over 30 days old, from the time of find's invocation.
• Each run of the script will take a few hours.
• The script will be running on a single CentOS 7 node, but it operates on a distributed file system exposed to hundreds of others.

Here is the actual script:

#!/usr/bin/env bash

NUM_PROCS=4
LIMIT_TIME=30 # days
PIDFILE="/tmp/cleanup.pid"
PID_NOW="$$"

function save-pid() {
    echo "$1" > "$PIDFILE"

    if [[ "$?" -ne 0 ]]; then
        # If we couldn't save the PID to the lockfile...
        echo "Failed to create PID file for PID $1 in $PIDFILE; exiting"
        exit 1
    fi
}

function clean-up() {
    rm "$PIDFILE"
}

cd /scratch

if [[ -f "$PIDFILE" ]]; then
    # If the lock file exists... (the script is either running another
    # instance or improperly exited earlier)
    read PID_SAVED < "$PIDFILE" # Get the PID of the running cleanup script

    if ps --pid "$PID_SAVED" &> /dev/null; then
        # If a PID with this process exists...
        echo "Cleanup script is already running with PID $PID_SAVED; exiting"
        exit 1
    else
        save-pid "$PID_NOW"
    fi
else
    save-pid "$PID_NOW"
fi

trap clean-up HUP INT QUIT ILL ABRT BUS SEGV PIPE TERM ERR
# If any further command raises a problematic signal or exits in error,
# clean up $PIDFILE on the way out

ls | xargs -P "$NUM_PROCS" -I{} \
    find {} -type f -mtime "+$LIMIT_TIME" -delete


clean-up

I'll consider any advice you have to offer, but I'm specifically curious about how I can improve my file locking and error handling.

Answer 1

General impressions - nice neat code, well commented. It's good that you use trap to clean up on exit.

Shellcheck picks up a couple of oversights:

206596.sh:11:11: note: Check exit code directly with e.g. 'if mycmd;', not indirectly with $?. [SC2181]
206596.sh:22:1: warning: Use 'cd ... || exit' or 'cd ... || return' in case cd fails. [SC2164]
206596.sh:27:5: note: read without -r will mangle backslashes. [SC2162]
206596.sh:44:1: warning: Use 'find .. -print0 | xargs -0 ..' or 'find .. -exec .. +' to allow non-alphanumeric filenames. [SC2011]

---

Let's do a walk-through:

• #!/bin/bash
  

  Why bash? I don't think there's anything here that can't be achieved with standard POSIX shell.

• NUM_PROCS=4
  

  What's special about 4? If it's the number of cores you have available, then we could automate that:

  NUM_PROCS=$(getconf _NPROCESSORS_ONLN)
  

• PIDFILE="/tmp/cleanup.pid"
  

  I'm not sure about CentOS, but on the platforms I'm more familiar with, pidfiles normally live under /run (or /var/run on older installations).

• PID_NOW="$$"
  

  Never modified; I think it's clearer to simply use $$ where it's needed.

• echo "$1" > "$PIDFILE"
  
  if [[ "$?" -ne 0 ]]; then
  

  Instead of testing $?, we should use the result directly (and since we never call this with anything other than our own pid, let's just embed that in the function rather than passing as argument). Give the function a POSIX-compliant name, and declare it without the deprecated function keyword:

  save_pid() {
      if ! echo $$ >"$PIDFILE"
      then
          # If we couldn't save the PID to the lockfile...
          echo >&2 "Failed to create PID file for PID $$ in $PIDFILE; exiting"
          exit 1
      fi
  }
  

  Note that the message is printed to standard error (stream 2) rather than standard output.

• cd /scratch
  

  There's no checking that this command succeeded. We could append || exit 1 or similar, but we can catch more errors by setting shell flags -e and -u at the beginning of the script.

  There's really no need to change working directory at all; we could just omit this command, and use the directory name where we need it.

• if [[ -f "$PIDFILE" ]]
  

  The comment says we're testing for existence, so we should be using -e (or change the comment). And we can use the standard [ command; there's nothing to be gained from Bash-specific [[.

•     read PID_SAVED < "$PIDFILE"
  

  Shellcheck complains about using read without -r option. Whilst we don't expect to see shell escapes in this file, there's no harm in pacifying the warning. Or, we could just interpolate the file directly:

  if ps --quick-pid "$(cat $PIDFILE)" &> /dev/null
  

  Or just test for the existence in procfs:

  if [ -e "/proc/$(cat $PIDFILE)" ]
  

  We could add further tests to ensure that we don't have a re-used pid that's running a different program, but I won't pursue that here.

• This structure can be simplified:

  if a
  then
      if b
      then exit 1
      else do-c
      fi
  else do-c
  fi
  

  It's exactly equivalent to

  if a && b
  then exit 1
  fi
  do-c
  

  That simplification gives us:

  if [ -e "$PIDFILE" ] && [ -e "/proc/$(cat $PIDFILE)" ]
  then
      # A process exists with our saved PID
      echo >&2 "Cleanup script is already running with PID $PID_SAVED; exiting"
      exit 1
  fi
  
  save_pid
  

• trap clean-up HUP INT QUIT ILL ABRT BUS SEGV PIPE TERM ERR
  

  Instead of listing all these signals, we just want to clean up however the shell exits:

  trap clean_up EXIT
  

• ls | xargs -P "$NUM_PROCS" -I{} \
  

  It's dangerous to pipe the output of ls like that. Files not under your control can contain almost anything, including newlines and other significant space. I don't think we really want to ignore dot-files, either. I recommend find -maxdepth 1 -mindepth 1, which can then write safely to xargs using --print0. We might want to eliminate symlinks from the listing, although we know that the inner find won't follow them without -H or -L.

• save_pid and clean_up are only used once each now, so could be usefully inlined.

---

Modified code

#!/bin/sh

set -eu

LIMIT_TIME=30 # days
NUM_PROCS=$(getconf _NPROCESSORS_ONLN)
PIDFILE="/var/run/cleanup.pid"

# check pidfile
if [ -e "$PIDFILE" ] && [ -e "/proc/$(cat $PIDFILE)" ]
then
    # A process exists with our saved PID
    echo >&2 "Cleanup script is already running with PID $PID_SAVED; exiting"
    exit 1
fi

# write pidfile
    if ! echo $$ >"$PIDFILE"
then
    # If we couldn't save the PID to the lockfile...
    echo >&2 "Failed to create PID file for PID $$ in $PIDFILE; exiting"
        exit 1
    fi

trap 'rm "$PIDFILE"' EXIT

# Now start working
find /scratch -maxdepth 1 -mindepth 1 \! -type l --print0 \
     | xargs -0 -P "$NUM_PROCS" -I{} \
             find {} -xdev -type f -mtime "+$LIMIT_TIME" -delete

---

After all that, consider that you might be reinventing a wheel. Most likely, you have a tmpreaper package available that can be configured to do exactly this task.