[PATCH 0/2] compat/mingw: fix EACCESS when opening files with `O_CREAT | O_EXCL`

[PATCH 0/2] compat/mingw: fix EACCESS when opening files with `O_CREAT | O_EXCL`

[Patrick Steinhardt]

Hi,

I finally found some time to have a look at why t0610 is failing
regularly in MinGW. As it turns out the root cause is our emulation of
open(3p): when trying to open a file with `_wopen(..., O_CREAT|O_EXCL)`
the call fails in case another process has marked the same file for
deletion via `DeleteFileW()`. This gets triggered by t0610 because we
race around locking the reftable stack and thus causes the failure.

The fix is simple: we get `ERROR_ACCESS_DENIED` in this situation, so
instead of translating that error to `EACCESS` we translate it to
`EEXIST`. This fixes the flake on my machine, but as usual when it comes
to Windows I would very much like to ask those in the know to point out
any obvious mistakes I did.

The other patch is a while-at-it patch that I was wondering about while
debugging the issue. It's not needed and I'm happy to drop it if you
don't think we should include it.

Thanks!

Patrick

---
Patrick Steinhardt (2):
      compat/mingw: handle O_CLOEXEC in `mingw_open_existing()`
      compat/mingw: fix EACCESS when opening files with `O_CREAT | O_EXCL`

 compat/mingw.c | 17 +++++++++++++++--
 1 file changed, 15 insertions(+), 2 deletions(-)


---
base-commit: 4b68faf6b93311254efad80e554780e372deb42f
change-id: 20250313-b4-pks-mingw-lockfile-flake-49dfcce8e7c2

[PATCH 1/2] compat/mingw: handle O_CLOEXEC in `mingw_open_existing()`

[Patrick Steinhardt]

Our MinGW emulation of the open(3p) syscall uses one of three different
code paths depending on the flags passed by the caller. Ideally, we
would just use `_wopen()` for all of these directly and instead rely on
the Windows SDK to implement the logic for us. But unfortunately, this
interface does not allow us to set the `FILE_SHARING_*` flags, which we
need to have control over to implement POSIX semantics.

One of the code paths is for opening existing files, where we end up
calling `mingw_open_existing()`. While this code path is executed when
the user passes `O_NOINHERIT`, we don't know to handle `O_CLOEXEC` yet,
which causes a couple of code paths that use the flag to not use the
emulation. The consequence is that those code paths do not support POSIX
semantics because we don't know to set the sharing mode correctly.

Supporting `O_CLOEXEC` is quite trivial: we don't have to do anything,
as Windows already closes the file handle by default when exec'ing into
another process. This is further supported by the fact that we indeed
define `O_CLOEXEC` as `O_NOINHERIT` in case the former isn't defined in
"compat/mingw.h".

Adapt the code so that we know to handle `O_CLOEXEC` in case it has a
different definition than `O_NOINHERIT` to improve our POSIX semantics
handling.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 compat/mingw.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/compat/mingw.c b/compat/mingw.c
index f524c54d06d..101e380c5a3 100644
--- a/compat/mingw.c
+++ b/compat/mingw.c
@@ -560,7 +560,7 @@ static int mingw_open_existing(const wchar_t *filename, int oflags, ...)
 	int fd;
 
 	/* We only support basic flags. */
