Re: [PATCH v3 6/7] builtin/maintenance: add a `--detach` flag

From: Jeff King <peff@peff.net>
Date: Aug 17, 2024, 07:09 UTC
Message-ID: <20240817070924.GA1438563@coredump.intra.peff.net>
In-Reply-To: <347d0a200201ce215f5b2c46d23de0cdd0181956.1723804990.git.ps@pks.im>

On Fri, Aug 16, 2024 at 12:45:15PM +0200, Patrick Steinhardt wrote:

Show 24 quoted lines

> +test_expect_success '--no-detach causes maintenance to not run in background' '
> [...]
> +		# We have no better way to check whether or not the task ran in
> +		# the background than to verify whether it output anything. The
> +		# next testcase checks the reverse, making this somewhat safer.
> +		git maintenance run --no-detach >out 2>&1 &&
> +		test_line_count = 1 out
> [...]
> +test_expect_success '--detach causes maintenance to run in background' '
> +	test_when_finished "rm -rf repo" &&
> +	git init repo &&
> +	(
> +		cd repo &&
> +
> +		test_commit something &&
> +		git config set maintenance.gc.enabled false &&
> +		git config set maintenance.loose-objects.enabled true &&
> +		git config set maintenance.loose-objects.auto 1 &&
> +		git config set maintenance.incremental-repack.enabled true &&
> +
> +		git maintenance run --detach >out 2>&1 &&
> +		test_must_be_empty out
> +	)
> +'

This second test seems to fail racily (or maybe always? see below). In CI on Windows, I saw:

  'out' is not empty, it contains:
  fc9fea69579f349e3b02e3264cffbef03e4b1852

That would make sense to me if the detached process still held the original stdout/stderr channel open (in which case we'd racily see the same line as in the no-detach case). But we do appear to call daemonize(), which closes both.

Curiously, the code in gc.c does this:

          /* Failure to daemonize is ok, we'll continue in foreground. */
          if (opts->detach > 0)
                  daemonize();

and the only way for daemonize to fail is if NO_POSIX_GOODIES is set. Which I'd expect on Windows. But then I'd expect this test to _always_ fail on Windows. Does it? If so, should it be marked with !MINGW?

While investigating that, I ran it with --stress locally (on Linux) and got some odd (and definitely racy) results. The test itself passes, but the "rm -rf repo" in the test_when_finished sometimes fails with:

  rm: cannot remove 'repo/.git/objects': Directory not empty

or similar (sometimes it's another directory like 'repo/.git'). My guess is that the background process is still running and creating files in the repository, racing with rm's call to rmdir().

Even if we remove the test_when_finished, it would mean that the final cleanup after test_done might similarly fail, leaving a crufty trash directory. I think to make this robust, we'd need some way of detecting when the background process has finished. I don't think we report the pid anywhere, and the daemonize() call means it won't even be in the same process group. Maybe we could spin looking for the incremental pack it will create (and timeout after N seconds)? That feels pretty hacky, but I can't think of anything better.

-Peff