[PATCH 0/2] t7900: fix flaky "maintenance.strategy" test

[PATCH 0/2] t7900: fix flaky "maintenance.strategy" test

[Patrick Steinhardt]

Hi,

I've recently noticed that t7900 is flaky, see for example [1].
The root cause of the flake is the auto-detaching logic of
git-maintenance(1), which sometimes causes us to skip maintenance
altogether when the foreground process is racing with background
maintenance.

Thanks!

Patrick

[1]: https://gitlab.com/gitlab-org/git/-/jobs/15762975482

---
Patrick Steinhardt (2):
      t7900: adapt some tests to use a throwaway repository
      t7900: fix flaky "maintenance.strategy" test

 t/t7900-maintenance.sh | 76 ++++++++++++++++++++++++++++++--------------------
 1 file changed, 46 insertions(+), 30 deletions(-)


---
base-commit: 2c78326f810173a4f3aefd8021f1e07575412481
change-id: 20260807-pks-t7900-fix-flaky-test-160abfcef65a

[PATCH 1/2] t7900: adapt some tests to use a throwaway repository

[Patrick Steinhardt]

Many of the tests in t7900 operate inside the main trash repository
that's set up by default by our test suite. This is overall quite
fragile as we're exercising repository maintenance in those tests, and
maintenance is of course intricately tied towards the on-disk state of a
repository. Consequently, the tests can easily impact one another.

Furthermore, in the next commit we'll have to modify the environment in
a handful of those tests. As tests don't run in a subshell, doing so
would impact all subsequent tests by default, as well.

Adapt exactly those tests to use a throwaway repository. This makes the
tests more neatly self-contained and allows us to trivially modify the
environment in the next commit.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 t/t7900-maintenance.sh | 70 +++++++++++++++++++++++++++++++-------------------
 1 file changed, 43 insertions(+), 27 deletions(-)

diff --git a/t/t7900-maintenance.sh b/t/t7900-maintenance.sh
index 4238569b68..6735a9e082 100755
--- a/t/t7900-maintenance.sh
+++ b/t/t7900-maintenance.sh
@@ -67,41 +67,57 @@ test_expect_success 'run [--auto|--quiet] with gc strategy' '
 '
 
 test_expect_success 'maintenance.auto config option' '
-	GIT_TRACE2_EVENT="$(pwd)/default" git commit --quiet --allow-empty -m 1 &&
-	test_subcommand git maintenance run --auto --quiet --detach <default &&
-	GIT_TRACE2_EVENT="$(pwd)/true" \
-		git -c maintenance.auto=true \
-		commit --quiet --allow-empty -m 2 &&
-	test_subcommand git maintenance run --auto --quiet --detach <true &&
-	GIT_TRACE2_EVENT="$(pwd)/false" \
-		git -c maintenance.auto=false \
-		commit --quiet --allow-empty -m 3 &&
-	test_subcommand ! git maintenance run --auto --quiet --detach <false
+	test_when_finished "rm -rf repo" &&
+	git init repo &&
+	(
+		cd repo &&
+
+		GIT_TRACE2_EVENT="$(pwd)/default" git commit --quiet --allow-empty -m 1 &&
+		test_subcommand git maintenance run --auto --quiet --detach <default &&
+		GIT_TRACE2_EVENT="$(pwd)/true" \
+			git -c maintenance.auto=true \
+			commit --quiet --allow-empty -m 2 &&
+		test_subcommand git maintenance run --auto --quiet --detach <true &&
+		GIT_TRACE2_EVENT="$(pwd)/false" \
+			git -c maintenance.auto=false \
+			commit --quiet --allow-empty -m 3 &&
+		test_subcommand ! git maintenance run --auto --quiet --detach <false
+	)
 '
 
 test_expect_success 'gc.auto config option' '
-	GIT_TRACE2_EVENT="$(pwd)/default" git commit --quiet --allow-empty -m 1 &&
-	test_subcommand git maintenance run --auto --quiet --detach <default &&
-	GIT_TRACE2_EVENT="$(pwd)/true" \
-		git -c gc.auto=1 commit --quiet --allow-empty -m 2 &&
-	test_subcommand git maintenance run --auto --quiet --detach <true &&
-	GIT_TRACE2_EVENT="$(pwd)/false" \
-		git -c gc.auto=0 commit --quiet --allow-empty -m 3 &&
-	test_subcommand ! git maintenance run --auto --quiet --detach <false
+	test_when_finished "rm -rf repo" &&
+	git init repo &&
+	(
+		cd repo &&
+
+		GIT_TRACE2_EVENT="$(pwd)/default" git commit --quiet --allow-empty -m 1 &&
+		test_subcommand git maintenance run --auto --quiet --detach <default &&
+		GIT_TRACE2_EVENT="$(pwd)/true" \
+			git -c gc.auto=1 commit --quiet --allow-empty -m 2 &&
+		test_subcommand git maintenance run --auto --quiet --detach <true &&
+		GIT_TRACE2_EVENT="$(pwd)/false" \
+			git -c gc.auto=0 commit --quiet --allow-empty -m 3 &&
+		test_subcommand ! git maintenance run --auto --quiet --detach <false
+	)
 '
 
 test_expect_success 'maintenance.auto overrides gc.auto' '
