On Fri, Aug 16, 2024 at 12:45:15PM +0200, Patrick Steinhardt wrote:
> +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
