[PATCH 2/3] cgit: do not let a stale lockfile pin the cached repolist

[Konstantin Ryabitsev]

When scan-path is used, the discovered repolist is cached in an "rc-"
file below cache-root and refreshed from a forked child once it is older
than cache-scanrc-ttl. The refresh serializes on a lockfile which it
creates with O_EXCL and renames into place when it is done, so a process
which dies before that rename leaves the lockfile behind. Since nothing
ever removes it, every later refresh fails with EEXIST and quietly gives
up, and the expired repolist is served unchanged from then on.

The stale repolist keeps handing out a repo.path for a repository that
has since been removed, which is exactly the input that the previous
patch stops crashing on. A crash is also one way to leave the lockfile
behind in the first place, so the two faults sustain each other: cgit
dies while the refresh child is torn down with it, and the lockfile it
leaves guarantees the next request reads the same bad path again.

Serialize on an advisory lock instead. The kernel releases it when the
holder dies, so a concurrent refresh is still skipped while a lockfile
whose owner is gone is simply taken over and renamed into place. This
also cleans up any lockfile left behind by an older cgit.

Fixes: d746827 ("cgit.c: add support for caching autodetected repositories")
Assisted-by: LLM [analysis, codegen, tests]
Signed-off-by: Konstantin Ryabitsev <konstantin at linuxfoundation.org>
---
 cgit.c                        | 45 +++++++++++++++++++++----
 tests/t0024-repolist-cache.sh | 78 +++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 116 insertions(+), 7 deletions(-)

diff --git a/cgit.c b/cgit.c
index 47471b0..2dfba9e 100644
--- a/cgit.c
+++ b/cgit.c
@@ -881,20 +881,51 @@ static void print_repolist(FILE *f, struct cgit_repolist *list, int start)
 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 result = 0;
 	int idx;
+	int fd;
 	FILE *f;
 
 	strbuf_addf(&locked_rc, "%s.lock", cached_rc);
-	f = fopen(locked_rc.buf, "wx");
-	if (!f) {
-		/* Inform about the error unless the lockfile already existed,
-		 * since that only means we've got concurrent requests.
+	fd = open(locked_rc.buf, O_WRONLY | O_CREAT, S_IRUSR | S_IWUSR);
+	if (fd == -1) {
+		result = errno;
+		fprintf(stderr, "[cgit] Error opening %s: %s (%d)\n",
+			locked_rc.buf, strerror(result), result);
+		goto out;
+	}
+	/* Serialize on an advisory lock rather than on the mere existence of
+	 * the lockfile: a cgit process which dies before it can rename the
+	 * lockfile into place must not keep every later process from
+	 * refreshing the cached repolist.
+	 */
+	if (fcntl(fd, F_SETLK, &lock) == -1) {
+		/* Another process is generating the repolist, so leave it to
+		 * that one. This is not an error worth reporting.
 		 */
 		result = errno;
-		if (result != EEXIST)
-			fprintf(stderr, "[cgit] Error opening %s: %s (%d)\n",
-				locked_rc.buf, strerror(result), result);
+		close(fd);
+		goto out;
+	}
+	if (ftruncate(fd, 0) == -1) {
+		result = errno;
+		fprintf(stderr, "[cgit] Error truncating %s: %s (%d)\n",
+			locked_rc.buf, strerror(result), result);
+		close(fd);
+		goto out;
+	}
+	f = fdopen(fd, "w");
+	if (!f) {
+		result = errno;
+		fprintf(stderr, "[cgit] Error opening %s: %s (%d)\n",
+			locked_rc.buf, strerror(result), result);
+		close(fd);
 		goto out;
 	}
 	idx = cgit_repolist.count;
diff --git a/tests/t0024-repolist-cache.sh b/tests/t0024-repolist-cache.sh
new file mode 100755
index 0000000..dadc37e
--- /dev/null
+++ b/tests/t0024-repolist-cache.sh
@@ -0,0 +1,78 @@
+#!/bin/sh
+
+test_description='Check that the cached repolist recovers from a stale lockfile'
+. ./setup.sh
+
+# With scan-path, cgit caches the discovered repolist in an "rc-" file below
+# cache-root and refreshes it from a forked child once it is older than
+# cache-scanrc-ttl. That refresh used to serialize on the mere existence of a
+# lockfile, so a cgit process which died before it could rename the lockfile
+# into place left the lockfile behind for good: every later refresh quietly
+# gave up on it, and a repository which had since been removed stayed in the
+# repolist forever.
+
+scan_url() {
+	CGIT_CONFIG="$PWD/cgitrc.scan" QUERY_STRING="url=$1" cgit
+}
+
+# The refresh runs in a forked child, so poll for its result rather than
+# assuming it has already finished.
+wait_for_rescan() {
+	i=0
+	while test $i -lt 10
+	do
+		grep -q "^repo.url=doomed" cache-scan/rc-* || return 0
+		sleep 1
+		i=$((i + 1))
+	done
+	return 1
+}
+
+warm_repolist() {
+	rm -rf scan cache-scan &&
+	mkdir -p cache-scan &&
+	git init -q --bare scan/keep.git &&
+	git init -q --bare scan/doomed.git &&
+	scan_url "" >/dev/null &&
+	grep -q "^repo.url=doomed" cache-scan/rc-*
+}
+
+test_expect_success 'setup' '
+	cat >cgitrc.scan <<-EOF
+	virtual-root=/
+	cache-root=$PWD/cache-scan
+	cache-size=1021
+	cache-scanrc-ttl=0
+	scan-path=$PWD/scan
+	EOF
+'
+
+test_expect_success 'a removed repository leaves an expired repolist' '
+	warm_repolist &&
+	rm -rf scan/doomed.git &&
+	sleep 1 &&
+	scan_url "" >/dev/null &&
+	wait_for_rescan
+'
+
+test_expect_success 'a stale lockfile does not pin the repolist' '
+	warm_repolist &&
+	rm -rf scan/doomed.git &&
+	for rc in cache-scan/rc-*
+	do
+		: >"$rc.lock" || return 1
+	done &&
+	sleep 1 &&
+	scan_url "" >/dev/null &&
+	wait_for_rescan
+'
+
+test_expect_success 'the stale lockfile is not left behind' '
+	test -z "$(ls cache-scan/rc-*.lock 2>/dev/null)"
+'
+
+test_expect_success 'the surviving repository is still listed' '
+	grep -q "^repo.url=keep" cache-scan/rc-*
+'
+
+test_done

-- 
2.55.0