From: Justin Tobler Date: Mon, 23 Feb 2026 16:01:48 GMT Subject: Re: [PATCH 1/8] t: fix races caused by background maintenance Message-ID: In-Reply-To: <20260220-b4-pks-maintenance-default-geometric-strategy-v1-1-faeb321ad13b@pks.im> On 26/02/20 11:15AM, Patrick Steinhardt wrote: > Many Git commands spawn git-maintenance(1) to optimize the repository in > the background. By default, performing the maintenance is for most of > the part asynchronous: we fork the executable and then continue with the > rest of our business logic. > > This is working as expected for our users, but this behaviour is > somewhat problematic for our test suite as this is inherently racy. We > have many tests that verify the on-disk state of repositories, and those > tests may easily race with our background maintenance. In a similar > fashion, we may end up with processes that "leak" out of a current test > case. > > Until now this tends to not be much of a problem. Our maintenance uses > git-gc(1) by default, which knows to bail out in case there aren't > either too many packfiles or too many loose objects. So even if other > data structures would need to be optimized, we won't do so unless the > object database also needs optimizations. > > This is about to change though, as a subsequent commit will switch to > the "geometric" maintenance strategy as a default. The consequence is > that we will run required optimizations even if the object database is > well-optimized. And this uncovers races between our test suite and > background maintenance all over the place. > > Disabling maintenance outright in our test suite is not really an > option, as it would result in significantly divergence from the "real s/significantly/significant/ > world" and reduce our test coverage. But we've got an alternative up our > sleeves: we can ensure that garbage collection runs synchronously by > overriding the "maintenance.autoDetach" configuration. > > Of course that also diverges from the real world, as we now stop testing > that background maintenance interacts in a benign way with normal Git > commands. But on the other hand this ensures that the maintenance itself > does not for example lead to data loss in a more reproducible way. > > Another concern is that this would make execution of the test suite much > slower. But a quick benchmark on my machine demonstrates that this does > not seem to be the case: > > Benchmark 1: meson test (revision = HEAD~) > Time (mean ± σ): 131.182 s ± 1.293 s [User: 853.737 s, System: 1160.479 s] > Range (min … max): 130.001 s … 132.563 s 3 runs > > Benchmark 2: meson test (revision = HEAD) > Time (mean ± σ): 129.554 s ± 0.507 s [User: 849.040 s, System: 1152.664 s] > Range (min … max): 129.000 s … 129.994 s 3 runs > > Summary > meson test (revision = HEAD) ran > 1.01 ± 0.01 times faster than meson test (revision = HEAD~) > > Funny enough, it even seems as if this speeds up test execution ever so > slightly, but that may just as well be noise. > > Introduce a new `GIT_TEST_MAINT_AUTO_DETACH` environment variable that > allows us to override the auto-detach behaviour and set that varibale in s/varibale/variable/ > our tests. > > Signed-off-by: Patrick Steinhardt > --- > run-command.c | 2 +- > t/t5616-partial-clone.sh | 6 +++--- > t/t7900-maintenance.sh | 1 + > t/test-lib.sh | 4 ++++ > 4 files changed, 9 insertions(+), 4 deletions(-) > > diff --git a/run-command.c b/run-command.c > index e3e02475cc..438a290d30 100644 > --- a/run-command.c > +++ b/run-command.c > @@ -1828,7 +1828,7 @@ int prepare_auto_maintenance(int quiet, struct child_process *maint) > */ > if (repo_config_get_bool(the_repository, "maintenance.autodetach", &auto_detach) && > repo_config_get_bool(the_repository, "gc.autodetach", &auto_detach)) > - auto_detach = 1; > + auto_detach = git_env_bool("GIT_TEST_MAINT_AUTO_DETACH", true); So now if "maintenance.autodetach" and "gc.autodetach" are both not set, we then check for the "GIT_TEST_MAINT_AUTO_DETACH" env before defaulting to true. Looks good. > > maint->git_cmd = 1; > maint->close_object_store = 1; [snip] > diff --git a/t/t7900-maintenance.sh b/t/t7900-maintenance.sh > index 7cc0ce57f8..d11d6f8f15 100755 > --- a/t/t7900-maintenance.sh > +++ b/t/t7900-maintenance.sh > @@ -6,6 +6,7 @@ test_description='git maintenance builtin' > > GIT_TEST_COMMIT_GRAPH=0 > GIT_TEST_MULTI_PACK_INDEX=0 > +sane_unset GIT_TEST_MAINT_AUTO_DETACH I assume here we are unsetting the env for testing purposes. It might be nice to leave some sort of breadcrumb comment here to explain to future readers. > test_lazy_prereq XMLLINT ' > xmllint --version > diff --git a/t/test-lib.sh b/t/test-lib.sh > index 0fb76f7d11..aa805a01ce 100644 > --- a/t/test-lib.sh > +++ b/t/test-lib.sh > @@ -1947,6 +1947,10 @@ test_lazy_prereq COMPAT_HASH ' > GIT_TEST_MAINT_SCHEDULER="none:exit 1" > export GIT_TEST_MAINT_SCHEDULER > > +# Ensure that tests cannot race with background maintenance by default. > +GIT_TEST_MAINT_AUTO_DETACH="false" > +export GIT_TEST_MAINT_AUTO_DETACH Looks good. -Justin