cleanup: Improve robustness when multiple cleanups run concurrently

The file count/size counters are now intentionally subtracted even if
there file to delete has disappeared since the final cache size
calculation will be incorrect if they aren’t. This can happen when there
are several parallel ongoing cleanups of the same subdirectory.

Also removed the “delete sibling files” logic; it’s unnecessary for all
siblings except .stderr since that’s the only file in a result that is
optional. Any other missing file will be detected by
get_file_from_cache.

4 files changed, 89 insertions, 86 deletions

diff --git a/doc/NEWS.adoc b/doc/NEWS.adoc
index 3ac64788..f3f54716 100644
--- a/doc/NEWS.adoc
+++ b/doc/NEWS.adoc
@@ -8,6 +8,12 @@ Release date: unknown
 Bug fixes
 ~~~~~~~~~
 
+- The cleanup algorithm has been fixed to not misbehave when files are removed
+  by another process while the cleanup process is running. Previously, too many
+  files could be removed from the cache if multiple cleanup processes were
+  triggered at the same time, in extreme cases trimming the cache to a much
+  smaller size than the configured limits.
+
 - Correctly hash preprocessed headers located in a ``.gch directory''.
   Previously, ccache would not pick up changes to such precompiled headers,
   risking false positive cache hits.
diff --git a/src/cleanup.c b/src/cleanup.c
index 557a5ad8..8c46eac5 100644
--- a/src/cleanup.c
+++ b/src/cleanup.c
@@ -89,27 +89,19 @@ out:
 }
 
 static void
-delete_file(const char *path, size_t size)
+delete_file(const char *path, size_t size, bool update_counters)
 {
-	if (x_unlink(path) == 0) {
+	bool deleted = x_try_unlink(path) == 0;
+	if (!deleted && errno != ENOENT && errno != ESTALE) {
+		cc_log("Failed to unlink %s (%s)", path, strerror(errno));
+	} else if (update_counters) {
+		// The counters are intentionally subtracted even if there was no file to
+		// delete since the final cache size calculation will be incorrect if they
+		// aren't. (This can happen when there are several parallel ongoing
+		// cleanups of the same directory.)
 		cache_size -= size;
 		files_in_cache--;
-	} else if (errno != ENOENT && errno != ESTALE) {
-		cc_log("Failed to unlink %s (%s)", path, strerror(errno));
-	}
-}
-
-static void
-delete_sibling_file(const char *base, const char *extension)
-{
-	struct stat st;
-	char *path = format("%s%s", base, extension);
-	if (lstat(path, &st) == 0) {
-		delete_file(path, file_size(&st));
-	} else if (errno != ENOENT && errno != ESTALE) {
-		cc_log("Failed to stat %s: %s", path, strerror(errno));
 	}
-	free(path);
 }
 
 // Sort the files we've found and delete the oldest ones until we are below the
@@ -123,7 +115,6 @@ sort_and_clean(void)
 	}
 
 	// Delete enough files to bring us below the threshold.
-	char *last_base = x_strdup("");
 	bool cleaned = false;
 	for (unsigned i = 0; i < num_files; i++) {
 		const char *ext;
@@ -136,33 +127,29 @@ sort_and_clean(void)
 		}
 
 		ext = get_extension(files[i]->fname);
-		if (str_eq(ext, ".o")
-		    || str_eq(ext, ".d")
-		    || str_eq(ext, ".gcno")
-		    || str_eq(ext, ".dia")
-		    || str_eq(ext, ".stderr")) {
+		if (str_eq(ext, ".stderr")) {
+			// Make sure that the .o file is deleted before .stderr, because if the
+			// ccache process gets killed after deleting the .stderr but before
+			// deleting the .o, the cached result will be inconsistent. (.stderr is
+			// the only file that is optional; any other file missing from the cache
+			// will be detected by get_file_from_cache.)
 			char *base = remove_extension(files[i]->fname);
-			if (!str_eq(base, last_base)) { // Avoid redundant unlinks.
-				// Make sure that all sibling files are deleted so that a cached result
-				// is removed completely. Note the order of deletions -- the stderr
-				// file must be deleted last because if the ccache process gets killed
-				// after deleting the .stderr but before deleting the .o, the cached
-				// result would be inconsistent.
-				delete_sibling_file(base, ".o");
-				delete_sibling_file(base, ".d");
-				delete_sibling_file(base, ".gcno");
-				delete_sibling_file(base, ".dia");
-				delete_sibling_file(base, ".stderr");
-			}
-			free(last_base);
-			last_base = base;
-		} else {
-			// .manifest or unknown file.
-			delete_file(files[i]->fname, files[i]->size);
+			char *o_file = format("%s.o", base);
+
+			// Don't subtract this extra deletion from the cache size; that
+			// bookkeeping will be done when the loop reaches the .o file. If the
+			// loop doesn't reach the .o file since the target limits have been
+			// reached, the bookkeeping won't happen, but that small counter
+			// discrepancy won't do much harm and it will correct itself in the next
+			// cleanup.
+			delete_file(o_file, 0, false);
+
+			free(o_file);
+			free(base);
 		}
+		delete_file(files[i]->fname, files[i]->size, true);
 		cleaned = true;
 	}
