Let a crashed scan's lock expire with its process

1 file changed, 40 insertions, 9 deletions

diff --git a/source/cgit.c b/source/cgit.cindex f284ad9..6644213 100644--- a/source/cgit.c+++ b/source/cgit.c@@ -335,22 +335,49 @@ static void parse_args(int argc, const char **argv) 	} } +/*+ * The lock is a fcntl lock rather than the mere existence of the lock file,+ * because a lock the kernel drops with its process cannot outlive a scan that+ * was killed mid-run. A leftover lock file used to count as a scan in+ * progress, and one crash would silently freeze the repolist for good.+ */ static int generate_cached_repolist(const char *path, const char *cached_rc) { 	struct strbuf locked_rc = STRBUF_INIT;+	struct flock lock = {+		.l_type = F_WRLCK,+		.l_whence = SEEK_SET,+		.l_start = 0,+		.l_len = 0,+	}; 	int err = 0;+	int fd; 	int first; 	FILE *f;  	strbuf_addf(&locked_rc, "%s.lock", cached_rc);-	f = fopen(locked_rc.buf, "wx");-	if (!f) {-		// An existing lock file only means concurrent requests, which+	fd = open(locked_rc.buf, O_RDWR | O_CREAT, S_IRUSR | S_IWUSR);+	if (fd == -1) {+		err = errno;+		fprintf(stderr, "[cgit] Error opening %s: %s (%d)\n",+			locked_rc.buf, strerror(err), err);+		goto out;+	}+	if (fcntl(fd, F_SETLK, &lock) < 0) {+		// A lock held elsewhere only means concurrent requests, which 		// is not worth a line in the server log. 		err = errno;-		if (err != EEXIST)-			fprintf(stderr, "[cgit] Error opening %s: %s (%d)\n",-				locked_rc.buf, strerror(err), err);+		close(fd);+		goto out;+	}+	// A run that died before its rename leaves the lock file behind, so+	// start from empty now that nobody else can be writing it.+	if (ftruncate(fd, 0) < 0 || !(f = fdopen(fd, "w"))) {+		err = errno;+		fprintf(stderr, "[cgit] Error writing %s: %s (%d)\n",+			locked_rc.buf, strerror(err), err);+		unlink(locked_rc.buf);+		close(fd); 		goto out; 	} 	first = cgit_repolist.count;@@ -359,14 +386,15 @@ static int generate_cached_repolist(const char *path, const char *cached_rc) 	else 		scan_tree(path); 	print_repolist(f, &cgit_repolist, first);-	// Closed before the rename, because print_repolist writes through stdio-	// and a rename over the live file would otherwise publish a repolist+	// Flushed before the rename, because print_repolist writes through+	// stdio and a rename of the file would otherwise publish a repolist 	// that stops wherever the buffer happened to end.-	if (fclose(f)) {+	if (fflush(f) || ferror(f)) { 		err = errno; 		fprintf(stderr, "[cgit] Error writing %s: %s (%d)\n", 			locked_rc.buf, strerror(err), err); 		unlink(locked_rc.buf);+		fclose(f); 		goto out; 	} 	if (rename(locked_rc.buf, cached_rc)) {@@ -375,6 +403,9 @@ static int generate_cached_repolist(const char *path, const char *cached_rc) 			locked_rc.buf, cached_rc, strerror(err), err); 		unlink(locked_rc.buf); 	}+	// Closed after the rename so the lock is held until the fresh list is+	// in place, and nobody can truncate the file being renamed.+	fclose(f); out: 	strbuf_release(&locked_rc); 	return err;
diff --git a/source/cgit.c b/source/cgit.c
index f284ad9..6644213 100644
--- a/source/cgit.c
+++ b/source/cgit.c
@@ -335,22 +335,49 @@ static void parse_args(int argc, const char **argv)
 	}
 }
 
+/*
+ * The lock is a fcntl lock rather than the mere existence of the lock file,
+ * because a lock the kernel drops with its process cannot outlive a scan that
+ * was killed mid-run. A leftover lock file used to count as a scan in
+ * progress, and one crash would silently freeze the repolist for good.
+ */
 static int generate_cached_repolist(const char *path, const char *cached_rc)
 {
 	struct strbuf locked_rc = STRBUF_INIT;
+	struct flock lock = {
+		.l_type = F_WRLCK,
+		.l_whence = SEEK_SET,
+		.l_start = 0,
+		.l_len = 0,
+	};
 	int err = 0;
+	int fd;
 	int first;
 	FILE *f;
 
 	strbuf_addf(&locked_rc, "%s.lock", cached_rc);
-	f = fopen(locked_rc.buf, "wx");
-	if (!f) {
-		// An existing lock file only means concurrent requests, which
+	fd = open(locked_rc.buf, O_RDWR | O_CREAT, S_IRUSR | S_IWUSR);
+	if (fd == -1) {
+		err = errno;
+		fprintf(stderr, "[cgit] Error opening %s: %s (%d)\n",
+			locked_rc.buf, strerror(err), err);
+		goto out;
+	}
+	if (fcntl(fd, F_SETLK, &lock) < 0) {
+		// A lock held elsewhere only means concurrent requests, which
 		// is not worth a line in the server log.
 		err = errno;
-		if (err != EEXIST)
-			fprintf(stderr, "[cgit] Error opening %s: %s (%d)\n",
-				locked_rc.buf, strerror(err), err);
+		close(fd);
+		goto out;
+	}
+	// A run that died before its rename leaves the lock file behind, so
+	// start from empty now that nobody else can be writing it.
+	if (ftruncate(fd, 0) < 0 || !(f = fdopen(fd, "w"))) {
+		err = errno;
+		fprintf(stderr, "[cgit] Error writing %s: %s (%d)\n",
+			locked_rc.buf, strerror(err), err);
+		unlink(locked_rc.buf);
+		close(fd);
 		goto out;
 	}
 	first = cgit_repolist.count;
@@ -359,14 +386,15 @@ static int generate_cached_repolist(const char *path, const char *cached_rc)
 	else
 		scan_tree(path);
 	print_repolist(f, &cgit_repolist, first);
-	// Closed before the rename, because print_repolist writes through stdio
-	// and a rename over the live file would otherwise publish a repolist
+	// Flushed before the rename, because print_repolist writes through
+	// stdio and a rename of the file would otherwise publish a repolist
 	// that stops wherever the buffer happened to end.
-	if (fclose(f)) {
+	if (fflush(f) || ferror(f)) {
 		err = errno;
 		fprintf(stderr, "[cgit] Error writing %s: %s (%d)\n",
 			locked_rc.buf, strerror(err), err);
 		unlink(locked_rc.buf);
+		fclose(f);
 		goto out;
 	}
 	if (rename(locked_rc.buf, cached_rc)) {
@@ -375,6 +403,9 @@ static int generate_cached_repolist(const char *path, const char *cached_rc)
 			locked_rc.buf, cached_rc, strerror(err), err);
 		unlink(locked_rc.buf);
 	}
+	// Closed after the rename so the lock is held until the fresh list is
+	// in place, and nobody can truncate the file being renamed.
+	fclose(f);
 out:
 	strbuf_release(&locked_rc);
 	return err;