[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

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

[Pushkar Singh]

[snip]
> 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.
>
Alright, I'll split these into two separate patches.

[snip]
> Why don't you use strerror(3p) for both cases?
>
Makes sense. I'll use strerror(err) consistently.

[snip]
> There's some duplication here with the auto-flag case. Can't we
> deduplicate that?
>
I'll clean that up as well.

[snip]
> 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.
>
Okay, I'll make sure to give some time for others to review before
sending another revision from now on :-)

I'll make these changes and send the revised series tomorrow.

Thanks!

Pushkar

[PATCH v3 0/2] maintenance: improve lock acquisition failure handling

[Pushkar Singh]

Greetings,

This series improves how "git maintenance" handles failures to
acquire the maintenance lock.

The first patch changes the handling of lock acquisition failures
so that manual invocations report an error and return a non-zero
exit status, while automatic invocations print a warning and exit
successfully.

The second patch adds a Trace2 data event to make it easier to
identify cases where automatic maintenance is repeatedly skipped
due to lock acquisition failures.

Changes in v3:
- Split the changes into two separate patches: one for lock
  failure handling and another for Trace2 instrumentation.
- Use `strerror(err)` consistently for reporting lock acquisition
  failures in Trace2.
- Deduplicate the diagnostic message construction while preserving
  the distinction between warnings and errors.

Thanks,
Pushkar

Pushkar Singh (2):
  maintenance: report lock acquisition failures
  maintenance: trace lock acquisition failures

[PATCH v3 1/2] maintenance: report lock acquisition failures

[Pushkar Singh]

When git maintenance run fails to acquire the maintenance
lock, report the failure instead of silently succeeding.

For automatic invocations, print a warning while preserving
the successful exit status. For manual invocations, report
the failure as an error and return a non-zero exit status.

Respect the --quiet option when reporting the failure.

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

diff --git a/builtin/gc.c b/builtin/gc.c
index 81cecf6b6c..9ca9a3584b 100644
--- a/builtin/gc.c
+++ b/builtin/gc.c
@@ -1402,6 +1402,7 @@ 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 +1410,17 @@ 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->quiet) {
+		    struct strbuf buf = STRBUF_INIT;
+			unable_to_lock_message(lock_path, err, &buf);
+			if (opts->auto_flag)
+				warning("%s", buf.buf);
+			else
+				error("%s", buf.buf);
+			strbuf_release(&buf);
+		}
 		free(lock_path);
-		return 0;
+		return opts->auto_flag ? 0 : -1;
 	}
 	free(lock_path);
 
diff --git a/t/t7900-maintenance.sh b/t/t7900-maintenance.sh
index 9d610a29fe..3288efb391 100755
--- a/t/t7900-maintenance.sh
+++ b/t/t7900-maintenance.sh
@@ -387,6 +387,30 @@ test_expect_success 'loose-objects task' '
 	test_cmp packs-between packs-after
 '
 
+test_expect_success 'auto maintenance warns when lock exists' '
+	test_when_finished "rm -f err .git/objects/maintenance.lock" &&
+	: >.git/objects/maintenance.lock &&
+	git maintenance run --auto --no-quiet 2>err &&
+	test_grep "Unable to create.*maintenance\.lock" err &&
+	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

[PATCH v3 2/2] maintenance: trace 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.

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

diff --git a/builtin/gc.c b/builtin/gc.c
index 9ca9a3584b..eaa0d71285 100644
--- a/builtin/gc.c
+++ b/builtin/gc.c
@@ -1410,6 +1410,10 @@ 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)
+			trace2_data_string("maintenance", r, "lock/skip",
+			    strerror(err));
+
 		if (!opts->quiet) {
 		    struct strbuf buf = STRBUF_INIT;
 			unable_to_lock_message(lock_path, err, &buf);
diff --git a/t/t7900-maintenance.sh b/t/t7900-maintenance.sh
index 3288efb391..294ab0db75 100755
--- a/t/t7900-maintenance.sh
+++ b/t/t7900-maintenance.sh
@@ -411,6 +411,15 @@ test_expect_success 'manual maintenance with --quiet suppresses diagnostic' '
 	test_path_is_file .git/objects/maintenance.lock
 '
 
+test_expect_success 'auto maintenance traces existing lock' '
+	test_when_finished "rm -f trace.txt .git/objects/maintenance.lock" &&
+	: >.git/objects/maintenance.lock &&
+	GIT_TRACE2_EVENT="$(pwd)/trace.txt" \
+		git maintenance run --auto --quiet &&
+	test_trace2_data maintenance lock/skip "File exists" <trace.txt &&
+	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

[Aniket Gupta]

Hi Pushkar,

I tried v2 while another maintenance run was holding the lock. The
scheduled background runs, the ones set up by "git maintenance start",
now exit with an error instead of 0. Two overlapping runs end in a
failure where master just skips them and the docs say it is normal.

Also, running the prefetch task by hand now prints a lock warning and I
think it comes from the fetch it starts internally.

Thanks,
Aniket