-	test_when_finished "rm -f trace" &&
+	test_when_finished "rm -rf repo" &&
+	git init repo &&
+	(
+		cd repo &&
 
-	test_config maintenance.auto false &&
-	test_config gc.auto 1 &&
-	GIT_TRACE2_EVENT="$(pwd)/trace" git commit --quiet --allow-empty -m 1 &&
-	test_subcommand ! git maintenance run --auto --quiet --detach <trace &&
+		git config set maintenance.auto false &&
+		git config set gc.auto 1 &&
+		GIT_TRACE2_EVENT="$(pwd)/trace" git commit --quiet --allow-empty -m 1 &&
+		test_subcommand ! git maintenance run --auto --quiet --detach <trace &&
 
-	test_config maintenance.auto true &&
-	test_config gc.auto 0 &&
-	GIT_TRACE2_EVENT="$(pwd)/trace" git commit --quiet --allow-empty -m 1 &&
-	test_subcommand git maintenance run --auto --quiet --detach <trace
+		git config set maintenance.auto true &&
+		git config set gc.auto 0 &&
+		GIT_TRACE2_EVENT="$(pwd)/trace" git commit --quiet --allow-empty -m 1 &&
+		test_subcommand git maintenance run --auto --quiet --detach <trace
+	)
 '
 
 for cfg in maintenance.autoDetach gc.autoDetach

-- 
2.55.0.679.g6767b8d81c.dirty

Re: [PATCH 1/2] t7900: adapt some tests to use a throwaway repository

[Karthik Nayak]

[-- Attachment #1: Type: text/plain, Size: 5078 bytes --]

Patrick Steinhardt <ps@pks.im> writes:

> Many of the tests in t7900 operate inside the main trash repository
> that's set up by default by our test suite. This is overall quite
> fragile as we're exercising repository maintenance in those tests, and
> maintenance is of course intricately tied towards the on-disk state of a
> repository. Consequently, the tests can easily impact one another.
>
> Furthermore, in the next commit we'll have to modify the environment in
> a handful of those tests. As tests don't run in a subshell, doing so
> would impact all subsequent tests by default, as well.
>
> Adapt exactly those tests to use a throwaway repository. This makes the
> tests more neatly self-contained and allows us to trivially modify the
> environment in the next commit.
>
> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> ---
>  t/t7900-maintenance.sh | 70 +++++++++++++++++++++++++++++++-------------------
>  1 file changed, 43 insertions(+), 27 deletions(-)
>
> diff --git a/t/t7900-maintenance.sh b/t/t7900-maintenance.sh
> index 4238569b68..6735a9e082 100755
> --- a/t/t7900-maintenance.sh
> +++ b/t/t7900-maintenance.sh
> @@ -67,41 +67,57 @@ test_expect_success 'run [--auto|--quiet] with gc strategy' '
>  '
>
>  test_expect_success 'maintenance.auto config option' '
> -	GIT_TRACE2_EVENT="$(pwd)/default" git commit --quiet --allow-empty -m 1 &&
> -	test_subcommand git maintenance run --auto --quiet --detach <default &&
> -	GIT_TRACE2_EVENT="$(pwd)/true" \
> -		git -c maintenance.auto=true \
> -		commit --quiet --allow-empty -m 2 &&
> -	test_subcommand git maintenance run --auto --quiet --detach <true &&
> -	GIT_TRACE2_EVENT="$(pwd)/false" \
> -		git -c maintenance.auto=false \
> -		commit --quiet --allow-empty -m 3 &&
> -	test_subcommand ! git maintenance run --auto --quiet --detach <false
> +	test_when_finished "rm -rf repo" &&
> +	git init repo &&
> +	(
> +		cd repo &&
> +
> +		GIT_TRACE2_EVENT="$(pwd)/default" git commit --quiet --allow-empty -m 1 &&
> +		test_subcommand git maintenance run --auto --quiet --detach <default &&
> +		GIT_TRACE2_EVENT="$(pwd)/true" \
> +			git -c maintenance.auto=true \
> +			commit --quiet --allow-empty -m 2 &&
> +		test_subcommand git maintenance run --auto --quiet --detach <true &&
> +		GIT_TRACE2_EVENT="$(pwd)/false" \
> +			git -c maintenance.auto=false \
> +			commit --quiet --allow-empty -m 3 &&
> +		test_subcommand ! git maintenance run --auto --quiet --detach <false
> +	)
>  '
>
>  test_expect_success 'gc.auto config option' '
> -	GIT_TRACE2_EVENT="$(pwd)/default" git commit --quiet --allow-empty -m 1 &&
> -	test_subcommand git maintenance run --auto --quiet --detach <default &&
> -	GIT_TRACE2_EVENT="$(pwd)/true" \
> -		git -c gc.auto=1 commit --quiet --allow-empty -m 2 &&
> -	test_subcommand git maintenance run --auto --quiet --detach <true &&
> -	GIT_TRACE2_EVENT="$(pwd)/false" \
> -		git -c gc.auto=0 commit --quiet --allow-empty -m 3 &&
> -	test_subcommand ! git maintenance run --auto --quiet --detach <false
> +	test_when_finished "rm -rf repo" &&
> +	git init repo &&
> +	(
> +		cd repo &&
> +
> +		GIT_TRACE2_EVENT="$(pwd)/default" git commit --quiet --allow-empty -m 1 &&
> +		test_subcommand git maintenance run --auto --quiet --detach <default &&
> +		GIT_TRACE2_EVENT="$(pwd)/true" \
> +			git -c gc.auto=1 commit --quiet --allow-empty -m 2 &&
> +		test_subcommand git maintenance run --auto --quiet --detach <true &&
> +		GIT_TRACE2_EVENT="$(pwd)/false" \
> +			git -c gc.auto=0 commit --quiet --allow-empty -m 3 &&
> +		test_subcommand ! git maintenance run --auto --quiet --detach <false
> +	)
>  '
>
>  test_expect_success 'maintenance.auto overrides gc.auto' '
> -	test_when_finished "rm -f trace" &&
> +	test_when_finished "rm -rf repo" &&
> +	git init repo &&
> +	(
> +		cd repo &&
>
> -	test_config maintenance.auto false &&
> -	test_config gc.auto 1 &&
> -	GIT_TRACE2_EVENT="$(pwd)/trace" git commit --quiet --allow-empty -m 1 &&
> -	test_subcommand ! git maintenance run --auto --quiet --detach <trace &&
> +		git config set maintenance.auto false &&
> +		git config set gc.auto 1 &&

So we change from using `test_config` to `git config`, I assume this is
because earlier since we used a shared folder, we had to undo any config
changes made. Now that's no longer needed. Nit: This is okay, but
would've been nicer to call out.

> +		GIT_TRACE2_EVENT="$(pwd)/trace" git commit --quiet --allow-empty -m 1 &&
> +		test_subcommand ! git maintenance run --auto --quiet --detach <trace &&
>
> -	test_config maintenance.auto true &&
> -	test_config gc.auto 0 &&
> -	GIT_TRACE2_EVENT="$(pwd)/trace" git commit --quiet --allow-empty -m 1 &&
> -	test_subcommand git maintenance run --auto --quiet --detach <trace
> +		git config set maintenance.auto true &&
> +		git config set gc.auto 0 &&
> +		GIT_TRACE2_EVENT="$(pwd)/trace" git commit --quiet --allow-empty -m 1 &&
> +		test_subcommand git maintenance run --auto --quiet --detach <trace
> +	)
>  '
>
>  for cfg in maintenance.autoDetach gc.autoDetach
>
> --
> 2.55.0.679.g6767b8d81c.dirty

The rest looks as expected.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 690 bytes --]

