v2.47.0-rc1 test failure on cygwin

> Hi Patrick,
> 
> Just a quick heads up: t0610-reftable-basics.sh test 47 (ref transaction: many
> concurrent writers) fails on cygwin. The tail end of the debug output for this
> test looks like:
> 

> 
> t0610-reftable-basics.sh passed on 'rc0', but this test (and the timeout facility)
> is new in 'rc1'. I tried simply increasing the timeout (10 fold), but that didn't
> change the result. (I didn't really expect it to - the 'reftable: transaction
> prepare: I/O error' does not look timing related!).
> 
> Again, just a heads up. (I can't look at it until tomorrow now; any ideas?)

> On Fri, Oct 04, 2024 at 02:02:44AM +0100, Ramsay Jones wrote:
> > Hi Patrick,
> > 
> > Just a quick heads up: t0610-reftable-basics.sh test 47 (ref transaction: many
> > concurrent writers) fails on cygwin. The tail end of the debug output for this
> > test looks like:
> > 
> [snip]
> > 
> > t0610-reftable-basics.sh passed on 'rc0', but this test (and the timeout facility)
> > is new in 'rc1'. I tried simply increasing the timeout (10 fold), but that didn't
> > change the result. (I didn't really expect it to - the 'reftable: transaction
> > prepare: I/O error' does not look timing related!).
> > 
> > Again, just a heads up. (I can't look at it until tomorrow now; any ideas?)
> 
> This failure is kind of known and discussed in [1]. Just to make it
> explicit: this test failure doesn't really surface a regression, the
> reftable code already failed for concurrent writes before. I fixed that
> and added the test that is now flaky, as the fix itself is seemingly
> only sufficient on Linux and macOS.
> 
> I didn't yet have the time to look at whether I can fix it, but should
> finally find the time to do so today.

> On Fri, Oct 04, 2024 at 05:59:30AM +0200, Patrick Steinhardt wrote:
> > On Fri, Oct 04, 2024 at 02:02:44AM +0100, Ramsay Jones wrote:
> > >
> > > Just a quick heads up: t0610-reftable-basics.sh test 47 (ref
> > > transaction: many concurrent writers) fails on cygwin. The tail end
> > > of the debug output for this test looks like:
> > >
> > [snip]
> > >
> > > t0610-reftable-basics.sh passed on 'rc0', but this test (and the
> > > timeout facility) is new in 'rc1'. I tried simply increasing the
> > > timeout (10 fold), but that didn't change the result. (I didn't
> > > really expect it to - the 'reftable: transaction prepare: I/O error'
> > > does not look timing related!).
> > >
> > > Again, just a heads up. (I can't look at it until tomorrow now; any
> > > ideas?)
> >
> > This failure is kind of known and discussed in [1]. Just to make it
> > explicit: this test failure doesn't really surface a regression, the
> > reftable code already failed for concurrent writes before. I fixed
> > that and added the test that is now flaky, as the fix itself is
> > seemingly only sufficient on Linux and macOS.
> >
> > I didn't yet have the time to look at whether I can fix it, but should
> > finally find the time to do so today.
>
> Hm, interestingly enough I cannot reproduce the issue on Cygwin myself,
> but I can reproduce the issue with MinGW. And in fact, the logs you have
> sent all indicate that we cannot acquire the lock, there is no sign of
> I/O errors here. So I guess you're running into timeout issues. Does the
> following patch fix this for you?
>
> diff --git a/t/t0610-reftable-basics.sh b/t/t0610-reftable-basics.sh
> index 2d951c8ceb..b5cad805d4 100755
> --- a/t/t0610-reftable-basics.sh
> +++ b/t/t0610-reftable-basics.sh
> @@ -455,10 +455,7 @@ test_expect_success 'ref transaction: many concurrent writers' '
>  	git init repo &&
>  	(
>  		cd repo &&
> -		# Set a high timeout such that a busy CI machine will not abort
> -		# early. 10 seconds should hopefully be ample of time to make
> -		# this non-flaky.
> -		git config set reftable.lockTimeout 10000 &&
> +		git config set reftable.lockTimeout -1 &&
>  		test_commit --no-tag initial &&
>
>  		head=$(git rev-parse HEAD) &&

> Hi Patrick,
> 
> On Fri, 4 Oct 2024, Patrick Steinhardt wrote:
> 
> > On Fri, Oct 04, 2024 at 05:59:30AM +0200, Patrick Steinhardt wrote:
> > > On Fri, Oct 04, 2024 at 02:02:44AM +0100, Ramsay Jones wrote:
> > > >
> > > > Just a quick heads up: t0610-reftable-basics.sh test 47 (ref
> > > > transaction: many concurrent writers) fails on cygwin. The tail end
> > > > of the debug output for this test looks like:
> > > >
> > > [snip]
> > > >
> > > > t0610-reftable-basics.sh passed on 'rc0', but this test (and the
> > > > timeout facility) is new in 'rc1'. I tried simply increasing the
> > > > timeout (10 fold), but that didn't change the result. (I didn't
> > > > really expect it to - the 'reftable: transaction prepare: I/O error'
> > > > does not look timing related!).
> > > >
> > > > Again, just a heads up. (I can't look at it until tomorrow now; any
> > > > ideas?)
> > >
> > > This failure is kind of known and discussed in [1]. Just to make it
> > > explicit: this test failure doesn't really surface a regression, the
> > > reftable code already failed for concurrent writes before. I fixed
> > > that and added the test that is now flaky, as the fix itself is
> > > seemingly only sufficient on Linux and macOS.
> > >
> > > I didn't yet have the time to look at whether I can fix it, but should
> > > finally find the time to do so today.
> >
> > Hm, interestingly enough I cannot reproduce the issue on Cygwin myself,
> > but I can reproduce the issue with MinGW. And in fact, the logs you have
> > sent all indicate that we cannot acquire the lock, there is no sign of
> > I/O errors here. So I guess you're running into timeout issues. Does the
> > following patch fix this for you?
> >
> > diff --git a/t/t0610-reftable-basics.sh b/t/t0610-reftable-basics.sh
> > index 2d951c8ceb..b5cad805d4 100755
> > --- a/t/t0610-reftable-basics.sh
> > +++ b/t/t0610-reftable-basics.sh
> > @@ -455,10 +455,7 @@ test_expect_success 'ref transaction: many concurrent writers' '
> >  	git init repo &&
> >  	(
> >  		cd repo &&
> > -		# Set a high timeout such that a busy CI machine will not abort
> > -		# early. 10 seconds should hopefully be ample of time to make
> > -		# this non-flaky.
> > -		git config set reftable.lockTimeout 10000 &&
> > +		git config set reftable.lockTimeout -1 &&
> >  		test_commit --no-tag initial &&
> >
> >  		head=$(git rev-parse HEAD) &&
> 
> That looks plausible to me, as this test is exercising the POSIX emulation
> layer of Cygwin much more than Git itself (and therefore I expect this
> test case to take a really long time on Cygwin). If it turns out that this
> works around the issue, I would propose to use something like this
> instead:
> 
> 		# Cygwin needs a slightly longer timeout
> 		if have_prereq !CYGWIN
> 		then
> 			git config set reftable.lockTimeout 10000
> 		else
> 			git config set reftable.lockTimeout 60000
> 		fi &&

> > The issue on Win32 is different: we cannot commit the "tables.list" lock
> > via rename(3P) because the target file may be open for reading by a
> > concurrent process. I guess that Cygwin has proper POSIX semantics for
> > rename(3P) and thus doesn't hit the same issue.
> 
> Indeed, this is where the symptom lies. I worked around it in Git for
> Windows v2.47.0-rc1 via this patch:
> 
> -- snipsnap --
> diff --git a/compat/mingw.c b/compat/mingw.c
> index c1030e40f227..56db193d1360 100644
> --- a/compat/mingw.c
> +++ b/compat/mingw.c
> @@ -784,6 +784,7 @@ int mingw_open (const char *filename, int oflags, ...)
>  	int fd, create = (oflags & (O_CREAT | O_EXCL)) == (O_CREAT | O_EXCL);
>  	wchar_t wfilename[MAX_LONG_PATH];
>  	open_fn_t open_fn;
> +	int tries = 0;
> 
>  	va_start(args, oflags);
>  	mode = va_arg(args, int);
> @@ -813,7 +814,11 @@ int mingw_open (const char *filename, int oflags, ...)
>  	else if (xutftowcs_long_path(wfilename, filename) < 0)
>  		return -1;
> 
> -	fd = open_fn(wfilename, oflags, mode);
> +	do {
> +		fd = open_fn(wfilename, oflags, mode);
> +	} while (fd < 0 && GetLastError() == ERROR_SHARING_VIOLATION &&
> +		 retry_ask_yes_no(&tries, "Opening '%s' failed because another "
> +			"application accessed it. Try again?", filename));
> 
>  	if ((oflags & O_CREAT) && fd >= 0 && are_wsl_compatible_mode_bits_enabled()) {
>  		_mode_t wsl_mode = S_IFREG | (mode&0777);

> On Fri, Oct 04, 2024 at 11:13:34AM +0200, Johannes Schindelin wrote:
> >
> > On Fri, 4 Oct 2024, Patrick Steinhardt wrote:
> >
> > > diff --git a/t/t0610-reftable-basics.sh b/t/t0610-reftable-basics.sh
> > > index 2d951c8ceb..b5cad805d4 100755
> > > --- a/t/t0610-reftable-basics.sh
> > > +++ b/t/t0610-reftable-basics.sh
> > > @@ -455,10 +455,7 @@ test_expect_success 'ref transaction: many concurrent writers' '
> > >  	git init repo &&
> > >  	(
> > >  		cd repo &&
> > > -		# Set a high timeout such that a busy CI machine will not abort
> > > -		# early. 10 seconds should hopefully be ample of time to make
> > > -		# this non-flaky.
> > > -		git config set reftable.lockTimeout 10000 &&
> > > +		git config set reftable.lockTimeout -1 &&
> > >  		test_commit --no-tag initial &&
> > >
> > >  		head=$(git rev-parse HEAD) &&
> >
> > That looks plausible to me, as this test is exercising the POSIX emulation
> > layer of Cygwin much more than Git itself (and therefore I expect this
> > test case to take a really long time on Cygwin). If it turns out that this
> > works around the issue, I would propose to use something like this
> > instead:
> >
> > 		# Cygwin needs a slightly longer timeout
> > 		if have_prereq !CYGWIN
> > 		then
> > 			git config set reftable.lockTimeout 10000
> > 		else
> > 			git config set reftable.lockTimeout 60000
> > 		fi &&
>
> We also saw that this creates an issue when running e.g. with the memory
> and leak sanitizers. So I'd just generally raise the timeout value to
> something way higher to avoid flakiness, like 5 minutes.

> > > The issue on Win32 is different: we cannot commit the "tables.list" lock
> > > via rename(3P) because the target file may be open for reading by a
> > > concurrent process. I guess that Cygwin has proper POSIX semantics for
> > > rename(3P) and thus doesn't hit the same issue.
> >
> > Indeed, this is where the symptom lies. I worked around it in Git for
> > Windows v2.47.0-rc1 via this patch:
> >
> > -- snipsnap --
> > diff --git a/compat/mingw.c b/compat/mingw.c
> > index c1030e40f227..56db193d1360 100644
> > --- a/compat/mingw.c
> > +++ b/compat/mingw.c
> > @@ -784,6 +784,7 @@ int mingw_open (const char *filename, int oflags, ...)
> >  	int fd, create = (oflags & (O_CREAT | O_EXCL)) == (O_CREAT | O_EXCL);
> >  	wchar_t wfilename[MAX_LONG_PATH];
> >  	open_fn_t open_fn;
> > +	int tries = 0;
> >
> >  	va_start(args, oflags);
> >  	mode = va_arg(args, int);
> > @@ -813,7 +814,11 @@ int mingw_open (const char *filename, int oflags, ...)
> >  	else if (xutftowcs_long_path(wfilename, filename) < 0)
> >  		return -1;
> >
> > -	fd = open_fn(wfilename, oflags, mode);
> > +	do {
> > +		fd = open_fn(wfilename, oflags, mode);
> > +	} while (fd < 0 && GetLastError() == ERROR_SHARING_VIOLATION &&
> > +		 retry_ask_yes_no(&tries, "Opening '%s' failed because another "
> > +			"application accessed it. Try again?", filename));
> >
> >  	if ((oflags & O_CREAT) && fd >= 0 && are_wsl_compatible_mode_bits_enabled()) {
> >  		_mode_t wsl_mode = S_IFREG | (mode&0777);
>
> Wait, this is surprising to me as I saw the error happening when calling
> rename, not open. So why would retries in `open()` fix the symptom? I'm
> probably missing something.

>
> Patrick
>
> @ -2152,9 +2152,12 @@ int mingw_accept(int sockfd1, struct sockaddr *sa, socklen_t *sz)
>  int mingw_rename(const char *pold, const char *pnew)
>  {
>         DWORD attrs, gle;
> -       int tries = 0;
> +       int tries = 0, wpnew_len, wpold_len;
>         wchar_t wpold[MAX_PATH], wpnew[MAX_PATH];
> -       if (xutftowcs_path(wpold, pold) < 0 || xutftowcs_path(wpnew, pnew) < 0)
> +
> +       wpold_len = xutftowcs_path(wpold, pold);
> +       wpnew_len = xutftowcs_path(wpnew, pnew);
> +       if (wpold_len < 0 || wpnew_len < 0)
>                 return -1;
>
>         /*
> @@ -2166,6 +2169,58 @@ int mingw_rename(const char *pold, const char *pnew)
>         if (errno != EEXIST)
>                 return -1;
>  repeat:
> +       {
> +#define FileRenameInfoEx                      22
> +               enum {
> +                       FILE_RENAME_REPLACE_IF_EXISTS         = 0x01,
> +                       FILE_RENAME_POSIX_SEMANTICS           = 0x02,
> +                       FILE_RENAME_IGNORE_READONLY_ATTRIBUTE = 0x40,
> +               };
> +               typedef struct FILE_RENAME_INFORMATION {
> +                       union {
> +                               BOOLEAN ReplaceIfExists;
> +                               ULONG Flags;
> +                       };
> +                       HANDLE RootDirectory;
> +                       ULONG FileNameLength;
> +                       WCHAR FileName[1];
> +               } *PFILE_RENAME_INFORMATION;
> +               size_t wpnew_size = wpnew_len * sizeof(wchar_t);
> +               PFILE_RENAME_INFORMATION fri = NULL;
> +               HANDLE handle = NULL;
> +               BOOL success;
> +               int ret;
> +
> +               handle = CreateFileW(wpold, DELETE,
> +                                    FILE_SHARE_READ | FILE_SHARE_WRITE | FILE_SHARE_DELETE, NULL,
> +                                    OPEN_EXISTING, FILE_ATTRIBUTE_NORMAL, NULL);
> +               if (handle == INVALID_HANDLE_VALUE) {
> +                       errno = ENOENT;
> +                       ret = -1;
> +                       goto out;
> +               }
> +
> +               fri = xcalloc(1, sizeof(*fri) + wpnew_size);
> +               fri->Flags = FILE_RENAME_REPLACE_IF_EXISTS | FILE_RENAME_POSIX_SEMANTICS |
> +                            FILE_RENAME_IGNORE_READONLY_ATTRIBUTE;
> +               fri->FileNameLength = wpnew_len;
> +               memcpy(fri->FileName, wpnew, wpnew_size);
> +
> +               success = SetFileInformationByHandle(handle, FileRenameInfoEx,
> +                                                    fri, sizeof(*fri) + wpnew_size);
> +               if (!success) {
> +                       errno = err_win_to_posix(GetLastError());
> +                       ret = -1;
> +                       goto out;
> +               }
> +
> +               ret = 0;
> +out:
> +               CloseHandle(handle);
> +               free(fri);
> +               return ret;
> +       }
> +
>
>

> Hi Patrick,
> > > > The issue on Win32 is different: we cannot commit the "tables.list" lock
> > > > via rename(3P) because the target file may be open for reading by a
> > > > concurrent process. I guess that Cygwin has proper POSIX semantics for
> > > > rename(3P) and thus doesn't hit the same issue.
> > >
> > > Indeed, this is where the symptom lies. I worked around it in Git for
> > > Windows v2.47.0-rc1 via this patch:
> > >
> > > -- snipsnap --
> > > diff --git a/compat/mingw.c b/compat/mingw.c
> > > index c1030e40f227..56db193d1360 100644
> > > --- a/compat/mingw.c
> > > +++ b/compat/mingw.c
> > > @@ -784,6 +784,7 @@ int mingw_open (const char *filename, int oflags, ...)
> > >  	int fd, create = (oflags & (O_CREAT | O_EXCL)) == (O_CREAT | O_EXCL);
> > >  	wchar_t wfilename[MAX_LONG_PATH];
> > >  	open_fn_t open_fn;
> > > +	int tries = 0;
> > >
> > >  	va_start(args, oflags);
> > >  	mode = va_arg(args, int);
> > > @@ -813,7 +814,11 @@ int mingw_open (const char *filename, int oflags, ...)
> > >  	else if (xutftowcs_long_path(wfilename, filename) < 0)
> > >  		return -1;
> > >
> > > -	fd = open_fn(wfilename, oflags, mode);
> > > +	do {
> > > +		fd = open_fn(wfilename, oflags, mode);
> > > +	} while (fd < 0 && GetLastError() == ERROR_SHARING_VIOLATION &&
> > > +		 retry_ask_yes_no(&tries, "Opening '%s' failed because another "
> > > +			"application accessed it. Try again?", filename));
> > >
> > >  	if ((oflags & O_CREAT) && fd >= 0 && are_wsl_compatible_mode_bits_enabled()) {
> > >  		_mode_t wsl_mode = S_IFREG | (mode&0777);
> >
> > Wait, this is surprising to me as I saw the error happening when calling
> > rename, not open. So why would retries in `open()` fix the symptom? I'm
> > probably missing something.
> 
> I am sorry, I did not really read the bug report in detail, I only meant
> to unblock Git for Windows v2.47.0-rc1 and thought I had a fix in hand. It
> certainly fixed the failures on my local machine, but it unfortunately did
> not fix the problems in CI.

> > In any case I also tried something like the below patch (sorry,
> > whitespace-broken).
> 
> Oh, you reminded me that the `mingw_rename()` function looks
> _substantially_ different in Git for Windows than in Git. I did not have
> the time (or the strength of mind, more like) to upstream those changes
> yet.
> 
> > But unfortunately this still caused permission errors when the new path
> > was held open by another process.
> 
> Yes, this will _always_ be a problem, I think. The
> `FILE_RENAME_POSIX_SEMANTICS` as per its documentation should help, but if
> it does not in your tests it might actually not quite work as advertised
> (wouldn't be the first time I encounter such an issue).

> > I think for now I'd still lean into the direction of adding the !WINDOWS
> > prerequisite to the test and increasing timeouts such that I can
> > continue to investigate without time pressure.
> 
> Let me bang my head against this problem for a little while longer. You
> might be right, though, that this is a thing we cannot fix in time for
> v2.47.0, which would be sad.

>> But unfortunately this still caused permission errors when the new path
>> was held open by another process.
>
> Yes, this will _always_ be a problem, I think. The
> `FILE_RENAME_POSIX_SEMANTICS` as per its documentation should help, but if
> it does not in your tests it might actually not quite work as advertised
> (wouldn't be the first time I encounter such an issue).
>
> I tried to read through the code (it's a lot!) to figure out whether there
> is potentially any situation when the `tables.list` file is opened but not
> closed immediately, but couldn't find any. Do you know off-hand of any
> such scenario?
>
>> I think for now I'd still lean into the direction of adding the !WINDOWS
>> prerequisite to the test and increasing timeouts such that I can
>> continue to investigate without time pressure.
>
> Let me bang my head against this problem for a little while longer. You
> might be right, though, that this is a thing we cannot fix in time for
> v2.47.0, which would be sad.

> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> 
> >> But unfortunately this still caused permission errors when the new path
> >> was held open by another process.
> >
> > Yes, this will _always_ be a problem, I think. The
> > `FILE_RENAME_POSIX_SEMANTICS` as per its documentation should help, but if
> > it does not in your tests it might actually not quite work as advertised
> > (wouldn't be the first time I encounter such an issue).
> >
> > I tried to read through the code (it's a lot!) to figure out whether there
> > is potentially any situation when the `tables.list` file is opened but not
> > closed immediately, but couldn't find any. Do you know off-hand of any
> > such scenario?
> >
> >> I think for now I'd still lean into the direction of adding the !WINDOWS
> >> prerequisite to the test and increasing timeouts such that I can
> >> continue to investigate without time pressure.
> >
> > Let me bang my head against this problem for a little while longer. You
> > might be right, though, that this is a thing we cannot fix in time for
> > v2.47.0, which would be sad.
> 
> If you folks think it would help stabilizing the tentative fix, I am
> open to the idea of delaying the 2.47 by a few days.  Currently the
> 2.47-final is scheduled on the 7th (Monday), but we can do 2.47-rc2
> on that day instead, and move the final to 10th (Thu) or 11th (Fri)
> [*].
> 
> Thanks, all, for working together.
> 
> 
> [Footnote]
> 
>  * All dates are US/Pacific, 10:00 am

> Right now I don't yet have a good idea for how to fix the issue. I've
> been trying a bunch of different things that, according to Windows docs,
> should've made the renames work. But they didn't, and I don't really
> have an alternative right now. So I'll need to keep on thinking about
> this, and maybe get some more help from people familiar with Windows.
>
> So deferring Git 2.47 because of it probably does not make much sense
> unless somebody can up with a solution. Also, as noted already, this is
> not a regression, the behaviour actually improved even on Windows. Not
> to the degree I was hoping for, but at least a bit. So that's another
> reason why I don't think it is worth deferring over this.

> Now the two reports are somewhat different from one another:
> 
>   - On Cygwin we hit timeouts because we fail to lock the "tables.list"
>     file within 10 seconds. The renames themselves succeed even when the
>     target file is open because Cygwin provides extensive compatibility
>     logic to make them work even when the target file is open already.

> 
>   - On MinGW we hit I/O errors on rename. While we do have some retry
>     logic in place to make the rename work in some cases, this is
>     seemingly not sufficient when there is this much contention around
>     the files.

> 
> Neither of these cases is a regression: the logic didn't work before the
> mentioned commit, and after the commit it performs well on Linux, macOS
> and in Cygwin, and at least a bit better with MinGW. But the tests show
> that we need to put more thought into how to make this work properly on
> MinGW systems.

> 
> The fact that Cygwin can work around this issue with better emulation of
> POSIX-style atomic renames shows that we can in theory make MinGW work
> better, as well. But doing so likely requires quite some fiddling with
> Windows internals, and Git v2.47 is about to be released in a couple
> days. This makes any potential fix quite risky as it would have to
> happen deep down in our rename(3P) implementation in "compat/mingw.c".
> 
> Let's instead work around both issues by disabling the test on MinGW
> and by significantly increasing the locking timeout for Cygwin. This
> bumped timeout also helps when running with e.g. the address and memory
> sanitizers, which also tend to significantly extend the runtime of this
> test.

> 
> This should be revisited after Git v2.47 is out.
> 
> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> ---

> On 04/10/2024 13:16, Patrick Steinhardt wrote:
> [snip]
> 
> > Now the two reports are somewhat different from one another:
> > 
> >   - On Cygwin we hit timeouts because we fail to lock the "tables.list"
> >     file within 10 seconds. The renames themselves succeed even when the
> >     target file is open because Cygwin provides extensive compatibility
> >     logic to make them work even when the target file is open already.
> 
> Hmm, not so much for me! :(
> 
> > 
> >   - On MinGW we hit I/O errors on rename. While we do have some retry
> >     logic in place to make the rename work in some cases, this is
> >     seemingly not sufficient when there is this much contention around
> >     the files.
> 
> I am seeing I/O errors.

> This fix can be applied to remove some of the stress with the Git v2.47
> release pending. If would of course be preferable to find an alternate
> fix that makes MinGW work as required, but if you take the 500 lines of
> code that is the rename(3P) implemenation of Cygwin as a hint you
> quickly figure out that this is a rather complex problem.

> In 6241ce2170 (refs/reftable: reload locked stack when preparing
> transaction, 2024-09-24) we have introduced a new test that exercises
> how the reftable backend behaves with many concurrent writers all racing
> with each other. This test was introduced after a couple of fixes in
> this context that should make concurrent writes behave gracefully. As it
> turns out though, Windows systems do not yet handle concurrent writes
> properly, as we've got two reports for Cygwin and MinGW failing in this
> newly added test.
> 
> The root cause of this is how we update the "tables.list" file: when
> writing a new stack of tables we first write the data into a lockfile
> and then rename that file into place. But Windows forbids us from doing
> that rename when the target path is open for reading by another process.
> And as the test races both readers and writers with each other we are
> quite likely to hit this edge case.
> 
> This is not a regression: the logic didn't work before the mentioned
> commit, and after the commit it performs well on Linux and macOS, and
> the situation on Windows should have at least improved a bit. But the
> test shows that we need to put more thought into how to make this work
> properly there.
> 
> Work around the issue by disabling the test on Windows for now. While at
> it, increase the locking timeout to address reported timeouts when using
> either the address or memory sanitizer, which also tend to significantly
> extend the runtime of this test.
> 
> This should be revisited after Git v2.47 is out.
> 
> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> ---
>  t/t0610-reftable-basics.sh | 17 ++++++++++++-----
>  1 file changed, 12 insertions(+), 5 deletions(-)
> 
> diff --git a/t/t0610-reftable-basics.sh b/t/t0610-reftable-basics.sh
> index 2d951c8ceb..babec7993e 100755
> --- a/t/t0610-reftable-basics.sh
> +++ b/t/t0610-reftable-basics.sh
> @@ -450,15 +450,22 @@ test_expect_success 'ref transaction: retry acquiring tables.list lock' '
>  	)
>  '
>  
> -test_expect_success 'ref transaction: many concurrent writers' '
> +# This test fails most of the time on Windows systems. The root cause is
> +# that Windows does not allow us to rename the "tables.list.lock" file into
> +# place when "tables.list" is open for reading by a concurrent process.
> +test_expect_success !WINDOWS 'ref transaction: many concurrent writers' '
>  	test_when_finished "rm -rf repo" &&
>  	git init repo &&
>  	(
>  		cd repo &&
> -		# Set a high timeout such that a busy CI machine will not abort
> -		# early. 10 seconds should hopefully be ample of time to make
> -		# this non-flaky.
> -		git config set reftable.lockTimeout 10000 &&
> +		# Set a high timeout. While a couple of seconds should be
> +		# plenty, using the address sanitizer will significantly slow
> +		# us down here. So we are aiming way higher than you would ever
> +		# think is necessary just to keep us from flaking. We could
> +		# also lock indefinitely by passing -1, but that could
> +		# potentially block CI jobs indefinitely if there was a bug
> +		# here.
> +		git config set reftable.lockTimeout 300000 &&
>  		test_commit --no-tag initial &&
>  
>  		head=$(git rev-parse HEAD) &&

> This should be revisited after Git v2.47 is out.
>
> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> ---
>  t/t0610-reftable-basics.sh | 17 ++++++++++++-----
>  1 file changed, 12 insertions(+), 5 deletions(-)

> Work around the issue by disabling the test on Windows for now. While at
> it, increase the locking timeout to address reported timeouts when using
> either the address or memory sanitizer, which also tend to significantly
> extend the runtime of this test.
> 
> This should be revisited after Git v2.47 is out.