Re: [PATCH 1/8] t: fix races caused by background maintenance

[Justin Tobler <jltobler@gmail.com>]

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 <ps@pks.im>
> ---
>  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