Re: [PATCH 1/2] t7900: adapt some tests to use a throwaway repository

[Patrick Steinhardt]

On Wed, Aug 12, 2026 at 01:19:13AM -0700, Karthik Nayak wrote:
> Patrick Steinhardt <ps@pks.im> writes:
> > diff --git a/t/t7900-maintenance.sh b/t/t7900-maintenance.sh
> > index 4238569b68..6735a9e082 100755
> > --- a/t/t7900-maintenance.sh
> > +++ b/t/t7900-maintenance.sh
> > @@ -67,41 +67,57 @@ test_expect_success 'run [--auto|--quiet] with gc strategy' '
[snip]
> >  test_expect_success 'maintenance.auto overrides gc.auto' '
> > -	test_when_finished "rm -f trace" &&
> > +	test_when_finished "rm -rf repo" &&
> > +	git init repo &&
> > +	(
> > +		cd repo &&
> >
> > -	test_config maintenance.auto false &&
> > -	test_config gc.auto 1 &&
> > -	GIT_TRACE2_EVENT="$(pwd)/trace" git commit --quiet --allow-empty -m 1 &&
> > -	test_subcommand ! git maintenance run --auto --quiet --detach <trace &&
> > +		git config set maintenance.auto false &&
> > +		git config set gc.auto 1 &&
> 
> So we change from using `test_config` to `git config`, I assume this is
> because earlier since we used a shared folder, we had to undo any config
> changes made. Now that's no longer needed. Nit: This is okay, but
> would've been nicer to call out.

The issue with `test_config` is that it executes `test_when_finished`,
and that function cannot run in subshells. So we have to use `git config
set` instead, but because it's a throw-away repository it doesn't
matter.

Patrick

Re: [PATCH 1/2] t7900: adapt some tests to use a throwaway repository

[Karthik Nayak]

[-- Attachment #1: Type: text/plain, Size: 5078 bytes --]

Patrick Steinhardt <ps@pks.im> writes:

> Many of the tests in t7900 operate inside the main trash repository
> that's set up by default by our test suite. This is overall quite
> fragile as we're exercising repository maintenance in those tests, and
> maintenance is of course intricately tied towards the on-disk state of a
> repository. Consequently, the tests can easily impact one another.
>
> Furthermore, in the next commit we'll have to modify the environment in
> a handful of those tests. As tests don't run in a subshell, doing so
> would impact all subsequent tests by default, as well.
>
> Adapt exactly those tests to use a throwaway repository. This makes the
> tests more neatly self-contained and allows us to trivially modify the
> environment in the next commit.
>
> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> ---
>  t/t7900-maintenance.sh | 70 +++++++++++++++++++++++++++++++-------------------
>  1 file changed, 43 insertions(+), 27 deletions(-)
>
> diff --git a/t/t7900-maintenance.sh b/t/t7900-maintenance.sh
> index 4238569b68..6735a9e082 100755
> --- a/t/t7900-maintenance.sh
> +++ b/t/t7900-maintenance.sh
> @@ -67,41 +67,57 @@ test_expect_success 'run [--auto|--quiet] with gc strategy' '
>  '
>
>  test_expect_success 'maintenance.auto config option' '
> -	GIT_TRACE2_EVENT="$(pwd)/default" git commit --quiet --allow-empty -m 1 &&
> -	test_subcommand git maintenance run --auto --quiet --detach <default &&
> -	GIT_TRACE2_EVENT="$(pwd)/true" \
> -		git -c maintenance.auto=true \
> -		commit --quiet --allow-empty -m 2 &&
> -	test_subcommand git maintenance run --auto --quiet --detach <true &&
> -	GIT_TRACE2_EVENT="$(pwd)/false" \
> -		git -c maintenance.auto=false \
> -		commit --quiet --allow-empty -m 3 &&
> -	test_subcommand ! git maintenance run --auto --quiet --detach <false
> +	test_when_finished "rm -rf repo" &&
> +	git init repo &&
> +	(
> +		cd repo &&
> +
> +		GIT_TRACE2_EVENT="$(pwd)/default" git commit --quiet --allow-empty -m 1 &&
> +		test_subcommand git maintenance run --auto --quiet --detach <default &&
> +		GIT_TRACE2_EVENT="$(pwd)/true" \
> +			git -c maintenance.auto=true \
> +			commit --quiet --allow-empty -m 2 &&
> +		test_subcommand git maintenance run --auto --quiet --detach <true &&
> +		GIT_TRACE2_EVENT="$(pwd)/false" \
> +			git -c maintenance.auto=false \
> +			commit --quiet --allow-empty -m 3 &&
> +		test_subcommand ! git maintenance run --auto --quiet --detach <false
> +	)
>  '
>
>  test_expect_success 'gc.auto config option' '
> -	GIT_TRACE2_EVENT="$(pwd)/default" git commit --quiet --allow-empty -m 1 &&
> -	test_subcommand git maintenance run --auto --quiet --detach <default &&
> -	GIT_TRACE2_EVENT="$(pwd)/true" \
> -		git -c gc.auto=1 commit --quiet --allow-empty -m 2 &&
> -	test_subcommand git maintenance run --auto --quiet --detach <true &&
> -	GIT_TRACE2_EVENT="$(pwd)/false" \
> -		git -c gc.auto=0 commit --quiet --allow-empty -m 3 &&
> -	test_subcommand ! git maintenance run --auto --quiet --detach <false
> +	test_when_finished "rm -rf repo" &&
> +	git init repo &&
> +	(
> +		cd repo &&
> +
> +		GIT_TRACE2_EVENT="$(pwd)/default" git commit --quiet --allow-empty -m 1 &&
> +		test_subcommand git maintenance run --auto --quiet --detach <default &&
> +		GIT_TRACE2_EVENT="$(pwd)/true" \
> +			git -c gc.auto=1 commit --quiet --allow-empty -m 2 &&
> +		test_subcommand git maintenance run --auto --quiet --detach <true &&
> +		GIT_TRACE2_EVENT="$(pwd)/false" \
> +			git -c gc.auto=0 commit --quiet --allow-empty -m 3 &&
> +		test_subcommand ! git maintenance run --auto --quiet --detach <false
> +	)
>  '
>
>  test_expect_success 'maintenance.auto overrides gc.auto' '
> -	test_when_finished "rm -f trace" &&
> +	test_when_finished "rm -rf repo" &&
> +	git init repo &&
> +	(
> +		cd repo &&
>
> -	test_config maintenance.auto false &&
> -	test_config gc.auto 1 &&
> -	GIT_TRACE2_EVENT="$(pwd)/trace" git commit --quiet --allow-empty -m 1 &&
> -	test_subcommand ! git maintenance run --auto --quiet --detach <trace &&
> +		git config set maintenance.auto false &&
> +		git config set gc.auto 1 &&

So we change from using `test_config` to `git config`, I assume this is
because earlier since we used a shared folder, we had to undo any config
changes made. Now that's no longer needed. Nit: This is okay, but
would've been nicer to call out.

> +		GIT_TRACE2_EVENT="$(pwd)/trace" git commit --quiet --allow-empty -m 1 &&
> +		test_subcommand ! git maintenance run --auto --quiet --detach <trace &&
>
> -	test_config maintenance.auto true &&
> -	test_config gc.auto 0 &&
> -	GIT_TRACE2_EVENT="$(pwd)/trace" git commit --quiet --allow-empty -m 1 &&
> -	test_subcommand git maintenance run --auto --quiet --detach <trace
> +		git config set maintenance.auto true &&
> +		git config set gc.auto 0 &&
> +		GIT_TRACE2_EVENT="$(pwd)/trace" git commit --quiet --allow-empty -m 1 &&
> +		test_subcommand git maintenance run --auto --quiet --detach <trace
> +	)
>  '
>
>  for cfg in maintenance.autoDetach gc.autoDetach
>
> --
> 2.55.0.679.g6767b8d81c.dirty

The rest looks as expected.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 690 bytes --]

[PATCH 2/2] t7900: fix flaky "maintenance.strategy" test

[Patrick Steinhardt]

One of our tests for whether "maintenance.strategy" is being respected
in t7900 is flaky in our CI systems:

    + GIT_TRACE2_EVENT=/tmp/test-output/trash directory.t7900-maintenance/repo/trace2.txt git -c maintenance.strategy=incremental maintenance run --quiet
    + test_maintenance_tasks trace2.txt
    + cat
    + sed -ne s/.*"region_enter".*"category":"maintenance\([^"]*\)".*"label":"\([^"][^"]*\)".*/\2\1/p trace2.txt
    + test_cmp expect actual
    + test 2 -ne 2
    + eval /usr/bin/diff -u "$@"
    + /usr/bin/diff -u expect actual
    --- expect	2026-08-07 06:20:51.388322602 +0000
    +++ actual	2026-08-07 06:20:51.388322602 +0000
    @@ -1,2 +0,0 @@
    -gc foreground
    -gc

