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

Karthik Nayak <[email protected]> Wed, 12 Aug 2026 01:46:14 -0700
Newsgroups	org.kernel.vger.git
Message-ID	<CAOLa=ZSW+Ta5ktauamTUvp+fmjC4HHDpKOQ0sri+pBfLGq6mOg@mail.gmail.com>
Patrick Steinhardt <[email protected]> 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 <[email protected]>
> ---
>  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.
signature.asc (application/pgp-signature, 690 B)