From: Omar Polo <op@omarpolo.com>
Subject: Re: don't delete pack files that are too younger
To: Stefan Sperling <stsp@stsp.name>
Cc: gameoftrees@openbsd.org
Date: Tue, 11 Jul 2023 17:50:50 +0200

On 2023/07/11 17:27:05 +0200, Stefan Sperling <stsp@stsp.name> wrote:
> On Tue, Jul 11, 2023 at 05:15:58PM +0200, Omar Polo wrote:
> > similar to what we do for the loose objects.  This prevents races when
> > gotadmin load, got fetch or gotd are installing a new pack file and a
> > concurrent gotadmin cleanup wants to delete it as redundant because it
> > hasn't seen the new refs.
> 
> > @@ -1203,6 +1204,17 @@ purge_redundant_pack(struct got_repository *repo, cons
> >  		return got_error(GOT_ERR_NO_SPACE);
> >  
> >  	/*
> > +	 * Do not delete pack files which are younger than our maximum
> > +	 * modification time threshold.  This prevents a race where a
> > +	 * new pack file which is being added to the repository
> > +	 * concurrently would be deleted.
> > +	 */
> > +	if (fstatat(got_repo_get_fd(repo), path, &sb, 0) == -1)
> 
> Shouldn't this ignore ENOENT?

not sure; `path' is the path to an .idx file we've opened and parsed
already so if it disappeared it could be a hard error.

does it read better with the ENOENT handling?

diff /home/op/w/got
commit - 77c65f86323fa18ae23ab5bb93c486a0c840f308
path + /home/op/w/got
blob - a12b9a6780a79ce5cb1abda9df2ddeadfba7f34f
file + lib/repository_admin.c
--- lib/repository_admin.c
+++ lib/repository_admin.c
@@ -1191,7 +1191,8 @@ purge_redundant_pack(struct got_repository *repo, cons
 
 static const struct got_error *
 purge_redundant_pack(struct got_repository *repo, const char *packidx_path,
-    int dry_run, int *remove, off_t *size_before, off_t *size_after)
+    int dry_run, int ignore_mtime, time_t max_mtime,
+    int *remove, off_t *size_before, off_t *size_after)
 {
 	static const char *ext[] = {".idx", ".pack", ".rev", ".bitmap",
 	    ".promisor", ".mtimes"};
@@ -1203,6 +1204,20 @@ purge_redundant_pack(struct got_repository *repo, cons
 		return got_error(GOT_ERR_NO_SPACE);
 
 	/*
+	 * Do not delete pack files which are younger than our maximum
+	 * modification time threshold.  This prevents a race where a
+	 * new pack file which is being added to the repository
+	 * concurrently would be deleted.
+	 */
+	if (fstatat(got_repo_get_fd(repo), path, &sb, 0) == -1) {
+		if (errno == ENOENT)
+			return NULL;
+		return got_error_from_errno2("fstatat", path);
+	}
+	if (!ignore_mtime && sb.st_mtime > max_mtime)
+		*remove = 0;
+
+	/*
 	 * For compatibility with Git, if a matching .keep file exist
 	 * don't delete the packfile.
 	 */
@@ -1308,8 +1323,9 @@ repo_purge_redundant_packfiles(struct got_repository *
 static const struct got_error *
 repo_purge_redundant_packfiles(struct got_repository *repo,
     struct got_object_idset *traversed_ids,
-    off_t *size_before, off_t *size_after, int dry_run,
-    int nloose, int ncommits, int npurged, struct got_ratelimit *rl,
+    off_t *size_before, off_t *size_after, int dry_run, int ignore_mtime,
+    time_t max_mtime, int nloose, int ncommits, int npurged,
+    struct got_ratelimit *rl,
     got_cleanup_progress_cb progress_cb, void *progress_arg,
     got_cancel_cb cancel_cb, void *cancel_arg)
 {
@@ -1362,7 +1378,7 @@ repo_purge_redundant_packfiles(struct got_repository *
 		if (err)
 			goto done;
 		err = purge_redundant_pack(repo, sorted[i].path, dry_run,
-		    &remove, size_before, size_after);
+		    ignore_mtime, max_mtime, &remove, size_before, size_after);
 		if (err)
 			goto done;
 		if (!remove)
@@ -1467,8 +1483,9 @@ got_repo_cleanup(struct got_repository *repo,
 		goto done;
 
 	err = repo_purge_redundant_packfiles(repo, traversed_ids,
-	    pack_before, pack_after, dry_run, *nloose, *ncommits, npurged,
-	    &rl, progress_cb, progress_arg, cancel_cb, cancel_arg);
+	    pack_before, pack_after, dry_run, ignore_mtime, max_mtime,
+	    *nloose, *ncommits, npurged, &rl, progress_cb, progress_arg,
+	    cancel_cb, cancel_arg);
 	if (err)
 		goto done;
 
blob - 757a755adad5a2bca886449ea29bc283017dd221
file + regress/cmdline/cleanup.sh
--- regress/cmdline/cleanup.sh
+++ regress/cmdline/cleanup.sh
@@ -274,7 +274,7 @@ test_cleanup_redundant_pack_files() {
 	(cd "$testroot/repo" && git checkout -q master)
 	(cd "$testroot/repo" && got branch -d tempbranch) >/dev/null
 
-	gotadmin cleanup -q -r "$testroot/repo"
+	gotadmin cleanup -a -q -r "$testroot/repo"
 	n=$(gotadmin info -r "$testroot/repo" | awk '/^pack files/{print $3}')
 	if [ "$n" -ne 2 ]; then
 		echo "expected 2 pack files left, $n found instead" >&2
@@ -301,7 +301,7 @@ test_cleanup_redundant_pack_files() {
 	done
 	gotadmin pack -r "$testroot/repo" >/dev/null
 
-	gotadmin cleanup -q -r "$testroot/repo"
+	gotadmin cleanup -a -q -r "$testroot/repo"
 
 	n=$(gotadmin info -r "$testroot/repo" | awk '/^pack files/{print $3}')
 	if [ "$n" -ne 3 ]; then
@@ -324,7 +324,7 @@ test_cleanup_redundant_pack_files() {
 
 	gotadmin pack -a -x master -r "$testroot/repo" >/dev/null
 
-	gotadmin cleanup -q -r "$testroot/repo"
+	gotadmin cleanup -a -q -r "$testroot/repo"
 	n=$(gotadmin info -r "$testroot/repo" | awk '/^pack files/{print $3}')
 	if [ "$n" -ne 3 ]; then
 		echo "expected 3 pack files left, $n found instead" >&2