When running with the "incremental" strategy, we expect two git-gc(1)
tasks to have been executed, but sometimes the test simply doesn't
execute any of those tasks.

A first hunch may be that maybe the disk-state is sometimes different
and thus we decide not to run maintenance. But git-maintenance(1)
doesn't run with the "--auto" switch, so we should execute those tasks
regardless of the on-disk state.

But there's a second condition that may cause us to not execute tasks,
namely when the "maintenance.lock" file exists due to a concurrently
running tasks. We usually disable auto-maintenance from detaching in our
test suite to avoid exactly these kinds of race conditions, but in t7900
we unset "GIT_TEST_MAINT_AUTO_DETACH" and thus enable the auto-detach
logic. The intent of this is to exercise git-maintenance(1) closer to
how it would run in a real-world scenario, but it does cause us to race
when the detached maintenance job that was triggered by `test_commit()`
lives long enough.

We could trivially fix this race by disabling auto-maintenance for this
specific test. But that doesn't fix this class of races in this test
suite: while I haven't seen any of the other tests fail in the same way,
a bunch of them have this race, as well.

Instead, let's retain "GIT_TEST_MAINT_AUTO_DETACH" and only unset it as
required.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 t/t7900-maintenance.sh | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/t/t7900-maintenance.sh b/t/t7900-maintenance.sh
index 6735a9e082..5fbb16f0f0 100755
--- a/t/t7900-maintenance.sh
+++ b/t/t7900-maintenance.sh
@@ -7,9 +7,6 @@ test_description='git maintenance builtin'
 GIT_TEST_COMMIT_GRAPH=0
 GIT_TEST_MULTI_PACK_INDEX=0
 