-	free(last_base);
 	return cleaned;
 }
 
diff --git a/test/run b/test/run
index f065316c..82a80541 100755
--- a/test/run
+++ b/test/run
@@ -97,7 +97,13 @@ sed_in_place() {
 }
 
 backdate() {
-    touch -t 199901010000 "$@"
+    if [[ $1 =~ ^[0-9]+$ ]]; then
+        m=$1
+        shift
+    else
+        m=0
+    fi
+    touch -t 1999010100$(printf "%02u" $m) "$@"
 }
 
 expect_stat() {
@@ -176,7 +182,7 @@ expect_file_count() {
     local expected=$1
     local pattern=$2
     local dir=$3
-    local actual=`find $dir -name "$pattern" | wc -l`
+    local actual=`find $dir -type f -name "$pattern" | wc -l`
     if [ $actual -ne $expected ]; then
         test_failed "Found $actual (expected $expected) $pattern files in $dir"
     fi
diff --git a/test/suites/cleanup.bash b/test/suites/cleanup.bash
index 42638c76..8eaac4f6 100644
--- a/test/suites/cleanup.bash
+++ b/test/suites/cleanup.bash
@@ -5,11 +5,9 @@ prepare_cleanup_test_dir() {
     mkdir -p $dir
     for i in $(seq 0 9); do
         printf '%4017s' '' | tr ' ' 'A' >$dir/result$i-4017.o
-        touch $dir/result$i-4017.stderr
-        touch $dir/result$i-4017.d
-        if [ $i -gt 5 ]; then
-            backdate $dir/result$i-4017.stderr
-        fi
+        backdate $((3 * i + 1)) $dir/result$i-4017.o
+        backdate $((3 * i + 2)) $dir/result$i-4017.d
+        backdate $((3 * i + 3)) $dir/result$i-4017.stderr
     done
     # NUMFILES: 30, TOTALSIZE: 40 KiB, MAXFILES: 0, MAXSIZE: 0
     echo "0 0 0 0 0 0 0 0 0 0 0 30 40 0 0" >$dir/stats
@@ -59,21 +57,21 @@ SUITE_cleanup() {
 
     # Reduce file limit
     #
-    # 21 * 16 = 336
-    $CCACHE -F 336 -M 0 >/dev/null
+    # 22 * 16 = 352
+    $CCACHE -F 352 -M 0 >/dev/null
     $CCACHE -c >/dev/null
     expect_file_count 7 '*.o' $CCACHE_DIR
     expect_file_count 7 '*.d' $CCACHE_DIR
-    expect_file_count 7 '*.stderr' $CCACHE_DIR
-    expect_stat 'files in cache' 21
+    expect_file_count 8 '*.stderr' $CCACHE_DIR
+    expect_stat 'files in cache' 22
     expect_stat 'cleanups performed' 1
-    for i in 0 1 2 3 4 5 9; do
+    for i in 0 1 2; do
         file=$CCACHE_DIR/a/result$i-4017.o
-        expect_file_exists $file
+        expect_file_missing $CCACHE_DIR/a/result$i-4017.o
     done
-    for i in 6 7 8; do
+    for i in 3 4 5 6 7 8 9; do
         file=$CCACHE_DIR/a/result$i-4017.o
-        expect_file_missing $file
+        expect_file_exists $file
     done
 
     # -------------------------------------------------------------------------
@@ -91,17 +89,17 @@ SUITE_cleanup() {
     $CCACHE -F 0 -M 256K >/dev/null
     CCACHE_LOGFILE=/tmp/foo $CCACHE -c >/dev/null
     expect_file_count 3 '*.o' $CCACHE_DIR
-    expect_file_count 3 '*.d' $CCACHE_DIR
-    expect_file_count 3 '*.stderr' $CCACHE_DIR
-    expect_stat 'files in cache' 9
+    expect_file_count 4 '*.d' $CCACHE_DIR
+    expect_file_count 4 '*.stderr' $CCACHE_DIR
+    expect_stat 'files in cache' 11
     expect_stat 'cleanups performed' 1
-    for i in 3 4 5; do
+    for i in 0 1 2 3 4 5 6; do
         file=$CCACHE_DIR/a/result$i-4017.o
-        expect_file_exists $file
+        expect_file_missing $file
     done
-    for i in 0 1 2 6 7 8 9; do
+    for i in 7 8 9; do
         file=$CCACHE_DIR/a/result$i-4017.o
-        expect_file_missing $file
+        expect_file_exists $file
     done
 
     # -------------------------------------------------------------------------
@@ -122,9 +120,9 @@ SUITE_cleanup() {
     touch empty.c
     CCACHE_LIMIT_MULTIPLE=0.9 $CCACHE_COMPILE -c empty.c -o empty.o
     expect_file_count 159 '*.o' $CCACHE_DIR
-    expect_file_count 158 '*.d' $CCACHE_DIR
-    expect_file_count 158 '*.stderr' $CCACHE_DIR
-    expect_stat 'files in cache' 475
+    expect_file_count 159 '*.d' $CCACHE_DIR
+    expect_file_count 159 '*.stderr' $CCACHE_DIR
+    expect_stat 'files in cache' 477
     expect_stat 'cleanups performed' 1
 
     # -------------------------------------------------------------------------
@@ -145,32 +143,38 @@ SUITE_cleanup() {
     touch empty.c
     CCACHE_LIMIT_MULTIPLE=0.7 $CCACHE_COMPILE -c empty.c -o empty.o
     expect_file_count 157 '*.o' $CCACHE_DIR
-    expect_file_count 156 '*.d' $CCACHE_DIR
-    expect_file_count 156 '*.stderr' $CCACHE_DIR
-    expect_stat 'files in cache' 469
+    expect_file_count 157 '*.d' $CCACHE_DIR
+    expect_file_count 157 '*.stderr' $CCACHE_DIR
+    expect_stat 'files in cache' 471
     expect_stat 'cleanups performed' 1
 
     # -------------------------------------------------------------------------
-    TEST "Cleanup of sibling files"
+    TEST ".o file is removed before .stderr"
 
     prepare_cleanup_test_dir $CCACHE_DIR/a
+    $CCACHE -F 474 -M 0 >/dev/null
+    backdate 0 $CCACHE_DIR/a/result9-4017.stderr
+    $CCACHE -c >/dev/null
+    expect_file_missing $CCACHE_DIR/a/result9-4017.stderr
+    expect_file_missing $CCACHE_DIR/a/result9-4017.o
+
+    # Counters expectedly doesn't match reality if x.stderr is found before
+    # x.o and the cleanup stops before x.o is found.
+    expect_stat 'files in cache' 29
+    expect_file_count 28 '*.*' $CCACHE_DIR/a
+
+    # -------------------------------------------------------------------------
+    TEST ".stderr file is not removed before .o"
 
-    $CCACHE -F 336 -M 0 >/dev/null
-    backdate $CCACHE_DIR/a/result2-4017.stderr
+    prepare_cleanup_test_dir $CCACHE_DIR/a
+    $CCACHE -F 474 -M 0 >/dev/null
+    backdate 0 $CCACHE_DIR/a/result9-4017.o
     $CCACHE -c >/dev/null
-    # floor(0.8 * 9) = 7
-    expect_file_count 7 '*.o' $CCACHE_DIR
-    expect_file_count 7 '*.d' $CCACHE_DIR
-    expect_file_count 7 '*.stderr' $CCACHE_DIR
-    expect_stat 'files in cache' 21
-    for i in 0 1 3 4 5 8 9; do
-        file=$CCACHE_DIR/a/result$i-4017.o
-        expect_file_exists $file
-    done
-    for i in 2 6 7; do
-        file=$CCACHE_DIR/a/result$i-4017.o
-        expect_file_missing $file
-    done
+    expect_file_exists $CCACHE_DIR/a/result9-4017.stderr
+    expect_file_missing $CCACHE_DIR/a/result9-4017.o
+
+    expect_stat 'files in cache' 29
+    expect_file_count 29 '*.*' $CCACHE_DIR/a
 
     # -------------------------------------------------------------------------
     TEST "No cleanup of new unknown file"
@@ -184,7 +188,7 @@ SUITE_cleanup() {
     $CCACHE -F 480 -M 0 >/dev/null
     $CCACHE -c >/dev/null
     expect_file_exists $CCACHE_DIR/a/abcd.unknown
-    expect_stat 'files in cache' 28
+    expect_stat 'files in cache' 30
 
     # -------------------------------------------------------------------------
     TEST "Cleanup of old unknown file"