-	if (oflags & ~(O_ACCMODE | O_NOINHERIT)) {
+	if (oflags & ~(O_ACCMODE | O_NOINHERIT | O_CLOEXEC)) {
 		errno = ENOSYS;
 		return -1;
 	}
@@ -632,7 +632,7 @@ int mingw_open (const char *filename, int oflags, ...)
 
 	if ((oflags & O_APPEND) && !is_local_named_pipe_path(filename))
 		open_fn = mingw_open_append;
-	else if (!(oflags & ~(O_ACCMODE | O_NOINHERIT)))
+	else if (!(oflags & ~(O_ACCMODE | O_NOINHERIT | O_CLOEXEC)))
 		open_fn = mingw_open_existing;
 	else
 		open_fn = _wopen;

-- 
2.49.0.rc2.394.gf6994c5077.dirty

[PATCH 2/2] compat/mingw: fix EACCESS when opening files with `O_CREAT | O_EXCL`

[Patrick Steinhardt]

In our CI systems we can observe that t0610 fails rather frequently.
This testcase races a bunch of git-update-ref(1) processes with one
another which are all trying to update a unique reference, where we
expect that all processes succeed and end up updating the reftable
stack. The error message in this case looks like the following:

    fatal: update_ref failed for ref 'refs/heads/branch-88': reftable: transaction prepare: I/O error

Instrumenting the code with a couple of calls to `BUG()` in relevant
sites where we return `REFTABLE_IO_ERROR` quickly leads one to discover
that this error is caused when calling `flock_acquire()`, which is a
thin wrapper around our lockfile API. Curiously, the error code we get
in such cases is `EACCESS`, indicating that we are not allowed to access
the file.

The root cause of this is an oddity of `CreateFileW()`, which is what
`_wopen()` uses internally. Quoting its documentation [1]:

    If you call CreateFile on a file that is pending deletion as a
    result of a previous call to DeleteFile, the function fails. The
    operating system delays file deletion until all handles to the file
    are closed. GetLastError returns ERROR_ACCESS_DENIED.

This behaviour is triggered quite often in the above testcase because
all the processes race with one another trying to acquire the lock for
the "tables.list" file. This is due to how locking works in the reftable
library when compacting a stack:

    1. Lock the "tables.list" file and reads its contents.

    2. Decide which tables to compact.

    3. Lock each of the individual tables that we are about to compact.

    4. Unlock the "tables.list" file.

    5. Compact the individual tables into one large table.

    6. Re-lock the "tables.list" file.

    7. Write the new list of tables into it.

    8. Commit the "tables.list" file.

The important step is (4): we don't commit the file directly by renaming
it into place, but instead we delete the lockfile so that concurrent
processes can continue to append to the reftable stack while we compact
the tables. And because we use `DeleteFileW()` to do so, we may now race
with another process that wants to acquire that lockfile. So if we are
unlucky, we would now see `ERROR_ACCESS_DENIED` instead of the expected
`ERROR_FILE_EXISTS`, which the lockfile subsystem isn't prepared to
handle and thus it will bail out without retrying to acquire the lock.

In theory, the issue is not limited to the reftable library and can be
triggered by every other user of the lockfile subsystem, as well. My gut
feeling tells me it's rather unlikely to surface elsewhere though.

Fix the issue by translating the error to `EEXIST`. This makes the
lockfile subsystem handle the error correctly: in case a timeout is set
it will now retry acquiring the lockfile until the timeout has expired.

With this, t0610 is now always passing on my machine whereas it was
previously failing in around 20-30% of all test runs.

[1]: https://learn.microsoft.com/en-us/windows/win32/api/fileapi/nf-fileapi-createfilew

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 compat/mingw.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/compat/mingw.c b/compat/mingw.c
index 101e380c5a3..fb61de759c7 100644
--- a/compat/mingw.c
+++ b/compat/mingw.c
@@ -644,6 +644,19 @@ int mingw_open (const char *filename, int oflags, ...)
 
 	fd = open_fn(wfilename, oflags, mode);
 
+	/*
+	 * Internally, `_wopen()` uses the `CreateFile()` API with CREATE_NEW,
+	 * which may error out with ERROR_ACCESS_DENIED when the file is
+	 * scheduled for deletion via `DeleteFileW()`. The file essentially
+	 * exists, so we map this error to ERROR_ALREADY_EXISTS so that callers
+	 * don't have to special-case this.
+	 *
+	 * This fixes issues for example with the lockfile interface when one
+	 * process has a lock that it is about to commit or release while
+	 * another process wants to acquire it.
+	 */
+	if (fd < 0 && create && GetLastError() == ERROR_ACCESS_DENIED)
+		errno = EEXIST;
 	if (fd < 0 && (oflags & O_ACCMODE) != O_RDONLY && errno == EACCES) {
 		DWORD attrs = GetFileAttributesW(wfilename);
 		if (attrs != INVALID_FILE_ATTRIBUTES && (attrs & FILE_ATTRIBUTE_DIRECTORY))

-- 
2.49.0.rc2.394.gf6994c5077.dirty

Re: [PATCH 1/2] compat/mingw: handle O_CLOEXEC in `mingw_open_existing()`

[Junio C Hamano]

Patrick Steinhardt <ps@pks.im> writes:

> Our MinGW emulation of the open(3p) syscall uses one of three different
> code paths depending on the flags passed by the caller. Ideally, we
> would just use `_wopen()` for all of these directly and instead rely on
> the Windows SDK to implement the logic for us. But unfortunately, this
> interface does not allow us to set the `FILE_SHARING_*` flags, which we
> need to have control over to implement POSIX semantics.
>
> One of the code paths is for opening existing files, where we end up
> calling `mingw_open_existing()`. While this code path is executed when
> the user passes `O_NOINHERIT`, we don't know to handle `O_CLOEXEC` yet,
> which causes a couple of code paths that use the flag to not use the
> emulation. The consequence is that those code paths do not support POSIX
> semantics because we don't know to set the sharing mode correctly.
>
> Supporting `O_CLOEXEC` is quite trivial: we don't have to do anything,
> as Windows already closes the file handle by default when exec'ing into
> another process. This is further supported by the fact that we indeed
> define `O_CLOEXEC` as `O_NOINHERIT` in case the former isn't defined in
> "compat/mingw.h".
>
> Adapt the code so that we know to handle `O_CLOEXEC` in case it has a
> different definition than `O_NOINHERIT` to improve our POSIX semantics
> handling.

This one looks quite sensible and straight-forward even to a
non-Windows person like me.

> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> ---
>  compat/mingw.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/compat/mingw.c b/compat/mingw.c
> index f524c54d06d..101e380c5a3 100644
> --- a/compat/mingw.c
> +++ b/compat/mingw.c
> @@ -560,7 +560,7 @@ static int mingw_open_existing(const wchar_t *filename, int oflags, ...)
>  	int fd;
>  
>  	/* We only support basic flags. */
> -	if (oflags & ~(O_ACCMODE | O_NOINHERIT)) {
> +	if (oflags & ~(O_ACCMODE | O_NOINHERIT | O_CLOEXEC)) {
>  		errno = ENOSYS;
>  		return -1;
>  	}
> @@ -632,7 +632,7 @@ int mingw_open (const char *filename, int oflags, ...)
>  
>  	if ((oflags & O_APPEND) && !is_local_named_pipe_path(filename))
>  		open_fn = mingw_open_append;
> -	else if (!(oflags & ~(O_ACCMODE | O_NOINHERIT)))
> +	else if (!(oflags & ~(O_ACCMODE | O_NOINHERIT | O_CLOEXEC)))
>  		open_fn = mingw_open_existing;
>  	else
>  		open_fn = _wopen;

Re: [PATCH 2/2] compat/mingw: fix EACCESS when opening files with `O_CREAT | O_EXCL`

[Junio C Hamano]

Patrick Steinhardt <ps@pks.im> writes:

> +	/*
> +	 * Internally, `_wopen()` uses the `CreateFile()` API with CREATE_NEW,
> +	 * which may error out with ERROR_ACCESS_DENIED when the file is
> +	 * scheduled for deletion via `DeleteFileW()`. The file essentially
> +	 * exists, so we map this error to ERROR_ALREADY_EXISTS so that callers
> +	 * don't have to special-case this.
> +	 *
> +	 * This fixes issues for example with the lockfile interface when one
> +	 * process has a lock that it is about to commit or release while
> +	 * another process wants to acquire it.
> +	 */

The above may explain how the code gets ERROR_ACCESS_DENIED when
there is a pending DeleteFileW() on the file.  I however cannot
judge if the opposite is also always true, i.e. ERROR_ACCESS_DENIED
always mean the file did exist and hasn't gone away and no other
reason the code would ever get that error status.  Somebody with
better understanding on Windows API behaviour hopefully can review
it.

Thanks.

> +	if (fd < 0 && create && GetLastError() == ERROR_ACCESS_DENIED)
> +		errno = EEXIST;
>  	if (fd < 0 && (oflags & O_ACCMODE) != O_RDONLY && errno == EACCES) {
>  		DWORD attrs = GetFileAttributesW(wfilename);
>  		if (attrs != INVALID_FILE_ATTRIBUTES && (attrs & FILE_ATTRIBUTE_DIRECTORY))

Re: [PATCH 2/2] compat/mingw: fix EACCESS when opening files with `O_CREAT | O_EXCL`

[Johannes Schindelin]

Hi Patrick,

On Thu, 13 Mar 2025, Patrick Steinhardt wrote:

> In our CI systems we can observe that t0610 fails rather frequently.
> This testcase races a bunch of git-update-ref(1) processes with one
> another which are all trying to update a unique reference, where we
> expect that all processes succeed and end up updating the reftable
> stack. The error message in this case looks like the following:
>
>     fatal: update_ref failed for ref 'refs/heads/branch-88': reftable: transaction prepare: I/O error

I saw this error plenty of times and was wondering whether there would be
a way to get more useful information in the error message.

After all, I/O errors come in all shapes and forms, and telling the user
that _something_ was wrong but forcing them to recreate the issue in a GDB
session is an excellent recipe to cause frustration.

So I'd like to suggest to improve the user experience substantially by
augmenting the rather generic `I/O error` with details as to what
operation failed, with what exact error, on what file.

> Instrumenting the code with a couple of calls to `BUG()` in relevant
> sites where we return `REFTABLE_IO_ERROR` quickly leads one to discover
> that this error is caused when calling `flock_acquire()`, which is a
> thin wrapper around our lockfile API. Curiously, the error code we get
> in such cases is `EACCESS`, indicating that we are not allowed to access
> the file.
>
> The root cause of this is an oddity of `CreateFileW()`, which is what
> `_wopen()` uses internally. Quoting its documentation [1]:
>
>     If you call CreateFile on a file that is pending deletion as a
>     result of a previous call to DeleteFile, the function fails. The
>     operating system delays file deletion until all handles to the file
>     are closed. GetLastError returns ERROR_ACCESS_DENIED.
>
> This behaviour is triggered quite often in the above testcase because
> all the processes race with one another trying to acquire the lock for
> the "tables.list" file. This is due to how locking works in the reftable
> library when compacting a stack:
>
>     1. Lock the "tables.list" file and reads its contents.
>
>     2. Decide which tables to compact.
>
>     3. Lock each of the individual tables that we are about to compact.
>
>     4. Unlock the "tables.list" file.
>
>     5. Compact the individual tables into one large table.
>
>     6. Re-lock the "tables.list" file.
>
>     7. Write the new list of tables into it.
>
>     8. Commit the "tables.list" file.
>
> The important step is (4): we don't commit the file directly by renaming
> it into place, but instead we delete the lockfile so that concurrent
> processes can continue to append to the reftable stack while we compact
> the tables. And because we use `DeleteFileW()` to do so, we may now race
> with another process that wants to acquire that lockfile. So if we are
> unlucky, we would now see `ERROR_ACCESS_DENIED` instead of the expected
> `ERROR_FILE_EXISTS`, which the lockfile subsystem isn't prepared to
> handle and thus it will bail out without retrying to acquire the lock.
>
> In theory, the issue is not limited to the reftable library and can be
> triggered by every other user of the lockfile subsystem, as well. My gut
> feeling tells me it's rather unlikely to surface elsewhere though.
>
> Fix the issue by translating the error to `EEXIST`. This makes the
> lockfile subsystem handle the error correctly: in case a timeout is set
> it will now retry acquiring the lockfile until the timeout has expired.
>
> With this, t0610 is now always passing on my machine whereas it was
> previously failing in around 20-30% of all test runs.

It is good that you fixed this issue!

However, `ERROR_ACCESS_DENIED` most often means one of two things:

- The file in question exists but is opened exclusively by another process
  (which might be Defender, the anti-malware scanner), or

- The current user lacks the permission to create this particular file,
  i.e. it is really what `EACCES` would mean on Linux.

While the first condition clearly can be interpreted as "file exists" in
the way this patch wants to do, the latter cannot be. And the patch
touches a function that is exclusively used by the `lockfile` machinery,
each and every caller of `open(..., ... O_CREAT)` is affected by this
change.

This has ramifications e.g. when running in a worktree where the user has
no write permission (but which they indicated as safe via
`safe.directory`). Git would then no longer report correctly whe it cannot
write files because the user lacks permission to do that, but would
instead claim that the file already exists, when that is not true.

Maybe there is a place higher in the stack trace where Git could instead
learn to handle `EACCES`? E.g. treat it the same as `EEXIST`, or maybe
alternatively make it Windows-specific and introduce a back-off plan?

Ciao,
Johannes

>
> [1]: https://learn.microsoft.com/en-us/windows/win32/api/fileapi/nf-fileapi-createfilew
>
> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> ---
>  compat/mingw.c | 13 +++++++++++++
>  1 file changed, 13 insertions(+)
>
> diff --git a/compat/mingw.c b/compat/mingw.c
> index 101e380c5a3..fb61de759c7 100644
> --- a/compat/mingw.c
> +++ b/compat/mingw.c
> @@ -644,6 +644,19 @@ int mingw_open (const char *filename, int oflags, ...)
>
>  	fd = open_fn(wfilename, oflags, mode);
>
> +	/*
> +	 * Internally, `_wopen()` uses the `CreateFile()` API with CREATE_NEW,
> +	 * which may error out with ERROR_ACCESS_DENIED when the file is
> +	 * scheduled for deletion via `DeleteFileW()`. The file essentially
> +	 * exists, so we map this error to ERROR_ALREADY_EXISTS so that callers
> +	 * don't have to special-case this.
> +	 *
> +	 * This fixes issues for example with the lockfile interface when one
> +	 * process has a lock that it is about to commit or release while
> +	 * another process wants to acquire it.
> +	 */
> +	if (fd < 0 && create && GetLastError() == ERROR_ACCESS_DENIED)
> +		errno = EEXIST;
>  	if (fd < 0 && (oflags & O_ACCMODE) != O_RDONLY && errno == EACCES) {
>  		DWORD attrs = GetFileAttributesW(wfilename);
>  		if (attrs != INVALID_FILE_ATTRIBUTES && (attrs & FILE_ATTRIBUTE_DIRECTORY))
>
> --
> 2.49.0.rc2.394.gf6994c5077.dirty
>
>
>

Re: [PATCH 2/2] compat/mingw: fix EACCESS when opening files with `O_CREAT | O_EXCL`

[Patrick Steinhardt]

On Sun, Mar 16, 2025 at 01:01:25AM +0100, Johannes Schindelin wrote:
> On Thu, 13 Mar 2025, Patrick Steinhardt wrote:
> > In our CI systems we can observe that t0610 fails rather frequently.
> > This testcase races a bunch of git-update-ref(1) processes with one
> > another which are all trying to update a unique reference, where we
> > expect that all processes succeed and end up updating the reftable
> > stack. The error message in this case looks like the following:
> >
> >     fatal: update_ref failed for ref 'refs/heads/branch-88': reftable: transaction prepare: I/O error
> 
> I saw this error plenty of times and was wondering whether there would be
> a way to get more useful information in the error message.
> 
> After all, I/O errors come in all shapes and forms, and telling the user
> that _something_ was wrong but forcing them to recreate the issue in a GDB
> session is an excellent recipe to cause frustration.
> 
> So I'd like to suggest to improve the user experience substantially by
> augmenting the rather generic `I/O error` with details as to what
> operation failed, with what exact error, on what file.

Agreed, the error handling isn't great. The very least we should be
doing is to print `errno`, but even that I consider to be suboptimal.
Ideally we'd have structured error handling that allows us to return
richer errors to the caller, but that is a much bigger undertaking.

[snip]
> > Fix the issue by translating the error to `EEXIST`. This makes the
> > lockfile subsystem handle the error correctly: in case a timeout is set
> > it will now retry acquiring the lockfile until the timeout has expired.
> >
> > With this, t0610 is now always passing on my machine whereas it was
> > previously failing in around 20-30% of all test runs.
> 
> It is good that you fixed this issue!
> 
> However, `ERROR_ACCESS_DENIED` most often means one of two things:
> 
> - The file in question exists but is opened exclusively by another process
>   (which might be Defender, the anti-malware scanner), or
> 
> - The current user lacks the permission to create this particular file,
>   i.e. it is really what `EACCES` would mean on Linux.
> 
> While the first condition clearly can be interpreted as "file exists" in
> the way this patch wants to do, the latter cannot be. And the patch
> touches a function that is exclusively used by the `lockfile` machinery,
> each and every caller of `open(..., ... O_CREAT)` is affected by this
> change.

I feared as much. I was hoping that the second case would cause a
different error equivalent to EPERM, and the documentation didn't really
say anything about this.

> This has ramifications e.g. when running in a worktree where the user has
> no write permission (but which they indicated as safe via
> `safe.directory`). Git would then no longer report correctly whe it cannot
> write files because the user lacks permission to do that, but would
> instead claim that the file already exists, when that is not true.
> 
> Maybe there is a place higher in the stack trace where Git could instead
> learn to handle `EACCES`? E.g. treat it the same as `EEXIST`, or maybe
> alternatively make it Windows-specific and introduce a back-off plan?

The place that would need to learn about it is the lockfile subsystem.
But we basically have the same issue here that we cannot know why we got
EACCESS in the first place. So retrying may or may not be the correct
thing to do in this context, same as in `mingw_open()`.

While implementing the workaround I wondered whether we are able to get
clearer error messages if we were able to verify a few additional data
points:

  - If creating the file fails with ERROR_ACCESS_DENIED we could check
    whether the parent directory is accessible to us, and if it is then
    we can assume that the error is due to an existing file. But that
    falls apart rather quickly when thinking about edge cases, like an
    unwritable file in a writable directory.

  - We could stat the file in question to check whether it exists. But
    given that our case only happens when we have lost a race it may be
    unwise to build on top of an already-racy mechanism.

All of these feel hacky, so... I don't have a good idea for how to fix
this. It is unfortunate that `CreateFileW()` throws these two errors
into the same bag and doesn't give us any hints which of both errors has
happened.

Patrick

[PATCH v2 0/2] compat/mingw: fix EACCESS when opening files with `O_CREAT | O_EXCL`

[Patrick Steinhardt]

Hi,

I finally found some time to have a look at why t0610 is failing
regularly in MinGW. As it turns out the root cause is our emulation of
open(3p): when trying to open a file with `_wopen(..., O_CREAT|O_EXCL)`
the call fails in case another process has marked the same file for
deletion via `DeleteFileW()`. This gets triggered by t0610 because we
race around locking the reftable stack and thus causes the failure.

The fix is simple: we get `ERROR_ACCESS_DENIED` in this situation, so
instead of translating that error to `EACCESS` we translate it to
`EEXIST`. This fixes the flake on my machine, but as usual when it comes
to Windows I would very much like to ask those in the know to point out
any obvious mistakes I did.

The other patch is a while-at-it patch that I was wondering about while
debugging the issue. It's not needed and I'm happy to drop it if you
don't think we should include it.

Changes in v2:
  - Make the workaround more specific by also paying attention to the
    NtStatus code. Like this, we only translate the error when we see
    that the error code was `STATUS_DELETE_PENDING`, which should rule
    out that the translation triggers in unintended cases.
  - A new patch for Meson that makes us pull in "compat/msvc.c" instead
    of "compat/mingw.c". This is more of a while-at-it fix that I
    spotted while working on this patch series. It doesn't have any
    ramifications for what I'm doing.
  - Drop the patch that makes us handle O_CLOEXEC. It's not needed, and
    I'd rather focus on changes that actually improve the situation.
  - Link to v1: https://lore.kernel.org/r/20250313-b4-pks-mingw-lockfile-flake-v1-0-bc5d3e70f516@pks.im

Thanks!

Patrick

---
Patrick Steinhardt (2):
      meson: fix compat sources when compiling with MSVC
      compat/mingw: fix EACCESS when opening files with `O_CREAT | O_EXCL`

 compat/mingw.c | 20 ++++++++++++++++++++
 meson.build    |  4 +++-
 2 files changed, 23 insertions(+), 1 deletion(-)

Range-diff versus v1:

1:  c2c1f988729 < -:  ----------- compat/mingw: handle O_CLOEXEC in `mingw_open_existing()`
2:  fd698866034 < -:  ----------- compat/mingw: fix EACCESS when opening files with `O_CREAT | O_EXCL`
-:  ----------- > 1:  9a2798b1b63 meson: fix compat sources when compiling with MSVC
-:  ----------- > 2:  ff5bf477747 compat/mingw: fix EACCESS when opening files with `O_CREAT | O_EXCL`

---
base-commit: 4b68faf6b93311254efad80e554780e372deb42f
change-id: 20250313-b4-pks-mingw-lockfile-flake-49dfcce8e7c2

[PATCH v2 1/2] meson: fix compat sources when compiling with MSVC

[Patrick Steinhardt]

In our compat library we have both "msvc.c" and "mingw.c". The former is
mostly a thin wrapper around the latter as it directly includes it, but
it has a couple of extra headers that aren't included in "mingw.c" and
is expected to be used with the Visual Studio compiler toolchain.

While our Makefile knows to pick up the correct file depending on
whether or not the Visual Studio toolchain is used, we don't do the same
with Meson. Fix this.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 meson.build | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/meson.build b/meson.build
index efe2871c9db..2cf9da3474b 100644
--- a/meson.build
+++ b/meson.build
@@ -1107,7 +1107,6 @@ if host_machine.system() == 'cygwin'
   ]
 elif host_machine.system() == 'windows'
   libgit_sources += [
-    'compat/mingw.c',
     'compat/winansi.c',
     'compat/win32/dirent.c',
     'compat/win32/flush.c',
@@ -1134,6 +1133,9 @@ elif host_machine.system() == 'windows'
   libgit_include_directories += 'compat/win32'
   if compiler.get_id() == 'msvc'
     libgit_include_directories += 'compat/vcbuild/include'
+    libgit_sources += 'compat/msvc.c'
+  else
+    libgit_sources += 'compat/mingw.c'
   endif
 endif
 

-- 
2.49.0.472.ge94155a9ec.dirty

[PATCH v2 2/2] compat/mingw: fix EACCESS when opening files with `O_CREAT | O_EXCL`

[Patrick Steinhardt]

In our CI systems we can observe that t0610 fails rather frequently.
This testcase races a bunch of git-update-ref(1) processes with one
another which are all trying to update a unique reference, where we
expect that all processes succeed and end up updating the reftable
stack. The error message in this case looks like the following:

    fatal: update_ref failed for ref 'refs/heads/branch-88': reftable: transaction prepare: I/O error

Instrumenting the code with a couple of calls to `BUG()` in relevant
sites where we return `REFTABLE_IO_ERROR` quickly leads one to discover
that this error is caused when calling `flock_acquire()`, which is a
thin wrapper around our lockfile API. Curiously, the error code we get
in such cases is `EACCESS`, indicating that we are not allowed to access
the file.

The root cause of this is an oddity of `CreateFileW()`, which is what
`_wopen()` uses internally. Quoting its documentation [1]:

    If you call CreateFile on a file that is pending deletion as a
    result of a previous call to DeleteFile, the function fails. The
    operating system delays file deletion until all handles to the file
    are closed. GetLastError returns ERROR_ACCESS_DENIED.

This behaviour is triggered quite often in the above testcase because
all the processes race with one another trying to acquire the lock for
the "tables.list" file. This is due to how locking works in the reftable
library when compacting a stack:

    1. Lock the "tables.list" file and reads its contents.

    2. Decide which tables to compact.

    3. Lock each of the individual tables that we are about to compact.

    4. Unlock the "tables.list" file.

    5. Compact the individual tables into one large table.

    6. Re-lock the "tables.list" file.

    7. Write the new list of tables into it.

    8. Commit the "tables.list" file.

The important step is (4): we don't commit the file directly by renaming
it into place, but instead we delete the lockfile so that concurrent
processes can continue to append to the reftable stack while we compact
the tables. And because we use `DeleteFileW()` to do so, we may now race
with another process that wants to acquire that lockfile. So if we are
unlucky, we would now see `ERROR_ACCESS_DENIED` instead of the expected
`ERROR_FILE_EXISTS`, which the lockfile subsystem isn't prepared to
handle and thus it will bail out without retrying to acquire the lock.

In theory, the issue is not limited to the reftable library and can be
triggered by every other user of the lockfile subsystem, as well. My gut
feeling tells me it's rather unlikely to surface elsewhere though.

Fix the issue by translating the error to `EEXIST`. This makes the
lockfile subsystem handle the error correctly: in case a timeout is set
it will now retry acquiring the lockfile until the timeout has expired.

With this, t0610 is now always passing on my machine whereas it was
previously failing in around 20-30% of all test runs.

[1]: https://learn.microsoft.com/en-us/windows/win32/api/fileapi/nf-fileapi-createfilew

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 compat/mingw.c | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

diff --git a/compat/mingw.c b/compat/mingw.c
index f524c54d06d..50c80b1b750 100644
--- a/compat/mingw.c
+++ b/compat/mingw.c
@@ -21,6 +21,9 @@
 #include "gettext.h"
 #define SECURITY_WIN32
 #include <sspi.h>
+#include <winternl.h>
+
+#define STATUS_DELETE_PENDING ((NTSTATUS) 0xC0000056)
 
 #define HCAST(type, handle) ((type)(intptr_t)handle)
 
@@ -621,6 +624,8 @@ int mingw_open (const char *filename, int oflags, ...)
 	wchar_t wfilename[MAX_PATH];
 	open_fn_t open_fn;
 
+	DECLARE_PROC_ADDR(ntdll.dll, NTSTATUS, NTAPI, RtlGetLastNtStatus, void);
+
 	va_start(args, oflags);
 	mode = va_arg(args, int);
 	va_end(args);
@@ -644,6 +649,21 @@ int mingw_open (const char *filename, int oflags, ...)
 
 	fd = open_fn(wfilename, oflags, mode);
 
+	/*
+	 * Internally, `_wopen()` uses the `CreateFile()` API with CREATE_NEW,
+	 * which may error out with ERROR_ACCESS_DENIED and an NtStatus of
+	 * STATUS_DELETE_PENDING when the file is scheduled for deletion via
+	 * `DeleteFileW()`. The file essentially exists, so we map errno to
+	 * EEXIST instead of EACCESS so that callers don't have to special-case
+	 * this.
+	 *
+	 * This fixes issues for example with the lockfile interface when one
+	 * process has a lock that it is about to commit or release while
+	 * another process wants to acquire it.
+	 */
+	if (fd < 0 && create && GetLastError() == ERROR_ACCESS_DENIED &&
+	    INIT_PROC_ADDR(RtlGetLastNtStatus) && RtlGetLastNtStatus() == STATUS_DELETE_PENDING)
+		errno = EEXIST;
 	if (fd < 0 && (oflags & O_ACCMODE) != O_RDONLY && errno == EACCES) {
 		DWORD attrs = GetFileAttributesW(wfilename);
 		if (attrs != INVALID_FILE_ATTRIBUTES && (attrs & FILE_ATTRIBUTE_DIRECTORY))

-- 
2.49.0.472.ge94155a9ec.dirty

Re: [PATCH v2 2/2] compat/mingw: fix EACCESS when opening files with `O_CREAT | O_EXCL`

[Johannes Schindelin]

Hi Patrick,

On Thu, 20 Mar 2025, Patrick Steinhardt wrote:

> In our CI systems we can observe that t0610 fails rather frequently.
> This testcase races a bunch of git-update-ref(1) processes with one
> another which are all trying to update a unique reference, where we
> expect that all processes succeed and end up updating the reftable
> stack. The error message in this case looks like the following:
>
>     fatal: update_ref failed for ref 'refs/heads/branch-88': reftable: transaction prepare: I/O error
>
> Instrumenting the code with a couple of calls to `BUG()` in relevant
> sites where we return `REFTABLE_IO_ERROR` quickly leads one to discover
> that this error is caused when calling `flock_acquire()`, which is a
> thin wrapper around our lockfile API. Curiously, the error code we get
> in such cases is `EACCESS`, indicating that we are not allowed to access
> the file.
>
> The root cause of this is an oddity of `CreateFileW()`, which is what
> `_wopen()` uses internally. Quoting its documentation [1]:
>
>     If you call CreateFile on a file that is pending deletion as a
>     result of a previous call to DeleteFile, the function fails. The
>     operating system delays file deletion until all handles to the file
>     are closed. GetLastError returns ERROR_ACCESS_DENIED.
>
> This behaviour is triggered quite often in the above testcase because
> all the processes race with one another trying to acquire the lock for
> the "tables.list" file. This is due to how locking works in the reftable
> library when compacting a stack:
>
>     1. Lock the "tables.list" file and reads its contents.
>
>     2. Decide which tables to compact.
>
>     3. Lock each of the individual tables that we are about to compact.
>
>     4. Unlock the "tables.list" file.
>
>     5. Compact the individual tables into one large table.
>
>     6. Re-lock the "tables.list" file.
>
>     7. Write the new list of tables into it.
>
>     8. Commit the "tables.list" file.
>
> The important step is (4): we don't commit the file directly by renaming
> it into place, but instead we delete the lockfile so that concurrent
> processes can continue to append to the reftable stack while we compact
> the tables. And because we use `DeleteFileW()` to do so, we may now race
> with another process that wants to acquire that lockfile. So if we are
> unlucky, we would now see `ERROR_ACCESS_DENIED` instead of the expected
> `ERROR_FILE_EXISTS`, which the lockfile subsystem isn't prepared to
> handle and thus it will bail out without retrying to acquire the lock.
>
> In theory, the issue is not limited to the reftable library and can be
> triggered by every other user of the lockfile subsystem, as well. My gut
> feeling tells me it's rather unlikely to surface elsewhere though.
>
> Fix the issue by translating the error to `EEXIST`. This makes the
> lockfile subsystem handle the error correctly: in case a timeout is set
> it will now retry acquiring the lockfile until the timeout has expired.
>
> With this, t0610 is now always passing on my machine whereas it was
> previously failing in around 20-30% of all test runs.
>
> [1]: https://learn.microsoft.com/en-us/windows/win32/api/fileapi/nf-fileapi-createfilew

Couldn't we simply handle `EACCES` the same way as `EEXIST` in step 4?

This suggestion is different from v1, which would have affected all
callers of `mingw_open()`.

The reason I ask is that `RtlGetLastNtStatus()` is undocumented, and
should therefore not be used. I know that I will be tasked with removing
that call should it be introduced into Git's source code, and naturally
I'd like to avoid that.

I know that e.g. PostgreSQL used this undocumented function at least at
some stage, but SQLite avoided it by introducing a simple poll strategy.
We could also do that, but if there is already code in the reftable
library that skips doing things if a `.lock` file exists, then doing the
same if the `.lock` file cannot be created, too, should be a safe argument
to make.

Ciao,
Johannes

>
> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> ---
>  compat/mingw.c | 20 ++++++++++++++++++++
>  1 file changed, 20 insertions(+)
>
> diff --git a/compat/mingw.c b/compat/mingw.c
> index f524c54d06d..50c80b1b750 100644
> --- a/compat/mingw.c
> +++ b/compat/mingw.c
> @@ -21,6 +21,9 @@
>  #include "gettext.h"
>  #define SECURITY_WIN32
>  #include <sspi.h>
> +#include <winternl.h>
> +
> +#define STATUS_DELETE_PENDING ((NTSTATUS) 0xC0000056)
>
>  #define HCAST(type, handle) ((type)(intptr_t)handle)
>
> @@ -621,6 +624,8 @@ int mingw_open (const char *filename, int oflags, ...)
>  	wchar_t wfilename[MAX_PATH];
>  	open_fn_t open_fn;
>
> +	DECLARE_PROC_ADDR(ntdll.dll, NTSTATUS, NTAPI, RtlGetLastNtStatus, void);
> +
>  	va_start(args, oflags);
>  	mode = va_arg(args, int);
>  	va_end(args);
> @@ -644,6 +649,21 @@ int mingw_open (const char *filename, int oflags, ...)
>
>  	fd = open_fn(wfilename, oflags, mode);
>
> +	/*
> +	 * Internally, `_wopen()` uses the `CreateFile()` API with CREATE_NEW,
> +	 * which may error out with ERROR_ACCESS_DENIED and an NtStatus of
> +	 * STATUS_DELETE_PENDING when the file is scheduled for deletion via
> +	 * `DeleteFileW()`. The file essentially exists, so we map errno to
> +	 * EEXIST instead of EACCESS so that callers don't have to special-case
> +	 * this.
> +	 *
> +	 * This fixes issues for example with the lockfile interface when one
> +	 * process has a lock that it is about to commit or release while
> +	 * another process wants to acquire it.
> +	 */
> +	if (fd < 0 && create && GetLastError() == ERROR_ACCESS_DENIED &&
> +	    INIT_PROC_ADDR(RtlGetLastNtStatus) && RtlGetLastNtStatus() == STATUS_DELETE_PENDING)
> +		errno = EEXIST;
>  	if (fd < 0 && (oflags & O_ACCMODE) != O_RDONLY && errno == EACCES) {
>  		DWORD attrs = GetFileAttributesW(wfilename);
>  		if (attrs != INVALID_FILE_ATTRIBUTES && (attrs & FILE_ATTRIBUTE_DIRECTORY))
>
> --
> 2.49.0.472.ge94155a9ec.dirty
>
>

Re: [PATCH v2 2/2] compat/mingw: fix EACCESS when opening files with `O_CREAT | O_EXCL`

[Patrick Steinhardt]

On Wed, Mar 26, 2025 at 01:20:12PM +0100, Johannes Schindelin wrote:
> On Thu, 20 Mar 2025, Patrick Steinhardt wrote:
> > In our CI systems we can observe that t0610 fails rather frequently.
> > This testcase races a bunch of git-update-ref(1) processes with one
> > another which are all trying to update a unique reference, where we
> > expect that all processes succeed and end up updating the reftable
> > stack. The error message in this case looks like the following:
> >
> >     fatal: update_ref failed for ref 'refs/heads/branch-88': reftable: transaction prepare: I/O error
> >
> > Instrumenting the code with a couple of calls to `BUG()` in relevant
> > sites where we return `REFTABLE_IO_ERROR` quickly leads one to discover
> > that this error is caused when calling `flock_acquire()`, which is a
> > thin wrapper around our lockfile API. Curiously, the error code we get
> > in such cases is `EACCESS`, indicating that we are not allowed to access
> > the file.
> >
> > The root cause of this is an oddity of `CreateFileW()`, which is what
> > `_wopen()` uses internally. Quoting its documentation [1]:
> >
> >     If you call CreateFile on a file that is pending deletion as a
> >     result of a previous call to DeleteFile, the function fails. The
> >     operating system delays file deletion until all handles to the file
> >     are closed. GetLastError returns ERROR_ACCESS_DENIED.
> >
> > This behaviour is triggered quite often in the above testcase because
> > all the processes race with one another trying to acquire the lock for
> > the "tables.list" file. This is due to how locking works in the reftable
> > library when compacting a stack:
> >
> >     1. Lock the "tables.list" file and reads its contents.
> >
> >     2. Decide which tables to compact.
> >
> >     3. Lock each of the individual tables that we are about to compact.
> >
> >     4. Unlock the "tables.list" file.
> >
> >     5. Compact the individual tables into one large table.
> >
> >     6. Re-lock the "tables.list" file.
> >
> >     7. Write the new list of tables into it.
> >
> >     8. Commit the "tables.list" file.
> >
> > The important step is (4): we don't commit the file directly by renaming
> > it into place, but instead we delete the lockfile so that concurrent
> > processes can continue to append to the reftable stack while we compact
> > the tables. And because we use `DeleteFileW()` to do so, we may now race
> > with another process that wants to acquire that lockfile. So if we are
> > unlucky, we would now see `ERROR_ACCESS_DENIED` instead of the expected
> > `ERROR_FILE_EXISTS`, which the lockfile subsystem isn't prepared to
> > handle and thus it will bail out without retrying to acquire the lock.
> >
> > In theory, the issue is not limited to the reftable library and can be
> > triggered by every other user of the lockfile subsystem, as well. My gut
> > feeling tells me it's rather unlikely to surface elsewhere though.
> >
> > Fix the issue by translating the error to `EEXIST`. This makes the
> > lockfile subsystem handle the error correctly: in case a timeout is set
> > it will now retry acquiring the lockfile until the timeout has expired.
> >
> > With this, t0610 is now always passing on my machine whereas it was
> > previously failing in around 20-30% of all test runs.
> >
> > [1]: https://learn.microsoft.com/en-us/windows/win32/api/fileapi/nf-fileapi-createfilew
> 
> Couldn't we simply handle `EACCES` the same way as `EEXIST` in step 4?
> 
> This suggestion is different from v1, which would have affected all
> callers of `mingw_open()`.

Yeah, but it basically has the same problem: we cannot tell whether
EACCESS is caused by the race or by insufficient privileges. So the
behaviour would be more self-contained, but it would still not be
correct in the same way as it would be incorrect in `mingw_open()`. We
do want to retry locking the file in case we raced, but when EACCESS is
raised due to insufficient permissions we don't.

> The reason I ask is that `RtlGetLastNtStatus()` is undocumented, and
> should therefore not be used. I know that I will be tasked with removing
> that call should it be introduced into Git's source code, and naturally
> I'd like to avoid that.

Unfortunate, but fair enough. It's quite surprising that it is not
possible to figure out the exact status code without relying on
undocumented functions.

> I know that e.g. PostgreSQL used this undocumented function at least at
> some stage, but SQLite avoided it by introducing a simple poll strategy.
> We could also do that, but if there is already code in the reftable
> library that skips doing things if a `.lock` file exists, then doing the
> same if the `.lock` file cannot be created, too, should be a safe argument
> to make.

I did stumble over the PostgreSQL patch at one point indeed, yeah.

Thanks for the pointer to SQLite. It indeed has the following snippet:

    #define winIoerrCanRetry1(a) (((a)==ERROR_ACCESS_DENIED)        || \
                                  ((a)==ERROR_SHARING_VIOLATION)    || \
                                  ((a)==ERROR_LOCK_VIOLATION)       || \
                                  ((a)==ERROR_DEV_NOT_EXIST)        || \
                                  ((a)==ERROR_NETNAME_DELETED)      || \
                                  ((a)==ERROR_SEM_TIMEOUT)          || \
                                  ((a)==ERROR_NETWORK_UNREACHABLE))

The function gets used via `winRetryIoerr()`, which is used in various
I/O functions to retry the operation, including `winOpen()` to open or
create a file. And it indeed uses a rather simple polling system there
where it sleeps for 25ms up to 10 times.

This certainly is something we could implement in `mingw_open()`: when
we see that `CreateFileW()` has returned any of the above errors we
simply retry the operation. It wouldn't fix the race itself, but it
would hopefully make it less likely to hit. If you would be okay with
such a solution I can implement it.

Also, one thing to note: this problem isn't caused by the reftable
library, it's caused by the lockfile subsystem. So if we don't want to
do this in `mingw_open()`, any self-contained fix should go into the
lockfile system, not into the reftable library, because we may hit the
same symptoms anywhere else where we race around creation/deletion of a
lockfile. We just happen to hit this case in the reftable library
because the test is intentionally stress-testing and racing this code
path.

Patrick

Re: [PATCH v2 2/2] compat/mingw: fix EACCESS when opening files with `O_CREAT | O_EXCL`

[Johannes Schindelin]

Hi Patrick,

On Fri, 28 Mar 2025, Patrick Steinhardt wrote:

> On Wed, Mar 26, 2025 at 01:20:12PM +0100, Johannes Schindelin wrote:
> > On Thu, 20 Mar 2025, Patrick Steinhardt wrote:
> > > In our CI systems we can observe that t0610 fails rather frequently.
> > > This testcase races a bunch of git-update-ref(1) processes with one
> > > another which are all trying to update a unique reference, where we
> > > expect that all processes succeed and end up updating the reftable
> > > stack. The error message in this case looks like the following:
> > >
> > >     fatal: update_ref failed for ref 'refs/heads/branch-88': reftable: transaction prepare: I/O error
> > >
> > > Instrumenting the code with a couple of calls to `BUG()` in relevant
> > > sites where we return `REFTABLE_IO_ERROR` quickly leads one to discover
> > > that this error is caused when calling `flock_acquire()`, which is a
> > > thin wrapper around our lockfile API. Curiously, the error code we get
> > > in such cases is `EACCESS`, indicating that we are not allowed to access
> > > the file.
> > >
> > > The root cause of this is an oddity of `CreateFileW()`, which is what
> > > `_wopen()` uses internally. Quoting its documentation [1]:
> > >
> > >     If you call CreateFile on a file that is pending deletion as a
> > >     result of a previous call to DeleteFile, the function fails. The
> > >     operating system delays file deletion until all handles to the file
> > >     are closed. GetLastError returns ERROR_ACCESS_DENIED.
> > >
> > > This behaviour is triggered quite often in the above testcase because
> > > all the processes race with one another trying to acquire the lock for
> > > the "tables.list" file. This is due to how locking works in the reftable
> > > library when compacting a stack:
> > >
> > >     1. Lock the "tables.list" file and reads its contents.
> > >
> > >     2. Decide which tables to compact.
> > >
> > >     3. Lock each of the individual tables that we are about to compact.
> > >
> > >     4. Unlock the "tables.list" file.
> > >
> > >     5. Compact the individual tables into one large table.
> > >
> > >     6. Re-lock the "tables.list" file.
> > >
> > >     7. Write the new list of tables into it.
> > >
> > >     8. Commit the "tables.list" file.
> > >
> > > The important step is (4): we don't commit the file directly by renaming
> > > it into place, but instead we delete the lockfile so that concurrent
> > > processes can continue to append to the reftable stack while we compact
> > > the tables. And because we use `DeleteFileW()` to do so, we may now race
> > > with another process that wants to acquire that lockfile. So if we are
> > > unlucky, we would now see `ERROR_ACCESS_DENIED` instead of the expected
> > > `ERROR_FILE_EXISTS`, which the lockfile subsystem isn't prepared to
> > > handle and thus it will bail out without retrying to acquire the lock.
> > >
> > > In theory, the issue is not limited to the reftable library and can be
> > > triggered by every other user of the lockfile subsystem, as well. My gut
> > > feeling tells me it's rather unlikely to surface elsewhere though.
> > >
> > > Fix the issue by translating the error to `EEXIST`. This makes the
> > > lockfile subsystem handle the error correctly: in case a timeout is set
> > > it will now retry acquiring the lockfile until the timeout has expired.
> > >
> > > With this, t0610 is now always passing on my machine whereas it was
> > > previously failing in around 20-30% of all test runs.
> > >
> > > [1]: https://learn.microsoft.com/en-us/windows/win32/api/fileapi/nf-fileapi-createfilew
> >
> > Couldn't we simply handle `EACCES` the same way as `EEXIST` in step 4?
> >
> > This suggestion is different from v1, which would have affected all
> > callers of `mingw_open()`.
>
> Yeah, but it basically has the same problem: we cannot tell whether
> EACCESS is caused by the race or by insufficient privileges. So the
> behaviour would be more self-contained, but it would still not be
> correct in the same way as it would be incorrect in `mingw_open()`. We
> do want to retry locking the file in case we raced, but when EACCESS is
> raised due to insufficient permissions we don't.

My thinking was that in the call chain from reftable, it seems that the
failure to lock the file is less bad than in other cases, that's why I
asked whether that might be a layer where we could add a work-around
without resorting to undocumented functions.

> > The reason I ask is that `RtlGetLastNtStatus()` is undocumented, and
> > should therefore not be used. I know that I will be tasked with removing
> > that call should it be introduced into Git's source code, and naturally
> > I'd like to avoid that.
>
> Unfortunate, but fair enough. It's quite surprising that it is not
> possible to figure out the exact status code without relying on
> undocumented functions.

Indeed.

> > I know that e.g. PostgreSQL used this undocumented function at least at
> > some stage, but SQLite avoided it by introducing a simple poll strategy.
> > We could also do that, but if there is already code in the reftable
> > library that skips doing things if a `.lock` file exists, then doing the
> > same if the `.lock` file cannot be created, too, should be a safe argument
> > to make.
>
> I did stumble over the PostgreSQL patch at one point indeed, yeah.
>
> Thanks for the pointer to SQLite. It indeed has the following snippet:
>
>     #define winIoerrCanRetry1(a) (((a)==ERROR_ACCESS_DENIED)        || \
>                                   ((a)==ERROR_SHARING_VIOLATION)    || \
>                                   ((a)==ERROR_LOCK_VIOLATION)       || \
>                                   ((a)==ERROR_DEV_NOT_EXIST)        || \
>                                   ((a)==ERROR_NETNAME_DELETED)      || \
>                                   ((a)==ERROR_SEM_TIMEOUT)          || \
>                                   ((a)==ERROR_NETWORK_UNREACHABLE))
>
> The function gets used via `winRetryIoerr()`, which is used in various
> I/O functions to retry the operation, including `winOpen()` to open or
> create a file. And it indeed uses a rather simple polling system there
> where it sleeps for 25ms up to 10 times.
>
> This certainly is something we could implement in `mingw_open()`: when
> we see that `CreateFileW()` has returned any of the above errors we
> simply retry the operation. It wouldn't fix the race itself, but it
> would hopefully make it less likely to hit. If you would be okay with
> such a solution I can implement it.
>
> Also, one thing to note: this problem isn't caused by the reftable
> library, it's caused by the lockfile subsystem. So if we don't want to
> do this in `mingw_open()`, any self-contained fix should go into the
> lockfile system, not into the reftable library, because we may hit the
> same symptoms anywhere else where we race around creation/deletion of a
> lockfile. We just happen to hit this case in the reftable library
> because the test is intentionally stress-testing and racing this code
> path.

As I mentioned, I had hoped that we could address this at another layer.

But let's move forward with the `RtlGetLastNtStatus()` solution because,
as you correctly pointed out, it is the only solution so far that lets Git
determine precisely whether the underlying problem is a pending delete.

I had only one remaining concern: If `RtlGetLastNtStatus()` has not yet
been initialized, would we not potentially overwrite the last NTSTATUS
while initializing it? And the answer I can give to myself is: unlikely.
The `ntdll` is already loaded, so there won't be an update to the
`NTSTATUS` there, likewise the `GetProcAddress()` call won't fail and
hence also not update it.

So let's go ahead with v2!

Ciao,
Johannes

Re: [PATCH v2 2/2] compat/mingw: fix EACCESS when opening files with `O_CREAT | O_EXCL`

[Patrick Steinhardt]

On Fri, Mar 28, 2025 at 04:41:05PM +0100, Johannes Schindelin wrote:
> On Fri, 28 Mar 2025, Patrick Steinhardt wrote:
> > On Wed, Mar 26, 2025 at 01:20:12PM +0100, Johannes Schindelin wrote:
> > > I know that e.g. PostgreSQL used this undocumented function at least at
> > > some stage, but SQLite avoided it by introducing a simple poll strategy.
> > > We could also do that, but if there is already code in the reftable
> > > library that skips doing things if a `.lock` file exists, then doing the
> > > same if the `.lock` file cannot be created, too, should be a safe argument
> > > to make.
> >
> > I did stumble over the PostgreSQL patch at one point indeed, yeah.
> >
> > Thanks for the pointer to SQLite. It indeed has the following snippet:
> >
> >     #define winIoerrCanRetry1(a) (((a)==ERROR_ACCESS_DENIED)        || \
> >                                   ((a)==ERROR_SHARING_VIOLATION)    || \
> >                                   ((a)==ERROR_LOCK_VIOLATION)       || \
> >                                   ((a)==ERROR_DEV_NOT_EXIST)        || \
> >                                   ((a)==ERROR_NETNAME_DELETED)      || \
> >                                   ((a)==ERROR_SEM_TIMEOUT)          || \
> >                                   ((a)==ERROR_NETWORK_UNREACHABLE))
> >
> > The function gets used via `winRetryIoerr()`, which is used in various
> > I/O functions to retry the operation, including `winOpen()` to open or
> > create a file. And it indeed uses a rather simple polling system there
> > where it sleeps for 25ms up to 10 times.
> >
> > This certainly is something we could implement in `mingw_open()`: when
> > we see that `CreateFileW()` has returned any of the above errors we
> > simply retry the operation. It wouldn't fix the race itself, but it
> > would hopefully make it less likely to hit. If you would be okay with
> > such a solution I can implement it.
> >
> > Also, one thing to note: this problem isn't caused by the reftable
> > library, it's caused by the lockfile subsystem. So if we don't want to
> > do this in `mingw_open()`, any self-contained fix should go into the
> > lockfile system, not into the reftable library, because we may hit the
> > same symptoms anywhere else where we race around creation/deletion of a
> > lockfile. We just happen to hit this case in the reftable library
> > because the test is intentionally stress-testing and racing this code
> > path.
> 
> As I mentioned, I had hoped that we could address this at another layer.
> 
> But let's move forward with the `RtlGetLastNtStatus()` solution because,
> as you correctly pointed out, it is the only solution so far that lets Git
> determine precisely whether the underlying problem is a pending delete.
> 
> I had only one remaining concern: If `RtlGetLastNtStatus()` has not yet
> been initialized, would we not potentially overwrite the last NTSTATUS
> while initializing it? And the answer I can give to myself is: unlikely.
> The `ntdll` is already loaded, so there won't be an update to the
> `NTSTATUS` there, likewise the `GetProcAddress()` call won't fail and
> hence also not update it.
> 
> So let's go ahead with v2!

Great, thanks a lot for your expertise and guidance!

Patrick