-# Ensure that auto-maintenance detaches as usual.
-sane_unset GIT_TEST_MAINT_AUTO_DETACH
-
 test_lazy_prereq XMLLINT '
 	xmllint --version
 '
@@ -71,6 +68,7 @@ test_expect_success 'maintenance.auto config option' '
 	git init repo &&
 	(
 		cd repo &&
+		sane_unset GIT_TEST_MAINT_AUTO_DETACH &&
 
 		GIT_TRACE2_EVENT="$(pwd)/default" git commit --quiet --allow-empty -m 1 &&
 		test_subcommand git maintenance run --auto --quiet --detach <default &&
@@ -90,6 +88,7 @@ test_expect_success 'gc.auto config option' '
 	git init repo &&
 	(
 		cd repo &&
+		sane_unset GIT_TEST_MAINT_AUTO_DETACH &&
 
 		GIT_TRACE2_EVENT="$(pwd)/default" git commit --quiet --allow-empty -m 1 &&
 		test_subcommand git maintenance run --auto --quiet --detach <default &&
@@ -107,6 +106,7 @@ test_expect_success 'maintenance.auto overrides gc.auto' '
 	git init repo &&
 	(
 		cd repo &&
+		sane_unset GIT_TEST_MAINT_AUTO_DETACH &&
 
 		git config set maintenance.auto false &&
 		git config set gc.auto 1 &&

-- 
2.55.0.679.g6767b8d81c.dirty

Re: [PATCH 2/2] t7900: fix flaky "maintenance.strategy" test

[Karthik Nayak]

[-- Attachment #1: Type: text/plain, Size: 3927 bytes --]

Patrick Steinhardt <ps@pks.im> writes:

> One of our tests for whether "maintenance.strategy" is being respected
> in t7900 is flaky in our CI systems:
>
>     + GIT_TRACE2_EVENT=/tmp/test-output/trash directory.t7900-maintenance/repo/trace2.txt git -c maintenance.strategy=incremental maintenance run --quiet
>     + test_maintenance_tasks trace2.txt
>     + cat
>     + sed -ne s/.*"region_enter".*"category":"maintenance\([^"]*\)".*"label":"\([^"][^"]*\)".*/\2\1/p trace2.txt
>     + test_cmp expect actual
>     + test 2 -ne 2
>     + eval /usr/bin/diff -u "$@"
>     + /usr/bin/diff -u expect actual
>     --- expect	2026-08-07 06:20:51.388322602 +0000
>     +++ actual	2026-08-07 06:20:51.388322602 +0000
>     @@ -1,2 +0,0 @@
>     -gc foreground
>     -gc
>
> When running with the "incremental" strategy, we expect two git-gc(1)
> tasks to have been executed, but sometimes the test simply doesn't
> execute any of those tasks.
>
> A first hunch may be that maybe the disk-state is sometimes different
> and thus we decide not to run maintenance. But git-maintenance(1)
> doesn't run with the "--auto" switch, so we should execute those tasks
> regardless of the on-disk state.
>
> But there's a second condition that may cause us to not execute tasks,
> namely when the "maintenance.lock" file exists due to a concurrently

Nit: s/a//

> running tasks. We usually disable auto-maintenance from detaching in our
> test suite to avoid exactly these kinds of race conditions, but in t7900
> we unset "GIT_TEST_MAINT_AUTO_DETACH" and thus enable the auto-detach
> logic. The intent of this is to exercise git-maintenance(1) closer to
> how it would run in a real-world scenario, but it does cause us to race
> when the detached maintenance job that was triggered by `test_commit()`
> lives long enough.

GIT_TEST_MAINT_AUTO_DETACH when set to true enables auto-detach, but
also the default value when unset is true. That's why unsetting it
enables auto-detach. That's a bit confusing.

>
> We could trivially fix this race by disabling auto-maintenance for this
> specific test. But that doesn't fix this class of races in this test
> suite: while I haven't seen any of the other tests fail in the same way,
> a bunch of them have this race, as well.
>
> Instead, let's retain "GIT_TEST_MAINT_AUTO_DETACH" and only unset it as
> required.
>
> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> ---
>  t/t7900-maintenance.sh | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/t/t7900-maintenance.sh b/t/t7900-maintenance.sh
> index 6735a9e082..5fbb16f0f0 100755
> --- a/t/t7900-maintenance.sh
> +++ b/t/t7900-maintenance.sh
> @@ -7,9 +7,6 @@ test_description='git maintenance builtin'
>  GIT_TEST_COMMIT_GRAPH=0
>  GIT_TEST_MULTI_PACK_INDEX=0
>
> -# Ensure that auto-maintenance detaches as usual.
> -sane_unset GIT_TEST_MAINT_AUTO_DETACH
> -
>  test_lazy_prereq XMLLINT '
>  	xmllint --version
>  '
> @@ -71,6 +68,7 @@ test_expect_success 'maintenance.auto config option' '
>  	git init repo &&
>  	(
>  		cd repo &&
> +		sane_unset GIT_TEST_MAINT_AUTO_DETACH &&
>
>  		GIT_TRACE2_EVENT="$(pwd)/default" git commit --quiet --allow-empty -m 1 &&
>  		test_subcommand git maintenance run --auto --quiet --detach <default &&
> @@ -90,6 +88,7 @@ test_expect_success 'gc.auto config option' '
>  	git init repo &&
>  	(
>  		cd repo &&
> +		sane_unset GIT_TEST_MAINT_AUTO_DETACH &&
>
>  		GIT_TRACE2_EVENT="$(pwd)/default" git commit --quiet --allow-empty -m 1 &&
>  		test_subcommand git maintenance run --auto --quiet --detach <default &&
> @@ -107,6 +106,7 @@ test_expect_success 'maintenance.auto overrides gc.auto' '
>  	git init repo &&
>  	(
>  		cd repo &&
> +		sane_unset GIT_TEST_MAINT_AUTO_DETACH &&
>
>  		git config set maintenance.auto false &&
>  		git config set gc.auto 1 &&
>
> --
> 2.55.0.679.g6767b8d81c.dirty

So instead of unset everywhere we only do it selectively. Looks good.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 690 bytes --]

Re: [PATCH 2/2] t7900: fix flaky "maintenance.strategy" test

[Patrick Steinhardt]

On Wed, Aug 12, 2026 at 01:46:14AM -0700, Karthik Nayak wrote:
> Patrick Steinhardt <ps@pks.im> writes:
> 
> > One of our tests for whether "maintenance.strategy" is being respected
> > in t7900 is flaky in our CI systems:
> >
> >     + GIT_TRACE2_EVENT=/tmp/test-output/trash directory.t7900-maintenance/repo/trace2.txt git -c maintenance.strategy=incremental maintenance run --quiet
> >     + test_maintenance_tasks trace2.txt
> >     + cat
> >     + sed -ne s/.*"region_enter".*"category":"maintenance\([^"]*\)".*"label":"\([^"][^"]*\)".*/\2\1/p trace2.txt
> >     + test_cmp expect actual
> >     + test 2 -ne 2
> >     + eval /usr/bin/diff -u "$@"
> >     + /usr/bin/diff -u expect actual
> >     --- expect	2026-08-07 06:20:51.388322602 +0000
> >     +++ actual	2026-08-07 06:20:51.388322602 +0000
> >     @@ -1,2 +0,0 @@
> >     -gc foreground
> >     -gc
> >
> > When running with the "incremental" strategy, we expect two git-gc(1)
> > tasks to have been executed, but sometimes the test simply doesn't
> > execute any of those tasks.
> >
> > A first hunch may be that maybe the disk-state is sometimes different
> > and thus we decide not to run maintenance. But git-maintenance(1)
> > doesn't run with the "--auto" switch, so we should execute those tasks
> > regardless of the on-disk state.
> >
> > But there's a second condition that may cause us to not execute tasks,
> > namely when the "maintenance.lock" file exists due to a concurrently
> 
> Nit: s/a//
> 
> > running tasks. We usually disable auto-maintenance from detaching in our
> > test suite to avoid exactly these kinds of race conditions, but in t7900
> > we unset "GIT_TEST_MAINT_AUTO_DETACH" and thus enable the auto-detach
> > logic. The intent of this is to exercise git-maintenance(1) closer to
> > how it would run in a real-world scenario, but it does cause us to race
> > when the detached maintenance job that was triggered by `test_commit()`
> > lives long enough.
> 
> GIT_TEST_MAINT_AUTO_DETACH when set to true enables auto-detach, but
> also the default value when unset is true. That's why unsetting it
> enables auto-detach. That's a bit confusing.

I'll reword this paragraph a bit. Thanks!

Patrick

[PATCH v2 0/2] t7900: fix flaky "maintenance.strategy" test

[Patrick Steinhardt]

Hi,

I've recently noticed that t7900 is flaky, see for example [1].
The root cause of the flake is the auto-detaching logic of
git-maintenance(1), which sometimes causes us to skip maintenance
altogether when the foreground process is racing with background
maintenance.

Changes in v2:
  - Perform some word smithing on commit messages.
  - Link to v1: https://patch.msgid.link/20260807-pks-t7900-fix-flaky-test-v1-0-08d0ea0fbbc5@pks.im

Thanks!

Patrick

[1]: https://gitlab.com/gitlab-org/git/-/jobs/15762975482

---
Patrick Steinhardt (2):
      t7900: adapt some tests to use a throwaway repository
      t7900: fix flaky "maintenance.strategy" test

 t/t7900-maintenance.sh | 76 ++++++++++++++++++++++++++++++--------------------
 1 file changed, 46 insertions(+), 30 deletions(-)

Range-diff versus v1:

1:  10521f07ad ! 1:  1f3f8aa538 t7900: adapt some tests to use a throwaway repository
    @@ Commit message
         tests more neatly self-contained and allows us to trivially modify the
         environment in the next commit.
     
    +    Note that we adapt calls to `test_config ()` to use git-config(1)
    +    instead. This is because on the one hand we don't need the auto-revert
    +    logic of `test_config ()` as we're using a throwaway repository anyway.
    +    On the other hand it's not possible to use `test_config ()` as it uses
    +    `test_when_finished ()`, which errors out when we run it in a subshell.
    +
         Signed-off-by: Patrick Steinhardt <ps@pks.im>
     
      ## t/t7900-maintenance.sh ##
2:  71cb84a4a7 ! 2:  ba1fbb27f9 t7900: fix flaky "maintenance.strategy" test
    @@ Commit message
     
         But there's a second condition that may cause us to not execute tasks,
         namely when the "maintenance.lock" file exists due to a concurrently
    -    running tasks. We usually disable auto-maintenance from detaching in our
    -    test suite to avoid exactly these kinds of race conditions, but in t7900
    +    running git-maintenance(1) process. We usually disable auto-maintenance
    +    from detaching in our test suite to avoid exactly these kinds of race
    +    conditions by exporting `GIT_TEST_MAINT_AUTO_DETACH=false`. But in t7900
         we unset "GIT_TEST_MAINT_AUTO_DETACH" and thus enable the auto-detach
         logic. The intent of this is to exercise git-maintenance(1) closer to
         how it would run in a real-world scenario, but it does cause us to race

---
base-commit: 2c78326f810173a4f3aefd8021f1e07575412481
change-id: 20260807-pks-t7900-fix-flaky-test-160abfcef65a

[PATCH v2 1/2] t7900: adapt some tests to use a throwaway repository

[Patrick Steinhardt]

Many of the tests in t7900 operate inside the main trash repository
that's set up by default by our test suite. This is overall quite
fragile as we're exercising repository maintenance in those tests, and
maintenance is of course intricately tied towards the on-disk state of a
repository. Consequently, the tests can easily impact one another.

Furthermore, in the next commit we'll have to modify the environment in
a handful of those tests. As tests don't run in a subshell, doing so
would impact all subsequent tests by default, as well.

Adapt exactly those tests to use a throwaway repository. This makes the
tests more neatly self-contained and allows us to trivially modify the
environment in the next commit.

Note that we adapt calls to `test_config ()` to use git-config(1)
instead. This is because on the one hand we don't need the auto-revert
logic of `test_config ()` as we're using a throwaway repository anyway.
On the other hand it's not possible to use `test_config ()` as it uses
`test_when_finished ()`, which errors out when we run it in a subshell.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 t/t7900-maintenance.sh | 70 +++++++++++++++++++++++++++++++-------------------
 1 file changed, 43 insertions(+), 27 deletions(-)

diff --git a/t/t7900-maintenance.sh b/t/t7900-maintenance.sh
index 4238569b68..6735a9e082 100755
--- a/t/t7900-maintenance.sh
+++ b/t/t7900-maintenance.sh
@@ -67,41 +67,57 @@ test_expect_success 'run [--auto|--quiet] with gc strategy' '
 '
 
 test_expect_success 'maintenance.auto config option' '
-	GIT_TRACE2_EVENT="$(pwd)/default" git commit --quiet --allow-empty -m 1 &&
-	test_subcommand git maintenance run --auto --quiet --detach <default &&
-	GIT_TRACE2_EVENT="$(pwd)/true" \
-		git -c maintenance.auto=true \
-		commit --quiet --allow-empty -m 2 &&
-	test_subcommand git maintenance run --auto --quiet --detach <true &&
-	GIT_TRACE2_EVENT="$(pwd)/false" \
-		git -c maintenance.auto=false \
-		commit --quiet --allow-empty -m 3 &&
-	test_subcommand ! git maintenance run --auto --quiet --detach <false
+	test_when_finished "rm -rf repo" &&
+	git init repo &&
+	(
+		cd repo &&
+
+		GIT_TRACE2_EVENT="$(pwd)/default" git commit --quiet --allow-empty -m 1 &&
+		test_subcommand git maintenance run --auto --quiet --detach <default &&
+		GIT_TRACE2_EVENT="$(pwd)/true" \
+			git -c maintenance.auto=true \
+			commit --quiet --allow-empty -m 2 &&
+		test_subcommand git maintenance run --auto --quiet --detach <true &&
+		GIT_TRACE2_EVENT="$(pwd)/false" \
+			git -c maintenance.auto=false \
+			commit --quiet --allow-empty -m 3 &&
+		test_subcommand ! git maintenance run --auto --quiet --detach <false
+	)
 '
 
 test_expect_success 'gc.auto config option' '
-	GIT_TRACE2_EVENT="$(pwd)/default" git commit --quiet --allow-empty -m 1 &&
-	test_subcommand git maintenance run --auto --quiet --detach <default &&
-	GIT_TRACE2_EVENT="$(pwd)/true" \
-		git -c gc.auto=1 commit --quiet --allow-empty -m 2 &&
-	test_subcommand git maintenance run --auto --quiet --detach <true &&
-	GIT_TRACE2_EVENT="$(pwd)/false" \
-		git -c gc.auto=0 commit --quiet --allow-empty -m 3 &&
-	test_subcommand ! git maintenance run --auto --quiet --detach <false
+	test_when_finished "rm -rf repo" &&
+	git init repo &&
+	(
+		cd repo &&
+
+		GIT_TRACE2_EVENT="$(pwd)/default" git commit --quiet --allow-empty -m 1 &&
+		test_subcommand git maintenance run --auto --quiet --detach <default &&
+		GIT_TRACE2_EVENT="$(pwd)/true" \
+			git -c gc.auto=1 commit --quiet --allow-empty -m 2 &&
+		test_subcommand git maintenance run --auto --quiet --detach <true &&
+		GIT_TRACE2_EVENT="$(pwd)/false" \
+			git -c gc.auto=0 commit --quiet --allow-empty -m 3 &&
+		test_subcommand ! git maintenance run --auto --quiet --detach <false
+	)
 '
 
 test_expect_success 'maintenance.auto overrides gc.auto' '
-	test_when_finished "rm -f trace" &&
+	test_when_finished "rm -rf repo" &&
+	git init repo &&
+	(
+		cd repo &&
 
-	test_config maintenance.auto false &&
-	test_config gc.auto 1 &&
-	GIT_TRACE2_EVENT="$(pwd)/trace" git commit --quiet --allow-empty -m 1 &&
-	test_subcommand ! git maintenance run --auto --quiet --detach <trace &&
+		git config set maintenance.auto false &&
+		git config set gc.auto 1 &&
+		GIT_TRACE2_EVENT="$(pwd)/trace" git commit --quiet --allow-empty -m 1 &&
+		test_subcommand ! git maintenance run --auto --quiet --detach <trace &&
 
-	test_config maintenance.auto true &&
-	test_config gc.auto 0 &&
-	GIT_TRACE2_EVENT="$(pwd)/trace" git commit --quiet --allow-empty -m 1 &&
-	test_subcommand git maintenance run --auto --quiet --detach <trace
+		git config set maintenance.auto true &&
+		git config set gc.auto 0 &&
+		GIT_TRACE2_EVENT="$(pwd)/trace" git commit --quiet --allow-empty -m 1 &&
+		test_subcommand git maintenance run --auto --quiet --detach <trace
+	)
 '
 
 for cfg in maintenance.autoDetach gc.autoDetach

-- 
2.55.0.679.g6767b8d81c.dirty

[PATCH v2 2/2] t7900: fix flaky "maintenance.strategy" test

[Patrick Steinhardt]

One of our tests for whether "maintenance.strategy" is being respected
in t7900 is flaky in our CI systems:

    + GIT_TRACE2_EVENT=/tmp/test-output/trash directory.t7900-maintenance/repo/trace2.txt git -c maintenance.strategy=incremental maintenance run --quiet
    + test_maintenance_tasks trace2.txt
    + cat
    + sed -ne s/.*"region_enter".*"category":"maintenance\([^"]*\)".*"label":"\([^"][^"]*\)".*/\2\1/p trace2.txt
    + test_cmp expect actual
    + test 2 -ne 2
    + eval /usr/bin/diff -u "$@"
    + /usr/bin/diff -u expect actual
    --- expect	2026-08-07 06:20:51.388322602 +0000
    +++ actual	2026-08-07 06:20:51.388322602 +0000
    @@ -1,2 +0,0 @@
    -gc foreground
    -gc

