patch, 2 partst7900: fix flaky "maintenance.strategy" test

13 messages between Aug 7, 2026 and Aug 13, 2026, from Patrick Steinhardt, Karthik Nayak.

Plain Markdown or JSON for tools and agents. Diffs are folded; open one to read it.

Patrick SteinhardtAug 7, 2026, 10:59 UTC on lore

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

Patrick SteinhardtAug 7, 2026, 10:59 UTC in reply to Patrick Steinhardt on lore

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

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(-)

Show changes to t/t7900-maintenance.sh +43 −27

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

Patrick SteinhardtAug 7, 2026, 10:59 UTC in reply to Patrick Steinhardt on lore

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

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(-)

Show changes to t/t7900-maintenance.sh +3 −3

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

Karthik NayakAug 12, 2026, 08:19 UTC in reply to Patrick Steinhardt on lore

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

Patrick Steinhardt <ps@pks.im> writes:

Show 93 quoted lines

> 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.

Show 18 quoted lines

> +		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.

Karthik NayakAug 12, 2026, 08:19 UTC in reply to Patrick Steinhardt on lore

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

Patrick Steinhardt <ps@pks.im> writes:

Show 93 quoted lines

> 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 &&

Show 18 quoted lines

> +		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.

Karthik NayakAug 12, 2026, 08:46 UTC in reply to Patrick Steinhardt on lore

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

Patrick Steinhardt <ps@pks.im> writes:

Show 28 quoted lines

> 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//

Show 7 quoted lines

> 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.

Show 55 quoted lines

>
> 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.

Patrick SteinhardtAug 12, 2026, 10:07 UTC in reply to Karthik Nayak on lore

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

On Wed, Aug 12, 2026 at 01:19:13AM -0700, Karthik Nayak wrote:

Show 6 quoted lines

> 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]

Show 18 quoted lines

> >  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

Patrick SteinhardtAug 12, 2026, 10:09 UTC in reply to Karthik Nayak on lore

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

On Wed, Aug 12, 2026 at 01:46:14AM -0700, Karthik Nayak wrote:

Show 44 quoted lines

> 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

Patrick SteinhardtAug 12, 2026, 10:11 UTC in reply to Patrick Steinhardt on lore

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

Hi,

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

Patrick SteinhardtAug 12, 2026, 10:11 UTC in reply to Patrick Steinhardt on lore

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

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(-)

Show changes to t/t7900-maintenance.sh +43 −27

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

Patrick SteinhardtAug 12, 2026, 10:11 UTC in reply to Patrick Steinhardt on lore

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

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.

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.

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(-)

Show changes to t/t7900-maintenance.sh +3 −3

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

Karthik NayakAug 13, 2026, 12:07 UTC in reply to Patrick Steinhardt on lore

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

Patrick Steinhardt <ps@pks.im> writes:

Show 33 quoted lines

> 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

Right, that slipped my mind entirely.

Karthik NayakAug 13, 2026, 12:08 UTC in reply to Patrick Steinhardt on lore

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

Patrick Steinhardt <ps@pks.im> writes:

Show 59 quoted lines

> 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

The range diff and this version looks good. Thanks!