On Wed, Mar 24, 2021 at 11:57:46PM +0100, Ævar Arnfjörð Bjarmason wrote:

On Wed, Mar 24 2021, SZEDER Gábor wrote:

quoted

On Wed, Mar 24, 2021 at 01:01:49PM +0100, Johannes Schindelin wrote:

quoted

Hi Ævar,

On Sat, 20 Mar 2021, Ævar Arnfjörð Bjarmason wrote:

quoted

On Fri, Mar 19 2021, Johannes Schindelin wrote:

quoted

quoted

On Tue, Jan 29 2019, Johannes Schindelin via GitGitGadget wrote:

quoted

From: Johannes Schindelin <redacted>

It seems that every once in a while in the Git for Windows SDK, there
are some transient file locking issues preventing the test clean up to
delete the trash directory. Let's be gentle and try again five seconds
later, and only error out if it still fails the second time.

This change helps Windows, and does not hurt any other platform
(normally, it is highly unlikely that said deletion fails, and if it
does, normally it will fail again even 5 seconds later).

Signed-off-by: Johannes Schindelin <redacted>
---
 t/test-lib.sh | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/t/test-lib.sh b/t/test-lib.sh
index f31a1c8f79..9c0ca5effb 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh

@@ -1104,7 +1104,11 @@ test_done () {
 			error "Tests passed but trash directory already removed before test cleanup; aborting"

 			cd "$TRASH_DIRECTORY/.." &&
-			rm -fr "$TRASH_DIRECTORY" ||
+			rm -fr "$TRASH_DIRECTORY" || {
+				# try again in a bit
+				sleep 5;
+				rm -fr "$TRASH_DIRECTORY"
+			} ||
 			error "Tests passed but test cleanup failed; aborting"
 		fi
 		test_at_end_hook_

I saw this sleep while reading some test-lib.sh code, doesn't this break
df4c0d1a79 (test-lib: abort when can't remove trash directory,
2017-04-20) for non-Windows platforms?

It does not really break it, it just delays the inevitable failure.

I still think this is the best answer to this (implicit) question:

quoted

In any case, your patch clearly undoes whatever canary for gc issues
df4c0d1a792 was trying to put into the test-lib, but didn't say so in
its commit message.

I was not _really_ paying attention to that commit when I implemented the
work-around you mentioned above. At the same time I think it does _not_
undo the canary. If the trash directory cannot be removed via `rm -fr`,
and if that is an indicator for something fishy going on, chances are that
the second `rm -fr` a couple seconds later will _also_ fail, and we still
get that error message.

The only reason why the second `rm` should succeed, at least that I can
think of, is that something on Windows blocked those files from being
deleted, and it is no longer blocking after a couple seconds, and that
usually means that an anti-malware scanned those files.

Both commits referenced in df4c0d1a79's log message fixed races
between 'test_done's cleanup and a still running background 'git gc',
and df4c0d1a79 was meant to draw our attention to similar issues in
the future.  And it did:

  https://public-inbox.org/git/20190602091919.GN951@szeder.dev/
  
So no, the failure is not inevitable, there are other reasons why the
second 'rm' might still succeed after the first failed, even just a
fraction of a second later.  And yes, that 'sleep' added in the patch
above did unquestionably break that canary,

Having read that thread now I agree, but I also left with a "who cares?"
and "so let's keep the sleep then?".

I.e. is this a problem that any of the software we're maintaining is
going to care about in the wild, it's not like people are expecting gc,
repack, fast-import etc. to behave well in the face of rm -rfing the
directory they're operating on.

So it seems like just an issue that crops up because of how our test
suite manages and removes per-test trash directories.

Not at all.  The real problem is that some stray background git
process is *still* actively writing to the test repository when the
test script is already supposed to be finished.

So it seems better
to:

 1. Just keep that "sleep a bit" and retry hack

 2. Maybe on some/most platforms we can use cgroups or whatever passes
    for a reliable "I started a process tree starting at this PID, kill
    -9 the whole thing please" before cleanup these days.

What really seems better:

  3. Keep that "sleep a bit" hack only on platforms that can't delete
     opened files.