When running with the "incremental" strategy, we expect two git-gc(1)
tasks to have been executed, but sometimes the test simply doesn't
execute any of those tasks.

A first hunch may be that maybe the disk-state is sometimes different
and thus we decide not to run maintenance. But git-maintenance(1)
doesn't run with the "--auto" switch, so we should execute those tasks
regardless of the on-disk state.

But there's a second condition that may cause us to not execute tasks,
namely when the "maintenance.lock" file exists due to a concurrently
running git-maintenance(1) process. We usually disable auto-maintenance
from detaching in our test suite to avoid exactly these kinds of race
conditions by exporting `GIT_TEST_MAINT_AUTO_DETACH=false`. But in t7900
we unset "GIT_TEST_MAINT_AUTO_DETACH" and thus enable the auto-detach
logic. The intent of this is to exercise git-maintenance(1) closer to
how it would run in a real-world scenario, but it does cause us to race
when the detached maintenance job that was triggered by `test_commit()`
lives long enough.

We could trivially fix this race by disabling auto-maintenance for this
specific test. But that doesn't fix this class of races in this test
suite: while I haven't seen any of the other tests fail in the same way,
a bunch of them have this race, as well.

Instead, let's retain "GIT_TEST_MAINT_AUTO_DETACH" and only unset it as
required.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 t/t7900-maintenance.sh | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/t/t7900-maintenance.sh b/t/t7900-maintenance.sh
index 6735a9e082..5fbb16f0f0 100755
--- a/t/t7900-maintenance.sh
+++ b/t/t7900-maintenance.sh
@@ -7,9 +7,6 @@ test_description='git maintenance builtin'
 GIT_TEST_COMMIT_GRAPH=0
 GIT_TEST_MULTI_PACK_INDEX=0
 
