[PATCH] maintenance: trace when auto maintenance skips due to lock

[PATCH] maintenance: trace when auto maintenance skips due to lock

[Pushkar Singh]

When git maintenance run --auto encounters an existing
maintenance.lock, it silently skips running maintenance and
returns successfully.

This makes it difficult to identify cases where automatic
maintenance is being skipped repeatedly, for example due to
a stale lock left behind by an interrupted maintenance
process.

Emit a Trace2 data event when automatic maintenance skips
due to an existing lock, allowing users to identify these
skips when Trace2 event logging is enabled.

Signed-off-by: Pushkar Singh <pushkarkumarsingh1970@gmail.com>
---
 builtin/gc.c           | 2 ++
 t/t7900-maintenance.sh | 8 ++++++++
 2 files changed, 10 insertions(+)

diff --git a/builtin/gc.c b/builtin/gc.c
index 81cecf6b6c..e73d1f1d49 100644
--- a/builtin/gc.c
+++ b/builtin/gc.c
@@ -1409,6 +1409,8 @@ static int maintenance_run_tasks(struct maintenance_run_opts *opts,
 		 * recursive process stack. Do not report an error in
 		 * that case.
 		 */
+		if (opts->auto_flag && errno == EEXIST)
+			trace2_data_string("maintenance", r, "lock/skip", "exists");
 		if (!opts->auto_flag && !opts->quiet)
 			warning(_("lock file '%s' exists, skipping maintenance"),
 				lock_path);
diff --git a/t/t7900-maintenance.sh b/t/t7900-maintenance.sh
index 9d610a29fe..440eeb956d 100755
--- a/t/t7900-maintenance.sh
+++ b/t/t7900-maintenance.sh
@@ -387,6 +387,14 @@ test_expect_success 'loose-objects task' '
 	test_cmp packs-between packs-after
 '
 
+test_expect_success 'auto maintenance traces existing lock' '
+	test_when_finished "rm -f trace .git/objects/maintenance.lock" &&
+	: >.git/objects/maintenance.lock &&
+	GIT_TRACE2_EVENT="$PWD/trace" git maintenance run --auto &&
+	test_grep "\"event\":\"data\".*\"category\":\"maintenance\".*\"key\":\"lock/skip\".*\"value\":\"exists\"" trace &&
+	test_path_is_file .git/objects/maintenance.lock
+'
+
 test_expect_success 'maintenance.loose-objects.auto' '
 	git repack -adk &&
 	GIT_TRACE2_EVENT="$(pwd)/trace-lo1.txt" \
-- 
2.53.0

Re: [PATCH] maintenance: trace when auto maintenance skips due to lock

[Patrick Steinhardt]

On Sun, Nov 01, 2026 at 10:59:00AM +0000, Pushkar Singh wrote:
> diff --git a/builtin/gc.c b/builtin/gc.c
> index 81cecf6b6c..e73d1f1d49 100644
> --- a/builtin/gc.c
> +++ b/builtin/gc.c
> @@ -1409,6 +1409,8 @@ static int maintenance_run_tasks(struct maintenance_run_opts *opts,
>  		 * recursive process stack. Do not report an error in
>  		 * that case.
>  		 */
> +		if (opts->auto_flag && errno == EEXIST)
> +			trace2_data_string("maintenance", r, "lock/skip", "exists");

So we now know to report failure when the lock exists already, but in
any other case we don't log anything at all. But wouldn't the caller
want to know when we failed because of for example EPERM, too, or
because of any other error?

>  		if (!opts->auto_flag && !opts->quiet)
>  			warning(_("lock file '%s' exists, skipping maintenance"),
>  				lock_path);

It's quite curious that we only print a warning when `git maintenance
run` has failed to acquire the lock instead of bubbling up an error. I
would have expected that we:

  - Exit successfully but print a warning in the case where we are
    executed with "--auto".

  - Exit with an error code and print an error in the case where we
    execute without "--auto".

Silently swallowing the fact that we didn't do anything in the latter
case certainly feels wrong to me. And git-gc(1) for example dies right
away when the lock exists, so that to me is another indicator that we
should follow and do the same for git-maintenance(1).

This is of course outside the scope of your patch series, but if you
feel like it I think it could be easily fixed while at it in a second
patch.

Thanks!

Patrick

Re: [PATCH] maintenance: trace when auto maintenance skips due to lock

[Pushkar Singh]

[snip]
> So we now know to report failure when the lock exists already, but in
> any other case we don't log anything at all. But wouldn't the caller
> want to know when we failed because of for example EPERM, too, or
> because of any other error?
>
At first I only considered EEXIST, but you're right, we should be
reporting other lock acquisition failures too. I've included the actual
reason in the Trace2 event.

> >               if (!opts->auto_flag && !opts->quiet)
> >                       warning(_("lock file '%s' exists, skipping maintenance"),
> >                               lock_path);
>
> It's quite curious that we only print a warning when `git maintenance
> run` has failed to acquire the lock instead of bubbling up an error. I
> would have expected that we:
>
>   - Exit successfully but print a warning in the case where we are
>     executed with "--auto".
>
>   - Exit with an error code and print an error in the case where we
>     execute without "--auto".
>
Agreed. The current behavior for manual invocations is a bit odd,
especially since we end up returning success even though we couldn't
acquire the lock.

[snip]
> This is of course outside the scope of your patch series, but if you
> feel like it I think it could be easily fixed while at it in a second
> patch.
>
I was thinking of including this in the same patch series since both
changes are around the same lock acquisition path. I've included this
in the v2 as well, so manual invocations now report an error and return
a non-zero status, while --auto still exits successfully but prints
a warning.

Thanks for the review :-)

Pushkar

[PATCH v2] maintenance: improve handling of lock acquisition failures

[Pushkar Singh]

When git maintenance run --auto fails to acquire the maintenance
lock, emit a Trace2 data event describing the reason for the
failure. This makes it easier to identify cases where automatic
maintenance is repeatedly skipped.

For manual invocations, report lock acquisition failures as
errors and return a non-zero exit status instead of silently
succeeding. Respect the --quiet option when reporting the error.

Signed-off-by: Pushkar Singh <pushkarkumarsingh1970@gmail.com>
---
Changes in v2:
- Report lock acquisition failures for manual invocations as errors.
- Print a warning for automatic invocations while preserving successful exit status.
- Trace the reason for all automatic lock acquisition failures, not just EEXIST.
- Add tests for automatic and manual lock failure behavior.

 builtin/gc.c           | 28 ++++++++++++++++++++++++----
 t/t7900-maintenance.sh | 26 ++++++++++++++++++++++++++
 2 files changed, 50 insertions(+), 4 deletions(-)

diff --git a/builtin/gc.c b/builtin/gc.c
index 81cecf6b6c..9223409750 100644
--- a/builtin/gc.c
+++ b/builtin/gc.c
@@ -1402,6 +1402,8 @@ static int maintenance_run_tasks(struct maintenance_run_opts *opts,
 	enum auto_gc_hook_result auto_gc_hook_result = AUTO_GC_HOOK_UNDECIDED;
 
 	if (repo_hold_lock_file_for_update(r, &lk, lock_path, LOCK_NO_DEREF) < 0) {
+		int err = errno;
+
 		/*
 		 * Another maintenance command is running.
 		 *
@@ -1409,11 +1411,29 @@ static int maintenance_run_tasks(struct maintenance_run_opts *opts,
 		 * recursive process stack. Do not report an error in
 		 * that case.
 		 */
-		if (!opts->auto_flag && !opts->quiet)
-			warning(_("lock file '%s' exists, skipping maintenance"),
-				lock_path);
+		if (opts->auto_flag) {
+			trace2_data_string("maintenance", r, "lock/skip",
+				(err == EEXIST) ? "exists" : strerror(err));
+
+			if (!opts->quiet) {
+				struct strbuf buf = STRBUF_INIT;
+				unable_to_lock_message(lock_path, err, &buf);
+				warning("%s", buf.buf);
+				strbuf_release(&buf);
+			}
+
+			free(lock_path);
+			return 0;
+		}
+
+		if (!opts->quiet) {
+			struct strbuf buf = STRBUF_INIT;
+			unable_to_lock_message(lock_path, err, &buf);
+			error("%s", buf.buf);
+			strbuf_release(&buf);
+		}
 		free(lock_path);
-		return 0;
+		return -1;
 	}
 	free(lock_path);
 
diff --git a/t/t7900-maintenance.sh b/t/t7900-maintenance.sh
index 9d610a29fe..c0763cb6e2 100755
--- a/t/t7900-maintenance.sh
+++ b/t/t7900-maintenance.sh
@@ -387,6 +387,32 @@ test_expect_success 'loose-objects task' '
 	test_cmp packs-between packs-after
 '
 
+test_expect_success 'auto maintenance warns and traces existing lock' '
+	test_when_finished "rm -f trace.txt err .git/objects/maintenance.lock" &&
+	: >.git/objects/maintenance.lock &&
+	GIT_TRACE2_EVENT="$(pwd)/trace.txt" \
+		git maintenance run --auto --no-quiet 2>err &&
+	test_grep "Unable to create.*maintenance\.lock" err &&
+	test_trace2_data maintenance lock/skip exists <trace.txt &&
+	test_path_is_file .git/objects/maintenance.lock
+'
+
+test_expect_success 'manual maintenance fails with existing lock' '
+	test_when_finished "rm -f err .git/objects/maintenance.lock" &&
+	: >.git/objects/maintenance.lock &&
+	test_must_fail git maintenance run --no-quiet 2>err &&
+	test_grep "maintenance\.lock" err &&
+	test_path_is_file .git/objects/maintenance.lock
+'
+
+test_expect_success 'manual maintenance with --quiet suppresses diagnostic' '
+	test_when_finished "rm -f err .git/objects/maintenance.lock" &&
+	: >.git/objects/maintenance.lock &&
+	test_must_fail git maintenance run --quiet 2>err &&
+	test_must_be_empty err &&
+	test_path_is_file .git/objects/maintenance.lock
+'
+
 test_expect_success 'maintenance.loose-objects.auto' '
 	git repack -adk &&
 	GIT_TRACE2_EVENT="$(pwd)/trace-lo1.txt" \
-- 
2.53.0

Re: [PATCH v2] maintenance: improve handling of lock acquisition failures

[Patrick Steinhardt]

On Mon, Nov 02, 2026 at 12:29:56PM +0000, Pushkar Singh wrote:
> Changes in v2:
> - Report lock acquisition failures for manual invocations as errors.
> - Print a warning for automatic invocations while preserving successful exit status.
> - Trace the reason for all automatic lock acquisition failures, not just EEXIST.
> - Add tests for automatic and manual lock failure behavior.

This really should be two patches now:

  - One patch that changes how we handle the failure to lock.

  - One patch that introduces your trace2 string.

One note though: please wait a bit before rerolling again so that others
can chime in first. You should ideally reroll at most once per day.

> diff --git a/builtin/gc.c b/builtin/gc.c
> index 81cecf6b6c..9223409750 100644
> --- a/builtin/gc.c
> +++ b/builtin/gc.c
> @@ -1402,6 +1402,8 @@ static int maintenance_run_tasks(struct maintenance_run_opts *opts,
>  	enum auto_gc_hook_result auto_gc_hook_result = AUTO_GC_HOOK_UNDECIDED;
>  
>  	if (repo_hold_lock_file_for_update(r, &lk, lock_path, LOCK_NO_DEREF) < 0) {
> +		int err = errno;
> +
>  		/*
>  		 * Another maintenance command is running.
>  		 *
> @@ -1409,11 +1411,29 @@ static int maintenance_run_tasks(struct maintenance_run_opts *opts,
>  		 * recursive process stack. Do not report an error in
>  		 * that case.
>  		 */
> -		if (!opts->auto_flag && !opts->quiet)
> -			warning(_("lock file '%s' exists, skipping maintenance"),
> -				lock_path);
> +		if (opts->auto_flag) {
> +			trace2_data_string("maintenance", r, "lock/skip",
> +				(err == EEXIST) ? "exists" : strerror(err));

Why don't you use strerror(3p) for both cases?

> +			if (!opts->quiet) {
> +				struct strbuf buf = STRBUF_INIT;
> +				unable_to_lock_message(lock_path, err, &buf);
> +				warning("%s", buf.buf);
> +				strbuf_release(&buf);
> +			}
> +
> +			free(lock_path);
> +			return 0;
> +		}
> +
> +		if (!opts->quiet) {
> +			struct strbuf buf = STRBUF_INIT;
> +			unable_to_lock_message(lock_path, err, &buf);
> +			error("%s", buf.buf);
> +			strbuf_release(&buf);

There's some duplication here with the auto-flag case. Can't we
deduplicate that?

Thanks!

Patrick