{"thread":{"id":"63109","subject":"[PATCH 0/2] compat/mingw: fix EACCESS when opening files with `O_CREAT | O_EXCL`","startedAt":"2025-03-13T14:17:57Z","lastAt":"2025-03-31T06:57:38Z","messageCount":14,"participants":["Patrick Steinhardt","Junio C Hamano","Johannes Schindelin"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"514184","messageId":"20250313-b4-pks-mingw-lockfile-flake-v1-0-bc5d3e70f516@pks.im","threadId":"63109","inReplyTo":null,"subject":"[PATCH 0/2] compat/mingw: fix EACCESS when opening files with `O_CREAT | O_EXCL`","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-03-13T14:17:42Z","receivedAt":"2025-03-13T14:17:57Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"Hi,\n\nI finally found some time to have a look at why t0610 is failing\nregularly in MinGW. As it turns out the root cause is our emulation of\nopen(3p): when trying to open a file with `_wopen(..., O_CREAT|O_EXCL)`\nthe call fails in case another process has marked the same file for\ndeletion via `DeleteFileW()`. This gets triggered by t0610 because we\nrace around locking the reftable stack and thus causes the failure.\n\nThe fix is simple: we get `ERROR_ACCESS_DENIED` in this situation, so\ninstead of translating that error to `EACCESS` we translate it to\n`EEXIST`. This fixes the flake on my machine, but as usual when it comes\nto Windows I would very much like to ask those in the know to point out\nany obvious mistakes I did.\n\nThe other patch is a while-at-it patch that I was wondering about while\ndebugging the issue. It's not needed and I'm happy to drop it if you\ndon't think we should include it.\n\nThanks!\n\nPatrick\n\n---\nPatrick Steinhardt (2):\n      compat/mingw: handle O_CLOEXEC in `mingw_open_existing()`\n      compat/mingw: fix EACCESS when opening files with `O_CREAT | O_EXCL`\n\n compat/mingw.c | 17 +++++++++++++++--\n 1 file changed, 15 insertions(+), 2 deletions(-)\n\n\n---\nbase-commit: 4b68faf6b93311254efad80e554780e372deb42f\nchange-id: 20250313-b4-pks-mingw-lockfile-flake-49dfcce8e7c2\n\n"},{"id":"514185","messageId":"20250313-b4-pks-mingw-lockfile-flake-v1-2-bc5d3e70f516@pks.im","threadId":"63109","inReplyTo":"20250313-b4-pks-mingw-lockfile-flake-v1-0-bc5d3e70f516@pks.im","subject":"[PATCH 2/2] compat/mingw: fix EACCESS when opening files with `O_CREAT | O_EXCL`","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-03-13T14:17:44Z","receivedAt":"2025-03-13T14:17:59Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"In our CI systems we can observe that t0610 fails rather frequently.\nThis testcase races a bunch of git-update-ref(1) processes with one\nanother which are all trying to update a unique reference, where we\nexpect that all processes succeed and end up updating the reftable\nstack. The error message in this case looks like the following:\n\n    fatal: update_ref failed for ref 'refs/heads/branch-88': reftable: transaction prepare: I/O error\n\nInstrumenting the code with a couple of calls to `BUG()` in relevant\nsites where we return `REFTABLE_IO_ERROR` quickly leads one to discover\nthat this error is caused when calling `flock_acquire()`, which is a\nthin wrapper around our lockfile API. Curiously, the error code we get\nin such cases is `EACCESS`, indicating that we are not allowed to access\nthe file.\n\nThe root cause of this is an oddity of `CreateFileW()`, which is what\n`_wopen()` uses internally. Quoting its documentation [1]:\n\n    If you call CreateFile on a file that is pending deletion as a\n    result of a previous call to DeleteFile, the function fails. The\n    operating system delays file deletion until all handles to the file\n    are closed. GetLastError returns ERROR_ACCESS_DENIED.\n\nThis behaviour is triggered quite often in the above testcase because\nall the processes race with one another trying to acquire the lock for\nthe \"tables.list\" file. This is due to how locking works in the reftable\nlibrary when compacting a stack:\n\n    1. Lock the \"tables.list\" file and reads its contents.\n\n    2. Decide which tables to compact.\n\n    3. Lock each of the individual tables that we are about to compact.\n\n    4. Unlock the \"tables.list\" file.\n\n    5. Compact the individual tables into one large table.\n\n    6. Re-lock the \"tables.list\" file.\n\n    7. Write the new list of tables into it.\n\n    8. Commit the \"tables.list\" file.\n\nThe important step is (4): we don't commit the file directly by renaming\nit into place, but instead we delete the lockfile so that concurrent\nprocesses can continue to append to the reftable stack while we compact\nthe tables. And because we use `DeleteFileW()` to do so, we may now race\nwith another process that wants to acquire that lockfile. So if we are\nunlucky, we would now see `ERROR_ACCESS_DENIED` instead of the expected\n`ERROR_FILE_EXISTS`, which the lockfile subsystem isn't prepared to\nhandle and thus it will bail out without retrying to acquire the lock.\n\nIn theory, the issue is not limited to the reftable library and can be\ntriggered by every other user of the lockfile subsystem, as well. My gut\nfeeling tells me it's rather unlikely to surface elsewhere though.\n\nFix the issue by translating the error to `EEXIST`. This makes the\nlockfile subsystem handle the error correctly: in case a timeout is set\nit will now retry acquiring the lockfile until the timeout has expired.\n\nWith this, t0610 is now always passing on my machine whereas it was\npreviously failing in around 20-30% of all test runs.\n\n[1]: https://learn.microsoft.com/en-us/windows/win32/api/fileapi/nf-fileapi-createfilew\n\nSigned-off-by: Patrick Steinhardt <ps@pks.im>\n---\n compat/mingw.c | 13 +++++++++++++\n 1 file changed, 13 insertions(+)\n\ndiff --git a/compat/mingw.c b/compat/mingw.c\nindex 101e380c5a3..fb61de759c7 100644\n--- a/compat/mingw.c\n+++ b/compat/mingw.c\n@@ -644,6 +644,19 @@ int mingw_open (const char *filename, int oflags, ...)\n \n \tfd = open_fn(wfilename, oflags, mode);\n \n+\t/*\n+\t * Internally, `_wopen()` uses the `CreateFile()` API with CREATE_NEW,\n+\t * which may error out with ERROR_ACCESS_DENIED when the file is\n+\t * scheduled for deletion via `DeleteFileW()`. The file essentially\n+\t * exists, so we map this error to ERROR_ALREADY_EXISTS so that callers\n+\t * don't have to special-case this.\n+\t *\n+\t * This fixes issues for example with the lockfile interface when one\n+\t * process has a lock that it is about to commit or release while\n+\t * another process wants to acquire it.\n+\t */\n+\tif (fd < 0 && create && GetLastError() == ERROR_ACCESS_DENIED)\n+\t\terrno = EEXIST;\n \tif (fd < 0 && (oflags & O_ACCMODE) != O_RDONLY && errno == EACCES) {\n \t\tDWORD attrs = GetFileAttributesW(wfilename);\n \t\tif (attrs != INVALID_FILE_ATTRIBUTES && (attrs & FILE_ATTRIBUTE_DIRECTORY))\n\n-- \n2.49.0.rc2.394.gf6994c5077.dirty\n\n"},{"id":"514186","messageId":"20250313-b4-pks-mingw-lockfile-flake-v1-1-bc5d3e70f516@pks.im","threadId":"63109","inReplyTo":"20250313-b4-pks-mingw-lockfile-flake-v1-0-bc5d3e70f516@pks.im","subject":"[PATCH 1/2] compat/mingw: handle O_CLOEXEC in `mingw_open_existing()`","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-03-13T14:17:43Z","receivedAt":"2025-03-13T14:17:59Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"Our MinGW emulation of the open(3p) syscall uses one of three different\ncode paths depending on the flags passed by the caller. Ideally, we\nwould just use `_wopen()` for all of these directly and instead rely on\nthe Windows SDK to implement the logic for us. But unfortunately, this\ninterface does not allow us to set the `FILE_SHARING_*` flags, which we\nneed to have control over to implement POSIX semantics.\n\nOne of the code paths is for opening existing files, where we end up\ncalling `mingw_open_existing()`. While this code path is executed when\nthe user passes `O_NOINHERIT`, we don't know to handle `O_CLOEXEC` yet,\nwhich causes a couple of code paths that use the flag to not use the\nemulation. The consequence is that those code paths do not support POSIX\nsemantics because we don't know to set the sharing mode correctly.\n\nSupporting `O_CLOEXEC` is quite trivial: we don't have to do anything,\nas Windows already closes the file handle by default when exec'ing into\nanother process. This is further supported by the fact that we indeed\ndefine `O_CLOEXEC` as `O_NOINHERIT` in case the former isn't defined in\n\"compat/mingw.h\".\n\nAdapt the code so that we know to handle `O_CLOEXEC` in case it has a\ndifferent definition than `O_NOINHERIT` to improve our POSIX semantics\nhandling.\n\nSigned-off-by: Patrick Steinhardt <ps@pks.im>\n---\n compat/mingw.c | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/compat/mingw.c b/compat/mingw.c\nindex f524c54d06d..101e380c5a3 100644\n--- a/compat/mingw.c\n+++ b/compat/mingw.c\n@@ -560,7 +560,7 @@ static int mingw_open_existing(const wchar_t *filename, int oflags, ...)\n \tint fd;\n \n \t/* We only support basic flags. */\n-\tif (oflags & ~(O_ACCMODE | O_NOINHERIT)) {\n+\tif (oflags & ~(O_ACCMODE | O_NOINHERIT | O_CLOEXEC)) {\n \t\terrno = ENOSYS;\n \t\treturn -1;\n \t}\n@@ -632,7 +632,7 @@ int mingw_open (const char *filename, int oflags, ...)\n \n \tif ((oflags & O_APPEND) && !is_local_named_pipe_path(filename))\n \t\topen_fn = mingw_open_append;\n-\telse if (!(oflags & ~(O_ACCMODE | O_NOINHERIT)))\n+\telse if (!(oflags & ~(O_ACCMODE | O_NOINHERIT | O_CLOEXEC)))\n \t\topen_fn = mingw_open_existing;\n \telse\n \t\topen_fn = _wopen;\n\n-- \n2.49.0.rc2.394.gf6994c5077.dirty\n\n"},{"id":"514219","messageId":"xmqqo6y4om26.fsf@gitster.g","threadId":"63109","inReplyTo":"20250313-b4-pks-mingw-lockfile-flake-v1-1-bc5d3e70f516@pks.im","subject":"Re: [PATCH 1/2] compat/mingw: handle O_CLOEXEC in `mingw_open_existing()`","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-03-13T17:52:17Z","receivedAt":"2025-03-13T17:53:25Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Patrick Steinhardt <ps@pks.im> writes:\n\n> Our MinGW emulation of the open(3p) syscall uses one of three different\n> code paths depending on the flags passed by the caller. Ideally, we\n> would just use `_wopen()` for all of these directly and instead rely on\n> the Windows SDK to implement the logic for us. But unfortunately, this\n> interface does not allow us to set the `FILE_SHARING_*` flags, which we\n> need to have control over to implement POSIX semantics.\n>\n> One of the code paths is for opening existing files, where we end up\n> calling `mingw_open_existing()`. While this code path is executed when\n> the user passes `O_NOINHERIT`, we don't know to handle `O_CLOEXEC` yet,\n> which causes a couple of code paths that use the flag to not use the\n> emulation. The consequence is that those code paths do not support POSIX\n> semantics because we don't know to set the sharing mode correctly.\n>\n> Supporting `O_CLOEXEC` is quite trivial: we don't have to do anything,\n> as Windows already closes the file handle by default when exec'ing into\n> another process. This is further supported by the fact that we indeed\n> define `O_CLOEXEC` as `O_NOINHERIT` in case the former isn't defined in\n> \"compat/mingw.h\".\n>\n> Adapt the code so that we know to handle `O_CLOEXEC` in case it has a\n> different definition than `O_NOINHERIT` to improve our POSIX semantics\n> handling.\n\nThis one looks quite sensible and straight-forward even to a\nnon-Windows person like me.\n\n> Signed-off-by: Patrick Steinhardt <ps@pks.im>\n> ---\n>  compat/mingw.c | 4 ++--\n>  1 file changed, 2 insertions(+), 2 deletions(-)\n>\n> diff --git a/compat/mingw.c b/compat/mingw.c\n> index f524c54d06d..101e380c5a3 100644\n> --- a/compat/mingw.c\n> +++ b/compat/mingw.c\n> @@ -560,7 +560,7 @@ static int mingw_open_existing(const wchar_t *filename, int oflags, ...)\n>  \tint fd;\n>  \n>  \t/* We only support basic flags. */\n> -\tif (oflags & ~(O_ACCMODE | O_NOINHERIT)) {\n> +\tif (oflags & ~(O_ACCMODE | O_NOINHERIT | O_CLOEXEC)) {\n>  \t\terrno = ENOSYS;\n>  \t\treturn -1;\n>  \t}\n> @@ -632,7 +632,7 @@ int mingw_open (const char *filename, int oflags, ...)\n>  \n>  \tif ((oflags & O_APPEND) && !is_local_named_pipe_path(filename))\n>  \t\topen_fn = mingw_open_append;\n> -\telse if (!(oflags & ~(O_ACCMODE | O_NOINHERIT)))\n> +\telse if (!(oflags & ~(O_ACCMODE | O_NOINHERIT | O_CLOEXEC)))\n>  \t\topen_fn = mingw_open_existing;\n>  \telse\n>  \t\topen_fn = _wopen;\n"},{"id":"514220","messageId":"xmqqjz8solkg.fsf@gitster.g","threadId":"63109","inReplyTo":"20250313-b4-pks-mingw-lockfile-flake-v1-2-bc5d3e70f516@pks.im","subject":"Re: [PATCH 2/2] compat/mingw: fix EACCESS when opening files with `O_CREAT | O_EXCL`","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-03-13T18:02:55Z","receivedAt":"2025-03-13T18:02:57Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Patrick Steinhardt <ps@pks.im> writes:\n\n> +\t/*\n> +\t * Internally, `_wopen()` uses the `CreateFile()` API with CREATE_NEW,\n> +\t * which may error out with ERROR_ACCESS_DENIED when the file is\n> +\t * scheduled for deletion via `DeleteFileW()`. The file essentially\n> +\t * exists, so we map this error to ERROR_ALREADY_EXISTS so that callers\n> +\t * don't have to special-case this.\n> +\t *\n> +\t * This fixes issues for example with the lockfile interface when one\n> +\t * process has a lock that it is about to commit or release while\n> +\t * another process wants to acquire it.\n> +\t */\n\nThe above may explain how the code gets ERROR_ACCESS_DENIED when\nthere is a pending DeleteFileW() on the file.  I however cannot\njudge if the opposite is also always true, i.e. ERROR_ACCESS_DENIED\nalways mean the file did exist and hasn't gone away and no other\nreason the code would ever get that error status.  Somebody with\nbetter understanding on Windows API behaviour hopefully can review\nit.\n\nThanks.\n\n> +\tif (fd < 0 && create && GetLastError() == ERROR_ACCESS_DENIED)\n> +\t\terrno = EEXIST;\n>  \tif (fd < 0 && (oflags & O_ACCMODE) != O_RDONLY && errno == EACCES) {\n>  \t\tDWORD attrs = GetFileAttributesW(wfilename);\n>  \t\tif (attrs != INVALID_FILE_ATTRIBUTES && (attrs & FILE_ATTRIBUTE_DIRECTORY))\n"},{"id":"514366","messageId":"239e4222-cdce-90b6-2a8b-ee48c3e024eb@gmx.de","threadId":"63109","inReplyTo":"20250313-b4-pks-mingw-lockfile-flake-v1-2-bc5d3e70f516@pks.im","subject":"Re: [PATCH 2/2] compat/mingw: fix EACCESS when opening files with `O_CREAT | O_EXCL`","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2025-03-16T00:01:25Z","receivedAt":"2025-03-16T00:01:27Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Patrick,\n\nOn Thu, 13 Mar 2025, Patrick Steinhardt wrote:\n\n> In our CI systems we can observe that t0610 fails rather frequently.\n> This testcase races a bunch of git-update-ref(1) processes with one\n> another which are all trying to update a unique reference, where we\n> expect that all processes succeed and end up updating the reftable\n> stack. The error message in this case looks like the following:\n>\n>     fatal: update_ref failed for ref 'refs/heads/branch-88': reftable: transaction prepare: I/O error\n\nI saw this error plenty of times and was wondering whether there would be\na way to get more useful information in the error message.\n\nAfter all, I/O errors come in all shapes and forms, and telling the user\nthat _something_ was wrong but forcing them to recreate the issue in a GDB\nsession is an excellent recipe to cause frustration.\n\nSo I'd like to suggest to improve the user experience substantially by\naugmenting the rather generic `I/O error` with details as to what\noperation failed, with what exact error, on what file.\n\n> Instrumenting the code with a couple of calls to `BUG()` in relevant\n> sites where we return `REFTABLE_IO_ERROR` quickly leads one to discover\n> that this error is caused when calling `flock_acquire()`, which is a\n> thin wrapper around our lockfile API. Curiously, the error code we get\n> in such cases is `EACCESS`, indicating that we are not allowed to access\n> the file.\n>\n> The root cause of this is an oddity of `CreateFileW()`, which is what\n> `_wopen()` uses internally. Quoting its documentation [1]:\n>\n>     If you call CreateFile on a file that is pending deletion as a\n>     result of a previous call to DeleteFile, the function fails. The\n>     operating system delays file deletion until all handles to the file\n>     are closed. GetLastError returns ERROR_ACCESS_DENIED.\n>\n> This behaviour is triggered quite often in the above testcase because\n> all the processes race with one another trying to acquire the lock for\n> the \"tables.list\" file. This is due to how locking works in the reftable\n> library when compacting a stack:\n>\n>     1. Lock the \"tables.list\" file and reads its contents.\n>\n>     2. Decide which tables to compact.\n>\n>     3. Lock each of the individual tables that we are about to compact.\n>\n>     4. Unlock the \"tables.list\" file.\n>\n>     5. Compact the individual tables into one large table.\n>\n>     6. Re-lock the \"tables.list\" file.\n>\n>     7. Write the new list of tables into it.\n>\n>     8. Commit the \"tables.list\" file.\n>\n> The important step is (4): we don't commit the file directly by renaming\n> it into place, but instead we delete the lockfile so that concurrent\n> processes can continue to append to the reftable stack while we compact\n> the tables. And because we use `DeleteFileW()` to do so, we may now race\n> with another process that wants to acquire that lockfile. So if we are\n> unlucky, we would now see `ERROR_ACCESS_DENIED` instead of the expected\n> `ERROR_FILE_EXISTS`, which the lockfile subsystem isn't prepared to\n> handle and thus it will bail out without retrying to acquire the lock.\n>\n> In theory, the issue is not limited to the reftable library and can be\n> triggered by every other user of the lockfile subsystem, as well. My gut\n> feeling tells me it's rather unlikely to surface elsewhere though.\n>\n> Fix the issue by translating the error to `EEXIST`. This makes the\n> lockfile subsystem handle the error correctly: in case a timeout is set\n> it will now retry acquiring the lockfile until the timeout has expired.\n>\n> With this, t0610 is now always passing on my machine whereas it was\n> previously failing in around 20-30% of all test runs.\n\nIt is good that you fixed this issue!\n\nHowever, `ERROR_ACCESS_DENIED` most often means one of two things:\n\n- The file in question exists but is opened exclusively by another process\n  (which might be Defender, the anti-malware scanner), or\n\n- The current user lacks the permission to create this particular file,\n  i.e. it is really what `EACCES` would mean on Linux.\n\nWhile the first condition clearly can be interpreted as \"file exists\" in\nthe way this patch wants to do, the latter cannot be. And the patch\ntouches a function that is exclusively used by the `lockfile` machinery,\neach and every caller of `open(..., ... O_CREAT)` is affected by this\nchange.\n\nThis has ramifications e.g. when running in a worktree where the user has\nno write permission (but which they indicated as safe via\n`safe.directory`). Git would then no longer report correctly whe it cannot\nwrite files because the user lacks permission to do that, but would\ninstead claim that the file already exists, when that is not true.\n\nMaybe there is a place higher in the stack trace where Git could instead\nlearn to handle `EACCES`? E.g. treat it the same as `EEXIST`, or maybe\nalternatively make it Windows-specific and introduce a back-off plan?\n\nCiao,\nJohannes\n\n>\n> [1]: https://learn.microsoft.com/en-us/windows/win32/api/fileapi/nf-fileapi-createfilew\n>\n> Signed-off-by: Patrick Steinhardt <ps@pks.im>\n> ---\n>  compat/mingw.c | 13 +++++++++++++\n>  1 file changed, 13 insertions(+)\n>\n> diff --git a/compat/mingw.c b/compat/mingw.c\n> index 101e380c5a3..fb61de759c7 100644\n> --- a/compat/mingw.c\n> +++ b/compat/mingw.c\n> @@ -644,6 +644,19 @@ int mingw_open (const char *filename, int oflags, ...)\n>\n>  \tfd = open_fn(wfilename, oflags, mode);\n>\n> +\t/*\n> +\t * Internally, `_wopen()` uses the `CreateFile()` API with CREATE_NEW,\n> +\t * which may error out with ERROR_ACCESS_DENIED when the file is\n> +\t * scheduled for deletion via `DeleteFileW()`. The file essentially\n> +\t * exists, so we map this error to ERROR_ALREADY_EXISTS so that callers\n> +\t * don't have to special-case this.\n> +\t *\n> +\t * This fixes issues for example with the lockfile interface when one\n> +\t * process has a lock that it is about to commit or release while\n> +\t * another process wants to acquire it.\n> +\t */\n> +\tif (fd < 0 && create && GetLastError() == ERROR_ACCESS_DENIED)\n> +\t\terrno = EEXIST;\n>  \tif (fd < 0 && (oflags & O_ACCMODE) != O_RDONLY && errno == EACCES) {\n>  \t\tDWORD attrs = GetFileAttributesW(wfilename);\n>  \t\tif (attrs != INVALID_FILE_ATTRIBUTES && (attrs & FILE_ATTRIBUTE_DIRECTORY))\n>\n> --\n> 2.49.0.rc2.394.gf6994c5077.dirty\n>\n>\n>\n"},{"id":"514409","messageId":"Z9g85X8sNqFOwRaU@pks.im","threadId":"63109","inReplyTo":"239e4222-cdce-90b6-2a8b-ee48c3e024eb@gmx.de","subject":"Re: [PATCH 2/2] compat/mingw: fix EACCESS when opening files with `O_CREAT | O_EXCL`","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-03-17T15:16:53Z","receivedAt":"2025-03-17T15:17:03Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Sun, Mar 16, 2025 at 01:01:25AM +0100, Johannes Schindelin wrote:\n> On Thu, 13 Mar 2025, Patrick Steinhardt wrote:\n> > In our CI systems we can observe that t0610 fails rather frequently.\n> > This testcase races a bunch of git-update-ref(1) processes with one\n> > another which are all trying to update a unique reference, where we\n> > expect that all processes succeed and end up updating the reftable\n> > stack. The error message in this case looks like the following:\n> >\n> >     fatal: update_ref failed for ref 'refs/heads/branch-88': reftable: transaction prepare: I/O error\n> \n> I saw this error plenty of times and was wondering whether there would be\n> a way to get more useful information in the error message.\n> \n> After all, I/O errors come in all shapes and forms, and telling the user\n> that _something_ was wrong but forcing them to recreate the issue in a GDB\n> session is an excellent recipe to cause frustration.\n> \n> So I'd like to suggest to improve the user experience substantially by\n> augmenting the rather generic `I/O error` with details as to what\n> operation failed, with what exact error, on what file.\n\nAgreed, the error handling isn't great. The very least we should be\ndoing is to print `errno`, but even that I consider to be suboptimal.\nIdeally we'd have structured error handling that allows us to return\nricher errors to the caller, but that is a much bigger undertaking.\n\n[snip]\n> > Fix the issue by translating the error to `EEXIST`. This makes the\n> > lockfile subsystem handle the error correctly: in case a timeout is set\n> > it will now retry acquiring the lockfile until the timeout has expired.\n> >\n> > With this, t0610 is now always passing on my machine whereas it was\n> > previously failing in around 20-30% of all test runs.\n> \n> It is good that you fixed this issue!\n> \n> However, `ERROR_ACCESS_DENIED` most often means one of two things:\n> \n> - The file in question exists but is opened exclusively by another process\n>   (which might be Defender, the anti-malware scanner), or\n> \n> - The current user lacks the permission to create this particular file,\n>   i.e. it is really what `EACCES` would mean on Linux.\n> \n> While the first condition clearly can be interpreted as \"file exists\" in\n> the way this patch wants to do, the latter cannot be. And the patch\n> touches a function that is exclusively used by the `lockfile` machinery,\n> each and every caller of `open(..., ... O_CREAT)` is affected by this\n> change.\n\nI feared as much. I was hoping that the second case would cause a\ndifferent error equivalent to EPERM, and the documentation didn't really\nsay anything about this.\n\n> This has ramifications e.g. when running in a worktree where the user has\n> no write permission (but which they indicated as safe via\n> `safe.directory`). Git would then no longer report correctly whe it cannot\n> write files because the user lacks permission to do that, but would\n> instead claim that the file already exists, when that is not true.\n> \n> Maybe there is a place higher in the stack trace where Git could instead\n> learn to handle `EACCES`? E.g. treat it the same as `EEXIST`, or maybe\n> alternatively make it Windows-specific and introduce a back-off plan?\n\nThe place that would need to learn about it is the lockfile subsystem.\nBut we basically have the same issue here that we cannot know why we got\nEACCESS in the first place. So retrying may or may not be the correct\nthing to do in this context, same as in `mingw_open()`.\n\nWhile implementing the workaround I wondered whether we are able to get\nclearer error messages if we were able to verify a few additional data\npoints:\n\n  - If creating the file fails with ERROR_ACCESS_DENIED we could check\n    whether the parent directory is accessible to us, and if it is then\n    we can assume that the error is due to an existing file. But that\n    falls apart rather quickly when thinking about edge cases, like an\n    unwritable file in a writable directory.\n\n  - We could stat the file in question to check whether it exists. But\n    given that our case only happens when we have lost a race it may be\n    unwise to build on top of an already-racy mechanism.\n\nAll of these feel hacky, so... I don't have a good idea for how to fix\nthis. It is unfortunate that `CreateFileW()` throws these two errors\ninto the same bag and doesn't give us any hints which of both errors has\nhappened.\n\nPatrick\n"},{"id":"514754","messageId":"20250320-b4-pks-mingw-lockfile-flake-v2-0-a84c90cfc6c2@pks.im","threadId":"63109","inReplyTo":"20250313-b4-pks-mingw-lockfile-flake-v1-0-bc5d3e70f516@pks.im","subject":"[PATCH v2 0/2] compat/mingw: fix EACCESS when opening files with `O_CREAT | O_EXCL`","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-03-20T10:37:45Z","receivedAt":"2025-03-20T10:37:51Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"Hi,\n\nI finally found some time to have a look at why t0610 is failing\nregularly in MinGW. As it turns out the root cause is our emulation of\nopen(3p): when trying to open a file with `_wopen(..., O_CREAT|O_EXCL)`\nthe call fails in case another process has marked the same file for\ndeletion via `DeleteFileW()`. This gets triggered by t0610 because we\nrace around locking the reftable stack and thus causes the failure.\n\nThe fix is simple: we get `ERROR_ACCESS_DENIED` in this situation, so\ninstead of translating that error to `EACCESS` we translate it to\n`EEXIST`. This fixes the flake on my machine, but as usual when it comes\nto Windows I would very much like to ask those in the know to point out\nany obvious mistakes I did.\n\nThe other patch is a while-at-it patch that I was wondering about while\ndebugging the issue. It's not needed and I'm happy to drop it if you\ndon't think we should include it.\n\nChanges in v2:\n  - Make the workaround more specific by also paying attention to the\n    NtStatus code. Like this, we only translate the error when we see\n    that the error code was `STATUS_DELETE_PENDING`, which should rule\n    out that the translation triggers in unintended cases.\n  - A new patch for Meson that makes us pull in \"compat/msvc.c\" instead\n    of \"compat/mingw.c\". This is more of a while-at-it fix that I\n    spotted while working on this patch series. It doesn't have any\n    ramifications for what I'm doing.\n  - Drop the patch that makes us handle O_CLOEXEC. It's not needed, and\n    I'd rather focus on changes that actually improve the situation.\n  - Link to v1: https://lore.kernel.org/r/20250313-b4-pks-mingw-lockfile-flake-v1-0-bc5d3e70f516@pks.im\n\nThanks!\n\nPatrick\n\n---\nPatrick Steinhardt (2):\n      meson: fix compat sources when compiling with MSVC\n      compat/mingw: fix EACCESS when opening files with `O_CREAT | O_EXCL`\n\n compat/mingw.c | 20 ++++++++++++++++++++\n meson.build    |  4 +++-\n 2 files changed, 23 insertions(+), 1 deletion(-)\n\nRange-diff versus v1:\n\n1:  c2c1f988729 < -:  ----------- compat/mingw: handle O_CLOEXEC in `mingw_open_existing()`\n2:  fd698866034 < -:  ----------- compat/mingw: fix EACCESS when opening files with `O_CREAT | O_EXCL`\n-:  ----------- > 1:  9a2798b1b63 meson: fix compat sources when compiling with MSVC\n-:  ----------- > 2:  ff5bf477747 compat/mingw: fix EACCESS when opening files with `O_CREAT | O_EXCL`\n\n---\nbase-commit: 4b68faf6b93311254efad80e554780e372deb42f\nchange-id: 20250313-b4-pks-mingw-lockfile-flake-49dfcce8e7c2\n\n"},{"id":"514755","messageId":"20250320-b4-pks-mingw-lockfile-flake-v2-1-a84c90cfc6c2@pks.im","threadId":"63109","inReplyTo":"20250320-b4-pks-mingw-lockfile-flake-v2-0-a84c90cfc6c2@pks.im","subject":"[PATCH v2 1/2] meson: fix compat sources when compiling with MSVC","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-03-20T10:37:46Z","receivedAt":"2025-03-20T10:37:53Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"In our compat library we have both \"msvc.c\" and \"mingw.c\". The former is\nmostly a thin wrapper around the latter as it directly includes it, but\nit has a couple of extra headers that aren't included in \"mingw.c\" and\nis expected to be used with the Visual Studio compiler toolchain.\n\nWhile our Makefile knows to pick up the correct file depending on\nwhether or not the Visual Studio toolchain is used, we don't do the same\nwith Meson. Fix this.\n\nSigned-off-by: Patrick Steinhardt <ps@pks.im>\n---\n meson.build | 4 +++-\n 1 file changed, 3 insertions(+), 1 deletion(-)\n\ndiff --git a/meson.build b/meson.build\nindex efe2871c9db..2cf9da3474b 100644\n--- a/meson.build\n+++ b/meson.build\n@@ -1107,7 +1107,6 @@ if host_machine.system() == 'cygwin'\n   ]\n elif host_machine.system() == 'windows'\n   libgit_sources += [\n-    'compat/mingw.c',\n     'compat/winansi.c',\n     'compat/win32/dirent.c',\n     'compat/win32/flush.c',\n@@ -1134,6 +1133,9 @@ elif host_machine.system() == 'windows'\n   libgit_include_directories += 'compat/win32'\n   if compiler.get_id() == 'msvc'\n     libgit_include_directories += 'compat/vcbuild/include'\n+    libgit_sources += 'compat/msvc.c'\n+  else\n+    libgit_sources += 'compat/mingw.c'\n   endif\n endif\n \n\n-- \n2.49.0.472.ge94155a9ec.dirty\n\n"},{"id":"514756","messageId":"20250320-b4-pks-mingw-lockfile-flake-v2-2-a84c90cfc6c2@pks.im","threadId":"63109","inReplyTo":"20250320-b4-pks-mingw-lockfile-flake-v2-0-a84c90cfc6c2@pks.im","subject":"[PATCH v2 2/2] compat/mingw: fix EACCESS when opening files with `O_CREAT | O_EXCL`","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-03-20T10:37:47Z","receivedAt":"2025-03-20T10:37:54Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"In our CI systems we can observe that t0610 fails rather frequently.\nThis testcase races a bunch of git-update-ref(1) processes with one\nanother which are all trying to update a unique reference, where we\nexpect that all processes succeed and end up updating the reftable\nstack. The error message in this case looks like the following:\n\n    fatal: update_ref failed for ref 'refs/heads/branch-88': reftable: transaction prepare: I/O error\n\nInstrumenting the code with a couple of calls to `BUG()` in relevant\nsites where we return `REFTABLE_IO_ERROR` quickly leads one to discover\nthat this error is caused when calling `flock_acquire()`, which is a\nthin wrapper around our lockfile API. Curiously, the error code we get\nin such cases is `EACCESS`, indicating that we are not allowed to access\nthe file.\n\nThe root cause of this is an oddity of `CreateFileW()`, which is what\n`_wopen()` uses internally. Quoting its documentation [1]:\n\n    If you call CreateFile on a file that is pending deletion as a\n    result of a previous call to DeleteFile, the function fails. The\n    operating system delays file deletion until all handles to the file\n    are closed. GetLastError returns ERROR_ACCESS_DENIED.\n\nThis behaviour is triggered quite often in the above testcase because\nall the processes race with one another trying to acquire the lock for\nthe \"tables.list\" file. This is due to how locking works in the reftable\nlibrary when compacting a stack:\n\n    1. Lock the \"tables.list\" file and reads its contents.\n\n    2. Decide which tables to compact.\n\n    3. Lock each of the individual tables that we are about to compact.\n\n    4. Unlock the \"tables.list\" file.\n\n    5. Compact the individual tables into one large table.\n\n    6. Re-lock the \"tables.list\" file.\n\n    7. Write the new list of tables into it.\n\n    8. Commit the \"tables.list\" file.\n\nThe important step is (4): we don't commit the file directly by renaming\nit into place, but instead we delete the lockfile so that concurrent\nprocesses can continue to append to the reftable stack while we compact\nthe tables. And because we use `DeleteFileW()` to do so, we may now race\nwith another process that wants to acquire that lockfile. So if we are\nunlucky, we would now see `ERROR_ACCESS_DENIED` instead of the expected\n`ERROR_FILE_EXISTS`, which the lockfile subsystem isn't prepared to\nhandle and thus it will bail out without retrying to acquire the lock.\n\nIn theory, the issue is not limited to the reftable library and can be\ntriggered by every other user of the lockfile subsystem, as well. My gut\nfeeling tells me it's rather unlikely to surface elsewhere though.\n\nFix the issue by translating the error to `EEXIST`. This makes the\nlockfile subsystem handle the error correctly: in case a timeout is set\nit will now retry acquiring the lockfile until the timeout has expired.\n\nWith this, t0610 is now always passing on my machine whereas it was\npreviously failing in around 20-30% of all test runs.\n\n[1]: https://learn.microsoft.com/en-us/windows/win32/api/fileapi/nf-fileapi-createfilew\n\nSigned-off-by: Patrick Steinhardt <ps@pks.im>\n---\n compat/mingw.c | 20 ++++++++++++++++++++\n 1 file changed, 20 insertions(+)\n\ndiff --git a/compat/mingw.c b/compat/mingw.c\nindex f524c54d06d..50c80b1b750 100644\n--- a/compat/mingw.c\n+++ b/compat/mingw.c\n@@ -21,6 +21,9 @@\n #include \"gettext.h\"\n #define SECURITY_WIN32\n #include <sspi.h>\n+#include <winternl.h>\n+\n+#define STATUS_DELETE_PENDING ((NTSTATUS) 0xC0000056)\n \n #define HCAST(type, handle) ((type)(intptr_t)handle)\n \n@@ -621,6 +624,8 @@ int mingw_open (const char *filename, int oflags, ...)\n \twchar_t wfilename[MAX_PATH];\n \topen_fn_t open_fn;\n \n+\tDECLARE_PROC_ADDR(ntdll.dll, NTSTATUS, NTAPI, RtlGetLastNtStatus, void);\n+\n \tva_start(args, oflags);\n \tmode = va_arg(args, int);\n \tva_end(args);\n@@ -644,6 +649,21 @@ int mingw_open (const char *filename, int oflags, ...)\n \n \tfd = open_fn(wfilename, oflags, mode);\n \n+\t/*\n+\t * Internally, `_wopen()` uses the `CreateFile()` API with CREATE_NEW,\n+\t * which may error out with ERROR_ACCESS_DENIED and an NtStatus of\n+\t * STATUS_DELETE_PENDING when the file is scheduled for deletion via\n+\t * `DeleteFileW()`. The file essentially exists, so we map errno to\n+\t * EEXIST instead of EACCESS so that callers don't have to special-case\n+\t * this.\n+\t *\n+\t * This fixes issues for example with the lockfile interface when one\n+\t * process has a lock that it is about to commit or release while\n+\t * another process wants to acquire it.\n+\t */\n+\tif (fd < 0 && create && GetLastError() == ERROR_ACCESS_DENIED &&\n+\t    INIT_PROC_ADDR(RtlGetLastNtStatus) && RtlGetLastNtStatus() == STATUS_DELETE_PENDING)\n+\t\terrno = EEXIST;\n \tif (fd < 0 && (oflags & O_ACCMODE) != O_RDONLY && errno == EACCES) {\n \t\tDWORD attrs = GetFileAttributesW(wfilename);\n \t\tif (attrs != INVALID_FILE_ATTRIBUTES && (attrs & FILE_ATTRIBUTE_DIRECTORY))\n\n-- \n2.49.0.472.ge94155a9ec.dirty\n\n"},{"id":"515094","messageId":"d149a082-cef7-3fe9-f9ce-6a4588cc3d1c@gmx.de","threadId":"63109","inReplyTo":"20250320-b4-pks-mingw-lockfile-flake-v2-2-a84c90cfc6c2@pks.im","subject":"Re: [PATCH v2 2/2] compat/mingw: fix EACCESS when opening files with `O_CREAT | O_EXCL`","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2025-03-26T12:20:12Z","receivedAt":"2025-03-26T12:20:20Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Patrick,\n\nOn Thu, 20 Mar 2025, Patrick Steinhardt wrote:\n\n> In our CI systems we can observe that t0610 fails rather frequently.\n> This testcase races a bunch of git-update-ref(1) processes with one\n> another which are all trying to update a unique reference, where we\n> expect that all processes succeed and end up updating the reftable\n> stack. The error message in this case looks like the following:\n>\n>     fatal: update_ref failed for ref 'refs/heads/branch-88': reftable: transaction prepare: I/O error\n>\n> Instrumenting the code with a couple of calls to `BUG()` in relevant\n> sites where we return `REFTABLE_IO_ERROR` quickly leads one to discover\n> that this error is caused when calling `flock_acquire()`, which is a\n> thin wrapper around our lockfile API. Curiously, the error code we get\n> in such cases is `EACCESS`, indicating that we are not allowed to access\n> the file.\n>\n> The root cause of this is an oddity of `CreateFileW()`, which is what\n> `_wopen()` uses internally. Quoting its documentation [1]:\n>\n>     If you call CreateFile on a file that is pending deletion as a\n>     result of a previous call to DeleteFile, the function fails. The\n>     operating system delays file deletion until all handles to the file\n>     are closed. GetLastError returns ERROR_ACCESS_DENIED.\n>\n> This behaviour is triggered quite often in the above testcase because\n> all the processes race with one another trying to acquire the lock for\n> the \"tables.list\" file. This is due to how locking works in the reftable\n> library when compacting a stack:\n>\n>     1. Lock the \"tables.list\" file and reads its contents.\n>\n>     2. Decide which tables to compact.\n>\n>     3. Lock each of the individual tables that we are about to compact.\n>\n>     4. Unlock the \"tables.list\" file.\n>\n>     5. Compact the individual tables into one large table.\n>\n>     6. Re-lock the \"tables.list\" file.\n>\n>     7. Write the new list of tables into it.\n>\n>     8. Commit the \"tables.list\" file.\n>\n> The important step is (4): we don't commit the file directly by renaming\n> it into place, but instead we delete the lockfile so that concurrent\n> processes can continue to append to the reftable stack while we compact\n> the tables. And because we use `DeleteFileW()` to do so, we may now race\n> with another process that wants to acquire that lockfile. So if we are\n> unlucky, we would now see `ERROR_ACCESS_DENIED` instead of the expected\n> `ERROR_FILE_EXISTS`, which the lockfile subsystem isn't prepared to\n> handle and thus it will bail out without retrying to acquire the lock.\n>\n> In theory, the issue is not limited to the reftable library and can be\n> triggered by every other user of the lockfile subsystem, as well. My gut\n> feeling tells me it's rather unlikely to surface elsewhere though.\n>\n> Fix the issue by translating the error to `EEXIST`. This makes the\n> lockfile subsystem handle the error correctly: in case a timeout is set\n> it will now retry acquiring the lockfile until the timeout has expired.\n>\n> With this, t0610 is now always passing on my machine whereas it was\n> previously failing in around 20-30% of all test runs.\n>\n> [1]: https://learn.microsoft.com/en-us/windows/win32/api/fileapi/nf-fileapi-createfilew\n\nCouldn't we simply handle `EACCES` the same way as `EEXIST` in step 4?\n\nThis suggestion is different from v1, which would have affected all\ncallers of `mingw_open()`.\n\nThe reason I ask is that `RtlGetLastNtStatus()` is undocumented, and\nshould therefore not be used. I know that I will be tasked with removing\nthat call should it be introduced into Git's source code, and naturally\nI'd like to avoid that.\n\nI know that e.g. PostgreSQL used this undocumented function at least at\nsome stage, but SQLite avoided it by introducing a simple poll strategy.\nWe could also do that, but if there is already code in the reftable\nlibrary that skips doing things if a `.lock` file exists, then doing the\nsame if the `.lock` file cannot be created, too, should be a safe argument\nto make.\n\nCiao,\nJohannes\n\n>\n> Signed-off-by: Patrick Steinhardt <ps@pks.im>\n> ---\n>  compat/mingw.c | 20 ++++++++++++++++++++\n>  1 file changed, 20 insertions(+)\n>\n> diff --git a/compat/mingw.c b/compat/mingw.c\n> index f524c54d06d..50c80b1b750 100644\n> --- a/compat/mingw.c\n> +++ b/compat/mingw.c\n> @@ -21,6 +21,9 @@\n>  #include \"gettext.h\"\n>  #define SECURITY_WIN32\n>  #include <sspi.h>\n> +#include <winternl.h>\n> +\n> +#define STATUS_DELETE_PENDING ((NTSTATUS) 0xC0000056)\n>\n>  #define HCAST(type, handle) ((type)(intptr_t)handle)\n>\n> @@ -621,6 +624,8 @@ int mingw_open (const char *filename, int oflags, ...)\n>  \twchar_t wfilename[MAX_PATH];\n>  \topen_fn_t open_fn;\n>\n> +\tDECLARE_PROC_ADDR(ntdll.dll, NTSTATUS, NTAPI, RtlGetLastNtStatus, void);\n> +\n>  \tva_start(args, oflags);\n>  \tmode = va_arg(args, int);\n>  \tva_end(args);\n> @@ -644,6 +649,21 @@ int mingw_open (const char *filename, int oflags, ...)\n>\n>  \tfd = open_fn(wfilename, oflags, mode);\n>\n> +\t/*\n> +\t * Internally, `_wopen()` uses the `CreateFile()` API with CREATE_NEW,\n> +\t * which may error out with ERROR_ACCESS_DENIED and an NtStatus of\n> +\t * STATUS_DELETE_PENDING when the file is scheduled for deletion via\n> +\t * `DeleteFileW()`. The file essentially exists, so we map errno to\n> +\t * EEXIST instead of EACCESS so that callers don't have to special-case\n> +\t * this.\n> +\t *\n> +\t * This fixes issues for example with the lockfile interface when one\n> +\t * process has a lock that it is about to commit or release while\n> +\t * another process wants to acquire it.\n> +\t */\n> +\tif (fd < 0 && create && GetLastError() == ERROR_ACCESS_DENIED &&\n> +\t    INIT_PROC_ADDR(RtlGetLastNtStatus) && RtlGetLastNtStatus() == STATUS_DELETE_PENDING)\n> +\t\terrno = EEXIST;\n>  \tif (fd < 0 && (oflags & O_ACCMODE) != O_RDONLY && errno == EACCES) {\n>  \t\tDWORD attrs = GetFileAttributesW(wfilename);\n>  \t\tif (attrs != INVALID_FILE_ATTRIBUTES && (attrs & FILE_ATTRIBUTE_DIRECTORY))\n>\n> --\n> 2.49.0.472.ge94155a9ec.dirty\n>\n>\n"},{"id":"515222","messageId":"Z-Zp9Dvi_Iz7CZ-J@pks.im","threadId":"63109","inReplyTo":"d149a082-cef7-3fe9-f9ce-6a4588cc3d1c@gmx.de","subject":"Re: [PATCH v2 2/2] compat/mingw: fix EACCESS when opening files with `O_CREAT | O_EXCL`","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-03-28T09:20:52Z","receivedAt":"2025-03-28T09:21:00Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Wed, Mar 26, 2025 at 01:20:12PM +0100, Johannes Schindelin wrote:\n> On Thu, 20 Mar 2025, Patrick Steinhardt wrote:\n> > In our CI systems we can observe that t0610 fails rather frequently.\n> > This testcase races a bunch of git-update-ref(1) processes with one\n> > another which are all trying to update a unique reference, where we\n> > expect that all processes succeed and end up updating the reftable\n> > stack. The error message in this case looks like the following:\n> >\n> >     fatal: update_ref failed for ref 'refs/heads/branch-88': reftable: transaction prepare: I/O error\n> >\n> > Instrumenting the code with a couple of calls to `BUG()` in relevant\n> > sites where we return `REFTABLE_IO_ERROR` quickly leads one to discover\n> > that this error is caused when calling `flock_acquire()`, which is a\n> > thin wrapper around our lockfile API. Curiously, the error code we get\n> > in such cases is `EACCESS`, indicating that we are not allowed to access\n> > the file.\n> >\n> > The root cause of this is an oddity of `CreateFileW()`, which is what\n> > `_wopen()` uses internally. Quoting its documentation [1]:\n> >\n> >     If you call CreateFile on a file that is pending deletion as a\n> >     result of a previous call to DeleteFile, the function fails. The\n> >     operating system delays file deletion until all handles to the file\n> >     are closed. GetLastError returns ERROR_ACCESS_DENIED.\n> >\n> > This behaviour is triggered quite often in the above testcase because\n> > all the processes race with one another trying to acquire the lock for\n> > the \"tables.list\" file. This is due to how locking works in the reftable\n> > library when compacting a stack:\n> >\n> >     1. Lock the \"tables.list\" file and reads its contents.\n> >\n> >     2. Decide which tables to compact.\n> >\n> >     3. Lock each of the individual tables that we are about to compact.\n> >\n> >     4. Unlock the \"tables.list\" file.\n> >\n> >     5. Compact the individual tables into one large table.\n> >\n> >     6. Re-lock the \"tables.list\" file.\n> >\n> >     7. Write the new list of tables into it.\n> >\n> >     8. Commit the \"tables.list\" file.\n> >\n> > The important step is (4): we don't commit the file directly by renaming\n> > it into place, but instead we delete the lockfile so that concurrent\n> > processes can continue to append to the reftable stack while we compact\n> > the tables. And because we use `DeleteFileW()` to do so, we may now race\n> > with another process that wants to acquire that lockfile. So if we are\n> > unlucky, we would now see `ERROR_ACCESS_DENIED` instead of the expected\n> > `ERROR_FILE_EXISTS`, which the lockfile subsystem isn't prepared to\n> > handle and thus it will bail out without retrying to acquire the lock.\n> >\n> > In theory, the issue is not limited to the reftable library and can be\n> > triggered by every other user of the lockfile subsystem, as well. My gut\n> > feeling tells me it's rather unlikely to surface elsewhere though.\n> >\n> > Fix the issue by translating the error to `EEXIST`. This makes the\n> > lockfile subsystem handle the error correctly: in case a timeout is set\n> > it will now retry acquiring the lockfile until the timeout has expired.\n> >\n> > With this, t0610 is now always passing on my machine whereas it was\n> > previously failing in around 20-30% of all test runs.\n> >\n> > [1]: https://learn.microsoft.com/en-us/windows/win32/api/fileapi/nf-fileapi-createfilew\n> \n> Couldn't we simply handle `EACCES` the same way as `EEXIST` in step 4?\n> \n> This suggestion is different from v1, which would have affected all\n> callers of `mingw_open()`.\n\nYeah, but it basically has the same problem: we cannot tell whether\nEACCESS is caused by the race or by insufficient privileges. So the\nbehaviour would be more self-contained, but it would still not be\ncorrect in the same way as it would be incorrect in `mingw_open()`. We\ndo want to retry locking the file in case we raced, but when EACCESS is\nraised due to insufficient permissions we don't.\n\n> The reason I ask is that `RtlGetLastNtStatus()` is undocumented, and\n> should therefore not be used. I know that I will be tasked with removing\n> that call should it be introduced into Git's source code, and naturally\n> I'd like to avoid that.\n\nUnfortunate, but fair enough. It's quite surprising that it is not\npossible to figure out the exact status code without relying on\nundocumented functions.\n\n> I know that e.g. PostgreSQL used this undocumented function at least at\n> some stage, but SQLite avoided it by introducing a simple poll strategy.\n> We could also do that, but if there is already code in the reftable\n> library that skips doing things if a `.lock` file exists, then doing the\n> same if the `.lock` file cannot be created, too, should be a safe argument\n> to make.\n\nI did stumble over the PostgreSQL patch at one point indeed, yeah.\n\nThanks for the pointer to SQLite. It indeed has the following snippet:\n\n    #define winIoerrCanRetry1(a) (((a)==ERROR_ACCESS_DENIED)        || \\\n                                  ((a)==ERROR_SHARING_VIOLATION)    || \\\n                                  ((a)==ERROR_LOCK_VIOLATION)       || \\\n                                  ((a)==ERROR_DEV_NOT_EXIST)        || \\\n                                  ((a)==ERROR_NETNAME_DELETED)      || \\\n                                  ((a)==ERROR_SEM_TIMEOUT)          || \\\n                                  ((a)==ERROR_NETWORK_UNREACHABLE))\n\nThe function gets used via `winRetryIoerr()`, which is used in various\nI/O functions to retry the operation, including `winOpen()` to open or\ncreate a file. And it indeed uses a rather simple polling system there\nwhere it sleeps for 25ms up to 10 times.\n\nThis certainly is something we could implement in `mingw_open()`: when\nwe see that `CreateFileW()` has returned any of the above errors we\nsimply retry the operation. It wouldn't fix the race itself, but it\nwould hopefully make it less likely to hit. If you would be okay with\nsuch a solution I can implement it.\n\nAlso, one thing to note: this problem isn't caused by the reftable\nlibrary, it's caused by the lockfile subsystem. So if we don't want to\ndo this in `mingw_open()`, any self-contained fix should go into the\nlockfile system, not into the reftable library, because we may hit the\nsame symptoms anywhere else where we race around creation/deletion of a\nlockfile. We just happen to hit this case in the reftable library\nbecause the test is intentionally stress-testing and racing this code\npath.\n\nPatrick\n"},{"id":"515238","messageId":"801fac5d-dd98-d157-6ff3-c3e8aa6a68ae@gmx.de","threadId":"63109","inReplyTo":"Z-Zp9Dvi_Iz7CZ-J@pks.im","subject":"Re: [PATCH v2 2/2] compat/mingw: fix EACCESS when opening files with `O_CREAT | O_EXCL`","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2025-03-28T15:41:05Z","receivedAt":"2025-03-28T15:41:12Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Patrick,\n\nOn Fri, 28 Mar 2025, Patrick Steinhardt wrote:\n\n> On Wed, Mar 26, 2025 at 01:20:12PM +0100, Johannes Schindelin wrote:\n> > On Thu, 20 Mar 2025, Patrick Steinhardt wrote:\n> > > In our CI systems we can observe that t0610 fails rather frequently.\n> > > This testcase races a bunch of git-update-ref(1) processes with one\n> > > another which are all trying to update a unique reference, where we\n> > > expect that all processes succeed and end up updating the reftable\n> > > stack. The error message in this case looks like the following:\n> > >\n> > >     fatal: update_ref failed for ref 'refs/heads/branch-88': reftable: transaction prepare: I/O error\n> > >\n> > > Instrumenting the code with a couple of calls to `BUG()` in relevant\n> > > sites where we return `REFTABLE_IO_ERROR` quickly leads one to discover\n> > > that this error is caused when calling `flock_acquire()`, which is a\n> > > thin wrapper around our lockfile API. Curiously, the error code we get\n> > > in such cases is `EACCESS`, indicating that we are not allowed to access\n> > > the file.\n> > >\n> > > The root cause of this is an oddity of `CreateFileW()`, which is what\n> > > `_wopen()` uses internally. Quoting its documentation [1]:\n> > >\n> > >     If you call CreateFile on a file that is pending deletion as a\n> > >     result of a previous call to DeleteFile, the function fails. The\n> > >     operating system delays file deletion until all handles to the file\n> > >     are closed. GetLastError returns ERROR_ACCESS_DENIED.\n> > >\n> > > This behaviour is triggered quite often in the above testcase because\n> > > all the processes race with one another trying to acquire the lock for\n> > > the \"tables.list\" file. This is due to how locking works in the reftable\n> > > library when compacting a stack:\n> > >\n> > >     1. Lock the \"tables.list\" file and reads its contents.\n> > >\n> > >     2. Decide which tables to compact.\n> > >\n> > >     3. Lock each of the individual tables that we are about to compact.\n> > >\n> > >     4. Unlock the \"tables.list\" file.\n> > >\n> > >     5. Compact the individual tables into one large table.\n> > >\n> > >     6. Re-lock the \"tables.list\" file.\n> > >\n> > >     7. Write the new list of tables into it.\n> > >\n> > >     8. Commit the \"tables.list\" file.\n> > >\n> > > The important step is (4): we don't commit the file directly by renaming\n> > > it into place, but instead we delete the lockfile so that concurrent\n> > > processes can continue to append to the reftable stack while we compact\n> > > the tables. And because we use `DeleteFileW()` to do so, we may now race\n> > > with another process that wants to acquire that lockfile. So if we are\n> > > unlucky, we would now see `ERROR_ACCESS_DENIED` instead of the expected\n> > > `ERROR_FILE_EXISTS`, which the lockfile subsystem isn't prepared to\n> > > handle and thus it will bail out without retrying to acquire the lock.\n> > >\n> > > In theory, the issue is not limited to the reftable library and can be\n> > > triggered by every other user of the lockfile subsystem, as well. My gut\n> > > feeling tells me it's rather unlikely to surface elsewhere though.\n> > >\n> > > Fix the issue by translating the error to `EEXIST`. This makes the\n> > > lockfile subsystem handle the error correctly: in case a timeout is set\n> > > it will now retry acquiring the lockfile until the timeout has expired.\n> > >\n> > > With this, t0610 is now always passing on my machine whereas it was\n> > > previously failing in around 20-30% of all test runs.\n> > >\n> > > [1]: https://learn.microsoft.com/en-us/windows/win32/api/fileapi/nf-fileapi-createfilew\n> >\n> > Couldn't we simply handle `EACCES` the same way as `EEXIST` in step 4?\n> >\n> > This suggestion is different from v1, which would have affected all\n> > callers of `mingw_open()`.\n>\n> Yeah, but it basically has the same problem: we cannot tell whether\n> EACCESS is caused by the race or by insufficient privileges. So the\n> behaviour would be more self-contained, but it would still not be\n> correct in the same way as it would be incorrect in `mingw_open()`. We\n> do want to retry locking the file in case we raced, but when EACCESS is\n> raised due to insufficient permissions we don't.\n\nMy thinking was that in the call chain from reftable, it seems that the\nfailure to lock the file is less bad than in other cases, that's why I\nasked whether that might be a layer where we could add a work-around\nwithout resorting to undocumented functions.\n\n> > The reason I ask is that `RtlGetLastNtStatus()` is undocumented, and\n> > should therefore not be used. I know that I will be tasked with removing\n> > that call should it be introduced into Git's source code, and naturally\n> > I'd like to avoid that.\n>\n> Unfortunate, but fair enough. It's quite surprising that it is not\n> possible to figure out the exact status code without relying on\n> undocumented functions.\n\nIndeed.\n\n> > I know that e.g. PostgreSQL used this undocumented function at least at\n> > some stage, but SQLite avoided it by introducing a simple poll strategy.\n> > We could also do that, but if there is already code in the reftable\n> > library that skips doing things if a `.lock` file exists, then doing the\n> > same if the `.lock` file cannot be created, too, should be a safe argument\n> > to make.\n>\n> I did stumble over the PostgreSQL patch at one point indeed, yeah.\n>\n> Thanks for the pointer to SQLite. It indeed has the following snippet:\n>\n>     #define winIoerrCanRetry1(a) (((a)==ERROR_ACCESS_DENIED)        || \\\n>                                   ((a)==ERROR_SHARING_VIOLATION)    || \\\n>                                   ((a)==ERROR_LOCK_VIOLATION)       || \\\n>                                   ((a)==ERROR_DEV_NOT_EXIST)        || \\\n>                                   ((a)==ERROR_NETNAME_DELETED)      || \\\n>                                   ((a)==ERROR_SEM_TIMEOUT)          || \\\n>                                   ((a)==ERROR_NETWORK_UNREACHABLE))\n>\n> The function gets used via `winRetryIoerr()`, which is used in various\n> I/O functions to retry the operation, including `winOpen()` to open or\n> create a file. And it indeed uses a rather simple polling system there\n> where it sleeps for 25ms up to 10 times.\n>\n> This certainly is something we could implement in `mingw_open()`: when\n> we see that `CreateFileW()` has returned any of the above errors we\n> simply retry the operation. It wouldn't fix the race itself, but it\n> would hopefully make it less likely to hit. If you would be okay with\n> such a solution I can implement it.\n>\n> Also, one thing to note: this problem isn't caused by the reftable\n> library, it's caused by the lockfile subsystem. So if we don't want to\n> do this in `mingw_open()`, any self-contained fix should go into the\n> lockfile system, not into the reftable library, because we may hit the\n> same symptoms anywhere else where we race around creation/deletion of a\n> lockfile. We just happen to hit this case in the reftable library\n> because the test is intentionally stress-testing and racing this code\n> path.\n\nAs I mentioned, I had hoped that we could address this at another layer.\n\nBut let's move forward with the `RtlGetLastNtStatus()` solution because,\nas you correctly pointed out, it is the only solution so far that lets Git\ndetermine precisely whether the underlying problem is a pending delete.\n\nI had only one remaining concern: If `RtlGetLastNtStatus()` has not yet\nbeen initialized, would we not potentially overwrite the last NTSTATUS\nwhile initializing it? And the answer I can give to myself is: unlikely.\nThe `ntdll` is already loaded, so there won't be an update to the\n`NTSTATUS` there, likewise the `GetProcAddress()` call won't fail and\nhence also not update it.\n\nSo let's go ahead with v2!\n\nCiao,\nJohannes\n"},{"id":"515322","messageId":"Z-o83wSIrYgLCBDQ@pks.im","threadId":"63109","inReplyTo":"801fac5d-dd98-d157-6ff3-c3e8aa6a68ae@gmx.de","subject":"Re: [PATCH v2 2/2] compat/mingw: fix EACCESS when opening files with `O_CREAT | O_EXCL`","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-03-31T06:57:35Z","receivedAt":"2025-03-31T06:57:38Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Fri, Mar 28, 2025 at 04:41:05PM +0100, Johannes Schindelin wrote:\n> On Fri, 28 Mar 2025, Patrick Steinhardt wrote:\n> > On Wed, Mar 26, 2025 at 01:20:12PM +0100, Johannes Schindelin wrote:\n> > > I know that e.g. PostgreSQL used this undocumented function at least at\n> > > some stage, but SQLite avoided it by introducing a simple poll strategy.\n> > > We could also do that, but if there is already code in the reftable\n> > > library that skips doing things if a `.lock` file exists, then doing the\n> > > same if the `.lock` file cannot be created, too, should be a safe argument\n> > > to make.\n> >\n> > I did stumble over the PostgreSQL patch at one point indeed, yeah.\n> >\n> > Thanks for the pointer to SQLite. It indeed has the following snippet:\n> >\n> >     #define winIoerrCanRetry1(a) (((a)==ERROR_ACCESS_DENIED)        || \\\n> >                                   ((a)==ERROR_SHARING_VIOLATION)    || \\\n> >                                   ((a)==ERROR_LOCK_VIOLATION)       || \\\n> >                                   ((a)==ERROR_DEV_NOT_EXIST)        || \\\n> >                                   ((a)==ERROR_NETNAME_DELETED)      || \\\n> >                                   ((a)==ERROR_SEM_TIMEOUT)          || \\\n> >                                   ((a)==ERROR_NETWORK_UNREACHABLE))\n> >\n> > The function gets used via `winRetryIoerr()`, which is used in various\n> > I/O functions to retry the operation, including `winOpen()` to open or\n> > create a file. And it indeed uses a rather simple polling system there\n> > where it sleeps for 25ms up to 10 times.\n> >\n> > This certainly is something we could implement in `mingw_open()`: when\n> > we see that `CreateFileW()` has returned any of the above errors we\n> > simply retry the operation. It wouldn't fix the race itself, but it\n> > would hopefully make it less likely to hit. If you would be okay with\n> > such a solution I can implement it.\n> >\n> > Also, one thing to note: this problem isn't caused by the reftable\n> > library, it's caused by the lockfile subsystem. So if we don't want to\n> > do this in `mingw_open()`, any self-contained fix should go into the\n> > lockfile system, not into the reftable library, because we may hit the\n> > same symptoms anywhere else where we race around creation/deletion of a\n> > lockfile. We just happen to hit this case in the reftable library\n> > because the test is intentionally stress-testing and racing this code\n> > path.\n> \n> As I mentioned, I had hoped that we could address this at another layer.\n> \n> But let's move forward with the `RtlGetLastNtStatus()` solution because,\n> as you correctly pointed out, it is the only solution so far that lets Git\n> determine precisely whether the underlying problem is a pending delete.\n> \n> I had only one remaining concern: If `RtlGetLastNtStatus()` has not yet\n> been initialized, would we not potentially overwrite the last NTSTATUS\n> while initializing it? And the answer I can give to myself is: unlikely.\n> The `ntdll` is already loaded, so there won't be an update to the\n> `NTSTATUS` there, likewise the `GetProcAddress()` call won't fail and\n> hence also not update it.\n> \n> So let's go ahead with v2!\n\nGreat, thanks a lot for your expertise and guidance!\n\nPatrick\n"}]}