-# Ensure that auto-maintenance detaches as usual.
-sane_unset GIT_TEST_MAINT_AUTO_DETACH
-
 test_lazy_prereq XMLLINT '
 	xmllint --version
 '
@@ -71,6 +68,7 @@ test_expect_success 'maintenance.auto config option' '
 	git init repo &&
 	(
 		cd repo &&
+		sane_unset GIT_TEST_MAINT_AUTO_DETACH &&
 
 		GIT_TRACE2_EVENT="$(pwd)/default" git commit --quiet --allow-empty -m 1 &&
 		test_subcommand git maintenance run --auto --quiet --detach <default &&
@@ -90,6 +88,7 @@ test_expect_success 'gc.auto config option' '
 	git init repo &&
 	(
 		cd repo &&
+		sane_unset GIT_TEST_MAINT_AUTO_DETACH &&
 
 		GIT_TRACE2_EVENT="$(pwd)/default" git commit --quiet --allow-empty -m 1 &&
 		test_subcommand git maintenance run --auto --quiet --detach <default &&
@@ -107,6 +106,7 @@ test_expect_success 'maintenance.auto overrides gc.auto' '
 	git init repo &&
 	(
 		cd repo &&
+		sane_unset GIT_TEST_MAINT_AUTO_DETACH &&
 
 		git config set maintenance.auto false &&
 		git config set gc.auto 1 &&

-- 
2.55.0.679.g6767b8d81c.dirty